mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-06-19 13:21:46 +00:00
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.
This commit is contained in:
@@ -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);
|
||||
});
|
||||
|
||||
|
||||
@@ -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,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user