diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 18cee9b3e..1d1cc7d19 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -18,6 +18,9 @@ import { resolveConfig, ensureStateDir, readVersionHash, isPairAgentEnabled } fr import { parseProxyConfig, computeConfigHash, ProxyConfigError } from './proxy-config'; import { redactProxyUrl } from './proxy-redact'; import { spawnTerminalAgent } from './terminal-agent-control'; +// Zero side effects on import (documented invariant in token-registry.ts) — +// safe to pull the shared pairing default into the CLI. +import { DEFAULT_PAIR_SCOPES } from './token-registry'; const config = resolveConfig(); const IS_WINDOWS = process.platform === 'win32'; @@ -1216,6 +1219,29 @@ async function tunnelAgents(): Promise { return 0; } +/** Reject pair-agent scope-flag misuse BEFORE any consent or server work. + * Bare `--restrict` (or a flag-shaped value from a forgotten argument) used + * to parse as "no restriction" and silently grant FULL access — the exact + * 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 { + if (!hasFlag(args, '--restrict')) return; + const restrict = parseFlag(args, '--restrict'); + if (!restrict || !restrict.trim() || restrict.startsWith('--')) { + console.error('[browse] --restrict needs a scope list, e.g. --restrict read or --restrict "read,write". Bare --restrict would silently grant FULL access.'); + process.exit(1); + } + if (hasFlag(args, '--control') || hasFlag(args, '--admin')) { + // Server-side, the control flag wins and the scopes list is ignored. + console.warn('[browse] --restrict is ignored when --control/--admin is set (control implies full access).'); + return; + } + if (restrict.split(',').map(s => s.trim()).includes('control')) { + console.error('[browse] The control scope is not granted via --restrict. Re-run with --control.'); + process.exit(1); + } +} + async function handleTunnel(args: string[]): Promise { const sub = args[0]; if (sub === 'revoke' && args.length === 2 && args[1].trim()) { @@ -1236,8 +1262,12 @@ async function handlePairAgent(state: ServerState, args: string[]): Promise s.trim()) } : {}), + scopes: restrict + ? restrict.split(',').map(s => s.trim()) + : [...DEFAULT_PAIR_SCOPES], }), signal: AbortSignal.timeout(5000), }); @@ -1791,6 +1823,9 @@ Refs: After 'snapshot', use @e1, @e2... as selectors: // state, so replacing it kills nothing the user had. let pairAgentPreexistingDaemonAlive = false; if (command === 'pair-agent') { + // Scope-flag misuse is rejected before consent gates and ensureServer — + // an arg error must never boot a daemon. + validatePairAgentFlags(commandArgs); const preState = readState(); pairAgentPreexistingDaemonAlive = Boolean(preState?.pid && isProcessAlive(preState.pid)); } diff --git a/browse/src/server.ts b/browse/src/server.ts index 3f551e244..6efdee65c 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -31,7 +31,8 @@ import { initRegistry, validateToken as validateScopedToken, checkScope, checkDomain, checkRate, createToken, createSetupKey, exchangeSetupKey, revokeToken, listTokens, recordCommand, - isRootToken, checkConnectRateLimit, type TokenInfo, + isRootToken, checkConnectRateLimit, type TokenInfo, type ScopeCategory, + DEFAULT_PAIR_SCOPES, InvalidScopeError, } from './token-registry'; import { validateTempPath } from './path-security'; import { resolveConfig, ensureStateDir, readVersionHash, resolveChromiumProfile, cleanSingletonLocks, isPairAgentEnabled } from './config'; @@ -2316,7 +2317,14 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { scopes: session.scopes, agent: session.clientId, }), { status: 200, headers: { 'Content-Type': 'application/json' } }); - } catch { + } catch (err) { + // Name the caller's typo (bad scope, negative rateLimit) instead of + // hiding it behind the generic body error. + if (err instanceof InvalidScopeError) { + return new Response(JSON.stringify({ error: err.message }), { + status: 400, headers: { 'Content-Type': 'application/json' }, + }); + } return new Response(JSON.stringify({ error: 'Invalid request body' }), { status: 400, headers: { 'Content-Type': 'application/json' }, }); @@ -2384,12 +2392,20 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { } try { const pairBody = await req.json() as any; - // Default: full access (read+write+admin+meta). The trust boundary is - // the pairing ceremony itself, not the scope. --control adds browser-wide - // destructive commands (stop, restart, disconnect). --restrict limits scope. + // Default: DEFAULT_PAIR_SCOPES (full page access). The trust boundary + // is the pairing ceremony itself, not the scope. --control adds + // browser-wide destructive commands (stop, restart, disconnect). + // --restrict limits scope — but can never grant control: that scope + // stays behind the explicit control flag. + if (!pairBody.control && !pairBody.admin + && Array.isArray(pairBody.scopes) && pairBody.scopes.includes('control')) { + return new Response(JSON.stringify({ + error: 'The control scope requires the control flag (--control); it cannot be granted via a scopes list.', + }), { status: 400, headers: { 'Content-Type': 'application/json' } }); + } const scopes = pairBody.control || pairBody.admin - ? ['read', 'write', 'admin', 'meta', 'control'] as const - : (pairBody.scopes || ['read', 'write', 'admin', 'meta']) as const; + ? [...DEFAULT_PAIR_SCOPES, 'control' as const] + : ((pairBody.scopes || [...DEFAULT_PAIR_SCOPES]) as ScopeCategory[]); const setupKey = createSetupKey({ clientId: pairBody.clientId, scopes: [...scopes], @@ -2425,7 +2441,14 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { tunnel_url: verifiedTunnelUrl, server_url: `http://127.0.0.1:${browsePort}`, }), { status: 200, headers: { 'Content-Type': 'application/json' } }); - } catch { + } catch (err) { + // Name the caller's typo (bad scope, negative rateLimit) instead of + // hiding it behind the generic body error. + if (err instanceof InvalidScopeError) { + return new Response(JSON.stringify({ error: err.message }), { + status: 400, headers: { 'Content-Type': 'application/json' }, + }); + } return new Response(JSON.stringify({ error: 'Invalid request body' }), { status: 400, headers: { 'Content-Type': 'application/json' }, }); diff --git a/browse/src/token-registry.ts b/browse/src/token-registry.ts index 21bce0739..0787aabf0 100644 --- a/browse/src/token-registry.ts +++ b/browse/src/token-registry.ts @@ -82,6 +82,35 @@ const SCOPE_MAP: Record> = { meta: SCOPE_META, }; +/** + * Scopes granted by POST /pair when nothing narrower is requested. + * Deliberately full page access (b73f3644 / #907): the trust boundary is the + * pairing ceremony, not the scope. 'control' (browser-wide destructive ops) + * is the only scope that stays opt-in via the control flag. Referenced by + * BOTH server.ts (/pair default) and cli.ts (explicit send) so the two + * defaults cannot silently drift apart again. + */ +export const DEFAULT_PAIR_SCOPES: readonly ScopeCategory[] = ['read', 'write', 'admin', 'meta']; + +/** + * Typed error for caller-supplied token options (unknown scope, negative + * rateLimit). HTTP handlers catch it to 400 with the message at the endpoint + * where the typo happened — pre-fix, a bad scope sailed through /pair into a + * poisoned setup key and surfaced as a misleading "Invalid request body" to + * the remote agent at /connect. + */ +export class InvalidScopeError extends Error {} + +function assertValidTokenOptions(scopes: readonly string[], rateLimit: number): void { + const validScopes: ScopeCategory[] = ['read', 'write', 'admin', 'meta', 'control']; + for (const s of scopes) { + if (!validScopes.includes(s as ScopeCategory)) { + throw new InvalidScopeError(`Invalid scope: ${s}. Valid: ${validScopes.join(', ')}`); + } + } + if (rateLimit < 0) throw new InvalidScopeError('rateLimit must be >= 0'); +} + // ─── Types ────────────────────────────────────────────────────── export interface TokenInfo { @@ -200,13 +229,7 @@ export function createToken(opts: CreateTokenOptions): TokenInfo { } = opts; // Validate inputs - const validScopes: ScopeCategory[] = ['read', 'write', 'admin', 'meta', 'control']; - for (const s of scopes) { - if (!validScopes.includes(s as ScopeCategory)) { - throw new Error(`Invalid scope: ${s}. Valid: ${validScopes.join(', ')}`); - } - } - if (rateLimit < 0) throw new Error('rateLimit must be >= 0'); + assertValidTokenOptions(scopes, rateLimit); if (expiresSeconds !== null && expiresSeconds !== undefined && expiresSeconds < 0) { throw new Error('expiresSeconds must be >= 0 or null'); } @@ -248,6 +271,13 @@ 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 { + const scopes = opts.scopes || ['read', 'write']; + // ?? not ||: rateLimit 0 is documented as "unlimited" and must survive. + const rateLimit = opts.rateLimit ?? 10; + // Validate HERE, not only at exchange time in createToken — otherwise a + // typo mints a poisoned setup key whose failure surfaces to the wrong + // party (the remote agent, at /connect, as "Invalid request body"). + assertValidTokenOptions(scopes, rateLimit); const token = generateToken('gsk_setup_'); const now = new Date(); const expiresAt = new Date(now.getTime() + 5 * 60 * 1000).toISOString(); // 5 min @@ -256,10 +286,10 @@ export function createSetupKey(opts: Omit & { cl token, clientId: opts.clientId || `remote-${Date.now()}`, type: 'setup', - scopes: opts.scopes || ['read', 'write'], + scopes, domains: opts.domains, tabPolicy: opts.tabPolicy || 'own-only', - rateLimit: opts.rateLimit || 10, + rateLimit, expiresAt, createdAt: now.toISOString(), usesRemaining: 1, diff --git a/browse/test/pair-agent-e2e.test.ts b/browse/test/pair-agent-e2e.test.ts index 6e58123d6..a5a2f6d9d 100644 --- a/browse/test/pair-agent-e2e.test.ts +++ b/browse/test/pair-agent-e2e.test.ts @@ -194,6 +194,76 @@ describe('pair-agent flow end-to-end (HTTP only, no ngrok)', () => { expect(Array.isArray(scopes)).toBe(true); }); + // ─── Pair scope contract: defaults, explicit lists, typo naming ─────── + + test('default /connect scopes are exactly read,write,admin,meta', async () => { + const pairResp = await fetch(`${daemon.baseUrl}/pair`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'default-scopes' }), + }); + const { setup_key, scopes: pairScopes } = await pairResp.json() as any; + expect(pairScopes).toEqual(['read', 'write', 'admin', 'meta']); + const connectResp = await fetch(`${daemon.baseUrl}/connect`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ setup_key }), + }); + const { scopes } = await connectResp.json() as any; + expect(scopes).toEqual(['read', 'write', 'admin', 'meta']); + }); + + test('explicit scopes are honored end-to-end (the --restrict wire contract)', async () => { + const pairResp = await fetch(`${daemon.baseUrl}/pair`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'restricted-scopes', scopes: ['read'] }), + }); + const { setup_key } = await pairResp.json() as any; + const connectResp = await fetch(`${daemon.baseUrl}/connect`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ setup_key }), + }); + const { scopes } = await connectResp.json() as any; + expect(scopes).toEqual(['read']); + }); + + test('POST /pair with a scope typo fails fast, naming the scope', async () => { + // Regression: pre-fix this returned 200 with a poisoned setup key whose + // failure surfaced at /connect as a misleading "Invalid request body". + const resp = await fetch(`${daemon.baseUrl}/pair`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'typo-agent', scopes: ['raed'] }), + }); + expect(resp.status).toBe(400); + const body = await resp.json() as any; + expect(body.error).toContain('Invalid scope: raed'); + }); + + test('POST /token with a scope typo names the scope too', async () => { + const resp = await fetch(`${daemon.baseUrl}/token`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'typo-token', scopes: ['wirte'] }), + }); + expect(resp.status).toBe(400); + const body = await resp.json() as any; + expect(body.error).toContain('Invalid scope: wirte'); + }); + + test('control cannot ride in through a /pair scopes list without the control flag', async () => { + const resp = await fetch(`${daemon.baseUrl}/pair`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'sneaky', scopes: ['read', 'control'] }), + }); + expect(resp.status).toBe(400); + const body = await resp.json() as any; + expect(body.error).toContain('control'); + }); + // ─── 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/server-auth.test.ts b/browse/test/server-auth.test.ts index 76bd0445a..5bf906838 100644 --- a/browse/test/server-auth.test.ts +++ b/browse/test/server-auth.test.ts @@ -409,3 +409,32 @@ describe('Server auth security', () => { expect(routeSrc).toContain('SameSite=Strict'); }); }); + +describe('Pair scope defaults and revocation surface', () => { + // Regression: the CLI only sent scopes when --restrict was passed, so the + // effective pairing default lived in two places (CLI omission + server + // fallback) and could silently drift. Both sides must reference the shared + // DEFAULT_PAIR_SCOPES constant, and the CLI must send scopes + // unconditionally (the old conditional-spread shape is banned). + test('/pair default and CLI pairing body share DEFAULT_PAIR_SCOPES', () => { + const pairBlock = sliceBetween(SERVER_SRC, "url.pathname === '/pair'", "url.pathname === '/tunnel/start'"); + expect(pairBlock).toContain('DEFAULT_PAIR_SCOPES'); + const cliBlock = sliceBetween(CLI_SRC, 'async function handlePairAgent', 'Determine the URL to use'); + expect(cliBlock).toContain('DEFAULT_PAIR_SCOPES'); + expect(cliBlock).not.toContain('...(restrict ? { scopes'); + }); + + // control is the only scope behind an explicit flag; a scopes list must + // not be able to smuggle it into a pairing grant. + test('/pair rejects control inside a scopes list without the control flag', () => { + const pairBlock = sliceBetween(SERVER_SRC, "url.pathname === '/pair'", "url.pathname === '/tunnel/start'"); + expect(pairBlock).toContain("pairBody.scopes.includes('control')"); + }); + + // CLI-encoded clientIds (spaces, UTF-8) must round-trip through the revoke + // route; slicing the raw pathname 404s on every encoded name. + test('DELETE /token decodes the clientId path segment', () => { + const revokeBlock = sliceBetween(SERVER_SRC, "url.pathname.startsWith('/token/')", "url.pathname === '/agents'"); + expect(revokeBlock).toContain('decodeURIComponent'); + }); +}); diff --git a/browse/test/token-registry.test.ts b/browse/test/token-registry.test.ts index f68b6248b..fb5adbfa2 100644 --- a/browse/test/token-registry.test.ts +++ b/browse/test/token-registry.test.ts @@ -6,6 +6,7 @@ import { revokeToken, rotateRoot, listTokens, recordCommand, serializeRegistry, restoreRegistry, checkConnectRateLimit, SCOPE_READ, SCOPE_WRITE, SCOPE_ADMIN, SCOPE_CONTROL, SCOPE_META, + DEFAULT_PAIR_SCOPES, InvalidScopeError, __resetRegistry, } from '../src/token-registry'; @@ -347,6 +348,35 @@ describe('token-registry', () => { }); }); + describe('pair defaults and option validation', () => { + it('DEFAULT_PAIR_SCOPES is exactly read,write,admin,meta (b73f3644: the ceremony is the trust boundary)', () => { + expect([...DEFAULT_PAIR_SCOPES]).toEqual(['read', 'write', 'admin', 'meta']); + }); + + // Regression: only createToken validated options, so a scope typo minted + // a poisoned setup key at /pair and surfaced to the REMOTE agent at + // /connect as a misleading "Invalid request body". + it('createSetupKey rejects an unknown scope with InvalidScopeError naming it', () => { + expect(() => createSetupKey({ scopes: ['raed' as never] })) + .toThrow(InvalidScopeError); + expect(() => createSetupKey({ scopes: ['raed' as never] })) + .toThrow('Invalid scope: raed'); + }); + + it('createSetupKey rejects a negative rateLimit', () => { + expect(() => createSetupKey({ rateLimit: -5 })).toThrow(InvalidScopeError); + }); + + // Regression: `opts.rateLimit || 10` coerced the documented "0 = unlimited" + // into 10 on the /pair path while /token honored it. + it('createSetupKey preserves rateLimit 0 (unlimited)', () => { + const setup = createSetupKey({ rateLimit: 0 }); + expect(setup.rateLimit).toBe(0); + const session = exchangeSetupKey(setup.token)!; + expect(session.rateLimit).toBe(0); + }); + }); + describe('rotateRoot', () => { it('generates new root and invalidates all tokens', () => { const oldRoot = getRootToken(); diff --git a/browse/test/tunnel-revoke-cli.test.ts b/browse/test/tunnel-revoke-cli.test.ts index 20cb3d33c..4c82f96e5 100644 --- a/browse/test/tunnel-revoke-cli.test.ts +++ b/browse/test/tunnel-revoke-cli.test.ts @@ -97,6 +97,50 @@ describe('tunnel subcommand parsing', () => { }, 60_000); }); +// pair-agent scope-flag misuse shares this file's subprocess harness: like +// tunnel, the validation must run pre-server, so "no daemon spawned" is the +// load-bearing assertion. +describe('pair-agent scope-flag validation (pre-server)', () => { + test('bare --restrict → exit 1 usage error, NO daemon spawned (was: silent FULL grant)', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-restrict-bare-')); + const stateFile = path.join(tmpDir, 'browse.json'); + try { + const result = await runCli(['pair-agent', '--restrict'], baseEnv(stateFile)); + expect(result.code).toBe(1); + expect(result.stderr).toContain('--restrict needs a scope list'); + expect(fs.existsSync(stateFile)).toBe(false); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); + + test('--restrict swallowing the next flag → exit 1, NO daemon spawned', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-restrict-flag-')); + const stateFile = path.join(tmpDir, 'browse.json'); + try { + const result = await runCli(['pair-agent', '--restrict', '--client', 'bob'], baseEnv(stateFile)); + expect(result.code).toBe(1); + expect(result.stderr).toContain('--restrict needs a scope list'); + expect(fs.existsSync(stateFile)).toBe(false); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); + + test('--restrict "read,control" → exit 1 pointing at --control, NO daemon spawned', async () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-restrict-ctl-')); + const stateFile = path.join(tmpDir, 'browse.json'); + try { + const result = await runCli(['pair-agent', '--restrict', 'read,control'], baseEnv(stateFile)); + expect(result.code).toBe(1); + expect(result.stderr).toContain('Re-run with --control'); + expect(fs.existsSync(stateFile)).toBe(false); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); +}); + describe('tunnel against no daemon (#2254 — never boot one)', () => { test('revoke → exit 0, "No daemon running", nothing spawned', async () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-tunnel-dead-'));