Fix transformObjectKeys to preserve evaluation order in sequence expressions with side effects

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`
 

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(<ESTree.Expression>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;
     }
 
     /**

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 =
                     `` +

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 }));
+}

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 });
+}