[SECURITY-2450]

pom.xml

@@ -101,6 +101,7 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>
             <artifactId>script-security</artifactId>
+            <version>1172.v35f6a_0b_8207e</version> <!-- TODO: Remove once this version is included in BOM. -->
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>
@@ -148,6 +149,7 @@
             <groupId>org.jenkins-ci.plugins.workflow</groupId>
             <artifactId>workflow-job</artifactId>
             <scope>test</scope>
+            <version>1181.va_25d15548158</version> <!-- TODO: Remove once this version is included in BOM. -->
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins.workflow</groupId>

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition.java

@@ -24,6 +24,7 @@
 
 package org.jenkinsci.plugins.workflow.cps;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
 import hudson.Extension;
 import hudson.model.Action;
 import hudson.model.Item;
@@ -33,6 +34,8 @@
 import hudson.model.TaskListener;
 import hudson.util.FormValidation;
 import hudson.util.StreamTaskListener;
+import net.sf.json.JSONObject;
+import org.apache.commons.lang.StringUtils;
 import org.jenkinsci.plugins.workflow.cps.persistence.PersistIn;
 import org.jenkinsci.plugins.workflow.flow.DurabilityHintProvider;
 import org.jenkinsci.plugins.workflow.flow.FlowDefinition;
@@ -80,7 +83,8 @@ public CpsFlowDefinition(String script) {
     @DataBoundConstructor
     public CpsFlowDefinition(String script, boolean sandbox) {
         StaplerRequest req = Stapler.getCurrentRequest();
-        this.script = sandbox ? script : ScriptApproval.get().configuring(script, GroovyLanguage.get(), ApprovalContext.create().withCurrentUser().withItemAsKey(req != null ? req.findAncestorObject(Item.class) : null));
+        this.script = sandbox ? script : ScriptApproval.get().configuring(script, GroovyLanguage.get(),
+                ApprovalContext.create().withCurrentUser().withItemAsKey(req != null ? req.findAncestorObject(Item.class) : null), req == null);
         this.sandbox = sandbox;
     }
 
@@ -123,14 +127,41 @@ public CpsFlowExecution create(FlowExecutionOwner owner, TaskListener listener,
     @Extension
     public static class DescriptorImpl extends FlowDefinitionDescriptor {
 
+        /* In order to fix SECURITY-2450 without causing significant UX regressions, we decided to continue to
+         * automatically approve scripts on save if the script was modified by an administrator. To make this possible,
+         * we added a new hidden input field to the config.jelly to track the pre-save version of the script. Since
+         * CpsFlowDefinition calls ScriptApproval.configuring in its @DataBoundConstructor, the normal way to handle
+         * things would be to add an oldScript parameter to the constructor and perform the relevant logic there.
+         *
+         * However, that would have compatibility implications for tools like JobDSL, since @DataBoundConstructor
+         * parameters are required. We cannot use a @DataBoundSetter with a corresponding field and getter to trivially
+         * make oldScript optional, because we would need to call ScriptApproval.configuring after all
+         * @DataBoundSetters have been invoked (rather than in the @DataBoundConstructor), which is why we use Descriptor.newInstance.
+         */
+        @Override
+        public FlowDefinition newInstance(@NonNull StaplerRequest req, @NonNull JSONObject formData) throws FormException {
+            CpsFlowDefinition cpsFlowDefinition = (CpsFlowDefinition) super.newInstance(req, formData);
+            if (!cpsFlowDefinition.sandbox && formData.get("oldScript") != null) {
+                String oldScript = formData.getString("oldScript");
+                boolean approveIfAdmin = !StringUtils.equals(oldScript, cpsFlowDefinition.script);
+                if (approveIfAdmin) {
+                    ScriptApproval.get().configuring(cpsFlowDefinition.script, GroovyLanguage.get(),
+                            ApprovalContext.create().withCurrentUser().withItemAsKey(req.findAncestorObject(Item.class)), true);
+                }
+            }
+            return cpsFlowDefinition;
+        }
+
         @Override
         public String getDisplayName() {
             return "Pipeline script";
         }
 
         @RequirePOST
-        public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter boolean sandbox) {
-            return sandbox ? FormValidation.ok() : ScriptApproval.get().checking(value, GroovyLanguage.get());
+        public FormValidation doCheckScript(@QueryParameter String value, @QueryParameter String oldScript,
+                                            @QueryParameter boolean sandbox) {
+            return sandbox ? FormValidation.ok() :
+                    ScriptApproval.get().checking(value, GroovyLanguage.get(), !StringUtils.equals(oldScript, value));
         }
 
         @RequirePOST

src/main/resources/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition/config.jelly

@@ -24,6 +24,7 @@
   -->
 
 <j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:st="jelly:stapler" xmlns:wfe="/org/jenkinsci/plugins/workflow/editor">
+  <input type="hidden" name="oldScript" value="${instance.script}"/>
   <f:entry title="${%Script}" field="script">
     <wfe:workflow-editor />
   </f:entry>

src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinition2Test.java

@@ -25,19 +25,29 @@
 package org.jenkinsci.plugins.workflow.cps;
 
 import com.cloudbees.groovy.cps.CpsTransformer;
+import com.gargoylesoftware.htmlunit.html.HtmlCheckBoxInput;
+import com.gargoylesoftware.htmlunit.html.HtmlForm;
+import com.gargoylesoftware.htmlunit.html.HtmlInput;
+import com.gargoylesoftware.htmlunit.html.HtmlTextArea;
 import hudson.Functions;
 import hudson.model.Computer;
 import hudson.model.Describable;
 import hudson.model.Executor;
+import hudson.model.Item;
 import hudson.model.Result;
 import java.io.Serializable;
 import java.util.Collections;
+import java.util.List;
 import java.util.Set;
 
 import java.util.logging.Level;
+
+import hudson.security.Permission;
 import jenkins.model.Jenkins;
 
 import org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException;
+import org.jenkinsci.plugins.scriptsecurity.scripts.ScriptApproval;
+import org.jenkinsci.plugins.scriptsecurity.scripts.languages.GroovyLanguage;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
 import org.jenkinsci.plugins.workflow.steps.Step;
@@ -58,13 +68,16 @@
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.LoggerRule;
+import org.jvnet.hudson.test.MockAuthorizationStrategy;
 import org.jvnet.hudson.test.TestExtension;
 import org.kohsuke.stapler.DataBoundConstructor;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 public class CpsFlowDefinition2Test {
@@ -89,7 +102,7 @@ public void endlessRecursion() throws Exception {
         WorkflowRun r = jenkins.assertBuildStatus(Result.FAILURE, job.scheduleBuild2(0).get());
         jenkins.assertLogContains("look for unbounded recursion", r);
 
-        Assert.assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty());
+        assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty());
 
         for (Computer c : jenkins.jenkins.getComputers()) {
             for (Executor ex : c.getExecutors()) {
@@ -117,7 +130,7 @@ public void endlessRecursionNonCPS() throws Exception {
         // Should have failed with error about excessive recursion depth
         WorkflowRun r = jenkins.assertBuildStatus(Result.FAILURE, job.scheduleBuild2(0).get());
 
-        Assert.assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty());
+        assertTrue("No queued FlyWeightTask for job should remain after failure", jenkins.jenkins.getQueue().isEmpty());
 
         for (Computer c : jenkins.jenkins.getComputers()) {
             for (Executor ex : c.getExecutors()) {
@@ -852,6 +865,141 @@ public void scriptInitializerCallsCpsTransformedMethod() throws Exception {
         assertNull(Jenkins.get().getDescription());
     }
 
+    @Issue("SECURITY-2450")
+    @Test
+    public void cpsScriptNonAdminConfiguration() throws Exception {
+        jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm());
+
+        MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
+        mockStrategy.grant(Jenkins.READ).everywhere().to("devel");
+        for (Permission p : Item.PERMISSIONS.getPermissions()) {
+            mockStrategy.grant(p).everywhere().to("devel");
+        }
+        jenkins.jenkins.setAuthorizationStrategy(mockStrategy);
+
+        JenkinsRule.WebClient wcDevel = jenkins.createWebClient();
+        wcDevel.login("devel");
+
+        WorkflowJob p = jenkins.createProject(WorkflowJob.class);
+
+        HtmlForm config = wcDevel.getPage(p, "configure").getFormByName("config");
+        List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
+        // Get the last one, because previous ones might be from Lockable Resources during PCT.
+        HtmlTextArea script = scripts.get(scripts.size() - 1);
+        String groovy = "echo 'hi from cpsScriptNonAdminConfiguration'";
+        script.setText(groovy);
+
+        List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+        // Get the last one, because previous ones might be from Lockable Resources during PCT.
+        HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+        assertTrue(sandbox.isChecked());
+        sandbox.setChecked(false);
+
+        jenkins.submit(config);
+
+        assertEquals(1, ScriptApproval.get().getPendingScripts().size());
+        assertFalse(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get()));
+    }
+
+    @Issue("SECURITY-2450")
+    @Test
+    public void cpsScriptAdminConfiguration() throws Exception {
+        jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm());
+
+        MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
+        mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin");
+        for (Permission p : Item.PERMISSIONS.getPermissions()) {
+            mockStrategy.grant(p).everywhere().to("admin");
+        }
+        jenkins.jenkins.setAuthorizationStrategy(mockStrategy);
+
+        JenkinsRule.WebClient admin = jenkins.createWebClient();
+        admin.login("admin");
+
+        WorkflowJob p = jenkins.createProject(WorkflowJob.class);
+
+        HtmlForm config = admin.getPage(p, "configure").getFormByName("config");
+        List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
+        // Get the last one, because previous ones might be from Lockable Resources during PCT.
+        HtmlTextArea script = scripts.get(scripts.size() - 1);
+        String groovy = "echo 'hi from cpsScriptAdminConfiguration'";
+        script.setText(groovy);
+
+        List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+        // Get the last one, because previous ones might be from Lockable Resources during PCT.
+        HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+        assertTrue(sandbox.isChecked());
+        sandbox.setChecked(false);
+
+        jenkins.submit(config);
+
+        assertTrue(ScriptApproval.get().isScriptApproved(groovy, GroovyLanguage.get()));
+    }
+
+    @Issue("SECURITY-2450")
+    @Test
+    public void cpsScriptAdminModification() throws Exception {
+        jenkins.jenkins.setSecurityRealm(jenkins.createDummySecurityRealm());
+
+        MockAuthorizationStrategy mockStrategy = new MockAuthorizationStrategy();
+        mockStrategy.grant(Jenkins.READ).everywhere().to("devel");
+        mockStrategy.grant(Jenkins.ADMINISTER).everywhere().to("admin");
+        for (Permission p : Item.PERMISSIONS.getPermissions()) {
+            mockStrategy.grant(p).everywhere().to("devel");
+            mockStrategy.grant(p).everywhere().to("admin");
+        }
+        jenkins.jenkins.setAuthorizationStrategy(mockStrategy);
+
+        JenkinsRule.WebClient wc = jenkins.createWebClient();
+        wc.login("devel");
+
+        WorkflowJob p = jenkins.createProject(WorkflowJob.class);
+        String userGroovy = "echo 'hi from devel'";
+        String adminGroovy = "echo 'hi from admin'";
+
+        // initial configuration by user, script ends up in pending
+        {
+            HtmlForm config = wc.getPage(p, "configure").getFormByName("config");
+            List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
+            // Get the last one, because previous ones might be from Lockable Resources during PCT.
+            HtmlTextArea script = scripts.get(scripts.size() - 1);
+            script.setText(userGroovy);
+
+            List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+            // Get the last one, because previous ones might be from Lockable Resources during PCT.
+            HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+            assertTrue(sandbox.isChecked());
+            sandbox.setChecked(false);
+
+            jenkins.submit(config);
+
+            assertFalse(ScriptApproval.get().isScriptApproved(userGroovy, GroovyLanguage.get()));
+        }
+
+        wc.login("admin");
+
+        // modification by admin, script gets approved automatically
+        {
+            HtmlForm config = wc.getPage(p, "configure").getFormByName("config");
+            List<HtmlTextArea> scripts = config.getTextAreasByName("_.script");
+            // Get the last one, because previous ones might be from Lockable Resources during PCT.
+            HtmlTextArea script = scripts.get(scripts.size() - 1);
+            script.setText(adminGroovy);
+
+            List<HtmlInput> sandboxes = config.getInputsByName("_.sandbox");
+            // Get the last one, because previous ones might be from Lockable Resources during PCT.
+            HtmlCheckBoxInput sandbox = (HtmlCheckBoxInput) sandboxes.get(sandboxes.size() - 1);
+            assertFalse(sandbox.isChecked());
+
+            jenkins.submit(config);
+
+            // script content was modified by admin, so it should be approved upon save
+            // the one that had been submitted by the user previously stays in pending
+            assertTrue(ScriptApproval.get().isScriptApproved(adminGroovy, GroovyLanguage.get()));
+            assertFalse(ScriptApproval.get().isScriptApproved(userGroovy, GroovyLanguage.get()));
+        }
+    }
+
     public static class UnsafeParameterStep extends Step implements Serializable {
         private final UnsafeDescribable val;
         @DataBoundConstructor

src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowDefinitionRJRTest.java

@@ -0,0 +1,24 @@
+package org.jenkinsci.plugins.workflow.cps;
+
+import org.jenkinsci.plugins.workflow.job.WorkflowJob;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.RealJenkinsRule;
+
+public class CpsFlowDefinitionRJRTest {
+
+    @Rule
+    public RealJenkinsRule rjr = new RealJenkinsRule();
+
+    @Test
+    public void smokes() throws Throwable {
+        rjr.then(CpsFlowDefinitionRJRTest::doesItSmoke);
+    }
+
+    private static void doesItSmoke(JenkinsRule r) throws Exception {
+        WorkflowJob p = r.createProject(WorkflowJob.class, "p");
+        p.setDefinition(new CpsFlowDefinition("print Jenkins.get().getRootDir().toString()", false));
+        r.assertBuildStatusSuccess(p.scheduleBuild2(0));
+    }
+}