diff --git a/.changeset/mcp-oauth-stale-redirect-registration.md b/.changeset/mcp-oauth-stale-redirect-registration.md new file mode 100644 index 0000000000..4438993d90 --- /dev/null +++ b/.changeset/mcp-oauth-stale-redirect-registration.md @@ -0,0 +1,5 @@ +--- +'@moonshot-ai/kimi-code': patch +--- + +Fixed MCP OAuth re-authorization always failing with "Invalid redirect URI": the OAuth callback listener binds a random port per flow, but the dynamic client registration recorded the first flow's port, so every later interactive authorization was rejected at the authorization endpoint. A stale registration is now dropped automatically and the flow re-registers with the current callback URI. diff --git a/packages/agent-core-v2/src/mcpCore/oauth/provider.ts b/packages/agent-core-v2/src/mcpCore/oauth/provider.ts index 20c63cfb05..58b1928bf5 100644 --- a/packages/agent-core-v2/src/mcpCore/oauth/provider.ts +++ b/packages/agent-core-v2/src/mcpCore/oauth/provider.ts @@ -14,6 +14,13 @@ * The provider does not open browsers or run servers — it is the * persistence + flow-state shim. * + * `invalidateStaleRegistration` guards interactive flows: the callback + * listener binds a random port per flow while a DCR registration pins the + * redirect URIs of the flow that created it, so a reused registration whose + * URIs no longer cover the current callback would be rejected at the + * authorization endpoint ("invalid redirect URI", rendered only in the + * user's browser). Dropping it lets `auth()` re-register. + * * `clientName` is the product token for the default label * (` ()`), carrying the configured custom identity; it * is ignored when `clientLabel` states the whole label explicitly. @@ -171,6 +178,17 @@ export class McpOAuthClientProvider implements OAuthClientProvider { return this.discoveryCache; } + async invalidateStaleRegistration(redirectUri: string): Promise { + await this.ready; + const info = this.clientCache; + if (info === undefined || !('redirect_uris' in info)) return false; + const uris = info.redirect_uris; + if (!Array.isArray(uris) || uris.length === 0) return false; + if (uris.includes(redirectUri)) return false; + await this.invalidateCredentials('client'); + return true; + } + async invalidateCredentials( scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery', ): Promise { diff --git a/packages/agent-core-v2/src/mcpCore/oauth/service.ts b/packages/agent-core-v2/src/mcpCore/oauth/service.ts index c2972609fd..0e27e0fbee 100644 --- a/packages/agent-core-v2/src/mcpCore/oauth/service.ts +++ b/packages/agent-core-v2/src/mcpCore/oauth/service.ts @@ -111,6 +111,7 @@ export class McpOAuthService { provider.setRedirectUrl(new URL(callbackServer.redirectUri)); await provider.ready; + await provider.invalidateStaleRegistration(callbackServer.redirectUri); let authorizationUrl: URL | undefined; try { diff --git a/packages/agent-core-v2/test/mcpCore/oauth/store.test.ts b/packages/agent-core-v2/test/mcpCore/oauth/store.test.ts index b0d78e8dbc..35100c50bd 100644 --- a/packages/agent-core-v2/test/mcpCore/oauth/store.test.ts +++ b/packages/agent-core-v2/test/mcpCore/oauth/store.test.ts @@ -113,3 +113,46 @@ function token(accessToken: string): OAuthTokens { token_type: 'Bearer', }; } + +describe('McpOAuthClientProvider.invalidateStaleRegistration', () => { + function makeProvider() { + return new McpOAuthClientProvider({ + serverName: 'srv', + serverUrl: 'https://mcp.example.com/mcp', + store: createMemoryMcpOAuthStore(), + }); + } + + const registration: OAuthClientInformationFull = { + client_id: 'c1', + redirect_uris: ['http://127.0.0.1:11111/callback'], + }; + + it('drops a registration whose redirect_uris miss the current callback', async () => { + const provider = makeProvider(); + await provider.ready; + await provider.saveClientInformation(registration); + await expect( + provider.invalidateStaleRegistration('http://127.0.0.1:22222/callback'), + ).resolves.toBe(true); + await expect(provider.clientInformation()).resolves.toBeUndefined(); + }); + + it('keeps a registration that still covers the callback URI', async () => { + const provider = makeProvider(); + await provider.ready; + await provider.saveClientInformation(registration); + await expect( + provider.invalidateStaleRegistration('http://127.0.0.1:11111/callback'), + ).resolves.toBe(false); + await expect(provider.clientInformation()).resolves.toMatchObject({ client_id: 'c1' }); + }); + + it('is a no-op without a stored registration', async () => { + const provider = makeProvider(); + await provider.ready; + await expect( + provider.invalidateStaleRegistration('http://127.0.0.1:11111/callback'), + ).resolves.toBe(false); + }); +}); diff --git a/packages/agent-core/src/mcp/oauth/provider.ts b/packages/agent-core/src/mcp/oauth/provider.ts index be90113e70..7dd3828ba4 100644 --- a/packages/agent-core/src/mcp/oauth/provider.ts +++ b/packages/agent-core/src/mcp/oauth/provider.ts @@ -153,6 +153,29 @@ export class McpOAuthClientProvider implements OAuthClientProvider { return this.store.read(`${this.storeKey}${DISCOVERY_SUFFIX}`); } + /** + * Drop the persisted DCR client registration when its `redirect_uris` no + * longer cover `redirectUri`. Returns true when a stale registration was + * dropped. + * + * The callback listener binds a random port per flow, while a DCR + * registration pins the redirect URIs of the flow that created it. Reusing + * a registration whose URIs no longer match guarantees an + * "invalid redirect URI" rejection at the authorization endpoint — rendered + * only in the user's browser, while this client waits for a callback that + * never comes. Dropping the registration lets the next `auth()` call + * re-register with the current callback URI. + */ + invalidateStaleRegistration(redirectUri: string): boolean { + const info = this.clientInformation(); + if (info === undefined || !('redirect_uris' in info)) return false; + const uris = info.redirect_uris; + if (!Array.isArray(uris) || uris.length === 0) return false; + if (uris.includes(redirectUri)) return false; + this.invalidateCredentials('client'); + return true; + } + invalidateCredentials(scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery'): void { if (scope === 'verifier') { this._codeVerifier = undefined; diff --git a/packages/agent-core/src/mcp/oauth/service.ts b/packages/agent-core/src/mcp/oauth/service.ts index 9b2f807de6..ed7ba626c2 100644 --- a/packages/agent-core/src/mcp/oauth/service.ts +++ b/packages/agent-core/src/mcp/oauth/service.ts @@ -126,6 +126,10 @@ export class McpOAuthService { } provider.setRedirectUrl(new URL(callbackServer.redirectUri)); + // See invalidateStaleRegistration: a reused registration whose redirect + // URIs no longer cover this flow's random-port callback would be rejected + // at the authorization endpoint with an error only the browser ever sees. + provider.invalidateStaleRegistration(callbackServer.redirectUri); let authorizationUrl: URL | undefined; try { diff --git a/packages/agent-core/test/mcp/oauth-store.test.ts b/packages/agent-core/test/mcp/oauth-store.test.ts index bba389755a..def46df4dc 100644 --- a/packages/agent-core/test/mcp/oauth-store.test.ts +++ b/packages/agent-core/test/mcp/oauth-store.test.ts @@ -164,3 +164,47 @@ function token(accessToken: string): OAuthTokens { token_type: 'Bearer', }; } + +describe('McpOAuthClientProvider.invalidateStaleRegistration', () => { + let dir: string; + + beforeEach(async () => { + dir = await mkdtemp(join(tmpdir(), 'kimi-mcp-oauth-stale-')); + }); + afterEach(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + function makeProvider() { + return new McpOAuthClientProvider({ + serverName: 'srv', + serverUrl: 'https://mcp.example.com/mcp', + store: new JsonFileStore(dir), + }); + } + + it('drops a registration whose redirect_uris miss the current callback', () => { + const provider = makeProvider(); + provider.saveClientInformation({ + client_id: 'c1', + redirect_uris: ['http://127.0.0.1:11111/callback'], + }); + expect(provider.invalidateStaleRegistration('http://127.0.0.1:22222/callback')).toBe(true); + expect(provider.clientInformation()).toBeUndefined(); + }); + + it('keeps a registration that still covers the callback URI', () => { + const provider = makeProvider(); + provider.saveClientInformation({ + client_id: 'c1', + redirect_uris: ['http://127.0.0.1:11111/callback'], + }); + expect(provider.invalidateStaleRegistration('http://127.0.0.1:11111/callback')).toBe(false); + expect(provider.clientInformation()).toMatchObject({ client_id: 'c1' }); + }); + + it('is a no-op without a stored registration', () => { + const provider = makeProvider(); + expect(provider.invalidateStaleRegistration('http://127.0.0.1:11111/callback')).toBe(false); + }); +});