fix: quote shell interpolations in deploy and harden SSH#143
Closed
brianwtaylor wants to merge 9 commits intoNVIDIA:mainfrom
Closed
fix: quote shell interpolations in deploy and harden SSH#143brianwtaylor wants to merge 9 commits intoNVIDIA:mainfrom
brianwtaylor wants to merge 9 commits intoNVIDIA:mainfrom
Conversation
brianwtaylor
force-pushed
the
fix/deploy-shell-injection-hardening
branch
from
4269a51 to
2d883e1
Compare
5 tasks
brianwtaylor
force-pushed
the
fix/deploy-shell-injection-hardening
branch
3 times, most recently
from
daf0152 to
5ff147f
Compare
Extract deploy helpers (validateInstanceName, buildSshCommand, buildRsyncCommand) to prevent shell injection via instance names. Replace StrictHostKeyChecking=no with accept-new (TOFU) across all SSH and rsync commands in the deploy path. Signed-off-by: Brian Taylor <brian@briantaylor.xyz> Signed-off-by: Brian Taylor <brian.taylor818@gmail.com>
Add runArgv() and runCaptureArgv() to runner.js — shell-free alternatives that use spawnSync(prog, args) / execFileSync(prog, args) with no bash intermediary. Convert all SSH, scp, and rsync calls in deploy() to use argv arrays via runSsh(), runScp(), runRsync() helpers in deploy.js. This eliminates command injection at the root cause rather than escaping within the shell layer. Retains shellQuote() for brev CLI calls that require shell features. Adds 5 injection PoC tests proving argv arrays treat $(), backticks, semicolons, pipes, and && as literal text. Signed-off-by: Brian Taylor <brian.taylor818@gmail.com>
Signed-off-by: Brian Taylor <brian.taylor818@gmail.com>
brianwtaylor
force-pushed
the
fix/deploy-shell-injection-hardening
branch
from
4710a70 to
e8bff16
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds argv-based, shell-free runners and a new deploy helper that validate instance names, provide safe shell quoting, and build SSH/SCP/RSYNC argument arrays; integrates these into the main script and adds tests for argv safety, env sanitization, and SSH options. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Local as Local Script
participant Runner as runArgv / runCaptureArgv
participant Client as ssh/scp/rsync (client)
participant Remote as Remote Host
rect rgba(200,230,255,0.5)
Local->>Runner: build argv (["ssh", ...] / ["rsync", ...])
Runner->>Client: execFile/spawn (no shell, sanitized env)
Client->>Remote: connect / transfer / execute
Remote-->>Client: result / stdout
Client-->>Runner: exit status / stdout
Runner-->>Local: return result or exit on failure
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Around line 46-50: The spawnSync call currently spreads ...opts after setting
env so any opts.env passed by callers replaces the merged process.env; fix by
creating a spawn options object that ensures env is a merged object (const
mergedEnv = { ...process.env, ...opts.env }) and then pass that mergedEnv to
spawnSync while spreading opts without letting opts.env override it (e.g., build
spawnOpts = { ...opts, env: mergedEnv, stdio: "inherit", cwd: ROOT } and call
spawnSync(prog, args, spawnOpts)); update both the spawnSync usages (the block
around spawnSync(prog, args, { stdio: "inherit", cwd: ROOT, env: {
...process.env, ...opts.env }, ...opts }) and the similar one at lines 66-71) to
use this merged-env approach so process.env values like PATH/HOME are preserved.
- Around line 46-50: In runArgv(), the spawnSync call spreads opts which lets
callers override the intended no-shell behavior; ensure shell is locked to false
by explicitly setting shell: false in the options passed to spawnSync (or
validate and throw if opts.shell is true) so the "no bash, no injection" promise
holds—update the spawnSync invocation in runArgv() (the block constructing
options for spawnSync) to merge opts but override or reject any shell value.
In `@bin/nemoclaw.js`:
- Around line 129-133: The deploy runs use runSsh with the unsafe pattern "set
-a && . .env" which executes raw .env contents on the VM; change those runSsh
calls (the ones invoking "cd /home/ubuntu/nemoclaw && set -a && . .env && set +a
&& bash scripts/brev-setup.sh" and the similar start-services.sh invocation) to
load the .env without executing it by converting each line into an export with
the value single-quoted (i.e. produce/export lines like KEY='value' for each
non-comment line) before running the scripts so embedded shell metacharacters in
secrets are not executed.
- Line 125: runScp currently terminates the process on failure, so the temporary
secret file (envTmp) is never removed; update the call site or runScp so the
temp file is always deleted on error: either make runScp throw an error instead
of calling process.exit and wrap the call in a try/finally that unlinks envTmp,
or wrap the current runScp call in try/catch and in the catch block unlink
envTmp before rethrowing or exiting. Reference runScp and the envTmp variable to
locate where to add the try/finally or to change runScp's exit behavior so
cleanup always runs.
In `@test/deploy.test.js`:
- Around line 82-108: The tests in deploy.test.js currently call
spawnSync("echo", ...) instead of exercising our wrapper, so they don't validate
runArgv's non-shell behavior; update each security test that now uses spawnSync
to call runArgv(process.execPath, ["-e", ...], { stdio: "pipe", encoding:
"utf-8" }) so the wrapper is executed (use a short -e script that prints the
argument literally, e.g., console.log(process.argv[1]) or similar), replace the
echo-backed assertions to inspect runArgv's stdout for the literal injected
strings, and make the same replacement for the other test block mentioned (the
tests around the later range) so all shell-injection checks actually exercise
runArgv rather than spawnSync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d348f6f8-68f5-4566-b719-438062f2a510
📒 Files selected for processing (4)
bin/lib/deploy.jsbin/lib/runner.jsbin/nemoclaw.jstest/deploy.test.js
Comment on lines
+129
to
+133
| runSsh(name, "cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/brev-setup.sh", { tty: true }); | ||
|
|
||
| if (tgToken) { | ||
| console.log(" Starting services..."); | ||
| run(`ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR ${name} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/start-services.sh'`); | ||
| runSsh(name, "cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/start-services.sh"); |
There was a problem hiding this comment.
Quote the secret values before sourcing .env on the VM.
These commands still do set -a && . .env, which means the uploaded file is executed as shell. The file is built from raw token values above, so a credential containing $(), backticks, quotes, or ; will be parsed on the remote host and can also break deploys for perfectly valid secret values.
💡 Proposed fix
- const envLines = [`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`];
+ const envLines = [`NVIDIA_API_KEY=${shellQuote(process.env.NVIDIA_API_KEY)}`];
const ghToken = process.env.GITHUB_TOKEN;
- if (ghToken) envLines.push(`GITHUB_TOKEN=${ghToken}`);
+ if (ghToken) envLines.push(`GITHUB_TOKEN=${shellQuote(ghToken)}`);
const tgToken = getCredential("TELEGRAM_BOT_TOKEN");
- if (tgToken) envLines.push(`TELEGRAM_BOT_TOKEN=${tgToken}`);
+ if (tgToken) envLines.push(`TELEGRAM_BOT_TOKEN=${shellQuote(tgToken)}`);Also applies to: 139-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/nemoclaw.js` around lines 129 - 133, The deploy runs use runSsh with the
unsafe pattern "set -a && . .env" which executes raw .env contents on the VM;
change those runSsh calls (the ones invoking "cd /home/ubuntu/nemoclaw && set -a
&& . .env && set +a && bash scripts/brev-setup.sh" and the similar
start-services.sh invocation) to load the .env without executing it by
converting each line into an export with the value single-quoted (i.e.
produce/export lines like KEY='value' for each non-comment line) before running
the scripts so embedded shell metacharacters in secrets are not executed.
- Fix opts spread ordering in runArgv/runCaptureArgv so caller env merges with process.env instead of replacing it - Lock shell: false after opts spread to prevent callers from reopening the injection surface via shell: true - Quote secret values with shellQuote() in .env before sourcing on VM to prevent shell metacharacter interpretation in credentials - Wrap runScp in ignoreError + manual exit so temp secret file is always cleaned up even when scp fails - Rewrite injection PoC and security property tests to exercise runCaptureArgv wrapper instead of raw spawnSync - Add shell:true lock regression test
Strike team findings: - Block dangerous env vars (LD_PRELOAD, NODE_OPTIONS, BASH_ENV, GIT_SSH_COMMAND, etc.) from caller-supplied env via blocklist - Move cwd: ROOT after opts spread so callers cannot override it - Add typeof + length limit (253 chars) to validateInstanceName - Add tests for env blocklist, cwd lock, length limit, type check
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Around line 51-57: sanitizeEnv currently silently removes any keys present in
BLOCKED_ENV_VARS and thus allows callers to think they set restricted vars when
the process will instead inherit ambient process.env; change sanitizeEnv to
detect if callerEnv contains any blocked keys (check intersection between
Object.keys(callerEnv) and BLOCKED_ENV_VARS), and if any are found throw a
descriptive Error listing the blocked keys requested rather than silently
stripping them so callers cannot accidentally rely on ambient process.env; keep
returning the filtered env only when no blocked keys are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b02f34e-3ab5-42a5-806c-6c791ea19277
📒 Files selected for processing (3)
bin/lib/deploy.jsbin/lib/runner.jstest/deploy.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/deploy.test.js
- sanitizeEnv now throws with descriptive message listing the blocked keys, so callers cannot silently fall back to ambient process.env - Add JSDoc safety note to runSsh documenting that remoteCmd is executed by the remote shell and must use constant strings or shellQuote() for dynamic values
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/deploy.test.js (1)
94-119:⚠️ Potential issue | 🟠 MajorThese security suites still never execute
runArgv().Both blocks only exercise
runCaptureArgv(), so a regression in thespawnSyncpath used by deploy would still pass. Please driverunArgv()directly for at least one metacharacter case plus theshell: trueandcwdoverride checks.💡 Example adjustment
- it("$() subshell is literal, not expanded", () => { - const out = runCaptureArgv("echo", ["$(echo PWNED)"]); - assert.equal(out, "$(echo PWNED)"); + it("$() subshell is literal, not expanded", () => { + const r = runArgv( + process.execPath, + ["-e", "process.stdout.write(process.argv[1])", "$(echo PWNED)"], + { stdio: "pipe", encoding: "utf-8" } + ); + assert.equal(r.stdout, "$(echo PWNED)"); });Also applies to: 155-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/deploy.test.js` around lines 94 - 119, Tests currently only call runCaptureArgv(), so they miss the spawnSync path exercised by runArgv(); add test cases that invoke runArgv() directly: call runArgv("echo", ["$(echo PWNED)"]) (or another metacharacter case) and assert the literal output, and add separate tests that call runArgv with options { shell: true } to verify shell:true behavior and with a cwd override to verify working-directory handling; ensure you reference runArgv and spawnSync behavior in the new assertions so the deploy-related path is exercised alongside existing runCaptureArgv tests.bin/lib/deploy.js (1)
40-50:⚠️ Potential issue | 🟠 Major
runRsync()still isn't fully shell-free.
runArgv()only removes the local shell. Thehost:destoperand still goes through rsync's remote-shell transport, so an untrusteddestcan reopen injection here unless--protect-args/-sis enabled or this helper constrainsdestto safe constants.💡 Minimal hardening
function runRsync(sources, host, dest, opts = {}) { const args = [ - "-az", "--delete", + "-az", "--delete", "--protect-args", "--exclude", "node_modules", "--exclude", ".git", "--exclude", "src",Does rsync still let the remote shell parse the `path` part of a `host:path` destination unless `--protect-args` / `-s` is used?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/deploy.js` around lines 40 - 50, The runRsync helper builds an rsync remote operand using `${host}:${dest}` which still allows the remote shell to parse the path part; fix runRsync (and its use of runArgv) to either add the rsync --protect-args / -s flag to args to prevent remote-shell parsing or validate/sanitize the dest so it cannot contain shell-special characters or additional ':' segments; modify the function runRsync to append "--protect-args" (or "-s") to the args array and ensure SSH_OPTS and runArgv remain unchanged, or alternatively add strict validation logic for the dest parameter to only allow known-safe path patterns before constructing `${host}:${dest}`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Around line 42-58: The environment sanitizer currently allows callers to
override PATH, letting bare commands (ssh/scp/rsync) be resolved to
attacker-controlled binaries; update the BLOCKED_ENV_VARS Set to include "PATH"
so sanitizeEnv() will reject PATH overrides (ensure the thrown error message
still lists blocked keys), and add a regression test mirroring the existing
LD_PRELOAD/NODE_OPTIONS tests that calls sanitizeEnv (or invokes runArgv flow
that uses sanitizeEnv) with a PATH in the callerEnv and asserts an error is
thrown.
---
Duplicate comments:
In `@bin/lib/deploy.js`:
- Around line 40-50: The runRsync helper builds an rsync remote operand using
`${host}:${dest}` which still allows the remote shell to parse the path part;
fix runRsync (and its use of runArgv) to either add the rsync --protect-args /
-s flag to args to prevent remote-shell parsing or validate/sanitize the dest so
it cannot contain shell-special characters or additional ':' segments; modify
the function runRsync to append "--protect-args" (or "-s") to the args array and
ensure SSH_OPTS and runArgv remain unchanged, or alternatively add strict
validation logic for the dest parameter to only allow known-safe path patterns
before constructing `${host}:${dest}`.
In `@test/deploy.test.js`:
- Around line 94-119: Tests currently only call runCaptureArgv(), so they miss
the spawnSync path exercised by runArgv(); add test cases that invoke runArgv()
directly: call runArgv("echo", ["$(echo PWNED)"]) (or another metacharacter
case) and assert the literal output, and add separate tests that call runArgv
with options { shell: true } to verify shell:true behavior and with a cwd
override to verify working-directory handling; ensure you reference runArgv and
spawnSync behavior in the new assertions so the deploy-related path is exercised
alongside existing runCaptureArgv tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4ffb7dc-80db-4271-af9d-d573c212d24e
📒 Files selected for processing (3)
bin/lib/deploy.jsbin/lib/runner.jstest/deploy.test.js
Comment on lines
+42
to
+58
| const BLOCKED_ENV_VARS = new Set([ | ||
| "LD_PRELOAD", "LD_LIBRARY_PATH", "DYLD_INSERT_LIBRARIES", | ||
| "NODE_OPTIONS", "BASH_ENV", "ENV", | ||
| "GIT_SSH_COMMAND", "SSH_AUTH_SOCK", | ||
| "DOCKER_HOST", "KUBECONFIG", | ||
| "HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY", | ||
| "CURL_CA_BUNDLE", "SSL_CERT_FILE", "NODE_EXTRA_CA_CERTS", | ||
| ]); | ||
|
|
||
| function sanitizeEnv(callerEnv) { | ||
| if (!callerEnv) return {}; | ||
| const blocked = Object.keys(callerEnv).filter((k) => BLOCKED_ENV_VARS.has(k)); | ||
| if (blocked.length > 0) { | ||
| throw new Error(`runArgv() does not allow overriding: ${blocked.join(", ")}`); | ||
| } | ||
| return { ...callerEnv }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js child_process.spawnSyncandexecFileSync, when the command is a bare name like ssh, is the executable resolved using options.env.PATH?
💡 Result:
Yes.
For child_process.spawnSync() and child_process.execFileSync(), when you pass a bare executable name (e.g. "ssh"), Node resolves it using PATH from the environment it will pass to the child:
- If you provide
options.env, lookup usesoptions.env.PATH. - Otherwise, lookup uses
process.env.PATH. - If you provide
options.envbut omitPATH, then on Unix Node falls back to a default search path (typically/usr/bin:/binvia the OS exec semantics), while on Windows it uses the parent process’sPATH. (nodejs.org)
Citations:
- 1: https://nodejs.org/download/nightly/v26.0.0-nightly2026022776215dc993/docs/api/child_process.html
🏁 Script executed:
cd bin/lib && wc -l runner.jsRepository: NVIDIA/NemoClaw
Length of output: 72
🏁 Script executed:
cat -n bin/lib/runner.js | sed -n '42,95p'Repository: NVIDIA/NemoClaw
Length of output: 2202
🏁 Script executed:
grep -n "runArgv\|runCaptureArgv" bin/lib/deploy.js | head -20Repository: NVIDIA/NemoClaw
Length of output: 302
🏁 Script executed:
grep -n "ssh\|scp\|rsync" bin/lib/deploy.js | head -30Repository: NVIDIA/NemoClaw
Length of output: 300
🏁 Script executed:
cat -n bin/lib/deploy.js | head -60Repository: NVIDIA/NemoClaw
Length of output: 2394
🏁 Script executed:
fd -t f test || fd -t f "*.test.js" || fd -t f "*.spec.js" | head -20Repository: NVIDIA/NemoClaw
Length of output: 374
🏁 Script executed:
grep -r "PATH" bin/lib/deploy.js bin/lib/runner.jsRepository: NVIDIA/NemoClaw
Length of output: 136
🏁 Script executed:
cat -n test/deploy.test.js | head -100Repository: NVIDIA/NemoClaw
Length of output: 4213
🏁 Script executed:
grep -n "sanitizeEnv\|BLOCKED_ENV\|PATH" test/deploy.test.js test/cli.test.jsRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
grep -n "BLOCKED_ENV\|sanitizeEnv" test/deploy.test.js test/cli.test.jsRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
wc -l test/deploy.test.jsRepository: NVIDIA/NemoClaw
Length of output: 82
🏁 Script executed:
cat -n test/deploy.test.js | tail -80Repository: NVIDIA/NemoClaw
Length of output: 3392
Block PATH from caller overrides.
sanitizeEnv() does not block PATH, but bin/lib/deploy.js invokes helpers with bare command names (ssh, scp, rsync). Node.js resolves these using options.env.PATH, so a caller can redirect execution to attacker-controlled binaries. Add PATH to BLOCKED_ENV_VARS and include a regression test (similar to the existing LD_PRELOAD and NODE_OPTIONS checks).
Minimal fix
const BLOCKED_ENV_VARS = new Set([
+ "PATH",
"LD_PRELOAD", "LD_LIBRARY_PATH", "DYLD_INSERT_LIBRARIES",
"NODE_OPTIONS", "BASH_ENV", "ENV",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const BLOCKED_ENV_VARS = new Set([ | |
| "LD_PRELOAD", "LD_LIBRARY_PATH", "DYLD_INSERT_LIBRARIES", | |
| "NODE_OPTIONS", "BASH_ENV", "ENV", | |
| "GIT_SSH_COMMAND", "SSH_AUTH_SOCK", | |
| "DOCKER_HOST", "KUBECONFIG", | |
| "HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY", | |
| "CURL_CA_BUNDLE", "SSL_CERT_FILE", "NODE_EXTRA_CA_CERTS", | |
| ]); | |
| function sanitizeEnv(callerEnv) { | |
| if (!callerEnv) return {}; | |
| const blocked = Object.keys(callerEnv).filter((k) => BLOCKED_ENV_VARS.has(k)); | |
| if (blocked.length > 0) { | |
| throw new Error(`runArgv() does not allow overriding: ${blocked.join(", ")}`); | |
| } | |
| return { ...callerEnv }; | |
| } | |
| const BLOCKED_ENV_VARS = new Set([ | |
| "PATH", | |
| "LD_PRELOAD", "LD_LIBRARY_PATH", "DYLD_INSERT_LIBRARIES", | |
| "NODE_OPTIONS", "BASH_ENV", "ENV", | |
| "GIT_SSH_COMMAND", "SSH_AUTH_SOCK", | |
| "DOCKER_HOST", "KUBECONFIG", | |
| "HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY", | |
| "CURL_CA_BUNDLE", "SSL_CERT_FILE", "NODE_EXTRA_CA_CERTS", | |
| ]); | |
| function sanitizeEnv(callerEnv) { | |
| if (!callerEnv) return {}; | |
| const blocked = Object.keys(callerEnv).filter((k) => BLOCKED_ENV_VARS.has(k)); | |
| if (blocked.length > 0) { | |
| throw new Error(`runArgv() does not allow overriding: ${blocked.join(", ")}`); | |
| } | |
| return { ...callerEnv }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/runner.js` around lines 42 - 58, The environment sanitizer currently
allows callers to override PATH, letting bare commands (ssh/scp/rsync) be
resolved to attacker-controlled binaries; update the BLOCKED_ENV_VARS Set to
include "PATH" so sanitizeEnv() will reject PATH overrides (ensure the thrown
error message still lists blocked keys), and add a regression test mirroring the
existing LD_PRELOAD/NODE_OPTIONS tests that calls sanitizeEnv (or invokes
runArgv flow that uses sanitizeEnv) with a PATH in the callerEnv and asserts an
error is thrown.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/deploy.js (1)
58-69: Consider adding--protect-args(-s) to rsync for defense-in-depth.The
destparameter is interpolated into${host}:${dest}without escaping. While the local argv array prevents local shell injection, rsync by default allows the remote shell to interpret the destination path. Ifdestever contains shell metacharacters (spaces,$, backticks), they would be interpreted remotely.Adding
--protect-args(or-s) instructs rsync to quote arguments for the remote shell, providing an additional safety layer.🛡️ Proposed fix
function runRsync(sources, host, dest, opts = {}) { const args = [ - "-az", "--delete", + "-azs", "--delete", // -s = --protect-args "--exclude", "node_modules", "--exclude", ".git", "--exclude", "src",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/deploy.js` around lines 58 - 69, The runRsync function currently builds rsync args without protecting remote-shell interpretation of the destination; add the --protect-args (or -s) flag to the args array in runRsync so that `${host}:${dest}` is safe from remote shell metacharacters. Update the args constructed in runRsync (where "-az", "--delete", ... are listed) to include "--protect-args" (or "-s") alongside SSH options (SSH_OPTS) before spreading sources and `${host}:${dest}`, then keep returning runArgv("rsync", args, opts) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/deploy.js`:
- Around line 58-69: The runRsync function currently builds rsync args without
protecting remote-shell interpretation of the destination; add the
--protect-args (or -s) flag to the args array in runRsync so that
`${host}:${dest}` is safe from remote shell metacharacters. Update the args
constructed in runRsync (where "-az", "--delete", ... are listed) to include
"--protect-args" (or "-s") alongside SSH options (SSH_OPTS) before spreading
sources and `${host}:${dest}`, then keep returning runArgv("rsync", args, opts)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 09d5ba45-7613-4fd6-82d4-43e7826c83b3
📒 Files selected for processing (2)
bin/lib/deploy.jsbin/lib/runner.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/runner.js
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bin/lib/runner.js (1)
42-49:⚠️ Potential issue | 🟠 MajorAlso block
PATHoverrides here.Bare executables like
ssh,scp, andrsyncare still resolved through the child environment, so allowingopts.env.PATHlets a caller swap in a different binary and bypass the hardening this PR adds.💡 Minimal fix
const BLOCKED_ENV_VARS = new Set([ + "PATH", "LD_PRELOAD", "LD_LIBRARY_PATH", "DYLD_INSERT_LIBRARIES", "NODE_OPTIONS", "BASH_ENV", "ENV", "GIT_SSH_COMMAND", "SSH_AUTH_SOCK",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 42 - 49, Add PATH to the blocked environment variable set so callers cannot override executable resolution: update the BLOCKED_ENV_VARS Set to include the string "PATH" (the constant named BLOCKED_ENV_VARS in runner.js) and ensure any logic that filters opts.env continues to remove PATH; run/adjust related tests that assume PATH passthrough if present.
🧹 Nitpick comments (1)
bin/lib/deploy.js (1)
28-28: FreezeSSH_OPTSbefore exporting it.This array is shared mutable state today. Any importer can mutate it and silently weaken host-key checking for every later
runSsh()/runScp()/runRsync()call.💡 Minimal fix
-const SSH_OPTS = ["-o", "StrictHostKeyChecking=accept-new", "-o", "LogLevel=ERROR"]; +const SSH_OPTS = Object.freeze(["-o", "StrictHostKeyChecking=accept-new", "-o", "LogLevel=ERROR"]);Also applies to: 80-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/deploy.js` at line 28, SSH_OPTS is currently a shared mutable array which can be altered by importers and weaken host-key checking; make it immutable by freezing the exported options (e.g., replace the mutable array with an Object.freeze(...) value) so any importers cannot mutate SSH_OPTS, and ensure callers like runSsh, runScp, and runRsync use the frozen SSH_OPTS (or spread a copy when composing command args) to preserve immutability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/deploy.js`:
- Around line 32-37: Ensure the helpers validate hostnames internally: add a
call to validateInstanceName(host) at the start of runSsh and runRsync and throw
or return an error if validation fails so callers cannot skip it; for runScp,
refactor the API to accept destHost and destPath separately (or parse
destHostPath to extract and validate the host using validateInstanceName) before
building args with SSH_OPTS and calling runArgv, and keep the existing behavior
of honoring opts.tty and other options when constructing args.
In `@bin/lib/runner.js`:
- Around line 91-102: The runCaptureArgv function currently spreads opts into
execFileSync options allowing callers to override encoding or stdio which can
make execFileSync return a Buffer and break the .trim() call; change
runCaptureArgv to destructure encoding and stdio from opts (e.g. const { env,
encoding, stdio, ...execOpts } = opts), and if either encoding or stdio is
provided throw an Error (with a clear message) following the existing validation
pattern used for env/sanitizeEnv; then pass only the remaining execOpts plus the
hardcoded encoding: "utf-8" and stdio: ["pipe","pipe","pipe"] when calling
execFileSync so the call always returns a string safe to .trim().
---
Duplicate comments:
In `@bin/lib/runner.js`:
- Around line 42-49: Add PATH to the blocked environment variable set so callers
cannot override executable resolution: update the BLOCKED_ENV_VARS Set to
include the string "PATH" (the constant named BLOCKED_ENV_VARS in runner.js) and
ensure any logic that filters opts.env continues to remove PATH; run/adjust
related tests that assume PATH passthrough if present.
---
Nitpick comments:
In `@bin/lib/deploy.js`:
- Line 28: SSH_OPTS is currently a shared mutable array which can be altered by
importers and weaken host-key checking; make it immutable by freezing the
exported options (e.g., replace the mutable array with an Object.freeze(...)
value) so any importers cannot mutate SSH_OPTS, and ensure callers like runSsh,
runScp, and runRsync use the frozen SSH_OPTS (or spread a copy when composing
command args) to preserve immutability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 992e83a1-0958-4205-beb4-b5e8b6a8bc31
📒 Files selected for processing (2)
bin/lib/deploy.jsbin/lib/runner.js
- Block PATH from caller env overrides in BLOCKED_ENV_VARS (runner.js) - Add validateInstanceName() at entry of runSsh, runScp, runRsync (deploy.js) - Prevent encoding/stdio override in runCaptureArgv and ensure hardcoded values come after spread opts so they cannot be shadowed (runner.js) Signed-off-by: Brian Taylor <brian.taylor818@gmail.com>
Signed-off-by: Brian Taylor <brian.taylor818@gmail.com>
Contributor
Author
|
Closing — overlaps with merged #170 which already addresses shell quoting. Will rebase any remaining hardening into a follow-up if needed. |
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.
Summary
runArgv()andrunCaptureArgv()torunner.js— shell-free alternatives usingspawnSync(prog, args)/execFileSync(prog, args)with no bash intermediarydeploy()to argv arrays viarunSsh(),runScp(),runRsync()helpers — eliminates command injection at the root causevalidateInstanceName()with regex^[a-zA-Z0-9][a-zA-Z0-9._-]*$for defense-in-depth at the CLI entry pointStrictHostKeyChecking=nowithaccept-new(TOFU) across all SSH/scp/rsync in the deploy pathshellQuote()for brev CLI calls that require shell featuresWhy argv arrays?
The existing
run()passes commands tospawnSync("bash", ["-c", cmd]). Any user-controlled value interpolated into the command string enables OS command injection:Quoting within the shell (e.g.
shellQuote()) mitigates this but still relies on the shell to interpret the escaping correctly. Argv arrays bypass the shell entirely —spawnSync("ssh", ["-o", "StrictHostKeyChecking=accept-new", name, cmd])passes each argument directly to the kernel'sexecve(), making shell metacharacters impossible to interpret.SSH hardening:
StrictHostKeyChecking=no→accept-newno(old)accept-new(new)yes(strict)accept-newimplements Trust On First Use (ssh_config(5), OpenSSH 7.6+). Deploy targets are ephemeral Brev instances created moments before first SSH —accept-newis the standard for automated deployment tools (Ansible, Terraform, Packer).yeswould require pre-populatingknown_hostsbefore connecting.The
.envscp transfers API keys (NVIDIA_API_KEY,GITHUB_TOKEN,TELEGRAM_BOT_TOKEN) — MITM protection on repeat connections is critical.Test plan
Automated Tests
21 tests across 6 suites:
validateInstanceName(9 tests):my-box,prod.server,test_01foo;rm -rf /,$(whoami),`whoami`,foo|bar,foo barshellQuote(2 tests): Single-quote wrapping + embedded quote escapingSSH_OPTS(2 tests): Verifiesaccept-newpresent,noabsentargv injection proof-of-concept(5 tests):Proves all 5 shell injection methods are neutralized by argv arrays:
runCaptureArgv(3 tests): Stdout capture, error handling, literal$()passthroughSummary by CodeRabbit
New Features
Bug Fixes / Reliability
Tests