v1.68.2.0 fix: tunnel revoke exists and revokes everything — setup keys included, verified live (#2646)

* fix(browse): revokeToken deletes ALL tokens for a clientId, not the first Map hit revokeToken deleted the first Map entry matching the clientId and returned true. After a normal pairing, two entries share one clientId: the spent setup key (kept by exchangeSetupKey for idempotent re-exchange) and the session token, in that insertion order. Revoke ate the setup key, reported success, and the live session survived: DELETE /token/<id> returned a false 200 while /agents kept listing the agent. Worse, an unspent setup key created after the session survived revoke, so a "revoked" agent could POST /connect and mint a fresh session within the key's 5-minute validity window. revokeToken now deletes every matching entry and returns the delete count (truthy-compatible with the old boolean). The DELETE /token handler logs "Revoked N token(s)" and returns tokens_deleted so the multi-token class stays visible; revokeSkillToken wraps Boolean() to keep its documented contract. Regression tests pin shapes a (spent-key shadowing), b (re-grant hole), c (multiple pending keys), and bystander isolation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(browse): tunnel revoke/agents CLI with post-revoke verification `$B tunnel revoke <name>` was documented in the instruction block, pair-agent/SKILL.md, and REMOTE_BROWSER_ACCESS.md but implemented nowhere: the CLI forwarded it to the daemon as Unknown command 'tunnel', and nothing in the repo called DELETE /token/:clientId or GET /agents. New pre-server short-circuit (#2254 pattern: tokens are memory-only, never boot a daemon to revoke against it). `tunnel revoke <name>` DELETEs the token, prints the deleted count ("(count unknown)" for old daemons that answer {revoked} without tokens_deleted), then RE-READS GET /agents to prove the agent is gone. The still-listed branch is the version-skew net: a new CLI against a still-running old daemon with the first-match revoke bug exits 1 and says to re-run (each old-daemon call deletes the next match) or stop. An alive pid with an unreachable port reports "Could not reach daemon" (exit 1), never a false "no daemon". `tunnel agents` lists sessions plus pending (unexchanged) setup keys, which GET /agents now exposes via listTokens({includeSetup}) — without them the revocation view was blind to a paired-but-never-connected agent. Setup-key tokens never leave the server. DELETE /token/ now decodeURIComponents the clientId (400 on malformed encoding) so CLI-encoded names round-trip. Tests: subprocess CLI coverage (usage paths, no-daemon exit 0 without spawning, live pair/connect/revoke loop, pending-key listing), stub-daemon pins for the skew and unreachable branches, and e2e pins for revoke-all semantics, percent-encoded ids, and the second-DELETE-is-404 regression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(browse): CLI always sends explicit pair scopes via shared DEFAULT_PAIR_SCOPES The effective pairing default lived in two places: the CLI omitted scopes unless --restrict was passed, and the server filled in its own literal. handlePairAgent now always sends an explicit scopes list and both sides reference one exported constant, DEFAULT_PAIR_SCOPES, so the default cannot silently drift again (pinned by a server-auth source tripwire). Three input traps closed in the same surface: - Bare --restrict (or --restrict swallowing the next flag) parsed as "no restriction" and silently granted FULL access, the opposite of the user's intent. validatePairAgentFlags rejects it pre-server, before any consent gate, so an arg error never boots a daemon. - A scopes list could smuggle the control scope past the explicit flag: --restrict "read,control" minted a control-scoped session with no --control. /pair now 400s on control in a scopes list without the control flag, and the CLI points the user at --control. - Option typos validated only at exchange time: createSetupKey stored any scope string and any rateLimit, so /pair returned 200 with a poisoned setup key whose failure surfaced to the REMOTE agent at /connect as a misleading "Invalid request body". Shared validation now runs in both creators and throws typed InvalidScopeError; /pair and /token 400 with the message, naming the bad scope or negative rateLimit. Also `opts.rateLimit || 10` became `?? 10` so the documented "0 = unlimited" survives the /pair path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(browse): 403 hint stops recommending --admin; invariant names both scope defaults The scope-denied hint told restricted agents to "re-pair with --admin for eval/cookies/storage" — but --admin is a legacy alias for --control, so following it over-granted browser-wide destructive commands on top of the admin scope the default already carries. The hint now matches the CLI's sibling wording: re-pair without --restrict for page access, --control for browser control. Registry invariant #2 claimed "admin scope denied by default" three releases after b73f3644 deliberately made /pair grant admin. It now names BOTH defaults precisely (registry API functions default read+write; the /pair ceremony grants DEFAULT_PAIR_SCOPES) so the header cannot lie one layer down. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(pair-agent): document the full-access default, --restrict, and real revocation The pairing docs still described the pre-b73f3644 model: read+write default, --admin as the opt-in for JS/cookies/storage. Reality for three releases: /pair grants read+write+admin+meta (the pairing ceremony is the trust boundary) and --admin is a legacy alias for --control. A user following the skill believed they granted a sandboxed session and actually granted JS execution on their logged-in browser. pair-agent/SKILL.md.tmpl (SKILL.md regenerated in this commit) now states the real default, the tunnel-allowlist nuance (eval works remotely; the js/cookies/storage commands are local-only), --restrict for sandboxed sessions with an untrusted-content advisory (scope caps prompt-injection blast radius), and --control for browser-wide ops. "Revoking access" documents the now-real tunnel revoke (deletes session + pending setup keys, verifies against the agent list) and tunnel agents, and replaces the never-implemented `tunnel rotate` with `$B stop` — tokens are memory-only, so a daemon restart already rotates everything. REMOTE_BROWSER_ACCESS.md: /connect example shows the real default scopes, the scope table gains the control row, the 403 hint row matches the new server wording, and the false claim that /sidebar-chat is on the tunnel allowlist is gone (TUNNEL_PATHS is /connect + /command; /sidebar-chat no longer exists in server.ts at all). ARCHITECTURE.md drops the same phantom endpoint from the allowlist prose and endpoint table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * v1.68.2.0: revoke-all, real tunnel revoke, truthful pairing docs Version slot allocated against the live remote via bin/gstack-next-version (clean patch bump from 1.68.1.0, no collision). CHANGELOG entry covers the revoke-all fix, the new tunnel revoke/agents CLI, the explicit-scopes wire contract, and the pairing-docs truth pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(browse): adversarial-review hardening — 6 findings fixed, regression-pinned 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
2026-08-29 09:20:39 +02:00 · 2026-08-19 22:33:46 -07:00
parent 9da6692930
commit 51932eceef
15 changed files with 1186 additions and 70 deletions
@@ -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';

@@ -298,12 +299,81 @@ describe('token-registry', () => {
  describe('revokeToken', () => {
    it('revokes existing token', () => {
      const info = createToken({ clientId: 'to-revoke' });
-      expect(revokeToken('to-revoke')).toBe(true);
+      // revokeToken returns the delete count, not a boolean (truthy for callers)
+      expect(revokeToken('to-revoke')).toBe(1);
      expect(validateToken(info.token)).toBeNull();
    });

-    it('returns false for non-existent client', () => {
-      expect(revokeToken('no-such-client')).toBe(false);
+    it('returns 0 for non-existent client', () => {
+      expect(revokeToken('no-such-client')).toBe(0);
+    });
+
+    // Regression: revokeToken deleted only the FIRST matching Map entry. The
+    // spent setup key (kept for idempotent re-exchange) is inserted before the
+    // session token, so it shadowed the session: revoke reported success while
+    // the live session survived and DELETE /token returned a false 200.
+    it('revokes the session even when a spent setup key precedes it (shape a)', () => {
+      const setup = createSetupKey({ clientId: 'shadowed' });
+      const session = exchangeSetupKey(setup.token)!;
+      expect(revokeToken('shadowed')).toBe(2);
+      expect(validateToken(session.token)).toBeNull();
+      expect(exchangeSetupKey(setup.token)).toBeNull();
+    });
+
+    // Regression: an UNSPENT setup key created after the session survived the
+    // old first-match revoke, so a "revoked" agent could POST /connect and
+    // mint a brand-new session within the key's 5-minute validity window.
+    it('closes the re-grant hole: unspent setup key dies with the revoke (shape b)', () => {
+      const first = createSetupKey({ clientId: 'regrant' });
+      exchangeSetupKey(first.token);
+      const second = createSetupKey({ clientId: 'regrant' });
+      expect(revokeToken('regrant')).toBe(3);
+      expect(exchangeSetupKey(second.token)).toBeNull();
+      expect(listTokens().filter(t => t.clientId === 'regrant')).toHaveLength(0);
+    });
+
+    it('revokes multiple pending setup keys for one clientId in a single call (shape c)', () => {
+      const keys = [1, 2, 3].map(() => createSetupKey({ clientId: 'multi' }));
+      expect(revokeToken('multi')).toBe(3);
+      for (const k of keys) expect(exchangeSetupKey(k.token)).toBeNull();
+      expect(revokeToken('multi')).toBe(0); // idempotent: second call finds nothing
+    });
+
+    it('does not touch other clients\' tokens', () => {
+      const bystander = createToken({ clientId: 'bystander' });
+      createSetupKey({ clientId: 'target' });
+      createToken({ clientId: 'target' });
+      expect(revokeToken('target')).toBe(2);
+      expect(validateToken(bystander.token)).not.toBeNull();
+    });
+  });
+
+  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);
    });
  });

@@ -326,6 +396,18 @@ describe('token-registry', () => {
      createSetupKey({}); // setup keys not listed
      expect(listTokens()).toHaveLength(2);
    });
+
+    it('includeSetup lists pending setup keys but hides spent ones', () => {
+      createToken({ clientId: 'sess' });
+      createSetupKey({ clientId: 'pending' });
+      const spent = createSetupKey({ clientId: 'spent' });
+      exchangeSetupKey(spent.token);
+      expect(listTokens().map(t => t.clientId).sort()).toEqual(['sess', 'spent']);
+      const withSetup = listTokens({ includeSetup: true });
+      // Pending key = a live grant the operator must see; the SPENT key is
+      // re-exchange bookkeeping for the already-listed session and stays hidden.
+      expect(withSetup.filter(t => t.type === 'setup').map(t => t.clientId)).toEqual(['pending']);
+    });
  });

  describe('serialization', () => {