Revert "Recover gracefully when a PlaceholderTask is in the queue b…

…ut the associated build is complete (jenkinsci#185)"

This reverts commit 9c8d2f4.

src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepExecution.java

@@ -423,20 +423,6 @@ public String getCookie() {
         }
 
         @Override public CauseOfBlockage getCauseOfBlockage() {
-            Run<?, ?> run = runForDisplay();
-            if (!stopping && run != null && !run.isLogUpdated()) {
-                stopping = true;
-                LOGGER.warning(() -> "Refusing to build " + this + " and cancelling it because associated build is complete");
-                Timer.get().execute(() -> Queue.getInstance().cancel(this));
-            }
-            if (stopping) {
-                return new CauseOfBlockage() {
-                    @Override
-                    public String getShortDescription() {
-                        return "Stopping " + getDisplayName();
-                    }
-                };
-            }
             return null;
         }
 

src/test/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepTest.java

@@ -33,7 +33,6 @@
 import hudson.model.Executor;
 import hudson.model.Item;
 import hudson.model.Job;
-import hudson.model.Label;
 import hudson.model.Node;
 import hudson.model.Queue;
 import hudson.model.Result;
@@ -75,7 +74,8 @@
 import java.util.stream.Collectors;
 
 import hudson.util.VersionNumber;
-import java.nio.file.StandardCopyOption;
+import java.nio.charset.StandardCharsets;
+import java.util.Set;
 import jenkins.model.Jenkins;
 import jenkins.security.QueueItemAuthenticator;
 import jenkins.security.QueueItemAuthenticatorConfiguration;
@@ -1221,43 +1221,90 @@ public void getOwnerTaskPermissions() throws Throwable {
         });
     }
 
-    @Test public void placeholderTaskInQueueButAssociatedBuildComplete() throws Throwable {
-        logging.record(ExecutorStepExecution.class, Level.FINE).capture(50);
-        Path tempQueueFile = tmp.newFile().toPath();
+    @Issue("SECURITY-2428")
+    @Test
+    public void accessPermittedOnlyFromCurrentBuild() throws Throwable {
         sessions.then(r -> {
-            WorkflowJob p = r.createProject(WorkflowJob.class, "p");
-            p.setDefinition(new CpsFlowDefinition("node('custom-label') { }", true));
-            WorkflowRun b = p.scheduleBuild2(0).waitForStart();
-            // Get into a state where a PlaceholderTask is in the queue.
-            while (true) {
-                Queue.Item[] items = Queue.getInstance().getItems();
-                if (items.length == 1 && items[0].task instanceof ExecutorStepExecution.PlaceholderTask) {
-                    break;
+            // Adapted from RunningBuildFilePathFilterTest
+            ExtensionList.lookupSingleton(AdminWhitelistRule.class).setMasterKillSwitch(false);
+            WorkflowJob main = r.createProject(WorkflowJob.class, "main");
+            DumbSlave s = r.createOnlineSlave();
+            main.setDefinition(new CpsFlowDefinition("node('" + s.getNodeName() + "') {writeBack()}", true));
+            // Normal case: writing to our own build directory
+            WriteBackStep.controllerFile = new File(main.getBuildDir(), "1/stuff.txt");
+            r.buildAndAssertSuccess(main);
+            // Attacks:
+            WriteBackStep.legal = false;
+            // Writing to someone else’s build directory (covered by RunningBuildFilePathFilter)
+            FreeStyleProject other = r.createFreeStyleProject("other");
+            r.buildAndAssertSuccess(other);
+            WriteBackStep.controllerFile = new File(other.getBuildByNumber(1).getRootDir(), "hack");
+            r.buildAndAssertSuccess(main);
+            // Writing to some other directory (covered by AdminWhitelistRule)
+            WriteBackStep.controllerFile = new File(r.jenkins.getRootDir(), "hack");
+            r.buildAndAssertSuccess(main);
+            // Writing to a sensitive file even in my own build dir (covered by AdminWhitelistRule)
+            WriteBackStep.controllerFile = new File(main.getBuildDir(), "4/build.xml");
+            r.buildAndAssertSuccess(main);
+            // Writing to the directory of an earlier build
+            WriteBackStep.controllerFile = new File(main.getBuildByNumber(1).getRootDir(), "stuff.txt");
+            r.buildAndAssertSuccess(main);
+        });
+    }
+    public static final class WriteBackStep extends Step {
+        static File controllerFile;
+        static boolean legal = true;
+        @DataBoundConstructor public WriteBackStep() {}
+        @Override public StepExecution start(StepContext context) throws Exception {
+            return StepExecutions.synchronous(context, c -> {
+                Run<?, ?> build = c.get(Run.class);
+                TaskListener listener = c.get(TaskListener.class);
+                hudson.Launcher launcher = c.get(hudson.Launcher.class);
+                listener.getLogger().println("Will try to write to " + controllerFile + "; legal? " + legal);
+                String text = build.getExternalizableId();
+                try {
+                    launcher.getChannel().call(new WriteBackCallable(new FilePath(controllerFile), text));
+                    if (legal) {
+                        assertEquals(text, FileUtils.readFileToString(controllerFile, StandardCharsets.UTF_8));
+                        listener.getLogger().println("Allowed as expected");
+                    } else {
+                        fail("should not have been allowed");
+                    }
+                } catch (Exception x) {
+                    if (!legal && x.toString().contains("https://www.jenkins.io/redirect/security-144")) {
+                        Functions.printStackTrace(x, listener.error("Rejected as expected!"));
+                    } else {
+                        throw x;
+                    }
                 }
-                Thread.sleep(500L);
+                return null;
+            });
+        }
+        @TestExtension("accessPermittedOnlyFromCurrentBuild") public static final class DescriptorImpl extends StepDescriptor {
+            @Override public String getFunctionName() {
+                return "writeBack";
             }
-            // Copy queue.xml to a temp file while the PlaceholderTask is in the queue.
-            r.jenkins.getQueue().save();
-            Files.copy(sessions.getHome().toPath().resolve("queue.xml"), tempQueueFile, StandardCopyOption.REPLACE_EXISTING);
-            // Create a node with the correct label and let the build complete.
-            DumbSlave node = r.createOnlineSlave(Label.get("custom-label"));
-            r.assertBuildStatusSuccess(r.waitForCompletion(b));
-            // Remove node so that tasks requiring custom-label are stuck in the queue.
-            Jenkins.get().removeNode(node);
-        });
-        // Copy the temp queue.xml over the real one. The associated build has already completed, so the queue now
-        // has a bogus PlaceholderTask.
-        Files.copy(tempQueueFile, sessions.getHome().toPath().resolve("queue.xml"), StandardCopyOption.REPLACE_EXISTING);
-        sessions.then(r -> {
-            WorkflowJob p = r.jenkins.getItemByFullName("p", WorkflowJob.class);
-            WorkflowRun b = p.getBuildByNumber(1);
-            assertFalse(b.isLogUpdated());
-            r.assertBuildStatusSuccess(b);
-            while (Queue.getInstance().getItems().length > 0) {
-                Thread.sleep(100L);
+            @Override public Set<? extends Class<?>> getRequiredContext() {
+                return ImmutableSet.of(Run.class, TaskListener.class, hudson.Launcher.class);
             }
-            assertThat(logging.getMessages(), hasItem(startsWith("Refusing to build ExecutorStepExecution.PlaceholderTask{runId=p#")));
-        });
+        }
+        private static final class WriteBackCallable extends MasterToSlaveCallable<Void, IOException> {
+            private final FilePath controllerFile;
+            private final String text;
+            WriteBackCallable(FilePath controllerFile, String text) {
+                this.controllerFile = controllerFile;
+                this.text = text;
+            }
+            @Override public Void call() throws IOException {
+                assertTrue(controllerFile.isRemote());
+                try {
+                    controllerFile.write(text, null);
+                } catch (InterruptedException x) {
+                    throw new IOException(x);
+                }
+                return null;
+            }
+        }
     }
 
     private static class MainAuthenticator extends QueueItemAuthenticator {