--- phase: 04 slug: harden-clean-up-optional plan: 01 type: tdd wave: 1 depends_on: [] files_modified: - src/content/index.ts - tests/content/fetch-interception.test.ts - tests/content/navigation-tracking.test.ts - tests/content/rrweb-timestamps.test.ts autonomous: true requirements: [] tags: - audit-p1-polish - p1-11-fetch-url - p1-14-nav-url - p1-15-rrweb-timestamps - tdd - content-script - charter-d-p4-02 user_setup: [] must_haves: truths: - "fetch(new Request(url)) captures Request.url in the network_error UserEvent.target (NOT the literal string '[object Request]')" - "fetch(stringUrl) continues to capture the string URL in the network_error UserEvent.target (regression preserved)" - "popstate / hashchange navigation events carry the URL the operator just left in UserEvent.meta.previousUrl (NOT the literal string 'unknown')" - "rrweb emit events carry Unix-epoch timestamps (Date.now()-class, > 1e12 in 2026) so cleanupOldEvents' (now - event.timestamp) arithmetic is meaningful" - "vitest suite GREEN with +6 new tests across 3 new test files at tests/content/" artifacts: - path: "tests/content/fetch-interception.test.ts" provides: "Audit P1 #11 RED→GREEN — 3-test contract pinning Request-arg + string-arg URL extraction + network_error regression" contains: "instanceof Request" min_lines: 60 - path: "tests/content/navigation-tracking.test.ts" provides: "Audit P1 #14 RED→GREEN — 3-test contract pinning module-level previousUrl tracking" contains: "previousUrl" min_lines: 60 - path: "tests/content/rrweb-timestamps.test.ts" provides: "Audit P1 #15 RED→GREEN — 2-test contract pinning Unix-epoch timestamp semantics in rrweb emit + cleanupOldEvents arithmetic" contains: "Date.now()" min_lines: 50 - path: "src/content/index.ts" provides: "3 surgical edits — line 174 + 190 fetch URL extraction (Request-narrow); lines 99-109 + new module-level previousUrl; lines 286-289 rrweb emit timestamp normalization" contains: "instanceof Request" key_links: - from: "src/content/index.ts setupNetworkLogging fetch wrapper" to: "addUserEvent({type:'network_error', target: })" via: "args[0] instanceof Request ? args[0].url : String(args[0])" pattern: "instanceof Request" - from: "src/content/index.ts setupNavigationLogging handleNavigation closure" to: "module-level let previousUrl" via: "fromUrl = previousUrl; previousUrl = toUrl; emit with meta.previousUrl: fromUrl" pattern: "previousUrl = (window\\.location\\.href|toUrl)" - from: "src/content/index.ts initRrweb emit callback" to: "rrwebEvents.push(event) with event.timestamp = Date.now()" via: "event.timestamp = Date.now() before push" pattern: "event\\.timestamp = Date\\.now" --- Close the three Audit P1 correctness gaps in `src/content/index.ts` per D-P4-02 ("All three — Recommended"): - **P1 #11:** `args[0]?.toString()` becomes `'[object Request]'` when `fetch(new Request(url))` is called, masking the real failing URL in `events.json`. Replace with `args[0] instanceof Request ? args[0].url : String(args[0])` at both sites (line 174 ok-branch + line 190 catch-branch). - **P1 #14:** `history.state?.url || 'unknown'` always emits `'unknown'` in apps that don't populate `history.state` (which is almost all of them). Replace with module-level `let previousUrl` tracking — mutate on every navigation, emit the prior value in `meta.previousUrl`. - **P1 #15:** rrweb 2.0.0-alpha.4 emits page-load-relative timestamps (small integers); `cleanupOldEvents` at lines 27-50 does `(now - event.timestamp) < RRWEB_RETENTION_MS` arithmetic that's a category error. Normalize at emit time: `event.timestamp = Date.now()` before pushing to `rrwebEvents`. TDD-strict: Wave 0 creates 3 RED test files; the source edits in Wave 1 flip them GREEN. RED phase is mandatory per workflow.tdd_mode: true. Purpose: Each fix closes a real correctness bug (not stylistic). #11 unmasks the failing URL in saved archives, #14 unmasks the prior navigation URL operator left, #15 makes the rrweb cleanup arithmetic meaningful. These are operator-perceptible quality bugs in the archive contents — fixing them improves bug-reproduction value of saved sessions. Output: 3 new test files at `tests/content/` (NEW directory mirroring the source-tree convention `src/content/` → `tests/content/`); +6 GREEN tests on the vitest baseline; 3 surgical edits in `src/content/index.ts` (3-5 lines each). @$HOME/.claude/get-shit-done/workflows/execute-plan.md @$HOME/.claude/get-shit-done/templates/summary.md @.planning/PROJECT.md @.planning/ROADMAP.md @.planning/STATE.md @.planning/phases/04-harden-clean-up-optional/04-CONTEXT.md @.planning/phases/04-harden-clean-up-optional/04-RESEARCH.md @.planning/phases/04-harden-clean-up-optional/04-PATTERNS.md # Source files — locus of P1 fixes (read once; extract patterns) @src/content/index.ts @src/shared/types.ts # Analog test scaffold to mirror — pure-function-with-vi.fn-stub pattern @tests/background/start-video-capture-no-tab.test.ts @tests/background/onboarding.test.ts From src/content/index.ts (existing module-level state — additive pattern at lines 20-24): ```typescript let rrwebEvents: EventWithTime[] = []; // line 21 let userEvents: UserEvent[] = []; // line 24 // Phase 4 Plan 04-01 P1 #14 — ADD at line 25 (or adjacent to existing module-level state): let previousUrl = ''; // initialized in initOrSetUpInitialUrl() at content script init ``` From src/shared/types.ts (UserEvent shape — DO NOT MODIFY): ```typescript export interface UserEvent { timestamp: number; type: 'click' | 'input' | 'navigation' | 'js_error' | 'network_error'; target: string; value: string; url: string; meta?: Record; } ``` From src/content/index.ts:174 + :190 (existing P1 #11 bug sites — TWO instances, both must be fixed): ```typescript // Line ~174 (ok-branch): target: args[0]?.toString() || 'unknown', // Line ~190 (catch-branch): target: args[0]?.toString() || 'unknown', ``` After Phase 4 Plan 04-01: ```typescript // Helper at module scope OR inline in both branches: target: (args[0] instanceof Request ? args[0].url : String(args[0])) || 'unknown', ``` From src/content/index.ts:107 (existing P1 #14 bug site): ```typescript meta: { previousUrl: history.state?.url || 'unknown' } ``` After Phase 4 Plan 04-01: ```typescript // Module-level state add at ~line 25: let previousUrl = (typeof window !== 'undefined') ? window.location.href : ''; // In handleNavigation closure at line 100-109: const fromUrl = previousUrl; const toUrl = window.location.href; previousUrl = toUrl; addUserEvent({ timestamp: Date.now(), type: 'navigation', target: 'window', value: toUrl, url: toUrl, meta: { previousUrl: fromUrl }, }); ``` From src/content/index.ts:286-289 (existing P1 #15 bug site): ```typescript function initRrweb() { record({ emit(event) { rrwebEvents.push(event); }, ``` After Phase 4 Plan 04-01: ```typescript function initRrweb() { record({ emit(event) { event.timestamp = Date.now(); // Plan 04-01 P1 #15: normalize to Unix epoch for cleanupOldEvents arithmetic rrwebEvents.push(event); }, ``` Task 1: Wave 0 RED — three content-script unit-test scaffolds (one per audit P1 item) tests/content/fetch-interception.test.ts, tests/content/navigation-tracking.test.ts, tests/content/rrweb-timestamps.test.ts tests/background/start-video-capture-no-tab.test.ts, tests/background/onboarding.test.ts, src/content/index.ts, .planning/phases/04-harden-clean-up-optional/04-PATTERNS.md (sections "tests/content/fetch-interception.test.ts" through "tests/content/rrweb-timestamps.test.ts") fetch-interception.test.ts (3 tests): - Test A: `fetch(stringUrl)` where response.ok=false → `addUserEvent` captures `target === stringUrl` (NOT `'[object Object]'` or `'unknown'`). RED today (works for strings; PASS-LOOKING because String(string)===string is identity). - Test B: `fetch(new Request(url))` where response.ok=false → captures `target === request.url` (NOT `'[object Request]'`). RED today (bug — current code emits `'[object Request]'`). - Test C: regression — `fetch(stringUrl)` that throws (.catch path) → captures `target === stringUrl` AND `type === 'network_error'`. Pins the catch-branch fix too. navigation-tracking.test.ts (3 tests): - Test A: load content/index.ts; window.location starts at https://example.com/start; mutate to /next; dispatch popstate → captured UserEvent.meta.previousUrl === 'https://example.com/start' (NOT 'unknown'). RED today. - Test B: same shape via hashchange (location.hash mutation). RED today. - Test C: regression — history.pushState invocation still emits navigation with previousUrl honoring module state (not the just-pushed URL). rrweb-timestamps.test.ts (2 tests): - Test A: stub rrweb.record so its internal callback can be triggered with a synthetic event carrying timestamp=42 (page-load-relative class); after content script initRrweb runs, the captured rrwebEvents[0].timestamp > 1e12 (Date.now()-class). RED today. - Test B: regression — cleanupOldEvents at lines 27-50 correctly filters; the (now - event.timestamp) arithmetic is meaningful because both sides are Unix-epoch. Build a synthetic rrwebEvents containing one event with timestamp=Date.now()-RRWEB_RETENTION_MS-1000 (older than retention) + one event with timestamp=Date.now() (fresh); after cleanup, only the fresh event remains. Scaffold mirrors tests/background/start-video-capture-no-tab.test.ts: - `beforeEach(() => { vi.resetModules(); });` - Minimal `buildChromeStub()` providing `chrome.runtime.sendMessage` no-op + `chrome.runtime.onMessage.addListener` no-op (content script does not import chrome.tabs / chrome.storage directly). - Dynamic `await import('../../src/content/index.ts')` after stubbing window.fetch + history + rrweb (use vi.mock or stub-via-global). - For rrweb, mock the module: `vi.mock('rrweb', () => ({ record: vi.fn((opts) => { capturedEmit = opts.emit; }) }))`. 1. Create `tests/content/` directory (new — no existing tests at this path). 2. Create `tests/content/fetch-interception.test.ts` per the scaffold in 04-PATTERNS.md section "tests/content/fetch-interception.test.ts" (3-test contract). Use `vi.resetModules()` in `beforeEach`, `vi.mock('rrweb', ...)` to isolate from real rrweb (the rrweb module is irrelevant for fetch tests but the content script imports it at module top). The fetch wrapper is invoked by directly calling `window.fetch(...)` after the content script has run. 3. Create `tests/content/navigation-tracking.test.ts` per the same scaffold (3-test contract). Set up JSDOM-style window.location via vitest's environment: 'jsdom' in vitest.config.ts (already configured per the analog test). Dispatch popstate via `window.dispatchEvent(new PopStateEvent('popstate'))`. Read captured UserEvents via a global hook installed by the test (e.g., spy on `addUserEvent` via vi.spyOn after module import, OR capture via `chrome.runtime.onMessage` listener). 4. Create `tests/content/rrweb-timestamps.test.ts` per the same scaffold (2-test contract). Mock rrweb.record so the test can capture the `emit` callback handle, then synthesize an event with `timestamp: 42` (page-load-relative class) and assert that after `emit(synthetic)`, `rrwebEvents[0].timestamp > 1e12`. All 3 files use TypeScript strict mode (project default); imports use absolute paths from project root per CLAUDE.md ("Always use absolute imports"); filter-pipeline form, no `continue` per CLAUDE.md Control Flow rules. RED gate: run `npm test -- tests/content/ --run` — all 8 tests FAIL (Test A/C of fetch may pass on string args; Tests B + nav A/B + rrweb A guaranteed RED). npm test -- tests/content/ --run 2>&1 | tee /tmp/04-01-task-1-red.log; grep -c 'FAIL' /tmp/04-01-task-1-red.log - File `tests/content/fetch-interception.test.ts` exists with ≥ 3 `it(...)` blocks in a `describe('Audit P1 #11 ...')` block. - File `tests/content/navigation-tracking.test.ts` exists with ≥ 3 `it(...)` blocks in a `describe('Audit P1 #14 ...')` block. - File `tests/content/rrweb-timestamps.test.ts` exists with ≥ 2 `it(...)` blocks in a `describe('Audit P1 #15 ...')` block. - At least 4 tests FAIL when run (the RED tests for B/nav A/B/rrweb A); the regression tests may PASS pre-fix. - `grep -c "instanceof Request" tests/content/fetch-interception.test.ts | grep -v '^#'` returns ≥ 2 (Test B assertion + post-fix expectation). - `grep -c "previousUrl" tests/content/navigation-tracking.test.ts | grep -v '^#'` returns ≥ 3 (Tests A/B/C assertions). - `grep -cE "Date\\.now|timestamp > 1e12" tests/content/rrweb-timestamps.test.ts | grep -v '^#'` returns ≥ 2. 3 RED test files committed (8 tests; ≥ 4 RED, ≤ 4 passing-as-regression-baseline). Atomic commit: `test(04-01): Wave 0 RED — audit P1 #11/#14/#15 content-script test scaffolds`. Task 2: Wave 1 GREEN — 3 surgical edits in src/content/index.ts (P1 #11 + #14 + #15) src/content/index.ts src/content/index.ts (full; ~310 lines), tests/content/fetch-interception.test.ts, tests/content/navigation-tracking.test.ts, tests/content/rrweb-timestamps.test.ts All 8 unit tests from Task 1 flip GREEN after these 3 surgical edits. - Edit 1 (P1 #11): both fetch interception sites (ok-branch line ~174 + catch-branch line ~190) use `args[0] instanceof Request ? args[0].url : String(args[0])` to extract URL. - Edit 2 (P1 #14): add `let previousUrl = (typeof window !== 'undefined') ? window.location.href : '';` at module scope (~line 25); rewrite `handleNavigation` closure to swap `previousUrl` and emit prior value. - Edit 3 (P1 #15): in `initRrweb` (~line 286), prepend `event.timestamp = Date.now();` before `rrwebEvents.push(event);`. Read `src/content/index.ts` in full once (~310 lines, well under context limit). Extract exact line numbers for the 3 edit sites (they may have drifted since the PATTERNS.md mapping if any prior plan touched them — confirm via grep). All edits use the Edit tool with exact strings (no heredoc; no sed): Edit 1 — P1 #11 fetch URL extraction (TWO sites, identical pattern): - At the ok-branch (currently ~line 174): replace `target: args[0]?.toString() || 'unknown',` with `target: (args[0] instanceof Request ? args[0].url : String(args[0])) || 'unknown',` - At the catch-branch (currently ~line 190): same replacement (the two literal strings are identical so the Edit tool needs both replacements; use the `replace_all: false` discipline by using a slightly larger surrounding string per the Edit-tool spec, OR target each separately by including unique context). - Optionally factor out to a helper `function extractFetchUrl(arg: unknown): string` at module scope; either is acceptable. Edit 2 — P1 #14 navigation URL tracking: - Add at module scope (insert after the existing `let userEvents: UserEvent[] = [];` line ~24): `let previousUrl: string = (typeof window !== 'undefined') ? window.location.href : ''; // Plan 04-01 P1 #14 — module-level previous-URL tracker` - Rewrite the body of `handleNavigation` closure (currently lines 100-108): ```typescript const handleNavigation = () => { const fromUrl = previousUrl; const toUrl = window.location.href; previousUrl = toUrl; addUserEvent({ timestamp: Date.now(), type: 'navigation', target: 'window', value: toUrl, url: toUrl, meta: { previousUrl: fromUrl }, }); }; ``` - Preserve the `addEventListener('popstate', handleNavigation)` + `addEventListener('hashchange', handleNavigation)` + `history.pushState/replaceState` wrap exactly as-is (no behavior change to the dispatch mechanism). Edit 3 — P1 #15 rrweb emit timestamp normalization: - In the `record({ emit(event) { ... } })` block at ~line 286, prepend ONE line: `event.timestamp = Date.now(); // Plan 04-01 P1 #15 — normalize to Unix epoch for events.json downstream + cleanupOldEvents arithmetic correctness`. The existing `rrwebEvents.push(event);` stays unchanged. TypeScript-strict expectations: tsc-clean (`npx tsc --noEmit`). The `EventWithTime = any;` alias at line 10 already opens `event.timestamp` for write; if strict-mode complains, use a typed widening cast (NEVER `as any`) per CLAUDE.md rule. Run focused tests during edits: `npm test -- tests/content/ --run` should flip from 4-RED to 0-RED after all 3 edits. npm test -- tests/content/ --run 2>&1 | tee /tmp/04-01-task-2-green.log; grep -c 'PASS\\|✓' /tmp/04-01-task-2-green.log; npx tsc --noEmit 2>&1 | tee /tmp/04-01-task-2-tsc.log; grep -c 'error TS' /tmp/04-01-task-2-tsc.log - `npm test -- tests/content/ --run` exits 0 with 8/8 GREEN (was 4-RED in Task 1). - `npx tsc --noEmit` exits 0 (no new type errors). - `grep -c "instanceof Request" src/content/index.ts` returns ≥ 2 (both sites) AND `grep -cE "args\\[0\\]\\?\\.toString" src/content/index.ts` returns 0 (old pattern removed). - `grep -cE "^let previousUrl" src/content/index.ts` returns 1 (the module-level state declaration). - `grep -cE "meta: \\{ previousUrl: fromUrl \\}" src/content/index.ts` returns 1 (the new closure emit). - `grep -cE "event\\.timestamp = Date\\.now\\(\\)" src/content/index.ts | grep -v '^#'` returns ≥ 1 (the rrweb emit normalization; addUserEvent at line 54 already has its own `event.timestamp = Date.now()` so total may be 2). - Full vitest suite passes: `npm test -- --run` exits 0 with previous baseline + 8 new tests = ≥ 179 GREEN (171 baseline + 8 new). P1 #11 + #14 + #15 GREEN; tsc-clean; full vitest GREEN with +8. Atomic commit: `feat(04-01): Wave 1 GREEN — fix audit P1 #11 fetch URL + #14 nav URL + #15 rrweb timestamps`. ## Trust Boundaries | Boundary | Description | |----------|-------------| | page realm → content-script realm | `window.fetch` / `history.pushState` wrappers run in the content-script ISOLATED world after page realm dispatches; payloads (URLs, request objects) are page-controlled | | content script → SW (events.json downstream) | UserEvents accumulate in module-level arrays; flushed via `chrome.runtime.sendMessage` at archive-assembly time; the `meta` object is JSON-serialized verbatim | ## STRIDE Threat Register | Threat ID | Category | Component | Disposition | Mitigation Plan | |-----------|----------|-----------|-------------|-----------------| | T-04-01-01 | Information Disclosure | fetch wrapper line 174/190 — `args[0]?.toString()` masks the real URL with `'[object Request]'` when a page calls `fetch(new Request(url))` | mitigate | Replace with `args[0] instanceof Request ? args[0].url : String(args[0])` — explicit type-narrow before string conversion. This IS the V5 ASVS input-validation pattern. | | T-04-01-02 | Repudiation | navigation tracker always emits `previousUrl: 'unknown'` so support cannot determine where the operator was before the failing navigation; degrades the diagnostic value of saved archives | mitigate | Module-level `previousUrl` state; mutate after every emit; emit prior value. No third-party data introduced; same write surface as existing `rrwebEvents` / `userEvents` arrays. | | T-04-01-03 | Tampering | rrweb timestamps are page-load-relative; cleanupOldEvents' Unix-epoch-style arithmetic silently drops events incorrectly (`now - smallInt` is always >> retention) | mitigate | Normalize at emit: `event.timestamp = Date.now()`. rrweb's internal timestamps remain rrweb-internal (used by rrweb-player for replay timing); the wrapper writes Unix epoch only for events.json downstream. No external data introduced. | | T-04-01-04 | Information Disclosure | new previousUrl module state could leak across content-script reloads if the variable persisted across navigations | accept | content script is re-injected per Chrome's `` match-pattern on each top-level navigation; module state resets per content-script lifecycle. Same risk profile as existing `userEvents` / `rrwebEvents` arrays. | - `npm test -- --run` exits 0 (vitest baseline ≥ 171 → ≥ 179 with new tests). - `npx tsc --noEmit` exits 0. - `grep -c "instanceof Request" src/content/index.ts` returns ≥ 2 (audit P1 #11 invariant pin). - `grep -c "previousUrl" src/content/index.ts` returns ≥ 4 (module-state declaration + closure body uses + emit). - `grep -cE "event\\.timestamp = Date\\.now\\(\\)" src/content/index.ts` returns ≥ 1 in the rrweb-emit region (lines 280-300 — verify with `grep -n` + line range check). - UAT harness 33/33 GREEN preserved (rrweb-emit timestamp change does NOT affect harness assertions which read EventType enum values, not timestamp ranges). - 3 RED test files committed (Task 1; 8 tests; ≥ 4 RED). - 3 surgical edits in src/content/index.ts committed (Task 2; flips 4 RED → 0 RED). - vitest baseline 171 → ≥ 179 GREEN (+8). - tsc-clean preserved. - UAT harness 33/33 GREEN preserved. - ROADMAP success criterion #2 (fetch + XHR network_error) PARTIAL — fetch URL extraction fixed; XHR variant is correct-by-construction (XHR.send uses `_url` captured at open-time, not args[0]); empirical XHR-404 verification deferred to Plan 04-04. After completion, create `.planning/phases/04-harden-clean-up-optional/04-01-SUMMARY.md` capturing: - 3 test files added with test counts (3 + 3 + 2 = 8) - 3 edits with before/after snippets (1-liner each) - vitest baseline before/after counts - tsc-clean confirmation - RED→GREEN cycle commit refs (Task 1 + Task 2) - Open items for downstream plans: Plan 04-04 (A34 fetch+XHR empirical harness extension exercises the fix end-to-end)