Gateway: policy engine, audit log, approvals and the first write tool - #408
Merged
Merged
Conversation
Task 1 of Gateway PR 4 (v3.5 approvals): commands/gateway/audit.rs adds AuditLog, appending one JSON line per gateway tool call to ~/.onebrain/gateway/audit/YYYY-MM.jsonl (0700 dir, 0600 files, home resolved via crate::home::home_dir — mirrors auth/store.rs's persistence discipline exactly). append(&self, entry) is infallible from the caller's view: it returns (), and any serialize/open/write/permission failure becomes a single tracing::warn! instead of a panic or propagated error, since the tool call an entry describes has already happened by the time append runs. AuditEntry.args_summary is documented as a caller-redacted, one-line description — never a raw note body, token, pairing code, or auth code — so later tasks wiring the approval gate can't accidentally log a secret through it. 6 unit tests: two-entry round trip with lowercase Decision/Outcome enum renames, month-boundary file separation, reopen-appends-not- truncates, 0700/0600 permission asserts, re-assert on a pre-existing looser dir, and an unwritable-dir append that proves the infallible contract without panicking. Zero new crate dependencies. This module has no external caller yet (Tasks 2-6 wire it up), so it carries a #![allow(dead_code)] mirroring auth/mod.rs's own precedent.
… TTL grants Adds RiskClass/PolicyMode/PolicyConfig/GrantKey/Grants/decide (policy.rs), wires GatewayConfig.policy (hand-written Default updated), and closes the gap where Principal.scope was issued and dropped on the floor: every existing tool now reads Principal via the Extension<http::request::Parts> seam, runs through decide, and gets audited per call. All four existing tools stay ReadOnly -> auto (no behavior change) but now flow through the policy/audit path end to end.
…on handling Task 1 review fixes: restore path.display() in audit.rs's tracing::warn!/ .context() calls (operator-only diagnostics are exempt from the "no secret or host path" constraint that governs client-facing responses and test messages; auth/store.rs already sets this precedent), re-assert the audit dir's 0700 on every append (not just at open_at, matching write_json_atomic's per-write cadence), and add direct tests for the out-of-range-ts month-file fallback and for append re-tightening a pre-existing loose month file back to 0600.
Gateway PR 4, Task 3: an in-memory, per-process human-approval registry (gateway/approval.rs — register/list/resolve/wait, oneshot-channel-backed, never holds its mutex across an await) plus its operator-only HTTP surface (gateway/approval_routes.rs — GET/POST /approvals, gated by the gateway's pairing code via X-OneBrain-Pairing, never by a connector's OAuth bearer token — the privilege-separation property the whole gate rests on). Also satisfies Task 2 review's three binding requirements: - Grants::record now uses saturating_add and gets its first production caller (resolve_approval, on Decision::Approve, using a config-derived TTL). - server.rs's extract_principal failure path now always leaves an audit entry instead of silently vanishing via an early `?` return. - record_audit now off-loads its blocking file write to tokio::task::spawn_blocking, consistent with this file's other filesystem work.
Adds a second channel that resolves the same Approvals registry Task 3 built: a native `display dialog` shown via `osascript`, alongside the `/approvals` HTTP surface. `is_available()` gates on macOS + osascript resolvable on PATH; `prompt()` runs the blocking osascript spawn inside tokio::task::spawn_blocking and calls Approvals::resolve on an explicit button click, relying on resolve's existing first-response-wins semantics rather than adding coordination between the two channels. client_id/tool/summary are attacker-influenceable, so every interpolated value is run through an AppleScript string-literal escaper before being embedded in the -e script text — same discipline as PR 3's html_escape for the /authorize consent page. Verified the escaper's correctness against real osascript (via `return`, never `display dialog`, to avoid any GUI-blocking risk) for the brief's do-shell-script injection payload, a backslash-heavy variant, and an embedded-newline variant. No production caller wires this into policy_gate yet, mirroring register/wait's own "later task" situation from Task 3.
Adds brain_capture, the gateway's first WRITE tool, and wires
policy_gate's NeedApproval arm to the Approvals registry (Task 3) and
the native macOS dialog channel (Task 4) that both previously had no
production caller.
brain_capture (RiskClass::Mutating, read_only_hint: false): derives
<inbox>/YYYY-MM-DD-<slug>.md from title (falling back to text, then a
fixed marker, for empty/punctuation-only/pure-unicode input; ASCII-only
sanitization strips slashes and dots so a traversal-shaped title never
survives to become a path segment), confines it via the new
resolve_create_under_vault (a sibling to resolve_under_vault: that
function canonicalizes the TARGET, useless on a create, so this one
canonicalizes the PARENT instead, after a syntactic Normal-components-only
check that rejects an absolute or ../-laden path before any filesystem
call), writes it with onebrain_fs::note::new_note (tags/created
frontmatter) + append_note (the actual body — new_note has no free-form
content parameter), and best-effort reindexes it via the daemon so
brain_search finds it immediately without making a reindex hiccup fail an
already-successful write.
await_approval (policy_gate's NeedApproval arm): registers a
PendingApproval, fires the native dialog when available, and awaits
Approvals::wait up to the new PolicyConfig::approval_wait_seconds (a
separate knob from grant_ttl_minutes — how long a call blocks for a first
decision vs. how long the resulting grant then lasts). On Approve it
records a Grants entry itself, in the waiter, so both resolution channels
(HTTP /approvals and the native dialog) honor ask_once identically even
though only the HTTP channel's own resolve_approval also records one.
GatewayState.approvals is now Arc<Approvals> (was a bare Approvals) so
approval_native::prompt's spawn_blocking closure can own a 'static
handle without widening its signature to the whole GatewayState.
audit::Decision::Blocked is removed — the state it named ("NeedApproval
with no channel to route to") no longer exists now that one is wired;
every outcome is Approved, Denied, or TimedOut instead.
…ve-channel disable switch
Gateway PR 4, Task 6: capabilities now reports each brain-pack tool's
RiskClass and the EFFECTIVE PolicyMode the live gateway.yml resolves it
to (server::ToolInfo, via the now-pub PolicyConfig::mode_for), plus an
approval_channels object (native/http/telegram) naming which approval
channels can actually deliver a prompt on this machine right now —
never told a write can be approved when no channel can carry the
prompt to a human.
approval_native.rs gains ONEBRAIN_GATEWAY_DISABLE_NATIVE_APPROVAL, an
env var checked first in is_available(), before the platform/osascript
probe. This is the mechanism that keeps tests/gateway_approval_e2e.rs
(the new e2e capstone, spawning a real onebrain gateway run subprocess
under policy.mutating: ask_once) from ever popping a real, unattended
osascript dialog on a macOS CI runner: cfg!(test) only ever protects
this crate's own #[cfg(test)] unit tests, never a separately spawned
binary. The env var is set in the spawned process's environment
instead, and is_available()'s truthful read of it flows straight
through to capabilities' approval_channels.native field, so a disabled
channel is reported honestly too. It covers ONLY the native dialog —
capture_note's best-effort daemon-reindex spawn is a separate,
pre-existing code path this switch does not touch.
The e2e test drives the full real flow over HTTP: OAuth token, a
brain_capture call that genuinely blocks under ask_once, GET
/approvals showing the pending entry, POST /approvals/{id} to
approve, the note landing on disk with the right content, a second
capture within the grant TTL needing no approval, both calls in the
audit log with the right decisions, and a connector's own bearer
token rejected on /approvals (GET and POST) — the privilege
separation property asserted end to end, not just at the router-fixture
level.
RiskClass and PolicyMode gain schemars::JsonSchema derives for
ToolInfo's structured-output schema.
…ge residuals docs/gateway.md gains a Policy & approvals section: the policy: block and its four modes, the two shipped approval channels (native macOS dialog, operator /approvals HTTP surface) plus Telegram's deferral to Gateway PR 5, capabilities' truthfulness contract and the ONEBRAIN_GATEWAY_DISABLE_NATIVE_APPROVAL test-only escape hatch, the audit log's on-disk location/format, and brain_capture itself. docs/reference/mcp.md's gateway tool table gains brain_capture and its risk class, plus a policy/approvals/audit summary pointing at docs/gateway.md for detail. CHANGELOG.md's [3.5.0] Added section gains bullets for the policy engine + approvals + audit trail, brain_capture, and capabilities truthfulness — none of Gateway PR 4's prior tasks had touched the changelog yet. docs/coverage.md documents two residuals surfaced by the full verification pass (94.54% total line, gate 94, no exclusions added): capture_note's best-effort reindex block joins the existing subprocess-capture-gap bullet (proven functionally by gateway_approval_e2e.rs, which leaves a real daemon process running against its tempdir vault), and a new entry for approval_native.rs's run_dialog/prompt bodies, which stay genuinely uncovered on purpose — no test, in-process or e2e, ever invokes a real osascript dialog, by design.
…pture Gateway PR 4 fix wave (F1-F21) on top of Task 6. Security / correctness: - The create-safe path guard no longer throws away the confined path it computes. resolve_create_under_vault gains a third layer: the confined path must EQUAL canonical_root.join(rel), which is the join new_note and append_note actually perform with no confinement of their own. What used to hold by inference from layer 1 is now an enforced invariant, and the returned path is used rather than discarded (it is what the same-day collision pre-check tests). Deliberate behavior change: an inbox symlinked to another directory INSIDE the vault now fails closed, since the guard did not vouch for the path the write would open. The doc comment also now states plainly what the guard does NOT cover — the parent is canonicalized before the write, so a parent swap in that window needs handle-relative I/O in onebrain-fs. - Grants are keyed (client_id, vault, RiskClass), not (client_id, RiskClass). The gateway is multi-vault and the human is always shown a vault when they approve, so approving one capture into ob-1 no longer silently authorizes Mutating writes into every other configured vault for grant_ttl_minutes. PendingApproval carries the vault so the HTTP channel records the same key the waiter would. The tool name stays deliberately OUT of the key; that choice is now written down in policy.rs so it is not re-litigated. - Pending approvals are bounded: 16 globally, 4 per client. Past either limit a gated call is refused with a policy error instead of queueing another human prompt — Task 5 is what made an unbounded fan-out of blocking osascript dialogs reachable. register prunes already-expired entries first, so a cancelled call cannot permanently consume a slot. - ask_always never records a grant, on either resolution channel. decide already ignores grants in that mode; leaving one in the map for a mode meaning "ask every time" is a trap for the next refactor. No test may spawn a daemon: - ONEBRAIN_GATEWAY_DISABLE_DAEMON_REINDEX joins Task 6's ONEBRAIN_GATEWAY_DISABLE_NATIVE_APPROVAL as the second external switch, gating capture_note's best-effort reindex. Task 6 confirmed empirically that the e2e left a real `onebrain daemon __run` process behind; cfg!(test) cannot reach a spawned binary and ONEBRAIN_NO_DAEMON gates only the passive routing path, never ensure_running. Proven three ways: the in-crate gate, a unit-test capture that must leave no $HOME/.onebrain/run/ behind, and the same assertion in the e2e against the real subprocess (which also dropped from ~29s to ~5s). - The reindex is also DETACHED (JoinHandle dropped, not awaited) so a capture no longer holds the MCP call open for a daemon cold start after the result is already determined. The gate is the outer condition, so a disabled channel spawns nothing at all and the e2e assertion is deterministic rather than racing a detached task. Client-facing behavior: - The collision error is this crate's own message naming the vault-relative path, no longer onebrain-fs's "(use --force)" — a CLI flag no MCP client can pass. - Empty or whitespace-only `text` is rejected as invalid_params instead of writing a titled, bodyless stub. - approval_wait_seconds: 0 stays legal (tests use it) and is documented as fail-closed-immediately; gateway run now warns at startup when a loaded config carries it. - capabilities no longer calls the brain pack "Read-only" in the same payload that reports brain_capture as mutating. - The env switches follow this crate's existing presence-switch convention (non-empty = on, set-but-empty = unset), shared via gateway::env_switch_on. Docs and stale comments: brain_pack_tools is described honestly as a hand-synced parallel list (with a new test pinning its name set against the router's), the audit log's `denied` row distinguishes a human refusal from a policy refusal, a dead #security-posture anchor is repointed, and the WaitOutcome/approval/audit module docs no longer describe a world with no production caller and a Blocked variant that was deleted. Coverage ratchet reviewed and deliberately left at 94: measured 94.53%, so 95 would fail outright and 94.5 would leave less headroom than the platform jitter the ratchet exists to absorb. Reasoning recorded in scripts/coverage.sh. Verification: 3683 passed / 0 failed across 52 suites; clippy clean under +1.98.0 --workspace --all-targets and the lex-only --no-default-features config; cargo fmt --check clean; coverage gate exit 0.
…ound, tracing, unicode slugs Six findings that survived three whole-branch review lenses and three independent skeptics each. A. The native approval dialog was never time-bounded, so the registry caps did not bound dialogs. `build_dialog_script` now emits `giving up after <n>`, sized to the pending entry's own remaining TTL, so a dialog dies when the waiter gives up rather than living forever and pinning a `spawn_blocking` thread. Clamped at both ends against behavior verified on a real osascript: `giving up after 0` means NEVER give up, and an operand above i32::MAX fails coercion and shows no dialog at all. The two documentation claims that said the caps prevented dialog fan-out now say what the caps actually bound (concurrent entries) and what bounds dialog lifetime (this clause). B. The operator pairing gate had no brute-force limiter, unlike the route that checks the same secret. `require_pairing_header` now goes through a new `AuthCtx::check_pairing_code` — the single sanctioned way to compare a pairing code — sharing `/authorize`'s counter, lockout and log line. One secret, one budget. C. `args_summary` is bounded at the two points where it is recorded (the audit entry and the pending-approval summary), not at each construction site. A read-only, auto-policy tool could otherwise land a multi-megabyte caller argument in an unrotated log, once per call. D. `gateway run` installed no tracing subscriber, so every operator diagnostic this PR relies on was discarded — including the full errors behind deliberately-sanitized client messages. Installs one following `daemon::init_tracing`'s conventions. The three integration harnesses no longer interpolate the gateway's whole stderr into a panic message, since it now carries diagnostics that may name host paths. E. Non-ASCII captures all collapsed to one filename, so only one could succeed per day. Slug charset widened to Unicode alphanumerics, matching `onebrain_fs::note::new`'s own helper. The confinement argument is re-established by test for the wider charset rather than inherited, and the length cap is now byte-aware. Repeated fallbacks get a random disambiguator; slugs derived from real input stay deterministic. F. `denying_does_not_record_a_grant` was vacuous — its class mapped to ask_always, so the inner guard suppressed the grant regardless of the decision. Rebuilt on a class that maps to ask_once, and verified by deleting the decision check: the old test passed the whole suite, the new one is the single failure.
… close secret leaks
Four test/doc items from the final verification review. No product code
changes shipped behavior; the only src/ edit is test names and comments.
1. Startup diagnostics survive again in all three gateway e2e harnesses.
The round-2 wave stopped `wait_for_gateway_url` from interpolating raw
stderr, but replaced it with a byte count — and the capture files live in
a TempDir deleted during the panic unwind, so a startup failure left no
diagnostic anywhere. Now stderr goes through a shared redactor
(`tests/support/mod.rs::redacted_capture_tail`): any token containing a
path separator is cut at the first one, pairing-code-shaped tokens are
replaced, and the tail is bounded to 12 lines / 1200 bytes. Stdout is
still never emitted at all — it is the one place the pairing code is
shown. The timeout branch gets the same tail, not just the exit branch.
Chosen over copying the file outside the TempDir or `TempDir::keep()`:
both of those have to name a host path to be useful, which this branch
forbids outright.
2. Docs and a test name no longer claim filename parity with
`onebrain note new`. `onebrain_fs::note::new` derives no filename from a
title at all (the caller passes the path; the title comes from its stem,
and its `slug` only fills `{{slug}}`), and where comparable the two still
differ: the gateway re-filters lowercase output and caps at 60 chars /
120 bytes. Restated as charset parity, with the three differences listed.
3. docs/gateway.md now states the adversarial direction of the shared
pairing-code lockout: an attacker with no OAuth token can hold the
operator out of /approvals and /authorize indefinitely, so a persistent
401 on a known-correct code is a recognizable symptom. Points at #404.
4. The two `read_pairing_code` helpers no longer interpolate pairing.json's
contents (which IS the pairing code when the file is truncated) or its
host path into panic messages.
…on every OS capture_collision_message used Path::display(), which emits the OS-native separator — so the same collision named 00-inbox/x.md to a client on unix and 00-inbox\\x.md on Windows, while brain_capture's success path already returned a to_slash'd path. One client could get two spellings of the same note depending on which machine the gateway ran on. onebrain-fs already carries to_slash for exactly this class of bug; the collision path just wasn't routed through it. The existing end-to-end test asserted 00-inbox/ but display() satisfies that on unix, so only Windows CI caught it. The new unit test builds the path from components, so a regression fails everywhere.
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.
Phase ④ of the v3.5 Gateway epic. The gateway can now say no.
Until this PR every tool the gateway exposed was read-only, so "should this call be allowed" had no teeth behind it. This adds the machinery that answers that question — a policy engine, an append-only audit trail, a human-in-the-loop approval path with two delivery channels — and then the first write tool to actually exercise it.
Spec:
01-projects/onebrain/shared/2026-08-28-remote-mcp-gateway-design.md§5.3/§5.4/§6, §9 phase ④.Follows #399 (rmcp 3.0.1), #401 (skeleton + Brain pack), #403 (OAuth 2.1 + pairing).
What's in it
Policy engine. Every tool carries a risk class (
ReadOnly/Mutating/Destructive); every class maps to a mode (auto/ask_once/ask_always/deny).decideis fail-closed by construction — an exhaustive match with no catch-all, so a future mode will not compile until every site handles it. An approvedask_oncerecords a TTL grant keyed by(client_id, vault, class).Audit log. Append-only JSONL at
~/.onebrain/gateway/audit/YYYY-MM.jsonl, oneO_APPENDwrite per entry,0600re-asserted on every append. Appending can never fail a tool call — the signature returns(), so that is a property of the type rather than a promise in a comment. Argument summaries are redacted and length-bounded; note bodies never appear.Approvals. A registry with a bounded set of pending requests (16 global, 4 per client), reachable through two channels: an operator HTTP surface gated by the pairing code, and a native macOS dialog. First response wins, atomically. The HTTP surface is deliberately not behind the connector bearer token — a client must not be able to approve its own request, and that separation is asserted end to end with a real minted token rather than a fabricated string.
brain_capture. The gateway's first write tool, annotatedread_only_hint = false, behind the policy gate. Becauseonebrain-fs's note-writing layer performs no path confinement (#407), the tool guards its own path: it canonicalizes the parent, asserts containment, and checks that the confined path is exactly the one the write will open.Truthful capabilities.
capabilitiesreports which approval channels are actually live. A caller is never told a write can be approved when no channel can deliver the prompt — that failure mode is a silent hang that looks like a bug.Security posture
Loopback-only. No tunnel, no
public_url. #404 (pre-tunnel security checklist) still blocks that phase.The path guard was traced by hand against thirteen adversarial title inputs — separators,
.., dots-only, NUL, newline, CJK, fullwidth, over-length, empty. None escape the inbox; none create or truncate anything outside the vault. Deny and timeout provably write nothing.Two carried limitations, both stated rather than papered over:
onebrain-fs— tracked in onebrain-fs: confine note writes to the vault, and stop atomic_write following symlinks on its temp file #407 along with anatomic_writetemp-file symlink issue found during review.brain_capture; explicitly not proportionate for a futureDestructivetool.Review
Six tasks, each reviewed on landing, then three whole-branch rounds through separate lenses — specification, adversarial security, and fresh-eyes integration — with every finding put to three independent skeptics instructed to refute it. Eight findings survived, six were refuted, and all eight are fixed.
Three of the eight were the same defect found independently from three angles: the native dialog had no
giving up afterclause, so a timed-out approval left itsosascriptprocess and blocked thread alive indefinitely. The caps bounded registry entries, not dialogs — while the code comment and the docs both claimed otherwise. Verifying the fix against realosascriptturned up two boundaries worth knowing:giving up after 0means never expire, and a value abovei32::MAXproduces no dialog at all, silently offlining the channel. Both are now clamped.The other five: the operator pairing gate accepted unlimited guesses at the same secret
/authorizerate-limits (now one shared budget, one counter);args_summarywas unbounded into a log with no rotation;gateway runinstalled no tracing subscriber, so every operator diagnostic this PR relies on went nowhere — including the full errors thatsanitized_internalstrips from client responses on the promise they reach the operator; a non-ASCII title collapsed to a fixed fallback slug, so only one capture per day could succeed in a language like Thai; and one test would still have passed with the condition it was named for deleted.Widening the slug charset to
char::is_alphanumericmeant re-establishing the containment argument rather than inheriting it, which surfaced a genuine hole:char::to_lowercasecan expand into a non-alphanumeric (U+0130→i+U+0307), so the lowercased output is filtered again. That is what makes "every emitted character is alphanumeric" an invariant instead of an approximation.Deferred, deliberately
Verification
cargo fmt --all --check·cargo +1.98.0 clippy --workspace --all-targets -- -D warnings, and again with--no-default-features· full workspace suite green (gateway module 300/300) ·scripts/coverage.sh --ci-gateat 94.81% lines against the 94 ratchet.Clippy is pinned to 1.98.0 deliberately — CI's stable is 1.98.0, and a local 1.97 clean run already turned an earlier PR in this series red.