From 139b1f60984b28d4e53de5292e666d502e1f1ada Mon Sep 17 00:00:00 2001 From: lntvan166 Date: Thu, 20 Aug 2026 20:05:23 +0700 Subject: [PATCH] fix: a page you visit should not be able to type into your agents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes that composed into one attack, and decision 12 predicted both. That decision required application/json on the three settings writes 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." /ws upgraded unconditionally. A WebSocket handshake is exempt from CORS entirely, so no preflight and no browser rule stood in the way, and hubWebSocket.open sends the whole snapshot on connect — so any page the operator visited could open ws://127.0.0.1:8787/ws and read every agent's name, id and screen. Then POST /api/agents/:id/text types arbitrary text into a live coding agent and reads its body with jsonBody, which never looks at the content type: an enctype="text/plain" form posts syntactically valid JSON to it as a CORS-simple request. Read the ids off the socket, type into the agent. And on the loopback listener there is no Access session to borrow — decision 3 gives that port no authentication at all, which is right for an operator on their own machine and no defence whatsoever against their own browser. origin.ts holds the rule as pure predicates. Two enforcement points call them: one middleware that both listeners inherit with the app, and the /ws interception that ws/serve.ts exists so there is only one of. Not per-route, because the guard belongs to the VERB — a future write route is covered by existing rather than by remembering to opt in. The allowlist is ONE thunk built in index.ts and handed to every consumer, after the first version derived it twice and left the gated listener's writes and its upgrades answering to different allowlists. Three asymmetries, each load-bearing. GET is unguarded: browsers omit Origin on same-origin GETs so a guard there would gate nothing, a cross-origin GET cannot read the response anyway, and guarding reads is how /sw.js breaks instead — the decision 3 failure from a new direction. A missing Origin passes a write and fails an upgrade: browsers always send it on a POST, so its absence is curl, and they always send it on a handshake, so requiring it there shuts out websocat — which matters because herdr's socket is a FILE with permissions and paddock's port is TCP that every uid on the host can reach. And the host allowlist is opportunistic: Origin == Host closes ordinary CSRF with no config, rebinding needs the real hostname, paddock already knows it from settings.publicUrl and the live tunnel URL, but it is enforced only when non-empty — publicUrl lives in the Notifications section, and making a Telegram convenience the difference between a working dashboard and a read-only one is the failure CLAUDE.md bans. Not authentication. Nothing here identifies anybody, no token is minted or held, and decision 3 stands. A refusal logs once per distinct origin -> host pair, capped, and names the remedy: a mismatched Origin/Host means a proxy rewriting Host, an agreeing pair refused anyway means publicUrl is wrong. Opposite fixes, so refusalReason picks between them rather than guessing. Verified beyond the suite. Against a live listener: cross-origin write 403, same-origin write through, no-Origin write through, foreign-Origin read 200, upgrade without Origin 403, with a foreign Origin 403, same-origin 101. Through `make dev`'s vite proxy, which forwards Host unchanged: writes through, WS 101, zero refusals. And origin-tunnel.test.ts drives the real gated listener over a RAW SOCKET for the three deployment shapes — desk, quick tunnel, named tunnel — because fetch silently drops a Host header, which had every tunnel case passing as the desk case. 903 tests pass. Co-Authored-By: Claude Opus 5 --- docs/decisions.md | 87 ++++++++++++++ docs/gotchas.md | 28 +++++ src/server/index.ts | 22 +++- src/server/origin.ts | 219 ++++++++++++++++++++++++++++++++++++ src/server/routes.ts | 84 ++++++++++++++ src/server/tunnel/run.ts | 13 ++- src/server/ws/serve.ts | 21 +++- tests/origin-gate.test.ts | 176 +++++++++++++++++++++++++++++ tests/origin-tunnel.test.ts | 209 ++++++++++++++++++++++++++++++++++ tests/origin.test.ts | 195 ++++++++++++++++++++++++++++++++ 10 files changed, 1051 insertions(+), 3 deletions(-) create mode 100644 src/server/origin.ts create mode 100644 tests/origin-gate.test.ts create mode 100644 tests/origin-tunnel.test.ts create mode 100644 tests/origin.test.ts diff --git a/docs/decisions.md b/docs/decisions.md index 1756757..09d80fc 100644 --- a/docs/decisions.md +++ b/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. diff --git a/docs/gotchas.md b/docs/gotchas.md index 5a6c27d..253dd3e 100644 --- a/docs/gotchas.md +++ b/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 diff --git a/src/server/index.ts b/src/server/index.ts index d0924dd..447e465 100644 --- a/src/server/index.ts +++ b/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) { diff --git a/src/server/origin.ts b/src/server/origin.ts new file mode 100644 index 0000000..802fa14 --- /dev/null +++ b/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); +} diff --git a/src/server/routes.ts b/src/server/routes.ts index a5c3152..08947ac 100644 --- a/src/server/routes.ts +++ b/src/server/routes.ts @@ -16,6 +16,8 @@ import type { Hub } from "@server/ws/hub"; import { formatCode } from "@server/tunnel/pairing"; import { setCookie } from "@server/tunnel/gate"; import { EMBEDDED } from "@server/embedded"; +import { allowWrite, hostOf, refusalReason } from "@server/origin"; +import { warn } from "@server/term"; import { isNavKey, type NotifyTrigger, type SettingsPatch } from "@shared/types"; import { diffScreens, digestOf } from "@shared/screen"; @@ -161,6 +163,36 @@ function pruneScreens(liveIds: Set): void { for (const id of [...recentScreens.keys()]) if (!liveIds.has(id)) recentScreens.delete(id); } +/** + * How many distinct `origin -> host` pairs a refusal will be reported for. + * + * Bounded because the caller is hostile by definition: a page that varies its + * origin could otherwise flood the operator's terminal, and the Set would grow + * without limit. Bounded rather than silent because the most likely cause of a + * refusal is NOT an attack — it is a proxy that rewrites `Host` so it no longer + * matches the browser's `Origin`, and a dashboard that stopped accepting + * replies with nothing on stderr would be unexplainable. `docs/gotchas.md` + * records exactly that class of failure. + */ +const REFUSAL_LOG_LIMIT = 20; +const refusalsSeen = new Set(); + +function reportRefusal(origin: string | null, host: string, hosts: readonly string[]): void { + const key = `${origin ?? "(none)"} -> ${host}`; + if (refusalsSeen.has(key) || refusalsSeen.size >= REFUSAL_LOG_LIMIT) return; + refusalsSeen.add(key); + + warn(`paddock: refused a write — origin ${origin ?? "(none)"}, host \`${host}\``); + // The remedy, not just the fact. These two causes need opposite fixes, and + // sending an operator to their proxy config when their proxy is correct is + // worse than saying nothing. + if (refusalReason(origin, host, hosts) === "host-not-allowed") { + warn(" this host is not the public URL saved in settings — correct it, or clear it"); + } else { + warn(" if this is your own dashboard, the proxy in front is rewriting `Host`"); + } +} + /** * A request body as an object, whatever the client actually sent. * @@ -371,6 +403,13 @@ export interface AppDeps { }; /** The live tunnel URL, for the settings view. */ tunnelUrl?: () => string | null; + /** + * The hostnames this deployment is legitimately reached on, for the + * same-origin gate — see `origin.ts`. Omitted in tests and in any caller with + * no public hostname, which is the documented INACTIVE case: the origin/Host + * comparison still applies, only DNS-rebinding cover is absent. + */ + publicHosts?: () => readonly string[]; } export function createApp(deps: AppDeps) { @@ -392,6 +431,51 @@ export function createApp(deps: AppDeps) { */ app.use("*", compress()); + /** + * The same-origin gate on every state-changing request — the other half of + * decision 12, which restored the CORS preflight for the three settings + * routes and said what it did not cover: "the pre-existing action routes are + * POST already and carry larger levers ... It is a floor, not a fix." + * + * ONE middleware rather than a check per route, because the guard belongs to + * the VERB, not to a handler's dependencies: `/ack` is registered even in demo + * mode, `/text` only when `actions` is present, and a future write route must + * be covered by existing to be a write rather than by remembering to opt in. + * Both listeners share this app, so this covers the desk's 8787 and the + * tunnel's gated listener at once. + * + * GET and HEAD are deliberately NOT guarded. Browsers omit `Origin` on + * same-origin GETs, so a guard there would have to accept a missing one and + * would gate nothing; meanwhile a cross-origin GET cannot read the response, + * since paddock sends no CORS headers. What guarding reads WOULD achieve is + * breaking `/sw.js` and the app shell — decision 3's exact failure, arrived at + * from a new direction. `/ws` is not a route on this app at all: it is + * intercepted before `app.fetch`, and `ws/serve.ts` guards it there. + * + * This is still not authentication. Nothing here identifies anybody; it asks + * the one question a browser cannot lie about — which page this request acts + * for — so decision 3 stands untouched. + */ + app.use("*", async (c, next) => { + const method = c.req.method; + if (method === "GET" || method === "HEAD") return next(); + + const host = hostOf(c.req.raw); + const origin = c.req.header("origin") ?? null; + // Read through the thunk on every request, never cached: `publicUrl` is + // editable from the settings UI while the process runs, and a tunnel URL + // appears mid-run. Taken as a DEPENDENCY rather than derived here from + // `settings` and `tunnelUrl`, because `ws/serve.ts` needs the same list and + // two derivations of one fact is how they come to disagree — which is not + // hypothetical: the first version of this middleware derived its own, and + // the gated listener's HTTP writes and its WebSocket upgrades then answered + // to different allowlists. + if (allowWrite(origin, host, deps.publicHosts?.() ?? [])) return next(); + + reportRefusal(origin, host, deps.publicHosts?.() ?? []); + return c.json({ ok: false, detail: "cross-origin rejected" }, 403); + }); + // No authentication middleware. Cloudflare Access is the only gate — see // docs/decisions.md before adding one. app.get("/api/health", (c) => c.json(deps.health())); diff --git a/src/server/tunnel/run.ts b/src/server/tunnel/run.ts index 15f6e37..e12b5ef 100644 --- a/src/server/tunnel/run.ts +++ b/src/server/tunnel/run.ts @@ -35,6 +35,17 @@ export interface TunnelDeps { deadlineMs?: number | null; startTunnel?: typeof realStartTunnel; setPublicUrl?: (url: string | null) => void; + /** + * The origins a `/ws` upgrade on THIS listener may claim, for the same-origin + * gate in `ws/serve.ts`. A thunk because a tunnel run learns its own hostname + * partway through starting up. + * + * Optional, defaulting to none: empty is `origin.ts`'s documented "no public + * hostname is known" case, under which the origin/Host comparison still + * applies. A test that omits it is therefore testing the same rule an + * operator without a saved `publicUrl` runs under, not a relaxed one. + */ + publicHosts?: () => readonly string[]; /** * The environment and the terminal, injected for the same reason every clock * in this codebase is. Read from the real ones in production; a test that @@ -85,7 +96,7 @@ export function serveGated(deps: TunnelDeps): { port: number; stop(): void } { // Past the gate, this listener serves EXACTLY what the plain one serves, // from one definition — see `ws/serve.ts`. `null` means the request is // not the socket route and belongs to the app. - const ws = tryUpgradeWs(req, srv); + const ws = tryUpgradeWs(req, srv, deps.publicHosts?.() ?? []); if (ws !== null) return ws; return deps.app.fetch(req); }, diff --git a/src/server/ws/serve.ts b/src/server/ws/serve.ts index d92cc10..ebb3bc3 100644 --- a/src/server/ws/serve.ts +++ b/src/server/ws/serve.ts @@ -1,6 +1,7 @@ import type { Server, WebSocketHandler } from "bun"; import type { AgentStore } from "@server/state/store"; import type { Hub, HubClient } from "@server/ws/hub"; +import { allowUpgrade, hostOf } from "@server/origin"; /** * What a socket carries: the hub client it was added as, so `close` can remove @@ -30,9 +31,27 @@ export interface HubSocketDeps { * - `null` — not this route; the caller falls through to `app.fetch`. * - `undefined` — upgraded. Bun's own signal that the response IS the upgrade. * - a `Response` — the upgrade was refused. + * + * The ORIGIN CHECK is here, before `srv.upgrade`, and it has to be: a WebSocket + * handshake is exempt from CORS entirely, so no preflight and no browser rule + * stopped a hostile page opening this socket — and `open` below sends the whole + * snapshot, so a refusal that arrived any later would have already disclosed + * every agent's name, id and screen. `allowUpgrade` refuses a MISSING `Origin` + * as well as a mismatched one; `origin.ts` says why that is right here and wrong + * for a write. + * + * `publicHosts` defaults to empty, which is the correct value for "no public + * hostname is known" rather than a weakening — see `publicHostsFrom`. */ -export function tryUpgradeWs(req: Request, srv: Server): Response | undefined | null { +export function tryUpgradeWs( + req: Request, + srv: Server, + publicHosts: readonly string[] = [], +): Response | undefined | null { if (new URL(req.url).pathname !== "/ws") return null; + if (!allowUpgrade(req.headers.get("origin"), hostOf(req), publicHosts)) { + return new Response("cross-origin rejected", { status: 403 }); + } const upgraded = srv.upgrade(req, { data: {} }); return upgraded ? undefined : new Response("upgrade failed", { status: 400 }); } diff --git a/tests/origin-gate.test.ts b/tests/origin-gate.test.ts new file mode 100644 index 0000000..7950373 --- /dev/null +++ b/tests/origin-gate.test.ts @@ -0,0 +1,176 @@ +import { expect, test } from "bun:test"; +import { createApp } from "@server/routes"; +import { AgentStore } from "@server/state/store"; +import { Hub } from "@server/ws/hub"; +import { tryUpgradeWs, type WsData } from "@server/ws/serve"; +import type { Server } from "bun"; +import type { Agent } from "@shared/types"; + +/** + * The two ENFORCEMENT POINTS, as opposed to `tests/origin.test.ts`, which pins + * the rule. Both are the single shared definition for their transport — one + * Hono middleware that both listeners inherit with the app, and the one `/ws` + * interception `ws/serve.ts` exists so there is only one of — so a hole here is + * a hole on the desk's 8787 and on the tunnel's gated listener at once. + * + * `app.request()` builds a request for `http://localhost/…`, so `http://localhost` + * is the same-origin case throughout this file. + */ + +const NOW = 1_700_000_000_000; + +const health = () => ({ + ok: true, hostId: "dev-box", agents: 1, clients: 0, herdrConnected: true, + lastEventAt: NOW, lastNotifyError: null, version: "0.0.0-dev", latestKnown: null, + herdrProtocol: null, schemaWarning: null, +}); + +function agent(state: Agent["state"] = "blocked"): Agent { + return { + hostId: "dev-box", agentId: "w1:p1", name: "api-refactor", + task: "Extract auth middleware", state, workspaceId: "w1", + workspaceLabel: "api work", cwd: "/srv/project", + stateSince: NOW, updatedAt: NOW, acknowledgedAt: null, + }; +} + +function harness(state: Agent["state"] = "blocked") { + const store = new AgentStore("dev-box"); + store.replaceAll([agent(state)], NOW); + const calls: string[] = []; + const actions = { + async readOutput() { calls.push("readOutput"); return { lines: ["out"], source: "visible" as const }; }, + async readDetection() { calls.push("readDetection"); return ""; }, + async sendOptionKey(_t: string, k: string) { calls.push(`key:${k}`); }, + async sendNavKey(_t: string, k: string) { calls.push(`nav:${k}`); }, + async sendReply(_t: string, text: string) { calls.push(`reply:${text}`); }, + async waitUntilUnblocked() { calls.push("wait"); }, + }; + const app = createApp({ + store, actions, now: () => NOW, health, + hub: new Hub({ now: () => NOW }), + }); + return { app, calls, store }; +} + +const post = (app: ReturnType, path: string, origin: string | null, body: object = {}) => + app.request(path, { + method: "POST", + headers: { + "content-type": "application/json", + ...(origin === null ? {} : { origin }), + }, + body: JSON.stringify(body), + }); + +test("a cross-origin reply is refused, and never reaches the agent", () => { + // THE hole this work closes. `/text` types arbitrary text into a live coding + // agent, and `jsonBody` never looks at the content type — so before this gate + // an `enctype="text/plain"` form on any page the operator visited could post + // valid JSON here with no preflight. The second assertion is the one that + // matters: a 403 that still typed the text would be no fix at all. + return (async () => { + const { app, calls } = harness(); + const res = await post(app, "/api/agents/w1:p1/text", "https://evil.example", { text: "rm -rf /" }); + expect(res.status).toBe(403); + expect(calls).toEqual([]); + })(); +}); + +test("the refusal names cross-origin as the reason", async () => { + const { app } = harness(); + const res = await post(app, "/api/agents/w1:p1/text", "https://evil.example", { text: "hi" }); + const body = await res.json(); + expect(body.ok).toBe(false); + expect(body.detail).toContain("cross-origin"); +}); + +test("a same-origin reply still works", async () => { + const { app, calls } = harness(); + const res = await post(app, "/api/agents/w1:p1/text", "http://localhost", { text: "yes" }); + expect(res.status).toBe(200); + expect(calls).toContain("reply:yes"); +}); + +test("a reply with no Origin still works — the command line is not a CSRF vector", async () => { + const { app, calls } = harness(); + const res = await post(app, "/api/agents/w1:p1/text", null, { text: "yes" }); + expect(res.status).toBe(200); + expect(calls).toContain("reply:yes"); +}); + +test("every write verb is covered, not just the ones with an actions dep", async () => { + // `/ack` touches only paddock's own store and the hub, and is registered even + // in demo mode. It must be gated by the same middleware — the guard belongs to + // the verb, not to a route's dependencies. `done`, because /ack refuses any + // other state with a 409 of its own and this test is about the gate. + const { app } = harness("done"); + expect((await post(app, "/api/agents/w1:p1/ack", "https://evil.example")).status).toBe(403); + expect((await post(app, "/api/agents/w1:p1/ack", "http://localhost")).status).toBe(200); +}); + +test("a foreign Origin on a READ is allowed", async () => { + // Deliberate, and load-bearing twice over. Browsers omit `Origin` on + // same-origin GETs, so a GET guard would have to accept a missing one anyway + // and would gate nothing. And a cross-origin GET cannot READ the response — + // paddock sends no CORS headers — so there is nothing to protect. Guarding + // GETs is how `/sw.js` and the app shell would break instead. + const { app } = harness(); + const res = await app.request("/api/agents", { headers: { origin: "https://evil.example" } }); + expect(res.status).toBe(200); +}); + +/** A `Server` that records whether `upgrade` was reached at all. */ +function fakeServer(): { srv: Server; calls: { upgrades: number } } { + const calls = { upgrades: 0 }; + const srv = { + upgrade() { + calls.upgrades += 1; + return true; + }, + } as unknown as Server; + return { srv, calls }; +} + +const upgradeReq = (origin: string | null) => + new Request("http://127.0.0.1:8787/ws", { + headers: { + upgrade: "websocket", + connection: "Upgrade", + ...(origin === null ? {} : { origin }), + }, + }); + +test("a cross-origin upgrade is refused before `upgrade` is called", () => { + // The socket sends the whole snapshot in `open`, so a refusal that arrived + // after the upgrade would have already disclosed every agent's screen. + const { srv, calls } = fakeServer(); + const res = tryUpgradeWs(upgradeReq("https://evil.example"), srv); + expect(res).toBeInstanceOf(Response); + expect((res as Response).status).toBe(403); + expect(calls.upgrades).toBe(0); +}); + +test("an upgrade with no Origin is refused", () => { + // Browsers always send it on a handshake, so this is `websocat` or a script — + // and every uid on the host can reach a TCP port, unlike herdr's socket file. + const { srv, calls } = fakeServer(); + const res = tryUpgradeWs(upgradeReq(null), srv); + expect((res as Response).status).toBe(403); + expect(calls.upgrades).toBe(0); +}); + +test("a same-origin upgrade proceeds", () => { + const { srv, calls } = fakeServer(); + const res = tryUpgradeWs(upgradeReq("http://127.0.0.1:8787"), srv); + // `undefined` is Bun's own signal that the response IS the upgrade. + expect(res).toBeUndefined(); + expect(calls.upgrades).toBe(1); +}); + +test("a request that is not /ws is still not this route's business", () => { + // The guard must not turn a fall-through into a refusal: `null` is how the + // caller learns the request belongs to the app. + const { srv } = fakeServer(); + expect(tryUpgradeWs(new Request("http://127.0.0.1:8787/api/agents"), srv)).toBeNull(); +}); diff --git a/tests/origin-tunnel.test.ts b/tests/origin-tunnel.test.ts new file mode 100644 index 0000000..122c2b6 --- /dev/null +++ b/tests/origin-tunnel.test.ts @@ -0,0 +1,209 @@ +import { expect, test } from "bun:test"; +import { connect } from "node:net"; +import { createApp } from "@server/routes"; +import { AgentStore } from "@server/state/store"; +import { COOKIE_NAME, Pairing } from "@server/tunnel/pairing"; +import { serveGated } from "@server/tunnel/run"; +import { Hub } from "@server/ws/hub"; +import type { Agent } from "@shared/types"; + +/** + * The three DEPLOYMENT SHAPES, through the real gated listener. + * + * `tests/origin-gate.test.ts` covers the two enforcement points in isolation. + * This file answers the question an operator actually has: does my deployment + * still work? Each test names the shape it stands for — the desk, a quick + * tunnel, a named tunnel — because the same-origin rule is the one change in + * this area that could turn a working phone into a read-only screen, and the + * failure would appear on a device the suite never runs on. + * + * The gated listener rather than the plain app on purpose: a request from a + * tunnel passes the PAIRING gate first and the origin gate second, and only + * this shape exercises both in the order production runs them. + */ + +const NOW = 1_700_000_000_000; +const TUNNEL_HOST = "apple-berry-cat-dog.trycloudflare.com"; +const NAMED_HOST = "paddock.example.com"; + +const health = () => ({ + ok: true, hostId: "dev-box", agents: 1, clients: 0, herdrConnected: true, + lastEventAt: NOW, lastNotifyError: null, version: "0.0.0-dev", latestKnown: null, + herdrProtocol: null, schemaWarning: null, +}); + +function agent(): Agent { + return { + hostId: "dev-box", agentId: "w1:p1", name: "docs-cleanup", + task: "Tidy the README", state: "done", workspaceId: "w1", + workspaceLabel: "docs", cwd: "/srv/project", + stateSince: NOW, updatedAt: NOW, acknowledgedAt: null, + }; +} + +/** + * A live gated listener whose allowlist is whatever the deployment knows. + * + * `publicHosts` is passed as the thunk `index.ts` passes, so what is under test + * is the wiring an operator runs, not a value inlined for the convenience of + * the assertion. + */ +function harness(publicHosts: readonly string[] = []) { + const store = new AgentStore("dev-box"); + store.replaceAll([agent()], NOW); + const pairing = new Pairing({ now: () => NOW }); + // The SAME thunk reaches the app's write middleware and the listener's `/ws` + // upgrade, exactly as `index.ts` hands it to both. Passing it to only one is + // the defect this file caught: writes and upgrades then answer to different + // allowlists. + const hosts = () => publicHosts; + const app = createApp({ + store, pairing, now: () => NOW, health, + hub: new Hub({ now: () => NOW }), + publicHosts: hosts, + }); + const server = serveGated({ + app, pairing, store, + hub: new Hub({ now: () => NOW }), + hostId: "dev-box", + port: 0, // the OS picks, so the suite never collides with a real tunnel + env: {}, + isTty: false, + publicHosts: hosts, + }); + const paired = pairing.attempt(pairing.current().code); + if (paired.kind !== "paired") throw new Error("unreachable: a fresh code must pair"); + return { server, token: paired.token }; +} + +/** + * A write as a browser on `origin` sends it, arriving with `host` as a proxy set + * it — sent over a RAW SOCKET, not `fetch`. + * + * `fetch` silently drops a `Host` header (it is forbidden to script), so every + * request it makes to a loopback listener claims a loopback host. An earlier + * version of this file used it and every tunnel case passed as the DESK case: + * green, and testing nothing it claimed to. Bytes on a socket are also exactly + * what `cloudflared` puts there, so this is the faithful shape rather than a + * clever one. + */ +function rawWrite(port: number, token: string, o: { host: string; origin: string }): Promise { + const body = "{}"; + const req = [ + "POST /api/agents/w1:p1/ack HTTP/1.1", + `Host: ${o.host}`, + `Origin: ${o.origin}`, + `Cookie: ${COOKIE_NAME}=${token}`, + "content-type: application/json", + `content-length: ${body.length}`, + "connection: close", + "", + body, + ].join("\r\n"); + return new Promise((resolve, reject) => { + const sock = connect({ host: "127.0.0.1", port }, () => sock.write(req)); + let out = ""; + sock.on("data", (d) => { out += d.toString("latin1"); }); + sock.on("error", reject); + sock.on("close", () => { + const status = /^HTTP\/1\.1 (\d{3})/.exec(out); + if (status === null) reject(new Error(`no status line: ${out.slice(0, 120)}`)); + else resolve(Number(status[1])); + }); + }); +} + +test("the desk still works: loopback, no public hostname known", async () => { + const { server, token } = harness([]); + try { + const status = await rawWrite(server.port, token, { + host: "127.0.0.1:8788", + origin: "http://127.0.0.1:8788", + }); + expect(status).toBe(200); + } finally { server.stop(); } +}); + +test("a quick tunnel still works: the run's own hostname", async () => { + // `paddock tunnel`. `publicHostsFrom` learns this hostname from the live + // value, since it is never written to settings.json. + const { server, token } = harness([TUNNEL_HOST]); + try { + const status = await rawWrite(server.port, token, { + host: TUNNEL_HOST, + origin: `https://${TUNNEL_HOST}`, + }); + expect(status).toBe(200); + } finally { server.stop(); } +}); + +test("a named tunnel still works: publicUrl's hostname", async () => { + const { server, token } = harness([NAMED_HOST]); + try { + const status = await rawWrite(server.port, token, { + host: NAMED_HOST, + origin: `https://${NAMED_HOST}`, + }); + expect(status).toBe(200); + } finally { server.stop(); } +}); + +test("a named tunnel works with publicUrl unset — the allowlist is inactive", async () => { + // The case that keeps `publicUrl` optional: an operator who does not use + // Telegram has never had a reason to set it, and must not lose the reply path + // for that. Same-origin is still enforced; only rebinding is uncovered. + const { server, token } = harness([]); + try { + const status = await rawWrite(server.port, token, { + host: NAMED_HOST, + origin: `https://${NAMED_HOST}`, + }); + expect(status).toBe(200); + } finally { server.stop(); } +}); + +test("desk browsing survives a populated allowlist", async () => { + // Setting a public URL must not break the loopback the operator uses at their + // own machine, or `make dev`. + const { server, token } = harness([NAMED_HOST]); + try { + const status = await rawWrite(server.port, token, { + host: "127.0.0.1:8788", + origin: "http://127.0.0.1:8788", + }); + expect(status).toBe(200); + } finally { server.stop(); } +}); + +test("a paired but cross-origin write is still refused", async () => { + // Pairing proves the DEVICE reached the tunnel, never which page is asking. + // A paired phone that later visits a hostile page must not become a lever on + // the agents, so the origin gate has to sit behind the pairing gate rather + // than being satisfied by it. + const { server, token } = harness([TUNNEL_HOST]); + try { + const status = await rawWrite(server.port, token, { + host: TUNNEL_HOST, + origin: "https://evil.example", + }); + expect(status).toBe(403); + } finally { server.stop(); } +}); + +test("a publicUrl naming a hostname the deployment is NOT reached on locks out writes", async () => { + // The one way this bites, recorded as a test so it is a known trade-off and + // not a surprise: a stale or mistyped `publicUrl`, or a second legitimate + // hostname for the same paddock, is refused even though Origin and Host + // agree — which is exactly the rebinding protection working as designed, + // pointed at the operator. `docs/gotchas.md` carries the symptom and the fix + // (correct the value, or clear it); the refusal names both headers on stderr + // so it is diagnosable rather than mysterious. + const { server, token } = harness(["stale.example"]); + try { + const status = await rawWrite(server.port, token, { + host: NAMED_HOST, + origin: `https://${NAMED_HOST}`, + }); + expect(status).toBe(403); + } finally { server.stop(); } +}); diff --git a/tests/origin.test.ts b/tests/origin.test.ts new file mode 100644 index 0000000..d58eef7 --- /dev/null +++ b/tests/origin.test.ts @@ -0,0 +1,195 @@ +import { expect, test } from "bun:test"; +import { + allowUpgrade, + allowWrite, + isLoopbackHost, + publicHostsFrom, + refusalReason, +} from "@server/origin"; + +/** + * The gate these predicates implement is decision 12's missing half. + * + * That decision restored the CORS preflight requirement for the three settings + * routes 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." `POST /api/agents/:id/text` types arbitrary text + * into a coding agent and reads a body with `jsonBody`, which never looks at + * the content type — so it was reachable from an `enctype="text/plain"` form on + * any page the operator visited, and `/ws` handed that same page every agent's + * screen because a WebSocket handshake is exempt from CORS entirely. + * + * These tests pin the rule, not the plumbing. The two predicates differ in + * exactly one place — a missing `Origin` — and that difference is load-bearing + * enough to be asserted from both sides. + */ + +const LOOPBACK = "127.0.0.1:8787"; + +test("a write with no Origin is allowed — a non-browser client is not a CSRF vector", () => { + // curl, a script, `paddock doctor`: none of them carry an attacker's page. + // The threat is a browser acting on a hostile page's behalf, and browsers + // ALWAYS send Origin on a POST. Refusing here would buy nothing and break + // every command-line use of the API. + expect(allowWrite(null, LOOPBACK, [])).toBe(true); +}); + +test("a same-origin write is allowed", () => { + expect(allowWrite("http://127.0.0.1:8787", LOOPBACK, [])).toBe(true); +}); + +test("a cross-origin write is refused", () => { + // The drive-by: any page the operator visits POSTing into a live agent. + expect(allowWrite("https://evil.example", LOOPBACK, [])).toBe(false); +}); + +test("an opaque Origin is refused", () => { + // A sandboxed iframe or a `data:` document sends the literal string "null". + // It parses as a URL nowhere, and it is nobody's first-party request. + expect(allowWrite("null", LOOPBACK, [])).toBe(false); +}); + +test("an unparseable Origin is refused", () => { + expect(allowWrite("not a url", LOOPBACK, [])).toBe(false); +}); + +test("a port mismatch is cross-origin", () => { + // Same host, different port is a different origin to a browser, and must be + // one here: another service on the operator's own machine is exactly the + // attacker the loopback listener is exposed to. + expect(allowWrite("http://127.0.0.1:9999", LOOPBACK, [])).toBe(false); +}); + +test("Host is compared case-insensitively", () => { + // Hostnames are case-insensitive and a proxy may not preserve case; a 403 + // over letter case would be an unexplainable outage. + expect(allowWrite("https://paddock.example.com", "Paddock.Example.COM", [])).toBe(true); +}); + +test("a rebinding request is refused once a public host is known", () => { + // DNS rebinding is the one attack a bare same-origin check cannot see: the + // browser is tricked into resolving evil.example to 127.0.0.1, so Host and + // Origin AGREE and the comparison passes. The allowlist is what catches it. + expect(allowWrite("https://evil.example", "evil.example", ["paddock.example.com"])).toBe(false); +}); + +test("a rebinding request passes when no public host is known — the documented limit", () => { + // Deliberate, and the reason `publicUrl` stays optional. `publicUrl` lives in + // the Notifications section: making it load-bearing for the reply path would + // turn a Telegram setting into the difference between a working dashboard and + // a read-only one, for a reason no operator could guess. So an empty + // allowlist means "we do not know this deployment's hostname", not "allow + // nothing" — CSRF is still closed, rebinding is not. + expect(allowWrite("https://evil.example", "evil.example", [])).toBe(true); +}); + +test("loopback is allowed even when it is not on a populated allowlist", () => { + // Desk browsing and `make dev` must not start failing the moment an operator + // sets a public URL for notification deep links. + expect(allowWrite("http://127.0.0.1:8787", LOOPBACK, ["paddock.example.com"])).toBe(true); +}); + +test("a known public host is allowed", () => { + expect( + allowWrite("https://paddock.example.com", "paddock.example.com", ["paddock.example.com"]), + ).toBe(true); +}); + +test("a host outside a populated allowlist is refused even when Origin agrees", () => { + expect(allowWrite("https://other.example", "other.example", ["paddock.example.com"])).toBe(false); +}); + +test("an upgrade with no Origin is REFUSED, unlike a write", () => { + // The one place the two predicates disagree. Browsers always send Origin on + // a WebSocket handshake — same-origin included, unlike a GET — so requiring + // it costs a browser nothing and shuts out `websocat`. That matters because + // herdr's 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. `/ws` + // hands over every agent's screen on connect, so it is the read worth + // closing. + expect(allowUpgrade(null, LOOPBACK, [])).toBe(false); +}); + +test("a same-origin upgrade is allowed", () => { + expect(allowUpgrade("http://127.0.0.1:8787", LOOPBACK, [])).toBe(true); +}); + +test("a cross-origin upgrade is refused", () => { + expect(allowUpgrade("https://evil.example", LOOPBACK, [])).toBe(false); +}); + +test("an upgrade obeys the same allowlist as a write", () => { + expect(allowUpgrade("https://evil.example", "evil.example", ["paddock.example.com"])).toBe(false); +}); + +test("isLoopbackHost accepts the three loopback spellings, with or without a port", () => { + expect(isLoopbackHost("127.0.0.1")).toBe(true); + expect(isLoopbackHost("127.0.0.1:8787")).toBe(true); + expect(isLoopbackHost("localhost")).toBe(true); + expect(isLoopbackHost("localhost:5173")).toBe(true); + expect(isLoopbackHost("[::1]:8787")).toBe(true); +}); + +test("isLoopbackHost is not fooled by a hostname that merely starts with one", () => { + // `127.0.0.1.evil.com` resolves wherever its owner points it. + expect(isLoopbackHost("127.0.0.1.evil.com")).toBe(false); + expect(isLoopbackHost("localhost.evil.com")).toBe(false); + expect(isLoopbackHost("notlocalhost")).toBe(false); +}); + +test("publicHostsFrom returns nothing when no public hostname is known", () => { + // Empty is INACTIVE, not empty-so-deny. See the rebinding test above. + expect(publicHostsFrom(null, null)).toEqual([]); + expect(publicHostsFrom("", null)).toEqual([]); +}); + +test("publicHostsFrom takes the host of a configured publicUrl, port included", () => { + expect(publicHostsFrom("https://paddock.example.com", null)).toEqual(["paddock.example.com"]); + expect(publicHostsFrom("https://paddock.example.com:8443/", null)).toEqual([ + "paddock.example.com:8443", + ]); +}); + +test("publicHostsFrom includes a live tunnel URL", () => { + // A `paddock tunnel` run's hostname is different on every run and is never + // written to settings.json, so it can only come from the live value. + expect(publicHostsFrom(null, "https://apple-berry-cat-dog.trycloudflare.com")).toEqual([ + "apple-berry-cat-dog.trycloudflare.com", + ]); +}); + +test("publicHostsFrom holds both a saved deployment and a live tunnel at once", () => { + // `paddock tunnel` run against a host that also has a named-tunnel publicUrl + // saved: both hostnames are legitimate for the life of that run. + expect( + publicHostsFrom("https://paddock.example.com", "https://apple-berry.trycloudflare.com"), + ).toEqual(["paddock.example.com", "apple-berry.trycloudflare.com"]); +}); + +test("publicHostsFrom ignores a value that is not a URL", () => { + // `publicUrl` is a free-text settings field. A typo must not silently + // populate the allowlist with garbage and lock the operator out — an + // unparseable value is no knowledge at all, which is the inactive case. + expect(publicHostsFrom("paddock.example.com", null)).toEqual([]); + expect(publicHostsFrom("¯\\_(ツ)_/¯", null)).toEqual([]); +}); + +test("publicHostsFrom lowercases what it stores, so the allowlist matches Host", () => { + expect(publicHostsFrom("https://Paddock.Example.COM", null)).toEqual(["paddock.example.com"]); +}); + +/** + * The refusal has TWO causes that need different fixes, and telling an operator + * the wrong one costs them the afternoon. A mismatched `Origin`/`Host` means a + * proxy is rewriting `Host`; a matched pair refused anyway means `publicUrl` + * names a hostname this deployment is not actually reached on. The message that + * blames a proxy for the second is worse than no message. + */ +test("refusalReason distinguishes a rewriting proxy from a wrong publicUrl", () => { + // Origin and Host agree, and the allowlist is what refused: publicUrl's fault. + expect(refusalReason("https://paddock.example.com", "paddock.example.com", ["stale.example"])) + .toBe("host-not-allowed"); + // Origin and Host disagree: a hostile page, or a proxy that rewrote Host. + expect(refusalReason("https://evil.example", "paddock.example.com", [])).toBe("cross-origin"); + expect(refusalReason(null, "paddock.example.com", [])).toBe("no-origin"); +});