mirror of
https://github.com/garrytan/gbrain.git
synced 2026-07-31 04:07:52 +00:00
fix(oauth): default omitted authorize scope to client's full grant
When a client omits `scope` on /authorize, the authorize() grant computed `(params.scopes || []).filter(...)` → the empty set. That empty grant was written to oauth_codes and propagated into the access AND refresh tokens, so every request failed `insufficient_scope` even though the client was registered with e.g. `read write`. Because refresh inherits the stored grant, it never self-healed — reconnecting just minted another empty-scoped token. Some MCP connectors (observed with Claude Desktop) omit `scope` on /authorize, so they hit this on every connection. Fix: when no scope is requested, default to the client's full registered scope (RFC 6749 §3.3 permits a server default). This mirrors exchangeClientCredentials, which already does `requestedScope ? ... : allowedScopes`. The result is still clamped to the allowed set, so an explicit over-broad request cannot escalate. Adds test/oauth-authorize-scope-default.test.ts covering: omitted/empty → inherits full grant; explicit subset honored; clamp preserved (over-broad and disallowed-only requests cannot escalate or trigger inheritance). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
1eb430a2df
commit
1c9ccefc3d
@@ -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,
|
||||
|
||||
@@ -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<typeof sqlQueryForEngine>;
|
||||
|
||||
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<string[]> {
|
||||
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([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user