fix: name the download outcome and seal the Core Kit store at rest - #1242
Merged
Conversation
A save returned a bare boolean, which could not tell a browser that never fetched from a stream the broker gave up on. `whenStreamIdle` resolves true for any ticket a body ever claimed, so a read that died after its first byte reported success and left a truncated file with no banner; the batch loop then read the same false as a refusal and dropped every file after it. `save` now returns 'saved' | 'refused' | 'failed', and the hook subscribes to `onStreamError` for its own ticket so a broker-abandoned read sets the error. `saveAll` owns the loop: it stops only at a refusal and names the files whose reads failed. The Core Kit store now reaches localStorage as AES-GCM ciphertext under a non-extractable key kept in IndexedDB, minted under a Web Lock so tabs that cold-start together share one. A value this device cannot open — a store written before the seal, or one whose key was evicted — is dropped on read, which costs one re-login rather than a wedge. Logout takes the wrapping key with the store. Also replaces the real-timer deadline in the broker's port-replacement test with fake timers; the ~15 ms of margin it left made it flaky.
WalkthroughThe PR adds encrypted asynchronous Core Kit persistence with IndexedDB wrapping keys. It also changes media and file downloads to use structured outcomes, report failures, and continue or stop batches based on the outcome. ChangesCore Kit sealed storage
Structured download outcomes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CoreKitProvider
participant Web3AuthSession
participant SealedStore
participant IndexedDB
CoreKitProvider->>Web3AuthSession: Create session with sealed store
Web3AuthSession->>SealedStore: Restore or persist session
SealedStore->>IndexedDB: Read or create wrapping key
Web3AuthSession->>SealedStore: Purge corrupt data or cleanup on logout
sequenceDiagram
participant FileBrowserActions
participant FileDownload
participant MediaService
participant MediaBroker
FileBrowserActions->>FileDownload: saveAll(SaveRequest[])
FileDownload->>MediaService: Wait for stream idle
MediaService->>MediaBroker: Request IdleOutcome
MediaBroker-->>FileDownload: Return read status and failure
FileDownload-->>FileBrowserActions: Continue, stop, or report batch result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
…'s context Review findings from the simplify, security and crypto-privacy passes. The download failure now travels on `whenIdle`'s own result rather than through a per-save `onStreamError` subscription. The broker holds the message and the waiters are already indexed by ticket, so recording it there costs six lines and removes three defects the consumer-side patch carried: a latch that a re-opened ticket could never clear, a match on a URL the package documents as having several forms, and a dependence on `fail` notifying synchronously after it resolves the waiter — which nothing asserted, and whose regression direction was to report a truncated file as saved. `saveAll` no longer overwrites the message that stopped the batch: a failure before a refusal used to lose the refusal, which is the actionable half. On the sealed store: - `restore()` treated an unreachable key store as a corrupt one and purged the ciphertext, which is the exact case `getItem` throws rather than drops for. Only a parse failure condemns the store now. - A decrypt failure re-reads the key store once before giving up, so a tab that logged out and back in elsewhere does not force a second re-login here. - The seal binds the storage key and the envelope version as AAD. The version travels outside the sealed bytes, so a v1 ciphertext relabelled `v2` would otherwise open under a future build's semantics. - `indexedDB.open` rejects on `blocked`; it runs inside the wrapping-key lock, and an open that never settles would queue every tab on the origin at login. - The header no longer claims a copied profile is covered. `extractable: false` bars export to script, not presence on disk, and a whole-profile copy carries the IndexedDB key with it. - `createCoreKitSession` takes its store rather than defaulting to one, so the production composition sits at the composition root in `main.tsx`.
FSM1
marked this pull request as ready for review
August 10, 2026 19:35
![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
🤖 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 `@apps/web/src/auth/coreKit.test.ts`:
- Around line 100-105: The test around session.restore must expect the
SyntaxError produced by malformed JSON, or separately seed readable stored data
for the SDK refusal scenario. Split the malformed-storage coverage from the
SDK-refusal coverage so each test asserts the error actually reached in its
respective path, using the existing created, store, and sdk fixtures.
In `@packages/client/src/media/broker.ts`:
- Around line 451-454: Update the cursor cleanup flow around drop and settleIdle
so waiter.failure is recorded only when cursor.pin.cursors reaches zero,
preventing an earlier failed concurrent cursor from contaminating a later
successful completion. Pass the terminal status into drop, move the idle-waiter
failure update into the final-cursor path, and add coverage for one
shared-ticket cursor failing while the last cursor completes normally.
🪄 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: d756bcb5-3502-44b9-9379-4c9218f59376
📒 Files selected for processing (15)
apps/web/src/auth/CoreKitProvider.test.tsxapps/web/src/auth/coreKit.test.tsapps/web/src/auth/coreKit.tsapps/web/src/auth/sealedStore.test.tsapps/web/src/auth/sealedStore.tsapps/web/src/components/file-browser/FileBrowserActions.test.tsxapps/web/src/components/file-browser/FileBrowserActions.tsxapps/web/src/hooks/useFileDownload.test.tsapps/web/src/hooks/useFileDownload.tsapps/web/src/main.tsxapps/web/src/test/storeFakes.tspackages/client/src/media/broker.test.tspackages/client/src/media/broker.tspackages/client/src/media/service.test.tspackages/client/src/media/service.ts
FSM1
marked this pull request as draft
August 10, 2026 19:47
Two bodies can hold one ticket. When one fails and the last ends tidily, the outcome still names the failure — the truncated bytes the first one left are not made whole by the second finishing. whenIdle documents that the boolean cannot tell a short transfer from a whole one and that only the failure can, so discarding it there would reintroduce exactly the confusion the field exists to prevent. Behaviour is unchanged; the test pins a choice that was implicit.
Owner
Author
Review disposition — CodeRabbit on
|
| Finding | Disposition |
|---|---|
coreKit.test.ts expects an unreachable REFUSED; "the test fails" |
Rejected — false positive. There is no readStore(), and no SyntaxError escapes restore(). restore() calls init() first (coreKit.ts:45), which is what throws REFUSED; the malformed JSON is read afterwards by storeIsCorrupt(), whose JSON.parse is inside a try/catch returning a boolean (:120-125). The test passes 8/8 as written, and applying the committable suggestion verbatim breaks it. CI being green across 25 jobs on this head is what made "the test fails" worth checking rather than acting on. |
broker.ts: record the terminal failure only when the final cursor drops |
Mechanism confirmed, fix rejected. The interleaving is real — I wrote the test and it passes against current code. But the proposed change makes a truncated transfer report as clean. whenIdle documents that "only the failure tells a short transfer from a whole one", and useFileDownload:85 maps a null failure to 'saved' — so dropping cursor A's failure because cursor B ended tidily tells a member a file saved when part of it never arrived. Applying the diff verbatim fails the new test. |
Taken from the second finding: the coverage it asked for
keeps the failure of one shared-ticket cursor when the last one ends clean — two cursors on one ticket, the first fails, the last closes cleanly, and the outcome is still { read: true, failure: 'the record is gone' }. Behaviour is unchanged; what changes is that the choice is now a pinned contract rather than an implicit one, so a future change in the direction proposed here fails loudly.
Gates on fa1283e
pnpm lint 0 · pnpm typecheck 0 · pnpm --filter @cipherbox/client test 0 (29 files, 408 tests, including the new case) · apps/web coreKit.test.ts 8/8 unchanged.
Both refutations were verified in both directions — the current code passing, and the proposed change failing — rather than argued from reading.
Generated by Claude Code
FSM1
marked this pull request as ready for review
August 11, 2026 00:09
FSM1
added a commit
that referenced
this pull request
Aug 11, 2026
…le (#1270) * docs: carve browser-held key custody out of the all-crypto-in-Rust rule Rule 4 said without qualification that TypeScript has no crypto of its own, and main now contradicts it: #1242 shipped a non-extractable AES-GCM wrapping key for the Core Kit store, and ADR 0009 needs a device identity key that signs an approval exchange before the vault key exists. Neither case can move to the engine. A WASM implementation materializes key bytes in linear memory, which is the property non-extractability denies, and before start(secret) there is no session to derive from. Bounds the exception rather than widening it: local state only, no KDF edge, no wire format, no KAT, and the key never leaves WebCrypto. Entire-Checkpoint: eedb31e3ced7 * docs: link the ADR 0009 citation to the decision corpus Matches how blueprint/core.md and blueprint/engine.md already cite ADRs. Entire-Checkpoint: 89dd578ce0c3
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.
Two
apps/web-owned fixes, plus thepackages/client/src/mediachange the first one needed.A failed download reported success and cancelled the rest of the batch
savereturned a bareboolean, which cannot tell a refusal from a failure.MediaBroker.acquiresetswaiter.read = truethe moment a body claims a ticket, andfail()settles the waiter throughdrop()— sowhenIdleresolvedtruefor a stream that died after its first byte. The hook returned that as success and the user kept a truncated file with no banner. Separately the batch loop broke on anyfalse, so one unreadable file silently dropped every file after it.The failure is now carried on
whenIdle's own result rather than recovered from theonStreamErrorfan-out. The issue suggested the fan-out, but the broker already has the message in hand and its waiters are already indexed by ticket, so{ read, failure }costs six lines there and avoids three defects a consumer-side subscription carries:failnotifying synchronously after it resolves the waiter. That ordering held, but it was unasserted, and its regression direction was to report a truncated file as saved.On top of that:
savereturns'saved' | 'refused' | 'failed'— a string literal union, per the repo's standing preference over TS enums — and takes oneSaveRequestrather than three positional arguments.saveAllowns the batch: it stops only at arefused, carries afailedforward, names the files that failed, and leaves the message that stopped the batch on the end rather than overwriting it.Tests cover the broker recording the read it gave up on, a waiter armed after a settled one carrying no stale failure, a mid-read failure surfacing as
failed, a mid-batch failure that still attempts the next file, and a refusal that stops the batch.Also replaces the real-timer deadline in
broker.test.ts— a 40 ms budget around two 25 ms waits — with fake timers.Sealing the Core Kit store at rest
corekit_storeheld asessionIdthat both addresses and decrypts the Web3Auth record carrying the login secret, in plaintextlocalStorage. It now reaches storage as an AES-GCM envelope under a 256-bitCryptoKeyminted withextractable: falseand kept in IndexedDB, with the storage key and the envelope version bound as AAD.indexedDB.openrejects onblocked, because that open happens inside the lock.restore()now honours that: only a store that opens and fails to parse is condemned.What this narrows is stated at the module head rather than implied: it defeats a reader of
localStoragealone — a scraping extension, a partial backup, a grep over a disk image.extractable: falsebars export to script, not presence on disk, so a whole-profile copy carries the IndexedDB key with it, and same-origin script can open that database and call the handle directly. Nothing else should be relaxed on the strength of this.No crypto was implemented here: the primitives are the browser's WebCrypto, and non-extractability is a property only a WebCrypto-held key can have — a Rust/WASM implementation would put the key bytes in linear memory and destroy the one control this exists for. That is a carve-out from
AGENTS.mdrule 4 as written, and worth a line in the rule orblueprint/web-client.mdso the next reader does not have to re-derive it.Not done, and why
indexedDB.databases().tests/web-e2e/**is outside this change's ownership and is being edited in parallel. The existing allow-list still passes: this adds nolocalStoragekey.toBase64/fromBase64and the IndexedDB promise wrapper are duplicated frompackages/client/src/seams/bytes.tsandseams/idb.ts. Reusing them needs two names added topackages/client/src/index.ts, which is outside this change's ownership —apps/webcannot deep-import.SerialLocksduplicatesFakeLockManager(packages/client/src/testkit.ts), which is excluded from the build and not exported. Reaching it needs a./testkitexport and dropping the build exclude, which would ship test doubles.indexedDbWrappingKeyshas no unit coverage — jsdom has no IndexedDB. Covering it means addingfake-indexeddb, and a lockfile change risks conflicting with the changes running in parallel.TypeErrorfrom inside the SDK into a clear message on a non-secure origin. Failing closed is already the right policy perblueprint/web-client.md; only the diagnosis is missing.Gates
pnpm lint:tracker-refs,pnpm typecheck,pnpm lint,pnpm test— all exit 0.Closes #1236
Closes #1175
Note
Seal the Core Kit store at rest with AES-GCM and name download outcomes
SealedStorein sealedStore.ts: encrypts localStorage values with AES-GCM under non-extractable wrapping keys stored in IndexedDB, with LockManager serialization, envelope versioning, and drop-on-invalid behavior.Web3AuthSessionto useSealedStoreinstead of rawStorage; on restore failure it only clears the store when the blob is corrupt by JSON parse semantics, and purges both value and wrapping key on logout.sealedCoreKitStore()in main.tsx.SaveOutcome('saved'|'refused'|'failed') andSaveRequestinuseFileDownload, addingsaveAllfor batch downloads with aggregated error reporting and refusal short-circuiting.MediaBroker.whenIdleandMediaService.whenStreamIdleas structuredIdleOutcomeobjects instead of booleans.Macroscope summarized fa1283e.
Summary by CodeRabbit
New Features
Bug Fixes