mirror of
https://github.com/garrytan/gbrain.git
synced 2026-08-14 08:53:22 +00:00
fix(auth): accept comma-separated --scopes and reject zero-token input (#3990)
`gbrain auth register-client --scopes` only split on whitespace, so the comma-joined form the CLI's own registration hint recommends (`--scopes read,write,admin`, printed by init.ts) fell through as a single unrecognized token and was rejected with `Unknown scope "read,write,admin"` -- self-contradicting the hint. parseRegisterClientArgs now normalizes comma- and/or whitespace-separated --scopes input to the canonical space-joined form before it reaches registerClientManual's assertAllowedScopes gate. The shared parseScopeString (RFC 6749 space-delimited OAuth wire format, also used for untrusted DCR/refresh/request-scope parsing) is left untouched. Input that normalizes to zero tokens (comma-only, whitespace-only, or empty) is rejected at the parser boundary with a clear usage error, rather than silently registering a client with no usable scopes -- closing a hole that a comma-only first pass at this fix left open (caught by review). Tests cover comma/space/mixed forms parsing to the identical scope set, a genuinely unknown scope still being rejected, and zero-token input being rejected.
This commit is contained in:
+30
-1
@@ -388,7 +388,36 @@ export function parseRegisterClientArgs(args: string[]): RegisterClientArgs {
|
||||
i += 2;
|
||||
break;
|
||||
}
|
||||
case '--scopes': out.scopes = requireValue(); i += 2; break;
|
||||
case '--scopes': {
|
||||
// v0.42.x: accept comma-separated input (`--scopes read,write,admin`)
|
||||
// in addition to the space-separated OAuth wire form
|
||||
// (`--scopes "read write admin"`). init.ts's own registration hint
|
||||
// (line ~730) recommends the comma form, but the parser previously
|
||||
// only split on whitespace, so a comma-joined string fell through
|
||||
// as a single unrecognized token and registerClientManual's
|
||||
// assertAllowedScopes rejected it as `Unknown scope
|
||||
// "read,write,admin"` — self-contradicting the hint. Normalizing
|
||||
// here (rather than in the shared parseScopeString) keeps that
|
||||
// function's OAuth-wire-format (RFC 6749 space-delimited) contract
|
||||
// intact for DCR/refresh/request-scope parsing, which stays
|
||||
// comma-agnostic on purpose.
|
||||
const v = requireValue();
|
||||
const normalized = v.split(/[\s,]+/).filter(Boolean).join(' ');
|
||||
// Zero-token input (`--scopes ","`, `--scopes ",,,"`, `--scopes " "`,
|
||||
// `--scopes ""`) collapses to an empty string under the split above.
|
||||
// parseScopeString('') returns [] downstream, and
|
||||
// assertAllowedScopes([]) passes vacuously on an empty list — so
|
||||
// without this guard, registerClientManual would silently register
|
||||
// a client with no usable scopes instead of reporting malformed
|
||||
// input. Reject here, at the parser boundary, with a clear message
|
||||
// rather than relying on whatever downstream error the raw string
|
||||
// happens to produce.
|
||||
if (!normalized) {
|
||||
throw new Error(`--scopes requires at least one scope (got ${JSON.stringify(v)})`);
|
||||
}
|
||||
out.scopes = normalized;
|
||||
i += 2; break;
|
||||
}
|
||||
case '--source': out.sourceId = requireValue(); i += 2; break;
|
||||
case '--federated-read': {
|
||||
const v = requireValue();
|
||||
|
||||
@@ -12,6 +12,7 @@
|
||||
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { parseRegisterClientArgs } from '../src/commands/auth.ts';
|
||||
import { assertAllowedScopes, parseScopeString } from '../src/core/scope.ts';
|
||||
|
||||
describe('parseRegisterClientArgs', () => {
|
||||
test('empty args → all defaults', () => {
|
||||
@@ -40,6 +41,75 @@ describe('parseRegisterClientArgs', () => {
|
||||
expect(out.scopes).toBe('read write');
|
||||
});
|
||||
|
||||
// init.ts's own `gbrain auth register-client` hint (line ~730) recommends
|
||||
// `--scopes read,write,admin`, but the parser previously only split on
|
||||
// whitespace, so the comma-joined string fell through as a single
|
||||
// unrecognized token and registerClientManual's assertAllowedScopes
|
||||
// rejected it with `Unknown scope "read,write,admin"` — self-contradicting
|
||||
// the hint it printed. These pin the comma form as accepted, alongside the
|
||||
// pre-existing space form, without changing the shared OAuth-wire-format
|
||||
// parseScopeString (RFC 6749 space-delimited) used for DCR/refresh/request
|
||||
// scope parsing.
|
||||
describe('--scopes comma-separated (matches init.ts hint)', () => {
|
||||
test('comma-separated → normalized to the space-joined wire form', () => {
|
||||
const out = parseRegisterClientArgs(['--scopes', 'read,write,admin']);
|
||||
expect(out.scopes).toBe('read write admin');
|
||||
});
|
||||
|
||||
test('mixed comma+space form normalizes the same way', () => {
|
||||
const out = parseRegisterClientArgs(['--scopes', 'read, write ,admin']);
|
||||
expect(out.scopes).toBe('read write admin');
|
||||
});
|
||||
|
||||
test('single scope (no separators) is unaffected', () => {
|
||||
const out = parseRegisterClientArgs(['--scopes', 'read']);
|
||||
expect(out.scopes).toBe('read');
|
||||
});
|
||||
|
||||
test('comma, space, and mixed forms all parse to the identical scope set downstream', () => {
|
||||
const comma = parseRegisterClientArgs(['--scopes', 'read,write,admin']).scopes;
|
||||
const spaced = parseRegisterClientArgs(['--scopes', 'read write admin']).scopes;
|
||||
const mixed = parseRegisterClientArgs(['--scopes', 'read, write ,admin']).scopes;
|
||||
const expected = ['read', 'write', 'admin'];
|
||||
expect(parseScopeString(comma)).toEqual(expected);
|
||||
expect(parseScopeString(spaced)).toEqual(expected);
|
||||
expect(parseScopeString(mixed)).toEqual(expected);
|
||||
// Control: all three forms also clear the registration-time allowlist
|
||||
// gate that init.ts's hint promises works.
|
||||
expect(() => assertAllowedScopes(parseScopeString(comma))).not.toThrow();
|
||||
expect(() => assertAllowedScopes(parseScopeString(spaced))).not.toThrow();
|
||||
expect(() => assertAllowedScopes(parseScopeString(mixed))).not.toThrow();
|
||||
});
|
||||
|
||||
// Control: comma-splitting must not smuggle a genuinely unknown scope
|
||||
// past the registration-time allowlist gate.
|
||||
test('a genuinely unknown scope in comma form is still rejected downstream', () => {
|
||||
const out = parseRegisterClientArgs(['--scopes', 'read,flying-unicorn']);
|
||||
expect(out.scopes).toBe('read flying-unicorn');
|
||||
expect(() => assertAllowedScopes(parseScopeString(out.scopes)))
|
||||
.toThrow(/Unknown scope "flying-unicorn"/);
|
||||
});
|
||||
|
||||
// Codex review finding (round 1): separator-only input (e.g. "," or
|
||||
// ",,,") split to zero tokens under the comma/whitespace regex, which
|
||||
// collapsed to `''` and would have passed assertAllowedScopes
|
||||
// vacuously downstream (empty scope list silently accepted).
|
||||
//
|
||||
// Codex review finding (round 2, P1): an initial fix that forwarded the
|
||||
// raw string only for comma-only input left whitespace-only (`" "`)
|
||||
// and empty-string (`""`) inputs unguarded — those also normalize to
|
||||
// zero tokens under the same regex (whitespace is a supported
|
||||
// separator too), so the same silent-empty-scope-set hole remained for
|
||||
// them. The parser now rejects ANY input that normalizes to zero
|
||||
// tokens, uniformly, with a clear CLI usage error.
|
||||
test('zero-token --scopes input (commas, whitespace, or empty) is rejected at parse time', () => {
|
||||
for (const raw of [',', ',,,', ' , ', ' ', '']) {
|
||||
expect(() => parseRegisterClientArgs(['--scopes', raw]))
|
||||
.toThrow(/--scopes requires at least one scope/);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
test('--source scopes the OAuth client', () => {
|
||||
const out = parseRegisterClientArgs(['--source', 'dept-x']);
|
||||
expect(out.sourceId).toBe('dept-x');
|
||||
|
||||
Reference in New Issue
Block a user