diff --git a/CHANGELOG.md b/CHANGELOG.md index 29d164864..a2442c85d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ v5.4.0 --- * Add support for `import attributes`. Fixes https://github.com/javascript-obfuscator/javascript-obfuscator/issues/1256 * Fixed `reservedNames` not preserving class method and property names when `stringArray` or `deadCodeInjection` is enabled. Fixes https://github.com/javascript-obfuscator/javascript-obfuscator/issues/1279 +* Fixed `transformObjectKeys` changing evaluation order when object expression is inside a sequence expression with preceding side effects (e.g. `return aux(ys), { min }`). Fixes https://github.com/javascript-obfuscator/javascript-obfuscator/issues/1246 * Replaced `mkdirp` dependency with native `fs.mkdirSync({ recursive: true })`. Fixes https://github.com/javascript-obfuscator/javascript-obfuscator/issues/1275. Thank you https://github.com/roli-lpci! * Replaced `conf` dependency with custom implementation using `env-paths` and native `fs` diff --git a/src/node-transformers/converting-transformers/ObjectExpressionKeysTransformer.ts b/src/node-transformers/converting-transformers/ObjectExpressionKeysTransformer.ts index 05bc0af6c..9918d1d23 100644 --- a/src/node-transformers/converting-transformers/ObjectExpressionKeysTransformer.ts +++ b/src/node-transformers/converting-transformers/ObjectExpressionKeysTransformer.ts @@ -124,10 +124,7 @@ export class ObjectExpressionKeysTransformer extends AbstractNodeTransformer { objectExpressionNode, objectExpressionParentNode ) || - ObjectExpressionKeysTransformer.isProhibitedSequenceExpression( - objectExpressionNode, - objectExpressionHostStatement - ) || + ObjectExpressionKeysTransformer.isProhibitedSequenceExpression(objectExpressionNode) || ObjectExpressionKeysTransformer.isProhibitedLoopBody(objectExpressionNode) ) { return true; @@ -191,21 +188,73 @@ export class ObjectExpressionKeysTransformer extends AbstractNodeTransformer { /** * @param {ObjectExpression} objectExpressionNode - * @param {Node} objectExpressionHostNode * @returns {boolean} */ - private static isProhibitedSequenceExpression( - objectExpressionNode: ESTree.ObjectExpression, - objectExpressionHostNode: ESTree.Node - ): boolean { - return ( - NodeGuards.isExpressionStatementNode(objectExpressionHostNode) && - NodeGuards.isSequenceExpressionNode(objectExpressionHostNode.expression) && - objectExpressionHostNode.expression.expressions.some( - (expressionNode: ESTree.Expression) => - NodeGuards.isCallExpressionNode(expressionNode) && NodeGuards.isSuperNode(expressionNode.callee) - ) - ); + private static isProhibitedSequenceExpression(objectExpressionNode: ESTree.ObjectExpression): boolean { + const parentNode: ESTree.Node | undefined = objectExpressionNode.parentNode; + + if (!parentNode) { + return false; + } + + // Case 1: object is a direct child of a sequence expression + // e.g. `return aux(ys), { min }` + if (NodeGuards.isSequenceExpressionNode(parentNode)) { + const index: number = parentNode.expressions.indexOf(objectExpressionNode); + + return index > 0; + } + + // Case 2: object is nested inside a sequence expression via assignment/etc + // e.g. `super(), this.state = { foo: 1 }` + // Walk up to find if we're inside a sequence expression at a non-first position + let currentNode: ESTree.Node = parentNode; + + while (currentNode.parentNode) { + const currentParent: ESTree.Node = currentNode.parentNode; + + if (NodeGuards.isSequenceExpressionNode(currentParent)) { + const index: number = currentParent.expressions.indexOf(currentNode); + + if (index > 0) { + // Only prohibit if earlier expressions contain calls (side effects) + return currentParent.expressions.slice(0, index).some( + (expr: ESTree.Expression) => { + let hasCall: boolean = false; + + estraverse.traverse(expr, { + enter: (node: ESTree.Node) => { + if ( + NodeGuards.isCallExpressionNode(node) || + NodeGuards.isNewExpressionNode(node) + ) { + hasCall = true; + + return estraverse.VisitorOption.Break; + } + } + }); + + return hasCall; + } + ); + } + + return false; + } + + if ( + NodeGuards.isFunctionNode(currentParent) || + NodeGuards.isProgramNode(currentParent) || + NodeGuards.isBlockStatementNode(currentParent) + ) { + break; + } + + currentNode = currentParent; + } + + return false; } /** diff --git a/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/ObjectExpressionKeysTransformer.spec.ts b/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/ObjectExpressionKeysTransformer.spec.ts index 41b4e9eb9..da3408845 100644 --- a/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/ObjectExpressionKeysTransformer.spec.ts +++ b/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/ObjectExpressionKeysTransformer.spec.ts @@ -2100,7 +2100,67 @@ describe('ObjectExpressionKeysTransformer', () => { }); }); - describe('Variant #16: conditional expression `this` reference', () => { + describe('Variant #16: return statement sequence expression with side effects', () => { + let testFunc: () => void; + + before(() => { + const code: string = readFileAsString( + __dirname + '/fixtures/return-statement-sequence-expression-side-effect.js' + ); + + testFunc = () => { + const obfuscatedCode: string = JavaScriptObfuscator.obfuscate(code, { + ...NO_ADDITIONAL_NODES_PRESET, + transformObjectKeys: true + }).getObfuscatedCode(); + + const result = eval(`${obfuscatedCode} compErr(5);`); + + if (result.min !== 5) { + throw new Error( + `Expected min to be 5, got ${result.min}. ` + + `Object extraction changed evaluation order in sequence expression.` + ); + } + }; + }); + + it('should not change evaluation order when extracting object keys from sequence expression', () => { + assert.doesNotThrow(testFunc); + }); + }); + + describe('Variant #17: return statement sequence expression with nested assignment and side effects', () => { + let testFunc: () => void; + + before(() => { + const code: string = readFileAsString( + __dirname + '/fixtures/return-statement-sequence-expression-nested-assignment-side-effect.js' + ); + + testFunc = () => { + const obfuscatedCode: string = JavaScriptObfuscator.obfuscate(code, { + ...NO_ADDITIONAL_NODES_PRESET, + transformObjectKeys: true + }).getObfuscatedCode(); + + const result = eval(`${obfuscatedCode} test();`); + + if (result.foo !== 42) { + throw new Error( + `Expected foo to be 42, got ${result.foo}. ` + + `Object extraction changed evaluation order in nested sequence expression.` + ); + } + }; + }); + + it('should not change evaluation order when extracting object keys from nested assignment in sequence expression', () => { + assert.doesNotThrow(testFunc); + }); + }); + + describe('Variant #18: conditional expression `this` reference', () => { describe('Variant #1: conditional expression identifier reference', () => { const match: string = `` + diff --git a/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/fixtures/return-statement-sequence-expression-nested-assignment-side-effect.js b/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/fixtures/return-statement-sequence-expression-nested-assignment-side-effect.js new file mode 100644 index 000000000..d0e1892c5 --- /dev/null +++ b/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/fixtures/return-statement-sequence-expression-nested-assignment-side-effect.js @@ -0,0 +1,9 @@ +function test() { + let x, y, val = 0; + + function sideEffect() { + val = 42; + } + + return (sideEffect(), x = (y = { foo: val })); +} diff --git a/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/fixtures/return-statement-sequence-expression-side-effect.js b/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/fixtures/return-statement-sequence-expression-side-effect.js new file mode 100644 index 000000000..279259a6e --- /dev/null +++ b/test/functional-tests/node-transformers/converting-transformers/object-expression-keys-transformer/fixtures/return-statement-sequence-expression-side-effect.js @@ -0,0 +1,9 @@ +function compErr(ys) { + let min = Infinity; + + function aux(y) { + if (y < min) min = y; + } + + return (aux(ys), { min }); +}