Guard OAuth return pages against prototype pollution

[vharseko]

Summary

Fixes the CodeQL high js/remote-property-injection alerts on the two OAuth return pages (originally #2041 / #2042, re-reported as #2082 / #2083), and — on the mock provider page — the js/xss / js/client-side-unvalidated-url-redirection alerts (#2085–#2088).

Both pages built a queryParams object by writing map[name] = value for every URL parameter, where the parameter name is user-controlled — the pattern CodeQL flags as a user-keyed property write (CWE-1321). There is no known exploit path in the old code: every written value is a string from decodeURIComponent, and the __proto__ setter silently ignores non-object values, so Object.prototype could not actually be polluted; the only observable effect was shadowing names like constructor on a local throwaway map that was read through its three known keys only. The rewrite removes the flagged pattern rather than closing a live vulnerability.

The mock page fix goes further: it also stops an open redirect (CWE-601) / DOM XSS (CWE-79), which was a real (test-scope) flaw — see below.

Changes

• ui/commons/.../oauthReturn.html and ui/mock/.../mockOAuthAuthorization.html — replace the query-parameter map with a getQueryParam(name) helper that scans the query string for a single known name. The pages read only the parameters they actually use (state, code, redirect_uri), so no property is ever written under a user-controlled name and the alert source is gone entirely. A side benefit: the old code decoded every parameter, so a malformed unrelated one (?junk=%&state=ok) threw URIError and killed the page; the helper decodes only the requested parameter.
• mockOAuthAuthorization.html additionally gains an open-redirect / XSS fix: the page previously navigated to the user-supplied redirect_uri verbatim, accepting javascript: and cross-origin targets. The new getSafeRedirectUri() resolves redirect_uri with the URL API against the current document, rejects anything not on the page's own origin, and rebuilds the target as origin + pathname (any query/fragment on the incoming value is dropped deliberately — the caller appends its own ?code=…&state=…). The countdown interval is now also cleared once the redirect fires, fixing a pre-existing timer leak.

An earlier revision of this PR kept the map and skipped the reserved names __proto__ / constructor / prototype; CodeQL does not recognize that blocklist as a sanitizer (the query flags any user-keyed property write, not just prototype pollution), so the map was removed altogether.

Verification

Node logic test of the helpers:

• normal parsing preserved — state / code / redirect_uri are read correctly, including the encoded-fragment example from the oauthReturn.html doc comment;
• duplicate parameters keep last-wins semantics and a missing parameter yields undefined, matching the old map lookup;
• the origin check rejects javascript:alert(1), data: URIs, //evil.com/x, and http://localhost:8080@evil.com/x, while same-origin absolute and relative paths rebuild unchanged.

[maximthomas]

The parsing rewrite is correct and strictly safer than what it replaces — I verified the new helper against the old reduce across seven query-string shapes and behaviour is identical except for malformed input, where the new code is better (?junk=%&state=ok used to throw URIError and kill the page; it now returns ok). clearInterval(timer) fixes a real pre-existing leak, and the redirect_uri origin check correctly rejects javascript:, data:, //evil.com/x, and http://localhost:8080@evil.com/x. Two things to fix before merge, both small.

PR description misstates the vulnerability (medium)

The body claims:

?__proto__=polluted no longer sets Object.prototype.polluted

That was never achievable with the old code. map["__proto__"] = <string> invokes the __proto__ setter, which silently ignores non-object values — and every value here comes from decodeURIComponent, which always returns a string. Verified:

var map = {};
map["__proto__"] = decodeURIComponent("polluted");
({}).polluted                       // undefined
Object.getOwnPropertyNames(map)     // []
Object.getPrototypeOf(map) === Object.prototype  // true

The old code's only real effect was shadowing names like constructor on a local throwaway map, which was inert because only .state / .code / .redirect_uri were ever read.

The change is still worth making — it clears the CodeQL alert and the code is simpler — but the PR carries the security label, so this text lands in the security changelog. Please reword to what it actually is: removing a user-keyed property write flagged by js/remote-property-injection, with no known exploit path.

While there: the body frames this as prototype-pollution only, but /home/maxim/Documents/_projects/forgerock/commons/ui/mock/src/main/resources/mockOAuthAuthorization.html also gains an open-redirect fix (CWE-601) — a genuinely different and larger change. Worth its own sentence so it's findable later.

getSafeRedirectUri silently drops query and fragment (low)

/home/maxim/Documents/_projects/forgerock/commons/ui/mock/src/main/resources/mockOAuthAuthorization.html

return window.location.protocol + "//" + window.location.host + url.pathname;

Only pathname survives — any ?… or #… on the incoming redirect_uri is discarded (/app/oauthReturn.html?foo=bar#frag → /app/oauthReturn.html). Harmless today, because OAuth.getRedirectURI() always yields a path-only URL and the caller appends its own ?code=…, but it's an unstated narrowing in a mock provider whose whole job is exercising client flows. Add a one-line comment saying search/hash are dropped deliberately.

Also, the url.origin !== window.location.origin check has just passed, so the manual rebuild is equivalent to and clearer as:

return url.origin + url.pathname;

[vharseko]

Thanks for the thorough review — both points are addressed.

PR description — rewritten. It no longer claims ?__proto__=polluted could pollute Object.prototype; it now states explicitly that every value the old code wrote was a string from decodeURIComponent, the __proto__ setter silently ignores non-object values, and there was no known exploit path — the rewrite removes the user-keyed property write pattern that js/remote-property-injection flags, rather than closing a live vulnerability. The open-redirect / XSS fix in mockOAuthAuthorization.html now has its own paragraph in both Summary and Changes.

getSafeRedirectUri — simplified to url.origin + url.pathname and a comment now states that any query/fragment on the incoming redirect_uri is dropped deliberately, since the caller appends its own ?code=...&state=... (3d94a57). Re-verified in Node after the change: same-origin absolute and relative paths rebuild unchanged, and javascript:alert(1), data:, //evil.com/x, and http://localhost:8080@evil.com/x are still rejected.