fix(security): harden shell execution, CORS, and path validation#135
Conversation
- Replace execAsync with spawnAsync in user.js git config (shell injection) - Sanitize pattern args in openrouter.js and cli-chat.js tool handlers - Add source validation to news logs endpoint (path traversal) - Restrict CORS to configured origins (was open to all)
- Grep handlers: use -e and -- to prevent argv option injection from patterns that start with dashes (e.g. --files) - spawnAsync: add maxBuffer (1MB) with SIGTERM on overflow to prevent unbounded memory usage from Glob/Find on large trees - CORS: filter empty strings from origin allowlist to handle trailing commas or whitespace-only entries in env var
Both stdout and stderr are now limited to maxBuffer (1MB default). Previously only stdout was capped, leaving stderr as an unbounded memory growth vector on commands producing large error output.
Zhang-Henry
left a comment
There was a problem hiding this comment.
Thanks for the security fixes — the shell injection and CORS issues are real and the spawnAsync approach is correct. Found a few bugs that should be addressed before merging.
| cwd: dir, timeout: 30_000, | ||
| }); | ||
| const lines = stdout.split('\n').filter(Boolean).slice(0, 300); | ||
| return trunc(lines.join('\n') || '(no matches)'); |
There was a problem hiding this comment.
Bug: Glob silently returns "(no matches)" on large repos — regression from removing | head -300
The old code had | head -300 in the shell pipeline, which limited rg output to ~300 lines via SIGPIPE before it hit the buffer. The new code removes that limit — rg --files dumps ALL filenames into memory, and only after full collection does JS .slice(0, 300).
For a repo with ~50k files (~2-3MB of path output), this exceeds the default 1MB maxBuffer in spawnAsync, which triggers:
child.kill('SIGTERM')→ non-zero exit → Promise rejects- Outer
catch→ falls through tofindfallback → same overflow → rejects - Inner
catch→return '(no matches)'
Result: Glob appears to work but silently returns empty on any moderately large codebase.
Same issue in cli-chat.js Glob handler.
Suggested fix: Make spawnAsync resolve (not reject) with truncated stdout when maxBuffer is exceeded, so the caller still gets the first 1MB of data. Alternatively, limit output at the process level (e.g., pipe through a line counter, or increase maxBuffer for Glob specifically).
| app.use(cors({ | ||
| origin: process.env.DR_CLAW_CORS_ORIGINS | ||
| ? process.env.DR_CLAW_CORS_ORIGINS.split(',').map((o) => o.trim()).filter(Boolean) | ||
| : [`http://localhost:${getFrontendPortSync()}`, `http://127.0.0.1:${getFrontendPortSync()}`], |
There was a problem hiding this comment.
Bug: getFrontendPortSync() called without argument — CORS may block legitimate requests when VITE_PORT is customized
Every other call site in this file passes REQUESTED_VITE_PORT as the fallback:
getFrontendPortSync(REQUESTED_VITE_PORT) // L2898, L3129, L3139Here it's called bare, so the fallback is the hardcoded DEFAULT_FRONTEND_PORT. If a user sets VITE_PORT=3002, the frontend runs on 3002 but CORS only allows the default port (e.g. 5173). Since the origin array is computed once at middleware setup, it never updates dynamically.
Note: REQUESTED_VITE_PORT is defined at L3105, after this middleware setup runs — so you'll also need to hoist the port variable or move the CORS config later in the file.
|
|
||
| const execAsync = promisify(exec); | ||
|
|
||
| function spawnAsync(command, args, options = {}) { |
There was a problem hiding this comment.
Code quality: spawnAsync is copy-pasted 3 times (here, cli-chat.js, and user.js)
The user.js copy already diverges — it lacks maxBuffer / truncation support. These will drift further apart over time.
Please extract to a shared module (e.g. server/utils/spawnAsync.js) and import it from all three files.
Address all three review comments from @Zhang-Henry: 1. Glob maxBuffer regression — spawnAsync was rejecting when stdout exceeded maxBuffer (child killed → non-zero exit → reject), causing Glob to silently return "(no matches)" on large repos. The shared spawnAsync now resolves with truncated data when the child is killed by our own SIGTERM, while genuine non-zero exits still reject. Partial-line output is trimmed at the last newline to avoid phantom filenames. 2. CORS port bug — getFrontendPortSync() was called without the VITE_PORT fallback because REQUESTED_VITE_PORT was defined after the CORS middleware setup. Now computes CORS_VITE_PORT early and passes it to getFrontendPortSync(). 3. spawnAsync deduplication — extracted the copy-pasted function from openrouter.js, cli-chat.js, and user.js into a shared module at server/utils/spawnAsync.js. The user.js copy now also gets maxBuffer/truncation support. Assisted-by: Claude Code
|
Thanks @Zhang-Henry — all three issues addressed in fca7ba5: 1. Glob maxBuffer regression
2. CORS port bug
3. spawnAsync deduplication
|
Summary
Four security fixes addressing shell injection, path traversal, and CORS misconfiguration across 5 files.
Changes
1. Shell injection in
server/routes/user.js(Critical)Before: Git user name/email interpolated into shell strings via
execAsync:A name like
`whoami`or$(rm -rf /)bypasses the double-quote escaping.After: Uses
spawnAsyncwithshell: falseand argument arrays — no shell parsing, no injection vector. Same pattern as our merged PR #88 forserver/routes/git.js.2. Shell injection in
server/openrouter.js+server/cli-chat.js(Critical)Before: LLM-driven tool calls (Glob, Grep) passed
args.patterndirectly into shell commands:`rg --files --glob '${args.pattern}'`After:
spawnAsyncwith argument arrays (shell: false)-e pattern -- targetto prevent argv option injectionspawnAsyncenforces 1MBmaxBufferwith SIGTERM to prevent DoS from large tree output3. Path traversal in
server/routes/news.js(Medium)Before:
/api/news/logs/:sourceusedreq.params.sourceinpath.joinwithout validation.After: Added
getSourceEntry()check before path construction, matching the validation pattern used by all other:sourceendpoints in the same file. Returns 404 for unknown sources.4. CORS misconfiguration in
server/index.js(Medium)Before:
cors()with no restrictions — any origin can make credentialed requests tolocalhost:3001.After: CORS restricted to
DR_CLAW_CORS_ORIGINSenv var (comma-separated), defaulting tolocalhostand127.0.0.1on the frontend port. Empty entries are filtered.Testing
user.jsgit config still works with normal names