add PCH-02-001 fix draft: sanitizeEntryPatch routes canary/tags/passkey through sanitizeCanary/sanitizePasskey/sanitizeTags, producer compatibility audit, 8 regression tests

@@ -0,0 +1,124 @@1+# PCH-02-001 — UPDATE_ENTRY patch path routes canary/tags/passkey through existing sanitizers2+3+**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-0016+7+## Problem8+9+`sanitizeEntryPatch` copies `patch.canary` (2375–2377), `patch.passkey`10+(2379–2384), and `patch.tags` (2386–2388) through verbatim, while every other11+vault-entry funnel rebuilds those shapes field-by-field:12+13+- 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)17+18+`UPDATE_ENTRY` is legitimately sent from content-script context (inline editor,19+canary-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 same21+sanitizers, not to gate the sender.22+23+## Fix — exact edit24+25+In `sanitizeEntryPatch` (vault.ts), replace the three verbatim branches:26+27+```typescript28+// BEFORE (vault.ts:2375-2388)29+  if ("canary" in patch) {30+    nextPatch.canary = patch.canary;31+  }32+33+  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+  }39+40+  if ("tags" in patch) {41+    nextPatch.tags = patch.tags;42+  }43+```44+45+```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+  }54+55+  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+  }71+72+  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+```79+80+No import changes needed — `sanitizeCanary`, `sanitizePasskey`, and81+`sanitizeTags` are module-local functions in the same file (vault.ts:1657,82+1705, 1730).83+84+## Compatibility audit of every existing UPDATE_ENTRY producer85+86+Verified against source before choosing these semantics:87+88+| 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 |96+97+Note 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; the100+patch path simply joins them.101+102+## messages.ts handler verification (no bypass)103+104+`UPDATE_ENTRY` (messages.ts:478–501) has exactly one mutation path:105+`vault.updateEntry(id, patch)` → `sanitizeEntryPatch` (vault.ts:1836). There is106+no 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.109+110+**Adjacent observation (no change made):** `vault.updateEntryDirect`111+(vault.ts:1978) spreads a patch verbatim, but it has **zero production112+callers** (grep: only its definition; the sync path uses113+`applyMergedRemoteEntries`, which funnels through `sanitizeRestoredEntry`).114+Flagged for a future dead-code pass; out of scope for this finding.115+116+## Tests117+118+`vault.update-entry-sanitize.test.ts` (in this folder) — 8 regression tests.119+Destination: `src/background/vault.update-entry-sanitize.test.ts` (imports are120+already in destination form).121+122+## Save message (integration)123+124+`fix: PCH-02-001 UPDATE_ENTRY patch routes canary/tags/passkey through existing sanitizers`

@@ -0,0 +1,213 @@1+// PCH-02-001 regression tests — UPDATE_ENTRY patch path must route2+// canary/tags/passkey through the same sanitizers the add/restore/sync3+// funnels use (sanitizeCanary / sanitizePasskey / sanitizeTags).4+//5+// DESTINATION: src/background/vault.update-entry-sanitize.test.ts6+// Imports are written in destination form; this file is excluded from the7+// suite while it lives under src/drafts/.8+9+import { beforeEach, describe, expect, it, vi } from "vitest";10+import type { PasskeyData, VaultEntry } from "../shared/types";11+12+type StorageShape = Record<string, unknown>;13+14+describe("PCH-02-001 — updateEntry patch sanitizers", () => {15+  let storage: StorageShape;16+17+  beforeEach(() => {18+    vi.resetModules();19+    storage = {};20+21+    const chromeMock = {22+      storage: {23+        local: {24+          get: vi.fn(async (key?: string | string[]) => {25+            if (!key) return storage;26+            if (typeof key === "string") {27+              return { [key]: storage[key] };28+            }29+            return key.reduce<Record<string, unknown>>((result, entry) => {30+              result[entry] = storage[entry];31+              return result;32+            }, {});33+          }),34+          set: vi.fn(async (next: Record<string, unknown>) => {35+            Object.assign(storage, next);36+          }),37+          remove: vi.fn(async (keys: string | string[]) => {38+            const list = Array.isArray(keys) ? keys : [keys];39+            for (const key of list) delete storage[key];40+          }),41+        },42+      },43+      alarms: {44+        clear: vi.fn(),45+        create: vi.fn(),46+        onAlarm: { addListener: vi.fn() },47+      },48+      idle: {49+        setDetectionInterval: vi.fn(),50+        onStateChanged: { addListener: vi.fn() },51+      },52+    } as unknown as typeof chrome;53+54+    (globalThis as { chrome?: typeof chrome }).chrome = chromeMock;55+56+    // Keep the fire-and-forget breach check off the network.57+    vi.stubGlobal("fetch", vi.fn(async () => {58+      throw new Error("network disabled in tests");59+    }));60+  });61+62+  async function vaultWithEntry(extra: Partial<VaultEntry> = {}) {63+    const vault = await import("./vault");64+    await vault.createVault("correct horse battery staple");65+    const entry = await vault.addEntry({66+      title: "Example",67+      username: "june",68+      password: "secret-1",69+      url: "https://login.example.com",70+      ...extra,71+    } as Parameters<typeof vault.addEntry>[0]);72+    return { vault, entry };73+  }74+75+  const validPasskey: PasskeyData = {76+    rpId: "example.com",77+    rpName: "Example",78+    credentialId: "Y3JlZC0x",79+    publicKeyCose: "cGstY29zZQ",80+    userHandle: "dXNlci0x",81+    userName: "june",82+    userDisplayName: "June",83+    algorithm: -7,84+    signCount: 0,85+    aaguid: "",86+    backupEligible: false,87+    backupState: false,88+    createdAt: 1_700_000_000_000,89+  };90+91+  it("strips a non-https canary webhook from an UPDATE_ENTRY patch", async () => {92+    const { vault, entry } = await vaultWithEntry();93+94+    await vault.updateEntry(entry.id, {95+      canary: { webhookUrl: "http://attacker.example.com/exfil" },96+    });97+98+    const updated = vault.getEntries().find((e) => e.id === entry.id);99+    expect(updated?.canary).toBeUndefined();100+  });101+102+  it("preserves a valid https canary (and its bump fields) through a patch", async () => {103+    const { vault, entry } = await vaultWithEntry();104+105+    // Shape the content-script canary-ping bump sends (content/index.ts:336-348).106+    await vault.updateEntry(entry.id, {107+      canary: {108+        webhookUrl: "https://legit.example.com/hook",109+        label: "desk drawer",110+        lastPinged: 1_700_000_111_000,111+        pingCount: 3,112+      },113+    });114+115+    const updated = vault.getEntries().find((e) => e.id === entry.id);116+    expect(updated?.canary).toEqual({117+      webhookUrl: "https://legit.example.com/hook",118+      label: "desk drawer",119+      lastPinged: 1_700_000_111_000,120+      pingCount: 3,121+    });122+  });123+124+  it("rebuilds the canary field-by-field — unknown keys are dropped", async () => {125+    const { vault, entry } = await vaultWithEntry();126+127+    await vault.updateEntry(entry.id, {128+      canary: {129+        webhookUrl: "https://legit.example.com/hook",130+        exfil: "smuggled",131+        __proto__: { polluted: true },132+      } as unknown as VaultEntry["canary"],133+    });134+135+    const updated = vault.getEntries().find((e) => e.id === entry.id);136+    expect(updated?.canary?.webhookUrl).toBe("https://legit.example.com/hook");137+    // sanitizeCanary rebuilds the object — only the four whitelisted keys exist.138+    const keys = Object.keys(updated!.canary!);139+    expect(keys.every((k) => ["webhookUrl", "label", "lastPinged", "pingCount"].includes(k))).toBe(true);140+    expect((updated!.canary as Record<string, unknown>).exfil).toBeUndefined();141+    expect((updated!.canary as Record<string, unknown>).polluted).toBeUndefined();142+  });143+144+  it("rejects a malformed passkey patch (missing credentialId) and keeps the entry unchanged", async () => {145+    const { vault, entry } = await vaultWithEntry();146+147+    await expect(148+      vault.updateEntry(entry.id, {149+        passkey: {150+          rpId: "example.com",151+          // credentialId missing — must not replace/attach a credential152+          publicKeyCose: "cGstY29zZQ",153+          userHandle: "dXNlci0x",154+          userName: "june",155+        } as unknown as PasskeyData,156+      }),157+    ).rejects.toThrow("invalid passkey credential");158+159+    const updated = vault.getEntries().find((e) => e.id === entry.id);160+    expect(updated?.passkey).toBeUndefined();161+    expect(updated?.password).toBe("secret-1");162+  });163+164+  it("accepts a complete passkey patch (field-by-field rebuild)", async () => {165+    const { vault, entry } = await vaultWithEntry();166+167+    await vault.updateEntry(entry.id, { passkey: validPasskey });168+169+    const updated = vault.getEntries().find((e) => e.id === entry.id);170+    expect(updated?.passkey?.rpId).toBe("example.com");171+    expect(updated?.passkey?.credentialId).toBe("Y3JlZC0x");172+    expect(updated?.passkey?.algorithm).toBe(-7);173+  });174+175+  it("still removes a passkey via `passkey: undefined`", async () => {176+    const { vault, entry } = await vaultWithEntry();177+    await vault.updateEntry(entry.id, { passkey: validPasskey });178+179+    await vault.updateEntry(entry.id, { passkey: undefined });180+181+    const updated = vault.getEntries().find((e) => e.id === entry.id);182+    expect(updated?.passkey).toBeUndefined();183+  });184+185+  it("collapses a proto-pollution tags shape (non-array) to undefined", async () => {186+    const { vault, entry } = await vaultWithEntry();187+    await vault.updateEntry(entry.id, { tags: ["keep-me"] });188+189+    await vault.updateEntry(entry.id, {190+      tags: JSON.parse('{"__proto__": ["evil"], "0": "fake"}') as unknown as string[],191+    });192+193+    const updated = vault.getEntries().find((e) => e.id === entry.id);194+    expect(updated?.tags).toBeUndefined();195+    expect(({} as Record<string, unknown>).polluted).toBeUndefined();196+  });197+198+  it("bounds patch tags: strings only, ≤256 chars each, ≤255 tags", async () => {199+    const { vault, entry } = await vaultWithEntry();200+201+    const oversized = "x".repeat(500);202+    const many = Array.from({ length: 300 }, (_, i) => `tag-${i}`);203+    await vault.updateEntry(entry.id, {204+      tags: [oversized, 42, null, "  ", ...many] as unknown as string[],205+    });206+207+    const updated = vault.getEntries().find((e) => e.id === entry.id);208+    expect(updated?.tags).toBeDefined();209+    expect(updated!.tags!.length).toBeLessThanOrEqual(255);210+    expect(updated!.tags![0]).toBe("x".repeat(256));211+    expect(updated!.tags!.every((t) => typeof t === "string" && t.length <= 256)).toBe(true);212+  });213+});