mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-12 02:52:36 +01:00
🛤️ fix: Base URL Fallback for Path-based OAuth Discovery in Token Refresh (#12164)
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Oreon Lothamer
GitHub
parent
ad5c51f62b
commit
eb6328c1d9
2 changed files with 191 additions and 20 deletions
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<typeof discoverAuthorizationServerMetadata> {
|
||||
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) {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue