security: strip credentials from migration snapshots and enforce blueprint digest#156
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens sandbox migration and blueprint integrity checks to prevent credential exposure in the sandbox bundle and to ensure blueprint tampering cannot bypass verification via an empty digest.
Changes:
- Sanitize migration sandbox bundle output by removing
auth-profiles.jsonand stripping credential-like fields fromopenclaw.json. - Update blueprint digest verification to fail when the manifest digest is missing/empty.
- Add a new security-focused test/PoC file intended to demonstrate the vulnerabilities and verify the fixes.
Reviewed changes
Copilot reviewed 3 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/security-credential-exposure.test.js | Adds a security PoC/test suite for credential exposure + digest verification behavior. |
| nemoclaw/src/commands/migration-state.ts | Adds credential sanitization helpers and applies them during sandbox bundle preparation. |
| nemoclaw/src/blueprint/verify.ts | Treats empty/missing digest as a verification error instead of silently passing. |
| nemoclaw/dist/commands/migration-state.js | Built output reflecting the new sanitization behavior. |
| nemoclaw/dist/commands/migration-state.js.map | Source map update for the built migration-state output. |
| nemoclaw/dist/commands/migration-state.d.ts.map | Type map update for the built migration-state output. |
| nemoclaw/dist/blueprint/verify.js | Built output reflecting the new empty-digest verification failure. |
| nemoclaw/dist/blueprint/verify.js.map | Source map update for the built verify output. |
| nemoclaw/dist/blueprint/verify.d.ts.map | Type map update for the built verify output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
gn00295120
force-pushed
the
security/sandbox-credential-exposure-and-blueprint-bypass
branch
3 times, most recently
from
edb87b5 to
a5b3097
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens NemoClaw’s migration and blueprint handling by preventing credential leakage into the sandbox and tightening blueprint integrity verification.
Changes:
- Sanitize migration snapshots/bundles by removing
auth-profiles.jsonand stripping credential-like fields fromopenclaw.json. - Treat missing/empty blueprint digests as a verification failure and add basic YAML value “cleaning” for manifest header parsing.
- Add a security-focused test suite intended to cover credential exposure and digest enforcement scenarios.
Reviewed changes
Copilot reviewed 4 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/security-credential-exposure.test.js | Adds security tests for credential exposure and blueprint digest enforcement (but currently reimplements logic instead of calling production code). |
| nemoclaw/src/commands/migration-state.ts | Adds credential sanitization for prepared sandbox state and external root snapshots. |
| nemoclaw/src/blueprint/verify.ts | Fails verification when manifest digest is missing/empty; improves error messaging. |
| nemoclaw/src/blueprint/resolve.ts | Adds cleanYamlValue to strip quotes/comments from manifest header fields (currently has a parsing bug for quoted values with comments). |
| nemoclaw/dist/commands/migration-state.js | Built output for migration sanitization changes. |
| nemoclaw/dist/commands/migration-state.js.map | Sourcemap update for migration-state build output. |
| nemoclaw/dist/commands/migration-state.d.ts.map | Type sourcemap update for migration-state build output. |
| nemoclaw/dist/blueprint/verify.js | Built output for blueprint digest verification changes. |
| nemoclaw/dist/blueprint/verify.js.map | Sourcemap update for blueprint verify build output. |
| nemoclaw/dist/blueprint/verify.d.ts.map | Type sourcemap update for blueprint verify build output. |
| nemoclaw/dist/blueprint/resolve.js | Built output for blueprint manifest parsing changes. |
| nemoclaw/dist/blueprint/resolve.js.map | Sourcemap update for blueprint resolve build output. |
| nemoclaw/dist/blueprint/resolve.d.ts.map | Type sourcemap update for blueprint resolve build output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
1. cleanYamlValue: remove inline comment BEFORE stripping surrounding quotes so that `"" # Computed at release time` correctly yields an empty string instead of the still-quoted `""`. 2. cleanYamlValue: broaden the inline-comment regex from `/\s+#\s/` to `/\s+#/` so that YAML comments without a trailing space (e.g. `abc123 #no-space`) are also stripped, matching the YAML spec. 3. walkAndRemoveFile: wrap `readdirSync(dirPath)` in its own try/catch and return early on failure so permission errors or transiently missing directories cannot abort snapshot creation (the function is documented as non-fatal). 4. Manifest-parsing tests now call the production `readCachedManifest` / `getCacheDir` exports instead of reimplementing the parsing logic, ensuring regressions in `cleanYamlValue` or `parseManifestHeader` are caught by the test suite. 5. "Fix verification" tests now call `detectHostOpenClaw` + `createSnapshotBundle` from the production migration-state module and assert on the produced bundle, replacing the previous duplicate local `stripCredentials` implementation. A second negative test confirms that raw credential strings are absent from the bundle. All 9 tests pass after the changes (node --test).
wscurran
added
Migration
|
Thanks for addressing the critical vulnerabilities related to migration snapshots and blueprint digests, this could significantly improve the security of NemoClaw. |
wscurran
added
priority: high
|
Really appreciate the security work here, @gn00295120 — stripping credentials from migration snapshots and enforcing blueprint digest are both important hardening steps. Since this was opened, we've added CI checks and shipped quite a few changes, so the base has drifted a fair bit. Could you rebase this onto the latest main so we can evaluate it against the current state of things? Would love to get a fresh look at this. Thanks for the effort! |
gn00295120
force-pushed
the
security/sandbox-credential-exposure-and-blueprint-bypass
branch
from
caa669d to
77fbc62
Compare
|
Thanks @cv — rebased onto latest main. Here's what changed during the rebase: Scope reduced to credential sanitization only. The blueprint digest bypass fix ( What remains (2 files changed):
The credential sanitization merged cleanly with the current codebase, including the pyright strict typing changes from #523. All tests pass. |
|
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 credential-sanitization utilities and applies them during migration bundle/snapshot creation: defines credential field allowlist, recursively strips credential values from Changes
Sequence DiagramsequenceDiagram
autonumber
participant User as User
participant MP as MigrationProcess
participant SE as SanitizationEngine
participant FS as FileSystem
User->>MP: start snapshot/bundle creation
MP->>FS: read sandbox (openclaw.json, agents/, external roots)
MP->>SE: sanitize openclaw.json (CREDENTIAL_FIELDS)
SE->>SE: recursively strip credential fields
SE-->>MP: return sanitized openclaw.json
MP->>FS: write sanitized openclaw.json
MP->>FS: remove `agents/*/auth-profiles.json`
MP->>FS: copy & sanitize external-root snapshots
MP->>FS: archive sanitized bundle/snapshot
FS-->>User: deliver sanitized snapshot/bundle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
nemoclaw/src/commands/migration-state.ts (2)
553-561: Consider extending credential field denylist for defense in depth.The current list covers OpenClaw's known credential patterns, but additional common field names could provide broader protection:
accessToken/access_tokenprivateKey/private_keysecretKey/secret_keyrefreshToken/refresh_tokenbearer,authorization,credentialsSince
auth-profiles.jsonis deleted entirely, this is a secondary defense layer. The current list likely covers existing OpenClaw schemas, but expanding it would guard against future credential fields.🛡️ Suggested extension
const CREDENTIAL_FIELDS = new Set([ "apiKey", "api_key", "token", "secret", "password", "resolvedKey", "keyRef", + "accessToken", + "access_token", + "privateKey", + "private_key", + "secretKey", + "secret_key", + "refreshToken", + "refresh_token", + "bearer", + "authorization", ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.ts` around lines 553 - 561, Update the CREDENTIAL_FIELDS Set used in migration-state.ts to include additional common credential field names for defense-in-depth: add accessToken and access_token, privateKey and private_key, secretKey and secret_key, refreshToken and refresh_token, bearer, authorization, credentials (and any camelCase/snake_case variants you expect). Modify the CREDENTIAL_FIELDS constant so these additional identifiers are present alongside the existing entries to ensure the denylist catches more potential secret fields.
627-641: Consider logging failed file removals for observability.The empty catch block silently swallows all errors. While individual failures may be non-fatal, completely suppressing them could hide situations where credential files weren't removed due to permission issues—undermining the security intent.
🔧 Suggested improvement
-function walkAndRemoveFile(dirPath: string, targetName: string): void { +function walkAndRemoveFile(dirPath: string, targetName: string, logger?: PluginLogger): void { for (const entry of readdirSync(dirPath)) { const fullPath = path.join(dirPath, entry); try { const stat = lstatSync(fullPath); if (stat.isDirectory()) { - walkAndRemoveFile(fullPath, targetName); + walkAndRemoveFile(fullPath, targetName, logger); } else if (entry === targetName) { rmSync(fullPath, { force: true }); } - } catch { - // Non-fatal: skip files that disappeared or lack permissions + } catch (err: unknown) { + // Non-fatal but worth logging for security audit + logger?.warn?.(`Failed to process ${fullPath}: ${err instanceof Error ? err.message : String(err)}`); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.ts` around lines 627 - 641, The empty catch in walkAndRemoveFile silently swallows errors; change it to capture the exception and log a warning/error including the fullPath and error details so failed removals are observable (use the module's logger if available, e.g., logger.error/ warn, otherwise console.error). Keep behavior non-fatal (do not rethrow), but ensure the log message names the function walkAndRemoveFile and includes dirPath/targetName context and the thrown error to aid debugging and auditing.test/security-credential-exposure.test.js (1)
130-147: Test duplicates implementation logic instead of testing actual functions.This test re-implements
CREDENTIAL_FIELDSandstripCredentialsrather than importing them from the source module. This means:
- If the implementation changes, this test won't catch regressions
- The test has subtly different logic—Line 140 only strips strings/objects, while the source (line 576 in migration-state.ts) strips all value types unconditionally
Consider importing and testing the actual exported functions, or at minimum, ensure the test logic matches the source exactly.
🔧 Align test logic with source or import actual functions
If keeping inline logic, match the source exactly:
for (const [key, value] of Object.entries(obj)) { - if (CREDENTIAL_FIELDS.has(key) && (typeof value === "string" || typeof value === "object")) { + if (CREDENTIAL_FIELDS.has(key)) { result[key] = "[STRIPPED_BY_MIGRATION]"; } else { result[key] = stripCredentials(value); } }Alternatively, if
stripCredentialsandsanitizeCredentialsInBundlewere exported, the test could import and exercise them directly for better regression coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/security-credential-exposure.test.js` around lines 130 - 147, The test re-implements CREDENTIAL_FIELDS and stripCredentials which can drift from the source; update the test to either import the canonical definitions (e.g., import CREDENTIAL_FIELDS, stripCredentials or sanitizeCredentialsInBundle from migration-state.ts) and exercise those exports, or change the inline stripCredentials to match the source behavior exactly by stripping credential keys for all value types unconditionally (remove the current typeof check that limits stripping to strings/objects) so test behavior matches the real implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 553-561: Update the CREDENTIAL_FIELDS Set used in
migration-state.ts to include additional common credential field names for
defense-in-depth: add accessToken and access_token, privateKey and private_key,
secretKey and secret_key, refreshToken and refresh_token, bearer, authorization,
credentials (and any camelCase/snake_case variants you expect). Modify the
CREDENTIAL_FIELDS constant so these additional identifiers are present alongside
the existing entries to ensure the denylist catches more potential secret
fields.
- Around line 627-641: The empty catch in walkAndRemoveFile silently swallows
errors; change it to capture the exception and log a warning/error including the
fullPath and error details so failed removals are observable (use the module's
logger if available, e.g., logger.error/ warn, otherwise console.error). Keep
behavior non-fatal (do not rethrow), but ensure the log message names the
function walkAndRemoveFile and includes dirPath/targetName context and the
thrown error to aid debugging and auditing.
In `@test/security-credential-exposure.test.js`:
- Around line 130-147: The test re-implements CREDENTIAL_FIELDS and
stripCredentials which can drift from the source; update the test to either
import the canonical definitions (e.g., import CREDENTIAL_FIELDS,
stripCredentials or sanitizeCredentialsInBundle from migration-state.ts) and
exercise those exports, or change the inline stripCredentials to match the
source behavior exactly by stripping credential keys for all value types
unconditionally (remove the current typeof check that limits stripping to
strings/objects) so test behavior matches the real implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5edec13d-48bd-4bac-bd86-ff284e7cd658
📒 Files selected for processing (2)
nemoclaw/src/commands/migration-state.tstest/security-credential-exposure.test.js
C-2: Dockerfile CHAT_UI_URL Python code injection
- Build-arg was interpolated directly into `python3 -c` source string
- A single-quote in the URL closes the Python literal, allowing
arbitrary code execution at image build time
- Fix: promote to ENV, read via os.environ['CHAT_UI_URL']
- Also fixes identical pattern for NEMOCLAW_MODEL
- 11 tests: PoC proving injection, fix verification, regression guards
C-3: Telegram & Discord always-on in baseline sandbox policy
- Both messaging APIs had no binaries: restriction in baseline
- Any sandbox process could POST to attacker-controlled bots/webhooks
- Fix: remove from baseline; opt-in presets already exist
- 7 tests: host deny-list, binaries coverage invariant, preset existence
C-4: Snapshot manifest path traversal (arbitrary host write)
- restoreSnapshotToHost() used manifest.stateDir/configPath as write
targets with no path validation
- Tampered snapshot.json could overwrite arbitrary host files
- Fix: validate paths within manifest.homeDir via isWithinRoot()
- 9 tests: PoC proving traversal, fix rejection, legitimate path success
Also updates audit report to mark C-1 as tracked by PR NVIDIA#156.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
10 parallel security scans across the entire NemoClaw codebase (~9,600 LoC) identified 3 CRITICAL, 25 HIGH, 28 MEDIUM, and 16 LOW severity findings. Critical findings (fixed in companion PRs): - C-2: CHAT_UI_URL Python code injection in Dockerfile - C-3: Telegram/Discord always-on in baseline policy (exfil channels) - C-4: Snapshot manifest path traversal (arbitrary host write) - C-1: Migration credential exposure (tracked by PR NVIDIA#156) Also adds DRAFT-*.md to .gitignore to prevent accidental disclosure.
There was a problem hiding this comment.
Pull request overview
This PR hardens the migration snapshot/bundle flow to prevent host credentials (API keys/tokens) from being copied into the sandbox, and adds regression tests demonstrating the prior exposure and the intended sanitization behavior.
Changes:
- Add bundle sanitization in
migration-state.tsto removeauth-profiles.jsonand strip credential-like fields fromopenclaw.json. - Sanitize external root snapshots by removing
auth-profiles.jsonbefore archiving/sending into the sandbox. - Add a security-focused test suite covering credential exposure and sanitization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
nemoclaw/src/commands/migration-state.ts |
Adds credential stripping/removal during bundle preparation and external root snapshotting. |
test/security-credential-exposure.test.js |
Adds tests demonstrating the credential exposure PoC and verifying sanitization outcomes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nemoclaw/src/commands/migration-state.ts (1)
553-561: Consider expanding credential field coverage.The allowlist covers common cases, but may miss other credential-bearing fields. Consider adding:
accessToken,access_token,refreshToken,refresh_tokenprivateKey,private_keyclientSecret,client_secretbearer,bearerTokencredentials,authAlternatively, consider a pattern-based approach (e.g., matching fields ending in
Key,Token,Secret) as a defense-in-depth measure, though this risks false positives.💡 Proposed expansion
const CREDENTIAL_FIELDS = new Set([ "apiKey", "api_key", "token", "secret", "password", "resolvedKey", "keyRef", + "accessToken", + "access_token", + "refreshToken", + "refresh_token", + "privateKey", + "private_key", + "clientSecret", + "client_secret", + "bearerToken", + "bearer_token", + "credentials", ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.ts` around lines 553 - 561, The CREDENTIAL_FIELDS Set in migration-state.ts is too narrow and may miss other sensitive keys; update the CREDENTIAL_FIELDS constant to include additional common names such as accessToken, access_token, refreshToken, refresh_token, privateKey, private_key, clientSecret, client_secret, bearer, bearerToken, credentials, and auth, and optionally add a defensive pattern-based check (e.g., regexp tests in the same module for field names ending with Key|Token|Secret|SecretKey) to catch variants—modify references that use CREDENTIAL_FIELDS (look for usages of the CREDENTIAL_FIELDS Set) to consult the expanded allowlist or the new pattern matcher so credential-bearing fields are consistently detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 612-615: sanitizeExternalRootSnapshot currently only deletes
auth-profiles.json; update it to also locate config files in the external root
(e.g., openclaw.json and any other bundle config JSONs) and apply the existing
stripCredentials(...) routine to those files before writing them back, while
keeping the walkAndRemoveFile(rootSnapshotDir, "auth-profiles.json") behavior;
locate sanitizeExternalRootSnapshot and call stripCredentials on each discovered
config file (read -> stripCredentials -> write) rather than only removing
auth-profiles.json so embedded credential fields are sanitized.
---
Nitpick comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 553-561: The CREDENTIAL_FIELDS Set in migration-state.ts is too
narrow and may miss other sensitive keys; update the CREDENTIAL_FIELDS constant
to include additional common names such as accessToken, access_token,
refreshToken, refresh_token, privateKey, private_key, clientSecret,
client_secret, bearer, bearerToken, credentials, and auth, and optionally add a
defensive pattern-based check (e.g., regexp tests in the same module for field
names ending with Key|Token|Secret|SecretKey) to catch variants—modify
references that use CREDENTIAL_FIELDS (look for usages of the CREDENTIAL_FIELDS
Set) to consult the expanded allowlist or the new pattern matcher so
credential-bearing fields are consistently detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dac7d3f9-cb1f-4599-ae10-aafcd9779ad7
📒 Files selected for processing (1)
nemoclaw/src/commands/migration-state.ts
Fixes a critical vulnerability where createSnapshotBundle() copies the entire ~/.openclaw directory — including auth-profiles.json with live API keys, GitHub PATs, and npm tokens — into the sandbox filesystem. A compromised agent can read these credentials and exfiltrate them. Fix: Two sanitization layers in migration-state.ts: - sanitizeCredentialsInBundle(): deletes auth-profiles.json from agents/ subtree, strips credential fields from openclaw.json - sanitizeExternalRootSnapshot(): strips auth-profiles.json from external root snapshots before archiving Note: Blueprint digest bypass fix (verify.ts/resolve.ts) was dropped from this PR — those modules were deleted upstream in PR NVIDIA#492 as dead code when the openclaw nemoclaw CLI commands were removed. Signed-off-by: Lucas Wang <lucas_wang@lucas-futures.com>
Signed-off-by: Lucas Wang <lucas_wang@lucas-futures.com>
- Move readdirSync inside try/catch to handle permission errors
- Add { force: true } to rmSync for race-condition safety
- Log warnings instead of silently swallowing removal failures
- Remove unused node:crypto import from test
Signed-off-by: Lucas Wang <lucas_wang@lucas-futures.com>
gn00295120
force-pushed
the
security/sandbox-credential-exposure-and-blueprint-bypass
branch
from
95af20d to
d3b47f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nemoclaw/src/commands/migration-state.ts (1)
612-615:⚠️ Potential issue | 🔴 CriticalReplace the filename sweep with config-aware external-root sanitization.
Line 614 only deletes
auth-profiles.json. That still leaves credential-bearingopenclaw.jsonor similar config files inside copiedagentDir/workspace/skillsExtraDirroots, so secrets can still reach the sandbox. It is also over-broad for arbitrary workspaces because any unrelated file with that name gets dropped. ReusestripCredentials()for known OpenClaw config files here and keep hard deletes narrowly scoped to OpenClaw-owned auth-profile files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/migration-state.ts` around lines 612 - 615, sanitizeExternalRootSnapshot currently just calls walkAndRemoveFile to delete "auth-profiles.json", which is both over-broad and misses other OpenClaw credential files; update sanitizeExternalRootSnapshot to iterate known OpenClaw config locations (e.g., agentDir, workspace, skillsExtraDir roots inside the provided rootSnapshotDir) and for each file call stripCredentials() to sanitize config content for known filenames (e.g., openclaw.json and any other OpenClaw config names), and only use walkAndRemoveFile narrowly for OpenClaw-owned auth-profile files; reference sanitizeExternalRootSnapshot, walkAndRemoveFile, and stripCredentials when making the change so credential-bearing configs are sanitized rather than broadly deleted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 627-646: The walkAndRemoveFile function currently swallows all FS
errors which can leave credentials unremoved; update it so any filesystem error
from readdirSync, lstatSync, or rmSync that is not an ENOENT is re-thrown (or
converted to an Error) so caller can abort; specifically inside
walkAndRemoveFile, replace the blanket console.warn on catch with logic that
ignores only err.code === 'ENOENT' and otherwise throws the error, and ensure
callers such as createSnapshotBundle propagate that exception to abort bundle
creation rather than returning success.
- Around line 553-561: CREDENTIAL_FIELDS currently includes "keyRef" but that
value is a runtime reference (metadata) not a secret; remove "keyRef" from the
CREDENTIAL_FIELDS Set so only actual secret fields like "resolvedKey" (and the
other keys) are stripped during migration-state processing; update any related
logic that relies on CREDENTIAL_FIELDS (e.g., code that strips credentials) to
preserve keyRef so credential injection can restore secrets at runtime.
---
Duplicate comments:
In `@nemoclaw/src/commands/migration-state.ts`:
- Around line 612-615: sanitizeExternalRootSnapshot currently just calls
walkAndRemoveFile to delete "auth-profiles.json", which is both over-broad and
misses other OpenClaw credential files; update sanitizeExternalRootSnapshot to
iterate known OpenClaw config locations (e.g., agentDir, workspace,
skillsExtraDir roots inside the provided rootSnapshotDir) and for each file call
stripCredentials() to sanitize config content for known filenames (e.g.,
openclaw.json and any other OpenClaw config names), and only use
walkAndRemoveFile narrowly for OpenClaw-owned auth-profile files; reference
sanitizeExternalRootSnapshot, walkAndRemoveFile, and stripCredentials when
making the change so credential-bearing configs are sanitized rather than
broadly deleted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 764deb5a-3d9a-4add-a86f-0f9028927734
📒 Files selected for processing (3)
commitlint.config.jsnemoclaw/src/commands/migration-state.tstest/security-credential-exposure.test.js
✅ Files skipped from review due to trivial changes (2)
- commitlint.config.js
- test/security-credential-exposure.test.js
…hots sanitizeExternalRootSnapshot only removed auth-profiles.json but did not strip credential fields from openclaw.json files that may exist in external roots (agentDir, workspace, skills). Add walkAndStripCredentials to apply the same field-level sanitization used for the main bundle. Signed-off-by: Lucas Wang <lucas_wang@lucas-futures.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
gn00295120
force-pushed
the
security/sandbox-credential-exposure-and-blueprint-bypass
branch
from
03c80da to
612a857
Compare
…symlink guards - Remove keyRef from CREDENTIAL_FIELDS (metadata, not a secret) - Add CREDENTIAL_FIELD_PATTERN for broader credential detection (accessToken, refreshToken, privateKey, clientSecret, etc.) - Use JSON5.parse instead of JSON.parse for openclaw.json consistency - Add symlink guards in walkAndStripCredentials and walkAndRemoveFile to prevent modifying/deleting files outside the snapshot boundary - Rewrite tests to align with production logic, add pattern matching, symlink safety and value-type coverage tests
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Minor: three missing docstrings are causing the lint (docstring coverage) check to fail. Suggestions below should fix it.
| } | ||
| for (const entry of entries) { | ||
| const fullPath = path.join(dirPath, entry); | ||
| try { |
There was a problem hiding this comment.
Missing docstring — causing the docstring coverage lint failure.
| try { | |
| /** Recursively walk dirPath and remove any files matching targetName. */ | |
| function walkAndRemoveFile(dirPath: string, targetName: string): void { |
| const stateDir = path.join(tmpDir, ".openclaw"); | ||
| fs.mkdirSync(stateDir, { recursive: true }); | ||
|
|
||
| const config = { |
There was a problem hiding this comment.
Missing docstring — contributes to the docstring coverage lint failure.
| const config = { | |
| /** Create a mock ~/.openclaw directory tree populated with fake credential files. */ | |
| function createMockOpenClawHome(tmpDir) { |
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Missing docstring — contributes to the docstring coverage lint failure.
| } | |
| /** Local reimplementation of stripCredentials for test isolation. */ | |
| function stripCredentials(obj) { |
Address PR review feedback from cv: add JSDoc comments to walkAndRemoveFile, createMockOpenClawHome, and stripCredentials.
…print digest 1. Filter auth-profiles.json from snapshot bundles during createSnapshotBundle() using cpSync's filter option to exclude credential-sensitive basenames. 2. Strip gateway config (contains auth tokens) from sandbox openclaw.json in prepareSandboxState() — sandbox entrypoint regenerates it at startup. 3. Add blueprint digest verification: record SHA-256 of blueprint.yaml in SnapshotManifest at snapshot time, validate on restore. Empty/missing digest is a hard failure; old snapshots without the field skip validation for backward compatibility. Bump SNAPSHOT_VERSION 2→3 for the new manifest field. Closes #156
|
This overlaps significantly with #743 (ericksoa) but takes a different approach:
Neither is a strict superset of the other. #743 misses credential fields that aren't These should be reconciled — either merge the pattern-based stripping from #156 into #743, or coordinate so they don't conflict. Both modify |
… blueprint digest Reconciles NVIDIA#156 and NVIDIA#743 into a single comprehensive solution: - Filter auth-profiles.json at copy time via cpSync filter (from NVIDIA#743) - Recursive stripCredentials() with pattern-based field detection for deep config sanitization (from NVIDIA#156: CREDENTIAL_FIELDS set + CREDENTIAL_FIELD_PATTERN regex) - Remove gateway config section (contains auth tokens) from sandbox openclaw.json - Blueprint digest verification (SHA-256): recorded at snapshot time, validated on restore, empty/missing digest is a hard failure - Backward compatible: old snapshots without blueprintDigest skip validation - Bump SNAPSHOT_VERSION 2 → 3 Attack chain (now broken at multiple points): Telegram message → shell injection → read auth-profiles.json ↑ BLOCKED: filtered at copy Telegram message → shell injection → read openclaw.json credentials ↑ BLOCKED: stripped by pattern Blueprint tampering → digest: "" → verification passes ↑ BLOCKED: hard failure Supersedes NVIDIA#156 and NVIDIA#743.
|
Thanks for the analysis — agreed that neither is a strict superset. I've opened #769 which reconciles both approaches into a single PR:
53 unit tests (41 existing + 12 new), 319/319 full suite pass. → #769 |
Summary
Fixes two critical vulnerabilities that together enable a one-message supply chain attack: a single Telegram message can steal all host credentials from the sandbox and use them to push backdoors to GitHub and npm.
Vulnerability 1 (CRITICAL): Migration copies all host credentials into sandbox
createSnapshotBundle()inmigration-state.tscallscpSync(hostState.stateDir, ...)which copies the entire~/.openclawdirectory — includingauth-profiles.jsonwith live API keys, GitHub PATs, and npm tokens — verbatim into the sandbox filesystem.A compromised agent (e.g., via the Telegram bridge injection in #118) can read
/sandbox/.openclaw/agents/main/agent/auth-profiles.jsonand exfiltrate all credentials viaapi.telegram.org(which is allowed in the baseline network policy).Fix: Two sanitization layers:
prepareSandboxState()→sanitizeCredentialsInBundle(): deletesauth-profiles.jsonfromagents/subtree, strips credential fields fromopenclaw.jsoncreateSnapshotBundle()→sanitizeExternalRootSnapshot(): stripsauth-profiles.jsonfrom external root snapshots (agentDir, workspace, skills) before archivingVulnerability 2 (HIGH): Blueprint integrity verification silently bypassed
verifyBlueprintDigest()inverify.tschecksif (manifest.digest && ...)— butblueprint.yamlships withdigest: ""which is falsy in JavaScript. The entire integrity check is silently skipped.Fix: Empty/missing digest is now a hard verification failure. Also fixed
parseManifestHeaderinresolve.tsto properly strip YAML quotes and inline comments from the digest field viacleanYamlValue().Changes by file
migration-state.tsstripCredentials(),sanitizeCredentialsInBundle(),sanitizeExternalRootSnapshot(),removeAuthProfileFiles()verify.tsresolve.tscleanYamlValue()— strips YAML quotes and inline comments from manifest fieldstest/security-credential-exposure.test.jsAttack chain (now broken at two points)
Related PRs
run()→runArgv())dangerouslyDisableDeviceAuth(different file, no conflict)Test plan
openclaw nemoclaw migratecompletes with sanitized bundleNote on breaking change
The shipped
blueprint.yamlhasdigest: "". After this change, workflows using that blueprint will fail verification until the digest is populated at release time. This is intentional — running with an unverified blueprint should be an explicit operator decision, not a silent default.Summary by CodeRabbit
Security
Tests
Chores