feat: source the read-accelerator bearer from the session and pin two guards - #1243
Merged
Merged
Conversation
… guards The read accelerator is CipherBox's own token-authed gateway, but nothing supplied its bearer: `EngineHandle::new` took an `acceleratorBearer` no host could fill, because a `VITE_`-prefixed variable is inlined into the public bundle and a bearer is a per-session credential. The browser engine therefore reached the accelerator unauthenticated and every read fell through to the public fallbacks. The credential is now the session access token the API client already holds in linear memory. `SessionBearer` is a shared, zeroizing, `Debug`-redacted cell: the client writes it on login and on every refresh rotation and clears it at logout and at engine shutdown, and the accelerator leg reads it per request, so a rotation lands without the gateway being rebuilt. `GatewayConfig` carries base URLs only, so a host cannot configure a gateway credential at all, and a public fallback has none by construction. A host-supplied `acceleratorBearer` is refused rather than silently dropped. Alongside it, two regression guards that were raised as review nitpicks and never landed: - The prune dead-letter test now asserts the unpruned history survives the failed expansion. `publish_prune` computes `prune_debt` before the truncate and the publish precisely so a debt it cannot compute leaves the history standing; moving it back left every existing assertion passing while the record silently lost a version. - The KAT suite pins ephemeral-scalar freshness within each vector file. HPKE ephemeral reuse under one recipient key and `info` is a confidentiality break, and several accept families seal every vector to the same recipient under the same `info` — only a regenerated corpus could introduce a repeat, which is exactly when it would slip in unnoticed. The walk covers every embedded fixture rather than a registered list, so a family added later is guarded without being enrolled.
WalkthroughThe PR adds shared session bearer storage for gateway credentials, connects the bearer to the engine and API client session lifecycle, updates gateway configuration APIs, adds WASM rejection checks, strengthens gateway tests, and validates HPKE ephemeral-scalar uniqueness and family coverage. ChangesSession-bound gateway credentials
HPKE KAT manifest validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ionale Review findings from the self-review passes, folded back in. The accelerator base URL is host configuration and the token now bound to it authorizes the whole API, so a stale or mistyped value turned a "reads are slow" misconfiguration into session-credential disclosure. The bearer is now bound only to a leg that can keep it — TLS, or a loopback host so the local Kubo in `apps/web/.env.example` still works. A denied leg still serves reads, unauthenticated. Teardown is a one-way latch. `shut_down` sealed the cell rather than clearing it, because a refresh parked on the network resumes after the engine is gone and would otherwise write a fresh token into a cell nothing will clear again. The cold-start failure arm clears it too: login has already stored a token by then, and that arm's whole job is leaving no key material resident. The freshness test's stated rationale was false. It claimed distinct vector files seal under distinct recipients; `content_key_accept` and `settings_record_accept` in fact share a recipient key *and* both ephemeral scalars, and are separated only by their HPKE `info`. The comment now states what is actually true and checkable, and the anti-vacuity anchor is a named `(file, count)` family list rather than a bare total, so a family that stops emitting `ephemeralScalar` names itself instead of being masked by another family gaining a vector. Scalars fold to lowercase before the uniqueness check so a hex-case change cannot hide a byte-level repeat. Also: drop `PartialEq`/`Eq` from the gateway types, which existed only so fixtures could compare and made a variable-time comparison of a credential reachable; narrow the `SessionBearer` mutation surface to the crate and its fixture constructor to tests; collapse the API client's one-field `State`; add `GatewaySource::public` for the six no-auth construction sites; and prove the wasm refusal really does precede the `apiBaseUrl` check instead of only claiming it.
FSM1
marked this pull request as ready for review
August 10, 2026 18:22
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/core/tests/kat_manifest.rs (1)
1478-1482: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the rationale comments.
Keep the invariant and scope. Remove the extended explanation of excluded corpus-wide behavior and current fixture examples. The test name and assertions already show the implementation detail.
As per coding guidelines, “Comments should explain short, non-obvious rationale” and must not describe absent paths.
Also applies to: 1514-1524
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/core/tests/kat_manifest.rs` around lines 1478 - 1482, Shorten the rationale comments around the HPKE ephemeral vector-file anchors and the related assertions near the freshness check. Preserve the invariant and scope, but remove discussion of corpus-wide exclusions, absent emission paths, and fixture examples; retain only a concise explanation of the non-obvious rationale.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/tests/kat_manifest.rs`:
- Around line 1499-1511: The ephemeral_scalars helper must return decoded binary
scalars rather than lowercase hex strings. Update its return type and parsing
logic to decode each vector’s ephemeralScalar as strict hex, require exactly 32
bytes, and fail with the vector name and path for missing, non-string,
malformed, or incorrectly sized values instead of silently filtering them out;
preserve the existing JSON parse context.
In `@crates/engine/tests/write_plane.rs`:
- Around line 7969-7973: Update the test around plant_record to capture both
expected CoreVersion values before they are consumed, then compare
published_versions(&world.record_store, &blocks, file) directly with that
expected vector. Replace the length-only assertion while preserving the existing
verification that the unexpanded history entries, including their CIDs and
metadata, remain unchanged.
---
Nitpick comments:
In `@crates/core/tests/kat_manifest.rs`:
- Around line 1478-1482: Shorten the rationale comments around the HPKE
ephemeral vector-file anchors and the related assertions near the freshness
check. Preserve the invariant and scope, but remove discussion of corpus-wide
exclusions, absent emission paths, and fixture examples; retain only a concise
explanation of the non-obvious rationale.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c756a180-a536-45f1-88cf-56afe0f9173a
📒 Files selected for processing (14)
crates/core/tests/kat_manifest.rscrates/engine/src/api/client.rscrates/engine/src/content/mod.rscrates/engine/src/content/read.rscrates/engine/src/facade.rscrates/engine/src/lib.rscrates/engine/src/net/adopter.rscrates/engine/src/net/rotation.rscrates/engine/src/settings.rscrates/engine/src/testkit/content.rscrates/engine/tests/vault_settings.rscrates/engine/tests/write_plane.rscrates/fuse/tests/fuse_op_core.rscrates/wasm/src/host.rs
FSM1
marked this pull request as draft
August 10, 2026 18:37
ephemeral_scalars kept each scalar as hex text, so the freshness check compared spellings and a malformed or wrong-length field passed through unread. It now decodes through unhex32, which already enforces the lowercase-hex contract and the 32-byte length and names the vector that broke it. A vector carrying no ephemeralScalar is still skipped: that absence is how a file with no HPKE ephemerals is recognised, and the per-file counts pinned above catch a family that goes dark. The prune test asserted its surviving history by length alone, so a pass that replaced an entry or changed its CID still satisfied a claim about leaving the history standing. It compares the versions themselves now. Comment rationale trimmed to what is not visible from the code: the fixture inventories named which files share a scalar today, which the corpus regeneration this test exists to catch would falsify.
Owner
Author
Review disposition — CodeRabbit on
|
| Finding | Disposition |
|---|---|
ephemeral_scalars keeps hex text instead of decoding |
Taken, with one departure. Decoded through this file's existing unhex32 (lowercase-hex contract + 32-byte length + names the vector), so the freshness check compares bytes and the .to_ascii_lowercase() workaround is gone. Not taken: failing on a missing field. That helper scans every fixture in the corpus, and continues on files that yield none — absence is how a file with no HPKE ephemerals is recognised, so panicking there would fire on every non-HPKE vector. Field absent → skip; field present → must be valid 32-byte lowercase hex. |
| prune test asserts the surviving history by length only | Taken. len() == 2 satisfied any pass that swapped an entry or changed a CID, while claiming the history was left standing. The planted versions are captured and compared entry for entry. |
The "silently omits a vector" premise was checked and is already covered: HPKE_EPHEMERAL_FAMILIES pins per-file counts and the test ends in assert_eq!(pinned, expected, "hpke ephemeral family coverage drift"), so a vector losing its scalar fails there naming the file. Verified against the corpus — all ten files carry a 64-hex-char scalar on every vector.
Nitpick (1) — partly taken
| Nitpick | Disposition |
|---|---|
| Shorten the rationale comments (1478-1482, 1514-1524) | Split. Removed the fixture inventories — "owner_local_accept alone spans four info values", "content_key_accept and settings_record_accept share both their recipient and both their scalars today". Those are claims about corpus state at a moment, and the corpus regeneration this test exists to catch is exactly what falsifies them. Kept the scope rationale for why uniqueness is per-file and not corpus-wide: that is not visible from the code, and without it the obvious "improvement" is to tighten the check corpus-wide, which fails immediately. Net 11 lines → 8, and 5 → 4 on the constant. |
Verification
Both fixes were verified as discrimination checks, not re-runs:
- swapping a scalar for invalid hex now fails with
vector empty-info-empty-aad: bad hex: Invalid character 'z' at position 0; the oldStringpath accepted it - a same-length history whose second entry carries a different root CID now fails with a
left == rightdiff;len() == 2passed it
Gates on f126425
cargo fmt --all --check 0 · cargo clippy --workspace --all-targets -- -D warnings 0 · cargo test 1841 passed · cargo check -p cipherbox-wasm --target wasm32-unknown-unknown 0 · pnpm lint:tracker-refs 0 · kat_gen leaves crates/core/kat/ byte-identical.
Generated by Claude Code
FSM1
marked this pull request as ready for review
August 10, 2026 23:00
This was referenced Aug 18, 2026
Closed
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.
Closes #1237
Closes #1238
Closes #1033
Three Rust changes over disjoint files: one runtime fix and two regression guards that were raised as review nitpicks and never landed.
The read-accelerator bearer now comes from the session
The read accelerator is CipherBox's own token-authed Kubo trustless gateway, but nothing supplied its bearer.
EngineHandle::newtook anacceleratorBearerno host could fill: aVITE_-prefixed variable is inlined into the public bundle and a bearer is a per-session credential, so the browser config surface deliberately omitted it. The browser engine therefore reached the accelerator unauthenticated and every read fell through to the public gateway fallbacks — correct fail-open behaviour, but the member acceleration was dead.The credential is now the session access token the API client already holds in linear memory and never surfaces.
SessionBearer(crates/engine/src/content/read.rs) is a shared, zeroizing,Debug-redacted cell.ApiClientwrites it on login and on every refresh rotation, and clears it at logout, on a failed refresh, and on an unusable token;Engine::shut_downclears it too, because a parked tick's gateway clone outlives the engine.GatewayConfignow carries base URLs only. A host cannot configure a gateway credential at all, and a public fallback has none by construction rather than by convention.apps/web/.env.examplestill works. The accelerator URL is host configuration and the token authorizes the whole API, so a stale or mistyped value must cost the member their acceleration, not their session. A denied leg still serves reads, unauthenticated.shut_downseals the cell rather than clearing it, because a refresh parked on the network resumes after the engine is gone and would otherwise re-arm it. The cold-start failure arm clears it too — login has already stored a token by then, and that arm exists to leave no key material resident.acceleratorBeareris refused with anErr, not silently dropped: a bearer reaching that constructor came from a build-time variable in the public bundle, and the host should hear that.Covered by
the_accelerator_presents_whatever_the_session_cell_currently_holds(no session → bare; set → carried; rotated → new value; cleared → bare),into_gateway_binds_the_session_bearer_to_the_accelerator_alone,an_accelerator_that_cannot_keep_a_credential_is_never_handed_one,a_tls_or_loopback_accelerator_is_handed_the_session_bearer,a_sealed_bearer_refuses_a_late_token,login_binds_the_session_token_to_the_accelerator_and_shutdown_drops_it, and the wasm-sidea_host_supplied_accelerator_bearer_is_refused_first. The existing "public fallback carries no bearer token" assertion is unchanged.The credential is the full API session JWT, not a read-scoped token.
blueprint/api.mdsays the gateway is "gated by a CipherBox auth token" without saying which; this makes it the session token, which also authorizes uploads, quota and mailbox writes. That is the only credential the client has without a new API endpoint, and both it andapiBaseUrlcome from the same deployment's configuration — but it widens the blast radius of any leak at the gateway tier, and the accelerator now sees which CIDs a member reads and when. Narrowing it to a read-scoped, audience-bound token is #1244.The prune dead-letter test guards the surviving history
publish_prunecomputesprune_debtbeforeversions.truncate(...)and beforepublish_node, precisely so a debt the pass cannot compute leaves the history it was read from standing.a_prune_whose_root_no_source_serves_spends_its_budget_and_dead_lettersasserted the dead-letter reason, an unchanged retire-target count, and a zero pending reclaim — every one of which a regression movingprune_debtafter the publish would leave passing while the record silently lost a version. It now asserts the history survives.The KAT suite pins ephemeral-scalar freshness
HPKE ephemeral reuse under one recipient key and one
infois a confidentiality break (crates/core/src/seal/owner_local.rs), and several accept families seal every vector to the same recipient under the sameinfo— two of the four owner-local accept vectors carrykind: received-shares. About 35 loops pinned vector names for uniqueness and none pinned the ephemeral.Rather than adding a set to each of the ten HPKE-sealing loops, one test reads every embedded fixture for
ephemeralScalarand asserts uniqueness within each file, scalars folded to lowercase so a hex-case change cannot hide a byte-level repeat.Per-file is the right scope because a vector file is a superset of each
(recipient, info)group inside it —owner_local_acceptalone spans fourinfovalues and pins two vectors under one — so it forbids every real repeat plus some harmless ones. It is deliberately not corpus-wide:content_key_acceptandsettings_record_acceptshare a recipient key and both ephemeral scalars today, separated only by their HPKEinfo, so a global assertion would fail onmain.HPKE_EPHEMERAL_FAMILIESis the anti-vacuity anchor as a named(file, count)list, matching this file'sCODEC_DECODE_REACHABLE_CHECKS/ALL_STRUCT_TAGSconvention — a bare total would let one family going dark be masked by another gaining a vector.The vectors themselves are untouched —
cargo run -p cipherbox-core --example kat_genleavescrates/core/kat/clean.Gates
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo test --workspacecargo check -p cipherbox-wasm --target wasm32-unknown-unknownpnpm lint:tracker-refskat_gen+git status --porcelain crates/core/kat/True negatives
Each guard was verified to fail when the thing it guards is broken.
prune_debtmoved afterpublish_nodeincrates/engine/src/sync/drain.rsa_prune_whose_root_no_source_serves_spends_its_budget_and_dead_letters— the new assertion reportsleft: 1, right: 2once the harness's attempt-budget assert is stood down; with the harness intact the same reorder fails the test at the budget instead, so CI is red either wayempty-bodyduplicated ontoreceived-shares-bodyinowner_local_accept.json, in uppercase hex — bothkind: received-shares, so one recipient key and oneinfoephemeral_scalars_are_fresh_within_each_vector_file— "vector received-shares-body repeats another vector's ephemeral scalar", so the case fold holdsephemeralScalarrenamed inmailbox_accept.json, i.e. one family silently leaving the walkAll breaks were reverted;
crates/core/kat/andcrates/engine/src/sync/drain.rsare byte-identical tomain.Review passes
/simplify,/security-reviewand/crypto-privacy-reviewwere run over the diff and their findings folded back in a second commit — the transport gate on the bearer binding (both security passes ranked it highest), the seal-on-teardown latch, clearing on cold-start failure, droppingPartialEq/Eqfrom the gateway types (they made a variable-time comparison of a credential reachable and existed only for fixtures), narrowing theSessionBearermutation surface, collapsing the API client's one-fieldState, andGatewaySource::publicfor the six no-auth sites.The crypto pass also caught a factual error in my own comment: it claimed distinct vector files seal under distinct recipients, which the corpus contradicts. That is corrected above and in the code.
Not done
EngineHandle::newkeeps itsacceleratorBearerparameter position rather than dropping it.packages/client/src/worker/engineWasm.tsandengineHost.tspass it positionally asundefined, andEngineWasmis a hand-written interface, so removing the parameter would shiftpublicGatewaysinto the headroom slot without a TypeScript error — a silent runtime regression. Those files are outside this change's ownership, so the four have to move together: #1245.The accelerator gate is transport-only. Requiring the accelerator host to match the API host was considered and rejected: staging runs them on sibling subdomains, and a same-registrable-domain test needs a public-suffix list. Narrowing the credential itself (#1244) is the better lever.
Note
Source the read-accelerator bearer from the session and pin HPKE ephemeral guards
SessionBearerin read.rs, a shared, zeroizing, sealableRc<RefCell<BearerCell>>that holds the API access token and is readable by the read-accelerator leg at request time.ApiClientto accept aSessionBearerviawith_session_bearer; token rotations and clears now propagate to all consumers sharing the cell.GatewayConfig::into_gatewaynow accepts aSessionBearerand attaches it to the accelerator source only when the URL is TLS or loopback (carries_credentials_safely); public fallbacks never receive a credential.Engineholds the shared bearer, passes a clone to the gateway at construction, sets it on the API client at login, clears it on cold-start failure, and seals it during shutdown to prevent late re-arming.EngineHandle::new(WASM host) now rejects a caller-suppliedacceleratorBearerwith an explicit error, as credentials are session-scoped.GatewayConfigno longer accepts credentials at construction; callers that previously embedded a bearer directly must migrate tointo_gateway(SessionBearer); WASM callers passingacceleratorBearernow receive a hard error.Macroscope summarized f126425.
Summary by CodeRabbit
Security
Configuration
Bug Fixes