diff --git a/src/core/oauth-provider.ts b/src/core/oauth-provider.ts index 8b4ac91d3..ba89beb8e 100644 --- a/src/core/oauth-provider.ts +++ b/src/core/oauth-provider.ts @@ -389,10 +389,18 @@ export class GBrainOAuthProvider implements OAuthServerProvider { // as a fully-admin access token. Mirrors the filter pattern already used // by exchangeClientCredentials (this file) and exchangeRefreshToken's F3 // subset enforcement (RFC 6749 §6) so all three grant entry points clamp - // consistently. Empty/omitted requested scope inherits the empty-stored - // shape (existing behavior; not a security boundary). + // consistently. When the client requests NO scope, RFC 6749 §3.3 lets the + // server fall back to a default — we default to the client's full + // registered scope (matching exchangeClientCredentials, which already does + // `requestedScope ? ... : allowedScopes`). Previously an omitted request + // granted the empty set, which then propagated into the access+refresh + // tokens and never self-healed: every op failed `insufficient_scope` even + // though the client was registered with `read write`. Clients that omit + // `scope` on /authorize (e.g. some MCP connectors) hit this. Still clamped + // to the allowed set, so an explicit over-broad request can't escalate. const allowedScopes = parseScopeString(client.scope); - const grantedScopes = (params.scopes || []).filter(s => hasScope(allowedScopes, s)); + const requestedScopes = (params.scopes && params.scopes.length) ? params.scopes : allowedScopes; + const grantedScopes = requestedScopes.filter(s => hasScope(allowedScopes, s)); await this.sql` INSERT INTO oauth_codes (code_hash, client_id, scopes, code_challenge, diff --git a/test/oauth-authorize-scope-default.test.ts b/test/oauth-authorize-scope-default.test.ts new file mode 100644 index 000000000..4dc2c3d81 --- /dev/null +++ b/test/oauth-authorize-scope-default.test.ts @@ -0,0 +1,89 @@ +/** + * Authorize-grant scope default (RFC 6749 §3.3). + * + * When a client omits `scope` on /authorize, the granted scope must default to + * the client's full registered scope — NOT the empty set. Regression guard for + * the bug where an omitted request granted [], which then propagated into the + * access + refresh tokens and never self-healed: every op failed + * `insufficient_scope` even though the client was registered `read write` + * (some MCP connectors omit `scope` on /authorize). The clamp must still hold — + * an explicit over-broad request cannot escalate past the client's allowed set. + */ +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { GBrainOAuthProvider } from '../src/core/oauth-provider.ts'; +import { sqlQueryForEngine } from '../src/core/sql-query.ts'; + +let engine: PGLiteEngine; +let provider: GBrainOAuthProvider; +let sql: ReturnType; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + sql = sqlQueryForEngine(engine); + provider = new GBrainOAuthProvider({ sql }); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await (engine as any).db.exec('DELETE FROM oauth_tokens'); + await (engine as any).db.exec('DELETE FROM oauth_codes'); + await (engine as any).db.exec('DELETE FROM oauth_clients'); +}); + +// authorize() writes the granted scope into oauth_codes then redirects; we +// assert on the stored grant directly, so the redirect is a no-op. +const noopRes = { redirect() {} } as any; + +async function authorizeAndReadScopes( + scope: string, + requested: string[] | undefined, +): Promise { + const reg = await provider.registerClientManual( + 'authz-test', ['authorization_code'], scope, ['https://example.test/cb'], + ); + const client = await provider.clientsStore.getClient(reg.clientId); + expect(client).toBeTruthy(); + await provider.authorize( + client!, + { + scopes: requested, + codeChallenge: 'test-challenge', + redirectUri: 'https://example.test/cb', + state: 'xyz', + } as any, + noopRes, + ); + const rows = (await sql` + SELECT scopes FROM oauth_codes WHERE client_id = ${reg.clientId} + `) as Array<{ scopes: string[] }>; + expect(rows.length).toBe(1); + return rows[0].scopes ?? []; +} + +describe('authorize() scope default — omitted scope inherits client grant', () => { + test('omitted scope → inherits full registered scope', async () => { + expect((await authorizeAndReadScopes('read write', undefined)).sort()).toEqual(['read', 'write']); + }); + + test('empty scope array → inherits full registered scope', async () => { + expect((await authorizeAndReadScopes('read write', [])).sort()).toEqual(['read', 'write']); + }); + + test('explicit subset is honored (not overridden to full)', async () => { + expect(await authorizeAndReadScopes('read write admin', ['read'])).toEqual(['read']); + }); + + test('clamp preserved: over-broad request cannot escalate', async () => { + expect(await authorizeAndReadScopes('read', ['read', 'admin'])).toEqual(['read']); + }); + + test('clamp preserved: requesting only a disallowed scope grants nothing (no inheritance)', async () => { + expect(await authorizeAndReadScopes('read write', ['admin'])).toEqual([]); + }); +});