Repository navigation
v1.68.2.0 fix: tunnel revoke exists and revokes everything — setup keys included, verified live - #2646
Merged
Conversation
…irst 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>
`$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>
…_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>
…h 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 b73f364 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>
…eal 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>
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>
…ion-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>
|
😎 This pull request was merged. |
Contributor
E2E Evals: ✅ PASS8/8 tests passed | $1.48 total cost
ubicloud-standard-8 runners (Docker: pre-baked toolchain + deps) | wall clock ≈ slowest suite |
4 of 5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why (in your own words)
Revoking a paired agent was a lie twice over.
revokeTokendeleted only the first Map entry for a clientId, and the spent setup key kept for connection retries always sat first in line:DELETE /token/<agent>returned 200 while the live session kept driving the browser, a leftover unspent setup key could mint a brand-new session for a "revoked" agent inside the key's 5-minute window, and a second DELETE returned 200 again. The documented way out,$B tunnel revoke, did not exist — the CLI forwarded it to the daemon asUnknown command: 'tunnel'. And the pairing docs still promised a read+write sandbox ("Cannot execute arbitrary JavaScript, read cookies, or access storage") three releases after b73f364 deliberately made pairing full-access, so a user following the skill believed they granted a sandboxed session and actually granted JS execution on their logged-in browser.This PR makes revocation total and verified, makes the documented CLI real, pins the pairing default in one shared constant, and rewrites the docs to the truth. Along the way it closes four adjacent traps found during review: bare
--restrictsilently granted FULL access (the opposite of intent), a scopes list could smuggle in thecontrolscope, scope typos surfaced as a misleading error to the remote agent at/connect, andrateLimit: 0(documented "unlimited") silently became 10 on the/pairpath.Live evidence
BEFORE — origin/main @ 9da6692,
BROWSE_HEADLESS_SKIP=1daemon,mallorypaired + connected, second setup key pending:AFTER — this branch, same ceremony:
Fail-first: the shape a/b regression tests run against origin/main fail (0 pass, 2 fail — session survives revoke; leftover key re-connects); on this branch the committed versions pass.
Targeted suite on the branch:
bun test browse/test/token-registry.test.ts browse/test/pair-agent-e2e.test.ts browse/test/tunnel-revoke-cli.test.ts browse/test/server-auth.test.ts browse/test/tab-isolation.test.ts browse/test/skill-token.test.ts→ 178 pass, 0 fail.bun run gen:skill-docs --host all→ zero drift.bun run build→ clean.Scope
browse/src/token-registry.ts(revoke-all + count,DEFAULT_PAIR_SCOPES,InvalidScopeError, shared option validation,listTokens({includeSetup}), invariant comment),browse/src/server.ts(/paircontrol gate + typed 400s,/agentspending keys,DELETE /tokendecode +tokens_deleted, 403 hint),browse/src/cli.ts(tunnel revoke/tunnel agentswith post-revoke verify,validatePairAgentFlags, explicit scopes),browse/src/skill-token.ts(Boolean wrap), docs (pair-agent/SKILL.md.tmpl+ regeneratedSKILL.md,docs/REMOTE_BROWSER_ACCESS.md,ARCHITECTURE.md), tests (35 new cases incl. a subprocess CLI harness with stub daemons pinning the version-skew net and every CLI error branch), VERSION/CHANGELOG.bun run build, and a pre-push adversarial review (4 lenses, refute-style verification: 13 raw findings, 7 refuted, 6 confirmed and fixed in the final commit — including an--restrict=readequals-form bypass of the new validation).TUNNEL_PATHSstill/connect+/command). The full free suite in my sandbox has pre-existing environment failures OUTSIDEbrowse/(relink/session-update/skill-validation fail identically or worse on untouched origin/main there — missingfilebinary, symlink collisions with a live gstack install); allbrowse/shards pass locally, and the authoritative full run is this PR'sfree-tests.ymllane.Notes for the reviewer:
/agentsre-read, exits 1, and suggests re-running (which converges on old daemons) orstop. Pinned by a stub-daemon test.POST /token(root direct mint) intentionally still accepts explicitcontrolin scopes — root minting explicit scopes is the deliberate path; only the/pairceremony gates it behind the control flag.Liveness proof (required)
Maintainer-authored PR, prepared by Claude Code in a cloud sandbox at the owner's request — there is no human screen in the loop to photograph. @garrytan: attach a
GSTACK PRscreenshot from your machine if you want the ritual observed for maintainer PRs, or waive it.Checklist
GSTACK PRtyped live into a real surface (owner to attach or waive — see above)tunnel revoke/tunnel agentsimplement the already-documented CLI surface)🤖 Generated with Claude Code