From 90972ff4b6e95687ef438b286695dbf5b1dfcd12 Mon Sep 17 00:00:00 2001 From: Genmin Date: Thu, 30 Apr 2026 09:26:23 -0700 Subject: [PATCH] fix: validate oauth code redirect uri --- .changeset/fix-v1x-oauth-code-redirect.md | 5 ++ .../server/demoInMemoryOAuthProvider.ts | 19 ++++ src/server/auth/handlers/token.ts | 42 +++++---- src/server/auth/provider.ts | 17 ++++ .../server/demoInMemoryOAuthProvider.test.ts | 53 +++++++++++ test/server/auth/handlers/token.test.ts | 90 +++++++++++++++++++ 6 files changed, 210 insertions(+), 16 deletions(-) create mode 100644 .changeset/fix-v1x-oauth-code-redirect.md diff --git a/.changeset/fix-v1x-oauth-code-redirect.md b/.changeset/fix-v1x-oauth-code-redirect.md new file mode 100644 index 0000000000..f4bc7ee48e --- /dev/null +++ b/.changeset/fix-v1x-oauth-code-redirect.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/sdk': patch +--- + +Validate authorization code token requests against the original redirect URI when providers expose it, and allow providers to revoke tokens issued for replayed authorization codes. diff --git a/src/examples/server/demoInMemoryOAuthProvider.ts b/src/examples/server/demoInMemoryOAuthProvider.ts index 1abc040ce0..63072f9984 100644 --- a/src/examples/server/demoInMemoryOAuthProvider.ts +++ b/src/examples/server/demoInMemoryOAuthProvider.ts @@ -39,6 +39,7 @@ export class DemoInMemoryAuthProvider implements OAuthServerProvider { } >(); private tokens = new Map(); + private codeToTokens = new Map(); constructor(private validateResource?: (resource?: URL) => boolean) {} @@ -92,6 +93,11 @@ export class DemoInMemoryAuthProvider implements OAuthServerProvider { return codeData.params.codeChallenge; } + async redirectUriForAuthorizationCode(_client: OAuthClientInformationFull, authorizationCode: string): Promise { + const codeData = this.codes.get(authorizationCode); + return codeData?.params.redirectUri; + } + async exchangeAuthorizationCode( client: OAuthClientInformationFull, authorizationCode: string, @@ -125,6 +131,7 @@ export class DemoInMemoryAuthProvider implements OAuthServerProvider { }; this.tokens.set(token, tokenData); + this.codeToTokens.set(authorizationCode, [token]); return { access_token: token, @@ -143,6 +150,18 @@ export class DemoInMemoryAuthProvider implements OAuthServerProvider { throw new Error('Not implemented for example demo'); } + async revokeTokensForAuthorizationCode(_client: OAuthClientInformationFull, authorizationCode: string): Promise { + const tokens = this.codeToTokens.get(authorizationCode); + if (!tokens) { + return; + } + + for (const token of tokens) { + this.tokens.delete(token); + } + this.codeToTokens.delete(authorizationCode); + } + async verifyAccessToken(token: string): Promise { const tokenData = this.tokens.get(token); if (!tokenData || !tokenData.expiresAt || tokenData.expiresAt < Date.now()) { diff --git a/src/server/auth/handlers/token.ts b/src/server/auth/handlers/token.ts index 4cc4e8ab8b..cf22249244 100644 --- a/src/server/auth/handlers/token.ts +++ b/src/server/auth/handlers/token.ts @@ -96,24 +96,34 @@ export function tokenHandler({ provider, rateLimit: rateLimitConfig }: TokenHand const skipLocalPkceValidation = provider.skipLocalPkceValidation; - // Perform local PKCE validation unless explicitly skipped - // (e.g. to validate code_verifier in upstream server) - if (!skipLocalPkceValidation) { - const codeChallenge = await provider.challengeForAuthorizationCode(client, code); - if (!(await verifyChallenge(code_verifier, codeChallenge))) { - throw new InvalidGrantError('code_verifier does not match the challenge'); + try { + const originalRedirectUri = await provider.redirectUriForAuthorizationCode?.(client, code); + if (originalRedirectUri !== undefined && originalRedirectUri !== redirect_uri) { + throw new InvalidGrantError('redirect_uri does not match the authorization request'); } - } - // Passes the code_verifier to the provider if PKCE validation didn't occur locally - const tokens = await provider.exchangeAuthorizationCode( - client, - code, - skipLocalPkceValidation ? code_verifier : undefined, - redirect_uri, - resource ? new URL(resource) : undefined - ); - res.status(200).json(tokens); + // Perform local PKCE validation unless explicitly skipped + // (e.g. to validate code_verifier in upstream server) + if (!skipLocalPkceValidation) { + const codeChallenge = await provider.challengeForAuthorizationCode(client, code); + if (!(await verifyChallenge(code_verifier, codeChallenge))) { + throw new InvalidGrantError('code_verifier does not match the challenge'); + } + } + + // Passes the code_verifier to the provider if PKCE validation didn't occur locally + const tokens = await provider.exchangeAuthorizationCode( + client, + code, + skipLocalPkceValidation ? code_verifier : undefined, + redirect_uri, + resource ? new URL(resource) : undefined + ); + res.status(200).json(tokens); + } catch (error) { + await provider.revokeTokensForAuthorizationCode?.(client, code); + throw error; + } break; } diff --git a/src/server/auth/provider.ts b/src/server/auth/provider.ts index cf1c306def..90b28fd50c 100644 --- a/src/server/auth/provider.ts +++ b/src/server/auth/provider.ts @@ -34,6 +34,14 @@ export interface OAuthServerProvider { */ challengeForAuthorizationCode(client: OAuthClientInformationFull, authorizationCode: string): Promise; + /** + * Returns the `redirect_uri` that was used when the indicated authorization began, if available. + * + * When implemented, the token handler validates that the token request uses the same + * redirect URI, as required by RFC 6749 section 4.1.3. + */ + redirectUriForAuthorizationCode?(client: OAuthClientInformationFull, authorizationCode: string): Promise; + /** * Exchanges an authorization code for an access token. */ @@ -62,6 +70,15 @@ export interface OAuthServerProvider { */ revokeToken?(client: OAuthClientInformationFull, request: OAuthTokenRevocationRequest): Promise; + /** + * Revokes all tokens previously issued for the indicated authorization code. + * + * OAuth 2.1 section 4.1.3 recommends revoking previously issued tokens when + * authorization code reuse is detected. Providers that track code-to-token + * relationships can implement this hook to let the token handler trigger that cleanup. + */ + revokeTokensForAuthorizationCode?(client: OAuthClientInformationFull, authorizationCode: string): Promise; + /** * Whether to skip local PKCE validation. * diff --git a/test/examples/server/demoInMemoryOAuthProvider.test.ts b/test/examples/server/demoInMemoryOAuthProvider.test.ts index a49a8b426a..836f0950ba 100644 --- a/test/examples/server/demoInMemoryOAuthProvider.test.ts +++ b/test/examples/server/demoInMemoryOAuthProvider.test.ts @@ -137,6 +137,37 @@ describe('DemoInMemoryAuthProvider', () => { }); }); + describe('redirectUriForAuthorizationCode', () => { + const validClient: OAuthClientInformationFull = { + client_id: 'test-client', + client_secret: 'test-secret', + redirect_uris: ['https://example.com/callback'], + scope: 'test-scope' + }; + + const params: AuthorizationParams = { + redirectUri: 'https://example.com/callback', + state: 'test-state', + codeChallenge: 'test-challenge-value', + scopes: ['test-scope'] + }; + + it('returns the redirect URI for a valid authorization code', async () => { + await provider.authorize(validClient, params, mockResponse); + const code = new URL(mockResponse.getRedirectUrl()).searchParams.get('code')!; + + const redirectUri = await provider.redirectUriForAuthorizationCode(validClient, code); + + expect(redirectUri).toBe('https://example.com/callback'); + }); + + it('returns undefined for an invalid authorization code', async () => { + const redirectUri = await provider.redirectUriForAuthorizationCode(validClient, 'invalid-code'); + + expect(redirectUri).toBeUndefined(); + }); + }); + describe('exchangeAuthorizationCode', () => { const validClient: OAuthClientInformationFull = { client_id: 'test-client', @@ -211,6 +242,28 @@ describe('DemoInMemoryAuthProvider', () => { await expect(provider.exchangeAuthorizationCode(validClient, code)).rejects.toThrow('Invalid authorization code'); }); + it('revokes tokens issued for an authorization code', async () => { + const params: AuthorizationParams = { + redirectUri: 'https://example.com/callback', + state: 'test-state', + codeChallenge: 'test-challenge', + scopes: ['test-scope'] + }; + + await provider.authorize(validClient, params, mockResponse); + const code = new URL(mockResponse.getRedirectUrl()).searchParams.get('code')!; + + const tokens = await provider.exchangeAuthorizationCode(validClient, code); + await expect(provider.verifyAccessToken(tokens.access_token)).resolves.toMatchObject({ + token: tokens.access_token, + clientId: validClient.client_id + }); + + await provider.revokeTokensForAuthorizationCode(validClient, code); + + await expect(provider.verifyAccessToken(tokens.access_token)).rejects.toThrow('Invalid or expired token'); + }); + it('should validate resource when validateResource is provided', async () => { const validateResource = vi.fn().mockReturnValue(false); const strictProvider = new DemoInMemoryAuthProvider(validateResource); diff --git a/test/server/auth/handlers/token.test.ts b/test/server/auth/handlers/token.test.ts index 658142b4b5..fee70b2cd3 100644 --- a/test/server/auth/handlers/token.test.ts +++ b/test/server/auth/handlers/token.test.ts @@ -243,6 +243,96 @@ describe('Token Handler', () => { expect(response.body.error).toBe('invalid_grant'); }); + it('rejects redirect_uri mismatches when the provider exposes the original authorization redirect', async () => { + const mockStoredRedirectUri = vi.fn().mockResolvedValue('https://example.com/callback'); + const mockExchangeCode = vi.spyOn(mockProvider, 'exchangeAuthorizationCode'); + mockProvider.redirectUriForAuthorizationCode = mockStoredRedirectUri; + + const response = await supertest(app).post('/token').type('form').send({ + client_id: 'valid-client', + client_secret: 'valid-secret', + grant_type: 'authorization_code', + code: 'valid_code', + code_verifier: 'valid_verifier', + redirect_uri: 'https://attacker.example/callback' + }); + + expect(response.status).toBe(400); + expect(response.body.error).toBe('invalid_grant'); + expect(response.body.error_description).toContain('redirect_uri'); + expect(mockStoredRedirectUri).toHaveBeenCalledWith(validClient, 'valid_code'); + expect(mockExchangeCode).not.toHaveBeenCalled(); + }); + + it('rejects missing redirect_uri when the authorization request used one', async () => { + mockProvider.redirectUriForAuthorizationCode = vi.fn().mockResolvedValue('https://example.com/callback'); + + const response = await supertest(app).post('/token').type('form').send({ + client_id: 'valid-client', + client_secret: 'valid-secret', + grant_type: 'authorization_code', + code: 'valid_code', + code_verifier: 'valid_verifier' + }); + + expect(response.status).toBe(400); + expect(response.body.error).toBe('invalid_grant'); + expect(response.body.error_description).toContain('redirect_uri'); + }); + + it('allows matching redirect_uri when the provider exposes the original authorization redirect', async () => { + mockProvider.redirectUriForAuthorizationCode = vi.fn().mockResolvedValue('https://example.com/callback'); + + const response = await supertest(app).post('/token').type('form').send({ + client_id: 'valid-client', + client_secret: 'valid-secret', + grant_type: 'authorization_code', + code: 'valid_code', + code_verifier: 'valid_verifier', + redirect_uri: 'https://example.com/callback' + }); + + expect(response.status).toBe(200); + expect(response.body.access_token).toBe('mock_access_token'); + }); + + it('revokes tokens for the presented authorization code when code exchange fails', async () => { + const mockRevokeCodeTokens = vi.fn().mockResolvedValue(undefined); + mockProvider.revokeTokensForAuthorizationCode = mockRevokeCodeTokens; + mockProvider.exchangeAuthorizationCode = vi + .fn() + .mockRejectedValue(new InvalidGrantError('Authorization code was already used')); + + const response = await supertest(app).post('/token').type('form').send({ + client_id: 'valid-client', + client_secret: 'valid-secret', + grant_type: 'authorization_code', + code: 'valid_code', + code_verifier: 'valid_verifier' + }); + + expect(response.status).toBe(400); + expect(response.body.error).toBe('invalid_grant'); + expect(mockRevokeCodeTokens).toHaveBeenCalledWith(validClient, 'valid_code'); + }); + + it('revokes tokens for the presented authorization code when local code validation fails', async () => { + const mockRevokeCodeTokens = vi.fn().mockResolvedValue(undefined); + mockProvider.revokeTokensForAuthorizationCode = mockRevokeCodeTokens; + + const response = await supertest(app).post('/token').type('form').send({ + client_id: 'valid-client', + client_secret: 'valid-secret', + grant_type: 'authorization_code', + code: 'expired_code', + code_verifier: 'valid_verifier' + }); + + expect(response.status).toBe(400); + expect(response.body.error).toBe('invalid_grant'); + expect(mockRevokeCodeTokens).toHaveBeenCalledWith(validClient, 'expired_code'); + }); + it('returns tokens for valid code exchange', async () => { const mockExchangeCode = vi.spyOn(mockProvider, 'exchangeAuthorizationCode'); const response = await supertest(app).post('/token').type('form').send({