From 4bf162006195eca43c50bbf362342193aedae079 Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Fri, 4 Oct 2019 20:58:25 +0200 Subject: [PATCH 1/9] [JENKINS-59656] when stopping a Run via the executors widget, make sure it's the intended one --- core/src/main/java/hudson/model/Executor.java | 25 ++++++++++++++++--- .../main/resources/lib/hudson/executors.jelly | 6 ++++- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index b0a6b4a45c63..c7ce72ff2381 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -39,6 +39,7 @@ import org.acegisecurity.Authentication; import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.HttpResponses; +import org.kohsuke.stapler.QueryParameter; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; import org.kohsuke.stapler.export.Exported; @@ -835,21 +836,39 @@ public void start() { @RequirePOST @Deprecated public void doStop( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException { - doStop().generateResponse(req,rsp,this); + doStop(req != null ? Util.fixEmpty(req.getParameter("what")) : null).generateResponse(req,rsp,this); } /** * Stops the current build. * * @since 1.489 + * @deprecated as of 2.??? + * Use {@link #doStop(String)}. */ @RequirePOST + @Deprecated public HttpResponse doStop() { + return doStop(null); + } + + /** + * Stops the current build. + * + * @param what + * if not null, the externalizable id ({@link Run#getExternalizableId()}) + * of the build the user expects to interrupt + * @since 2.??? + */ + @RequirePOST + public HttpResponse doStop(@CheckForNull @QueryParameter(fixEmpty = true) String what) { lock.writeLock().lock(); // need write lock as interrupt will change the field try { if (executable != null) { - getParentOf(executable).getOwnerTask().checkAbortPermission(); - interrupt(); + if (what == null || (executable instanceof Run && what.equals(((Run) executable).getExternalizableId()))) { + getParentOf(executable).getOwnerTask().checkAbortPermission(); + interrupt(); + } } } finally { lock.writeLock().unlock(); diff --git a/core/src/main/resources/lib/hudson/executors.jelly b/core/src/main/resources/lib/hudson/executors.jelly index 578808d47f1d..b769b919c9ce 100644 --- a/core/src/main/resources/lib/hudson/executors.jelly +++ b/core/src/main/resources/lib/hudson/executors.jelly @@ -110,7 +110,11 @@ THE SOFTWARE. - + + + + + From bd7858aed14d475a77071ca28e9b919d90840c0c Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Mon, 7 Oct 2019 00:08:20 +0200 Subject: [PATCH 2/9] [JENKINS-59656] added Executor.stopBuild(String) instead of .stop(String) --- core/src/main/java/hudson/model/Executor.java | 24 ++++++++++--------- .../main/resources/lib/hudson/executors.jelly | 2 +- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index c7ce72ff2381..592476183e39 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -831,41 +831,43 @@ public void start() { /** * @deprecated as of 1.489 - * Use {@link #doStop()}. + * Use {@link #doStop()} or {@link #doStopBuild(String)}. */ @RequirePOST @Deprecated public void doStop( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException { - doStop(req != null ? Util.fixEmpty(req.getParameter("what")) : null).generateResponse(req,rsp,this); + (req != null ? doStopBuild(req.getParameter("runExtId")) : doStop()).generateResponse(req,rsp,this); } /** - * Stops the current build. + * Stops the current build.
+ * You can use {@link #doStopBuild(String)} instead to ensure what will be + * interrupted is actually what you want to interrupt. * * @since 1.489 - * @deprecated as of 2.??? - * Use {@link #doStop(String)}. + * @see #doStopBuild(String) */ @RequirePOST - @Deprecated public HttpResponse doStop() { - return doStop(null); + return doStopBuild(null); } /** - * Stops the current build. + * Stops the current build, if matching the specified external id + * (or no id is specified, or the current {@link Executable} is not a {@link Run}). * - * @param what + * @param runExtId * if not null, the externalizable id ({@link Run#getExternalizableId()}) * of the build the user expects to interrupt * @since 2.??? */ @RequirePOST - public HttpResponse doStop(@CheckForNull @QueryParameter(fixEmpty = true) String what) { + public HttpResponse doStopBuild(@CheckForNull @QueryParameter(fixEmpty = true) String runExtId) { lock.writeLock().lock(); // need write lock as interrupt will change the field try { if (executable != null) { - if (what == null || (executable instanceof Run && what.equals(((Run) executable).getExternalizableId()))) { + if (runExtId == null || runExtId.isEmpty() + || (executable instanceof Run && runExtId.equals(((Run) executable).getExternalizableId()))) { getParentOf(executable).getOwnerTask().checkAbortPermission(); interrupt(); } diff --git a/core/src/main/resources/lib/hudson/executors.jelly b/core/src/main/resources/lib/hudson/executors.jelly index b769b919c9ce..5e74de4f5126 100644 --- a/core/src/main/resources/lib/hudson/executors.jelly +++ b/core/src/main/resources/lib/hudson/executors.jelly @@ -114,7 +114,7 @@ THE SOFTWARE. - + From e2d2e4d99ca6d71d00ebc647c21ce50347c47b2e Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Wed, 9 Oct 2019 23:22:22 +0200 Subject: [PATCH 3/9] revert change to deprecated doStop(StaplerRequest,StaplerResponse), useless now that it's doStopBuild(String) which takes a runExtId parameter --- core/src/main/java/hudson/model/Executor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index 592476183e39..0c5e811ee251 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -836,7 +836,7 @@ public void start() { @RequirePOST @Deprecated public void doStop( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException { - (req != null ? doStopBuild(req.getParameter("runExtId")) : doStop()).generateResponse(req,rsp,this); + doStop().generateResponse(req,rsp,this); } /** From ac74aec74b554c9f28008d7dbc3e871438cd2135 Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Wed, 9 Oct 2019 23:31:59 +0200 Subject: [PATCH 4/9] javadoc: "@since TODO" rather than "@since 2.???" --- core/src/main/java/hudson/model/Executor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index 0c5e811ee251..05f050231129 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -859,7 +859,7 @@ public HttpResponse doStop() { * @param runExtId * if not null, the externalizable id ({@link Run#getExternalizableId()}) * of the build the user expects to interrupt - * @since 2.??? + * @since TODO */ @RequirePOST public HttpResponse doStopBuild(@CheckForNull @QueryParameter(fixEmpty = true) String runExtId) { From 993011ef62d2d57f70dd9788c01f35329973ec85 Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Fri, 11 Oct 2019 10:26:08 +0200 Subject: [PATCH 5/9] jelly cleanup with h.urlEncode(String) --- core/src/main/resources/lib/hudson/executors.jelly | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/core/src/main/resources/lib/hudson/executors.jelly b/core/src/main/resources/lib/hudson/executors.jelly index 5e74de4f5126..bf2d99a6be6a 100644 --- a/core/src/main/resources/lib/hudson/executors.jelly +++ b/core/src/main/resources/lib/hudson/executors.jelly @@ -110,11 +110,7 @@ THE SOFTWARE. - - - - - + From d72e0c35a48cbe22eb01628c361b6e7330781be7 Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Fri, 11 Oct 2019 10:22:10 +0200 Subject: [PATCH 6/9] [JENKINS-59656] added test case --- .../security/stapler/Security400Test.java | 110 +++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) diff --git a/test/src/test/java/jenkins/security/stapler/Security400Test.java b/test/src/test/java/jenkins/security/stapler/Security400Test.java index afb4db75aaaf..78382f933f4f 100644 --- a/test/src/test/java/jenkins/security/stapler/Security400Test.java +++ b/test/src/test/java/jenkins/security/stapler/Security400Test.java @@ -55,6 +55,7 @@ import java.io.IOException; import java.net.HttpURLConnection; import java.net.URL; +import java.net.URLEncoder; import java.util.concurrent.Semaphore; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -261,7 +262,114 @@ public void ensureDoStopStillReachable() throws Exception { assertEquals(3, atomicResult.get()); } } - + + /* + * Similar to ensureDoStopStillReachable()}, but tests Executor.doStopBuild(String) instead of Executor.doStop(). + * Implemented here (instead of ExecutorTest) for convenience (uses SemaphoredBuilder). + */ + @Test + @Issue("JENKINS-59656") + public void ensureDoStopBuildWorks() throws Exception { + j.jenkins.setCrumbIssuer(null); + JenkinsRule.WebClient wc = j.createWebClient(); + wc.getOptions().setThrowExceptionOnFailingStatusCode(false); // we expect 404 + wc.getOptions().setPrintContentOnFailingStatusCode(false); // be less verbose + + // gives access to the build result code + final AtomicInteger atomicResult = new AtomicInteger(0); + // blocks execution of the build step + final Semaphore semaphore = new Semaphore(0); + // the test project with a semaphored build step + FreeStyleProject p = j.createFreeStyleProject(); + p.getBuildersList().add(new SemaphoredBuilder(semaphore, atomicResult)); + + // to be sure to reach the correct one + j.jenkins.setNumExecutors(1); + + { // preliminary test, calling stopBuild without any executor results in 404 + WebRequest request = new WebRequest(new URL(j.getURL() + "computers/0/executors/0/stopBuild"), HttpMethod.POST); + Page page = wc.getPage(request); + assertEquals(404, page.getWebResponse().getStatusCode()); + assertRequestWasNotBlocked(); + } + + { // first try, we let the build finishes normally + // reset semaphore and result code + semaphore.drainPermits(); + atomicResult.set(0); + + QueueTaskFuture futureBuild = p.scheduleBuild2(0); + futureBuild.waitForStart(); + + // let the build finishes + semaphore.release(1); + + j.assertBuildStatus(Result.SUCCESS, futureBuild); + assertEquals(1, atomicResult.get()); + } + + { // second try, calling stopBuild without parameter interrupts the build (same as calling stop) + // reset semaphore and result code + semaphore.drainPermits(); + atomicResult.set(0); + + QueueTaskFuture futureBuild = p.scheduleBuild2(0); + futureBuild.waitForStart(); + + WebRequest request = new WebRequest(new URL(j.getURL() + "computers/0/executors/0/stopBuild"), HttpMethod.POST); + Page page = wc.getPage(request); + assertEquals(404, page.getWebResponse().getStatusCode()); + assertRequestWasNotBlocked(); + + // let the build finish quickly (if not interrupted already) + semaphore.release(1); + + j.assertBuildStatus(Result.FAILURE, futureBuild); + assertEquals(3, atomicResult.get()); // interrupted + } + + { // third try, calling stopBuild with the right parameter interrupts the build + // reset semaphore and result code + semaphore.drainPermits(); + atomicResult.set(0); + + QueueTaskFuture futureBuild = p.scheduleBuild2(0); + FreeStyleBuild build = futureBuild.waitForStart(); + String runExtId = URLEncoder.encode(build.getExternalizableId(), "UTF-8"); + + WebRequest request = new WebRequest(new URL(j.getURL() + "computers/0/executors/0/stopBuild?runExtId=" + runExtId), HttpMethod.POST); + Page page = wc.getPage(request); + assertEquals(404, page.getWebResponse().getStatusCode()); + assertRequestWasNotBlocked(); + + // let the build finish quickly (if not interrupted already) + semaphore.release(1); + + j.assertBuildStatus(Result.FAILURE, futureBuild); + assertEquals(3, atomicResult.get()); // interrupted + } + + { // fourth try, calling stopBuild with a parameter not matching build id doesn't interrupt the build + // reset semaphore and result code + semaphore.drainPermits(); + atomicResult.set(0); + + QueueTaskFuture futureBuild = p.scheduleBuild2(0); + futureBuild.waitForStart(); + + WebRequest request = new WebRequest(new URL(j.getURL() + "computers/0/executors/0/stopBuild?runExtId=whatever"), HttpMethod.POST); + Page page = wc.getPage(request); + assertEquals(404, page.getWebResponse().getStatusCode()); + assertRequestWasNotBlocked(); + + // let the build finishes + semaphore.release(1); + + j.assertBuildStatus(Result.SUCCESS, futureBuild); + assertEquals(1, atomicResult.get()); + } + } + @Test @Issue("SECURITY-404") public void anonCannotReadTextConsole() throws Exception { From db5d364eab255606174e4490eb6b0c78c56b944f Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Thu, 21 Nov 2019 21:31:59 +0100 Subject: [PATCH 7/9] make Functions.urlEncode(null) return empty String --- core/src/main/java/hudson/Functions.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/Functions.java b/core/src/main/java/hudson/Functions.java index 3902d952f541..1adda1c21d78 100644 --- a/core/src/main/java/hudson/Functions.java +++ b/core/src/main/java/hudson/Functions.java @@ -761,11 +761,15 @@ public static String encode(String s) { /** * Shortcut function for calling {@link URLEncoder#encode(String,String)} (with UTF-8 encoding).
- * Useful for encoding URL query parameters in jelly code (as in {@code "...?param=${h.urlEncode(something)}"}). + * Useful for encoding URL query parameters in jelly code (as in {@code "...?param=${h.urlEncode(something)}"}).
+ * For convenience in jelly code, it also accepts null parameter, and then returns an empty string. * * @since 2.200 */ public static String urlEncode(String s) { + if (s == null) { + return ""; + } try { return URLEncoder.encode(s, StandardCharsets.UTF_8.name()); } catch (UnsupportedEncodingException e) { From 275b6ca0c8416a6a06fb24a604c868b5a8da38c0 Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Thu, 21 Nov 2019 22:05:14 +0100 Subject: [PATCH 8/9] Executor.doStopBuild(runExtId): ignore runExtId if executable is not a Run --- core/src/main/java/hudson/model/Executor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index 60ad26345f7e..0987beefc010 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -864,7 +864,7 @@ public HttpResponse doStopBuild(@CheckForNull @QueryParameter(fixEmpty = true) S lock.writeLock().lock(); // need write lock as interrupt will change the field try { if (executable != null) { - if (runExtId == null || runExtId.isEmpty() + if (runExtId == null || runExtId.isEmpty() || ! (executable instanceof Run) || (executable instanceof Run && runExtId.equals(((Run) executable).getExternalizableId()))) { getParentOf(executable).getOwnerTask().checkAbortPermission(); interrupt(); From f01385e6a86d0a3116e993539c56f7a0694681d4 Mon Sep 17 00:00:00 2001 From: Thomas de Grenier de Latour Date: Wed, 4 Dec 2019 09:07:55 +0100 Subject: [PATCH 9/9] Executor.doStopBuild(runExtId) doesn't have to be a public API --- core/src/main/java/hudson/model/Executor.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/java/hudson/model/Executor.java b/core/src/main/java/hudson/model/Executor.java index 0987beefc010..e69fa2100b51 100644 --- a/core/src/main/java/hudson/model/Executor.java +++ b/core/src/main/java/hudson/model/Executor.java @@ -860,6 +860,7 @@ public HttpResponse doStop() { * @since TODO */ @RequirePOST + @Restricted(NoExternalUse.class) public HttpResponse doStopBuild(@CheckForNull @QueryParameter(fixEmpty = true) String runExtId) { lock.writeLock().lock(); // need write lock as interrupt will change the field try {