diff --git a/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java b/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java index f3ebf73..29d3b7f 100644 --- a/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java +++ b/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) diff --git a/src/main/java/org/kohsuke/groovy/sandbox/ScopeTrackingClassCodeExpressionTransformer.java b/src/main/java/org/kohsuke/groovy/sandbox/ScopeTrackingClassCodeExpressionTransformer.java index f85859c..09dcd47 100644 --- a/src/main/java/org/kohsuke/groovy/sandbox/ScopeTrackingClassCodeExpressionTransformer.java +++ b/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); diff --git a/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java b/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java index f84a7cf..ff834d7 100644 --- a/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java +++ b/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"); + } + }