From eb6328c1d980274b4db6d6af1b5184ec67d56636 Mon Sep 17 00:00:00 2001 From: Oreon Lothamer <73498677+oreonl@users.noreply.github.com> Date: Tue, 10 Mar 2026 09:04:35 -1000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A4=EF=B8=8F=20fix:=20Base=20URL=20Fal?= =?UTF-8?q?lback=20for=20Path-based=20OAuth=20Discovery=20in=20Token=20Ref?= =?UTF-8?q?resh=20(#12164)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add base URL fallback for path-based OAuth discovery in token refresh The two `refreshOAuthTokens` paths in `MCPOAuthHandler` were missing the origin-URL fallback that `initiateOAuthFlow` already had. With MCP SDK 1.27.1, `buildDiscoveryUrls` appends the server path to the `.well-known` URL (e.g. `/.well-known/oauth-authorization-server/mcp`), which returns 404 for servers like Sentry that only expose the root discovery endpoint (`/.well-known/oauth-authorization-server`). Without the fallback, discovery returns null during refresh, the token endpoint resolves to the wrong URL, and users are prompted to re-authenticate every time their access token expires instead of the refresh token being exchanged silently. Both refresh paths now mirror the `initiateOAuthFlow` pattern: if discovery fails and the server URL has a non-root path, retry with just the origin URL. Co-Authored-By: Claude Sonnet 4.6 * refactor: extract discoverWithOriginFallback helper; add tests Extract the duplicated path-based URL retry logic from both `refreshOAuthTokens` branches into a single private static helper `discoverWithOriginFallback`, reducing the risk of the two paths drifting in the future. Add three tests covering the new behaviour: - stored clientInfo path: asserts discovery is called twice (path then origin) and that the token endpoint from the origin discovery is used - auto-discovered path: same assertions for the branchless path - root URL: asserts discovery is called only once when the server URL already has no path component Co-Authored-By: Claude Sonnet 4.6 * refactor: use discoverWithOriginFallback in discoverMetadata too Remove the inline duplicate of the origin-fallback logic from `discoverMetadata` and replace it with a call to the shared `discoverWithOriginFallback` helper, giving all three discovery sites a single implementation. Co-Authored-By: Claude Sonnet 4.6 * test: use mock.calls + .href/.toString() for URL assertions Replace brittle `toHaveBeenNthCalledWith(new URL(...))` comparisons with `expect.any(URL)` matchers and explicit `.href`/`.toString()` checks on the captured call args, consistent with the existing mock.calls pattern used throughout handler.test.ts. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- .../api/src/mcp/__tests__/handler.test.ts | 162 ++++++++++++++++++ packages/api/src/mcp/oauth/handler.ts | 49 +++--- 2 files changed, 191 insertions(+), 20 deletions(-) diff --git a/packages/api/src/mcp/__tests__/handler.test.ts b/packages/api/src/mcp/__tests__/handler.test.ts index db88afe581..e5d94b23e3 100644 --- a/packages/api/src/mcp/__tests__/handler.test.ts +++ b/packages/api/src/mcp/__tests__/handler.test.ts @@ -1439,5 +1439,167 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { }), ); }); + + describe('path-based URL origin fallback', () => { + it('retries with origin URL when path-based discovery fails (stored clientInfo path)', async () => { + const metadata = { + serverName: 'sentry', + serverUrl: 'https://mcp.sentry.dev/mcp', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + grant_types: ['authorization_code', 'refresh_token'], + }, + }; + + const originMetadata = { + issuer: 'https://mcp.sentry.dev/', + authorization_endpoint: 'https://mcp.sentry.dev/oauth/authorize', + token_endpoint: 'https://mcp.sentry.dev/oauth/token', + token_endpoint_auth_methods_supported: ['client_secret_post'], + response_types_supported: ['code'], + jwks_uri: 'https://mcp.sentry.dev/.well-known/jwks.json', + subject_types_supported: ['public'], + id_token_signing_alg_values_supported: ['RS256'], + } as AuthorizationServerMetadata; + + // First call (path-based URL) fails, second call (origin URL) succeeds + mockDiscoverAuthorizationServerMetadata + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(originMetadata); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + const result = await MCPOAuthHandler.refreshOAuthTokens( + 'test-refresh-token', + metadata, + {}, + {}, + ); + + // Discovery attempted twice: once with path URL, once with origin URL + expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenCalledTimes(2); + expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenNthCalledWith( + 1, + expect.any(URL), + expect.any(Object), + ); + expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenNthCalledWith( + 2, + expect.any(URL), + expect.any(Object), + ); + const firstDiscoveryUrl = mockDiscoverAuthorizationServerMetadata.mock.calls[0][0] as URL; + const secondDiscoveryUrl = mockDiscoverAuthorizationServerMetadata.mock.calls[1][0] as URL; + expect(firstDiscoveryUrl).toBeInstanceOf(URL); + expect(firstDiscoveryUrl.href).toBe('https://mcp.sentry.dev/mcp'); + expect(secondDiscoveryUrl).toBeInstanceOf(URL); + expect(secondDiscoveryUrl.href).toBe('https://mcp.sentry.dev/'); + + // Token endpoint from origin discovery metadata is used + expect(mockFetch).toHaveBeenCalled(); + const [fetchUrl, fetchOptions] = mockFetch.mock.calls[0]; + expect(String(fetchUrl)).toBe('https://mcp.sentry.dev/oauth/token'); + expect(fetchOptions).toEqual(expect.objectContaining({ method: 'POST' })); + expect(result.access_token).toBe('new-access-token'); + }); + + it('retries with origin URL when path-based discovery fails (auto-discovered path)', async () => { + // No clientInfo — uses the auto-discovered branch + const metadata = { + serverName: 'sentry', + serverUrl: 'https://mcp.sentry.dev/mcp', + }; + + const originMetadata = { + issuer: 'https://mcp.sentry.dev/', + authorization_endpoint: 'https://mcp.sentry.dev/oauth/authorize', + token_endpoint: 'https://mcp.sentry.dev/oauth/token', + response_types_supported: ['code'], + jwks_uri: 'https://mcp.sentry.dev/.well-known/jwks.json', + subject_types_supported: ['public'], + id_token_signing_alg_values_supported: ['RS256'], + } as AuthorizationServerMetadata; + + // First call (path-based URL) fails, second call (origin URL) succeeds + mockDiscoverAuthorizationServerMetadata + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce(originMetadata); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + const result = await MCPOAuthHandler.refreshOAuthTokens( + 'test-refresh-token', + metadata, + {}, + {}, + ); + + // Discovery attempted twice: once with path URL, once with origin URL + expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenCalledTimes(2); + expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenNthCalledWith( + 1, + expect.any(URL), + expect.any(Object), + ); + expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenNthCalledWith( + 2, + expect.any(URL), + expect.any(Object), + ); + const firstDiscoveryUrl = mockDiscoverAuthorizationServerMetadata.mock.calls[0][0] as URL; + const secondDiscoveryUrl = mockDiscoverAuthorizationServerMetadata.mock.calls[1][0] as URL; + expect(firstDiscoveryUrl).toBeInstanceOf(URL); + expect(firstDiscoveryUrl.href).toBe('https://mcp.sentry.dev/mcp'); + expect(secondDiscoveryUrl).toBeInstanceOf(URL); + expect(secondDiscoveryUrl.href).toBe('https://mcp.sentry.dev/'); + + // Token endpoint from origin discovery metadata is used + expect(mockFetch).toHaveBeenCalled(); + const [fetchUrl, fetchOptions] = mockFetch.mock.calls[0]; + expect(fetchUrl).toBeInstanceOf(URL); + expect(fetchUrl.toString()).toBe('https://mcp.sentry.dev/oauth/token'); + expect(fetchOptions).toEqual(expect.objectContaining({ method: 'POST' })); + expect(result.access_token).toBe('new-access-token'); + }); + + it('does not retry with origin when server URL has no path (root URL)', async () => { + const metadata = { + serverName: 'test-server', + serverUrl: 'https://auth.example.com/', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + }, + }; + + // Root URL discovery fails — no retry + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce(undefined); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ access_token: 'new-token', expires_in: 3600 }), + } as Response); + + await MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}); + + // Only one discovery attempt for a root URL + expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenCalledTimes(1); + }); + }); }); }); diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index 92b9f1211c..6ef444bf47 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -161,20 +161,7 @@ export class MCPOAuthHandler { logger.debug( `[MCPOAuth] Discovering OAuth metadata from ${sanitizeUrlForLogging(authServerUrl)}`, ); - let rawMetadata = await discoverAuthorizationServerMetadata(authServerUrl, { - fetchFn, - }); - - // If discovery failed and we're using a path-based URL, try the base URL - if (!rawMetadata && authServerUrl.pathname !== '/') { - const baseUrl = new URL(authServerUrl.origin); - logger.debug( - `[MCPOAuth] Discovery failed with path, trying base URL: ${sanitizeUrlForLogging(baseUrl)}`, - ); - rawMetadata = await discoverAuthorizationServerMetadata(baseUrl, { - fetchFn, - }); - } + const rawMetadata = await this.discoverWithOriginFallback(authServerUrl, fetchFn); if (!rawMetadata) { /** @@ -221,6 +208,27 @@ export class MCPOAuthHandler { }; } + /** + * Discovers OAuth authorization server metadata with origin-URL fallback. + * If discovery fails for a path-based URL, retries with just the origin. + * Mirrors the fallback behavior in `discoverMetadata` and `initiateOAuthFlow`. + */ + private static async discoverWithOriginFallback( + serverUrl: URL, + fetchFn: FetchLike, + ): ReturnType { + const metadata = await discoverAuthorizationServerMetadata(serverUrl, { fetchFn }); + // If discovery failed and we're using a path-based URL, try the base URL + if (!metadata && serverUrl.pathname !== '/') { + const baseUrl = new URL(serverUrl.origin); + logger.debug( + `[MCPOAuth] Discovery failed with path, trying base URL: ${sanitizeUrlForLogging(baseUrl)}`, + ); + return discoverAuthorizationServerMetadata(baseUrl, { fetchFn }); + } + return metadata; + } + /** * Registers an OAuth client dynamically */ @@ -735,9 +743,10 @@ export class MCPOAuthHandler { throw new Error('No token URL available for refresh'); } else { /** Auto-discover OAuth configuration for refresh */ - const oauthMetadata = await discoverAuthorizationServerMetadata(metadata.serverUrl, { - fetchFn: this.createOAuthFetch(oauthHeaders), - }); + const serverUrl = new URL(metadata.serverUrl); + const fetchFn = this.createOAuthFetch(oauthHeaders); + const oauthMetadata = await this.discoverWithOriginFallback(serverUrl, fetchFn); + if (!oauthMetadata) { /** * No metadata discovered - use fallback /token endpoint. @@ -911,9 +920,9 @@ export class MCPOAuthHandler { } /** Auto-discover OAuth configuration for refresh */ - const oauthMetadata = await discoverAuthorizationServerMetadata(metadata.serverUrl, { - fetchFn: this.createOAuthFetch(oauthHeaders), - }); + const serverUrl = new URL(metadata.serverUrl); + const fetchFn = this.createOAuthFetch(oauthHeaders); + const oauthMetadata = await this.discoverWithOriginFallback(serverUrl, fetchFn); let tokenUrl: URL; if (!oauthMetadata?.token_endpoint) {