From 3457f4e8a152335a059a4890ef721bc2f7e11b47 Mon Sep 17 00:00:00 2001 From: tt-a1i <53142663+tt-a1i@users.noreply.github.com> Date: Fri, 19 Jun 2026 02:34:01 +0800 Subject: [PATCH] fix(core): validate oauth expires_in values --- packages/core/src/mcp/oauth-provider.test.ts | 159 +++++++++++++++++++ packages/core/src/mcp/oauth-provider.ts | 66 ++++++-- 2 files changed, 215 insertions(+), 10 deletions(-) diff --git a/packages/core/src/mcp/oauth-provider.test.ts b/packages/core/src/mcp/oauth-provider.test.ts index 2edf45860f6..1d206709cc8 100644 --- a/packages/core/src/mcp/oauth-provider.test.ts +++ b/packages/core/src/mcp/oauth-provider.test.ts @@ -246,6 +246,54 @@ describe('MCPOAuthProvider', () => { ); }); + it('should preserve expires_in=0 as an immediate expiry', async () => { + vi.spyOn(Date, 'now').mockReturnValue(1_700_000_000_000); + + let callbackHandler: unknown; + vi.mocked(http.createServer).mockImplementation((handler) => { + callbackHandler = handler; + return mockHttpServer as unknown as http.Server; + }); + + mockHttpServer.listen.mockImplementation((port, callback) => { + callback?.(); + setTimeout(() => { + const mockReq = { + url: '/oauth/callback?code=auth_code_123&state=bW9ja19zdGF0ZV8xNl9ieXRlcw', + }; + const mockRes = { + writeHead: vi.fn(), + end: vi.fn(), + }; + (callbackHandler as (req: unknown, res: unknown) => void)( + mockReq, + mockRes, + ); + }, 10); + }); + + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/x-www-form-urlencoded', + text: 'access_token=access_token_123&token_type=Bearer&expires_in=0&refresh_token=refresh_token_456', + }), + ); + + const authProvider = new MCPOAuthProvider(); + const result = await authProvider.authenticate('test-server', mockConfig); + + expect(result.expiresAt).toBe(1_700_000_000_000); + const tokenStorage = new MCPOAuthTokenStorage(); + expect(tokenStorage.saveToken).toHaveBeenCalledWith( + 'test-server', + expect.objectContaining({ expiresAt: 1_700_000_000_000 }), + 'test-client-id', + 'https://auth.example.com/token', + undefined, + ); + }); + it('should handle OAuth discovery when no authorization URL provided', async () => { // Use a mutable config object const configWithoutAuth: MCPOAuthConfig = { @@ -785,6 +833,52 @@ describe('MCPOAuthProvider', () => { ); }); + it('should normalize JSON string expires_in values', async () => { + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify({ + access_token: 'new_access_token', + token_type: 'Bearer', + expires_in: '3600', + }), + }), + ); + + const authProvider = new MCPOAuthProvider(); + const result = await authProvider.refreshAccessToken( + mockConfig, + 'refresh_token', + 'https://auth.example.com/token', + ); + + expect(result.expires_in).toBe(3600); + }); + + it('should reject malformed JSON expires_in values', async () => { + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify({ + access_token: 'new_access_token', + token_type: 'Bearer', + expires_in: '3600abc', + }), + }), + ); + + const authProvider = new MCPOAuthProvider(); + await expect( + authProvider.refreshAccessToken( + mockConfig, + 'refresh_token', + 'https://auth.example.com/token', + ), + ).rejects.toThrow('Invalid expires_in value'); + }); + it('should include client secret in refresh request when available', async () => { mockFetch.mockResolvedValueOnce( createMockResponse({ @@ -827,6 +921,25 @@ describe('MCPOAuthProvider', () => { 'Token refresh failed: invalid_request - Invalid refresh token', ); }); + + it('should reject malformed form-urlencoded expires_in values', async () => { + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/x-www-form-urlencoded', + text: 'access_token=new_access_token&token_type=Bearer&expires_in=3600abc', + }), + ); + + const authProvider = new MCPOAuthProvider(); + await expect( + authProvider.refreshAccessToken( + mockConfig, + 'refresh_token', + 'https://auth.example.com/token', + ), + ).rejects.toThrow('Invalid expires_in value'); + }); }); describe('getValidToken', () => { @@ -901,6 +1014,52 @@ describe('MCPOAuthProvider', () => { ); }); + it('should preserve expires_in=0 when refreshing an expired token', async () => { + vi.spyOn(Date, 'now').mockReturnValue(1_700_000_000_000); + + const expiredCredentials = { + serverName: 'test-server', + token: { ...mockToken, expiresAt: Date.now() - 3600000 }, + clientId: 'test-client-id', + tokenUrl: 'https://auth.example.com/token', + updatedAt: Date.now(), + }; + + const tokenStorage = new MCPOAuthTokenStorage(); + vi.mocked(tokenStorage.getCredentials).mockResolvedValue( + expiredCredentials, + ); + vi.mocked(tokenStorage.isTokenExpired).mockReturnValue(true); + + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify({ + access_token: 'new_access_token', + token_type: 'Bearer', + expires_in: 0, + refresh_token: 'new_refresh_token', + }), + }), + ); + + const authProvider = new MCPOAuthProvider(); + const result = await authProvider.getValidToken( + 'test-server', + mockConfig, + ); + + expect(result).toBe('new_access_token'); + expect(tokenStorage.saveToken).toHaveBeenCalledWith( + 'test-server', + expect.objectContaining({ expiresAt: 1_700_000_000_000 }), + 'test-client-id', + 'https://auth.example.com/token', + undefined, + ); + }); + it('should return null when no credentials exist', async () => { const tokenStorage = new MCPOAuthTokenStorage(); vi.mocked(tokenStorage.getCredentials).mockResolvedValue(null); diff --git a/packages/core/src/mcp/oauth-provider.ts b/packages/core/src/mcp/oauth-provider.ts index ce92e97dac7..58726892807 100644 --- a/packages/core/src/mcp/oauth-provider.ts +++ b/packages/core/src/mcp/oauth-provider.ts @@ -80,6 +80,34 @@ export interface OAuthTokenResponse { scope?: string; } +function parseExpiresIn(value: unknown): number | undefined { + if (value === undefined || value === null) { + return undefined; + } + + const seconds = + typeof value === 'number' + ? value + : typeof value === 'string' && /^\d+$/.test(value.trim()) + ? Number(value.trim()) + : NaN; + + if (!Number.isSafeInteger(seconds) || seconds < 0) { + throw new Error(`Invalid expires_in value: ${String(value)}`); + } + + return seconds; +} + +function normalizeTokenResponse( + response: OAuthTokenResponse, +): OAuthTokenResponse { + return { + ...response, + expires_in: parseExpiresIn(response.expires_in), + }; +} + /** * Dynamic client registration request. */ @@ -496,8 +524,16 @@ export class MCPOAuthProvider { // Try to parse as JSON first, fall back to form-urlencoded try { - return JSON.parse(responseText) as OAuthTokenResponse; - } catch { + return normalizeTokenResponse( + JSON.parse(responseText) as OAuthTokenResponse, + ); + } catch (error) { + if ( + error instanceof Error && + error.message.startsWith('Invalid expires_in value') + ) { + throw error; + } // Parse form-urlencoded response const tokenParams = new URLSearchParams(responseText); const accessToken = tokenParams.get('access_token'); @@ -518,7 +554,7 @@ export class MCPOAuthProvider { return { access_token: accessToken, token_type: tokenType, - expires_in: expiresIn ? parseInt(expiresIn, 10) : undefined, + expires_in: parseExpiresIn(expiresIn), refresh_token: refreshToken || undefined, scope: scope || undefined, } as OAuthTokenResponse; @@ -618,8 +654,16 @@ export class MCPOAuthProvider { // Try to parse as JSON first, fall back to form-urlencoded try { - return JSON.parse(responseText) as OAuthTokenResponse; - } catch { + return normalizeTokenResponse( + JSON.parse(responseText) as OAuthTokenResponse, + ); + } catch (error) { + if ( + error instanceof Error && + error.message.startsWith('Invalid expires_in value') + ) { + throw error; + } // Parse form-urlencoded response const tokenParams = new URLSearchParams(responseText); const accessToken = tokenParams.get('access_token'); @@ -640,7 +684,7 @@ export class MCPOAuthProvider { return { access_token: accessToken, token_type: tokenType, - expires_in: expiresIn ? parseInt(expiresIn, 10) : undefined, + expires_in: parseExpiresIn(expiresIn), refresh_token: refreshToken || undefined, scope: scope || undefined, } as OAuthTokenResponse; @@ -866,8 +910,9 @@ export class MCPOAuthProvider { scope: tokenResponse.scope, }; - if (tokenResponse.expires_in) { - token.expiresAt = Date.now() + tokenResponse.expires_in * 1000; + const expiresIn = parseExpiresIn(tokenResponse.expires_in); + if (expiresIn !== undefined) { + token.expiresAt = Date.now() + expiresIn * 1000; } // Save token @@ -958,8 +1003,9 @@ export class MCPOAuthProvider { scope: newTokenResponse.scope || token.scope, }; - if (newTokenResponse.expires_in) { - newToken.expiresAt = Date.now() + newTokenResponse.expires_in * 1000; + const expiresIn = parseExpiresIn(newTokenResponse.expires_in); + if (expiresIn !== undefined) { + newToken.expiresAt = Date.now() + expiresIn * 1000; } await this.tokenStorage.saveToken(