fix(privacy): one reach gate for every HTTP route that names a chat or a knowledge base (QA H2, M1, M2, F0) - #237
Merged
Merged
Conversation
This was referenced Sep 11, 2026
Broccolito
added a commit
that referenced
this pull request
Sep 11, 2026
serve-decisions.md conflicted: main's #226 added SD-9 (the launch token works until the daemon stops) and made the same SD-1 route fix this branch made. SD-1 takes main's wording; main's SD-9 stays SD-9; this branch's record becomes SD-12, after it. SD-10 and SD-11 are claimed by open #237 and #240, so SD-12 duplicates no number on main or in any open PR. Every reference to this branch's record moves with it (code comments, the two OpenAPI 409 descriptions, the daemon's keyless warning, the docs and their anchors, CLAUDE.md). References to main's SD-9 are untouched. CLAUDE.md's '(SD-1..SD-9)' range is replaced by no number at all, since every record that lands would make it stale.
Broccolito
added a commit
that referenced
this pull request
Sep 12, 2026
…not the renderer's restraint #260 stopped the renderer calling this route for a subagent's chat, because it answers through `get_or_create_agent` and would mint a bare placeholder agent under the child's session id. The review's point is that routing a client around an ungated route leaves the route ungated: it had no gate of any kind — not `session_reach`, not the subagent refusal — so any caller holding the daemon secret could name any chat and have an agent materialised for it, with the route's own 424 reporting what it had found. Gated the way the tier-bearing session reads are (`GET /sessions/{id}`, `POST /agent/resume`): `session_reach` FIRST, before the agent is fetched. The ordering is the substance — a gate under the fetch would satisfy a status assertion and still create the agent, which is why the test asserts `peek_agent(..).is_none()` as well. This is the hazard `agent_add_extension` already spells out at its own gate. #237 does not cover it. `routes::session_reach` is already on `main` and `get_callable_tool_count` was not among its four call sites in this file, so there is no conflict to avoid. ⚠ The review's premise about the sibling is wrong and worth recording: `GET /agent/tools` is also ungated, deliberately — it is the unfiltered permission-editor surface, so a person can administer private tools a public model cannot see, and it is not on `session_reach`'s gated list. `callable_tool_count` is the opposite kind of route, the model-facing count after Gate E. The handler now takes `HeaderMap` and returns `ErrorResponse` rather than a bare `StatusCode`, so its 424s keep the `{"message": …}` shape every other refusal in the file has instead of changing envelope with the failure (`agent_not_initialized`). Fail-before: 424 where 403 is now required. The new 403 is pinned in the generated spec by `openapi_describes_the_agent_route_failures_clients_must_handle` — extended to carry a method, so a GET route can be asserted at all — and the contract is regenerated.
This was referenced Sep 12, 2026
… unverified The agent writing these was killed by a session rate limit mid-task, between "all checks pass" and splitting the work into three commits. Committed by the coordinator to preserve it; NOT re-verified in this state. Covers the three review findings on #237: - HIGH: `next_offset` was an index into the UNFILTERED recency ordering, so a caller shown only public rows could subtract consecutive cursors to recover the exact count of hidden private chats — and, because `updated_at` is stamped on every token, poll it for a live activity side-channel. - MEDIUM: `POST /knowledge/bases` answered "already exists at <path>" for a private base, an existence oracle plus a filesystem-path disclosure. - MEDIUM: the `route_layer` doc claimed routes added later are gated; axum's route_layer is a snapshot, so that guarantee is false. Re-run before trusting: privacy_capability, privacy_guard_wiring, knowledge_routes, and the regenerated OpenAPI/TS client.
Eleven conflicts, and two of them were substantive rather than textual. `routes/agent.rs` — both sides gated `GET /agent/callable_tool_count`, this branch through a wrapper and main (8437e94, the SD-8 review of #260) inline. The resolution keeps ONE gate: main's body and its fuller reasoning, in this branch's wrapper shape, because the gate has to hand back the refusal the way `session_reach` wrote it. Taking main's `?` into this route's `ErrorResponse` compiled and passed 526 tests, then failed the two that matter — the refusal came back as `{"message": …}` where every other gated route answers the PLAIN-TEXT bytes `GET /sessions/{id}` returns. One boundary, one body; the envelope is part of the boundary, and `get_tools` beside it has the same shape for the same reason. `useSidebarSessions.ts` — auto-merged with no conflict and did not compile. Main's deleted-chat handler (#264) decremented `nextOffsetRef`, which this branch had renamed; git took both halves. The decrement is now gone rather than renamed: it existed because an offset counts rows and the server's list had lost one, and a cursor names the sort key of the last row a page returned, so the boundary lands correctly even when the deleted row is the one it names. The rest: - `CLAUDE.md`, `serve-decisions.md` — this branch appended SD-10, main SD-11; both kept, in number order, and the CLAUDE.md range is now SD-1..SD-11. - `programmatic-session-access.md` — main's turn-control row placed with the other lifecycle rows; and the `next_offset` this branch renamed, which this file still spelled the old way. - `session_reach.rs` — main's SD-11 wording supersedes ours on `/agent/cancel` (SD-11 changed that fact); main's row joins the gated-list table beside the other helper-gated one; the "last two rows" sentence now NAMES those rows, since both sides appended after them and it had already stopped being true. `get_session_extensions` is dropped as an over-read control — this branch gated it, so main's row for it would now fail — and main's `reply.rs` `routes(` control is kept. - `privacy_guard_wiring.rs` — the `agent.rs` row is the union at c(6, 6, 0), not a second row: both sides gated `callable_tool_count` (one call either way) and this branch also gated `GET /agent/tools`; main's own text says SD-11 added no call in that file. - `KnowledgeContext.tsx` — main's #249 had already absorbed this branch's proof header and error handling into `refreshBases`/`fetchKnowledgeSelection`, so main's side stands, with the one thing only our comment said (a filtered list is pruned against, so a missing base reads as "deleted") folded in. - `useKnowledgeBases.ts` — our proof header on the delete, without our `setPrimaryKbId(null)`: main removed that deliberately, the daemon repairs the pointer, and `primaryKbId` is no longer in scope. - `openapi.json`, `types.gen.ts` — regenerated from the merged routes. Verified on the merged tree: privacy_capability 4, privacy_guard_wiring 3, biorouter-server --lib routes:: 528, knowledge_routes 68, biorouter --lib session::session_manager 215, frontend lint:check + format:check clean, vitest 306 over BioRouterSidebar + knowledge. `cargo fmt --check` clean. The OpenAPI spec and TS client regenerate to no diff.
…ch's `./scripts/clippy-lint.sh` is red on pristine `origin/main` in three independent ways, measured at 9096d37 in a detached worktree: `result_large_err` on `reply::authorize_steer` (SD-11a), `string_slice` on `commands/agent.rs:523`, and two baseline `too_many_lines` violations that are not in `clippy-baselines/too_many_lines.txt` (`handle_execute_code`, `backfill_privacy_from_recorded_provenance`). None of them is touched here — they are main's to fix, and an allow slipped into a security branch's merge is how that debt stops being visible. What IS this branch's, and is fixed: - `session_reach.rs` — the two source-scan helpers the WIP commit added index strings at offsets `str::find` returned. Those are char boundaries by construction, which the restriction lint cannot see, so they are allowed with that stated, as nine existing sites in this repo already do. - `knowledge/service.rs` — spelling the two refusals carefully enough to name no filesystem path pushed `create_base_as_with_checkpoint` past `too_many_lines`. The stale-registry refusal is lifted into `refuse_if_the_id_is_registered`, which is the part of that function that was never about creating anything. After this the merged tree's baseline violations are EXACTLY main's two, set for set — the branch contributes none. Worth knowing before anyone treats the above as a CI regression: CI does not run this script. `rust.yml`'s clippy step is labelled "informational until warnings are burned down" and runs `cargo clippy --workspace --all-targets --locked` without `-D warnings` and without the baseline check, so none of these gate a pull request today. Verified after the change: biorouter-mcp --lib knowledge:: 799, biorouter-server --lib routes:: 528, privacy_capability 4, privacy_guard_wiring 3, knowledge_routes 68, cargo fmt --check clean.
Merged with `-X diff-algorithm=patience`, which is what kept `session_reach.rs` to four hunks of ≤14 lines instead of one interleaved block: 20 hunks over 10 files, and the two branches had already agreed about the thing that mattered most. **`callable_tool_count` needed no reconciling.** #257 and #237 resolved it identically — main's body inside a wrapper that returns the refusal through `SessionOutOfReach`'s own `IntoResponse`, plain text, not `?` through this route's JSON `ErrorResponse`. All three hunks in `routes/agent.rs` were comment-only; the code matched line for line. One gate remains, and it is that one. The two ⚠ notes are combined: the integration side names the mechanism and where the rule is written down, this branch's names the sibling with the same shape (`get_tools`) and the test that fails on the wrapping alone with the words unchanged. **The census caught what neither branch could see.** `refuse_unless_reachable` was declared `c(4, 0, 0)` on BOTH sides and the count auto-merged clean, because each branch had added one subject and written "four": #257/#278 added `work_reach`'s no-chat arm (and folded `lists_session`/`lists_work` into `HttpCaller::admits`), #237 added `mints_knowledge_base`. Measured on the merged tree: **5 calls**. Corrected to `c(5, 0, 0)` by running the census, not by counting with grep. The other rows needed no number: `agent.rs` stayed `c(6, 6, 0)` and `knowledge.rs` auto-merged to `c(1, 7, 0)`, where only this branch's prose explains the seventh ref — `http_caller` is called from four handlers now that `POST /knowledge/bases` is gated. One row per file throughout; `lists_work`, `work_reach` and `mints_knowledge_base` are three separate registry entries, kept in the file's thematic order. `routes/session_reach.rs`, four hunks: the module header takes this branch's correction (the sidebar does NOT page by scanning any more, and `has_more` cannot count what it hid because the tier is a SQL predicate and the page resumes from a keyset) *and* the integration's `GET /active_work` addition. The other three take the integration side, which is a strict superset: it counts three helper-gated rows and seven routes where this branch counted two and six, and it names `reply::steer_refusal`. `authorize_steer` no longer exists, so every mention of it here was stale — SD-11's settled ruling is that `/interrupt` asks for the proof on both kinds of daemon. Docs: `serve-decisions.md` keeps SD-1..SD-13 in number order, nothing renumbered, no duplicate heading. `CLAUDE.md` keeps the numbering-free "(the `SD-n` records)" — this branch's "SD-1..SD-11" was already wrong at SD-13. `programmatic-session-access.md` keeps the integration's `/schedule/list` field-redaction row and `/active_work` section, with this branch's `next_offset` → `next_cursor` correction. The execution-plan note takes the integration's fuller form, which carries the anchors and says why AR-15 is closed. `openapi.json` and `types.gen.ts` were not hand-resolved: one side taken, then `generate_schema` + `openapi-ts` run, and a second run confirmed zero diff. Verified on the merged result, BIOROUTER_DISABLE_KEYRING=true throughout: biorouter-server --lib routes:: 544 · knowledge_routes 68 · privacy_capability 4 · privacy_guard_wiring 3 · privacy_toggle 1 · privacy_disclosure_toggle 4 · biorouter --lib privacy:: 246 · cargo fmt --check clean · frontend lint:check and format:check clean · vitest 5293 passed, 1 skipped, 0 failed over 470 files. The `declassify` DEFERRED-transaction race did not fire in this run.
…anch already holds Content-free by construction: `git diff b1d3232 origin/main` is empty, because #275 merged the integration branch this PR was already merged with. Recorded so the branch CONTAINS main in its ancestry and not merely in its content, which is what stops a later read of this PR coming back DIRTY. Tree unchanged — verified by comparing the tree hash either side of this commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What QA measured (merged
main7c96d79, 2026-09-10,X-Secret-Keyonly)GET /knowledge/bases/{id}/page,/pages,/pages/{path},/graph,/history,/export,/locationandGET /knowledge/baseshanded a private base's content to a caller holding only the daemon secret. The tool path refused the same caller.GET /sessionsreturned every row, private ones included. This was the open question insession_reach.rs.GET /agent/tools?session_id=<private>listed a private chat's extension tools.GET /sessions/{private}returned 403, butDELETE /sessions/{private}returned 200 in 4 of 4 tries.The root cause is the same for all four. The axum handlers call the services directly, and the plan's scope note ("the Knowledge view is the user, not a model") treated holding the secret as proof that a human was asking. It is not: the agent can recover the secret (AR-11).
The fix: one gate, applied everywhere
Every HTTP route that names a chat or a knowledge base now asks the same question the single-chat read asks, through
routes::session_reach:X-User-Action) keeps full access, and so does one that states a private capability (X-Caller-Provider).Where it is wired:
route_layer(gate_knowledge_base) covers a sub-router that holds every/knowledge/bases/{id}…route: reads, writes, macros, tier, merge, restore, export, delete and the credibility override. A route added there later is gated automatically.GET /knowledge/basesandGET|POST /knowledge/activeare filtered, and a write to the selection can only change what that caller can see (KnowledgeService::set_selection_within).ingest-conversationgates each session it reads.session_reach.rs. Refusing the whole list would break every client for public chats, which the gate is designed to leave alone.GET /sessions,/sessions/sidebar(paged correctly across the filter) and/schedule/{id}/sessionsshow each caller exactly the rows the single-chat gate would let it open. Rows are omitted, not redacted.GET /agent/tools?session_id=and/agent/callable_tool_countgo through the gate.DELETE /sessions/{id}is gated beforecancel_turn. The sweep also gated rename,user_workflow_values, in-placeedit_message,/sessions/{id}/extensions,/sessions/{id}/usage,POST /workflows/create(which reads the transcript) andPOST /skills/session.userActionHeaders()), so the desktop app works as it did before. The OpenAPI spec and the generated client are regenerated.biorouter serve: SD-10 (new decision record)A
servedaemon holds no proof, because its stdin is closed (SD-7). A request that carries the served page'sHttpOnly; SameSite=Strictcookie gets the tier of the provider the operator configured (SD-1) on listings and knowledge bases only. A private provider therefore keeps the History list and the Knowledge view it had before. A public provider, or anything holding only the secret, is treated as public.The transcript gate never reads this standing. Opening, exporting, deleting or renaming a private chat from the browser is refused exactly as before. Whether a serve operator should reach private transcripts is recorded as an open decision and is not taken here. The cookie narrows a caller that already holds the secret; it never admits anyone, so there is no CSRF surface. Details:
docs/deployment/serve-decisions.mdSD-10.Interactions with other open PRs
X-Caller-Provider, and the transcript routes judge that statement the same way they judge any caller's. SD-10's cookie standing still never reaches a transcript. fix(serve): Stop and steering work on a daemon with no user-action key (SD-11) #240 took SD-11.userActionHeaders()to the same three renderer calls:MentionPopover.tsx,KnowledgeContext.tsxandCreateWorkflowFromSessionModal.tsx. Whichever PR merges second gets a textual conflict. Resolve it by keeping oneheaders: await userActionHeaders()per call. Desktop: read a private chat's knowledge selection with the user's proof (KB chip, / palette, workflow capture) #235 deliberately leaves the machine-widegetActivewithout the proof, because onmainthat call is not gated. Under this PR the machine-wide selection is filtered for a caller without the proof, so this PR sends the proof there too. Keep that side.tests/*.rsbinary in CI). If it lands, it picks up the newserve_operator_reachbinary and theh2_http_barriermodule. The binary needs no network and must stay in a process of its own, because the serve standing is a process-globalOnceLock.session_reachunchanged, so that phrase means the same thing on both branches.Refused before, and still refused
No
floor,raise_privacyor.call_tool(call site was added. The privacy wiring census passes with the new guards registered.session_reachitself is unchanged, and a source-scan test pins that it never reads the serve standing.Evidence
Runtime: before and after on my own sandboxed daemons
Both daemons ran against identical fixtures: one private knowledge base (
rt-private-probe), one private chat (imported), and empty data otherwise. The config came from the seed and the digest was piped on stdin. Every row below used the secret only:GET /knowledge/bases/{kb}/pageGET …/pagesGET …/pages/{path}GET …/graphGET …/historyGET …/export.brkb)GET …/locationGET /knowledge/basesGET /sessionsGET /agent/tools?session_id={private}GET /sessions/{private}DELETE /sessions/{private}X-User-Action), after: every row returns 200, the transcript included, andDELETEreturns 200.GET /sessions/{id}'s refusal (4/4, including a missing id).Desktop app
I ran a sandboxed dev GUI from this worktree on CDP port 9391, with this branch's
biorouterd:GET /sessions/{id}carriesX-User-Action.biorouter serveSame fixtures, CLI and daemon from this branch, driven in a headless browser:
versa_azure). The daemon logscapability=Private. History lists the private chat. Opening it shows the SD-7 keyless refusal ("Nothing was read and nothing was changed … use the desktop app."). The Knowledge view reads the private base.claude_code). The daemon logscapability=Public. History shows no chats, and the private base's name appears nowhere in the Knowledge view. The stored machine-wide selection (.active-kb, which still names the private base) was not rewritten while this caller was browsing.Fail before
These are the new tests run against base sources (logs kept locally):
routes::session_reachlib tests: 6 failed, 1 passed.GET /sessions/{id}refused a secret-only caller while 20 routes answered it differently:DELETE …200,PUT …/name200,GET /agent/tools?session_id=200 with tool names,…/extensions200,edit_message200, and more.knowledge_routes::h2_http_barrier: 4 failed. The private page was served with 200 where 403 was expected, andingest-conversationanswered 400 where 403 was expected.Mutation checks on the new lib tests
Suites
BIOROUTER_DISABLE_KEYRING=true)cargo fmt --all -- --checkcargo test -p biorouter-server --lib --binscargo test -p biorouter-server --test knowledge_routes --test privacy_toggle_config --test serve_operator_reachcargo test -p biorouter --lib privacy::cargo test -p biorouter-mcp --lib knowledge::tiercargo test -p biorouter --test privacy_guard_wiring(census)./scripts/clippy-lint.shjust generate-openapi+npm run generate-apiui/desktop:npm run lint:check,npx vitest run⚠ Until #239 lands, CI runs only
cargo test --workspace --lib --bins(rust.yml:261), so integration binaries are not what keeps these gates in place. That includesknowledge_routes,serve_operator_reachand the privacy wiring census. The knowledge-base route sweep and the serve standing therefore also have copies inroutes::session_reach's lib tests, driven throughroutes::configure. Clippy caught one of those copies failing to build in the bin target;--libalone would not have.Documentation
docs/security/privacy-tiers.md("What shipped")docs/deployment/serve-decisions.md(SD-10),browser-access.md,serve-architecture.mdandprogrammatic-session-access.md(the routes that honour the header, and the listing and knowledge-base table)CLAUDE.mdbullet: holding the daemon secret does not make a caller the userLeft open on purpose (documented as residuals)
GET /sessions/running,/sessions/changes, insights/activity,GET /active_workand the skills catalog were not gated here. They are listed inprogrammatic-session-access.md.servedaemon, the History empty state reads "No chats yet" even when private chats exist. The copy could say that some chats are hidden.Security-sensitive (auth and privacy gates), so it needs human review. Do not merge without it.
🤖 Generated with Claude Code