Merge pull request #481 from yaroslavafenkin/form-apply

Do not use `FormApply#applyResponse` to execute arbitrary javascript

src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java

@@ -38,7 +38,6 @@
 import hudson.model.User;
 import hudson.security.AccessControlled;
 import hudson.security.Permission;
-import hudson.util.FormApply;
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -596,24 +595,37 @@ public WrappedCredentialsStore(@NonNull ContextResolver resolver, @NonNull Crede
          * @throws ServletException if something goes wrong.
          */
         @RequirePOST
-        public void doAddCredentials(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {
+        @Restricted(NoExternalUse.class)
+        public JSONObject doAddCredentials(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {
             if (!store.isDomainsModifiable()) {
                 hudson.util.HttpResponses.status(400).generateResponse(req, rsp, null);
-                FormApply.applyResponse("window.alert('Domain is read-only')").generateResponse(req, rsp, null);
+                return new JSONObject()
+                        .element("message", "Domain is read-only")
+                        .element("notificationType", "ERROR");
             }
             store.checkPermission(CredentialsStoreAction.CREATE);
             JSONObject data = req.getSubmittedForm();
             String domainName = data.getString("domain");
             CredentialsStoreAction.DomainWrapper wrapper = getWrappers().get(domainName);
             if (!store.getDomains().contains(wrapper.getDomain())) {
                 hudson.util.HttpResponses.status(400).generateResponse(req, rsp, null);
-                FormApply.applyResponse("window.alert('Store does not have selected domain')")
-                        .generateResponse(req, rsp, null);
+                return new JSONObject()
+                        .element("message", "Store does not have selected domain")
+                        .element("notificationType", "ERROR");
             }
             store.checkPermission(CredentialsStoreAction.CREATE);
             Credentials credentials = Descriptor.bindJSON(req, Credentials.class, data.getJSONObject("credentials"));
-            store.addCredentials(wrapper.getDomain(), credentials);
-            FormApply.applyResponse("window.credentials.refreshAll();").generateResponse(req, rsp, null);
+            boolean credentialsWereAdded = store.addCredentials(wrapper.getDomain(), credentials);
+            if (credentialsWereAdded) {
+                return new JSONObject()
+                        .element("message", "Credentials created")
+                        .element("notificationType", "SUCCESS");
+            } else {
+                return new JSONObject()
+                        .element("message", "Credentials with specified ID already exist in " + domainName + " domain")
+                        // TODO: or domain does not exist at all?
+                        .element("notificationType", "ERROR");
+            }
         }
 
         /**

src/main/resources/lib/credentials/select/select.js

@@ -120,68 +120,32 @@ window.credentials.refreshAll = function () {
         h();
     });
 };
-window.credentials.addSubmit = function (e) {
-    var id;
-    var containerId = "container" + (iota++);
+window.credentials.addSubmit = function (_) {
+    const form = document.getElementById('credentials-dialog-form');
+    buildFormTree(form);
+    ajaxFormSubmit(form);
 
-    var responseDialog = new YAHOO.widget.Panel("wait" + (iota++), {
-        fixedcenter: true,
-        close: true,
-        draggable: true,
-        zindex: 4,
-        modal: true,
-        visible: false
-    });
-
-    responseDialog.setHeader("Error");
-    responseDialog.setBody("<div id='" + containerId + "'></div>");
-    responseDialog.render(document.body);
-    var target; // iframe
-
-    function attachIframeOnload(target, f) {
-        if (target.attachEvent) {
-            target.attachEvent("onload", f);
-        } else {
-            target.onload = f;
-        }
-    }
-
-    var f = document.getElementById('credentials-dialog-form');
-    // create a throw-away IFRAME to avoid back button from loading the POST result back
-    id = "iframe" + (iota++);
-    target = document.createElement("iframe");
-    target.setAttribute("id", id);
-    target.setAttribute("name", id);
-    target.setAttribute("style", "height:100%; width:100%");
-    document.getElementById(containerId).appendChild(target);
-
-    attachIframeOnload(target, function () {
-        if (target.contentWindow && target.contentWindow.applyCompletionHandler) {
-            // apply-aware server is expected to set this handler
-            target.contentWindow.applyCompletionHandler(window);
-        } else {
-            // otherwise this is possibly an error from the server, so we need to render the whole content.
-            var r = YAHOO.util.Dom.getClientRegion();
-            responseDialog.cfg.setProperty("width", r.width * 3 / 4 + "px");
-            responseDialog.cfg.setProperty("height", r.height * 3 / 4 + "px");
-            responseDialog.center();
-            responseDialog.show();
-        }
-        window.setTimeout(function () {// otherwise Firefox will fail to leave the "connecting" state
-            document.getElementById(id).remove();
-        }, 0)
-    });
-
-    f.target = target.id;
-    try {
-        buildFormTree(f);
-        f.submit();
-    } finally {
-        f.target = null;
+    function ajaxFormSubmit(form) {
+        fetch(form.action, {
+            method: form.method,
+            headers: crumb.wrap({}),
+            body: new FormData(form)
+        })
+            .then(res => res.json())
+            .then(data => {
+                window.notificationBar.show(data.message, window.notificationBar[data.notificationType]);
+                window.credentials.refreshAll();
+            })
+            .catch((e) => {
+                // notificationBar.show(...) with logging ID could be handy here?
+                console.error("Could not add credentials:", e);
+            })
+            .finally(() => {
+                window.credentials.dialog.hide();
+            });
     }
-    window.credentials.dialog.hide();
-    return false;
 };
+
 Behaviour.specify("BUTTON.credentials-add-menu", 'credentials-select', -99, function(e) {
     var btn = e;
     var menu = btn.nextElementSibling;

src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java

@@ -0,0 +1,73 @@
+package com.cloudbees.plugins.credentials;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import com.cloudbees.plugins.credentials.common.UsernamePasswordCredentials;
+import hudson.model.UnprotectedRootAction;
+import java.util.List;
+import org.hamcrest.Matchers;
+import org.htmlunit.html.HtmlButton;
+import org.htmlunit.html.HtmlForm;
+import org.htmlunit.html.HtmlInput;
+import org.htmlunit.html.HtmlListItem;
+import org.htmlunit.html.HtmlPage;
+import org.htmlunit.html.HtmlSpan;
+import org.junit.Rule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.TestExtension;
+
+public class CredentialsSelectHelperTest {
+    @Rule
+    public JenkinsRule j = new JenkinsRule();
+
+    @Test
+    public void doAddCredentialsFromPopupWorksAsExpected() throws Exception {
+        try (JenkinsRule.WebClient wc = j.createWebClient()) {
+            HtmlPage htmlPage = wc.goTo("credentials-selection");
+            HtmlButton addCredentialsButton = htmlPage.querySelector(".credentials-add-menu");
+            addCredentialsButton.click();
+            HtmlListItem li = htmlPage.querySelector(".credentials-add-menu-items li");
+            li.click();
+            wc.waitForBackgroundJavaScript(4000);
+            HtmlForm form = htmlPage.querySelector("#credentialsDialog form");
+
+            HtmlInput username = form.querySelector("input[name='_.username']");
+            username.setValue("bob");
+            HtmlInput password = form.querySelector("input[name='_.password']");
+            password.setValue("secret");
+            HtmlInput id = form.querySelector("input[name='_.id']");
+            id.setValue("test");
+
+            HtmlSpan formSubmitButton = form.querySelector("#credentials-add-submit");
+            formSubmitButton.fireEvent("click");
+            wc.waitForBackgroundJavaScript(5000);
+
+            // check if credentials were added
+            List<UsernamePasswordCredentials> creds = CredentialsProvider.lookupCredentials(UsernamePasswordCredentials.class);
+            assertThat(creds, Matchers.hasSize(1));
+            UsernamePasswordCredentials cred = creds.get(0);
+            assertThat(cred.getUsername(), is("bob"));
+            assertThat(cred.getPassword().getPlainText(), is("secret"));
+        }
+    }
+
+    @TestExtension
+    public static class CredentialsSelectionAction implements UnprotectedRootAction {
+        @Override
+        public String getIconFileName() {
+            return null;
+        }
+
+        @Override
+        public String getDisplayName() {
+            return null;
+        }
+
+        @Override
+        public String getUrlName() {
+            return "credentials-selection";
+        }
+    }
+}

src/test/resources/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest/CredentialsSelectionAction/index.jelly

@@ -0,0 +1,8 @@
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:l="/lib/layout" xmlns:c="/lib/credentials">
+    <l:layout title="Credentials Select Helper">
+        <l:main-panel>
+            <c:select/>
+        </l:main-panel>
+    </l:layout>
+</j:jelly>