From 8d7ebcdd68d6f12a0e8d8d8f1f71b57a9f87f4cf Mon Sep 17 00:00:00 2001 From: Evilebot Tnawi Date: Fri, 8 Dec 2017 14:47:49 +0300 Subject: [PATCH] refactor: use `serialize-javascript` package instead own implementation (#183) --- package-lock.json | 5 ++ package.json | 1 + src/index.js | 6 +- src/uglify/index.js | 4 +- src/uglify/serialization.js | 34 ----------- src/uglify/worker.js | 8 ++- test/__snapshots__/cache-options.test.js.snap | 56 +++++++++---------- test/cache-options.test.js | 30 +++++----- test/uglify/serialization.test.js | 35 ------------ test/uglify/worker.test.js | 14 ++--- 10 files changed, 68 insertions(+), 125 deletions(-) delete mode 100644 src/uglify/serialization.js delete mode 100644 test/uglify/serialization.test.js diff --git a/package-lock.json b/package-lock.json index e071cfdf..9b410b2a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7030,6 +7030,11 @@ "semver": "5.4.1" } }, + "serialize-javascript": { + "version": "1.4.0", + "resolved": "https://registry.npmjs.org/serialize-javascript/-/serialize-javascript-1.4.0.tgz", + "integrity": "sha1-fJWFFNtqwkQ6irwGLcn3iGp/YAU=" + }, "set-blocking": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/set-blocking/-/set-blocking-2.0.0.tgz", diff --git a/package.json b/package.json index b35636d9..9ea3f3c0 100644 --- 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 index 4834978c..b7f89e2d 100644 --- 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 index dc8e095e..c04ae64e 100644 --- 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 deleted file mode 100644 index 7ac19574..00000000 --- a/src/uglify/serialization.js +++ /dev/null @@ -1,34 +0,0 @@ -/* eslint-disable no-new-func */ -const toType = value => (Object.prototype.toString.call(value).slice(8, -1)); - -export const encode = (key, value) => { - const type = toType(value); - if (encode[type]) { - return `<${type}>${encode[type](value, key)}`; - } - return value; -}; - -encode.RegExp = value => String(value); -encode.Function = value => String(value); - -export const decode = (key, value) => { - if (typeof value === 'string') { - const regex = /^<([A-Z]\w+)>([\w\W]*)$/; - const match = value.match(regex); - if (match && decode[match[1]]) { - return decode[match[1]](match[2], key); - } - } - - return value; -}; - -decode.RegExp = value => (Function(`return ${value}`)()); -decode.Function = (value, key) => Function(` - try { - return ${value}.apply(null, arguments); - } catch(err) { - throw new Error('the option "${key}" performs an error in the child process: ' + err.message); - } -`); diff --git a/src/uglify/worker.js b/src/uglify/worker.js index 924d544a..73a251b4 100644 --- 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 index dc8bccef..a0b37bbb 100644 --- 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 { 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 deleted file mode 100644 index 032a11de..00000000 --- a/test/uglify/serialization.test.js +++ /dev/null @@ -1,35 +0,0 @@ -import { encode, decode } from '../../src/uglify/serialization'; - -describe('serialization data', () => { - it('serialization function', () => { - const input = { - func: (a, b) => a + b, - }; - const string = JSON.stringify(input, encode); - const json = JSON.parse(string, decode); - expect(json.func).not.toBe(input.func); - expect(typeof json.func).toBe('function'); - expect(json.func(1, 2)).toBe(input.func(1, 2)); - }); - - it('serialization regexp', () => { - const input = { - regexp: /\s+/, - }; - const string = JSON.stringify(input, encode); - const json = JSON.parse(string, decode); - expect(json.regexp).not.toBe(input.regexp); - expect(json.regexp instanceof RegExp).toBe(true); - expect(json.regexp.toString()).toBe(input.regexp.toString()); - }); - - it('function operation failure should report critical information.', () => { - const a = 0; - const b = 1; - const input = { - func: () => a + b, - }; - const string = JSON.stringify(input, encode); - expect(() => (JSON.parse(string, decode)).func()).toThrow('the option "func" performs an error in the child process:'); - }); -}); diff --git a/test/uglify/worker.test.js b/test/uglify/worker.test.js index 7cb105d1..4aa341e2 100644 --- 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; }