fix(security): harden low-risk code scanning findings - #3657
Merged
Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR hardens NemoClaw by redacting sensitive data from logs and messages, sanitizing user input, replacing cryptographic random sources with proper UUIDs, securing temporary file and PID handling, and updating related tests across the codebase. ChangesSecurity & Data Redaction Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/_ext/search_assets/modules/SearchEngine.js (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd required SPDX license header.
This file is missing the SPDX license header required by coding guidelines. As per coding guidelines, all
.jsfiles must include the SPDX header at the top.📄 Required SPDX header
Add these lines at the very top of the file:
+// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + /** * SearchEngine Module * Handles Lunr.js integration and search logic with filtering and grouping */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/_ext/search_assets/modules/SearchEngine.js` at line 1, This file is missing the required SPDX license header; add the specified SPDX header lines as the very first lines of the file (above any comments or code) so the file begins with the license declaration; update modules/SearchEngine.js (the module containing the SearchEngine implementation) by inserting the header at the top of the file before the existing /** comment and any exports or definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/_ext/search_assets/modules/SearchEngine.js`:
- Line 1: This file is missing the required SPDX license header; add the
specified SPDX header lines as the very first lines of the file (above any
comments or code) so the file begins with the license declaration; update
modules/SearchEngine.js (the module containing the SearchEngine implementation)
by inserting the header at the top of the file before the existing /** comment
and any exports or definitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b023b824-dfc3-4afd-a121-c8714abe9e59
📒 Files selected for processing (17)
docs/_ext/search_assets/modules/SearchEngine.jsnemoclaw-blueprint/scripts/slack-channel-guard.jsnemoclaw/src/onboard/config.tssrc/commands/internal/installer/plan.tssrc/lib/actions/dev/npm-link-or-shim.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/adapters/openshell/client.test.tssrc/lib/adapters/openshell/client.tssrc/lib/cli/nemoclaw-oclif-command.tssrc/lib/diagnostics/debug.tssrc/lib/onboard.tssrc/lib/onboard/preflight.tssrc/lib/onboard/summary.test.tssrc/lib/onboard/summary.tssrc/lib/security/redact.tssrc/lib/state/onboard-session.tssrc/lib/tunnel/services.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/preflight.test.ts (1)
1286-1286: 💤 Low valueIndentation inconsistency.
This
it()block has 4 leading spaces while all other sibling test declarations in thisdescribeblock use 2 spaces. This will likely be flagged by the Biome formatter.- it("treats thrown runCapture errors as error reason", () => { + it("treats thrown runCapture errors as error reason", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/preflight.test.ts` at line 1286, Fix the indentation of the test declaration starting with it("treats thrown runCapture errors as error reason", () => { so it uses 2 leading spaces like its sibling tests instead of 4; locate this test in src/lib/onboard/preflight.test.ts and adjust the leading whitespace of that it() block to match the surrounding describe block's 2-space indentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/onboard/preflight.test.ts`:
- Line 1286: Fix the indentation of the test declaration starting with
it("treats thrown runCapture errors as error reason", () => { so it uses 2
leading spaces like its sibling tests instead of 4; locate this test in
src/lib/onboard/preflight.test.ts and adjust the leading whitespace of that it()
block to match the surrounding describe block's 2-space indentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: df9e156e-a4f1-484b-81d8-151bcf1eb002
📒 Files selected for processing (2)
src/lib/onboard/preflight.test.tssrc/lib/onboard/preflight.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25979435423
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25979731454
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25981516199
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25999142366
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26000346929
|
ericksoa
approved these changes
May 18, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Reviewed locally at head f68362b against current origin/main. I focused on the issue #3654 high-reward/low-risk security bucket: log/JSON redaction, Slack and trycloudflare host parsing, temporary/PID file hardening, randomness, DNS parsing, and docs search query sanitization.
No blocking correctness or security regressions found. Local validation included build:cli, typecheck:cli, the focused security test slices, the Slack channel guard slice, nested nemoclaw config tests, source-shape:check, nested nemoclaw build, and npm run check after generated artifacts were present. GitHub checks are green and there are no active review threads.
Residual note: final CodeQL alert closure still depends on the next code-scanning/default-branch alert update, but this PR is in good shape to merge.
hunglp6d
pushed a commit
that referenced
this pull request
May 18, 2026
## Summary Harden the high-reward/low-risk code scanning findings by redacting logged values, replacing predictable temp/random patterns, and tightening URL/search validation paths. This targets the first remediation bucket from the code scanning risk/reward tracker. ## Related Issue Refs #3654 ## Changes - Add recursive log redaction for JSON command output and remove credential/env names from user-facing diagnostics. - Replace `Math.random()` session/temp suffixes with `crypto.randomUUID()`. - Move temp config/SSH files into private `mkdtemp` directories and write service PID files via secured file descriptors. - Parse Slack and Cloudflare hosts via URL/anchored host validation instead of broad substring URL checks. - Sanitize docs search terms before passing user input into Lunr query strategies. - Update affected tests for redacted messages and OpenShell error wording. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [ ] `npx prek run --all-files` passes - Ran twice; the full hook suite reached the CLI test phase but failed on existing 5s timeout/flaky host-environment tests unrelated to this patch. Plugin tests passed in the hook output. - [ ] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional checks run: - [x] `npm run typecheck:cli` passes - [x] `cd nemoclaw && npm run build` passes - [x] Targeted Vitest suite passes: `src/lib/security/redact.test.ts`, `src/lib/adapters/openshell/client.test.ts`, `src/lib/onboard/summary.test.ts`, `src/lib/onboard/preflight.test.ts`, `src/lib/tunnel/services.test.ts`, `src/lib/state/onboard-session.test.ts`, `src/lib/diagnostics/debug.test.ts`, `src/lib/actions/dev/npm-link-or-shim.test.ts`, `test/rebuild-credential-preflight.test.ts` - [x] `npx vitest run test/nemoclaw-start.test.ts -t "Slack channel guard"` passes - [x] `cd nemoclaw && npm test -- src/onboard/config.test.ts` passes --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Enhanced security redaction for sensitive data in logs and JSON output * **Bug Fixes** * Improved detection of Slack-related connection errors * Better DNS probing for container connectivity validation * **Improvements** * Cleaner onboarding messages that avoid exposing environment variable names and technical paths * More secure temporary file and directory handling with restrictive permissions * Generic error messages that provide clarity without revealing sensitive system information * Hardened sandbox rebuild logging with automatic redaction <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3657?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
5 tasks
6 tasks
This was referenced Jul 3, 2026
19 tasks
apurvvkumaria
pushed a commit
that referenced
this pull request
Jul 23, 2026
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3657?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
…output (#7356) ## Summary `sandbox doctor --json` resolved with the raw report, while `sandbox status --json` has redacted its machine-readable report since #4310. Both doctor JSON egress points now pass through the centralized `redactForLog`, so token-shaped values in check details no longer reach JSON consumers. Exit codes are still computed from the raw report before redaction. ## Related Issue Refs #4310 (the `status --json` redaction precedent this change mirrors). The asymmetry was found while verifying the redaction contract for the serving-process health design discussion on #7003 (refs only; this PR does not close either issue). ## Changes - `src/lib/actions/sandbox/doctor-report.ts`: the `renderDoctorReport` JSON branch prints via `console.log`, which bypasses the oclif `logJson` redaction boundary (#3657); the report is now routed through `redactForLog` before printing. - `src/commands/sandbox/doctor.ts`: the oclif `--json` path now returns `redactForLog(report)`, so programmatic consumers of the resolved value — not just the `logJson`-printed stdout — never see raw token-shaped detail. `process.exitCode` is derived from the raw report first. - Tests: one render-level and one command-boundary redaction test. Each uniquely pins its egress point: the command test mocks `runSandboxDoctor` (so `renderDoctorReport` never runs on that path), and the render test calls `renderDoctorReport` directly (the oclif path bypasses it via `quietJson`). Both were red before the fix. Review notes: - Pre-existing quirk (unchanged here, identical in `status --json`): `redactFull`'s word-context patterns can consume a short word following a credential-shaped env-var name, e.g. `…_API_KEY for NVIDIA managed health…` renders as `…_API_KEY <REDACTED> NVIDIA managed health…`. A candidate follow-up is to give `FULL_REDACT_PATTERNS` the same `{10,}` minimum value length `CONTEXT_PATTERNS` already uses. - The `renderDoctorReport` JSON branch currently has no production caller (the oclif command always passes `quietJson: true` with `--json`); it is redacted anyway because that `console.log` egress has no other redaction layer for any future direct caller. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: `doctor --json` is documented only as "Emit the report as JSON" with no field-level or verbatim-detail contract (`docs/reference/commands.mdx`); this change enforces the existing CLI redaction contract in `docs/security/best-practices.mdx`. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: [cjagwani approved PR SHA `92aa402`](#7356 (review)) after reviewing the redaction coverage, resolved automated feedback, required checks, and selected E2E. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: Not applicable - Supporting evidence: Not applicable — this PR does not change `scripts/prepare-dgx-station-host.sh`. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable — `npm run check:diff` passed (hooks skipped locally; commitlint reports one non-blocking `footer-leading-blank` warning) - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli src/lib/actions/sandbox/doctor-report.test.ts src/commands/sandbox/oclif-command-adapters.test.ts src/lib/actions/sandbox/doctor-flow.test.ts src/lib/actions/sandbox/doctor-inference.test.ts` → 42/42 passed - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable (targeted change) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed --- Signed-off-by: Dongni-Yang <dongniy@nvidia.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Sandbox diagnostic reports in JSON mode now redact token-shaped values from returned and displayed details. * Preserved failure status reporting while preventing sensitive token values from appearing in machine-readable output. * **Tests** * Added coverage confirming redaction in both CLI results and printed JSON reports. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Dongni-Yang <dongniy@nvidia.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
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
Harden the high-reward/low-risk code scanning findings by redacting logged values, replacing predictable temp/random patterns, and tightening URL/search validation paths. This targets the first remediation bucket from the code scanning risk/reward tracker.
Related Issue
Refs #3654
Changes
Math.random()session/temp suffixes withcrypto.randomUUID().mkdtempdirectories and write service PID files via secured file descriptors.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional checks run:
npm run typecheck:clipassescd nemoclaw && npm run buildpassessrc/lib/security/redact.test.ts,src/lib/adapters/openshell/client.test.ts,src/lib/onboard/summary.test.ts,src/lib/onboard/preflight.test.ts,src/lib/tunnel/services.test.ts,src/lib/state/onboard-session.test.ts,src/lib/diagnostics/debug.test.ts,src/lib/actions/dev/npm-link-or-shim.test.ts,test/rebuild-credential-preflight.test.tsnpx vitest run test/nemoclaw-start.test.ts -t "Slack channel guard"passescd nemoclaw && npm test -- src/onboard/config.test.tspassesSigned-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements