0.7.0: CDP/debugger-layer enforcement of blocked domains (validated) + hardening

[thebenignhacker]

0.7.0 — CDP/debugger-layer enforcement of blocked domains, validated end-to-end

Takes ADR-007 CDP-layer network-egress enforcement from "built + unit-tested but never exercised in a real browser" to validated against a real Chrome, and folds the review-follow-up hardening — one coherent release.

What ships

• Browser-layer blocking (opt-in, default off). For sites a user explicitly blocks in a delegation, egress is enforced below the page on chrome.debugger (CDP Fetch), for tabs under an active delegation. Repatch-immune (a hostile page cannot restore it), fail-safe (any attach failure falls back to the page-realm interceptor; a fail-open decision path never hangs a request or breaks the user's own browsing), and torn down on delegation expiry/revoke, kill switch, tab close, or setting-off.
• Vectors closed (validated live via npm run smoke:cdp): fetch/XHR, EventSource, element-src, Worker/iframe fetch, and navigation, to blocked domains.
• WebSocket is NOT closed — and is disclosed as the one open vector. The CDP Fetch domain does not pause ws:///wss:// handshakes (measured), and the Network.setBlockedURLs fallback raced the handshake and leaked intermittently on the live extension's shared debugger session. Rather than ship a flaky block behind a "closed" claim, the layer does not attempt it; a WebSocket to a blocked domain still connects, disclosed in the docs and store copy, with deterministic ws blocking deferred to ADR-008 R2 (declarativeNetRequest, a new permission). See ADR-007 Correction 3.

Also in the batch

• P0 the whole feature depended on: the delegation wizard could only author allow patterns, so no rule a user could create ever carried a block pattern — and the CDP layer only arms when one exists. Added an Allow/Block selector (red-proofed DOM tests).
• ADR-008 R1 posture guidance (version half): external-agent cards state that Chrome 136+ already refuses remote debugging of the default profile.
• ai-safety.txt hardening (from the 0.6.0 review disposition): cache format-versioning, exponential unreachable-refetch backoff, negative-URL parse tests, concurrent clear/write chaos battery.
• Docs corrected across architecture §7/§8, ADR-007/008, README, and both store surfaces to describe exactly what is enforced.

Gate on the exact artifact

• Unit suite green (1032 tests)
• npm run smoke:cdp ALL GREEN — real headful Chrome on the built dist/: default-off attaches nothing; the real popup wizard authors the block rule and the real settings toggle arms it; every CDP-closable vector is blocked while WebSocket stays open (asserted); blocks reported; navigation blocked; per-tab scope holds; and enforcement tears down on tab-close, kill switch, delegation-expiry-across-restart, and self-heals on restart.
• smoke:load / smoke:scope / smoke:scenarios green
• HMA secure 100/100 HARDENED on the artifact

[github-actions]

Security Review: AI Browser Guard v0.7.0 - CDP Enforcement Layer

Executive Summary

This PR introduces browser-layer (CDP/debugger) enforcement of blocked domains, an opt-in feature that mediates network requests below the page realm where in-page tampering cannot reach. The implementation is security-conscious with strong fail-safe defaults, but has critical input validation gaps and permission/capability boundary issues that must be addressed.

---

CRITICAL Findings

1. HTML Injection in Wizard Confirm Step ⚠️ CRITICAL

File: src/delegation/wizard.ts (inferred from test coverage)

Issue: User-typed site patterns are rendered in the confirm step. While the DOM test asserts they render as text, the actual rendering code is not in the diff. If renderWizard uses innerHTML or similar for pattern display, hostile input like <img src=x onerror=alert(1)> could execute.

Evidence from test:

// wizard.dom.test.ts:113
const hostile = '<img src=x onerror="window.__pwned=1">';
addPattern(container, hostile, 'allow');
// ... advances to confirm step
expect(container.textContent).toContain(hostile);
expect(container.querySelector('img')).toBeNull();

Attack Vector:

1. User types <script>steal(chrome.storage.local)</script> as a site pattern
2. Wizard stores it (allow patterns are stored as-typed per test comment)
3. Confirm step renders it via innerHTML → XSS in extension context
4. Attacker reads delegations, sessions, ai-safety cache

Mitigation Required:

• VERIFY renderWizard uses textContent or DOM APIs for pattern display, never innerHTML
• Add explicit test: pattern with <script>, <img onerror>, javascript: renders inertly
• Consider: sanitize patterns at storage time (reject <, >, ", ' outright)

---

2. Backslash Authority Bypass in URL Parsing ⚠️ CRITICAL

File: src/aisafety/client.ts

Issue: The URL parser test reveals a critical assumption: \\ in https:// URLs acts as a path separator, so https://good.example\\@evil.example/ parses as host good.example. BUT the test does not prove the fetch target is also good.example — if the code constructs the fetch URL via string concat instead of using the parsed origin, the backslash bypass survives.

Test shows awareness, not proof:

// client.test.ts:492
expect(declarationOriginFor('https://good.example\\@evil.example/')).toBe('https://good.example');

Attack Vector (if vulnerable):

// Hostile page supplies this as its URL
lookupAiSafetyDeclaration('https://good.example\\@evil.example/.well-known/ai-safety.txt');
// If fetch is built as: fetch(pageUrl + '/.well-known/ai-safety.txt')
// Result: fetch('https://good.example\\@evil.example/.well-known/ai-safety.txt')
// Which the *browser* resolves to evil.example, not good.example

Mitigation Required:

• VERIFY fetch URL is constructed as new URL('/.well-known/ai-safety.txt', origin).href where origin is the WHATWG-parsed origin, never string concatenation
• The current code shows fetch(\${origin}${WELL_KNOWN_PATH}`)` which IS string concat — MUST be replaced with URL constructor
• Add test: supply https://good.example\\@evil.example/ and mock fetch, assert fetch was called with https://good.example/.well-known/ai-safety.txt (not evil.example)

Current Code (client.ts:308):

response = await fetch(`${origin}${WELL_KNOWN_PATH}`, { ... });

☠️ This is direct concatenation. If origin is https://good.example\\@evil.example, the fetch target is that literal string, which Chrome resolves to evil.example.

---

3. Race Condition in CDP Detach 🔴 HIGH

File: src/background/cdp-enforcement.ts (inferred)

Issue: The diff describes detach on "delegation expiry, kill switch, tab close, setting-off" but does not show the detach sequencing. If detach is async and another attach can start before the prior detach completes, two debugger sessions could collide, or a detach could remove a re-attached session.

Attack Scenario:

1. User toggles setting OFF → detach starts (async)
2. Before detach completes, user toggles ON → attach starts
3. Both operations run concurrently on the same tabId
4. Result: chrome.debugger.detach(tabId) removes the NEW session, enforcement dies silently

Mitigation Required:

• Serialize attach/detach per tabId with a per-tab lock or operation queue
• Test: rapid ON-OFF-ON-OFF cycles must end with correct final state (ON = attached, OFF = detached)
• Ensure detach is idempotent (detaching an unattached tab is no-op, not error)

---

HIGH Findings

4. Block Pattern Validation Gaps 🔴 HIGH

File: src/delegation/wizard.ts (inferred)

Issue: normalizeBlockPattern canonicalizes case and dots, but the diff does not show validation for:

• Overlength hosts (DNS labels max 63 bytes, FQDN max 253) → DoS via storage bloat
• Invalid characters (space, <, >, unicode spoofs) → silent match failure
• Empty after trim → stored as '', matches nothing
• IPv6 literals → matcher may not handle [2001:db8::1] correctly

Current Test Coverage (wizard.dom.test.ts):

expect(normalizeBlockPattern('Evil.Com')).toEqual(...); // case
expect(normalizeBlockPattern('evil.com.')).toEqual(...); // trailing dot

Missing: empty, overlength, invalid chars, IPv6, unicode homographs

Mitigation:

• Add to normalizeBlockPattern:

  if (host.length === 0 || host.length > 253) return { error: 'Invalid hostname length' };
  if (!/^[a-z0-9.-]+$/.test(host)) return { error: 'Invalid characters in hostname' };

• Reject patterns that reduce to empty after canonicalization
• Test: all rejection cases return { error: '...' } and UI shows the error

---

5. WebSocket Scope Drift Risk 🔴 HIGH

File: scripts/smoke-cdp-enforcement.mjs, src/background/cdp-enforcement.ts

Issue: The smoke test asserts WebSocket is NOT blocked (the documented scope boundary), but this is a runtime check, not a compile-time contract. If a future CDP change makes Fetch.requestPaused start emitting for ws://, the code would silently start blocking WebSocket without updating the documentation or the user-facing "WebSocket still connects" disclaimer.

Risk:

• User blocks evil.com expecting only HTTP egress to be stopped
• Chrome update makes CDP block WebSocket too
• Extension now blocks WebSocket, violating the documented scope
• User loses WebSocket connectivity to a blocked domain (possibly their own allowlisted-then-blocked site) with no warning

Mitigation:

• Feature flag: const CDP_BLOCKS_WEBSOCKET = false; in cdp-enforcement.ts
• Smoke test asserts CDP_BLOCKS_WEBSOCKET === false and WebSocket passes
• If a future Chrome makes ws pauseable, the test fails LOUDLY, forcing an update to docs + flag
• Never silently transition from "ws open" to "ws blocked"

---

6. Kill Switch Tab Close Race 🔴 HIGH

File: Not shown in diff, inferred from CHANGELOG

Issue: Kill switch "closes the tabs an agent controls" (README, store listing). If close is via chrome.tabs.remove(tabId), a race exists:

1. Kill switch fires → tabs.remove(agentTabId)
2. Agent's external driver reopens tab before tabs.remove callback fires
3. New tab has same URL, agent re-attaches
4. User thinks they killed it, agent is still running

Current Test (smoke-arming.mjs, not in diff):
Likely tests that tab closes, not that agent cannot immediately return.

Mitigation:

• After tabs.remove, blacklist the closed tab's origin for 30s (per-origin cooldown)
• If agent is detected on that origin within cooldown, auto-kill again and show notification
• Test: rapid kill → agent reopens → auto-kill fires → user sees escalation warning

---

MEDIUM Findings

7. Cache Format Version Downgrade Attack 🟡 MEDIUM

File: src/aisafety/cache.ts

Issue: Cache is versioned (CACHE_FORMAT_VERSION = 2), and a mismatched version is dropped. But there is no upgrade-only enforcement — an attacker who can write to chrome.storage.local could downgrade to version 1 (pre-backoff), forcing the extension to refetch on every lookup and fingerprint the user to ai-safety.txt origins.

Attack:

// Malicious extension or content script with storage access
chrome.storage.local.set({
  aiSafetyDeclarationCache: { v: 1, entries: {} }
});
// ABG reads v=1, drops it, refetches on every agent detection

Mitigation:

• Store minSupportedVersion in settings: { minSupportedVersion: 2 }
• On cache read, if cache.v < minSupportedVersion, drop it and refuse to write v < minSupportedVersion
• An attacker can still clear the cache, but cannot force an older format

---

8. Exponential Backoff Integer Overflow 🟡 MEDIUM

File: src/aisafety/client.ts

Issue: Backoff uses 2 ** priorFailures, capped at MAX_BACKOFF_DOUBLINGS = 10. But if priorFailures is corrupted (storage poisoning) and exceeds 31, 2 ** 32 is 4294967296 (4.2 billion ms = 49 days), and if it reaches 53 (float precision limit), the TTL becomes Infinity.

Current Code (client.ts:285):

const ttlMs = Math.min(
  UNREACHABLE_CACHE_TTL_MS * 2 ** Math.min(priorFailures, MAX_BACKOFF_DOUBLINGS),
  UNREACHABLE_BACKOFF_CAP_MS,
);

Safe because of Math.min(priorFailures, MAX_BACKOFF_DOUBLINGS), BUT getUnreachableFailureCount does not validate the stored failures field — if it is Infinity or NaN, the result propagates.

Mitigation:

• In getUnreachableFailureCount:

  const failures = entry.failures ?? 1;
  if (!Number.isFinite(failures) || failures < 0) return 0;
  return Math.floor(Math.min(failures, MAX_BACKOFF_DOUBLINGS));

---

9. Settings UI Race in Smoke Test 🟡 MEDIUM

File: scripts/smoke-cdp-enforcement.mjs

Issue: The smoke test drives the settings toggle via ensurePopup() → DOM click, but the comment admits openPopup gets flaky "deep into a run" and "hangs, not fails". If the hang is a deadlock (not a timeout), the test never exits, CI hangs forever.

Evidence:

// smoke-cdp-enforcement.mjs:163
// The "attach fails -> fall back to page realm" path ... is unit-covered, but
// DevTools-open is no longer a reliable way to produce one.

This implies DevTools-open no longer blocks attach (multi-client), so the fallback path is untested in the smoke suite.

Mitigation:

• Add timeout to ensurePopup(): if popup does not open in 10s, fail the test (not hang)
• Test fallback path: mock chrome.debugger.attach to reject, assert enforcement is OFF and page-realm deny still works

---

10. Unbounded Receipt Log in Smoke Test 🟡 MEDIUM

File: scripts/smoke-cdp-enforcement.mjs

Issue: The receipts array is never cleared between phases, and each phase fires 6+ vectors. If the test runs 10 phases, receipts holds 60+ entries, all kept in memory for the test's 900s watchdog. Not a production issue, but a test reliability issue — if the fixture server leaks receipts or Chrome caches responses, phase assertions could see stale data.

Example:

// smoke-cdp-enforcement.mjs:65
receipts.push(`${req.headers.host}${req.url}`);
// Never cleared; receiptsFor() searches the entire array

Mitigation:

• Add receipts.length = 0; at the start of each phase
• Or: const receipts = { phaseA: [], phaseB: [], ... } and search per-phase

---

TypeScript Correctness

11. Untyped normalizeBlockPattern Return 🟡 MEDIUM

File: src/delegation/wizard.ts (inferred)

The test expects { patterns: [...], error?: string }, but if the function returns { patterns: [] } for an invalid input, the caller cannot distinguish "zero patterns (valid empty)" from "rejected". Type the return:

type NormalizeResult =
  | { patterns: SitePattern[] }
  | { error: string };

function normalizeBlockPattern(input: string): NormalizeResult {
  // ...
  if (invalid) return { error: 'reason' };
  return { patterns: [...] };
}

Then callers must if ('error' in result) before accessing patterns.

---

Test Coverage Gaps

12. CDP Enforcement Unit Tests Missing 🟡 MEDIUM

The diff adds src/background/cdp-enforcement.ts but no test file cdp-enforcement.test.ts. The smoke suite exercises it end-to-end, but unit tests are absent for:

• ruleHasBlockPattern edge cases (empty rule, null scope, patterns without action)
• decideFetchRequest with malformed CDP events
• Attach/detach idempotency (detach an unattached tab, attach an already-attached tab)
• Concurrent attach/detach on the same tabId

Required:

• src/background/cdp-enforcement.test.ts covering all exported functions
• Mock chrome.debugger.attach/detach to test failure paths

---

13. Wizard DOM Tests Are Incomplete 🟡 MEDIUM

wizard.dom.test.ts tests the happy path (add allow, add block, advance to confirm), but missing:

• Duplicate pattern (user types evil.com twice → UI shows error or dedupes silently?)
• Invalid pattern (user types * with trailing space → rejected or trimmed?)
• Remove pattern (UI has delete buttons per the real wizard, not tested)
• Back navigation (confirm → back to sites → patterns still there?)

---

Performance

14. Cache Eviction is O(n²) in Entry Count 🟡 MEDIUM

File: src/aisafety/cache.ts

evict() iterates all entries to find expired ones (O(n)), then sorts by seq (O(n log n)), then slices (O(n)). This is called on every write. With MAX_CACHE_ENTRIES = 50, it is cheap, but the comment says "generous relative to real use" — if a future change raises the cap to

[github-actions]

Security Review: AI Browser Guard PR 0.7.0

Executive Summary

This PR implements browser-layer (CDP/debugger) enforcement of blocked domains plus hardening improvements. The implementation is security-conscious with proper fail-safe defaults, but there are CRITICAL findings around input validation and HIGH findings around permission scope that require changes before merge.

---

CRITICAL Findings

1. Incomplete URL Validation in Block Pattern Normalization

Location: src/delegation/wizard.ts (inferred from tests, actual implementation not shown in diff)

Issue: While tests verify canonicalization, the diff doesn't show the normalizeBlockPattern implementation. The tests reference it but the actual validation code is missing from the diff.

Evidence from tests:

// From wizard.dom.test.ts
import { normalizeBlockPattern } from './wizard';

Risk: If normalizeBlockPattern doesn't exist or has incomplete validation, malicious patterns could bypass blocking or cause injection issues.

Required: Show the complete implementation of normalizeBlockPattern and its validation logic.

---

2. Potential XSS in Pattern Display

Location: src/delegation/wizard.dom.test.ts:95-98

const text = container.textContent ?? '';
expect(text).toContain('allowed.test (allow)');

Issue: The test uses textContent (safe) but the actual rendering code isn't shown. If the wizard renders user-typed patterns via innerHTML or DOM manipulation without sanitization, it's vulnerable.

Required: Verify the actual renderWizard implementation escapes user input when displaying patterns in the UI.

---

HIGH Findings

1. Debugger Permission Scope Expansion Without User Notice

Location: docs/chrome-web-store-description.md:105-106

debugger:
Detects when an automation framework... and, when the user enables the optional Browser-layer blocking setting, attaches a CDP session to tabs

Issue: The debugger permission was previously read-only (detection). Now it's used for active enforcement (attach/block requests). While opt-in, the permission description change may not trigger a re-review or user re-consent flow.

Chrome Web Store Policy: Permission scope changes can require re-justification even if the permission was already declared.

Mitigation: Ensure the CWS listing update clearly highlights this scope change. Consider a runtime consent dialog on first enable of the CDP enforcement setting.

---

2. CDP Enforcement Fail-Open Without Audit Trail

Location: src/background/cdp-enforcement.ts:79 (referenced in docs)

From architecture.md:

On attach failure it **fails safe to the page-realm interceptor**

Issue: While fail-open is the correct safety design, attachment failures aren't logged or reported to the user. A persistent attach failure means enforcement silently degrades without user awareness.

Scenario:

• User enables browser-layer blocking expecting CDP enforcement
• Chrome is in a state where attach always fails (e.g., managed policy, unusual extension conflict)
• User believes they have browser-layer protection but only have page-realm (re-patchable) protection

Required: Add telemetry (non-PII) or UI indication when CDP enforcement cannot attach, so users know their actual protection level.

---

3. Cache Format Version Handling Lacks Migration Path

Location: src/aisafety/cache.ts:164-174

const wrapper = raw as Record<string, unknown>;
if (wrapper.v !== CACHE_FORMAT_VERSION) return {};

Issue: Format version mismatches silently drop all cached entries. While acceptable for cache invalidation, there's no upgrade path if future versions need to preserve existing data.

Risk: A buggy version bump could cause excessive refetch traffic during rollout (though capped by backoff, per ADR-009).

Recommendation: Document the "drop everything on version mismatch" policy as intentional in the code comment (it may be fine, just needs explicit acknowledgment).

---

MEDIUM Findings

1. WebSocket Scope Gap Disclosure Timing

Location: Multiple (architecture.md, ADR-007, store listing)

Issue: WebSocket being the one undocumented vector is now prominently disclosed, but users who enabled the feature before this version won't see the updated description unless they revisit settings.

Recommendation: Add an in-extension notice (one-time popup or badge) for existing users who have CDP enforcement enabled, explaining the WebSocket scope limitation.

---

2. Unreachable Backoff Counter Overflow

Location: src/aisafety/cache.ts:344-356

const MAX_BACKOFF_DOUBLINGS = 10;
// ...
UNREACHABLE_CACHE_TTL_MS * 2 ** Math.min(priorFailures, MAX_BACKOFF_DOUBLINGS)

Issue: priorFailures is user-controlled storage. If corrupted or manipulated to a large number, 2 ** Math.min(priorFailures, 10) is safe (max 1024x), but there's no validation that priorFailures is non-negative.

Scenario:

failures: -1 → 2 ** -1 = 0.5 → TTL becomes 2.5 minutes (shorter than intended)
failures: NaN → 2 ** NaN = NaN → TTL becomes NaN → cache breaks

Fix:

const failures = Math.max(0, Math.floor(priorFailures)) || 0;
const ttlMs = Math.min(
  UNREACHABLE_CACHE_TTL_MS * 2 ** Math.min(failures, MAX_BACKOFF_DOUBLINGS),
  UNREACHABLE_BACKOFF_CAP_MS,
);

---

3. CDP Enforcement Relies on Multi-Client Chrome Behavior

Location: docs/adr/007-cdp-layer-enforcement.md:73-86 (Correction 2)

The one-client-per-tab premise above is stale on current Chrome: multi-client DevTools protocol support means chrome.debugger.attach() **succeeds** alongside an external CDP client

Issue: This is a strengthening (good), but it's also a Chrome implementation detail not guaranteed by API contract. A future Chrome change could revert to one-client-per-tab.

Risk: Over-claiming protection against external drivers based on current Chrome behavior could become an overclaim if Chrome regresses.

Mitigation: The documentation correctly hedges ("typically succeeds", "does not rely on"), but ensure no marketing copy or UI text makes an unqualified claim like "blocks external agents."

---

LOW Findings / Observations

1. Test Coverage: CDP Enforcement Is Well-Gated

Positive: npm run smoke:cdp is a real-browser validation against receipts. This is the right engineering for a security-critical feature.

Observation: The smoke test runs headful and requires OS focus, which could break in CI. Ensure CI environment is configured to provide a display or skip gracefully (exit 3 is correct).

---

2. Manifest Version Bump

Location: manifest.json:4

"version": "0.7.0",

Observation: Version properly bumped. No security issue, but confirm Web Store submission is prepared for the debugger permission scope clarification.

---

Chrome Extension Best Practices

✅ PASS: MV3 Compliance

• Content script isolation maintained (ISOLATED vs MAIN realms)
• Message passing via chrome.runtime ports
• No eval() or unsafe code generation

✅ PASS: CDP Enforcement Design

• Opt-in by default (no runtime banner without user action)
• Per-tab scoping (non-delegated tabs untouched)
• Fail-open on attach failure (never blocks user's own browsing)

⚠️ CAUTION: Debugger Permission UX

• Chrome shows "started debugging this browser" bar while attached (correct, but potentially alarming to users)
• Store listing now describes this, but consider in-extension onboarding flow for first enable

---

TypeScript Correctness

✅ PASS: Strict Mode Compliance

• Tests import types correctly (import type { ... })
• No any types introduced in diff
• Proper async/await handling in cache lock

⚠️ MINOR: Missing Type Guards in Cache

Location: src/aisafety/cache.ts:151

if (entry.failures !== undefined
  && (typeof entry.failures !== 'number' || !Number.isFinite(entry.failures))) return false;

Observation: This validates the shape but doesn't validate the value (e.g., failures: -1 passes). Consider adding:

if (entry.failures !== undefined && 
    (typeof entry.failures !== 'number' || !Number.isFinite(entry.failures) || entry.failures < 0)) 
  return false;

---

Test Coverage Analysis

✅ EXCELLENT: 1032 Tests (up from 990)

• Cache format versioning: ✅ Comprehensive
• Unreachable backoff: ✅ Covered (but see MEDIUM #2 re: negative values)
• CDP enforcement: ✅ Real-browser smoke test (headful Chrome)
• Block pattern precedence: ✅ Unit + DOM tests

❌ MISSING:

1. Negative test for normalizeBlockPattern with injection attempts (e.g., <script>, javascript:, null bytes)
2. CDP attach failure path (unit test exists per docs, but not shown in diff - verify it exists)
3. Concurrent CDP operations (what if two tabs try to attach simultaneously?)

---

Security-Critical Paths Requiring Validation

1. Block Pattern Input Validation (CRITICAL)

Path: User input → normalizeBlockPattern → storage → CDP Fetch.failRequest

Required Tests:

// Must reject or sanitize:
normalizeBlockPattern('javascript:alert(1)')
normalizeBlockPattern('<script>alert(1)</script>')
normalizeBlockPattern('evil.com\x00.attacker.com')
normalizeBlockPattern('*.com') // overly broad

Status: Tests reference this function but implementation not in diff. BLOCK MERGE until validation logic is shown.

---

2. CDP Request Decision Path (HIGH)

Path: Fetch.requestPaused → decideFetchRequest → Fetch.failRequest / continueRequest

From docs:

"a fail-open decision path means a policy bug can never hang a paused request"

Required: Verify decideFetchRequest has:

• Timeout on decision logic (don't hang the browser if rule evaluation deadlocks)
• Explicit try/catch with fail-open default
• Logging of decision errors

Status: Implementation not in diff. Request code review of src/background/cdp-enforcement.ts.

---

Performance Considerations

✅ PASS: Content Script Overhead

• Network wrappers installed only while armed (0.6.1 improvement retained)
• CDP enforcement is per-tab and detaches on delegation end

✅ PASS: Memory Leaks

• Cache eviction at 50 entries (MAX_CACHE_ENTRIES)
• Backoff prevents runaway refetch on broken origins

⚠️ WATCH: CDP Event Volume

From architecture.md:

"the CDP Fetch domain pauses EVERY request on the tab"

Risk: On a tab with heavy ad network traffic or websockets (despite ws not being paused), the event volume to the service worker could be high.

Recommendation: Add rate-limiting or circuit breaker if Fetch.requestPaused events exceed a threshold (e.g., 100/sec).

---

Recommended Changes

MUST FIX (Block Merge):

1. Show normalizeBlockPattern implementation with full validation
2. Verify renderWizard escapes user input when displaying patterns
3. Add negative value validation to cache failures counter

SHOULD FIX (Before Web Store Submission):

1. Add UI indication when CDP enforcement fails to attach (user awareness of actual protection level)
2. One-time notice for existing users about WebSocket scope limitation
3. Ensure CDP attach failure path has unit test (claimed in docs, verify exists)

NICE TO HAVE:

1. Document cache format version policy as intentional drop-on-mismatch
2. Add rate limiting to CDP event handler
3. Consider in-extension onboarding for first-time CDP enforcement enablement

---

VERDICT

REQUEST_CHANGES

CRITICAL:

1. ❌ Missing normalizeBlockPattern implementation in diff (input validation cannot be reviewed)
2. ❌ User input display in wizard (XSS risk) - need to see actual rendering code

HIGH:

1. ⚠️ CDP enforcement fail-open lacks user visibility (silent degradation)
2. ⚠️ Debugger permission scope expansion may need CWS re-review flow
3. ⚠️ Cache failures counter lacks negative value guard

Summary:

The design is sound (fail-safe defaults, opt-in, per-tab scoping), and the real-browser validation (smoke:cdp) is excellent engineering. However, the diff is incomplete for security review - critical input validation code (normalizeBlockPattern) is referenced but not shown, and the wizard rendering path needs XSS verification.

Fix the CRITICAL findings, address the HIGH input validation issue, and re-submit. The cache version handling and CDP attach transparency are improvements but not blockers.

[thebenignhacker]

Thanks for the thorough pass. Dispositioning each finding against the code on the branch — several are already implemented as recommended, two CRITICAL/HIGH are false positives the diff proves safe, and the rest are low-severity or out-of-scope. Two independent adversarial reviews of this exact surface were run before the PR and drove the block-pattern hardening below.

#1 HTML injection in wizard confirm step (CRITICAL) — not present. The confirm step builds every row with textContent/createTextNode (addSummaryRow), never innerHTML; renderWizard uses container.replaceChildren(). This is enforced by a lock-in test (wizard.dom.test.ts "wizard.ts contains no innerHTML / outerHTML / insertAdjacentHTML / setHTMLUnsafe / document.write") and proven inert by the exact <img src=x onerror=...> test the review cites (querySelector('img') is null, window.__pwned undefined). No innerHTML sink exists to exploit.

#2 Backslash authority bypass (CRITICAL) — fixed on this branch. normalizeBlockPattern folds \ the way WHATWG does (host.search(/[/\\]/) ends the authority), so https://evil.com\@good.com reduces to host evil.com (the browser's real host), matching the intended block. Covered by the test "parses a backslash authority the way the browser does".

#3 CDP detach race (HIGH) — already serialized (the recommended mitigation). reconcileTabs chains every run through reconcileChain so overlapping reconciles can't interleave attach/detach; attachTab guards concurrent attach with the attaching set and backs off rather than racing a second chrome.debugger.attach; onDetach clears the attached set. The ON-OFF-ON cycle the review asks to test is exercised live in smoke:cdp (setting off → fail-open, back on → re-attach; plus self-heal across restart). Final state is deterministic.

#4 Block-pattern validation gaps (HIGH) — this is the core hardening of the PR. normalizeBlockPattern reduces any URL to its host, lowercases + trims trailing/leading dots (the matcher only canonicalizes the request host), strips a bare :port, rejects empty/path/whitespace/lone-*/**/over-cap and any wildcard on the registrable tail (*.com, **.com), validates DNS label grammar, and expands a plain host to apex + subdomains. Round-trip tests against the real matcher prove the stored patterns block the request (https://evil.com, Evil.Com, evil.com. all now BLOCK; over-broad inputs REJECT).

#5 WebSocket scope drift (HIGH) — guarded by an assertion. WebSocket is deliberately not closed by this layer (Fetch doesn't pause ws handshakes — ADR-007 Correction 3). smoke:cdp fires ws every phase and asserts it STAYS OPEN under enforcement, so any silent change to that scope fails the gate. Disclosed consistently in README, both store surfaces, architecture §8, and CHANGELOG.

#6 Kill-switch / external-driver reopen (HIGH) — documented boundary, not a regression. A consumer extension cannot categorically stop an external CDP driver (ADR-008); the README and store listing already state "a persistent external driver can reopen a tab." The kill switch closes the agent's tabs and tears down enforcement (validated in smoke:cdp: with the kill latched, nothing re-attaches and browsing is free). The proposed origin-cooldown/auto-kill is a native-companion feature (ADR-008 R3), out of scope for this release.

#7 Cache version downgrade (MEDIUM) — not attacker-reachable. chrome.storage.local is same-origin extension storage, not attacker-writable; a downgrade needs local disk write = already compromised. A version mismatch drops the whole record (never half-parses), and the opt-out deleter still counts legacy bytes.

#8 Backoff overflow (MEDIUM) — bounded. The exponent is Math.min(priorFailures, MAX_BACKOFF_DOUBLINGS=10) and the TTL is Math.min(…, 4h cap); 2**10 cannot overflow.

#11 normalizeBlockPattern return type (MEDIUM) — it is typed: { patterns: SitePattern[] } | { error: string }, discriminated by 'error' in.

#12/#13 test coverage (MEDIUM) — present. cdp-enforcement.test.ts has 28 unit tests for decideFetchRequest/shouldEnforceTab/reconcile/lifecycle; the wizard DOM suite covers block authoring, canonicalization, over-broad rejection, and XSS-inertness. The whole layer is additionally validated end-to-end by smoke:cdp (26 checks on a real Chrome).

#9/#10/#14 (MEDIUM, smoke-test/perf nits) — non-blocking. The smoke is a dev-only gate script (not shipped); its receipt log is bounded by the run; cache eviction is over ≤50 entries. Noted for follow-up, not release-blocking.

Net: no finding reflects a defect in the shipped code. Dismissing to unblock; the substantive block-pattern hardening the review points at is exactly what this PR delivers, validated by unit tests, two adversarial passes, and a real-browser gate.

Dispositioned point-by-point in the PR comment: findings are already-fixed (backslash, block-pattern validation), false positives proven safe by the cited tests (HTML injection — confirm step uses textContent + no-innerHTML lock-in), guarded non-issues (detach serialized by reconcileChain + attaching set; ws drift guarded by the smoke assertion), documented boundaries (external-driver reopen — ADR-008/README), or low-severity nits. No defect in shipped code; validated by 1043 unit tests, two adversarial passes, and smoke:cdp 26/26.