-
Notifications
You must be signed in to change notification settings - Fork 2.1k
security: strip credentials from migration snapshots and enforce blueprint digest #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
gn00295120
wants to merge
6
commits into
NVIDIA:main
from
gn00295120:security/sandbox-credential-exposure-and-blueprint-bypass
+401
−0
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
e03c9a9
security: strip credentials from migration snapshots
gn00295120 e24b6ce
chore: add security to commitlint allowed types
gn00295120 c29c2ae
fix: harden walkAndRemoveFile error handling in credential sanitization
gn00295120 612a857
fix: strip credential fields from config files in external root snaps…
gn00295120 41b182f
fix: harden credential sanitization with pattern matching, JSON5 and …
gn00295120 4b1dc9c
fix: add missing docstrings to resolve lint coverage failure
gn00295120 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ module.exports = { | |
| "test", | ||
| "ci", | ||
| "perf", | ||
| "security", | ||
| ], | ||
| ], | ||
| }, | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -544,6 +544,162 @@ function setConfigValue( | |||||||
| (current as Record<string, unknown>)[finalToken] = value; | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Credential field names that MUST be stripped from config and auth files | ||||||||
| * before they are sent into the sandbox. Credentials should be injected | ||||||||
| * at runtime via OpenShell's provider credential mechanism, not baked | ||||||||
| * into the sandbox filesystem. | ||||||||
| */ | ||||||||
| // keyRef is metadata (points to env var source), not a secret value — excluded intentionally. | ||||||||
| const CREDENTIAL_FIELDS = new Set([ | ||||||||
| "apiKey", | ||||||||
| "api_key", | ||||||||
| "token", | ||||||||
| "secret", | ||||||||
| "password", | ||||||||
| "resolvedKey", | ||||||||
| ]); | ||||||||
|
|
||||||||
| /** | ||||||||
| * Pattern-based detection for credential field names not covered by the | ||||||||
| * explicit set above. Matches common suffixes like accessToken, privateKey, | ||||||||
| * clientSecret, etc. | ||||||||
| */ | ||||||||
| const CREDENTIAL_FIELD_PATTERN = | ||||||||
| /(?:access|refresh|client|bearer|auth|api|private|public|signing|session)(?:Token|Key|Secret|Password)$/; | ||||||||
|
|
||||||||
| /** | ||||||||
| * Check whether a JSON key is a credential field that must be stripped. | ||||||||
| */ | ||||||||
| function isCredentialField(key: string): boolean { | ||||||||
| return CREDENTIAL_FIELDS.has(key) || CREDENTIAL_FIELD_PATTERN.test(key); | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Recursively strip credential fields from a JSON-like object. | ||||||||
| * Returns a new object with sensitive values replaced by a placeholder. | ||||||||
| * Any value type (string, object, boolean, number, null) is stripped if | ||||||||
| * the key matches CREDENTIAL_FIELDS or CREDENTIAL_FIELD_PATTERN. | ||||||||
| */ | ||||||||
| function stripCredentials(obj: unknown): unknown { | ||||||||
| if (obj === null || obj === undefined) return obj; | ||||||||
| if (typeof obj !== "object") return obj; | ||||||||
| if (Array.isArray(obj)) return obj.map(stripCredentials); | ||||||||
|
|
||||||||
| const result: Record<string, unknown> = {}; | ||||||||
| for (const [key, value] of Object.entries(obj as Record<string, unknown>)) { | ||||||||
| if (isCredentialField(key)) { | ||||||||
| result[key] = "[STRIPPED_BY_MIGRATION]"; | ||||||||
| } else { | ||||||||
| result[key] = stripCredentials(value); | ||||||||
| } | ||||||||
gn00295120 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| } | ||||||||
| return result; | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Remove auth-profiles.json from the agents/ subtree and strip credential | ||||||||
| * fields from openclaw.json inside the prepared sandbox state directory. | ||||||||
| * | ||||||||
| * Note: when hasExternalConfig is true, prepareSandboxState has already | ||||||||
| * merged the external config into openclaw.json — so stripping that file | ||||||||
| * covers both the inline and external config cases. | ||||||||
| */ | ||||||||
| function sanitizeCredentialsInBundle(preparedStateDir: string): void { | ||||||||
| // Remove auth-profiles.json files from agents/ subtree | ||||||||
| removeAuthProfileFiles(preparedStateDir); | ||||||||
|
|
||||||||
| // Strip credential fields from openclaw.json | ||||||||
| const configPath = path.join(preparedStateDir, "openclaw.json"); | ||||||||
| if (existsSync(configPath)) { | ||||||||
| const raw = readFileSync(configPath, "utf-8"); | ||||||||
| const config = JSON5.parse(raw) as Record<string, unknown>; | ||||||||
| const sanitized = stripCredentials(config) as Record<string, unknown>; | ||||||||
| writeFileSync(configPath, JSON.stringify(sanitized, null, 2)); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Sanitize a snapshot directory for an external root (agentDir, workspace, | ||||||||
| * skills) before it is archived and sent into the sandbox. External roots | ||||||||
| * may contain their own auth-profiles.json or credential files. | ||||||||
| */ | ||||||||
| function sanitizeExternalRootSnapshot(rootSnapshotDir: string): void { | ||||||||
| // Remove auth-profiles.json anywhere in the external root | ||||||||
| walkAndRemoveFile(rootSnapshotDir, "auth-profiles.json"); | ||||||||
|
|
||||||||
| // Strip credential fields from any openclaw.json found in the external root | ||||||||
| walkAndStripCredentials(rootSnapshotDir, "openclaw.json"); | ||||||||
gn00295120 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| } | ||||||||
|
|
||||||||
| /** | ||||||||
| * Recursively find files matching targetName and strip credential fields | ||||||||
| * from their JSON content. | ||||||||
| */ | ||||||||
| function walkAndStripCredentials(dirPath: string, targetName: string): void { | ||||||||
| let entries: string[]; | ||||||||
| try { | ||||||||
| entries = readdirSync(dirPath); | ||||||||
| } catch (err) { | ||||||||
| console.warn(`[credential-sanitize] Unable to read directory ${dirPath}: ${err}`); | ||||||||
| return; | ||||||||
| } | ||||||||
| for (const entry of entries) { | ||||||||
| const fullPath = path.join(dirPath, entry); | ||||||||
| try { | ||||||||
| const stat = lstatSync(fullPath); | ||||||||
| // Skip symlinks — only sanitize real files within the snapshot | ||||||||
| if (stat.isSymbolicLink()) continue; | ||||||||
| if (stat.isDirectory()) { | ||||||||
| walkAndStripCredentials(fullPath, targetName); | ||||||||
| } else if (entry === targetName) { | ||||||||
| const raw = readFileSync(fullPath, "utf-8"); | ||||||||
| const config = JSON5.parse(raw) as Record<string, unknown>; | ||||||||
| const sanitized = stripCredentials(config) as Record<string, unknown>; | ||||||||
| writeFileSync(fullPath, JSON.stringify(sanitized, null, 2)); | ||||||||
| } | ||||||||
| } catch (err) { | ||||||||
| console.warn(`[credential-sanitize] Unable to process ${fullPath}: ${err}`); | ||||||||
gn00295120 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| /** | ||||||||
| * Remove auth-profiles.json files from known OpenClaw credential locations. | ||||||||
| * Scoped to the agents/ subtree to avoid traversing large workspace directories. | ||||||||
| */ | ||||||||
| function removeAuthProfileFiles(preparedStateDir: string): void { | ||||||||
| const agentsDir = path.join(preparedStateDir, "agents"); | ||||||||
| if (!existsSync(agentsDir)) return; | ||||||||
| walkAndRemoveFile(agentsDir, "auth-profiles.json"); | ||||||||
| } | ||||||||
|
|
||||||||
| /** Recursively walk dirPath and remove any files matching targetName. */ | ||||||||
| function walkAndRemoveFile(dirPath: string, targetName: string): void { | ||||||||
| let entries: string[]; | ||||||||
| try { | ||||||||
| entries = readdirSync(dirPath); | ||||||||
| } catch (err) { | ||||||||
| console.warn(`[credential-sanitize] Unable to read directory ${dirPath}: ${err}`); | ||||||||
| return; | ||||||||
| } | ||||||||
| for (const entry of entries) { | ||||||||
| const fullPath = path.join(dirPath, entry); | ||||||||
| try { | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing docstring — causing the docstring coverage lint failure.
Suggested change
|
||||||||
| const stat = lstatSync(fullPath); | ||||||||
| // Skip symlinks — only operate on real files within the snapshot | ||||||||
| if (stat.isSymbolicLink()) continue; | ||||||||
| if (stat.isDirectory()) { | ||||||||
| walkAndRemoveFile(fullPath, targetName); | ||||||||
| } else if (entry === targetName) { | ||||||||
| rmSync(fullPath, { force: true }); | ||||||||
| } | ||||||||
| } catch (err) { | ||||||||
| console.warn(`[credential-sanitize] Unable to process ${fullPath}: ${err}`); | ||||||||
| } | ||||||||
gn00295120 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| } | ||||||||
gn00295120 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| } | ||||||||
|
|
||||||||
| function prepareSandboxState(snapshotDir: string, manifest: SnapshotManifest): string { | ||||||||
| const preparedStateDir = path.join(snapshotDir, "sandbox-bundle", "openclaw"); | ||||||||
| rmSync(preparedStateDir, { recursive: true, force: true }); | ||||||||
|
|
@@ -560,6 +716,13 @@ function prepareSandboxState(snapshotDir: string, manifest: SnapshotManifest): s | |||||||
| } | ||||||||
|
|
||||||||
| writeFileSync(path.join(preparedStateDir, "openclaw.json"), JSON.stringify(config, null, 2)); | ||||||||
|
|
||||||||
| // SECURITY: Strip all credentials from the bundle before it enters the sandbox. | ||||||||
| // Credentials must be injected at runtime via OpenShell's provider credential | ||||||||
| // mechanism, not baked into the sandbox filesystem where a compromised agent | ||||||||
| // can read them. | ||||||||
| sanitizeCredentialsInBundle(preparedStateDir); | ||||||||
|
|
||||||||
| return preparedStateDir; | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -597,6 +760,9 @@ export function createSnapshotBundle( | |||||||
| const destination = path.join(parentDir, root.snapshotRelativePath); | ||||||||
| mkdirSync(path.dirname(destination), { recursive: true }); | ||||||||
| copyDirectory(root.sourcePath, destination); | ||||||||
| // SECURITY: strip credential files from external root snapshots | ||||||||
| // before they are archived and sent into the sandbox. | ||||||||
| sanitizeExternalRootSnapshot(destination); | ||||||||
| externalRoots.push({ | ||||||||
| ...root, | ||||||||
| symlinkPaths: collectSymlinkPaths(root.sourcePath), | ||||||||
|
|
||||||||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.