Fix double-escaping of URLs in dropdown menu items - #27221
Conversation
|
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
|
@aroy114 thanks for the pull request. Please don't request expedited review as a new contributor. Expedited review time is precious for Jenkins core maintainers and needs to be reserved for cases where the expedited review provides significant benefit to the community. When you @ mention core-pr-reviewers on something that should not have an expedited review, you distract maintainers from other work. Please restore the maintainer checklist that you removed from the end of the pull request template. Maintainers use it as a reminder of the things they need to consider. |
|
Thanks for the feedback @MarkEWaite and apologies for both. |
|
I think this should be opt.event.postTo rather than pt.event.postTo. I couldn't find pt defined in this scope, and ESLint reports it as undefined. Could you check this? |
|
Could we also add a test for tryConfirmationPost()? This path wasn't covered in the manual testing, and a test here could help catch issues like this. |
c3f0001 to
1ed8b8e
Compare
|
Thank you @preetham-18-developer, that was a typo ( Happy to add a test for |
|
@aroy114 Sure. I'll check the existing tests around the dropdown components and share a relevant example. |
Fixes #27202
Root cause:
templates.jsappliesxmlEscape()to Action URLs in threeplaces where it either double-escapes an already-correctly-escaped
value, or escapes a value headed to a non-HTML-parsing sink
(
fetch()andform.setAttribute()), where entity escaping is neverundone and instead corrupts the literal string sent over the wire.
menuItem()— the dropdown/jumplisthrefwas escaped once whenbuilding
url, then escaped again insideoptionalVal(). Thisleft literal
&text in the rendered href instead of a real&, so only the first query parameter reached the server.tryPost()— thefetch()URL was escaped, butfetch()sendsthe string verbatim; escaping here corrupted POST action URLs the
same way.
tryConfirmationPost()— same issue viaform.setAttribute("action", ...),which also sets a literal value, not HTML-parsed content.
All three now pass the raw URL through unescaped.
optionalVal()remains the single source of truth for HTML-attribute escaping and
still correctly escapes the
hrefexactly once inmenuItem().Testing done
Reproduced the bug from #27202 locally and verified the fix for the
dropdown/jumplist
hrefpath (menuItem()):buildAddUrlwith a URL containing multiple&-joined query parameters (?a=1&b=2&c=3)hrefinDevTools showed
...?a=1&b=2&c=3(literal&text);clicking it only passed
a=1to the target job,bandcweredropped
hrefrenders as...?a=1&b=2&c=3(confirmed viathe "This URL requires POST" interstitial, which echoes the exact
URL being accessed with a real
&in place), matching the workingsidebar-rendered link
target job's Parameters page that all three parameters landed
correctly:
a=1,b=2,c=3The
tryPost()andtryConfirmationPost()fixes address the sameroot-cause pattern (
xmlEscape()applied to a URL/value passed to anon-HTML-parsing sink —
fetch()andform.setAttribute()respectively, neither of which decode HTML entities). These were
identified by code inspection rather than independently exercised in
this testing pass; happy to add manual verification steps for these
if a reviewer can point me to an existing core Action that uses
either path.
Screenshots (UI changes only)
N/A — this is a behavioral fix (URL escaping), not a visual UI change.
See Testing done above for verification evidence.
Before
Dropdown/jumplist href contains literal
&text; only the firstquery parameter is passed through on click.
After
URL shown on the "This URL requires POST" interstitial contains a
real
&; target job's Parameters page shows all three parameters(
a=1,b=2,c=3) populated correctly.Proposed changelog entries
&due to incorrect URL escapingProposed changelog category
/label bug
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.N/A — no new public classes/fields/methods.
@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.N/A — no deprecations.
evalcalls introduced.N/A — no dependency changes.
N/A — no new APIs.
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered.