security: redact secret patterns from CLI log and error output#794
Closed
100rabhkr wants to merge 3 commits into
Closed
security: redact secret patterns from CLI log and error output#794100rabhkr wants to merge 3 commits into
100rabhkr wants to merge 3 commits into
Conversation
Add a redactSecrets() helper to bin/lib/runner.js that masks known secret patterns (NVIDIA_API_KEY, GITHUB_TOKEN, TELEGRAM_BOT_TOKEN, SLACK_BOT_TOKEN, DISCORD_BOT_TOKEN, OPENAI_API_KEY, bare nvapi- tokens) in all CLI error output. Applied to: - run() and runInteractive() error messages (console.error) - runCapture() thrown error .message and .stderr fields Patterns are created inside the function on each call to avoid the shared /g lastIndex gotcha with module-level regex constants. Includes 11 Vitest tests covering each secret pattern, no-op for clean strings, multiple secrets in one string, empty input, and consecutive-call consistency. Closes NVIDIA#664
Contributor
📝 WalkthroughWalkthroughAdded a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/runner-redact.test.js (1)
27-55: Add regression tests for quotedKEY=valueforms.Current coverage misses
KEY="secret"andKEY='secret', which are common CLI patterns and critical for this feature.✅ Suggested test additions
it("redacts GITHUB_TOKEN=value", () => { const result = redactSecrets("GITHUB_TOKEN=ghp_1234567890abcdef"); expect(result).not.toContain("ghp_1234567890abcdef"); expect(result).toContain("GITHUB_TOKEN=***"); }); + + it("redacts GITHUB_TOKEN with double-quoted value", () => { + const result = redactSecrets('GITHUB_TOKEN="ghp_quotedSecret123"'); + expect(result).toContain("GITHUB_TOKEN=***"); + expect(result).not.toContain("ghp_quotedSecret123"); + }); + + it("redacts TELEGRAM_BOT_TOKEN with single-quoted value", () => { + const result = redactSecrets("TELEGRAM_BOT_TOKEN='123456:ABC-DEF'"); + expect(result).toContain("TELEGRAM_BOT_TOKEN=***"); + expect(result).not.toContain("123456:ABC-DEF"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner-redact.test.js` around lines 27 - 55, Tests only cover unquoted KEY=value forms; add regression tests to test/runner-redact.test.js that assert redactSecrets also redacts quoted values like KEY="secret" and KEY='secret' (e.g., GITHUB_TOKEN="ghp_123...", GITHUB_TOKEN='ghp_123...') for the same keys already tested (GITHUB_TOKEN, TELEGRAM_BOT_TOKEN, OPENAI_API_KEY, SLACK_BOT_TOKEN, DISCORD_BOT_TOKEN) to ensure redactSecrets handles both double- and single-quoted values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Around line 20-26: The current regex entries like /GITHUB_TOKEN=[^\s"']*/g,
/OPENAI_API_KEY=[^\s"']*/g, etc., fail to match values wrapped in quotes; update
each secret-pattern regex in the runner.js regex list (the literals shown such
as /GITHUB_TOKEN=[^\s"']*/g, /OPENAI_API_KEY=[^\s"']*/g,
/NVIDIA_API_KEY=[^\s"']*/g, /TELEGRAM_BOT_TOKEN=[^\s"']*/g,
/SLACK_BOT_TOKEN=[^\s"']*/g, /DISCORD_BOT_TOKEN=[^\s"']*/g and the similar ones
on lines 30-33) so they also match quoted values by allowing an optional
surrounding single or double quote and using a non-greedy match up to the
matching quote or whitespace; replace each pattern accordingly so quoted secrets
like GITHUB_TOKEN="ghp_xxx" or GITHUB_TOKEN='ghp_xxx' are redacted.
- Around line 82-87: The error handling in runCapture currently redacts
err.message and err.stderr but misses err.stdout; update the catch/rethrow logic
in runCapture (where redactSecrets is applied to err.message and err.stderr) to
also check for and set err.stdout = redactSecrets(err.stdout) when present so
any captured stdout is scrubbed before the error is rethrown or logged.
---
Nitpick comments:
In `@test/runner-redact.test.js`:
- Around line 27-55: Tests only cover unquoted KEY=value forms; add regression
tests to test/runner-redact.test.js that assert redactSecrets also redacts
quoted values like KEY="secret" and KEY='secret' (e.g.,
GITHUB_TOKEN="ghp_123...", GITHUB_TOKEN='ghp_123...') for the same keys already
tested (GITHUB_TOKEN, TELEGRAM_BOT_TOKEN, OPENAI_API_KEY, SLACK_BOT_TOKEN,
DISCORD_BOT_TOKEN) to ensure redactSecrets handles both double- and
single-quoted values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f4bed39-e791-4563-bc89-973229d17794
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner-redact.test.js
- Rewrite redactSecrets to handle KEY="val", KEY='val', and KEY=val forms. Previous regex stopped at quote chars, leaving quoted secret values exposed. - Redact err.stdout in runCapture thrown errors alongside message and stderr, since execSync errors include stdout and callers may log it. - Add tests for double-quoted and single-quoted secret values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Line 27: The exported function redactSecrets currently assigns let result =
str and will throw if callers pass null/undefined; at the top of redactSecrets
coerce the incoming argument to a stable string (e.g. use String(str ?? "") or
result = String(str ?? "")) before any .replace calls so result.replace cannot
throw; update the initialization of the result variable in redactSecrets to this
defensive coercion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34e4a72f-a5cd-445f-98e5-a2a2473fd5a8
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner-redact.test.js
✅ Files skipped from review due to trivial changes (1)
- test/runner-redact.test.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/runner-redact.test.js (1)
100-117: Add a regression case for quoted secrets crossing the 80-char log boundary.Current integration coverage uses an unquoted token only. Please add a case where
run()logs a long command containingGITHUB_TOKEN="..."such that truncation would cut inside the quoted value, and assert the raw token never appears.🧪 Suggested test addition
it("run() error output redacts secrets (integration)", () => { @@ expect(result.stderr).not.toContain("nvapi-realSecretValue123"); }); + + it("run() error output redacts quoted secrets when snippet is truncated (integration)", () => { + const runnerPath = path.join(import.meta.dirname, "..", "bin", "lib", "runner"); + const script = ` + const { run } = require(${JSON.stringify(runnerPath)}); + process.exit = () => {}; + const prefix = "x".repeat(58); + run(\`\${prefix} GITHUB_TOKEN="ghp_realSecretValue1234567890" false\`); + `; + const result = spawnSync("node", ["-e", script], { + cwd: path.join(import.meta.dirname, ".."), + encoding: "utf-8", + timeout: 10000, + }); + expect(result.stderr).toContain("GITHUB_TOKEN=***"); + expect(result.stderr).not.toContain("ghp_realSecretValue1234567890"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runner-redact.test.js` around lines 100 - 117, Add an integration test variant in the existing runner-redact.test.js that calls run() with a long command containing a quoted secret (e.g., GITHUB_TOKEN="realSecret...") so the 80-char truncation boundary falls inside the quoted value; spawn a child (like the existing test that uses run("false NVIDIA_API_KEY=...")) with a long filler string before the quoted token to push the boundary, capture stderr, and assert the redacted form appears (e.g., GITHUB_TOKEN=***) while the raw secret string does not; reference the run function and mirror the existing spawnSync pattern and assertions to ensure the quoted-token case is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/runner.js`:
- Line 47: The error logging currently truncates the command with cmd.slice(0,
80) before calling redactSecrets, which can cut quoted secrets and bypass
redaction; update the console.error calls that log failures (the lines using
console.error with redactSecrets(cmd.slice(...))) to first call redactSecrets on
the full cmd string and then truncate the redacted result (e.g.,
redactSecrets(cmd).slice(0,80)); apply the same change to the similar occurrence
around the other failure log (the second console.error at the other location) so
all command snippets are redacted before truncation.
---
Nitpick comments:
In `@test/runner-redact.test.js`:
- Around line 100-117: Add an integration test variant in the existing
runner-redact.test.js that calls run() with a long command containing a quoted
secret (e.g., GITHUB_TOKEN="realSecret...") so the 80-char truncation boundary
falls inside the quoted value; spawn a child (like the existing test that uses
run("false NVIDIA_API_KEY=...")) with a long filler string before the quoted
token to push the boundary, capture stderr, and assert the redacted form appears
(e.g., GITHUB_TOKEN=***) while the raw secret string does not; reference the run
function and mirror the existing spawnSync pattern and assertions to ensure the
quoted-token case is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4da9de94-9cb7-40f0-9402-172e6bbd8de9
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner-redact.test.js
| }); | ||
| if (result.status !== 0 && !opts.ignoreError) { | ||
| console.error(` Command failed (exit ${result.status}): ${cmd.slice(0, 80)}`); | ||
| console.error(` Command failed (exit ${result.status}): ${redactSecrets(cmd.slice(0, 80))}`); |
Contributor
There was a problem hiding this comment.
Redact before truncating command snippets to prevent partial quoted-secret leakage.
cmd.slice(0, 80) runs before redaction, so a quoted assignment can be cut mid-value and miss the KEY="..." / KEY='...' match, leaking secret fragments in error logs.
🔐 Proposed fix
function run(cmd, opts = {}) {
@@
if (result.status !== 0 && !opts.ignoreError) {
- console.error(` Command failed (exit ${result.status}): ${redactSecrets(cmd.slice(0, 80))}`);
+ const safeCmd = redactSecrets(cmd).slice(0, 80);
+ console.error(` Command failed (exit ${result.status}): ${safeCmd}`);
process.exit(result.status || 1);
}
@@
function runInteractive(cmd, opts = {}) {
@@
if (result.status !== 0 && !opts.ignoreError) {
- console.error(` Command failed (exit ${result.status}): ${redactSecrets(cmd.slice(0, 80))}`);
+ const safeCmd = redactSecrets(cmd).slice(0, 80);
+ console.error(` Command failed (exit ${result.status}): ${safeCmd}`);
process.exit(result.status || 1);
}Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/runner.js` at line 47, The error logging currently truncates the
command with cmd.slice(0, 80) before calling redactSecrets, which can cut quoted
secrets and bypass redaction; update the console.error calls that log failures
(the lines using console.error with redactSecrets(cmd.slice(...))) to first call
redactSecrets on the full cmd string and then truncate the redacted result
(e.g., redactSecrets(cmd).slice(0,80)); apply the same change to the similar
occurrence around the other failure log (the second console.error at the other
location) so all command snippets are redacted before truncation.
Closed
2 tasks
8 tasks
This was referenced Mar 25, 2026
This was referenced Apr 2, 2026
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
3 participants
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
Add a
redactSecrets()helper tobin/lib/runner.jsthat masks known secret patterns in all CLI error output, preventing accidental credential leakage when commands fail.Patterns redacted
NVIDIA_API_KEY,GITHUB_TOKEN,TELEGRAM_BOT_TOKEN,SLACK_BOT_TOKEN,DISCORD_BOT_TOKEN,OPENAI_API_KEY, and barenvapi-tokens.Applied to
run()andrunInteractive()— error messages viaconsole.errorrunCapture()— thrown error.messageand.stderrfieldsRegex patterns are created inside the function on each call to avoid the well-known
/glastIndexstate leak across calls.Test plan
run()with a secret in a failing command, asserts stderr containsNVIDIA_API_KEY=***and does NOT contain the real valueFiles changed (2)
bin/lib/runner.jsredactSecrets(), apply torun()/runInteractive()error output andrunCapture()thrown errors, export functiontest/runner-redact.test.jsCloses #664
Summary by CodeRabbit