Skip to content

fix(http-bridge): add validateSelector guard in handleUpload - #36

Merged
RapierCraft merged 1 commit into
mainfrom
fix/http-bridge-selector-validation-33
Jun 22, 2026
Merged

fix(http-bridge): add validateSelector guard in handleUpload#36
RapierCraft merged 1 commit into
mainfrom
fix/http-bridge-selector-validation-33

Conversation

@RapierCraft

Copy link
Copy Markdown
Owner

Summary

Adds defense-in-depth selector validation at the HTTP bridge handler boundary in `handleUpload()`. This mirrors the guard added to `src/index.ts` in PR #32, which left the parallel HTTP entry point unprotected.

The inner guard in `cdp-client.ts` still fires as a fallback — this is a consistency fix, not a functional vulnerability patch.

Changes

  • `src/http-bridge.ts`: Add `validateSelector` to import from `upload-validator.js`
  • `src/http-bridge.ts`: Add conditional validation guard in `handleUpload()` before `cometClient.uploadFile()` call

Testing

  • POST /tools/comet_upload with valid selector: upload proceeds normally [type:api]
  • POST /tools/comet_upload without selector: auto-detect still works (undefined guard) [type:api]
  • POST /tools/comet_upload with malicious selector (e.g. <script>): returns structured error before reaching CDP [type:api]
  • npm run build passes [type:unit]

Closes #33
Implementation branch: fix/http-bridge-selector-validation-33
Base: main

Defense-in-depth: validate the selector parameter at the HTTP handler
boundary before it reaches cometClient.uploadFile(). Mirrors the guard
added to src/index.ts in PR #32 (fix/validate-selector-27).

- Import validateSelector from upload-validator.js
- Add conditional guard (only when selector !== undefined) with try/catch
- Return structured ToolResult error on invalid selector

The inner guard in cdp-client.ts still fires as a fallback. This change
ensures the HTTP bridge entry point is consistent with the MCP tool
handler pattern established in PR #32.
@RapierCraft

Copy link
Copy Markdown
Owner Author

Security & Code Quality Review — PR #36 (commit e1f1d72)

Agent: General Security (SEC)
Scope: src/http-bridge.ts — validateSelector import + handleUpload guard


Step 1: Diff Verification

Reviewed commit e1f1d72 (e1f1d72). Current HEAD matches pinned SHA — review is current.

Changes: 12 additions, 0 deletions, 1 file changed (src/http-bridge.ts).


Step 2: Security Scan

1. Injection check: CLEAR

  • validateSelector() uses a strict allowlist regex (ASCII letters, digits, CSS punctuation only). No user input is interpolated into any query, shell command, or template.
  • The guard throws on invalid input and returns { error: e.message } — the error message is a static string. User-supplied selector value is NOT echoed back. XSS-via-error-response risk: NONE.

2. Path traversal: NOT APPLICABLE — this PR touches selector validation, not file paths.

3. Error message safety: CONFIRMED SAFE

  • validateSelector() error messages are static — they do NOT reflect the invalid input value back to the caller. This correctly implements the project's established anti-XSS pattern.

4. Type correctness: CONFIRMED CORRECT

  • Return { success: false, content: "", error: e.message } fully satisfies ToolResult type ({ success: boolean; content: string | [...]; error?: string }).

5. Optional selector handling: CONFIRMED CORRECT

  • Guard fires only when args.selector !== undefined. When selector is absent (undefined), the guard is skipped and cometClient.uploadFile(resolvedPath, undefined) proceeds — auto-detect mode. No regression for the common case.
  • When selector is empty string: guard fires, validateSelector("") throws length error, returns structured error. Correct behavior.

6. Sibling handler sweep: CLEAR

  • Only handleUpload() calls cometClient.uploadFile() with a user-supplied selector. All other handlers (handleAsk, handleTabs, handleMode, etc.) do not pass user-supplied selectors to downstream methods. No other fix sites missed.

7. Defense-in-depth consistency: CONFIRMED CORRECT

  • Pattern mirrors src/index.ts:1132-1140 exactly: conditional on !== undefined, try/catch, error return shape.
  • Inner guard at src/cdp-client.ts:1839 still fires as fallback — layered defense is intact.

8. Import correctness: CONFIRMED CORRECT

9. Infrastructure posture checks: NOT APPLICABLE

  • No Dockerfile, docker-compose, or workflow files in diff.

Step 3: Code Quality Scan

Error handling: Correct — exception is caught, not swallowed; returns structured error.
Resource leaks: None — no connections, files, or async tasks opened in the new code.
Logic errors: None — guard condition, try/catch, and return are all correct.
Type safety: All types are explicit; e: any is the project's established catch pattern.
Scope creep: 12 additions match stated scope exactly — no unexplained additions.


Overall Assessment

This PR is a clean, minimal, correct implementation of the defense-in-depth pattern documented in issue #33. No findings at any confidence level. Build passes. Pattern is consistent with src/index.ts and src/cdp-client.ts.


@RapierCraft RapierCraft left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVED: commit e1f1d72 after context-aware security review (1 agent: General Security). 0 findings. Build passes. Safe to merge.

Defense-in-depth selector validation guard in handleUpload() is correct and consistent with the established pattern in src/index.ts. No blocking issues.

@RapierCraft
RapierCraft merged commit ed5a855 into main Jun 22, 2026
1 check passed
@RapierCraft

Copy link
Copy Markdown
Owner Author

PR Review Summary: #36 — fix(http-bridge): add validateSelector guard in handleUpload

Review Integrity

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

Verdict: APPROVED

Context-Aware Review

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

Integration Checks (Phase 2.5)

Import verification: PASS — validateSelector exported at upload-validator.ts:213
Type conformance: PASS — return shape satisfies ToolResult
Upstream call signature: PASS — uploadFile(filePath, selector?: string) accepts undefined
SOPS deploy chain: N/A — no env vars introduced
Purpose Regression Gate (7A): N/A — non-milestone PR

Risk Matrix

Category Risk Blocking? Confidence
XSS via error message LOW (error is static, not user echo) No CONFIRMED safe
Type mismatch LOW No CONFIRMED correct
Undefined selector regression LOW No CONFIRMED handled

Findings

None — 0 findings at any confidence level.

Automated Checks

Check Result
npm run build (tsc) PASS
Prettier formatting PASS (unchanged)
Secrets detection PASS (none found)
Env var audit PASS (no new vars)
Contract scope PASS (1 file, matches contract)

Recommendation

Merge approved. PR implements the defense-in-depth selector validation guard at the HTTP bridge handler boundary, consistent with the pattern established in src/index.ts by PR #32.


Context-aware review complete. 1 agent + integration checks. 0 findings triaged. PR #36 merged to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: http-bridge handleUpload missing selector validation at handler boundary (review finding — PR #32)

1 participant