New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-34650] Test for workflowLibs sandbox behavior #2

Merged
merged 1 commit into from Jun 15, 2016

Conversation

Projects
None yet
4 participants
@jglick
Member

jglick commented May 6, 2016

JENKINS-34650

Can reproduce problem but have no idea how to fix. Maybe @kohsuke knows. I tried

diff --git a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsGroovyShell.java b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsGroovyShell.java
index fdbb0da..cf761cc 100644
--- a/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsGroovyShell.java
+++ b/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsGroovyShell.java
@@ -5,6 +5,7 @@ import com.cloudbees.groovy.cps.NonCPS;
 import com.cloudbees.groovy.cps.SandboxCpsTransformer;
 import com.cloudbees.groovy.cps.TransformerConfiguration;
 import groovy.lang.Binding;
+import groovy.lang.GroovyClassLoader;
 import groovy.lang.GroovyCodeSource;
 import groovy.lang.GroovyShell;
 import groovy.lang.Script;
@@ -98,6 +99,15 @@ class CpsGroovyShell extends GroovyShell {
         return s;
     }

+    @Override
+    public GroovyClassLoader getClassLoader() {
+        CompilerConfiguration cc = new CompilerConfiguration();
+        CpsTransformer t = new CpsTransformer(); // TODO conditionally use SandboxCpsTransformer
+        t.setConfiguration(new TransformerConfiguration().withClosureType(CpsClosure2.class));
+        cc.addCompilationCustomizers(t);
+        return new GroovyClassLoader(super.getClassLoader(), cc);
+    }
+
     /**
      * Used internally to reload the script back when coming back from the persisted state
      * (therefore we don't want to record this.)

but then the resulting loader does not work at all:

[p #1] org.codehaus.groovy.control.MultipleCompilationErrorsException: startup failed:
[p #1] WorkflowScript: 1: unable to resolve class pkg.Privileged 
[p #1]  @ line 1, column 1.
[p #1]    new pkg.Privileged().write('direct-false')
[p #1]    ^

In the meantime, the test at least covers the control case.

@reviewbybees

@reviewbybees

This comment has been minimized.

reviewbybees commented May 6, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@@ -143,4 +148,55 @@ public void userDefinedGlobalVariable() throws Exception {
}
});
}
/** Global libraries should run outside the sandbox, regardless of whether the caller is sandboxed. */

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 9, 2016

Member

IMHO such behavior should be configurable.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented May 9, 2016

I have no idea how to fix it as well :(
Maybe we need a kind of the "Pipeline stacktrace" support with markers ("Acquired lock", "Entered CPS library section", etc.)

@jglick

This comment has been minimized.

Member

jglick commented May 9, 2016

@kohsuke suggested using CodeSource.

@abayer

This comment has been minimized.

Member

abayer commented May 9, 2016

How would that be done?

@jglick

This comment has been minimized.

Member

jglick commented May 10, 2016

Well, if I knew, I would have done it already! :-)

@abayer

This comment has been minimized.

Member

abayer commented May 10, 2016

@jglick

This comment has been minimized.

Member

jglick commented Jun 15, 2016

@abayer or anyone else care to give me a bee? The scaffolding of the test is useful to have in place even without a fix.

@abayer

This comment has been minimized.

Member

abayer commented Jun 15, 2016

🐝!

@jglick jglick merged commit d516587 into jenkinsci:master Jun 15, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:RUN_SCRIPTS-JENKINS-34650 branch Jun 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment