refactor: use serialize-javascript package instead own implementation (#183)

evilebottnawi · michael-ciniawsky · commit 8d7ebcdd68d6 · 2017-12-08T12:47:49.000+01:00
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -32,6 +32,7 @@
   "dependencies": {
     "cacache": "^10.0.0",
     "find-cache-dir": "^1.0.0",
+    "serialize-javascript": "^1.4.0",
     "schema-utils": "^0.3.0",
     "source-map": "^0.6.1",
     "uglify-es": "^3.2.0",
diff --git a/src/index.js b/src/index.js
@@ -8,9 +8,9 @@ import { SourceMapSource, RawSource, ConcatSource } from 'webpack-sources';
 import RequestShortener from 'webpack/lib/RequestShortener';
 import ModuleFilenameHelpers from 'webpack/lib/ModuleFilenameHelpers';
 import validateOptions from 'schema-utils';
+import serialize from 'serialize-javascript';
 import schema from './options.json';
 import Uglify from './uglify';
-import { encode } from './uglify/serialization';
 import versions from './uglify/versions';
 
 /* eslint-disable
@@ -155,13 +155,13 @@ class UglifyJsPlugin {
               };
 
               if (this.options.cache) {
-                task.cacheKey = JSON.stringify({
+                task.cacheKey = serialize({
                   'uglify-es': versions.uglify,
                   'uglifyjs-webpack-plugin': versions.plugin,
                   'uglifyjs-webpack-plugin-options': this.options,
                   path: compiler.outputPath ? `${compiler.outputPath}/${file}` : file,
                   input,
-                }, encode);
+                });
               }
 
               tasks.push(task);
diff --git a/src/uglify/index.js b/src/uglify/index.js
@@ -2,8 +2,8 @@ import os from 'os';
 import cacache from 'cacache';
 import findCacheDir from 'find-cache-dir';
 import workerFarm from 'worker-farm';
+import serialize from 'serialize-javascript';
 import minify from './minify';
-import { encode } from './serialization';
 
 let workerFile = require.resolve('./worker');
 
@@ -28,7 +28,7 @@ export default class {
     if (this.maxConcurrentWorkers > 0) {
       const workerOptions = process.platform === 'win32' ? { maxConcurrentWorkers: this.maxConcurrentWorkers, maxConcurrentCallsPerWorker: 1 } : { maxConcurrentWorkers: this.maxConcurrentWorkers };
       this.workers = workerFarm(workerOptions, workerFile);
-      this.boundWorkers = (options, cb) => this.workers(JSON.stringify(options, encode), cb);
+      this.boundWorkers = (options, cb) => this.workers(serialize(options), cb);
     } else {
       this.boundWorkers = (options, cb) => {
         try {
diff --git a/src/uglify/serialization.js b/src/uglify/serialization.js
diff --git a/src/uglify/worker.js b/src/uglify/worker.js
@@ -1,9 +1,13 @@
 import minify from './minify';
-import { decode } from './serialization';
 
 module.exports = (options, callback) => {
   try {
-    callback(null, minify(JSON.parse(options, decode)));
+    // 'use strict' => this === undefined (Clean Scope)
+    // Safer for possible security issues, albeit not critical at all here
+    // eslint-disable-next-line no-new-func, no-param-reassign
+    options = new Function(`'use strict'\nreturn ${options}`)();
+
+    callback(null, minify(options));
   } catch (errors) {
     callback(errors);
   }
diff --git a/test/__snapshots__/cache-options.test.js.snap b/test/__snapshots__/cache-options.test.js.snap
@@ -12,34 +12,6 @@ exports[`cache \`string\`: asset main.0c220ec66316af2c1b24.js 1`] = `"webpackJso
 
 exports[`cache \`string\`: asset manifest.6afe1bc6685e9ab36c1c.js 1`] = `"!function(e){function n(r){if(t[r])return t[r].exports;var o=t[r]={i:r,l:!1,exports:{}};return e[r].call(o.exports,o,o.exports,n),o.l=!0,o.exports}var r=window.webpackJsonp;window.webpackJsonp=function(t,c,a){for(var i,u,f,l=0,s=[];l<t.length;l++)u=t[l],o[u]&&s.push(o[u][0]),o[u]=0;for(i in c)Object.prototype.hasOwnProperty.call(c,i)&&(e[i]=c[i]);for(r&&r(t,c,a);s.length;)s.shift()();if(a)for(l=0;l<a.length;l++)f=n(n.s=a[l]);return f};var t={},o={1:0};n.e=function(e){function r(){i.onerror=i.onload=null,clearTimeout(u);var n=o[e];0!==n&&(n&&n[1](new Error(\\"Loading chunk \\"+e+\\" failed.\\")),o[e]=void 0)}var t=o[e];if(0===t)return new Promise(function(e){e()});if(t)return t[2];var c=new Promise(function(n,r){t=o[e]=[n,r]});t[2]=c;var a=document.getElementsByTagName(\\"head\\")[0],i=document.createElement(\\"script\\");i.type=\\"text/javascript\\",i.charset=\\"utf-8\\",i.async=!0,i.timeout=12e4,n.nc&&i.setAttribute(\\"nonce\\",n.nc),i.src=n.p+\\"\\"+e+\\".\\"+({0:\\"main\\"}[e]||e)+\\".\\"+{0:\\"0c220ec66316af2c1b24\\"}[e]+\\".js\\";var u=setTimeout(r,12e4);return i.onerror=i.onload=r,a.appendChild(i),c},n.m=e,n.c=t,n.d=function(e,r,t){n.o(e,r)||Object.defineProperty(e,r,{configurable:!1,enumerable:!0,get:t})},n.n=function(e){var r=e&&e.__esModule?function(){return e.default}:function(){return e};return n.d(r,\\"a\\",r),r},n.o=function(e,n){return Object.prototype.hasOwnProperty.call(e,n)},n.p=\\"\\",n.oe=function(e){throw console.error(e),e}}([]);"`;
 
-exports[`cache \`string\`: cached entry test.js 1`] = `
-Array [
-  "test.js",
-  "function test(foo) { foo = 1; }",
-]
-`;
-
-exports[`cache \`string\`: cached entry test1.js 1`] = `
-Array [
-  "test1.js",
-  "function test1(foo) { foo = 1; }",
-]
-`;
-
-exports[`cache \`string\`: cached entry test2.js 1`] = `
-Array [
-  "test2.js",
-  "function test2(foo) { foo = 1; }",
-]
-`;
-
-exports[`cache \`string\`: cached entry test3.js 1`] = `
-Array [
-  "test3.js",
-  "function test3(foo) { foo = 1; }",
-]
-`;
-
 exports[`cache \`string\`: errors 1`] = `Array []`;
 
 exports[`cache \`string\`: warnings 1`] = `Array []`;
@@ -55,27 +27,55 @@ Array [
 ]
 `;
 
+exports[`cache \`true\`: cached entry test.js 2`] = `
+Array [
+  "test.js",
+  "function test(foo) { foo = 1; }",
+]
+`;
+
 exports[`cache \`true\`: cached entry test1.js 1`] = `
 Array [
   "test1.js",
   "function test1(foo) { foo = 1; }",
 ]
 `;
 
+exports[`cache \`true\`: cached entry test1.js 2`] = `
+Array [
+  "test1.js",
+  "function test1(foo) { foo = 1; }",
+]
+`;
+
 exports[`cache \`true\`: cached entry test2.js 1`] = `
 Array [
   "test2.js",
   "function test2(foo) { foo = 1; }",
 ]
 `;
 
+exports[`cache \`true\`: cached entry test2.js 2`] = `
+Array [
+  "test2.js",
+  "function test2(foo) { foo = 1; }",
+]
+`;
+
 exports[`cache \`true\`: cached entry test3.js 1`] = `
 Array [
   "test3.js",
   "function test3(foo) { foo = 1; }",
 ]
 `;
 
+exports[`cache \`true\`: cached entry test3.js 2`] = `
+Array [
+  "test3.js",
+  "function test3(foo) { foo = 1; }",
+]
+`;
+
 exports[`cache \`true\`: errors 1`] = `Array []`;
 
 exports[`cache \`true\`: warnings 1`] = `Array []`;
diff --git a/test/cache-options.test.js b/test/cache-options.test.js
@@ -112,9 +112,9 @@ describe('when options.cache', () => {
               cacache
                 .ls(cacheDir)
                 .then((cacheEntriesList) => {
-                  const cacheEntriesListKeys = Object.keys(cacheEntriesList);
+                  const cacheKeys = Object.keys(cacheEntriesList);
 
-                  expect(cacheEntriesListKeys.length).toBe(0);
+                  expect(cacheKeys.length).toBe(0);
                   done();
                 });
             });
@@ -231,15 +231,16 @@ describe('when options.cache', () => {
               cacache
                 .ls(cacheDir)
                 .then((cacheEntriesList) => {
-                  const cacheEntriesListKeys = Object.keys(cacheEntriesList);
+                  const cacheKeys = Object.keys(cacheEntriesList);
 
                   // Make sure that we cached files
-                  expect(cacheEntriesListKeys.length).toBe(files.length);
-                  cacheEntriesListKeys.forEach((cacheJSONEntry) => {
-                    const cacheEntry = JSON.parse(cacheJSONEntry);
+                  expect(cacheKeys.length).toBe(files.length);
+                  cacheKeys.forEach((cacheEntry) => {
+                    // eslint-disable-next-line no-new-func
+                    const cacheEntryOptions = new Function(`'use strict'\nreturn ${cacheEntry}`)();
 
-                    expect([cacheEntry.path, cacheEntry.input])
-                      .toMatchSnapshot(`cache \`true\`: cached entry ${cacheEntry.path}`);
+                    expect([cacheEntryOptions.path, cacheEntryOptions.input])
+                      .toMatchSnapshot(`cache \`true\`: cached entry ${cacheEntryOptions.path}`);
                   });
 
                   // Reset compilation assets and mocks
@@ -374,15 +375,16 @@ describe('when options.cache', () => {
               cacache
                 .ls(othercacheDir)
                 .then((cacheEntriesList) => {
-                  const cacheEntriesListKeys = Object.keys(cacheEntriesList);
+                  const cacheKeys = Object.keys(cacheEntriesList);
 
                   // Make sure that we cached files
-                  expect(cacheEntriesListKeys.length).toBe(files.length);
-                  cacheEntriesListKeys.forEach((cacheJSONEntry) => {
-                    const cacheEntry = JSON.parse(cacheJSONEntry);
+                  expect(cacheKeys.length).toBe(files.length);
+                  cacheKeys.forEach((cacheEntry) => {
+                    // eslint-disable-next-line no-new-func
+                    const cacheEntryOptions = new Function(`'use strict'\nreturn ${cacheEntry}`)();
 
-                    expect([cacheEntry.path, cacheEntry.input])
-                      .toMatchSnapshot(`cache \`string\`: cached entry ${cacheEntry.path}`);
+                    expect([cacheEntryOptions.path, cacheEntryOptions.input])
+                      .toMatchSnapshot(`cache \`true\`: cached entry ${cacheEntryOptions.path}`);
                   });
 
                   // Reset compilation assets and mocks
diff --git a/test/uglify/serialization.test.js b/test/uglify/serialization.test.js
diff --git a/test/uglify/worker.test.js b/test/uglify/worker.test.js
@@ -1,5 +1,5 @@
+import serialize from 'serialize-javascript';
 import worker from '../../src/uglify/worker';
-import { encode } from '../../src/uglify/serialization';
 
 describe('matches snapshot', () => {
   it('normalizes when options.extractComments is regex', () => {
@@ -8,7 +8,7 @@ describe('matches snapshot', () => {
       input: 'var foo = 1;/* hello */',
       extractComments: /foo/,
     };
-    worker(JSON.stringify(options, encode), (error, data) => {
+    worker(serialize(options), (error, data) => {
       if (error) {
         throw error;
       }
@@ -26,7 +26,7 @@ describe('matches snapshot', () => {
         },
       },
     };
-    worker(JSON.stringify(options, encode), (error, data) => {
+    worker(serialize(options), (error, data) => {
       if (error) {
         throw error;
       }
@@ -44,7 +44,7 @@ describe('matches snapshot', () => {
         },
       },
     };
-    worker(JSON.stringify(options, encode), (error, data) => {
+    worker(serialize(options), (error, data) => {
       if (error) {
         throw error;
       }
@@ -63,7 +63,7 @@ describe('matches snapshot', () => {
       },
       extractComments: 1,
     };
-    worker(JSON.stringify(options, encode), (error, data) => {
+    worker(serialize(options), (error, data) => {
       if (error) {
         throw error;
       }
@@ -90,7 +90,7 @@ describe('matches snapshot', () => {
         },
       },
     };
-    worker(JSON.stringify(options, encode), (error, data) => {
+    worker(serialize(options), (error, data) => {
       if (error) {
         throw error;
       }
@@ -109,7 +109,7 @@ describe('matches snapshot', () => {
         mappings: 'AAAA,QAASA,KAAIC,GACT,GAAIA,EAAG,CACH,MAAOC,MACPC',
       },
     };
-    worker(JSON.stringify(options, encode), (error, data) => {
+    worker(serialize(options), (error, data) => {
       if (error) {
         throw error;
       }
