From a1b2e9623f51394ff69a482508528f77c5d08c5d Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Fri, 20 Mar 2026 18:09:26 -0700 Subject: [PATCH 1/2] fix: validate and quote instance name in deploy shell commands Extends the RFC 1123 validation and double-quoting pattern from #170 (sandbox names) to the deploy() function. All 7 unquoted ${name} interpolations in ssh/scp/rsync commands are now quoted, and instance names are validated at entry to prevent command injection. --- bin/nemoclaw.js | 38 ++++++++---- test/deploy-instance-name.test.js | 97 +++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 12 deletions(-) create mode 100644 test/deploy-instance-name.test.js diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index 1d108632b..7a08958c5 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -105,7 +105,17 @@ async function deploy(instanceName) { if (isRepoPrivate("NVIDIA/OpenShell")) { await ensureGithubToken(); } - const name = instanceName; + const name = instanceName.trim(); + + // Validate: same RFC 1123 subdomain rules used for sandbox names — lowercase + // alphanumeric and hyphens, must start and end with alphanumeric. + if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(name) || name.length > 253) { + console.error(` Invalid instance name: '${name}'`); + console.error(" Names must be lowercase, contain only letters, numbers, and hyphens,"); + console.error(" and must start and end with a letter or number (max 253 chars)."); + process.exit(1); + } + const gpu = process.env.NEMOCLAW_GPU || "a2-highgpu-1g:nvidia-tesla-a100:1"; console.log(""); @@ -127,7 +137,7 @@ async function deploy(instanceName) { if (!exists) { console.log(` Creating Brev instance '${name}' (${gpu})...`); - run(`brev create ${name} --gpu "${gpu}"`); + run(`brev create "${name}" --gpu "${gpu}"`); } else { console.log(` Brev instance '${name}' already exists.`); } @@ -137,7 +147,7 @@ async function deploy(instanceName) { 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=no "${name}" 'echo ok' 2>/dev/null`, { encoding: "utf-8", stdio: "pipe" }); break; } catch { if (i === 59) { @@ -149,31 +159,35 @@ 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(`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/`); const envLines = [`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`]; const ghToken = process.env.GITHUB_TOKEN; if (ghToken) envLines.push(`GITHUB_TOKEN=${ghToken}`); const tgToken = getCredential("TELEGRAM_BOT_TOKEN"); 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`); - fs.unlinkSync(envTmp); + const envDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-env-")); + const envTmp = path.join(envDir, ".env"); + try { + fs.writeFileSync(envTmp, envLines.join("\n") + "\n", { mode: 0o600, flag: "wx" }); + run(`scp -q -o StrictHostKeyChecking=no -o LogLevel=ERROR "${envTmp}" "${name}":/home/ubuntu/nemoclaw/.env`); + } finally { + fs.rmSync(envDir, { recursive: true, force: true }); + } console.log(" Running setup..."); - runInteractive(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR ${name} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/brev-setup.sh'`); + runInteractive(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR "${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(`ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR "${name}" 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/start-services.sh'`); } console.log(""); console.log(" Connecting to sandbox..."); console.log(""); - runInteractive(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR ${name} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw'`); + runInteractive(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR "${name}" 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw'`); } async function start() { diff --git a/test/deploy-instance-name.test.js b/test/deploy-instance-name.test.js new file mode 100644 index 000000000..2171ee2a3 --- /dev/null +++ b/test/deploy-instance-name.test.js @@ -0,0 +1,97 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Verify that deploy() validates and quotes the instance name in shell +// commands to prevent command injection. + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const path = require("node:path"); + +const ROOT = path.resolve(__dirname, ".."); +const source = fs.readFileSync(path.join(ROOT, "bin/nemoclaw.js"), "utf-8"); + +// Extract the full deploy() function body using brace-depth counting +// so we don't stop at the first inner closing brace. +function extractDeploy(src) { + const start = src.indexOf("async function deploy("); + if (start === -1) return null; + const open = src.indexOf("{", start); + if (open === -1) return null; + + let depth = 0; + let end = -1; + for (let i = open; i < src.length; i++) { + if (src[i] === "{") depth++; + if (src[i] === "}") depth--; + if (depth === 0) { + end = i + 1; + break; + } + } + if (end === -1) return null; + return src.slice(start, end); +} + +const deployBody = extractDeploy(source); + +describe("deploy instance name hardening", () => { + it("can extract deploy() function body", () => { + assert.ok(deployBody, "Could not find deploy() function"); + }); + + it("enforces RFC 1123 validation inside deploy()", () => { + assert.match( + deployBody, + /\^\[a-z0-9\]/, + "deploy() must validate the instance name against RFC 1123 subdomain rules" + ); + }); + + it("rejects names longer than 253 characters inside deploy()", () => { + assert.match( + deployBody, + /name\.length\s*>\s*253/, + "deploy() must enforce a 253-character limit on instance names" + ); + }); + + it("does not interpolate unquoted instance name in shell commands", () => { + // Every ${name} in a shell command string must be exactly "${name}". + // Skip console.log/error lines which are display-only. + const lines = deployBody.split("\n"); + for (const line of lines) { + if (line.includes("${name}") && !line.match(/console\.(log|error)/)) { + const all = [...line.matchAll(/\$\{name\}/g)].length; + const quoted = [...line.matchAll(/"\$\{name\}"/g)].length; + assert.equal( + quoted, + all, + `Unquoted \${name} in shell command: ${line.trim()}` + ); + } + } + }); + + it("quotes instance name in brev create", () => { + assert.ok( + deployBody.includes('brev create "${name}"'), + 'brev create must quote the instance name: brev create "${name}"' + ); + }); + + it("quotes instance name in rsync destination", () => { + assert.ok( + deployBody.includes('"${name}":/home/ubuntu/nemoclaw/'), + 'rsync destination must quote the instance name: "${name}":/path' + ); + }); + + it("quotes instance name in scp destination", () => { + assert.ok( + deployBody.includes('"${name}":/home/ubuntu/nemoclaw/.env'), + 'scp destination must quote the instance name: "${name}":/path' + ); + }); +}); From 6fb00dec0feda33cc0e2ee5874b956250115bfb8 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Sat, 21 Mar 2026 09:02:03 -0700 Subject: [PATCH 2/2] fix: align deploy hardening with centralized shellQuote pattern Align with ericksoa's #584 approach: - Centralize shellQuote in runner.js, remove local copy from nemoclaw.js - Replace double-quoting with shellQuote() single-quote wrapping - Switch execSync to execFileSync in deploy() - Quote env values (API keys, tokens) with shellQuote - Max 63 chars (RFC 1123 label) instead of 253 (subdomain) - Update tests to verify shellQuote patterns --- bin/lib/runner.js | 10 +++++- bin/nemoclaw.js | 55 ++++++++++++++++--------------- test/deploy-instance-name.test.js | 45 ++++++++----------------- 3 files changed, 51 insertions(+), 59 deletions(-) diff --git a/bin/lib/runner.js b/bin/lib/runner.js index 53ec88996..97742a281 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -58,4 +58,12 @@ function runCapture(cmd, opts = {}) { } } -module.exports = { ROOT, SCRIPTS, run, runCapture, runInteractive }; +/** + * Shell-quote a value for safe interpolation into bash -c strings. + * Wraps in single quotes and escapes embedded single quotes. + */ +function shellQuote(value) { + return `'${String(value).replace(/'/g, `'\\''`)}'`; +} + +module.exports = { ROOT, SCRIPTS, run, runCapture, runInteractive, shellQuote }; diff --git a/bin/nemoclaw.js b/bin/nemoclaw.js index 7a08958c5..e7b285688 100755 --- a/bin/nemoclaw.js +++ b/bin/nemoclaw.js @@ -2,12 +2,12 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -const { execSync, spawnSync } = require("child_process"); +const { execFileSync, spawnSync } = require("child_process"); const path = require("path"); const fs = require("fs"); const os = require("os"); -const { ROOT, SCRIPTS, run, runCapture, runInteractive } = require("./lib/runner"); +const { ROOT, SCRIPTS, run, runCapture, runInteractive, shellQuote } = require("./lib/runner"); const { ensureApiKey, ensureGithubToken, @@ -28,10 +28,6 @@ const GLOBAL_COMMANDS = new Set([ const REMOTE_UNINSTALL_URL = "https://raw.githubusercontent.com/NVIDIA/NemoClaw/refs/heads/main/uninstall.sh"; -function shellQuote(value) { - return `'${String(value).replace(/'/g, `'\\''`)}'`; -} - function resolveUninstallScript() { const candidates = [ path.join(ROOT, "uninstall.sh"), @@ -107,14 +103,15 @@ async function deploy(instanceName) { } const name = instanceName.trim(); - // Validate: same RFC 1123 subdomain rules used for sandbox names — lowercase - // alphanumeric and hyphens, must start and end with alphanumeric. - if (!/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(name) || name.length > 253) { + // Validate: RFC 1123 label — lowercase alphanumeric and hyphens, + // must start and end with alphanumeric, max 63 chars. + if (!name || name.length > 63 || !/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(name)) { console.error(` Invalid instance name: '${name}'`); console.error(" Names must be lowercase, contain only letters, numbers, and hyphens,"); - console.error(" and must start and end with a letter or number (max 253 chars)."); + console.error(" and must start and end with a letter or number (max 63 chars)."); process.exit(1); } + const qname = shellQuote(name); const gpu = process.env.NEMOCLAW_GPU || "a2-highgpu-1g:nvidia-tesla-a100:1"; @@ -123,7 +120,7 @@ async function deploy(instanceName) { console.log(""); try { - execSync("which brev", { stdio: "ignore" }); + execFileSync("which", ["brev"], { stdio: "ignore" }); } catch { console.error("brev CLI not found. Install: https://brev.nvidia.com"); process.exit(1); @@ -131,13 +128,16 @@ async function deploy(instanceName) { let exists = false; try { - const out = execSync("brev ls 2>&1", { encoding: "utf-8" }); + const out = execFileSync("brev", ["ls"], { encoding: "utf-8" }); exists = out.includes(name); - } catch {} + } catch (err) { + if (err.stdout && err.stdout.includes(name)) exists = true; + if (err.stderr && err.stderr.includes(name)) exists = true; + } if (!exists) { console.log(` Creating Brev instance '${name}' (${gpu})...`); - run(`brev create "${name}" --gpu "${gpu}"`); + run(`brev create ${qname} --gpu ${shellQuote(gpu)}`); } else { console.log(` Brev instance '${name}' already exists.`); } @@ -147,7 +147,7 @@ async function deploy(instanceName) { 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" }); + execFileSync("ssh", ["-o", "ConnectTimeout=5", "-o", "StrictHostKeyChecking=no", name, "echo", "ok"], { encoding: "utf-8", stdio: "ignore" }); break; } catch { if (i === 59) { @@ -159,35 +159,36 @@ 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(`ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR ${qname} '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" ${qname}:/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 envDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-env-")); - const envTmp = path.join(envDir, ".env"); + const envTmp = path.join(envDir, "env"); + fs.writeFileSync(envTmp, envLines.join("\n") + "\n", { mode: 0o600 }); try { - fs.writeFileSync(envTmp, envLines.join("\n") + "\n", { mode: 0o600, flag: "wx" }); - run(`scp -q -o StrictHostKeyChecking=no -o LogLevel=ERROR "${envTmp}" "${name}":/home/ubuntu/nemoclaw/.env`); + run(`scp -q -o StrictHostKeyChecking=no -o LogLevel=ERROR ${shellQuote(envTmp)} ${qname}:/home/ubuntu/nemoclaw/.env`); } finally { - fs.rmSync(envDir, { recursive: true, force: true }); + try { fs.unlinkSync(envTmp); } catch {} + try { fs.rmdirSync(envDir); } catch {} } console.log(" Running setup..."); - runInteractive(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR "${name}" 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/brev-setup.sh'`); + runInteractive(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR ${qname} '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(`ssh -o StrictHostKeyChecking=no -o LogLevel=ERROR ${qname} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && bash scripts/start-services.sh'`); } console.log(""); console.log(" Connecting to sandbox..."); console.log(""); - runInteractive(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR "${name}" 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw'`); + runInteractive(`ssh -t -o StrictHostKeyChecking=no -o LogLevel=ERROR ${qname} 'cd /home/ubuntu/nemoclaw && set -a && . .env && set +a && openshell sandbox connect nemoclaw'`); } async function start() { diff --git a/test/deploy-instance-name.test.js b/test/deploy-instance-name.test.js index 2171ee2a3..f506a4d18 100644 --- a/test/deploy-instance-name.test.js +++ b/test/deploy-instance-name.test.js @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 // -// Verify that deploy() validates and quotes the instance name in shell +// Verify that deploy() validates and shell-quotes the instance name in // commands to prevent command injection. const { describe, it } = require("node:test"); @@ -45,53 +45,36 @@ describe("deploy instance name hardening", () => { assert.match( deployBody, /\^\[a-z0-9\]/, - "deploy() must validate the instance name against RFC 1123 subdomain rules" + "deploy() must validate the instance name against RFC 1123 label rules" ); }); - it("rejects names longer than 253 characters inside deploy()", () => { + it("enforces max 63 character limit", () => { assert.match( deployBody, - /name\.length\s*>\s*253/, - "deploy() must enforce a 253-character limit on instance names" + /length\s*>\s*63/, + "deploy() must enforce a 63-character limit on instance names" ); }); - it("does not interpolate unquoted instance name in shell commands", () => { - // Every ${name} in a shell command string must be exactly "${name}". - // Skip console.log/error lines which are display-only. - const lines = deployBody.split("\n"); - for (const line of lines) { - if (line.includes("${name}") && !line.match(/console\.(log|error)/)) { - const all = [...line.matchAll(/\$\{name\}/g)].length; - const quoted = [...line.matchAll(/"\$\{name\}"/g)].length; - assert.equal( - quoted, - all, - `Unquoted \${name} in shell command: ${line.trim()}` - ); - } - } - }); - - it("quotes instance name in brev create", () => { + it("uses shellQuote for instance name in shell commands", () => { assert.ok( - deployBody.includes('brev create "${name}"'), - 'brev create must quote the instance name: brev create "${name}"' + deployBody.includes("qname = shellQuote(name)"), + "deploy() must create a shellQuoted name variable" ); }); - it("quotes instance name in rsync destination", () => { + it("does not use execSync (prefer execFileSync)", () => { assert.ok( - deployBody.includes('"${name}":/home/ubuntu/nemoclaw/'), - 'rsync destination must quote the instance name: "${name}":/path' + !deployBody.includes("execSync("), + "deploy() must use execFileSync instead of execSync" ); }); - it("quotes instance name in scp destination", () => { + it("shell-quotes env values", () => { assert.ok( - deployBody.includes('"${name}":/home/ubuntu/nemoclaw/.env'), - 'scp destination must quote the instance name: "${name}":/path' + deployBody.includes("shellQuote(process.env.NVIDIA_API_KEY"), + "NVIDIA_API_KEY must be shellQuoted in env file" ); }); });