fix: encrypt upload recovery token at rest - #148
Conversation
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review (cycle 1)
🔴 Blocking: draft-advisory disclosure on a public repo. This PR fixes a security issue whose GitHub Security Advisory is still in DRAFT (the PR body itself says to keep the PR draft until the advisory is published). A draft advisory is embargoed, so the draft advisory id and the vulnerability mechanism must not appear in any world-readable artifact yet. Right now they leak in six places:
src/background/token-crypto.ts— header comment names the advisory id and spells out the mechanism (see inline)src/background/state.ts— the block comment, the@deprecatedfield doc, and therecoverTokencomment each name the advisory id (see inline)tests/in-flight-uploads.test.ts— two test names embed the advisory id (see inline)- the PR title
- the PR body — names the advisory id and includes the full exploit write-up
- the commit message body — same
Please fix all of these in one sweep, keeping the code fix byte-identical:
- Genericize every code comment and test name to neutral wording (drop the advisory id and the mechanism; keep a short generic "why").
- Genericize the PR title and PR body.
- Reword the commit message body (
git commit --amend, then verify the tree is unchanged withgit diffand force-push). - Keep the branch name (it carries no advisory id) and keep the PR a draft.
- Restore the full detail once the advisory is published. A tree-wide grep for the advisory-id prefix + the mechanism keywords should come back clean.
🟢 The fix logic itself is correct and well-covered — persist/load round-trip, undecryptable-record drop, legacy-plaintext upgrade path, and a no-cleartext-on-disk assertion. Title format (fix:) and the Closes #147 keyword are fine.
🟡 Non-blocking: the vitest suite runs in Node where indexedDB is undefined, so every test exercises only the in-memory memoryKey fallback — the real non-extractable-key-in-IndexedDB backend has no CI coverage (it was verified manually with fake-indexeddb). Consider a jsdom + fake-indexeddb test so the production key path stays covered in CI. Not a blocker.
Sending back to the PR Dobby to genericize the artifacts.
Short-lived upload-session credentials should not be persisted to disk in
the clear. persistInFlightUploads() previously wrote the recovery token
verbatim to browser.storage.local; wrap it so it never touches disk in
cleartext.
- New token-crypto.ts holds a per-install, NON-EXTRACTABLE AES-GCM key in
IndexedDB (a different backend than the credential store; non-extractable
so JS can never export the raw key bytes).
- persistInFlightUploads() now stores { uuid, token: {iv, data}, startedAt }
with the recoveryToken encrypted; the in-memory map keeps plaintext for
resumeUpload. uuid/startedAt stay clear so stale records prune without a
decrypt (uuid alone is useless without the token).
- loadInFlightUploads() decrypts on read; undecryptable/malformed records
are dropped instead of crashing the startup probe. Legacy plaintext
records from earlier builds are read once, then re-persisted encrypted.
A proportionate mitigation for a local-access, <=24h credential. Full key
protection would need an OS keystore Thunderbird does not expose to
extensions.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cf18a2f to
f1949be
Compare
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review (cycle 2)
✅ The cycle-1 blocker is resolved. The embargoed advisory identifier and the specific vulnerability mechanism no longer appear in any public artifact — code comments, test names, PR title, PR body, and the commit-message body now all use neutral hardening language. A tree-wide grep for the advisory-id prefix and the mechanism keywords comes back clean, and the branch name carries no identifier.
✅ Fix logic is correct and well-covered. AES-GCM with a fresh 12-byte IV per encryption; a per-install non-extractable key kept in IndexedDB (a separate backend from the credential store); decrypt-on-load with undecryptable/malformed records dropped rather than crashing the startup probe; and a legacy-plaintext upgrade path that reads old records once and re-persists them encrypted. Persist/load round-trip, no-cleartext-on-disk, undecryptable-drop, and legacy-compat are all tested — 212 passing, typecheck + build green. Title (fix:) and the Closes #147 keyword are correct.
🟡 Non-blocking (informational, carried from cycle 1): the vitest suite runs under Node where indexedDB is undefined, so the tests only exercise the in-memory-key fallback in token-crypto.ts — the production non-extractable-CryptoKey-in-IndexedDB path has no automated coverage (it was verified manually with fake-indexeddb). A jsdom + fake-indexeddb test would lock that path into CI. Not a blocker.
No blocking findings — signing off on the change.
Hardens persistence of the short-lived Cryptify upload-session credential, tracked in #147.
Problem
persistInFlightUploads()insrc/background/state.tswrote the upload-session recovery token verbatim intobrowser.storage.local. A short-lived session credential should not be persisted to disk in the clear. Scope is limited to in-flight sessions (≤24h, pruned byMAX_IN_FLIGHT_AGE_MS); no email content or key material is involved.Fix
Encrypt the credential before it is written to disk. (Re-deriving it on restart isn't feasible: the token is a server-issued session secret, and
resumeUploadstill needs it.)src/background/token-crypto.ts— AES-GCMencryptString/decryptStringbacked by a per-installation key that is:generateKey(..., false, ...)) so JavaScript can never export the raw key bytes, andstate.tspersistInFlightUploads()now writes{ uuid, token: { iv, data }, startedAt }with therecoveryTokenencrypted. The in-memoryinFlightUploadsmap keeps the plaintext token (needed to callresumeUpload).uuid/startedAtstay in the clear so stale records prune without a decrypt — anduuidalone is useless without the token.loadInFlightUploads()decrypts on read; undecryptable/malformed records are dropped (never crash the startup probe). Legacy records from earlier builds are read once so an active session survives the upgrade, then re-persisted encrypted.Scope
This is a proportionate mitigation for a local-access, short-lived (≤24h) credential: the credential is no longer persisted in cleartext, and recovering it now requires defeating the browser's non-extractable-key storage. It is not a claim of protection against an attacker who can already read and fully reverse-engineer the entire profile — Thunderbird exposes no OS keystore to extensions, so a hardware-backed secret is out of reach for a client-side add-on.
Testing
npm test— 212 passing (tests/in-flight-uploads.test.tsextended: no-cleartext-on-disk assertion, encrypt→decrypt round-trip, undecryptable-record drop, legacy-record compatibility).npm run typecheck— clean.npm run build— succeeds.Closes #147.