security: suppress /health token when a tunnel is active#1026
Closed
garagon wants to merge 1 commit into
Closed
Conversation
The v0.15.13.0 wave-3 fix that restricted AUTH_TOKEN on /health to chrome-extension:// origins was later refactored with an OR that re-allows the token whenever the daemon is in 'headed' connection mode. Headed mode is the default for every pair-agent and interactive Playwright session, and 'pair-agent --client' explicitly opens an ngrok tunnel via POST /tunnel/start. Put those two together and an unauthenticated remote curl https://xxx.ngrok-free.app/health returns {token: <root>} from the gstack daemon, handing a bearer token that accepts /command (arbitrary JS), /read (cookie/storage read), /upload (file write). Full browser takeover from one ngrok URL. Root cause is that 'headed' mode was trusted as a proxy for 'local only'. pair-agent breaks that assumption. Gate the headed branch on !tunnelActive — the module-level flag already tracks tunnel state for /tunnel/start and /tunnel/stop. Same-origin chrome-extension:// callers continue to receive the token because MV3 isolation makes that path localhost-safe regardless of tunnel state. Tests (browse/test/server-auth.test.ts): - Existing 'serves token in headed or chrome-extension' test still passes (expects headed + chrome-extension:// strings — both still present). - New source-integrity test '/health suppresses the token when a tunnel is active' asserts the /health block references tunnelActive and contains a negation (!tunnelActive or tunnelActive === false). Reverting server.ts to main turns this single test red; the fix makes it green. Reproduction before the fix: BROWSE_HEADED=1 bun run dev serve --tunnel & curl $(cat ~/.gstack/tunnel-url)/health | jq -r .token # → returns AUTH_TOKEN with no auth, from anywhere on the internet. After the fix: # same request returns { status, mode, uptime, tabs, ... } with no # token field regardless of connection mode. No API change. The token is still served for the legitimate cases (same-origin extension bootstrap, local headed development without an open tunnel).
garrytan
added a commit
that referenced
this pull request
Apr 22, 2026
Spawns the browse daemon as a subprocess with BROWSE_HEADLESS_SKIP=1 so
the HTTP layer runs without a real browser. Exercises:
* GET /health — token delivery for chrome-extension origin, withheld
otherwise (the F1 + PR #1026 invariant)
* GET /connect — alive probe returns {alive:true} unauth
* POST /pair — root Bearer required (403 without), returns setup_key
* POST /connect — setup_key exchange mints a distinct scoped token
* POST /command — 401 without auth
* POST /sse-session — Bearer required, Set-Cookie has HttpOnly +
SameSite=Strict (the N1 invariant)
* GET /activity/stream — 401 without auth
* GET /activity/stream?token= — 401 (the old ?token= query param is
REJECTED, which is the whole point of N1)
* GET /welcome — serves HTML, does not leak /etc/passwd content under
the default 'unknown' slug (E3 regex gate)
12 behavioral tests, ~220ms end-to-end, no network dependencies, no
ngrok, no real browser. This is the receipt for the wave's central
'pair-agent still works + the security boundary holds' claim.
Tunnel-port binding (/tunnel/start) is deliberately NOT exercised here
— it requires an ngrok authtoken and live network. The dual-listener
route allowlist is covered by source-level guards in
dual-listener.test.ts; behavioral tunnel testing belongs in a separate
paid-evals harness.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
garrytan
added a commit
that referenced
this pull request
Apr 22, 2026
…0) (#1137) * refactor(security): loosen /connect rate limit from 3/min to 300/min Setup keys are 24 random bytes (unbruteforceable), so a tight rate limit does not meaningfully prevent key guessing. It exists only to cap bandwidth, CPU, and log-flood damage from someone who discovered the ngrok URL. A legitimate pair-agent session hits /connect once; 300/min is 60x that pattern and never hit accidentally. 3/min caused pairing to fail on any retry flow (network blip, second paired client) with no upside. Per-IP tracking was considered and rejected — adds a bounded Map + LRU for defense already adequate at the global layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(security): add tunnel-denial-log module for attack visibility Append-only log of tunnel-surface auth denials to ~/.gstack/security/attempts.jsonl. Gives operators visibility into who is probing tunneled daemons so the next security wave can be driven by real attack data instead of speculation. Design notes: - Async via fs.promises.appendFile. Never appendFileSync — blocking the event loop on every denial during a flood is what an attacker wants (prior learning: sync-audit-log-io, 10/10 confidence). - In-process rate cap at 60 writes/minute globally. Excess denials are counted in memory but not written to disk — prevents disk DoS. - Writes to the same ~/.gstack/security/attempts.jsonl used by the prompt-injection attempt log. File rotation is handled by the existing security pipeline (10MB, 5 generations). No consumers in this commit; wired up in the dual-listener refactor that follows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(security): dual-listener tunnel architecture The /health endpoint leaked AUTH_TOKEN to any caller that hit the ngrok URL (spoofing chrome-extension:// origin, or catching headed mode). Surfaced by @garagon in PR #1026; the original fix was header-inference on the single port. Codex's outside-voice review during /plan-ceo-review called that approach brittle (ngrok header behavior could change, local proxies would false-positive), and pushed for the structural fix. This is that fix. Stop making /health a root-token bootstrap endpoint on any surface the tunnel can reach. The server now binds two HTTP listeners when a tunnel is active. The local listener (extension, CLI, sidebar) stays on 127.0.0.1 and is never exposed to ngrok. ngrok forwards only to the tunnel listener, which serves only /connect (unauth, rate-limited) and /command with a locked allowlist of browser-driving commands. Security property comes from physical port separation, not from header inference — a tunnel caller cannot reach /health or /cookie-picker or /inspector because they live on a different TCP socket. What this commit adds to browse/src/server.ts: * Surface type ('local' | 'tunnel') and TUNNEL_PATHS + TUNNEL_COMMANDS allowlists near the top of the file. * makeFetchHandler(surface) factory replacing the single fetch arrow; closure-captures the surface so the filter that runs before route dispatch knows which socket accepted the request. * Tunnel filter at dispatch entry: 404s anything not on TUNNEL_PATHS, 403s root-token bearers with a clear pairing hint, 401s non-/connect requests that lack a scoped token. Every denial is logged via logTunnelDenial (from tunnel-denial-log). * GET /connect alive probe (unauth on both surfaces) so /pair and /tunnel/start can detect dead ngrok tunnels without reaching /health — /health is no longer tunnel-reachable. * Lazy tunnel listener lifecycle. /tunnel/start binds a dedicated Bun.serve on an ephemeral port, points ngrok.forward at THAT port (not the local port), hard-fails on bind error (no local fallback), tears down cleanly on ngrok failure. BROWSE_TUNNEL=1 startup uses the same pattern. * closeTunnel() helper — single teardown path for both the ngrok listener and the tunnel Bun.serve listener. * resolveNgrokAuthtoken() helper — shared authtoken lookup across /tunnel/start and BROWSE_TUNNEL=1 startup (was duplicated). * TUNNEL_COMMANDS check in /command dispatch: on the tunnel surface, commands outside the allowlist return 403 with a list of allowed commands as a hint. * Probe paths in /pair and /tunnel/start migrated from /health to GET /connect — the only unauth path reachable on the tunnel surface under the new architecture. Test updates in browse/test/server-auth.test.ts: * /pair liveness-verify test: assert via closeTunnel() helper instead of the inline `tunnelActive = false; tunnelUrl = null` lines that the helper subsumes. * /tunnel/start cached-tunnel test: same closeTunnel() adaptation. Credit Derived from PR #1026 by @garagon — thanks for flagging the critical bug that drove the architectural rewrite. The per-request isTunneledRequest approach from #1026 is superseded by physical port separation here; the underlying report remains the root cause for the entire v1.6.0.0 wave. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(security): add source-level guards for dual-listener architecture 23 source-level assertions that keep future contributors from silently widening the tunnel surface during a routine refactor. Covers: * Surface type + tunnelServer state variable shape * TUNNEL_PATHS is a closed set of /connect, /command, /sidebar-chat (and NOT /health, /welcome, /cookie-picker, /inspector/*, /pair, /token, /refs, /activity/stream, /tunnel/{start,stop}) * TUNNEL_COMMANDS includes browser-driving ops only (and NOT launch-browser, tunnel-start, token-mint, cookie-import, etc.) * makeFetchHandler(surface) factory exists and is wired to both listeners with the correct surface parameter * Tunnel filter runs BEFORE any route dispatch, with 404/403/401 responses and logged denials for each reason * GET /connect returns {alive: true} unauth * /command dispatch enforces TUNNEL_COMMANDS on tunnel surface * closeTunnel() helper tears down ngrok + Bun.serve listener * /tunnel/start binds on ephemeral port, points ngrok at TUNNEL_PORT (not local port), hard-fails on bind error (no fallback), probes cached tunnel via GET /connect (not /health), tears down on ngrok.forward failure * BROWSE_TUNNEL=1 startup uses the dual-listener pattern * logTunnelDenial wired for all three denial reasons * /connect rate limit is 300/min, not 3/min All 23 tests pass. Behavioral integration tests (spawn subprocess, real network) live in the E2E suite that lands later in this wave. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security: gate download + scrape through validateNavigationUrl (SSRF) The `goto` command was correctly wired through validateNavigationUrl, but `download` and `scrape` called page.request.fetch(url, ...) directly. A caller with the default write scope could hit the /command endpoint and ask the daemon to fetch http://169.254.169.254/latest/meta-data/ (AWS IMDSv1) or the GCP/Azure/internal equivalents. The response body comes back as base64 or lands on disk where GET /file serves it. Fix: call validateNavigationUrl(url) immediately before each page.request.fetch() call site in download and in the scrape loop. Same blocklist that already protects `goto`: file://, javascript:, data:, chrome://, cloud metadata (IPv4 all encodings, IPv6 ULA, metadata.*.internal). Tests: extend browse/test/url-validation.test.ts with a source-level guard that walks every `await page.request.fetch(` call site and asserts a validateNavigationUrl call precedes it within the same branch. Regression trips before code review if a future refactor drops the gate. * security: route splitForScoped through envelope sentinel escape The scoped-token snapshot path in snapshot.ts built its untrusted block by pushing the raw accessibility-tree lines between the literal `═══ BEGIN UNTRUSTED WEB CONTENT ═══` / `═══ END UNTRUSTED WEB CONTENT ═══` sentinels. The full-page wrap path in content-security.ts already applied a zero-width-space escape on those exact strings to prevent sentinel injection, but the scoped path skipped it. Net effect: a page whose rendered text contains the literal sentinel can close the envelope early from inside untrusted content and forge a fake "trusted" block for the LLM. That includes fabricating interactive `@eN` references the agent will act on. Fix: * Extract the zero-width-space escape into a named, exported helper `escapeEnvelopeSentinels(content)` in content-security.ts. * Have `wrapUntrustedPageContent` call it (behavior unchanged on that path — same bytes out). * Import the helper in snapshot.ts and map it over `untrustedLines` in the `splitForScoped` branch before pushing the BEGIN sentinel. Tests: add a describe block in content-security.test.ts that covers * `escapeEnvelopeSentinels` defuses BEGIN and END markers; * `escapeEnvelopeSentinels` leaves normal text untouched; * `wrapUntrustedPageContent` still emits exactly one real envelope pair when hostile content contains forged sentinels; * snapshot.ts imports the helper; * the scoped-snapshot branch calls `escapeEnvelopeSentinels` before pushing the BEGIN sentinel (source-level regression — if a future refactor reorders this, the test trips). * security: extend hidden-element detection to all DOM-reading channels The Confusion Protocol envelope wrap (`wrapUntrustedPageContent`) covers every scoped PAGE_CONTENT_COMMAND, but the hidden-element ARIA-injection detection layer only ran for `text`. Other DOM-reading channels (html, links, forms, accessibility, attrs, data, media, ux-audit) returned their output through the envelope with no hidden- content filter, so a page serving a display:none div that instructs the agent to disregard prior system messages, or an aria-label that claims to put the LLM in admin mode, leaked the injection payload on any non-text channel. The envelope alone does not mitigate this, and the page itself never rendered the hostile content to the human operator. Fix: * New export `DOM_CONTENT_COMMANDS` in commands.ts — the subset of PAGE_CONTENT_COMMANDS that derives its output from the live DOM. Console and dialog stay out; they read separate runtime state. * server.ts runs `markHiddenElements` + `cleanupHiddenMarkers` for every scoped command in this set. `text` keeps its existing `getCleanTextWithStripping` path (hidden elements physically stripped before the read). All other channels keep their output format but emit flagged elements as CONTENT WARNINGS on the envelope, so the LLM sees what it would otherwise have consumed silently. * Hidden-element descriptions merge into `combinedWarnings` alongside content-filter warnings before the wrap call. Tests: new describe block in content-security.test.ts covering * `DOM_CONTENT_COMMANDS` export shape and channel membership; * dispatch gates on `DOM_CONTENT_COMMANDS.has(command)`, not the literal `text` string; * hiddenContentWarnings plumbs into `combinedWarnings` and reaches wrapUntrustedPageContent; * DOM_CONTENT_COMMANDS is a strict subset of PAGE_CONTENT_COMMANDS. Existing datamarking, envelope wrap, centralized-wrapping, and chain security suites stay green (52 pass, 0 fail). * security: validate --from-file payload paths for parity with direct paths The direct `load-html <file>` path runs every caller-supplied file path through validateReadPath() so reads stay confined to SAFE_DIRECTORIES (cwd, TEMP_DIR). The `load-html --from-file <payload.json>` shortcut and its sibling `pdf --from-file <payload.json>` skipped that check and went straight to fs.readFileSync(). An MCP caller that picks the payload path (or any caller whose payload argument is reachable from attacker-influenced text) could use --from-file as a read-anywhere escape hatch for the safe-dirs policy. Fix: call validateReadPath(path.resolve(payloadPath)) before readFileSync at both sites. Error surface mirrors the direct-path branch so ops and agent errors stay consistent. Test coverage in browse/test/from-file-path-validation.test.ts: - source-level: validateReadPath precedes readFileSync in the load-html --from-file branch (write-commands.ts) and the pdf --from-file parser (meta-commands.ts) - error-message parity: both sites reference SAFE_DIRECTORIES Related security audit pattern: R3 F002 (validateNavigationUrl gap on download/scrape) and R3 F008 (markHiddenElements gap on 10 DOM commands) were the same shape — a defense that existed on the primary code path but not its shortcut sibling. This PR closes the same class of gap on the --from-file shortcuts. * fix(design): escape url.origin when injecting into served HTML serve.ts injected url.origin into a single-quoted JS string in the response body. A local request with a crafted Host header (e.g. Host: "evil'-alert(1)-'x") would break out of the string and execute JS in the 127.0.0.1:<port> origin opened by the design board. Low severity — bound to localhost, requires a local attacker — but no reason not to escape. Fix: JSON.stringify(url.origin) produces a properly quoted, escaped JS string literal in one call. Also includes Prettier reformatting (single→double quotes, trailing commas, line wrapping) applied by the repo's PostToolUse formatter hook. Security change is the one line in the HTML injection; everything else is whitespace/style. * fix(scripts): drop shell:true from slop-diff npx invocations spawnSync('npx', [...], { shell: true }) invokes /bin/sh -c with the args concatenated, subjecting them to shell parsing (word splitting, glob expansion, metacharacter interpretation). No user input reaches these calls today, so not exploitable — but the posture is wrong: npx + shell args should be direct. Fix: scope shell:true to process.platform === 'win32' where npx is actually a .cmd requiring the shell. POSIX runs the npx binary directly with array-form args. Also includes Prettier reformatting (single→double quotes, trailing commas, line wrapping) applied by the repo's PostToolUse formatter hook. Security-relevant change is just the two shell:true -> shell: process.platform === 'win32' lines; everything else is whitespace/style. * security(E3): gate GSTACK_SLUG on /welcome path traversal The /welcome handler interpolates GSTACK_SLUG directly into the filesystem path used to locate the project-local welcome page. Without validation, a slug like "../../etc/passwd" would resolve to ~/.gstack/projects/../../etc/passwd/designs/welcome-page-20260331/finalized.html — classic path traversal. Not exploitable today: GSTACK_SLUG is set by the gstack CLI at daemon launch, and an attacker would already need local env-var access to poison it. But the gate is one regex (^[a-z0-9_-]+$), and a defense-in-depth pass costs us nothing when the cost of being wrong is arbitrary file read via /welcome. Fall back to the safe 'unknown' literal when the slug fails validation — same fallback the code already uses when GSTACK_SLUG is unset. No behavior change for legitimate slugs (they all match the regex). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security(N1): replace ?token= SSE auth with HttpOnly session cookie Activity stream and inspector events SSE endpoints accepted the root AUTH_TOKEN via `?token=` query param (EventSource can't send Authorization headers). URLs leak to browser history, referer headers, server logs, crash reports, and refactoring accidents. Codex flagged this during the /plan-ceo-review outside voice pass. New auth model: the extension calls POST /sse-session with a Bearer token and receives a view-only session cookie (HttpOnly, SameSite=Strict, 30-min TTL). EventSource is opened with `withCredentials: true` so the browser sends the cookie back on the SSE connection. The ?token= query param is GONE — no more URL-borne secrets. Scope isolation (prior learning cookie-picker-auth-isolation, 10/10 confidence): the SSE session cookie grants access to /activity/stream and /inspector/events ONLY. The token is never valid against /command, /token, or any mutating endpoint. A leaked cookie can watch activity; it cannot execute browser commands. Components * browse/src/sse-session-cookie.ts — registry: mint/validate/extract/ build-cookie. 256-bit tokens, 30-min TTL, lazy expiry pruning, no imports from token-registry (scope isolation enforced by module boundary). * browse/src/server.ts — POST /sse-session mint endpoint (requires Bearer). /activity/stream and /inspector/events now accept Bearer OR the session cookie, and reject ?token= query param. * extension/sidepanel.js — ensureSseSessionCookie() bootstrap call, EventSource opened with withCredentials:true on both SSE endpoints. Tested via the source guards; behavioral test is the E2E pairing flow that lands later in the wave. * browse/test/sse-session-cookie.test.ts — 20 unit tests covering mint entropy, TTL enforcement, cookie flag invariants, cookie parsing from multi-cookie headers, and scope-isolation contract guard (module must not import token-registry). * browse/test/server-auth.test.ts — existing /activity/stream auth test updated to assert the new cookie-based gate and the absence of the ?token= query param. Cookie flag choices: * HttpOnly: token not readable from page JS (mitigates XSS exfiltration). * SameSite=Strict: cookie not sent on cross-site requests (mitigates CSRF). Fine for SSE because the extension connects to 127.0.0.1 directly. * Path=/: cookie scoped to the whole origin. * Max-Age=1800: 30 minutes, matches TTL. Extension re-mints on reconnect when daemon restarts. * Secure NOT set: daemon binds to 127.0.0.1 over plain HTTP. Adding Secure would block the browser from ever sending the cookie back. Add Secure when gstack ships over HTTPS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security(N2): document Windows v20 ABE elevation path on CDP port The existing comment around the cookie-import-browser --remote-debugging-port launch claimed "threat model: no worse than baseline." That's wrong on Windows with App-Bound Encryption v20. A same-user local process that opens the cookie SQLite DB directly CANNOT decrypt v20 values (DPAPI context is bound to the browser process). The CDP port lets them bypass that: connect to the debug port, call Network.getAllCookies inside Chrome, walk away with decrypted v20 cookies. The correct fix is to switch from TCP --remote-debugging-port to --remote-debugging-pipe so the CDP transport is a stdio pipe, not a socket. That requires restructuring the CDP WebSocket client in this module and Playwright doesn't expose the pipe transport out of the box. Non-trivial, deferred from the v1.6.0.0 wave. This commit updates the comment to correctly describe the threat and points at the tracking issue. No code change to the launch itself. Follow-up: #1136. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(E2): document dual-listener tunnel architecture in ARCHITECTURE.md Adds an explicit per-endpoint disposition table to the Security model section, covering the v1.6.0.0 dual-listener refactor. Every HTTP endpoint now has a documented local-vs-tunnel answer. Future audits (and future contributors wondering "is it safe to add X to the tunnel surface?") can read this instead of reverse-engineering server.ts. Also documents: * Why physical port separation beats per-request header inference (ngrok behavior drift, local proxies can forge headers, etc.) * Tunnel surface denial logging → ~/.gstack/security/attempts.jsonl * SSE session cookie model (gstack_sse, 30-min TTL, stream-scope only, module-boundary-enforced scope isolation) * N2 non-goal for Windows v20 ABE via CDP port (tracking #1136) No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(E1): end-to-end pair-agent flow against a spawned daemon Spawns the browse daemon as a subprocess with BROWSE_HEADLESS_SKIP=1 so the HTTP layer runs without a real browser. Exercises: * GET /health — token delivery for chrome-extension origin, withheld otherwise (the F1 + PR #1026 invariant) * GET /connect — alive probe returns {alive:true} unauth * POST /pair — root Bearer required (403 without), returns setup_key * POST /connect — setup_key exchange mints a distinct scoped token * POST /command — 401 without auth * POST /sse-session — Bearer required, Set-Cookie has HttpOnly + SameSite=Strict (the N1 invariant) * GET /activity/stream — 401 without auth * GET /activity/stream?token= — 401 (the old ?token= query param is REJECTED, which is the whole point of N1) * GET /welcome — serves HTML, does not leak /etc/passwd content under the default 'unknown' slug (E3 regex gate) 12 behavioral tests, ~220ms end-to-end, no network dependencies, no ngrok, no real browser. This is the receipt for the wave's central 'pair-agent still works + the security boundary holds' claim. Tunnel-port binding (/tunnel/start) is deliberately NOT exercised here — it requires an ngrok authtoken and live network. The dual-listener route allowlist is covered by source-level guards in dual-listener.test.ts; behavioral tunnel testing belongs in a separate paid-evals harness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * release(v1.6.0.0): bump VERSION + CHANGELOG for security wave Architectural bump, not patch: dual-listener HTTP refactor changes the daemon's tunnel-exposure model. See CHANGELOG for the full release summary (~950 words) covering the five root causes this wave closes: 1. /health token leak over ngrok (F1 + E3 + test infra) 2. /cookie-picker + /inspector exposed over the tunnel (F1) 3. ?token=<ROOT> in SSE URLs leaking to logs/referer/history (N1) 4. /welcome GSTACK_SLUG path traversal (E3) 5. Windows v20 ABE elevation via CDP port (N2 — documented non-goal, tracked as #1136) Plus the base PRs: SSRF gate (#1029), envelope sentinel escape (#1031), DOM-channel hidden-element coverage (#1032), --from-file path validation (#1103), and 2 commits from #1073 (@theqazi). VERSION + package.json bumped to 1.6.0.0. CHANGELOG entry covers credits (@garagon, @Hybirdss, @HMAKT99, @theqazi), review lineage (CEO → Codex outside voice → Eng), and the non-goal tracking issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: pre-landing review findings (4 auto-fixes) Addresses 4 findings from the Claude adversarial subagent on the v1.6.0.0 security wave diff. No user-visible behavior change; all are defense-in-depth hardening of newly-introduced code. 1. GET /connect rate-limited (was POST-only) [HIGH conf 8/10] Attacker discovering the ngrok URL could probe unlimited GETs for daemon enumeration. Now shares the global /connect counter. 2. ngrok listener leak on tunnel startup failure [MEDIUM conf 8/10] If ngrok.forward() resolved but tunnelListener.url() or the state-file write threw, the Bun listener was torn down but the ngrok session was leaked. Fixed in BOTH /tunnel/start and BROWSE_TUNNEL=1 startup paths. 3. GSTACK_SKILL_ROOT path-traversal gate [MEDIUM conf 8/10] Symmetric with E3's GSTACK_SLUG regex gate — reject values containing '..' before interpolating into the welcome-page path. 4. SSE session registry pruning [LOW conf 7/10] pruneExpired() only checked 10 entries per mint call. Now runs on every validate too, checks 20 entries, with a hard 10k cap as backstop. Prevents registry growth under sustained extension reconnect pressure. Tests remain green (56/56 in sse-session-cookie + dual-listener + pair-agent-e2e suites). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: update project documentation for v1.6.0.0 Reflect the dual-listener tunnel architecture, SSE session cookies, SSRF guards, and Windows v20 ABE non-goal across the three docs users actually read for remote-agent and browser auth context: - docs/REMOTE_BROWSER_ACCESS.md: rewrote Architecture diagram for dual listeners, fixed /connect rate limit (3/min → 300/min), removed stale "/health requires no auth" (now 404 on tunnel), added SSE cookie auth, expanded Security Model with tunnel allowlist, SSRF guards, /welcome path traversal defense, and the Windows v20 ABE tracking note. - BROWSER.md: added dual-listener paragraph to Authentication and linked to ARCHITECTURE.md endpoint table. Replaced the stale ?token= SSE auth note with the HttpOnly gstack_sse cookie flow. - CLAUDE.md: added Transport-layer security section above the sidebar prompt-injection stack so contributors editing server.ts, sse-session-cookie.ts, or tunnel-denial-log.ts see the load-bearing module boundaries before touching them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(make-pdf): write --from-file payload to /tmp, not os.tmpdir() make-pdf's browseClient wrote its --from-file payload to os.tmpdir(), which is /var/folders/... on macOS. v1.6.0.0's PR #1103 cherry-pick tightened browse load-html --from-file to validate against the safe-dirs allowlist ([TEMP_DIR, cwd] where TEMP_DIR is '/tmp' on macOS/Linux, os.tmpdir() on Windows). This closed a CLI/API parity gap but broke make-pdf on macOS because /var/folders/... is outside the allowlist. Fix: mirror browse's TEMP_DIR convention — use '/tmp' on non-Windows, os.tmpdir() on Windows. The make-pdf-gate CI failure on macOS-latest (run 72440797490) is caused by exactly this: the payload file was rejected by validateReadPath. Verified locally: the combined-gate e2e test now passes after rebuilding make-pdf/dist/pdf. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sidebar): killAgent resets per-tab state; align tests with current agent event format Two pre-existing bugs surfaced while running the full e2e suite on the sec-wave branch. Both pre-date v1.6.0.0 (same failures on main at e23ff28) but blocked the ship verification, so fixing now. ### Bug 1: killAgent leaked stale per-tab state `killAgent()` reset the legacy globals (agentProcess, agentStatus, etc.) but never touched the per-tab `tabAgents` Map. Meanwhile `/sidebar-command` routes on `tabState.status` from that Map, not the legacy globals. Consequence: after a kill (including the implicit kill in `/sidebar-session/new`), the next /sidebar-command on the same tab saw `tabState.status === 'processing'` and fell into the queue branch, silently NOT spawning an agent. Integration tests that called resetState between cases all failed with empty queues. Fix: when targetTabId is supplied, reset that one tab's state; when called without a tab (session-new, full kill), reset ALL tab states. Matches the semantic boundary already used for the cancel-file write. ### Bug 2: sidebar-integration tests drifted from current event format `agent events appear in /sidebar-chat` posted the raw Claude streaming format (`{type: 'assistant', message: {content: [...]}}`) but `processAgentEvent` in server.ts only handles the simplified types that sidebar-agent.ts pre-processes into (text, text_delta, tool_use, result, agent_error, security_event). The architecture moved pre-processing into sidebar-agent.ts at some point and this test never got updated. Fixed by sending the pre-processed `{type: 'text', text: '...'}` format — which is actually what the server sees in production. Also removed the `entry.prompt` URL-containment check in the queue-write test. The URL is carried on entry.pageUrl (metadata) by design: the system prompt tells Claude to run `browse url` to fetch the actual page rather than trust any URL in the prompt body. That's the URL-based prompt-injection defense. The prompt SHOULD NOT contain the URL, so the test assertion was wrong for the current security posture. ### Verification - `bun test browse/test/sidebar-integration.test.ts` → 13/13 pass (was 6/13 on both main and branch before this commit) - Full `bun run test` → exit 0, zero fail markers - No behavior change for production sidebar flows: killAgent was already supposed to return the agent to idle; it just wasn't fully doing so. Per-tab reset now matches the documented semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: gus <gustavoraularagon@gmail.com> Co-authored-by: Mohammed Qazi <10266060+theqazi@users.noreply.github.com>
Contributor
Author
|
Closing — fixed in |
gonnabe88
pushed a commit
to gonnabe88/gstack
that referenced
this pull request
May 9, 2026
…0) (garrytan#1137) * refactor(security): loosen /connect rate limit from 3/min to 300/min Setup keys are 24 random bytes (unbruteforceable), so a tight rate limit does not meaningfully prevent key guessing. It exists only to cap bandwidth, CPU, and log-flood damage from someone who discovered the ngrok URL. A legitimate pair-agent session hits /connect once; 300/min is 60x that pattern and never hit accidentally. 3/min caused pairing to fail on any retry flow (network blip, second paired client) with no upside. Per-IP tracking was considered and rejected — adds a bounded Map + LRU for defense already adequate at the global layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(security): add tunnel-denial-log module for attack visibility Append-only log of tunnel-surface auth denials to ~/.gstack/security/attempts.jsonl. Gives operators visibility into who is probing tunneled daemons so the next security wave can be driven by real attack data instead of speculation. Design notes: - Async via fs.promises.appendFile. Never appendFileSync — blocking the event loop on every denial during a flood is what an attacker wants (prior learning: sync-audit-log-io, 10/10 confidence). - In-process rate cap at 60 writes/minute globally. Excess denials are counted in memory but not written to disk — prevents disk DoS. - Writes to the same ~/.gstack/security/attempts.jsonl used by the prompt-injection attempt log. File rotation is handled by the existing security pipeline (10MB, 5 generations). No consumers in this commit; wired up in the dual-listener refactor that follows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(security): dual-listener tunnel architecture The /health endpoint leaked AUTH_TOKEN to any caller that hit the ngrok URL (spoofing chrome-extension:// origin, or catching headed mode). Surfaced by @garagon in PR garrytan#1026; the original fix was header-inference on the single port. Codex's outside-voice review during /plan-ceo-review called that approach brittle (ngrok header behavior could change, local proxies would false-positive), and pushed for the structural fix. This is that fix. Stop making /health a root-token bootstrap endpoint on any surface the tunnel can reach. The server now binds two HTTP listeners when a tunnel is active. The local listener (extension, CLI, sidebar) stays on 127.0.0.1 and is never exposed to ngrok. ngrok forwards only to the tunnel listener, which serves only /connect (unauth, rate-limited) and /command with a locked allowlist of browser-driving commands. Security property comes from physical port separation, not from header inference — a tunnel caller cannot reach /health or /cookie-picker or /inspector because they live on a different TCP socket. What this commit adds to browse/src/server.ts: * Surface type ('local' | 'tunnel') and TUNNEL_PATHS + TUNNEL_COMMANDS allowlists near the top of the file. * makeFetchHandler(surface) factory replacing the single fetch arrow; closure-captures the surface so the filter that runs before route dispatch knows which socket accepted the request. * Tunnel filter at dispatch entry: 404s anything not on TUNNEL_PATHS, 403s root-token bearers with a clear pairing hint, 401s non-/connect requests that lack a scoped token. Every denial is logged via logTunnelDenial (from tunnel-denial-log). * GET /connect alive probe (unauth on both surfaces) so /pair and /tunnel/start can detect dead ngrok tunnels without reaching /health — /health is no longer tunnel-reachable. * Lazy tunnel listener lifecycle. /tunnel/start binds a dedicated Bun.serve on an ephemeral port, points ngrok.forward at THAT port (not the local port), hard-fails on bind error (no local fallback), tears down cleanly on ngrok failure. BROWSE_TUNNEL=1 startup uses the same pattern. * closeTunnel() helper — single teardown path for both the ngrok listener and the tunnel Bun.serve listener. * resolveNgrokAuthtoken() helper — shared authtoken lookup across /tunnel/start and BROWSE_TUNNEL=1 startup (was duplicated). * TUNNEL_COMMANDS check in /command dispatch: on the tunnel surface, commands outside the allowlist return 403 with a list of allowed commands as a hint. * Probe paths in /pair and /tunnel/start migrated from /health to GET /connect — the only unauth path reachable on the tunnel surface under the new architecture. Test updates in browse/test/server-auth.test.ts: * /pair liveness-verify test: assert via closeTunnel() helper instead of the inline `tunnelActive = false; tunnelUrl = null` lines that the helper subsumes. * /tunnel/start cached-tunnel test: same closeTunnel() adaptation. Credit Derived from PR garrytan#1026 by @garagon — thanks for flagging the critical bug that drove the architectural rewrite. The per-request isTunneledRequest approach from garrytan#1026 is superseded by physical port separation here; the underlying report remains the root cause for the entire v1.6.0.0 wave. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(security): add source-level guards for dual-listener architecture 23 source-level assertions that keep future contributors from silently widening the tunnel surface during a routine refactor. Covers: * Surface type + tunnelServer state variable shape * TUNNEL_PATHS is a closed set of /connect, /command, /sidebar-chat (and NOT /health, /welcome, /cookie-picker, /inspector/*, /pair, /token, /refs, /activity/stream, /tunnel/{start,stop}) * TUNNEL_COMMANDS includes browser-driving ops only (and NOT launch-browser, tunnel-start, token-mint, cookie-import, etc.) * makeFetchHandler(surface) factory exists and is wired to both listeners with the correct surface parameter * Tunnel filter runs BEFORE any route dispatch, with 404/403/401 responses and logged denials for each reason * GET /connect returns {alive: true} unauth * /command dispatch enforces TUNNEL_COMMANDS on tunnel surface * closeTunnel() helper tears down ngrok + Bun.serve listener * /tunnel/start binds on ephemeral port, points ngrok at TUNNEL_PORT (not local port), hard-fails on bind error (no fallback), probes cached tunnel via GET /connect (not /health), tears down on ngrok.forward failure * BROWSE_TUNNEL=1 startup uses the dual-listener pattern * logTunnelDenial wired for all three denial reasons * /connect rate limit is 300/min, not 3/min All 23 tests pass. Behavioral integration tests (spawn subprocess, real network) live in the E2E suite that lands later in this wave. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security: gate download + scrape through validateNavigationUrl (SSRF) The `goto` command was correctly wired through validateNavigationUrl, but `download` and `scrape` called page.request.fetch(url, ...) directly. A caller with the default write scope could hit the /command endpoint and ask the daemon to fetch http://169.254.169.254/latest/meta-data/ (AWS IMDSv1) or the GCP/Azure/internal equivalents. The response body comes back as base64 or lands on disk where GET /file serves it. Fix: call validateNavigationUrl(url) immediately before each page.request.fetch() call site in download and in the scrape loop. Same blocklist that already protects `goto`: file://, javascript:, data:, chrome://, cloud metadata (IPv4 all encodings, IPv6 ULA, metadata.*.internal). Tests: extend browse/test/url-validation.test.ts with a source-level guard that walks every `await page.request.fetch(` call site and asserts a validateNavigationUrl call precedes it within the same branch. Regression trips before code review if a future refactor drops the gate. * security: route splitForScoped through envelope sentinel escape The scoped-token snapshot path in snapshot.ts built its untrusted block by pushing the raw accessibility-tree lines between the literal `═══ BEGIN UNTRUSTED WEB CONTENT ═══` / `═══ END UNTRUSTED WEB CONTENT ═══` sentinels. The full-page wrap path in content-security.ts already applied a zero-width-space escape on those exact strings to prevent sentinel injection, but the scoped path skipped it. Net effect: a page whose rendered text contains the literal sentinel can close the envelope early from inside untrusted content and forge a fake "trusted" block for the LLM. That includes fabricating interactive `@eN` references the agent will act on. Fix: * Extract the zero-width-space escape into a named, exported helper `escapeEnvelopeSentinels(content)` in content-security.ts. * Have `wrapUntrustedPageContent` call it (behavior unchanged on that path — same bytes out). * Import the helper in snapshot.ts and map it over `untrustedLines` in the `splitForScoped` branch before pushing the BEGIN sentinel. Tests: add a describe block in content-security.test.ts that covers * `escapeEnvelopeSentinels` defuses BEGIN and END markers; * `escapeEnvelopeSentinels` leaves normal text untouched; * `wrapUntrustedPageContent` still emits exactly one real envelope pair when hostile content contains forged sentinels; * snapshot.ts imports the helper; * the scoped-snapshot branch calls `escapeEnvelopeSentinels` before pushing the BEGIN sentinel (source-level regression — if a future refactor reorders this, the test trips). * security: extend hidden-element detection to all DOM-reading channels The Confusion Protocol envelope wrap (`wrapUntrustedPageContent`) covers every scoped PAGE_CONTENT_COMMAND, but the hidden-element ARIA-injection detection layer only ran for `text`. Other DOM-reading channels (html, links, forms, accessibility, attrs, data, media, ux-audit) returned their output through the envelope with no hidden- content filter, so a page serving a display:none div that instructs the agent to disregard prior system messages, or an aria-label that claims to put the LLM in admin mode, leaked the injection payload on any non-text channel. The envelope alone does not mitigate this, and the page itself never rendered the hostile content to the human operator. Fix: * New export `DOM_CONTENT_COMMANDS` in commands.ts — the subset of PAGE_CONTENT_COMMANDS that derives its output from the live DOM. Console and dialog stay out; they read separate runtime state. * server.ts runs `markHiddenElements` + `cleanupHiddenMarkers` for every scoped command in this set. `text` keeps its existing `getCleanTextWithStripping` path (hidden elements physically stripped before the read). All other channels keep their output format but emit flagged elements as CONTENT WARNINGS on the envelope, so the LLM sees what it would otherwise have consumed silently. * Hidden-element descriptions merge into `combinedWarnings` alongside content-filter warnings before the wrap call. Tests: new describe block in content-security.test.ts covering * `DOM_CONTENT_COMMANDS` export shape and channel membership; * dispatch gates on `DOM_CONTENT_COMMANDS.has(command)`, not the literal `text` string; * hiddenContentWarnings plumbs into `combinedWarnings` and reaches wrapUntrustedPageContent; * DOM_CONTENT_COMMANDS is a strict subset of PAGE_CONTENT_COMMANDS. Existing datamarking, envelope wrap, centralized-wrapping, and chain security suites stay green (52 pass, 0 fail). * security: validate --from-file payload paths for parity with direct paths The direct `load-html <file>` path runs every caller-supplied file path through validateReadPath() so reads stay confined to SAFE_DIRECTORIES (cwd, TEMP_DIR). The `load-html --from-file <payload.json>` shortcut and its sibling `pdf --from-file <payload.json>` skipped that check and went straight to fs.readFileSync(). An MCP caller that picks the payload path (or any caller whose payload argument is reachable from attacker-influenced text) could use --from-file as a read-anywhere escape hatch for the safe-dirs policy. Fix: call validateReadPath(path.resolve(payloadPath)) before readFileSync at both sites. Error surface mirrors the direct-path branch so ops and agent errors stay consistent. Test coverage in browse/test/from-file-path-validation.test.ts: - source-level: validateReadPath precedes readFileSync in the load-html --from-file branch (write-commands.ts) and the pdf --from-file parser (meta-commands.ts) - error-message parity: both sites reference SAFE_DIRECTORIES Related security audit pattern: R3 F002 (validateNavigationUrl gap on download/scrape) and R3 F008 (markHiddenElements gap on 10 DOM commands) were the same shape — a defense that existed on the primary code path but not its shortcut sibling. This PR closes the same class of gap on the --from-file shortcuts. * fix(design): escape url.origin when injecting into served HTML serve.ts injected url.origin into a single-quoted JS string in the response body. A local request with a crafted Host header (e.g. Host: "evil'-alert(1)-'x") would break out of the string and execute JS in the 127.0.0.1:<port> origin opened by the design board. Low severity — bound to localhost, requires a local attacker — but no reason not to escape. Fix: JSON.stringify(url.origin) produces a properly quoted, escaped JS string literal in one call. Also includes Prettier reformatting (single→double quotes, trailing commas, line wrapping) applied by the repo's PostToolUse formatter hook. Security change is the one line in the HTML injection; everything else is whitespace/style. * fix(scripts): drop shell:true from slop-diff npx invocations spawnSync('npx', [...], { shell: true }) invokes /bin/sh -c with the args concatenated, subjecting them to shell parsing (word splitting, glob expansion, metacharacter interpretation). No user input reaches these calls today, so not exploitable — but the posture is wrong: npx + shell args should be direct. Fix: scope shell:true to process.platform === 'win32' where npx is actually a .cmd requiring the shell. POSIX runs the npx binary directly with array-form args. Also includes Prettier reformatting (single→double quotes, trailing commas, line wrapping) applied by the repo's PostToolUse formatter hook. Security-relevant change is just the two shell:true -> shell: process.platform === 'win32' lines; everything else is whitespace/style. * security(E3): gate GSTACK_SLUG on /welcome path traversal The /welcome handler interpolates GSTACK_SLUG directly into the filesystem path used to locate the project-local welcome page. Without validation, a slug like "../../etc/passwd" would resolve to ~/.gstack/projects/../../etc/passwd/designs/welcome-page-20260331/finalized.html — classic path traversal. Not exploitable today: GSTACK_SLUG is set by the gstack CLI at daemon launch, and an attacker would already need local env-var access to poison it. But the gate is one regex (^[a-z0-9_-]+$), and a defense-in-depth pass costs us nothing when the cost of being wrong is arbitrary file read via /welcome. Fall back to the safe 'unknown' literal when the slug fails validation — same fallback the code already uses when GSTACK_SLUG is unset. No behavior change for legitimate slugs (they all match the regex). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security(N1): replace ?token= SSE auth with HttpOnly session cookie Activity stream and inspector events SSE endpoints accepted the root AUTH_TOKEN via `?token=` query param (EventSource can't send Authorization headers). URLs leak to browser history, referer headers, server logs, crash reports, and refactoring accidents. Codex flagged this during the /plan-ceo-review outside voice pass. New auth model: the extension calls POST /sse-session with a Bearer token and receives a view-only session cookie (HttpOnly, SameSite=Strict, 30-min TTL). EventSource is opened with `withCredentials: true` so the browser sends the cookie back on the SSE connection. The ?token= query param is GONE — no more URL-borne secrets. Scope isolation (prior learning cookie-picker-auth-isolation, 10/10 confidence): the SSE session cookie grants access to /activity/stream and /inspector/events ONLY. The token is never valid against /command, /token, or any mutating endpoint. A leaked cookie can watch activity; it cannot execute browser commands. Components * browse/src/sse-session-cookie.ts — registry: mint/validate/extract/ build-cookie. 256-bit tokens, 30-min TTL, lazy expiry pruning, no imports from token-registry (scope isolation enforced by module boundary). * browse/src/server.ts — POST /sse-session mint endpoint (requires Bearer). /activity/stream and /inspector/events now accept Bearer OR the session cookie, and reject ?token= query param. * extension/sidepanel.js — ensureSseSessionCookie() bootstrap call, EventSource opened with withCredentials:true on both SSE endpoints. Tested via the source guards; behavioral test is the E2E pairing flow that lands later in the wave. * browse/test/sse-session-cookie.test.ts — 20 unit tests covering mint entropy, TTL enforcement, cookie flag invariants, cookie parsing from multi-cookie headers, and scope-isolation contract guard (module must not import token-registry). * browse/test/server-auth.test.ts — existing /activity/stream auth test updated to assert the new cookie-based gate and the absence of the ?token= query param. Cookie flag choices: * HttpOnly: token not readable from page JS (mitigates XSS exfiltration). * SameSite=Strict: cookie not sent on cross-site requests (mitigates CSRF). Fine for SSE because the extension connects to 127.0.0.1 directly. * Path=/: cookie scoped to the whole origin. * Max-Age=1800: 30 minutes, matches TTL. Extension re-mints on reconnect when daemon restarts. * Secure NOT set: daemon binds to 127.0.0.1 over plain HTTP. Adding Secure would block the browser from ever sending the cookie back. Add Secure when gstack ships over HTTPS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security(N2): document Windows v20 ABE elevation path on CDP port The existing comment around the cookie-import-browser --remote-debugging-port launch claimed "threat model: no worse than baseline." That's wrong on Windows with App-Bound Encryption v20. A same-user local process that opens the cookie SQLite DB directly CANNOT decrypt v20 values (DPAPI context is bound to the browser process). The CDP port lets them bypass that: connect to the debug port, call Network.getAllCookies inside Chrome, walk away with decrypted v20 cookies. The correct fix is to switch from TCP --remote-debugging-port to --remote-debugging-pipe so the CDP transport is a stdio pipe, not a socket. That requires restructuring the CDP WebSocket client in this module and Playwright doesn't expose the pipe transport out of the box. Non-trivial, deferred from the v1.6.0.0 wave. This commit updates the comment to correctly describe the threat and points at the tracking issue. No code change to the launch itself. Follow-up: garrytan#1136. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(E2): document dual-listener tunnel architecture in ARCHITECTURE.md Adds an explicit per-endpoint disposition table to the Security model section, covering the v1.6.0.0 dual-listener refactor. Every HTTP endpoint now has a documented local-vs-tunnel answer. Future audits (and future contributors wondering "is it safe to add X to the tunnel surface?") can read this instead of reverse-engineering server.ts. Also documents: * Why physical port separation beats per-request header inference (ngrok behavior drift, local proxies can forge headers, etc.) * Tunnel surface denial logging → ~/.gstack/security/attempts.jsonl * SSE session cookie model (gstack_sse, 30-min TTL, stream-scope only, module-boundary-enforced scope isolation) * N2 non-goal for Windows v20 ABE via CDP port (tracking garrytan#1136) No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(E1): end-to-end pair-agent flow against a spawned daemon Spawns the browse daemon as a subprocess with BROWSE_HEADLESS_SKIP=1 so the HTTP layer runs without a real browser. Exercises: * GET /health — token delivery for chrome-extension origin, withheld otherwise (the F1 + PR garrytan#1026 invariant) * GET /connect — alive probe returns {alive:true} unauth * POST /pair — root Bearer required (403 without), returns setup_key * POST /connect — setup_key exchange mints a distinct scoped token * POST /command — 401 without auth * POST /sse-session — Bearer required, Set-Cookie has HttpOnly + SameSite=Strict (the N1 invariant) * GET /activity/stream — 401 without auth * GET /activity/stream?token= — 401 (the old ?token= query param is REJECTED, which is the whole point of N1) * GET /welcome — serves HTML, does not leak /etc/passwd content under the default 'unknown' slug (E3 regex gate) 12 behavioral tests, ~220ms end-to-end, no network dependencies, no ngrok, no real browser. This is the receipt for the wave's central 'pair-agent still works + the security boundary holds' claim. Tunnel-port binding (/tunnel/start) is deliberately NOT exercised here — it requires an ngrok authtoken and live network. The dual-listener route allowlist is covered by source-level guards in dual-listener.test.ts; behavioral tunnel testing belongs in a separate paid-evals harness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * release(v1.6.0.0): bump VERSION + CHANGELOG for security wave Architectural bump, not patch: dual-listener HTTP refactor changes the daemon's tunnel-exposure model. See CHANGELOG for the full release summary (~950 words) covering the five root causes this wave closes: 1. /health token leak over ngrok (F1 + E3 + test infra) 2. /cookie-picker + /inspector exposed over the tunnel (F1) 3. ?token=<ROOT> in SSE URLs leaking to logs/referer/history (N1) 4. /welcome GSTACK_SLUG path traversal (E3) 5. Windows v20 ABE elevation via CDP port (N2 — documented non-goal, tracked as garrytan#1136) Plus the base PRs: SSRF gate (garrytan#1029), envelope sentinel escape (garrytan#1031), DOM-channel hidden-element coverage (garrytan#1032), --from-file path validation (garrytan#1103), and 2 commits from garrytan#1073 (@theqazi). VERSION + package.json bumped to 1.6.0.0. CHANGELOG entry covers credits (@garagon, @Hybirdss, @HMAKT99, @theqazi), review lineage (CEO → Codex outside voice → Eng), and the non-goal tracking issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: pre-landing review findings (4 auto-fixes) Addresses 4 findings from the Claude adversarial subagent on the v1.6.0.0 security wave diff. No user-visible behavior change; all are defense-in-depth hardening of newly-introduced code. 1. GET /connect rate-limited (was POST-only) [HIGH conf 8/10] Attacker discovering the ngrok URL could probe unlimited GETs for daemon enumeration. Now shares the global /connect counter. 2. ngrok listener leak on tunnel startup failure [MEDIUM conf 8/10] If ngrok.forward() resolved but tunnelListener.url() or the state-file write threw, the Bun listener was torn down but the ngrok session was leaked. Fixed in BOTH /tunnel/start and BROWSE_TUNNEL=1 startup paths. 3. GSTACK_SKILL_ROOT path-traversal gate [MEDIUM conf 8/10] Symmetric with E3's GSTACK_SLUG regex gate — reject values containing '..' before interpolating into the welcome-page path. 4. SSE session registry pruning [LOW conf 7/10] pruneExpired() only checked 10 entries per mint call. Now runs on every validate too, checks 20 entries, with a hard 10k cap as backstop. Prevents registry growth under sustained extension reconnect pressure. Tests remain green (56/56 in sse-session-cookie + dual-listener + pair-agent-e2e suites). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: update project documentation for v1.6.0.0 Reflect the dual-listener tunnel architecture, SSE session cookies, SSRF guards, and Windows v20 ABE non-goal across the three docs users actually read for remote-agent and browser auth context: - docs/REMOTE_BROWSER_ACCESS.md: rewrote Architecture diagram for dual listeners, fixed /connect rate limit (3/min → 300/min), removed stale "/health requires no auth" (now 404 on tunnel), added SSE cookie auth, expanded Security Model with tunnel allowlist, SSRF guards, /welcome path traversal defense, and the Windows v20 ABE tracking note. - BROWSER.md: added dual-listener paragraph to Authentication and linked to ARCHITECTURE.md endpoint table. Replaced the stale ?token= SSE auth note with the HttpOnly gstack_sse cookie flow. - CLAUDE.md: added Transport-layer security section above the sidebar prompt-injection stack so contributors editing server.ts, sse-session-cookie.ts, or tunnel-denial-log.ts see the load-bearing module boundaries before touching them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(make-pdf): write --from-file payload to /tmp, not os.tmpdir() make-pdf's browseClient wrote its --from-file payload to os.tmpdir(), which is /var/folders/... on macOS. v1.6.0.0's PR garrytan#1103 cherry-pick tightened browse load-html --from-file to validate against the safe-dirs allowlist ([TEMP_DIR, cwd] where TEMP_DIR is '/tmp' on macOS/Linux, os.tmpdir() on Windows). This closed a CLI/API parity gap but broke make-pdf on macOS because /var/folders/... is outside the allowlist. Fix: mirror browse's TEMP_DIR convention — use '/tmp' on non-Windows, os.tmpdir() on Windows. The make-pdf-gate CI failure on macOS-latest (run 72440797490) is caused by exactly this: the payload file was rejected by validateReadPath. Verified locally: the combined-gate e2e test now passes after rebuilding make-pdf/dist/pdf. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sidebar): killAgent resets per-tab state; align tests with current agent event format Two pre-existing bugs surfaced while running the full e2e suite on the sec-wave branch. Both pre-date v1.6.0.0 (same failures on main at e23ff28) but blocked the ship verification, so fixing now. ### Bug 1: killAgent leaked stale per-tab state `killAgent()` reset the legacy globals (agentProcess, agentStatus, etc.) but never touched the per-tab `tabAgents` Map. Meanwhile `/sidebar-command` routes on `tabState.status` from that Map, not the legacy globals. Consequence: after a kill (including the implicit kill in `/sidebar-session/new`), the next /sidebar-command on the same tab saw `tabState.status === 'processing'` and fell into the queue branch, silently NOT spawning an agent. Integration tests that called resetState between cases all failed with empty queues. Fix: when targetTabId is supplied, reset that one tab's state; when called without a tab (session-new, full kill), reset ALL tab states. Matches the semantic boundary already used for the cancel-file write. ### Bug 2: sidebar-integration tests drifted from current event format `agent events appear in /sidebar-chat` posted the raw Claude streaming format (`{type: 'assistant', message: {content: [...]}}`) but `processAgentEvent` in server.ts only handles the simplified types that sidebar-agent.ts pre-processes into (text, text_delta, tool_use, result, agent_error, security_event). The architecture moved pre-processing into sidebar-agent.ts at some point and this test never got updated. Fixed by sending the pre-processed `{type: 'text', text: '...'}` format — which is actually what the server sees in production. Also removed the `entry.prompt` URL-containment check in the queue-write test. The URL is carried on entry.pageUrl (metadata) by design: the system prompt tells Claude to run `browse url` to fetch the actual page rather than trust any URL in the prompt body. That's the URL-based prompt-injection defense. The prompt SHOULD NOT contain the URL, so the test assertion was wrong for the current security posture. ### Verification - `bun test browse/test/sidebar-integration.test.ts` → 13/13 pass (was 6/13 on both main and branch before this commit) - Full `bun run test` → exit 0, zero fail markers - No behavior change for production sidebar flows: killAgent was already supposed to return the agent to idle; it just wasn't fully doing so. Per-tab reset now matches the documented semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: gus <gustavoraularagon@gmail.com> Co-authored-by: Mohammed Qazi <10266060+theqazi@users.noreply.github.com>
RyotaKun
pushed a commit
to RyotaKun/gstack
that referenced
this pull request
May 18, 2026
…0) (garrytan#1137) * refactor(security): loosen /connect rate limit from 3/min to 300/min Setup keys are 24 random bytes (unbruteforceable), so a tight rate limit does not meaningfully prevent key guessing. It exists only to cap bandwidth, CPU, and log-flood damage from someone who discovered the ngrok URL. A legitimate pair-agent session hits /connect once; 300/min is 60x that pattern and never hit accidentally. 3/min caused pairing to fail on any retry flow (network blip, second paired client) with no upside. Per-IP tracking was considered and rejected — adds a bounded Map + LRU for defense already adequate at the global layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(security): add tunnel-denial-log module for attack visibility Append-only log of tunnel-surface auth denials to ~/.gstack/security/attempts.jsonl. Gives operators visibility into who is probing tunneled daemons so the next security wave can be driven by real attack data instead of speculation. Design notes: - Async via fs.promises.appendFile. Never appendFileSync — blocking the event loop on every denial during a flood is what an attacker wants (prior learning: sync-audit-log-io, 10/10 confidence). - In-process rate cap at 60 writes/minute globally. Excess denials are counted in memory but not written to disk — prevents disk DoS. - Writes to the same ~/.gstack/security/attempts.jsonl used by the prompt-injection attempt log. File rotation is handled by the existing security pipeline (10MB, 5 generations). No consumers in this commit; wired up in the dual-listener refactor that follows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(security): dual-listener tunnel architecture The /health endpoint leaked AUTH_TOKEN to any caller that hit the ngrok URL (spoofing chrome-extension:// origin, or catching headed mode). Surfaced by @garagon in PR garrytan#1026; the original fix was header-inference on the single port. Codex's outside-voice review during /plan-ceo-review called that approach brittle (ngrok header behavior could change, local proxies would false-positive), and pushed for the structural fix. This is that fix. Stop making /health a root-token bootstrap endpoint on any surface the tunnel can reach. The server now binds two HTTP listeners when a tunnel is active. The local listener (extension, CLI, sidebar) stays on 127.0.0.1 and is never exposed to ngrok. ngrok forwards only to the tunnel listener, which serves only /connect (unauth, rate-limited) and /command with a locked allowlist of browser-driving commands. Security property comes from physical port separation, not from header inference — a tunnel caller cannot reach /health or /cookie-picker or /inspector because they live on a different TCP socket. What this commit adds to browse/src/server.ts: * Surface type ('local' | 'tunnel') and TUNNEL_PATHS + TUNNEL_COMMANDS allowlists near the top of the file. * makeFetchHandler(surface) factory replacing the single fetch arrow; closure-captures the surface so the filter that runs before route dispatch knows which socket accepted the request. * Tunnel filter at dispatch entry: 404s anything not on TUNNEL_PATHS, 403s root-token bearers with a clear pairing hint, 401s non-/connect requests that lack a scoped token. Every denial is logged via logTunnelDenial (from tunnel-denial-log). * GET /connect alive probe (unauth on both surfaces) so /pair and /tunnel/start can detect dead ngrok tunnels without reaching /health — /health is no longer tunnel-reachable. * Lazy tunnel listener lifecycle. /tunnel/start binds a dedicated Bun.serve on an ephemeral port, points ngrok.forward at THAT port (not the local port), hard-fails on bind error (no local fallback), tears down cleanly on ngrok failure. BROWSE_TUNNEL=1 startup uses the same pattern. * closeTunnel() helper — single teardown path for both the ngrok listener and the tunnel Bun.serve listener. * resolveNgrokAuthtoken() helper — shared authtoken lookup across /tunnel/start and BROWSE_TUNNEL=1 startup (was duplicated). * TUNNEL_COMMANDS check in /command dispatch: on the tunnel surface, commands outside the allowlist return 403 with a list of allowed commands as a hint. * Probe paths in /pair and /tunnel/start migrated from /health to GET /connect — the only unauth path reachable on the tunnel surface under the new architecture. Test updates in browse/test/server-auth.test.ts: * /pair liveness-verify test: assert via closeTunnel() helper instead of the inline `tunnelActive = false; tunnelUrl = null` lines that the helper subsumes. * /tunnel/start cached-tunnel test: same closeTunnel() adaptation. Credit Derived from PR garrytan#1026 by @garagon — thanks for flagging the critical bug that drove the architectural rewrite. The per-request isTunneledRequest approach from garrytan#1026 is superseded by physical port separation here; the underlying report remains the root cause for the entire v1.6.0.0 wave. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(security): add source-level guards for dual-listener architecture 23 source-level assertions that keep future contributors from silently widening the tunnel surface during a routine refactor. Covers: * Surface type + tunnelServer state variable shape * TUNNEL_PATHS is a closed set of /connect, /command, /sidebar-chat (and NOT /health, /welcome, /cookie-picker, /inspector/*, /pair, /token, /refs, /activity/stream, /tunnel/{start,stop}) * TUNNEL_COMMANDS includes browser-driving ops only (and NOT launch-browser, tunnel-start, token-mint, cookie-import, etc.) * makeFetchHandler(surface) factory exists and is wired to both listeners with the correct surface parameter * Tunnel filter runs BEFORE any route dispatch, with 404/403/401 responses and logged denials for each reason * GET /connect returns {alive: true} unauth * /command dispatch enforces TUNNEL_COMMANDS on tunnel surface * closeTunnel() helper tears down ngrok + Bun.serve listener * /tunnel/start binds on ephemeral port, points ngrok at TUNNEL_PORT (not local port), hard-fails on bind error (no fallback), probes cached tunnel via GET /connect (not /health), tears down on ngrok.forward failure * BROWSE_TUNNEL=1 startup uses the dual-listener pattern * logTunnelDenial wired for all three denial reasons * /connect rate limit is 300/min, not 3/min All 23 tests pass. Behavioral integration tests (spawn subprocess, real network) live in the E2E suite that lands later in this wave. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security: gate download + scrape through validateNavigationUrl (SSRF) The `goto` command was correctly wired through validateNavigationUrl, but `download` and `scrape` called page.request.fetch(url, ...) directly. A caller with the default write scope could hit the /command endpoint and ask the daemon to fetch http://169.254.169.254/latest/meta-data/ (AWS IMDSv1) or the GCP/Azure/internal equivalents. The response body comes back as base64 or lands on disk where GET /file serves it. Fix: call validateNavigationUrl(url) immediately before each page.request.fetch() call site in download and in the scrape loop. Same blocklist that already protects `goto`: file://, javascript:, data:, chrome://, cloud metadata (IPv4 all encodings, IPv6 ULA, metadata.*.internal). Tests: extend browse/test/url-validation.test.ts with a source-level guard that walks every `await page.request.fetch(` call site and asserts a validateNavigationUrl call precedes it within the same branch. Regression trips before code review if a future refactor drops the gate. * security: route splitForScoped through envelope sentinel escape The scoped-token snapshot path in snapshot.ts built its untrusted block by pushing the raw accessibility-tree lines between the literal `═══ BEGIN UNTRUSTED WEB CONTENT ═══` / `═══ END UNTRUSTED WEB CONTENT ═══` sentinels. The full-page wrap path in content-security.ts already applied a zero-width-space escape on those exact strings to prevent sentinel injection, but the scoped path skipped it. Net effect: a page whose rendered text contains the literal sentinel can close the envelope early from inside untrusted content and forge a fake "trusted" block for the LLM. That includes fabricating interactive `@eN` references the agent will act on. Fix: * Extract the zero-width-space escape into a named, exported helper `escapeEnvelopeSentinels(content)` in content-security.ts. * Have `wrapUntrustedPageContent` call it (behavior unchanged on that path — same bytes out). * Import the helper in snapshot.ts and map it over `untrustedLines` in the `splitForScoped` branch before pushing the BEGIN sentinel. Tests: add a describe block in content-security.test.ts that covers * `escapeEnvelopeSentinels` defuses BEGIN and END markers; * `escapeEnvelopeSentinels` leaves normal text untouched; * `wrapUntrustedPageContent` still emits exactly one real envelope pair when hostile content contains forged sentinels; * snapshot.ts imports the helper; * the scoped-snapshot branch calls `escapeEnvelopeSentinels` before pushing the BEGIN sentinel (source-level regression — if a future refactor reorders this, the test trips). * security: extend hidden-element detection to all DOM-reading channels The Confusion Protocol envelope wrap (`wrapUntrustedPageContent`) covers every scoped PAGE_CONTENT_COMMAND, but the hidden-element ARIA-injection detection layer only ran for `text`. Other DOM-reading channels (html, links, forms, accessibility, attrs, data, media, ux-audit) returned their output through the envelope with no hidden- content filter, so a page serving a display:none div that instructs the agent to disregard prior system messages, or an aria-label that claims to put the LLM in admin mode, leaked the injection payload on any non-text channel. The envelope alone does not mitigate this, and the page itself never rendered the hostile content to the human operator. Fix: * New export `DOM_CONTENT_COMMANDS` in commands.ts — the subset of PAGE_CONTENT_COMMANDS that derives its output from the live DOM. Console and dialog stay out; they read separate runtime state. * server.ts runs `markHiddenElements` + `cleanupHiddenMarkers` for every scoped command in this set. `text` keeps its existing `getCleanTextWithStripping` path (hidden elements physically stripped before the read). All other channels keep their output format but emit flagged elements as CONTENT WARNINGS on the envelope, so the LLM sees what it would otherwise have consumed silently. * Hidden-element descriptions merge into `combinedWarnings` alongside content-filter warnings before the wrap call. Tests: new describe block in content-security.test.ts covering * `DOM_CONTENT_COMMANDS` export shape and channel membership; * dispatch gates on `DOM_CONTENT_COMMANDS.has(command)`, not the literal `text` string; * hiddenContentWarnings plumbs into `combinedWarnings` and reaches wrapUntrustedPageContent; * DOM_CONTENT_COMMANDS is a strict subset of PAGE_CONTENT_COMMANDS. Existing datamarking, envelope wrap, centralized-wrapping, and chain security suites stay green (52 pass, 0 fail). * security: validate --from-file payload paths for parity with direct paths The direct `load-html <file>` path runs every caller-supplied file path through validateReadPath() so reads stay confined to SAFE_DIRECTORIES (cwd, TEMP_DIR). The `load-html --from-file <payload.json>` shortcut and its sibling `pdf --from-file <payload.json>` skipped that check and went straight to fs.readFileSync(). An MCP caller that picks the payload path (or any caller whose payload argument is reachable from attacker-influenced text) could use --from-file as a read-anywhere escape hatch for the safe-dirs policy. Fix: call validateReadPath(path.resolve(payloadPath)) before readFileSync at both sites. Error surface mirrors the direct-path branch so ops and agent errors stay consistent. Test coverage in browse/test/from-file-path-validation.test.ts: - source-level: validateReadPath precedes readFileSync in the load-html --from-file branch (write-commands.ts) and the pdf --from-file parser (meta-commands.ts) - error-message parity: both sites reference SAFE_DIRECTORIES Related security audit pattern: R3 F002 (validateNavigationUrl gap on download/scrape) and R3 F008 (markHiddenElements gap on 10 DOM commands) were the same shape — a defense that existed on the primary code path but not its shortcut sibling. This PR closes the same class of gap on the --from-file shortcuts. * fix(design): escape url.origin when injecting into served HTML serve.ts injected url.origin into a single-quoted JS string in the response body. A local request with a crafted Host header (e.g. Host: "evil'-alert(1)-'x") would break out of the string and execute JS in the 127.0.0.1:<port> origin opened by the design board. Low severity — bound to localhost, requires a local attacker — but no reason not to escape. Fix: JSON.stringify(url.origin) produces a properly quoted, escaped JS string literal in one call. Also includes Prettier reformatting (single→double quotes, trailing commas, line wrapping) applied by the repo's PostToolUse formatter hook. Security change is the one line in the HTML injection; everything else is whitespace/style. * fix(scripts): drop shell:true from slop-diff npx invocations spawnSync('npx', [...], { shell: true }) invokes /bin/sh -c with the args concatenated, subjecting them to shell parsing (word splitting, glob expansion, metacharacter interpretation). No user input reaches these calls today, so not exploitable — but the posture is wrong: npx + shell args should be direct. Fix: scope shell:true to process.platform === 'win32' where npx is actually a .cmd requiring the shell. POSIX runs the npx binary directly with array-form args. Also includes Prettier reformatting (single→double quotes, trailing commas, line wrapping) applied by the repo's PostToolUse formatter hook. Security-relevant change is just the two shell:true -> shell: process.platform === 'win32' lines; everything else is whitespace/style. * security(E3): gate GSTACK_SLUG on /welcome path traversal The /welcome handler interpolates GSTACK_SLUG directly into the filesystem path used to locate the project-local welcome page. Without validation, a slug like "../../etc/passwd" would resolve to ~/.gstack/projects/../../etc/passwd/designs/welcome-page-20260331/finalized.html — classic path traversal. Not exploitable today: GSTACK_SLUG is set by the gstack CLI at daemon launch, and an attacker would already need local env-var access to poison it. But the gate is one regex (^[a-z0-9_-]+$), and a defense-in-depth pass costs us nothing when the cost of being wrong is arbitrary file read via /welcome. Fall back to the safe 'unknown' literal when the slug fails validation — same fallback the code already uses when GSTACK_SLUG is unset. No behavior change for legitimate slugs (they all match the regex). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security(N1): replace ?token= SSE auth with HttpOnly session cookie Activity stream and inspector events SSE endpoints accepted the root AUTH_TOKEN via `?token=` query param (EventSource can't send Authorization headers). URLs leak to browser history, referer headers, server logs, crash reports, and refactoring accidents. Codex flagged this during the /plan-ceo-review outside voice pass. New auth model: the extension calls POST /sse-session with a Bearer token and receives a view-only session cookie (HttpOnly, SameSite=Strict, 30-min TTL). EventSource is opened with `withCredentials: true` so the browser sends the cookie back on the SSE connection. The ?token= query param is GONE — no more URL-borne secrets. Scope isolation (prior learning cookie-picker-auth-isolation, 10/10 confidence): the SSE session cookie grants access to /activity/stream and /inspector/events ONLY. The token is never valid against /command, /token, or any mutating endpoint. A leaked cookie can watch activity; it cannot execute browser commands. Components * browse/src/sse-session-cookie.ts — registry: mint/validate/extract/ build-cookie. 256-bit tokens, 30-min TTL, lazy expiry pruning, no imports from token-registry (scope isolation enforced by module boundary). * browse/src/server.ts — POST /sse-session mint endpoint (requires Bearer). /activity/stream and /inspector/events now accept Bearer OR the session cookie, and reject ?token= query param. * extension/sidepanel.js — ensureSseSessionCookie() bootstrap call, EventSource opened with withCredentials:true on both SSE endpoints. Tested via the source guards; behavioral test is the E2E pairing flow that lands later in the wave. * browse/test/sse-session-cookie.test.ts — 20 unit tests covering mint entropy, TTL enforcement, cookie flag invariants, cookie parsing from multi-cookie headers, and scope-isolation contract guard (module must not import token-registry). * browse/test/server-auth.test.ts — existing /activity/stream auth test updated to assert the new cookie-based gate and the absence of the ?token= query param. Cookie flag choices: * HttpOnly: token not readable from page JS (mitigates XSS exfiltration). * SameSite=Strict: cookie not sent on cross-site requests (mitigates CSRF). Fine for SSE because the extension connects to 127.0.0.1 directly. * Path=/: cookie scoped to the whole origin. * Max-Age=1800: 30 minutes, matches TTL. Extension re-mints on reconnect when daemon restarts. * Secure NOT set: daemon binds to 127.0.0.1 over plain HTTP. Adding Secure would block the browser from ever sending the cookie back. Add Secure when gstack ships over HTTPS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * security(N2): document Windows v20 ABE elevation path on CDP port The existing comment around the cookie-import-browser --remote-debugging-port launch claimed "threat model: no worse than baseline." That's wrong on Windows with App-Bound Encryption v20. A same-user local process that opens the cookie SQLite DB directly CANNOT decrypt v20 values (DPAPI context is bound to the browser process). The CDP port lets them bypass that: connect to the debug port, call Network.getAllCookies inside Chrome, walk away with decrypted v20 cookies. The correct fix is to switch from TCP --remote-debugging-port to --remote-debugging-pipe so the CDP transport is a stdio pipe, not a socket. That requires restructuring the CDP WebSocket client in this module and Playwright doesn't expose the pipe transport out of the box. Non-trivial, deferred from the v1.6.0.0 wave. This commit updates the comment to correctly describe the threat and points at the tracking issue. No code change to the launch itself. Follow-up: garrytan#1136. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(E2): document dual-listener tunnel architecture in ARCHITECTURE.md Adds an explicit per-endpoint disposition table to the Security model section, covering the v1.6.0.0 dual-listener refactor. Every HTTP endpoint now has a documented local-vs-tunnel answer. Future audits (and future contributors wondering "is it safe to add X to the tunnel surface?") can read this instead of reverse-engineering server.ts. Also documents: * Why physical port separation beats per-request header inference (ngrok behavior drift, local proxies can forge headers, etc.) * Tunnel surface denial logging → ~/.gstack/security/attempts.jsonl * SSE session cookie model (gstack_sse, 30-min TTL, stream-scope only, module-boundary-enforced scope isolation) * N2 non-goal for Windows v20 ABE via CDP port (tracking garrytan#1136) No code changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(E1): end-to-end pair-agent flow against a spawned daemon Spawns the browse daemon as a subprocess with BROWSE_HEADLESS_SKIP=1 so the HTTP layer runs without a real browser. Exercises: * GET /health — token delivery for chrome-extension origin, withheld otherwise (the F1 + PR garrytan#1026 invariant) * GET /connect — alive probe returns {alive:true} unauth * POST /pair — root Bearer required (403 without), returns setup_key * POST /connect — setup_key exchange mints a distinct scoped token * POST /command — 401 without auth * POST /sse-session — Bearer required, Set-Cookie has HttpOnly + SameSite=Strict (the N1 invariant) * GET /activity/stream — 401 without auth * GET /activity/stream?token= — 401 (the old ?token= query param is REJECTED, which is the whole point of N1) * GET /welcome — serves HTML, does not leak /etc/passwd content under the default 'unknown' slug (E3 regex gate) 12 behavioral tests, ~220ms end-to-end, no network dependencies, no ngrok, no real browser. This is the receipt for the wave's central 'pair-agent still works + the security boundary holds' claim. Tunnel-port binding (/tunnel/start) is deliberately NOT exercised here — it requires an ngrok authtoken and live network. The dual-listener route allowlist is covered by source-level guards in dual-listener.test.ts; behavioral tunnel testing belongs in a separate paid-evals harness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * release(v1.6.0.0): bump VERSION + CHANGELOG for security wave Architectural bump, not patch: dual-listener HTTP refactor changes the daemon's tunnel-exposure model. See CHANGELOG for the full release summary (~950 words) covering the five root causes this wave closes: 1. /health token leak over ngrok (F1 + E3 + test infra) 2. /cookie-picker + /inspector exposed over the tunnel (F1) 3. ?token=<ROOT> in SSE URLs leaking to logs/referer/history (N1) 4. /welcome GSTACK_SLUG path traversal (E3) 5. Windows v20 ABE elevation via CDP port (N2 — documented non-goal, tracked as garrytan#1136) Plus the base PRs: SSRF gate (garrytan#1029), envelope sentinel escape (garrytan#1031), DOM-channel hidden-element coverage (garrytan#1032), --from-file path validation (garrytan#1103), and 2 commits from garrytan#1073 (@theqazi). VERSION + package.json bumped to 1.6.0.0. CHANGELOG entry covers credits (@garagon, @Hybirdss, @HMAKT99, @theqazi), review lineage (CEO → Codex outside voice → Eng), and the non-goal tracking issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: pre-landing review findings (4 auto-fixes) Addresses 4 findings from the Claude adversarial subagent on the v1.6.0.0 security wave diff. No user-visible behavior change; all are defense-in-depth hardening of newly-introduced code. 1. GET /connect rate-limited (was POST-only) [HIGH conf 8/10] Attacker discovering the ngrok URL could probe unlimited GETs for daemon enumeration. Now shares the global /connect counter. 2. ngrok listener leak on tunnel startup failure [MEDIUM conf 8/10] If ngrok.forward() resolved but tunnelListener.url() or the state-file write threw, the Bun listener was torn down but the ngrok session was leaked. Fixed in BOTH /tunnel/start and BROWSE_TUNNEL=1 startup paths. 3. GSTACK_SKILL_ROOT path-traversal gate [MEDIUM conf 8/10] Symmetric with E3's GSTACK_SLUG regex gate — reject values containing '..' before interpolating into the welcome-page path. 4. SSE session registry pruning [LOW conf 7/10] pruneExpired() only checked 10 entries per mint call. Now runs on every validate too, checks 20 entries, with a hard 10k cap as backstop. Prevents registry growth under sustained extension reconnect pressure. Tests remain green (56/56 in sse-session-cookie + dual-listener + pair-agent-e2e suites). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: update project documentation for v1.6.0.0 Reflect the dual-listener tunnel architecture, SSE session cookies, SSRF guards, and Windows v20 ABE non-goal across the three docs users actually read for remote-agent and browser auth context: - docs/REMOTE_BROWSER_ACCESS.md: rewrote Architecture diagram for dual listeners, fixed /connect rate limit (3/min → 300/min), removed stale "/health requires no auth" (now 404 on tunnel), added SSE cookie auth, expanded Security Model with tunnel allowlist, SSRF guards, /welcome path traversal defense, and the Windows v20 ABE tracking note. - BROWSER.md: added dual-listener paragraph to Authentication and linked to ARCHITECTURE.md endpoint table. Replaced the stale ?token= SSE auth note with the HttpOnly gstack_sse cookie flow. - CLAUDE.md: added Transport-layer security section above the sidebar prompt-injection stack so contributors editing server.ts, sse-session-cookie.ts, or tunnel-denial-log.ts see the load-bearing module boundaries before touching them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(make-pdf): write --from-file payload to /tmp, not os.tmpdir() make-pdf's browseClient wrote its --from-file payload to os.tmpdir(), which is /var/folders/... on macOS. v1.6.0.0's PR garrytan#1103 cherry-pick tightened browse load-html --from-file to validate against the safe-dirs allowlist ([TEMP_DIR, cwd] where TEMP_DIR is '/tmp' on macOS/Linux, os.tmpdir() on Windows). This closed a CLI/API parity gap but broke make-pdf on macOS because /var/folders/... is outside the allowlist. Fix: mirror browse's TEMP_DIR convention — use '/tmp' on non-Windows, os.tmpdir() on Windows. The make-pdf-gate CI failure on macOS-latest (run 72440797490) is caused by exactly this: the payload file was rejected by validateReadPath. Verified locally: the combined-gate e2e test now passes after rebuilding make-pdf/dist/pdf. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sidebar): killAgent resets per-tab state; align tests with current agent event format Two pre-existing bugs surfaced while running the full e2e suite on the sec-wave branch. Both pre-date v1.6.0.0 (same failures on main at e23ff28) but blocked the ship verification, so fixing now. ### Bug 1: killAgent leaked stale per-tab state `killAgent()` reset the legacy globals (agentProcess, agentStatus, etc.) but never touched the per-tab `tabAgents` Map. Meanwhile `/sidebar-command` routes on `tabState.status` from that Map, not the legacy globals. Consequence: after a kill (including the implicit kill in `/sidebar-session/new`), the next /sidebar-command on the same tab saw `tabState.status === 'processing'` and fell into the queue branch, silently NOT spawning an agent. Integration tests that called resetState between cases all failed with empty queues. Fix: when targetTabId is supplied, reset that one tab's state; when called without a tab (session-new, full kill), reset ALL tab states. Matches the semantic boundary already used for the cancel-file write. ### Bug 2: sidebar-integration tests drifted from current event format `agent events appear in /sidebar-chat` posted the raw Claude streaming format (`{type: 'assistant', message: {content: [...]}}`) but `processAgentEvent` in server.ts only handles the simplified types that sidebar-agent.ts pre-processes into (text, text_delta, tool_use, result, agent_error, security_event). The architecture moved pre-processing into sidebar-agent.ts at some point and this test never got updated. Fixed by sending the pre-processed `{type: 'text', text: '...'}` format — which is actually what the server sees in production. Also removed the `entry.prompt` URL-containment check in the queue-write test. The URL is carried on entry.pageUrl (metadata) by design: the system prompt tells Claude to run `browse url` to fetch the actual page rather than trust any URL in the prompt body. That's the URL-based prompt-injection defense. The prompt SHOULD NOT contain the URL, so the test assertion was wrong for the current security posture. ### Verification - `bun test browse/test/sidebar-integration.test.ts` → 13/13 pass (was 6/13 on both main and branch before this commit) - Full `bun run test` → exit 0, zero fail markers - No behavior change for production sidebar flows: killAgent was already supposed to return the agent to idle; it just wasn't fully doing so. Per-tab reset now matches the documented semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: gus <gustavoraularagon@gmail.com> Co-authored-by: Mohammed Qazi <10266060+theqazi@users.noreply.github.com>
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
1 participant
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.
Summary
gstack ships
pair-agent --client fooas the documented onboarding flow: run it, get a shareablehttps://…ngrok-free.appURL, send it to the other person, they point their agent at it and start driving your local browser. The flow works because the daemon opens an ngrok tunnel viaPOST /tunnel/startand stays in headed mode so you can watch the session.Same daemon exposes
/health. That route has an audit history — the v0.15.13.0 wave-3 fix restricted theAUTH_TOKENfield tochrome-extension://origins so a tunneled instance couldn't leak a bearer that unlocks/command,/read,/upload. Later refactors broadened the gate with an||:The comment two lines above reads "in headed mode the server is always local, so return token unconditionally." That assumption doesn't hold once
pair-agent(orBROWSE_TUNNEL=1) is the default onboarding flow. Headed and tunneled is the explicitly-documented case.Net effect: any unauthenticated client that knows the tunnel URL can do
curl https://xxx.ngrok-free.app/health | jq -r .tokenand receive the root
AUTH_TOKEN. That token is accepted by/command(arbitrary JS in the browser),/read(cookies + storage),/upload(file write on the server's disk), and everything else guarded byvalidateAuth. One-shot remote browser takeover.This isn't a 0-day against a local install — it's a live vector against anyone who ran
pair-agent --clientand didn't explicitly stop the tunnel. That's the documented shape of the feature.Root cause
The
||that was added to fix the Playwright-extension-bootstrap case collapsed two very different conditions into one trust boundary:Origin: chrome-extension://<id>connectionMode === 'headed'pair-agent,BROWSE_HEADED=1, every interactive session. Orthogonal to whether a tunnel is open.The
tunnelActiveflag atbrowse/src/server.ts:61already tracks tunnel state (set by/tunnel/start, cleared by/tunnel/stopand by the liveness probe at line 1505). The/healthgate didn't consult it.Fix
chrome-extension://caller still gets the token. MV3 isolation is the localhost guarantee; the token is already inside the extension's process, emitting it back adds no new surface.pair-agent/BROWSE_TUNNEL=1/ any caller hitsPOST /tunnel/start,tunnelActiveflips totrueand/healthstops emitting the token, regardless of mode.tunnelActive(ngrok died externally), the local-only branch re-opens automatically.Reproduction
Before the fix, with the shipped defaults:
After the fix, the same
/healthrequest returns{status, mode, uptime, tabs, …}with notokenfield./commandwithout the token returns 401.Sibling review
Grepped every
AUTH_TOKENemission inbrowse/src/for the same pattern:AUTH_TOKENserver.ts:~1347(/health)headed || chrome-extensionserver.tselsewherevalidateAuth(Authorization header compare)cli.ts,sidebar-agent.tsNo other HTTP surface emits the raw token. The fix is scoped to the one
/healthbranch.Tests
browse/test/server-auth.test.ts— source-level guardrails. Kept DB-free and framework-independent, matching the convention in that file.New regression test
If a future refactor drops the
tunnelActivegate this test turns red before the code reaches a reviewer.Negative control
Reverting
browse/src/server.tstoorigin/mainand rerunningbun test browse/test/server-auth.test.ts:The one failing test is the new one. Applying the fix: 38/38 pass.
Full suite
(The "FAIL" rows inside the self-tests of the evals framework are expected by those self-tests — they assert that the framework marks simulated failures correctly.)
What stayed the same
validateAuthremains the primary auth path; this PR only closes the one HTTP response that handed the token out unauthenticated.tunnelActivelifecycle — the fix reads the existing flag.Files
How to verify