peach-code-review.md

32.2 KB · raw

rendered · source

1# Peach Browser Extension — Independent Code Review2**Reviewer:** Claude Fable 53**Date:** 2026-06-124**Codebase:** peach-browser (v4.0.0)5**Method:** Complete static read of all security-relevant source files67## Overview89Peach is a local-first, zero-knowledge browser password manager (Manifest V3,10Chrome + Firefox, plus a native Android port that is out of scope here). The11vault is encrypted on-device with AES-256-GCM under an Argon2id-derived key; no12plaintext ever leaves the machine. Beyond storage it implements browser13autofill, WebAuthn/passkey provisioning, a Pro-gated LAN device-sync feature14(audio "chirp" pairing → Noise-encrypted WebSocket), a paper backup system15("Codex": Reed-Solomon-coded Aztec matrices rendered to PDF), and a duress/decoy16two-slot vault.1718The dominant impression from reading the code is that it has been through19serious, repeated security review and that the fixes were integrated20thoughtfully rather than bolted on. Nearly every security-relevant decision21carries an inline rationale tied to a specific finding ID (audit `HV-*`/`MV-*`/22`LV-*`, technical-report `PCH-01-*`). Cryptographic primitives are standard and23correctly parameterised. The extension's trust boundaries — content script vs.24background, page origin vs. vault — are enforced with browser-attested signals25rather than payload-supplied ones. Error handling consistently fails closed for26secrets and fails gracefully for availability.2728The findings below are, with one exception, Informational or Low. I found no29Critical or High issues. This is consistent with a codebase that has already30absorbed two prior review rounds; what remains is mostly defense-in-depth31hardening, documented residual risks, and a small number of asymmetries worth32tightening.3334## Scope3536Read completely:3738- **Cryptographic layer:** `src/crypto/argon2.ts`, `src/crypto/zstd.ts`,39  `src/crypto/totp.ts`, `src/crypto/peachScript.ts`, `src/crypto/peachPayload.ts`,40  `src/background/aes.ts` (the canonical AES module — there is no41  `src/crypto/aes.ts`; see note in Scope deviations), `src/background/self-test.ts`,42  `src/shared/noise-xx.ts`, `src/shared/noise-js.ts` (the Noise NK43  implementation — there is no `src/shared/noise-nk.ts`), `src/shared/sync-identity.ts`,44  `src/shared/recovery-words.ts`.45- **Vault core:** `src/background/vault.ts` (entire file).46- **Extension boundary:** `src/background/messages.ts`, `src/background/entry.ts`,47  `src/background/clipboard.ts`, `src/content/index.ts`, `src/content/detect.ts`,48  `src/content/save.ts`, and the relevant interfaces of `src/content/inject.ts`.49- **Sync + pairing:** `src/background/chirp-sync-coordinator.ts`,50  `src/background/chirp-sync-vault-adapter.ts`, `src/shared/chirp-sync-transport.ts`,51  `src/background/chirp-sync-runner.ts`, `src/background/paired-devices.ts`,52  `src/shared/chirp-payload.ts`, `src/shared/chirp-handshake.ts`,53  `src/shared/device-manifest.ts`, `src/shared/sync-limits.ts`.54- **Backup + recovery:** `src/popup/codex/generate.ts`, `restore.ts`,55  `pdf-import.ts`, `erasure.ts`, `src/shared/import.ts`, `src/shared/backup.ts`,56  `src/shared/import-limits.ts` (the `erasure.ts` referenced in the brief as57  `src/shared/erasure.ts` is at `src/popup/codex/erasure.ts`).58- **Configuration:** `manifest.json`, `manifest.firefox.json`, `package.json`.5960**Scope deviations:** three brief paths do not exist as written. AES-GCM lives in61`src/background/aes.ts` and inline in `peachPayload.ts` (a header comment in62`aes.ts:4` records that a duplicate `src/crypto/aes.ts` was removed); the Noise NK63pattern is in `noise-js.ts`; the Codex erasure module is in `src/popup/codex/`.64All three were located and read.6566## Cryptographic Implementation6768**Argon2id (master-password KDF)** — `argon2.ts`, also inline in `peachPayload.ts`69and as parameters in `codex-types`. m = 65536 KiB (64 MiB), t = 3, p = 4, 32-byte70output, via `hash-wasm`. This meets the OWASP password-manager tier and exceeds71it on parallelism. The password is NFC-normalised before derivation72(`argon2.ts:32`) so the same passphrase typed under different Unicode73normalisation forms (macOS NFD dead-keys vs. mobile NFC) derives the same key and74stays cross-platform-compatible; a `deriveKeyRaw` legacy path75(`argon2.ts:40-45`) exists solely to open vaults sealed before that fix. The salt76is 16 bytes from `crypto.getRandomValues`, fresh at creation and on every password77change (`argon2.ts:50-52`). Appropriate and correct.7879**AES-256-GCM (vault-at-rest and export envelope)** — `aes.ts`,80`peachPayload.ts:997-1051`. 96-bit random nonce per encryption (`aes.ts:56`),81128-bit tag (the maximum, explicitly pinned and commented as non-reducible at82`aes.ts:51-55`). Keys are imported non-extractable, used once, then zeroed by83callers (`vault.ts` zeroes `derivedKey`/`rootKey` on lock and after key swaps).84The AAD carries a per-slot domain tag (`"slot_a_v1"`, `"slot_b_v1"`, `"pw_root_v1"`)85with a legacy single-byte-AAD fallback on decrypt so pre-hardening vaults still86open (`aes.ts:104-129`). The construction is textbook and the nonce-reuse hazard87is correctly avoided with fresh randomness rather than a counter. Note that GCM's88~2³² safe-message bound per key is irrelevant here — a vault is re-encrypted as a89single message per write.9091**BLAKE3 (payload checksum, vault id, card fingerprints)** — via `hash-wasm`. In92`peachPayload.ts` a 4-byte truncated BLAKE3 checksum sits *inside* the encrypted93envelope (`blake3Truncated`, `1053-1061`) purely to fail fast on94compressed-payload corruption before decompression — authenticity is GCM's job,95not the checksum's, and the code says so (`peachPayload.ts:27-29`). The checksum96comparison is constant-time (`1063-1068`). `computeVaultId` is97`BLAKE3(masterKey || "peach-vault-id")` (`sync-identity.ts:68-75`).9899**zstd / pako (compression)** — `zstd.ts`, `generate.ts`. Compression is not a100security primitive but is a decompression-bomb surface; it is bounded101(see PCH-02-008 context and `import-limits.ts`).102103**X25519 + ChaCha20-Poly1305 + BLAKE2s (Noise XX / NK)** — `noise-xx.ts`,104`noise-js.ts`. Both patterns (`Noise_XX_25519_ChaChaPoly_BLAKE2s` for mutual-auth105WebSocket sync, `Noise_NK_…` for post-chirp pairing) derive the protocol-name106init hash and the HKDF chain over BLAKE2s, matching the named pattern (a prior107round had used SHA-256 here; `PCH-01-002` corrected it). Because `@stablelib`'s108HKDF reuses one HMAC across output blocks and BLAKE2s's `restoreState` leaves the109`finished` flag set, the code runs HKDF-Expand manually with a fresh HMAC-BLAKE2s110per block (`noise-xx.ts:46-58`), documented as byte-identical to the Android111BouncyCastle side. Transport keys are split correctly and direction-swapped on112the responder; nonces are per-frame counters from 0. The XX initiator pins the113responder's static key *before* transmitting its own static in message 3114(`noise-xx.ts:78-133`, `PCH-01-003`), so a probed-but-wrong LAN host never learns115the prober's long-term identity.116117**Ed25519 (device-manifest signing)** — `sync-identity.ts:89-161`,118`device-manifest.ts`. Deterministically derived from the root key under a119distinct HKDF salt (`"peach-sync-signing-v1"`) so the signing key is120single-purpose and survives reinstall. Manifests are signed over a canonical121JSON encoding (sorted keys, no insignificant whitespace, non-finite numbers122rejected — `device-manifest.ts:183-204`) and verified with strict per-device123field validation and a freshness window. The private key is imported as124extractable, which the inline comment justifies and `PCH-01-022` documents: the12532-byte seed is already in memory, so a JWK export to recover the public key adds126no attack surface.127128**HKDF-SHA256 (sync identity, recovery KEK, per-entry sync keys)** —129`sync-identity.ts`, `recovery-words.ts:130-155`, `chirp-sync-vault-adapter.ts:355-369`.130Each use is domain-separated. The per-entry sync envelope key is131`HKDF(rootKey, info = entryId + "\x00" + "sync-entry-v1")`; the null-byte132separator makes the (id, suffix) boundary unambiguous (`PCH-01-005`), and the133adapter rejects any decrypted envelope whose `entry.id` does not equal the map134key it arrived under, so a ciphertext minted for one slot cannot be replayed into135another (`chirp-sync-vault-adapter.ts:153-155`).136137**PBKDF2-SHA256 (legacy export, self-test)** — `backup.ts:146-173` derives the138key for the *legacy* encrypted-JSON import format with PBKDF2-SHA256 at 250,000139iterations. `self-test.ts` uses PBKDF2 at 1,000 iterations deliberately, only as140a fast Known-Answer-Test vehicle on the same AES-GCM path (it pins the expected141ciphertext, `PCH-01-020`, so a self-consistent-but-broken WebCrypto fails). See142PCH-02-005.143144**BIP39 recovery words** — `recovery-words.ts`. A 12-word phrase encodes a145128-bit prefix of the 256-bit root key plus a 4-bit SHA-256 checksum, exactly per146BIP39. The code is explicit and repeated about the security model147(`recovery-words.ts:20-26`, `116-120`, `vault.ts:282-296`): recovery is 128-bit,148the words wrap the *full* root key via an HKDF-derived KEK from a *prefix* of that149key, and a future 256-bit-recovery scheme would need independent entropy. This is150a correct and unusually well-documented construction.151152**TOTP** — `totp.ts` delegates to the `otpauth` library; the entry's stored153algorithm/digits/period are threaded through (`PCH-01-019`) with RFC defaults154(SHA-1/6/30) when absent. The raw base32 seed never crosses to a content script155(see Extension boundary); the background generates the current code and ships only156that plus seconds-remaining.157158**PeachScript** — `peachScript.ts` is a trivial byte → Unicode-PUA codec159(U+E000…U+E0FF) for the device-to-device export string. It is presentation, not160cryptography — the payload it carries is already an AES-GCM envelope — and the161file says so. Decode validates every code point is in range.162163## Code Review Findings164165### Finding PCH-02-001: Update-entry patch path bypasses the canary/tags/passkey sanitizers used on add and restore166**Severity:** Low167**Location:** `src/background/vault.ts:2317-2417` (`sanitizeEntryPatch`),168`src/background/messages.ts:478-501` (`UPDATE_ENTRY` handler)169**Description:** Entries entering the vault via add (`sanitizeEntryDraft`,170`vault.ts:2256-2315`), restore, or sync (`sanitizeRestoredEntry`,171`vault.ts:1603-1650`) are run through field whitelists that rebuild `passkey`172and `canary` sub-objects field-by-field and bound `tags`173(`sanitizePasskey`/`sanitizeCanary`/`sanitizeTags`). The `UPDATE_ENTRY` path does174not: `sanitizeEntryPatch` copies `patch.canary` (`2375-2377`), `patch.tags`175(`2386-2388`), and `patch.passkey` (`2379-2384`) through verbatim. `UPDATE_ENTRY`176is also not in `DESTRUCTIVE_MESSAGE_TYPES` (`messages.ts:141-146`), so unlike177`DESTROY_VAULT`/`RESTORE_*` it is not gated to extension-page senders — and it is178legitimately sent from content-script context (the inline editor and the179canary-ping bump at `content/index.ts:336-348` and `content/save.ts:57-61`), so180it cannot simply be gated. The net effect is that a caller able to send an181`UPDATE_ENTRY` for a known entry id could attach an arbitrary `passkey`182sub-object, set unbounded `tags`, or replace a `canary` object on an entry that183already has one without the field rebuild the other paths enforce.184**Notes:** Exploitability is genuinely low: web pages cannot call185`chrome.runtime.sendMessage` (no `externally_connectable`), so this requires186compromising the content script itself, and Peach's injected UI mounts in *closed*187shadow roots (`content/index.ts:164`, `inject.ts` `attachShadow({mode:"closed"})`188throughout), which removes the obvious in-page foothold. Adding a *new* canary is189separately Pro-gated (`messages.ts:491-498`) and the canary fire path re-validates190the webhook with the full SSRF guard (`isAllowedCanaryWebhookUrl`,191`messages.ts:1568-1600`). The cleanest fix is to route `sanitizeEntryPatch`'s192`canary`/`tags`/`passkey` branches through the same sanitizers the other paths193use; the change is local and does not alter the trust boundary.194195### Finding PCH-02-002: Noise XX message 1 omits the canonical empty-payload hash step196**Severity:** Informational197**Location:** `src/shared/noise-xx.ts:93-97` (initiator), `184-191` (responder)198**Description:** In a canonical Noise handshake, every message — including the199first `-> e` — ends by processing its payload through `EncryptAndHash`, which for200an empty payload with no symmetric key yet still performs `MixHash(empty)`. The201Peach XX implementation processes the ephemeral (`mixHash(h, e.publicKey)`) and202sends it, but does not perform the empty-payload `MixHash` for message 1. The NK203implementation *does* hash its message payloads (`noise-js.ts:90-91`, `112-116`),204because by those points a key exists and a real AEAD tag is produced. The205consequence is that Peach's XX transcript hash diverges by one `MixHash(empty)`206from what a stock Noise library (e.g. `snow`) would compute, so a third-party207Noise stack would not interoperate with Peach's XX handshake.208**Notes:** This is not a confidentiality or authenticity weakness: both Peach209peers (and the Android hand-port) compute `h` identically, the handshake hash210still binds the full transcript that both sides observe, and the transport keys211derive from the chaining key, not `h`. The only practical effect is interop with212external implementations, which Peach does not rely on (the doc comment at213`noise-js.ts:11-14` notes interop is held "by porting discipline, not by a shared214library"). The header comment in `noise-js.ts:12` that "a stock `snow` stack215would interoperate" holds for NK but is not strictly true for XX; worth a216one-line correction so the claim is not over-read.217218### Finding PCH-02-003: Pairing authentication rests on the user-confirmed SAS and an unverifiable peer-side gate219**Severity:** Informational220**Location:** `src/shared/chirp-handshake.ts:125-216`221**Description:** The post-chirp pairing handshake uses Noise NK, which222authenticates only the *responder* (it proves the initiator encrypted to the223responder's chirp-broadcast ephemeral). It does not cryptographically224authenticate the initiator, and there is no proof-of-possession over the225`long_term_public_key_hex` a peer advertises in its `PAIR_IDENTITY` frame. The226compensating control is a 6-digit Short Authentication String derived from the227shared Noise handshake hash (`derivePairingSAS`, `163-174`), which the extension228pairing flow requires the user to confirm matches the peer's before any device is229persisted (`gateOnSAS`, referenced as living in `src/popup/chirp-pair.ts`).230**Notes:** This is called out thoroughly and honestly in the source itself231(`chirp-handshake.ts:125-154`) — one of the better in-code threat statements in232the codebase. Residual risk is the standard SAS bound: a MITM/relay survives with233probability ~1e-6 per attempt *and only if the user does not actually compare the234codes*. The open item the code names — that the Android/native peer must enforce235the identical SAS gate before persisting — cannot be verified from this236repository and is the more important dependency: if the app auto-confirms, the237channel is only as strong as its weakest end. Recommend confirming the app-side238gate against the Android source, and (as the comment suggests) considering a239future proof-of-possession round via the already-present240`performInitiatorAuth`/`performResponderAuth` so authentication becomes structural241rather than dependent on user diligence.242243### Finding PCH-02-004: Crypto-adjacent dependencies are caret-ranged while core crypto deps are exact-pinned244**Severity:** Informational245**Location:** `package.json:31-52`246**Description:** The core vault/crypto/transport dependencies are exact-pinned247(no caret): `@stablelib/*` 2.0.1, `hash-wasm` 4.12.0, `@bokuweb/zstd-wasm` 0.0.27,248`@subspace/reed-solomon-erasure.wasm` 0.2.5, `tldts` 7.0.28, `pdfjs-dist`2495.7.284, `@zxcvbn-ts/*` (this is the `LV-9` exact-pin discipline). Several250dependencies on or adjacent to a security path remain caret-ranged: `otpauth`251`^9.5.0` (TOTP generation), `pako` `^2.1.0` (Codex decompression on the import252side), `@zxing/library` `^0.23.0` and `bwip-js` `^4.10.1` (Aztec decode/encode),253`pdf-lib` `^1.17.1`, `papaparse` `^5.5.3`.254**Notes:** With a committed lockfile (`bun.lock`, `packageManager` pinned to255`[email protected]`, frozen-lockfile CI per `LV-6`) the practical exposure is limited to256fresh resolves, but extending the exact-pin discipline to `otpauth` and `pako` in257particular would make the supply-chain posture uniform with the rest of the258crypto surface.259260### Finding PCH-02-005: Legacy encrypted-JSON import uses PBKDF2-SHA256 rather than Argon2id261**Severity:** Informational262**Location:** `src/shared/backup.ts:146-173`263**Description:** The legacy `.json` export/import envelope derives its key with264PBKDF2-SHA256 at 250,000 iterations, whereas every current path uses Argon2id265(m=64 MiB, t=3, p=4). PBKDF2 with no memory-hardness is materially weaker against266GPU/ASIC attackers than Argon2id at equivalent wall-clock cost.267**Notes:** Scope is bounded and the decision is reasonable: this format's UI268export was removed in v2.5.0 (`backup.ts:5-12`), the helper is import-only269(plus a test fixture generator), and current exports use PeachScript / the QR270matrix encoder backed by Argon2id. No action is strictly required; flagged so the271KDF asymmetry is visible to anyone who still relies on old `.json` backups.272273### Finding PCH-02-006: vault_id is a stable, LAN-broadcast, cross-device-linkable identifier274**Severity:** Informational275**Location:** `src/shared/sync-identity.ts:62-75`276**Description:** `computeVaultId` returns `BLAKE3(masterKey || "peach-vault-id")`,277a deterministic 64-hex value that, per its own comment, is carried in the mDNS TXT278record and the first sync handshake (`SYNC_HELLO`). It is therefore a stable279identifier visible to any device on the same LAN, and it is identical across all280of a user's paired devices, making them linkable to a passive local observer for281as long as the master key (root key) is unchanged.282**Notes:** This is inherent to the discovery design — peers must recognise "same283vault" before exchanging entries — and it is one-way (the master key is not284recoverable from it). It is a minor privacy property worth stating, not a285confidentiality flaw. "Rotate sync identity" (`vault.ts:2771-2814`) changes it by286re-keying the root key.287288### Finding PCH-02-007: HIBP breach check omits the Add-Padding response-padding header289**Severity:** Informational290**Location:** `src/background/vault.ts:216-238`291**Description:** `checkBreach` queries the Have I Been Pwned k-anonymity range292API with the SHA-1 prefix but does not send `Add-Padding: true`. HIBP supports293that header to pad the response to a fixed bucket count; without it, the number294of suffixes returned for a given prefix is a (weak) side channel that a295network-position observer could use to narrow which prefix was queried.296**Notes:** Impact is small — only the 5-hex-char prefix is ever sent, never the297password or full hash, and breach monitoring is an opt-out convenience feature.298Adding the padding header is a one-line hardening that costs nothing.299300### Finding PCH-02-008: peachPayload BinaryReader performs no explicit bounds checking301**Severity:** Informational302**Location:** `src/crypto/peachPayload.ts:940-969` (`BinaryReader`),303consumed in `decodeEntry` `638-813`304**Description:** The binary entry decoder reads `u8`/`u16`/`u32`/length-prefixed305strings without validating that the cursor stays within the buffer. Past-end306reads yield `undefined`/`NaN` (for `u8`) or throw `RangeError` (DataView), and307length-prefixed reads clamp via `subarray`. The decoded bytes are normally inside308an AES-GCM-authenticated envelope, but the same decoder runs on imported `.peach`309payloads where an attacker who also knows the passphrase controls the plaintext.310**Notes:** This is a robustness observation, not a memory-safety issue — the311runtime is JavaScript, so the worst case is a thrown exception (caught by the312import UI) or a malformed entry that is later rejected by `sanitizeRestoredEntry`313at the vault boundary. The declared section-length and cumulative-size caps314(`peachPayload.ts:301-311`, `MV-4`) already bound total work before this point.315Explicit cursor bounds checks would make the decoder's failure modes uniform and316self-evidently safe.317318### Finding PCH-02-009: X25519 shared-secret computations do not reject low-order/zero outputs319**Severity:** Informational320**Location:** `src/shared/noise-xx.ts:108,119,140,196,204,231`,321`src/shared/noise-js.ts:84,106,175,248,263,362`322**Description:** All `sharedKey(secret, peerPublic)` calls use `@stablelib/x25519`323with the default `rejectZeroSharedKey = false`, so a peer public key chosen as a324low-order point (yielding an all-zero shared secret) is not rejected at the DH325step.326**Notes:** The practical impact is neutralised by the surrounding protocol: NK327pre-mixes the known responder static key, the XX responder static is pinned328against the paired-device registry before and after the handshake329(`chirp-sync-transport.ts:130-154`), and pairing is additionally gated by the330human SAS comparison. An attacker forcing a zero shared secret would still fail331the static-key pin and/or the SAS. Passing `rejectZero = true` would make the DH332contributory-by-construction and remove the need to reason about this at all.333334### Finding PCH-02-010: Clipboard auto-clear is unconditional and does not compare against the parked hash335**Severity:** Informational336**Location:** `src/background/clipboard.ts:98-117,124-178`337**Description:** On copy, a SHA-256 of the copied value is parked in338`storage.session` purely as a "a clear is owed" flag (`98-103`); the auto-clear339routine (`clearClipboardNow`) then writes an empty string to the clipboard340unconditionally when the timer/alarm fires, without reading the current clipboard341to confirm it still holds the Peach-copied value. If the user copies something342else within the clear window, that later value is wiped too.343**Notes:** This is a deliberate trade-off in favour of the344secret-must-not-persist invariant (the alarm path is eviction-proof, `HV-2`, and345clearing an already-changed clipboard is harmless to confidentiality), and a346content/popup-side write with a real user gesture is preferred when available347(`alreadyWritten`). It is a minor UX surprise rather than a security issue; noted348for completeness because the parked hash is computed but never used for a match349check, which could read as dead bookkeeping to a future maintainer.350351## Positive Observations352353The codebase's strengths are specific and worth recording at the same resolution354as the findings.355356- **Two-slot duress with a real plausible-deniability story.** Both vault slots357  always exist; the decoy/noise slot is filled with CSPRNG bytes that are358  indistinguishable at rest from a GCM ciphertext (`vault.ts:704-716`), and when359  duress is configured both slots are padded to a fixed 256 KiB ceiling so their360  lengths cannot reveal that a decoy exists (`vault.ts:62-72,1038,2213`). The361  duress bit lives only inside slot A's encrypted payload — no external flag362  (`vault.ts:44-45,1051-1053`).363364- **Constant-time unlock.** `attemptUnlockWithDerive` loads both slots, derives365  both keys, and attempts both decrypts in parallel and unconditionally366  (`vault.ts:1124-1154`), so wall-clock time does not distinguish a real-vault367  unlock from a decoy unlock (`DUR-002`).368369- **Origin enforcement uses browser-attested signals only.**370  `attestedSenderOrigin` reads `sender.origin`/`sender.url` (which a page cannot371  forge, since pages reach the background only via the content-script relay) and372  never payload-supplied origins (`messages.ts:217-231`). `GET_ENTRY_FOR_FILL`373  re-validates the entry's domain set against the requesting tab's attested374  origin and refuses cross-domain fills unless the user explicitly summoned them375  (`messages.ts:698-727`, `MV-1`); `GET_ENTRY_FOR_EDIT` mirrors it (`PCH-01-006`).376377- **Secrets are minimised at the boundary.** The fill response strips the passkey378  credential, the raw TOTP base32 seed, and the full password history; the379  background generates the *current* TOTP code and sends only that plus380  seconds-remaining (`messages.ts:728-760`). The TOTP toast deliberately does not381  render the live code into page-readable DOM (`content/index.ts:374-379`).382383- **Destructive operations are gated to extension-page senders.**384  `DESTROY_VAULT`/`CLEAR_VAULT_ENTRIES`/`RESTORE_VAULT_SNAPSHOT`/`RESTORE_FROM_CODEX`385  require an extension-origin `sender.url`, so a compromised content script cannot386  escalate to vault destruction (`messages.ts:267-278`, `PCH-01-018`).387388- **Transactional disk writes with rollback.** `addEntry`/`updateEntry`/389  `deleteEntry` clone-and-restore in-memory state if the persist throws390  (`vault.ts:1783-1790,1883-1888,1927-1933`), and `persistSlotA` restores the391  prior slot A if a later write fails under quota pressure392  (`vault.ts:2193-2198`). `changePassword` writes the new ciphertext, salt, and393  re-wrapped root key in a single storage transaction and swaps the in-memory key394  synchronously before any further await (`vault.ts:2849-2879`).395396- **Monotonic timestamps and clamped peer input.** Entry timestamps are forced397  strictly increasing against wall-clock regressions (`vault.ts:585-594`), and398  every peer-supplied `modifiedAt`/`deletedAt` is clamped to `now + 5 min` before399  any comparison (`sync-limits.ts:13-25`, applied in400  `vault.ts:2035-2040,2078` and at the adapter boundary), so a far-future remote401  value cannot permanently pin against honest local edits (`HV-1`). Remote402  password replacements and deletes of live entries raise durable, user-visible403  alerts (`vault.ts:2052-2097`).404405- **Sync resource bounds are layered.** Oversized frames are refused at the406  WebSocket `onmessage` before buffering (`chirp-sync-transport.ts:70-79`), again407  before `JSON.parse` in the coordinator (`chirp-sync-coordinator.ts:283-285`),408  and the adapter caps entry/tombstone counts and per-entry plaintext size before409  and after decryption (`chirp-sync-vault-adapter.ts:133-152,167`, `MV-3`). The410  subnet probe is rate-limited to once per 60 s per peer pubkey411  (`chirp-sync-transport.ts:208-251`).412413- **Vouched devices require explicit confirmation.** A signed extension manifest414  can introduce a never-seen device, but it is queued in a pending-vouched store415  for user Allow/Reject rather than becoming a sync target on the signature alone416  (`chirp-sync-vault-adapter.ts:270-287`, `MV-7`); a key change under a stable417  device id raises a durable alert (`MV-8d`).418419- **Codex generation verifies itself end-to-end.** Generation gates each stage420  (compression round-trip `generate.ts:341-350`, per-stripe erasure simulation421  `359-410`, full reconstruction with the maximum tolerated chunk loss422  `412-468`) and aborts with a typed `CodexError` on any mismatch — appropriate423  for a disaster-recovery artifact where silent corruption is the worst outcome.424425- **Codex restore treats every scanned field as attacker-controlled.** The426  stripe count `S`, within-stripe total `t`, chunk index `i`, and declared427  fingerprint length are all validated/bounded before they can drive allocation428  or pass a check (`restore.ts:128-173,354`,429  `PCH-01-010/011/012/013`), and decompression is streamed with a hard output430  ceiling (`generate.ts:320-339`, `MV-4`).431432- **PDF import is hardened in depth.** `pdfjs.getDocument` is called with433  `isEvalSupported:false`, `disableFontFace`, `disableRange`, `disableStream`434  (`pdf-import.ts:303-312`, `MV-9`, defending the CVE-2024-4367 font/eval class),435  with file-size, page-count, and rendered-dimension caps436  (`pdf-import.ts:119-126,238,273,327`, `PCH-01-014`).437438- **A real Known-Answer self-test runs on every unlock.** `self-test.ts` pins the439  expected AES-GCM ciphertext for a fixed vector and compares constant-time440  (`83-90`), so a WebCrypto that is broken-but-self-consistent (would pass a bare441  `decrypt(encrypt(x))===x`) is caught and the vault re-locks442  (`messages.ts:304-311`).443444- **Import data is sanitised at the vault boundary, not just at the parser.**445  Every restore/sync path funnels through `sanitizeRestoredEntry` →446  `normalizeEntry` field whitelist with field-by-field `passkey`/`canary` rebuilds447  and bounded tags (`vault.ts:1603-1741`, `MV-6`), so parser leniency in448  `import.ts`/`backup.ts` cannot smuggle unexpected fields into the vault. (The449  one path that skips this is `UPDATE_ENTRY` — see PCH-02-001.)450451- **Honest, specific in-code threat documentation.** The pairing trust-model452  comment (`chirp-handshake.ts:125-154`), the per-entry-key replay rationale453  (`chirp-sync-vault-adapter.ts:9-21`), the recovery-words 128-bit-prefix model,454  and the `removePairedDeviceById` "removal is insufficient, rotate instead" note455  (`paired-devices.ts:72-95`, `MV-8`) all describe residual risk accurately rather456  than overselling the controls.457458- **Minimal, justified manifest.** Permissions are459  `storage`, `unlimitedStorage`, `activeTab`, `scripting`, `clipboardWrite`,460  `alarms`, `idle`, `favicon`, `webAuthenticationProxy`, `offscreen`; the broad461  `tabs` permission and the ggwave web-accessible-resource were both removed462  (`MV-10`), `audioCapture` is `optional_permissions`, and the CSP is463  `script-src 'self' 'wasm-unsafe-eval'; object-src 'none'` with no `unsafe-inline`464  or `unsafe-eval` (`manifest.json:55-75`). Content-script UI is mounted in closed465  shadow roots throughout, page-synthesised keydowns are ignored via `e.isTrusted`466  (`PCH-01-007`), and no production content code uses `innerHTML`.467468## Summary Table469470| ID | Title | Severity | Location |471|---|---|---|---|472| PCH-02-001 | Update-entry patch path bypasses canary/tags/passkey sanitizers | Low | `vault.ts:2317-2417`, `messages.ts:478-501` |473| PCH-02-002 | Noise XX message 1 omits canonical empty-payload hash step | Informational | `noise-xx.ts:93-97,184-191` |474| PCH-02-003 | Pairing auth rests on user SAS + unverifiable peer-side gate | Informational | `chirp-handshake.ts:125-216` |475| PCH-02-004 | Crypto-adjacent deps caret-ranged vs. exact-pinned core | Informational | `package.json:31-52` |476| PCH-02-005 | Legacy JSON import uses PBKDF2 rather than Argon2id | Informational | `backup.ts:146-173` |477| PCH-02-006 | vault_id is stable, LAN-broadcast, cross-device-linkable | Informational | `sync-identity.ts:62-75` |478| PCH-02-007 | HIBP check omits Add-Padding header | Informational | `vault.ts:216-238` |479| PCH-02-008 | peachPayload BinaryReader lacks explicit bounds checks | Informational | `peachPayload.ts:940-969` |480| PCH-02-009 | X25519 DH does not reject low-order/zero outputs | Informational | `noise-xx.ts`, `noise-js.ts` (DH call sites) |481| PCH-02-010 | Clipboard auto-clear unconditional; parked hash unused for match | Informational | `clipboard.ts:98-178` |482483## Conclusions484485Peach is a carefully engineered, security-conscious password manager whose486cryptographic core is standard and correctly parameterised, and whose extension487trust boundaries are enforced with browser-attested origins, closed shadow-DOM UI,488sender-gated destructive operations, and consistent secret minimisation across the489content-script boundary. The vault layer handles the hard cases — atomic490key-rotation writes, in-memory rollback on failed persists, monotonic timestamps,491constant-time duress unlock — with evident care, and the higher-risk feature492surfaces (LAN sync, Codex PDF import) are bounded against malformed and malicious493input at multiple layers.494495The review found no Critical or High issues. The single Low finding is a496sanitizer asymmetry on the update path that is meaningfully mitigated by the497extension's messaging model and is a local fix. The remaining items are498Informational: documented residual risks (the pairing SAS dependency on an499unverifiable peer-side gate is the most important to track, because its weakest500link lives in a repository this review cannot see), small defense-in-depth501hardening opportunities (reject zero DH outputs, add HIBP padding, bound the502binary reader explicitly), and supply-chain/KDF consistency notes.503504A technically literate reader should come away with high confidence in the505correctness of the encryption-at-rest and autofill-boundary implementations, and506with a short, concrete list of hardening steps rather than any structural concern.507The pervasive, accurate in-code documentation of *why* each control exists is508itself a quality signal: it makes the security model auditable from the source509alone, which is exactly what a transparency-focused publication of this review510should want to demonstrate. The two cross-repository dependencies — the Android511Noise hand-port and the Android SAS gate — are the parts of the trust model that512this review could not close from the browser source, and they are the right place513to direct any follow-up verification.