From ef08c305105f64ee4ae86c0383e21506f36a0039 Mon Sep 17 00:00:00 2001 From: Yaroslav Afenkin Date: Wed, 6 Sep 2023 18:29:01 +0300 Subject: [PATCH 1/3] Do not use `FormApply#applyResponse` to execute arbitrary javascript --- .../credentials/CredentialsSelectHelper.java | 17 ++-- .../lib/credentials/select/select.js | 82 ++++++------------- .../CredentialsSelectHelperTest.java | 73 +++++++++++++++++ .../CredentialsSelectionAction/index.jelly | 8 ++ 4 files changed, 115 insertions(+), 65 deletions(-) create mode 100644 src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java create mode 100644 src/test/resources/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest/CredentialsSelectionAction/index.jelly diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java index 127beb026..1a2f703f7 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java +++ b/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,10 +595,13 @@ 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() + .accumulate("message", "Domain is read-only") + .accumulate("notificationType", "ERROR"); } store.checkPermission(CredentialsStoreAction.CREATE); JSONObject data = req.getSubmittedForm(); @@ -607,13 +609,16 @@ public void doAddCredentials(StaplerRequest req, StaplerResponse rsp) throws IOE 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() + .accumulate("message", "Store does not have selected domain") + .accumulate("notificationType", "ERROR"); } store.checkPermission(CredentialsStoreAction.CREATE); Credentials credentials = req.bindJSON(Credentials.class, data.getJSONObject("credentials")); store.addCredentials(wrapper.getDomain(), credentials); - FormApply.applyResponse("window.credentials.refreshAll();").generateResponse(req, rsp, null); + return new JSONObject() + .accumulate("message", "Credentials created") + .accumulate("notificationType", "SUCCESS"); } /** diff --git a/src/main/resources/lib/credentials/select/select.js b/src/main/resources/lib/credentials/select/select.js index 74af1724e..a5f44aa2a 100644 --- a/src/main/resources/lib/credentials/select/select.js +++ b/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("
"); - 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; diff --git a/src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java b/src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java new file mode 100644 index 000000000..f00f6a380 --- /dev/null +++ b/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(2000); + 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(1000); + + // check if credentials were added + List 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"; + } + } +} diff --git a/src/test/resources/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest/CredentialsSelectionAction/index.jelly b/src/test/resources/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest/CredentialsSelectionAction/index.jelly new file mode 100644 index 000000000..6400a9d23 --- /dev/null +++ b/src/test/resources/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest/CredentialsSelectionAction/index.jelly @@ -0,0 +1,8 @@ + + + + + + + + From 53e74eb384566db610b1e9ab5edd47172ae9a83f Mon Sep 17 00:00:00 2001 From: Yaroslav Afenkin Date: Fri, 15 Sep 2023 12:05:01 +0300 Subject: [PATCH 2/3] Set sensible `waitForBackgroundJavaScript` timeouts --- .../plugins/credentials/CredentialsSelectHelperTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java b/src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java index f00f6a380..b2e5515dd 100644 --- a/src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java +++ b/src/test/java/com/cloudbees/plugins/credentials/CredentialsSelectHelperTest.java @@ -30,7 +30,7 @@ public void doAddCredentialsFromPopupWorksAsExpected() throws Exception { addCredentialsButton.click(); HtmlListItem li = htmlPage.querySelector(".credentials-add-menu-items li"); li.click(); - wc.waitForBackgroundJavaScript(2000); + wc.waitForBackgroundJavaScript(4000); HtmlForm form = htmlPage.querySelector("#credentialsDialog form"); HtmlInput username = form.querySelector("input[name='_.username']"); @@ -42,7 +42,7 @@ public void doAddCredentialsFromPopupWorksAsExpected() throws Exception { HtmlSpan formSubmitButton = form.querySelector("#credentials-add-submit"); formSubmitButton.fireEvent("click"); - wc.waitForBackgroundJavaScript(1000); + wc.waitForBackgroundJavaScript(5000); // check if credentials were added List creds = CredentialsProvider.lookupCredentials(UsernamePasswordCredentials.class); From d71602e7cc03a96bf876171da18a19d8dcb7efb9 Mon Sep 17 00:00:00 2001 From: Yaroslav Afenkin Date: Thu, 28 Sep 2023 14:53:02 +0300 Subject: [PATCH 3/3] Address review feedback * use `#element` over `#accumulate` * show different notification when credentials weren't added --- .../credentials/CredentialsSelectHelper.java | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java b/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java index 1a2f703f7..ccf6c41fb 100644 --- a/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java +++ b/src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java @@ -600,8 +600,8 @@ public JSONObject doAddCredentials(StaplerRequest req, StaplerResponse rsp) thro if (!store.isDomainsModifiable()) { hudson.util.HttpResponses.status(400).generateResponse(req, rsp, null); return new JSONObject() - .accumulate("message", "Domain is read-only") - .accumulate("notificationType", "ERROR"); + .element("message", "Domain is read-only") + .element("notificationType", "ERROR"); } store.checkPermission(CredentialsStoreAction.CREATE); JSONObject data = req.getSubmittedForm(); @@ -610,15 +610,22 @@ public JSONObject doAddCredentials(StaplerRequest req, StaplerResponse rsp) thro if (!store.getDomains().contains(wrapper.getDomain())) { hudson.util.HttpResponses.status(400).generateResponse(req, rsp, null); return new JSONObject() - .accumulate("message", "Store does not have selected domain") - .accumulate("notificationType", "ERROR"); + .element("message", "Store does not have selected domain") + .element("notificationType", "ERROR"); } store.checkPermission(CredentialsStoreAction.CREATE); Credentials credentials = req.bindJSON(Credentials.class, data.getJSONObject("credentials")); - store.addCredentials(wrapper.getDomain(), credentials); - return new JSONObject() - .accumulate("message", "Credentials created") - .accumulate("notificationType", "SUCCESS"); + 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"); + } } /**