fix(security): validate selector parameter before DOM.querySelector

[RapierCraft]

Summary

Adds validateSelector() to src/upload-validator.ts and calls it before the DOM.querySelector CDP invocation in cdp-client.ts, plus defense-in-depth validation at the comet_upload tool-handler boundary in index.ts.

A pathologically long or deeply nested selector can cause the Comet renderer to spend significant CPU time parsing — denial-of-service against the browser process. This PR caps selectors at 500 characters and enforces a CSS-grammar character allowlist.

Changes

• src/upload-validator.ts: New validateSelector(selector: string): string export — max 500 chars, CSS character allowlist, error does not echo raw value
• src/cdp-client.ts: Import validateSelector; call it in uploadFile() and uploadFiles() before DOM.querySelector
• src/index.ts: Import validateSelector; call it in comet_upload handler for defense-in-depth

Testing

• comet_upload with a valid selector (e.g. input[type="file"]) still works [type:manual]
• comet_upload with a 501-character selector returns an error without crashing [type:manual]
• comet_upload with a selector containing <script> characters is rejected [type:manual]
• comet_upload with selector omitted (auto-detect) still works [type:manual]
• npm run build passes [type:unit]

---

Closes #27
Implementation branch: fix/validate-selector-27
Base: main

[RapierCraft]

Security Review — PR #32

Reviewed commit: 5707217 | Agents: General Security

Automated Checks

Check	Result
TypeScript build (tsc)	✅ PASS
Prettier formatting	✅ PASS
Secrets detection	✅ PASS
New env vars	✅ None introduced

---

Finding 1: CONFIRMED — http-bridge.ts missing selector validation at handler boundary

File: src/http-bridge.ts:607

Code:

const result = await cometClient.uploadFile(resolvedPath, args.selector);

Evidence (full code path):

1. HTTP client → POST to bridge endpoint with {"tool": "comet_upload", "selector": "<501-char-string>"}
2. src/http-bridge.ts:965 → handleUpload(args) dispatched
3. src/http-bridge.ts:607 → cometClient.uploadFile(resolvedPath, args.selector) — args.selector is user-controlled, unvalidated at this layer
4. src/cdp-client.ts:1834-1848 → validateSelector(selector) IS called here — inner guard fires
5. Inner guard catches invalid selectors and returns early with error

Assessment: The cdp-client.ts inner guard (added in this PR) fully covers the http-bridge.ts path — the malformed selector is caught before reaching DOM.querySelector. The HTTP bridge endpoint is protected by the inner guard.

However, index.ts received defense-in-depth validation at the tool-handler boundary (the PR's own stated goal), but http-bridge.ts — which is an equally exposed HTTP endpoint for external clients (n8n, remote automation) — did not receive the same tool-handler-level defense-in-depth. The PR contract listed index.ts for defense-in-depth but http-bridge.ts was not in scope.

Verdict on severity: Since the inner guard fully covers the attack path, this is NOT a functional security gap. It is a missed defense-in-depth opportunity on a second exposed surface. The PR correctly addresses the CDP-layer vulnerability. The http-bridge.ts gap is advisory — it should be fixed but is not a blocking issue for this PR.

Confidence: CONFIRMED (code path traced) — the inner guard fires; the outer layer is just missing consistent hardening.

---

Finding 2: POSSIBLE — validateSelector allowlist permits raw newlines via \s

File: src/upload-validator.ts:220

Code: /^[A-Za-z0-9\s\.\#...]+$/

Evidence: \s matches \n, \r, \t. Raw newlines in a CSS selector passed to DOM.querySelector are handled safely by the browser engine (not code execution), but could produce unexpected behavior in error messages that echo the selector (existing code at cdp-client.ts:1625 echoes the selector in an error message: No element found matching selector: ${selector}). The DoS guard (500-char length cap) is the primary protection and is correctly in place. This is advisory only.

---

Positive Findings

• validateSelector() correctly rejects < (HTML injection) while allowing > (valid CSS combinator)
• Error messages in validateSelector() do NOT echo the raw selector — consistent with validateDomain() pattern
• Both uploadFile() and uploadFiles() are protected in cdp-client.ts
• Defense-in-depth guard in index.ts is correctly placed before the CDP call
• The 500-character length cap is the right primary DoS guard
• npm run build passes cleanly

---

Summary

The core fix is correct and sufficient. The inner guard in cdp-client.ts protects all callers including http-bridge.ts. The two findings are:

• SEC-1: Advisory — http-bridge.ts lacks tool-boundary defense-in-depth (inner guard covers it; no functional gap)
• SEC-2: Possible/Low — \s in allowlist allows raw newlines (DoS guard still in place)

Verdict: APPROVED. The PR addresses the stated issue. SEC-1 should be tracked as a follow-up finding issue.

[RapierCraft]

APPROVED: commit 5707217 after security review (1 agent: General Security). 2 findings created as issues (#33, #34) — both LOW severity, neither blocking.

Key points:

• Core fix is correct: validateSelector() in cdp-client.ts protects all callers including http-bridge.ts
• Defense-in-depth in index.ts is properly placed
• Build passes, formatting clean, no secrets
• Issue #33 (SEC-1, LOW): http-bridge.ts missing outer handler validation — inner guard covers it
• Issue #34 (SEC-2, LOW/POSSIBLE): \s in allowlist permits newlines — advisory only

Safe to merge.

[RapierCraft]

PR Review Summary: #32 - fix(security): validate selector parameter before DOM.querySelector

Review Integrity

Reviewed commit: 5707217 | Current HEAD: 5707217 | Status: CURRENT

Verdict: APPROVED ✅

Context-Aware Review

Domains: Security (Validation) | Agents: 1 (General Security)

Integration Checks (Phase 2.5)

Scope gap found: src/http-bridge.ts:607 — parallel HTTP endpoint lacks tool-boundary validateSelector() call. Inner guard in cdp-client.ts covers the attack path. Tracked as issue #33.
SOPS deploy chain: N/A
Purpose Regression Gate: N/A — non-milestone PR

Risk Matrix

Category	Risk	Blocking?	Confidence
Core selector DoS fix	LOW (fixed by PR)	No	CONFIRMED
http-bridge.ts outer guard	LOW (inner guard covers)	No	CONFIRMED
Allowlist newlines via \s	LOW (advisory)	No	POSSIBLE

Findings

Finding	Severity	Confidence	Issue
http-bridge.ts missing outer validateSelector	LOW	CONFIRMED	#33
\s in allowlist permits newlines	LOW	POSSIBLE	#34

Automated Checks

Check	Result
TypeScript build (tsc)	✅ PASS
Prettier formatting	✅ PASS
Secrets detection	✅ PASS
New env vars	✅ None

Recommendation

Merged. Follow-up: fix #33 (http-bridge.ts outer guard) in a separate PR to complete the defense-in-depth parity.

---

Security review complete. 1 agent + integration checks. 2 findings triaged. PR merged to main.