Do not use FormApply#applyResponse to execute arbitrary javascript
#481
Conversation
| } | ||
| 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") |
There was a problem hiding this comment.
Well, that doesn't always result in the creation of credentials, e.g when ID already exists, it won't create anything.
You could use the boolean result of addCredentials(...) to adapt your message.
There was a problem hiding this comment.
You could use the boolean result of addCredentials(...) to adapt your message.
Oh, that sounds great to me, I've missed that there's return value from addCredentials.
There was a problem hiding this comment.
I think it would be weird to show a notification saying "Credentials not added" and not specifying reason why. So I'm unsure at the moment how to act on this, will give it another thought.
There was a problem hiding this comment.
Existing implementations seem to return false when the specified domain already contains a credential with the specified ID. Unsure whether it's valid to just claim that's the cause though (even if it looks plausible).
There was a problem hiding this comment.
And also if domain doesn't exist it seems. See
Unsure under what circumstances that path would be reachable.
src/main/java/com/cloudbees/plugins/credentials/CredentialsSelectHelper.java
Outdated
Show resolved
Hide resolved
* use `#element` over `#accumulate` * show different notification when credentials weren't added
Motivated by jenkinsci/jenkins#8351.
That PR changes
FormApply#applyResponsemethod to make it CSP compatible in a way that does not allow to execute arbitrary javascript as a callback.Looking at the usages of
FormApply#applyResponsein the ecosystem its most common (and perhaps intended) use is to show a notification after "Apply" button (or alike) was clicked. In credentials-plugin it's used in a "add credentials in-place" form, and the calls are rather uncommon.Two calls are
FormApply.applyResponse("window.alert(...)"), which could easily be replaced by the new CSP compatible versionFormApply.showNotificationfrom the core PR linked above.Third call is worse -
FormApply.applyResponse("window.credentials.refreshAll();").My initial attempt was to keep using
FormApply.applyResponsemechanism, but movingwindow.credentials.refreshAll()to .js and making that execute at the right time turned out to be far from trivial.After analysing the code for a bit more there seems to be no reason to keep using
FormApply.applyResponse, hence this PR gets rid of it. It changes the way that form is submitted, and gets rid of javascript code needed for theFormApply.applyResponsemechanism to work. Instead server now responds with data for the notification, and javascript code uses that data to callnotificationBar.show(...).window.credentials.refreshAll()was also moved to .js file, and is executed after credentials were successfully added and response was received.Testing done
Tested interactively by adding credentials via "add credentials in-place" form. Made sure credentials were added successfully and immediately available in the dropdown after the form popup was closed.
Tested error paths by attaching the debugger and changing values through that, since I found no way to make credentials domain non-modifiable via UI.
Submitter checklist