|
| 1 | +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. |
| 2 | +// SPDX-License-Identifier: Apache-2.0 |
| 3 | +// |
| 4 | +// Credential exposure regression tests. |
| 5 | +// |
| 6 | +// Verifies that real API secrets are NEVER present as literal values in any |
| 7 | +// --credential CLI argument across all three execution layers: |
| 8 | +// 1. bin/lib/onboard.js (legacy CLI layer) |
| 9 | +// 2. nemoclaw/src/commands/onboard.ts (plugin layer) |
| 10 | +// 3. nemoclaw-blueprint/orchestrator/runner.py (blueprint/K8s layer) |
| 11 | +// |
| 12 | +// The safe form is --credential KEY (env-var lookup — openshell reads the |
| 13 | +// value from the environment, never from the process argument list). |
| 14 | +// The UNSAFE form is --credential KEY=value (leaks secret in `ps aux`). |
| 15 | +// |
| 16 | +// Allowlisted dummy/stub values that are explicitly NOT secrets: |
| 17 | +// OPENAI_API_KEY=dummy (vllm-local placeholder) |
| 18 | +// OPENAI_API_KEY=ollama (ollama-local placeholder) |
| 19 | +// |
| 20 | +// See: https://github.com/NVIDIA/NemoClaw/issues/325 |
| 21 | + |
| 22 | +const { describe, it } = require("node:test"); |
| 23 | +const assert = require("node:assert/strict"); |
| 24 | +const fs = require("node:fs"); |
| 25 | +const path = require("node:path"); |
| 26 | + |
| 27 | +const ROOT = path.resolve(__dirname, ".."); |
| 28 | + |
| 29 | +// Safe dummy/stub credential values that are explicitly not secrets. |
| 30 | +// These are fine to pass as KEY=VALUE because they are not real credentials. |
| 31 | +const ALLOWED_LITERAL_CREDENTIALS = new Set([ |
| 32 | + "OPENAI_API_KEY=dummy", |
| 33 | + "OPENAI_API_KEY=ollama", |
| 34 | + "OPENAI_API_KEY=not-needed", |
| 35 | +]); |
| 36 | + |
| 37 | +const FILES_TO_SCAN = [ |
| 38 | + { path: "bin/lib/onboard.js", lang: "js" }, |
| 39 | + { path: "nemoclaw/src/commands/onboard.ts", lang: "ts" }, |
| 40 | + { path: "nemoclaw-blueprint/orchestrator/runner.py", lang: "py" }, |
| 41 | +]; |
| 42 | + |
| 43 | +// ── Static source scan ──────────────────────────────────────────── |
| 44 | + |
| 45 | +describe("credential exposure: no secrets in --credential CLI args (issue #325)", () => { |
| 46 | + for (const file of FILES_TO_SCAN) { |
| 47 | + it(`${file.path}: --credential args use env-lookup form (KEY only, not KEY=VALUE)`, () => { |
| 48 | + const fullPath = path.join(ROOT, file.path); |
| 49 | + if (!fs.existsSync(fullPath)) return; // skip if file absent |
| 50 | + |
| 51 | + const content = fs.readFileSync(fullPath, "utf-8"); |
| 52 | + const lines = content.split("\n"); |
| 53 | + |
| 54 | + const violations = []; |
| 55 | + |
| 56 | + for (let i = 0; i < lines.length; i++) { |
| 57 | + const line = lines[i]; |
| 58 | + const lineNum = i + 1; |
| 59 | + |
| 60 | + // Skip full-line comments |
| 61 | + const trimmed = line.trim(); |
| 62 | + if (trimmed.startsWith("//") || trimmed.startsWith("#")) continue; |
| 63 | + // Skip inline comments after code (crude but sufficient for our patterns) |
| 64 | + |
| 65 | + // Match: --credential <optional quote><KEY>=<VALUE><optional quote/bracket> |
| 66 | + // This regex catches both JS template literals and Python f-strings |
| 67 | + const m = line.match(/--credential\s+['"`]?([A-Z_]{3,64})=([^'"`\s,)]+)/); |
| 68 | + if (!m) continue; |
| 69 | + |
| 70 | + const key = m[1]; |
| 71 | + const value = m[2].replace(/['"}`\s]/g, ""); |
| 72 | + const combined = `${key}=${value}`; |
| 73 | + |
| 74 | + if (ALLOWED_LITERAL_CREDENTIALS.has(combined)) continue; |
| 75 | + |
| 76 | + violations.push( |
| 77 | + ` ${file.path}:${lineNum}: --credential passes literal secret: "${combined}"\n` + |
| 78 | + ` Fix: set process.env["${key}"] = <value> before the call, then pass --credential "${key}"` |
| 79 | + ); |
| 80 | + } |
| 81 | + |
| 82 | + assert.equal( |
| 83 | + violations.length, |
| 84 | + 0, |
| 85 | + `\n\nCREDENTIAL EXPOSURE DETECTED (issue #325):\n\n${violations.join("\n\n")}\n` |
| 86 | + ); |
| 87 | + }); |
| 88 | + } |
| 89 | + |
| 90 | + // ── Layer-specific structural assertions ───────────────────────── |
| 91 | + |
| 92 | + it("bin/lib/onboard.js: nvidia-nim block uses env-lookup form (no NVIDIA_API_KEY=$ interpolation)", () => { |
| 93 | + const content = fs.readFileSync(path.join(ROOT, "bin/lib/onboard.js"), "utf-8"); |
| 94 | + |
| 95 | + // The --credential argument must NOT have NVIDIA_API_KEY value interpolated. |
| 96 | + // Note: "-- env NVIDIA_API_KEY=value" is a separate openshell sandbox-startup |
| 97 | + // injection protocol, NOT the --credential flag, so we match specifically. |
| 98 | + assert.ok( |
| 99 | + !content.match(/--credential[^\n]*NVIDIA_API_KEY=\${/), |
| 100 | + 'onboard.js must not pass NVIDIA_API_KEY value to --credential arg.\n' + |
| 101 | + 'Use env-lookup form: --credential "NVIDIA_API_KEY" (with env set on the child process)' |
| 102 | + ); |
| 103 | + }); |
| 104 | + |
| 105 | + it("nemoclaw/src/commands/onboard.ts: sets process.env before passing credential name to execOpenShell", () => { |
| 106 | + const tsPath = path.join(ROOT, "nemoclaw/src/commands/onboard.ts"); |
| 107 | + if (!fs.existsSync(tsPath)) return; |
| 108 | + const content = fs.readFileSync(tsPath, "utf-8"); |
| 109 | + |
| 110 | + assert.ok( |
| 111 | + content.includes("process.env[credentialEnv] = apiKey"), |
| 112 | + "onboard.ts must set process.env[credentialEnv] = apiKey before calling execOpenShell" |
| 113 | + ); |
| 114 | + |
| 115 | + // The --credential arg must pass the env var NAME (credentialEnv), not its value |
| 116 | + assert.ok( |
| 117 | + !content.match(/["'`]--credential["'`],\s*[`"']\$\{credentialEnv\}=\$\{apiKey\}/), |
| 118 | + "onboard.ts must not pass credentialEnv=apiKey as the --credential value" |
| 119 | + ); |
| 120 | + }); |
| 121 | + |
| 122 | + it("nemoclaw-blueprint/orchestrator/runner.py: sets os.environ before passing credential name", () => { |
| 123 | + const pyPath = path.join(ROOT, "nemoclaw-blueprint/orchestrator/runner.py"); |
| 124 | + if (!fs.existsSync(pyPath)) return; |
| 125 | + const content = fs.readFileSync(pyPath, "utf-8"); |
| 126 | + |
| 127 | + assert.ok( |
| 128 | + content.includes("os.environ[target_cred_env] = credential"), |
| 129 | + "runner.py must set os.environ[target_cred_env] = credential before run_cmd" |
| 130 | + ); |
| 131 | + |
| 132 | + assert.ok( |
| 133 | + !content.includes('f"OPENAI_API_KEY={credential}"'), |
| 134 | + 'runner.py must not pass f"OPENAI_API_KEY={credential}" as --credential value' |
| 135 | + ); |
| 136 | + |
| 137 | + // Must not pass f"{target_cred_env}={credential}" either (from PR #191's partial fix) |
| 138 | + assert.ok( |
| 139 | + !content.match(/f['"]\{target_cred_env\}=\{credential\}['"]/), |
| 140 | + 'runner.py must not pass f"{target_cred_env}={credential}" as --credential value' |
| 141 | + ); |
| 142 | + }); |
| 143 | + |
| 144 | + it("nemoclaw-blueprint/blueprint.yaml: default profile has credential_env set", () => { |
| 145 | + const bpPath = path.join(ROOT, "nemoclaw-blueprint/blueprint.yaml"); |
| 146 | + if (!fs.existsSync(bpPath)) return; |
| 147 | + const content = fs.readFileSync(bpPath, "utf-8"); |
| 148 | + |
| 149 | + // The default profile block should have credential_env. |
| 150 | + // Profile names sit at 6-space indent; their fields are at 8-space indent. |
| 151 | + // We grab everything from " default:" up to the next 6-space sibling key. |
| 152 | + const defaultBlockMatch = content.match(/ {6}default:\s*\n([\s\S]*?)(?=\n {6}\w)/); |
| 153 | + assert.ok(defaultBlockMatch, "blueprint.yaml must have a default profile"); |
| 154 | + assert.ok( |
| 155 | + defaultBlockMatch[0].includes("credential_env"), |
| 156 | + "blueprint.yaml default profile must define credential_env (missing causes silent auth failure)" |
| 157 | + ); |
| 158 | + }); |
| 159 | +}); |
| 160 | + |
| 161 | +// ── Runtime injection PoC ───────────────────────────────────────── |
| 162 | + |
| 163 | +describe("runCaptureArgv: injection PoC (proves fix works)", () => { |
| 164 | + const { runCaptureArgv } = require("../bin/lib/runner"); |
| 165 | + |
| 166 | + it("OLD bash -c IS vulnerable to subshell expansion", () => { |
| 167 | + // Demonstrate what the old code did — we use a safe payload |
| 168 | + const { execSync } = require("node:child_process"); |
| 169 | + const malicious = "safe_prefix_$(echo INJECTED_PROOF)"; |
| 170 | + let stdout; |
| 171 | + try { |
| 172 | + stdout = execSync(`echo ${malicious}`, { encoding: "utf-8" }).trim(); |
| 173 | + } catch { |
| 174 | + stdout = ""; |
| 175 | + } |
| 176 | + // The old bash -c pattern WOULD expand the subshell |
| 177 | + assert.ok( |
| 178 | + stdout.includes("INJECTED_PROOF") || stdout.includes("safe_prefix_"), |
| 179 | + "Confirming bash -c expands $() — this is the vulnerability" |
| 180 | + ); |
| 181 | + }); |
| 182 | + |
| 183 | + it("NEW runCaptureArgv is NOT vulnerable to subshell expansion", () => { |
| 184 | + const malicious = "safe_prefix_$(echo INJECTED_PROOF)"; |
| 185 | + const out = runCaptureArgv("echo", [malicious]); |
| 186 | + assert.ok( |
| 187 | + out.includes("$(echo INJECTED_PROOF)"), |
| 188 | + `Expected literal subshell syntax in output, got: "${out}"` |
| 189 | + ); |
| 190 | + assert.ok( |
| 191 | + !out.includes("INJECTED_PROOF") || out.includes("$(echo INJECTED_PROOF)"), |
| 192 | + `runCaptureArgv must pass args literally — injection detected! Output: "${out}"` |
| 193 | + ); |
| 194 | + }); |
| 195 | + |
| 196 | + it("NEW runCaptureArgv is NOT vulnerable to && chaining", () => { |
| 197 | + const malicious = "ignored && echo CHAINED"; |
| 198 | + const out = runCaptureArgv("echo", [malicious]); |
| 199 | + assert.ok( |
| 200 | + out.includes("&&"), |
| 201 | + "&& must be passed literally, not interpreted as command chaining" |
| 202 | + ); |
| 203 | + }); |
| 204 | +}); |
0 commit comments