fix: redact secrets from CLI output and stop exposing API keys (#579, #664)#780
Closed
cluster2600 wants to merge 2 commits intoNVIDIA:mainfrom
Closed
fix: redact secrets from CLI output and stop exposing API keys (#579, #664)#780cluster2600 wants to merge 2 commits intoNVIDIA:mainfrom
cluster2600 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
- uninstall.sh: check parent directory writability instead of file writability when deciding whether sudo is needed for removal. The previous check followed symlinks, so a symlink to a user-writable target in a root-owned directory would attempt unprivileged rm and fail with EACCES. - scripts/install.sh: replace NODE_MGR-based sudo heuristic with a direct writability check on the npm global prefix. The old approach always used sudo for nodesource installs, which changes PATH and breaks environments where the prefix is already user-writable.
…in process args (NVIDIA#579, NVIDIA#664) Add redactSecrets() to runner.js with patterns matching env assignments, nvapi- prefixes, GitHub PATs, and Bearer tokens. Apply to error messages in run() and runInteractive() so failed commands never leak credentials. Fix setupSpark() to pass NVIDIA_API_KEY via env option instead of interpolating into the command string (visible in ps aux). Fix walkthrough.sh to use tmux -e for env propagation instead of embedding the key in the shell command. Fix telegram-bridge.js to use SSH SendEnv instead of command-line export. 9 new tests covering redaction patterns and regression guards.
Contributor
📝 WalkthroughWalkthroughThis pull request addresses a critical security vulnerability by preventing NVIDIA API keys and other sensitive credentials from being exposed in process arguments and terminal output. Changes include adding a secret redaction function, refactoring multiple scripts and JavaScript files to pass credentials via environment variables rather than command-line arguments, and updating shell scripts to avoid printing or embedding API keys in visible command strings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
bin/lib/runner.js (1)
73-86: Consider applying redaction torunCaptureerrors for consistency.
runandrunInteractivenow redact secrets from error output, butrunCapturere-throws the raw error which may contain command snippets with secrets inerr.cmdorerr.message. For defense-in-depth, consider redacting before re-throwing:} catch (err) { if (opts.ignoreError) return ""; + if (err.message) err.message = redactSecrets(err.message); throw err; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 73 - 86, runCapture currently re-throws the raw error which may contain secrets in err.cmd or err.message; update runCapture to redact secrets before re-throwing by using the same redaction utility used by run/runInteractive (e.g., redactSecrets or redact) to sanitize err.message and err.cmd, and then throw a new/error with the redacted message (preserving original stack if possible) while keeping existing opts.ignoreError behavior intact.scripts/telegram-bridge.js (1)
145-146: Consider redacting secrets from error output.The
stderrcontent returned on failure could potentially contain secrets if the remote command somehow outputs them. Consider applyingredactSecretsfrombin/lib/runner.jshere for defense-in-depth:const { redactSecrets } = require("../bin/lib/runner"); // ... resolve(`Agent exited with code ${code}. ${redactSecrets(stderr.trim().slice(0, 500))}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/telegram-bridge.js` around lines 145 - 146, Require and use the redactSecrets helper to sanitize stderr before including it in the resolved error string: add "const { redactSecrets } = require('../bin/lib/runner');" near the top of scripts/telegram-bridge.js and update the error branch where it currently does resolve(`Agent exited with code ${code}. ${stderr.trim().slice(0, 500)}`) to instead call redactSecrets on the stderr snippet (e.g. resolve(`Agent exited with code ${code}. ${redactSecrets(stderr.trim().slice(0, 500))}`)) so any potential secrets are redacted.test/runner.test.js (1)
255-267: Fragile regex for function extraction.The regex
async function setupSpark\b[\s\S]*?\n\}uses a non-greedy match that stops at the first\n}. This works for the current simplesetupSparkfunction, but would capture only a partial body if the function grows to include nested blocks (e.g.,ifstatements with braces).Consider a more robust assertion that doesn't require extracting the full function body:
♻️ Alternative approach
- const match = src.match(/async function setupSpark\b[\s\S]*?\n\}/); - assert.ok(match, "setupSpark function must exist"); - const body = match[0]; - assert.ok( - !body.includes("NVIDIA_API_KEY=") || body.includes("env:"), - "setupSpark must pass API key via env option, not in the command string", - ); + // Find setupSpark and its run() call + const funcStart = src.indexOf("async function setupSpark"); + assert.ok(funcStart !== -1, "setupSpark function must exist"); + // Check the run() call pattern after setupSpark declaration + const afterFunc = src.slice(funcStart, funcStart + 500); + assert.ok( + afterFunc.includes('env: { NVIDIA_API_KEY:'), + "setupSpark must pass API key via env option", + ); + assert.ok( + !afterFunc.includes('NVIDIA_API_KEY=${') && !afterFunc.includes('NVIDIA_API_KEY="'), + "setupSpark must not interpolate API key in command string", + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner.test.js` around lines 255 - 267, The current regex extracting setupSpark is brittle; replace the brittle match with a stable AST-based check: parse the file (e.g., with acorn/espree), locate the FunctionDeclaration/FunctionExpression named setupSpark, obtain its body node source, and assert that any child CallExpression that builds the command (or calls spawn/exec) passes an options object using an env property (i.e., options object has a property named "env" or the call uses a second argument referencing an env object). Update the test in test/runner.test.js to use the AST lookup instead of the regex (referencing setupSpark, match, and body in the test) so the assertion no longer depends on fragile string matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/runner.js`:
- Around line 73-86: runCapture currently re-throws the raw error which may
contain secrets in err.cmd or err.message; update runCapture to redact secrets
before re-throwing by using the same redaction utility used by
run/runInteractive (e.g., redactSecrets or redact) to sanitize err.message and
err.cmd, and then throw a new/error with the redacted message (preserving
original stack if possible) while keeping existing opts.ignoreError behavior
intact.
In `@scripts/telegram-bridge.js`:
- Around line 145-146: Require and use the redactSecrets helper to sanitize
stderr before including it in the resolved error string: add "const {
redactSecrets } = require('../bin/lib/runner');" near the top of
scripts/telegram-bridge.js and update the error branch where it currently does
resolve(`Agent exited with code ${code}. ${stderr.trim().slice(0, 500)}`) to
instead call redactSecrets on the stderr snippet (e.g. resolve(`Agent exited
with code ${code}. ${redactSecrets(stderr.trim().slice(0, 500))}`)) so any
potential secrets are redacted.
In `@test/runner.test.js`:
- Around line 255-267: The current regex extracting setupSpark is brittle;
replace the brittle match with a stable AST-based check: parse the file (e.g.,
with acorn/espree), locate the FunctionDeclaration/FunctionExpression named
setupSpark, obtain its body node source, and assert that any child
CallExpression that builds the command (or calls spawn/exec) passes an options
object using an env property (i.e., options object has a property named "env" or
the call uses a second argument referencing an env object). Update the test in
test/runner.test.js to use the AST lookup instead of the regex (referencing
setupSpark, match, and body in the test) so the assertion no longer depends on
fragile string matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 775a71e7-a4a5-4992-8416-9b124a3ea015
⛔ Files ignored due to path filters (1)
research/results.tsvis excluded by!**/*.tsv
📒 Files selected for processing (7)
bin/lib/runner.jsbin/nemoclaw.jsscripts/install.shscripts/telegram-bridge.jsscripts/walkthrough.shtest/runner.test.jsuninstall.sh
Contributor
Author
|
Closing — overlaps with existing PRs already addressing the same issues. Cheers. |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
graph LR A[CLI output] --> B{redactSecrets} B --> C[env assignments masked] B --> D[nvapi- tokens masked] B --> E[ghp_ tokens masked] B --> F[Bearer tokens masked] style B fill:#f96,stroke:#333redactSecrets()with 4 patternssetupSparkpasses API key via environment, not command-line argumentswalkthrough.shusestmux -efor credential isolationtelegram-bridge.jsuses SSHSendEnvCloses #579, #664.
Test plan
--verboseoutputSummary by CodeRabbit