diff --git a/src/main/java/org/jenkinsci/plugins/configfiles/ConfigFilesManagement.java b/src/main/java/org/jenkinsci/plugins/configfiles/ConfigFilesManagement.java index dfd9e894..346de2fa 100644 --- a/src/main/java/org/jenkinsci/plugins/configfiles/ConfigFilesManagement.java +++ b/src/main/java/org/jenkinsci/plugins/configfiles/ConfigFilesManagement.java @@ -47,6 +47,7 @@ of this software and associated documentation files (the "Software"), to deal import org.kohsuke.stapler.StaplerProxy; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; +import org.kohsuke.stapler.interceptor.RequirePOST; import org.kohsuke.stapler.verb.POST; /** @@ -256,6 +257,7 @@ private void checkPermission(Permission permission) { * @return forward to 'index' * @throws IOException */ + @RequirePOST public HttpResponse doRemoveConfig(StaplerRequest res, StaplerResponse rsp, @QueryParameter("id") String configId) throws IOException { checkPermission(Jenkins.ADMINISTER); diff --git a/src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java b/src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java index 5092f371..b4b61128 100644 --- a/src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java +++ b/src/main/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileAction.java @@ -38,6 +38,7 @@ import jenkins.model.TransientActionFactory; import net.sf.json.JSONObject; +import org.kohsuke.stapler.interceptor.RequirePOST; import org.kohsuke.stapler.verb.POST; public class FolderConfigFileAction implements Action, ConfigFilesUIContract, StaplerProxy { @@ -225,6 +226,7 @@ public void doSelectProvider(StaplerRequest req, StaplerResponse rsp) throws IOE req.getView(this, JELLY_RESOURCES_PATH + "selectprovider.jelly").forward(req, rsp); } + @RequirePOST @Override public HttpResponse doRemoveConfig(StaplerRequest res, StaplerResponse rsp, @QueryParameter("id") String configId) throws IOException { checkPermission(Job.CONFIGURE); diff --git a/src/main/resources/lib/configfiles/configfiles.jelly b/src/main/resources/lib/configfiles/configfiles.jelly index 6ee59b74..e46b53e6 100644 --- a/src/main/resources/lib/configfiles/configfiles.jelly +++ b/src/main/resources/lib/configfiles/configfiles.jelly @@ -49,10 +49,10 @@ THE SOFTWARE. src="${imagesURL}/16x16/document_edit.gif"/> - + - + @@ -78,14 +78,4 @@ THE SOFTWARE. - diff --git a/src/main/resources/lib/configfiles/providerlist.jelly b/src/main/resources/lib/configfiles/providerlist.jelly index 0799d06f..d97f18eb 100644 --- a/src/main/resources/lib/configfiles/providerlist.jelly +++ b/src/main/resources/lib/configfiles/providerlist.jelly @@ -47,10 +47,10 @@ THE SOFTWARE. src="${imagesURL}/16x16/document_edit.gif" /> - + - + ${t.name} @@ -64,16 +64,4 @@ THE SOFTWARE. - - - diff --git a/src/test/java/org/jenkinsci/plugins/configfiles/ConfigFilesSEC1253Test.java b/src/test/java/org/jenkinsci/plugins/configfiles/ConfigFilesSEC1253Test.java index 9d31e9aa..6525f7e0 100644 --- a/src/test/java/org/jenkinsci/plugins/configfiles/ConfigFilesSEC1253Test.java +++ b/src/test/java/org/jenkinsci/plugins/configfiles/ConfigFilesSEC1253Test.java @@ -55,7 +55,7 @@ public void regularCaseStillWorking() throws Exception { assertThat(store.getConfigs(), hasSize(1)); HtmlPage configFiles = wc.goTo("configfiles"); - HtmlAnchor removeAnchor = configFiles.getDocumentElement().getOneHtmlElementByAttribute("a", "href", "removeConfig?id=" + CONFIG_ID); + HtmlAnchor removeAnchor = configFiles.getDocumentElement().getFirstByXPath("//a[contains(@onclick, 'removeConfig?id=" + CONFIG_ID + "')]"); AtomicReference confirmCalled = new AtomicReference<>(false); wc.setConfirmHandler((page, s) -> { @@ -87,7 +87,7 @@ public void xssPrevention() throws Exception { JenkinsRule.WebClient wc = j.createWebClient(); HtmlPage configFiles = wc.goTo("configfiles"); - HtmlAnchor removeAnchor = configFiles.getDocumentElement().getOneHtmlElementByAttribute("a", "href", "removeConfig?id=" + CONFIG_ID); + HtmlAnchor removeAnchor = configFiles.getDocumentElement().getFirstByXPath("//a[contains(@onclick, 'removeConfig?id=" + CONFIG_ID + "')]"); AtomicReference confirmCalled = new AtomicReference<>(false); AtomicReference alertCalled = new AtomicReference<>(false); diff --git a/src/test/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileActionTest.java b/src/test/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileActionTest.java index 456f0c2f..940ce2fb 100644 --- a/src/test/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileActionTest.java +++ b/src/test/java/org/jenkinsci/plugins/configfiles/folder/FolderConfigFileActionTest.java @@ -1,6 +1,8 @@ package org.jenkinsci.plugins.configfiles.folder; import com.cloudbees.hudson.plugins.folder.Folder; +import com.gargoylesoftware.htmlunit.html.HtmlAnchor; +import com.gargoylesoftware.htmlunit.html.HtmlPage; import org.hamcrest.Matchers; import org.jenkinsci.lib.configprovider.ConfigProvider; import org.jenkinsci.lib.configprovider.model.Config; @@ -15,14 +17,22 @@ import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.BuildWatcher; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import java.io.IOException; import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; -import static org.junit.Assert.*; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.collection.IsEmptyCollection.empty; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; /** * Created by domi on 11/10/16. @@ -199,6 +209,47 @@ public void sameFolderPropertyAfterConfiguration() throws Exception { assertThat(store, Matchers.is(getStore(f1))); } + @Test + @Issue("SECURITY-2202") + public void xssPreventionInFolder() throws Exception { + final String CONFIG_ID = "myid"; + + // ---------- + // Create a new configuration in a new folder + Folder f1 = createFolder(); + ConfigFileStore store = getStore(f1); + + CustomConfig config = new CustomConfig(CONFIG_ID, "name", "comment", "content"); + store.save(config); + + assertEquals(1, store.getConfigs().size()); + + /// ---------- + // Check removing it by GET doesn't work + JenkinsRule.WebClient wc = r.createWebClient(); + + // If we try to call the URL directly (via GET), it fails with a 405 - Method not allowed + wc.assertFails(f1.getUrl() + "configfiles/removeConfig?id=" + CONFIG_ID, 405); + assertEquals(1, store.getConfigs().size()); + + // ---------- + // Clicking the button works + // If we click on the link, it goes via POST, therefore it removes it successfully + HtmlPage configFiles = wc.goTo(f1.getUrl() + "configfiles"); + HtmlAnchor removeAnchor = configFiles.getDocumentElement().getFirstByXPath("//a[contains(@onclick, 'removeConfig?id=" + CONFIG_ID + "')]"); + + AtomicReference confirmCalled = new AtomicReference<>(false); + wc.setConfirmHandler((page, s) -> { + confirmCalled.set(true); + return true; + }); + + assertThat(confirmCalled.get(), is(false)); + removeAnchor.click(); + assertThat(confirmCalled.get(), is(true)); + assertThat(store.getConfigs(), empty()); + } + private CpsFlowDefinition getNewJobDefinition() { return new CpsFlowDefinition("" + "node {\n" + diff --git a/src/test/java/org/jenkinsci/plugins/configfiles/sec/Security2002Test.java b/src/test/java/org/jenkinsci/plugins/configfiles/sec/Security2002Test.java new file mode 100644 index 00000000..9e24abda --- /dev/null +++ b/src/test/java/org/jenkinsci/plugins/configfiles/sec/Security2002Test.java @@ -0,0 +1,66 @@ +package org.jenkinsci.plugins.configfiles.sec; + +import com.gargoylesoftware.htmlunit.html.HtmlAnchor; +import com.gargoylesoftware.htmlunit.html.HtmlPage; +import jenkins.model.GlobalConfiguration; +import org.jenkinsci.plugins.configfiles.GlobalConfigFiles; +import org.jenkinsci.plugins.configfiles.custom.CustomConfig; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; + +import java.util.concurrent.atomic.AtomicReference; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.collection.IsCollectionWithSize.hasSize; +import static org.hamcrest.collection.IsEmptyCollection.empty; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; + +public class Security2002Test { + private static final String CONFIG_ID = "ConfigFilesTestId"; + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Test + @Issue("SECURITY-2202") + public void xssPrevention() throws Exception { + + // ---------- + // Create a new configuration + GlobalConfigFiles store = j.getInstance().getExtensionList(GlobalConfiguration.class).get(GlobalConfigFiles.class); + assertNotNull(store); + assertThat(store.getConfigs(), empty()); + + CustomConfig config = new CustomConfig(CONFIG_ID, "My configuration", "comment", "content"); + store.save(config); + + assertThat(store.getConfigs(), hasSize(1)); + + // ---------- + // Check removing it by GET doesn't work + JenkinsRule.WebClient wc = j.createWebClient(); + + // If we try to call the URL directly (via GET), it fails with a 405 - Method not allowed + wc.assertFails("configfiles/removeConfig?id=" + CONFIG_ID, 405); + + // ---------- + // Clicking the button works + // If we click on the link, it goes via POST, therefore it removes it successfully + HtmlPage configFiles = wc.goTo("configfiles"); + HtmlAnchor removeAnchor = configFiles.getDocumentElement().getFirstByXPath("//a[contains(@onclick, 'removeConfig?id=" + CONFIG_ID + "')]"); + + AtomicReference confirmCalled = new AtomicReference<>(false); + wc.setConfirmHandler((page, s) -> { + confirmCalled.set(true); + return true; + }); + + assertThat(confirmCalled.get(), is(false)); + removeAnchor.click(); + assertThat(confirmCalled.get(), is(true)); + assertThat(store.getConfigs(), empty()); + } +}