diff --git a/README.md b/README.md index cd4a16a..6dbcd6f 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,8 @@ It's built single-user and tailnet-only. The defenses: - **Optional identity gate** — set `COLLIE_TRUSTED_USER` to reject anyone but you. - **Optional per-device gate** — behind a proxy that injects a device-identity header, set `COLLIE_DEVICE_HEADER` + `COLLIE_DEVICE_ALLOWLIST` so only allowlisted devices can drive agents; - any other device is read-only. Off by default; revoke a device by dropping it from the list. + any other device is read-only, and so is a request that arrives without the header at all. Off by + default; revoke a device by dropping it from the list. See [Deployment variants](#deployment-variants) for the proxy this requires. - **Same-origin gate + strict CSP**; pane output renders as React text nodes, never `innerHTML`. - **Optional Host allowlist** — set `COLLIE_PUBLIC_HOSTS` to the exact host(s) you serve on (e.g. @@ -407,17 +408,32 @@ This is the right choice unless you specifically need per-device control. Use this when some devices should **drive** agents and others should be **read-only** — e.g. your phone can reply, but a shared/less-trusted device can only watch. Collie reads an opaque device id from a request header (`COLLIE_DEVICE_HEADER`) and checks it against `COLLIE_DEVICE_ALLOWLIST`: -allow-listed → full access, any other id → read-only, header absent → treated as the on-host -operator (full access). +allow-listed → full access, any other id → read-only, header absent → read-only as well. -That last rule is the catch: **device-auth only works behind a reverse proxy that authenticates the +Treating an absent header as read-only is the point: switching this on is you asserting that your +proxy sets the header on every request, so a request without one did not come through that proxy and +must not drive a terminal. **Device-auth only works behind a reverse proxy that authenticates the device and injects the header.** It is not a standalone flag. -> ⚠️ **Do not enable `COLLIE_DEVICE_HEADER` on plain `tailscale serve`.** An *absent* header means -> full access, and `tailscale serve` injects only its own `Tailscale-*` headers — it *forwards* an -> arbitrary `X-Device-Id` untouched. So a remote request with no header gets full access (the gate -> is a no-op), and a client that *sets* `X-Device-Id: my-phone` itself is trusted (spoofable). -> Sound only behind a proxy that does both things below. +Note what "read-only" means here: the gate covers writes (replies, keys, uploads, pane and tab +create/close). Reading panes, polling the snapshot and listing sessions stay open to any caller that +gets past the same-origin and Host checks, exactly as they do for a device that is simply not on the +allowlist. Pane text can contain anything your agents printed, so the header is not a confidentiality +boundary. + +Two consequences worth knowing before you turn this on: + +- **The bridge's own loopback URL becomes read-only.** `http://127.0.0.1:$COLLIE_PORT` bypasses your + proxy, so the PWA loaded from it sends no device header and shows its read-only state. Drive the + herd through the proxied URL instead. +- **To drive a pane from the host by hand**, send an allowlisted id yourself, against the loopback + bridge rather than the public URL (the proxy's mandatory override in point 2 below would replace + your header): `curl -H 'X-Device-Id: my-laptop' http://127.0.0.1:$COLLIE_PORT/api/...` + +> ⚠️ **Do not enable `COLLIE_DEVICE_HEADER` on plain `tailscale serve`.** `tailscale serve` injects +> only its own `Tailscale-*` headers and *forwards* an arbitrary `X-Device-Id` untouched, so a +> client that *sets* `X-Device-Id: my-phone` itself is trusted. Spoofing is what makes this unsound, +> and only a proxy that **overrides** the header (point 2 below) closes it. Your fronting proxy **must**: @@ -458,7 +474,8 @@ location / { Revoke a device by dropping its id from `COLLIE_DEVICE_ALLOWLIST` and `systemctl --user restart collie`. With the header set but the allowlist **empty**, every device is -read-only (fail-closed). +read-only (fail-closed), and so is a request that arrives without the header. In that state nothing +can drive a pane, including a hand-made `curl`; recovery is an `.env` edit plus a restart. ### Variant C — reverse proxy as the only front door (no Tailscale) diff --git a/bridge/server.test.ts b/bridge/server.test.ts index 329255b..1daf915 100644 --- a/bridge/server.test.ts +++ b/bridge/server.test.ts @@ -5,6 +5,7 @@ import { cacheControlFor, checkAccess, deviceAuth, + guard, isHostAllowed, normalizeTabLabel, paneReadResponse, @@ -342,21 +343,73 @@ describe("deviceAuth — per-device authorisation", () => { }); }); - test("feature on, header absent: authorised and unchanged (on-host loopback operator)", () => { + test("feature on, header absent: refused", () => { const c = cfg({ deviceHeader: HDR, deviceAllowlist: ["phone"] }); expect(deviceAuth(req({ host: "h" }), c)).toEqual({ enforced: true, device: null, - authorized: true, + authorized: false, }); // A blank/whitespace header value is treated as absent, not as a device named "". expect(deviceAuth(req({ host: "h", "x-device-id": " " }), c)).toEqual({ enforced: true, device: null, - authorized: true, + authorized: false, }); }); + // The absent-header case has no loopback exemption, and a loopback-looking Host must not create + // one by the back door: Host is set by the caller and rewritten by the proxy, so it attests + // nothing. This pins that no future "but it came from localhost" shortcut sneaks in here. + test("feature on, header absent: a loopback Host does not buy an exemption", () => { + const c = cfg({ deviceHeader: HDR, deviceAllowlist: ["phone"] }); + for (const host of ["127.0.0.1", "127.0.0.1:8787", "localhost", "[::1]:8787"]) { + expect(deviceAuth(req({ host }), c).authorized).toBe(false); + } + }); +}); + +// deviceAuth being right in isolation proves nothing if the wiring in guard() regresses, and that +// wiring is where the whole gate lives: it consults deviceAuth for "write" and deliberately not for +// "read". Both halves are asserted here, so neither the gate nor the read-only scope can drift +// silently. The write cases carry a matching Origin so checkAccess passes and the device decision is +// the only thing under test. +describe("guard applies the device gate to writes only", () => { + const HDR = "x-device-id"; + const c = cfg({ deviceHeader: HDR, deviceAllowlist: ["phone"] }); + const write = (headers: Record) => + guard(req({ host: "collie.ts.net", origin: "https://collie.ts.net", ...headers }), c, "write"); + const read = (headers: Record) => + guard(req({ host: "collie.ts.net", ...headers }), c, "read"); + + test("write with no device header is refused with 403", () => { + const denied = write({}); + expect(denied).not.toBeNull(); + expect(denied!.status).toBe(403); + }); + + test("write with a non-allowlisted device is refused with 403", () => { + const denied = write({ "x-device-id": "intruder" }); + expect(denied).not.toBeNull(); + expect(denied!.status).toBe(403); + }); + + test("write with an allowlisted device proceeds", () => { + expect(write({ "x-device-id": "phone" })).toBeNull(); + }); + + // The scope of the gate, stated as a test rather than only in prose: a header-less caller keeps + // READ access (it is read-only, not rejected outright). If someone later tightens this, it should + // be a deliberate change with this test updated, not an accident. + test("read with no device header still proceeds (read-only, not rejected)", () => { + expect(read({})).toBeNull(); + expect(read({ "x-device-id": "intruder" })).toBeNull(); + }); + + test("with the feature off, a write with no device header proceeds", () => { + expect(guard(req({ host: "127.0.0.1:8787" }), cfg(), "write")).toBeNull(); + }); + test("feature on, allowlisted device: authorised and attributed (header is trimmed)", () => { const c = cfg({ deviceHeader: HDR, deviceAllowlist: ["phone", "laptop"] }); expect(deviceAuth(req({ host: "h", "x-device-id": " phone " }), c)).toEqual({ diff --git a/bridge/server.ts b/bridge/server.ts index 1bc89c6..46e914c 100644 --- a/bridge/server.ts +++ b/bridge/server.ts @@ -852,8 +852,11 @@ export function isHostAllowed(host: string, cfg: Config): boolean { * (same-origin / CSRF + optional Tailscale identity). A `"write"` request — one that types into a * terminal or creates panes — must additionally come from an authorised device (see * {@link deviceAuth}). Returns a 403 Response to short-circuit on denial, or null to proceed. + * + * Exported for tests: {@link deviceAuth} being correct in isolation proves nothing if this wiring + * regresses, and the write/read asymmetry below is exactly what a device gate stands or falls on. */ -function guard(req: Request, cfg: Config, level: "read" | "write"): Response | null { +export function guard(req: Request, cfg: Config, level: "read" | "write"): Response | null { const gate = checkAccess(req, cfg, level); if (!gate.ok) return text(gate.reason, 403); if (level === "write" && !deviceAuth(req, cfg).authorized) { @@ -869,19 +872,31 @@ function guard(req: Request, cfg: Config, level: "read" | "write"): Response | n * so a direct client can't forge it (the same trust basis as the Tailscale identity header). Matrix: * * - feature off (no header configured) → not enforced, fully authorised (today's behaviour). - * - header absent → authorised, unchanged. The proxy injects the header for - * real device traffic; an absent header is the on-host - * loopback operator (same tolerance as a missing identity). + * - header absent → read-only, same as an unlisted device. Configuring the + * header is the operator asserting that the proxy sets it + * on every request, so a request without one did not come + * through that proxy and must not drive a terminal. * - header present, value allowlisted → authorised; the session is attributed to that device. * - header present, value not listed → read-only. The "unknown" sentinel is never authorised, * and an empty allowlist makes every device read-only — a * fail-closed default for a security toggle you turned on. + * + * "Read-only" is the whole scope of this gate, deliberately: {@link guard} consults it only for + * `"write"`, so a header-less caller still reads panes. That is the existing design (a read-only + * device is meant to watch), and this function does not change it. What changes is that a missing + * header no longer counts as the operator. + * + * The absent-header case deliberately has no loopback exemption. It looks like the natural place for + * one, but every supported front door is a proxy co-located with the bridge (tailscale serve and the + * documented reverse proxies all connect to 127.0.0.1), so a loopback peer says nothing about + * whether the caller is the operator on the host or a remote client whose proxy failed to inject the + * header. Driving a pane from the host is still one flag away: send an allowlisted id yourself. */ export function deviceAuth(req: Request, cfg: Config): DeviceAuth { if (!cfg.deviceHeader) return { enforced: false, device: null, authorized: true }; const raw = req.headers.get(cfg.deviceHeader); const device = raw?.trim() ? raw.trim() : null; - if (!device) return { enforced: true, device: null, authorized: true }; + if (!device) return { enforced: true, device: null, authorized: false }; const authorized = device !== "unknown" && cfg.deviceAllowlist.includes(device); return { enforced: true, device, authorized }; }