From f2de42d7621576e604acb5f230c20b6f29e7c950 Mon Sep 17 00:00:00 2001 From: Brian Taylor Date: Thu, 19 Mar 2026 14:49:16 -0700 Subject: [PATCH] security: add shell-injection prevention and pass NGC_API_KEY to NIM container - Add shellQuote() to runner.js for safe shell interpolation - Pass NGC_API_KEY (with NVIDIA_API_KEY fallback) to NIM docker container, fixing issue #210 where containers cannot pull model weights - Quote CHAT_UI_URL, NVIDIA_API_KEY, and NGC_API_KEY in sandbox env - Double-quote model names in openshell inference set commands - Move shellQuote from onboard.js local function to shared runner.js export - Add 11 tests covering shellQuote, NGC_API_KEY escaping, and env fallback Intentionally does not touch --credential lines (deferred to #330). Signed-off-by: Brian Taylor --- bin/lib/nim.js | 9 +++-- bin/lib/onboard.js | 20 +++++------ bin/lib/runner.js | 7 +++- test/nim.test.js | 86 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 109 insertions(+), 13 deletions(-) diff --git a/bin/lib/nim.js b/bin/lib/nim.js index 4f2233e435f..0ce384b1a79 100644 --- a/bin/lib/nim.js +++ b/bin/lib/nim.js @@ -3,7 +3,7 @@ // // NIM container management — pull, start, stop, health-check NIM images. -const { run, runCapture } = require("./runner"); +const { run, runCapture, shellQuote } = require("./runner"); const nimImages = require("./nim-images.json"); function containerName(sandboxName) { @@ -136,9 +136,14 @@ function startNimContainer(sandboxName, model, port = 8000) { // Stop any existing container with same name run(`docker rm -f ${name} 2>/dev/null || true`, { ignoreError: true }); + // Pass NGC_API_KEY so the container can pull model weights from NGC. + // Falls back to NVIDIA_API_KEY for environments that only set that. + const ngcKey = process.env.NGC_API_KEY || process.env.NVIDIA_API_KEY || ""; + const envFlags = ngcKey ? `-e NGC_API_KEY=${shellQuote(ngcKey)}` : ""; + console.log(` Starting NIM container: ${name}`); run( - `docker run -d --gpus all -p ${port}:8000 --name ${name} --shm-size 16g ${image}` + `docker run -d --gpus all -p ${port}:8000 --name ${name} --shm-size 16g ${envFlags} ${image}` ); return name; } diff --git a/bin/lib/onboard.js b/bin/lib/onboard.js index 060b5ba2120..25d782fdc40 100644 --- a/bin/lib/onboard.js +++ b/bin/lib/onboard.js @@ -7,7 +7,7 @@ const fs = require("fs"); const path = require("path"); -const { ROOT, SCRIPTS, run, runCapture } = require("./runner"); +const { ROOT, SCRIPTS, run, runCapture, shellQuote } = require("./runner"); const { getDefaultOllamaModel, getLocalProviderBaseUrl, @@ -85,10 +85,6 @@ function step(n, total, msg) { console.log(` ${"─".repeat(50)}`); } -function shellQuote(value) { - return `'${String(value).replace(/'/g, `'\\''`)}'`; -} - function pythonLiteralJson(value) { return JSON.stringify(JSON.stringify(value)); } @@ -464,9 +460,13 @@ async function createSandbox(gpu) { console.log(` Creating sandbox '${sandboxName}' (this takes a few minutes on first run)...`); const chatUiUrl = process.env.CHAT_UI_URL || 'http://127.0.0.1:18789'; - const envArgs = [`CHAT_UI_URL=${chatUiUrl}`]; + const envArgs = [`CHAT_UI_URL=${shellQuote(chatUiUrl)}`]; if (process.env.NVIDIA_API_KEY) { - envArgs.push(`NVIDIA_API_KEY=${process.env.NVIDIA_API_KEY}`); + envArgs.push(`NVIDIA_API_KEY=${shellQuote(process.env.NVIDIA_API_KEY)}`); + } + const ngcKey = process.env.NGC_API_KEY || process.env.NVIDIA_API_KEY || ""; + if (ngcKey) { + envArgs.push(`NGC_API_KEY=${shellQuote(ngcKey)}`); } // Run without piping through awk — the pipe masked non-zero exit codes @@ -733,7 +733,7 @@ async function setupInference(sandboxName, model, provider) { { ignoreError: true } ); run( - `openshell inference set --no-verify --provider nvidia-nim --model ${model} 2>/dev/null || true`, + `openshell inference set --no-verify --provider nvidia-nim --model "${model}" 2>/dev/null || true`, { ignoreError: true } ); } else if (provider === "vllm-local") { @@ -752,7 +752,7 @@ async function setupInference(sandboxName, model, provider) { { ignoreError: true } ); run( - `openshell inference set --no-verify --provider vllm-local --model ${model} 2>/dev/null || true`, + `openshell inference set --no-verify --provider vllm-local --model "${model}" 2>/dev/null || true`, { ignoreError: true } ); } else if (provider === "ollama-local") { @@ -772,7 +772,7 @@ async function setupInference(sandboxName, model, provider) { { ignoreError: true } ); run( - `openshell inference set --no-verify --provider ollama-local --model ${model} 2>/dev/null || true`, + `openshell inference set --no-verify --provider ollama-local --model "${model}" 2>/dev/null || true`, { ignoreError: true } ); console.log(` Priming Ollama model: ${model}`); diff --git a/bin/lib/runner.js b/bin/lib/runner.js index 53ec88996aa..504cd7fcd76 100644 --- a/bin/lib/runner.js +++ b/bin/lib/runner.js @@ -58,4 +58,9 @@ function runCapture(cmd, opts = {}) { } } -module.exports = { ROOT, SCRIPTS, run, runCapture, runInteractive }; +/** Single-quote a value for safe interpolation into shell commands. */ +function shellQuote(value) { + return `'${String(value).replace(/'/g, `'\\''`)}'`; +} + +module.exports = { ROOT, SCRIPTS, run, runCapture, runInteractive, shellQuote }; diff --git a/test/nim.test.js b/test/nim.test.js index 8166cf6c430..e375f9e837b 100644 --- a/test/nim.test.js +++ b/test/nim.test.js @@ -5,6 +5,7 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); const nim = require("../bin/lib/nim"); +const { shellQuote } = require("../bin/lib/runner"); describe("nim", () => { describe("listModels", () => { @@ -74,4 +75,89 @@ describe("nim", () => { assert.equal(st.running, false); }); }); + + describe("NGC_API_KEY escaping", () => { + it("single-quotes a normal API key", () => { + const key = "nvapi-abc123DEF456"; + const envFlags = key ? `-e NGC_API_KEY=${shellQuote(key)}` : ""; + assert.equal(envFlags, "-e NGC_API_KEY='nvapi-abc123DEF456'"); + }); + + it("escapes embedded single quotes in key", () => { + const key = "key'with'quotes"; + const envFlags = `-e NGC_API_KEY=${shellQuote(key)}`; + assert.equal(envFlags, "-e NGC_API_KEY='key'\\''with'\\''quotes'"); + }); + + it("produces empty string for empty key", () => { + const key = ""; + const envFlags = key ? `-e NGC_API_KEY=${shellQuote(key)}` : ""; + assert.equal(envFlags, ""); + }); + + it("blocks shell metacharacters via single quotes", () => { + const key = '$(whoami)"; rm -rf /; echo "'; + const envFlags = `-e NGC_API_KEY=${shellQuote(key)}`; + assert.ok(envFlags.startsWith("-e NGC_API_KEY='")); + assert.ok(envFlags.endsWith("'")); + }); + + it("prefers NGC_API_KEY over NVIDIA_API_KEY in fallback", () => { + const origNgc = process.env.NGC_API_KEY; + const origNvidia = process.env.NVIDIA_API_KEY; + try { + process.env.NGC_API_KEY = "ngc-primary"; + process.env.NVIDIA_API_KEY = "nvidia-fallback"; + const ngcKey = process.env.NGC_API_KEY || process.env.NVIDIA_API_KEY || ""; + assert.equal(ngcKey, "ngc-primary"); + } finally { + if (origNgc === undefined) delete process.env.NGC_API_KEY; + else process.env.NGC_API_KEY = origNgc; + if (origNvidia === undefined) delete process.env.NVIDIA_API_KEY; + else process.env.NVIDIA_API_KEY = origNvidia; + } + }); + + it("falls back to NVIDIA_API_KEY when NGC_API_KEY empty", () => { + const origNgc = process.env.NGC_API_KEY; + const origNvidia = process.env.NVIDIA_API_KEY; + try { + process.env.NGC_API_KEY = ""; + process.env.NVIDIA_API_KEY = "nvidia-fallback"; + const ngcKey = process.env.NGC_API_KEY || process.env.NVIDIA_API_KEY || ""; + assert.equal(ngcKey, "nvidia-fallback"); + } finally { + if (origNgc === undefined) delete process.env.NGC_API_KEY; + else process.env.NGC_API_KEY = origNgc; + if (origNvidia === undefined) delete process.env.NVIDIA_API_KEY; + else process.env.NVIDIA_API_KEY = origNvidia; + } + }); + }); + + describe("shellQuote (shared helper)", () => { + it("wraps a simple value in single quotes", () => { + assert.equal(shellQuote("hello"), "'hello'"); + }); + + it("escapes embedded single quotes", () => { + assert.equal(shellQuote("it's"), "'it'\\''s'"); + }); + + it("handles empty string", () => { + assert.equal(shellQuote(""), "''"); + }); + + it("neutralizes shell metacharacters", () => { + const result = shellQuote('$(whoami); rm -rf /'); + assert.ok(result.startsWith("'")); + assert.ok(result.endsWith("'")); + assert.ok(result.includes("$(whoami)")); + }); + + it("coerces non-string values", () => { + assert.equal(shellQuote(12345), "'12345'"); + assert.equal(shellQuote(null), "'null'"); + }); + }); });