diff --git a/crates/biorouter-mcp/src/knowledge/service.rs b/crates/biorouter-mcp/src/knowledge/service.rs index 0a27ad87..d6bd4d81 100644 --- a/crates/biorouter-mcp/src/knowledge/service.rs +++ b/crates/biorouter-mcp/src/knowledge/service.rs @@ -1812,6 +1812,47 @@ impl KnowledgeService { ) } + /// Refuse an id the registry already holds, distinguishing a live row from an + /// **orphan** one. + /// + /// #158: a bare "already registered" is a dead end when the directory is gone + /// — `kb_list_bases` does not show the base, so the id can be neither seen, + /// read, deleted nor re-created. Naming the stale row gives the refusal + /// somewhere to point. (`registry::register` carries the same distinction for + /// its own callers; this exists because create refuses here first and never + /// reaches it.) + /// + /// ⚠ **It names the registry FILE, not its absolute path** (adversarial + /// security review 2026-09-12, MEDIUM), for the same reason the + /// already-exists bail alongside it stopped naming one: this string is a + /// `POST /knowledge/bases` response body. The file sits in the knowledge + /// root, which whoever can act on the message already has. + /// + /// Lifted out of [`Self::create_base_as_with_checkpoint`] rather than left + /// inline: spelling the message this carefully pushed that function past + /// `clippy::too_many_lines`, and a self-contained refusal is the part of it + /// that was never about creating anything. + fn refuse_if_the_id_is_registered(&self, id: &str) -> Result<()> { + let Some(stale) = registry::load(&self.root)? + .into_iter() + .find(|entry| entry.id == id) + else { + return Ok(()); + }; + if stale.path.exists() { + anyhow::bail!("kb-id '{id}' already registered"); + } + anyhow::bail!( + "kb-id '{id}' is registered but its directory is missing. The row is stale, \ + which is why this id is neither listed nor creatable. Remove the '{id}' entry \ + from '{}' in your Biorouter knowledge directory to free the id.", + registry::registry_path(&self.root) + .file_name() + .map(|name| name.to_string_lossy().into_owned()) + .unwrap_or_else(|| "the knowledge registry".to_string()), + ); + } + fn create_base_as_with_checkpoint( &self, spec: CreateBaseSpec<'_>, @@ -1829,32 +1870,19 @@ impl KnowledgeService { paths::validate_kb_id(id)?; let kb_root = paths::kb_root(&self.root, id); if kb_root.exists() { - anyhow::bail!("kb '{id}' already exists at {}", kb_root.display()); + // ⚠ **No path in this message** (adversarial security review + // 2026-09-12, MEDIUM). `POST /knowledge/bases` returns whatever this + // says verbatim, and it used to end `… at + // /Users//.config/biorouter/knowledge/` — the machine's + // absolute config path, handed to whoever asked. The caller supplied + // the id and already knows the root if it is entitled to know + // anything here, so the path added nothing but the disclosure. The + // route's own gate is what stops an unentitled caller reaching this + // line at all; this is the second layer. + anyhow::bail!("kb '{id}' already exists"); } let metadata = BasePublicationSnapshot::capture(&self.root)?; - // #158: this is the guard a user actually hits, and a bare "already - // registered" is a dead end when the row is an ORPHAN — the directory is - // gone (checked immediately above), so `kb_list_bases` does not show the - // base and the id can be neither seen, read, deleted nor re-created. - // Name the stale row and where it lives so the refusal points somewhere. - // - // `registry::register` carries the same distinction for its own callers; - // this one exists because create refuses here first and never reaches it. - if let Some(stale) = registry::load(&self.root)? - .into_iter() - .find(|entry| entry.id == id) - { - if stale.path.exists() { - anyhow::bail!("kb-id '{id}' already registered"); - } - anyhow::bail!( - "kb-id '{id}' is registered but its directory is missing ({}). The row is \ - stale, which is why this id is neither listed nor creatable. Remove it from \ - {} to free the id.", - stale.path.display(), - registry::registry_path(&self.root).display() - ); - } + self.refuse_if_the_id_is_registered(id)?; let staged_root = self .root .join(format!(".creating-{id}-{}", uuid::Uuid::new_v4())); diff --git a/crates/biorouter-server/src/routes/agent.rs b/crates/biorouter-server/src/routes/agent.rs index 678e5302..1b574dc1 100644 --- a/crates/biorouter-server/src/routes/agent.rs +++ b/crates/biorouter-server/src/routes/agent.rs @@ -1481,9 +1481,12 @@ async fn permission_editor_tools( responses( (status = 200, description = "Model-visible callable tool count", body = CallableToolCountResponse), (status = 401, description = "Unauthorized - invalid secret key"), - (status = 403, description = "Refused by a privacy boundary: the same refusal, word for \ - word, that `GET /sessions/{session_id}` gives (body = plain \ - text)"), + (status = 403, description = "Refused by a privacy boundary (issue #56 Task 58 / #47): \ + the named chat is private (or absent, and an unproven caller \ + is told the same thing for both) and the request carried \ + neither a capability that covers it nor proof it came from \ + the user. It is the same refusal, word for word, that \ + `GET /sessions/{session_id}` gives (body = plain text)"), (status = 424, description = "Agent not initialized") ) )] @@ -1512,7 +1515,11 @@ async fn get_callable_tool_count( // `ErrorResponse`, which would wrap the same words in a JSON envelope.** One // boundary has one body (see the module header of `routes::session_reach`), // and that is the only reason the gate lives in this wrapper and the work - // lives in the function below rather than all in one body. + // lives in the function below rather than all in one body — `get_tools` + // beside it has the same shape for the same reason. A caller must not be + // able to tell the gated routes apart by their envelopes, which is what + // `every_route_that_names_a_private_chat_refuses_it_exactly_as_the_read_does` + // measures: it fails on the wrapping alone, with the words unchanged. if let Err(refusal) = crate::routes::session_reach::session_reach( state.session_manager(), &query.session_id, diff --git a/crates/biorouter-server/src/routes/knowledge.rs b/crates/biorouter-server/src/routes/knowledge.rs index 1f15ee64..ea854fc8 100644 --- a/crates/biorouter-server/src/routes/knowledge.rs +++ b/crates/biorouter-server/src/routes/knowledge.rs @@ -32,18 +32,32 @@ use tokio::task::JoinHandle; use tokio_util::sync::CancellationToken; use utoipa::ToSchema; -/// Build the knowledge router. The router owns an `Arc` directly so -/// it can be tested without constructing a full `AppState`. +/// Every route that names a knowledge base by `{id}`, and nothing else — +/// **ungated**, because the one place that consumes it applies +/// [`session_reach::gate_knowledge_base`](crate::routes::session_reach::gate_knowledge_base) +/// to the value it returns (issue #56, QA 2026-09-10 H2). /// -/// ⚠ **Every route that names a base by `{id}` lives in `base_routes`, and -/// nothing else does.** That sub-router carries -/// `session_reach::gate_knowledge_base` as a `route_layer`, so each of its -/// routes — and any added to it later — answers a caller who may not reach the -/// named base with the same refusal before its handler runs (issue #56, QA -/// 2026-09-10 H2). A route that names a base and is registered on the outer -/// router instead is ungated: put it here. -pub fn router(svc: Arc) -> Router { - let base_routes = Router::new() +/// ⚠ **`Router::route_layer` is a SNAPSHOT, not a rule the router keeps.** It +/// consumes the routes present *at the moment it is called* and returns a map of +/// wrapped ones; a route registered afterwards is not wrapped, silently. The doc +/// on this pair used to claim that a route "added to it later" was gated, which +/// is not something axum offers — and because the `.route_layer(...)` sat last +/// in this chain, the natural way to add a route (append one more `.route(…)`) +/// produced an **ungated** `/bases/{id}` route that looked right. +/// +/// So the layer is no longer part of the chain. Appending a `.route(…)` here — +/// anywhere, including after the last one — is gated, because the gate is +/// applied to whatever this function returns. Appending to the *call site* +/// instead is visibly outside the gate, which is the point: the mistake is now +/// one you can see. +/// +/// Pinned from two directions by +/// `every_route_that_names_a_base_is_inside_the_gated_sub_router` — which reads +/// this file's own source rather than trusting the sentence above — and by +/// `base_addressing_routes`, whose probe list must cover every route registered +/// here and which the H2 tests drive against a real private base. +fn base_routes() -> Router> { + Router::new() .route( "/bases/{id}", get(get_base).put(update_base).delete(delete_base), @@ -73,11 +87,17 @@ pub fn router(svc: Arc) -> Router { "/bases/{id}/sources/{sid}/credibility", put(override_credibility), ) - .route_layer(axum::middleware::from_fn_with_state( - svc.clone(), - crate::routes::session_reach::gate_knowledge_base, - )); +} +/// Build the knowledge router. The router owns an `Arc` directly so +/// it can be tested without constructing a full `AppState`. +/// +/// ⚠ **No route registered on THIS router may name a base by `{id}`** — those +/// live in [`base_routes`], which is gated on the line below. A `{id}` route +/// added here is ungated, and +/// `every_route_that_names_a_base_is_inside_the_gated_sub_router` fails when one +/// is. +pub fn router(svc: Arc) -> Router { Router::new() .route("/bases", get(list_bases).post(create_base)) .route( @@ -89,7 +109,14 @@ pub fn router(svc: Arc) -> Router { .route("/expand-path", post(expand_path)) .route("/active", get(get_active).post(set_active)) .route("/check-model", post(check_model)) - .merge(base_routes) + // The gate is applied HERE, to the whole of `base_routes()`, rather than + // inside it — see that function for why the position is load-bearing. + .merge( + base_routes().route_layer(axum::middleware::from_fn_with_state( + svc.clone(), + crate::routes::session_reach::gate_knowledge_base, + )), + ) .with_state(svc) } @@ -501,18 +528,46 @@ pub async fn list_bases( )) } +// `POST /knowledge/bases` — mint a base. +// +// ⚠ **Gated, and gated on the namespace rather than on `body.id`** (adversarial +// security review 2026-09-12, MEDIUM). Create refuses an id that is taken, so +// before this it was an existence oracle for exactly the ids +// `KNOWLEDGE_BASE_OUT_OF_REACH` exists to withhold: a secret-only caller POSTed +// a guessed id and read `400 kb '' already exists at ` when a **private** base had it, and `200` when nothing did. +// KB ids are user-authored names, so a short dictionary enumerated the private +// bases on the machine by name — with the path as a bonus. +// +// `HttpCaller::mints_knowledge_base` takes no id, which is what makes the answer +// the same for every id, including one that does not exist. See its doc for what +// the refusal costs. +// +// Deliberately `//` and not `///`: utoipa publishes a doc comment here as the +// operation's `description`, and this is a note to the next engineer rather than +// API reference for a client. The wire contract is in the `responses` below. #[utoipa::path( post, path = "/knowledge/bases", request_body = CreateBaseBody, responses( (status = 200, description = "Created knowledge base", body = Manifest), (status = 400, description = "Duplicate id, invalid id, or unknown format"), + (status = 403, description = "This caller may not mint a knowledge-base id: it is a \ + public model and the request carried no proof it came from \ + the person at the keyboard. The same answer for every id, \ + taken or free, so that creating is not a way to ask which \ + private bases exist (body = plain text)"), ) )] pub async fn create_base( State(svc): State>, + headers: HeaderMap, Json(body): Json, ) -> Result, (StatusCode, String)> { + crate::routes::session_reach::http_caller(&headers) + .await + .mints_knowledge_base() + .map_err(|refusal| (refusal.status, refusal.message.to_string()))?; // Refused before anything is created: `create_base_in` writes the manifest, // the scaffolded tree and `schema.md` in one transaction precisely because // those are three statements about one base, and a request this route diff --git a/crates/biorouter-server/src/routes/session.rs b/crates/biorouter-server/src/routes/session.rs index 2f0ed7e5..1d60565d 100644 --- a/crates/biorouter-server/src/routes/session.rs +++ b/crates/biorouter-server/src/routes/session.rs @@ -20,7 +20,7 @@ use biorouter::privacy::declassify::{ use biorouter::privacy::SessionClassification; use biorouter::session::extension_data::ExtensionState; use biorouter::session::session_manager::{ - ActivityWindow, ModelUsageRow, SessionInsights, TruncateOutcome, + ActivityWindow, ModelUsageRow, SessionInsights, SidebarCursor, TruncateOutcome, }; use biorouter::session::{EnabledExtensionsState, Session, SessionSummary, SessionType}; use biorouter::workflow::Workflow; @@ -120,13 +120,52 @@ fn minted_capability_without_proof(child: SessionClassification, had_user_action pub struct SidebarSessionsQuery { #[serde(default = "default_sidebar_session_limit")] limit: u32, + /// The previous page's `next_cursor`, passed back unchanged. Absent for the + /// first page. + /// + /// ⚠ **This replaced an `offset`** (adversarial security review 2026-09-12, + /// HIGH). serde ignores unknown query fields, so a client still sending + /// `offset=…` is served the first page rather than a 400 — it pages from the + /// top instead of failing, which is the degradation to prefer for a listing. #[serde(default)] - offset: u32, + cursor: Option, /// BR-71: include `sub_agent` sessions (grouped under `parent_session_id`). #[serde(default)] include_subagents: bool, } +/// What `next_cursor` carries on the wire. +/// +/// ⚠ **It is not signed, and it does not need to be.** A cursor names the sort +/// key of a row the caller was just handed, and the page it opens is assembled +/// by the same filter as every other page — so a caller that forges one, or +/// replays someone else's, still sees exactly the rows it may see. What the +/// encoding buys is that the value carries **no position**: there is nothing in +/// it to subtract from the next one, which is the whole defect it replaces. +/// +/// Base64 rather than the two fields in the open so that no client starts +/// parsing it and pins a shape this route must then keep. +fn encode_sidebar_cursor(cursor: &SidebarCursor) -> String { + use base64::Engine as _; + base64::engine::general_purpose::URL_SAFE_NO_PAD + .encode(format!("{}\u{1f}{}", cursor.updated_at, cursor.id)) +} + +/// The inverse of [`encode_sidebar_cursor`]. `None` for anything this route did +/// not mint. +fn decode_sidebar_cursor(encoded: &str) -> Option { + use base64::Engine as _; + let raw = base64::engine::general_purpose::URL_SAFE_NO_PAD + .decode(encoded) + .ok()?; + let raw = String::from_utf8(raw).ok()?; + let (updated_at, id) = raw.split_once('\u{1f}')?; + (!updated_at.is_empty() && !id.is_empty()).then(|| SidebarCursor { + updated_at: updated_at.to_string(), + id: id.to_string(), + }) +} + /// Query parameters for `GET /sessions`. #[derive(Debug, Deserialize, utoipa::IntoParams)] pub struct ListSessionsQuery { @@ -158,7 +197,11 @@ fn listed_session_types(include_subagents: bool) -> &'static [SessionType] { pub struct SidebarSessionListResponse { sessions: Vec, has_more: bool, - next_offset: Option, + /// Where the next page resumes — pass it back as `cursor`, unchanged, and do + /// not parse it. `null` when this was the last page. + // Not a doc comment: utoipa publishes those as the schema's `description`, + // and the reasoning belongs beside the codec. See `encode_sidebar_cursor`. + next_cursor: Option, } #[derive(Deserialize, ToSchema)] @@ -373,17 +416,18 @@ async fn list_sessions( path = "/sessions/sidebar", params( ("limit" = Option, Query, description = "Session summaries per page (default 10, clamped to 1..=50)"), - ("offset" = Option, Query, description = "Number of session summaries to skip"), + ("cursor" = Option, Query, description = "The previous page's `next_cursor`, passed back unchanged. Omit for the first page; an unrecognised value is answered 400"), ("include_subagents" = Option, Query, description = "Include sub_agent sessions (grouped under parent_session_id); default false") ), responses( (status = 200, description = "Paginated lightweight session summaries for the sidebar, \ holding only the sessions this caller could open (see \ - `GET /sessions`). `next_offset` is where the next page \ - starts; for a caller shown every session it is `offset + \ - limit` as before, and for one shown a filtered view it is a \ - position in the underlying ordering, so pass it back as \ - given rather than computing it", body = SidebarSessionListResponse), + `GET /sessions`). `next_cursor` is an OPAQUE continuation \ + token: pass it back as `cursor` and do not parse it. It is \ + not a position and not a count — it names the last row this \ + page returned, so it says nothing about rows that were \ + filtered out", body = SidebarSessionListResponse), + (status = 400, description = "The `cursor` was not one this route issued"), (status = 401, description = "Unauthorized - Invalid or missing API key"), (status = 500, description = "Internal server error") ), @@ -400,84 +444,55 @@ async fn list_sidebar_sessions( let limit = query.limit.clamp(1, MAX_SIDEBAR_SESSION_LIMIT); let caller = crate::routes::session_reach::http_caller(&headers).await; - // A caller shown every row — the desktop app, a private-capability program, - // or any caller with tiers switched off — pages exactly as it always did, - // one query per page. - if caller.lists_session(SessionClassification::Private) { - let mut sessions = state - .session_manager() - .list_session_summaries( - limit.saturating_add(1), - query.offset, - query.include_subagents, - false, - ) - .await - .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; - - let has_more = sessions.len() > limit as usize; - sessions.truncate(limit as usize); - let next_offset = has_more.then(|| query.offset.saturating_add(limit)); - - return Ok(Json(SidebarSessionListResponse { - sessions, - has_more, - next_offset, - })); - } - - // Issue #56, QA 2026-09-10 M1: every other caller is shown the public rows - // only, so the page is assembled by SCANNING the ordering rather than by - // filtering one `LIMIT` window — a window filtered after the fact hands back - // short, ragged pages, and a `has_more` counted before the filter would - // report the private rows it hid, which is the count oracle omission exists - // to close. `workspace_list` pages a filtered view the same way. + // Issue #56, QA 2026-09-10 M1: a caller that may not open a private chat is + // not shown one here either — the listing is the union of what the singular + // gate admits, and nothing more. // - // `offset` and `next_offset` are therefore positions in the UNFILTERED - // ordering: the next page starts exactly where this one stopped, so a walk - // that passes `next_offset` back sees every visible row once. + // ⚠ **The filter is a SQL predicate, and that is the security property, not + // an optimisation** (adversarial security review 2026-09-12, HIGH). This + // route first answered M1 by scanning the unfiltered ordering and dropping + // private rows in Rust, then resuming from the *position it had reached*. + // The positions it handed back were positions among the rows it had hidden, + // so subtracting two of them gave their exact count — and because + // `updated_at` is stamped on every token written, polling it watched private + // chats start and finish. Rows the caller may not see now never leave the + // database, and a page resumes from a keyset of the last row it RETURNED, + // which is a fact the caller already holds. // - // The scan is bounded per request. Hitting the bound is not the end of the - // list: the page says where to resume, so a machine whose history is mostly - // private is walked in several requests rather than silently cut short. - const SCAN_CHUNK: u32 = 200; - const MAX_SCANNED_ROWS: u32 = 20_000; - let manager = state.session_manager(); - let mut sessions = Vec::with_capacity(limit as usize); - let mut next_offset = None; - let mut position = query.offset; - 'scan: loop { - if position.saturating_sub(query.offset) >= MAX_SCANNED_ROWS { - next_offset = Some(position); - break; - } - let chunk = manager - .list_session_summaries(SCAN_CHUNK, position, query.include_subagents, false) - .await - .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; - let fetched = chunk.len() as u32; - for (index, summary) in chunk.into_iter().enumerate() { - if !caller.lists_session(summary.privacy_tier) { - continue; - } - if sessions.len() == limit as usize { - // A visible row beyond this page exists, so there is a next - // page, and it starts at this row. - next_offset = Some(position.saturating_add(index as u32)); - break 'scan; - } - sessions.push(summary); - } - if fetched < SCAN_CHUNK { - break; - } - position = position.saturating_add(fetched); - } + // One path for both callers. The proven caller could still page by offset + // cheaply, but two shapes of continuation token is how the first one came to + // mean something different from the other. + let after = match query.cursor.as_deref() { + Some(encoded) => match decode_sidebar_cursor(encoded) { + Some(cursor) => Some(cursor), + None => return Err(StatusCode::BAD_REQUEST), + }, + None => None, + }; + let public_only = !caller.lists_session(SessionClassification::Private); + + let mut rows = state + .session_manager() + .list_session_summaries_page( + limit.saturating_add(1), + after.as_ref(), + query.include_subagents, + false, + public_only, + ) + .await + .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + + let has_more = rows.len() > limit as usize; + rows.truncate(limit as usize); + let next_cursor = has_more + .then(|| rows.last().map(|row| encode_sidebar_cursor(&row.cursor))) + .flatten(); Ok(Json(SidebarSessionListResponse { - sessions, - has_more: next_offset.is_some(), - next_offset, + sessions: rows.into_iter().map(|row| row.summary).collect(), + has_more, + next_cursor, })) } @@ -1953,7 +1968,7 @@ pub(crate) mod diverge_tests { #[serial] async fn sidebar_route_returns_paginated_lightweight_sessions() { let state = AppState::new().await.unwrap(); - let (status, body) = get_sidebar_sessions(state, "?limit=2&offset=0").await; + let (status, body) = get_sidebar_sessions(state, "?limit=2").await; assert_eq!(status, axum::http::StatusCode::OK); let sessions = body @@ -1962,7 +1977,7 @@ pub(crate) mod diverge_tests { .expect("sessions array"); assert!(sessions.len() <= 2); assert!(body.get("has_more").is_some()); - assert!(body.get("next_offset").is_some()); + assert!(body.get("next_cursor").is_some()); if let Some(session) = sessions.first().and_then(|session| session.as_object()) { for field in [ @@ -1984,6 +1999,60 @@ pub(crate) mod diverge_tests { } } + /// The cursor round-trips, and carries the two fields it claims to — no + /// position among them (adversarial security review 2026-09-12, HIGH). + #[test] + fn a_sidebar_cursor_carries_one_rows_sort_key_and_nothing_else() { + let cursor = SidebarCursor { + updated_at: "2026-09-11 04:05:06".to_string(), + id: "20260911_040506".to_string(), + }; + let encoded = encode_sidebar_cursor(&cursor); + assert_eq!(decode_sidebar_cursor(&encoded), Some(cursor.clone())); + + // Whatever a caller decodes out of it, it is the row it was just handed. + use base64::Engine as _; + let plain = String::from_utf8( + base64::engine::general_purpose::URL_SAFE_NO_PAD + .decode(&encoded) + .unwrap(), + ) + .unwrap(); + assert!(plain.contains(&cursor.updated_at) && plain.contains(&cursor.id)); + + for junk in ["", "not base64!!", "Zm9v", "AB8=", "\u{1f}"] { + assert_eq!( + decode_sidebar_cursor(junk), + None, + "{junk:?} was accepted as a cursor" + ); + } + // Neither half may be empty: an empty id would compare `> ''`, which is + // every row, and an empty timestamp would resume before the beginning. + let half = + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode("2026-09-11 04:05:06\u{1f}"); + assert_eq!(decode_sidebar_cursor(&half), None); + } + + /// A cursor this route did not mint is a 400, and the `offset` the parameter + /// replaced is IGNORED rather than rejected — a client built against the old + /// shape pages from the top instead of breaking. + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn the_sidebar_rejects_a_forged_cursor_and_ignores_a_stale_offset() { + let state = AppState::new().await.unwrap(); + let (status, _) = get_sidebar_sessions(state.clone(), "?limit=1&cursor=not-a-cursor").await; + assert_eq!(status, axum::http::StatusCode::BAD_REQUEST); + + let (status, body) = get_sidebar_sessions(state.clone(), "?limit=1&offset=40").await; + assert_eq!(status, axum::http::StatusCode::OK); + let first = get_sidebar_sessions(state, "?limit=1").await.1; + assert_eq!( + body["sessions"], first["sessions"], + "a stale `offset` moved the page it was ignored on" + ); + } + /// BR-71: the two type slices `GET /sessions` chooses between. A wrong slice /// — a dropped `Scheduled`, say, which would silently empty History of every /// scheduled run — compiles and passes every route test in this file, because diff --git a/crates/biorouter-server/src/routes/session_reach.rs b/crates/biorouter-server/src/routes/session_reach.rs index 7ead215a..a38ffb4f 100644 --- a/crates/biorouter-server/src/routes/session_reach.rs +++ b/crates/biorouter-server/src/routes/session_reach.rs @@ -82,8 +82,15 @@ //! ([`HttpCaller::lists_session`]), so the list is the union of what per-id //! probing could learn and nothing more. **Filter, not refuse**: a refused //! list would break every client on the public chats the gate is deliberately -//! inert on. The sidebar pages its filtered view by scanning, so `has_more` -//! cannot count the rows it hid. `GET /schedule/{id}/sessions` takes the same +//! inert on. ⚠ **The sidebar's pagination was the second half of this and got +//! it wrong.** It answered M1 by scanning the unfiltered ordering, dropping +//! the private rows in Rust and resuming from the position it had reached — so +//! two continuation values subtracted gave the exact number of private chats +//! between two visible ones, and because `updated_at` is stamped on every +//! token written, polling the route reported when a private chat was running. +//! Since the adversarial review of 2026-09-12 the tier is a **SQL predicate** +//! and the page resumes from a keyset of the last row it RETURNED, so there is +//! nothing hidden left to count. `GET /schedule/{id}/sessions` takes the same //! filter, and so, since 2026-09-11, does `GET /active_work`. A row of running //! work is not a chat, so [`HttpCaller::lists_work`] resolves the chat it //! belongs to, and a row that names no chat, or one that cannot be read, is @@ -800,6 +807,43 @@ impl HttpCaller { ) .map_err(SessionOutOfReach::for_knowledge_base) } + + /// May this caller **mint** a knowledge-base id — `POST /knowledge/bases` + /// (adversarial security review 2026-09-12, MEDIUM)? + /// + /// ⚠ **It takes no id, and that is the fix rather than an omission.** Create + /// refuses an id that is taken, so an answer that depended on the id would + /// tell its caller which ids are taken — and a private base's id is content + /// the listing deliberately omits ([`KNOWLEDGE_BASE_OUT_OF_REACH`] says so + /// in as many words). The old route answered a colliding private id with + /// `400 kb '' already exists at /Users/…/knowledge/` and a free one + /// with `200`, so a short dictionary of plausible names enumerated the + /// machine's private bases, with the absolute path thrown in. + /// + /// The question asked instead is about the **namespace**: a not-yet-existing + /// id has exactly the tier [`TargetTier::Unreadable`] names, and a caller + /// that may not be told about such an id may not take one either. Because + /// the id is never read, the refusal is the same for every id — which is the + /// property, stated as a type rather than as a promise. + /// + /// What this costs, stated plainly: a caller holding nothing but the daemon + /// secret can no longer create a knowledge base over HTTP. The desktop sends + /// the user's proof, a program stating a private provider passes on its + /// capability, and a `biorouter serve` operator on a private provider passes + /// on theirs. A public serve operator is refused, and that is the same + /// answer they already get for every private base on that machine. + /// + /// DR-15's opt-out is inert here as everywhere: with tiers off, creation is + /// exactly what it was. + pub fn mints_knowledge_base(&self) -> Result<(), SessionOutOfReach> { + refuse_unless_reachable( + self.enforced, + TargetTier::Unreadable, + self.capability(), + self.proof, + ) + .map_err(SessionOutOfReach::for_knowledge_base) + } } /// A named knowledge base, reduced to the bit the gate turns on. @@ -920,13 +964,22 @@ pub async fn gate_knowledge_active( /// sends, or by the private capability a program states. /// /// ⚠ **A layer on a sub-router of exactly the routes that name a base, not a -/// list of routes.** `knowledge::router` puts every `{id}` route in one router -/// and `route_layer`s this onto it, so the gate reads the `id` the router -/// itself matched — percent-decoded exactly as each handler's `Path` sees it — -/// and a route added there later is gated by construction. Reads and writes -/// alike: a caller that may not read a base may not rewrite, restore, merge or -/// delete it either, which is F0's lesson applied here before anyone measured -/// it. +/// list of routes.** `knowledge::base_routes` puts every `{id}` route in one +/// router and `knowledge::router` `route_layer`s this onto it, so the gate reads +/// the `id` the router itself matched — percent-decoded exactly as each handler's +/// `Path` sees it. Reads and writes alike: a caller that may not read a base may +/// not rewrite, restore, merge or delete it either, which is F0's lesson applied +/// here before anyone measured it. +/// +/// ⚠ **"and any route added later is gated by construction" was a claim axum does +/// not support, and this doc made it until 2026-09-12.** `Router::route_layer` +/// wraps the routes present when it is *called* and returns a new map; a route +/// registered afterwards is not wrapped, and nothing says so. `base_routes` is +/// therefore now ungated by construction and the layer is applied to its return +/// value at its single call site, which is the shape that makes appending a +/// route safe — see that function, and +/// `every_route_that_names_a_base_is_inside_the_gated_sub_router`, which reads +/// the file rather than trusting either doc. /// /// It runs before the handler's own extractors, so a refused request never has /// its body parsed, its model constructed or its base looked up. @@ -1608,6 +1661,10 @@ mod tests { agent_rs, "async fn get_callable_tool_count(", "session_reach(", + // The delegate, which is where the agent fetch lives: the gate is + // in the wrapper so the refusal keeps its own plain-text body + // rather than this route's JSON envelope, exactly as `get_tools` + // does beside it. "model_visible_tool_count(", "the agent fetch, which mints an agent for the chat", ), @@ -2194,38 +2251,14 @@ mod tests { ); } - /// Every route that names a base by `{id}` sits in `base_routes`, behind - /// `gate_knowledge_base`, and none sits on the outer router. The HTTP tests - /// in `tests/knowledge_routes.rs` prove the layer FIRES on the routes that - /// exist today; this is what stops a route added tomorrow landing on the - /// wrong router, where it would be ungated and nothing would say so. - #[test] - fn every_route_that_names_a_base_sits_behind_the_knowledge_base_gate() { - let router = body_of(include_str!("knowledge.rs"), "pub fn router("); - let (gated, outer) = router - .split_once(".route_layer(") - .expect("the knowledge router no longer layers the base-reach gate"); - assert!( - outer.contains("session_reach::gate_knowledge_base"), - "the knowledge router's route layer is no longer the base-reach gate" - ); - let (layer, outer) = outer - .split_once("Router::new()") - .expect("the outer knowledge router moved"); - assert!(layer.contains("gate_knowledge_base")); - assert!( - gated.matches("\"/bases/{id}").count() >= 20, - "fewer routes than expected sit behind the gate:\n{gated}" - ); - assert!( - !outer.contains("{id}"), - "a route naming a base by `{{id}}` is registered on the ungated outer router:\n{outer}" - ); - assert!( - !gated.contains("\"/bases\"") && !gated.contains("\"/active\""), - "a route that names no base was put behind the base gate" - ); - } + // The router-shape scan that used to live here — "every `{id}` route sits + // behind `gate_knowledge_base`" — moved to + // `every_route_that_names_a_base_is_inside_the_gated_sub_router`, beside + // `base_addressing_routes`, when the adversarial review of 2026-09-12 found + // that the claim it checked was weaker than the doc it was checking. It now + // also asserts that every gated route is actually PROBED, which needs that + // list in scope; and it was tied to a `.route_layer(` inside `pub fn + // router(`, which is exactly the shape that had to change. } #[cfg(test)] @@ -4231,11 +4264,338 @@ mod bypass_tests { seeded } + /// **`POST /knowledge/bases` was an existence oracle**, as a named + /// regression test (adversarial security review 2026-09-12, MEDIUM). + /// + /// Create refuses an id that is taken, and it used to say so for a + /// **private** base — to a caller holding nothing but the daemon secret, with + /// the machine's absolute config path in the body. KB ids are user-authored + /// names, so a short dictionary enumerated the private bases the listing + /// deliberately omits. + /// + /// What is asserted is indistinguishability, byte for byte: the id of a + /// private base, the id of a public base and an id that has never existed all + /// get the SAME answer. That is only checkable if the answer does not depend + /// on the id, which is why `mints_knowledge_base` does not take one. + /// + /// And the user is unaffected: with the proof, the collision is reported + /// truthfully, a free id is created — and neither answer names a path. + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn creating_a_base_cannot_be_used_to_ask_which_private_bases_exist() { + install_test_user_action_key(); + let state = AppState::new().await.unwrap(); + let bases = seed_bases(&state, "create-oracle").await; + let absent = format!("qa-create-oracle-absent-{}", std::process::id()); + + let mint = |id: String, headers: Vec<(&'static str, &'static str)>| { + let state = state.clone(); + async move { + call( + state, + "POST", + "/knowledge/bases", + Some(serde_json::json!({ "id": id, "name": "minted by a probe" })), + &headers, + ) + .await + } + }; + + // The caller AR-11 measured: the daemon secret and nothing else. + let private = mint(bases.private.clone(), vec![]).await; + let public = mint(bases.public.clone(), vec![]).await; + let free = mint(absent.clone(), vec![]).await; + assert_eq!( + private, + ( + StatusCode::FORBIDDEN, + KNOWLEDGE_BASE_OUT_OF_REACH.to_string() + ), + "a private base's id was answered differently from every other id" + ); + assert_eq!( + public, private, + "a taken PUBLIC id and a taken PRIVATE id must be answered the same way here, or \ + the difference between them is the oracle" + ); + assert_eq!( + free, private, + "an id that does not exist was answered differently from a private base's id, which \ + is the oracle: 403 means taken by something this caller may not see" + ); + // …and the person at the keyboard is told the truth, without a path. + let (status, collision) = mint(bases.private.clone(), vec![PROOF]).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{collision}"); + assert!( + collision.contains("already exists"), + "the user was not told the id is taken: {collision}" + ); + + // No body on this route names a directory, whichever caller asked. + let root = state + .knowledge_service + .root() + .to_string_lossy() + .into_owned(); + for body in [&private.1, &public.1, &free.1, &collision] { + assert!( + !body.contains(&root), + "an error body on the create path named the knowledge root: {body}" + ); + assert!( + !body.contains(&format!("/{}", bases.private)), + "an error body on the create path named a base's directory: {body}" + ); + } + + let (status, body) = mint(absent.clone(), vec![PROOF]).await; + assert_eq!( + status, + StatusCode::OK, + "the user can no longer create a knowledge base: {body}" + ); + let _ = state.knowledge_service.delete_base(&absent); + let _ = std::fs::remove_dir_all(state.knowledge_service.root().join(&absent)); + } + + /// `crates/biorouter-server/src/routes/knowledge.rs`, as text, for the router + /// shape assertions below. `include_str!` rather than a runtime read so a + /// moved file is a compile error rather than a skipped check. + const KNOWLEDGE_ROUTES_SOURCE: &str = include_str!("knowledge.rs"); + + /// The body of a top-level `fn` in a `routes/*.rs` file: from its first `{` + /// to the first line that is a bare `}` in column 0. + /// + /// A brace counter would be the obvious implementation and is the wrong one + /// here: the bodies this reads are full of `"{id}"` and `"{*page_path}"` + /// literals. Column-0 `}` is what `cargo fmt` guarantees for a top-level + /// item, and nothing inside a function body can produce one. + /// + /// ⚠ **Line comments are removed**, and the first draft of this did not do + /// that — a comment in `router()` reading "applied here, to the whole of + /// `base_routes()`" made the single-consumer assertion below count two. A + /// scanner that reads prose as code is the failure mode `privacy_guard_wiring` + /// was written to avoid; the same applies here. Nothing in these bodies puts + /// `//` inside a string literal, which is the case this does not handle. + #[allow(clippy::string_slice)] // every index comes from `find`: a char boundary + fn top_level_fn_body(source: &str, signature: &str) -> String { + let start = source + .find(signature) + .unwrap_or_else(|| panic!("`{signature}` is not in knowledge.rs any more")); + let after = &source[start..]; + let open = after + .find('{') + .unwrap_or_else(|| panic!("`{signature}` has no body")); + let rest = &after[open + 1..]; + let end = rest + .find("\n}\n") + .unwrap_or_else(|| panic!("`{signature}` is not closed in column 0")); + rest[..end] + .lines() + .map(|line| match line.find("//") { + Some(at) => &line[..at], + None => line, + }) + .collect::>() + .join("\n") + } + + /// Every `.route("", )` in a router-builder body, as + /// `(path, methods)`. + #[allow(clippy::string_slice)] // every index comes from `find`: a char boundary + fn registered_routes(body: &str) -> Vec<(String, Vec)> { + body.split(".route(") + .skip(1) + .map(|chunk| { + let quote = chunk + .find('"') + .unwrap_or_else(|| panic!("a `.route(` with no path literal: {chunk:.80}")); + let rest = &chunk[quote + 1..]; + let close = rest.find('"').expect("unterminated route path literal"); + let path = rest[..close].to_string(); + let args = &rest[close + 1..]; + let methods = ["get", "post", "put", "delete", "patch"] + .into_iter() + .filter(|verb| args.contains(&format!("{verb}("))) + .map(str::to_uppercase) + .collect(); + (path, methods) + }) + .collect() + } + + /// Does `uri` (a concrete request path, `/knowledge` prefix and query string + /// included) match the axum route pattern `pattern` (`/bases/{id}/…`)? + fn uri_matches_route(uri: &str, pattern: &str) -> bool { + let path = uri.split('?').next().unwrap_or(uri); + let path = path.strip_prefix("/knowledge").unwrap_or(path); + let mut actual = path.trim_start_matches('/').split('/'); + let expected: Vec<&str> = pattern.trim_start_matches('/').split('/').collect(); + for (index, segment) in expected.iter().enumerate() { + if segment.starts_with("{*") { + // A wildcard capture eats the whole remainder, which must be + // non-empty. + return actual.next().is_some(); + } + let Some(got) = actual.next() else { + return false; + }; + if segment.starts_with('{') { + if got.is_empty() { + return false; + } + } else if *segment != got { + return false; + } + if index + 1 == expected.len() { + return actual.next().is_none(); + } + } + false + } + + /// **The guarantee the doc used to assert and axum does not provide** + /// (adversarial security review 2026-09-12, MEDIUM). + /// + /// `Router::route_layer` wraps the routes that exist when it is called and + /// nothing added afterwards, so "every `/bases/{id}` route is gated" is a + /// property of how `knowledge.rs` is *written*, not of what axum promises. + /// The restructure makes the natural edit safe — the layer is applied to + /// `base_routes()`'s return value at its one call site — and this reads the + /// file to check the three things that restructure depends on, plus the one + /// thing the restructure cannot give: that the probe list the H2 tests drive + /// actually reaches every route registered. + #[test] + fn every_route_that_names_a_base_is_inside_the_gated_sub_router() { + let gated = top_level_fn_body( + KNOWLEDGE_ROUTES_SOURCE, + "fn base_routes() -> Router>", + ); + let outer = top_level_fn_body(KNOWLEDGE_ROUTES_SOURCE, "pub fn router(svc: Arc= 20, + "only {} `/bases/{{id}}` routes were found; the scanner has stopped reading \ + knowledge.rs", + registered.len() + ); + for (path, methods) in ®istered { + assert!( + !methods.is_empty(), + "no HTTP method was read off `{path}`; the scanner needs a new verb" + ); + for method in methods { + assert!( + probes + .iter() + .any(|(probe_method, uri, _)| probe_method == method + && uri_matches_route(uri, path)), + "`{method} {path}` is gated but never probed: add it to \ + `base_addressing_routes` so the H2 tests drive it against a real private \ + base" + ); + } + } + } + + /// The scanner's own corners, so a silently-matching-everything matcher + /// cannot make the assertion above vacuous. + #[test] + fn the_route_scanner_reads_paths_methods_and_matches_exactly() { + let parsed = registered_routes( + r#" + .route("/bases/{id}", get(a).put(b).delete(c)) + .route("/bases/{id}/pages/{*page_path}", get(d).put(e)) + .route("/active", post(f)) + "#, + ); + assert_eq!( + parsed, + vec![ + ( + "/bases/{id}".to_string(), + vec!["GET".to_string(), "PUT".to_string(), "DELETE".to_string()] + ), + ( + "/bases/{id}/pages/{*page_path}".to_string(), + vec!["GET".to_string(), "PUT".to_string()] + ), + ("/active".to_string(), vec!["POST".to_string()]), + ] + ); + + assert!(uri_matches_route("/knowledge/bases/kb1", "/bases/{id}")); + assert!(uri_matches_route( + "/knowledge/bases/kb1/page?path=knowledge/x.md", + "/bases/{id}/page" + )); + assert!(uri_matches_route( + "/knowledge/bases/kb1/pages/knowledge/x.md", + "/bases/{id}/pages/{*page_path}" + )); + assert!(!uri_matches_route( + "/knowledge/bases/kb1/pages", + "/bases/{id}/pages/{*page_path}" + )); + assert!(!uri_matches_route( + "/knowledge/bases/kb1/tier", + "/bases/{id}" + )); + assert!(!uri_matches_route( + "/knowledge/bases/kb1", + "/bases/{id}/tier" + )); + assert!(!uri_matches_route( + "/knowledge/bases/kb1/graph", + "/bases/{id}/tier" + )); + } + /// Every route under `/knowledge/bases/{id}` in the served tree, as /// `(method, uri, body)`. Macros name a provider the registry does not /// know, so an admitted one stops with a 400 long before any model. /// /// ⚠ **Destructive last**, for the reason the chat sweep gives. + /// + /// ⚠ **This list is checked for completeness**, by + /// `every_route_that_names_a_base_is_inside_the_gated_sub_router`: a route + /// added to `knowledge::base_routes` and not added here fails that test. fn base_addressing_routes( id: &str, sha: &str, @@ -4579,38 +4939,172 @@ mod bypass_tests { ); } - /// Every id the sidebar hands this caller, walking `next_offset` to the end. + /// One sidebar page, as the route's own clients take it: `cursor` passed + /// back unchanged, never computed. + async fn sidebar_page( + state: &Arc, + limit: u32, + cursor: Option<&str>, + headers: &[(&str, &str)], + ) -> serde_json::Value { + // The cursor is base64url without padding, so every byte of it is + // already safe in a query string — no escaping, and a client that had to + // escape it would be a client that had parsed it. + let uri = match cursor { + Some(cursor) => format!("/sessions/sidebar?limit={limit}&cursor={cursor}"), + None => format!("/sessions/sidebar?limit={limit}"), + }; + let (status, body) = call(state.clone(), "GET", &uri, None, headers).await; + assert_eq!(status, StatusCode::OK, "{body}"); + serde_json::from_str(&body).unwrap() + } + + fn page_ids(page: &serde_json::Value) -> Vec { + page["sessions"] + .as_array() + .unwrap() + .iter() + .map(|row| row["id"].as_str().unwrap().to_string()) + .collect() + } + + /// Every id the sidebar hands this caller, walking `next_cursor` to the end. async fn sidebar_ids( state: &Arc, limit: u32, headers: &[(&str, &str)], ) -> Vec { let mut ids = Vec::new(); - let mut offset = 0u64; + let mut cursor: Option = None; for _ in 0..10_000 { - let (status, body) = call( - state.clone(), - "GET", - &format!("/sessions/sidebar?limit={limit}&offset={offset}"), - None, - headers, - ) - .await; - assert_eq!(status, StatusCode::OK, "{body}"); - let page: serde_json::Value = serde_json::from_str(&body).unwrap(); - for row in page["sessions"].as_array().unwrap() { - ids.push(row["id"].as_str().unwrap().to_string()); - } + let page = sidebar_page(state, limit, cursor.as_deref(), headers).await; + ids.extend(page_ids(&page)); if page["has_more"] != serde_json::Value::Bool(true) { return ids; } - offset = page["next_offset"] - .as_u64() - .expect("has_more without next_offset"); + cursor = Some( + page["next_cursor"] + .as_str() + .expect("has_more without next_cursor") + .to_string(), + ); } panic!("the sidebar never reported its last page"); } + /// **The count oracle, as a named regression test** (adversarial security + /// review 2026-09-12, HIGH). This is the test that would have caught it. + /// + /// The sidebar filters its rows for a caller that may not open a private + /// chat, and it used to resume the next page from the position it had + /// reached in the UNFILTERED ordering. So the continuation value counted the + /// rows it had hidden: ask for page 1 twice with N private chats created in + /// between and the value moves by exactly N. `updated_at` is stamped on + /// every token written in this tree, so a private chat merely *running a + /// turn* moves it — which turns a listing into a live activity monitor on + /// chats the singular read refuses outright. + /// + /// Two assertions, and the first is the one that fails on the old code: + /// + /// 1. the continuation value does not move when hidden chats appear; and + /// 2. the walk still reaches the same visible rows across that churn — a + /// position-based resume does not, because the position it was given now + /// points at a different row. + /// + /// The sleep is load-bearing: `updated_at` is `datetime('now')`, one-second + /// granularity, so without it the seeded rows tie and SQLite breaks the tie + /// by `id ASC` — which would put the private rows *below* the boundary and + /// leave the old code's value accidentally unmoved. + /// + /// ⚠ The measurement is **retried**, and that is a statement about this + /// binary rather than about the route. The head of the listing is the whole + /// machine's newest visible chat; `#[serial]` keeps the other serial tests + /// out, but a non-serial test that creates a chat can land a foreign row at + /// the head inside the second this waits — which makes the test's PREMISE + /// false (a different visible row) rather than its subject wrong. A repeated + /// displacement still fails, and says so. + #[tokio::test(flavor = "multi_thread")] + #[serial] + async fn the_sidebar_continuation_value_is_not_a_count_of_the_chats_it_hid() { + install_test_user_action_key(); + let state = AppState::new().await.unwrap(); + + // Two visible rows, in one `datetime('now')` second. Which of them sorts + // first does not matter — only that the pair is stable across the churn + // below, which it is, because nothing here touches them again. + let _visible_a = seed_chat( + &state, + "count-oracle visible A (test fixture)", + SessionClassification::Public, + ) + .await; + let _visible_b = seed_chat( + &state, + "count-oracle visible B (test fixture)", + SessionClassification::Public, + ) + .await; + + // "Secret only" — the caller AR-11 measured, and the one this gate + // answers as a public model. + let secret_only: &[(&str, &str)] = &[]; + const HIDDEN: usize = 3; + let mut displacements = Vec::new(); + + for _ in 0..3 { + let before = sidebar_page(&state, 1, None, secret_only).await; + let first_page_ids = page_ids(&before); + let token_before = before["next_cursor"].clone(); + assert!( + !token_before.is_null(), + "two visible chats were just seeded and the first page reported no next page: \ + {before}" + ); + let second_page_ids = + page_ids(&sidebar_page(&state, 1, token_before.as_str(), secret_only).await); + + // Now the hidden rows, stamped into a strictly later second so they + // sort above everything seeded above. The guards drop at the end of + // each attempt, so a retry starts from the state this one did. + tokio::time::sleep(std::time::Duration::from_millis(1_100)).await; + let mut hidden = Vec::new(); + for i in 0..HIDDEN { + hidden.push( + seed_private_chat(&state, &format!("count-oracle hidden {i} (test fixture)")) + .await, + ); + } + + let after = sidebar_page(&state, 1, None, secret_only).await; + if page_ids(&after) != first_page_ids { + displacements.push(format!("{first_page_ids:?} -> {:?}", page_ids(&after))); + continue; + } + assert_eq!( + after["next_cursor"], token_before, + "the continuation value moved when {HIDDEN} private chats were created. Its \ + displacement IS their count, and because `updated_at` is stamped on every token \ + written, polling this route reports when a private chat is running." + ); + + // …and the value the caller was given still walks to the same row, + // which a position into the unfiltered ordering no longer does once + // that ordering has shifted underneath it. + assert_eq!( + page_ids(&sidebar_page(&state, 1, token_before.as_str(), secret_only).await), + second_page_ids, + "the same continuation value reached a different visible row after private chats \ + were created" + ); + return; + } + + panic!( + "the head of the listing moved under every attempt, so nothing was measured — \ + another test in this binary is creating chats: {displacements:?}" + ); + } + /// A private chat with no message at all — `/workflows/create` answers such /// a chat before it builds an agent. async fn seed_private_chat_without_messages(state: &Arc, label: &str) -> SeededChat { diff --git a/crates/biorouter-server/tests/knowledge_ingest_stream.rs b/crates/biorouter-server/tests/knowledge_ingest_stream.rs index 56420f51..156559a0 100644 --- a/crates/biorouter-server/tests/knowledge_ingest_stream.rs +++ b/crates/biorouter-server/tests/knowledge_ingest_stream.rs @@ -23,7 +23,20 @@ use biorouter_mcp::knowledge::service::KnowledgeService; use std::sync::Arc; use tower::ServiceExt; +/// Issue #56 / the adversarial review of 2026-09-12: `POST /bases` is gated like +/// every other base-naming route, because refusing a taken id told a secret-only +/// caller which PRIVATE ids were taken. So every test here that mints a base +/// speaks as the person at the keyboard — which is who mints one in the product. +const TEST_USER_ACTION_KEY: &str = "knowledge-ingest-stream-user-action-key"; + +fn install_test_user_action_key() { + let digest: [u8; 32] = + ::digest(TEST_USER_ACTION_KEY.as_bytes()).into(); + biorouter_server::auth::install_user_action_digest(Some(digest)); +} + fn build_app() -> (tempfile::TempDir, axum::Router) { + install_test_user_action_key(); let dir = tempfile::tempdir().unwrap(); let svc = Arc::new(KnowledgeService::new(dir.path().to_path_buf())); let app = biorouter_server::routes::knowledge::router(svc); @@ -37,6 +50,7 @@ async fn create_kb(app: &axum::Router, id: &str) { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from( serde_json::to_vec(&serde_json::json!({"id": id, "name": id})).unwrap(), diff --git a/crates/biorouter-server/tests/knowledge_routes.rs b/crates/biorouter-server/tests/knowledge_routes.rs index 1fcc001b..56491cc3 100644 --- a/crates/biorouter-server/tests/knowledge_routes.rs +++ b/crates/biorouter-server/tests/knowledge_routes.rs @@ -14,6 +14,7 @@ use std::sync::Arc; use tower::ServiceExt; fn build_test_router() -> (tempfile::TempDir, Router) { + tier_route::install_test_user_action_key(); let dir = tempfile::tempdir().unwrap(); let svc = Arc::new(KnowledgeService::new(dir.path().to_path_buf())); let router = biorouter_server::routes::knowledge::router(svc); @@ -81,6 +82,7 @@ async fn get_active(app: &Router, session_id: Option<&str>) -> serde_json::Value /// can seed files directly on disk (needed for routes that read from `raw/` /// where there is no write API). fn build_test_router_with_root() -> (tempfile::TempDir, std::path::PathBuf, Router) { + tier_route::install_test_user_action_key(); let dir = tempfile::tempdir().unwrap(); let root = dir.path().to_path_buf(); let svc = Arc::new(KnowledgeService::new(root.clone())); @@ -98,6 +100,7 @@ async fn get_location_returns_kb_path() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -195,6 +198,7 @@ async fn create_then_get_base() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -277,6 +281,7 @@ async fn update_base_metadata_roundtrip() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -366,6 +371,7 @@ async fn get_graph_returns_ok_on_new_kb() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -407,6 +413,7 @@ async fn list_pages_empty_on_new_kb() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -457,6 +464,7 @@ async fn a_page_written_over_http_reaches_the_graph() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -520,6 +528,7 @@ async fn write_then_read_page_roundtrip() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -579,6 +588,7 @@ async fn read_page_on_missing_path_returns_404() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -618,6 +628,7 @@ async fn history_write_restore_roundtrip() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -901,6 +912,7 @@ async fn add_raw_source_text() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -951,6 +963,7 @@ async fn add_raw_source_html_multipart_uses_part_mime() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -1126,6 +1139,7 @@ async fn add_raw_source_rejects_empty_body() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -1165,6 +1179,7 @@ async fn export_then_import_roundtrip() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -1381,6 +1396,7 @@ async fn reclassify_route_returns_credibility() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -1450,6 +1466,7 @@ async fn create_kb(app: Router, id: &str, name: &str) { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -1765,6 +1782,7 @@ async fn read_page_returns_markdown_body() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -1867,6 +1885,7 @@ async fn read_page_returns_raw_source_md() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -2047,6 +2066,7 @@ async fn active_kb_roundtrip() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -2185,6 +2205,7 @@ async fn primary_kb_can_be_scoped_per_session() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -2240,6 +2261,7 @@ async fn create_bases(app: &Router, ids: &[&str]) { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(body)) .unwrap(), @@ -2451,6 +2473,7 @@ async fn primary_must_be_a_member_of_the_resulting_set() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -2487,6 +2510,7 @@ async fn set_only_edit_keeps_the_primary_until_it_leaves_the_set() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -2595,6 +2619,7 @@ async fn the_users_own_export_route_is_not_subject_to_the_models_location_rule() Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(create_body)) .unwrap(), @@ -3056,6 +3081,7 @@ mod tier_route { /// so a test against it would assert that a route nobody guards lets everyone /// through. pub(super) fn guarded_router() -> (tempfile::TempDir, std::path::PathBuf, Router) { + install_test_user_action_key(); let dir = tempfile::tempdir().unwrap(); let root = dir.path().to_path_buf(); let svc = Arc::new(biorouter_mcp::knowledge::service::KnowledgeService::new( @@ -3109,6 +3135,7 @@ mod tier_route { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .header("X-Secret-Key", TEST_SECRET) .body(Body::from( @@ -3320,6 +3347,7 @@ mod okf_surface { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from(serde_json::to_vec(&body).unwrap())) .unwrap(), @@ -3713,6 +3741,7 @@ mod merge_route { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", tier_route::TEST_USER_ACTION_KEY) .header("content-type", "application/json") .header("X-Secret-Key", TEST_SECRET) .body(Body::from( diff --git a/crates/biorouter-server/tests/knowledge_routes_e2e.rs b/crates/biorouter-server/tests/knowledge_routes_e2e.rs index 651b2bc7..30096145 100644 --- a/crates/biorouter-server/tests/knowledge_routes_e2e.rs +++ b/crates/biorouter-server/tests/knowledge_routes_e2e.rs @@ -15,7 +15,20 @@ use biorouter_mcp::knowledge::{page_fixtures::valid_page, service::KnowledgeServ use std::sync::Arc; use tower::ServiceExt; +/// Issue #56 / the adversarial review of 2026-09-12: `POST /bases` is gated like +/// every other base-naming route, because refusing a taken id told a secret-only +/// caller which PRIVATE ids were taken. So every test here that mints a base +/// speaks as the person at the keyboard — which is who mints one in the product. +const TEST_USER_ACTION_KEY: &str = "knowledge-e2e-user-action-key"; + +fn install_test_user_action_key() { + let digest: [u8; 32] = + ::digest(TEST_USER_ACTION_KEY.as_bytes()).into(); + biorouter_server::auth::install_user_action_digest(Some(digest)); +} + fn build_app() -> (tempfile::TempDir, axum::Router) { + install_test_user_action_key(); let dir = tempfile::tempdir().unwrap(); let svc = Arc::new(KnowledgeService::new(dir.path().to_path_buf())); let app = biorouter_server::routes::knowledge::router(svc); @@ -40,6 +53,7 @@ async fn e2e_create_raw_history_graph_export_import() { Request::builder() .method("POST") .uri("/bases") + .header("X-User-Action", TEST_USER_ACTION_KEY) .header("content-type", "application/json") .body(Body::from( serde_json::to_vec(&serde_json::json!({"id": "e2e", "name": "E2E"})).unwrap(), diff --git a/crates/biorouter/src/session/session_manager.rs b/crates/biorouter/src/session/session_manager.rs index 17581f66..87b5ca70 100644 --- a/crates/biorouter/src/session/session_manager.rs +++ b/crates/biorouter/src/session/session_manager.rs @@ -540,6 +540,37 @@ pub struct SessionSummary { pub privacy_tier: SessionClassification, } +/// Where a sidebar page resumes: the sort key of the last row that page emitted. +/// +/// ⚠ **A keyset, deliberately not an offset** (adversarial security review +/// 2026-09-12, HIGH). `GET /sessions/sidebar` pages a view that omits the chats +/// its caller may not see, and it used to resume by *position in the unfiltered +/// ordering* — so the continuation value counted the rows it had hidden, and a +/// caller that polled it watched private chats start and finish, because +/// `updated_at` is stamped on every token written. A keyset is a fact about a +/// row the caller was **just handed**, so it can carry nothing the caller did +/// not already have. +/// +/// `updated_at` is the **stored text**, verbatim, not a re-serialised +/// `DateTime`. Rows stamped by `datetime('now')` and rows written from a bound +/// `DateTime` do not spell the same instant the same way, and the ordering +/// this resumes into compares the stored bytes — so a round-trip through +/// `chrono` would put the boundary in the wrong place for one of the two +/// spellings. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SidebarCursor { + pub updated_at: String, + pub id: String, +} + +/// One row of a sidebar page: the summary the caller is shown, and the +/// [`SidebarCursor`] a later page resumes after it. +#[derive(Debug, Clone)] +pub struct SidebarRow { + pub summary: SessionSummary, + pub cursor: SidebarCursor, +} + /// One turn's token usage, applied additively and atomically in SQL. #[derive(Debug, Clone, Copy, Default)] pub struct TokenDelta { @@ -1968,6 +1999,28 @@ impl SessionManager { .await } + /// One keyset page of the sidebar's view. See + /// [`SessionStorage::list_session_summaries_page`] — in particular why + /// `public_only` filters in SQL rather than in the caller. + pub async fn list_session_summaries_page( + &self, + limit: u32, + after: Option<&SidebarCursor>, + include_subagents: bool, + include_empty: bool, + public_only: bool, + ) -> Result> { + self.storage + .list_session_summaries_page( + limit, + after, + include_subagents, + include_empty, + public_only, + ) + .await + } + pub async fn list_sessions_by_types(&self, types: &[SessionType]) -> Result> { self.storage.list_sessions_by_types(types).await } @@ -3078,6 +3131,24 @@ impl sqlx::FromRow<'_, sqlx::sqlite::SqliteRow> for SessionSummary { } } +impl sqlx::FromRow<'_, sqlx::sqlite::SqliteRow> for SidebarRow { + fn from_row(row: &sqlx::sqlite::SqliteRow) -> Result { + use sqlx::Row; + + let summary = SessionSummary::from_row(row)?; + Ok(SidebarRow { + cursor: SidebarCursor { + // The projection aliases `CAST(s.updated_at AS TEXT)` to this + // name, so it is the bytes the ordering compares rather than a + // value chrono has been through. See [`SidebarCursor`]. + updated_at: row.try_get("cursor_updated_at")?, + id: summary.id.clone(), + }, + summary, + }) + } +} + impl sqlx::FromRow<'_, sqlx::sqlite::SqliteRow> for Session { fn from_row(row: &sqlx::sqlite::SqliteRow) -> Result { use sqlx::Row; @@ -6920,6 +6991,97 @@ impl SessionStorage { .map_err(Into::into) } + /// [`Self::list_session_summaries`] as a **keyset** page over a view that + /// may be filtered, for `GET /sessions/sidebar`. + /// + /// Two differences from the offset form, and both exist for the same reason + /// (adversarial security review 2026-09-12, HIGH): + /// + /// * `public_only` filters **in SQL**. The route used to fetch unfiltered + /// windows and drop the private rows in Rust, which made every position it + /// reported a count of what it had hidden. `sessions.privacy_tier` is a + /// real column, so the rows the caller may not see never leave the + /// database and there is nothing left to count. The comparison is + /// `= 'public'`, which fails **closed** exactly as [`read_privacy_tier`] + /// does: a row whose column is absent, `NULL` or unrecognised is withheld + /// rather than shown. + /// * the page resumes from a [`SidebarCursor`] — the sort key of the last + /// row the previous page emitted — rather than from a row count. + /// + /// The keyset predicate mirrors `ORDER BY s.updated_at DESC, s.id ASC` + /// exactly: strictly older, or the same instant with a larger id. Both sides + /// of the comparison are `CAST(... AS TEXT)` so the boundary is evaluated on + /// the same bytes the cursor carries. + async fn list_session_summaries_page( + &self, + limit: u32, + after: Option<&SidebarCursor>, + include_subagents: bool, + include_empty: bool, + public_only: bool, + ) -> Result> { + let type_filter = if include_subagents { + "('user', 'scheduled', 'sub_agent')" + } else { + "('user', 'scheduled')" + }; + // See [`Self::list_session_summaries`] for why the sidebar and + // `workspace_list` want opposite joins here. + let join = if include_empty { + "LEFT JOIN messages m ON s.id = m.session_id" + } else { + "INNER JOIN messages m ON s.id = m.session_id" + }; + let tier_filter = if public_only { + "AND s.privacy_tier = 'public'" + } else { + "" + }; + let keyset = if after.is_some() { + "AND (CAST(s.updated_at AS TEXT) < ? \ + OR (CAST(s.updated_at AS TEXT) = ? AND s.id > ?))" + } else { + "" + }; + let query = format!( + r#" + SELECT s.id, + s.working_dir, + COALESCE(NULLIF(s.name, ''), NULLIF(s.description, ''), 'Untitled chat') AS name, + s.user_set_name, + s.created_at, + s.updated_at, + CAST(s.updated_at AS TEXT) AS cursor_updated_at, + s.parent_session_id, + s.session_type, + s.diverged_from, + s.privacy_tier, + COUNT(m.id) AS message_count + FROM sessions s + {join} + WHERE s.session_type IN {type_filter} + {tier_filter} + {keyset} + GROUP BY s.id + ORDER BY s.updated_at DESC, s.id ASC + LIMIT ? + "# + ); + + let mut q = sqlx::query_as::<_, SidebarRow>(&query); + if let Some(cursor) = after { + q = q + .bind(cursor.updated_at.clone()) + .bind(cursor.updated_at.clone()) + .bind(cursor.id.clone()); + } + let pool = self.pool().await?; + q.bind(i64::from(limit)) + .fetch_all(pool) + .await + .map_err(Into::into) + } + async fn delete_session(&self, session_id: &str) -> Result<()> { let pool = self.pool().await?; let mut tx = pool.begin().await?; @@ -12562,6 +12724,87 @@ mod tests { assert!(summary.user_set_name); } + /// The keyset page, at the corner an offset page never had to think about: + /// every row sharing one `updated_at`. + /// + /// `updated_at` is `datetime('now')` — one-second granularity — so several + /// chats really do tie in practice, and the ordering breaks the tie by + /// `id ASC`. A resume predicate of `updated_at < :ts` alone would skip the + /// rest of the tied group; one of `<=` would repeat it forever. So the + /// boundary is `(< ts) OR (= ts AND id > last_id)`, and this walks a tied + /// group one row at a time to assert every row is seen exactly once. + /// + /// It also drives `public_only`, which is the security half: the rows a + /// filtered caller may not see never leave the database, which is what + /// leaves the continuation value with nothing hidden to count (adversarial + /// security review 2026-09-12). + #[tokio::test] + async fn a_keyset_page_walks_a_tied_updated_at_group_exactly_once() { + let temp_dir = TempDir::new().unwrap(); + let sm = SessionManager::new(temp_dir.path().to_path_buf()); + + let mut public_ids = Vec::new(); + let mut private_ids = Vec::new(); + for index in 0..6 { + let session = sm + .create_session( + temp_dir.path().to_path_buf(), + format!("tied {index}"), + SessionType::User, + ) + .await + .unwrap(); + sm.add_message(&session.id, &umsg(10, "hello")) + .await + .unwrap(); + if index % 2 == 0 { + public_ids.push(session.id.clone()); + } else { + sm.update(&session.id) + .raise_privacy(SessionClassification::Private, "turn:test") + .apply() + .await + .unwrap(); + private_ids.push(session.id.clone()); + } + } + + for public_only in [false, true] { + let mut seen = Vec::new(); + let mut cursor: Option = None; + for _ in 0..20 { + let page = sm + .list_session_summaries_page(1, cursor.as_ref(), false, false, public_only) + .await + .unwrap(); + let Some(row) = page.into_iter().next() else { + break; + }; + seen.push(row.summary.id.clone()); + cursor = Some(row.cursor); + } + + let mut sorted = seen.clone(); + sorted.sort(); + sorted.dedup(); + assert_eq!( + sorted.len(), + seen.len(), + "the keyset walk repeated a row: {seen:?}" + ); + for id in &public_ids { + assert!(seen.contains(id), "the walk skipped the public chat {id}"); + } + for id in &private_ids { + assert_eq!( + seen.contains(id), + !public_only, + "public_only={public_only} handled the private chat {id} wrongly" + ); + } + } + } + #[test] fn legacy_session_summary_json_defaults_user_set_name_to_false() { let summary: SessionSummary = serde_json::from_value(serde_json::json!({ diff --git a/crates/biorouter/tests/privacy_guard_wiring.rs b/crates/biorouter/tests/privacy_guard_wiring.rs index 3aaaa361..576fd6fb 100644 --- a/crates/biorouter/tests/privacy_guard_wiring.rs +++ b/crates/biorouter/tests/privacy_guard_wiring.rs @@ -371,15 +371,18 @@ const REGISTRY: &[Guard] = &[ }, Site { file: "crates/biorouter-server/src/routes/knowledge.rs", - counts: c(1, 6, 0), + counts: c(1, 7, 0), kind: SiteKind::Guard, what: "`POST /knowledge/bases/{id}/ingest-conversation`, one call inside the \ loop over the chats the request names, before any transcript is read: \ the route streams what a model makes of those chats back to its caller, \ and a caller holding only the daemon secret could name a private model \ - (QA 2026-09-10 H2). The other five refs are the MODULE qualifier on \ - `session_reach::gate_knowledge_base`, `http_caller` (three handlers) and \ - `HttpCaller` — names that live beside the gate, not the gate", + (QA 2026-09-10 H2). The other seven refs are the MODULE qualifier on \ + `session_reach::gate_knowledge_base`, `http_caller` (FOUR handlers \ + since the adversarial review of 2026-09-12 gated `POST \ + /knowledge/bases`) and `HttpCaller` — names that live beside the gate, \ + not the gate. `calls` is unmoved: the create gate asks \ + `mints_knowledge_base`, its own row below, not this function", }, Site { file: "crates/biorouter-server/src/routes/mod.rs", @@ -512,17 +515,23 @@ const REGISTRY: &[Guard] = &[ status: Status::WiredThrough("session_reach"), sites: &[Site { file: SESSION_REACH, - counts: c(4, 0, 0), + counts: c(5, 0, 0), kind: SiteKind::Guard, what: "`session_reach` itself, which is this predicate plus the two lookups that \ feed it; and since QA's 2026-09-10 sweep `HttpCaller::admits` (a listing is \ the rows this decision admits, one at a time — the one spelling that both \ `lists_session` and, for running work, `lists_work` are) and \ `HttpCaller::reach_knowledge_base` (the same decision with a knowledge base \ - as the target); and `work_reach`'s arm for running work that names no chat, \ - whose target is `Unreadable` because there is no chat to resolve. ONE \ - decision, four subjects: a second spelling of it is what this census exists \ - to stop", + as the target); `work_reach`'s arm for running work that names no chat, \ + whose target is `Unreadable` because there is no chat to resolve; and, from \ + the adversarial review of 2026-09-12, `HttpCaller::mints_knowledge_base` — \ + whether a caller may TAKE a base id, asked at `TargetTier::Unreadable` and \ + WITHOUT the id, because create refuses one that is taken and an answer that \ + varied with the id would say which private bases exist. ONE decision, five \ + subjects: a second spelling of it is what this census exists to stop. \ + ⚠ This row read `four` on both sides of the #237 merge — each branch added \ + a subject and neither could see the other's — which is exactly the arithmetic \ + this census is here to refuse to take on trust", }], }, Guard { @@ -543,11 +552,13 @@ const REGISTRY: &[Guard] = &[ }, Site { file: "crates/biorouter-server/src/routes/knowledge.rs", - counts: c(3, 0, 0), + counts: c(4, 0, 0), kind: SiteKind::Guard, what: "`GET /knowledge/bases` (a private base omitted from a caller that \ - cannot open it) and both halves of `/knowledge/active` (the selection \ - filtered, and a write unable to move what its caller cannot see)", + cannot open it), both halves of `/knowledge/active` (the selection \ + filtered, and a write unable to move what its caller cannot see), and \ + `POST /knowledge/bases` — which refused a taken id and so answered \ + whether a PRIVATE base held it (adversarial review 2026-09-12)", }, Site { file: "crates/biorouter-server/src/routes/schedule.rs", @@ -595,11 +606,14 @@ const REGISTRY: &[Guard] = &[ }, Site { file: "crates/biorouter-server/src/routes/session.rs", - counts: c(3, 0, 0), + counts: c(2, 0, 0), kind: SiteKind::Guard, - what: "`GET /sessions`, and `GET /sessions/sidebar` twice: once to take the \ - one-query fast path for a caller shown every row, once per row of the \ - scan that pages a filtered view without ragged pages or a count oracle", + what: "`GET /sessions`, filtered row by row, and `GET /sessions/sidebar` ONCE \ + — it now asks whether this caller is shown private chats and hands the \ + answer to SQL as a predicate. It used to ask per row of a scan over the \ + unfiltered ordering and resume from the position it reached, which made \ + the continuation value a count of the rows it hid (adversarial review \ + 2026-09-12)", }, ], }, @@ -670,6 +684,22 @@ const REGISTRY: &[Guard] = &[ }, ], }, + Guard { + ident: "mints_knowledge_base", + defined_in: SESSION_REACH, + decides: "whether a caller may take a knowledge-base id at all — asked WITHOUT the id, \ + because create refuses one that is taken and an answer that varied with the id \ + would say which private bases exist", + status: Status::Wired, + sites: &[Site { + file: "crates/biorouter-server/src/routes/knowledge.rs", + counts: c(1, 0, 0), + kind: SiteKind::Guard, + what: "`POST /knowledge/bases`, before `create_base_in` reads the id — which used \ + to answer a guessed private id with `400 … already exists at ` to a caller holding nothing but the daemon secret", + }], + }, Guard { ident: "reach_knowledge_base", defined_in: SESSION_REACH, diff --git a/docs/deployment/programmatic-session-access.md b/docs/deployment/programmatic-session-access.md index 01033722..874bebf3 100644 --- a/docs/deployment/programmatic-session-access.md +++ b/docs/deployment/programmatic-session-access.md @@ -177,6 +177,7 @@ one of them resolves the target's tier **before** it touches the session, so a r | `POST /reply` | Runs an agent turn, with tools, in the named session. | | `POST /agent/continuation/recover` | Resumes a parked continuation. | | `POST /agent/resume` · `restart` · `stop` | Lifecycle control of the session's agent. | +| `POST /agent/cancel` · `POST /agent/continuation/abandon` · `recover` | Stops or settles the session's running turn — **on a daemon that holds no user-action key only**, such as `biorouter serve` or a hand-run `biorouterd` ([SD-11](serve-decisions.md#sd-11--stop-works-on-a-daemon-with-no-key-steering-does-not-and-a-subagents-tab-stays-the-persons)). There they admit exactly the callers `POST /agent/stop` admits, a subagent's session excepted. `POST /interrupt` is **not** among them: it takes the proof on either kind of daemon, because injecting text into a turn already running is the one thing no other route on a keyless daemon can do. | | `POST /agent/update_provider` | Rebinds the model (also needs `X-User-Action` to raise a tier). | | `POST /agent/update_from_session` | Adopts another session's provider configuration. | | `POST /agent/update_working_dir` | Repoints the session at a directory. | @@ -205,7 +206,7 @@ the caller could not open: | Route | What a caller without the header or the proof gets | |---|---| | `GET /schedule/list` | **Every** schedule, including idle and paused ones — but with `current_session_id` and `creator_session_id` omitted from any row naming a chat the caller could not open. ⚠ The one listing that REDACTS FIELDS instead of dropping rows, because a schedule is not a chat: it names chats, and an idle one names none, so a row-level rule would empty the Schedules view for every ordinary caller rather than close an association. | -| `GET /sessions`, `GET /sessions/sidebar`, `GET /schedule/{id}/sessions` | The public chats only. A private chat is omitted, never redacted. It is not shown with its title removed. The sidebar still pages cleanly: follow `next_offset` as returned rather than computing it. | +| `GET /sessions`, `GET /sessions/sidebar`, `GET /schedule/{id}/sessions` | The public chats only. A private chat is omitted, never redacted. It is not shown with its title removed. The sidebar still pages cleanly: follow `next_cursor` as returned and do not parse it: it names the last row the page returned, so it is not a position and counts nothing that was left out. | | Every `/knowledge/bases/{id}…` route: pages, graph, history, location, export, preview, and the writes | A private base is refused with a knowledge-base twin of the chat refusal. A base that does not exist, and a malformed id, get the same refusal. | | `GET /knowledge/bases`, `GET`/`POST /knowledge/active` | The public bases only. A write to the selection cannot hide, reveal or unpin a base the caller cannot see. | | `GET /active_work` | The running work of public chats only. Each row carries its chat's `sessionId` and a `title` and `detail` holding the shell command or task prompt, which is the chat's content. A row whose chat is private, or cannot be read, is omitted. So is a row that names no chat at all (see below). | diff --git a/docs/security/privacy-tiers-execution-plan.md b/docs/security/privacy-tiers-execution-plan.md index b64b23fd..aeb5783a 100644 --- a/docs/security/privacy-tiers-execution-plan.md +++ b/docs/security/privacy-tiers-execution-plan.md @@ -7319,6 +7319,30 @@ stop: that is a different product decision and it is [Open question 15](#open-qu > proves nothing, and the user proves it on every request. Half (b) of > [Open question 15](#open-questions) is answered by this. Record: > [`privacy-tiers.md`](privacy-tiers.md#shipped), the knowledge-base tier entry. +> +> ⚠ **AMENDED 2026-09-12 by an adversarial review, which found the sentence above true and +> incomplete.** "Every route that names a base by `{id}`" was the right set to layer and the wrong +> set to stop at: `POST /knowledge/bases` names a base by `body.id` rather than by path, sat on the +> outer router, and **refuses an id that is taken** — so a caller holding only the daemon secret +> POSTed a guessed id and read `400 kb '' already exists at ` when a +> **private** base held it, and `200` when nothing did. KB ids are user-authored names, so a short +> dictionary enumerated by name exactly the bases `KNOWLEDGE_BASE_OUT_OF_REACH` exists to withhold. +> It is gated now by `HttpCaller::mints_knowledge_base`, which takes **no id** — that is what makes +> the answer the same for a private id, a public id and an id that has never existed — and neither +> error body on that route names a directory any more. What it costs is stated rather than hidden: a +> caller holding nothing but the secret can no longer create a knowledge base over HTTP, which is +> the same answer it already got for every private base on the machine. +> +> The second half of the same finding was about the layer itself. `Router::route_layer` is a +> **snapshot** — it wraps the routes present when it is called and, silently, nothing added +> afterwards — so "and any route added to it later is gated by construction", which +> `routes/knowledge.rs` asserted in as many words, was never something axum offered. Measured: a +> `{id}` route appended after that call answered **200 with a private base's manifest** to a +> secret-only caller. The sub-router is now built ungated by `base_routes()` and the layer is applied +> to its return value at its single call site, so appending a route there is gated and appending one +> at the call site is visibly outside the gate; +> `every_route_that_names_a_base_is_inside_the_gated_sub_router` reads the file rather than trusting +> either sentence, and also asserts that every gated route is actually driven by the H2 probe list. - [ ] **Step 1: Write the failing tests** diff --git a/docs/security/privacy-tiers.md b/docs/security/privacy-tiers.md index 9be473f7..102580c0 100644 --- a/docs/security/privacy-tiers.md +++ b/docs/security/privacy-tiers.md @@ -83,9 +83,22 @@ this section is the ledger. `/workflows/create`, `/skills/session`, and each chat `ingest-conversation` names. Each refuses with `GET /sessions/{id}`'s own words, and answers a chat that does not exist the same way. - **Chat listings.** `GET /sessions`, `/sessions/sidebar` and `/schedule/{id}/sessions` omit the - rows that gate would refuse. + rows that gate would refuse. ⚠ **Omitting a row is not enough if the pagination still counts + it.** The sidebar first filtered after fetching, and resumed each page from the position it had + reached in the *unfiltered* ordering — so two continuation values subtracted gave the exact + number of private chats between two visible ones, and because `updated_at` is stamped on every + token written, polling the route reported when a private chat was running. Since an adversarial + review on 2026-09-12 the tier is a **SQL predicate** (the rows never leave the database) and a + page resumes from an opaque keyset of the last row it *returned*, so there is nothing hidden + left to count. - **Knowledge bases.** One route layer covers every route that names a base by `{id}`, reads and - writes alike. An absent or malformed id is answered as a private one. + writes alike. An absent or malformed id is answered as a private one. **Creating** one + (`POST /knowledge/bases`, which names a base by `body.id`) is gated too, since the same review: + it refuses an id that is taken, so answering that for a private base enumerated the machine's + private bases by name — and named the config path while doing it. The gate is asked **without + the id**, which is what makes the answer the same for a taken private id, a taken public id and + an id that has never existed; the cost is that a caller holding nothing but the daemon secret + can no longer create a base over HTTP. - **Knowledge-base listings.** `GET /knowledge/bases` and `/knowledge/active` omit what the caller cannot reach, and a selection write cannot move a base its caller cannot see. - **Running work.** `GET /active_work` omits every row whose chat the caller could not open. A diff --git a/ui/desktop/openapi.json b/ui/desktop/openapi.json index 9a739622..2472aa41 100644 --- a/ui/desktop/openapi.json +++ b/ui/desktop/openapi.json @@ -270,7 +270,7 @@ "description": "Unauthorized - invalid secret key" }, "403": { - "description": "Refused by a privacy boundary: the same refusal, word for word, that `GET /sessions/{session_id}` gives (body = plain text)" + "description": "Refused by a privacy boundary (issue #56 Task 58 / #47): the named chat is private (or absent, and an unproven caller is told the same thing for both) and the request carried neither a capability that covers it nor proof it came from the user. It is the same refusal, word for word, that `GET /sessions/{session_id}` gives (body = plain text)" }, "424": { "description": "Agent not initialized" @@ -2003,6 +2003,9 @@ }, "400": { "description": "Duplicate id, invalid id, or unknown format" + }, + "403": { + "description": "This caller may not mint a knowledge-base id: it is a public model and the request carried no proof it came from the person at the keyboard. The same answer for every id, taken or free, so that creating is not a way to ask which private bases exist (body = plain text)" } } } @@ -4115,15 +4118,13 @@ } }, { - "name": "offset", + "name": "cursor", "in": "query", - "description": "Number of session summaries to skip", + "description": "The previous page's `next_cursor`, passed back unchanged. Omit for the first page; an unrecognised value is answered 400", "required": false, "schema": { - "type": "integer", - "format": "int32", - "nullable": true, - "minimum": 0 + "type": "string", + "nullable": true } }, { @@ -4139,7 +4140,7 @@ ], "responses": { "200": { - "description": "Paginated lightweight session summaries for the sidebar, holding only the sessions this caller could open (see `GET /sessions`). `next_offset` is where the next page starts; for a caller shown every session it is `offset + limit` as before, and for one shown a filtered view it is a position in the underlying ordering, so pass it back as given rather than computing it", + "description": "Paginated lightweight session summaries for the sidebar, holding only the sessions this caller could open (see `GET /sessions`). `next_cursor` is an OPAQUE continuation token: pass it back as `cursor` and do not parse it. It is not a position and not a count — it names the last row this page returned, so it says nothing about rows that were filtered out", "content": { "application/json": { "schema": { @@ -4148,6 +4149,9 @@ } } }, + "400": { + "description": "The `cursor` was not one this route issued" + }, "401": { "description": "Unauthorized - Invalid or missing API key" }, @@ -12465,11 +12469,10 @@ "has_more": { "type": "boolean" }, - "next_offset": { - "type": "integer", - "format": "int32", - "nullable": true, - "minimum": 0 + "next_cursor": { + "type": "string", + "description": "Where the next page resumes — pass it back as `cursor`, unchanged, and do\nnot parse it. `null` when this was the last page.", + "nullable": true }, "sessions": { "type": "array", diff --git a/ui/desktop/src/api/types.gen.ts b/ui/desktop/src/api/types.gen.ts index 102e28ae..1840d2f8 100644 --- a/ui/desktop/src/api/types.gen.ts +++ b/ui/desktop/src/api/types.gen.ts @@ -3504,7 +3504,11 @@ export type Severity = 'error' | 'warning' | 'info'; export type SidebarSessionListResponse = { has_more: boolean; - next_offset?: number | null; + /** + * Where the next page resumes — pass it back as `cursor`, unchanged, and do + * not parse it. `null` when this was the last page. + */ + next_cursor?: string | null; sessions: Array; }; @@ -4413,7 +4417,7 @@ export type GetCallableToolCountErrors = { */ 401: unknown; /** - * Refused by a privacy boundary: the same refusal, word for word, that `GET /sessions/{session_id}` gives (body = plain text) + * Refused by a privacy boundary (issue #56 Task 58 / #47): the named chat is private (or absent, and an unproven caller is told the same thing for both) and the request carried neither a capability that covers it nor proof it came from the user. It is the same refusal, word for word, that `GET /sessions/{session_id}` gives (body = plain text) */ 403: unknown; /** @@ -5789,6 +5793,10 @@ export type CreateBaseErrors = { * Duplicate id, invalid id, or unknown format */ 400: unknown; + /** + * This caller may not mint a knowledge-base id: it is a public model and the request carried no proof it came from the person at the keyboard. The same answer for every id, taken or free, so that creating is not a way to ask which private bases exist (body = plain text) + */ + 403: unknown; }; export type CreateBaseResponses = { @@ -7416,9 +7424,9 @@ export type ListSidebarSessionsData = { */ limit?: number | null; /** - * Number of session summaries to skip + * The previous page's `next_cursor`, passed back unchanged. Omit for the first page; an unrecognised value is answered 400 */ - offset?: number | null; + cursor?: string | null; /** * Include sub_agent sessions (grouped under parent_session_id); default false */ @@ -7428,6 +7436,10 @@ export type ListSidebarSessionsData = { }; export type ListSidebarSessionsErrors = { + /** + * The `cursor` was not one this route issued + */ + 400: unknown; /** * Unauthorized - Invalid or missing API key */ @@ -7440,7 +7452,7 @@ export type ListSidebarSessionsErrors = { export type ListSidebarSessionsResponses = { /** - * Paginated lightweight session summaries for the sidebar, holding only the sessions this caller could open (see `GET /sessions`). `next_offset` is where the next page starts; for a caller shown every session it is `offset + limit` as before, and for one shown a filtered view it is a position in the underlying ordering, so pass it back as given rather than computing it + * Paginated lightweight session summaries for the sidebar, holding only the sessions this caller could open (see `GET /sessions`). `next_cursor` is an OPAQUE continuation token: pass it back as `cursor` and do not parse it. It is not a position and not a count — it names the last row this page returned, so it says nothing about rows that were filtered out */ 200: SidebarSessionListResponse; }; diff --git a/ui/desktop/src/components/BioRouterSidebar/AppSidebar.test.tsx b/ui/desktop/src/components/BioRouterSidebar/AppSidebar.test.tsx index f24c06ec..fee82247 100644 --- a/ui/desktop/src/components/BioRouterSidebar/AppSidebar.test.tsx +++ b/ui/desktop/src/components/BioRouterSidebar/AppSidebar.test.tsx @@ -57,7 +57,7 @@ beforeAll(() => { beforeEach(() => { mocks.listSessions.mockResolvedValue({ data: { sessions: [session] } }); mocks.listSidebarSessions.mockResolvedValue({ - data: { sessions: [session], has_more: false, next_offset: null }, + data: { sessions: [session], has_more: false, next_cursor: null }, }); }); diff --git a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts index b2a86319..f8698c69 100644 --- a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts +++ b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.test.ts @@ -57,10 +57,10 @@ describe('useSidebarSessions', () => { const secondPage = [makeSummary(10), makeSummary(11)]; mocks.listSidebarSessions .mockResolvedValueOnce({ - data: { sessions: firstPage, has_more: true, next_offset: 10 }, + data: { sessions: firstPage, has_more: true, next_cursor: 'cursor-page-2' }, }) .mockResolvedValueOnce({ - data: { sessions: secondPage, has_more: false, next_offset: null }, + data: { sessions: secondPage, has_more: false, next_cursor: null }, }); const { result } = renderHook(() => useSidebarSessions()); @@ -68,7 +68,7 @@ describe('useSidebarSessions', () => { await waitFor(() => expect(result.current.sessions).toHaveLength(10)); expect(result.current.hasMore).toBe(true); expect(mocks.listSidebarSessions).toHaveBeenNthCalledWith(1, { - query: { limit: 10, offset: 0 }, + query: { limit: 10 }, headers: { 'X-User-Action': 'test-proof' }, throwOnError: true, }); @@ -78,7 +78,7 @@ describe('useSidebarSessions', () => { await waitFor(() => expect(result.current.sessions).toHaveLength(12)); expect(result.current.hasMore).toBe(false); expect(mocks.listSidebarSessions).toHaveBeenNthCalledWith(2, { - query: { limit: 10, offset: 10 }, + query: { limit: 10, cursor: 'cursor-page-2' }, headers: { 'X-User-Action': 'test-proof' }, throwOnError: true, }); @@ -93,13 +93,20 @@ describe('useSidebarSessions', () => { ]; mocks.listSidebarSessions .mockResolvedValueOnce({ - data: { sessions: firstPage, has_more: true, next_offset: 10 }, + data: { sessions: firstPage, has_more: true, next_cursor: 'cursor-page-2' }, + }) + .mockResolvedValueOnce({ + data: { sessions: secondPage, has_more: true, next_cursor: 'cursor-page-3' }, }) .mockResolvedValueOnce({ - data: { sessions: secondPage, has_more: true, next_offset: 20 }, + data: { sessions: refreshedFirstPage, has_more: true, next_cursor: 'cursor-page-2' }, }) .mockResolvedValueOnce({ - data: { sessions: refreshedFirstPage, has_more: true, next_offset: 10 }, + data: { + sessions: [makeSummary(20)], + has_more: false, + next_cursor: null, + }, }); const { result } = renderHook(() => useSidebarSessions()); @@ -116,7 +123,20 @@ describe('useSidebarSessions', () => { expect(result.current.sessions).toHaveLength(20); expect(mocks.listSidebarSessions).toHaveBeenNthCalledWith(3, { - query: { limit: 10, offset: 0 }, + query: { limit: 10 }, + headers: { 'X-User-Action': 'test-proof' }, + throwOnError: true, + }); + + // …and the refresh must not rewind the tail. `next_cursor` is opaque and + // names the last row of the page that issued it, so a refresh of the HEAD + // carries the cursor for page 2 — adopting it would make "Load more" refetch + // rows the list already holds and appear to do nothing. The furthest cursor + // wins. + act(() => result.current.loadMore()); + await waitFor(() => expect(mocks.listSidebarSessions).toHaveBeenCalledTimes(4)); + expect(mocks.listSidebarSessions).toHaveBeenNthCalledWith(4, { + query: { limit: 10, cursor: 'cursor-page-3' }, headers: { 'X-User-Action': 'test-proof' }, throwOnError: true, }); diff --git a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts index 7928dda3..a8c1e59c 100644 --- a/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts +++ b/ui/desktop/src/components/BioRouterSidebar/useSidebarSessions.ts @@ -38,7 +38,7 @@ export default function useSidebarSessions(): SidebarSessionsState { const [hasMore, setHasMore] = useState(true); const [isLoading, setIsLoading] = useState(true); const sessionsRef = useRef([]); - const nextOffsetRef = useRef(0); + const nextCursorRef = useRef(null); const hasMoreRef = useRef(true); const hasLoadedRef = useRef(false); const loadingRef = useRef(false); @@ -46,7 +46,7 @@ export default function useSidebarSessions(): SidebarSessionsState { const loadPage = useCallback(async (reset: boolean) => { if (loadingRef.current || (!reset && !hasMoreRef.current)) return; - const offset = reset ? 0 : nextOffsetRef.current; + const cursor = reset ? null : nextCursorRef.current; loadingRef.current = true; setIsLoading(true); @@ -54,7 +54,7 @@ export default function useSidebarSessions(): SidebarSessionsState { // With the user's proof: without it the daemon pages a view with every // private chat omitted (issue #56, QA 2026-09-10 M1). const response = await listSidebarSessions({ - query: { limit: SIDEBAR_SESSION_PAGE_SIZE, offset }, + query: { limit: SIDEBAR_SESSION_PAGE_SIZE, ...(cursor ? { cursor } : {}) }, headers: await userActionHeaders(), throwOnError: true, }); @@ -65,9 +65,16 @@ export default function useSidebarSessions(): SidebarSessionsState { sessionsRef.current = mergedSessions; setSessions(mergedSessions); - nextOffsetRef.current = reset - ? mergedSessions.length - : (page.next_offset ?? offset + page.sessions.length); + // `next_cursor` is opaque and names the last row of the page that + // returned it, so — unlike the offset this replaced — it cannot be + // recomputed from the list we hold. A refresh re-reads the HEAD of the + // list; the tail we already paged through is still held, and the cursor we + // already have still points just past it. So a reset keeps it, and only a + // list that has none (first load, or one that had reached the end) adopts + // the one this page carries. + nextCursorRef.current = reset + ? (nextCursorRef.current ?? page.next_cursor ?? null) + : (page.next_cursor ?? null); hasMoreRef.current = pageHasMore; hasLoadedRef.current = true; setHasMore(pageHasMore); @@ -113,14 +120,19 @@ export default function useSidebarSessions(): SidebarSessionsState { // renderer reload. Splice it out by id instead — exact, and with no risk of // evicting a live chat that merely fell out of the first page. // - // `nextOffsetRef` moves down with it: the server's list lost the same row, - // so leaving the offset alone would make the next `loadMore` skip one. + // ⚠ **Nothing is adjusted alongside it, and that is the keyset's doing.** + // This handler arrived while the page resumed from an OFFSET, and it had to + // decrement that offset by one: the server's list had lost the same row, so + // a position left alone would make the next `loadMore` skip a chat. A + // cursor names the sort key of the last row a page RETURNED, so it is a + // boundary compared against values, not a count of rows — the row it names + // does not have to exist for the comparison to put the next page in the + // right place, including when the deleted row is that very one. const unsubscribeRemoved = subscribeSessionRemoved((sessionId) => { const remaining = sessionsRef.current.filter((session) => session.id !== sessionId); if (remaining.length === sessionsRef.current.length) return; sessionsRef.current = remaining; setSessions(remaining); - nextOffsetRef.current = Math.max(0, nextOffsetRef.current - 1); }); window.addEventListener('session-created', scheduleRefresh); diff --git a/ui/desktop/src/components/knowledge/KnowledgeContext.tsx b/ui/desktop/src/components/knowledge/KnowledgeContext.tsx index e621b394..6c3f362c 100644 --- a/ui/desktop/src/components/knowledge/KnowledgeContext.tsx +++ b/ui/desktop/src/components/knowledge/KnowledgeContext.tsx @@ -237,7 +237,11 @@ export function KnowledgeProvider({ try { // With the proof, for the reason `fetchKnowledgeSelection` gives: a daemon that // filters what an unproven caller may see would otherwise hand this list - // back with the user's own private bases missing. + // back with the user's own private bases missing — and the effects below + // prune the selection AGAINST this list, so a missing base reads as "that + // base was deleted" and would be dropped from the primary and the hidden + // set. The daemon also refuses such a caller a move it cannot see + // (`set_selection_within`), but the list the user is shown must be whole. const res = await listBases({ headers: await userActionHeaders(), throwOnError: true }); setBases(res.data || []); setBasesLoaded(true); diff --git a/ui/desktop/src/components/knowledge/hooks/useKnowledgeBases.ts b/ui/desktop/src/components/knowledge/hooks/useKnowledgeBases.ts index 142aba9c..baaa2d07 100644 --- a/ui/desktop/src/components/knowledge/hooks/useKnowledgeBases.ts +++ b/ui/desktop/src/components/knowledge/hooks/useKnowledgeBases.ts @@ -15,6 +15,13 @@ export function useKnowledgeBases() { * that has no opinion still gets the daemon's default rather than the * renderer asserting one on its behalf — `CreateBaseBody.format` is * `Option` on the wire for exactly that reason. + * + * ⚠ **With the user's proof, like `remove` below.** Since the adversarial + * review of 2026-09-12, `POST /knowledge/bases` is gated like every other + * base-naming route: it refuses a taken id, and answering that for a private + * base told a caller holding only the daemon secret which private bases + * exist. A request that forgot the header is refused outright rather than + * degraded, so this is not optional. */ const create = useCallback( async ( @@ -24,6 +31,7 @@ export function useKnowledgeBases() { ): Promise => { const res = await apiCreate({ throwOnError: true, + headers: await userActionHeaders(), body: { id, name, @@ -75,6 +83,10 @@ export function useKnowledgeBases() { */ const remove = useCallback( async (id: string): Promise => { + // The proof, because `DELETE /knowledge/bases/{id}` is behind the base + // reach gate now. No selection write follows it: the daemon repairs the + // pointer when its base goes (QA F14/D2), and the renderer asserting one + // on its own is what #249 removed. await apiDelete({ throwOnError: true, path: { id }, headers: await userActionHeaders() }); await refresh(); },