Merge pull request #15 from jglick/report-JENKINS-26481

[JENKINS-26481] More clearly report problem passing closures to binary methods
jenkinsci · Jun 10, 2016 · 0043928 · 0043928
2 parents 7364fe7 + 4266f0c
commit 0043928
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 15 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java
@@ -1,24 +1,39 @@
 package org.jenkinsci.plugins.workflow.cps;
 
+import com.cloudbees.groovy.cps.impl.CpsClosure;
+import groovy.lang.Closure;
 import groovy.lang.GroovyClassLoader;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
 
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
+import java.lang.reflect.Member;
 import java.lang.reflect.Method;
+import org.codehaus.groovy.runtime.DefaultGroovyMethods;
+import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
+import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelist;
+import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist;
 
 /**
  * Allow any calls into the scripted compiled in the sandbox.
  *
  * IOW, allow the user to call his own methods.
- *
- * @author Kohsuke Kawaguchi
  */
 class GroovyClassLoaderWhitelist extends Whitelist {
+
     private final ClassLoader scriptLoader;
 
-    public GroovyClassLoaderWhitelist(GroovyClassLoader scriptLoader) {
+    /**
+     * {@link ProxyWhitelist} has an optimization which bypasses {@code permits*} calls
+     * when it detects {@link EnumeratingWhitelist} delegates,
+     * so we must do delegation manually.
+     * For the same reason, doing this check inside {@link CpsWhitelist} is tricky.
+     */
+    private final Whitelist delegate;
+
+    GroovyClassLoaderWhitelist(GroovyClassLoader scriptLoader, Whitelist delegate) {
         this.scriptLoader = scriptLoader;
+        this.delegate = delegate;
     }
 
     private boolean permits(Class<?> declaringClass) {
@@ -30,30 +45,48 @@ private boolean permits(Class<?> declaringClass) {
     }
 
     @Override public boolean permitsMethod(Method method, Object receiver, Object[] args) {
-        return permits(method.getDeclaringClass());
+        return checkJenkins26481(args, method, method.getParameterTypes()) || delegate.permitsMethod(method, receiver, args);
     }
 
     @Override public boolean permitsConstructor(Constructor<?> constructor, Object[] args) {
-        return permits(constructor.getDeclaringClass());
+        return checkJenkins26481(args, constructor, constructor.getParameterTypes()) || delegate.permitsConstructor(constructor, args);
     }
 
     @Override public boolean permitsStaticMethod(Method method, Object[] args) {
-        return permits(method.getDeclaringClass());
+        return checkJenkins26481(args, method, method.getParameterTypes()) || delegate.permitsStaticMethod(method, args);
     }
 
     @Override public boolean permitsFieldGet(Field field, Object receiver) {
-        return permits(field.getDeclaringClass());
+        return permits(field.getDeclaringClass()) || delegate.permitsFieldGet(field, receiver);
     }
 
     @Override public boolean permitsFieldSet(Field field, Object receiver, Object value) {
-        return permits(field.getDeclaringClass());
+        return permits(field.getDeclaringClass()) || delegate.permitsFieldSet(field, receiver, value);
     }
 
     @Override public boolean permitsStaticFieldGet(Field field) {
-        return permits(field.getDeclaringClass());
+        return permits(field.getDeclaringClass()) || delegate.permitsStaticFieldGet(field);
     }
 
     @Override public boolean permitsStaticFieldSet(Field field, Object value) {
-        return permits(field.getDeclaringClass());
+        return permits(field.getDeclaringClass()) || delegate.permitsStaticFieldSet(field, value);
+    }
+
+    /**
+     * Blocks accesses to some {@link DefaultGroovyMethods}, or any other binary method taking a closure, until JENKINS-26481 is fixed.
+     * Only improves diagnostics for scripts using the sandbox.
+     * Note that we do not simply return false for these cases, as that would throw {@link RejectedAccessException} without a meaningful explanation.
+     */
+    private boolean checkJenkins26481(Object[] args, /* TODO Java 8: just take Executable */ Member method, Class<?>[] parameterTypes) throws UnsupportedOperationException {
+        if (permits(method.getDeclaringClass())) { // fine for source-defined methods to take closures
+            return true;
+        }
+        for (int i = 0; i < args.length; i++) {
+            if (args[i] instanceof CpsClosure && parameterTypes.length > i && parameterTypes[i] == Closure.class) {
+                throw new UnsupportedOperationException("Calling " + method + " on a CPS-transformed closure is not yet supported (JENKINS-26481); encapsulate in a @NonCPS method, or use Java-style loops");
+            }
+        }
+        return false;
     }
+
 }
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/SandboxContinuable.java
@@ -4,7 +4,6 @@
 import com.cloudbees.groovy.cps.Outcome;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox;
-import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.ProxyWhitelist;
 import org.jenkinsci.plugins.scriptsecurity.scripts.ApprovalContext;
 import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
 
@@ -37,7 +36,7 @@ public Outcome call() {
                     }
                     return outcome;
                 }
-            }, new ProxyWhitelist(new GroovyClassLoaderWhitelist(thread.group.getExecution().getShell().getClassLoader()),CpsWhitelist.get()));
+            }, new GroovyClassLoaderWhitelist(thread.group.getExecution().getShell().getClassLoader(), CpsWhitelist.get()));
         } catch (RuntimeException e) {
             throw e;
         } catch (Exception e) {

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java b/src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java
@@ -1,5 +1,6 @@
 package org.jenkinsci.plugins.workflow;
 
+import groovy.lang.Closure;
 import hudson.model.Result;
 import hudson.slaves.DumbSlave;
 import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
@@ -234,6 +235,43 @@ public void stop(Throwable cause) throws Exception {
         });
     }
 
+    /**
+     * Verifies that we are not throwing {@link UnsupportedOperationException} too aggressively.
+     * In particular:
+     * <ul>
+     * <li>on non-CPS-transformed {@link Closure}s
+     * <li>on closures passed to methods defined in Pipeline script
+     * <li>on closures passed to methods which did not declare {@link Closure} as a parameter type and so presumably are not going to try to call them
+     * </ul>
+     */
+    @Issue("JENKINS-26481")
+    @Test public void eachClosureNonCps() {
+        story.addStep(new Statement() {
+            @Override public void evaluate() throws Throwable {
+                ScriptApproval.get().approveSignature("staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods plus java.util.Collection java.lang.Object");
+                p = jenkins().createProject(WorkflowJob.class, "demo");
+                p.setDefinition(new CpsFlowDefinition(
+                    "@NonCPS def fine() {\n" +
+                    "  def text = ''\n" +
+                    "  ['a', 'b', 'c'].each {it -> text += it}\n" +
+                    "  text\n" +
+                    "}\n" +
+                    "def takesMyOwnClosure(body) {\n" +
+                    "  node {\n" +
+                    "    def list = []\n" +
+                    "    list += body\n" +
+                    "    echo list[0]()\n" +
+                    "  }\n" +
+                    "}\n" +
+                    "takesMyOwnClosure {\n" +
+                    "  fine()\n" +
+                    "}\n", true));
+                b = story.j.assertBuildStatusSuccess(p.scheduleBuild2(0));
+                story.j.assertLogContains("abc", b);
+            }
+        });
+    }
+
     @Ignore("TODO JENKINS-31314: calls writeFile just once, echoes null (i.e., return value of writeFile), then succeeds")
     @Test public void nonCpsContinuable() {
         story.addStep(new Statement() {

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/cps/steps/RestartingLoadStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/cps/steps/RestartingLoadStepTest.java
@@ -7,9 +7,11 @@
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.jenkinsci.plugins.workflow.test.steps.SemaphoreStep;
 import static org.junit.Assert.*;
+import org.junit.ClassRule;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runners.model.Statement;
+import org.jvnet.hudson.test.BuildWatcher;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.RestartableJenkinsRule;
 
@@ -19,6 +21,8 @@
  * @author Kohsuke Kawaguchi
  */
 public class RestartingLoadStepTest {
+
+    @ClassRule public static BuildWatcher buildWatcher = new BuildWatcher();
     @Rule public RestartableJenkinsRule story = new RestartableJenkinsRule();
 
     @Inject
@@ -34,8 +38,8 @@ public void persistenceOfLoadedScripts() throws Exception {
                 WorkflowJob p = jenkins.createProject(WorkflowJob.class, "p");
                 jenkins.getWorkspaceFor(p).child("test.groovy").write(
                     "def answer(i) { return i*2; }\n" +
-                    "def foo() {\n" +
-                    "    def i=21;\n" +
+                    "def foo(body) {\n" +
+                    "    def i = body()\n" +
                     "    semaphore 'watchA'\n" +
                     "    return answer(i);\n" +
                     "}\n" +
@@ -44,7 +48,7 @@ public void persistenceOfLoadedScripts() throws Exception {
                     "node {\n" +
                     "  println 'started'\n" +
                     "  def o = load 'test.groovy'\n" +
-                    "  println 'o=' + o.foo();\n" +
+                    "  println 'o=' + o.foo({21})\n" +
                     "}"));
 
                 // get the build going