[JENKINS-26481] Fail the script meaningfully when it attempts to pass…

… a CPS-transformed closure to a binary method.
jenkinsci · Jun 6, 2016 · efeee2b · efeee2b · batmat · Jun 10, 2016
1 parent 632bc78
commit efeee2b
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 12 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,38 @@
 package org.jenkinsci.plugins.workflow.cps;
 
+import com.cloudbees.groovy.cps.impl.CpsClosure;
 import groovy.lang.GroovyClassLoader;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
 
 import java.lang.reflect.Constructor;
+import java.lang.reflect.Executable;
 import java.lang.reflect.Field;
 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 +44,48 @@ private boolean permits(Class<?> declaringClass) {
     }
 
     @Override public boolean permitsMethod(Method method, Object receiver, Object[] args) {
-        return permits(method.getDeclaringClass());
+        return checkJenkins26481(args, method) || delegate.permitsMethod(method, receiver, args);
     }
 
     @Override public boolean permitsConstructor(Constructor<?> constructor, Object[] args) {
-        return permits(constructor.getDeclaringClass());
+        return checkJenkins26481(args, constructor) || delegate.permitsConstructor(constructor, args);
     }
 
     @Override public boolean permitsStaticMethod(Method method, Object[] args) {
-        return permits(method.getDeclaringClass());
+        return checkJenkins26481(args, method) || 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, Executable method) throws UnsupportedOperationException {
+        if (permits(method.getDeclaringClass())) { // fine for source-defined methods to take closures
+            return true;
+        }
+        for (Object arg : args) {
+            if (arg instanceof CpsClosure) {
+                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
@@ -233,6 +233,31 @@ public void stop(Throwable cause) throws Exception {
         });
     }
 
+    @Issue("JENKINS-26481")
+    @Test public void eachClosureNonCps() {
+        story.addStep(new Statement() {
+            @Override public void evaluate() throws Throwable {
+                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" +
+                    "    echo body()\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() {