From 8ed715d92de3fbe1f25036568436e0fd1af77b27 Mon Sep 17 00:00:00 2001 From: Optic00 Date: Sun, 26 Jul 2026 10:26:21 +0200 Subject: [PATCH] fix(bridge): refuse writes when the device header is absent deviceAuth() treated a request that carries no device header as an authorised on-host operator: if (!device) return { enforced: true, device: null, authorized: true }; The comment above the branch documents the intent as "an absent header is the on-host loopback operator", but the request's origin was never checked, so the branch granted full write access to any caller that reached the bridge without the header. README.md already documented the consequence, in the Variant B warning: an absent header means full access, so the gate is a no-op. Configuring COLLIE_DEVICE_HEADER is the operator asserting that their proxy sets the header on every request. A request without one did not come through that proxy, and is now treated exactly like a device that is not on the allowlist: read-only. Read-only is the accurate word, not "rejected". guard() consults deviceAuth only for writes, so a header-less caller still reads panes, as an unlisted device always could. That is the existing design and this change does not touch it; what changes is that a missing header no longer counts as the operator. The README now says so explicitly, including that pane text is therefore not behind this gate. There is deliberately no loopback exemption, which is worth recording because it looks like the obvious way to keep the on-host convenience. Every supported front door is a proxy co-located with the bridge: collie-ctl.sh maps tailscale serve to 127.0.0.1:PORT, and the documented Variant B and C proxies use a loopback proxy_pass. A header-less remote request therefore arrives with a loopback peer just like a local one, so neither the peer address nor the Host header separates the operator on the host from a remote client whose proxy failed to inject the header. An exemption would have re-opened the very path this closes. The cost is that the bridge's own loopback URL is read-only while device auth is on, since it bypasses the proxy that would inject the header. The README states that, and gives the by-hand alternative: curl the loopback bridge with an allowlisted id, not the public URL, where the proxy's mandatory override would replace it. guard() is exported so the wiring is tested, not just deviceAuth in isolation: the gate lives in the write/read asymmetry there, and deviceAuth being correct on its own would not catch that regressing. Both halves are asserted. This closes the absent-header half of the documented hole. The other half, a client setting the header itself on plain tailscale serve, is unchanged and still needs a proxy that overrides it; the README warning now says just that. A deployment that does not set COLLIE_DEVICE_HEADER is unaffected. --- README.md | 37 +++++++++++++++++++-------- bridge/server.test.ts | 59 ++++++++++++++++++++++++++++++++++++++++--- bridge/server.ts | 25 ++++++++++++++---- 3 files changed, 103 insertions(+), 18 deletions(-) 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 }; }