From 783504d7ead9edfa3e151d3441620cb46ae5d3aa Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 20 Aug 2026 22:14:36 +0000 Subject: [PATCH] fix(pairing): reject reserved clientId 'root' at all token writers 'root' is the sentinel checkScope/checkDomain/checkRate and the server command gate use for the omnipotent caller, so a scoped token carrying it bypasses every enforcement path. Add ReservedClientIdError + a shared assertValidClientId; createToken/createSetupKey throw, restoreRegistry skips-and-logs (a corrupt state file must not brick boot). /pair and /token surface it as a named 400, and the CLI fast-fails --client root. Co-Authored-By: Claude Fable 5 --- browse/src/cli.ts | 8 +++++++ browse/src/server.ts | 14 ++++++------ browse/src/token-registry.ts | 34 +++++++++++++++++++++++++++++ browse/test/pair-agent-e2e.test.ts | 27 +++++++++++++++++++++++ browse/test/token-registry.test.ts | 35 +++++++++++++++++++++++++++++- 5 files changed, 110 insertions(+), 8 deletions(-) diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 8a361fa95..ff620014b 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -1225,6 +1225,14 @@ async function tunnelAgents(): Promise { * opposite of the user's intent. And `control` never rides in via --restrict: * browser-wide destructive ops stay behind the explicit --control flag. */ function validatePairAgentFlags(args: string[]): void { + // `root` is the sentinel that bypasses all scope/domain/rate/tab enforcement; + // naming an agent that way would silently un-sandbox it. Reject client-side + // before hitting the daemon (the server rejects it too). + const client = parseFlag(args, '--client'); + if (client !== undefined && client.trim().toLowerCase() === 'root') { + console.error("[browse] --client 'root' is reserved — it would bypass all scope enforcement. Choose another name."); + process.exit(1); + } // hasFlag/parseFlag are exact-token matches, so `--restrict=read` would // sail past every check below and silently grant FULL access. if (args.some(a => a.startsWith('--restrict='))) { diff --git a/browse/src/server.ts b/browse/src/server.ts index 40d5b6c62..92ecdeebb 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -32,7 +32,7 @@ import { checkRate, createToken, createSetupKey, exchangeSetupKey, revokeToken, listTokens, recordCommand, isRootToken, checkConnectRateLimit, type TokenInfo, type ScopeCategory, - DEFAULT_PAIR_SCOPES, InvalidScopeError, + DEFAULT_PAIR_SCOPES, InvalidScopeError, ReservedClientIdError, } from './token-registry'; import { validateTempPath } from './path-security'; import { resolveConfig, ensureStateDir, readVersionHash, resolveChromiumProfile, cleanSingletonLocks, isPairAgentEnabled } from './config'; @@ -2318,9 +2318,9 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { agent: session.clientId, }), { status: 200, headers: { 'Content-Type': 'application/json' } }); } catch (err) { - // Name the caller's typo (bad scope, negative rateLimit) instead of - // hiding it behind the generic body error. - if (err instanceof InvalidScopeError) { + // Name the caller's typo (bad scope, negative rateLimit, reserved + // clientId) instead of hiding it behind the generic body error. + if (err instanceof InvalidScopeError || err instanceof ReservedClientIdError) { return new Response(JSON.stringify({ error: err.message }), { status: 400, headers: { 'Content-Type': 'application/json' }, }); @@ -2442,9 +2442,9 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { server_url: `http://127.0.0.1:${browsePort}`, }), { status: 200, headers: { 'Content-Type': 'application/json' } }); } catch (err) { - // Name the caller's typo (bad scope, negative rateLimit) instead of - // hiding it behind the generic body error. - if (err instanceof InvalidScopeError) { + // Name the caller's typo (bad scope, negative rateLimit, reserved + // clientId) instead of hiding it behind the generic body error. + if (err instanceof InvalidScopeError || err instanceof ReservedClientIdError) { return new Response(JSON.stringify({ error: err.message }), { status: 400, headers: { 'Content-Type': 'application/json' }, }); diff --git a/browse/src/token-registry.ts b/browse/src/token-registry.ts index 23c9a388f..211fc447d 100644 --- a/browse/src/token-registry.ts +++ b/browse/src/token-registry.ts @@ -116,6 +116,26 @@ function assertValidTokenOptions(scopes: readonly string[], rateLimit: number): if (rateLimit < 0) throw new InvalidScopeError('rateLimit must be >= 0'); } +/** + * Typed error for a reserved or malformed clientId. `root` is the sentinel that + * checkScope/checkDomain/checkRate and the server command gate use to mean "the + * omnipotent root caller" (validateToken:360), so a scoped token carrying it + * would bypass every enforcement path. Empty/non-string ids collapse distinct + * agents together and break revoke-by-clientId. Request-path writers throw; + * restoreRegistry skips-and-logs so one bad state-file entry can't drop later + * sessions or brick boot. + */ +export class ReservedClientIdError extends Error {} + +export function assertValidClientId(clientId: unknown): asserts clientId is string { + if (typeof clientId !== 'string' || clientId.trim() === '') { + throw new ReservedClientIdError('clientId must be a non-empty string'); + } + if (clientId.trim().toLowerCase() === 'root') { + throw new ReservedClientIdError("clientId 'root' is reserved"); + } +} + // ─── Types ────────────────────────────────────────────────────── export interface TokenInfo { @@ -234,6 +254,7 @@ export function createToken(opts: CreateTokenOptions): TokenInfo { } = opts; // Validate inputs + assertValidClientId(clientId); assertValidTokenOptions(scopes, rateLimit); if (expiresSeconds !== null && expiresSeconds !== undefined && expiresSeconds < 0) { throw new Error('expiresSeconds must be >= 0 or null'); @@ -276,6 +297,9 @@ export function createToken(opts: CreateTokenOptions): TokenInfo { * Setup keys expire in 5 minutes and can only be exchanged once. */ export function createSetupKey(opts: Omit & { clientId?: string }): TokenInfo { + // Only validate when a clientId is supplied; an omitted one gets a safe + // generated `remote-` default below. + if (opts.clientId !== undefined) assertValidClientId(opts.clientId); const scopes = opts.scopes || ['read', 'write']; // ?? not ||: rateLimit 0 is documented as "unlimited" and must survive. const rateLimit = opts.rateLimit ?? 10; @@ -535,6 +559,16 @@ export function restoreRegistry(state: TokenRegistryState): void { // Skip expired tokens if (data.expiresAt && new Date(data.expiresAt) < now) continue; + // Skip-and-log rather than throw: a hand-edited or corrupt state file must + // not brick boot or drop every later valid session. A persisted clientId + // 'root' would otherwise inject a token that bypasses all scope checks. + try { + assertValidClientId(clientId); + } catch (err) { + console.warn(`[browse] restoreRegistry: skipping invalid clientId ${JSON.stringify(clientId)}: ${err instanceof Error ? err.message : String(err)}`); + continue; + } + tokens.set(data.token, { ...data, clientId, diff --git a/browse/test/pair-agent-e2e.test.ts b/browse/test/pair-agent-e2e.test.ts index 5a59dce4e..36aec9e58 100644 --- a/browse/test/pair-agent-e2e.test.ts +++ b/browse/test/pair-agent-e2e.test.ts @@ -291,6 +291,33 @@ describe('pair-agent flow end-to-end (HTTP only, no ngrok)', () => { expect(body.hint).not.toContain('--admin'); }); + // ─── D2: reserved clientId is rejected with a named 400 ─────────────── + + test('POST /pair with clientId "root" returns 400 naming the reservation', async () => { + const resp = await fetch(`${daemon.baseUrl}/pair`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'root' }), + }); + expect(resp.status).toBe(400); + const body = await resp.json() as any; + // The reservation is named, NOT hidden behind the generic "Invalid request body". + expect(body.error).toContain('root'); + expect(body.error).not.toBe('Invalid request body'); + }); + + test('POST /token with clientId "root" returns 400 naming the reservation', async () => { + const resp = await fetch(`${daemon.baseUrl}/token`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'root' }), + }); + expect(resp.status).toBe(400); + const body = await resp.json() as any; + expect(body.error).toContain('root'); + expect(body.error).not.toBe('Invalid request body'); + }); + // ─── Revocation e2e: revoke-all + the /agents verification surface ──── test('DELETE /token revokes session AND setup keys; agent leaves /agents; token 401s; re-connect fails', async () => { diff --git a/browse/test/token-registry.test.ts b/browse/test/token-registry.test.ts index fb5adbfa2..07e202889 100644 --- a/browse/test/token-registry.test.ts +++ b/browse/test/token-registry.test.ts @@ -6,7 +6,7 @@ import { revokeToken, rotateRoot, listTokens, recordCommand, serializeRegistry, restoreRegistry, checkConnectRateLimit, SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN, SCOPE_CONTROL, SCOPE_META, - DEFAULT_PAIR_SCOPES, InvalidScopeError, + DEFAULT_PAIR_SCOPES, InvalidScopeError, ReservedClientIdError, __resetRegistry, } from '../src/token-registry'; @@ -19,6 +19,39 @@ describe('token-registry', () => { initRegistry('root-token-for-tests'); }); + // D2: `root` is the sentinel checkScope/checkDomain/checkRate use for the + // omnipotent caller; a scoped token carrying it bypasses all enforcement. + describe('reserved clientId (D2)', () => { + it('createToken rejects clientId "root" and lookalikes', () => { + expect(() => createToken({ clientId: 'root' })).toThrow(ReservedClientIdError); + expect(() => createToken({ clientId: 'ROOT' })).toThrow(ReservedClientIdError); + expect(() => createToken({ clientId: ' root ' })).toThrow(ReservedClientIdError); + }); + + it('createToken rejects empty / whitespace clientId', () => { + expect(() => createToken({ clientId: '' })).toThrow(ReservedClientIdError); + expect(() => createToken({ clientId: ' ' })).toThrow(ReservedClientIdError); + }); + + it('createSetupKey rejects clientId "root" but allows an omitted one', () => { + expect(() => createSetupKey({ clientId: 'root' })).toThrow(ReservedClientIdError); + // Omitted clientId gets a safe generated default, not a throw. + const key = createSetupKey({}); + expect(key.clientId.startsWith('remote-')).toBe(true); + }); + + it('restoreRegistry skips a persisted "root" entry instead of injecting a bypass token', () => { + restoreRegistry({ agents: { + root: { token: 'gsk_sess_evil', type: 'session', scopes: ['read', 'write', 'admin', 'meta', 'control'], tabPolicy: 'shared', rateLimit: 0, expiresAt: null, createdAt: new Date().toISOString() } as any, + good: { token: 'gsk_sess_good', type: 'session', scopes: ['read'], tabPolicy: 'own-only', rateLimit: 10, expiresAt: null, createdAt: new Date().toISOString() } as any, + } }); + // The evil root entry is dropped; the valid one still restores. + expect(validateToken('gsk_sess_evil')).toBeNull(); + const good = validateToken('gsk_sess_good'); + expect(good?.clientId).toBe('good'); + }); + }); + describe('root token', () => { it('identifies root token correctly', () => { expect(isRootToken('root-token-for-tests')).toBe(true);