From 52306ed438d8b03fc7835cc5344921244643954e Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:48:11 +0900 Subject: [PATCH] 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. --- src/commands/auth.ts | 31 +++++++++++- test/auth-register-client-args.test.ts | 70 ++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/commands/auth.ts b/src/commands/auth.ts index ce972bc41..d7eb78f08 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -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(); diff --git a/test/auth-register-client-args.test.ts b/test/auth-register-client-args.test.ts index 26eba6ab8..b206fc6d4 100644 --- a/test/auth-register-client-args.test.ts +++ b/test/auth-register-client-args.test.ts @@ -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');