fix(web): biorouter web serves no transcripts, gates the turn that reaches a chat, and stops reflecting the URL into script context (#56)

[Broccolito]

⚠ Security-sensitive — wants human review

Per HOWTOAI.md ("Always get human review for: security sensitive code"), this
touches an authentication-adjacent surface and closes a remote-code-execution
path. Please do not merge on CI alone.

Two things land here, on one surface: the reach gate d8cd7be7 added to
biorouter web is verified and finished, and the reflected XSS on the same
page is closed.

---

Part 1 — the reach gate, audited and finished

d8cd7be7 removed GET /api/sessions and GET /api/sessions/{id} (which served
every chat on the machine, and any chat's full transcript, behind no reach check
and — without --auth-token — behind no credential) and added turn_reach so a
WebSocket message naming a private chat cannot run a turn there.

I audited every path in web.rs that can reach a chat. There are five routes
and two socket message types:

Path	Reaches a chat?	Gated
GET /	creates a new chat only	n/a
GET /session/{name}	reads nothing from the store — same HTML for any name, so it is not an existence oracle either	n/a
GET /ws → message	runs a turn in the named chat	✅ turn_reach
GET /ws → cancel	was not gated	✅ fixed here
GET /api/health	static JSON, deliberately auth-exempt	n/a
GET /static/{*path}	literal match on four names, no filesystem read	n/a

Every store access in the file (grep for session_manager., get_session,
agent.reply) is either a creation, the gate's own metadata-only read, or
downstream of the gate.

The one gap: cancel. handle_cancel_message named a chat and was not
judged. It could only ever abort a turn the gate had already admitted — a handle
lands in cancellations only after handle_user_message passed — so nothing
private could be disturbed. What it could do is answer: it replied
Cancelled when a handle existed and said nothing when it did not, which is one
bit about a chat the sender may not reach. It is gated now, which also makes the
property flat rather than derived: every socket message that names a chat is
judged before the chat is touched.

Residual I did not close, deliberately. turn_reach runs before the
tokio::spawn, so there is a narrow window in which another process could ratchet
the chat private between the gate and agent.reply. That is the same shape the
daemon's own session_reach has, and Gate B at the top of Agent::reply still
governs the model binding. Closing it would mean re-judging inside the spawned
task; worth a maintainer's opinion rather than a unilateral restructure.

Fail-before evidence

d8cd7be7's parent 6455bc21 has both routes (web.rs:245-246) and the
unescaped sink (web.rs:354), so the new tests cannot compile against it — the
functions they call do not exist there. To get real fail-before signal I removed
both turn_reach call sites from the current tree and re-ran:

a_page_on_a_private_model_keeps_its_own_chats_and_no_others ... FAILED
a_page_on_a_public_model_reaches_no_private_chat ............. FAILED
a_cancel_naming_an_unreachable_chat_is_refused_like_a_message  FAILED
(the three XSS tests stayed green)
test result: FAILED. 13 passed; 3 failed

Then restored, and all 16 pass. The two concerns are independently pinned.

The refusal is not an existence oracle

Asserted as byte equality, at every capability, rather than as each answer
being vague — a vagueness check passes an implementation that adds one helpful
clause to the branch it can tell apart:

assert_eq!(
    refuse_turn_unless_reachable(true, capability, None),
    refuse_turn_unless_reachable(true, capability, Some(SessionClassification::Private)),
);

Plus end-to-end over the real socket: server.send(&private) and
server.send("19700101_0") both return the identical CHAT_OUT_OF_REACH frame,
and now server.cancel(...) does too. This was already correct in d8cd7be7;
I verified it rather than changed it.

SD-13 is the right number

docs/deployment/serve-decisions.md carries SD-1…SD-9. SD-10 is claimed by
#237, SD-11 by #240, SD-12 by #229 — all unmerged, so those numbers stay free.
SD-13 is this branch's. Nothing else was renumbered. The XSS is recorded as a
subsection inside SD-13 rather than as SD-14, so this PR claims exactly one
number and cannot collide with another in-flight record.

---

Part 2 — the reflected XSS

The sink

serve_session built the page with:

"<script>window.BIOROUTER_SESSION_NAME = '{}'; window.BIOROUTER_WS_TOKEN = '{}';</script>", session_name, state.ws_token

session_name is the {session_name} path segment — whatever the sender typed —
and a loopback bind requires no --auth-token, so nothing stands in front of it.
A ' ends the string literal; a </script> ends the element. Captured verbatim
from the pre-fix handler by the new test:

<script>window.BIOROUTER_SESSION_NAME = '</script><img src=x onerror=alert(1)>"'&'; window.BIOROUTER_WS_TOKEN = 'test-ws-token';</script>

Why this is RCE, not defacement

The injected script runs on the server's own origin. It reads the WebSocket token
out of the very document it was injected into, opens /ws?token=…, and sends a
message to an agent holding developer__shell. WebSockets are not subject to
the same-origin policy, so that token is the only thing standing between a
drive-by page and the socket — and the injection is handed it. One link the
operator clicks is arbitrary command execution as the operator.

The fix, and the two alternatives rejected

The values leave script context entirely. They are written as attributes on a
<div id="biorouter-boot" hidden data-session-name="…" data-ws-token="…"> and
read back through dataset.

• ❌ HTML-escape inside the <script>. Not a fix, and it is the trap: the HTML
  parser does not decode entities inside <script>, so </script> reaches
  the JavaScript parser verbatim and nothing has been neutralised.
• ❌ A JSON <script type="application/json"> block. serde_json escapes for
  JSON, which says nothing about HTML — it leaves < and / alone, so a value
  holding </script still ends the element. It needs a second HTML-specific
  escape on top, which is the attribute answer with an extra step.
• ❌ Validate the id's shape and 404 the rest. Rejected as the primary fix: a
  guess about a format that has changed before, it would refuse ids this route
  serves today, and a correct escape does not need it. Fine to add later as depth.
• ✅ A double-quoted HTML attribute. The parser does decode entities there,
  so JS gets the value exactly as it arrived, and no byte can leave the attribute.

escape_html_attribute covers & < > " ' as a character walk rather than a chain
of replaces — a chain has an ordering hazard (escape & anywhere but first and
it re-escapes the & of every entity the earlier steps wrote) and a walk cannot.
state.ws_token goes through the same escape even though the server generates it:
a value is escaped for where it is going, never for where the reader believes it
came from.

Defence in depth, not the fix: the response now carries
Content-Security-Policy: … script-src 'self' …, so a future missed sink on
this page is inert. That is why index.html's five suggestion pills lost their
onclick attributes and bind through data-suggestion in script.js — an inline
handler is exactly what script-src 'self' refuses, so the two move together.

Every other reflected value I audited

Value	Where	Verdict
session_name → inline <script>	serve_session	FIXED — the reported bug
state.ws_token → inline <script>	serve_session	FIXED — server-generated, but the same unsafe mechanism
data.tool_name → innerHTML	script.js handleToolRequest	FIXED — model-controlled, reachable by prompt injection
data.tool_name → innerHTML	script.js handleToolConfirmation	FIXED — same
JSON.stringify(data.arguments) → innerHTML	handleToolRequest	FIXED — JSON escaping is not HTML escaping
JSON.stringify(data.arguments) → innerHTML	handleToolConfirmation	FIXED — same
data.arguments.command (as action) → innerHTML	handleToolRequest	FIXED — its sibling path was already escaped; this one was not
uri.query() → Location: header	serve_index	SAFE, audited. uri.query() is the raw (still percent-encoded) query, hyper rejects raw CR/LF in a request target, and the target is always the relative /session/{server-generated-id}?…. No header injection, no open redirect
assistant message text	formatMessageContent	SAFE — escapes & < > before any markup is added, and the one attribute it writes is language-${lang} with lang constrained to (\w+)?
?q= URL parameter	getQueryParam	SAFE — assigned to textarea.value, not innerHTML
?session= / ?name= URL parameters	getSessionId	SAFE — becomes the socket's session_id, which Part 1's gate judges
tool result text, error/cancel messages	script.js	SAFE — already escapeHtml'd, all in element content

escapeHtml in script.js does not escape ", which is adequate for all of the
above and only because every one of those holes is in element content, never in
an attribute value. Noted in a comment beside them so the next person adding one
does not assume otherwise.

Flagged, not fixed → CLOSED in the second review round

build_cors_layer allow-listed http://localhost:3000 / 127.0.0.1:3000, so a page there could
read /session/… cross-origin and lift the WebSocket token — the same capability the XSS gave.

This was originally left open here as a maintainer's call, and that was the wrong call. It is
closed in a6f0d246; see "Second review round" below, finding 4. Three further holes on the same
surface were found and closed in that round, including a missing Origin check on the WebSocket
itself. Read that section — it supersedes this one.

Recommendation: delete biorouter web

biorouter web is deprecated in favour of biorouter serve, which serves the real
interface. Every hole above lives in a page nothing else uses. Deleting the
command would close all of them permanently and retire SD-13's whole surface with
it. I fixed the holes rather than deleting the command, because that is not a call
to make unilaterally — but it is the answer I would pick.

---

Tests

crates/biorouter-cli/src/commands/web.rs, 16 tests in the module (7 new):

• an_attribute_escape_neutralises_every_character_that_could_leave_one — the
  escaper, including that < comes back as < and not doubled.
• a_session_name_cannot_reach_script_context — drives the real router over a
  real socket with </script><img src=x onerror=alert(1)>"'&. Asserted as "the
  page has exactly the script elements its own template has" rather than as "the
  payload does not appear", because the weaker form passes an implementation that
  HTML-escapes inside the <script> body — which is not a fix.
• an_ordinary_session_name_still_reaches_the_page — a breakout test alone passes
  a handler that drops the value entirely.
• the_page_is_served_under_a_policy_that_refuses_inline_script — the header, and
  that the template carries no onclick for it to refuse.
• the_page_reads_its_boot_values_from_attributes — no window.BIOROUTER_* left,
  so the server cannot be pushed back into script context to satisfy the page.
• model_controlled_values_are_escaped_before_they_become_markup — the four
  script.js holes stay closed.
• a_cancel_naming_an_unreachable_chat_is_refused_like_a_message — the gate, plus
  that a reachable cancel with nothing running answers nothing, so the refusal is
  the gate and not merely "no such turn".

Verification run

cargo test -p biorouter --test privacy_guard_wiring --test privacy_capability   3 + 4 passed
cargo test -p biorouter-cli (isolated HOME)                                    444 + 3 + 3 passed, 0 failed
cargo test -p biorouter-cli --lib -- commands::web                             16 passed
cargo fmt --check                                                              clean
./scripts/clippy-lint.sh                                                       1 pre-existing finding

Notes on the last two:

• The census collision with fix(privacy): one reach gate for every HTTP route that names a chat or a knowledge base (QA H2, M1, M2, F0) #237 did not materialise. origin/main does not
  carry fix(privacy): one reach gate for every HTTP route that names a chat or a knowledge base (QA H2, M1, M2, F0) #237 yet, so privacy_guard_wiring.rs merged with no textual or
  semantic conflict. I ran it rather than assumed it, and separately confirmed the
  may_read/may_write counts in web.rs are still c(1, 0, 1) each — this PR
  adds a turn_reach call site, which the census does not count.
• Clippy's only finding is pre-existing and in a file this PR does not touch:
  clippy::too_many_lines on send_prompt_turn
  (crates/biorouter/src/agents/workspace_extension.rs:3406, 101/100 lines),
  already red on a clean main and being fixed elsewhere. No new findings.
• (Superseded by the re-verification in the second review round below.)
• serve_lifecycle's three tests fail on a bare cargo test -p biorouter-cli
  because target/debug/biorouterd does not exist — the precondition the test
  itself prints, and documented in CLAUDE.md. After
  cargo build -p biorouter-server --bin biorouterd all three pass.

🤖 Generated with Claude Code

---

Second review round — four more holes on the same surface, all closed

A second reviewer went over this PR independently. Everything they found was real, with two
corrections to how it was stated. All four are fixed in a6f0d246. The through-line: this PR's
headline was that the reflected XSS could no longer hand out the WebSocket token and drive an
agent — and there were three other routes to that same capability.

1 — HIGH: websocket_handler had no Origin check on any path

Confirmed, and it was byte-identical to main. /ws is the chat: a message on it runs a turn and
streams the reply. CORS does not govern a WebSocket handshake, so a page on any origin holding
the token could drive an agent carrying developer__shell (CSWSH). The tree's other two upgrade
sites — routes/workspace.rs:63 and routes/apps.rs:560 — have had such a check all along.

Measured before the fix, from the new test, with a real handshake:

assertion `left == right` failed: http://localhost:3000 opened the socket

On reusing origin_matches_host rather than writing a third rule

I could not call it, and the reason is a crate boundary, not a preference:
origin_matches_host lives in crates/biorouter-server/src/routes/mod.rs, and biorouter-cli
does not depend on biorouter-server (crates/biorouter-cli/Cargo.toml — no such entry).
That is SD-7: serve spawns biorouterd as a subprocess precisely so the command-line
interface does not link the server. Adding that dependency to share a six-line comparison would
undo it.

So origin_is_this_server mirrors the rule, exactly as token_matches in this same file
already mirrors the daemon's secret_matches ("Mirrors the daemon's secret_matches
(biorouter-server auth.rs)", web.rs:151) for the identical reason. If the two ever need to be one
symbol, the move is into the biorouter core library both already depend on — noted in the doc
comment and in SD-13.

I read #233 (fix/qa-d-f7-ws-same-origin) and took its shape, not the older one. Specifically
its ruling that is_local_origin is out of socket gates — "is_local_origin is the CORS rule
now and nothing else; do not hand it back to a socket". So mine is that rule's strict core with
neither of its exceptions, and each absence is deliberate:

• No is_local_origin. Beyond QA-D F7: the WebSocket origin gates are a real same-origin test #233's ruling, here it would re-open finding 4 below by
  admitting a page on localhost:3000.
• No file://, no declared-renderer origin. Those exist for the Electron renderer, which
  reaches the daemon from another local origin. This server serves its own page from its own origin
  and has no such client, so an opaque origin is refused like any other.

A client sending no Origin is still let past, as the daemon's gates let one past: non-browser
client, token still guards it.

Correction to how the finding was stated

The composed attack as described — "a foreign page reaches chats this server started", via
started_here plus the guessable <day>_<N> id — does not complete, once finding 4 lands. A
foreign page can still cause GET / to create sessions (CORS never stopped requests being sent,
only responses being read) and can guess their ids, but it cannot read the redirect's Location
cross-origin, and after finding 4 it cannot read the page carrying the token either. The socket
gate is therefore the second independent lock, not the only one — which is exactly why both
belong here: they fail independently. I am implementing it because the divergence from the other
two upgrade sites is real and the layering is worth having, not because the path was open.

2 — The token check was skipped when --auth-token was set

Confirmed, and "just make it unconditional" is a trap that I have to flag explicitly, because
taking the instruction literally would have made things worse while looking like a fix.

handle_web made ws_token = String::new() in exactly the mode where the check was skipped —
so the skip was load-bearing, not an oversight. And token_matches("", "") is true. Deleting the
if without touching the generation would have made query.token.as_deref().unwrap_or("") match
the empty expected token, admitting every socket, unauthenticated, while reading as a
tightening in review.

What the branch was for, established before removing it: in --auth-token mode the handshake
is authenticated by auth_middleware (the layer applies to /ws), and the page could not put a
token in the query because there was none to put. That reasoning was sound; the empty-string
generation it relied on was the landmine.

The fix, therefore, is three things and not one: generation is unconditional (a v4 UUID in both
modes, so the page always has one to send and both modes gain a second lock on top of the
middleware), the check is unconditional, and an empty expected token is refused outright so the
landmine cannot be re-armed. Pinned by a test that asserts token_matches("", "") is true — i.e.
that documents why the empty case must never reach the handler — and then drives a server with an
empty token and asserts it refuses both ?token= absent and ?token= empty.

3 — --auth-token "" satisfied the network guard

Confirmed and fixed at parse time, with one correction: it does not literally admit everyone. It
admits anyone who sends Authorization: Bearer with nothing after it (or Basic decoding to
x:); a request with no Authorization header still gets 401. The practical upshot is the same
and arguably worse for being non-obvious: --host 0.0.0.0 --auth-token "" binds to every interface
behind a trivially guessable credential, past the one check whose entire job is to insist on
protection (validate_network_auth tested auth_token.is_none(), and Some("") is not None).

cli.rs's new parse_auth_token refuses an empty or whitespace-only value at argument-parse time.
validate_network_auth also treats one as absent, because handle_web is a public function and
the guard must not depend on its one caller having been careful.

4 — The CORS token-lift (from the first review round)

Closed, as asked, the narrowest way that is still honest — and my own description was the argument
for closing it, so thank you for pushing back.

build_cors_layer allow-listed http://localhost:3000, http://127.0.0.1:3000 and this server's
own origin whenever no --auth-token was passed. Without a token the middleware lets everything
through, so a cross-origin fetch of /session/… that the browser permits reads the page, and
data-ws-token is in it.

The grant's shape is worth recording, because it looks harmless until the port moves. --port
defaults to 3000, so on a default run all three entries are this server and the allowance
means nothing. On any other port it hands http://…:3000 — a frontend dev server, or a page the
operator was talked into opening — read access to a chat page on, say, :8080. --port 8080 is one
of this command's documented invocations, and scripts/test_web.sh uses exactly it.

I took option 1 and did not add an opt-in flag, because there is nothing left to opt into. The
two routes a cross-origin browser client could have wanted, /api/sessions and
/api/sessions/{id}, are the ones this PR's first commit deleted. What remains is the page,
/static/*, a static /api/health, and the WebSocket, which CORS does not govern. Nothing in the
repository reads any of it from another origin — scripts/test_web.sh uses curl, which ignores
CORS entirely. An opt-in would therefore be an opt-in to the token leak and to nothing else. The
layer is kept rather than deleted so a preflight gets a definite answer from code that says why.

Measured before the fix: http://localhost:3000 is told it may read the page holding the token.

Sequencing: #264 must merge first

Added to the PR body as asked, and to SD-13. This PR's reach gate keys on a session id; #264
establishes that ids were being reissued after a delete and adds a high-water allocator. Until
#264 lands, a reissued id defeats the gate. Not a defect in this PR, but the merge order is not
interchangeable: #264, then this.

New tests (5), all fail-before

Test	Measured failure before the fix
a_handshake_from_another_origin_is_refused_even_with_the_right_token	http://localhost:3000 opened the socket
the_socket_token_is_required_on_every_path_and_never_empty	the empty-token server admitted an empty ?token=
no_other_origin_may_read_the_page_that_carries_the_ws_token	http://localhost:3000 is told it may read the page holding the token
the_page_still_serves_the_origin_it_is_served_from	— the control: a cross-origin refusal is worthless if it also broke the page
an_origin_is_this_server_only_when_it_matches_this_request_s_host	— unit corners, incl. null / file:// / a different port
an_empty_auth_token_is_refused_and_does_not_satisfy_the_network_guard	— parse_auth_token does not exist before the fix

Two notes on how these are driven, both of which cost me a wrong first attempt:

• The handshake tests send a real WebSocket handshake over a raw socket. WebSocketUpgrade is
  extracted before the handler body runs, so a request without the upgrade headers is rejected
  400 by the extractor and never reaches the rule under test.
• handshake does not reuse the suite's raw_request, which sends Connection: close and reads to
  EOF: a successful 101 leaves the connection open, so reading to EOF would hang. One bounded read.

The CORS test asserts on the header, not the body — the token is still in the page, because the
page needs it. What must not happen is a browser being told another origin may read that page.

Re-verification after this round

cargo test -p biorouter --test privacy_guard_wiring --test privacy_capability   4 + 3 passed, exit 0
cargo test -p biorouter-cli (isolated HOME)                          450 + 3 + 3 passed, exit 0
cargo test -p biorouter-cli --lib -- commands::web                             22 passed
cargo fmt --check                                                              clean
./scripts/clippy-lint.sh                                    1 pre-existing finding, same site

Clippy's only finding is still too_many_lines on send_prompt_turn
(crates/biorouter/src/agents/workspace_extension.rs:3406, 101/100) — red on a clean main, in a
file this PR does not touch.

⚠ One trap worth recording for whoever runs these: session::tests::cli_plan_mode_refuses_to_ship_a_private_transcript_elsewhere
fails without the isolated HOME (it reads the real config). It passes under the incantation
above, and under the full suite. I hit it on an ad-hoc run and it reads exactly like a regression.

Still deliberately not closed

• The narrow turn_reach → agent.reply TOCTOU, as agreed: same shape as the daemon's
  session_reach, Gate B still governs the model binding, a maintainer's call.
• Deleting biorouter web outright. Still my recommendation, stated as strongly as I can:
  every hole in both review rounds lives in a page nothing else uses, on a command biorouter serve
  superseded. Eight distinct defects have now been found on this one surface across two reviews.
  That is not a page that has been hardened; it is a page that keeps producing findings because
  nobody uses it enough to notice. Deleting it closes all of them permanently and retires SD-13's
  whole surface. I am not taking that decision, and I understand why you are not either.