From e8769fd5b7d210de400ba99d9910d9a14ef1fee8 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Tue, 6 Oct 2026 21:21:53 -0700 Subject: [PATCH 1/2] Validate the Host header on every request (defense in depth) Every plan, review, annotate, archive and goal-setup server, in both the Bun and Pi runtimes, now checks the request's Host header before any route (app HTML, API, WebSocket upgrades). Loopback names are always accepted; remote mode also accepts IP literals, the configured urlHost and the machine's hostname; a --tailscale session accepts its serve name. Other hostnames get a 403 naming PLANNOTATOR_ALLOWED_HOSTS, the new escape hatch for proxies and tunnels that forward their own hostname. The port is not compared, so forwarded ports keep working. --- AGENTS.md | 17 +- .../docs/guides/remote-and-devcontainers.md | 8 + .../docs/reference/environment-variables.md | 1 + apps/pi-extension/pi-session-bridge.test.ts | 16 +- .../server/request-host-guard.test.ts | 69 ++++++ .../pi-extension/server/request-host-guard.ts | 100 ++++++++ apps/pi-extension/server/request-host.test.ts | 16 ++ apps/pi-extension/server/serverAnnotate.ts | 5 + apps/pi-extension/server/serverPlan.ts | 5 + apps/pi-extension/server/serverReview.ts | 5 + apps/pi-extension/vendor.sh | 2 +- packages/server/annotate.ts | 6 + packages/server/goal-setup.ts | 6 + packages/server/index.ts | 6 + packages/server/request-host-guard.ts | 86 +++++++ packages/server/request-host.scenarios.ts | 219 ++++++++++++++++++ packages/server/request-host.test.ts | 72 ++++++ packages/server/review.ts | 6 + packages/server/tailscale-serve.ts | 5 + packages/shared/package.json | 1 + packages/shared/request-host.test.ts | 99 ++++++++ packages/shared/request-host.ts | 178 ++++++++++++++ 22 files changed, 921 insertions(+), 7 deletions(-) create mode 100644 apps/pi-extension/server/request-host-guard.test.ts create mode 100644 apps/pi-extension/server/request-host-guard.ts create mode 100644 apps/pi-extension/server/request-host.test.ts create mode 100644 packages/server/request-host-guard.ts create mode 100644 packages/server/request-host.scenarios.ts create mode 100644 packages/server/request-host.test.ts create mode 100644 packages/shared/request-host.test.ts create mode 100644 packages/shared/request-host.ts diff --git a/AGENTS.md b/AGENTS.md index cd4d7292fd..eb92e84f30 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -144,6 +144,7 @@ claude --plugin-dir ./apps/hook | `PLANNOTATOR_AGENT_TERMINAL_REMOTE` | Set to `1` / `true` to enable the annotate-mode agent terminal while `PLANNOTATOR_REMOTE` is active or the session is published with `--tailscale`. Off by default in both cases because the session is reachable by network peers and the PTY token is not an auth boundary. | | `PLANNOTATOR_PORT` | Fixed port to use. Default: random locally, `19432` for remote sessions. | | `PLANNOTATOR_URL_HOST` | Display-only hostname for advertised session URLs (issue #657), e.g. a Tailscale MagicDNS name or tailnet IP, so remote-mode links are reachable from another device instead of `http://localhost:`. Host only — bare hostname, IPv4, or bracketed IPv6 (`[fd7a::1]`); the runtime-chosen port is always appended, and anything carrying a scheme, port, path, credentials, or whitespace warns once on stderr and falls back to `localhost`. Strictly display-only and remote-only: binding stays governed by `PLANNOTATOR_REMOTE`; a local session ignores the override (localhost is advertised and opened, since only loopback is bound) with a once-per-process stderr warning to set `PLANNOTATOR_REMOTE=1`, and spawned agent-review jobs keep a pinned `http://127.0.0.1:` API URL so a tailnet-only hostname cannot break local jobs. The sentinel `auto` resolves the host from Tailscale once per process, at first use in a remote session: `tailscale status --json` → `Self.DNSName` (trailing dot stripped), falling back to the single `tailscale ip -4` CGNAT (100.64.0.0/10) address; detection failure warns once on stderr and falls back to `localhost`, and detection never changes binding — `auto` is as display-only as any explicit host. Can also be set via `~/.plannotator/config.json` (`{ "urlHost": "host" }` or `{ "urlHost": "auto" }`); the env var takes precedence, and an empty-but-set env var (`PLANNOTATOR_URL_HOST=`) suppresses a config-file `urlHost`. Default: unset (`localhost`). | +| `PLANNOTATOR_ALLOWED_HOSTS` | Extra hostnames the servers answer on, for the Host allowlist every plan, review, annotate, archive and goal-setup server applies to every request (see "Host allowlist" under Server API). Comma list of hostnames; a leading dot (`.example.com`) also matches subdomains; `*` turns the check off. For proxies and tunnels that forward their own hostname (a Codespaces `*.app.github.dev` URL, a reverse proxy, a Docker service name). Read once per server start. Default: unset. | | `PLANNOTATOR_BROWSER` | Custom browser to open plans in. macOS: app name or path. Linux/Windows: executable path. | | `PLANNOTATOR_AI` | Set to `disabled` to disable Ask AI and the Review Agents / Guided Review execution surfaces, including provider and agent-job endpoints. Persisted guide data is retained and its server APIs remain available, but the in-app history browser is hidden while AI is disabled. External agents can still open reviews and submit annotations. The explicit annotate-mode agent terminal is separate and remains controlled by its own settings. Default: enabled. | | `PLANNOTATOR_SHARE` | Set to `disabled` to turn off URL sharing entirely, including Guided Review share links (the review UI hides "Create share link", `POST /api/guide/:jobId/share` answers `403 { error: "sharing disabled" }`, and `plannotator guide share` refuses with exit 1). Default: enabled. Can also be set via `~/.plannotator/config.json` (`{ "share": "disabled" }`); the env var takes precedence. | @@ -209,7 +210,7 @@ export PLANNOTATOR_PORT=9999 **Compressed app page for network sessions (#1617):** remote-mode and `--tailscale` sessions serve the single-file app HTML compressed, negotiated on `Accept-Encoding` (brotli q5, else gzip 6), with `Vary: Accept-Encoding`. Browsers advertise `br` only to HTTPS and localhost origins, so direct plain-http remote mode is served gzip and `--tailscale` (HTTPS) brotli; each server prewarms that likely encoding at start and compresses the other lazily, cached per process (bounded to 4 bodies, which persist for the life of long-lived Pi/OpenCode hosts). Plain local sessions serve the page exactly as before, because on loopback compression only added latency. One shared helper, `packages/shared/app-html.ts` (vendored to Pi), serves every app-HTML route in both runtimes; API responses are never compressed. The single-file builds also drop KaTeX's `woff`/`ttf` fallbacks (`build/katex-woff2-only.ts`); every supported browser loads `woff2`. -**Tailnet sessions (`--tailscale`):** `plannotator review --tailscale` (also `annotate` and `annotate-last`/`last`; other subcommands reject the flag with a clear error; Bun CLI only, not mirrored to Pi) publishes the session over the user's tailnet instead of remote mode: the server stays **loopback-bound** and the CLI orchestrates `tailscale serve --bg --https= http://127.0.0.1:`, so devices on the tailnet reach the session over HTTPS while nothing listens beyond localhost and nothing is ever public (serve, never funnel). The advertised HTTPS URL prints on stderr with a terminal QR code (TTY only), and a publishing failure exits `1` — or `2` under a strict annotate gate (`--require-approval` / `--result-file`), where `1` is reserved for "the reviewer did not approve" and a publish failure is a startup failure like any other — with an actionable message instead of leaving the loopback server hanging (CLI missing, daemon down or logged out, unparsable `serve status` output — which fails closed, or no serve URL matching the session port). A pre-existing serve mapping on the chosen port — background or foreground session, which Tailscale prefers — aborts rather than being stolen, and mappings on other ports are never touched. Mappings the process creates are cleaned up on normal exit and on SIGINT/SIGTERM/SIGHUP (all routed through `process.exit` so exit handlers run; the SIGHUP route is installed by `enableTailscaleServe` only once a mapping exists — an unconditional SIGHUP listener would override the ignored disposition `nohup` depends on, so plain non-tailscale sessions keep no SIGHUP listener and `nohup plannotator review &` survives terminal close); a failed teardown retries once and then warns with the exact manual command, and a hard kill (SIGKILL) or reboot can leave the mapping — `tailscale serve --bg` state persists — so remove it with `tailscale serve --https= off`. Combined with `PLANNOTATOR_REMOTE`/SSH detection, `--tailscale` wins and forces local mode with a stderr notice — the wide `0.0.0.0` bind would only broaden exposure (`urlHost` is also suppressed for the run; the advertised URL comes from serve). The annotate agent terminal is **off by default** in `--tailscale` sessions, exactly like remote mode, because the session is reachable across the tailnet and the PTY token is not an auth boundary; enable it with `PLANNOTATOR_AGENT_TERMINAL_REMOTE=1`. Orchestration lives in `packages/server/tailscale-serve.ts` on shared parsers in `packages/shared/tailscale.ts`. +**Tailnet sessions (`--tailscale`):** `plannotator review --tailscale` (also `annotate` and `annotate-last`/`last`; other subcommands reject the flag with a clear error; Bun CLI only, not mirrored to Pi) publishes the session over the user's tailnet instead of remote mode: the server stays **loopback-bound** and the CLI orchestrates `tailscale serve --bg --https= http://127.0.0.1:`, so devices on the tailnet reach the session over HTTPS while nothing listens beyond localhost and nothing is ever public (serve, never funnel). The advertised HTTPS URL prints on stderr with a terminal QR code (TTY only), and a publishing failure exits `1` — or `2` under a strict annotate gate (`--require-approval` / `--result-file`), where `1` is reserved for "the reviewer did not approve" and a publish failure is a startup failure like any other — with an actionable message instead of leaving the loopback server hanging (CLI missing, daemon down or logged out, unparsable `serve status` output — which fails closed, or no serve URL matching the session port). A pre-existing serve mapping on the chosen port — background or foreground session, which Tailscale prefers — aborts rather than being stolen, and mappings on other ports are never touched. Mappings the process creates are cleaned up on normal exit and on SIGINT/SIGTERM/SIGHUP (all routed through `process.exit` so exit handlers run; the SIGHUP route is installed by `enableTailscaleServe` only once a mapping exists — an unconditional SIGHUP listener would override the ignored disposition `nohup` depends on, so plain non-tailscale sessions keep no SIGHUP listener and `nohup plannotator review &` survives terminal close); a failed teardown retries once and then warns with the exact manual command, and a hard kill (SIGKILL) or reboot can leave the mapping — `tailscale serve --bg` state persists — so remove it with `tailscale serve --https= off`. Combined with `PLANNOTATOR_REMOTE`/SSH detection, `--tailscale` wins and forces local mode with a stderr notice — the wide `0.0.0.0` bind would only broaden exposure (`urlHost` is also suppressed for the run; the advertised URL comes from serve). `tailscale serve` forwards the browser's own Host header (the MagicDNS name), so the CLI adds the serve hostname to the server's Host allowlist once serve reports it (see "Host allowlist" under Server API). The annotate agent terminal is **off by default** in `--tailscale` sessions, exactly like remote mode, because the session is reachable across the tailnet and the PTY token is not an auth boundary; enable it with `PLANNOTATOR_AGENT_TERMINAL_REMOTE=1`. Orchestration lives in `packages/server/tailscale-serve.ts` on shared parsers in `packages/shared/tailscale.ts`. ## Plan Review Flow @@ -1943,6 +1944,20 @@ All servers use random ports locally or fixed port (`19432`) in remote mode. **Stale-tab guard (`serverSession`).** Because a port can be reused (a fixed `PLANNOTATOR_PORT`, remote mode's `19432`, a rare random-port collision), a tab left open on an old session could post its decision to a NEW server on the same address and approve a document it never showed. Every plan, annotate and review server (Bun and Pi) therefore issues one random `serverSession` nonce per process (`packages/core/server-session.ts`, vendored to Pi) and advertises it on `/api/plan` and on every diff payload (`/api/diff` and the switch/PR endpoints, beside `approvalNotesSupported`). The client (`packages/ui/utils/serverSession.ts`) echoes it on every decision: `serverSession` in the body of plan `/api/approve` + `/api/deny`, annotate `/api/approve` + `/api/feedback`, review `/api/feedback` (the platform status post included), and as `?serverSession=` on `/api/exit` (whose body is empty). A server holding a DIFFERENT nonce answers `409 { code: "session_mismatch", error }` before it claims or settles anything, and the tab shows `ServerSessionReplacedBanner` ("This review was replaced — reload"). A decision WITHOUT the field is accepted, so an older client (and any host posting decisions by hand) keeps working; only a present, different nonce is refused. `/api/pr-action` (which posts the review to GitHub, GitLab or Bitbucket) carries the nonce in its body too and is guarded the same way in both runtimes: a mismatch posts nothing and the tab shows the same banner. +### Host allowlist (every request) + +Every plan, review, annotate (archive and goal-setup included, and the servers OpenCode embeds) checks the request's `Host` header BEFORE any route, in both runtimes: the app HTML, every API and every WebSocket upgrade. The rule is one pure predicate, `isAllowedRequestHost` in `packages/shared/request-host.ts` (vendored to Pi); the adapters are `packages/server/request-host-guard.ts` (Bun, first lines of each `fetch`) and `apps/pi-extension/server/request-host-guard.ts` (node:http, first line of each handler, plus an `upgrade` interception, because node:http hands an upgrade to every `upgrade` listener, such as the annotate agent terminal's). Why: `isSameOriginOrNoOrigin` compares Origin with Host, which cannot tell the session's own page from a page served under another DNS name that resolves to this machine (both carry matching Origin and Host), and most GETs had no check at all. IP literals and loopback names cannot be re-pointed by DNS, so the allowlist is built from them: + +- **Every mode:** loopback names: `localhost`, `*.localhost`, `127.0.0.0/8` literals, `[::1]` (`isLoopbackHostname`). +- **Remote mode** (`PLANNOTATOR_REMOTE` / SSH, wide bind): also any IPv4 or IPv6 literal (LAN or container address), the configured urlHost (`auto` resolved through the same cached Tailscale lookup the advertised URL uses), and this machine's `os.hostname()` (full and first label). These names are resolved lazily, on the first request that is neither loopback nor an IP literal. +- **`--tailscale`** (local mode, Bun CLI only): `tailscale serve` forwards the browser's own Host (the MagicDNS name; for TCP backends it sets `r.Out.Host = r.In.Host`), so `enableTailscaleServe` registers the serve URL's hostname with `allowServedHostname` once serve reports it. +- **`PLANNOTATOR_ALLOWED_HOSTS`**: extra names, `.suffix` entries, or `*` (off). See the env table. +- **No Host header** (an HTTP/1.0 non-browser client): allowed; browsers always send one. A malformed Host is refused. +- **The port is never compared.** Forwarded ports legitimately arrive with another port (`ssh -L 8080:localhost:19432`, VS Code / devcontainer auto-forwarding to a loopback-bound server when the local port is taken, `docker -p`), and a foreign name is refused by its hostname alone. The VS Code cookie proxy rewrites Host to the upstream's `localhost:`, a loopback name. The narrower loopback-AND-port checks of the pull bridge and host control stay as they were, behind this one. +- **Refusal:** `403`, `text/plain`, `X-Plannotator-Error: host_not_allowed`, a one-paragraph body naming the hostname (only when it parses as one) and `PLANNOTATOR_ALLOWED_HOSTS`; never a 404, so it cannot be mistaken for the app catch-all. + +Not covered, by design: the live-app proxy (`live-proxy-core.ts` has its own Host, Origin and WebSocket-origin checks), the paste service (a public service, not a session) and the guides.show local static dev server (serves build output only). Tests: `packages/shared/request-host.test.ts` (the predicate), `packages/server/request-host.scenarios.ts` run by `packages/server/request-host.test.ts` (Bun, plus goal setup and the tailnet name) and `apps/pi-extension/server/request-host.test.ts` (Pi), and `apps/pi-extension/server/request-host-guard.test.ts` (the upgrade interception). + ### Host session control `GET /api/host/status` and `POST /api/host/close` (`packages/shared/host-control.ts`, diff --git a/apps/marketing/src/content/docs/guides/remote-and-devcontainers.md b/apps/marketing/src/content/docs/guides/remote-and-devcontainers.md index 05d413665d..725abf1eec 100644 --- a/apps/marketing/src/content/docs/guides/remote-and-devcontainers.md +++ b/apps/marketing/src/content/docs/guides/remote-and-devcontainers.md @@ -93,6 +93,14 @@ Or via `docker run`: docker run -e PLANNOTATOR_REMOTE=1 -e PLANNOTATOR_PORT=9999 -p 9999:9999 your-image ``` +## Proxies and forwarded hostnames + +Plannotator answers only the hostnames it expects in the `Host` header: `localhost` and other loopback names everywhere, plus, in remote mode, IP addresses, this machine's hostname and `PLANNOTATOR_URL_HOST`. Port forwarding (SSH, VS Code, `docker -p`) arrives as `localhost` or an IP address and needs nothing. If a proxy or tunnel forwards its own hostname (for example a Codespaces `*.app.github.dev` URL or a reverse proxy), Plannotator answers `403` and names the setting to change: + +```bash +export PLANNOTATOR_ALLOWED_HOSTS=my-proxy.example.com +``` + ## Custom browser The `PLANNOTATOR_BROWSER` environment variable lets you specify a custom browser or script for opening the UI. diff --git a/apps/marketing/src/content/docs/reference/environment-variables.md b/apps/marketing/src/content/docs/reference/environment-variables.md index 0bbae3678e..1a8d6d5055 100644 --- a/apps/marketing/src/content/docs/reference/environment-variables.md +++ b/apps/marketing/src/content/docs/reference/environment-variables.md @@ -15,6 +15,7 @@ All Plannotator environment variables and their defaults. | `PLANNOTATOR_REMOTE` | auto-detect | Set to `1` or `true` to force remote mode, `0` or `false` to force local mode, or leave unset to auto-detect via `SSH_TTY` / `SSH_CONNECTION`. Uses a fixed port in remote mode; browser-opening behavior depends on the environment. Remote mode also turns off [live local app annotation](/docs/commands/annotate/#local-apps): `plannotator annotate http://localhost:5173` exits with a startup failure suggesting `--static` rather than opening a live session. | | `PLANNOTATOR_PORT` | random (local) / `19432` (remote) | Fixed server port or inclusive range such as `19432-19463`. A range uses the first available port. When not set, local sessions use a random port; remote sessions default to `19432`. | | `PLANNOTATOR_URL_HOST` | unset (`localhost`) | Display-only hostname for advertised session URLs, e.g. a Tailscale MagicDNS name or tailnet IP, so remote-mode links are reachable from another device instead of `http://localhost:`. Host only: bare hostname, IPv4, or bracketed IPv6 such as `[fd7a::1]` — the runtime-chosen port is always appended, and a value carrying a scheme, port, path, or whitespace warns on stderr and falls back to `localhost`. Strictly display-only and remote-only: it never changes which interface the server binds (that stays governed by `PLANNOTATOR_REMOTE`), and a local session ignores the override — the advertised URL stays `localhost`, with a stderr warning to set `PLANNOTATOR_REMOTE=1`. Can also be set via `~/.plannotator/config.json` (`{ "urlHost": "host" }`); the env var takes precedence, and setting it to an empty value suppresses a config-file `urlHost`. | +| `PLANNOTATOR_ALLOWED_HOSTS` | unset | Extra hostnames the session answers on. Every Plannotator server checks the `Host` header of each request: loopback names (`localhost`, `127.x.x.x`, `[::1]`, `*.localhost`) are always accepted; remote mode also accepts IP addresses, this machine's hostname and the configured `PLANNOTATOR_URL_HOST`; a `--tailscale` session accepts its tailnet name. Any other hostname gets `403`. If you reach Plannotator through a proxy or tunnel that keeps its own hostname (a Codespaces or Gitpod forwarded URL, a reverse proxy, a Docker service name), list it here, comma-separated; a leading dot (`.example.com`) also matches subdomains, and `*` turns the check off. The port is never compared. | | `PLANNOTATOR_BROWSER` | system default | Custom browser to open the UI in. macOS: app name or path. Linux/Windows: executable path. Can also be a script. Takes priority over `BROWSER`. Also settable per-invocation with `--browser`. | | `BROWSER` | (none) | Standard env var for specifying a browser. VS Code sets this automatically in devcontainers. Used as fallback when `PLANNOTATOR_BROWSER` is not set. | | `PLANNOTATOR_ORIGIN` | auto-detect | Explicit agent-origin override. Valid values: `claude-code`, `amp`, `droid`, `opencode`, `codex`, `copilot-cli`, `pi`, `oh-my-pi`, `gemini-cli`, `kiro-cli`, `mistral-vibe`. Invalid values silently fall through to env-based detection. | diff --git a/apps/pi-extension/pi-session-bridge.test.ts b/apps/pi-extension/pi-session-bridge.test.ts index dcb72fb255..04245c1157 100644 --- a/apps/pi-extension/pi-session-bridge.test.ts +++ b/apps/pi-extension/pi-session-bridge.test.ts @@ -491,12 +491,14 @@ describe("Pi annotate server with Ask this session", () => { }), })).json() as { sessionId: string }; - // DNS rebinding: same socket, foreign Host header. Refused before the - // question reaches the session. + // Same socket, foreign Host header. Refused before the question + // reaches the session: by the server-wide Host allowlist for a + // foreign name, and by the bridge's own loopback-and-port check for + // a loopback name carrying another port. const port = Number(new URL(server.url).port); - const rebound = await new Promise<{ status: number; body: string }>((resolve, reject) => { + const askWithHost = (hostHeader: string) => new Promise<{ status: number; body: string }>((resolve, reject) => { const req = httpRequest( - { host: "127.0.0.1", port, method: "POST", path: "/api/ai/query", headers: { host: `evil.example:${port}`, "content-type": "application/json" } }, + { host: "127.0.0.1", port, method: "POST", path: "/api/ai/query", headers: { host: hostHeader, "content-type": "application/json" } }, (res) => { let body = ""; res.on("data", (chunk) => { body += chunk; }); @@ -506,8 +508,12 @@ describe("Pi annotate server with Ask this session", () => { req.on("error", reject); req.end(JSON.stringify({ sessionId, prompt: "injected" })); }); + const rebound = await askWithHost(`evil.example:${port}`); expect(rebound.status).toBe(403); - expect(rebound.body).toContain("session_bridge_forbidden_host"); + expect(rebound.body).toContain("PLANNOTATOR_ALLOWED_HOSTS"); + const otherPort = await askWithHost(`localhost:${port + 1}`); + expect(otherPort.status).toBe(403); + expect(otherPort.body).toContain("session_bridge_forbidden_host"); expect(host.sent).toHaveLength(0); // Node flushes SSE headers with the first chunk, so drive the turn before awaiting. diff --git a/apps/pi-extension/server/request-host-guard.test.ts b/apps/pi-extension/server/request-host-guard.test.ts new file mode 100644 index 0000000000..df476c6e19 --- /dev/null +++ b/apps/pi-extension/server/request-host-guard.test.ts @@ -0,0 +1,69 @@ +/** + * The Pi guard's WebSocket half: node:http hands an upgrade to every + * `upgrade` listener (the annotate agent terminal attaches one), so the guard + * intercepts the event. A refused Host must never reach the listener. + */ +import { afterEach, expect, test } from "bun:test"; +import { createServer, request, type Server } from "node:http"; +import type { AddressInfo } from "node:net"; +import { createRequestHostGuard } from "./request-host-guard.ts"; + +const saved = { remote: process.env.PLANNOTATOR_REMOTE, allowed: process.env.PLANNOTATOR_ALLOWED_HOSTS }; +let server: Server | undefined; + +afterEach(async () => { + if (saved.remote === undefined) delete process.env.PLANNOTATOR_REMOTE; + else process.env.PLANNOTATOR_REMOTE = saved.remote; + if (saved.allowed === undefined) delete process.env.PLANNOTATOR_ALLOWED_HOSTS; + else process.env.PLANNOTATOR_ALLOWED_HOSTS = saved.allowed; + await new Promise((resolve) => (server ? server.close(() => resolve()) : resolve())); + server = undefined; +}); + +// Real Node answers the refused upgrade with the guard's 403 and completes +// the allowed one; Bun's node:http emulation (which runs this suite) does not +// relay raw bytes written to an upgrade socket, so both read as a closed +// socket there. What must hold in both is who reached the listener. +function upgrade(port: number, host: string): Promise { + return new Promise((resolve) => { + const req = request({ + host: "127.0.0.1", + port, + path: "/ws", + headers: { host, connection: "Upgrade", upgrade: "websocket" }, + }); + req.on("response", (res) => { + res.resume(); + resolve(res.statusCode ?? 0); + }); + req.on("upgrade", (_res, socket) => { + socket.destroy(); + resolve("upgraded"); + }); + req.on("error", () => resolve("closed")); + req.end(); + }); +} + +test("a refused Host never reaches an upgrade listener; an allowed one does", async () => { + process.env.PLANNOTATOR_REMOTE = "0"; + delete process.env.PLANNOTATOR_ALLOWED_HOSTS; + const guard = createRequestHostGuard(); + const reached: string[] = []; + server = createServer((req, res) => { + if (guard.refuse(req, res)) return; + res.end("ok"); + }); + guard.attach(server); + server.on("upgrade", (req, socket) => { + reached.push(String(req.headers.host)); + socket.end("HTTP/1.1 101 Switching Protocols\r\nConnection: Upgrade\r\nUpgrade: websocket\r\n\r\n"); + }); + await new Promise((resolve) => server!.listen(0, "127.0.0.1", resolve)); + const port = (server.address() as AddressInfo).port; + + expect([403, "closed"]).toContain(await upgrade(port, `evil.example:${port}`)); + expect(reached).toEqual([]); + expect(["upgraded", "closed"]).toContain(await upgrade(port, `localhost:${port}`)); + expect(reached).toEqual([`localhost:${port}`]); +}); diff --git a/apps/pi-extension/server/request-host-guard.ts b/apps/pi-extension/server/request-host-guard.ts new file mode 100644 index 0000000000..025471ecf3 --- /dev/null +++ b/apps/pi-extension/server/request-host-guard.ts @@ -0,0 +1,100 @@ +/** + * node:http adapter for the Host-header allowlist + * (packages/shared/request-host.ts, vendored to generated/request-host.ts). + * Mirrors packages/server/request-host-guard.ts: every Pi server (plan, + * review, annotate) builds one guard at start, refuses a disallowed Host + * before any route, and guards WebSocket upgrades too (the annotate agent + * terminal attaches its own `upgrade` listener, which never sees a refused + * request). Pi has no `--tailscale`, so there are no served hostnames here. + */ + +import type { IncomingMessage, Server, ServerResponse } from "node:http"; +import type { Duplex } from "node:stream"; +import os from "node:os"; +import { loadConfig, resolveUrlHost } from "../generated/config.ts"; +import { + ALLOWED_HOSTS_ENV, + HOST_NOT_ALLOWED_CODE, + hostNotAllowedMessage, + isAllowedRequestHost, + machineHostnames, + parseAllowedHosts, + type RequestHostPolicy, +} from "../generated/request-host.ts"; +import { isAutoUrlHost, resolveAutoHostCached } from "../generated/tailscale.ts"; +import { isRemoteSession } from "./network.ts"; + +function remoteExtraHosts(): string[] { + const hosts = machineHostnames(os.hostname()); + try { + const urlHost = resolveUrlHost(loadConfig()); + const resolved = urlHost && isAutoUrlHost(urlHost) ? resolveAutoHostCached() : urlHost; + if (resolved) hosts.push(resolved); + } catch { + // A broken config never widens the allowlist. + } + return hosts; +} + +function hostHeader(req: IncomingMessage): string | undefined { + const value = req.headers.host; + return typeof value === "string" ? value : undefined; +} + +export interface PiRequestHostGuard { + /** Answers 403 and returns true when the request's Host is not allowed. */ + refuse(req: IncomingMessage, res: ServerResponse): boolean; + /** Refuse disallowed WebSocket upgrades on `server` (call before it listens). */ + attach(server: Server): void; +} + +/** Build the guard for one server. Env and config are read once, here. */ +export function createRequestHostGuard(): PiRequestHostGuard { + const remote = isRemoteSession(); + const allowed = parseAllowedHosts(process.env[ALLOWED_HOSTS_ENV]); + // Remote-mode names are resolved on the first request that needs them, so + // loopback and IP-literal requests never read config or ask Tailscale. + let remoteHosts: string[] | null = null; + const allows = (req: IncomingMessage): boolean => { + const host = hostHeader(req); + const policy: RequestHostPolicy = { remote, allowed }; + if (isAllowedRequestHost(host, policy)) return true; + if (!remote) return false; + remoteHosts ??= remoteExtraHosts(); + return isAllowedRequestHost(host, { ...policy, extraHosts: remoteHosts }); + }; + + return { + attach(server) { + // node:http hands an upgrade to EVERY `upgrade` listener, so a + // prepended listener cannot stop the others. Intercept the event. + const emit = server.emit.bind(server) as (event: string | symbol, ...args: unknown[]) => boolean; + server.emit = ((event: string | symbol, ...args: unknown[]) => { + if (event === "upgrade") { + const req = args[0] as IncomingMessage; + const socket = args[1] as Duplex; + if (!allows(req)) { + const body = hostNotAllowedMessage(hostHeader(req)); + socket.end( + `HTTP/1.1 403 Forbidden\r\nContent-Type: text/plain; charset=utf-8\r\nContent-Length: ${Buffer.byteLength(body)}\r\nConnection: close\r\n\r\n${body}`, + ); + return true; + } + } + return emit(event, ...args); + }) as typeof server.emit; + }, + refuse(req, res) { + if (allows(req)) return false; + const body = hostNotAllowedMessage(hostHeader(req)); + res.writeHead(403, { + "Content-Type": "text/plain; charset=utf-8", + "X-Content-Type-Options": "nosniff", + "X-Plannotator-Error": HOST_NOT_ALLOWED_CODE, + "Content-Length": Buffer.byteLength(body), + }); + res.end(body); + return true; + }, + }; +} diff --git a/apps/pi-extension/server/request-host.test.ts b/apps/pi-extension/server/request-host.test.ts new file mode 100644 index 0000000000..de3f02e1bd --- /dev/null +++ b/apps/pi-extension/server/request-host.test.ts @@ -0,0 +1,16 @@ +/** + * Pi servers: the Host-header allowlist. Same scenarios as the Bun servers + * (packages/server/request-host.scenarios.ts), so the mirror cannot drift. + */ +import { defineRequestHostScenarios } from "../../../packages/server/request-host.scenarios.ts"; +import { startPlanReviewServer } from "./serverPlan.ts"; +import { startAnnotateServer } from "./serverAnnotate.ts"; +import { startReviewServer } from "./serverReview.ts"; + +const PATCH = "diff --git a/a.txt b/a.txt\n--- a/a.txt\n+++ b/a.txt\n@@ -1 +1 @@\n-a\n+b\n"; + +defineRequestHostScenarios("pi", async (kind, { docPath, htmlContent }) => { + if (kind === "plan") return startPlanReviewServer({ plan: "# Plan\n\n[notes](notes.md)", origin: "pi", htmlContent }); + if (kind === "review") return startReviewServer({ rawPatch: PATCH, gitRef: "HEAD", htmlContent }); + return startAnnotateServer({ markdown: "# Notes\n\nsecret body\n", filePath: docPath, htmlContent, mode: "annotate" }); +}); diff --git a/apps/pi-extension/server/serverAnnotate.ts b/apps/pi-extension/server/serverAnnotate.ts index 9c9da5e9bd..58893b6853 100644 --- a/apps/pi-extension/server/serverAnnotate.ts +++ b/apps/pi-extension/server/serverAnnotate.ts @@ -22,6 +22,7 @@ import { isPathAllowed } from "../generated/doc-resolve.ts"; import { countUnsentDraftComments, hostSessionClosedEvent } from "../generated/host-control.ts"; import { checkServerSession, createServerSessionNonce, serverSessionMismatchBody } from "../generated/server-session.ts"; import { handleHostControl, resolveHostControlToken, type HostControl } from "./host-control.ts"; +import { createRequestHostGuard } from "./request-host-guard.ts"; import { getPlanVersion, getVersionCount, listVersions } from "../generated/storage.ts"; import { computeAnnotateHistory, deriveAnnotateHistorySlug, persistAnnotateSubmission, type AnnotateHistoryResult } from "../generated/annotate-history.ts"; import { htmlDiff } from "../generated/html-diff.ts"; @@ -821,7 +822,10 @@ export async function startAnnotateServer(options: { let liveSessionToken = ""; let liveAppUrl = ""; + const requestHostGuard = createRequestHostGuard(); const server = createServer(async (req, res) => { + // Host allowlist first, before any route (request-host-guard.ts). + if (requestHostGuard.refuse(req, res)) return; const url = requestUrl(req); if (handleHostControl(req, res, url, { token: hostControlToken, getServerPort: () => boundPort, control: hostControl })) return; @@ -1396,6 +1400,7 @@ export async function startAnnotateServer(options: { let listened: Awaited>; try { + requestHostGuard.attach(server); listened = await listenOnPort(server); } catch (error) { disposeFailedStart(); diff --git a/apps/pi-extension/server/serverPlan.ts b/apps/pi-extension/server/serverPlan.ts index f7bfae2826..15a13f8293 100644 --- a/apps/pi-extension/server/serverPlan.ts +++ b/apps/pi-extension/server/serverPlan.ts @@ -7,6 +7,7 @@ import { contentHash, deleteDraft, loadDraft } from "../generated/draft.ts"; import { countUnsentDraftComments } from "../generated/host-control.ts"; import { checkServerSession, createServerSessionNonce, serverSessionMismatchBody } from "../generated/server-session.ts"; import { handleHostControl, resolveHostControlToken, type HostControl } from "./host-control.ts"; +import { createRequestHostGuard } from "./request-host-guard.ts"; import { type ArchivedPlan, generateSlug, @@ -292,7 +293,10 @@ export async function startPlanReviewServer(options: { // Lazy cache for in-session archive tab let cachedArchivePlans: ArchivedPlan[] | null = null; + const requestHostGuard = createRequestHostGuard(); const server = createServer(async (req, res) => { + // Host allowlist first, before any route (request-host-guard.ts). + if (requestHostGuard.refuse(req, res)) return; const url = requestUrl(req); if (handleHostControl(req, res, url, { token: hostControlToken, getServerPort: () => boundPort, control: hostControl })) return; @@ -644,6 +648,7 @@ export async function startPlanReviewServer(options: { } }); + requestHostGuard.attach(server); const { port, portSource } = await listenOnPort(server); boundPort = port; // Remote sessions serve the app page compressed (#1617); start gzip (what diff --git a/apps/pi-extension/server/serverReview.ts b/apps/pi-extension/server/serverReview.ts index aae7d63fed..b8aa453582 100644 --- a/apps/pi-extension/server/serverReview.ts +++ b/apps/pi-extension/server/serverReview.ts @@ -11,6 +11,7 @@ import { contentHash } from "../generated/draft.ts"; import { countUnsentDraftComments, hostSessionClosedEvent } from "../generated/host-control.ts"; import { checkServerSession, createServerSessionNonce, serverSessionMismatchBody } from "../generated/server-session.ts"; import { handleHostControl, resolveHostControlToken, type HostControl } from "./host-control.ts"; +import { createRequestHostGuard } from "./request-host-guard.ts"; import { createReviewDraftSession, prDraftTargetKey, type ReviewDraftKeys } from "../generated/review-draft.ts"; import { agentToolSaveFailed, loadConfig, saveConfig, detectGitUser, getServerConfig, parseReviewAnalysisConfig, resolveAIEnabled, resolveSharingEnabled, resolveClaudeSandbox, resolveCursorSandbox, resolveFeedbackHistory, resolveGuideHistory, resolveGuideShareUrl, resolveGitRemoteCheck, resolveReviewProgress } from "../generated/config.ts"; import { appendFeedbackRecord, countChangedFiles, deriveFeedbackProject, type FeedbackDecision, type FeedbackReviewTarget } from "../generated/feedback-archive.ts"; @@ -1962,7 +1963,10 @@ export async function startReviewServer(options: { let boundPort: number | undefined; const aiRuntime = aiEnabled ? await createPiAIRuntime({ getCwd: resolveAgentCwd, sessionBridge: options.sessionBridge, getServerPort: () => boundPort }) : null; + const requestHostGuard = createRequestHostGuard(); const server = createServer(async (req, res) => { + // Host allowlist first, before any route (request-host-guard.ts). + if (requestHostGuard.refuse(req, res)) return; const url = requestUrl(req); if (handleHostControl(req, res, url, { token: hostControlToken, getServerPort: () => boundPort, control: hostControl })) return; @@ -3960,6 +3964,7 @@ export async function startReviewServer(options: { } }); + requestHostGuard.attach(server); const { port, portSource } = await listenOnPort(server); boundPort = port; // Remote sessions serve the app page compressed (#1617); start gzip (what diff --git a/apps/pi-extension/vendor.sh b/apps/pi-extension/vendor.sh index 293cd33f45..e6490145ae 100755 --- a/apps/pi-extension/vendor.sh +++ b/apps/pi-extension/vendor.sh @@ -29,7 +29,7 @@ for f in config-types storage-types workspace-status-types; do done # Everything else in the original flat list stays sourced from packages/shared. -for f in annotate-bundle plannotator-tool prompts review-core review-image review-progress generated-files feedback-archive cli-pagination jj-core gitbutler-core vcs-core review-args review-open-state draft review-draft annotate-draft annotate-history pr-types pr-context-live pr-artifact-document pr-provider pr-stack pr-github pr-gitlab pr-bitbucket checklist integrations-common repo reference-common markdown-extensions resolve-file doc-resolve file-browser-watch-core annotate-reference-roots-node worktree worktree-pool html-to-markdown html-diff html-assets html-assets-node app-html url-to-markdown tour annotate-args annotate-target at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff call-flow-types call-flow-languages call-flow-pack-locks call-flow-install-lock call-flow call-flow-install single-flight source-save-node review-profiles guide-store guide-instructions-store commit-avatars commit-history port-range annotate-client-lease annotate-decision archive-mode tailscale live-proxy-core live-probe live-proxy-node loopback-host host-control request-origin; do +for f in annotate-bundle plannotator-tool prompts review-core review-image review-progress generated-files feedback-archive cli-pagination jj-core gitbutler-core vcs-core review-args review-open-state draft review-draft annotate-draft annotate-history pr-types pr-context-live pr-artifact-document pr-provider pr-stack pr-github pr-gitlab pr-bitbucket checklist integrations-common repo reference-common markdown-extensions resolve-file doc-resolve file-browser-watch-core annotate-reference-roots-node worktree worktree-pool html-to-markdown html-diff html-assets html-assets-node app-html url-to-markdown tour annotate-args annotate-target at-reference review-workspace-node review-workspace pfm-reminder improvement-hooks code-nav data-dir semantic-diff-types semantic-diff call-flow-types call-flow-languages call-flow-pack-locks call-flow-install-lock call-flow call-flow-install single-flight source-save-node review-profiles guide-store guide-instructions-store commit-avatars commit-history port-range annotate-client-lease annotate-decision archive-mode tailscale live-proxy-core live-probe live-proxy-node loopback-host host-control request-origin request-host; do src="../../packages/shared/$f.ts" # Shared modules that import browser-safe siblings from @plannotator/core # (e.g. guide-store → core/guide-format): generated/ is flat and vendors the diff --git a/packages/server/annotate.ts b/packages/server/annotate.ts index cef024e13a..e9b1a0a695 100644 --- a/packages/server/annotate.ts +++ b/packages/server/annotate.ts @@ -14,6 +14,7 @@ import { isSameOriginOrNoOrigin } from "@plannotator/shared/request-origin"; import { appHtmlResponse, likelyAppHtmlEncoding, prewarmAppHtml } from "@plannotator/shared/app-html"; import { isRemoteSession, getServerHostname, startBunServerOnAvailablePort, buildAdvertisedUrl } from "./remote"; +import { createRequestHostGuard } from "./request-host-guard"; import { getRepoInfo } from "./repo"; import type { Origin } from "@plannotator/shared/agents"; import { handleImage, handleUpload, handleServerReady, handleApiNotFound, handleFavicon, handleReferenceSkills, handleReferenceSkillContent, handleSaveNotes, readDraftGenerationFromBody, readDraftGenerationFromUrl } from "./shared-handlers"; @@ -845,6 +846,7 @@ export async function startAnnotateServer( let liveSessionToken = ""; let liveAppUrl = ""; + const requestHostGuard = createRequestHostGuard(); const server = await startBunServerOnAvailablePort((port) => Bun.serve({ hostname: getServerHostname(), @@ -854,6 +856,10 @@ export async function startAnnotateServer( idleTimeout: 0, async fetch(req, server) { + // Host allowlist first, before any route (request-host-guard.ts). + const hostRefusal = requestHostGuard.check(req); + if (hostRefusal) return hostRefusal; + const url = new URL(req.url); const hostControlResponse = handleHostControl(req, url, { diff --git a/packages/server/goal-setup.ts b/packages/server/goal-setup.ts index 475cde0945..dc854bc21a 100644 --- a/packages/server/goal-setup.ts +++ b/packages/server/goal-setup.ts @@ -17,6 +17,7 @@ import { type GoalSetupResult, } from "@plannotator/shared/goal-setup"; import { isRemoteSession, getServerHostname, startBunServerOnAvailablePort, buildAdvertisedUrl } from "./remote"; +import { createRequestHostGuard } from "./request-host-guard"; import { getRepoInfo } from "./repo"; import { handleFavicon, @@ -102,6 +103,7 @@ export async function startGoalSetupServer( resolveDecision(result); }; + const requestHostGuard = createRequestHostGuard(); const server = await startBunServerOnAvailablePort((port) => Bun.serve({ hostname: getServerHostname(), @@ -110,6 +112,10 @@ export async function startGoalSetupServer( idleTimeout: 0, async fetch(req) { + // Host allowlist first, before any route (request-host-guard.ts). + const hostRefusal = requestHostGuard.check(req); + if (hostRefusal) return hostRefusal; + const url = new URL(req.url); if ( diff --git a/packages/server/index.ts b/packages/server/index.ts index 9c6a87517a..83a3401c62 100644 --- a/packages/server/index.ts +++ b/packages/server/index.ts @@ -17,6 +17,7 @@ import { appHtmlResponse, likelyAppHtmlEncoding, prewarmAppHtml } from "@plannot import type { Origin } from "@plannotator/shared/agents"; import { resolve } from "path"; import { isRemoteSession, getServerHostname, startBunServerOnAvailablePort, buildAdvertisedUrl } from "./remote"; +import { createRequestHostGuard } from "./request-host-guard"; import { openEditorDiff } from "./ide"; import { saveToObsidian, @@ -350,6 +351,7 @@ export async function startPlannotatorServer( planRevision, }, { status: 409 }); + const requestHostGuard = createRequestHostGuard(); const server = await startBunServerOnAvailablePort((port) => Bun.serve({ hostname: getServerHostname(), @@ -359,6 +361,10 @@ export async function startPlannotatorServer( idleTimeout: 0, async fetch(req, server) { + // Host allowlist first, before any route (request-host-guard.ts). + const hostRefusal = requestHostGuard.check(req); + if (hostRefusal) return hostRefusal; + const url = new URL(req.url); const hostControlResponse = handleHostControl(req, url, { diff --git a/packages/server/request-host-guard.ts b/packages/server/request-host-guard.ts new file mode 100644 index 0000000000..5e46196945 --- /dev/null +++ b/packages/server/request-host-guard.ts @@ -0,0 +1,86 @@ +/** + * Bun adapter for the Host-header allowlist (packages/shared/request-host.ts). + * Every Bun server (plan, review, annotate, goal setup) builds one guard at + * start and runs it first in `fetch`, before any route, so the app HTML, every + * API and the WebSocket upgrades are covered alike. The Pi mirror is + * apps/pi-extension/server/request-host-guard.ts. + */ + +import os from "node:os"; +import { loadConfig, resolveUrlHost } from "@plannotator/shared/config"; +import { + ALLOWED_HOSTS_ENV, + HOST_NOT_ALLOWED_CODE, + hostNotAllowedMessage, + isAllowedRequestHost, + machineHostnames, + parseAllowedHosts, + type RequestHostPolicy, +} from "@plannotator/shared/request-host"; +import { isAutoUrlHost, resolveAutoHostCached } from "@plannotator/shared/tailscale"; +import { isRemoteSession } from "./remote"; + +/** + * Hostnames this process publishes sessions under besides loopback: the + * `tailscale serve` MagicDNS name of a `--tailscale` session, registered by + * enableTailscaleServe once serve reports its URL. `tailscale serve` forwards + * the browser's own Host header to the loopback backend, so without this the + * tailnet URL would be refused. + */ +const servedHostnames = new Set(); + +export function allowServedHostname(hostname: string): void { + if (hostname) servedHostnames.add(hostname.toLowerCase()); +} + +export function resetServedHostnamesForTests(): void { + servedHostnames.clear(); +} + +/** Remote-mode names: the configured urlHost (auto resolved) and this machine's hostname. */ +function remoteExtraHosts(): string[] { + const hosts = machineHostnames(os.hostname()); + try { + const urlHost = resolveUrlHost(loadConfig()); + const resolved = urlHost && isAutoUrlHost(urlHost) ? resolveAutoHostCached() : urlHost; + if (resolved) hosts.push(resolved); + } catch { + // A broken config never widens the allowlist. + } + return hosts; +} + +export interface RequestHostGuard { + /** A 403 response when the request's Host is not allowed, else null. */ + check(req: Request): Response | null; +} + +/** Build the guard for one server. Env and config are read once, here. */ +export function createRequestHostGuard(): RequestHostGuard { + const remote = isRemoteSession(); + const allowed = parseAllowedHosts(process.env[ALLOWED_HOSTS_ENV]); + // Remote-mode names are resolved on the first request that needs them, so + // loopback and IP-literal requests never read config or ask Tailscale. + let remoteHosts: string[] | null = null; + const allows = (host: string | null): boolean => { + const policy: RequestHostPolicy = { remote, allowed, extraHosts: servedHostnames }; + if (isAllowedRequestHost(host, policy)) return true; + if (!remote) return false; + remoteHosts ??= remoteExtraHosts(); + return isAllowedRequestHost(host, { ...policy, extraHosts: remoteHosts }); + }; + return { + check(req) { + const host = req.headers.get("host"); + if (allows(host)) return null; + return new Response(hostNotAllowedMessage(host), { + status: 403, + headers: { + "Content-Type": "text/plain; charset=utf-8", + "X-Content-Type-Options": "nosniff", + "X-Plannotator-Error": HOST_NOT_ALLOWED_CODE, + }, + }); + }, + }; +} diff --git a/packages/server/request-host.scenarios.ts b/packages/server/request-host.scenarios.ts new file mode 100644 index 0000000000..a5b61b77df --- /dev/null +++ b/packages/server/request-host.scenarios.ts @@ -0,0 +1,219 @@ +/** + * Host-header allowlist (packages/shared/request-host.ts), driven over HTTP + * against real plan, review and annotate servers. Shared by the Bun suite + * (request-host.test.ts) and the Pi mirror + * (apps/pi-extension/server/request-host.test.ts), so both runtimes answer + * the same scenarios. + * + * Requests go through node:http so the Host header is exactly the one under + * test (fetch sets its own). Every test runs under a temp + * PLANNOTATOR_DATA_DIR; nothing touches the real ~/.plannotator. + */ + +import { afterEach, describe, expect, test } from "bun:test"; +import { writeFileSync } from "node:fs"; +import { request } from "node:http"; +import { hostname } from "node:os"; +import { machineHostnames } from "../shared/request-host"; +import { join } from "node:path"; +import { createTestEnvironment } from "../../tests/helpers/environment"; +import { closeServer, occupyConsecutivePorts } from "../../tests/helpers/ports"; + +export interface HostScenarioServer { + url: string; + stop: () => void; +} + +export type HostScenarioKind = "plan" | "review" | "annotate"; + +export type StartHostScenarioServer = ( + kind: HostScenarioKind, + options: { docPath: string; htmlContent: string }, +) => Promise; + +const HTML = "host-scenario-app"; +const EVIL = "evil.example"; + +/** The JSON read each kind's page loads first. */ +const FIRST_READ: Record = { + plan: "/api/plan", + annotate: "/api/plan", + review: "/api/diff", +}; + +/** The decision endpoint each kind takes. */ +const DECISION: Record = { + plan: "/api/approve", + annotate: "/api/feedback", + review: "/api/feedback", +}; + +export function defineRequestHostScenarios(runtime: string, start: StartHostScenarioServer): void { + const environment = createTestEnvironment( + [ + "PLANNOTATOR_PORT", + "PLANNOTATOR_REMOTE", + "PLANNOTATOR_DATA_DIR", + "PLANNOTATOR_BROWSER", + "PLANNOTATOR_AI", + "PLANNOTATOR_URL_HOST", + "PLANNOTATOR_ALLOWED_HOSTS", + "SSH_TTY", + "SSH_CONNECTION", + ], + "plannotator-request-host-", + ); + + let docPath = ""; + + async function isolate(options: { remote?: boolean; allowedHosts?: string } = {}): Promise { + environment.reset(); + const dir = environment.makeTempDir(); + process.env.PLANNOTATOR_DATA_DIR = join(dir, "data"); + process.env.PLANNOTATOR_BROWSER = "/usr/bin/true"; + process.env.PLANNOTATOR_AI = "disabled"; + process.env.PLANNOTATOR_REMOTE = options.remote ? "1" : "0"; + if (options.allowedHosts !== undefined) process.env.PLANNOTATOR_ALLOWED_HOSTS = options.allowedHosts; + if (options.remote) { + // Remote mode would otherwise bind the fixed 19432. + const { start: port, servers } = await occupyConsecutivePorts(1); + await closeServer(servers[0]); + process.env.PLANNOTATOR_PORT = String(port); + } + docPath = join(dir, "notes.md"); + writeFileSync(docPath, "# Notes\n\nsecret body\n"); + } + + afterEach(() => environment.restore()); + + const portOf = (server: HostScenarioServer) => new URL(server.url).port; + const get = (server: HostScenarioServer, path: string, host: string) => + rawRequest(`${loopbackBase(server)}${path}`, { headers: { host } }); + + for (const kind of ["plan", "review", "annotate"] as const) { + describe(`${runtime} ${kind} server: Host allowlist`, () => { + test("loopback Hosts are answered, a foreign name is refused on every route", async () => { + await isolate(); + const server = await start(kind, { docPath, htmlContent: HTML }); + try { + const port = portOf(server); + for (const host of [`localhost:${port}`, `127.0.0.1:${port}`, `[::1]:${port}`]) { + const page = await get(server, "/", host); + expect([host, page.status]).toEqual([host, 200]); + expect(page.body.toString("utf8")).toContain("host-scenario-app"); + const read = await get(server, FIRST_READ[kind], host); + expect([host, read.status]).toEqual([host, 200]); + const externals = await get(server, "/api/external-annotations", host); + expect([host, externals.status]).toEqual([host, 200]); + } + + const evil = `${EVIL}:${port}`; + const refusedReads = ["/", FIRST_READ[kind], "/api/external-annotations", "/api/draft"]; + if (kind !== "review") refusedReads.push(`/api/doc?path=${encodeURIComponent(docPath)}`); + for (const path of refusedReads) { + const res = await get(server, path, evil); + expect([path, res.status]).toEqual([path, 403]); + const body = res.body.toString("utf8"); + expect(body).toContain("PLANNOTATOR_ALLOWED_HOSTS"); + expect(body).not.toContain("secret body"); + expect(body).not.toContain("host-scenario-app"); + } + + // Writes are refused before they reach a handler: no decision, no config write. + const decision = await rawRequestWithBody(server, DECISION[kind], evil, { feedback: "x", annotations: [] }); + expect(decision.status).toBe(403); + const config = await rawRequestWithBody(server, "/api/config", evil, { displayName: "rebound" }); + expect(config.status).toBe(403); + + // A WebSocket upgrade never succeeds: a 403, or (node:http with no + // upgrade listener attached, i.e. no agent terminal) a closed + // socket. The guarded-listener case is request-host-guard.test.ts. + const upgrade = await rawRequest(`${loopbackBase(server)}/api/agent-terminal/pty/x`, { + headers: { host: evil, connection: "Upgrade", upgrade: "websocket", "sec-websocket-version": "13", "sec-websocket-key": "dGhlIHNhbXBsZSBub25jZQ==" }, + }).then((res) => res.status, () => "closed"); + expect([403, "closed"]).toContain(upgrade); + + // The session is still undecided and answers its own page. + const after = await get(server, FIRST_READ[kind], `localhost:${port}`); + expect(after.status).toBe(200); + } finally { + server.stop(); + } + }); + + test("remote mode also answers IP literals, the urlHost and this machine's name, and still refuses a foreign name", async () => { + await isolate({ remote: true }); + process.env.PLANNOTATOR_URL_HOST = "review-box.example"; + const server = await start(kind, { docPath, htmlContent: HTML }); + try { + const port = portOf(server); + expect((await get(server, "/", `review-box.example:${port}`)).status).toBe(200); + const [machine] = machineHostnames(hostname()); + if (machine) expect((await get(server, "/", `${machine}:${port}`)).status).toBe(200); + expect((await get(server, FIRST_READ[kind], `192.168.1.20:${port}`)).status).toBe(200); + expect((await get(server, "/", `[fd00::5]:${port}`)).status).toBe(200); + expect((await get(server, "/", `localhost:${port}`)).status).toBe(200); + expect((await get(server, FIRST_READ[kind], `${EVIL}:${port}`)).status).toBe(403); + } finally { + server.stop(); + } + }); + + test("PLANNOTATOR_ALLOWED_HOSTS admits a forwarded hostname; * turns the check off", async () => { + await isolate({ allowedHosts: "preview.example.com" }); + const server = await start(kind, { docPath, htmlContent: HTML }); + try { + const port = portOf(server); + expect((await get(server, "/", `preview.example.com:${port}`)).status).toBe(200); + expect((await get(server, "/", `${EVIL}:${port}`)).status).toBe(403); + } finally { + server.stop(); + environment.restore(); + } + await isolate({ allowedHosts: "*" }); + const open = await start(kind, { docPath, htmlContent: HTML }); + try { + expect((await get(open, "/", `${EVIL}:${portOf(open)}`)).status).toBe(200); + } finally { + open.stop(); + } + }); + }); + } +} + +function rawRequestWithBody(server: HostScenarioServer, path: string, host: string, body: unknown) { + const text = JSON.stringify(body); + return new Promise<{ status: number }>((resolve, reject) => { + // node:http directly: rawRequest sends no body. + const req = request( + `${loopbackBase(server)}${path}`, + { method: "POST", headers: { host, "content-type": "application/json", "content-length": String(Buffer.byteLength(text)) } }, + (res) => { + res.resume(); + res.on("end", () => resolve({ status: res.statusCode ?? 0 })); + }, + ); + req.on("error", reject); + req.end(text); + }); +} + +/** Connect over loopback whatever URL the session advertises (urlHost). */ +function loopbackBase(server: HostScenarioServer): string { + return `http://127.0.0.1:${new URL(server.url).port}`; +} + +/** A GET (or bodiless request) with exactly these headers; fetch would set its own Host. */ +function rawRequest(url: string, options: { headers: Record }): Promise<{ status: number; body: Buffer }> { + return new Promise((resolve, reject) => { + const req = request(url, { method: "GET", headers: options.headers }, (res) => { + const chunks: Buffer[] = []; + res.on("data", (chunk: Buffer) => chunks.push(chunk)); + res.on("end", () => resolve({ status: res.statusCode ?? 0, body: Buffer.concat(chunks) })); + res.on("error", reject); + }); + req.on("error", reject); + req.end(); + }); +} diff --git a/packages/server/request-host.test.ts b/packages/server/request-host.test.ts new file mode 100644 index 0000000000..211a618ead --- /dev/null +++ b/packages/server/request-host.test.ts @@ -0,0 +1,72 @@ +/** + * Bun servers: the Host-header allowlist. Scenarios are shared with the Pi + * mirror (request-host.scenarios.ts); the goal-setup server and the + * `--tailscale` served-name registration are Bun-only and tested here. + */ +import { afterEach, describe, expect, test } from "bun:test"; +import { normalizeGoalSetupBundle } from "@plannotator/shared/goal-setup"; +import { createTestEnvironment } from "../../tests/helpers/environment"; +import { rawRequest } from "../../tests/helpers/app-html"; +import { defineRequestHostScenarios } from "./request-host.scenarios"; +import { startPlannotatorServer } from "./index"; +import { startAnnotateServer } from "./annotate"; +import { startReviewServer } from "./review"; +import { startGoalSetupServer } from "./goal-setup"; +import { allowServedHostname, resetServedHostnamesForTests } from "./request-host-guard"; + +const PATCH = "diff --git a/a.txt b/a.txt\n--- a/a.txt\n+++ b/a.txt\n@@ -1 +1 @@\n-a\n+b\n"; + +defineRequestHostScenarios("bun", async (kind, { docPath, htmlContent }) => { + if (kind === "plan") return startPlannotatorServer({ plan: "# Plan\n\n[notes](notes.md)", origin: "claude-code", htmlContent }); + if (kind === "review") return startReviewServer({ rawPatch: PATCH, gitRef: "HEAD", htmlContent }); + return startAnnotateServer({ markdown: "# Notes\n\nsecret body\n", filePath: docPath, htmlContent, mode: "annotate" }); +}); + +describe("bun goal-setup server and tailnet names", () => { + const environment = createTestEnvironment( + ["PLANNOTATOR_PORT", "PLANNOTATOR_REMOTE", "PLANNOTATOR_DATA_DIR", "PLANNOTATOR_BROWSER", "PLANNOTATOR_ALLOWED_HOSTS", "SSH_TTY", "SSH_CONNECTION"], + "plannotator-request-host-bun-", + ); + afterEach(() => { + resetServedHostnamesForTests(); + environment.restore(); + }); + + function isolate(): void { + environment.reset(); + process.env.PLANNOTATOR_DATA_DIR = environment.makeTempDir(); + process.env.PLANNOTATOR_BROWSER = "/usr/bin/true"; + process.env.PLANNOTATOR_REMOTE = "0"; + } + + test("goal setup refuses a foreign Host", async () => { + isolate(); + const server = await startGoalSetupServer({ + bundle: normalizeGoalSetupBundle({ stage: "interview", title: "Goal", questions: [{ id: "q", prompt: "Q?" }] }), + htmlContent: "goal", + origin: "claude-code", + }); + try { + const port = new URL(server.url).port; + expect((await rawRequest(`${server.url}/api/plan`, { headers: { host: `localhost:${port}` } })).status).toBe(200); + expect((await rawRequest(`${server.url}/api/plan`, { headers: { host: `evil.example:${port}` } })).status).toBe(403); + } finally { + server.stop(); + } + }); + + test("a --tailscale session answers the serve name it registered (tailscale serve forwards the browser's Host)", async () => { + isolate(); + const server = await startReviewServer({ rawPatch: PATCH, gitRef: "HEAD", htmlContent: "r", tailnetPublished: true }); + try { + const port = new URL(server.url).port; + const served = `devbox.tailnet-1234.ts.net:${port}`; + expect((await rawRequest(`${server.url}/`, { headers: { host: served } })).status).toBe(403); + allowServedHostname("devbox.tailnet-1234.ts.net"); + expect((await rawRequest(`${server.url}/`, { headers: { host: served } })).status).toBe(200); + expect((await rawRequest(`${server.url}/`, { headers: { host: `evil.example:${port}` } })).status).toBe(403); + } finally { + server.stop(); + } + }); +}); diff --git a/packages/server/review.ts b/packages/server/review.ts index d3f1a5f3d8..75c93093f4 100644 --- a/packages/server/review.ts +++ b/packages/server/review.ts @@ -12,6 +12,7 @@ import { isSameOriginOrNoOrigin } from "@plannotator/shared/request-origin"; import { appHtmlResponse, likelyAppHtmlEncoding, prewarmAppHtml } from "@plannotator/shared/app-html"; import { isRemoteSession, getServerHostname, startBunServerOnAvailablePort, buildAdvertisedUrl } from "./remote"; +import { createRequestHostGuard } from "./request-host-guard"; import type { Origin } from "@plannotator/shared/agents"; import { type DiffType, type GitContext, runVcsDiff, getVcsFileContentsForDiff, getVcsFileBytesForDiff, getVcsDiffFingerprint, canStageFiles, stageFile, unstageFile, resolveVcsCwd, validateFilePath, getVcsContext, detectRemoteDefaultCompareTarget, resolveAvailableDiffType, vcsOwnsDiffType, vcsSupportsSnapshot, materializeVcsSnapshot, gitRuntime } from "./vcs"; import { basename } from "node:path"; @@ -2008,6 +2009,7 @@ export async function startReviewServer( }, }; + const requestHostGuard = createRequestHostGuard(); const server = await startBunServerOnAvailablePort((port) => Bun.serve({ hostname: getServerHostname(), @@ -2018,6 +2020,10 @@ export async function startReviewServer( idleTimeout: 0, async fetch(req, server) { + // Host allowlist first, before any route (request-host-guard.ts). + const hostRefusal = requestHostGuard.check(req); + if (hostRefusal) return hostRefusal; + const url = new URL(req.url); const hostControlResponse = handleHostControl(req, url, { diff --git a/packages/server/tailscale-serve.ts b/packages/server/tailscale-serve.ts index 428b10e928..b202278d18 100644 --- a/packages/server/tailscale-serve.ts +++ b/packages/server/tailscale-serve.ts @@ -35,6 +35,7 @@ import { TAILSCALE_SERVE_TIMEOUT_MS, type TailscaleRunner, } from "@plannotator/shared/tailscale"; +import { allowServedHostname, resetServedHostnamesForTests } from "./request-host-guard"; const activePorts = new Set(); let exitCleanupInstalled = false; @@ -122,6 +123,9 @@ export function enableTailscaleServe( `--tailscale: could not find an https:// URL for port ${port} in \`tailscale serve\` output.`, ); } + // tailscale serve forwards the browser's Host (the MagicDNS name) to the + // loopback backend, so the server's Host allowlist must know the name. + allowServedHostname(new URL(url).hostname); activePorts.add(port); cleanupRunner = run; if (!exitCleanupInstalled) { @@ -161,6 +165,7 @@ export function disableTailscaleServe(port: number, run: TailscaleRunner = runTa */ export function resetTailscaleServeForTests(): void { activePorts.clear(); + resetServedHostnamesForTests(); cleanupRunner = runTailscale; if (exitCleanupInstalled) { exitCleanupInstalled = false; diff --git a/packages/shared/package.json b/packages/shared/package.json index 9dd63f0927..69f0bfabd0 100644 --- a/packages/shared/package.json +++ b/packages/shared/package.json @@ -103,6 +103,7 @@ "./live-proxy-core": "./live-proxy-core.ts", "./loopback-host": "./loopback-host.ts", "./request-origin": "./request-origin.ts", + "./request-host": "./request-host.ts", "./host-control": "./host-control.ts", "./live-probe": "./live-probe.ts", "./live-proxy-node": "./live-proxy-node.ts" diff --git a/packages/shared/request-host.test.ts b/packages/shared/request-host.test.ts new file mode 100644 index 0000000000..3b177cdf26 --- /dev/null +++ b/packages/shared/request-host.test.ts @@ -0,0 +1,99 @@ +import { describe, expect, test } from "bun:test"; +import { + hostnameFromHostHeader, + hostNotAllowedMessage, + isAllowedRequestHost, + isIpLiteralHostname, + machineHostnames, + parseAllowedHosts, +} from "./request-host"; + +const local = { remote: false }; +const remote = { remote: true }; + +describe("isAllowedRequestHost", () => { + test("loopback names are allowed in local and remote mode, with or without a port", () => { + for (const host of ["localhost:1234", "LOCALHOST:1234", "localhost", "127.0.0.1:1234", "127.8.9.10:80", "[::1]:1234", "[::1]", "app.localhost:5173", "localhost.:1234"]) { + expect([host, isAllowedRequestHost(host, local)]).toEqual([host, true]); + expect([host, isAllowedRequestHost(host, remote)]).toEqual([host, true]); + } + }); + + test("the port is not compared (forwarded ports arrive with another one)", () => { + expect(isAllowedRequestHost("localhost:9", local)).toBe(true); + expect(isAllowedRequestHost("127.0.0.1:65535", local)).toBe(true); + }); + + test("other names are refused in local mode, including names that only look like loopback", () => { + for (const host of ["evil.example:1234", "evil.example", "127.0.0.1.evil.example:1234", "localhost.evil.example", "192.168.1.20:19432", "[fd7a::1]:19432", "0.0.0.0:1234"]) { + expect([host, isAllowedRequestHost(host, local)]).toEqual([host, false]); + } + }); + + test("remote mode allows IPv4 and IPv6 literals but still refuses DNS names", () => { + expect(isAllowedRequestHost("192.168.1.20:19432", remote)).toBe(true); + expect(isAllowedRequestHost("10.0.0.5", remote)).toBe(true); + expect(isAllowedRequestHost("[fd7a:115c:a1e0::1]:19432", remote)).toBe(true); + expect(isAllowedRequestHost("evil.example:19432", remote)).toBe(false); + expect(isAllowedRequestHost("999.1.1.1:19432", remote)).toBe(false); + }); + + test("extra hosts (urlHost, machine hostname, tailscale serve name) are allowed by exact name", () => { + const policy = { remote: true, extraHosts: ["devbox.tailnet.ts.net", "Devbox"] }; + expect(isAllowedRequestHost("devbox.tailnet.ts.net:19432", policy)).toBe(true); + expect(isAllowedRequestHost("DEVBOX:19432", policy)).toBe(true); + expect(isAllowedRequestHost("x.devbox.tailnet.ts.net:19432", policy)).toBe(false); + // A local --tailscale session: only loopback plus the served name. + expect(isAllowedRequestHost("devbox.tailnet.ts.net:4443", { remote: false, extraHosts: ["devbox.tailnet.ts.net"] })).toBe(true); + }); + + test("a missing Host is allowed (non-browser client); a malformed one is refused", () => { + expect(isAllowedRequestHost(undefined, local)).toBe(true); + expect(isAllowedRequestHost(null, local)).toBe(true); + expect(isAllowedRequestHost("", local)).toBe(true); + for (const host of ["localhost:abc", "localhost:99999", "user@localhost", "[::1", "::1", "localhost:1:2", "[localhost]:1", " :1234", "local host"]) { + expect([host, isAllowedRequestHost(host, local)]).toEqual([host, false]); + } + }); + + test("PLANNOTATOR_ALLOWED_HOSTS adds exact names and dotted suffixes; * turns the check off", () => { + const allowed = parseAllowedHosts(" Preview.Example.com , .internal.test,,"); + expect(allowed).toEqual({ any: false, names: ["preview.example.com", ".internal.test"] }); + const policy = { remote: false, allowed }; + expect(isAllowedRequestHost("preview.example.com:443", policy)).toBe(true); + expect(isAllowedRequestHost("a.internal.test", policy)).toBe(true); + expect(isAllowedRequestHost("internal.test", policy)).toBe(true); + expect(isAllowedRequestHost("evilinternal.test", policy)).toBe(false); + expect(isAllowedRequestHost("evil.example", policy)).toBe(false); + expect(isAllowedRequestHost("evil.example", { remote: false, allowed: parseAllowedHosts("*") })).toBe(true); + expect(parseAllowedHosts(undefined)).toEqual({ any: false, names: [] }); + }); +}); + +describe("helpers", () => { + test("hostname extraction", () => { + expect(hostnameFromHostHeader("Example.COM.:80")).toBe("example.com"); + expect(hostnameFromHostHeader("[::1]:1")).toBe("::1"); + expect(hostnameFromHostHeader(undefined)).toBeUndefined(); + expect(hostnameFromHostHeader("bad host")).toBeNull(); + }); + + test("IP literal detection", () => { + expect(isIpLiteralHostname("192.168.0.1")).toBe(true); + expect(isIpLiteralHostname("fd7a::1")).toBe(true); + expect(isIpLiteralHostname("256.0.0.1")).toBe(false); + expect(isIpLiteralHostname("1.2.3.4.example")).toBe(false); + }); + + test("machine hostnames: full name and first label", () => { + expect(machineHostnames("DevBox.local")).toEqual(["devbox.local", "devbox"]); + expect(machineHostnames("devbox")).toEqual(["devbox"]); + expect(machineHostnames("")).toEqual([]); + }); + + test("the refusal names the env var and never echoes a malformed header", () => { + expect(hostNotAllowedMessage("evil.example:1")).toContain("PLANNOTATOR_ALLOWED_HOSTS"); + expect(hostNotAllowedMessage("evil.example:1")).toContain('"evil.example"'); + expect(hostNotAllowedMessage("