fix(client): preserve authorization server subpath in fallback URLs#1832
fix(client): preserve authorization server subpath in fallback URLs#1832claygeo wants to merge 4 commits intomodelcontextprotocol:mainfrom
Conversation
When AS metadata discovery fails and the authorization server has a non-root path (e.g., https://example.com/admin), the fallback URL construction used `new URL('/authorize', serverUrl)` which silently discards the path, producing https://example.com/authorize instead of https://example.com/admin/authorize. Adds buildFallbackUrl() helper that appends the endpoint to the existing pathname. Applied to all three fallback locations: /authorize, /token, and /register. Closes modelcontextprotocol#1716
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * `https://example.com/admin/authorize` (path preserved). | ||
| */ | ||
| function buildFallbackUrl(authorizationServerUrl: string | URL, endpoint: string): URL { | ||
| const url = typeof authorizationServerUrl === 'string' ? new URL(authorizationServerUrl) : new URL(authorizationServerUrl.href); | ||
| const basePath = url.pathname.endsWith('/') ? url.pathname.slice(0, -1) : url.pathname; | ||
| url.pathname = `${basePath}${endpoint}`; | ||
| return url; | ||
| } |
There was a problem hiding this comment.
🔴 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:
- startAuthorization (line 1387): buildFallbackUrl(authorizationServerUrl, '/authorize')
- executeTokenRequest (line 1469): buildFallbackUrl(authorizationServerUrl, '/token')
- registerClient (line 1716): buildFallbackUrl(authorizationServerUrl, '/register')
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
- An RFC 9728 protected resource metadata document returns: { "authorization_servers": ["https://example.com/admin?realm=test"] }
- discoverOAuthServerInfo sets authorizationServerUrl = "https://example.com/admin?realm=test"
- AS metadata discovery fails (or returns no endpoints), so the fallback path is taken.
- buildFallbackUrl("https://example.com/admin?realm=test", "/token") is called.
- url = new URL("https://example.com/admin?realm=test") — url.search is "?realm=test"
- url.pathname is set to /admin/token — url.search is untouched.
- The function returns https://example.com/admin/token?realm=test.
- The token request is sent to that URL; the server sees the unexpected realm=test parameter and may reject it.
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.
| ); | ||
| }); | ||
|
|
||
| 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' | ||
| }) |
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 registrationUrl = buildFallbackUrl(authorizationServerUrl, '/register') is only reached when metadata is undefined. There is no test that exercises this path with a non-root authorizationServerUrl such as https://auth.example.com/admin, which would verify that the resulting registrationUrl.pathname is /admin/register.
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 new URL('/register', authorizationServerUrl) would not be caught by the current test suite.
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 registrationUrl = new URL('/register', authorizationServerUrl) instead of buildFallbackUrl. With authorizationServerUrl = 'https://auth.example.com/admin', this would silently produce https://auth.example.com/register (subpath lost), sending dynamic registration requests to a nonexistent endpoint. No test in the current suite would catch this.
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.
2 participants
Summary
Closes #1716
When AS metadata discovery fails and the authorization server has a non-root path (e.g.,
https://example.com/admin), the fallback URL construction silently discards the path prefix, redirecting users to a nonexistent endpoint.Before:
new URL('/authorize', 'https://example.com/admin')→https://example.com/authorizeAfter:
buildFallbackUrl('https://example.com/admin', '/authorize')→https://example.com/admin/authorizeChanges
packages/client/src/client/auth.tsbuildFallbackUrl()helper that appends the endpoint path to the server's existing pathname/authorize(startAuthorization),/token(executeTokenRequest),/register(registerClient)Test plan
/authorizeURL/tokenURL