From 41125f6d28a4ea6ce79f5e3e6014c84e91957ee4 Mon Sep 17 00:00:00 2001 From: Jerome Lacoste Date: Mon, 30 Oct 2023 16:07:08 +0100 Subject: [PATCH 1/8] Fix SECURITY-1025 --- src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java b/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java index 40a2055..d01e27a 100644 --- a/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java +++ b/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java @@ -27,6 +27,7 @@ import org.kohsuke.stapler.AncestorInPath; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.QueryParameter; +import org.kohsuke.stapler.verb.POST; import java.io.IOException; import java.io.PrintStream; @@ -147,6 +148,7 @@ public ListBoxModel doFillTaskItems(@QueryParameter String project, @AncestorInP } @Restricted(NoExternalUse.class) + @POST public FormValidation doCheckProject(@QueryParameter String project) { if (project.startsWith("/")) { return FormValidation.warning(Messages.BatchTaskInvoker_ForwardSlash()); @@ -162,6 +164,7 @@ public FormValidation doCheckProject(@QueryParameter String project) { } @Restricted(NoExternalUse.class) + @POST public FormValidation doCheckTask(@QueryParameter String project, @QueryParameter String task) { if (!project.isEmpty() && task.isEmpty()) { return FormValidation.warning(Messages.BatchTaskInvoker_NoBatchTaskExists(task)); From 0e461e230b40d8eeae6e2dfbf00ba7b461ddb0a9 Mon Sep 17 00:00:00 2001 From: Jerome Lacoste Date: Tue, 31 Oct 2023 13:27:02 +0100 Subject: [PATCH 2/8] SECURITY-1025 protect more paths --- src/main/java/hudson/plugins/batch_task/BatchRun.java | 2 ++ src/main/java/hudson/plugins/batch_task/BatchTask.java | 4 ++++ src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java | 1 + 3 files changed, 7 insertions(+) diff --git a/src/main/java/hudson/plugins/batch_task/BatchRun.java b/src/main/java/hudson/plugins/batch_task/BatchRun.java index d09c828..4657c9c 100644 --- a/src/main/java/hudson/plugins/batch_task/BatchRun.java +++ b/src/main/java/hudson/plugins/batch_task/BatchRun.java @@ -16,6 +16,7 @@ import org.kohsuke.stapler.StaplerResponse; import org.kohsuke.stapler.export.Exported; import org.kohsuke.stapler.framework.io.LargeText; +import org.kohsuke.stapler.verb.POST; import java.io.File; import java.io.FileOutputStream; @@ -326,6 +327,7 @@ public String getUrlName() { /** * Handles incremental log output. */ + @POST public void doProgressiveLog(StaplerRequest req, StaplerResponse rsp) throws IOException { new LargeText(getLogFile(), !isRunning()).doProgressText(req, rsp); } diff --git a/src/main/java/hudson/plugins/batch_task/BatchTask.java b/src/main/java/hudson/plugins/batch_task/BatchTask.java index 7e87d9a..ee443b3 100644 --- a/src/main/java/hudson/plugins/batch_task/BatchTask.java +++ b/src/main/java/hudson/plugins/batch_task/BatchTask.java @@ -39,6 +39,7 @@ import java.util.regex.Pattern; import com.thoughtworks.xstream.converters.basic.AbstractSingleValueConverter; +import org.kohsuke.stapler.verb.POST; /** * A batch task. @@ -264,6 +265,7 @@ public Object getDynamic(String token, StaplerRequest req, StaplerResponse rsp) /** * Schedules the execution */ + @POST public synchronized void doExecute( StaplerRequest req, StaplerResponse rsp ) throws IOException, ServletException { getACL().checkPermission(AbstractProject.BUILD); @@ -278,6 +280,7 @@ public synchronized void doExecute( StaplerRequest req, StaplerResponse rsp ) th /** * Deletes this task. */ + @POST public synchronized void doDoDelete(StaplerResponse rsp) throws IOException, ServletException { getACL().checkPermission(AbstractProject.DELETE); @@ -319,6 +322,7 @@ private int[] parse(String num) { private static final Pattern BUILD_NUMBER_PATTERN = Pattern.compile("(\\d+)-(\\d+)"); + @POST public void doCancelQueue(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException { checkAbortPermission(); diff --git a/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java b/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java index d01e27a..7ec2b6a 100644 --- a/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java +++ b/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java @@ -132,6 +132,7 @@ public String getDisplayName() { return ""; } + @POST public ListBoxModel doFillTaskItems(@QueryParameter String project, @AncestorInPath AbstractProject context) { // when the item is not found, the user should be getting an error from elsewhere. ListBoxModel r = new ListBoxModel(); From ffe48df5eacea6e87baf337175dd446f8ea2fe7a Mon Sep 17 00:00:00 2001 From: Jerome Lacoste Date: Tue, 31 Oct 2023 16:23:30 +0100 Subject: [PATCH 3/8] Adjust tests now that we have enforced security --- .../plugins/batch_task/BatchTaskTest.java | 7 +-- .../hudson/plugins/batch_task/TestHelper.java | 46 +++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 src/test/java/hudson/plugins/batch_task/TestHelper.java diff --git a/src/test/java/hudson/plugins/batch_task/BatchTaskTest.java b/src/test/java/hudson/plugins/batch_task/BatchTaskTest.java index 73338b6..8eebb07 100644 --- a/src/test/java/hudson/plugins/batch_task/BatchTaskTest.java +++ b/src/test/java/hudson/plugins/batch_task/BatchTaskTest.java @@ -57,10 +57,11 @@ public class BatchTaskTest { */ @Test public void testNoBuilds() throws Exception { + r.jenkins.setCrumbIssuer(null); //Not really testing csrf right now FreeStyleProject p = r.createFreeStyleProject(); p.addProperty(new BatchTaskProperty(new BatchTask("test", "echo hello"))); JenkinsRule.WebClient wc = r.createWebClient(); - HtmlPage page = wc.getPage(p, "batchTasks/task/test/execute"); + HtmlPage page = TestHelper.post(wc, p.getUrl() + "batchTasks/task/test/execute", null, null); String path = page.getWebResponse().getWebRequest().getUrl().getPath(); assertTrue("should redirect to noBuilds page: " + path, path.endsWith("/noBuild")); } @@ -72,7 +73,7 @@ public void testNoBuilds() throws Exception { */ @Test public void testExecute() throws Exception { - + r.jenkins.setCrumbIssuer(null); //Not really testing csrf right now r.jenkins.getGlobalNodeProperties().add(new EnvironmentVariablesNodeProperty( new EnvironmentVariablesNodeProperty.Entry("GLOBAL", "global-property"), new EnvironmentVariablesNodeProperty.Entry("OVERRIDE_ME", "foo"))); @@ -93,7 +94,7 @@ public void testExecute() throws Exception { while (freeStyleBuild.isBuilding()) { Thread.sleep(100); } - r.createWebClient().getPage(p, "batchTasks/task/test/execute"); + TestHelper.post(r.createWebClient(), p.getUrl() + "batchTasks/task/test/execute", null, null); BatchRun run = task.getLastRun(); assertNotNull("task did not run", run); CauseAction ca = run.getAction(CauseAction.class); diff --git a/src/test/java/hudson/plugins/batch_task/TestHelper.java b/src/test/java/hudson/plugins/batch_task/TestHelper.java new file mode 100644 index 0000000..d66eb8d --- /dev/null +++ b/src/test/java/hudson/plugins/batch_task/TestHelper.java @@ -0,0 +1,46 @@ +package hudson.plugins.batch_task; + +import org.htmlunit.FailingHttpStatusCodeException; +import org.htmlunit.HttpMethod; +import org.htmlunit.WebRequest; +import org.htmlunit.html.HtmlPage; +import org.htmlunit.util.UrlUtils; +import org.jvnet.hudson.test.JenkinsRule; + +import java.io.IOException; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.junit.Assert.assertEquals; + +public class TestHelper { + /** + * Performs an HTTP POST request to the relative url. + * + * @param webClient the client + * @param relative the url relative to the context path + * @param expectedContentType if expecting specific content type or null if not + * @param expectedStatus if expecting a failing http status code or null if not + * @throws IOException if so + */ + public static HtmlPage post(JenkinsRule.WebClient webClient, String relative, + String expectedContentType, Integer expectedStatus) throws IOException { + WebRequest request = new WebRequest( + UrlUtils.toUrlUnsafe(webClient.getContextPath() + relative), + HttpMethod.POST); + try { + HtmlPage p = webClient.getPage(request); + if (expectedContentType != null) { + assertThat(p.getWebResponse().getContentType(), is(expectedContentType)); + } + return p; + } catch (FailingHttpStatusCodeException e) { + if (expectedStatus != null) { + assertEquals(expectedStatus.intValue(), e.getStatusCode()); + return null; + } else { + throw e; + } + } + } +} From 2f0bdaa0a41326f35644330774e0d269690e7060 Mon Sep 17 00:00:00 2001 From: Jerome Lacoste Date: Thu, 2 Nov 2023 10:30:05 +0100 Subject: [PATCH 4/8] Address code review, remove unecessary POST guards --- src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java b/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java index 7ec2b6a..40a2055 100644 --- a/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java +++ b/src/main/java/hudson/plugins/batch_task/BatchTaskInvoker.java @@ -27,7 +27,6 @@ import org.kohsuke.stapler.AncestorInPath; import org.kohsuke.stapler.DataBoundConstructor; import org.kohsuke.stapler.QueryParameter; -import org.kohsuke.stapler.verb.POST; import java.io.IOException; import java.io.PrintStream; @@ -132,7 +131,6 @@ public String getDisplayName() { return ""; } - @POST public ListBoxModel doFillTaskItems(@QueryParameter String project, @AncestorInPath AbstractProject context) { // when the item is not found, the user should be getting an error from elsewhere. ListBoxModel r = new ListBoxModel(); @@ -149,7 +147,6 @@ public ListBoxModel doFillTaskItems(@QueryParameter String project, @AncestorInP } @Restricted(NoExternalUse.class) - @POST public FormValidation doCheckProject(@QueryParameter String project) { if (project.startsWith("/")) { return FormValidation.warning(Messages.BatchTaskInvoker_ForwardSlash()); @@ -165,7 +162,6 @@ public FormValidation doCheckProject(@QueryParameter String project) { } @Restricted(NoExternalUse.class) - @POST public FormValidation doCheckTask(@QueryParameter String project, @QueryParameter String task) { if (!project.isEmpty() && task.isEmpty()) { return FormValidation.warning(Messages.BatchTaskInvoker_NoBatchTaskExists(task)); From 82070da3a05bffaf94d1e94def4b03e712697202 Mon Sep 17 00:00:00 2001 From: Jerome Lacoste Date: Thu, 2 Nov 2023 11:01:52 +0100 Subject: [PATCH 5/8] Address code review, use proper function to test with Crumb --- .../hudson/plugins/batch_task/BatchTaskTest.java | 6 ++---- .../java/hudson/plugins/batch_task/TestHelper.java | 14 +++++++------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/test/java/hudson/plugins/batch_task/BatchTaskTest.java b/src/test/java/hudson/plugins/batch_task/BatchTaskTest.java index 8eebb07..ea91b05 100644 --- a/src/test/java/hudson/plugins/batch_task/BatchTaskTest.java +++ b/src/test/java/hudson/plugins/batch_task/BatchTaskTest.java @@ -57,11 +57,10 @@ public class BatchTaskTest { */ @Test public void testNoBuilds() throws Exception { - r.jenkins.setCrumbIssuer(null); //Not really testing csrf right now FreeStyleProject p = r.createFreeStyleProject(); p.addProperty(new BatchTaskProperty(new BatchTask("test", "echo hello"))); JenkinsRule.WebClient wc = r.createWebClient(); - HtmlPage page = TestHelper.post(wc, p.getUrl() + "batchTasks/task/test/execute", null, null); + HtmlPage page = TestHelper.assertPost(wc, p.getUrl() + "batchTasks/task/test/execute", "text/html", 200); String path = page.getWebResponse().getWebRequest().getUrl().getPath(); assertTrue("should redirect to noBuilds page: " + path, path.endsWith("/noBuild")); } @@ -73,7 +72,6 @@ public void testNoBuilds() throws Exception { */ @Test public void testExecute() throws Exception { - r.jenkins.setCrumbIssuer(null); //Not really testing csrf right now r.jenkins.getGlobalNodeProperties().add(new EnvironmentVariablesNodeProperty( new EnvironmentVariablesNodeProperty.Entry("GLOBAL", "global-property"), new EnvironmentVariablesNodeProperty.Entry("OVERRIDE_ME", "foo"))); @@ -94,7 +92,7 @@ public void testExecute() throws Exception { while (freeStyleBuild.isBuilding()) { Thread.sleep(100); } - TestHelper.post(r.createWebClient(), p.getUrl() + "batchTasks/task/test/execute", null, null); + TestHelper.assertPost(r.createWebClient(), p.getUrl() + "batchTasks/task/test/execute", "text/html", 200); BatchRun run = task.getLastRun(); assertNotNull("task did not run", run); CauseAction ca = run.getAction(CauseAction.class); diff --git a/src/test/java/hudson/plugins/batch_task/TestHelper.java b/src/test/java/hudson/plugins/batch_task/TestHelper.java index d66eb8d..8632af6 100644 --- a/src/test/java/hudson/plugins/batch_task/TestHelper.java +++ b/src/test/java/hudson/plugins/batch_task/TestHelper.java @@ -4,7 +4,6 @@ import org.htmlunit.HttpMethod; import org.htmlunit.WebRequest; import org.htmlunit.html.HtmlPage; -import org.htmlunit.util.UrlUtils; import org.jvnet.hudson.test.JenkinsRule; import java.io.IOException; @@ -20,19 +19,20 @@ public class TestHelper { * @param webClient the client * @param relative the url relative to the context path * @param expectedContentType if expecting specific content type or null if not - * @param expectedStatus if expecting a failing http status code or null if not + * @param expectedStatus if expecting a http status code or null if not * @throws IOException if so */ - public static HtmlPage post(JenkinsRule.WebClient webClient, String relative, - String expectedContentType, Integer expectedStatus) throws IOException { - WebRequest request = new WebRequest( - UrlUtils.toUrlUnsafe(webClient.getContextPath() + relative), - HttpMethod.POST); + public static HtmlPage assertPost(JenkinsRule.WebClient webClient, String relative, + String expectedContentType, Integer expectedStatus) throws IOException { + WebRequest request = new WebRequest(webClient.createCrumbedUrl(relative), HttpMethod.POST); try { HtmlPage p = webClient.getPage(request); if (expectedContentType != null) { assertThat(p.getWebResponse().getContentType(), is(expectedContentType)); } + if (expectedStatus != null) { + assertEquals(expectedStatus.intValue(), p.getWebResponse().getStatusCode()); + } return p; } catch (FailingHttpStatusCodeException e) { if (expectedStatus != null) { From 87dd079e5dedbbb1b7475dee553bda3367ebdcb1 Mon Sep 17 00:00:00 2001 From: Jerome Lacoste Date: Thu, 2 Nov 2023 11:52:52 +0100 Subject: [PATCH 6/8] Address code review, fix get/post and add confirmation upon delete --- .../hudson/plugins/batch_task/BatchTask/sidepanel.jelly | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/resources/hudson/plugins/batch_task/BatchTask/sidepanel.jelly b/src/main/resources/hudson/plugins/batch_task/BatchTask/sidepanel.jelly index 86990e1..9c95424 100644 --- a/src/main/resources/hudson/plugins/batch_task/BatchTask/sidepanel.jelly +++ b/src/main/resources/hudson/plugins/batch_task/BatchTask/sidepanel.jelly @@ -15,11 +15,11 @@ - + - + - - - - - -
- ${%description} - - -
- - \ No newline at end of file From 047141aa296cd8f3d16b1fca6fa4f371a8293744 Mon Sep 17 00:00:00 2001 From: Jerome Lacoste Date: Thu, 2 Nov 2023 15:27:37 +0100 Subject: [PATCH 8/8] Remove uneeded POST guard --- src/main/java/hudson/plugins/batch_task/BatchRun.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/hudson/plugins/batch_task/BatchRun.java b/src/main/java/hudson/plugins/batch_task/BatchRun.java index 4657c9c..1c98291 100644 --- a/src/main/java/hudson/plugins/batch_task/BatchRun.java +++ b/src/main/java/hudson/plugins/batch_task/BatchRun.java @@ -327,7 +327,6 @@ public String getUrlName() { /** * Handles incremental log output. */ - @POST public void doProgressiveLog(StaplerRequest req, StaplerResponse rsp) throws IOException { new LargeText(getLogFile(), !isRunning()).doProgressText(req, rsp); }