fix(security): redact secret patterns from CLI log and error output#1246
fix(security): redact secret patterns from CLI log and error output#1246cv merged 9 commits intoNVIDIA:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds secret-redaction utilities (exported as Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Runner as "bin/lib/runner.js"
participant Child as "child_process.spawnSync"
participant Console as "process.stdout/stderr"
Caller->>Runner: invoke run/runCapture/runInteractive(cmd, ...)
Runner->>Child: spawnSync(cmd, { stdio: ["ignore","pipe","pipe"], ... })
Child-->>Runner: result { status, stdout, stderr, error? }
Runner->>Runner: redact(stdout), redact(stderr), redactError(error?)
alt error present
Runner->>Caller: throw redacted Error
else
Runner->>Console: writeRedactedResult(stdout/stderr)
Runner-->>Caller: return result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Adds runner-level secret redaction so sensitive tokens/keys are masked before appearing in CLI log output or bubbled-up command execution errors.
Changes:
- Introduces
redact()/redactError()inbin/lib/runner.jsand applies redaction torun(),runInteractive(), andrunCapture()error paths. - Adds unit/regression tests covering multiple secret formats (NVIDIA keys, bearer tokens, GitHub tokens, key assignments) and multi-secret strings.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| bin/lib/runner.js | Adds secret redaction helpers and applies them to runner error/log output paths. |
| test/runner.test.js | Adds test coverage validating redaction behavior and runCapture error-field redaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/runner.js (1)
16-27:⚠️ Potential issue | 🔴 CriticalChild output with inherited stdio bypasses redaction in
run()andrunInteractive().Both functions default to inherited stdio (
["ignore", "inherit", "inherit"]and"inherit"respectively), which means the child process writes directly to the parent terminal. When a command fails, the sanitized command message is redacted (line 25, 40), but the actual child process output—which may contain raw secrets—reaches the terminal first. This creates a security gap: the redaction only covers the echoed command, not the failure output that precedes it.In contrast,
runCapture()uses piped stdio (["pipe", "pipe", "pipe"]) and properly redacts captured output viaredactError()before re-throwing. If sensitive commands are run throughrun()orrunInteractive(), their unredacted output is not protected by the current redaction strategy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 16 - 27, The current run() (and similarly runInteractive()) uses inherited stdio so child output can print raw secrets before we log a redacted error; change them to capture child stdout/stderr (use stdio: ["pipe","pipe","pipe"] or equivalent) and, on non-zero exit, run the captured output through redactError() (or redact()) before writing any console output or rethrowing; update run() to read result.stdout/result.stderr, redact them, then print the redacted message and exit, and apply the same approach in runInteractive() (or delegate to runCapture()) so no child output bypasses redaction.
🤖 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 66-80: The current redact function and SECRET_PATTERNS miss
authenticated URLs and auth-style query params; update redact (and
SECRET_PATTERNS) to also handle URL userinfo and auth query params by either
reusing the URL-aware redaction used in onboard-session.js or by adding
patterns/logic that (1) detect URL userinfo (e.g., https://user:pass@host/ ) and
replace the password portion after the colon with asterisks, and (2) detect auth
query parameters like auth, sig, signature (case-insensitive) and redact their
values (e.g., ?auth=VALUE -> ?auth=****); make these changes in the redact
function and/or SECRET_PATTERNS so calls to redact(...) will consistently mask
authenticated URLs and those query-param credentials.
---
Outside diff comments:
In `@bin/lib/runner.js`:
- Around line 16-27: The current run() (and similarly runInteractive()) uses
inherited stdio so child output can print raw secrets before we log a redacted
error; change them to capture child stdout/stderr (use stdio:
["pipe","pipe","pipe"] or equivalent) and, on non-zero exit, run the captured
output through redactError() (or redact()) before writing any console output or
rethrowing; update run() to read result.stdout/result.stderr, redact them, then
print the redacted message and exit, and apply the same approach in
runInteractive() (or delegate to runCapture()) so no child output bypasses
redaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f4cb343-9a4b-4b89-8ab5-3fe1b28db155
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner.test.js
This comment was marked as abuse.
This comment was marked as abuse.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/runner.js (1)
111-125: Minor:redactErrormutation may cause double-redaction if error is re-caught.Since
redactErrormutates the error in place, if an already-redacted error is somehow caught and passed throughredactErroragain, theredactMatchoutput (e.g.,nvap****) could theoretically be re-processed. This is unlikely in current flow sincerunCaptureonly calls it once before throwing, but worth noting for defensive coding.The existing behavior is acceptable given the current call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/runner.js` around lines 111 - 125, redactError currently mutates the passed-in Error which can lead to double-redaction if the same error is reprocessed; change redactError to operate on a shallow copy of the error object instead of mutating the original: create a new object (preserving non-enumerable Error semantics as needed) copy relevant properties from the input err, run redact(...) on message/cmd/stdout/stderr/output on the copy, compute the stack replacement using the originalMessage and the copy.message, and return the new redacted error while leaving the input err unchanged; keep references to the existing functions/variables redact and replaceAll and the function name redactError for locating the change.
🤖 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 111-125: redactError currently mutates the passed-in Error which
can lead to double-redaction if the same error is reprocessed; change
redactError to operate on a shallow copy of the error object instead of mutating
the original: create a new object (preserving non-enumerable Error semantics as
needed) copy relevant properties from the input err, run redact(...) on
message/cmd/stdout/stderr/output on the copy, compute the stack replacement
using the originalMessage and the copy.message, and return the new redacted
error while leaving the input err unchanged; keep references to the existing
functions/variables redact and replaceAll and the function name redactError for
locating the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30c362b7-7ddd-4fb8-b8cb-f781396df716
📒 Files selected for processing (2)
bin/lib/runner.jstest/runner.test.js
|
Review follow-up: the originally actionable runner issues on this PR are already addressed on
The remaining CodeRabbit note about |
Adds JSDoc comments to run(), runInteractive(), runCapture(), redactMatch(), redactUrl(), redact(), and writeRedactedResult() to bring docstring coverage above the 80% threshold.
|
Added JSDoc comments to all functions in |
|
@BenediktSchackenberg can you address CI failures |
…er tests TypeScript check was failing with TS2339 because Error type does not include .cmd and .output properties. Using @type {any} JSDoc casts to tell the type checker these are intentionally extended error objects (as produced by Node's execSync on failure). Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
@kjw3 — fixed the TypeScript check failure: added |
wscurran
added
security
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Thorough work. The stdio change from inherit to pipe is the right call — it is the only way to intercept child output before it reaches the terminal. URL credential redaction, the error field coverage, and the test suite are all solid.
One minor note for future: redactError mutates the error in place, so a re-caught error would get double-redacted. Acceptable for current call sites since runCapture only calls it once before throwing.
Finalizes the approach from #672 on top of current
main.Summary:
run(),runInteractive(), andrunCapture()pathsTested:
Summary by CodeRabbit
New Features
Bug Fixes
Tests