Repository navigation
Avoid URLSearchParams.size when parsing the callback URL - #355
Closed
choksi2212 wants to merge 1 commit into
Closed
choksi2212 wants to merge 1 commit into
choksi2212 wants to merge 1 commit into
Conversation
URLSearchParams.size shipped in Safari 17 (Sept 2023); on Safari 16 and other browsers without it, url.searchParams.size is undefined, so the condition was false and the query callback was skipped entirely. Apps that require a login then bounced between the application and the auth server, with the auth server rejecting the stale redirect URI, looping until the browser gives up (keycloak/keycloak#53895). Check the query string directly instead, mirroring the fragment branch directly below, which restores the behavior the URL parsing had before 82924d0 introduced the property. Also covers the degenerate '?'-only case the same way the old check did: the params parse to nothing, so the callback is still treated as absent. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
Author
|
First PR from this fork, so the CI workflows are waiting on maintainer approval before they can run. Everything's verified locally — happy to wait, just flagging it in case it slipped by. |
Contributor
|
Thanks for the fix, but we target baseline widely available as a minimum platform target. Since Closing this PR. |
8000
Closed
2 tasks done
Author
|
Understood — thanks for the context. Baseline "widely available" as the platform bar noted for future contributions; under that target |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #359
What
One-line change in
#parseCallbackUrl: check the query string directly instead ofurl.searchParams.size > 0, mirroring the fragment branch right below it.Root cause
URLSearchParams.sizeshipped in Safari 17 (Sept 2023), Chrome 113 and Firefox 112. On Safari 16 — which is what iPhone 8/X and other iOS 16-capped devices run —url.searchParams.sizeisundefined, soundefined > 0is false and the whole query branch is skipped. The callback is never parsed, the storedkc-callback-{state}entry is never consumed, and withonLoad: 'login-required'the adapter bounces back to the auth server with a staleredirect_urithat carries the old?code=...&state=..., which the server rejects (LOGIN_ERROR invalid_redirect_uri) — the infinite loop from the issue.The property was introduced by the URL parsing refactor in 82924d0 (#143); before that, the same check was done on the raw string and worked everywhere.
Verification
URLSearchParams.prototype.sizedeleted to simulate Safari 16:main: the standard?code=...&state=...callback returnsundefined— the exact miss from the issue.code,state,session_state,iss, and the strippednewUrl), and results are identical with and withoutsizepresent, so nothing changes for modern browsers. Error callbacks (error=...&state=...) parse too, unsupported params survive innewUrlwhile OAuth params are stripped, fragment mode is unaffected, and the degenerate?-onlyURL still counts as no callback, matching the old.size > 0behavior.login-without-searchparams-size.spec.ts) that deletes the property in the page viaaddInitScript, logs in withresponseMode: 'query', and expects the callback to be parsed and the user authenticated. On pristinemainthis test cannot pass because the callback is lost; it runs in the CI integration job.npm run lint(ts-standard) passes;tsc --noEmitreports no new errors for the touched region or the new spec.