From d8cd7be75018eb904003b15b5a2ffe97c2c8cbca Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 13:59:59 -0700 Subject: [PATCH 1/3] fix(web): biorouter web serves no transcripts, and gates the turn that reaches a chat (#56) `biorouter web`, the deprecated page `serve` superseded, answered `GET /api/sessions` with every user and scheduled chat on the machine and `GET /api/sessions/{id}` with any chat's full transcript, private ones included, behind no reach check. Without `--auth-token` (all a loopback bind requires) the auth middleware lets every request through; with one, the token sits in the process's argv, readable by any process of the same user. Both routes are removed rather than gated. The page never read the list, and read the transcript only for a message count and a tab title. The page's WebSocket was the larger way in. A message naming a private chat started elsewhere ran a turn there (Gate B rebinds the shared agent to the private model that chat's row names) and streamed the reply to whoever held the socket: the daemon's `POST /reply` under another name. `turn_reach` now judges it before anything touches the chat. The page is a public caller, except in a chat this server started, where it holds the tier of the provider the server was started on, so a private-model operator's own chat survives the first reply ratcheting it private. A private chat and an id that names nothing get one identical refusal. With privacy tiers off the gate is inert. Recorded as SD-13 in docs/deployment/serve-decisions.md. The wiring census gains the gate's may_read and may_write call sites. --- Cargo.lock | 1 + crates/biorouter-cli/Cargo.toml | 3 + crates/biorouter-cli/src/commands/web.rs | 481 ++++++++++++++++-- crates/biorouter-cli/static/script.js | 37 -- .../biorouter/tests/privacy_guard_wiring.rs | 33 +- docs/cli/command-reference.md | 4 +- docs/deployment/README.md | 2 +- docs/deployment/serve-decisions.md | 77 ++- 8 files changed, 540 insertions(+), 98 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e7e890ea9..8ef44c3f6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1172,6 +1172,7 @@ dependencies = [ "tikv-jemalloc-ctl", "tikv-jemallocator", "tokio", + "tokio-tungstenite", "tokio-util", "tower-http 0.5.2", "tracing", diff --git a/crates/biorouter-cli/Cargo.toml b/crates/biorouter-cli/Cargo.toml index bf20acac4..0472a6da0 100644 --- a/crates/biorouter-cli/Cargo.toml +++ b/crates/biorouter-cli/Cargo.toml @@ -99,6 +99,9 @@ serial_test = { workspace = true } # The workspace's process-wide environment lock, so env-mutating tests here # exclude every other one rather than only the `#[serial]` ones. env-lock = { workspace = true } +# A WebSocket client, so `commands::web`'s tests can drive the page's socket the +# way the page does. The version axum's `ws` feature already locks. +tokio-tungstenite = "0.28.0" # Issue #56 DR-20 / Task 55. `biorouter session declassify ` raises the OS # authentication prompt, so its TESTS would type a real password on every run # without a stand-in. diff --git a/crates/biorouter-cli/src/commands/web.rs b/crates/biorouter-cli/src/commands/web.rs index 595047715..b7fc50b2d 100644 --- a/crates/biorouter-cli/src/commands/web.rs +++ b/crates/biorouter-cli/src/commands/web.rs @@ -15,10 +15,13 @@ use base64::Engine; use biorouter::agents::turn_abort::TurnFailed; use biorouter::agents::{Agent, AgentEvent}; use biorouter::conversation::message::Message as BioRouterMessage; -use biorouter::session::session_manager::SessionType; +use biorouter::privacy::visibility::{may_read, may_write}; +use biorouter::privacy::{ProviderTier, SessionClassification}; +use biorouter::session::session_manager::{SessionManager, SessionType}; use futures::{sink::SinkExt, stream::StreamExt}; use serde::{Deserialize, Serialize}; use serde_json::Value; +use std::collections::HashSet; use std::{net::ToSocketAddrs, sync::Arc}; use tokio::sync::{Mutex, RwLock}; use tower_http::cors::{AllowOrigin, Any, CorsLayer}; @@ -33,6 +36,11 @@ struct AppState { cancellations: CancellationStore, auth_token: Option, ws_token: String, + /// The chats this server started, through `GET /`. See [`page_capability`]. + started_here: Arc>>, + /// The tier of the provider this server was started on, read once, before + /// the first turn. See [`page_capability`]. + server_tier: ProviderTier, } #[derive(Serialize, Deserialize)] @@ -190,7 +198,9 @@ fn get_provider_and_model() -> (String, String) { (provider_name, model) } -async fn create_agent(provider_name: &str, model: &str) -> Result { +/// The agent every chat on this server shares, and the tier of the provider it +/// was started on. +async fn create_agent(provider_name: &str, model: &str) -> Result<(Agent, ProviderTier)> { let model_config = biorouter::model::ModelConfig::new(model)?; let agent = Agent::new(); @@ -205,6 +215,9 @@ async fn create_agent(provider_name: &str, model: &str) -> Result { .await?; let provider = biorouter::providers::create(provider_name, model_config).await?; + // Read here, not off the agent later: the agent is shared by every chat, and + // a turn in a chat whose row names another provider rebinds it (Gate B). + let server_tier = provider.tier(); agent.update_provider(provider, &init_session.id).await?; let enabled_configs = biorouter::config::get_enabled_extensions(); @@ -214,7 +227,7 @@ async fn create_agent(provider_name: &str, model: &str) -> Result { } } - Ok(agent) + Ok((agent, server_tier)) } fn build_cors_layer(auth_token: &Option, host: &str, port: u16) -> CorsLayer { @@ -236,14 +249,21 @@ fn build_cors_layer(auth_token: &Option, host: &str, port: u16) -> CorsL } } +/// ⚠ **There is no `/api/sessions` route, and there must not be one again.** +/// `GET /api/sessions` listed every user and scheduled chat on the machine (id, +/// title, working directory), and `GET /api/sessions/{id}` returned any chat's +/// full transcript, private ones included, behind no reach check at all — and +/// behind no credential either unless `--auth-token` was passed, which puts the +/// token in this process's argv. The page read one of the two, for a message +/// count and a tab title. Both were removed rather than gated (issue #56, SD-13 +/// in `docs/deployment/serve-decisions.md`); a chat is reached through this +/// server only by sending it a message, which [`turn_reach`] judges. fn build_router(state: AppState, cors_layer: CorsLayer) -> Router { Router::new() .route("/", get(serve_index)) .route("/session/{session_name}", get(serve_session)) .route("/ws", get(websocket_handler)) .route("/api/health", get(health_check)) - .route("/api/sessions", get(list_sessions)) - .route("/api/sessions/{session_id}", get(get_session)) .route("/static/{*path}", get(serve_static)) .layer(middleware::from_fn_with_state( state.clone(), @@ -273,7 +293,7 @@ pub async fn handle_web( crate::logging::setup_logging(Some("biorouter-web"), None)?; let (provider_name, model) = get_provider_and_model(); - let agent = create_agent(&provider_name, &model).await?; + let (agent, server_tier) = create_agent(&provider_name, &model).await?; let ws_token = if auth_token.is_none() { uuid::Uuid::new_v4().to_string() @@ -286,6 +306,8 @@ pub async fn handle_web( cancellations: Arc::new(RwLock::new(std::collections::HashMap::new())), auth_token: auth_token.clone(), ws_token, + started_here: Arc::default(), + server_tier, }; let cors_layer = build_cors_layer(&auth_token, &host, port); @@ -333,6 +355,7 @@ async fn serve_index( ) .await .map_err(|err| (http::StatusCode::INTERNAL_SERVER_ERROR, err.to_string()))?; + state.started_here.write().await.insert(session.id.clone()); let redirect_url = if let Some(query) = uri.query() { format!("/session/{}?{}", session.id, query) @@ -392,50 +415,111 @@ async fn health_check() -> Json { })) } -async fn list_sessions(State(state): State) -> Json { - match state.agent.config.session_manager.list_sessions().await { - Ok(sessions) => { - let mut session_info = Vec::new(); - - for session in sessions { - session_info.push(serde_json::json!({ - "name": session.id, - "path": session.id, - "description": session.name, - "message_count": session.message_count, - "working_dir": session.working_dir - })); - } - Json(serde_json::json!({ - "sessions": session_info - })) - } - Err(e) => Json(serde_json::json!({ - "error": e.to_string() - })), +/// What a page is told when its message names a chat it may not reach. +/// +/// ⚠ **One sentence for "that chat is private" and for "there is no chat with +/// that id", deliberately**, for the reason the daemon's `SESSION_OUT_OF_REACH` +/// gives (`routes/session_reach.rs`): a refusal that told the two apart would +/// enumerate the machine's private chats one id at a time. It is fixed text and +/// names nothing about the chat, so the two answers are equal byte for byte. +/// +/// It states the page's own situation and forecloses the retry. The condition it +/// names cannot be met for a chat that already exists elsewhere, so it hands a +/// reader no way around itself; the one way through is the desktop app, where a +/// person can show they are at the keyboard. +const CHAT_OUT_OF_REACH: &str = + "That chat is private, or there is no chat with that id, and the two answers are \ + deliberately the same so that nothing about the chat is disclosed. `biorouter web` cannot \ + tell which model or which person is sending these messages, so it opens a private chat only \ + when it started that chat itself and runs a private model. Your message was not sent and \ + nothing was read; sending it again will be refused the same way. To continue a private \ + chat, open it in the Biorouter desktop app."; + +/// The capability a page brings to the chat its message names. +/// +/// ⚠ **Nothing on the socket says who is on the other end of it.** The server's +/// one credential proves nothing about that either: the WebSocket token is served +/// by `/session/{name}` to anyone who can reach the port, and `--auth-token` sits +/// in this process's argv, where any process of the same user reads it (AR-11 in +/// `docs/security/privacy-tiers-execution-plan.md`). So a page is a PUBLIC +/// caller, which is also how the daemon resolves a caller that states no +/// capability (`session_reach::caller_capability`). +/// +/// The exception is a chat this server started itself, which the page reaches at +/// the tier of the provider this server was started on. Without it a server on a +/// private model would give one reply per chat: that reply ratchets the chat to +/// private, and the next message would be refused. On a public model the +/// exception changes nothing, because the page is Public for every chat — so a +/// chat it started that was taken private somewhere else is refused like any +/// other. +fn page_capability(started_here: bool, server_tier: ProviderTier) -> ProviderTier { + if started_here { + server_tier + } else { + ProviderTier::Public } } -async fn get_session( - State(state): State, - axum::extract::Path(session_id): axum::extract::Path, -) -> Json { - match state - .agent - .config - .session_manager - .get_session(&session_id, true) - .await - { - Ok(session) => Json(serde_json::json!({ - "metadata": session, - "messages": session.conversation.unwrap_or_default().messages() - })), - Err(e) => Json(serde_json::json!({ - "error": e.to_string() - })), + +/// May a page with this capability run a turn in a chat in this state? +/// +/// `target` is the chat's classification, or `None` when its row could not be +/// read — no such chat, a deleted one, a store error — which is answered exactly +/// as a private chat is. `enforced` is DR-15's master switch, taken as an +/// argument so that "the switch is off" is a corner the tests drive; with it off +/// the gate is inert, like every other. +fn refuse_turn_unless_reachable( + enforced: bool, + capability: ProviderTier, + target: Option, +) -> Result<(), &'static str> { + if !enforced { + return Ok(()); + } + let target = target.unwrap_or(SessionClassification::Private); + // A turn reads the whole conversation into the model and writes into it, so + // it asks both verbs, as `workspace_send_prompt` does. They coincide today; + // asking both keeps a later narrowing of either from being skipped here. + if may_read(capability, target) && may_write(capability, target) { + Ok(()) + } else { + Err(CHAT_OUT_OF_REACH) } } +/// The gate on the one door into a chat this server keeps: a WebSocket message, +/// which runs a turn in whichever chat it names. +/// +/// ⚠ **It is the same door as the daemon's `POST /reply`, and it was open.** A +/// message naming a private chat started anywhere else ran a turn there — Gate B +/// rebinds the shared agent to the private model that chat's row names — and the +/// reply, which can quote the whole conversation, streamed back to whoever held +/// the socket. Removing `GET /api/sessions/{id}` alone would have closed the +/// smaller door and left this one. +/// +/// Called before anything touches the chat, as `session_reach` is. The row is +/// read metadata-only, so resolving the tier never loads the transcript this may +/// be about to refuse. +async fn turn_reach( + manager: &SessionManager, + started_here: &RwLock>, + server_tier: ProviderTier, + session_id: &str, +) -> Result<(), &'static str> { + // DR-15's master opt-out, read directly: a turn is not a tool call and has + // no sampled capability to inherit. Short-circuit before the store read. + let enforced = biorouter::privacy::privacy_tiers_enabled(); + if !enforced { + return Ok(()); + } + let capability = page_capability(started_here.read().await.contains(session_id), server_tier); + let target = manager + .get_session(session_id, false) + .await + .ok() + .map(|session| session.privacy_tier); + refuse_turn_unless_reachable(enforced, capability, target) +} + #[derive(Deserialize)] struct WsQuery { token: Option, @@ -505,6 +589,18 @@ async fn handle_user_message( sender: Arc>>, state: &AppState, ) { + if let Err(refusal) = turn_reach( + &state.agent.config.session_manager, + &state.started_here, + state.server_tier, + &session_id, + ) + .await + { + send_error(&sender, refusal).await; + return; + } + let agent = state.agent.clone(); let session_id_clone = session_id.clone(); @@ -595,10 +691,8 @@ async fn process_message_streaming( let session = agent .config .session_manager - .get_session(&session_id, true) + .get_session(&session_id, false) .await?; - let mut messages = session.conversation.unwrap_or_default(); - messages.push(user_message.clone()); let session_config = SessionConfig { id: session.id.clone(), @@ -795,7 +889,23 @@ async fn send_error( #[cfg(test)] mod tests { - use super::token_matches; + use super::*; + use biorouter::agents::AgentConfig; + use biorouter::config::permission::PermissionManager; + use biorouter::config::BioRouterMode; + use serde_json::json; + use std::net::SocketAddr; + use std::time::Duration; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio_tungstenite::tungstenite::Message as Frame; + + const TIERS: [ProviderTier; 2] = [ProviderTier::Public, ProviderTier::Private]; + const TARGETS: [Option; 3] = [ + Some(SessionClassification::Public), + Some(SessionClassification::Private), + None, + ]; + const WS_TOKEN: &str = "test-ws-token"; #[test] fn token_match_is_exact_and_length_checked() { @@ -807,4 +917,277 @@ mod tests { assert!(!token_matches("", "abc123")); assert!(token_matches("", "")); } + + /// Nothing on the socket names a model, so a page is a public caller — in + /// every chat but the ones its own server started, where it has the tier of + /// the model it has been talking to. + #[test] + fn a_page_is_a_public_caller_outside_the_chats_its_server_started() { + for server_tier in TIERS { + assert_eq!(page_capability(false, server_tier), ProviderTier::Public); + assert_eq!(page_capability(true, server_tier), server_tier); + } + } + + /// The rule at every corner, with the switch on. An unreadable row is judged + /// as a private one, so a public page is refused both. + #[test] + fn a_turn_reaches_a_chat_only_at_the_tier_the_page_holds() { + use SessionClassification::{Private, Public}; + #[rustfmt::skip] + let cases = [ + // capability target admitted + (ProviderTier::Public, Some(Public), true), + (ProviderTier::Public, Some(Private), false), + (ProviderTier::Public, None, false), + (ProviderTier::Private, Some(Public), true), + (ProviderTier::Private, Some(Private), true), + (ProviderTier::Private, None, true), + ]; + for (capability, target, admitted) in cases { + assert_eq!( + refuse_turn_unless_reachable(true, capability, target).is_ok(), + admitted, + "a {capability:?} page naming a chat classified {target:?}" + ); + } + } + + /// DR-15: with privacy tiers off, this gate refuses nothing, like every other. + #[test] + fn with_privacy_tiers_off_the_gate_refuses_nothing() { + for capability in TIERS { + for target in TARGETS { + assert_eq!( + refuse_turn_unless_reachable(false, capability, target), + Ok(()), + "{capability:?} / {target:?}" + ); + } + } + } + + /// A page cannot tell "no such chat" from "a private chat" by the answer. + /// Asserted as equality at every capability rather than as each answer being + /// vague, because a vagueness check passes an implementation that adds one + /// helpful clause to the branch it can tell apart. + #[test] + fn no_such_chat_and_a_private_chat_are_the_same_refusal() { + for capability in TIERS { + assert_eq!( + refuse_turn_unless_reachable(true, capability, None), + refuse_turn_unless_reachable( + true, + capability, + Some(SessionClassification::Private) + ), + "a {capability:?} page can tell a missing chat from a private one" + ); + } + assert_eq!( + refuse_turn_unless_reachable(true, ProviderTier::Public, None), + Err(CHAT_OUT_OF_REACH) + ); + } + + /// The page and the router agree: the page asks for no chat list and no + /// transcript, so the routes that served them could go. + #[test] + fn the_page_asks_for_no_chat_list_and_no_transcript() { + let page = include_str!("../../static/script.js"); + assert!( + !page.contains("/api/sessions"), + "the page fetches a route this server no longer has" + ); + } + + /// A server as `handle_web` builds one, on an ephemeral port and over its + /// own store, minus the provider. A message the gate admits reaches + /// `process_message_streaming` and is answered "not configured", which is how + /// these tests tell an admitted message from a refused one without a model. + struct TestServer { + addr: SocketAddr, + manager: Arc, + _store: tempfile::TempDir, + } + + impl TestServer { + async fn start(server_tier: ProviderTier) -> Self { + assert!( + biorouter::privacy::privacy_tiers_enabled(), + "these tests drive the enforced gate, and something in this binary turned the \ + master switch off" + ); + let store = tempfile::tempdir().unwrap(); + let manager = Arc::new(SessionManager::new(store.path().to_path_buf())); + let agent = Agent::with_config(AgentConfig::new( + Arc::clone(&manager), + PermissionManager::instance(), + None, + BioRouterMode::Auto, + )); + let state = AppState { + agent: Arc::new(agent), + cancellations: Arc::default(), + auth_token: None, + ws_token: WS_TOKEN.to_string(), + started_here: Arc::default(), + server_tier, + }; + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let app = build_router(state, build_cors_layer(&None, "127.0.0.1", addr.port())); + tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); + Self { + addr, + manager, + _store: store, + } + } + + /// `GET path` over a plain socket: the status code and any `Location`. + /// Hand-rolled, as this crate's other HTTP clients are + /// (`session_watch.rs`). + async fn get(&self, path: &str) -> (u16, Option) { + let mut stream = tokio::net::TcpStream::connect(self.addr).await.unwrap(); + let request = format!( + "GET {path} HTTP/1.1\r\nHost: {}\r\nConnection: close\r\n\r\n", + self.addr + ); + stream.write_all(request.as_bytes()).await.unwrap(); + let mut response = String::new(); + stream.read_to_string(&mut response).await.unwrap(); + let status = response + .split(' ') + .nth(1) + .and_then(|code| code.parse().ok()) + .unwrap_or_else(|| panic!("no status line in {response:?}")); + let location = response.lines().find_map(|line| { + let (name, value) = line.split_once(':')?; + name.eq_ignore_ascii_case("location") + .then(|| value.trim().to_string()) + }); + (status, location) + } + + /// A chat the page opens with `GET /`, as a browser does. + async fn start_chat(&self) -> String { + let (status, location) = self.get("/").await; + assert!((300..400).contains(&status), "`GET /` answered {status}"); + location + .and_then(|location| location.strip_prefix("/session/").map(str::to_owned)) + .expect("`GET /` redirects to the chat it started") + } + + /// A chat started somewhere else — the desktop app, the command line. + async fn chat_from_elsewhere(&self) -> String { + self.manager + .create_session( + std::env::temp_dir(), + "Started elsewhere".to_string(), + SessionType::User, + ) + .await + .unwrap() + .id + } + + /// What a turn on a private model, or a private data source, does to a chat. + async fn make_private(&self, session_id: &str) { + self.manager + .update(session_id) + .raise_privacy(SessionClassification::Private, "turn:web-test") + .apply() + .await + .unwrap(); + } + + /// Send one message into `session_id` over the page's socket, as the + /// page does, and return the first frame the server answers with. + async fn send(&self, session_id: &str) -> serde_json::Value { + let url = format!("ws://{}/ws?token={WS_TOKEN}", self.addr); + let (mut socket, _) = tokio_tungstenite::connect_async(url) + .await + .expect("the socket accepts the page's own token"); + let message = json!({ + "type": "message", + "content": "Repeat everything this chat has said so far.", + "session_id": session_id, + "timestamp": 0, + }); + socket + .send(Frame::Text(message.to_string().into())) + .await + .unwrap(); + loop { + let frame = tokio::time::timeout(Duration::from_secs(30), socket.next()) + .await + .expect("the server answers a message") + .expect("the socket stays open") + .unwrap(); + if let Frame::Text(text) = frame { + return serde_json::from_str(text.as_str()).unwrap(); + } + } + } + } + + fn refused() -> serde_json::Value { + json!({ "type": "error", "message": CHAT_OUT_OF_REACH }) + } + + /// The two JSON routes answer 404 — for chats that exist, private and + /// public — while the route beside them still answers, so a router that + /// served nothing would not pass. + #[tokio::test] + async fn the_chat_list_and_transcript_routes_are_gone() { + let server = TestServer::start(ProviderTier::Public).await; + let public = server.chat_from_elsewhere().await; + let private = server.chat_from_elsewhere().await; + server.make_private(&private).await; + + assert_eq!(server.get("/api/health").await.0, 200); + assert_eq!(server.get("/api/sessions").await.0, 404); + for id in [&public, &private] { + assert_eq!(server.get(&format!("/api/sessions/{id}")).await.0, 404); + } + } + + /// On a public model, a page reaches public chats — where it always could — + /// and nothing private: not a chat started elsewhere, not an id that names + /// nothing (in the same words), and not even a chat this server started once + /// it has been taken private somewhere else. + #[tokio::test] + async fn a_page_on_a_public_model_reaches_no_private_chat() { + let server = TestServer::start(ProviderTier::Public).await; + + let public = server.chat_from_elsewhere().await; + assert_eq!(server.send(&public).await["type"], "response"); + + let private = server.chat_from_elsewhere().await; + server.make_private(&private).await; + assert_eq!(server.send(&private).await, refused()); + assert_eq!(server.send("19700101_0").await, refused()); + + let started = server.start_chat().await; + assert_eq!(server.send(&started).await["type"], "response"); + server.make_private(&started).await; + assert_eq!(server.send(&started).await, refused()); + } + + /// On a private model, a chat the server started keeps working after its + /// first reply ratchets it private — and a private chat started anywhere + /// else is still refused, because the page is public there. + #[tokio::test] + async fn a_page_on_a_private_model_keeps_its_own_chats_and_no_others() { + let server = TestServer::start(ProviderTier::Private).await; + + let started = server.start_chat().await; + server.make_private(&started).await; + assert_eq!(server.send(&started).await["type"], "response"); + + let elsewhere = server.chat_from_elsewhere().await; + server.make_private(&elsewhere).await; + assert_eq!(server.send(&elsewhere).await, refused()); + } } diff --git a/crates/biorouter-cli/static/script.js b/crates/biorouter-cli/static/script.js index 8a1724606..a4cfb8372 100644 --- a/crates/biorouter-cli/static/script.js +++ b/crates/biorouter-cli/static/script.js @@ -149,9 +149,6 @@ function connectWebSocket() { connectionStatus.textContent = 'Connected'; connectionStatus.className = 'status connected'; sendButton.disabled = false; - - // Check if this session exists and load history if it does - loadSessionIfExists(); }; socket.onmessage = (event) => { @@ -456,40 +453,6 @@ function sendSuggestion(text) { sendMessage(); } -// Load session history if the session exists (like --resume in CLI) -async function loadSessionIfExists() { - try { - const response = await fetch(`/api/sessions/${sessionId}`); - if (response.ok) { - const sessionData = await response.json(); - if (sessionData.messages && sessionData.messages.length > 0) { - // Remove welcome message since we're resuming - const welcomeMessage = messagesContainer.querySelector('.welcome-message'); - if (welcomeMessage) { - welcomeMessage.remove(); - } - - // Display session resumed message - const resumeDiv = document.createElement('div'); - resumeDiv.className = 'message system-message'; - resumeDiv.innerHTML = `Session resumed: ${sessionData.messages.length} messages loaded`; - messagesContainer.appendChild(resumeDiv); - - // Update page title with session description if available - if (sessionData.metadata && sessionData.metadata.description) { - document.title = `biorouter chat - ${sessionData.metadata.description}`; - } - - messagesContainer.scrollTop = messagesContainer.scrollHeight; - } - } - } catch (error) { - console.log('No existing session found or error loading:', error); - // This is fine - just means it's a new session - } -} - - // Event listeners sendButton.addEventListener('click', sendMessage); diff --git a/crates/biorouter/tests/privacy_guard_wiring.rs b/crates/biorouter/tests/privacy_guard_wiring.rs index bb42dea5c..1a9a71aaf 100644 --- a/crates/biorouter/tests/privacy_guard_wiring.rs +++ b/crates/biorouter/tests/privacy_guard_wiring.rs @@ -164,8 +164,18 @@ const REGISTRY: &[Guard] = &[ ident: "may_read", defined_in: VISIBILITY, decides: "READ ⇔ VIS: whether a caller of tier C may read a session classified T", - status: Status::WiredThrough("refuse_unless_readable"), + // It was `WiredThrough("refuse_unless_readable")` until `biorouter web` became + // its first caller outside this file; the in-file row below still holds. + status: Status::Wired, sites: &[ + Site { + file: "crates/biorouter-cli/src/commands/web.rs", + counts: c(1, 0, 1), + kind: SiteKind::Guard, + what: "`refuse_turn_unless_reachable`, the gate on `biorouter web`'s WebSocket: \ + a message there runs a turn in whichever chat it names, so the page must \ + be able to read that chat. Plus its import", + }, Site { file: "crates/biorouter-mcp/src/memory/mod.rs", counts: c(2, 0, 0), @@ -238,12 +248,21 @@ const REGISTRY: &[Guard] = &[ spawned, read everything else — is retired: an agent may inject into any \ conversation, and the tier is the only boundary", status: Status::Wired, - sites: &[Site { - file: "crates/biorouter/src/agents/workspace_extension.rs", - counts: c(1, 0, 0), - kind: SiteKind::Guard, - what: "the shared writable adapter used by send_prompt, set_tools and close", - }], + sites: &[ + Site { + file: "crates/biorouter-cli/src/commands/web.rs", + counts: c(1, 0, 1), + kind: SiteKind::Guard, + what: "`refuse_turn_unless_reachable`, the write half: a `biorouter web` message \ + is written into the chat it names. Plus its import", + }, + Site { + file: "crates/biorouter/src/agents/workspace_extension.rs", + counts: c(1, 0, 0), + kind: SiteKind::Guard, + what: "the shared writable adapter used by send_prompt, set_tools and close", + }, + ], }, Guard { ident: "requires_first_crossing_approval", diff --git a/docs/cli/command-reference.md b/docs/cli/command-reference.md index 78c6e3592..683ee4a41 100644 --- a/docs/cli/command-reference.md +++ b/docs/cli/command-reference.md @@ -798,7 +798,9 @@ The printed URL carries an access token as `?t=`, minted per launch and s ### web -> **Deprecated.** Use [`serve`](#serve) instead. `web` serves a minimal standalone chat page rather than the Biorouter interface, and its default port collides with `biorouterd`'s. It is kept for now and unchanged; new deployments should not use it. +> **Deprecated.** Use [`serve`](#serve) instead. `web` serves a minimal standalone chat page rather than the Biorouter interface, and its default port collides with `biorouterd`'s. It is kept for now; new deployments should not use it. +> +> Since 2026-09-11 it lists no chats and returns no transcripts, and it opens a private chat only if it started that chat itself while running a private model. Continue any other private chat in the desktop app. [SD-13](../deployment/serve-decisions.md#sd-13--biorouter-web-serves-no-transcripts-and-opens-no-private-chat-it-did-not-start) records why. Start a new session in biorouter Web, a lightweight web-based interface launched via the CLI that mirrors the desktop app's chat experience. diff --git a/docs/deployment/README.md b/docs/deployment/README.md index 9a6160499..48142ac44 100644 --- a/docs/deployment/README.md +++ b/docs/deployment/README.md @@ -26,7 +26,7 @@ any deployment live in [configuration](../configuration/environment-variables.md | [Headless Linux deployment](headless-linux.md) | Running `biorouter serve` as a long-lived service on a Linux host with no graphical desktop: the CLI-only packages, the systemd unit, migrating secrets onto the host, and network exposure. | | [Reaching a private chat from a script](programmatic-session-access.md) | The `X-Caller-Provider` header: how a monitoring dashboard, a CI job or a shell script reads and follows a **private** conversation over the HTTP API, what the header is not (it is not authentication), and which routes honour it. | | [How browser-served Biorouter is built](serve-architecture.md) | Developer-facing architecture: what the daemon does with a web directory, how a browser is authenticated, and what the retired front door was replaced by. | -| [Decisions behind `biorouter serve`](serve-decisions.md) | The nine decision records governing the serving path — why a browser session cannot change its model, why the bind defaults to loopback, why the standalone binary was retired, and why the launch token is reusable until the daemon stops. | +| [Decisions behind `biorouter serve`](serve-decisions.md) | The ten decision records governing the serving path — why a browser session cannot change its model, why the bind defaults to loopback, why the standalone binary was retired, why the launch token is reusable until the daemon stops, and which chats the deprecated `biorouter web` may still open. | ## Related documentation diff --git a/docs/deployment/serve-decisions.md b/docs/deployment/serve-decisions.md index 24b74e3a5..b52be17fd 100644 --- a/docs/deployment/serve-decisions.md +++ b/docs/deployment/serve-decisions.md @@ -2,9 +2,9 @@ > **What this is.** The decision records governing browser-served Biorouter — why the daemon > serves the interface itself, why a browser session cannot change its model, why the -> standalone `biorouter-headless` binary was retired, and how long the launch token stays good -> for. Each record states the ruling, the alternatives it displaced, and the consequence a -> future change would have to accept. +> standalone `biorouter-headless` binary was retired, how long the launch token stays good for, +> and which chats the deprecated `biorouter web` may still open. Each record states the ruling, +> the alternatives it displaced, and the consequence a future change would have to accept. > **Status:** Current. > **Audience:** developers working on the daemon, the CLI, or release packaging; agents making > changes anywhere near the serving path. @@ -284,6 +284,77 @@ behaviour, so changing it means revisiting this record, not making a quiet fix. --- +## SD-13 — `biorouter web` serves no transcripts, and opens no private chat it did not start + +**Ruling (2026-09-11).** The deprecated `biorouter web` command no longer serves +`GET /api/sessions` or `GET /api/sessions/{id}`. The one way into a chat it keeps — a WebSocket +message, which runs a turn in whichever chat it names — is judged before anything touches that +chat. The page is a **public** caller, except in a chat this server started itself through +`GET /`, where it holds the tier of the provider the server was started on. A chat it may not +reach is refused with one sentence, identical for a private chat and for an id that names +nothing. + +**Why.** Both routes predate the privacy tiers (issue #56) and never learned them. The list +returned every user and scheduled chat on the machine with its title and working directory; the +transcript route returned any chat's full conversation, private ones included. The only +credential in front of them was the page's own, and it held nothing back: + +- **Without `--auth-token`** — the default, and all a loopback bind requires — the auth + middleware lets every request through, so anything that can reach the port reads every chat. + A model with a shell does it with `curl`. +- **With `--auth-token`**, the token is a command-line argument. Any process running as the same + user reads it with `ps -axww -o args` (measured on macOS), and on Linux `/proc//cmdline` + is readable by every user unless `/proc` is mounted with `hidepid`. That is + [AR-11](../security/privacy-tiers-execution-plan.md#ar-11--amended-by-dr-17--the-daemons-own-api-secret-is-recoverable)'s + recovery of the daemon's secret, through a channel that is more open than the environment. + +The page read the transcript route for a message count and a tab title, and never read the list. +Gating them would have kept two routes nobody needed, so both were deleted. + +The WebSocket could not be deleted, because it is the chat; it is gated instead. It was the +larger way in, and it was open as well. A message naming a private chat started anywhere else +ran a turn there — Gate B rebinds the one shared agent to the private model that chat's row +names — and streamed the reply, which can quote the whole conversation, back to whoever held +the socket. That is the daemon's `POST /reply` under another name, and `/reply` heads the +daemon's gated list because it dominates every read route. Deleting the transcript route alone +would have closed the smaller way in and left this one. + +**How the page's capability is decided.** Nothing on the socket names the model or the person on +the other end, so the page is a public caller, which is also how the daemon treats a caller that +states no capability. A chat this server started is the exception, reached at the tier of the +provider the server was started on. Without it, a server on a private model would give one reply +per chat: the first reply ratchets the chat to private, and the next message would be refused. +On a public model the exception changes nothing, so a chat this server started that was taken +private somewhere else is refused like any other. + +**Displaced alternatives.** + +- *Gate the two routes: list public chats only, and refuse a private transcript.* Rejected. It + keeps a list nothing reads and a transcript the page never showed, and every route kept is one + more place the reach rule has to be right. +- *Give the page the server's tier for every chat.* Rejected. On a private model, any process + that can reach the port — a public-model chat's shell included — would reach every private + chat on the machine without stating anything. The daemon's residual at least requires the + caller to name a private provider. +- *Refuse every private chat.* Rejected. It breaks the command on the second message of every + chat for exactly the operator who chose a private model. + +**What this is NOT.** It is not authentication. The page's credential is still within any local +process's reach — served to whoever can reach the port without `--auth-token`, read from argv +with it — so a local process can still drive public chats and the chats this server started, as +it could before; issue #47 is unchanged. Nothing that was refused before is permitted now: the +change removes two routes and refuses turns, and grants nothing. + +**Consequence to accept.** A chat is known as started here only for the life of the process. +After a restart it counts as started elsewhere, and a private one must be continued in the +desktop app. The page also stops showing "Session resumed: N messages loaded", because that +count came from the transcript route. Implemented in `crates/biorouter-cli/src/commands/web.rs` +(`turn_reach`, `page_capability` and `refuse_turn_unless_reachable`) and pinned by that module's +tests, three of which drive the real router and WebSocket handler over a socket, against a real +session store. + +--- + ## Related documentation - [Architecture of the serving path](serve-architecture.md) — how the decisions above are built. From 640cfe64c6ebf9c80897d9b32a4105892825c3b4 Mon Sep 17 00:00:00 2001 From: Wanjun Gu Date: Fri, 11 Sep 2026 19:36:24 -0700 Subject: [PATCH 2/3] fix(web): the chat page stops reflecting the URL into script context (#56) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /session/{name}` wrote the chat's name straight into an inline ` `session_name` is a path segment, so it is whatever the sender typed, and on the loopback bind that needs no `--auth-token` there is no credential in front of it. A `'` ended the string literal and a `` ended the element. Measured, verbatim, from the pre-fix handler: "'&'; … On this page that is not defacement. The injected script runs on the server's own origin, reads the WebSocket token out of the same document, opens `/ws` with it, and sends a message to an agent that holds `developer__shell`. WebSockets are not subject to the same-origin policy, so that token is the only thing between a drive-by page and the socket, and the injection is handed it. The two boot values now leave script context entirely: they are written as attributes on a `
` and read back through `dataset`. HTML-escaping a `"; + +/// What this page is allowed to do, sent as a header rather than a `` so +/// that injected markup cannot appear above it and displace it. +/// +/// `script-src 'self'` is the half that earns its keep: with it, an injected +/// ` +/// ``` +/// +/// where a `'` ended the string literal and `` ended the element, so +/// `GET /session/` ran the sender's code. On this +/// page that is not defacement: the injected script runs on the server's own +/// origin, reads the WebSocket token out of the very document it was injected +/// into, opens `/ws` with it, and sends a message to an agent holding +/// `developer__shell`. That token is the only thing standing between a drive-by +/// page and the socket — WebSockets are not subject to CORS — and the injection +/// is handed it. One link is remote code execution. +/// +/// ⚠ **The fix is to leave script context, not to escape for it.** HTML-escaping +/// a `", +) -> Response { + let html = include_str!("../../static/index.html").replace( + BOOT_ANCHOR, &format!( - "\n ", - session_name, - state.ws_token - ) + "\n {BOOT_ANCHOR}", + escape_html_attribute(&session_name), + escape_html_attribute(&state.ws_token), + ), ); - Html(html_with_session) + ( + [("content-security-policy", CONTENT_SECURITY_POLICY)], + Html(html), + ) + .into_response() } async fn serve_static(axum::extract::Path(path): axum::extract::Path) -> Response { @@ -634,11 +709,33 @@ async fn handle_user_message( }); } +/// ⚠ **Gated, even though the map it reads can only hold chats the gate already +/// admitted.** A handle lands in `cancellations` only after +/// [`handle_user_message`] passed [`turn_reach`], so a cancel naming an +/// unreachable chat could never abort anything it was not already allowed to. +/// What it *could* do is answer: the old code 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. Judging it first also makes the property this file +/// wants a flat one — **every socket message that names a chat is judged before +/// the chat is touched** — rather than a claim that has to be re-derived from +/// what else happens to be true of the map. async fn handle_cancel_message( session_id: String, sender: &Arc>>, state: &AppState, ) { + if let Err(refusal) = turn_reach( + &state.agent.config.session_manager, + &state.started_here, + state.server_tier, + &session_id, + ) + .await + { + send_error(sender, refusal).await; + return; + } + let abort_handle = { let mut cancellations = state.cancellations.write().await; cancellations.remove(&session_id) @@ -1001,6 +1098,155 @@ mod tests { ); } + /// A session name that tries to leave a double-quoted HTML attribute, and + /// the script context it used to be written into. Percent-encoded at the + /// call site because a raw `"` is not legal in a request target. + const BREAKOUT: &str = "\"'&"; + const BREAKOUT_ENCODED: &str = + "%3C%2Fscript%3E%3Cimg%20src%3Dx%20onerror%3Dalert(1)%3E%22%27%26"; + + #[test] + fn an_attribute_escape_neutralises_every_character_that_could_leave_one() { + assert_eq!( + escape_html_attribute(BREAKOUT), + "</script><img src=x onerror=alert(1)>"'&" + ); + // An `&` escaped anywhere but first would come back doubled. + assert_eq!(escape_html_attribute("<"), "&lt;"); + assert_eq!( + escape_html_attribute("plain-20260911_120000"), + "plain-20260911_120000" + ); + } + + /// ⚠ **The reflected XSS this branch closes.** `GET /session/{name}` wrote + /// the name of the chat straight into an inline `` ended the element and everything after it was parsed + /// as markup. The consequence is not cosmetic — see [`serve_session`]: the + /// injected script reads the WebSocket token out of the same document and + /// drives an agent that holds `developer__shell`. + /// + /// Asserted as **"the page has exactly the script elements its own template + /// has"** rather than as "the payload does not appear". The weaker form + /// passes an implementation that HTML-escapes inside the ``. + #[tokio::test] + async fn a_session_name_cannot_reach_script_context() { + let server = TestServer::start(ProviderTier::Public).await; + let template = include_str!("../../static/index.html"); + let response = server + .raw_get(&format!("/session/{BREAKOUT_ENCODED}")) + .await; + + assert_eq!( + response.matches("` that would + // close it, not against `onerror=` — that substring survives inside the + // escaped attribute, where it is text and not a handler. + assert!( + !response.contains(""), + "the payload kept a raw `>`, so something closed a tag:\n{response}" + ); + assert!( + response.contains(&format!( + "data-session-name=\"{}\"", + escape_html_attribute(BREAKOUT) + )), + "the name is not where the page reads it from, escaped:\n{response}" + ); + } + + /// The same route on the same server, with an ordinary name: the page still + /// gets the value it needs, unescaped once the parser has decoded it. A + /// breakout test alone passes a handler that drops the name entirely. + #[tokio::test] + async fn an_ordinary_session_name_still_reaches_the_page() { + let server = TestServer::start(ProviderTier::Public).await; + let started = server.start_chat().await; + let response = server.raw_get(&format!("/session/{started}")).await; + assert!( + response.contains(&format!("data-session-name=\"{started}\"")), + "the page cannot tell which chat it is in:\n{response}" + ); + assert!( + response.contains(&format!("data-ws-token=\"{WS_TOKEN}\"")), + "the page cannot open the socket:\n{response}" + ); + } + + /// Defence in depth behind the escape, and the two halves that have to move + /// together: a policy refusing inline script, and a template carrying none. + #[tokio::test] + async fn the_page_is_served_under_a_policy_that_refuses_inline_script() { + let server = TestServer::start(ProviderTier::Public).await; + let response = server.raw_get("/session/20260911_120000").await; + let policy = response + .lines() + .find_map(|line| { + let (name, value) = line.split_once(':')?; + name.eq_ignore_ascii_case("content-security-policy") + .then(|| value.trim().to_string()) + }) + .unwrap_or_else(|| panic!("no content-security-policy header in {response:?}")); + assert!( + policy.contains("script-src 'self'") && !policy.contains("unsafe-inline"), + "a policy that permits inline script is not one: {policy}" + ); + + let template = include_str!("../../static/index.html"); + assert!( + !template.contains("onclick="), + "the template carries an inline handler the policy above would refuse" + ); + } + + /// The page reads its boot values from attributes, not from globals an + /// inline `` used to end the element. See `serve_session` in +// crates/biorouter-cli/src/commands/web.rs for the full account. The HTML parser +// decodes entities inside an attribute, so what `dataset` hands back here is the +// value exactly as it arrived, with no byte able to escape the attribute. +function bootValue(name) { + const boot = document.getElementById('biorouter-boot'); + return (boot && boot.dataset[name]) || ''; +} + // Get session ID - either from URL parameter, injected session name, or generate new one function getSessionId() { - // Check if session name was injected by server (for /session/:name routes) - if (window.BIOROUTER_SESSION_NAME) { - return window.BIOROUTER_SESSION_NAME; + // Check if a session name was written into the page (for /session/:name routes) + const injected = bootValue('sessionName'); + if (injected) { + return injected; } - + // Check URL parameters const urlParams = new URLSearchParams(window.location.search); const sessionParam = urlParams.get('session') || urlParams.get('name'); @@ -138,7 +153,7 @@ function removeThinkingIndicator() { // Connect to WebSocket function connectWebSocket() { const protocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:'; - const token = window.BIOROUTER_WS_TOKEN || ''; + const token = bootValue('wsToken'); const wsUrl = `${protocol}//${window.location.host}/ws?token=${encodeURIComponent(token)}`; socket = new WebSocket(wsUrl); @@ -261,24 +276,32 @@ function handleToolRequest(data) { const headerDiv = document.createElement('div'); headerDiv.className = 'tool-header'; - headerDiv.innerHTML = `🔧 ${data.tool_name}`; - + // Every one of these interpolations is model-controlled: a tool name and a + // tool call's arguments are chosen by whatever the agent decided to run, and + // a prompt injection in a file or a web page reaches them. `escapeHtml` is + // adequate here and only here because every hole below sits in element + // content, never inside an attribute value — it does not escape `"`. + headerDiv.innerHTML = `🔧 ${escapeHtml(data.tool_name)}`; + const contentDiv = document.createElement('div'); contentDiv.className = 'tool-content'; - + // Format the arguments if (data.tool_name === 'developer__shell' && data.arguments.command) { contentDiv.innerHTML = `
${escapeHtml(data.arguments.command)}
`; } else if (data.tool_name === 'developer__text_editor') { const action = data.arguments.command || 'unknown'; const path = data.arguments.path || 'unknown'; - contentDiv.innerHTML = `
action: ${action}
`; + contentDiv.innerHTML = `
action: ${escapeHtml(action)}
`; contentDiv.innerHTML += `
path: ${escapeHtml(path)}
`; if (data.arguments.file_text) { contentDiv.innerHTML += `
content: 
${escapeHtml(data.arguments.file_text.substring(0, 200))}${data.arguments.file_text.length > 200 ? '...' : ''}
`; } } else { - contentDiv.innerHTML = `
${JSON.stringify(data.arguments, null, 2)}
`; + // `JSON.stringify` escapes for JSON, which says nothing about HTML: it + // leaves `<` and `/` alone, so an argument holding `` arrived here as live markup. + contentDiv.innerHTML = `
${escapeHtml(JSON.stringify(data.arguments, null, 2))}
`; } toolDiv.appendChild(headerDiv); @@ -343,8 +366,8 @@ function handleToolConfirmation(data) { confirmDiv.innerHTML = `
⚠️ Tool Confirmation Required
- ${data.tool_name} wants to execute with: - 
${JSON.stringify(data.arguments, null, 2)}
+ ${escapeHtml(data.tool_name)} wants to execute with: + 
${escapeHtml(JSON.stringify(data.arguments, null, 2))}
Auto-approved in web mode (UI coming soon)
`; @@ -456,6 +479,17 @@ function sendSuggestion(text) { // Event listeners sendButton.addEventListener('click', sendMessage); +// The welcome pills, bound here rather than through an `onclick` attribute in +// index.html: the page is served under `script-src 'self'`, which refuses inline +// handlers. Delegated from the container because the welcome block is removed +// once the first message is sent. +messagesContainer.addEventListener('click', (e) => { + const pill = e.target.closest('.suggestion-pill[data-suggestion]'); + if (pill) { + sendSuggestion(pill.dataset.suggestion); + } +}); + messageInput.addEventListener('keydown', (e) => { if (e.key === 'Enter' && !e.shiftKey) { e.preventDefault(); diff --git a/docs/deployment/serve-decisions.md b/docs/deployment/serve-decisions.md index b52be17fd..dfb7a76b8 100644 --- a/docs/deployment/serve-decisions.md +++ b/docs/deployment/serve-decisions.md @@ -353,6 +353,68 @@ count came from the transcript route. Implemented in `crates/biorouter-cli/src/c tests, three of which drive the real router and WebSocket handler over a socket, against a real session store. +### The same page reflected the URL into script context + +**Ruling (2026-09-11).** `GET /session/{name}` no longer writes anything into a `", session_name +``` + +`session_name` is a path segment, so it is whatever the sender typed — behind no credential at +all on the loopback bind that requires none. A `'` ended the string literal and a `` +ended the element. `GET /session/` was served back as: + +```html +… +``` + +On this page that is not defacement. The injected script runs on the server's own origin, reads +`data-ws-token` out of the very document it was injected into, opens `/ws` with it, and sends a +message to an agent that holds `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 remote code execution as the operator. + +**Displaced alternatives.** + +- *HTML-escape the value inside the `