security(paste-image): harden against 7 findings from PR #84 review#90
Open
aakhter wants to merge 7 commits into
Open
security(paste-image): harden against 7 findings from PR #84 review#90aakhter wants to merge 7 commits into
aakhter wants to merge 7 commits into
Conversation
paste-${Date.now()}${ext} silently overwrites when two tabs paste in the
same millisecond. Append 8 hex chars so concurrent pastes get distinct names.
Addresses LOW finding from PR Ark0N#84 security review.
A process inside session.workingDir (the agent itself, or a malicious postinstall in node_modules) can plant `.claude-images -> ~/.ssh/`. The existing existsSync+mkdirSync follow symlinks, so subsequent paste-image writes would silently land outside workingDir. - lstat imageDir before write; refuse if symlink or not-a-directory. - mkdir without `recursive` so the leaf creation does not follow symlinks either (workingDir is guaranteed to exist for a live session). - O_EXCL|O_NOFOLLOW on the file open so the actual write step is symlink-safe and TOCTOU-safe against last-microsecond swaps. Addresses MED finding from PR Ark0N#84 security review.
The multipart filename and Content-Type are both attacker-controlled. The actual file bytes are not. A polyglot HTML/PNG passes the existing extension+MIME allowlist and would later serve back with image/png MIME, giving an attacker a stored-content vector if the file viewer ever sniffs/renders it. Add a small magic-number sniffer keyed off the chosen ext. Reject 415 on mismatch. Signatures cover PNG, JPEG, GIF, WebP, BMP — the full allowlist. Addresses MED finding from PR Ark0N#84 security review.
The session cookie is SameSite=lax, multipart/form-data is a 'simple' CORS request type (no preflight), and the route has no Origin/Referer check — so a cross-origin <form enctype="multipart/form-data" target="_blank"> submit attaches the session cookie and writes a file. Chains with the content-validation findings into a one-click compromise vector. Require Origin or Referer to match req.host. Non-browser clients (no Origin AND no Referer — curl, mobile native, scripts) must send X-Codeman-CSRF: any value. Cross-origin browser forms cannot add custom headers without a CORS preflight, which our CORS config does not allow from other origins, so the header acts as a same-origin proof. The frontend already uses same-origin fetch() which sends Origin automatically; no client change required. Addresses HIGH-2 finding from PR Ark0N#84 security review.
The hand-rolled multipart parser had several edge-case bugs: - `indexOf(boundaryBuf)` matches the literal boundary anywhere in the body (including inside data), so a crafted file with the boundary in its bytes truncates or splits parts unpredictably. - Hard-coded `\\r\\n\\r\\n` / `+2` / `-2` offsets silently corrupt LF-only clients on the last byte. - No per-request part count cap. Switch to @fastify/multipart with explicit limits: fileSize 10MB (same as before, plugin-enforced), files: 1 (paste-image always single), fields: 4 (small headroom). Use req.file() to read the single image part and part.toBuffer() to materialize bytes. Validates fieldname === 'image' (existing behavior), preserves the same extension/MIME allowlist and downstream magic-byte + symlink checks. Error mapping: file-too-large → 413, missing/empty → 400, wrong field → 400. Addresses MED finding from PR Ark0N#84 security review.
Two MED findings from the PR Ark0N#84 security review: 1. **Disk-fill DoS.** No rate limit on /api/sessions/:id/paste-image. An authenticated attacker could loop 10MB POSTs until the disk filled. Add a per-(IP, sessionId) token bucket — 30 requests/min — with 429 + Retry-After on exceed. Bucket map self-prunes entries idle > 1h to bound memory. 2. **No periodic cleanup.** Files in {workingDir}/.claude-images/ only get removed on killMux=true session deletion, so long-lived sessions accumulate them indefinitely. New paste-image-gc service walks live sessions hourly and deletes `paste-*` files older than 7 days. lstat (not stat) so a planted symlink can't escape the image dir. unref()'d so it doesn't block process exit; explicitly stopped in WebServer.stop(). Addresses two MED findings from PR Ark0N#84 security review.
The Ctrl+V interceptor in terminal-ui.js routes all paste events through image-input.js _handleImagePaste(). For non-image clipboard content, the text was being sent via raw sendInput(), which strips xterm.js's bracketed- paste markers (CSI 200~ ... CSI 201~). The inner CLI (Claude Code) enables bracketed-paste mode and relies on those markers to distinguish typed input from pasted input — and uses that distinction as part of its prompt-injection defenses. Stripping them makes pasted text indistinguishable from typed input. Switch to terminal.paste(text), which xterm.js wraps with markers when bracketed-paste mode is active. Addresses LOW finding from PR Ark0N#84 security review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardens
/api/sessions/:id/paste-image(merged in #84) against the seven findings flagged in the dismissed security review on that PR. Each commit addresses one finding so they can be cherry-picked individually.The one finding already fixed in-merge — dropping
.svgfromALLOWED_IMAGE_EXTS(HIGH stored-XSS) — is not duplicated here.Commits
2ff9d26paste-${Date.now()}-${rand4}${ext}instead ofpaste-${Date.now()}${ext}. Two tabs pasting in the same millisecond no longer silently last-write-wins.5725d4aimageDir—lstatbefore write, non-recursivemkdir,O_EXCL|O_NOFOLLOWon open. An agent or postinstall planting.claude-images -> ~/.ssh/no longer redirects writes outsideworkingDir.5bb7517Content-Type: image/png) now rejected 415.67e2d3eOrigin/Refererto matchreq.host. Non-browser clients (noOriginAND noReferer) must sendX-Codeman-CSRF. Cross-origin<form enctype="multipart/form-data">submits are now rejected 403. Frontend uses same-originfetch()(sendsOriginautomatically) — no client change required.0e0edbe@fastify/multipartwithlimits: { fileSize: 10MB, files: 1, fields: 4 }. Removes three latent bugs: boundary-as-literal-in-body matching,\r\nhard-coding that silently corrupts LF-only clients, no part-count cap.bc597e0paste-*files older than 7d. Defends against disk-fill DoS and accumulation in long-lived sessions. Newpaste-image-gc.tsservice, started/stopped fromWebServer.start/stop.41e6009terminal.paste(text)instead ofsendInput(text)for text-only paste inimage-input.js. Preserves bracketed-paste markers (CSI 200~ ... CSI 201~) when the inner CLI has bracketed-paste mode on — Claude Code uses them as part of its prompt-injection defenses.New dependency
@fastify/multipart@^10.0.0(sibling to the existing@fastify/cookie,@fastify/compress,@fastify/static,@fastify/websocket)Verification
npm test -- test/routes/on this branch: 51 failed / 254 passed — identical toorigin/masterbaseline (verified via worktree). Zero regressions from this branch.rejects invalid X/returns error for unknown sessionvalidation tests in unrelated route files (case-routes, ralph-routes, system-routes, …). Out of scope here.Threat-model summary
Most exposed in tunneled deployments where
CODEMAN_PASSWORDis set but the server is reachable from the broader internet:.claude-images/while the admin is logged in elsewherenode_modulespostinstall hook in a project Claude is working on can plant the symlink, then your next paste lands in~/.ssh/Per the dismissed review
The original reviewer's full list (HIGH-1 SVG XSS, HIGH-2 CSRF, four MEDs, two LOWs) was dismissed when #84 merged. HIGH-1 was fixed inline; this PR addresses the remaining six findings. Happy to split into multiple PRs if individual fixes are contentious — each commit applies cleanly on
origin/master.🤖 Generated with Claude Code