From 5b44fe68b11ac7dd3ec19b564e5846f05a09d840 Mon Sep 17 00:00:00 2001 From: jubnl Date: Thu, 9 Apr 2026 23:47:53 +0200 Subject: [PATCH] fix(mcp): narrow OAuth scope to allowed intersection instead of rejecting When a client requests scopes it is not permitted for, silently drop them rather than failing the entire authorization flow. The token is issued with only the intersection of requested and allowed scopes. Also fix /authorize/validate to always return HTTP 200 so the consent page can surface the actual error_description instead of a generic axios failure message. --- server/src/routes/oauth.ts | 4 -- server/src/services/oauthService.ts | 14 ++++--- server/tests/integration/oauth.test.ts | 56 +++++++++++++++++++------- 3 files changed, 50 insertions(+), 24 deletions(-) diff --git a/server/src/routes/oauth.ts b/server/src/routes/oauth.ts index 0eb2a170..2739b07a 100644 --- a/server/src/routes/oauth.ts +++ b/server/src/routes/oauth.ts @@ -167,10 +167,6 @@ oauthApiRouter.get('/authorize/validate', (req: Request, res: Response) => { userId, ); - if (!result.valid) { - return res.status(400).json(result); - } - return res.json(result); }); diff --git a/server/src/services/oauthService.ts b/server/src/services/oauthService.ts index 224dbbfe..a9a34244 100644 --- a/server/src/services/oauthService.ts +++ b/server/src/services/oauthService.ts @@ -425,27 +425,29 @@ export function validateAuthorizeRequest( } const allowedScopes: string[] = JSON.parse(client.allowed_scopes); - const disallowed = requestedScopes.filter(s => !allowedScopes.includes(s)); - if (disallowed.length > 0) { - return { valid: false, error: 'invalid_scope', error_description: `Scopes not permitted for this client: ${disallowed.join(', ')}` }; + // Narrow to the intersection: drop scopes the client isn't permitted for rather + // than rejecting the whole request (per OAuth 2.0 §3.3 scope narrowing). + const grantedScopes = requestedScopes.filter(s => allowedScopes.includes(s)); + if (grantedScopes.length === 0) { + return { valid: false, error: 'invalid_scope', error_description: 'None of the requested scopes are permitted for this client' }; } if (userId === null) { return { valid: true, client: { name: client.name, allowed_scopes: allowedScopes }, - scopes: requestedScopes, + scopes: grantedScopes, loginRequired: true, }; } const existingConsent = getConsent(params.client_id, userId); - const consentRequired = !existingConsent || !isConsentSufficient(existingConsent, requestedScopes); + const consentRequired = !existingConsent || !isConsentSufficient(existingConsent, grantedScopes); return { valid: true, client: { name: client.name, allowed_scopes: allowedScopes }, - scopes: requestedScopes, + scopes: grantedScopes, consentRequired, }; } diff --git a/server/tests/integration/oauth.test.ts b/server/tests/integration/oauth.test.ts index 635c7443..a76a7850 100644 --- a/server/tests/integration/oauth.test.ts +++ b/server/tests/integration/oauth.test.ts @@ -471,40 +471,44 @@ describe('POST /oauth/revoke', () => { // ───────────────────────────────────────────────────────────────────────────── describe('GET /api/oauth/authorize/validate', () => { - it('OAUTH-019 — returns 400 when MCP addon disabled', async () => { + it('OAUTH-019 — returns 200 with valid:false when MCP addon disabled', async () => { isAddonEnabledMock.mockReturnValue(false); const res = await request(app) .get('/api/oauth/authorize/validate') .query({ response_type: 'code', client_id: 'x', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: 'c', code_challenge_method: 'S256' }); - expect(res.status).toBe(400); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); expect(res.body.error).toBe('mcp_disabled'); }); - it('OAUTH-020 — returns 400 for wrong response_type', async () => { + it('OAUTH-020 — returns 200 with valid:false for wrong response_type', async () => { const res = await request(app) .get('/api/oauth/authorize/validate') .query({ response_type: 'token', client_id: 'x', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: 'c', code_challenge_method: 'S256' }); - expect(res.status).toBe(400); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); expect(res.body.error).toBe('unsupported_response_type'); }); - it('OAUTH-021 — returns 400 for missing PKCE', async () => { + it('OAUTH-021 — returns 200 with valid:false for missing PKCE', async () => { const res = await request(app) .get('/api/oauth/authorize/validate') .query({ response_type: 'code', client_id: 'x', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read' }); - expect(res.status).toBe(400); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); expect(res.body.error).toBe('invalid_request'); }); - it('OAUTH-022 — returns 400 for unknown client_id', async () => { + it('OAUTH-022 — returns 200 with valid:false for unknown client_id', async () => { const res = await request(app) .get('/api/oauth/authorize/validate') .query({ response_type: 'code', client_id: 'unknown-client', redirect_uri: 'https://r.example.com/cb', scope: 'trips:read', code_challenge: 'abc', code_challenge_method: 'S256' }); - expect(res.status).toBe(400); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); expect(res.body.error).toBe('invalid_client'); }); - it('OAUTH-023 — returns 400 for mismatched redirect_uri', async () => { + it('OAUTH-023 — returns 200 with valid:false for mismatched redirect_uri', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); @@ -518,11 +522,12 @@ describe('GET /api/oauth/authorize/validate', () => { code_challenge: 'abc', code_challenge_method: 'S256', }); - expect(res.status).toBe(400); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); expect(res.body.error).toBe('invalid_redirect_uri'); }); - it('OAUTH-024 — returns 400 for empty scope', async () => { + it('OAUTH-024 — returns 200 with valid:false for empty scope', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); @@ -536,11 +541,32 @@ describe('GET /api/oauth/authorize/validate', () => { code_challenge: 'abc', code_challenge_method: 'S256', }); - expect(res.status).toBe(400); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); expect(res.body.error).toBe('invalid_scope'); }); - it('OAUTH-025 — returns 400 for scope not in client allowed_scopes', async () => { + it('OAUTH-025a — narrows scope to allowed intersection when client lacks some requested scopes', async () => { + const { user } = createUser(testDb); + const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); + + const res = await request(app) + .get('/api/oauth/authorize/validate') + .query({ + response_type: 'code', + client_id: r.client!.client_id, + redirect_uri: 'https://app.example.com/cb', + scope: 'trips:read trips:delete', + code_challenge: 'abc', + code_challenge_method: 'S256', + }); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(true); + // trips:delete was dropped — only trips:read granted + expect(res.body.scopes).toEqual(['trips:read']); + }); + + it('OAUTH-025b — returns 200 with valid:false when no requested scope is allowed', async () => { const { user } = createUser(testDb); const r = createOAuthClient(user.id, 'App', ['https://app.example.com/cb'], ['trips:read']); @@ -554,7 +580,8 @@ describe('GET /api/oauth/authorize/validate', () => { code_challenge: 'abc', code_challenge_method: 'S256', }); - expect(res.status).toBe(400); + expect(res.status).toBe(200); + expect(res.body.valid).toBe(false); expect(res.body.error).toBe('invalid_scope'); }); @@ -763,6 +790,7 @@ describe('Client CRUD — /api/oauth/clients', () => { .set('Cookie', authCookie(user.id)); expect(res.status).toBe(404); }); + }); // ─────────────────────────────────────────────────────────────────────────────