From b88f632f7b0b8164e8808535db5b8518ca18b3c8 Mon Sep 17 00:00:00 2001 From: Carroll Chiou Date: Tue, 16 Apr 2024 16:40:01 +0200 Subject: [PATCH] SECURITY-3341 Co-authored-by: Devin Nusbaum --- .../groovy/sandbox/GroovyValueFilter.java | 18 +- .../groovy/sandbox/SandboxTransformer.java | 202 ++++++++++-------- .../sandbox/SandboxTransformerTest.java | 83 ++++++- .../org/kohsuke/groovy/sandbox/TheTest.java | 146 +++++-------- 4 files changed, 248 insertions(+), 201 deletions(-) diff --git a/src/main/java/org/kohsuke/groovy/sandbox/GroovyValueFilter.java b/src/main/java/org/kohsuke/groovy/sandbox/GroovyValueFilter.java index ba4e345..c75c9f8 100644 --- a/src/main/java/org/kohsuke/groovy/sandbox/GroovyValueFilter.java +++ b/src/main/java/org/kohsuke/groovy/sandbox/GroovyValueFilter.java @@ -1,16 +1,12 @@ package org.kohsuke.groovy.sandbox; +import groovy.lang.Binding; +import groovy.lang.Script; + /** - * {@link GroovyInterceptor} that looks at individual values that are coming into/out of a call, - * without regard to any context. - * - *

- * One of the common strategies for filtering is to ensure that the sandboxed script don't acquire a reference - * to objects that they are not supposed to. This implementation works as a convenient base class for such interceptor - * by providing a smaller number of methods that can be overridden. - * - * @author Kohsuke Kawaguchi + * @deprecated */ +@Deprecated public class GroovyValueFilter extends GroovyInterceptor { /** * Called for every receiver. @@ -65,6 +61,10 @@ public Object onStaticCall(Invoker invoker, Class receiver, String method, Objec @Override public Object onNewInstance(Invoker invoker, Class receiver, Object... args) throws Throwable { + if (receiver == Script.class && args.length == 1 && args[0] instanceof Binding) { + // Ignore initial script instantiation. + return super.onNewInstance(invoker, receiver, args); + } return filterReturnValue(super.onNewInstance(invoker, (Class)filterReceiver(receiver), filterArguments(args))); } diff --git a/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java b/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java index 29d3b7f..a89f345 100644 --- a/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java +++ b/src/main/java/org/kohsuke/groovy/sandbox/SandboxTransformer.java @@ -1,11 +1,9 @@ package org.kohsuke.groovy.sandbox; -import groovy.lang.Script; import java.lang.reflect.Modifier; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; @@ -194,12 +192,14 @@ to account for it by ensuring that at least one parameter does not have a defaul } } - /** Do not care about {@code super} or {@code this} calls for classes extending these types. */ - private static final Set TRIVIAL_CONSTRUCTORS = new HashSet<>(Arrays.asList( - Object.class.getName(), - Script.class.getName(), - "com.cloudbees.groovy.cps.SerializableScript", - "org.jenkinsci.plugins.workflow.cps.CpsScript")); + /** + * Do not care about {@code super} calls for classes extending these types. + * + *

Entries in this list must not have any constructors with parameters whose types are not safe to construct in + * the sandbox, and they must be in a package that cannot be used to define new classes in the sandbox. + */ + private static final Set TRIVIAL_CONSTRUCTORS = Collections.singleton(Object.class.getName()); + /** * Apply SECURITY-582 (and part of SECURITY-1754) fix to constructors. * @@ -237,102 +237,112 @@ to account for it by ensuring that at least one parameter does not have a defaul public void processConstructors(final ClassCodeExpressionTransformer visitor, ClassNode classNode) { ClassNode superClass = classNode.getSuperClass(); List declaredConstructors = classNode.getDeclaredConstructors(); - if (TRIVIAL_CONSTRUCTORS.contains(superClass.getName())) { - for (ConstructorNode c : declaredConstructors) { - visitor.visitMethod(c); + if (declaredConstructors.isEmpty()) { + if (classNode.isInterface()) { + // Interfaces are expected to not have constructors. + return; } + // Default constructor should have already been added by InitialExpressionExpander + throw new AssertionError("No constructors for " + classNode); } else { - if (declaredConstructors.isEmpty()) { - // Default constructor should have already been added by InitialExpressionExpander - throw new AssertionError("No constructors for " + classNode); + declaredConstructors = new ArrayList<>(declaredConstructors); + } + for (ConstructorNode c : declaredConstructors) { + for (Parameter p : c.getParameters()) { + if (p.hasInitialExpression()) { + // All initial expressions should have already been removed by InitialExpressionExpander + throw new AssertionError("Found unexpected initial expression: " + p.getInitialExpression()); + } + } + Statement code = c.getCode(); + List body; + if (code instanceof BlockStatement) { + body = ((BlockStatement) code).getStatements(); } else { - declaredConstructors = new ArrayList<>(declaredConstructors); + body = Collections.singletonList(code); } - for (ConstructorNode c : declaredConstructors) { - for (Parameter p : c.getParameters()) { - if (p.hasInitialExpression()) { - // All initial expressions should have already been removed by InitialExpressionExpander - throw new AssertionError("Found unexpected initial expression: " + p.getInitialExpression()); - } - } - Statement code = c.getCode(); - List body; - if (code instanceof BlockStatement) { - body = ((BlockStatement) code).getStatements(); + ClassNode constructorCallType = ClassNode.SUPER; + TupleExpression constructorCallArgs = new TupleExpression(); + if (!body.isEmpty() && body.get(0) instanceof ExpressionStatement && ((ExpressionStatement) body.get(0)).getExpression() instanceof ConstructorCallExpression) { + ConstructorCallExpression cce = (ConstructorCallExpression) ((ExpressionStatement) body.get(0)).getExpression(); + if (cce.isThisCall()) { + constructorCallType = ClassNode.THIS; + body = body.subList(1, body.size()); + constructorCallArgs = ((TupleExpression) cce.getArguments()); + } else if (cce.isSuperCall()) { + body = body.subList(1, body.size()); + constructorCallArgs = ((TupleExpression) cce.getArguments()); } else { - body = Collections.singletonList(code); - } - ClassNode constructorCallType = ClassNode.SUPER; - TupleExpression constructorCallArgs = new TupleExpression(); - if (!body.isEmpty() && body.get(0) instanceof ExpressionStatement && ((ExpressionStatement) body.get(0)).getExpression() instanceof ConstructorCallExpression) { - ConstructorCallExpression cce = (ConstructorCallExpression) ((ExpressionStatement) body.get(0)).getExpression(); - if (cce.isThisCall()) { - constructorCallType = ClassNode.THIS; - body = body.subList(1, body.size()); - constructorCallArgs = ((TupleExpression) cce.getArguments()); - } else if (cce.isSuperCall()) { - body = body.subList(1, body.size()); - constructorCallArgs = ((TupleExpression) cce.getArguments()); - } + // Some other class, for example if `new String();` happens to be the first statement + // in a constructor. We handle this the same as an explicit call to `super()`. } - final TupleExpression _constructorCallArgs = constructorCallArgs; - final AtomicReference constructorCallArgsTransformed = new AtomicReference<>(); - ((ScopeTrackingClassCodeExpressionTransformer) visitor).withMethod(c, new Runnable() { - @Override - public void run() { - constructorCallArgsTransformed.set(((VisitorImpl) visitor).transformArguments(_constructorCallArgs)); - } - }); - // Create parameters for new constructor. - Parameter[] origParams = c.getParameters(); - Parameter[] params = new Parameter[origParams.length + 1]; - params[0] = new Parameter(new ClassNode(constructorCallType == ClassNode.THIS ? Checker.ThisConstructorWrapper.class : Checker.SuperConstructorWrapper.class), "$cw"); - System.arraycopy(origParams, 0, params, 1, origParams.length); - List paramTypes = new ArrayList<>(params.length); - for (Parameter p : params) { - paramTypes.add(new ClassExpression(p.getType())); - } - // Create arguments for call to synthetic constructor. - List thisArgs = new ArrayList<>(origParams.length + 1); - thisArgs.add(null); // Placeholder - List thisArgsWithoutWrapper = new ArrayList<>(origParams.length); - for (Parameter p : origParams) { - thisArgs.add(new VariableExpression(p)); - thisArgsWithoutWrapper.add(new VariableExpression(p)); - } - if (constructorCallType == ClassNode.THIS) { - thisArgs.set(0, ((VisitorImpl) visitor).makeCheckedCall("checkedThisConstructor", - new ClassExpression(classNode), - constructorCallArgsTransformed.get(), - new ArrayExpression(new ClassNode(Object.class), thisArgsWithoutWrapper), - new ArrayExpression(new ClassNode(Class.class), paramTypes))); - } else { - thisArgs.set(0, ((VisitorImpl) visitor).makeCheckedCall("checkedSuperConstructor", - new ClassExpression(classNode), - new ClassExpression(superClass), - constructorCallArgsTransformed.get(), - new ArrayExpression(new ClassNode(Object.class), thisArgsWithoutWrapper), - new ArrayExpression(new ClassNode(Class.class), paramTypes))); + } + // SECURITY-3341 + // Only sandbox-transform super() constructor calls if the parent class is nontrivial. Always sandbox-transform this() constructor calls. + if (constructorCallType == ClassNode.SUPER && TRIVIAL_CONSTRUCTORS.contains(superClass.getName())) { + visitor.visitMethod(c); + continue; + } + final TupleExpression _constructorCallArgs = constructorCallArgs; + final AtomicReference constructorCallArgsTransformed = new AtomicReference<>(); + ((ScopeTrackingClassCodeExpressionTransformer) visitor).withMethod(c, new Runnable() { + @Override + public void run() { + constructorCallArgsTransformed.set(((VisitorImpl) visitor).transformArguments(_constructorCallArgs)); } - c.setCode(new BlockStatement(new Statement[] {new ExpressionStatement(new ConstructorCallExpression(ClassNode.THIS, new TupleExpression(thisArgs)))}, c.getVariableScope())); - List cwArgs = new ArrayList<>(); - int x = 0; - for (Expression constructorCallArg : constructorCallArgs) { - cwArgs.add(/*new CastExpression(superArg.getType(), */new MethodCallExpression(new VariableExpression("$cw"), "arg", new ConstantExpression(x++))/*)*/); + }); + // Create parameters for new constructor. + Parameter[] origParams = c.getParameters(); + Parameter[] params = new Parameter[origParams.length + 1]; + params[0] = new Parameter(new ClassNode(constructorCallType == ClassNode.THIS ? Checker.ThisConstructorWrapper.class : Checker.SuperConstructorWrapper.class), "$cw"); + System.arraycopy(origParams, 0, params, 1, origParams.length); + List paramTypes = new ArrayList<>(params.length); + for (Parameter p : params) { + paramTypes.add(new ClassExpression(p.getType())); + } + // Create arguments for call to synthetic constructor. + List thisArgs = new ArrayList<>(origParams.length + 1); + thisArgs.add(null); // Placeholder + List thisArgsWithoutWrapper = new ArrayList<>(origParams.length); + for (Parameter p : origParams) { + if (p.getType().equals(superConstructorWrapperClass) || p.getType().equals(thisConstructorWrapperClass)) { + throw new SecurityException("Illegal constructor parameter for " + classNode + ": " + p); } - List body2 = new ArrayList<>(body.size() + 1); - body2.add(0, new ExpressionStatement(new ConstructorCallExpression(constructorCallType, new ArgumentListExpression(cwArgs)))); - body2.addAll(body); - ((ScopeTrackingClassCodeExpressionTransformer) visitor).withMethod(c, () -> { - for (int i = 1; i < body2.size(); i++) { // Skip the first statement, which is the constructor call. - body2.get(i).visit(visitor); - } - }); - final int SYNTHETIC = 0x00001000; // Not public in Modifier - ConstructorNode c2 = new ConstructorNode(Modifier.PRIVATE | SYNTHETIC, params, c.getExceptions(), new BlockStatement(body2, c.getVariableScope())); - // perhaps more misleading than helpful: c2.setSourcePosition(c); - classNode.addConstructor(c2); + thisArgs.add(new VariableExpression(p)); + thisArgsWithoutWrapper.add(new VariableExpression(p)); + } + if (constructorCallType == ClassNode.THIS) { + thisArgs.set(0, ((VisitorImpl) visitor).makeCheckedCall("checkedThisConstructor", + new ClassExpression(classNode), + constructorCallArgsTransformed.get(), + new ArrayExpression(new ClassNode(Object.class), thisArgsWithoutWrapper), + new ArrayExpression(new ClassNode(Class.class), paramTypes))); + } else { + thisArgs.set(0, ((VisitorImpl) visitor).makeCheckedCall("checkedSuperConstructor", + new ClassExpression(classNode), + new ClassExpression(superClass), + constructorCallArgsTransformed.get(), + new ArrayExpression(new ClassNode(Object.class), thisArgsWithoutWrapper), + new ArrayExpression(new ClassNode(Class.class), paramTypes))); + } + c.setCode(new BlockStatement(new Statement[] {new ExpressionStatement(new ConstructorCallExpression(ClassNode.THIS, new TupleExpression(thisArgs)))}, c.getVariableScope())); + List cwArgs = new ArrayList<>(); + int x = 0; + for (Expression constructorCallArg : constructorCallArgs) { + cwArgs.add(/*new CastExpression(superArg.getType(), */new MethodCallExpression(new VariableExpression("$cw"), "arg", new ConstantExpression(x++))/*)*/); } + List body2 = new ArrayList<>(body.size() + 1); + body2.add(0, new ExpressionStatement(new ConstructorCallExpression(constructorCallType, new ArgumentListExpression(cwArgs)))); + body2.addAll(body); + ((ScopeTrackingClassCodeExpressionTransformer) visitor).withMethod(c, () -> { + for (int i = 1; i < body2.size(); i++) { // Skip the first statement, which is the constructor call. + body2.get(i).visit(visitor); + } + }); + final int SYNTHETIC = 0x00001000; // Not public in Modifier + ConstructorNode c2 = new ConstructorNode(Modifier.PRIVATE | SYNTHETIC, params, c.getExceptions(), new BlockStatement(body2, c.getVariableScope())); + // perhaps more misleading than helpful: c2.setSourcePosition(c); + classNode.addConstructor(c2); } } @@ -1044,6 +1054,8 @@ public static boolean isKnownSafeCast(ClassNode type, Expression exp) { static final ClassNode checkerClass = new ClassNode(Checker.class); static final ClassNode ScriptBytecodeAdapterClass = new ClassNode(ScriptBytecodeAdapter.class); + static final ClassNode superConstructorWrapperClass = new ClassNode(Checker.SuperConstructorWrapper.class); + static final ClassNode thisConstructorWrapperClass = new ClassNode(Checker.ThisConstructorWrapper.class); /** * Expression that accesses the closure object itself from within the closure. diff --git a/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java b/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java index ff834d7..bfe21ec 100644 --- a/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java +++ b/src/test/java/org/kohsuke/groovy/sandbox/SandboxTransformerTest.java @@ -79,11 +79,13 @@ public void setUp() { unsandboxedSh = new GroovyShell(binding,cc); } + public void configureBinding() { } + /** * Use {@code ShouldFail.class} as the expected result for {@link #sandboxedEval} and {@link #unsandboxedEval} * when the expression is expected to throw an exception. */ - private static final class ShouldFail { } + public static final class ShouldFail { } @FunctionalInterface public interface ExceptionHandler { @@ -93,12 +95,12 @@ public interface ExceptionHandler { /** * Executes a Groovy expression inside of the sandbox. * @param expression The Groovy expression to execute. - * @return the result of executing the expression. */ - private void sandboxedEval(String expression, Object expectedResult, ExceptionHandler handler) { + public void sandboxedEval(String expression, Object expectedResult, ExceptionHandler handler) { cr.reset(); cr.register(); try { + configureBinding(); Object actual = sandboxedSh.evaluate(expression); String actualType = GroovyCallSiteSelector.getName(actual); String expectedType = GroovyCallSiteSelector.getName(expectedResult); @@ -121,10 +123,10 @@ private void sandboxedEval(String expression, Object expectedResult, ExceptionHa /** * Executes a Groovy expression outside of the sandbox. * @param expression The Groovy expression to execute. - * @return the result of executing the expression. */ private void unsandboxedEval(String expression, Object expectedResult, ExceptionHandler handler) { try { + configureBinding(); Object actual = unsandboxedSh.evaluate(expression); String actualType = GroovyCallSiteSelector.getName(actual); String expectedType = GroovyCallSiteSelector.getName(expectedResult); @@ -144,16 +146,33 @@ private void unsandboxedEval(String expression, Object expectedResult, Exception * @param expectedReturnValue The expected return value for running the script. * @param expectedCalls The method calls that are expected to be intercepted by the sandbox. */ - private void assertIntercept(String expression, Object expectedReturnValue, String... expectedCalls) { + public void assertIntercept(String expression, Object expectedReturnValue, String... expectedCalls) { assertEvaluate(expression, expectedReturnValue); assertIntercepted(expectedCalls); } + /** + * Check that the most recently executed expression intercepted the expected calls. + * Automatically adds {@code new Script(Binding)} to the list of intercepted calls. + * @param expectedCalls The method calls that were expected to be intercepted by the sandbox. + * @see #assertInterceptedExact + */ + public void assertIntercepted(String... expectedCalls) { + // Workaround to avoid having to update all existing tests. + String[] updatedExpectedCalls = expectedCalls; + if (expectedCalls.length == 0 || (expectedCalls.length > 0 && !expectedCalls[0].equals("new Script(Binding)"))) { + updatedExpectedCalls = new String[expectedCalls.length + 1]; + updatedExpectedCalls[0] = "new Script(Binding)"; + System.arraycopy(expectedCalls, 0, updatedExpectedCalls, 1, expectedCalls.length); + } + assertInterceptedExact(updatedExpectedCalls); + } + /** * Check that the most recently executed expression intercepted the expected calls. * @param expectedCalls The method calls that were expected to be intercepted by the sandbox. */ - private void assertIntercepted(String... expectedCalls) { + public void assertInterceptedExact(String... expectedCalls) { String[] interceptedCalls = cr.toString().split("\n"); if (interceptedCalls.length == 1 && interceptedCalls[0].equals("")) { interceptedCalls = new String[0]; @@ -167,7 +186,7 @@ private void assertIntercepted(String... expectedCalls) { * @param expression The Groovy expression to execute. * @param expectedReturnValue The expected return value for running the script. */ - private void assertEvaluate(String expression, Object expectedReturnValue) { + public void assertEvaluate(String expression, Object expectedReturnValue) { sandboxedEval(expression, expectedReturnValue, e -> { throw new RuntimeException("Failed to evaluate sandboxed expression: " + expression, e); }); @@ -429,6 +448,55 @@ private void assertFailsWithSameException(String expression) { "new Superclass()"); } + @Issue("SECURITY-3341") + @Test public void sandboxBlocksCastingInThisConstructorCalls() throws Exception { + sandboxedEval( + "class Subclass {\n" + + " def x\n" + + " Subclass() { this(['secret.key']) }\n" + + " Subclass(File f) { this.x = f }\n" + + "}\n" + + "new Subclass().x\n", + ShouldFail.class, + e -> assertThat(e.getMessage(), containsString("Unable to find constructor: new Subclass java.util.ArrayList"))); + sandboxedEval( + "class Subclass {\n" + + " def x\n" + + " Subclass(File f) { this.x = f }\n" + + "}\n" + + "(new Subclass(['secret.key']) { def getFoo() { x } }).foo\n", + ShouldFail.class, + e -> assertThat(e.getMessage(), containsString("Unable to find constructor: new Subclass java.util.ArrayList"))); + } + + @Issue("SECURITY-3341") + @Test public void sandboxBlocksCastingInSuperConstructorCalls() throws Exception { + sandboxedEval( + "package com.cloudbees.groovy.cps\n" + + "class SerializableScript {\n" + + " def x\n" + + " SerializableScript(File f) { this.x = f }\n" + + "}\n" + + "class Subclass extends SerializableScript {\n" + + " Subclass() { super(['secret.key']) }" + + "}\n" + + "new Subclass().x\n", + ShouldFail.class, + e -> assertThat(e.getMessage(), containsString("Unable to find constructor: new com.cloudbees.groovy.cps.SerializableScript java.util.ArrayList"))); + sandboxedEval( + "package java.lang\n" + + "class Object {\n" + + " def x\n" + + " Object(File f) { this.x = f }\n" + + "}\n" + + "class Subclass extends Object {\n" + + " Subclass() { super(['secret.key']) }" + + "}\n" + + "new Subclass().x\n", + ShouldFail.class, + e -> assertThat(e.getMessage(), containsString("Prohibited package name: java.lang"))); + } + @Issue({ "SECURITY-1754", "SECURITY-2824" }) @Test public void blocksDirectCallsToSyntheticConstructors() throws Exception { sandboxedEval( @@ -844,6 +912,7 @@ public void sandboxInterceptsImplicitCastsInitialParameterExpressions() { new File("secret.key"), "new Test()", "new File(String)", + "new Test(File)", "Test.x"); } diff --git a/src/test/java/org/kohsuke/groovy/sandbox/TheTest.java b/src/test/java/org/kohsuke/groovy/sandbox/TheTest.java index ab5c2d1..df8948f 100644 --- a/src/test/java/org/kohsuke/groovy/sandbox/TheTest.java +++ b/src/test/java/org/kohsuke/groovy/sandbox/TheTest.java @@ -1,8 +1,5 @@ package org.kohsuke.groovy.sandbox; -import groovy.lang.Binding; -import groovy.lang.GroovyShell; -import org.codehaus.groovy.control.customizers.ImportCustomizer; import org.codehaus.groovy.runtime.NullObject; import org.codehaus.groovy.runtime.ProxyGeneratorAdapter; import org.jvnet.hudson.test.Issue; @@ -15,14 +12,10 @@ import java.util.List; import java.util.Locale; import java.util.concurrent.atomic.AtomicLong; -import org.codehaus.groovy.control.CompilerConfiguration; import org.codehaus.groovy.runtime.ResourceGroovyMethods; -import org.kohsuke.groovy.sandbox.impl.GroovyCallSiteSelector; -import org.junit.Before; import org.junit.Test; import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.CoreMatchers.equalTo; import static org.hamcrest.CoreMatchers.instanceOf; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; @@ -32,25 +25,9 @@ * * @author Kohsuke Kawaguchi */ -public class TheTest { - GroovyShell sh, plain; - Binding binding = new Binding(); - ClassRecorder cr = new ClassRecorder(); - - @Before - public void setUp() { - CompilerConfiguration cc = new CompilerConfiguration(); - cc.addCompilationCustomizers(new ImportCustomizer().addImports(TheTest.class.getName()).addStarImports("org.kohsuke.groovy.sandbox")); - cc.addCompilationCustomizers(new SandboxTransformer()); - sh = new GroovyShell(binding,cc); - - cc = new CompilerConfiguration(); - cc.addCompilationCustomizers(new ImportCustomizer().addImports(TheTest.class.getName()).addStarImports("org.kohsuke.groovy.sandbox")); - plain = new GroovyShell(binding,cc); - - } - - private void initVars() { +public class TheTest extends SandboxTransformerTest { + @Override + public void configureBinding() { binding.setProperty("foo", "FOO"); binding.setProperty("bar", "BAR"); binding.setProperty("zot", 5); @@ -59,47 +36,21 @@ private void initVars() { binding.setProperty("intArray", new int[] { 0, 1, 2, 3, 4 }); } - /** - * Evaluates the expression while intercepting calls. - */ - private Object interceptedEval(String expression) throws Exception { - cr.reset(); - cr.register(); - try { - initVars(); - return sh.evaluate(expression); - } catch (Exception e) { - throw new Exception("Failed to evaluate " + expression, e); - } finally { - cr.unregister(); - } - } - - /** - * In addition to {@link #interceptedEval(String)}, verify that the result is the same as regular non-intercepted Groovy call. - */ - private Object eval(String expression) throws Exception { - initVars(); - Object expected = plain.evaluate(expression); - - Object actual = interceptedEval(expression); - String actualType = GroovyCallSiteSelector.getName(actual); - String expectedType = GroovyCallSiteSelector.getName(expected); - assertThat("Sandboxed result (" + actualType + ") does not match expected result (" + expectedType + ")", actual, equalTo(expected)); - return actual; + private void assertIntercept(String expectedCallSequence, Object expectedValue, String script) throws Exception { + String[] expectedCalls = expectedCallSequence.isEmpty() ? new String[0] : expectedCallSequence.split("/"); + assertIntercept(script, expectedValue, expectedCalls); } - private void assertIntercept(String expectedCallSequence, Object expectedValue, String script) throws Exception { - Object actual = eval(script); - assertEquals(expectedValue, actual); - assertEquals(expectedCallSequence.replace('/','\n').trim(), cr.toString().trim()); + private void assertInterceptNoScript(String expectedCallSequence, Object expectedValue, String script) throws Exception { + String[] expectedCalls = expectedCallSequence.isEmpty() ? new String[0] : expectedCallSequence.split("/"); + assertEvaluate(script, expectedValue); + assertInterceptedExact(expectedCalls); } private void assertIntercept(List expectedCallSequence, Object expectedValue, String script) throws Exception { - assertIntercept(String.join("\n", expectedCallSequence), expectedValue, script); + assertIntercept(script, expectedValue, expectedCallSequence.toArray(new String[0])); } - @Test public void testOK() throws Exception { // instance call assertIntercept( @@ -183,14 +134,14 @@ private void assertIntercept(List expectedCallSequence, Object expectedV } @Test public void testClass() throws Exception { - assertIntercept( + assertInterceptNoScript( "Integer.class/Class:forName(String)", null, "class foo { static void main(String[] args) throws Exception { 5.class.forName('java.lang.String') } }"); } @Test public void testInnerClass() throws Exception { - assertIntercept( + assertInterceptNoScript( "foo$bar:juu()/Integer.class/Class:forName(String)", null, "class foo {\n" + @@ -202,7 +153,7 @@ private void assertIntercept(List expectedCallSequence, Object expectedV } @Test public void testStaticInitializationBlock() throws Exception { - assertIntercept( + assertInterceptNoScript( "Integer.class/Class:forName(String)", null, "class foo {\n" + @@ -634,22 +585,26 @@ public Object f(Object[] arg) { } @Test public void testCatchStatement() throws Exception { - Exception e = (Exception)interceptedEval( + sandboxedEval( "def o = null\n" + "try {\n" + " o.hello()\n" + " return null\n" + "} catch (Exception e) {\n" + - " return e\n" + - "}"); - assertThat(e, instanceOf(NullPointerException.class)); + " throw new Exception('wrapped', e)\n" + + "}", + ShouldFail.class, + e -> { + assertThat(e.getMessage(), containsString("wrapped")); + assertThat(e.getCause(), instanceOf(NullPointerException.class)); + }); } /** * Makes sure the line number in the source code is preserved after translation. */ @Test public void testIssue21() throws Exception { - Exception e = (Exception)interceptedEval( + sandboxedEval( "\n" + // line 1 "def x = null\n" + "def cl = {\n" + @@ -658,39 +613,50 @@ public Object f(Object[] arg) { "try {\n" + " cl();\n" + // line 7 "} catch (Exception e) {\n" + - " return e\n" + - "}"); + " throw new Exception('wrapped', e)\n" + + "}", + ShouldFail.class, + e -> { + assertThat(e.getMessage(), containsString("wrapped")); + StringWriter sw = new StringWriter(); + e.printStackTrace(new PrintWriter(sw)); - StringWriter sw = new StringWriter(); - e.printStackTrace(new PrintWriter(sw)); - - String s = sw.toString(); - assertThat(s, containsString("Script1.groovy:4")); - assertThat(s, containsString("Script1.groovy:7")); + String s = sw.toString(); + assertThat(s, containsString("Script1.groovy:4")); + assertThat(s, containsString("Script1.groovy:7")); + }); } @Test public void testIssue15() throws Exception { - Object e = interceptedEval( + sandboxedEval( "try {\n" + " def x = null\n" + " return x.nullProp\n" + "} catch (Exception e) {\n" + - " return e\n" + - "}"); - assertThat(e, instanceOf(NullPointerException.class)); - // x.nullProp shouldn't be intercepted, so the record should be empty - assertEquals("", cr.toString().trim()); - - e = interceptedEval( + " throw new Exception('wrapped', e)\n" + + "}", + ShouldFail.class, + e -> { + assertThat(e.getMessage(), containsString("wrapped")); + assertThat(e.getCause(), instanceOf(NullPointerException.class)); + }); + // x.nullProp shouldn't be intercepted + assertIntercepted("new Exception(String,NullPointerException)"); + + sandboxedEval( "try {\n" + " def x = null\n" + " x.nullProp = 1\n" + "} catch (Exception e) {\n" + - " return e\n" + - "}"); - assertThat(e, instanceOf(NullPointerException.class)); - // x.nullProp shouldn't be intercepted, so the record should be empty - assertEquals("", cr.toString().trim()); + " throw new Exception('wrapped', e)\n" + + "}", + ShouldFail.class, + e -> { + assertThat(e.getMessage(), containsString("wrapped")); + assertThat(e.getCause(), instanceOf(NullPointerException.class)); + }); + // x.nullProp shouldn't be intercepted + assertIntercepted("new Exception(String,NullPointerException)"); } @Test public void testInOperator() throws Exception { @@ -774,7 +740,7 @@ public Object f(Object[] arg) { pxyCounterField.setAccessible(true); AtomicLong pxyCounterValue = (AtomicLong) pxyCounterField.get(null); pxyCounterValue.set(0); // make sure *_groovyProxy names are predictable - assertIntercept("Locale:getDefault()/Class2_groovyProxy.getDefault()", + assertIntercept("Locale:getDefault()/Class1_groovyProxy.getDefault()", Locale.getDefault(), "interface I {\n" + " Locale getDefault()\n" +