fix(client): preserve authorization server subpath in fallback URLs #1832
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1621,6 +1621,59 @@ describe('OAuth Authorization', () => { | |
| ); | ||
| }); | ||
|
|
||
| describe('fallback URL subpath preservation', () => { | ||
| const validClientInfo = { | ||
| client_id: 'client123', | ||
| client_secret: 'secret123', | ||
| redirect_uris: ['http://localhost:3000/callback'], | ||
| client_name: 'Test Client' | ||
| }; | ||
|
|
||
| it('preserves authorization server subpath in fallback authorize URL', async () => { | ||
| const { authorizationUrl } = await startAuthorization('https://auth.example.com/admin', { | ||
| metadata: undefined, | ||
| clientInformation: validClientInfo, | ||
| redirectUrl: 'http://localhost:3000/callback' | ||
| }); | ||
|
|
||
| expect(authorizationUrl.pathname).toBe('/admin/authorize'); | ||
| }); | ||
|
|
||
| it('preserves subpath in fallback token URL', async () => { | ||
| const mockFetch = vi.fn(); | ||
| mockFetch.mockResolvedValueOnce({ | ||
| ok: true, | ||
| status: 200, | ||
| json: async () => ({ | ||
| access_token: 'access123', | ||
| token_type: 'Bearer' | ||
| }) | ||
|
Comment on lines
1621
to
+1650
There was a problem hiding this comment. 🟡 The PR adds tests for the /authorize and /token fallback subpath preservation but omits a corresponding test for registerClient() when metadata is undefined and authorizationServerUrl has a non-root path. Since registerClient is one of the three call sites changed by this PR to use buildFallbackUrl, a regression (e.g., someone accidentally reverting that call site back to new URL(...)) would go undetected. Extended reasoning...What the gap is: The PR introduces buildFallbackUrl() and updates three call sites: startAuthorization (/authorize), executeTokenRequest (/token), and registerClient (/register). The new test group validates subpath preservation for the first two call sites but leaves the third uncovered. The specific untested code path: In registerClient() at line ~1716, the else branch Why existing tests do not catch a regression here: The two added tests call startAuthorization and exchangeAuthorization respectively. registerClient has a distinct code path (it does a POST for dynamic client registration) and neither of those tests exercises it. A future commit that accidentally reverts line 1716 back to Addressing the refutation: One verifier argued that since all three call sites use the exact same buildFallbackUrl helper, the behavior is guaranteed identical and testing the /register path is redundant. This is a reasonable observation about the helper's correctness, but it misses the point: the value of a per-call-site test is not to re-verify the helper's logic, but to guard the integration point, ensuring the call site itself continues to use buildFallbackUrl rather than reverting to the old new URL() construction. Helpers can be correct while a specific call site still regresses. Concrete proof of the gap: Suppose a developer later edits registerClient and accidentally writes How to fix: Add a test inside the fallback URL subpath preservation group that calls registerClient with metadata: undefined and a non-root server URL, using a mock fetch that captures the request URL, then asserts pathname === '/admin/register'. This mirrors the pattern used by the /token test. |
||
| }); | ||
|
|
||
| await exchangeAuthorization('https://auth.example.com/admin', { | ||
| metadata: undefined, | ||
| clientInformation: validClientInfo, | ||
| authorizationCode: 'code123', | ||
| codeVerifier: 'verifier123', | ||
| redirectUri: 'http://localhost:3000/callback', | ||
| fetchFn: mockFetch | ||
| }); | ||
|
|
||
| const fetchUrl = new URL(mockFetch.mock.calls[0]![0] as string); | ||
| expect(fetchUrl.pathname).toBe('/admin/token'); | ||
| }); | ||
|
|
||
| it('still works for root-path authorization servers', async () => { | ||
| const { authorizationUrl } = await startAuthorization('https://auth.example.com', { | ||
| metadata: undefined, | ||
| clientInformation: validClientInfo, | ||
| redirectUrl: 'http://localhost:3000/callback' | ||
| }); | ||
|
|
||
| expect(authorizationUrl.pathname).toBe('/authorize'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('exchangeAuthorization', () => { | ||
| const validTokens: OAuthTokens = { | ||
| access_token: 'access123', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 The new buildFallbackUrl helper preserves query parameters from authorizationServerUrl when building fallback endpoint URLs, unlike the old new URL("/authorize", base) idiom which dropped them. If an authorization server URL contains query parameters (e.g., sourced from resourceMetadata.authorization_servers[0]), those params are silently appended to the /authorize, /token, and /register fallback URLs. Fix: add url.search = ""; url.hash = ""; after setting url.pathname in buildFallbackUrl.
Extended reasoning...
What the bug is and how it manifests
The buildFallbackUrl function (auth.ts ~line 1014) constructs a fallback endpoint URL by cloning the authorization server URL, stripping a trailing slash from its pathname, and appending the endpoint path. It only sets url.pathname — it never clears url.search or url.hash. If the input URL carries query parameters, they survive untouched into the returned URL.
The specific code path that triggers it
All three fallback call sites are affected:
The authorizationServerUrl value can come from resourceMetadata.authorization_servers[0] (an arbitrary string from the protected resource metadata document, RFC 9728 §2) or be passed directly by external callers of the exported startAuthorization, executeTokenRequest, and registerClient functions.
Why existing code does not prevent it
The old idiom new URL('/authorize', base) naturally dropped query parameters because /authorize is an absolute-path reference — per the URL standard, resolving an absolute-path reference against a base resets the query component. The new helper copies the entire URL object and only reassigns pathname, so search remains whatever the input contained.
The discoverOAuthServerInfo fallback path uses new URL('/', serverUrl) to derive the authorization server URL, which also drops query params — but the authorizationServerUrl stored in discovery state is derived from resourceMetadata.authorization_servers[0] (a raw string), which may retain query params.
Impact
For the /authorize endpoint the stray query parameters are appended before OAuth parameters are added via searchParams.set(...), so they appear as extra (possibly conflicting) query string entries in the authorization redirect. For /token and /register, unexpected query parameters are almost certainly wrong and could cause the server to reject the request outright, breaking the OAuth flow silently for users whose AS URL happens to include query params.
How to fix it
Add two lines after setting url.pathname:
Step-by-step proof
Old behavior for comparison: new URL("/token", "https://example.com/admin?realm=test") produces https://example.com/token — the query string is dropped because /token is an absolute-path reference.