From d86edf1f8f6b4fb59d9d7a42f1507923bba6ddac Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 20 Aug 2026 22:46:48 +0000 Subject: [PATCH] fix(pairing): harden re-pair per adversarial review Adversarial review of the diff found four issues, now fixed: - Validate the requested grant BEFORE the supersede revoke: a reducing re-pair with a bad scope/rate no longer destroys the live session and then fails to mint a replacement (assertValidTokenOptions runs up front). - A re-pair with no live session releases tabs orphaned by an expired incarnation, closing the tab-inheritance gap /pair had (DELETE /token already released unconditionally). - Test the DELETE /token revoked=0/tabs>0 path and the /pair orphaned-tab release at the handler level (HTTP e2e can't, headless owns no tabs). - Test the CLI --client root fast-fail; fix its null-guard (parseFlag returns null when --client is absent). Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 4 +-- browse/src/cli.ts | 2 +- browse/src/server.ts | 14 ++++++++++- browse/src/token-registry.ts | 2 +- browse/test/pair-agent-e2e.test.ts | 18 ++++++++++++++ browse/test/server-factory.test.ts | 35 +++++++++++++++++++++++++++ browse/test/tunnel-revoke-cli.test.ts | 15 ++++++++++++ 7 files changed, 85 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0f611701..5598a0069 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,9 +26,9 @@ Re-pair is now the real tightening lever. Re-pair an agent with its **same `--cl ### Itemized changes #### Fixed -- A reducing re-pair (`/pair` with fewer scopes, tighter domains, a lower rate, or a stricter tab policy) revokes the client's live session and releases its tabs before minting the new key; the response carries `superseded`. Non-reducing re-pairs keep the session and only drop stale pending setup keys, so a broaden or refresh never strands a working agent. (`browse/src/server.ts`, `browse/src/token-registry.ts`) +- A reducing re-pair (`/pair` with fewer scopes, tighter domains, a lower rate, or a stricter tab policy) revokes the client's live session and releases its tabs before minting the new key; the response carries `superseded`. Non-reducing re-pairs keep the session and only drop stale pending setup keys, so a broaden or refresh never strands a working agent. The requested grant is validated before any revoke, so a re-pair with a bad scope or rate is rejected without knocking the live session offline. (`browse/src/server.ts`, `browse/src/token-registry.ts`) - A narrowing re-pair issued before the agent connects invalidates the earlier, broader setup key, so it can no longer be exchanged. (`browse/src/token-registry.ts`) -- Revoking an agent releases the tab ownership it held: `DELETE /token` runs the release unconditionally (ownership outlives the token) and reports `tabs_released`, and an own-only client re-paired under the same name can no longer read those tabs. (`browse/src/browser-manager.ts`, `browse/src/server.ts`) +- Revoking an agent releases the tab ownership it held: `DELETE /token` runs the release unconditionally (ownership outlives the token) and reports `tabs_released`, and an own-only client re-paired under the same name can no longer read those tabs. A re-pair with no live session likewise frees any tabs orphaned by an expired incarnation, so a fresh session can't inherit them. (`browse/src/browser-manager.ts`, `browse/src/server.ts`) - `root` is rejected as a `clientId` at every token writer, so a scoped token can never carry the sentinel that bypasses scope, domain, rate, and tab checks; `/pair` and `/token` return a named 400 and the CLI rejects `--client root` before it reaches the daemon. A persisted `root` entry is skipped when the registry is restored. (`browse/src/token-registry.ts`, `browse/src/cli.ts`) #### For contributors diff --git a/browse/src/cli.ts b/browse/src/cli.ts index a76686df0..55c69c670 100644 --- a/browse/src/cli.ts +++ b/browse/src/cli.ts @@ -1229,7 +1229,7 @@ function validatePairAgentFlags(args: string[]): void { // 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') { + if (client && client.trim().toLowerCase() === 'root') { console.error("[browse] --client 'root' is reserved — it would bypass all scope enforcement. Choose another name."); process.exit(1); } diff --git a/browse/src/server.ts b/browse/src/server.ts index e368ede93..47c2e2e4d 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -33,7 +33,7 @@ import { listTokens, recordCommand, isRootToken, checkConnectRateLimit, type TokenInfo, type ScopeCategory, DEFAULT_PAIR_SCOPES, InvalidScopeError, ReservedClientIdError, assertValidClientId, - revokeSetupKeys, getClientSession, grantReducesAccess, + assertValidTokenOptions, revokeSetupKeys, getClientSession, grantReducesAccess, } from './token-registry'; import { validateTempPath } from './path-security'; import { resolveConfig, ensureStateDir, readVersionHash, resolveChromiumProfile, cleanSingletonLocks, isPairAgentEnabled } from './config'; @@ -2431,6 +2431,12 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { rateLimit: pairBody.rateLimit ?? 10, tabPolicy: 'own-only' as const, }; + // Validate BEFORE any revoke (createSetupKey validates too, but that + // runs after the teardown below). A bad scope or negative rateLimit + // must 400 without knocking a live session offline — otherwise a + // reducing re-pair with a typo (--restrict red) destroys the session + // and mints no replacement. + assertValidTokenOptions(grant.scopes, grant.rateLimit); const priorSession = pairBody.clientId ? getClientSession(pairBody.clientId) : null; let superseded: { tokens_deleted: number; tabs_released: number } | undefined; if (priorSession && grantReducesAccess(priorSession, grant)) { @@ -2440,6 +2446,12 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { console.log(`[browse] Superseded ${tokensDeleted} token(s), released ${tabsReleased} tab(s) for reducing re-pair: ${pairBody.clientId}`); } else if (pairBody.clientId) { revokeSetupKeys(pairBody.clientId); + // No live session, but tab ownership outlives token expiry: free any + // tabs orphaned by an expired session so this re-pair can't inherit + // an earlier incarnation's authenticated pages (mirrors DELETE + // /token's unconditional release). A live-session broaden keeps its + // tabs — the working agent still owns them. + if (!priorSession) browserManager.releaseClientTabs(pairBody.clientId); } const setupKey = createSetupKey({ clientId: pairBody.clientId, diff --git a/browse/src/token-registry.ts b/browse/src/token-registry.ts index 7e5d9ace7..89b0a990c 100644 --- a/browse/src/token-registry.ts +++ b/browse/src/token-registry.ts @@ -106,7 +106,7 @@ export const DEFAULT_PAIR_SCOPES: readonly ScopeCategory[] = ['read', 'write', ' */ export class InvalidScopeError extends Error {} -function assertValidTokenOptions(scopes: readonly string[], rateLimit: number): void { +export 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)) { diff --git a/browse/test/pair-agent-e2e.test.ts b/browse/test/pair-agent-e2e.test.ts index 953bc0299..3891c3c11 100644 --- a/browse/test/pair-agent-e2e.test.ts +++ b/browse/test/pair-agent-e2e.test.ts @@ -373,6 +373,24 @@ describe('pair-agent flow end-to-end (HTTP only, no ngrok)', () => { expect((await statusWith(s)).status).not.toBe(401); // still working }); + test('a reducing re-pair with an INVALID scope 400s and leaves the live session intact', async () => { + // Regression: the supersede revoke must run AFTER validation. A scope typo + // (--restrict red) on a narrowing re-pair must not destroy the session and + // then fail to mint a replacement — the agent would be knocked offline. + const { setup_key: k } = await pairAs({ clientId: 'validate-me' }); + const { body: c } = await connectKey(k); + const s = c.token as string; + expect((await statusWith(s)).status).not.toBe(401); + const resp = await fetch(`${daemon.baseUrl}/pair`, { + method: 'POST', headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${daemon.token}` }, + body: JSON.stringify({ clientId: 'validate-me', scopes: ['red'] }), + }); + expect(resp.status).toBe(400); + expect((await resp.json() as any).error).toContain('red'); + // The working session survives the validation error (not revoked). + expect((await statusWith(s)).status).not.toBe(401); + }); + // ─── D3: DELETE /token releases tabs unconditionally; 404 only when empty ─ test('DELETE /token returns tabs_released and 404 only when nothing to revoke or release', async () => { diff --git a/browse/test/server-factory.test.ts b/browse/test/server-factory.test.ts index 474fa17e7..b6c1dc44d 100644 --- a/browse/test/server-factory.test.ts +++ b/browse/test/server-factory.test.ts @@ -370,6 +370,41 @@ describe('buildFetchHandler factory contract', () => { initRegistry('first-token-pad-to-16-chars'); expect(() => initRegistry('second-token-pad-to-16-chars')).toThrow(/already initialized/i); }); + + // D3 handler gating: tab ownership outlives token expiry, so DELETE /token + // must release tabs and 200 even when no token remains (revoked=0, tabs>0). + // Drives the handler into that exact state — HTTP e2e can't (headless-skip + // owns no real tabs), so the revoke-gated-release regression would pass there. + test('13. DELETE /token releases orphaned tabs and 200s when no token remains', async () => { + const cfg = makeMinimalConfig(); + (cfg.browserManager as any).tabOwnership.set(7, 'ghost'); // owned, no token + const handle = buildFetchHandler(cfg); + const req = new Request('http://127.0.0.1/token/ghost', { + method: 'DELETE', headers: { Authorization: `Bearer ${cfg.authToken}` }, + }); + const resp = await handle.fetchLocal(req, null); + expect(resp.status).toBe(200); + const body = await resp.json() as { tokens_deleted: number; tabs_released: number }; + expect(body.tokens_deleted).toBe(0); + expect(body.tabs_released).toBe(1); + expect(cfg.browserManager.getTabOwner(7)).toBeNull(); + }); + + // D1 F2: a re-pair with no LIVE session must release tabs orphaned by an + // expired incarnation, or the new session inherits its authenticated pages. + test('14. /pair releases tabs orphaned by an expired session (no inheritance)', async () => { + const cfg = makeMinimalConfig(); + (cfg.browserManager as any).tabOwnership.set(9, 'ghost'); // orphaned, no live session + const handle = buildFetchHandler(cfg); + const req = new Request('http://127.0.0.1/pair', { + method: 'POST', + headers: { Authorization: `Bearer ${cfg.authToken}`, 'Content-Type': 'application/json' }, + body: JSON.stringify({ clientId: 'ghost', scopes: ['read'] }), + }); + const resp = await handle.fetchLocal(req, null); + expect(resp.status).toBe(200); + expect(cfg.browserManager.getTabOwner(9)).toBeNull(); + }); }); // ─── Idle timer + onDisconnect dual-instance fix (v1.42.3.0) ────────── diff --git a/browse/test/tunnel-revoke-cli.test.ts b/browse/test/tunnel-revoke-cli.test.ts index 0f5d9b94d..251c12880 100644 --- a/browse/test/tunnel-revoke-cli.test.ts +++ b/browse/test/tunnel-revoke-cli.test.ts @@ -155,6 +155,21 @@ describe('pair-agent scope-flag validation (pre-server)', () => { fs.rmSync(tmpDir, { recursive: true, force: true }); } }, 30_000); + + test('--client root → exit 1 reserved-name error, NO daemon spawned', async () => { + // `root` is the sentinel that bypasses every scope/domain/rate/tab check; + // the CLI must reject it pre-server (the daemon also 400s it as defense-in-depth). + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'browse-client-root-')); + const stateFile = path.join(tmpDir, 'browse.json'); + try { + const result = await runCli(['pair-agent', '--client', 'root'], baseEnv(stateFile)); + expect(result.code).toBe(1); + expect(result.stderr).toContain('reserved'); + 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)', () => {