-
Notifications
You must be signed in to change notification settings - Fork 2.1k
fix: quote shell interpolations in deploy and harden SSH #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ee7c13c
6788269
e8bff16
9ecbbe2
10c4f22
920537d
ac3746d
92711c5
d8c930e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
| // Deploy helpers — input validation and shell-free command builders. | ||
| // | ||
| // SSH/rsync/scp use runArgv() (argv arrays, no shell) to eliminate command | ||
| // injection at the root cause. shellQuote() is retained for call sites that | ||
| // still need run() (e.g. brev CLI with shell features). | ||
|
|
||
| const { runArgv } = require("./runner"); | ||
|
|
||
| const INSTANCE_NAME_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; | ||
|
|
||
| /** | ||
| * Validate that name is a safe instance/hostname string. | ||
| * @param {string} name - Instance name to validate. | ||
| * @throws {Error} If name is invalid, non-string, or too long. | ||
| */ | ||
| function validateInstanceName(name) { | ||
| if (!name || typeof name !== "string" || name.length > 253 || !INSTANCE_NAME_RE.test(name)) { | ||
| throw new Error( | ||
| `Invalid instance name: ${JSON.stringify(String(name).slice(0, 40))}. ` + | ||
| "Must be a string, 1-253 chars, start with alphanumeric, and contain only [a-zA-Z0-9._-]." | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| const SSH_OPTS = ["-o", "StrictHostKeyChecking=accept-new", "-o", "LogLevel=ERROR"]; | ||
|
|
||
| /** @param remoteCmd — executed by the remote shell. Use only constant strings | ||
| * or values wrapped in shellQuote(). Never interpolate unsanitized input. */ | ||
| function runSsh(host, remoteCmd, opts = {}) { | ||
| validateInstanceName(host); | ||
| const args = [...SSH_OPTS]; | ||
| if (opts.tty) args.unshift("-t"); | ||
| args.push(host); | ||
| if (remoteCmd) args.push(remoteCmd); | ||
| return runArgv("ssh", args, opts); | ||
| } | ||
|
|
||
| /** | ||
| * Copy a file to a remote host via scp using argv arrays (no shell). | ||
| * @param {string} src - Local source path. | ||
| * @param {string} destHostPath - Remote destination in host:path format. | ||
| * @param {object} [opts] - Options forwarded to runArgv. | ||
| */ | ||
| function runScp(src, destHostPath, opts = {}) { | ||
| const [host] = destHostPath.split(":"); | ||
| validateInstanceName(host); | ||
| const args = ["-q", ...SSH_OPTS, src, destHostPath]; | ||
| return runArgv("scp", args, opts); | ||
| } | ||
|
|
||
| /** | ||
| * Sync files to a remote host via rsync using argv arrays (no shell). | ||
| * @param {string[]} sources - Local paths to sync. | ||
| * @param {string} host - Remote hostname (must pass validateInstanceName). | ||
| * @param {string} dest - Remote destination directory. | ||
| * @param {object} [opts] - Options forwarded to runArgv. | ||
| */ | ||
| function runRsync(sources, host, dest, opts = {}) { | ||
| validateInstanceName(host); | ||
| const args = [ | ||
| "-az", "--delete", | ||
| "--exclude", "node_modules", | ||
| "--exclude", ".git", | ||
| "--exclude", "src", | ||
| "-e", "ssh " + SSH_OPTS.join(" "), | ||
| ...sources, | ||
| `${host}:${dest}`, | ||
| ]; | ||
| return runArgv("rsync", args, opts); | ||
| } | ||
|
|
||
| /** | ||
| * Wrap a string in POSIX single quotes, escaping embedded quotes. | ||
| * @param {string} s - Value to quote. | ||
| * @returns {string} Shell-safe single-quoted string. | ||
| */ | ||
| function shellQuote(s) { | ||
| return "'" + s.replace(/'/g, "'\\''") + "'"; | ||
| } | ||
|
|
||
| module.exports = { | ||
| INSTANCE_NAME_RE, | ||
| validateInstanceName, | ||
| runSsh, | ||
| runScp, | ||
| runRsync, | ||
| shellQuote, | ||
| SSH_OPTS, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,7 @@ if (!process.env.DOCKER_HOST) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** Execute a shell command via `bash -c`; exits the process on failure unless opts.ignoreError is set. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function run(cmd, opts = {}) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = spawnSync("bash", ["-c", cmd], { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stdio: "inherit", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -37,6 +38,80 @@ function run(cmd, opts = {}) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Env vars that must never be overridden by callers — they enable code | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // execution, library injection, or trust-store hijacking in subprocesses. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Validate caller-supplied env vars against a blocklist of dangerous keys. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Throws if any blocked key is present; returns a shallow copy otherwise. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {object} [callerEnv] - Env overrides from the caller. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @returns {object} Sanitized env entries safe to merge with process.env. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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 }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+66
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Yes. For
Citations:
🏁 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
Minimal fix const BLOCKED_ENV_VARS = new Set([
+ "PATH",
"LD_PRELOAD", "LD_LIBRARY_PATH", "DYLD_INSERT_LIBRARIES",
"NODE_OPTIONS", "BASH_ENV", "ENV",📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Shell-free alternative to run(). Executes prog with an argv array via | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * spawnSync(prog, args) — no bash, no string interpolation, no injection. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Use this for any command that includes user-controlled values. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function runArgv(prog, args, opts = {}) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { env, ...spawnOpts } = opts; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = spawnSync(prog, args, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stdio: "inherit", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...spawnOpts, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cwd: ROOT, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env: { ...process.env, ...sanitizeEnv(env) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| shell: false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (result.status !== 0 && !opts.ignoreError) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.error(` Command failed (exit ${result.status}): ${prog} ${args.join(" ").slice(0, 60)}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| process.exit(result.status || 1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Shell-free alternative to runCapture(). Uses execFileSync(prog, args) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * with no shell. Returns trimmed stdout. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function runCaptureArgv(prog, args, opts = {}) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { env, encoding, stdio, ...execOpts } = opts; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (encoding !== undefined || stdio !== undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error("runCaptureArgv() does not allow overriding encoding or stdio"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const { execFileSync } = require("child_process"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return execFileSync(prog, args, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ...execOpts, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cwd: ROOT, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| env: { ...process.env, ...sanitizeEnv(env) }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| encoding: "utf-8", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| stdio: ["pipe", "pipe", "pipe"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| shell: false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }).trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (err) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (opts.ignoreError) return ""; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw err; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** Execute a shell command and return its trimmed stdout; returns "" on failure if opts.ignoreError is set. */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function runCapture(cmd, opts = {}) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return execSync(cmd, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -52,4 +127,4 @@ function runCapture(cmd, opts = {}) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| module.exports = { ROOT, SCRIPTS, run, runCapture }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| module.exports = { ROOT, SCRIPTS, run, runCapture, runArgv, runCaptureArgv }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.