fix(cdp): harden Comet launch and reject non-http(s) navigation

[sergei-aronsen]

Summary

Two defense-in-depth changes around the CDP transport.

`--remote-allow-origins`

Chromium's CDP endpoint has no auth. Historically any local process or any browser tab that resolves a hostname pointing at `127.0.0.1` could attach to it (DNS-rebinding-against-CDP, CVE-2018-6056 and follow-ons). The standard mitigation, shipped since m90, is `--remote-allow-origins=`: connections from origins not in the list are rejected at the handshake.

We now always launch Comet with `--remote-allow-origins=http://127.0.0.1\` (centralised in `cometLaunchArgs()`), so a drive-by website served to any browser on the host can no longer pivot through `ws://127.0.0.1:9223` to read Comet tabs.

Applied at all three native spawn sites and the PowerShell WSL launch. The WSL path now builds `-ArgumentList` from the same array via array-quote-escape join so the values cannot shell-inject through the PowerShell layer.

Protocol allowlist on `Page.navigate`

`navigate()` and `navigateWithRetry()` reject any URL whose protocol is not `http:` or `https:` via a new `assertNavigableUrl()` helper. Specifically blocks `javascript:`, `data:`, `vbscript:`, `file://`, `chrome:`, `devtools:`, `view-source:`, `about:`, and empty / unparseable strings.

Defense-in-depth against indirect prompt injection that targets the navigation primitives.

Compatibility

`http(s)` URLs work unchanged. Comet launches transparently with the new flag. Existing CDP clients connecting from `127.0.0.1` (this MCP server, the Chrome DevTools UI) are unaffected.

Test plan

• `npm run test:unit` passes
• Manual: `comet_connect` launches Comet on macOS / Windows / WSL
• Manual: navigation to https://example.com still works
• Adversarial: `Page.navigate` to `javascript:alert(1)` rejected before reaching CDP
• Adversarial: another browser tab cannot connect to `ws://localhost:9223` (Chromium handshake refuses)

Related

Part of an audit pass. Other open PRs: security (#11), correctness (#12), drop unused dep (#13), upload allowlist (#14).

🤖 Generated with Claude Code

[RapierCraft]

Code Review: fix(cdp): harden Comet launch and reject non-http(s) navigation

Reviewed commit: da00094

Overall Assessment: APPROVE

This is a well-structured defense-in-depth PR addressing two real attack surfaces: DNS rebinding against the unauthenticated CDP endpoint, and indirect prompt injection targeting navigation primitives. The implementation is correct and the scope is appropriately limited.

---

URL Validation (assertNavigableUrl)

Verdict: Correct and sound.

• Uses the URL constructor for parsing, which correctly handles edge cases like javascript:alert(1) (parses to protocol javascript:), JAVASCRIPT: (case-normalized by URL constructor to javascript:), and malformed strings.
• Allowlist approach (http: / https: only) is the right pattern — blocklisting dangerous protocols is fragile against future additions.
• Both navigate() and navigateWithRetry() are guarded. The only Page.navigate CDP calls in the file are at lines 1101 (inside navigate()) and 1126 (inside navigateWithRetry()), both reachable only after validation passes.
• The call at line 661 (await this.navigate(url, true)) routes through the validated navigate() method. No bypass path exists.

Minor observation (non-blocking): navigateWithRetry returns attempts: 0 on validation failure, which is a sensible sentinel value. Consider documenting this in the JSDoc for callers who check attempts >= 1 to distinguish "tried and failed" from "refused to try."

---

Launch Hardening (cometLaunchArgs + --remote-allow-origins)

Verdict: Correct.

• --remote-allow-origins=http://127.0.0.1 is the correct Chromium flag (available since M90+). It restricts WebSocket upgrade handshakes on the CDP port to only connections whose Origin header matches. This mitigates DNS rebinding attacks where a malicious page resolves to 127.0.0.1 but presents a different Origin.
• The function is applied at all three spawn(COMET_PATH, ...) call sites (lines 848, 882, 931) and the PowerShell WSL path (line 794). No spawn site was missed.
• The centralized cometLaunchArgs() helper is a good pattern — future flags only need to be added in one place.

---

PowerShell ArgumentList Escaping (WSL path)

Verdict: Correct with one minor concern.

The escaping logic:

const launchArgs = cometLaunchArgs(port)
  .map(a => `'${a.replace(/'/g, "''")}'`)
  .join(',');

This correctly handles PowerShell single-quote escaping (doubling single-quote to double-single-quote). Since the args come from cometLaunchArgs() which produces only --remote-debugging-port=NNNN and --remote-allow-origins=http://127.0.0.1, neither contains single quotes, so the escape is defensive-correct but not exercised today.

Minor concern (non-blocking): The cometPath variable (line 780/782) is still interpolated directly into the PowerShell command string without the same escaping treatment. If a Windows username contained a single quote (e.g., O'Brien), the Start-Process -FilePath would break. This is a pre-existing issue not introduced by this PR, but worth noting for a follow-up.

---

Security Considerations

1. No information leakage in error messages: The error messages include the protocol or "Invalid URL" — no sensitive data echoed back. Good.
2. No TOCTOU risk: assertNavigableUrl is called synchronously before Page.navigate in the same method body. No window for the URL to change between validation and use.
3. port parameter trust: port flows from this.state.port (default 9223) or the connect() argument. It is a number type — no injection risk into the arg string.

---

Suggestions (non-blocking, for consideration)

1. Unit tests: The PR description mentions npm run test:unit — adding explicit test cases for assertNavigableUrl with javascript:, data:, file://, empty string, and valid https:// would lock in the behavior and prevent regressions.

2. Consider --remote-allow-origins port matching: The current flag is http://127.0.0.1 (no port). Chromium matches on origin (scheme+host+port). Without a port, this allows connections from any http://127.0.0.1:* origin. If you want to restrict to only the DevTools port, use http://127.0.0.1:${port}. However, the current value is defensible — it blocks external origins while allowing local DevTools UI on any port.

3. Logging on rejection: For debugging MCP tool calls that hit the validation, consider emitting a debug/warn log when assertNavigableUrl throws, so operators can see which upstream input triggered the rejection.

---

Summary

Area	Status
URL validation correctness	Pass
All navigation paths guarded	Pass
Launch args applied at all spawn sites	Pass
PowerShell escaping	Pass
No new security issues introduced	Pass
No regressions to existing functionality	Pass

Recommendation: Safe to merge. The implementation is correct, well-documented, and addresses real security concerns with appropriate defense-in-depth measures.

[RapierCraft]

This PR has merge conflicts with main after recent merges (PRs #11, #12, #16, #18 landed).

The conflicts should be straightforward — the launch hardening and URL validation logic doesn't overlap semantically with what merged, just positional conflicts in shared files. Please rebase against main.

[RapierCraft]

Security Review: PR #15 — fix(cdp): harden Comet launch and reject non-http(s) navigation

Reviewed commit: b21d60c | Agents: Security Exploit Auditor + manual review
Verdict: APPROVE (with advisory notes)

---

CDP Launch Hardening — cometLaunchArgs()

Assessment: SOUND

• --remote-allow-origins=http://127.0.0.1 correctly mitigates DNS rebinding attacks against the CDP WebSocket endpoint (CVE-2018-6056 and successors)
• Centralized in cometLaunchArgs() — all three native spawn sites and the PowerShell WSL launch path use it
• PowerShell argument escaping via a.replace(/'/g, "''") is correct for PS single-quoted strings
• No command injection surface: port is validated as integer 1-65535, args are constructed from trusted values

URL Protocol Allowlist — assertNavigableUrl()

Assessment: SOUND

• Uses new URL(url) for parsing — handles edge cases (whitespace, encoding) correctly
• Allowlist approach (only http: and https:) is the right model — blocks javascript:, data:, file://, chrome:, devtools:, etc.
• Applied in both navigate() and navigateWithRetry() — no bypass path
• navigateWithRetry() validates before the retry loop — avoids wasting retries on bad URLs
• Error messages include the protocol but not the full URL body — prevents leaking attacker-controlled content in logs

Dead Code Removal

connectionCheckInterval — confirmed only declared, never assigned/read anywhere. Safe removal.

---

Advisory Notes (non-blocking)

1. MEDIUM — Error message in WSL timeout leaks args without --remote-allow-origins

cdp-client.ts:1080: The timeout error message suggests:

Try manually: powershell.exe ... -ArgumentList '--remote-debugging-port=${port}'

This omits --remote-allow-origins, so a user copy-pasting this troubleshooting command launches Comet without the security flag. Consider updating the help text to include both args.

2. LOW — assertNavigableUrl does not cover evaluateOnPage or evaluate

The URL validation only covers navigate() and navigateWithRetry(). If attacker-controlled input reaches Runtime.evaluate with a window.location = 'javascript:...' payload, the navigation occurs outside the validated path. This is defense-in-depth, not a direct vulnerability, since evaluate accepts arbitrary JS by design.

3. INFO — PR #15 is a superset of PR #14

This PR includes all the validateUploadPath + validateTabId changes from PR #14 plus the CDP hardening. The upload/tabId findings from the PR #14 review apply here too. Consider merging PR #15 only and closing PR #14 as superseded.

---

Summary

Clean, well-designed security hardening. The cometLaunchArgs() centralization is the right pattern — ensures all launch paths get the security flag. The URL protocol allowlist is a textbook defense-in-depth addition. No blocking issues found in the CDP-specific changes.

---

Security review complete. Reviewed: launch args, URL validation, PowerShell escaping, spawn sites, dead code.