fix: a page you visit should not be able to type into your agents - #8
Merged
Conversation
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Closes the hole decision 12 said it was leaving open: "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, and they composed into one attack:
/wsupgraded unconditionally. A WebSocket handshake is exempt from CORS entirely, so no preflight and no browser rule stood in the way — andhubWebSocket.opensends the whole snapshot on connect. Any page the operator visited could openws://127.0.0.1:8787/wsand read every agent's name, id and screen.POST /api/agents/:id/textwas CORS-simple. It types arbitrary text into a live coding agent and reads its body withjsonBody, which never inspects the content type — so anenctype="text/plain"form posts syntactically valid JSON to it with no preflight and 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 right for an operator on their own machine and no defence whatsoever against their own browser.
src/server/origin.tsholds the rule as pure predicates, called from exactly two enforcement points — one middleware both listeners inherit with the app, and the/wsinterceptionws/serve.tsexists so there is only one of. Not per-route: the guard belongs to the verb, so a future write route is covered by existing rather than by remembering to opt in.This is not authentication and decision 3 stands. Nothing here identifies anybody and no token is minted or held. It asks the one question a browser cannot lie about — which page this request acts for.
Three asymmetries, each load-bearing
Originon 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. What guarding reads would achieve is breaking/sw.js— decision 3's failure from a new direction.Originpasses a write, fails an upgrade. Browsers always send it on a POST, so its absence is curl, not a hostile page. They always send it on a handshake too, so requiring it there costs a browser nothing and shuts outwebsocat— which matters because herdr's socket is a FILE with permissions while paddock's port is TCP that every uid on the host can reach.Origin == Hostcloses ordinary CSRF with no config. Rebinding needs the real hostname, which paddock already knows fromsettings.publicUrlplus the live tunnel URL — but it is enforced only when non-empty.publicUrllives in the Notifications section, so making it unconditional would turn a Telegram convenience into the difference between a working dashboard and a read-only one, for a reason no operator could guess.The one way it bites
Once
publicUrlis set, a stale value, a typo, or a second legitimate hostname for the same paddock is refused even thoughOriginandHostagree. Loopback stays exempt, so it fails from the phone while the desk works.refusalReasonpicks the remedy rather than guessing, because the two causes need opposite fixes:versus the proxy-rewrites-
Hostcase. Both are indocs/gotchas.md; decision 17 has the full reasoning.Measurements
Verified beyond the suite, since a same-origin rule is the one change here that could turn a working phone into a read-only screen:
403, same-origin write through, no-Originwrite through, foreign-Originread200, upgrade with noOrigin403, foreignOrigin403, same-origin101.make dev's vite proxy (plain string target, sochangeOrigindefaults tofalseandHostis forwarded): writes through, WS101, zero refusals.tests/origin-tunnel.test.ts.Two things that test caught, both of which would otherwise have shipped:
fetchsilently drops a customHost(forbidden to script), so the first version of those tests had every tunnel case passing as the desk case — green, and testing nothing it claimed to. They now speak raw HTTP over a socket, which is what cloudflared does anyway.index.ts, handed to every consumer.Checklist
make check && make check-clean && make testpass — 903 tests