fix(browser/react/vue): pass url.search to hasAuthParamsInUrl instead of falling back to window.location.search - #1
Open
Sdnsoumy wants to merge 1 commit into
Conversation
… of falling back to window.location.search hasAuthParams(url, afterSignInUrl) accepted a URL argument but called hasAuthParamsInUrl() with no argument, silently ignoring url.search and always checking window.location.search instead. This makes the helpers untestable in isolation and incorrect whenever the provided URL differs from the current window location. Fix all call sites to pass url.search explicitly. Also correct the JSDoc on hasAuthParamsInUrl: the function only checks for 'code', not both 'code' and 'session_state' as the old description claimed.
There was a problem hiding this comment.
Pull request overview
Fixes browser auth-param detection to use the caller-provided URL consistently (instead of implicitly reading window.location.search), improving correctness in non-window scenarios (tests, navigation, SSR hydration) and aligning documentation with actual behavior.
Changes:
- Pass
url.searchexplicitly tohasAuthParamsInUrl(url.search)from the React hook and Vue provider call sites. - Update
hasAuthParamsInUrlJSDoc to reflect that it checks forcode(notcode+session_state).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/vue/src/providers/ThunderIDProvider.ts | Passes url.search to hasAuthParamsInUrl to ensure correct URL evaluation. |
| packages/react/src/hooks/useBrowserUrl.ts | Passes url.search to hasAuthParamsInUrl to avoid falling back to window.location.search. |
| packages/browser/src/utils/hasAuthParamsInUrl.ts | Updates JSDoc to match the function’s actual detection logic (code). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
60
to
62
| const hasAuthParams = (url: URL, afterSignInUrl: string): boolean => | ||
| (hasAuthParamsInUrl() && new URL(url.origin + url.pathname).toString() === new URL(afterSignInUrl).toString()) || | ||
| (hasAuthParamsInUrl(url.search) && new URL(url.origin + url.pathname).toString() === new URL(afterSignInUrl).toString()) || | ||
| // authParams?.authorizationCode || // FIXME: These are sent externally. Need to see what we can do about this. |
| * Utility to check if `code` and `session_state` are available in the URL as search params. | ||
| * Utility to check if `code` is available in the URL as a search param. | ||
| * | ||
| * @param params - The URL search params to check. Defaults to `window.location.search`. |
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.
Purpose
hasAuthParams(url, afterSignInUrl)accepts aURLargument but both the React hook (useBrowserUrl) and the Vue provider (ThunderIDProvider) calledhasAuthParamsInUrl()with no argument, silently ignoringurl.searchand always falling back towindow.location.search.This makes the helpers untestable in isolation (you cannot pass an arbitrary URL) and would produce incorrect results in any scenario where the caller's
urldiffers from the current window location (e.g. during navigation events, SSR hydration, or unit tests).Also fixes a JSDoc mismatch on
hasAuthParamsInUrl: the description claimed it checks for bothcodeandsession_state, but the implementation only checks forcode.Approach
url.searchexplicitly tohasAuthParamsInUrl(url.search)in both call sites (useBrowserUrl.tsandThunderIDProvider.ts).hasAuthParamsInUrl.tsto match the actual behaviour.No behaviour change in production (both callers already pass the current window URL), but the helpers are now correct by construction and fully testable.
Related Issues