From 1ae9aef9996867e61e05d856aa75832b8e4913f1 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 20 Aug 2026 04:29:07 +0000 Subject: [PATCH] =?UTF-8?q?fix(browse):=20adversarial-review=20hardening?= =?UTF-8?q?=20=E2=80=94=206=20findings=20fixed,=20regression-pinned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-push adversarial review (4 lenses, refute-style verification: 13 raw findings, 7 refuted, 6 confirmed) caught these; each fix carries a pin: 1. --restrict=read (equals form) sailed past validatePairAgentFlags — hasFlag/parseFlag are exact-token matches — so the user asked for a read-only sandbox and silently got FULL access: the exact failure mode this branch claims to close. The equals form is now a hard error before any server work. 2. handleTunnel trimmed the agent name but clientIds are stored verbatim, so a space-padded agent was unrevocable by the documented kill switch (trimmed DELETE 404'd while the grant stayed live). Names now pass through verbatim; the live-daemon test revokes ' padded'. 3. The sole pin for "CLI always sends explicit scopes" passed vacuously on a simulated revert: toContain('DEFAULT_PAIR_SCOPES') was satisfied by a comment. The tripwire now matches the code shape with a regex and bans the conditional spread formatting-insensitively. 4. The rewritten 403 scope hint was unpinned — new e2e asserts it names --restrict and --control and never --admin. 5. tunnelRevoke's verify-failure and HTTP-error branches and tunnelAgents' unreadable-list branch had no coverage — three stub-daemon pins added (an unreadable list must never render as "No paired agents"). 6. CHANGELOG claimed "40+ new test cases"; the honest count is 35. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 2 +- browse/src/cli.ts | 12 ++- browse/test/pair-agent-e2e.test.ts | 27 ++++++ browse/test/server-auth.test.ts | 6 +- browse/test/tunnel-revoke-cli.test.ts | 114 ++++++++++++++++++++++++++ 5 files changed, 156 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08d42af24..df5b486f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,7 +42,7 @@ Revoke means revoked: one command deletes the session and every setup key, print - `DELETE /token/:id` decodes percent-encoded client ids, so names with spaces round-trip from the CLI. ### For contributors -- 40+ new test cases: revoke-all regression shapes, a subprocess CLI harness with stub daemons pinning the version-skew net ("Revocation incomplete" on a lying daemon), e2e scope-contract pins, and source tripwires for `DEFAULT_PAIR_SCOPES` and the decode path. +- 35 new test cases: revoke-all regression shapes, a subprocess CLI harness with stub daemons pinning the version-skew net ("Revocation incomplete" on a lying daemon) and every CLI error branch, e2e scope-contract and 403-hint pins, and code-shape tripwires for `DEFAULT_PAIR_SCOPES` and the decode path. ## [1.68.1.0] - 2026-08-18 diff --git a/browse/src/cli.ts b/browse/src/cli.ts index 1d1cc7d19..8a361fa95 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -1225,6 +1225,12 @@ 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 { + // 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='))) { + console.error('[browse] --restrict takes a space-separated value: --restrict read or --restrict "read,write". The --restrict=... form is not supported.'); + process.exit(1); + } if (!hasFlag(args, '--restrict')) return; const restrict = parseFlag(args, '--restrict'); if (!restrict || !restrict.trim() || restrict.startsWith('--')) { @@ -1244,8 +1250,10 @@ function validatePairAgentFlags(args: string[]): void { async function handleTunnel(args: string[]): Promise { const sub = args[0]; - if (sub === 'revoke' && args.length === 2 && args[1].trim()) { - process.exit(await tunnelRevoke(args[1].trim())); + // The name passes through VERBATIM: clientIds are stored untrimmed, so a + // space-padded name must stay revocable (encodeURIComponent handles it). + if (sub === 'revoke' && args.length === 2 && args[1]) { + process.exit(await tunnelRevoke(args[1])); } if (sub === 'agents' && args.length === 1) { process.exit(await tunnelAgents()); diff --git a/browse/test/pair-agent-e2e.test.ts b/browse/test/pair-agent-e2e.test.ts index a5a2f6d9d..5a59dce4e 100644 --- a/browse/test/pair-agent-e2e.test.ts +++ b/browse/test/pair-agent-e2e.test.ts @@ -264,6 +264,33 @@ describe('pair-agent flow end-to-end (HTTP only, no ngrok)', () => { expect(body.error).toContain('control'); }); + test('scope-denied 403 hint points at --restrict/--control, never --admin', async () => { + // Regression: the old hint said "re-pair with --admin", which is a legacy + // alias for --control — following it over-granted browser-wide control. + const pairResp = await fetch(`${daemon.baseUrl}/pair`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'hint-agent', 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 { token } = await connectResp.json() as any; + const resp = await fetch(`${daemon.baseUrl}/command`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` }, + body: JSON.stringify({ command: 'goto', args: ['https://example.com'] }), + }); + expect(resp.status).toBe(403); + const body = await resp.json() as any; + expect(body.hint).toContain('--restrict'); + expect(body.hint).toContain('--control'); + expect(body.hint).not.toContain('--admin'); + }); + // ─── 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 5bf906838..5949f1f13 100644 --- a/browse/test/server-auth.test.ts +++ b/browse/test/server-auth.test.ts @@ -420,8 +420,10 @@ describe('Pair scope defaults and revocation surface', () => { 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'); + // Match the CODE shape, not a comment: a bare toContain('DEFAULT_PAIR_SCOPES') + // is satisfied by the explanatory comment and passes vacuously on a revert. + expect(cliBlock).toMatch(/scopes:\s*restrict\s*\?[\s\S]{0,200}?:\s*\[\.\.\.DEFAULT_PAIR_SCOPES\]/); + expect(cliBlock).not.toMatch(/\.\.\.\(restrict\s*\?/); }); // control is the only scope behind an explicit flag; a scopes list must diff --git a/browse/test/tunnel-revoke-cli.test.ts b/browse/test/tunnel-revoke-cli.test.ts index 4c82f96e5..0f5d9b94d 100644 --- a/browse/test/tunnel-revoke-cli.test.ts +++ b/browse/test/tunnel-revoke-cli.test.ts @@ -114,6 +114,22 @@ describe('pair-agent scope-flag validation (pre-server)', () => { } }, 30_000); + test('--restrict=read (equals form) → exit 1, NO daemon spawned', async () => { + // Regression: hasFlag/parseFlag are exact-token matches, so the equals + // form sailed past validatePairAgentFlags AND handlePairAgent's parse — + // the user asked for a read-only sandbox and silently got FULL access. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-restrict-eq-')); + const stateFile = path.join(tmpDir, 'browse.json'); + try { + const result = await runCli(['pair-agent', '--restrict=read'], baseEnv(stateFile)); + expect(result.code).toBe(1); + expect(result.stderr).toContain('--restrict takes a space-separated value'); + 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'); @@ -248,6 +264,19 @@ describe('tunnel against a live daemon (HTTP only, no browser)', () => { const empty = await runCli(['tunnel', 'agents'], baseEnv(stateFile)); expect(empty.code).toBe(0); expect(empty.stdout).toContain('No paired agents.'); + + // Regression: handleTunnel used to trim() the name, but clientIds are + // stored verbatim — a space-padded agent became unrevocable by the + // documented kill switch (trimmed DELETE 404'd while the grant lived). + const padPair = await fetch(`${baseUrl}/pair`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${rootToken}` }, + body: JSON.stringify({ clientId: ' padded' }), + }); + expect(padPair.status).toBe(200); + const padRevoke = await runCli(['tunnel', 'revoke', ' padded'], baseEnv(stateFile)); + expect(padRevoke.code).toBe(0); + expect(padRevoke.stdout).toContain('Verified: not in the active agent list.'); } finally { try { daemon.kill('SIGKILL'); } catch { /* already gone */ } fs.rmSync(tmpDir, { recursive: true, force: true }); @@ -289,6 +318,91 @@ describe('tunnel against a lying or unreachable daemon (stub harness)', () => { } }, 30_000); + test('DELETE succeeds but /agents errors → "could not verify", exit 1 (never a silent success)', async () => { + // Regression pin: a revoke whose verification read fails must NOT report + // clean success — the whole point of the re-read is proving the deletion. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-tunnel-noverify-')); + const stateFile = path.join(tmpDir, 'browse.json'); + const stub = Bun.serve({ + hostname: '127.0.0.1', + port: 0, + fetch(req) { + const url = new URL(req.url); + if (req.method === 'DELETE' && url.pathname.startsWith('/token/')) { + return Response.json({ revoked: 'mallory', tokens_deleted: 1 }); + } + if (url.pathname === '/agents') { + return new Response('boom', { status: 500 }); + } + return Response.json({ status: 'healthy' }); + }, + }); + try { + writeStateFile(stateFile, process.pid, stub.port); + const result = await runCli(['tunnel', 'revoke', 'mallory'], baseEnv(stateFile)); + expect(result.code).toBe(1); + expect(result.stderr).toContain('Revoked, but could not verify against the agent list.'); + } finally { + stub.stop(true); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); + + test('DELETE returns 500 → "Revoke failed" with the body error, exit 1', async () => { + // Regression pin: a non-404 HTTP failure must surface the daemon's error, + // not fall through to a success print. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-tunnel-500-')); + const stateFile = path.join(tmpDir, 'browse.json'); + const stub = Bun.serve({ + hostname: '127.0.0.1', + port: 0, + fetch(req) { + const url = new URL(req.url); + if (req.method === 'DELETE' && url.pathname.startsWith('/token/')) { + return Response.json({ error: 'registry exploded' }, { status: 500 }); + } + return Response.json({ status: 'healthy' }); + }, + }); + try { + writeStateFile(stateFile, process.pid, stub.port); + const result = await runCli(['tunnel', 'revoke', 'mallory'], baseEnv(stateFile)); + expect(result.code).toBe(1); + expect(result.stderr).toContain('Revoke failed: registry exploded'); + } finally { + stub.stop(true); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); + + test('tunnel agents with a broken /agents → "Could not read the agent list", exit 1', async () => { + // Regression pin: a 500 from /agents must not render as "No paired + // agents." — an unreadable list is not an empty list. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-tunnel-agents500-')); + const stateFile = path.join(tmpDir, 'browse.json'); + const stub = Bun.serve({ + hostname: '127.0.0.1', + port: 0, + fetch(req) { + const url = new URL(req.url); + if (url.pathname === '/agents') { + return new Response('boom', { status: 500 }); + } + return Response.json({ status: 'healthy' }); + }, + }); + try { + writeStateFile(stateFile, process.pid, stub.port); + const result = await runCli(['tunnel', 'agents'], baseEnv(stateFile)); + expect(result.code).toBe(1); + expect(result.stderr).toContain('Could not read the agent list'); + expect(result.stdout).not.toContain('No paired agents.'); + } finally { + stub.stop(true); + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }, 30_000); + test('alive pid but unreachable port → "Could not reach daemon", exit 1 (NOT "no daemon")', async () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-tunnel-unreach-')); const stateFile = path.join(tmpDir, 'browse.json');