PCH-02-001-vault-patch-sanitizers.patch.md

1# PCH-02-001 — UPDATE_ENTRY patch path routes canary/tags/passkey through existing sanitizers23**Severity:** Low (the only non-Informational finding in PCH-02)4**Target file:** `src/background/vault.ts` — `sanitizeEntryPatch`, lines 2375–23885**Report:** peach-code-review.md, Finding PCH-02-00167## Problem89`sanitizeEntryPatch` copies `patch.canary` (2375–2377), `patch.passkey`10(2379–2384), and `patch.tags` (2386–2388) through verbatim, while every other11vault-entry funnel rebuilds those shapes field-by-field:1213- add / batch-add → `sanitizeEntryDraft` (vault.ts:2256–2315) runs14  `sanitizeCanary` + `sanitizeTags` (PCH-01-016) and throws on invalid passkey15- restore / import / sync → `sanitizeRestoredEntry` (vault.ts:1603–1650) runs16  `sanitizePasskey` + `sanitizeCanary` + `sanitizeTags` (MV-6)1718`UPDATE_ENTRY` is legitimately sent from content-script context (inline editor,19canary-ping bump at `content/index.ts:336–348`), so it cannot be added to20`DESTRUCTIVE_MESSAGE_TYPES` — the fix is to make the patch path use the same21sanitizers, not to gate the sender.2223## Fix — exact edit2425In `sanitizeEntryPatch` (vault.ts), replace the three verbatim branches:2627```typescript28// BEFORE (vault.ts:2375-2388)29  if ("canary" in patch) {30    nextPatch.canary = patch.canary;31  }3233  if ("passkey" in patch) {34    // Passkey sub-object replaces all the legacy flat passkey fields. A35    // patch with `passkey: undefined` removes the credential from the entry36    // (used by "remove passkey" without deleting the whole entry).37    nextPatch.passkey = patch.passkey;38  }3940  if ("tags" in patch) {41    nextPatch.tags = patch.tags;42  }43```4445```typescript46// AFTER47  if ("canary" in patch) {48    // PCH-02-001: route the patch canary through the same MV-6 whitelist the49    // add/restore/sync funnels use. An invalid shape (non-https webhook,50    // missing webhookUrl) drops the canary — mirroring sanitizeRestoredEntry —51    // rather than rejecting the whole patch. `canary: undefined` still clears.52    nextPatch.canary = patch.canary ? sanitizeCanary(patch.canary) : undefined;53  }5455  if ("passkey" in patch) {56    // Passkey sub-object replaces all the legacy flat passkey fields. A57    // patch with `passkey: undefined` removes the credential from the entry58    // (used by "remove passkey" without deleting the whole entry).59    // PCH-02-001: a DEFINED passkey must pass the same field-by-field rebuild60    // the restore path applies. Refuse the patch on an invalid shape rather61    // than silently replacing the stored credential with a malformed one —62    // mirrors sanitizeEntryDraft, which throws "invalid passkey credential".63    if (patch.passkey === undefined || patch.passkey === null) {64      nextPatch.passkey = undefined;65    } else {66      const sanitized = sanitizePasskey(patch.passkey);67      if (!sanitized) throw new Error("invalid passkey credential");68      nextPatch.passkey = sanitized;69    }70  }7172  if ("tags" in patch) {73    // PCH-02-001: bound tags exactly like both import funnels (PCH-01-016) —74    // strings only, ≤256 chars each, ≤255 tags. Non-array collapses to75    // undefined, so `tags: []` (and junk shapes) clear the field.76    nextPatch.tags = sanitizeTags(patch.tags);77  }78```7980No import changes needed — `sanitizeCanary`, `sanitizePasskey`, and81`sanitizeTags` are module-local functions in the same file (vault.ts:1657,821705, 1730).8384## Compatibility audit of every existing UPDATE_ENTRY producer8586Verified against source before choosing these semantics:8788| Producer | Patch shape | Survives? |89|---|---|---|90| Content canary-ping bump (`content/index.ts:336–348`) | full existing canary + `lastPinged`/`pingCount` | Yes — `sanitizeCanary` preserves `webhookUrl`/`label`/`lastPinged`/`pingCount`; the stored webhook is already https |91| Popup/inline edit forms | scalar fields, full passkey object on edit | Yes — complete `PasskeyData` passes `sanitizePasskey` |92| "Remove passkey" UX | `passkey: undefined` | Yes — explicit removal branch kept |93| `ADD_ADDITIONAL_URL` handler (messages.ts:510) | `{ additionalUrls }` | Untouched branch |94| `SAVE_NOTE` handler (messages.ts:542) | `{ title, notes, modifiedAt }` | Untouched branches (`modifiedAt` was already ignored by the whitelist) |95| Breach updater (messages.ts:839) | `{ breach }` | Untouched branch |9697Note on URL normalization: `sanitizeCanary` round-trips the webhook through98`new URL(...).toString()`, so `https://x.example.com` persists as99`https://x.example.com/`. This already happens on the add/restore paths; the100patch path simply joins them.101102## messages.ts handler verification (no bypass)103104`UPDATE_ENTRY` (messages.ts:478–501) has exactly one mutation path:105`vault.updateEntry(id, patch)` → `sanitizeEntryPatch` (vault.ts:1836). There is106no second entry point that skips the sanitizer. The Pro gate for *new* canaries107(messages.ts:491–498) and the fire-time SSRF guard108(`isAllowedCanaryWebhookUrl`, messages.ts:1568) remain as the outer gates.109110**Adjacent observation (no change made):** `vault.updateEntryDirect`111(vault.ts:1978) spreads a patch verbatim, but it has **zero production112callers** (grep: only its definition; the sync path uses113`applyMergedRemoteEntries`, which funnels through `sanitizeRestoredEntry`).114Flagged for a future dead-code pass; out of scope for this finding.115116## Tests117118`vault.update-entry-sanitize.test.ts` (in this folder) — 8 regression tests.119Destination: `src/background/vault.update-entry-sanitize.test.ts` (imports are120already in destination form).121122## Save message (integration)123124`fix: PCH-02-001 UPDATE_ENTRY patch routes canary/tags/passkey through existing sanitizers`