Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/mcp-oauth-stale-redirect-registration.md
Original file line number Diff line number Diff line change
@@ -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.
18 changes: 18 additions & 0 deletions packages/agent-core-v2/src/mcpCore/oauth/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
* (`<clientName> (<serverName>)`), carrying the configured custom identity; it
* is ignored when `clientLabel` states the whole label explicitly.
Expand Down Expand Up @@ -171,6 +178,17 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
return this.discoveryCache;
}

async invalidateStaleRegistration(redirectUri: string): Promise<boolean> {
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<void> {
Expand Down
1 change: 1 addition & 0 deletions packages/agent-core-v2/src/mcpCore/oauth/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ export class McpOAuthService {

provider.setRedirectUrl(new URL(callbackServer.redirectUri));
await provider.ready;
await provider.invalidateStaleRegistration(callbackServer.redirectUri);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep the existing client registration through refresh

Dropping the stored DCR record before auth() runs removes the old clientInformation that the SDK uses for the refresh-token branch. On servers that bind refresh tokens to the original client_id, refreshAuthorization() will now fail with invalid_grant/invalid_client; the SDK re-throws those OAuth errors instead of falling through to the browser flow, so a user who still had a usable refresh token can lose reauth entirely. The same pattern is applied in the v1 service.

Useful? React with 👍 / 👎.


let authorizationUrl: URL | undefined;
try {
Expand Down
43 changes: 43 additions & 0 deletions packages/agent-core-v2/test/mcpCore/oauth/store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
23 changes: 23 additions & 0 deletions packages/agent-core/src/mcp/oauth/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,29 @@ export class McpOAuthClientProvider implements OAuthClientProvider {
return this.store.read<OAuthDiscoveryState>(`${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;
Expand Down
4 changes: 4 additions & 0 deletions packages/agent-core/src/mcp/oauth/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
44 changes: 44 additions & 0 deletions packages/agent-core/test/mcp/oauth-store.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
Loading