fix(security): address upload path validation vulnerabilities from PR #14 review

[RapierCraft]

Summary

Addresses all 6 security vulnerabilities identified in the PR #14 review. Validators are extracted into a shared module, the HTTP bridge is brought under source control with fixes applied.

Changes

• src/upload-validator.ts (new): Shared validateUploadPath() and validateTabId() — single source of truth for all callers.

  • Removes TOCTOU-prone existsSync pre-check; realpathSync now resolves atomically
  • Expands denylist: ~/.netrc, ~/.npmrc, ~/.git-credentials, ~/.pypirc, /proc/self/environ, shell histories, .env files anywhere on filesystem, /etc/passwd
  • Rejects COMET_UPLOAD_ROOT="/" and COMET_UPLOAD_ROOT=homedir() (before and after symlink resolution)
  • validateTabId tightened to strict UUID v4 regex

• src/index.ts (modified): Imports validators from shared module; removes inline definitions and unused imports

• src/cdp-client.ts (modified): Calls validateUploadPath() at entry of uploadFile() and uploadFiles() — defense-in-depth at the CDP layer

• src/http-bridge.ts (new — source for dist/http-bridge.js):

  • crypto.timingSafeEqual replaces timing-unsafe === for token comparison
  • CORS: no wildcard *; configurable via COMET_BRIDGE_CORS_ORIGIN env var
  • validateTabId() called in handleTabs() switch/close actions
  • comet_upload handler added with validateUploadPath() enforcement

Testing

• Upload ~/.netrc — expect rejection [type:manual]
• Upload with COMET_UPLOAD_ROOT="/" — expect "overly broad" error [type:manual]
• Symlink outside root when COMET_UPLOAD_ROOT is set — expect rejection [type:manual]
• HTTP bridge GET /tools includes comet_upload [type:api]
• HTTP bridge malformed tabId returns validation error [type:api]
• CORS headers absent when COMET_BRIDGE_CORS_ORIGIN unset [type:api]

---

Closes #24
Implementation branch: fix/upload-path-security-24
Base: main

[RapierCraft]

Security Review — PR #28 (commit 91342d9)

Agent: Security Domain
Domains covered: Security, Auth, Path Validation, CORS, Token Auth
Files reviewed: src/upload-validator.ts, src/http-bridge.ts, src/cdp-client.ts, src/index.ts

---

Automated Checks

Check	Result
TypeScript build (tsc)	PASS — no errors
New env vars in docs	ADVISORY — COMET_BRIDGE_CORS_ORIGIN not in README (see below)
Secrets detection	PASS — no secrets in diff
Package.json changes	N/A — no dependency changes

---

Positive Findings

All 6 original PR #14 review vulnerabilities are correctly addressed:

1. TOCTOU eliminated: existsSync removed from validateUploadPath; realpathSync wraps correctly with ENOENT catch.
2. Denylist expanded: Added ~/.netrc, ~/.npmrc, ~/.git-credentials, ~/.pypirc, /proc/self/environ, shell histories, .env files anywhere on filesystem, /etc/passwd.
3. Root rejection: COMET_UPLOAD_ROOT="/" and COMET_UPLOAD_ROOT=homedir() rejected both before and after symlink resolution. Robust.
4. UUID v4 regex: validateTabId correctly uses /^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{4}-[0-9A-Fa-f]{12}$/. Chrome CDP target IDs are UUID v4 — this is the correct format.
5. CDP-layer validation: uploadFile() and uploadFiles() in cdp-client.ts both call validateUploadPath at entry. Double-validation when called via index.ts is harmless (second call on already-resolved path is a no-op).
6. HTTP bridge security: timingSafeEqual with equal-length buffer guard, no wildcard CORS, validateTabId in both switch and close actions, comet_upload handler with path validation.

---

Findings

Finding 1 — POSSIBLE / MEDIUM
File: src/upload-validator.ts line ~148
Pattern: incomplete-denylist-coverage

The denylist adds /proc/self/environ (correct), but does not block the broader /proc/self/ prefix. An attacker could target /proc/self/maps (reveals memory layout), /proc/self/fd/3 (open file descriptors that may reference sensitive files), or /proc/1/environ (init process environment). These are regular files readable under the PR threat model.

Recommendation: Replace the single /proc/self/environ entry with /proc as a prefix (blocks all of /proc subtree). This is a narrow follow-up fix — the current PR already addresses the highest-risk entry.

Confidence: POSSIBLE (threat requires a specific attack scenario; the most obvious vector /proc/self/environ is already blocked)
Severity: MEDIUM

---

Finding 2 — POSSIBLE / LOW
File: README.md
Pattern: missing-env-var-documentation

COMET_BRIDGE_CORS_ORIGIN is introduced in src/http-bridge.ts with clear inline documentation but the top-level README has no HTTP bridge section at all (COMET_BRIDGE_TOKEN, COMET_BRIDGE_PORT, COMET_BRIDGE_HOST are also undocumented there). The existing HTTP bridge section in dist/http-bridge.js header covers this via JSDoc comments, so the gap is informational rather than a functional issue.

Recommendation: Add a brief HTTP Bridge section to README. Non-blocking — can be a follow-up issue.

Confidence: POSSIBLE
Severity: LOW

---

Verdict

APPROVED — All 6 original blocking vulnerabilities are correctly fixed. Both findings above are advisory (POSSIBLE confidence, MEDIUM/LOW severity) and do not constitute blocking issues for this security-fix PR. The /proc coverage gap is a follow-up improvement, not a regression.

[RapierCraft]

APPROVED: commit 91342d9 after security-domain review (1 agent). 2 findings created as issues — both POSSIBLE confidence, non-blocking. PR correctly addresses all 6 original vulnerabilities from PR #14 review. Safe to merge.

Review findings:

• Issue #29: expand /proc denylist to block entire subtree (MEDIUM, follow-up)
• README documentation gap for COMET_BRIDGE_CORS_ORIGIN (LOW, informational)

[RapierCraft]

PR Review Summary: #28 — fix(security): address upload path validation vulnerabilities from PR #14 review

Review Integrity

Reviewed commit: 91342d9 | Current HEAD: 91342d9 | Status: CURRENT

Verdict: APPROVED

Context-Aware Review

Domains: Security, Path Validation, Token Auth, CORS
Agents: 1 (Security Domain)

Integration Checks (Phase 2.5)

Code registration: PASS — all new modules compile and import correctly
Build verification: PASS — tsc clean, no errors
Secrets detection: PASS — no secrets in diff
Purpose Regression Gate (7A): N/A — non-milestone PR

Risk Matrix

Category	Risk	Blocking?	Confidence
/proc subtree coverage gap	MEDIUM	No (POSSIBLE confidence)	POSSIBLE
COMET_BRIDGE_CORS_ORIGIN undocumented	LOW	No	POSSIBLE

Findings

Finding	Severity	Confidence	Issue
/proc subtree not fully blocked	MEDIUM	POSSIBLE	#29
COMET_BRIDGE_CORS_ORIGIN not in README	LOW	POSSIBLE	follow-up

Automated Checks

Check	Result
TypeScript build	PASS
Secrets detection	PASS
Env var audit	ADVISORY (README gap only)
Package.json changes	N/A

Recommendation

All 6 original PR #14 vulnerabilities are correctly fixed. The two review findings are low-priority follow-ups that do not block this security fix PR. Merged to main.

---

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