diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fa607161..c51561e8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ Change Log +v5.4.5 +--- +* Fixed `controlFlowFlattening` intermittently dropping arguments of a spread call (e.g. `foo(...args)`) when it reused a control flow wrapper of a same-arity plain call. Fixes https://github.com/javascript-obfuscator/javascript-obfuscator/issues/1423 + v5.4.4 --- * Optimized scope identifiers transformer performance diff --git a/package.json b/package.json index 1caf7a7f4..2f3e5035d 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "javascript-obfuscator", - "version": "5.4.4", + "version": "5.4.5", "description": "JavaScript obfuscator", "keywords": [ "obfuscator", diff --git a/src/node-transformers/control-flow-transformers/control-flow-replacers/CallExpressionControlFlowReplacer.ts b/src/node-transformers/control-flow-transformers/control-flow-replacers/CallExpressionControlFlowReplacer.ts index c02b59c17..e0895f51b 100644 --- a/src/node-transformers/control-flow-transformers/control-flow-replacers/CallExpressionControlFlowReplacer.ts +++ b/src/node-transformers/control-flow-transformers/control-flow-replacers/CallExpressionControlFlowReplacer.ts @@ -64,14 +64,27 @@ export class CallExpressionControlFlowReplacer extends AbstractControlFlowReplac const isChainExpressionParent = NodeGuards.isChainExpressionNode(parentNode); - // Bucket reuse-eligible wrappers by both arg count AND optional-ness, so an - // optional `foo?.(arg)` call never reuses the wrapper of a non-optional `bar(arg)` - // that happens to share `arguments.length` — which would drop the `?.` short-circuit - // and crash on undefined callees (issue #1408). - const replacerId: string = `${callExpressionNode.arguments.length}-${isChainExpressionParent ? 'optional' : 'standard'}`; + const expressionArguments: (ESTree.Expression | ESTree.SpreadElement)[] = callExpressionNode.arguments; + + // Bucket reuse-eligible wrappers by the exact argument *shape* (which positions are + // spread vs. plain) AND optional-ness — not just `arguments.length`. + // + // A spread call like `foo(...args)` has the same `arguments.length` as a plain + // `bar(arg)`, but the generated wrapper differs: `(callee, ...p1) => callee(...p1)` + // vs `(callee, p1) => callee(p1)`. Reusing a plain wrapper for a spread call collapses + // the spread to its first element and silently drops the remaining arguments (issue #1423). + // + // Encoding the per-argument shape also preserves the optional-ness bucketing that keeps + // an optional `foo?.(arg)` call from reusing a non-optional `bar(arg)` wrapper, which + // would drop the `?.` short-circuit and crash on undefined callees (issue #1408). + const argumentsShape: string = expressionArguments + .map((argument: ESTree.Expression | ESTree.SpreadElement): string => + NodeGuards.isSpreadElementNode(argument) ? 's' : 'p' + ) + .join(''); + const replacerId: string = `${argumentsShape}-${isChainExpressionParent ? 'optional' : 'standard'}`; const callExpressionFunctionCustomNode: ICustomNode> = this.controlFlowCustomNodeFactory(ControlFlowCustomNode.CallExpressionFunctionNode); - const expressionArguments: (ESTree.Expression | ESTree.SpreadElement)[] = callExpressionNode.arguments; callExpressionFunctionCustomNode.initialize(expressionArguments, isChainExpressionParent); diff --git a/test/functional-tests/node-transformers/control-flow-transformers/control-flow-replacers/call-expression-control-flow-replacer/CallExpressionControlFlowReplacer.spec.ts b/test/functional-tests/node-transformers/control-flow-transformers/control-flow-replacers/call-expression-control-flow-replacer/CallExpressionControlFlowReplacer.spec.ts index 0259d32d0..74b208362 100644 --- a/test/functional-tests/node-transformers/control-flow-transformers/control-flow-replacers/call-expression-control-flow-replacer/CallExpressionControlFlowReplacer.spec.ts +++ b/test/functional-tests/node-transformers/control-flow-transformers/control-flow-replacers/call-expression-control-flow-replacer/CallExpressionControlFlowReplacer.spec.ts @@ -276,5 +276,32 @@ describe('CallExpressionControlFlowReplacer', function () { } }); }); + + describe('Variant #9 - spread call must not reuse a plain wrapper of the same arity (issue #1423)', () => { + const samplesCount: number = 200; + + it('should forward every spread-expanded argument on every obfuscation when a spread call and a plain call share the same arity', () => { + const code: string = readFileAsString( + __dirname + '/fixtures/issue-1423-spread-and-plain-calls.js' + ); + + for (let i = 0; i < samplesCount; i++) { + const obfuscatedCode: string = JavaScriptObfuscator.obfuscate(code, { + ...NO_ADDITIONAL_NODES_PRESET, + controlFlowFlattening: true, + controlFlowFlatteningThreshold: 1, + identifierNamesGenerator: 'mangled' + }).getObfuscatedCode(); + + const result: unknown = eval(obfuscatedCode); + + assert.strictEqual( + result, + 'ok', + `iteration ${i}: spread call reused a plain wrapper and dropped arguments (obfuscated code returned wrong value)` + ); + } + }); + }); }); }); diff --git a/test/functional-tests/node-transformers/control-flow-transformers/control-flow-replacers/call-expression-control-flow-replacer/fixtures/issue-1423-spread-and-plain-calls.js b/test/functional-tests/node-transformers/control-flow-transformers/control-flow-replacers/call-expression-control-flow-replacer/fixtures/issue-1423-spread-and-plain-calls.js new file mode 100644 index 000000000..ec2b11adf --- /dev/null +++ b/test/functional-tests/node-transformers/control-flow-transformers/control-flow-replacers/call-expression-control-flow-replacer/fixtures/issue-1423-spread-and-plain-calls.js @@ -0,0 +1,26 @@ +(function () { + function target (a, b, c) { + return '' + a + b + c; + } + + function id (x) { + return x; + } + + function forward () { + var rest = [1, 2, 3]; + + // A spread-forward call `target(...rest)` has `arguments.length === 1`, + // the same as the plain `id(x)` calls below. It must not reuse a plain + // single-argument control flow wrapper, otherwise every spread-expanded + // argument after the first is silently dropped (issue #1423). + id(1); + id(2); + id(3); + id(4); + + return target(...rest); + } + + return forward() === '123' ? 'ok' : 'broken'; +})();