fix(security): tighten validateSelector allowlist — replace \s with [ \t] (#34) - #37
Conversation
… \t] (#34) \s in a JS character class matches \n, \r, \f, \v and all Unicode whitespace, none of which are valid in CSS selectors. Use explicit space and tab instead. This prevents selectors containing raw newlines or control characters from passing validation and being echoed in error messages (cdp-client.ts:1858, cdp-client.ts:1988), which could corrupt structured log output. Also updates the JSDoc to accurately describe the whitespace characters that are permitted (space and tab only).
Security Review — PR #37Agent: General Security (SEC) SHA VerificationReviewThis PR fixes a single regex in Files changed: Security Checks1. Injection / Input Validation: ✅ PASS
New regex Verified with Node.js execution: 2. Caller coverage: ✅ PASS
3. Build verification: ✅ PASS 4. Secrets / env vars: ✅ PASS — none detected. 5. No regression: ✅ PASS 6. Error message safety: ✅ PASS 7. JSDoc accuracy: ✅ PASS Additional Observation (Informational — Not a Finding)The VerdictAPPROVED — No security findings. The fix is minimal, correct, complete, and well-documented. All callers automatically benefit from the tightened allowlist. |
RapierCraft
left a comment
There was a problem hiding this comment.
APPROVED: commit fb18892 after context-aware security review (1 agent: General Security). 0 findings. Safe to merge.
Review summary: Single-file regex fix replacing \s with [ \t] in the validateSelector() allowlist. All 4 callers benefit automatically. TypeScript build passes. No regression on valid CSS selectors.
PR Review Summary: #37 — fix(security): tighten validateSelector allowlistReview IntegrityReviewed commit: Verdict: APPROVEDContext-Aware ReviewDomains: Security (input validation) | Agents: 1 (General Security) Integration Checks (Phase 2.5)Code registration: PASS — all 4 Risk Matrix
FindingsNo findings. 0 issues created. Automated Checks
RecommendationMERGED — PR #37 merged to Context-aware review complete. 1 agent (Security) + integration checks. 0 findings. PR merged. |
Summary
Fixes the
validateSelector()allowlist insrc/upload-validator.tswhere\swas used as whitespace shorthand.\sin a JS regex character class matches\n,\r,\f,\v, and all Unicode whitespace — only space and tab are valid in CSS selectors.This means a selector like
"div\nspan"previously passed validation and could then be echoed into error messages (e.g.No element found matching selector: div\nspan), splitting the message across lines.Changes
src/upload-validator.ts:224: replace\s→[ \t]in allowlist character classsrc/upload-validator.ts:199-200: JSDoc updated from "whitespace" to "space and tab (the only whitespace valid in CSS selectors)"src/upload-validator.ts:218-221: inline comment explaining the\s→[ \t]rationaleTesting
validateSelector('div span')→ passes (space still valid)validateSelector('div\tnested')→ passes (tab still valid)validateSelector("div\nspan")→ throws (newline now rejected)validateSelector("div\rspan")→ throws (CR now rejected)validateSelector("div\fspan")→ throws (form feed now rejected)validateSelector("div\vspan")→ throws (vertical tab now rejected).class #id [attr="val"] > div + p) → passnpm run build— TypeScript compiles cleanlyCloses #34
Implementation branch:
fix/validate-selector-allowlist-34Base:
main