-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Changes from 4 commits
e03c9a9
e24b6ce
c29c2ae
612a857
41b182f
4b1dc9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ module.exports = { | |
| "test", | ||
| "ci", | ||
| "perf", | ||
| "security", | ||
| ], | ||
| ], | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -544,6 +544,142 @@ | |||||||
| (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. | ||||||||
| */ | ||||||||
| const CREDENTIAL_FIELDS = new Set([ | ||||||||
| "apiKey", | ||||||||
| "api_key", | ||||||||
| "token", | ||||||||
| "secret", | ||||||||
| "password", | ||||||||
| "resolvedKey", | ||||||||
| "keyRef", | ||||||||
| ]); | ||||||||
|
|
||||||||
| /** | ||||||||
| * 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. | ||||||||
| */ | ||||||||
| 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 (CREDENTIAL_FIELDS.has(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 = JSON.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); | ||||||||
| if (stat.isDirectory()) { | ||||||||
| walkAndStripCredentials(fullPath, targetName); | ||||||||
| } else if (entry === targetName) { | ||||||||
| const raw = readFileSync(fullPath, "utf-8"); | ||||||||
| const config = JSON.parse(raw) as Record<string, unknown>; | ||||||||
gn00295120 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| 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"); | ||||||||
| } | ||||||||
|
|
||||||||
| 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); | ||||||||
| 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 +696,13 @@ | |||||||
| } | ||||||||
|
|
||||||||
| 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 +740,9 @@ | |||||||
| 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), | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,176 @@ | ||||||||
| // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||||||||
| // SPDX-License-Identifier: Apache-2.0 | ||||||||
| // | ||||||||
| // Security tests for migration credential sanitization. | ||||||||
| // Demonstrates the credential exposure vulnerability and verifies the fix. | ||||||||
| // | ||||||||
| // Note: Blueprint digest tests were removed — the verify.ts and resolve.ts | ||||||||
| // modules were deleted upstream in PR #492 (CLI commands removed). | ||||||||
gn00295120 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| const { describe, it } = require("node:test"); | ||||||||
| const assert = require("node:assert/strict"); | ||||||||
| const fs = require("node:fs"); | ||||||||
| const path = require("node:path"); | ||||||||
| const os = require("node:os"); | ||||||||
|
|
||||||||
| // Deliberately non-matching fake tokens that will NOT trigger secret scanners. | ||||||||
| // These do NOT follow real token formats (no "ghp_", "nvapi-", "npm_" prefixes). | ||||||||
| const FAKE_NVIDIA_KEY = "test-fake-nvidia-key-0000000000000000"; | ||||||||
| const FAKE_GITHUB_TOKEN = "test-fake-github-token-1111111111111111"; | ||||||||
| const FAKE_NPM_TOKEN = "test-fake-npm-token-2222222222222222"; | ||||||||
| const FAKE_GATEWAY_TOKEN = "test-fake-gateway-token-333333333333"; | ||||||||
|
|
||||||||
| // ═══════════════════════════════════════════════════════════════════ | ||||||||
| // Helper: create a mock ~/.openclaw directory with credential files | ||||||||
| // ═══════════════════════════════════════════════════════════════════ | ||||||||
| function createMockOpenClawHome(tmpDir) { | ||||||||
| const stateDir = path.join(tmpDir, ".openclaw"); | ||||||||
| fs.mkdirSync(stateDir, { recursive: true }); | ||||||||
|
|
||||||||
| const config = { | ||||||||
|
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 — contributes to the docstring coverage lint failure.
Suggested change
|
||||||||
| agents: { | ||||||||
| defaults: { | ||||||||
| model: { primary: "nvidia/nemotron-3-super-120b-a12b" }, | ||||||||
| workspace: path.join(stateDir, "workspace"), | ||||||||
| }, | ||||||||
| }, | ||||||||
| gateway: { | ||||||||
| mode: "local", | ||||||||
| auth: { token: FAKE_GATEWAY_TOKEN }, | ||||||||
| }, | ||||||||
| nvidia: { apiKey: FAKE_NVIDIA_KEY }, | ||||||||
| }; | ||||||||
| fs.writeFileSync( | ||||||||
| path.join(stateDir, "openclaw.json"), | ||||||||
| JSON.stringify(config, null, 2), | ||||||||
| ); | ||||||||
|
|
||||||||
| const authDir = path.join(stateDir, "agents", "main", "agent"); | ||||||||
| fs.mkdirSync(authDir, { recursive: true }); | ||||||||
| const authProfiles = { | ||||||||
| "nvidia:manual": { | ||||||||
| type: "api_key", | ||||||||
| provider: "nvidia", | ||||||||
| keyRef: { source: "env", id: "NVIDIA_API_KEY" }, | ||||||||
| resolvedKey: FAKE_NVIDIA_KEY, | ||||||||
| profileId: "nvidia:manual", | ||||||||
| }, | ||||||||
| "github:pat": { | ||||||||
| type: "api_key", | ||||||||
| provider: "github", | ||||||||
| token: FAKE_GITHUB_TOKEN, | ||||||||
| profileId: "github:pat", | ||||||||
| }, | ||||||||
| "npm:publish": { | ||||||||
| type: "api_key", | ||||||||
| provider: "npm", | ||||||||
| token: FAKE_NPM_TOKEN, | ||||||||
| profileId: "npm:publish", | ||||||||
| }, | ||||||||
| }; | ||||||||
| fs.writeFileSync( | ||||||||
| path.join(authDir, "auth-profiles.json"), | ||||||||
| JSON.stringify(authProfiles, null, 2), | ||||||||
| ); | ||||||||
|
|
||||||||
| const workspace = path.join(stateDir, "workspace"); | ||||||||
| fs.mkdirSync(workspace, { recursive: true }); | ||||||||
| fs.writeFileSync(path.join(workspace, "project.md"), "# My Project\n"); | ||||||||
|
|
||||||||
| return { stateDir, config, authProfiles }; | ||||||||
| } | ||||||||
|
|
||||||||
| // ═══════════════════════════════════════════════════════════════════ | ||||||||
| // 1. Migration copies ALL credentials into sandbox (demonstrates vuln) | ||||||||
| // ═══════════════════════════════════════════════════════════════════ | ||||||||
| describe("Migration credential exposure (pre-fix behavior)", () => { | ||||||||
| it("raw cpSync copies auth-profiles.json with all tokens into sandbox", () => { | ||||||||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-poc-")); | ||||||||
| try { | ||||||||
| const mock = createMockOpenClawHome(tmpDir); | ||||||||
|
|
||||||||
| // Simulate the vulnerable codepath: cpSync(stateDir, snapshotDir) | ||||||||
| const snapshotDir = path.join(tmpDir, "snapshot", "openclaw"); | ||||||||
| fs.cpSync(mock.stateDir, snapshotDir, { recursive: true }); | ||||||||
|
|
||||||||
| const authPath = path.join(snapshotDir, "agents", "main", "agent", "auth-profiles.json"); | ||||||||
| const configPath = path.join(snapshotDir, "openclaw.json"); | ||||||||
|
|
||||||||
| assert.ok(fs.existsSync(authPath), "auth-profiles.json copied into sandbox"); | ||||||||
| assert.ok(fs.existsSync(configPath), "openclaw.json copied into sandbox"); | ||||||||
|
|
||||||||
| const stolenAuth = JSON.parse(fs.readFileSync(authPath, "utf-8")); | ||||||||
| const stolenConfig = JSON.parse(fs.readFileSync(configPath, "utf-8")); | ||||||||
|
|
||||||||
| // All tokens are fully readable — this is the vulnerability | ||||||||
| assert.strictEqual(stolenAuth["nvidia:manual"].resolvedKey, FAKE_NVIDIA_KEY); | ||||||||
| assert.strictEqual(stolenAuth["github:pat"].token, FAKE_GITHUB_TOKEN); | ||||||||
| assert.strictEqual(stolenAuth["npm:publish"].token, FAKE_NPM_TOKEN); | ||||||||
| assert.strictEqual(stolenConfig.gateway.auth.token, FAKE_GATEWAY_TOKEN); | ||||||||
| } finally { | ||||||||
| fs.rmSync(tmpDir, { recursive: true, force: true }); | ||||||||
| } | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
gn00295120 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| // ═══════════════════════════════════════════════════════════════════ | ||||||||
| // 2. Fix verification — sanitized migration blocks the attack chain | ||||||||
| // ═══════════════════════════════════════════════════════════════════ | ||||||||
| describe("Fix verification: sanitized migration blocks credential theft", () => { | ||||||||
| it("sanitizeCredentialsInBundle deletes auth-profiles.json and strips config tokens", () => { | ||||||||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-fix-")); | ||||||||
| try { | ||||||||
| const mock = createMockOpenClawHome(tmpDir); | ||||||||
|
|
||||||||
| // Simulate snapshot + prepare | ||||||||
| const bundleDir = path.join(tmpDir, "bundle", "openclaw"); | ||||||||
| fs.cpSync(mock.stateDir, bundleDir, { recursive: true }); | ||||||||
|
|
||||||||
| // Apply the same sanitization logic from the fix in migration-state.ts | ||||||||
| const CREDENTIAL_FIELDS = new Set([ | ||||||||
| "apiKey", "api_key", "token", "secret", "password", "resolvedKey", "keyRef", | ||||||||
| ]); | ||||||||
|
|
||||||||
| function stripCredentials(obj) { | ||||||||
| if (obj === null || obj === undefined) return obj; | ||||||||
| if (typeof obj !== "object") return obj; | ||||||||
| if (Array.isArray(obj)) return obj.map(stripCredentials); | ||||||||
| const result = {}; | ||||||||
| for (const [key, value] of Object.entries(obj)) { | ||||||||
| if (CREDENTIAL_FIELDS.has(key) && (typeof value === "string" || typeof value === "object")) { | ||||||||
| result[key] = "[STRIPPED_BY_MIGRATION]"; | ||||||||
| } else { | ||||||||
| result[key] = stripCredentials(value); | ||||||||
| } | ||||||||
| } | ||||||||
gn00295120 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| return result; | ||||||||
| } | ||||||||
gn00295120 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
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 — contributes to the docstring coverage lint failure.
Suggested change
|
||||||||
|
|
||||||||
| // Delete auth-profiles.json | ||||||||
| const authPath = path.join(bundleDir, "agents", "main", "agent", "auth-profiles.json"); | ||||||||
| if (fs.existsSync(authPath)) fs.rmSync(authPath, { force: true }); | ||||||||
|
|
||||||||
| // Strip config credentials | ||||||||
| const configPath = path.join(bundleDir, "openclaw.json"); | ||||||||
| const config = JSON.parse(fs.readFileSync(configPath, "utf-8")); | ||||||||
| fs.writeFileSync(configPath, JSON.stringify(stripCredentials(config), null, 2)); | ||||||||
|
|
||||||||
gn00295120 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
gn00295120 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| // Verify: auth-profiles.json deleted | ||||||||
| assert.ok(!fs.existsSync(authPath), "auth-profiles.json must be deleted"); | ||||||||
|
|
||||||||
| // Verify: config credentials stripped | ||||||||
| const sanitized = JSON.parse(fs.readFileSync(configPath, "utf-8")); | ||||||||
| assert.strictEqual(sanitized.nvidia.apiKey, "[STRIPPED_BY_MIGRATION]"); | ||||||||
| assert.strictEqual(sanitized.gateway.auth.token, "[STRIPPED_BY_MIGRATION]"); | ||||||||
|
|
||||||||
| // Verify: non-credential fields preserved | ||||||||
| assert.strictEqual(sanitized.agents.defaults.model.primary, "nvidia/nemotron-3-super-120b-a12b"); | ||||||||
| assert.strictEqual(sanitized.gateway.mode, "local"); | ||||||||
|
|
||||||||
| // Verify: workspace files untouched | ||||||||
| assert.ok(fs.existsSync(path.join(bundleDir, "workspace", "project.md"))); | ||||||||
| } finally { | ||||||||
| fs.rmSync(tmpDir, { recursive: true, force: true }); | ||||||||
| } | ||||||||
| }); | ||||||||
| }); | ||||||||
Uh oh!
There was an error while loading. Please reload this page.