Merge pull request #103 from dwnusbaum/JENKINS-70080

[JENKINS-70080] Do not generate invalid bytecode for field assignments that use compound operators

src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java

@@ -683,8 +683,21 @@ private Expression innerTransform(Expression exp) {
                                     // "this.x = y" must be handled specially to prevent the sandbox from using
                                     // reflection to assign values to final fields in constructors and initializers
                                     // and to prevent infinite loops in setter methods.
-                                    return new BinaryExpression(new FieldExpression(field), be.getOperation(),
+                                    Token op = be.getOperation();
+                                    if (op.getType() == Types.ASSIGN) {
+                                        return new BinaryExpression(new FieldExpression(field), op,
                                             makeCheckedGroovyCast(field.getType(), transform(be.getRightExpression())));
+                                    } else {
+                                        // Groovy does not support FieldExpression with compound assignment operators
+                                        // directly, so we must expand the expression ourselves.
+                                        Token plainAssignment = Token.newSymbol(Types.ASSIGN, op.getStartLine(), op.getStartColumn());
+                                        return new BinaryExpression(new FieldExpression(field), plainAssignment,
+                                            makeCheckedGroovyCast(field.getType(),
+                                                    makeCheckedCall("checkedBinaryOp",
+                                                            new FieldExpression(field),
+                                                            intExp(Ops.compoundAssignmentToBinaryOperator(op.getType())),
+                                                            transform(be.getRightExpression()))));
+                                    }
                                 } // else this is a property which we need to check
                             }
                             if (interceptProperty)

src/main/java/org/kohsuke/groovy/sandbox/ScopeTrackingClassCodeExpressionTransformer.java

@@ -111,6 +111,8 @@ def h() {
                 // When using Java-style for loops, the 3 expressions are a ClosureListExpression and ForStatement.getVariable is a dummy value that we need to ignore.
                 declareVariable(forLoop.getVariable());
             }
+            // Avoid super.visitForLoop because it transforms the collection expression but then recurses on the entire
+            // ForStatement, causing the collection expression to be visited a second time.
             forLoop.setCollectionExpression(transform(forLoop.getCollectionExpression()));
             forLoop.getLoopBlock().visit(this);
         }
@@ -138,6 +140,8 @@ public void visitSwitch(SwitchStatement statement) {
 
     @Override
     public void visitSynchronizedStatement(SynchronizedStatement sync) {
+        // Avoid super.visitSynchronizedStatement because it transforms the expression but then recurses on the entire
+        // SynchronizedStatement, causing the expression to be visited a second time.
         sync.setExpression(transform(sync.getExpression()));
         try (StackVariableSet scope = new StackVariableSet(this)) {
             sync.getCode().visit(this);
@@ -161,6 +165,8 @@ public void visitCatchStatement(CatchStatement statement) {
 
     @Override
     public void visitWhileLoop(WhileStatement loop) {
+        // Avoid super.visitWhileLoop because it transforms the boolean expression but then recurses on the entire
+        // WhileStatement, causing the boolean expression to be visited a second time.
         loop.setBooleanExpression((BooleanExpression) transform(loop.getBooleanExpression()));
         try (StackVariableSet scope = new StackVariableSet(this)) {
             loop.getLoopBlock().visit(this);

src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java

@@ -1163,4 +1163,93 @@ public void sandboxSupportsReturnStatementsInClosures() throws Exception {
                 "}\n");
     }
 
+    @Test
+    public void sandboxSupportsFinalFields() {
+        assertIntercept(
+                "class Test {\n" +
+                "  final String p\n" +
+                "  Test() {\n" +
+                "    p = 'value'\n" +
+                "  }\n" +
+                "}\n" +
+                "new Test().p",
+                "value",
+                // Intercepted operations:
+                "new Test()",
+                "Test.p");
+    }
+
+    @Test
+    public void sandboxDoesNotRecurseInfinitelyInSetters() {
+        assertIntercept(
+                "class Test {\n" +
+                "  String prop\n" +
+                "  def setProp(newProp) {\n" +
+                "    prop = newProp\n" +
+                "    prop\n" +
+                "  }\n" +
+                "}\n" +
+                "new Test().prop = 'value'",
+                "value",
+                // Intercepted operations:
+                "new Test()",
+                "Test.prop=String",
+                "Test.prop");
+    }
+
+    @Test
+    public void sandboxDoesNotPerformImplicitCastsForOverloadedOperators() {
+        // groovy.lang.MissingMethodException: No signature of method: OverridePlus.plus() is applicable for argument types: (java.util.ArrayList) values: [[secret.key]]
+        assertFailsWithSameException(
+                "class OverridePlus {\n" +
+                "  def file\n" +
+                "  OverridePlus plus(File file) {\n" +
+                "    this.file = file\n" +
+                "    this\n" +
+                "  }\n" +
+                "}\n" +
+                "new OverridePlus() + ['secret.key']\n");
+    }
+
+    @Issue("JENKINS-70080")
+    @Test
+    public void sandboxSupportsCompoundAssignmentsToFields() throws Throwable {
+        assertIntercept(
+                "class Test {\n" +
+                "  def map = [:]\n" +
+                "  def add(newMap) {\n" +
+                "    map += newMap\n" +
+                "    map\n" +
+                "  }\n" +
+                "}\n" +
+                "new Test().add([k: 'v'])\n",
+                Collections.singletonMap("k", "v"),
+                // Intercepted operations:
+                "new Test()",
+                "Test.add(LinkedHashMap)",
+                "LinkedHashMap.plus(LinkedHashMap)",
+                "Test.map");
+        assertIntercept(
+                "class Test {\n" +
+                "  final Map map = [:]\n" +
+                "  Test(newMap) {\n" +
+                "    map += newMap\n" + // Groovy is more lenient with 'final' than Java
+                "  }\n" +
+                "}\n" +
+                "new Test([k: 'v']).map\n",
+                Collections.singletonMap("k", "v"),
+                // Intercepted operations:
+                "new Test(LinkedHashMap)",
+                "LinkedHashMap.plus(LinkedHashMap)",
+                "Test.map");
+        assertFailsWithSameException(
+                "class Test {\n" +
+                "  final Map map\n" +
+                "  Test(newMap) {\n" +
+                "    map += newMap\n" + // java.lang.NullPointerException: Cannot execute null+{}
+                "  }\n" +
+                "}\n" +
+                "new Test([:]).map\n");
+    }
+
 }