fix(security): redact secret patterns from CLI log and error output

bin/lib/runner.js

@@ -14,30 +14,32 @@ if (dockerHost) {
 }
 
 function run(cmd, opts = {}) {
-  const stdio = opts.stdio ?? ["ignore", "inherit", "inherit"];
+  const stdio = opts.stdio ?? ["ignore", "pipe", "pipe"];
   const result = spawnSync("bash", ["-c", cmd], {
     ...opts,
     stdio,
     cwd: ROOT,
     env: { ...process.env, ...opts.env },
   });
+  writeRedactedResult(result, stdio);
   if (result.status !== 0 && !opts.ignoreError) {
-    console.error(`  Command failed (exit ${result.status}): ${cmd.slice(0, 80)}`);
+    console.error(`  Command failed (exit ${result.status}): ${redact(cmd).slice(0, 80)}`);
     process.exit(result.status || 1);
   }
   return result;
 }
 
 function runInteractive(cmd, opts = {}) {
-  const stdio = opts.stdio ?? "inherit";
+  const stdio = opts.stdio ?? ["inherit", "pipe", "pipe"];
   const result = spawnSync("bash", ["-c", cmd], {
     ...opts,
     stdio,
     cwd: ROOT,
     env: { ...process.env, ...opts.env },
   });
+  writeRedactedResult(result, stdio);
   if (result.status !== 0 && !opts.ignoreError) {
-    console.error(`  Command failed (exit ${result.status}): ${cmd.slice(0, 80)}`);
+    console.error(`  Command failed (exit ${result.status}): ${redact(cmd).slice(0, 80)}`);
     process.exit(result.status || 1);
   }
   return result;
@@ -54,7 +56,81 @@ function runCapture(cmd, opts = {}) {
     }).trim();
   } catch (err) {
     if (opts.ignoreError) return "";
-    throw err;
+    throw redactError(err);
+  }
+}
+
+/**
+ * Redact known secret patterns from a string to prevent accidental leaks
+ * in CLI log and error output. Covers NVIDIA API keys, bearer tokens,
+ * generic API key assignments, and base64-style long tokens.
+ */
+const SECRET_PATTERNS = [
+  /nvapi-[A-Za-z0-9_-]{10,}/g,
+  /nvcf-[A-Za-z0-9_-]{10,}/g,
+  /ghp_[A-Za-z0-9_-]{10,}/g,
+  /(?<=Bearer\s+)[A-Za-z0-9_.+/=-]{10,}/gi,
+  /(?<=(?:_KEY|API_KEY|SECRET|TOKEN|PASSWORD|CREDENTIAL)[=: ]['"]?)[A-Za-z0-9_.+/=-]{10,}/gi,
+];
+
+function redactMatch(match) {
+  return match.slice(0, 4) + "*".repeat(Math.min(match.length - 4, 20));
+}
+
+function redactUrl(value) {
+  if (typeof value !== "string" || value.length === 0) return value;
+  try {
+    const url = new URL(value);
+    if (url.password) {
+      url.password = "****";
+    }
+    for (const key of [...url.searchParams.keys()]) {
+      if (/(^|[-_])(?:signature|sig|token|auth|access_token)$/i.test(key)) {
+        url.searchParams.set(key, "****");
+      }
+    }
+    return url.toString();
+  } catch {
+    return value;
+  }
+}
+
+function redact(str) {
+  if (typeof str !== "string") return str;
+  let out = str.replace(/https?:\/\/[^\s'"]+/g, redactUrl);
+  for (const pat of SECRET_PATTERNS) {
+    out = out.replace(pat, redactMatch);
+  }
+  return out;
+}
+
+/**
+ * Redact sensitive fields on an error object before surfacing it to callers.
+ * NOTE: this mutates the original error instance in place.
+ */
+function redactError(err) {
+  if (!err || typeof err !== "object") return err;
+  const originalMessage = typeof err.message === "string" ? err.message : null;
+  if (typeof err.message === "string") err.message = redact(err.message);
+  if (typeof err.cmd === "string") err.cmd = redact(err.cmd);
+  if (typeof err.stdout === "string") err.stdout = redact(err.stdout);
+  if (typeof err.stderr === "string") err.stderr = redact(err.stderr);
+  if (Array.isArray(err.output)) {
+    err.output = err.output.map((value) => (typeof value === "string" ? redact(value) : value));
+  }
+  if (originalMessage && typeof err.stack === "string") {
+    err.stack = err.stack.replaceAll(originalMessage, err.message);
+  }
+  return err;
+}
+
+function writeRedactedResult(result, stdio) {
+  if (!result || stdio === "inherit" || !Array.isArray(stdio)) return;
+  if (stdio[1] === "pipe" && result.stdout) {
+    process.stdout.write(redact(result.stdout.toString()));
+  }
+  if (stdio[2] === "pipe" && result.stderr) {
+    process.stderr.write(redact(result.stderr.toString()));
   }
 }
 
@@ -85,4 +161,4 @@ function validateName(name, label = "name") {
   return name;
 }
 
-module.exports = { ROOT, SCRIPTS, run, runCapture, runInteractive, shellQuote, validateName };
+module.exports = { ROOT, SCRIPTS, redact, run, runCapture, runInteractive, shellQuote, validateName };

test/runner.test.js

@@ -6,7 +6,7 @@ import childProcess from "node:child_process";
 import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
-import { describe, expect, it } from "vitest";
+import { describe, expect, it, vi } from "vitest";
 
 import { runCapture } from "../bin/lib/runner";
 
@@ -39,7 +39,7 @@ describe("runner helpers", () => {
     // @ts-expect-error — intentional partial mock for testing
     childProcess.spawnSync = (...args) => {
       calls.push(args);
-      return { status: 0 };
+      return { status: 0, stdout: "", stderr: "" };
     };
 
     try {
@@ -53,8 +53,8 @@ describe("runner helpers", () => {
     }
 
     expect(calls).toHaveLength(2);
-    expect(calls[0][2].stdio).toEqual(["ignore", "inherit", "inherit"]);
-    expect(calls[1][2].stdio).toBe("inherit");
+    expect(calls[0][2].stdio).toEqual(["ignore", "pipe", "pipe"]);
+    expect(calls[1][2].stdio).toEqual(["inherit", "pipe", "pipe"]);
   });
 });
 
@@ -83,7 +83,7 @@ describe("runner env merging", () => {
     // @ts-expect-error — intentional partial mock for testing
     childProcess.spawnSync = (...args) => {
       calls.push(args);
-      return { status: 0 };
+      return { status: 0, stdout: "", stderr: "" };
     };
 
     try {
@@ -170,7 +170,233 @@ describe("validateName", () => {
   });
 });
 
+describe("redact", () => {
+  it("masks NVIDIA API keys", () => {
+    const { redact } = require(runnerPath);
+    expect(redact("key is nvapi-abc123XYZ_def456")).toBe("key is nvap******************");
+  });
+
+  it("masks NVCF keys", () => {
+    const { redact } = require(runnerPath);
+    expect(redact("nvcf-abcdef1234567890")).toBe("nvcf*****************");
+  });
+
+  it("masks bearer tokens", () => {
+    const { redact } = require(runnerPath);
+    expect(redact("Authorization: Bearer eyJhbGciOiJIUzI1NiJ9.payload")).toBe(
+      "Authorization: Bearer eyJh********************"
+    );
+  });
+
+  it("masks key assignments in commands", () => {
+    const { redact } = require(runnerPath);
+    expect(redact("export NVIDIA_API_KEY=nvapi-realkey12345")).toContain("nvap");
+    expect(redact("export NVIDIA_API_KEY=nvapi-realkey12345")).not.toContain("realkey12345");
+  });
+
+  it("masks variables ending in _KEY", () => {
+    const { redact } = require(runnerPath);
+    const output = redact('export SERVICE_KEY="supersecretvalue12345"');
+    expect(output).not.toContain("supersecretvalue12345");
+    expect(output).toContain('export SERVICE_KEY="supe');
+  });
+
+  it("masks bare GitHub personal access tokens", () => {
+    const { redact } = require(runnerPath);
+    const output = redact("token ghp_abcdefghijklmnopqrstuvwxyz1234567890");
+    expect(output).toContain("ghp_");
+    expect(output).not.toContain("abcdefghijklmnopqrstuvwxyz1234567890");
+  });
+
+  it("masks bearer tokens case-insensitively", () => {
+    const { redact } = require(runnerPath);
+    expect(redact("authorization: bearer someBearerToken")).toContain("some****");
+    expect(redact("authorization: bearer someBearerToken")).not.toContain("someBearerToken");
+    expect(redact("AUTHORIZATION: BEARER someBearerToken")).toContain("some****");
+    expect(redact("AUTHORIZATION: BEARER someBearerToken")).not.toContain("someBearerToken");
+  });
+
+  it("masks bearer tokens with repeated spacing", () => {
+    const { redact } = require(runnerPath);
+    const output = redact("Authorization: Bearer   someBearerToken");
+    expect(output).toContain("some****");
+    expect(output).not.toContain("someBearerToken");
+  });
+
+  it("masks quoted assignment values", () => {
+    const { redact } = require(runnerPath);
+    const output = redact('API_KEY="secret123abc"');
+    expect(output).not.toContain("secret123abc");
+    expect(output).toContain('API_KEY="sec');
+  });
+
+  it("masks multiple secrets in one string", () => {
+    const { redact } = require(runnerPath);
+    const output = redact("nvapi-firstkey12345 nvapi-secondkey67890");
+    expect(output).not.toContain("firstkey12345");
+    expect(output).not.toContain("secondkey67890");
+    expect(output).toContain("nvap");
+    expect(output).toContain(" ");
+  });
+
+  it("masks URL credentials and auth query parameters", () => {
+    const { redact } = require(runnerPath);
+    const output = redact(
+      "https://alice:secret@example.com/v1/models?auth=abc123456789&sig=def987654321&keep=yes"
+    );
+    expect(output).toBe("https://alice:****@example.com/v1/models?auth=****&sig=****&keep=yes");
+  });
+
+  it("masks auth-style query parameters case-insensitively", () => {
+    const { redact } = require(runnerPath);
+    const output = redact("https://example.com?Signature=secret123456&AUTH=anothersecret123");
+    expect(output).toBe("https://example.com/?Signature=****&AUTH=****");
+  });
+
+  it("leaves non-secret strings untouched", () => {
+    const { redact } = require(runnerPath);
+    expect(redact("docker run --name my-sandbox")).toBe("docker run --name my-sandbox");
+    expect(redact("openshell sandbox list")).toBe("openshell sandbox list");
+  });
+
+  it("handles non-string input gracefully", () => {
+    const { redact } = require(runnerPath);
+    expect(redact(null)).toBe(null);
+    expect(redact(undefined)).toBe(undefined);
+    expect(redact(42)).toBe(42);
+  });
+});
+
 describe("regression guards", () => {
+  it("runCapture redacts secrets before rethrowing errors", () => {
+    const originalExecSync = childProcess.execSync;
+    childProcess.execSync = () => {
+      throw new Error(
+        'command failed: export SERVICE_KEY="supersecretvalue12345" ghp_abcdefghijklmnopqrstuvwxyz1234567890'
+      );
+    };
+
+    try {
+      delete require.cache[require.resolve(runnerPath)];
+      const { runCapture } = require(runnerPath);
+
+      let error;
+      try {
+        runCapture("echo nope");
+      } catch (err) {
+        error = err;
+      }
+
+      expect(error).toBeInstanceOf(Error);
+      expect(error.message).toContain("ghp_");
+      expect(error.message).not.toContain("supersecretvalue12345");
+      expect(error.message).not.toContain("abcdefghijklmnopqrstuvwxyz1234567890");
+    } finally {
+      childProcess.execSync = originalExecSync;
+      delete require.cache[require.resolve(runnerPath)];
+    }
+  });
+
+  it("runCapture redacts execSync error cmd/output fields", () => {
+    const originalExecSync = childProcess.execSync;
+    childProcess.execSync = () => {
+      const err = new Error("command failed");
+      err.cmd = "echo nvapi-aaaabbbbcccc1111 && echo ghp_abcdefghijklmnopqrstuvwxyz123456";
+      err.output = ["stdout: nvapi-aaaabbbbcccc1111", "stderr: PASSWORD=secret123456"];
+      throw err;
+    };
+
+    try {
+      delete require.cache[require.resolve(runnerPath)];
+      const { runCapture } = require(runnerPath);
+
+      let error;
+      try {
+        runCapture("echo nope");
+      } catch (err) {
+        error = err;
+      }
+
+      expect(error).toBeDefined();
+      expect(error).toBeInstanceOf(Error);
+      expect(error.cmd).not.toContain("nvapi-aaaabbbbcccc1111");
+      expect(error.cmd).not.toContain("ghp_abcdefghijklmnopqrstuvwxyz123456");
+      expect(Array.isArray(error.output)).toBe(true);
+      expect(error.output[0]).not.toContain("nvapi-aaaabbbbcccc1111");
+      expect(error.output[1]).not.toContain("secret123456");
+      expect(error.output[0]).toContain("****");
+      expect(error.output[1]).toContain("****");
+    } finally {
+      childProcess.execSync = originalExecSync;
+      delete require.cache[require.resolve(runnerPath)];
+    }
+  });
+
+  it("run redacts captured child output before printing on failure", () => {
+    const originalSpawnSync = childProcess.spawnSync;
+    const originalExit = process.exit;
+    const stdoutSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true);
+    const stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
+    const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {});
+
+    // @ts-expect-error — intentional partial mock for testing
+    childProcess.spawnSync = () => ({
+      status: 1,
+      stdout: "token ghp_abcdefghijklmnopqrstuvwxyz1234567890\n",
+      stderr: 'export SERVICE_KEY="supersecretvalue12345"\n',
+    });
+    process.exit = ((code) => {
+      throw new Error(`exit:${code}`);
+    });
+
+    try {
+      delete require.cache[require.resolve(runnerPath)];
+      const { run } = require(runnerPath);
+      expect(() => run("echo fail")).toThrow("exit:1");
+      expect(stdoutSpy).toHaveBeenCalledWith("token ghp_********************\n");
+      expect(stderrSpy).toHaveBeenCalledWith('export SERVICE_KEY="supe*****************"\n');
+      expect(errorSpy).toHaveBeenCalledWith("  Command failed (exit 1): echo fail");
+    } finally {
+      childProcess.spawnSync = originalSpawnSync;
+      process.exit = originalExit;
+      stdoutSpy.mockRestore();
+      stderrSpy.mockRestore();
+      errorSpy.mockRestore();
+      delete require.cache[require.resolve(runnerPath)];
+    }
+  });
+
+  it("runInteractive keeps stdin inherited while redacting captured output", () => {
+    const originalSpawnSync = childProcess.spawnSync;
+    const stdoutSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true);
+    const stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
+    const calls = [];
+
+    // @ts-expect-error — intentional partial mock for testing
+    childProcess.spawnSync = (...args) => {
+      calls.push(args);
+      return {
+        status: 0,
+        stdout: "visit https://alice:secret@example.com/?token=abc123456789\n",
+        stderr: "",
+      };
+    };
+
+    try {
+      delete require.cache[require.resolve(runnerPath)];
+      const { runInteractive } = require(runnerPath);
+      runInteractive("echo interactive");
+      expect(calls[0][2].stdio).toEqual(["inherit", "pipe", "pipe"]);
+      expect(stdoutSpy).toHaveBeenCalledWith("visit https://alice:****@example.com/?token=****\n");
+      expect(stderrSpy).not.toHaveBeenCalled();
+    } finally {
+      childProcess.spawnSync = originalSpawnSync;
+      stdoutSpy.mockRestore();
+      stderrSpy.mockRestore();
+      delete require.cache[require.resolve(runnerPath)];
+    }
+  });
+
   it("nemoclaw.js does not use execSync", () => {
     const src = fs.readFileSync(
       path.join(import.meta.dirname, "..", "bin", "nemoclaw.js"),