fix: a page you visit should not be able to type into your agents

docs/decisions.md

@@ -290,3 +290,90 @@ session does not silently re-litigate them.
 
     Dismissal is keyed by **version**, not a boolean. A boolean would make the
     first dismissal permanent and the feature would quietly stop existing.
+
+17. **A same-origin gate on every write and on the `/ws` upgrade.** This is the
+    fix decision 12 said it was not. That decision required
+    `content-type: application/json` on the three settings routes, restoring the
+    CORS preflight, and scaled itself honestly: "the pre-existing action routes
+    are POST already and carry larger levers, so this is not new in kind. It is
+    a floor, not a fix."
+
+    Two holes were open, and they composed into one attack. `/ws` upgraded
+    unconditionally — a WebSocket handshake is exempt from CORS entirely, so no
+    preflight and no browser rule stood in the way — while `hubWebSocket.open`
+    sends the whole snapshot on connect, so any page the operator visited could
+    read every agent's name, id and screen from `127.0.0.1`. And
+    `POST /api/agents/:id/text` types arbitrary text into a live coding agent
+    while reading its body with `jsonBody`, which never inspects the content
+    type: an `enctype="text/plain"` form posts syntactically valid JSON to it as
+    a CORS-*simple* request, no preflight, no same-origin check. Read the ids off
+    the socket, then type into the agent. On the loopback listener there is not
+    even an Access session to borrow — decision 3 gives that port no
+    authentication at all, which is correct for an operator on their own machine
+    and no defence whatsoever against their own browser.
+
+    `src/server/origin.ts` holds the rule as pure predicates, called from exactly
+    two enforcement points: one Hono middleware in `routes.ts` that both
+    listeners inherit with the app, and the `/ws` interception in `ws/serve.ts`,
+    which is already the single shared definition of that upgrade. Neither is a
+    per-route check, because the guard belongs to the **verb**: a future write
+    route must be covered by existing, not by remembering to opt in.
+
+    The allowlist itself is ONE thunk, built in `index.ts` and handed to every
+    consumer — both apps' middleware and both listeners' upgrade — rather than
+    derived where it is used. That is not tidiness: the first version derived it
+    twice, once in the middleware from `settings`/`tunnelUrl` and once at the
+    call site, and the gated listener's writes and its WebSocket upgrades
+    consequently answered to DIFFERENT allowlists. A gate with a seam in it is
+    the failure this repo keeps rediscovering, and `tests/origin-tunnel.test.ts`
+    is what found this instance.
+
+    Three asymmetries are deliberate, and each one is load-bearing.
+
+    **GET and HEAD are not guarded.** Browsers omit `Origin` on same-origin
+    GETs, so a guard there would have to accept a missing one and would gate
+    nothing; a cross-origin GET cannot read the response anyway, since paddock
+    sends no CORS headers. What guarding reads *would* achieve is breaking
+    `/sw.js` and the app shell — decision 3's exact failure reached from a new
+    direction.
+
+    **A missing `Origin` passes a write but fails an upgrade.** Browsers always
+    send it on a POST, so its absence means a non-browser caller, which carries
+    no hostile page to act for; refusing would break every command-line use and
+    buy nothing. Browsers also always send it on a WebSocket handshake —
+    same-origin included, unlike a GET — so requiring it there costs a browser
+    nothing and shuts out a non-browser reader. That is worth having, because
+    herdr's control socket is a FILE whose permissions keep other local users
+    out, while paddock's port is TCP and every uid on the host can reach it.
+
+    **The host allowlist is opportunistic, and empty means inactive.** Writes
+    require `Origin` to equal `Host`, which closes ordinary cross-site CSRF with
+    no configuration at all. It cannot close DNS rebinding, where the browser is
+    tricked into resolving `evil.example` to `127.0.0.1` so that `Host` and
+    `Origin` *agree*. Catching that needs to know the deployment's real
+    hostname, and paddock already does — `settings.publicUrl` plus a live
+    `paddock tunnel` URL — so `publicHostsFrom` derives the allowlist from those
+    and needs no new setting. But it is enforced only when non-empty. Making it
+    unconditional would mean a named-tunnel operator who never set `publicUrl`
+    loses the reply path, and `publicUrl` lives in the **Notifications**
+    section: a Telegram convenience would silently become the difference between
+    a working dashboard and a read-only one, for a reason no operator could
+    guess. So knowing a public hostname buys rebinding protection on top;
+    not knowing one costs nothing that already worked. Loopback is always
+    allowed even against a populated list, or setting a public URL would break
+    desk browsing and `make dev`.
+
+    **This is not authentication and decision 3 still stands.** Nothing here
+    identifies anybody. It asks the one question a browser cannot lie about —
+    which page this request acts for — and no token is minted, held or checked.
+
+    A refusal is reported on stderr, once per distinct `origin -> host` pair and
+    at most 20 pairs, because the likeliest cause is not an attack but a
+    misconfiguration. A dashboard that quietly stopped accepting replies would be
+    unexplainable; unbounded logging from a caller who chooses the origin would
+    be a flood. The message names the REMEDY, and `refusalReason` picks it,
+    because the two causes need opposite fixes: `Origin` and `Host` disagreeing
+    means a proxy is rewriting `Host`, while the pair AGREEING and being refused
+    anyway means `publicUrl` names a hostname this deployment is not reached on.
+    Telling the second operator to check their proxy would send them to a file
+    that is already correct.

docs/gotchas.md

@@ -310,6 +310,34 @@ one, recorded here so they are not reintroduced.
   notification was worth sending. A longer Access session duration lowers the
   frequency and removes nothing.
 
+- **A proxy that rewrites `Host` breaks every write, and the browser reports
+  nothing useful.** Since decision 17, a state-changing request must carry an
+  `Origin` matching the request's `Host`. `cloudflared`'s ordinary HTTP ingress
+  forwards the browser's `Host` unchanged, which is what `docs/deploy-cloudflare.md`
+  describes and what makes the check hold — but a proxy configured to override
+  it (cloudflared's own `httpHostHeader`, or an nginx `proxy_set_header Host`
+  pointing at `localhost`) makes `Host` disagree with `Origin` on every request,
+  and paddock refuses all of them with a `403`. From the phone this reads as
+  replies silently failing while the dashboard still updates, because reads are
+  deliberately ungated. **The tell is on the host's stderr:** paddock logs
+  `refused a cross-origin write` once per distinct `origin -> host` pair, naming
+  both, precisely so this is diagnosable in seconds rather than guessed at. The
+  fix is to forward `Host` unchanged; adding the rewritten value to
+  `settings.publicUrl` would NOT help, because the mismatch is between the two
+  headers and not with any allowlist.
+
+- **A `publicUrl` naming a hostname you do not actually reach paddock on
+  refuses every write.** The other half of the entry above, and the opposite
+  fix. Once `settings.publicUrl` is set, its host becomes an allowlist (that is
+  what buys DNS-rebinding cover), so a stale value, a typo, or a SECOND
+  legitimate hostname for the same paddock is refused with a `403` even though
+  `Origin` and `Host` agree. Loopback is always exempt, so it fails from the
+  phone while the desk keeps working — which reads as "the tunnel broke". The
+  stderr line distinguishes it from a rewriting proxy: `this host is not the
+  public URL saved in settings`. Fix by correcting `publicUrl` to the hostname
+  in the browser's address bar, or by clearing it — clearing costs only the
+  rebinding cover and the Telegram deep link, never the reply path.
+
 - **A verification request must come from a context holding no Access
   session.** An already-authenticated browser renders the dashboard whether or
   not the policy is correct, so it cannot tell a working gate from a missing

src/server/index.ts

@@ -17,6 +17,7 @@ import { Supervisor } from "@server/supervisor";
 import { shapeMessage, shapeSummary } from "@server/herdr/shape";
 import { Hub } from "@server/ws/hub";
 import { hubWebSocket, tryUpgradeWs, type WsData } from "@server/ws/serve";
+import { publicHostsFrom } from "@server/origin";
 import { buildIdFrom } from "@server/build-id";
 import { SettingsStore, defaultConfigDir, isConfigured } from "@server/settings/store";
 import { recordState, removeState } from "@server/lifecycle/state";
@@ -496,8 +497,22 @@ if (DEMO) {
 // Named rather than inline so `paddock tunnel` can build a SECOND app from the
 // same dependencies plus the pairing gate — one description of the app, not two
 // that could drift.
+/**
+ * The public hostnames this process answers on, for the same-origin gate.
+ *
+ * ONE definition, handed to every consumer: both apps' write middleware and
+ * both listeners' `/ws` upgrade. They must never be able to disagree about
+ * which origins are legitimate — a write accepted from an origin the socket
+ * refuses (or the reverse) is a gate with a seam in it.
+ *
+ * Empty until an operator saves a `publicUrl` or a tunnel run sets one, which
+ * is `origin.ts`'s documented inactive case rather than a weakening.
+ */
+const publicHosts = () => publicHostsFrom(settings.current().publicUrl, tunnelUrl);
+
 const appDeps = {
   store,
+  publicHosts,
   hub,
   actions,
   settings,
@@ -533,7 +548,11 @@ try {
       // handlers below are shared with the tunnel's gated listener rather than
       // written out here — `ws/serve.ts` says why. `null` means this request
       // is not the socket route, so it belongs to the app.
-      const ws = tryUpgradeWs(req, server);
+      // The hostnames a `/ws` upgrade may claim to come from. Read through a
+      // thunk rather than captured: `publicUrl` is editable from the settings UI
+      // while the process runs, and `tunnelUrl` is set mid-run by `paddock
+      // tunnel`.
+      const ws = tryUpgradeWs(req, server, publicHosts());
       if (ws !== null) return ws;
       return app.fetch(req);
     },
@@ -724,6 +743,7 @@ if (command === "tunnel") {
       bin: tunnelBin,
       deadlineMs: tunnelDeadlineMs,
       setPublicUrl: (u) => { tunnelUrl = u; },
+      publicHosts,
       registerShutdown: (fn) => { onShutdown = fn; },
     });
   } catch (err) {

src/server/origin.ts

@@ -0,0 +1,219 @@
+/**
+ * The same-origin gate: two predicates and the allowlist they consult.
+ *
+ * WHY THIS EXISTS. `docs/decisions.md` decision 12 required
+ * `content-type: application/json` on the three settings writes, restoring the
+ * CORS preflight that is paddock's only CSRF control — and said plainly what it
+ * did not cover: "the pre-existing action routes are POST already and carry
+ * larger levers, so this is not new in kind. It is a floor, not a fix."
+ *
+ * This is the fix. Two holes were open. `POST /api/agents/:id/text` types
+ * arbitrary text into a live coding agent and reads its body with `jsonBody`,
+ * which never inspects the content type, so it was a CORS-SIMPLE request: an
+ * `enctype="text/plain"` form on any page the operator visited could post
+ * syntactically valid JSON to it with no preflight and no same-origin check.
+ * And `/ws` upgraded unconditionally — a WebSocket handshake is exempt from
+ * CORS entirely — while `hubWebSocket.open` sends the full snapshot on connect,
+ * so the same hostile page could read every agent's name, id and screen and
+ * then use those ids against the write route.
+ *
+ * WHY IT IS NOT AN AUTH TOKEN. Decision 3 stands: an application token would
+ * gate `/sw.js` and silently kill the service worker. Nothing here authenticates
+ * anybody. It asks one question a browser cannot lie about — which page is this
+ * request acting for — and refuses when the answer is "not paddock's own".
+ *
+ * PURE ON PURPOSE. No imports, no clock, no settings, no transport. Both
+ * enforcement points (`routes.ts`'s middleware and `ws/serve.ts`'s upgrade) call
+ * these, and `tests/origin.test.ts` calls them directly without booting
+ * `Bun.serve` — the shape `docs/roadmap.md` asks for where composition is
+ * otherwise untestable.
+ */
+
+/** The three spellings of loopback, as a `Host` header carries them. */
+const LOOPBACK_HOSTNAMES: readonly string[] = ["127.0.0.1", "localhost", "[::1]", "::1"];
+
+/**
+ * The hostname of a `Host` header value, port stripped, lowercased — or null if
+ * it does not parse.
+ *
+ * Parsed with `URL` rather than split on `:`, because `[::1]:8787` has two of
+ * those and a hand-rolled split gets it wrong. The `http://` prefix is a parsing
+ * scaffold and says nothing about the real scheme.
+ */
+function hostnameOf(host: string): string | null {
+  try {
+    const h = new URL(`http://${host}`).hostname;
+    return h === "" ? null : h.toLowerCase();
+  } catch {
+    return null;
+  }
+}
+
+/**
+ * The host this request was addressed to.
+ *
+ * The `Host` HEADER first, because that is what a browser sent and what a proxy
+ * may have rewritten — the value the check is actually about. The URL's host is
+ * the fallback, and in production the two are the same value: Bun builds
+ * `req.url` FROM the `Host` header, so neither can disagree with the other on a
+ * real request. The fallback exists because a `Request` constructed in a test
+ * carries no `Host` header at all — the header is added by the transport — and a
+ * predicate that only worked over a live socket could not be tested at the one
+ * layer worth testing it at.
+ */
+export function hostOf(req: Request): string {
+  const header = req.headers.get("host");
+  if (header !== null && header !== "") return header;
+  try {
+    return new URL(req.url).host;
+  } catch {
+    return "";
+  }
+}
+
+/**
+ * Whether a `Host` header names this machine.
+ *
+ * Matched on the parsed hostname, never on a prefix: `127.0.0.1.evil.com` and
+ * `localhost.evil.com` are ordinary registrable names that resolve wherever
+ * their owner points them, and a `startsWith` check would hand both of them the
+ * allowlist bypass below.
+ */
+export function isLoopbackHost(host: string): boolean {
+  const name = hostnameOf(host);
+  return name !== null && LOOPBACK_HOSTNAMES.includes(name);
+}
+
+/** The `host` of an origin string (`https://h:port` -> `h:port`), or null. */
+function hostOfOrigin(origin: string): string | null {
+  let url: URL;
+  try {
+    url = new URL(origin);
+  } catch {
+    // Includes the literal "null" a sandboxed iframe or a `data:` document
+    // sends — no scheme, so it parses as a URL nowhere.
+    return null;
+  }
+  if (url.protocol !== "http:" && url.protocol !== "https:") return null;
+  return url.host === "" ? null : url.host.toLowerCase();
+}
+
+/**
+ * The hostnames this deployment is legitimately reached on, or **empty when
+ * none is known**.
+ *
+ * Empty means INACTIVE, not "deny everything", and that distinction is the
+ * whole reason `publicUrl` can stay optional. It lives in the Notifications
+ * section — `docs/settings.md` calls it "what turns a bare 'docs-cleanup is
+ * blocked' into a tap-through link" — so an operator on a named tunnel who does
+ * not use Telegram has never had a reason to set it. Enforcing an allowlist
+ * built from it unconditionally would make a notification setting the
+ * difference between a working dashboard and a read-only one, for a reason no
+ * operator could guess. That is the failure mode `CLAUDE.md` bans, so:
+ * knowing a public hostname buys DNS-rebinding protection on top of the
+ * same-origin check; not knowing one costs nothing that was already working.
+ *
+ * A live tunnel URL is included as well as the saved one. A `paddock tunnel`
+ * run's hostname differs on every run and is deliberately never written to
+ * `settings.json` (see `docs/settings.md`), so the live value is the only place
+ * it exists.
+ */
+export function publicHostsFrom(
+  publicUrl: string | null,
+  tunnelUrl: string | null,
+): readonly string[] {
+  const hosts: string[] = [];
+  for (const candidate of [publicUrl, tunnelUrl]) {
+    if (candidate === null || candidate.trim() === "") continue;
+    // An unparseable value is no knowledge at all, not a lockout: `publicUrl`
+    // is a free-text field, and a typo in it must never populate the allowlist
+    // with garbage that then refuses every write.
+    const host = hostOfOrigin(candidate.trim());
+    if (host !== null && !hosts.includes(host)) hosts.push(host);
+  }
+  return hosts;
+}
+
+/**
+ * Whether the request's `Host` is one this deployment answers on.
+ *
+ * Checked BEFORE the origin comparison and fail-closed, because it is the only
+ * one of the two that can see a rebinding attack: a browser tricked into
+ * resolving `evil.example` to `127.0.0.1` sends `Host` and `Origin` that AGREE,
+ * so the comparison below passes on its own.
+ */
+function hostAllowed(host: string, publicHosts: readonly string[]): boolean {
+  if (publicHosts.length === 0) return true; // inactive — see publicHostsFrom
+  if (isLoopbackHost(host)) return true; // the desk, and `make dev`
+  return publicHosts.includes(host.toLowerCase());
+}
+
+function sameOrigin(origin: string, host: string, publicHosts: readonly string[]): boolean {
+  if (!hostAllowed(host, publicHosts)) return false;
+  const from = hostOfOrigin(origin);
+  return from !== null && from === host.toLowerCase();
+}
+
+/**
+ * Why a request was refused — because the two causes need OPPOSITE fixes, and
+ * naming the wrong one sends an operator to the wrong file.
+ *
+ * `cross-origin`: `Origin` and `Host` disagree. Either a hostile page, or a
+ * proxy in front rewriting `Host` (cloudflared's `httpHostHeader`, nginx's
+ * `proxy_set_header Host`). The fix is in the proxy.
+ *
+ * `host-not-allowed`: they AGREE and the allowlist refused anyway, so this
+ * deployment is being reached on a hostname `publicUrl` does not name. The fix
+ * is in settings — and telling this operator to look at their proxy would be
+ * actively misleading, since their proxy is behaving correctly.
+ */
+export type RefusalReason = "no-origin" | "cross-origin" | "host-not-allowed";
+
+export function refusalReason(
+  origin: string | null,
+  host: string,
+  publicHosts: readonly string[],
+): RefusalReason {
+  if (origin === null) return "no-origin";
+  if (!hostAllowed(host, publicHosts)) return "host-not-allowed";
+  return "cross-origin";
+}
+
+/**
+ * Whether a state-changing request may proceed.
+ *
+ * A MISSING `Origin` is allowed, and that is not an oversight. Browsers always
+ * send it on a POST, so its absence means the caller is not a browser — curl, a
+ * script, a health check — and a non-browser caller carries no hostile page on
+ * whose behalf it could be acting. Refusing here would buy nothing and break
+ * every command-line use of the API.
+ */
+export function allowWrite(
+  origin: string | null,
+  host: string,
+  publicHosts: readonly string[],
+): boolean {
+  if (origin === null) return true;
+  return sameOrigin(origin, host, publicHosts);
+}
+
+/**
+ * Whether a `/ws` upgrade may proceed.
+ *
+ * Identical to `allowWrite` except that a missing `Origin` is REFUSED, which is
+ * the one asymmetry worth stating twice. Browsers send `Origin` on every
+ * WebSocket handshake, same-origin included — unlike a GET, where they omit it —
+ * so requiring it costs a browser nothing and shuts out a non-browser reader.
+ * That is worth having: herdr's control socket is a FILE, so its permissions
+ * keep other local users out, while paddock's port is TCP and every uid on the
+ * host can reach it. `/ws` sends the whole snapshot on connect, so it is the
+ * read worth closing.
+ */
+export function allowUpgrade(
+  origin: string | null,
+  host: string,
+  publicHosts: readonly string[],
+): boolean {
+  if (origin === null) return false;
+  return sameOrigin(origin, host, publicHosts);
+}