mirror of
https://github.com/garrytan/gstack.git
synced 2026-05-05 21:25:27 +02:00
Garry Tan
67a511c0b1
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>