Fix Pocket pairing storage on Safari and retain device diagnostics - #629
Conversation
Deploying mouseterm with
|
| Latest commit: |
487119a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9130174d.mouseterm.pages.dev |
| Branch Preview URL: | https://pocket-storage-preflight.mouseterm.pages.dev |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress — not a merge verdict. Three things to weigh, plus one scope note; mark the PR ready when you want the full review.
Build & Test is red on this commit, caused by the diff: website/scripts/generate-docs.test.js → "takes a removed section subheadings and all" asserts data.selfhost.headings.every((h) => h.depth === 2). Until now every ### in SELF_HOST.md lived under "Installer contract (maintainers)", which the generator withholds; the two new ### under "Troubleshooting boundaries" are the first depth-3 headings the published runbook keeps. Both fixes are defensible and it's your call which the runbook wants — flatten the two new headings to ##, or narrow the assertion to the ids the test's own comment names ("The Installer contract's four ### subsections must go with their ##"), which the loop above line 114 already checks.
A single undecodable known-burrows record now takes the whole store down, with no way out of it. restore throws out of both get and list, so one bad record rejects list() entirely — loadBurrows never reaches setPhase({ at: 'burrows' }), so healthy Burrows disappear from the list along with the broken one and the Forget button that would remove it. forgetBurrow reads through get first, so naming the record directly doesn't help either, and PocketClient.pair calls get(burrowId) for the identity-mismatch check after the passkey prompt and the Burrow's approval — so re-pairing, the natural recovery, fails at the last step. That leaves clearing site data as the only exit, which also destroys the passkey and every other pairing. This is a new failure mode: before the diff neither accessor could throw on a stored record. The security intent behind the rejection is right — pocket-encrypted-storage.test.ts pins that a corrupt envelope never mints a replacement identity — and it survives a per-record version: let list() keep the readable records and mark the unreadable one as un-connectable so it can be forgotten, while get() on the connect path still refuses. Worth noting the trigger is the failure this PR already models: rejects silent loss of the encrypted wrapping key during the preflight is exactly a record whose wrappingKey didn't survive the clone, and the probe only samples that behaviour on a disposable database at pairing time.
The both-formats-failed message prints its four-sentence preamble twice. Both probe throws already carry POCKET_KEY_STORAGE_ERROR, so the composed string reads "…Report the diagnostic below. Diagnostic: write-record / DataError. Encrypted storage: This browser could not save and reload the private key needed for pairing. Pairing has not started. …" — on the one screen the feature exists to deliver. Nothing catches it because pocket-key-storage.test.ts imports probePocketKeyStorage as requirePocketKeyStorage: every message assertion in that file pins the single-probe function, and the only test touching the real composition checks the substring 'Encrypted storage:'. The alias is worth undoing on its own — the file name and the local name both claim to cover a function it never calls.
Scope note on the lint exception. The new FAIL IF reads "AES-GCM appears in non-diagnostic production source outside the local at-rest wrapper", but e2e-lint's scope is remote-lib-common/src/, lib/src/, and relay/src/ — lib/pocket/public/diagnostics/, where restart.js and capabilities.js use AES-GCM, is outside every tree the lint walks. So the word "non-diagnostic" describes a carve-out nothing enforces, and a later lib/pocket/public/ file carrying a second AEAD would pass silently. The excludeFiles mechanism and its self-test are sound; it's the spec line's claimed scope that's wider than the check.
…e-spelled The preflight wrote the commit-or-abort dance twice, the derive-and-compare three times, and byte equality twice by hand where `constantTimeEqual` already exists — in the one file where "did this key survive" must not drift. One `commitOrAbort` and one `deriveShared` carry all of it, and the comparison comes from the security package. `POCKET_KEY_STORAGE_ERROR` is prepended by `requirePocketKeyStorage` rather than by each probe, so the sentence the user reads appears once in a combined failure instead of twice mid-paragraph. The probe answers a diagnostic; the wrapper owns the sentence. `KnownBurrowStore.generateKey` is required rather than optional, which deletes the ternary at the one place a Client static is minted: the fallback branch was the unverified path this fix exists to close, and a store could reach it just by not implementing the method. The diagnostics harness exports `assert`, `equal`, `message` and a shared `commit`, so `restart.js` imports them from the sibling it already imports four names from rather than keeping its own copies. Also: one `TextEncoder` instead of one per encrypt and per decrypt; the three independent WebCrypto calls in the encrypted keygen run together rather than in a chain; and the AES-GCM lint exempts the at-rest wrapper through the `allow` hook it already has, rather than subtracting a file from every rule's file list — an exclusion the `exactly` rules would have silently miscounted, and which hid the file from the missing-file check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress — not a merge verdict; mark the PR ready when you want the full one. One new point on this commit.
The double preamble is gone and pocket-encrypted-storage.test.ts now pins it as exactly-once, which is the part the old alias could never have caught.
Parallelizing the PKCS#8 export moves it outside the wipe. Before this commit exportKey('pkcs8', …) was the last await before the try, so it ran only once the wrapping key existed and every plaintext buffer it produced reached clear.fill(0). Inside Promise.all it now runs concurrently with the AES-GCM generateKey — and the encrypted path exists precisely for browsers whose WebCrypto is patchy, so that generate rejecting is the case this feature is built around. Promise.all rejects on it while the export still settles, and the plaintext private-key buffer is dropped unreferenced with nothing to zero it. The finally calls its wipe best-effort against browser-internal copies, which is a different claim from a buffer the function itself allocated and then abandoned. What the parallelism buys is three CPU-local WebCrypto calls overlapping — generateKey and exportKey are not the network round trips "a phone pays for each round trip" reads as. Suggestions inline keep the two harmless calls parallel and leave the one that yields plaintext where the wipe still covers it.
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress — not a merge verdict; mark the PR ready when you want the full one. Two new findings on this commit, plus one note on what CI has not actually run.
subscribeToPush is the third metadata-only reader and it still takes the full record. The rule this commit adds to docs/specs/pocket-app.md -> "What Pocket stores" scopes summaries to "Burrow listing, push-subscription queries, and removal", and listPushSubscribedBurrows and forgetBurrow both moved. subscribeToPush in lib/src/remote/client/pocket-client.ts did not — it still opens with const record = await this.#knownBurrows.get(burrowId); and then reads nothing but record.authorization.state and record.authorization.deliveryId, which are exactly the fields a summary carries. Two consequences. On the normal path every push-enable pays an AES-GCM decrypt plus a PKCS#8 import whose result is discarded. On the damaged-envelope device this PR models — the one that can now list and forget — enabling push throws the decrypt error out of get instead of reaching the this phone is not paired with that computer branch written right below it, which is the one place left where a corrupt key still takes down an operation that does not need it. getSummary supplies both fields, and it is the same substitution the open thread on pair asks for.
Build was skipped on this commit, so none of the new Pocket build wiring has run in CI. In run 34625592158 the Test step fails and Build is skipped, and build:pocket — vite build --config vite.pocket.config.ts then node scripts/assert-pocket-worker.mjs — only runs inside Build. That leaves the riskiest part of this commit unexercised: the second rolldownOptions.input entry, and the resolution split the harness move creates inside lib/pocket/diagnostics/index.html, where src='/diagnostics/page.js' and src='/diagnostics/restart-page.js' must now resolve through the bundler while the sibling /diagnostics/style.css and /diagnostics/manifest.webmanifest must keep resolving verbatim out of public/. The new assertPocketShell(join(outDir, 'diagnostics')) gate is the check for exactly that, and it has not run either. Worth clearing the generate-docs failure first for that reason rather than as tidiness — until it is green nothing has built the diagnostics entry.
|
Addressed the actionable review findings in bebbb58: fixed the failing runbook assertion without flattening its structure; switched subscribeToPush and the re-pair pin check to metadata-only summaries; narrowed all worker store signatures; and made the AES lint scope explicit. Regression coverage proves unreadable keys do not block push registration, and a damaged persisted envelope can recover through fresh approved re-pairing while preserving the Burrow pin and retiring the old delivery capability. Earlier private-byte cleanup and duplicate-preamble findings remain covered. Local validation: 301 Pocket/client/worker tests, 42 generated-doc tests, typecheck, spec lint, e2e lint plus 27 mutation checks, and the production Pocket/diagnostics/worker build pass. CI is rerunning; keeping this PR draft. Hardware evidence is also updated: the v3 production-codec/AAD restart checkpoint passed on the Home Screen iPhone at 17:17:44 UTC, from preparation at 17:14:19 UTC; the report establishes a new page instance, while OS restart is operator evidence. |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Feedback on work in progress — not a merge verdict; mark the PR ready when you want the full one. Two new findings on this commit, plus two inline notes.
connect is the accessor split's remaining hole, and it is the one a user reaches first. subscribeToPush, forgetBurrow, the listing, and the re-pair pin check all moved to summaries, so a damaged envelope now survives every operation that does not need the key — except the Connect button. PocketClient.connect in lib/src/remote/client/pocket-client.ts still opens with a bare const record = await this.#knownBurrows.get(burrowId); and no guard around it, so loadPocketPrivateKey's crypto.subtle.decrypt rejects with a browser-authored OperationError that propagates out of connectTo to run's setError(err instanceof Error ? err.message : String(err)) in App.tsx. Two consequences. The screen paints a raw browser exception string, which is the property docs/specs/pocket-app.md states for the probe (never display browser exception messages or key material) and the one surface where it is now reachable outside it. And the throw skips decision.pairingRequired entirely — the flag whose own comment in connectTo says re-reading is what puts *Pair again* on the row — so the damaged device is denied the recovery re-pairs a damaged envelope only after approval, preserving the Burrow pin and retiring the old delivery id just proved works. This state is new in the PR: before it restore was a passthrough whose only failure was the project-authored Invalid Pocket private key. Returning { ok: false, message: CONNECTION_DENIAL_MESSAGES['pairing-required'], pairingRequired: true } when the read rejects routes it into that path. It sits outside the diff, so I've left it here rather than as a suggestion — happy to push it if you want it in this PR.
KnownBurrowStore.list() has no production caller left. After this commit the only get callers are connect and sw.ts's openNotification, and everything else is on getSummary/listSummaries; list() appears only in pocket-db.test.ts and pocket-encrypted-storage.test.ts. It is also the accessor that still carries the failure mode the open thread named — Promise.all(values.map(restore)) rejects the whole listing when any single record fails to decode — so keeping it leaves that hazard one call away from whatever reads the interface next, while docs/specs/pocket-app.md now states summaries as the rule for listing. Dropping it from KnownBurrowStore and from memoryKnownBurrows makes the rule structural instead of a convention. All four remaining assertions want only burrowId, label, authorization, or a length, so listSummaries() covers them.
|
Second review round addressed. Connect now catches stored-record read/decode failures and returns fixed retry/Scan a setup code instructions, without browser-authored exception text. I intentionally did not set pairingRequired for an arbitrary local read failure: that flag refreshes persisted authorization, and a transient IndexedDB error is not authenticated revocation. The existing Scan a setup code action already recovers through fresh approval while preserving the Burrow pin; the extended encrypted-storage regression proves that path, verifies the old identity is untouched before approval, and injects private browser error text to pin redaction. No record is automatically reset. Removed the unused full-record list API and switched its remaining tests to summaries. Both inline notes addressed. All 301 targeted tests, production typecheck, spec lint, and e2e lint/self-tests pass locally. CI and review are rerunning. |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
One finding on this commit, inline. The rest of the increment checks out: list() has no consumer left anywhere in the repo, the e2e-lint pin now carries the tree list, and notificationForPush already wraps the worker's get in its own catch, so connect was the last unguarded caller.
Holding the verdict on the copy-rot point below rather than on CI — Build & Test is still running on this commit.
2 participants
Summary
Fix Pocket pairing on browsers that can use X25519 keys but cannot persist their native CryptoKey representation. Originally stacked on #613, now merged; this PR targets main.
Security tradeoff
Encrypted fallback is not equivalent to directly persisted nonextractable X25519: compromised same-origin JavaScript can decrypt and extract the private bytes. The wrapping key remains nonextractable; plaintext private bytes are never persisted, and owned plaintext buffers are cleared on success/failure (best effort, not a browser-internal memory guarantee).
Per-Burrow approval, Noise, and fresh paired-passkey presence remain unchanged. The operator explicitly accepted the fallback tradeoff. Native storage remains preferred.
Verification
Manual follow-up on other devices
Run diagnostics in the intended browser/Home Screen context, pair using an existing passkey where available, and reconnect after closing/reopening Pocket. For restart diagnostics, prepare once, restart, verify the retained checkpoint, then remove the disposable test data.