fix(security): scrub migration and backup credentials consistently - #7765
fix(security): scrub migration and backup credentials consistently#7765Ayush7614 wants to merge 3 commits into
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:
📝 WalkthroughWalkthroughCredential sanitization is centralized into shared helpers, extended to YAML and ChangesCredential Sanitization
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant BackupWalker
participant CredentialFilter
participant Filesystem
BackupWalker->>Filesystem: inspect backup files
BackupWalker->>CredentialFilter: sanitize JSON/YAML configuration
BackupWalker->>CredentialFilter: sanitize .env files
CredentialFilter->>Filesystem: atomically write sanitized files
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
🔧 Fix failing CI
Comment |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 3 optional E2E recommendations
Blockers
|
![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
🧹 Nitpick comments (2)
nemoclaw/src/security/credential-filter.ts (1)
4-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate credential-detection logic maintained manually "in parity."
This file's field lists/regexes (
CREDENTIAL_FIELDS,CREDENTIAL_FIELD_PATTERN,isCredentialField,stripCredentials, etc.) are near-duplicates ofsrc/lib/security/credential-filter.ts, kept in sync only by convention/comment. Since nemoclaw ships as its own package inside the sandbox image (separate fromsrc/lib), a shared import isn't straightforward, but relying on a manual "kept in parity" comment risks exactly the kind of drift this PR is fixing. Consider a lightweight guard — e.g. a test or script that diffs the exported field lists/patterns between the two files — so future edits to one side fail CI if the other isn't updated.🤖 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 `@nemoclaw/src/security/credential-filter.ts` around lines 4 - 6, Introduce a lightweight parity guard for the credential-filter implementations, comparing the exported field lists and credential-detection patterns used by CREDENTIAL_FIELDS, CREDENTIAL_FIELD_PATTERN, isCredentialField, and stripCredentials in both files. Make the guard fail CI when either side changes without the corresponding update, while preserving the separate package structure and runtime behavior.src/lib/security/credential-filter.ts (1)
393-437: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
toConfigValuefails closed silently — consider a louder failure mode.If any nested value can't be represented in
ConfigValue(e.g. via a futuremerge: true/custom-tags change, or an unanticipated YAML construct),toConfigValuebails out withundefinedfor the entire subtree, andsanitizeYamlConfigFilethen just returns without writing anything — leaving the original, unsanitized file on disk with no warning. With the current defaultparseYamloptions this is unlikely to trigger in practice (default core schema only produces JSON-compatible types), but per the security-boundary path instructions this file should "preserve... fail-closed handling" rather than silently pass through. Consider logging/erroring instead of a quiet no-op so a future schema change or unexpected input doesn't silently reintroduce the leak this PR is fixing. Separately,stringifyYamlre-serializing the whole document will drop any existing comments in the source YAML — worth a heads-up for anyone editing Hermes configs by hand.As per path instructions for
src/lib/{security,credentials,shields}/**: "Preserve deny-by-default behavior, least privilege, redaction, and fail-closed handling."🤖 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/security/credential-filter.ts` around lines 393 - 437, Make the conversion failure in toConfigValue fail closed visibly rather than returning through sanitizeYamlConfigFile silently. Update sanitizeYamlConfigFile to detect an unrepresentable parsed value, report the failure through the established error/logging mechanism, and skip writing the original file; preserve the existing no-write behavior on failure. Ensure sanitization never leaves an unsanitized file without an observable failure.Source: Path instructions
🤖 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.
Inline comments:
In `@src/lib/security/credential-filter.ts`:
- Around line 367-382: Update sanitizeEnvFileContent to recognize and remove a
leading export prefix before evaluating the environment variable key, while
preserving the original formatting when replacing the value. Ensure credential
fields such as export DB_PASS are sanitized, and add negative-path coverage
proving export-prefixed secrets cannot bypass filtering or appear in sanitized
output.
---
Nitpick comments:
In `@nemoclaw/src/security/credential-filter.ts`:
- Around line 4-6: Introduce a lightweight parity guard for the
credential-filter implementations, comparing the exported field lists and
credential-detection patterns used by CREDENTIAL_FIELDS,
CREDENTIAL_FIELD_PATTERN, isCredentialField, and stripCredentials in both files.
Make the guard fail CI when either side changes without the corresponding
update, while preserving the separate package structure and runtime behavior.
In `@src/lib/security/credential-filter.ts`:
- Around line 393-437: Make the conversion failure in toConfigValue fail closed
visibly rather than returning through sanitizeYamlConfigFile silently. Update
sanitizeYamlConfigFile to detect an unrepresentable parsed value, report the
failure through the established error/logging mechanism, and skip writing the
original file; preserve the existing no-write behavior on failure. Ensure
sanitization never leaves an unsanitized file without an observable failure.
🪄 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: Enterprise
Run ID: c1aff186-95c1-413e-bccb-c2d3fda1928b
📒 Files selected for processing (6)
nemoclaw/src/commands/migration-state.tsnemoclaw/src/security/credential-filter.test.tsnemoclaw/src/security/credential-filter.tssrc/lib/security/credential-filter.test.tssrc/lib/security/credential-filter.tssrc/lib/state/sandbox.ts
|
Addressed the CodeRabbit finding on Skipped the maintainability nit about adding a cross-package parity guard/script for the duplicated plugin filter — that would be a new CI abstraction without a current breakage, and the plugin package cannot import |
Align host-to-sandbox migration and rebuild backups with the shared credential filter so bot tokens, env secrets, Authorization headers, Hermes YAML, and .env PASS fields cannot survive snapshot sanitization. Signed-off-by: Ayush7614 <ayushknj3@gmail.com>
Shell-sourced env files often use `export KEY=value`, which bypassed key detection. Strip the prefix before credential-field matching. Signed-off-by: Ayush7614 <ayushknj3@gmail.com>
66cf9d3 to
5759812
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@nemoclaw/src/security/credential-filter.ts`:
- Around line 156-173: Update the credential-field handling in stripCredentials
so null and undefined values remain unchanged instead of being replaced with
CREDENTIAL_PLACEHOLDER. Preserve existing placeholder validation for non-null
credential values and keep the array behavior through scrubArrayElement
unchanged.
In `@src/lib/security/credential-filter.ts`:
- Around line 424-439: Make YAML sanitization fail closed: update
sanitizeYamlConfigFile and the conversion flow around lines 395-417 to return an
explicit failure for parse or unrepresentable-value errors, and ensure no
original artifact remains. Propagate this outcome through the backup caller
around lines 447-465 so failed sanitization causes the copied artifact to be
omitted or deleted. Add negative-path tests for malformed and unsupported YAML
that verify credentials are not retained. Affected sites:
src/lib/security/credential-filter.ts lines 424-439 require failure reporting;
lines 395-417 require unrepresentable-value propagation; lines 447-465 require
exposing and honoring the outcome.
🪄 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: Enterprise
Run ID: 6ffeb9be-65f1-4470-b402-91762092bd48
📒 Files selected for processing (6)
nemoclaw/src/commands/migration-state.tsnemoclaw/src/security/credential-filter.test.tsnemoclaw/src/security/credential-filter.tssrc/lib/security/credential-filter.test.tssrc/lib/security/credential-filter.tssrc/lib/state/sandbox.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/security/credential-filter.test.ts
- nemoclaw/src/security/credential-filter.test.ts
- nemoclaw/src/commands/migration-state.ts
- src/lib/state/sandbox.ts
5759812 to
48037ba
Compare
Preserve unset credential fields, omit unsanitizable Hermes YAML from backups, normalize backup file extensions, and align migration secret shape detection with the canonical token patterns. Signed-off-by: Ayush7614 <ayushknj3@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/lib/security/credential-filter.ts`:
- Around line 427-442: Update sanitizeYamlConfigFile to catch errors from
stringifyYaml and writeFileAtomically during the sanitized YAML rewrite,
returning false when either operation fails so callers take the existing cleanup
path. Preserve the true result only for successful rewrites, and add a
negative-path test covering a failed rewrite.
🪄 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: Enterprise
Run ID: c0192e78-867e-456b-b701-8bbe080c19ca
📒 Files selected for processing (5)
nemoclaw/src/security/credential-filter.test.tsnemoclaw/src/security/credential-filter.tssrc/lib/security/credential-filter.test.tssrc/lib/security/credential-filter.tssrc/lib/state/sandbox.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/state/sandbox.ts
- src/lib/security/credential-filter.test.ts
- nemoclaw/src/security/credential-filter.ts
| export function sanitizeYamlConfigFile(configPath: string): boolean { | ||
| const rawConfig = readRegularFileNoFollow(configPath); | ||
| if (rawConfig === null) return; | ||
| let parsed: ConfigValue; | ||
| if (rawConfig === null) return false; | ||
| let parsed: unknown; | ||
| try { | ||
| parsed = parseJson<ConfigValue>(rawConfig); | ||
| parsed = parseYaml(rawConfig); | ||
| } catch { | ||
| return; // Not valid JSON — skip (may be YAML for Hermes) | ||
| return false; | ||
| } | ||
| if (!isConfigObject(parsed)) return; | ||
| const configValue = toConfigValue(parsed); | ||
| if (!isConfigObject(configValue)) return false; | ||
|
|
||
| const { gateway: _gateway, ...config } = parsed; | ||
| const { gateway: _gateway, ...config } = configValue; | ||
| const sanitized = stripCredentials(config); | ||
| writeFileAtomically(configPath, JSON.stringify(sanitized, null, 2)); | ||
| writeFileAtomically(configPath, stringifyYaml(sanitized)); | ||
| return true; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the atomic writer can throw and whether callers delete failed YAML artifacts.
rg -n -A30 -B5 'writeFileAtomically' src/lib/security/credential-filter.ts
rg -n -A25 -B5 'sanitizeConfigFile\(fullPath\)' src/lib/state/sandbox.tsRepository: NVIDIA/NemoClaw
Length of output: 6148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '420,445p' src/lib/security/credential-filter.ts
printf '\n---\n'
sed -n '635,665p' src/lib/security/credential-filter.ts
printf '\n---\n'
rg -n "sanitizeYamlConfigFile|sanitizeConfigFile\\(" src/lib/security src/lib/state -g '*test.ts' -g '*spec.ts'Repository: NVIDIA/NemoClaw
Length of output: 1726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,280p' src/lib/security/credential-filter.test.ts
printf '\n---\n'
sed -n '635,660p' src/lib/security/credential-filter.ts
printf '\n---\n'
sed -n '1,220p' src/lib/security/credential-filter.test.tsRepository: NVIDIA/NemoClaw
Length of output: 11027
Fail closed on YAML rewrite errors.
sanitizeYamlConfigFile() can still throw from stringifyYaml() or writeFileAtomically(), which skips the false path the backup walker relies on to delete raw YAML artifacts. Catch those errors, return false, and add a negative-path test for a failed rewrite.
🤖 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/security/credential-filter.ts` around lines 427 - 442, Update
sanitizeYamlConfigFile to catch errors from stringifyYaml and
writeFileAtomically during the sanitized YAML rewrite, returning false when
either operation fails so callers take the existing cleanup path. Preserve the
true result only for successful rewrites, and add a negative-path test covering
a failed rewrite.
Source: Path instructions
wscurran
added
area: sandbox
|
✨ Thanks for the PR. This fixes credential leakage in migration and backup snapshots by routing both paths through the hardened scrubber. Maintainers will review the sanitization coverage, snapshot scanning, and mutation boundary. |
<!-- markdownlint-disable MD041 --> ## Summary This is a clean, compliant replacement for #7765 that preserves @Ayush7614 as the author of the original three commits. Host-to-sandbox migration and rebuild backups now scrub the same credential shapes, and backup creation fails closed instead of retaining a raw configuration when sanitization or cleanup fails. ## Changes - Share credential-field, token-shape, auth-header, CLI-argument, and safe-placeholder filtering across migration and rebuild snapshots. - Recursively sanitize every copied migration JSON, YAML, and `.env` artifact, including external roots; omit authentication-state files and malformed artifacts. - Reject required malformed OpenClaw configuration copies and clean up incomplete staging directories. - Delete and reject incomplete rebuild backups if an unsafe artifact cannot be removed. - Bind migration and rebuild snapshot traversal and mutations to opened directory descriptors, fail closed on parent or root identity changes, and require POSIX `python3` support. - Scrub common OAuth credential fields, including access, refresh, and identity tokens. - Add cross-implementation parity, failure-injection, malformed-artifact, value-shape, and real backup permission/content coverage. - Document that migration and rebuild snapshots are filtered restore points rather than byte-for-byte copies. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [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: Codex Desktop completed the nine-category security review at `5b8dd525f6297ca6fe1f3b669801c464824e5704`; PASS for secrets/credentials, input validation/sanitization, authorization boundaries, dependencies, error handling/logging, data protection, secure configuration/defaults, security regression testing, and holistic abuse/availability review. Exact diff fingerprint: `cbe679f02c0b986b6e02aa4a9aeb7778acb4539f6aca760c5ff6edae8745064a`. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Independent Codex Desktop documentation writer review passed at exact head `5a27ad655cc1329e15ff37e28440762e24cb4dd6`; all three changed pages and generated OpenClaw, Hermes, and Deep Agents variants were reviewed; `npm run docs` completed with 0 errors and 2 existing Fern warnings. - Agent: Codex Desktop <!-- docs-review-head-sha: 5a27ad6 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## 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 validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — At exact head `5b8dd525f`, the `27/27` interpreter-boundary and sanitizer tests, pre-commit guardrails, and plugin/CLI pre-push typechecks passed. The documentation build passed at immediately preceding head `eb10756df`; the follow-up changes tests only. At signed feature commit `d59a0e3b4` before the docs-only main sync, the full `767/767` plugin suite and coverage ratchet (95.31% statements / 95.96% lines) passed; the feature diff is unchanged by the sync. - [x] Applicable broad gate passed — `npm run validate:pr`; PASS on current main. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [x] `npm run docs` builds without errors (2 pre-existing warnings) - [x] 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) --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Security Enhancements** * Migration and snapshot bundles now sanitize credentials across JSON, YAML, and `.env` files while preserving approved placeholders and null values. * Sensitive or unsafe artifacts are omitted or removed, with stricter fail-closed behavior and clearer error reporting. * Incomplete backups are cleaned up when sanitization fails. * **Documentation** * Expanded snapshot contents, retention, and sanitization guidance. * Documented the `python3` POSIX requirement and WSL support for Windows environments. * **Tests** * Added broader credential detection, redaction, parity, filesystem-safety, and failure-path coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Ayush7614 <ayushknj3@gmail.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Ayush7614 <ayushknj3@gmail.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
|
Closing as superseded by merged #7847. The replacement preserves @Ayush7614 as author of the original three signed commits and adds the fail-closed cleanup, traversal hardening, tests, documentation, and security review needed to resolve this PR's outstanding blocker. Thank you, @Ayush7614, for the original fix. |
4 participants
Summary
Host-to-sandbox migration and rebuild backups used a weaker credential scrubber than the shared filter, so Slack
botToken/appToken,Authorizationheaders, env secrets such asGITHUB_TOKENandDB_PASS, and Hermesconfig.yamlsecrets could survive into snapshots. This change routes both paths through the hardened scrubbing rules so those values are stripped (or excluded) before backup or migration copy.Changes
src/lib/security/credential-filter.tswithsanitizeEnvFile/sanitizeEnvFileContent, YAML config sanitization, and snapshot scanning for.yaml/.yml(still excluding dependency lockfiles).src/lib/state/sandbox.tsto sanitize JSON, Hermes YAML, and.envvia the shared helpers instead of the ad-hocKEY|TOKEN|SECRET|PASSWORD|CREDENTIALsubstring regex.nemoclaw/src/security/credential-filter.tsand wired host→sandbox migration innemoclaw/src/commands/migration-state.tsto it so migration strips the same field classes (channel tokens, headers, env secrets, CLI flag args, value-shaped secrets) and excludesauth.json/chatgpt-auth.jsonas well asauth-profiles.json.src/lib/security/credential-filter.test.tsandnemoclaw/src/security/credential-filter.test.ts.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededdocs/pages changed. Behavior is internal credential scrubbing for migration snapshots and rebuild backups; users do not configure or observe a new surface.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/security/credential-filter.test.ts src/lib/security/credential-filter-secret-patterns.test.ts→ 36 passed;npx vitest run --project plugin nemoclaw/src/security/credential-filter.test.ts→ 4 passed; CLI and plugin typecheck passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Ayush7614 ayushknj3@gmail.com
Summary by CodeRabbit
.yaml/.yml), including removal of thegatewaysection..env-style content and config files with safer placeholder handling and fail-closed behavior..envsanitization.