SECURITY-2202

jenkinsci · Apr 15, 2021 · 313e9d1 · 313e9d1
1 parent 5f845bc
commit 313e9d1
Show file tree

Hide file tree

Showing 7 changed files with 128 additions and 29 deletions.
diff --git 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
@@ -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
@@ -49,10 +49,10 @@ THE SOFTWARE.
                                      src="${imagesURL}/16x16/document_edit.gif"/>
                             </a>
                             <j:out value=" "/>
-                            <a href="removeConfig?id=${t.id}" onclick="return cfp_confirmDelete(this)" data-confirm="${t.name}">
+                            <l:confirmationLink href="removeConfig?id=${t.id}" post="true" message="Sure you want to delete [${t.name}]?">
                                 <img width="16" height="16" title="${%remove script} ${t.name}"
                                      src="${imagesURL}/16x16/edit-delete.gif"/>
-                            </a>
+                            </l:confirmationLink>
                         </td>
                         <td>
                             <i>
@@ -78,14 +78,4 @@ THE SOFTWARE.
             </table>
         </j:forEach>
     </div>
-    <script>
-        function cfp_confirmDelete(anchor) {
-            var confirmMessage = anchor.getAttribute("data-confirm");
-            if (confirm("Sure you want to delete ["+confirmMessage+"]?")) {
-                return true;
-            }else{
-                return false;
-            }
-        }
-    </script>
 </j:jelly>
diff --git 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" />
                             </a>
                             <j:out value=" " />
-                            <a href="removeConfig?id=${t.id}" onclick="return cfp_confirmDelete(this)" data-confirm="${t.name}">
+                            <l:confirmationLink href="removeConfig?id=${t.id}" post="true" message="Sure you want to delete [${t.name}]?">
                                 <img width="16" height="16" title="${%remove script} ${t.name}"
                                      src="${imagesURL}/16x16/edit-delete.gif" />
-                            </a>
+                            </l:confirmationLink>
                         </td>
                         <td >
                             <i>${t.name}</i>
@@ -64,16 +64,4 @@ THE SOFTWARE.
             </table>
         </j:forEach>
     </div>
-
-    <script>
-        function cfp_confirmDelete(anchor) {
-            var confirmMessage = anchor.getAttribute("data-confirm");
-            if (confirm("Sure you want to delete ["+confirmMessage+"]?")) {
-                return true;
-            }else{
-                return false;
-            }
-        }
-    </script>
-
 </j:jelly>
diff --git 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<Boolean> 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<Boolean> confirmCalled = new AtomicReference<>(false);
         AtomicReference<Boolean> 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
@@ -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<Boolean> 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
@@ -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<Boolean> 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());
+    }
+}