fix(upload): allowlist filesystem paths and validate tabId

[sergei-aronsen]

Summary

`comet_upload` accepted any local path and forwarded it to `DOM.setFileInputFiles` against whatever page Comet was on. Combined with indirect prompt-injection from page content, this is an arbitrary-file exfiltration vector: a malicious page can instruct the agent to call `comet_upload {filePath: "/Users/x/.ssh/id_rsa"}` and the file lands in the next form submission.

Fix

Layered allowlist:

1. Opt-in `COMET_UPLOAD_ROOT` env: when set, resolve symlinks with `realpathSync` and require the canonical path to live under the configured root.
2. Hardcoded denylist (active when env unset, preserves existing permissive default): refuse uploads from `/.ssh`, `/.aws`, `/.config/{gh,gcloud}`, `/.gnupg`, `/.kube`, `/.docker`, `~/Library/Keychains`, `/etc/shadow`, `/etc/sudoers`, `/root`.
3. Warning log when env unset, so users notice they can tighten this.

Symlink resolution defeats `~/uploads/sneak -> ~/.ssh/id_rsa` style bypasses. `statSync` confirms it's a regular file.

Also tightened `comet_tabs action=switch|close tabId=...`: validate value matches `/^[A-Fa-f0-9-]{16,64}$/` before passing to `Target.connect` / `Target.closeTarget`. CDP target ids are 32-char hex; bogus values otherwise produce cryptic errors and can be coaxed into traversing the local CDP HTTP API surface (`/json/version`, …).

Removed dead `connectionCheckInterval` field on `CometCDPClient` — declared but never assigned/cleared.

Compatibility

Default behaviour unchanged for any caller not uploading from a sensitive system path. The new env var is opt-in.

Test plan

• `npm run test:unit` passes
• Manual: `COMET_UPLOAD_ROOT=/tmp comet_upload filePath=/tmp/foo.png` works
• Manual: `COMET_UPLOAD_ROOT=/tmp comet_upload filePath=/etc/passwd` rejected
• Manual: unset env + `comet_upload filePath=~/.ssh/id_rsa` rejected
• Manual: unset env + normal-path upload works with warning to stderr
• Adversarial: symlink `~/uploads/x -> ~/.ssh/id_rsa` rejected
• Manual: `comet_tabs action=switch tabId="bogus"` returns validation error, not CDP error

🤖 Generated with Claude Code

[RapierCraft]

Security Review: PR #14 — fix(upload): allowlist filesystem paths and validate tabId

Reviewed commit: 48a193c
Reviewer focus: Path traversal, SSRF, validation bypasses, defense-in-depth

---

1. Path Allowlist Implementation (validateUploadPath)

1.1 Strengths

• Symlink resolution via realpathSync — correctly defeats ~/uploads/sneak -> ~/.ssh/id_rsa symlink attacks.
• statSync check — ensures the target is a regular file (blocks directory traversal attempts that resolve to a dir).
• Two-tier design — strict allowlist when COMET_UPLOAD_ROOT is set, denylist when not. Reasonable backward-compat story.
• Path separator normalization — appends PATH_SEP before startsWith check, preventing prefix confusion (e.g., /tmp-evil matching root /tmp).

1.2 Issues Found

MEDIUM — TOCTOU Race Condition (File existence -> symlink resolution -> stat -> upload)

if (!existsSync(filePath)) { ... }
const real = realpathSync(filePath);
const stat = statSync(real);
// ... later ...
const result = await cometClient.uploadFile(resolvedPath, selector);

Between realpathSync/statSync and the actual uploadFile call (which invokes CDP DOM.setFileInputFiles), an attacker with local write access could:

1. Have a benign file pass validation
2. Replace it with a symlink to a sensitive file before CDP reads it

Practical risk: Low — requires local attacker with file write access in the same directory, AND precise timing. CDP likely re-resolves the path itself. But for defense-in-depth, consider opening an fd and passing the handle, or doing a final re-check just before upload.

Verdict: Acceptable for this PR; document as a known limitation.

LOW — Denylist is Incomplete (by design, but worth noting)

The denylist covers common credential stores but misses:

• ~/.netrc (HTTP basic auth credentials)
• ~/.npmrc (npm tokens)
• ~/.env or any .env files
• ~/.bash_history, ~/.zsh_history (command history with secrets)
• ~/.config/op/ (1Password CLI)
• ~/.local/share/keyrings/ (GNOME keyring)
• Windows paths (%USERPROFILE%\.ssh, etc.)

Since the denylist is explicitly a "better than nothing" fallback and the recommended path is COMET_UPLOAD_ROOT, this is acceptable. But the warning message should more strongly encourage setting the env var.

LOW — No File Size Limit

A malicious prompt could instruct the agent to upload a very large file (e.g., a database dump), causing DoS or data exfiltration of large datasets. Consider adding a configurable COMET_UPLOAD_MAX_SIZE check after statSync.

---

2. tabId Validation (validateTabId)

2.1 Strengths

• Regex ^[A-Fa-f0-9-]{16,64}$ is restrictive and correct for CDP target IDs (which are typically 32-char hex, sometimes with hyphens in UUID-like formats).
• Validates BEFORE passing to Target.connect / Target.closeTarget.
• Applied to both switch and close action paths.

2.2 Issues Found

LOW — Regex Allows All-Hyphen Strings

A string like -------------------- (20 hyphens) would pass validation. This is harmless — CDP will simply fail to find the target — but the regex could be tightened to require at least some hex chars: /^[A-Fa-f0-9]+(-[A-Fa-f0-9]+)*$/ with length 16-64.

Verdict: Current regex is acceptable. The worst case is a CDP "target not found" error, which is handled gracefully.

---

3. Bypass Risk Analysis

3.1 Can an attacker bypass COMET_UPLOAD_ROOT?

Attack Vector	Mitigated?	Notes
Symlink from inside root to outside	YES	realpathSync resolves before prefix check
../ traversal in filePath	YES	realpathSync canonicalizes
Null bytes in path	YES	Node.js fs APIs throw on null bytes since v18+
Very long paths	PARTIAL	OS-level PATH_MAX applies, no explicit check
Unicode normalization attacks	YES	realpathSync works on the filesystem's canonical form
Environment variable manipulation	N/A	If attacker can set COMET_UPLOAD_ROOT=/, they win — but that requires process-level access
Race condition (TOCTOU)	PARTIAL	See finding above

3.2 Can an attacker bypass tabId validation?

Attack Vector	Mitigated?	Notes
CDP HTTP API traversal (/json/version)	YES	Slashes not in charset
Injection of extra CDP params	YES	Only hex + hyphens allowed
Excessively long IDs	YES	Max 64 chars
Empty string	YES	Min 16 chars

---

4. Code Quality

• Dead code removal (connectionCheckInterval) — clean and correct.
• Error handling — both validation functions throw with clear messages, caught at call sites.
• The resolvedPath variable is correctly used for the actual upload call instead of the original filePath. This is the critical detail — using the validated canonical path prevents post-validation manipulation.

---

5. Verdict

APPROVE — This PR materially improves security posture against LLM-driven file exfiltration (the primary threat model). The implementation is sound:

• Symlink resolution is correctly applied before path comparison
• Path separator handling prevents prefix-confusion bypasses
• tabId validation blocks CDP HTTP API surface traversal
• Error paths are handled gracefully

The findings above are informational/low severity and do not block merge. The TOCTOU race is theoretical given the threat model (untrusted LLM prompts, not local attacker with fs access).

Recommendations for follow-up (non-blocking):

1. Consider adding ~/.netrc, ~/.npmrc, ~/.bash_history to the denylist
2. Consider a file size limit (COMET_UPLOAD_MAX_SIZE)
3. Tighten tabId regex to require at least one hex digit between hyphens

---

Security-focused review at commit 48a193c.

[RapierCraft]

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

Conflicting files: src/cdp-client.ts, src/index.ts

Resolution (trivial — keep both sides):

• cdp-client.ts: Keep HEAD's reconnectPromise/consecutiveSuccesses fields (replacing the old isReconnecting boolean)
• index.ts: Merge both import sets (readFileSync/fileURLToPath/dirname/join + existsSync/realpathSync/statSync/resolvePath/homedir) and keep both readPackageVersion() and validateUploadPath()/validateTabId()

Please rebase against main.