From 9b2dd96733e2ad256c04169babdc7e76c225e6c8 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Wed, 29 Jul 2026 02:56:10 +0530 Subject: [PATCH 01/19] fix(security): scrub migration and backup credentials consistently 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 --- nemoclaw/src/commands/migration-state.ts | 63 +------ .../src/security/credential-filter.test.ts | 81 ++++++++ nemoclaw/src/security/credential-filter.ts | 177 ++++++++++++++++++ src/lib/security/credential-filter.test.ts | 83 +++++++- src/lib/security/credential-filter.ts | 106 ++++++++++- src/lib/state/sandbox.ts | 27 ++- 6 files changed, 453 insertions(+), 84 deletions(-) create mode 100644 nemoclaw/src/security/credential-filter.test.ts create mode 100644 nemoclaw/src/security/credential-filter.ts diff --git a/nemoclaw/src/commands/migration-state.ts b/nemoclaw/src/commands/migration-state.ts index 43aab3d3d31..5f8b494402d 100644 --- a/nemoclaw/src/commands/migration-state.ts +++ b/nemoclaw/src/commands/migration-state.ts @@ -20,6 +20,7 @@ import { create as createTar } from "tar"; import { createHash } from "node:crypto"; import JSON5 from "json5"; import type { PluginLogger } from "../index.js"; +import { isSensitiveFile, stripCredentials } from "../security/credential-filter.js"; import { isObjectRecord, type UnknownRecord } from "../shared/object-record.js"; const SANDBOX_MIGRATION_DIR = "/sandbox/.nemoclaw/migration"; @@ -504,65 +505,9 @@ export function detectHostOpenClaw(env: NodeJS.ProcessEnv = process.env): HostOp } // --------------------------------------------------------------------------- -// Credential sanitization +// Credential sanitization (shared with nemoclaw/src/security/credential-filter) // --------------------------------------------------------------------------- -/** - * Basenames that MUST NOT be copied into snapshot bundles. - * These files contain credential references or session tokens - * that should never cross the sandbox boundary. - */ -const CREDENTIAL_SENSITIVE_BASENAMES = new Set(["auth-profiles.json"]); - -/** - * Credential field names that MUST be stripped from config files - * before they enter the sandbox. Credentials should be injected - * at runtime via OpenShell's provider credential mechanism. - */ -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)$/; - -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. - */ -function stripCredentials(obj: unknown): unknown { - if (Array.isArray(obj)) return obj.map(stripCredentials); - if (!isObjectRecord(obj)) return obj; - - return stripCredentialsFromRecord(obj); -} - -function stripCredentialsFromRecord(obj: UnknownRecord): UnknownRecord { - const result: UnknownRecord = {}; - for (const [key, value] of Object.entries(obj)) { - if (isCredentialField(key)) { - result[key] = "[STRIPPED_BY_MIGRATION]"; - } else { - result[key] = stripCredentials(value); - } - } - return result; -} - /** * Strip credential fields from openclaw.json and remove the gateway * config section (contains auth tokens — regenerated by sandbox entrypoint). @@ -571,7 +516,7 @@ function sanitizeConfigFile(configPath: string): void { const config = loadConfigDocument(configPath); if (!config) return; delete config.gateway; - const sanitized = stripCredentialsFromRecord(config); + const sanitized = stripCredentials(config) as UnknownRecord; writeFileSync(configPath, JSON.stringify(sanitized, null, 2)); chmodSync(configPath, 0o600); } @@ -593,7 +538,7 @@ function copyDirectory( cpSync(sourcePath, destinationPath, { recursive: true, filter: options?.stripCredentials - ? (source: string) => !CREDENTIAL_SENSITIVE_BASENAMES.has(path.basename(source).toLowerCase()) + ? (source: string) => !isSensitiveFile(path.basename(source)) : undefined, }); } diff --git a/nemoclaw/src/security/credential-filter.test.ts b/nemoclaw/src/security/credential-filter.test.ts new file mode 100644 index 00000000000..65e21f117b1 --- /dev/null +++ b/nemoclaw/src/security/credential-filter.test.ts @@ -0,0 +1,81 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { + CREDENTIAL_PLACEHOLDER, + isCredentialField, + isSafeCredentialPlaceholder, + isSensitiveFile, + stripCredentials, + valueLooksLikeSecret, +} from "./credential-filter.js"; + +describe("plugin credential-filter", () => { + it("treats Slack botToken, Authorization, and GITHUB_TOKEN as credential fields", () => { + expect(isCredentialField("botToken")).toBe(true); + expect(isCredentialField("appToken")).toBe(true); + expect(isCredentialField("Authorization")).toBe(true); + expect(isCredentialField("GITHUB_TOKEN")).toBe(true); + expect(isCredentialField("DB_PASS")).toBe(true); + expect(isCredentialField("publicKey")).toBe(false); + expect(isCredentialField("NODE_ENV")).toBe(false); + }); + + it("strips channel tokens, headers, env secrets, and CLI flag args", () => { + const result = stripCredentials({ + channels: { + slack: { + accounts: { + default: { + botToken: "xoxb-raw-slack-token", + appToken: "xapp-raw-app-token", + }, + }, + }, + }, + mcp: { + headers: { Authorization: "Bearer sk-abcdefghijklmnopqrstuvwxyz" }, + env: { GITHUB_TOKEN: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", NODE_ENV: "test" }, + args: ["--api-key", "opaque-secret-value", "--verbose"], + }, + model: "keep-me", + publicKey: "verify-me", + apiKey: "openshell:resolve:env:NVIDIA_API_KEY", + }) as Record; + + const channels = result.channels as { + slack: { accounts: { default: { botToken: string; appToken: string } } }; + }; + expect(channels.slack.accounts.default.botToken).toBe(CREDENTIAL_PLACEHOLDER); + expect(channels.slack.accounts.default.appToken).toBe(CREDENTIAL_PLACEHOLDER); + + const mcp = result.mcp as { + headers: { Authorization: string }; + env: { GITHUB_TOKEN: string; NODE_ENV: string }; + args: string[]; + }; + expect(mcp.headers.Authorization).toBe(CREDENTIAL_PLACEHOLDER); + expect(mcp.env.GITHUB_TOKEN).toBe(CREDENTIAL_PLACEHOLDER); + expect(mcp.env.NODE_ENV).toBe("test"); + expect(mcp.args).toEqual(["--api-key", CREDENTIAL_PLACEHOLDER, "--verbose"]); + expect(result.model).toBe("keep-me"); + expect(result.publicKey).toBe("verify-me"); + expect(result.apiKey).toBe("openshell:resolve:env:NVIDIA_API_KEY"); + }); + + it("preserves safe placeholders and detects secret-shaped values", () => { + expect(isSafeCredentialPlaceholder("unused")).toBe(true); + expect(isSafeCredentialPlaceholder("openshell:resolve:env:TOKEN")).toBe(true); + expect(valueLooksLikeSecret("sk-abcdefghijklmnopqrstuvwxyz")).toBe(true); + expect(valueLooksLikeSecret("not-a-secret")).toBe(false); + }); + + it("excludes auth state basenames from migration copies", () => { + expect(isSensitiveFile("auth-profiles.json")).toBe(true); + expect(isSensitiveFile("auth.json")).toBe(true); + expect(isSensitiveFile("chatgpt-auth.json")).toBe(true); + expect(isSensitiveFile("openclaw.json")).toBe(false); + }); +}); diff --git a/nemoclaw/src/security/credential-filter.ts b/nemoclaw/src/security/credential-filter.ts new file mode 100644 index 00000000000..dce343eda68 --- /dev/null +++ b/nemoclaw/src/security/credential-filter.ts @@ -0,0 +1,177 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 +// +// Credential stripping for host→sandbox migration snapshots. +// Kept in parity with src/lib/security/credential-filter.ts so migration +// cannot leave channel tokens, env secrets, or auth headers in the sandbox. + +import { isObjectRecord, type UnknownRecord } from "../shared/object-record.js"; + +export const CREDENTIAL_PLACEHOLDER = "[STRIPPED_BY_MIGRATION]"; + +/** + * Basenames that MUST NOT be copied into snapshot bundles. + */ +export const CREDENTIAL_SENSITIVE_BASENAMES = new Set([ + "auth-profiles.json", + "auth.json", + "chatgpt-auth.json", +]); + +const CREDENTIAL_FIELDS = new Set([ + "apiKey", + "api_key", + "token", + "secret", + "password", + "pass", + "passwd", + "resolvedKey", +]); + +const CREDENTIAL_FIELD_PATTERN = + /(?:access|refresh|client|bearer|auth|api|private|public|signing|session|bot|app)(?:Token|Key|Secret|Password)$/; + +const ENV_SECRET_FIELD_PATTERN = + /^(?:[A-Z0-9]+_)*(?:TOKEN|KEY|SECRET|PASSWORD|PASSWD|PASS|PASSPHRASE|CREDENTIAL)S?$/; + +const CREDENTIAL_HEADER_NAMES: ReadonlySet = new Set([ + "authorization", + "proxy-authorization", + "cookie", + "set-cookie", +]); + +const HEADER_CREDENTIAL_PATTERN = /-(?:key|token|secret|password|passphrase|credential|auth)s?$/i; + +const PUBLIC_KEY_FIELD_PATTERN = /(?:^|[-_])public[-_]?keys?$/i; + +const SAFE_CREDENTIAL_PLACEHOLDER_PATTERNS: readonly RegExp[] = [ + /^openshell:resolve:env:[A-Za-z0-9_]+$/, + /^Bearer\s+openshell:resolve:env:[A-Za-z0-9_]+$/i, + /^xoxb-OPENSHELL-RESOLVE-ENV-[A-Za-z0-9_]+$/, + /^xapp-OPENSHELL-RESOLVE-ENV-[A-Za-z0-9_]+$/, +]; + +const SAFE_CREDENTIAL_PLACEHOLDER_LITERALS: ReadonlySet = new Set([ + "unused", + CREDENTIAL_PLACEHOLDER, +]); + +/** High-confidence raw secret shapes used as a value-level backstop. */ +const VALUE_SECRET_PATTERNS: readonly RegExp[] = [ + /nvapi-[A-Za-z0-9_-]{10,}/, + /ghp_[A-Za-z0-9_-]{10,}/, + /sk-proj-[A-Za-z0-9_-]{10,}/, + /sk-ant-[A-Za-z0-9_-]{10,}/, + /sk-[A-Za-z0-9_-]{20,}/, + /(?:xox[bpas]|xapp)-[A-Za-z0-9-]{10,}/, + /A(?:K|S)IA[A-Z0-9]{16}/, + /hf_[A-Za-z0-9]{10,}/, + /tvly-[A-Za-z0-9_-]{10,}/, +]; + +function hasPassCredentialSegment(key: string): boolean { + const normalized = key + .replace(/([A-Z]+)([A-Z][a-z])/g, "$1_$2") + .replace(/([a-z0-9])([A-Z])/g, "$1_$2") + .replace(/[^A-Za-z0-9]+/g, "_") + .replace(/^_+|_+$/g, "") + .toLowerCase(); + return ( + normalized === "pass" || + normalized === "passwd" || + normalized.endsWith("_pass") || + normalized.endsWith("_passwd") + ); +} + +export function isCredentialField(key: string): boolean { + if (PUBLIC_KEY_FIELD_PATTERN.test(key)) return false; + return ( + CREDENTIAL_FIELDS.has(key) || + CREDENTIAL_FIELD_PATTERN.test(key) || + hasPassCredentialSegment(key) || + ENV_SECRET_FIELD_PATTERN.test(key) || + HEADER_CREDENTIAL_PATTERN.test(key) || + CREDENTIAL_HEADER_NAMES.has(key.toLowerCase()) + ); +} + +export function valueLooksLikeSecret(value: string): boolean { + return VALUE_SECRET_PATTERNS.some((pattern) => pattern.test(value)); +} + +export function isSafeCredentialPlaceholder(value: unknown): boolean { + if (typeof value !== "string") return false; + const withoutScheme = value.replace(/^Bearer\s+/i, ""); + if ( + SAFE_CREDENTIAL_PLACEHOLDER_LITERALS.has(value) || + SAFE_CREDENTIAL_PLACEHOLDER_LITERALS.has(withoutScheme) + ) { + return true; + } + return SAFE_CREDENTIAL_PLACEHOLDER_PATTERNS.some((pattern) => pattern.test(value)); +} + +function scrubConfigValue(value: unknown): unknown { + if (typeof value === "string") { + if (isSafeCredentialPlaceholder(value)) return value; + return valueLooksLikeSecret(value) ? CREDENTIAL_PLACEHOLDER : value; + } + return stripCredentials(value); +} + +function cliFlagName(token: string): string | null { + const match = /^--?([A-Za-z0-9][A-Za-z0-9._-]*)$/.exec(token); + return match ? match[1] : null; +} + +function scrubArrayElement(value: unknown, previous: unknown): unknown { + if (typeof value !== "string") return stripCredentials(value); + if (isSafeCredentialPlaceholder(value)) return value; + + const eq = value.indexOf("="); + if (eq > 0 && value.startsWith("-")) { + const flagName = cliFlagName(value.slice(0, eq)); + if (flagName && isCredentialField(flagName)) { + const inlineValue = value.slice(eq + 1); + return isSafeCredentialPlaceholder(inlineValue) + ? value + : `${value.slice(0, eq)}=${CREDENTIAL_PLACEHOLDER}`; + } + } + + if (!value.startsWith("-") && typeof previous === "string") { + const prevFlag = cliFlagName(previous); + if (prevFlag && isCredentialField(prevFlag)) return CREDENTIAL_PLACEHOLDER; + } + + return valueLooksLikeSecret(value) ? CREDENTIAL_PLACEHOLDER : value; +} + +/** + * Recursively strip credential fields from a JSON-like object. + */ +export 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((value, index) => scrubArrayElement(value, obj[index - 1])); + } + if (!isObjectRecord(obj)) return obj; + + const result: UnknownRecord = {}; + for (const [key, value] of Object.entries(obj)) { + if (isCredentialField(key)) { + result[key] = isSafeCredentialPlaceholder(value) ? value : CREDENTIAL_PLACEHOLDER; + } else { + result[key] = scrubConfigValue(value); + } + } + return result; +} + +export function isSensitiveFile(filename: string): boolean { + return CREDENTIAL_SENSITIVE_BASENAMES.has(filename.toLowerCase()); +} diff --git a/src/lib/security/credential-filter.test.ts b/src/lib/security/credential-filter.test.ts index d7a8f4005ba..60e96a5b459 100644 --- a/src/lib/security/credential-filter.test.ts +++ b/src/lib/security/credential-filter.test.ts @@ -11,6 +11,8 @@ import { isSafeCredentialPlaceholder, isSensitiveFile, sanitizeConfigFile, + sanitizeEnvFile, + sanitizeEnvFileContent, shouldScanSnapshotFileForCredentials, stripCredentials, } from "./credential-filter.js"; @@ -209,6 +211,82 @@ describe("sanitizeConfigFile", () => { expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ apiKey: "sk-secret" }); }); + + it("strips Hermes YAML credentials and removes gateway", () => { + const configPath = join(tmpDir, "config.yaml"); + writeFileSync( + configPath, + [ + "model: hermes", + "api_key: sk-hermes-secret-key-value", + "botToken: xoxb-slack-bot-token-value", + "publicKey: keep-me", + "gateway:", + " authToken: gw-token", + "env:", + " GITHUB_TOKEN: ghp_abcdefghijklmnopqrstuvwxyz0123456789", + " NODE_ENV: production", + "", + ].join("\n"), + ); + + sanitizeConfigFile(configPath); + + const result = readFileSync(configPath, "utf-8"); + expect(result).toContain("model: hermes"); + expect(result).toContain("publicKey: keep-me"); + expect(result).toContain("NODE_ENV: production"); + expect(result).toContain("[STRIPPED_BY_MIGRATION]"); + expect(result).not.toContain("sk-hermes-secret-key-value"); + expect(result).not.toContain("xoxb-slack-bot-token-value"); + expect(result).not.toContain("ghp_abcdefghijklmnopqrstuvwxyz0123456789"); + expect(result).not.toContain("gateway:"); + }); +}); + +describe("sanitizeEnvFileContent", () => { + it("strips PASS/TOKEN secrets without over-matching KEYBOARD_LAYOUT", () => { + const input = [ + "# comment", + "NODE_ENV=production", + "KEYBOARD_LAYOUT=us", + "DB_PASS=super-secret", + "GITHUB_TOKEN=ghp_abcdefghijklmnopqrstuvwxyz0123456789", + "API_KEY=openshell:resolve:env:API_KEY", + "PASSPHRASE=raw-passphrase", + "", + ].join("\n"); + + const result = sanitizeEnvFileContent(input); + expect(result).toContain("NODE_ENV=production"); + expect(result).toContain("KEYBOARD_LAYOUT=us"); + expect(result).toContain("DB_PASS=[STRIPPED_BY_MIGRATION]"); + expect(result).toContain("GITHUB_TOKEN=[STRIPPED_BY_MIGRATION]"); + expect(result).toContain("API_KEY=openshell:resolve:env:API_KEY"); + expect(result).toContain("PASSPHRASE=[STRIPPED_BY_MIGRATION]"); + expect(result).toContain("# comment"); + }); +}); + +describe("sanitizeEnvFile", () => { + let tmpDir: string; + + beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), "cred-env-test-")); + }); + + afterEach(() => { + rmSync(tmpDir, { recursive: true, force: true }); + }); + + it("rewrites .env credentials in place", () => { + const envPath = join(tmpDir, ".env"); + writeFileSync(envPath, "DB_PASS=secret\nLOG_LEVEL=info\n"); + sanitizeEnvFile(envPath); + expect(readFileSync(envPath, "utf-8")).toBe( + "DB_PASS=[STRIPPED_BY_MIGRATION]\nLOG_LEVEL=info\n", + ); + }); }); describe("isSensitiveFile", () => { @@ -229,11 +307,13 @@ describe("isSensitiveFile", () => { }); describe("shouldScanSnapshotFileForCredentials", () => { - it("scans runtime config and env files", () => { + it("scans runtime config, env, and Hermes YAML files", () => { expect(shouldScanSnapshotFileForCredentials("openclaw.json")).toBe(true); expect(shouldScanSnapshotFileForCredentials("config.json")).toBe(true); expect(shouldScanSnapshotFileForCredentials(".env")).toBe(true); expect(shouldScanSnapshotFileForCredentials("service.env")).toBe(true); + expect(shouldScanSnapshotFileForCredentials("config.yaml")).toBe(true); + expect(shouldScanSnapshotFileForCredentials("config.yml")).toBe(true); }); it("skips dependency lockfiles that can contain non-secret package metadata matches", () => { @@ -246,5 +326,6 @@ describe("shouldScanSnapshotFileForCredentials", () => { it("applies lockfile exclusions to paths by basename", () => { expect(shouldScanSnapshotFileForCredentials("/tmp/snapshot/package-lock.json")).toBe(false); expect(shouldScanSnapshotFileForCredentials("/tmp/snapshot/config.json")).toBe(true); + expect(shouldScanSnapshotFileForCredentials("/tmp/snapshot/config.yaml")).toBe(true); }); }); diff --git a/src/lib/security/credential-filter.ts b/src/lib/security/credential-filter.ts index d430e62bf16..fd715cd10a3 100644 --- a/src/lib/security/credential-filter.ts +++ b/src/lib/security/credential-filter.ts @@ -22,6 +22,7 @@ import { writeFileSync, } from "node:fs"; import { basename, dirname, join } from "node:path"; +import { parse as parseYaml, stringify as stringifyYaml } from "yaml"; import { isObjectRecord } from "../core/json-types"; import { hasPassCredentialSegment, SECRET_PATTERNS } from "./secret-patterns"; @@ -358,23 +359,108 @@ function scrubArrayElement(value: ConfigValue, previous: ConfigValue): ConfigVal } /** - * Strip credential fields from a JSON config file in-place. + * Strip credential fields from a KEY=value env file body. + * Uses the same field-name rules as JSON scrubbing so `DB_PASS` and + * `PASSPHRASE` are stripped while benign names like `KEYBOARD_LAYOUT` + * and `NODE_ENV` are preserved. + */ +export function sanitizeEnvFileContent(content: string): string { + return content + .split("\n") + .map((line) => { + const trimmed = line.trim(); + if (trimmed === "" || trimmed.startsWith("#")) return line; + const eq = line.indexOf("="); + if (eq <= 0) return line; + const key = line.slice(0, eq).trim(); + if (!key || !isCredentialField(key)) return line; + const value = line.slice(eq + 1); + if (isSafeCredentialPlaceholder(value)) return line; + return `${line.slice(0, eq)}=${CREDENTIAL_PLACEHOLDER}`; + }) + .join("\n"); +} + +/** + * Strip credential lines from a `.env` file in-place. + */ +export function sanitizeEnvFile(filePath: string): void { + const raw = readRegularFileNoFollow(filePath); + if (raw === null) return; + writeFileAtomically(filePath, sanitizeEnvFileContent(raw)); +} + +function toConfigValue(value: unknown): ConfigValue | undefined { + if (value === null || value === undefined) return value; + if (typeof value === "boolean" || typeof value === "number" || typeof value === "string") { + return value; + } + if (Array.isArray(value)) { + const items: ConfigValue[] = []; + for (const entry of value) { + const converted = toConfigValue(entry); + if (converted === undefined && entry !== undefined && entry !== null) return undefined; + items.push(converted as ConfigValue); + } + return items; + } + if (!isObjectRecord(value)) return undefined; + const result: ConfigObject = {}; + for (const [key, entry] of Object.entries(value)) { + const converted = toConfigValue(entry); + if (converted === undefined && entry !== undefined && entry !== null) return undefined; + result[key] = converted as ConfigValue; + } + return result; +} + +/** + * Strip credential fields from a Hermes YAML config file in-place. + * Removes the "gateway" section when present (auth tokens — regenerated + * at startup), matching JSON sanitization. + */ +export function sanitizeYamlConfigFile(configPath: string): void { + const rawConfig = readRegularFileNoFollow(configPath); + if (rawConfig === null) return; + let parsed: unknown; + try { + parsed = parseYaml(rawConfig); + } catch { + return; + } + const configValue = toConfigValue(parsed); + if (!isConfigObject(configValue)) return; + + const { gateway: _gateway, ...config } = configValue; + const sanitized = stripCredentials(config); + writeFileAtomically(configPath, stringifyYaml(sanitized)); +} + +/** + * Strip credential fields from a JSON or YAML config file in-place. * Removes the "gateway" section (contains auth tokens — regenerated at startup). + * JSON is preferred when the file parses as JSON; otherwise YAML is tried + * so Hermes `config.yaml` secrets are scrubbed from rebuild backups. */ export function sanitizeConfigFile(configPath: string): void { const rawConfig = readRegularFileNoFollow(configPath); if (rawConfig === null) return; - let parsed: ConfigValue; + try { - parsed = parseJson(rawConfig); + const parsed = parseJson(rawConfig); + if (!isConfigObject(parsed)) return; + const { gateway: _gateway, ...config } = parsed; + const sanitized = stripCredentials(config); + writeFileAtomically(configPath, JSON.stringify(sanitized, null, 2)); + return; } catch { - return; // Not valid JSON — skip (may be YAML for Hermes) + // Fall through to YAML for Hermes and other non-JSON configs. } - if (!isConfigObject(parsed)) return; - const { gateway: _gateway, ...config } = parsed; - const sanitized = stripCredentials(config); - writeFileAtomically(configPath, JSON.stringify(sanitized, null, 2)); + const normalized = basename(configPath).toLowerCase(); + if (normalized.endsWith(".yaml") || normalized.endsWith(".yml")) { + sanitizeYamlConfigFile(configPath); + } } /** @@ -394,6 +480,8 @@ export function shouldScanSnapshotFileForCredentials(filename: string): boolean return ( normalizedBasename === ".env" || normalizedBasename.endsWith(".env") || - normalizedBasename.endsWith(".json") + normalizedBasename.endsWith(".json") || + normalizedBasename.endsWith(".yaml") || + normalizedBasename.endsWith(".yml") ); } diff --git a/src/lib/state/sandbox.ts b/src/lib/state/sandbox.ts index af6040273a7..8584c72be1a 100644 --- a/src/lib/state/sandbox.ts +++ b/src/lib/state/sandbox.ts @@ -42,7 +42,11 @@ import { } from "../domain/backup-failure.js"; import { shellQuote } from "../runner.js"; import { createTempSshConfig } from "../sandbox/temp-ssh-config.js"; -import { isSensitiveFile, sanitizeConfigFile } from "../security/credential-filter.js"; +import { + isSensitiveFile, + sanitizeConfigFile, + sanitizeEnvFile, +} from "../security/credential-filter.js"; import { buildRestoreCleanupCommand, buildRestoreTarArgs, @@ -639,24 +643,17 @@ function sanitizeBackupDirectory(dirPath: string): void { } catch { /* best effort */ } - } else if (entry.name.endsWith(".json")) { + } else if ( + entry.name.endsWith(".json") || + entry.name.endsWith(".yaml") || + entry.name.endsWith(".yml") + ) { + // JSON (OpenClaw) and YAML (Hermes config.yaml) both carry secrets. sanitizeConfigFile(fullPath); } else if (entry.name === ".env" || entry.name.endsWith(".env")) { - // Strip credential lines from .env files (KEY=value format). // Hermes stores API keys in .env alongside config.yaml. try { - const envContent = readFileSync(fullPath, "utf-8"); - const filtered = envContent - .split("\n") - .map((line) => { - const key = line.split("=")[0]?.trim().toUpperCase() || ""; - if (/KEY|TOKEN|SECRET|PASSWORD|CREDENTIAL/.test(key)) { - return `${line.split("=")[0]}=[STRIPPED_BY_MIGRATION]`; - } - return line; - }) - .join("\n"); - writeFileSync(fullPath, filtered); + sanitizeEnvFile(fullPath); chmodSync(fullPath, 0o600); } catch { /* best effort */ From 186929b3a2798e785138a93b963fbcdff46fddb3 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Wed, 29 Jul 2026 03:23:40 +0530 Subject: [PATCH 02/19] fix(security): strip export-prefixed secrets in .env scrubbing Shell-sourced env files often use `export KEY=value`, which bypassed key detection. Strip the prefix before credential-field matching. Signed-off-by: Ayush7614 --- src/lib/security/credential-filter.test.ts | 16 ++++++++++++++++ src/lib/security/credential-filter.ts | 4 +++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/lib/security/credential-filter.test.ts b/src/lib/security/credential-filter.test.ts index 60e96a5b459..5ee31b2a251 100644 --- a/src/lib/security/credential-filter.test.ts +++ b/src/lib/security/credential-filter.test.ts @@ -266,6 +266,22 @@ describe("sanitizeEnvFileContent", () => { expect(result).toContain("PASSPHRASE=[STRIPPED_BY_MIGRATION]"); expect(result).toContain("# comment"); }); + + it("strips credential keys that use a leading export prefix", () => { + const input = [ + "export DB_PASS=super-secret", + "export NODE_ENV=production", + " export GITHUB_TOKEN=ghp_abcdefghijklmnopqrstuvwxyz0123456789", + "", + ].join("\n"); + + const result = sanitizeEnvFileContent(input); + expect(result).toContain("export DB_PASS=[STRIPPED_BY_MIGRATION]"); + expect(result).toContain("export NODE_ENV=production"); + expect(result).toContain("export GITHUB_TOKEN=[STRIPPED_BY_MIGRATION]"); + expect(result).not.toContain("super-secret"); + expect(result).not.toContain("ghp_abcdefghijklmnopqrstuvwxyz0123456789"); + }); }); describe("sanitizeEnvFile", () => { diff --git a/src/lib/security/credential-filter.ts b/src/lib/security/credential-filter.ts index fd715cd10a3..e55e4d8614a 100644 --- a/src/lib/security/credential-filter.ts +++ b/src/lib/security/credential-filter.ts @@ -372,7 +372,9 @@ export function sanitizeEnvFileContent(content: string): string { if (trimmed === "" || trimmed.startsWith("#")) return line; const eq = line.indexOf("="); if (eq <= 0) return line; - const key = line.slice(0, eq).trim(); + const rawKey = line.slice(0, eq).trim(); + // Shell-sourced .env files often use `export KEY=value`. + const key = rawKey.replace(/^export\s+/i, "").trim(); if (!key || !isCredentialField(key)) return line; const value = line.slice(eq + 1); if (isSafeCredentialPlaceholder(value)) return line; From e06937954bef8e496537860ffee3126d72e133b5 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Wed, 29 Jul 2026 03:42:15 +0530 Subject: [PATCH 03/19] fix(security): harden credential scrubbing fail-closed paths 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 --- .../src/security/credential-filter.test.ts | 12 +++++++ nemoclaw/src/security/credential-filter.ts | 22 +++++++++++-- src/lib/security/credential-filter.test.ts | 18 ++++++++++- src/lib/security/credential-filter.ts | 31 ++++++++++++------- src/lib/state/sandbox.ts | 19 +++++++----- 5 files changed, 80 insertions(+), 22 deletions(-) diff --git a/nemoclaw/src/security/credential-filter.test.ts b/nemoclaw/src/security/credential-filter.test.ts index 65e21f117b1..02423a14027 100644 --- a/nemoclaw/src/security/credential-filter.test.ts +++ b/nemoclaw/src/security/credential-filter.test.ts @@ -69,9 +69,21 @@ describe("plugin credential-filter", () => { expect(isSafeCredentialPlaceholder("unused")).toBe(true); expect(isSafeCredentialPlaceholder("openshell:resolve:env:TOKEN")).toBe(true); expect(valueLooksLikeSecret("sk-abcdefghijklmnopqrstuvwxyz")).toBe(true); + expect(valueLooksLikeSecret("glpat-abcdefghijklmnopqrst")).toBe(true); + expect(valueLooksLikeSecret("nvcf-abcdefghij")).toBe(true); expect(valueLooksLikeSecret("not-a-secret")).toBe(false); }); + it("preserves null and undefined under credential field names", () => { + const result = stripCredentials({ apiKey: null, token: undefined, model: "keep" }) as Record< + string, + unknown + >; + expect(result.apiKey).toBeNull(); + expect(result.token).toBeUndefined(); + expect(result.model).toBe("keep"); + }); + it("excludes auth state basenames from migration copies", () => { expect(isSensitiveFile("auth-profiles.json")).toBe(true); expect(isSensitiveFile("auth.json")).toBe(true); diff --git a/nemoclaw/src/security/credential-filter.ts b/nemoclaw/src/security/credential-filter.ts index dce343eda68..72d45a7fd90 100644 --- a/nemoclaw/src/security/credential-filter.ts +++ b/nemoclaw/src/security/credential-filter.ts @@ -58,17 +58,32 @@ const SAFE_CREDENTIAL_PLACEHOLDER_LITERALS: ReadonlySet = new Set([ CREDENTIAL_PLACEHOLDER, ]); -/** High-confidence raw secret shapes used as a value-level backstop. */ +/** + * High-confidence raw secret shapes used as a value-level backstop. + * Kept aligned with TOKEN_PREFIX / STRUCTURED / SECRET_BLOCK patterns from + * src/lib/security/secret-patterns.ts (plugin package cannot import src/lib). + */ const VALUE_SECRET_PATTERNS: readonly RegExp[] = [ /nvapi-[A-Za-z0-9_-]{10,}/, + /nvcf-[A-Za-z0-9_-]{10,}/, /ghp_[A-Za-z0-9_-]{10,}/, + /(?:github_pat_)[A-Za-z0-9_]{30,}/, /sk-proj-[A-Za-z0-9_-]{10,}/, /sk-ant-[A-Za-z0-9_-]{10,}/, /sk-[A-Za-z0-9_-]{20,}/, /(?:xox[bpas]|xapp)-[A-Za-z0-9-]{10,}/, /A(?:K|S)IA[A-Z0-9]{16}/, /hf_[A-Za-z0-9]{10,}/, + /glpat-[A-Za-z0-9_-]{10,}/, + /gsk_[A-Za-z0-9]{10,}/, + /pypi-[A-Za-z0-9_-]{10,}/, + /\bbot\d{8,10}:[A-Za-z0-9_-]{35}\b/, + /\b\d{8,10}:[A-Za-z0-9_-]{35}\b/, + /\b[A-Za-z0-9]{24}\.[A-Za-z0-9_-]{6}\.[A-Za-z0-9_-]{27,}\b/, /tvly-[A-Za-z0-9_-]{10,}/, + /lsv2_(?:pt|sk)_[A-Za-z0-9]{10,}(?:_[A-Za-z0-9]+)*/, + /\beyJ[A-Za-z0-9_-]{5,}\.[A-Za-z0-9_-]{2,}\.[A-Za-z0-9_-]{10,}\b/, + /-----BEGIN (?:[A-Z0-9]+ )?PRIVATE KEY-----[\s\S]*?-----END (?:[A-Z0-9]+ )?PRIVATE KEY-----/, ]; function hasPassCredentialSegment(key: string): boolean { @@ -164,7 +179,10 @@ export function stripCredentials(obj: unknown): unknown { const result: UnknownRecord = {}; for (const [key, value] of Object.entries(obj)) { if (isCredentialField(key)) { - result[key] = isSafeCredentialPlaceholder(value) ? value : CREDENTIAL_PLACEHOLDER; + result[key] = + value === null || value === undefined || isSafeCredentialPlaceholder(value) + ? value + : CREDENTIAL_PLACEHOLDER; } else { result[key] = scrubConfigValue(value); } diff --git a/src/lib/security/credential-filter.test.ts b/src/lib/security/credential-filter.test.ts index 5ee31b2a251..d85820e2c29 100644 --- a/src/lib/security/credential-filter.test.ts +++ b/src/lib/security/credential-filter.test.ts @@ -13,6 +13,7 @@ import { sanitizeConfigFile, sanitizeEnvFile, sanitizeEnvFileContent, + sanitizeYamlConfigFile, shouldScanSnapshotFileForCredentials, stripCredentials, } from "./credential-filter.js"; @@ -85,6 +86,13 @@ describe("stripCredentials", () => { expect(stripCredentials(42)).toBe(42); }); + it("preserves null and undefined under credential field names", () => { + const result = stripCredentials({ apiKey: null, token: undefined, model: "keep" }); + expect(result.apiKey).toBeNull(); + expect(result.token).toBeUndefined(); + expect(result.model).toBe("keep"); + }); + it("preserves OpenShell resolve placeholders under credential fields (#5027)", () => { const input = { models: { providers: { nvidia: { apiKey: "unused", baseUrl: "https://x/v1" } } }, @@ -230,7 +238,7 @@ describe("sanitizeConfigFile", () => { ].join("\n"), ); - sanitizeConfigFile(configPath); + expect(sanitizeConfigFile(configPath)).toBe(true); const result = readFileSync(configPath, "utf-8"); expect(result).toContain("model: hermes"); @@ -242,6 +250,14 @@ describe("sanitizeConfigFile", () => { expect(result).not.toContain("ghp_abcdefghijklmnopqrstuvwxyz0123456789"); expect(result).not.toContain("gateway:"); }); + + it("fails closed for malformed Hermes YAML", () => { + const configPath = join(tmpDir, "broken.yaml"); + writeFileSync(configPath, "api_key: [unclosed\n"); + expect(sanitizeYamlConfigFile(configPath)).toBe(false); + expect(sanitizeConfigFile(configPath)).toBe(false); + expect(readFileSync(configPath, "utf-8")).toContain("api_key:"); + }); }); describe("sanitizeEnvFileContent", () => { diff --git a/src/lib/security/credential-filter.ts b/src/lib/security/credential-filter.ts index e55e4d8614a..f81feaa4af6 100644 --- a/src/lib/security/credential-filter.ts +++ b/src/lib/security/credential-filter.ts @@ -295,9 +295,12 @@ export function stripCredentials(obj: ConfigValue): ConfigValue { const result: ConfigObject = {}; for (const [key, value] of Object.entries(obj)) { if (isCredentialField(key)) { - // Preserve non-secret references (OpenShell resolve placeholders, the - // `unused` sentinel); scrub anything else that looks like a raw secret. - result[key] = isSafeCredentialPlaceholder(value) ? value : CREDENTIAL_PLACEHOLDER; + // Preserve unset credential fields and non-secret references (OpenShell + // resolve placeholders, the `unused` sentinel); scrub raw secrets. + result[key] = + value === null || value === undefined || isSafeCredentialPlaceholder(value) + ? value + : CREDENTIAL_PLACEHOLDER; } else { result[key] = scrubConfigValue(value); } @@ -421,21 +424,22 @@ function toConfigValue(value: unknown): ConfigValue | undefined { * Removes the "gateway" section when present (auth tokens — regenerated * at startup), matching JSON sanitization. */ -export function sanitizeYamlConfigFile(configPath: string): void { +export function sanitizeYamlConfigFile(configPath: string): boolean { const rawConfig = readRegularFileNoFollow(configPath); - if (rawConfig === null) return; + if (rawConfig === null) return false; let parsed: unknown; try { parsed = parseYaml(rawConfig); } catch { - return; + return false; } const configValue = toConfigValue(parsed); - if (!isConfigObject(configValue)) return; + if (!isConfigObject(configValue)) return false; const { gateway: _gateway, ...config } = configValue; const sanitized = stripCredentials(config); writeFileAtomically(configPath, stringifyYaml(sanitized)); + return true; } /** @@ -443,26 +447,29 @@ export function sanitizeYamlConfigFile(configPath: string): void { * Removes the "gateway" section (contains auth tokens — regenerated at startup). * JSON is preferred when the file parses as JSON; otherwise YAML is tried * so Hermes `config.yaml` secrets are scrubbed from rebuild backups. + * Returns false when a YAML/YML target cannot be sanitized so callers can + * fail closed instead of retaining the raw file. */ -export function sanitizeConfigFile(configPath: string): void { +export function sanitizeConfigFile(configPath: string): boolean { const rawConfig = readRegularFileNoFollow(configPath); - if (rawConfig === null) return; + if (rawConfig === null) return false; try { const parsed = parseJson(rawConfig); - if (!isConfigObject(parsed)) return; + if (!isConfigObject(parsed)) return false; const { gateway: _gateway, ...config } = parsed; const sanitized = stripCredentials(config); writeFileAtomically(configPath, JSON.stringify(sanitized, null, 2)); - return; + return true; } catch { // Fall through to YAML for Hermes and other non-JSON configs. } const normalized = basename(configPath).toLowerCase(); if (normalized.endsWith(".yaml") || normalized.endsWith(".yml")) { - sanitizeYamlConfigFile(configPath); + return sanitizeYamlConfigFile(configPath); } + return false; } /** diff --git a/src/lib/state/sandbox.ts b/src/lib/state/sandbox.ts index 8584c72be1a..ab1d1059e73 100644 --- a/src/lib/state/sandbox.ts +++ b/src/lib/state/sandbox.ts @@ -637,20 +637,25 @@ function sanitizeBackupDirectory(dirPath: string): void { if (entry.isDirectory()) { walk(fullPath); } else if (entry.isFile()) { + const name = entry.name.toLowerCase(); if (isSensitiveFile(entry.name)) { try { require("node:fs").unlinkSync(fullPath); } catch { /* best effort */ } - } else if ( - entry.name.endsWith(".json") || - entry.name.endsWith(".yaml") || - entry.name.endsWith(".yml") - ) { + } else if (name.endsWith(".json") || name.endsWith(".yaml") || name.endsWith(".yml")) { // JSON (OpenClaw) and YAML (Hermes config.yaml) both carry secrets. - sanitizeConfigFile(fullPath); - } else if (entry.name === ".env" || entry.name.endsWith(".env")) { + // Fail closed for YAML: omit the artifact when sanitization cannot run. + const sanitized = sanitizeConfigFile(fullPath); + if (!sanitized && (name.endsWith(".yaml") || name.endsWith(".yml"))) { + try { + require("node:fs").unlinkSync(fullPath); + } catch { + /* best effort */ + } + } + } else if (name === ".env" || name.endsWith(".env")) { // Hermes stores API keys in .env alongside config.yaml. try { sanitizeEnvFile(fullPath); From eb2b024b3a1657532f2b9d2f90427fde42f31487 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 29 Jul 2026 13:02:53 -0700 Subject: [PATCH 04/19] fix(security): fail closed on backup scrub errors Signed-off-by: Apurv Kumaria --- .../src/security/credential-filter.test.ts | 2 + nemoclaw/src/security/credential-filter.ts | 1 + src/lib/security/credential-filter.test.ts | 38 +++++++++- src/lib/security/credential-filter.ts | 62 +++++++++++------ .../state/sandbox-backup-sanitization.test.ts | 62 +++++++++++++++++ src/lib/state/sandbox.ts | 69 ++++++++++++++----- 6 files changed, 192 insertions(+), 42 deletions(-) create mode 100644 src/lib/state/sandbox-backup-sanitization.test.ts diff --git a/nemoclaw/src/security/credential-filter.test.ts b/nemoclaw/src/security/credential-filter.test.ts index 02423a14027..b8784d279a6 100644 --- a/nemoclaw/src/security/credential-filter.test.ts +++ b/nemoclaw/src/security/credential-filter.test.ts @@ -40,6 +40,7 @@ describe("plugin credential-filter", () => { env: { GITHUB_TOKEN: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", NODE_ENV: "test" }, args: ["--api-key", "opaque-secret-value", "--verbose"], }, + customHeader: "Bearer opaque-migration-secret", model: "keep-me", publicKey: "verify-me", apiKey: "openshell:resolve:env:NVIDIA_API_KEY", @@ -60,6 +61,7 @@ describe("plugin credential-filter", () => { expect(mcp.env.GITHUB_TOKEN).toBe(CREDENTIAL_PLACEHOLDER); expect(mcp.env.NODE_ENV).toBe("test"); expect(mcp.args).toEqual(["--api-key", CREDENTIAL_PLACEHOLDER, "--verbose"]); + expect(result.customHeader).toBe(CREDENTIAL_PLACEHOLDER); expect(result.model).toBe("keep-me"); expect(result.publicKey).toBe("verify-me"); expect(result.apiKey).toBe("openshell:resolve:env:NVIDIA_API_KEY"); diff --git a/nemoclaw/src/security/credential-filter.ts b/nemoclaw/src/security/credential-filter.ts index 72d45a7fd90..64980c53f36 100644 --- a/nemoclaw/src/security/credential-filter.ts +++ b/nemoclaw/src/security/credential-filter.ts @@ -83,6 +83,7 @@ const VALUE_SECRET_PATTERNS: readonly RegExp[] = [ /tvly-[A-Za-z0-9_-]{10,}/, /lsv2_(?:pt|sk)_[A-Za-z0-9]{10,}(?:_[A-Za-z0-9]+)*/, /\beyJ[A-Za-z0-9_-]{5,}\.[A-Za-z0-9_-]{2,}\.[A-Za-z0-9_-]{10,}\b/, + /(?<=Bearer\s+)[A-Za-z0-9_.+/=-]{10,}/i, /-----BEGIN (?:[A-Z0-9]+ )?PRIVATE KEY-----[\s\S]*?-----END (?:[A-Z0-9]+ )?PRIVATE KEY-----/, ]; diff --git a/src/lib/security/credential-filter.test.ts b/src/lib/security/credential-filter.test.ts index d85820e2c29..0c93c7937be 100644 --- a/src/lib/security/credential-filter.test.ts +++ b/src/lib/security/credential-filter.test.ts @@ -258,6 +258,29 @@ describe("sanitizeConfigFile", () => { expect(sanitizeConfigFile(configPath)).toBe(false); expect(readFileSync(configPath, "utf-8")).toContain("api_key:"); }); + + it("returns failure without changing the source when a YAML rewrite fails", () => { + const configPath = join(tmpDir, "config.yaml"); + const source = "api_key: sk-hermes-secret-key-value\n"; + writeFileSync(configPath, source); + + expect( + sanitizeYamlConfigFile(configPath, () => { + throw new Error("injected rewrite failure"); + }), + ).toBe(false); + expect(readFileSync(configPath, "utf-8")).toBe(source); + }); + + it("sanitizes valid JSON arrays instead of treating them as failures", () => { + const configPath = join(tmpDir, "config.json"); + writeFileSync(configPath, JSON.stringify([{ apiKey: "sk-secret-value-long-enough" }])); + + expect(sanitizeConfigFile(configPath)).toBe(true); + expect(JSON.parse(readFileSync(configPath, "utf-8"))).toEqual([ + { apiKey: "[STRIPPED_BY_MIGRATION]" }, + ]); + }); }); describe("sanitizeEnvFileContent", () => { @@ -314,11 +337,24 @@ describe("sanitizeEnvFile", () => { it("rewrites .env credentials in place", () => { const envPath = join(tmpDir, ".env"); writeFileSync(envPath, "DB_PASS=secret\nLOG_LEVEL=info\n"); - sanitizeEnvFile(envPath); + expect(sanitizeEnvFile(envPath)).toBe(true); expect(readFileSync(envPath, "utf-8")).toBe( "DB_PASS=[STRIPPED_BY_MIGRATION]\nLOG_LEVEL=info\n", ); }); + + it("returns failure without changing the source when an env rewrite fails", () => { + const envPath = join(tmpDir, ".env"); + const source = "DB_PASS=secret\n"; + writeFileSync(envPath, source); + + expect( + sanitizeEnvFile(envPath, () => { + throw new Error("injected rewrite failure"); + }), + ).toBe(false); + expect(readFileSync(envPath, "utf-8")).toBe(source); + }); }); describe("isSensitiveFile", () => { diff --git a/src/lib/security/credential-filter.ts b/src/lib/security/credential-filter.ts index f81feaa4af6..b60f26e79c3 100644 --- a/src/lib/security/credential-filter.ts +++ b/src/lib/security/credential-filter.ts @@ -389,13 +389,23 @@ export function sanitizeEnvFileContent(content: string): string { /** * Strip credential lines from a `.env` file in-place. */ -export function sanitizeEnvFile(filePath: string): void { +export function sanitizeEnvFile( + filePath: string, + writeSanitized: (targetPath: string, contents: string) => void = writeFileAtomically, +): boolean { const raw = readRegularFileNoFollow(filePath); - if (raw === null) return; - writeFileAtomically(filePath, sanitizeEnvFileContent(raw)); + if (raw === null) return false; + try { + writeSanitized(filePath, sanitizeEnvFileContent(raw)); + return true; + } catch { + return false; + } } -function toConfigValue(value: unknown): ConfigValue | undefined { +const UNREPRESENTABLE_CONFIG_VALUE = Symbol("unrepresentable-config-value"); + +function toConfigValue(value: unknown): ConfigValue | typeof UNREPRESENTABLE_CONFIG_VALUE { if (value === null || value === undefined) return value; if (typeof value === "boolean" || typeof value === "number" || typeof value === "string") { return value; @@ -404,17 +414,17 @@ function toConfigValue(value: unknown): ConfigValue | undefined { const items: ConfigValue[] = []; for (const entry of value) { const converted = toConfigValue(entry); - if (converted === undefined && entry !== undefined && entry !== null) return undefined; - items.push(converted as ConfigValue); + if (converted === UNREPRESENTABLE_CONFIG_VALUE) return UNREPRESENTABLE_CONFIG_VALUE; + items.push(converted); } return items; } - if (!isObjectRecord(value)) return undefined; + if (!isObjectRecord(value)) return UNREPRESENTABLE_CONFIG_VALUE; const result: ConfigObject = {}; for (const [key, entry] of Object.entries(value)) { const converted = toConfigValue(entry); - if (converted === undefined && entry !== undefined && entry !== null) return undefined; - result[key] = converted as ConfigValue; + if (converted === UNREPRESENTABLE_CONFIG_VALUE) return UNREPRESENTABLE_CONFIG_VALUE; + result[key] = converted; } return result; } @@ -424,22 +434,26 @@ function toConfigValue(value: unknown): ConfigValue | undefined { * Removes the "gateway" section when present (auth tokens — regenerated * at startup), matching JSON sanitization. */ -export function sanitizeYamlConfigFile(configPath: string): boolean { +export function sanitizeYamlConfigFile( + configPath: string, + writeSanitized: (targetPath: string, contents: string) => void = writeFileAtomically, +): boolean { const rawConfig = readRegularFileNoFollow(configPath); if (rawConfig === null) return false; - let parsed: unknown; try { - parsed = parseYaml(rawConfig); + const parsed = parseYaml(rawConfig); + const configValue = toConfigValue(parsed); + if (configValue === UNREPRESENTABLE_CONFIG_VALUE || !isConfigObject(configValue)) { + return false; + } + + const { gateway: _gateway, ...config } = configValue; + const sanitized = stripCredentials(config); + writeSanitized(configPath, stringifyYaml(sanitized)); + return true; } catch { return false; } - const configValue = toConfigValue(parsed); - if (!isConfigObject(configValue)) return false; - - const { gateway: _gateway, ...config } = configValue; - const sanitized = stripCredentials(config); - writeFileAtomically(configPath, stringifyYaml(sanitized)); - return true; } /** @@ -455,9 +469,13 @@ export function sanitizeConfigFile(configPath: string): boolean { if (rawConfig === null) return false; try { - const parsed = parseJson(rawConfig); - if (!isConfigObject(parsed)) return false; - const { gateway: _gateway, ...config } = parsed; + const parsed = parseJson(rawConfig); + if (!isConfigValue(parsed)) return false; + let config = parsed; + if (isConfigObject(parsed)) { + const { gateway: _gateway, ...withoutGateway } = parsed; + config = withoutGateway; + } const sanitized = stripCredentials(config); writeFileAtomically(configPath, JSON.stringify(sanitized, null, 2)); return true; diff --git a/src/lib/state/sandbox-backup-sanitization.test.ts b/src/lib/state/sandbox-backup-sanitization.test.ts new file mode 100644 index 00000000000..a3d089bbf29 --- /dev/null +++ b/src/lib/state/sandbox-backup-sanitization.test.ts @@ -0,0 +1,62 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; + +import { sanitizeBackupDirectory } from "./sandbox.js"; + +const testDirectories: string[] = []; + +function createBackup(): string { + const backupPath = mkdtempSync(join(tmpdir(), "nemoclaw-sanitize-backup-")); + testDirectories.push(backupPath); + mkdirSync(join(backupPath, "state"), { recursive: true }); + return backupPath; +} + +afterEach(() => { + for (const testDirectory of testDirectories.splice(0)) { + rmSync(testDirectory, { recursive: true, force: true }); + } +}); + +describe("rebuild backup credential sanitization", () => { + it("omits unsanitizable config and env artifacts", () => { + const backupPath = createBackup(); + const yamlPath = join(backupPath, "state", "config.yaml"); + const jsonPath = join(backupPath, "state", "config.json"); + const envPath = join(backupPath, "state", ".env"); + const safePath = join(backupPath, "state", "notes.txt"); + writeFileSync(yamlPath, "api_key: [unclosed\n"); + writeFileSync(jsonPath, '{"apiKey":'); + writeFileSync(envPath, "DB_PASS=raw-secret\n"); + writeFileSync(safePath, "safe"); + + sanitizeBackupDirectory(backupPath, { + sanitizeEnvFile: () => false, + }); + + expect(existsSync(yamlPath)).toBe(false); + expect(existsSync(jsonPath)).toBe(false); + expect(existsSync(envPath)).toBe(false); + expect(readFileSync(safePath, "utf-8")).toBe("safe"); + }); + + it("removes and rejects the whole backup when an unsafe artifact cannot be deleted", () => { + const backupPath = createBackup(); + const yamlPath = join(backupPath, "state", "config.yaml"); + writeFileSync(yamlPath, "api_key: [unclosed\n"); + + expect(() => + sanitizeBackupDirectory(backupPath, { + unlinkFile: () => { + throw new Error("injected unlink failure"); + }, + }), + ).toThrow("Credential sanitization failed; removed the incomplete backup"); + expect(existsSync(backupPath)).toBe(false); + }); +}); diff --git a/src/lib/state/sandbox.ts b/src/lib/state/sandbox.ts index ab1d1059e73..2c8515a6268 100644 --- a/src/lib/state/sandbox.ts +++ b/src/lib/state/sandbox.ts @@ -23,6 +23,7 @@ import { readlinkSync, rmSync, statSync, + unlinkSync, writeFileSync, } from "node:fs"; import os from "node:os"; @@ -628,8 +629,29 @@ function computeBlueprintDigest(): string | null { * Walk a local directory and sanitize any JSON config files found. * Also removes files that match CREDENTIAL_SENSITIVE_BASENAMES. */ -function sanitizeBackupDirectory(dirPath: string): void { +export interface BackupSanitizationOperations { + sanitizeConfigFile: (filePath: string) => boolean; + sanitizeEnvFile: (filePath: string) => boolean; + unlinkFile: (filePath: string) => void; + removeBackup: (backupPath: string) => void; + backupExists: (backupPath: string) => boolean; +} + +const DEFAULT_BACKUP_SANITIZATION_OPERATIONS: BackupSanitizationOperations = { + sanitizeConfigFile, + sanitizeEnvFile, + unlinkFile: unlinkSync, + removeBackup: (backupPath) => rmSync(backupPath, { recursive: true, force: true }), + backupExists: existsSync, +}; + +/** @visibleForTesting */ +export function sanitizeBackupDirectory( + dirPath: string, + overrides: Partial = {}, +): void { if (!existsSync(dirPath)) return; + const operations = { ...DEFAULT_BACKUP_SANITIZATION_OPERATIONS, ...overrides }; const walk = (current: string): void => { for (const entry of readdirSync(current, { withFileTypes: true })) { @@ -639,35 +661,44 @@ function sanitizeBackupDirectory(dirPath: string): void { } else if (entry.isFile()) { const name = entry.name.toLowerCase(); if (isSensitiveFile(entry.name)) { - try { - require("node:fs").unlinkSync(fullPath); - } catch { - /* best effort */ - } + operations.unlinkFile(fullPath); } else if (name.endsWith(".json") || name.endsWith(".yaml") || name.endsWith(".yml")) { // JSON (OpenClaw) and YAML (Hermes config.yaml) both carry secrets. - // Fail closed for YAML: omit the artifact when sanitization cannot run. - const sanitized = sanitizeConfigFile(fullPath); - if (!sanitized && (name.endsWith(".yaml") || name.endsWith(".yml"))) { - try { - require("node:fs").unlinkSync(fullPath); - } catch { - /* best effort */ - } + // Fail closed: omit the artifact when sanitization cannot run. + if (!operations.sanitizeConfigFile(fullPath)) { + operations.unlinkFile(fullPath); } } else if (name === ".env" || name.endsWith(".env")) { // Hermes stores API keys in .env alongside config.yaml. - try { - sanitizeEnvFile(fullPath); + if (operations.sanitizeEnvFile(fullPath)) { chmodSync(fullPath, 0o600); - } catch { - /* best effort */ + } else { + operations.unlinkFile(fullPath); } } } } }; - walk(dirPath); + + try { + walk(dirPath); + } catch (error) { + try { + operations.removeBackup(dirPath); + } catch (cleanupError) { + throw new Error("Credential sanitization failed and backup cleanup failed", { + cause: cleanupError, + }); + } + if (operations.backupExists(dirPath)) { + throw new Error("Credential sanitization failed and the incomplete backup remains", { + cause: error, + }); + } + throw new Error("Credential sanitization failed; removed the incomplete backup", { + cause: error, + }); + } } // ── Logging ──────────────────────────────────────────────────────── From 147c1f764171f46fd8b9ae3406fc994fcace0a65 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 29 Jul 2026 13:07:02 -0700 Subject: [PATCH 05/19] docs(security): clarify snapshot credential filtering Signed-off-by: Apurv Kumaria --- docs/manage-sandboxes/backup-restore.mdx | 5 +++++ docs/reference/host-files-and-state.mdx | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/manage-sandboxes/backup-restore.mdx b/docs/manage-sandboxes/backup-restore.mdx index a0cfab7835c..193cdb02f0e 100644 --- a/docs/manage-sandboxes/backup-restore.mdx +++ b/docs/manage-sandboxes/backup-restore.mdx @@ -38,6 +38,11 @@ Snapshots capture all workspace state directories defined in the agent manifest Agent manifests can also declare durable top-level state files. Treat snapshot directories as private local data. +Before NemoClaw marks a snapshot complete, it strips recognized credential fields from copied JSON, YAML, and environment files. +It preserves OpenShell credential placeholders so rebuild can reattach the host-side provider. +If NemoClaw cannot sanitize a copied configuration or environment file, it omits that file from the snapshot. +If it cannot remove the unsafe file, snapshot creation deletes the incomplete backup and returns an error. + Hermes snapshots include `SOUL.md`, the Web Dashboard profile under `.hermes/dashboard-home/`, the SQLite database behind `.hermes/state.db`, and the default kanban board in `.hermes/kanban.db`. NemoClaw uses SQLite's online backup API and restores those databases through SQLite instead of copying live raw database files. diff --git a/docs/reference/host-files-and-state.mdx b/docs/reference/host-files-and-state.mdx index 6bec4512cf6..07bf1c0d374 100644 --- a/docs/reference/host-files-and-state.mdx +++ b/docs/reference/host-files-and-state.mdx @@ -45,7 +45,7 @@ If you see `registry.json` in older tests, notes, or discussions, treat it as le | Path | Purpose | Safe to delete | |---|---|---| | `~/.nemoclaw/state/` | Operational coordination and history for lifecycle locks, shields transitions, timers, and audit events, local routing, and port-forward helpers. | No. Deleting it can disrupt an active operation and discard security or recovery context. | -| `~/.nemoclaw/snapshots/` | Full copies of host `~/.openclaw` state created by blueprint migration and rollback flows. | Only after you no longer need the corresponding rollback or restore point. The host CLI does not expose the direct runner's retention actions. | +| `~/.nemoclaw/snapshots/` | Copies of host `~/.openclaw` state created by blueprint migration and rollback flows. NemoClaw excludes known authentication-state files and strips recognized credential fields from `openclaw.json`. | Only after you no longer need the corresponding rollback or restore point. The host CLI does not expose the direct runner's retention actions. | | `~/.nemoclaw/rebuild-backups/` | Host-side snapshots written by `backup-all`, `snapshot create`, and rebuild flows. | Only after you no longer need rollback or restore points. | | `~/.nemoclaw/backups/` | Workspace backups written by legacy backup helpers and some recovery flows. | Only after confirming you no longer need those workspace archives. | | `~/.nemoclaw/mounts/` | Default local mount points created by share or mount commands. | Unmount first, then remove unused directories. | From 6e45761218919b657518df6150ffbb465d2f451d Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 29 Jul 2026 13:08:19 -0700 Subject: [PATCH 06/19] docs(security): refine snapshot cleanup behavior Signed-off-by: Apurv Kumaria --- docs/manage-sandboxes/backup-restore.mdx | 5 +++-- docs/reference/host-files-and-state.mdx | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/manage-sandboxes/backup-restore.mdx b/docs/manage-sandboxes/backup-restore.mdx index 193cdb02f0e..448194670c5 100644 --- a/docs/manage-sandboxes/backup-restore.mdx +++ b/docs/manage-sandboxes/backup-restore.mdx @@ -38,10 +38,11 @@ Snapshots capture all workspace state directories defined in the agent manifest Agent manifests can also declare durable top-level state files. Treat snapshot directories as private local data. -Before NemoClaw marks a snapshot complete, it strips recognized credential fields from copied JSON, YAML, and environment files. +Before NemoClaw marks a snapshot complete, it strips recognized credential values from copied JSON, YAML, and `.env` files. It preserves OpenShell credential placeholders so rebuild can reattach the host-side provider. If NemoClaw cannot sanitize a copied configuration or environment file, it omits that file from the snapshot. -If it cannot remove the unsafe file, snapshot creation deletes the incomplete backup and returns an error. +If it cannot remove the unsafe file, snapshot creation returns an error. +It deletes the incomplete backup when cleanup succeeds and reports when the backup remains. Hermes snapshots include `SOUL.md`, the Web Dashboard profile under `.hermes/dashboard-home/`, the SQLite database behind `.hermes/state.db`, and the default kanban board in `.hermes/kanban.db`. diff --git a/docs/reference/host-files-and-state.mdx b/docs/reference/host-files-and-state.mdx index 07bf1c0d374..2010f044137 100644 --- a/docs/reference/host-files-and-state.mdx +++ b/docs/reference/host-files-and-state.mdx @@ -45,7 +45,7 @@ If you see `registry.json` in older tests, notes, or discussions, treat it as le | Path | Purpose | Safe to delete | |---|---|---| | `~/.nemoclaw/state/` | Operational coordination and history for lifecycle locks, shields transitions, timers, and audit events, local routing, and port-forward helpers. | No. Deleting it can disrupt an active operation and discard security or recovery context. | -| `~/.nemoclaw/snapshots/` | Copies of host `~/.openclaw` state created by blueprint migration and rollback flows. NemoClaw excludes known authentication-state files and strips recognized credential fields from `openclaw.json`. | Only after you no longer need the corresponding rollback or restore point. The host CLI does not expose the direct runner's retention actions. | +| `~/.nemoclaw/snapshots/` | Copies of host `~/.openclaw` state created by blueprint migration and rollback flows. NemoClaw excludes known authentication-state files and strips recognized credential values from `openclaw.json`. | Only after you no longer need the corresponding rollback or restore point. The host CLI does not expose the direct runner's retention actions. | | `~/.nemoclaw/rebuild-backups/` | Host-side snapshots written by `backup-all`, `snapshot create`, and rebuild flows. | Only after you no longer need rollback or restore points. | | `~/.nemoclaw/backups/` | Workspace backups written by legacy backup helpers and some recovery flows. | Only after confirming you no longer need those workspace archives. | | `~/.nemoclaw/mounts/` | Default local mount points created by share or mount commands. | Unmount first, then remove unused directories. | From a59653920e2c69f03d52d976ee47e5736b4f4cd9 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 29 Jul 2026 13:44:50 -0700 Subject: [PATCH 07/19] fix(security): sanitize every migration snapshot artifact Signed-off-by: Apurv Kumaria --- ci/test-file-size-budget.json | 2 +- docs/reference/host-files-and-state.mdx | 6 +- nemoclaw/src/commands/migration-state.test.ts | 16 +- nemoclaw/src/commands/migration-state.ts | 59 ++++--- .../src/security/credential-filter.test.ts | 18 +++ nemoclaw/src/security/credential-filter.ts | 25 +++ .../src/security/snapshot-sanitizer.test.ts | 77 ++++++++++ nemoclaw/src/security/snapshot-sanitizer.ts | 144 ++++++++++++++++++ src/lib/security/credential-filter.test.ts | 28 ++++ src/lib/security/credential-filter.ts | 6 +- .../state/sandbox-backup-sanitization.test.ts | 40 ++++- test/credential-filter-parity.test.ts | 80 ++++++++++ 12 files changed, 464 insertions(+), 37 deletions(-) create mode 100644 nemoclaw/src/security/snapshot-sanitizer.test.ts create mode 100644 nemoclaw/src/security/snapshot-sanitizer.ts create mode 100644 test/credential-filter-parity.test.ts diff --git a/ci/test-file-size-budget.json b/ci/test-file-size-budget.json index 7fd8fc2d981..7fb675bf155 100644 --- a/ci/test-file-size-budget.json +++ b/ci/test-file-size-budget.json @@ -2,7 +2,7 @@ "$comment": "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.\nSPDX-License-Identifier: Apache-2.0", "defaultMaxLines": 1500, "legacyMaxLines": { - "nemoclaw/src/commands/migration-state.test.ts": 1565, + "nemoclaw/src/commands/migration-state.test.ts": 1563, "src/lib/inference/nim.test.ts": 2068, "src/lib/onboard/preflight.test.ts": 1904, "test/generate-openclaw-config.test.ts": 1941, diff --git a/docs/reference/host-files-and-state.mdx b/docs/reference/host-files-and-state.mdx index 2010f044137..95892c1852e 100644 --- a/docs/reference/host-files-and-state.mdx +++ b/docs/reference/host-files-and-state.mdx @@ -45,7 +45,7 @@ If you see `registry.json` in older tests, notes, or discussions, treat it as le | Path | Purpose | Safe to delete | |---|---|---| | `~/.nemoclaw/state/` | Operational coordination and history for lifecycle locks, shields transitions, timers, and audit events, local routing, and port-forward helpers. | No. Deleting it can disrupt an active operation and discard security or recovery context. | -| `~/.nemoclaw/snapshots/` | Copies of host `~/.openclaw` state created by blueprint migration and rollback flows. NemoClaw excludes known authentication-state files and strips recognized credential values from `openclaw.json`. | Only after you no longer need the corresponding rollback or restore point. The host CLI does not expose the direct runner's retention actions. | +| `~/.nemoclaw/snapshots/` | Copies of host `~/.openclaw` state and configured external roots created by blueprint migration and rollback flows. NemoClaw excludes known authentication-state files and strips recognized credential values from copied JSON, YAML, and `.env` files. | Only after you no longer need the corresponding rollback or restore point. The host CLI does not expose the direct runner's retention actions. | | `~/.nemoclaw/rebuild-backups/` | Host-side snapshots written by `backup-all`, `snapshot create`, and rebuild flows. | Only after you no longer need rollback or restore points. | | `~/.nemoclaw/backups/` | Workspace backups written by legacy backup helpers and some recovery flows. | Only after confirming you no longer need those workspace archives. | | `~/.nemoclaw/mounts/` | Default local mount points created by share or mount commands. | Unmount first, then remove unused directories. | @@ -53,6 +53,10 @@ If you see `registry.json` in older tests, notes, or discussions, treat it as le ## Migration Snapshot Retention +Before NemoClaw retains a migration snapshot, it recursively sanitizes the copied OpenClaw state and every configured external root. +It preserves empty or comment-only YAML files and omits copied JSON, YAML, or `.env` files that it cannot sanitize. +If NemoClaw cannot remove an unsafe copied artifact, snapshot creation fails and attempts to delete the incomplete snapshot directory. + The direct blueprint runner accepts these action arguments for migration snapshots: ```text diff --git a/nemoclaw/src/commands/migration-state.test.ts b/nemoclaw/src/commands/migration-state.test.ts index 5a2a1e7dcfe..83ad2b37c2d 100644 --- a/nemoclaw/src/commands/migration-state.test.ts +++ b/nemoclaw/src/commands/migration-state.test.ts @@ -1,21 +1,17 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { describe, it, expect, beforeEach, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; import type { PluginLogger } from "../index.js"; import { setConfigValue } from "./migration-state.js"; -// --------------------------------------------------------------------------- // fs mock — thin in-memory store keyed by absolute path -// --------------------------------------------------------------------------- interface FsEntry { type: "file" | "dir" | "symlink"; content?: string; } - const store = new Map(); - function addDir(p: string): void { store.set(p, { type: "dir" }); } @@ -60,7 +56,7 @@ vi.mock("node:fs", async (importOriginal) => { } } }), - rmSync: vi.fn(), + rmSync: vi.fn((p: string) => store.delete(p)), renameSync: vi.fn((oldPath: string, newPath: string) => { for (const [k, v] of store) { if (k === oldPath || k.startsWith(oldPath + "/")) { @@ -103,13 +99,13 @@ vi.mock("tar", () => ({ })); import { - detectHostOpenClaw, - createSnapshotBundle, cleanupSnapshotBundle, createArchiveFromDirectory, + createSnapshotBundle, + detectHostOpenClaw, + type HostOpenClawState, loadSnapshotManifest, restoreSnapshotToHost, - type HostOpenClawState, type SnapshotManifest, } from "./migration-state.js"; @@ -579,6 +575,8 @@ describe("commands/migration-state", () => { const logger = makeLogger(); addDir("/home/user/.openclaw"); addFile("/home/user/.openclaw/openclaw.json", JSON.stringify({ version: 1 })); + addDir("/home/user/.openclaw/agents"); + addDir("/home/user/.openclaw/agents/main"); addDir("/home/user/.openclaw/agents/main/agent"); addFile( "/home/user/.openclaw/agents/main/agent/auth-profiles.json", diff --git a/nemoclaw/src/commands/migration-state.ts b/nemoclaw/src/commands/migration-state.ts index 5f8b494402d..7335be304ae 100644 --- a/nemoclaw/src/commands/migration-state.ts +++ b/nemoclaw/src/commands/migration-state.ts @@ -1,6 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import { createHash } from "node:crypto"; import { chmodSync, copyFileSync, @@ -16,11 +17,14 @@ import { } from "node:fs"; import os from "node:os"; import path from "node:path"; -import { create as createTar } from "tar"; -import { createHash } from "node:crypto"; import JSON5 from "json5"; +import { create as createTar } from "tar"; import type { PluginLogger } from "../index.js"; -import { isSensitiveFile, stripCredentials } from "../security/credential-filter.js"; +import { isSensitiveFile } from "../security/credential-filter.js"; +import { + sanitizeMigrationDirectory, + sanitizeOpenClawConfigFile, +} from "../security/snapshot-sanitizer.js"; import { isObjectRecord, type UnknownRecord } from "../shared/object-record.js"; const SANDBOX_MIGRATION_DIR = "/sandbox/.nemoclaw/migration"; @@ -504,23 +508,6 @@ export function detectHostOpenClaw(env: NodeJS.ProcessEnv = process.env): HostOp }; } -// --------------------------------------------------------------------------- -// Credential sanitization (shared with nemoclaw/src/security/credential-filter) -// --------------------------------------------------------------------------- - -/** - * Strip credential fields from openclaw.json and remove the gateway - * config section (contains auth tokens — regenerated by sandbox entrypoint). - */ -function sanitizeConfigFile(configPath: string): void { - const config = loadConfigDocument(configPath); - if (!config) return; - delete config.gateway; - const sanitized = stripCredentials(config) as UnknownRecord; - writeFileSync(configPath, JSON.stringify(sanitized, null, 2)); - chmodSync(configPath, 0o600); -} - function computeFileDigest(filePath: string): string { if (!existsSync(filePath)) { throw new Error(`Blueprint file not found: ${filePath}`); @@ -699,7 +686,9 @@ function prepareSandboxState(snapshotDir: string, manifest: SnapshotManifest): s // 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. - sanitizeConfigFile(configPath); + if (!sanitizeOpenClawConfigFile(configPath)) { + throw new Error(`Failed to sanitize prepared OpenClaw config: ${configPath}`); + } return preparedStateDir; } @@ -726,14 +715,24 @@ export function createSnapshotBundle( mkdirSync(parentDir, { recursive: true }); const snapshotStateDir = path.join(parentDir, "openclaw"); copyDirectory(hostState.stateDir, snapshotStateDir, { stripCredentials: true }); - sanitizeConfigFile(path.join(snapshotStateDir, "openclaw.json")); + sanitizeMigrationDirectory(snapshotStateDir); + if ( + hostState.configPath && + !hostState.hasExternalConfig && + existsSync(hostState.configPath) && + !existsSync(path.join(snapshotStateDir, "openclaw.json")) + ) { + throw new Error("Failed to sanitize the copied OpenClaw configuration."); + } if (hostState.configPath && hostState.hasExternalConfig) { const configSnapshotDir = path.join(parentDir, "config"); mkdirSync(configSnapshotDir, { recursive: true }); const configSnapshotPath = path.join(configSnapshotDir, "openclaw.json"); copyFileSync(hostState.configPath, configSnapshotPath); - sanitizeConfigFile(configSnapshotPath); + if (!sanitizeOpenClawConfigFile(configSnapshotPath)) { + throw new Error("Failed to sanitize the copied external OpenClaw configuration."); + } } const externalRoots: MigrationExternalRoot[] = []; @@ -741,6 +740,7 @@ export function createSnapshotBundle( const destination = path.join(parentDir, root.snapshotRelativePath); mkdirSync(path.dirname(destination), { recursive: true }); copyDirectory(root.sourcePath, destination, { stripCredentials: true }); + sanitizeMigrationDirectory(destination); externalRoots.push({ ...root, symlinkPaths: collectSymlinkPaths(root.sourcePath), @@ -774,7 +774,18 @@ export function createSnapshotBundle( }; } catch (err: unknown) { const msg = err instanceof Error ? err.message : String(err); - logger.error(`Snapshot failed: ${msg}`); + let cleanupDetail = ""; + try { + rmSync(parentDir, { recursive: true, force: true }); + if (existsSync(parentDir)) { + cleanupDetail = " Incomplete snapshot cleanup did not remove the staging directory."; + } + } catch (cleanupError: unknown) { + cleanupDetail = ` Incomplete snapshot cleanup failed: ${ + cleanupError instanceof Error ? cleanupError.message : String(cleanupError) + }`; + } + logger.error(`Snapshot failed: ${msg}.${cleanupDetail}`); return null; } } diff --git a/nemoclaw/src/security/credential-filter.test.ts b/nemoclaw/src/security/credential-filter.test.ts index b8784d279a6..20a140720ed 100644 --- a/nemoclaw/src/security/credential-filter.test.ts +++ b/nemoclaw/src/security/credential-filter.test.ts @@ -8,6 +8,7 @@ import { isCredentialField, isSafeCredentialPlaceholder, isSensitiveFile, + sanitizeEnvFileContent, stripCredentials, valueLooksLikeSecret, } from "./credential-filter.js"; @@ -86,6 +87,23 @@ describe("plugin credential-filter", () => { expect(result.model).toBe("keep"); }); + it("strips secret-shaped env values stored under benign keys", () => { + const result = sanitizeEnvFileContent( + [ + "MODEL=keep-me", + "CUSTOM=ghp_abcdefghijklmnopqrstuvwxyz0123456789", + "ENDPOINT=Bearer opaque-migration-secret", + "SAFE=openshell:resolve:env:SAFE", + "", + ].join("\n"), + ); + + expect(result).toContain("MODEL=keep-me"); + expect(result).toContain("CUSTOM=[STRIPPED_BY_MIGRATION]"); + expect(result).toContain("ENDPOINT=[STRIPPED_BY_MIGRATION]"); + expect(result).toContain("SAFE=openshell:resolve:env:SAFE"); + }); + it("excludes auth state basenames from migration copies", () => { expect(isSensitiveFile("auth-profiles.json")).toBe(true); expect(isSensitiveFile("auth.json")).toBe(true); diff --git a/nemoclaw/src/security/credential-filter.ts b/nemoclaw/src/security/credential-filter.ts index 64980c53f36..bab6e15f5b8 100644 --- a/nemoclaw/src/security/credential-filter.ts +++ b/nemoclaw/src/security/credential-filter.ts @@ -191,6 +191,31 @@ export function stripCredentials(obj: unknown): unknown { return result; } +/** + * Strip credentials from a shell-style environment file body. + * + * Field-name matching handles ordinary secret variables while the value-shape + * backstop catches provider tokens stored under an otherwise benign key. + */ +export function sanitizeEnvFileContent(content: string): string { + return content + .split("\n") + .map((line) => { + const trimmed = line.trim(); + if (trimmed === "" || trimmed.startsWith("#")) return line; + const eq = line.indexOf("="); + if (eq <= 0) return line; + const rawKey = line.slice(0, eq).trim(); + const key = rawKey.replace(/^export\s+/i, "").trim(); + const value = line.slice(eq + 1); + if (isSafeCredentialPlaceholder(value)) return line; + if (!key) return line; + if (!isCredentialField(key) && !valueLooksLikeSecret(value)) return line; + return `${line.slice(0, eq)}=${CREDENTIAL_PLACEHOLDER}`; + }) + .join("\n"); +} + export function isSensitiveFile(filename: string): boolean { return CREDENTIAL_SENSITIVE_BASENAMES.has(filename.toLowerCase()); } diff --git a/nemoclaw/src/security/snapshot-sanitizer.test.ts b/nemoclaw/src/security/snapshot-sanitizer.test.ts new file mode 100644 index 00000000000..cee0c1abd6c --- /dev/null +++ b/nemoclaw/src/security/snapshot-sanitizer.test.ts @@ -0,0 +1,77 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { sanitizeMigrationDirectory, sanitizeOpenClawConfigFile } from "./snapshot-sanitizer.js"; + +const temporaryRoots: string[] = []; + +function makeRoot(): string { + const root = mkdtempSync(path.join(tmpdir(), "nemoclaw-migration-sanitizer-")); + temporaryRoots.push(root); + return root; +} + +afterEach(() => { + for (const root of temporaryRoots.splice(0)) rmSync(root, { force: true, recursive: true }); +}); + +describe("migration snapshot sanitizer", () => { + it("sanitizes credential-shaped values in every supported external artifact", () => { + const root = makeRoot(); + writeFileSync( + path.join(root, "config.json"), + JSON.stringify({ customValue: "ghp_abcdefghijklmnopqrstuvwxyz0123456789" }), + ); + writeFileSync(path.join(root, "config.yaml"), "api_key: sk-secret-value\nmodel: keep-me\n"); + writeFileSync( + path.join(root, "service.env"), + "BENIGN_NAME=Bearer opaque-secret\nLOG_LEVEL=info\n", + ); + + sanitizeMigrationDirectory(root); + + expect(readFileSync(path.join(root, "config.json"), "utf-8")).not.toContain("ghp_"); + expect(readFileSync(path.join(root, "config.yaml"), "utf-8")).not.toContain("sk-secret"); + expect(readFileSync(path.join(root, "config.yaml"), "utf-8")).toContain("model: keep-me"); + expect(readFileSync(path.join(root, "service.env"), "utf-8")).toContain( + "BENIGN_NAME=[STRIPPED_BY_MIGRATION]", + ); + expect(readFileSync(path.join(root, "service.env"), "utf-8")).toContain("LOG_LEVEL=info"); + }); + + it("omits malformed optional artifacts", () => { + const root = makeRoot(); + const nested = path.join(root, "nested"); + mkdirSync(nested); + writeFileSync(path.join(nested, "broken.json"), '{"apiKey":'); + writeFileSync(path.join(nested, "broken.yaml"), "api_key: [unclosed\n"); + + sanitizeMigrationDirectory(root); + + expect(() => readFileSync(path.join(nested, "broken.json"))).toThrow(); + expect(() => readFileSync(path.join(nested, "broken.yaml"))).toThrow(); + }); + + it("fails closed for an unparsable required OpenClaw configuration", () => { + const root = makeRoot(); + const configPath = path.join(root, "openclaw.json"); + writeFileSync(configPath, '{"apiKey":'); + + expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); + }); + + it("preserves empty and comment-only YAML artifacts", () => { + const root = makeRoot(); + writeFileSync(path.join(root, "empty.yaml"), ""); + writeFileSync(path.join(root, "comments.yaml"), "# retained context\n"); + + sanitizeMigrationDirectory(root); + + expect(readFileSync(path.join(root, "empty.yaml"), "utf-8")).toBe(""); + expect(readFileSync(path.join(root, "comments.yaml"), "utf-8")).toBe("# retained context\n"); + }); +}); diff --git a/nemoclaw/src/security/snapshot-sanitizer.ts b/nemoclaw/src/security/snapshot-sanitizer.ts new file mode 100644 index 00000000000..033c0b34009 --- /dev/null +++ b/nemoclaw/src/security/snapshot-sanitizer.ts @@ -0,0 +1,144 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { randomUUID } from "node:crypto"; +import { + chmodSync, + existsSync, + lstatSync, + readdirSync, + readFileSync, + renameSync, + rmSync, + writeFileSync, +} from "node:fs"; +import path from "node:path"; +import JSON5 from "json5"; +import { parse as parseYaml, stringify as stringifyYaml } from "yaml"; +import { isObjectRecord } from "../shared/object-record.js"; +import { + CREDENTIAL_PLACEHOLDER, + isSafeCredentialPlaceholder, + isSensitiveFile, + sanitizeEnvFileContent, + stripCredentials, + valueLooksLikeSecret, +} from "./credential-filter.js"; + +function sanitizeTopLevelValue(value: unknown): unknown { + if (typeof value !== "string" || isSafeCredentialPlaceholder(value)) { + return stripCredentials(value); + } + return valueLooksLikeSecret(value) ? CREDENTIAL_PLACEHOLDER : value; +} + +function withoutGateway(value: unknown): unknown { + if (!isObjectRecord(value)) return value; + const { gateway: _gateway, ...config } = value; + return config; +} + +function writeSnapshotFile(filePath: string, contents: string): boolean { + const temporaryPath = `${filePath}.${String(process.pid)}.${randomUUID()}.tmp`; + try { + writeFileSync(temporaryPath, contents, { mode: 0o600 }); + renameSync(temporaryPath, filePath); + chmodSync(filePath, 0o600); + return true; + } catch { + rmSync(temporaryPath, { force: true }); + return false; + } +} + +function readRegularSnapshotFile(filePath: string): string | null { + try { + const stat = lstatSync(filePath); + if (!stat.isFile() || stat.isSymbolicLink()) return null; + return readFileSync(filePath, "utf-8"); + } catch { + return null; + } +} + +function sanitizeJsonFile(filePath: string): boolean { + const raw = readRegularSnapshotFile(filePath); + if (raw === null) return false; + try { + const parsed: unknown = JSON5.parse(raw); + const sanitized = sanitizeTopLevelValue(withoutGateway(parsed)); + return writeSnapshotFile(filePath, JSON.stringify(sanitized, null, 2)); + } catch { + return false; + } +} + +function sanitizeYamlFile(filePath: string): boolean { + const raw = readRegularSnapshotFile(filePath); + if (raw === null) return false; + try { + const parsed: unknown = parseYaml(raw); + if (parsed === null || parsed === undefined) return true; + const sanitized = sanitizeTopLevelValue(withoutGateway(parsed)); + return writeSnapshotFile(filePath, stringifyYaml(sanitized)); + } catch { + return false; + } +} + +function sanitizeEnvFile(filePath: string): boolean { + const raw = readRegularSnapshotFile(filePath); + if (raw === null) return false; + return writeSnapshotFile(filePath, sanitizeEnvFileContent(raw)); +} + +function removeUnsafeArtifact(filePath: string): void { + rmSync(filePath, { force: true }); + if (existsSync(filePath)) { + throw new Error(`Unable to remove unsanitizable migration artifact: ${filePath}`); + } +} + +function sanitizeFile(filePath: string): void { + const name = path.basename(filePath).toLowerCase(); + if (isSensitiveFile(name)) { + removeUnsafeArtifact(filePath); + return; + } + + let sanitized = true; + if (name.endsWith(".json")) { + sanitized = sanitizeJsonFile(filePath); + } else if (name.endsWith(".yaml") || name.endsWith(".yml")) { + sanitized = sanitizeYamlFile(filePath); + } else if (name === ".env" || name.endsWith(".env")) { + sanitized = sanitizeEnvFile(filePath); + } + if (!sanitized) removeUnsafeArtifact(filePath); +} + +/** + * Recursively sanitize every copied migration artifact before it can be + * archived or prepared for the sandbox. Symlinks are left untouched for the + * existing manifest audit and are never followed by this walk. + */ +export function sanitizeMigrationDirectory(rootPath: string): void { + if (!existsSync(rootPath)) return; + for (const entry of readdirSync(rootPath)) { + const fullPath = path.join(rootPath, entry); + const stat = lstatSync(fullPath); + if (stat.isSymbolicLink()) continue; + if (stat.isDirectory()) { + sanitizeMigrationDirectory(fullPath); + } else if (stat.isFile()) { + sanitizeFile(fullPath); + } + } +} + +/** + * Sanitize a required OpenClaw configuration copy. + */ +export function sanitizeOpenClawConfigFile(configPath: string): boolean { + return sanitizeJsonFile(configPath); +} diff --git a/src/lib/security/credential-filter.test.ts b/src/lib/security/credential-filter.test.ts index 0c93c7937be..e9f9464aecb 100644 --- a/src/lib/security/credential-filter.test.ts +++ b/src/lib/security/credential-filter.test.ts @@ -272,6 +272,18 @@ describe("sanitizeConfigFile", () => { expect(readFileSync(configPath, "utf-8")).toBe(source); }); + it("preserves empty and comment-only YAML documents", () => { + for (const [name, source] of [ + ["empty.yaml", ""], + ["comments.yaml", "# nothing to sanitize\n"], + ]) { + const configPath = join(tmpDir, name); + writeFileSync(configPath, source); + expect(sanitizeYamlConfigFile(configPath)).toBe(true); + expect(readFileSync(configPath, "utf-8")).toBe(source); + } + }); + it("sanitizes valid JSON arrays instead of treating them as failures", () => { const configPath = join(tmpDir, "config.json"); writeFileSync(configPath, JSON.stringify([{ apiKey: "sk-secret-value-long-enough" }])); @@ -321,6 +333,22 @@ describe("sanitizeEnvFileContent", () => { expect(result).not.toContain("super-secret"); expect(result).not.toContain("ghp_abcdefghijklmnopqrstuvwxyz0123456789"); }); + + it("strips secret-shaped values stored under benign keys", () => { + const input = [ + "MODEL=keep-me", + "CUSTOM=ghp_abcdefghijklmnopqrstuvwxyz0123456789", + "ENDPOINT=Bearer opaque-migration-secret", + "SAFE=openshell:resolve:env:SAFE", + "", + ].join("\n"); + + const result = sanitizeEnvFileContent(input); + expect(result).toContain("MODEL=keep-me"); + expect(result).toContain("CUSTOM=[STRIPPED_BY_MIGRATION]"); + expect(result).toContain("ENDPOINT=[STRIPPED_BY_MIGRATION]"); + expect(result).toContain("SAFE=openshell:resolve:env:SAFE"); + }); }); describe("sanitizeEnvFile", () => { diff --git a/src/lib/security/credential-filter.ts b/src/lib/security/credential-filter.ts index b60f26e79c3..aa1a0052971 100644 --- a/src/lib/security/credential-filter.ts +++ b/src/lib/security/credential-filter.ts @@ -378,9 +378,10 @@ export function sanitizeEnvFileContent(content: string): string { const rawKey = line.slice(0, eq).trim(); // Shell-sourced .env files often use `export KEY=value`. const key = rawKey.replace(/^export\s+/i, "").trim(); - if (!key || !isCredentialField(key)) return line; const value = line.slice(eq + 1); if (isSafeCredentialPlaceholder(value)) return line; + if (!key) return line; + if (!isCredentialField(key) && !valueLooksLikeSecret(value)) return line; return `${line.slice(0, eq)}=${CREDENTIAL_PLACEHOLDER}`; }) .join("\n"); @@ -442,6 +443,9 @@ export function sanitizeYamlConfigFile( if (rawConfig === null) return false; try { const parsed = parseYaml(rawConfig); + // Empty and comment-only YAML documents contain no credentials. Preserve + // them instead of misclassifying the parser's null result as unsafe. + if (parsed === null || parsed === undefined) return true; const configValue = toConfigValue(parsed); if (configValue === UNREPRESENTABLE_CONFIG_VALUE || !isConfigObject(configValue)) { return false; diff --git a/src/lib/state/sandbox-backup-sanitization.test.ts b/src/lib/state/sandbox-backup-sanitization.test.ts index a3d089bbf29..45a7260ee10 100644 --- a/src/lib/state/sandbox-backup-sanitization.test.ts +++ b/src/lib/state/sandbox-backup-sanitization.test.ts @@ -1,7 +1,15 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + statSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -24,6 +32,19 @@ afterEach(() => { }); describe("rebuild backup credential sanitization", () => { + it("sanitizes a real env file and restricts its mode", () => { + const backupPath = createBackup(); + const envPath = join(backupPath, "state", ".env"); + writeFileSync(envPath, "CUSTOM=ghp_abcdefghijklmnopqrstuvwxyz0123456789\nLOG_LEVEL=info\n", { + mode: 0o644, + }); + + sanitizeBackupDirectory(backupPath); + + expect(readFileSync(envPath, "utf-8")).toBe("CUSTOM=[STRIPPED_BY_MIGRATION]\nLOG_LEVEL=info\n"); + expect(statSync(envPath).mode & 0o777).toBe(0o600); + }); + it("omits unsanitizable config and env artifacts", () => { const backupPath = createBackup(); const yamlPath = join(backupPath, "state", "config.yaml"); @@ -59,4 +80,21 @@ describe("rebuild backup credential sanitization", () => { ).toThrow("Credential sanitization failed; removed the incomplete backup"); expect(existsSync(backupPath)).toBe(false); }); + + it("reports when cleanup leaves an incomplete backup behind", () => { + const backupPath = createBackup(); + const yamlPath = join(backupPath, "state", "config.yaml"); + writeFileSync(yamlPath, "api_key: [unclosed\n"); + + expect(() => + sanitizeBackupDirectory(backupPath, { + unlinkFile: () => { + throw new Error("injected unlink failure"); + }, + removeBackup: () => undefined, + backupExists: () => true, + }), + ).toThrow("Credential sanitization failed and the incomplete backup remains"); + expect(existsSync(backupPath)).toBe(true); + }); }); diff --git a/test/credential-filter-parity.test.ts b/test/credential-filter-parity.test.ts new file mode 100644 index 00000000000..fb2d92ebd60 --- /dev/null +++ b/test/credential-filter-parity.test.ts @@ -0,0 +1,80 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { + isCredentialField as isPluginCredentialField, + isSafeCredentialPlaceholder as isPluginSafePlaceholder, + valueLooksLikeSecret as pluginValueLooksLikeSecret, + sanitizeEnvFileContent as sanitizePluginEnvFileContent, + stripCredentials as stripPluginCredentials, +} from "../nemoclaw/src/security/credential-filter.js"; +import { + isCredentialField as isSharedCredentialField, + isSafeCredentialPlaceholder as isSharedSafePlaceholder, + sanitizeEnvFileContent as sanitizeSharedEnvFileContent, + valueLooksLikeSecret as sharedValueLooksLikeSecret, + stripCredentials as stripSharedCredentials, +} from "../src/lib/security/credential-filter.js"; + +describe("credential filter parity", () => { + it("keeps plugin and shared classification rules aligned", () => { + const fields = [ + "botToken", + "appToken", + "GITHUB_TOKEN", + "Authorization", + "X-API-Key", + "DB_PASS", + "publicKey", + "NODE_ENV", + "model", + ]; + const values = [ + "ghp_abcdefghijklmnopqrstuvwxyz0123456789", + "Bearer opaque-migration-secret", + "openshell:resolve:env:GITHUB_TOKEN", + "Bearer openshell:resolve:env:REMOTE_MCP_TOKEN", + "keep-me", + ]; + + for (const field of fields) { + expect(isPluginCredentialField(field), field).toBe(isSharedCredentialField(field)); + } + for (const value of values) { + expect(pluginValueLooksLikeSecret(value), value).toBe(sharedValueLooksLikeSecret(value)); + expect(isPluginSafePlaceholder(value), value).toBe(isSharedSafePlaceholder(value)); + } + }); + + it("produces identical object and env-file sanitization", () => { + const fixture = { + channels: { + slack: { + accounts: { + default: { + botToken: "xoxb-raw-slack-token", + appToken: "xapp-raw-app-token", + }, + }, + }, + }, + mcp: { + headers: { Authorization: "Bearer opaque-migration-secret" }, + env: { GITHUB_TOKEN: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", NODE_ENV: "test" }, + args: ["--api-key", "opaque-secret-value", "--verbose"], + }, + model: "keep-me", + }; + const envFixture = [ + "CUSTOM=ghp_abcdefghijklmnopqrstuvwxyz0123456789", + "ENDPOINT=Bearer opaque-migration-secret", + "NODE_ENV=production", + "", + ].join("\n"); + + expect(stripPluginCredentials(fixture)).toEqual(stripSharedCredentials(fixture)); + expect(sanitizePluginEnvFileContent(envFixture)).toBe(sanitizeSharedEnvFileContent(envFixture)); + }); +}); From 993284c7ca1055803ec8d8594271f7e1ae87eec4 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 29 Jul 2026 15:00:28 -0700 Subject: [PATCH 08/19] fix(security): harden migration snapshot sanitization Signed-off-by: Apurv Kumaria --- ci/test-file-size-budget.json | 2 +- nemoclaw/src/commands/migration-state.test.ts | 21 +++- .../snapshot-sanitizer-failure.test.ts | 101 ++++++++++++++++++ .../src/security/snapshot-sanitizer.test.ts | 82 +++++++++++++- nemoclaw/src/security/snapshot-sanitizer.ts | 20 +++- 5 files changed, 219 insertions(+), 7 deletions(-) create mode 100644 nemoclaw/src/security/snapshot-sanitizer-failure.test.ts diff --git a/ci/test-file-size-budget.json b/ci/test-file-size-budget.json index 7fb675bf155..c3ae8824d85 100644 --- a/ci/test-file-size-budget.json +++ b/ci/test-file-size-budget.json @@ -2,7 +2,7 @@ "$comment": "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.\nSPDX-License-Identifier: Apache-2.0", "defaultMaxLines": 1500, "legacyMaxLines": { - "nemoclaw/src/commands/migration-state.test.ts": 1563, + "nemoclaw/src/commands/migration-state.test.ts": 1580, "src/lib/inference/nim.test.ts": 2068, "src/lib/onboard/preflight.test.ts": 1904, "test/generate-openclaw-config.test.ts": 1941, diff --git a/nemoclaw/src/commands/migration-state.test.ts b/nemoclaw/src/commands/migration-state.test.ts index 83ad2b37c2d..36cfb85c1fe 100644 --- a/nemoclaw/src/commands/migration-state.test.ts +++ b/nemoclaw/src/commands/migration-state.test.ts @@ -12,6 +12,8 @@ interface FsEntry { content?: string; } const store = new Map(); +const descriptors = new Map(); +let nextDescriptor = 100; function addDir(p: string): void { store.set(p, { type: "dir" }); } @@ -33,11 +35,25 @@ vi.mock("node:fs", async (importOriginal) => { addDir(p); }), chmodSync: vi.fn(), - readFileSync: (p: string) => { + readFileSync: (p: string | number) => { + const resolvedPath = typeof p === "number" ? descriptors.get(p) : p; + if (!resolvedPath) throw new Error(`EBADF: ${String(p)}`); + const entry = store.get(resolvedPath); + if (entry?.type !== "file") throw new Error(`ENOENT: ${resolvedPath}`); + return entry.content ?? ""; + }, + openSync: (p: string) => { const entry = store.get(p); if (entry?.type !== "file") throw new Error(`ENOENT: ${p}`); - return entry.content ?? ""; + const fd = nextDescriptor++; + descriptors.set(fd, p); + return fd; + }, + fstatSync: (fd: number) => { + const entry = store.get(descriptors.get(fd) ?? ""); + return { isFile: () => entry?.type === "file" }; }, + closeSync: (fd: number) => descriptors.delete(fd), writeFileSync: vi.fn((p: string, data: string) => { store.set(p, { type: "file", content: data }); }), @@ -121,6 +137,7 @@ function makeLogger(): PluginLogger { describe("commands/migration-state", () => { beforeEach(() => { store.clear(); + descriptors.clear(); vi.clearAllMocks(); }); diff --git a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts new file mode 100644 index 00000000000..126957d985f --- /dev/null +++ b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts @@ -0,0 +1,101 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const fsControl = vi.hoisted(() => ({ + failOpenNames: [] as string[], + preventRemoval: false, +})); + +vi.mock("node:fs", async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + constants: { ...original.constants, O_NOFOLLOW: undefined }, + openSync: (target: Parameters[0], flags: number) => { + if (fsControl.failOpenNames.some((name) => String(target).endsWith(name))) { + throw new Error(`simulated open failure: ${String(target)}`); + } + return original.openSync(target, flags); + }, + rmSync: (target: Parameters[0], options?: { force?: boolean }) => { + if (fsControl.preventRemoval && String(target).endsWith("auth.json")) return; + original.rmSync(target, options); + }, + }; +}); + +import { sanitizeMigrationDirectory, sanitizeOpenClawConfigFile } from "./snapshot-sanitizer.js"; + +const roots: string[] = []; + +function makeRoot(): string { + const root = mkdtempSync(path.join(tmpdir(), "nemoclaw-migration-sanitizer-failure-")); + roots.push(root); + return root; +} + +afterEach(() => { + fsControl.failOpenNames = []; + fsControl.preventRemoval = false; + for (const root of roots.splice(0)) rmSync(root, { force: true, recursive: true }); +}); + +describe("migration snapshot sanitizer fallbacks", () => { + it("validates a regular file when O_NOFOLLOW is unavailable", () => { + const configPath = path.join(makeRoot(), "openclaw.json"); + writeFileSync(configPath, JSON.stringify({ apiKey: "sk-secret-value" })); + + expect(sanitizeOpenClawConfigFile(configPath)).toBe(true); + expect(JSON.parse(readFileSync(configPath, "utf-8"))).toEqual({ + apiKey: "[STRIPPED_BY_MIGRATION]", + }); + }); + + it("rejects a symlink when O_NOFOLLOW is unavailable", () => { + const root = makeRoot(); + const targetPath = path.join(root, "target.json"); + const configPath = path.join(root, "openclaw.json"); + writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); + try { + symlinkSync(targetPath, configPath); + } catch (error) { + const code = error && typeof error === "object" ? (error as { code?: string }).code : ""; + if (code === "EPERM" || code === "EACCES") return; + throw error; + } + + expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); + expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ + apiKey: "sk-secret-value", + }); + }); + + it("fails closed when a sensitive artifact cannot be removed", () => { + const root = makeRoot(); + writeFileSync(path.join(root, "auth.json"), JSON.stringify({ token: "raw" })); + fsControl.preventRemoval = true; + + expect(() => sanitizeMigrationDirectory(root)).toThrow( + /Unable to remove unsanitizable migration artifact/u, + ); + }); + + it("omits YAML and env artifacts that cannot be opened safely", () => { + const root = makeRoot(); + const yamlPath = path.join(root, "blocked.yaml"); + const envPath = path.join(root, "blocked.env"); + writeFileSync(yamlPath, "model: keep-me\n"); + writeFileSync(envPath, "MODEL=keep-me\n"); + fsControl.failOpenNames = ["blocked.yaml", "blocked.env"]; + + sanitizeMigrationDirectory(root); + + expect(() => readFileSync(yamlPath)).toThrow(); + expect(() => readFileSync(envPath)).toThrow(); + }); +}); diff --git a/nemoclaw/src/security/snapshot-sanitizer.test.ts b/nemoclaw/src/security/snapshot-sanitizer.test.ts index cee0c1abd6c..cd26b9e7b9d 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.test.ts @@ -1,7 +1,15 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -43,6 +51,42 @@ describe("migration snapshot sanitizer", () => { expect(readFileSync(path.join(root, "service.env"), "utf-8")).toContain("LOG_LEVEL=info"); }); + it("sanitizes secret-shaped scalar JSON and preserves benign scalar JSON", () => { + const root = makeRoot(); + const secretPath = path.join(root, "secret.json"); + const benignPath = path.join(root, "benign.json"); + writeFileSync(secretPath, JSON.stringify("ghp_abcdefghijklmnopqrstuvwxyz0123456789")); + writeFileSync(benignPath, JSON.stringify("keep-me")); + + sanitizeMigrationDirectory(root); + + expect(JSON.parse(readFileSync(secretPath, "utf-8"))).toBe("[STRIPPED_BY_MIGRATION]"); + expect(JSON.parse(readFileSync(benignPath, "utf-8"))).toBe("keep-me"); + }); + + it("removes sensitive files without following unrelated symlinks", () => { + const root = makeRoot(); + const targetPath = path.join(root, "target.json"); + const linkPath = path.join(root, "linked.json"); + writeFileSync(path.join(root, "auth.json"), JSON.stringify({ token: "raw" })); + writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); + try { + symlinkSync(targetPath, linkPath); + } catch (error) { + const code = error && typeof error === "object" ? (error as { code?: string }).code : ""; + if (code === "EPERM" || code === "EACCES") return; + throw error; + } + + sanitizeMigrationDirectory(path.join(root, "missing")); + sanitizeMigrationDirectory(root); + + expect(() => readFileSync(path.join(root, "auth.json"))).toThrow(); + expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ + apiKey: "[STRIPPED_BY_MIGRATION]", + }); + }); + it("omits malformed optional artifacts", () => { const root = makeRoot(); const nested = path.join(root, "nested"); @@ -64,6 +108,42 @@ describe("migration snapshot sanitizer", () => { expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); }); + it("does not follow a required OpenClaw configuration symlink", () => { + const root = makeRoot(); + const targetPath = path.join(root, "target.json"); + const configPath = path.join(root, "openclaw.json"); + writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); + try { + symlinkSync(targetPath, configPath); + } catch (error) { + const code = error && typeof error === "object" ? (error as { code?: string }).code : ""; + if (code === "EPERM" || code === "EACCES") return; + throw error; + } + + expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); + expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ + apiKey: "sk-secret-value", + }); + }); + + it("rejects a non-regular required OpenClaw configuration", () => { + const root = makeRoot(); + expect(sanitizeOpenClawConfigFile(root)).toBe(false); + }); + + it("fails closed when a required sanitized file cannot be written", () => { + const root = makeRoot(); + const configPath = path.join(root, "openclaw.json"); + writeFileSync(configPath, JSON.stringify({ apiKey: "sk-secret-value" })); + chmodSync(root, 0o500); + try { + expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); + } finally { + chmodSync(root, 0o700); + } + }); + it("preserves empty and comment-only YAML artifacts", () => { const root = makeRoot(); writeFileSync(path.join(root, "empty.yaml"), ""); diff --git a/nemoclaw/src/security/snapshot-sanitizer.ts b/nemoclaw/src/security/snapshot-sanitizer.ts index 033c0b34009..faf4d806ceb 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.ts @@ -4,8 +4,12 @@ import { randomUUID } from "node:crypto"; import { chmodSync, + closeSync, + constants, existsSync, + fstatSync, lstatSync, + openSync, readdirSync, readFileSync, renameSync, @@ -52,13 +56,23 @@ function writeSnapshotFile(filePath: string, contents: string): boolean { } function readRegularSnapshotFile(filePath: string): string | null { + let fd: number; try { - const stat = lstatSync(filePath); - if (!stat.isFile() || stat.isSymbolicLink()) return null; - return readFileSync(filePath, "utf-8"); + if (typeof constants.O_NOFOLLOW !== "number") { + const stat = lstatSync(filePath); + if (!stat.isFile() || stat.isSymbolicLink()) return null; + } + const noFollowFlag = typeof constants.O_NOFOLLOW === "number" ? constants.O_NOFOLLOW : 0; + fd = openSync(filePath, constants.O_RDONLY | noFollowFlag); } catch { return null; } + try { + if (!fstatSync(fd).isFile()) return null; + return String(readFileSync(fd, "utf-8")); + } finally { + closeSync(fd); + } } function sanitizeJsonFile(filePath: string): boolean { From e3660863a36b56a2b212a626871a1761c3499174 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 29 Jul 2026 15:16:47 -0700 Subject: [PATCH 09/19] test(security): restore migration growth budget Signed-off-by: Apurv Kumaria --- ci/test-file-size-budget.json | 2 +- nemoclaw/src/commands/migration-state.test.ts | 44 ++++++------------- 2 files changed, 15 insertions(+), 31 deletions(-) diff --git a/ci/test-file-size-budget.json b/ci/test-file-size-budget.json index c3ae8824d85..1b0ad7132ca 100644 --- a/ci/test-file-size-budget.json +++ b/ci/test-file-size-budget.json @@ -2,7 +2,7 @@ "$comment": "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.\nSPDX-License-Identifier: Apache-2.0", "defaultMaxLines": 1500, "legacyMaxLines": { - "nemoclaw/src/commands/migration-state.test.ts": 1580, + "nemoclaw/src/commands/migration-state.test.ts": 1564, "src/lib/inference/nim.test.ts": 2068, "src/lib/onboard/preflight.test.ts": 1904, "test/generate-openclaw-config.test.ts": 1941, diff --git a/nemoclaw/src/commands/migration-state.test.ts b/nemoclaw/src/commands/migration-state.test.ts index 36cfb85c1fe..79f3c93d591 100644 --- a/nemoclaw/src/commands/migration-state.test.ts +++ b/nemoclaw/src/commands/migration-state.test.ts @@ -6,24 +6,18 @@ import type { PluginLogger } from "../index.js"; import { setConfigValue } from "./migration-state.js"; // fs mock — thin in-memory store keyed by absolute path - -interface FsEntry { - type: "file" | "dir" | "symlink"; - content?: string; -} +type FsEntry = { type: "file" | "dir" | "symlink"; content?: string }; const store = new Map(); const descriptors = new Map(); -let nextDescriptor = 100; -function addDir(p: string): void { - store.set(p, { type: "dir" }); -} - -function addFile(p: string, content: string): void { - store.set(p, { type: "file", content }); -} - -function addSymlink(p: string): void { - store.set(p, { type: "symlink" }); +const addDir = (p: string): void => void store.set(p, { type: "dir" }); +const addFile = (p: string, content: string): void => void store.set(p, { type: "file", content }); +const addSymlink = (p: string): void => void store.set(p, { type: "symlink" }); +function fileAt(p: string | number): FsEntry { + const resolvedPath = typeof p === "number" ? descriptors.get(p) : p; + if (!resolvedPath) throw new Error(`EBADF: ${String(p)}`); + const entry = store.get(resolvedPath); + if (entry?.type !== "file") throw new Error(`ENOENT: ${resolvedPath}`); + return entry; } vi.mock("node:fs", async (importOriginal) => { @@ -35,24 +29,14 @@ vi.mock("node:fs", async (importOriginal) => { addDir(p); }), chmodSync: vi.fn(), - readFileSync: (p: string | number) => { - const resolvedPath = typeof p === "number" ? descriptors.get(p) : p; - if (!resolvedPath) throw new Error(`EBADF: ${String(p)}`); - const entry = store.get(resolvedPath); - if (entry?.type !== "file") throw new Error(`ENOENT: ${resolvedPath}`); - return entry.content ?? ""; - }, + readFileSync: (p: string | number) => fileAt(p).content ?? "", openSync: (p: string) => { - const entry = store.get(p); - if (entry?.type !== "file") throw new Error(`ENOENT: ${p}`); - const fd = nextDescriptor++; + fileAt(p); + const fd = Math.max(99, ...descriptors.keys()) + 1; descriptors.set(fd, p); return fd; }, - fstatSync: (fd: number) => { - const entry = store.get(descriptors.get(fd) ?? ""); - return { isFile: () => entry?.type === "file" }; - }, + fstatSync: (fd: number) => ({ isFile: () => fileAt(fd).type === "file" }), closeSync: (fd: number) => descriptors.delete(fd), writeFileSync: vi.fn((p: string, data: string) => { store.set(p, { type: "file", content: data }); From f269b3823b74f8fa208b7c58790e0e6e5b3ce934 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Thu, 30 Jul 2026 11:50:59 -0700 Subject: [PATCH 10/19] fix(security): fail closed without no-follow support Signed-off-by: Apurv Kumaria --- nemoclaw/src/commands/migration-state.test.ts | 8 +- .../snapshot-sanitizer-failure.test.ts | 68 ++++++++++---- .../src/security/snapshot-sanitizer.test.ts | 88 +++++++------------ nemoclaw/src/security/snapshot-sanitizer.ts | 8 +- 4 files changed, 89 insertions(+), 83 deletions(-) diff --git a/nemoclaw/src/commands/migration-state.test.ts b/nemoclaw/src/commands/migration-state.test.ts index 79f3c93d591..59c772a5dd5 100644 --- a/nemoclaw/src/commands/migration-state.test.ts +++ b/nemoclaw/src/commands/migration-state.test.ts @@ -12,12 +12,12 @@ const descriptors = new Map(); const addDir = (p: string): void => void store.set(p, { type: "dir" }); const addFile = (p: string, content: string): void => void store.set(p, { type: "file", content }); const addSymlink = (p: string): void => void store.set(p, { type: "symlink" }); +// Keep mock failures explicit without adding control-flow branches to the test budget. function fileAt(p: string | number): FsEntry { const resolvedPath = typeof p === "number" ? descriptors.get(p) : p; - if (!resolvedPath) throw new Error(`EBADF: ${String(p)}`); - const entry = store.get(resolvedPath); - if (entry?.type !== "file") throw new Error(`ENOENT: ${resolvedPath}`); - return entry; + const entry = resolvedPath === undefined ? undefined : store.get(resolvedPath); + expect(entry?.type, `expected file at ${String(resolvedPath ?? p)}`).toBe("file"); + return entry as FsEntry; } vi.mock("node:fs", async (importOriginal) => { diff --git a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts index 126957d985f..fe9cb29686b 100644 --- a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts @@ -8,6 +8,8 @@ import { afterEach, describe, expect, it, vi } from "vitest"; const fsControl = vi.hoisted(() => ({ failOpenNames: [] as string[], + failWriteNames: [] as string[], + noFollowUnavailable: false, preventRemoval: false, })); @@ -15,16 +17,37 @@ vi.mock("node:fs", async (importOriginal) => { const original = await importOriginal(); return { ...original, - constants: { ...original.constants, O_NOFOLLOW: undefined }, + constants: { + ...original.constants, + get O_NOFOLLOW(): number | undefined { + return fsControl.noFollowUnavailable ? undefined : original.constants.O_NOFOLLOW; + }, + }, openSync: (target: Parameters[0], flags: number) => { - if (fsControl.failOpenNames.some((name) => String(target).endsWith(name))) { - throw new Error(`simulated open failure: ${String(target)}`); + switch (fsControl.failOpenNames.some((name) => String(target).endsWith(name))) { + case true: + throw new Error(`simulated open failure: ${String(target)}`); + default: + return original.openSync(target, flags); + } + }, + rmSync: (...args: Parameters) => { + const [target] = args; + switch (fsControl.preventRemoval && String(target).endsWith("auth.json")) { + case true: + return; + default: + return original.rmSync(...args); } - return original.openSync(target, flags); }, - rmSync: (target: Parameters[0], options?: { force?: boolean }) => { - if (fsControl.preventRemoval && String(target).endsWith("auth.json")) return; - original.rmSync(target, options); + writeFileSync: (...args: Parameters) => { + const [target] = args; + switch (fsControl.failWriteNames.some((name) => String(target).includes(name))) { + case true: + throw new Error(`simulated write failure: ${String(target)}`); + default: + return original.writeFileSync(...args); + } }, }; }); @@ -41,33 +64,31 @@ function makeRoot(): string { afterEach(() => { fsControl.failOpenNames = []; + fsControl.failWriteNames = []; + fsControl.noFollowUnavailable = false; fsControl.preventRemoval = false; for (const root of roots.splice(0)) rmSync(root, { force: true, recursive: true }); }); describe("migration snapshot sanitizer fallbacks", () => { - it("validates a regular file when O_NOFOLLOW is unavailable", () => { + it("fails closed for a regular file when O_NOFOLLOW is unavailable", () => { const configPath = path.join(makeRoot(), "openclaw.json"); writeFileSync(configPath, JSON.stringify({ apiKey: "sk-secret-value" })); + fsControl.noFollowUnavailable = true; - expect(sanitizeOpenClawConfigFile(configPath)).toBe(true); + expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); expect(JSON.parse(readFileSync(configPath, "utf-8"))).toEqual({ - apiKey: "[STRIPPED_BY_MIGRATION]", + apiKey: "sk-secret-value", }); }); - it("rejects a symlink when O_NOFOLLOW is unavailable", () => { + it.runIf(process.platform !== "win32")("rejects a symlink when O_NOFOLLOW is unavailable", () => { const root = makeRoot(); const targetPath = path.join(root, "target.json"); const configPath = path.join(root, "openclaw.json"); writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); - try { - symlinkSync(targetPath, configPath); - } catch (error) { - const code = error && typeof error === "object" ? (error as { code?: string }).code : ""; - if (code === "EPERM" || code === "EACCES") return; - throw error; - } + symlinkSync(targetPath, configPath); + fsControl.noFollowUnavailable = true; expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ @@ -75,6 +96,17 @@ describe("migration snapshot sanitizer fallbacks", () => { }); }); + it("fails closed when a required sanitized file cannot be written", () => { + const configPath = path.join(makeRoot(), "openclaw.json"); + writeFileSync(configPath, JSON.stringify({ apiKey: "sk-secret-value" })); + fsControl.failWriteNames = ["openclaw.json"]; + + expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); + expect(JSON.parse(readFileSync(configPath, "utf-8"))).toEqual({ + apiKey: "sk-secret-value", + }); + }); + it("fails closed when a sensitive artifact cannot be removed", () => { const root = makeRoot(); writeFileSync(path.join(root, "auth.json"), JSON.stringify({ token: "raw" })); diff --git a/nemoclaw/src/security/snapshot-sanitizer.test.ts b/nemoclaw/src/security/snapshot-sanitizer.test.ts index cd26b9e7b9d..94695fa85b4 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.test.ts @@ -1,15 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { - chmodSync, - mkdirSync, - mkdtempSync, - readFileSync, - rmSync, - symlinkSync, - writeFileSync, -} from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; @@ -64,28 +56,26 @@ describe("migration snapshot sanitizer", () => { expect(JSON.parse(readFileSync(benignPath, "utf-8"))).toBe("keep-me"); }); - it("removes sensitive files without following unrelated symlinks", () => { - const root = makeRoot(); - const targetPath = path.join(root, "target.json"); - const linkPath = path.join(root, "linked.json"); - writeFileSync(path.join(root, "auth.json"), JSON.stringify({ token: "raw" })); - writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); - try { + it.runIf(process.platform !== "win32")( + "removes sensitive files without following unrelated symlinks", + () => { + const root = makeRoot(); + const externalRoot = makeRoot(); + const targetPath = path.join(externalRoot, "target.json"); + const linkPath = path.join(root, "linked.json"); + writeFileSync(path.join(root, "auth.json"), JSON.stringify({ token: "raw" })); + writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); symlinkSync(targetPath, linkPath); - } catch (error) { - const code = error && typeof error === "object" ? (error as { code?: string }).code : ""; - if (code === "EPERM" || code === "EACCES") return; - throw error; - } - sanitizeMigrationDirectory(path.join(root, "missing")); - sanitizeMigrationDirectory(root); + sanitizeMigrationDirectory(path.join(root, "missing")); + sanitizeMigrationDirectory(root); - expect(() => readFileSync(path.join(root, "auth.json"))).toThrow(); - expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ - apiKey: "[STRIPPED_BY_MIGRATION]", - }); - }); + expect(() => readFileSync(path.join(root, "auth.json"))).toThrow(); + expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ + apiKey: "sk-secret-value", + }); + }, + ); it("omits malformed optional artifacts", () => { const root = makeRoot(); @@ -108,42 +98,28 @@ describe("migration snapshot sanitizer", () => { expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); }); - it("does not follow a required OpenClaw configuration symlink", () => { - const root = makeRoot(); - const targetPath = path.join(root, "target.json"); - const configPath = path.join(root, "openclaw.json"); - writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); - try { + it.runIf(process.platform !== "win32")( + "does not follow a required OpenClaw configuration symlink", + () => { + const root = makeRoot(); + const externalRoot = makeRoot(); + const targetPath = path.join(externalRoot, "target.json"); + const configPath = path.join(root, "openclaw.json"); + writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); symlinkSync(targetPath, configPath); - } catch (error) { - const code = error && typeof error === "object" ? (error as { code?: string }).code : ""; - if (code === "EPERM" || code === "EACCES") return; - throw error; - } - expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); - expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ - apiKey: "sk-secret-value", - }); - }); + expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); + expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ + apiKey: "sk-secret-value", + }); + }, + ); it("rejects a non-regular required OpenClaw configuration", () => { const root = makeRoot(); expect(sanitizeOpenClawConfigFile(root)).toBe(false); }); - it("fails closed when a required sanitized file cannot be written", () => { - const root = makeRoot(); - const configPath = path.join(root, "openclaw.json"); - writeFileSync(configPath, JSON.stringify({ apiKey: "sk-secret-value" })); - chmodSync(root, 0o500); - try { - expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); - } finally { - chmodSync(root, 0o700); - } - }); - it("preserves empty and comment-only YAML artifacts", () => { const root = makeRoot(); writeFileSync(path.join(root, "empty.yaml"), ""); diff --git a/nemoclaw/src/security/snapshot-sanitizer.ts b/nemoclaw/src/security/snapshot-sanitizer.ts index faf4d806ceb..9895dae7ea6 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.ts @@ -56,13 +56,11 @@ function writeSnapshotFile(filePath: string, contents: string): boolean { } function readRegularSnapshotFile(filePath: string): string | null { + const noFollowFlag = constants.O_NOFOLLOW; + if (typeof noFollowFlag !== "number") return null; + let fd: number; try { - if (typeof constants.O_NOFOLLOW !== "number") { - const stat = lstatSync(filePath); - if (!stat.isFile() || stat.isSymbolicLink()) return null; - } - const noFollowFlag = typeof constants.O_NOFOLLOW === "number" ? constants.O_NOFOLLOW : 0; fd = openSync(filePath, constants.O_RDONLY | noFollowFlag); } catch { return null; From fa903fd96a8d788aeff7416dd7647a06abdff697 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Thu, 30 Jul 2026 13:06:07 -0700 Subject: [PATCH 11/19] fix(security): fail closed without backup no-follow Signed-off-by: Apurv Kumaria --- .../credential-filter-failure.test.ts | 68 +++++++++++++++++++ src/lib/security/credential-filter.ts | 9 +-- 2 files changed, 71 insertions(+), 6 deletions(-) create mode 100644 src/lib/security/credential-filter-failure.test.ts diff --git a/src/lib/security/credential-filter-failure.test.ts b/src/lib/security/credential-filter-failure.test.ts new file mode 100644 index 00000000000..3849b138471 --- /dev/null +++ b/src/lib/security/credential-filter-failure.test.ts @@ -0,0 +1,68 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +const fsControl = vi.hoisted(() => ({ + noFollowUnavailable: false, +})); + +vi.mock("node:fs", async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + constants: { + ...original.constants, + get O_NOFOLLOW(): number | undefined { + return fsControl.noFollowUnavailable ? undefined : original.constants.O_NOFOLLOW; + }, + }, + }; +}); + +import { + sanitizeConfigFile, + sanitizeEnvFile, + sanitizeYamlConfigFile, +} from "./credential-filter.js"; + +const temporaryRoots: string[] = []; + +function makeRoot(): string { + const root = mkdtempSync(join(tmpdir(), "nemoclaw-credential-filter-failure-")); + temporaryRoots.push(root); + return root; +} + +afterEach(() => { + fsControl.noFollowUnavailable = false; + for (const root of temporaryRoots.splice(0)) { + rmSync(root, { recursive: true, force: true }); + } +}); + +describe("credential filter no-follow boundary", () => { + it("fails closed without atomic no-follow support", () => { + const root = makeRoot(); + const jsonPath = join(root, "openclaw.json"); + const yamlPath = join(root, "config.yaml"); + const envPath = join(root, ".env"); + const jsonSource = JSON.stringify({ apiKey: "sk-secret-value" }); + const yamlSource = "api_key: sk-secret-value\n"; + const envSource = "API_KEY=sk-secret-value\n"; + writeFileSync(jsonPath, jsonSource); + writeFileSync(yamlPath, yamlSource); + writeFileSync(envPath, envSource); + fsControl.noFollowUnavailable = true; + + expect(sanitizeConfigFile(jsonPath)).toBe(false); + expect(sanitizeYamlConfigFile(yamlPath)).toBe(false); + expect(sanitizeEnvFile(envPath)).toBe(false); + expect(readFileSync(jsonPath, "utf-8")).toBe(jsonSource); + expect(readFileSync(yamlPath, "utf-8")).toBe(yamlSource); + expect(readFileSync(envPath, "utf-8")).toBe(envSource); + }); +}); diff --git a/src/lib/security/credential-filter.ts b/src/lib/security/credential-filter.ts index aa1a0052971..cf6eea78179 100644 --- a/src/lib/security/credential-filter.ts +++ b/src/lib/security/credential-filter.ts @@ -15,7 +15,6 @@ import { closeSync, constants, fstatSync, - lstatSync, openSync, readFileSync, renameSync, @@ -32,13 +31,11 @@ function parseJson(text: string): T { } function readRegularFileNoFollow(filePath: string): string | null { + const noFollowFlag = constants.O_NOFOLLOW; + if (typeof noFollowFlag !== "number") return null; + let fd: number; try { - if (typeof constants.O_NOFOLLOW !== "number") { - const stat = lstatSync(filePath); - if (!stat.isFile() || stat.isSymbolicLink()) return null; - } - const noFollowFlag = typeof constants.O_NOFOLLOW === "number" ? constants.O_NOFOLLOW : 0; fd = openSync(filePath, constants.O_RDONLY | noFollowFlag); } catch { return null; From 21be52779c56391f6d3bf8dee742627461e94b2d Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Wed, 29 Jul 2026 15:56:10 -0700 Subject: [PATCH 12/19] fix(security): scrub common OAuth credential fields Signed-off-by: Apurv Kumaria --- .../src/security/credential-filter.test.ts | 22 ++++++++++++- nemoclaw/src/security/credential-filter.ts | 10 +++--- .../credential-filter-secret-patterns.test.ts | 31 +++++++++++++++++++ src/lib/security/credential-filter.ts | 10 +++--- test/credential-filter-parity.test.ts | 14 +++++++++ 5 files changed, 76 insertions(+), 11 deletions(-) diff --git a/nemoclaw/src/security/credential-filter.test.ts b/nemoclaw/src/security/credential-filter.test.ts index 20a140720ed..b3a27deab52 100644 --- a/nemoclaw/src/security/credential-filter.test.ts +++ b/nemoclaw/src/security/credential-filter.test.ts @@ -14,13 +14,23 @@ import { } from "./credential-filter.js"; describe("plugin credential-filter", () => { - it("treats Slack botToken, Authorization, and GITHUB_TOKEN as credential fields", () => { + it("treats channel, OAuth, header, and env credential names as sensitive", () => { expect(isCredentialField("botToken")).toBe(true); + expect(isCredentialField("bot_token")).toBe(true); expect(isCredentialField("appToken")).toBe(true); + expect(isCredentialField("access_token")).toBe(true); + expect(isCredentialField("personal_access_token")).toBe(true); + expect(isCredentialField("refresh-token")).toBe(true); + expect(isCredentialField("client_secret")).toBe(true); + expect(isCredentialField("auth_token")).toBe(true); + expect(isCredentialField("oauth_token")).toBe(true); + expect(isCredentialField("apikey")).toBe(true); + expect(isCredentialField("Token")).toBe(true); expect(isCredentialField("Authorization")).toBe(true); expect(isCredentialField("GITHUB_TOKEN")).toBe(true); expect(isCredentialField("DB_PASS")).toBe(true); expect(isCredentialField("publicKey")).toBe(false); + expect(isCredentialField("public.key")).toBe(false); expect(isCredentialField("NODE_ENV")).toBe(false); }); @@ -41,6 +51,11 @@ describe("plugin credential-filter", () => { env: { GITHUB_TOKEN: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", NODE_ENV: "test" }, args: ["--api-key", "opaque-secret-value", "--verbose"], }, + oauth: { + access_token: "opaque-access-value", + refresh_token: "opaque-refresh-value", + client_secret: "opaque-client-value", + }, customHeader: "Bearer opaque-migration-secret", model: "keep-me", publicKey: "verify-me", @@ -62,6 +77,11 @@ describe("plugin credential-filter", () => { expect(mcp.env.GITHUB_TOKEN).toBe(CREDENTIAL_PLACEHOLDER); expect(mcp.env.NODE_ENV).toBe("test"); expect(mcp.args).toEqual(["--api-key", CREDENTIAL_PLACEHOLDER, "--verbose"]); + expect(result.oauth).toEqual({ + access_token: CREDENTIAL_PLACEHOLDER, + refresh_token: CREDENTIAL_PLACEHOLDER, + client_secret: CREDENTIAL_PLACEHOLDER, + }); expect(result.customHeader).toBe(CREDENTIAL_PLACEHOLDER); expect(result.model).toBe("keep-me"); expect(result.publicKey).toBe("verify-me"); diff --git a/nemoclaw/src/security/credential-filter.ts b/nemoclaw/src/security/credential-filter.ts index bab6e15f5b8..99b5a4d5d44 100644 --- a/nemoclaw/src/security/credential-filter.ts +++ b/nemoclaw/src/security/credential-filter.ts @@ -19,18 +19,18 @@ export const CREDENTIAL_SENSITIVE_BASENAMES = new Set([ ]); const CREDENTIAL_FIELDS = new Set([ - "apiKey", + "apikey", "api_key", "token", "secret", "password", "pass", "passwd", - "resolvedKey", + "resolvedkey", ]); const CREDENTIAL_FIELD_PATTERN = - /(?:access|refresh|client|bearer|auth|api|private|public|signing|session|bot|app)(?:Token|Key|Secret|Password)$/; + /^(?:(?:personal[._-]?)?access|refresh|client|bearer|oauth|auth|api|private|public|signing|session|bot|app|resolved)[._-]?(?:tokens?|keys?|secrets?|passwords?|passphrases?|credentials?)$/i; const ENV_SECRET_FIELD_PATTERN = /^(?:[A-Z0-9]+_)*(?:TOKEN|KEY|SECRET|PASSWORD|PASSWD|PASS|PASSPHRASE|CREDENTIAL)S?$/; @@ -44,7 +44,7 @@ const CREDENTIAL_HEADER_NAMES: ReadonlySet = new Set([ const HEADER_CREDENTIAL_PATTERN = /-(?:key|token|secret|password|passphrase|credential|auth)s?$/i; -const PUBLIC_KEY_FIELD_PATTERN = /(?:^|[-_])public[-_]?keys?$/i; +const PUBLIC_KEY_FIELD_PATTERN = /(?:^|[._-])public[._-]?keys?$/i; const SAFE_CREDENTIAL_PLACEHOLDER_PATTERNS: readonly RegExp[] = [ /^openshell:resolve:env:[A-Za-z0-9_]+$/, @@ -105,7 +105,7 @@ function hasPassCredentialSegment(key: string): boolean { export function isCredentialField(key: string): boolean { if (PUBLIC_KEY_FIELD_PATTERN.test(key)) return false; return ( - CREDENTIAL_FIELDS.has(key) || + CREDENTIAL_FIELDS.has(key.toLowerCase()) || CREDENTIAL_FIELD_PATTERN.test(key) || hasPassCredentialSegment(key) || ENV_SECRET_FIELD_PATTERN.test(key) || diff --git a/src/lib/security/credential-filter-secret-patterns.test.ts b/src/lib/security/credential-filter-secret-patterns.test.ts index 8e22ffda161..0149b155b92 100644 --- a/src/lib/security/credential-filter-secret-patterns.test.ts +++ b/src/lib/security/credential-filter-secret-patterns.test.ts @@ -22,9 +22,17 @@ describe("isCredentialField", () => { it("matches pattern-based names", () => { expect(isCredentialField("accessToken")).toBe(true); + expect(isCredentialField("access_token")).toBe(true); + expect(isCredentialField("personal_access_token")).toBe(true); expect(isCredentialField("refreshToken")).toBe(true); + expect(isCredentialField("refresh-token")).toBe(true); expect(isCredentialField("clientSecret")).toBe(true); + expect(isCredentialField("client_secret")).toBe(true); expect(isCredentialField("bearerToken")).toBe(true); + expect(isCredentialField("auth_token")).toBe(true); + expect(isCredentialField("oauth_token")).toBe(true); + expect(isCredentialField("apikey")).toBe(true); + expect(isCredentialField("Token")).toBe(true); expect(isCredentialField("privateKey")).toBe(true); expect(isCredentialField("signingKey")).toBe(true); expect(isCredentialField("sessionToken")).toBe(true); @@ -32,7 +40,29 @@ describe("isCredentialField", () => { expect(isCredentialField("authKey")).toBe(true); // OpenClaw channel token fields (#5027). expect(isCredentialField("botToken")).toBe(true); + expect(isCredentialField("bot_token")).toBe(true); expect(isCredentialField("appToken")).toBe(true); + expect(isCredentialField("app_token")).toBe(true); + }); + + it("strips opaque values under common OAuth and channel field spellings", () => { + expect( + stripCredentials({ + access_token: "opaque-access-value", + refresh_token: "opaque-refresh-value", + client_secret: "opaque-client-value", + auth_token: "opaque-auth-value", + bot_token: "opaque-bot-value", + apikey: "opaque-api-value", + }), + ).toEqual({ + access_token: "[STRIPPED_BY_MIGRATION]", + refresh_token: "[STRIPPED_BY_MIGRATION]", + client_secret: "[STRIPPED_BY_MIGRATION]", + auth_token: "[STRIPPED_BY_MIGRATION]", + bot_token: "[STRIPPED_BY_MIGRATION]", + apikey: "[STRIPPED_BY_MIGRATION]", + }); }); it("matches terminal pass aliases without treating pass substrings as credentials", () => { @@ -117,6 +147,7 @@ describe("isCredentialField", () => { expect(isCredentialField("publicKey")).toBe(false); expect(isCredentialField("PUBLIC_KEY")).toBe(false); expect(isCredentialField("public-key")).toBe(false); + expect(isCredentialField("public.key")).toBe(false); expect(isCredentialField("X-Public-Key")).toBe(false); expect(isCredentialField("GITHUB_PUBLIC_KEY")).toBe(false); // But private keys and other secret fields still match. diff --git a/src/lib/security/credential-filter.ts b/src/lib/security/credential-filter.ts index cf6eea78179..ae27d4da48d 100644 --- a/src/lib/security/credential-filter.ts +++ b/src/lib/security/credential-filter.ts @@ -104,14 +104,14 @@ const SNAPSHOT_CREDENTIAL_SCAN_EXCLUDED_BASENAMES = new Set([ * Credential field names that MUST be stripped from config files. */ const CREDENTIAL_FIELDS = new Set([ - "apiKey", + "apikey", "api_key", "token", "secret", "password", "pass", "passwd", - "resolvedKey", + "resolvedkey", ]); /** @@ -121,7 +121,7 @@ const CREDENTIAL_FIELDS = new Set([ * (`botToken`, `appToken`) used by Slack/Telegram accounts. */ const CREDENTIAL_FIELD_PATTERN = - /(?:access|refresh|client|bearer|auth|api|private|public|signing|session|bot|app)(?:Token|Key|Secret|Password)$/; + /^(?:(?:personal[._-]?)?access|refresh|client|bearer|oauth|auth|api|private|public|signing|session|bot|app|resolved)[._-]?(?:tokens?|keys?|secrets?|passwords?|passphrases?|credentials?)$/i; /** * Environment-variable-style secret names (SCREAMING_SNAKE_CASE) such as an MCP @@ -161,7 +161,7 @@ const HEADER_CREDENTIAL_PATTERN = /-(?:key|token|secret|password|passphrase|cred * `PUBLIC_KEY`, `public-key`, and prefixed forms like `X-Public-Key` / * `GITHUB_PUBLIC_KEY`. Checked before the secret patterns below. */ -const PUBLIC_KEY_FIELD_PATTERN = /(?:^|[-_])public[-_]?keys?$/i; +const PUBLIC_KEY_FIELD_PATTERN = /(?:^|[._-])public[._-]?keys?$/i; /** * Check whether a field name should be treated as credential-bearing. @@ -169,7 +169,7 @@ const PUBLIC_KEY_FIELD_PATTERN = /(?:^|[-_])public[-_]?keys?$/i; export function isCredentialField(key: string): boolean { if (PUBLIC_KEY_FIELD_PATTERN.test(key)) return false; return ( - CREDENTIAL_FIELDS.has(key) || + CREDENTIAL_FIELDS.has(key.toLowerCase()) || CREDENTIAL_FIELD_PATTERN.test(key) || hasPassCredentialSegment(key) || ENV_SECRET_FIELD_PATTERN.test(key) || diff --git a/test/credential-filter-parity.test.ts b/test/credential-filter-parity.test.ts index fb2d92ebd60..3637cf9fa71 100644 --- a/test/credential-filter-parity.test.ts +++ b/test/credential-filter-parity.test.ts @@ -22,7 +22,16 @@ describe("credential filter parity", () => { it("keeps plugin and shared classification rules aligned", () => { const fields = [ "botToken", + "bot_token", "appToken", + "access_token", + "personal_access_token", + "refresh-token", + "client_secret", + "auth_token", + "oauth_token", + "apikey", + "Token", "GITHUB_TOKEN", "Authorization", "X-API-Key", @@ -65,6 +74,11 @@ describe("credential filter parity", () => { env: { GITHUB_TOKEN: "ghp_abcdefghijklmnopqrstuvwxyz0123456789", NODE_ENV: "test" }, args: ["--api-key", "opaque-secret-value", "--verbose"], }, + oauth: { + access_token: "opaque-access-value", + refresh_token: "opaque-refresh-value", + client_secret: "opaque-client-value", + }, model: "keep-me", }; const envFixture = [ From 16580e5f23bc693f843a57c3b1c69c200a8588d9 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 06:13:02 -0700 Subject: [PATCH 13/19] fix(security): pin migration sanitizer traversal Signed-off-by: Apurv Kumaria --- .../src/security/snapshot-sanitizer-python.ts | 439 ++++++++++++++++++ nemoclaw/src/security/snapshot-sanitizer.ts | 330 +++++++++---- 2 files changed, 682 insertions(+), 87 deletions(-) create mode 100644 nemoclaw/src/security/snapshot-sanitizer-python.ts diff --git a/nemoclaw/src/security/snapshot-sanitizer-python.ts b/nemoclaw/src/security/snapshot-sanitizer-python.ts new file mode 100644 index 00000000000..f390132b05a --- /dev/null +++ b/nemoclaw/src/security/snapshot-sanitizer-python.ts @@ -0,0 +1,439 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Descriptor-relative filesystem helper for migration snapshot sanitization. + * + * The plugin package publishes compiled JavaScript only, so the helper is + * passed as immutable source to an isolated Python interpreter. Every path + * component is opened relative to an already-pinned directory descriptor, + * and every mutation revalidates the exact inode version observed by the + * read pass before replacing or unlinking it. + */ +export const SNAPSHOT_SANITIZER_PYTHON = String.raw` +import base64 +import json +import os +import secrets +import stat +import sys + +MAX_FILE_BYTES = 16 * 1024 * 1024 +MAX_TOTAL_BYTES = 32 * 1024 * 1024 +MAX_ENTRIES = 100_000 +O_DIRECTORY = getattr(os, "O_DIRECTORY", 0) +O_NOFOLLOW = getattr(os, "O_NOFOLLOW", 0) +O_CLOEXEC = getattr(os, "O_CLOEXEC", 0) + + +def fail(message): + print(message, file=sys.stderr) + raise SystemExit(1) + + +def require_descriptor_support(): + if not O_DIRECTORY or not O_NOFOLLOW: + fail("descriptor-relative no-follow operations are unavailable") + supports = getattr(os, "supports_dir_fd", set()) + required = (os.open, os.stat, os.unlink, os.rename) + if any(operation not in supports for operation in required): + fail("descriptor-relative filesystem operations are unavailable") + + +def dir_flags(): + return os.O_RDONLY | O_DIRECTORY | O_NOFOLLOW | O_CLOEXEC + + +def file_flags(): + return os.O_RDONLY | O_NOFOLLOW | O_CLOEXEC + + +def metadata(value): + return { + "dev": str(value.st_dev), + "ino": str(value.st_ino), + "mode": str(value.st_mode), + "nlink": str(value.st_nlink), + "size": str(value.st_size), + "mtimeNs": str(value.st_mtime_ns), + "ctimeNs": str(value.st_ctime_ns), + } + + +def same_version(expected, actual): + return expected == metadata(actual) + + +def same_identity(expected, actual): + return expected.get("dev") == str(actual.st_dev) and expected.get("ino") == str(actual.st_ino) + + +def validate_name(name): + if ( + not isinstance(name, str) + or not name + or name in (".", "..") + or os.sep in name + or (os.altsep is not None and os.altsep in name) + ): + fail("snapshot entry name is unsafe") + return name + + +def validate_relative_path(value): + if not isinstance(value, str) or not value or os.path.isabs(value) or "\\" in value: + fail("snapshot relative path is unsafe") + parts = value.split("/") + if any(part in ("", ".", "..") for part in parts): + fail("snapshot relative path is unsafe") + for part in parts: + validate_name(part) + return parts + + +def open_absolute_dir_no_follow(value): + if not isinstance(value, str) or not os.path.isabs(value): + fail("snapshot root must be absolute") + normalized = os.path.normpath(value) + parts = [part for part in normalized.split(os.sep) if part] + fd = os.open(os.sep, dir_flags()) + try: + for part in parts: + next_fd = os.open(validate_name(part), dir_flags(), dir_fd=fd) + os.close(fd) + fd = next_fd + return fd + except Exception: + os.close(fd) + raise + + +def verify_opened_at(parent_fd, name, opened_fd, expected=None): + opened = os.fstat(opened_fd) + current = os.stat(name, dir_fd=parent_fd, follow_symlinks=False) + if (opened.st_dev, opened.st_ino) != (current.st_dev, current.st_ino): + fail("snapshot entry changed while it was opened") + if expected is not None and not same_identity(expected, opened): + fail("snapshot entry changed before sanitization") + return opened + + +def read_regular_file_at(parent_fd, name, observed): + fd = os.open(name, file_flags(), dir_fd=parent_fd) + try: + expected_observed = metadata(observed) + opened = verify_opened_at(parent_fd, name, fd, expected_observed) + if not same_version(expected_observed, opened): + fail("snapshot artifact changed before it was read") + if not stat.S_ISREG(opened.st_mode) or opened.st_nlink != 1: + fail("snapshot artifact is not a single regular file") + if opened.st_size > MAX_FILE_BYTES: + fail("snapshot artifact exceeds the sanitization size limit") + chunks = [] + total = 0 + while True: + chunk = os.read(fd, 65536) + if not chunk: + break + total += len(chunk) + if total > MAX_FILE_BYTES: + fail("snapshot artifact exceeds the sanitization size limit") + chunks.append(chunk) + final = os.fstat(fd) + current = os.stat(name, dir_fd=parent_fd, follow_symlinks=False) + expected = metadata(opened) + if not same_version(expected, final) or not same_version(expected, current): + fail("snapshot artifact changed while it was read") + return b"".join(chunks), expected + finally: + os.close(fd) + + +def should_read(name, sensitive_names): + lower = name.lower() + return ( + lower in sensitive_names + or lower.endswith(".json") + or lower.endswith(".yaml") + or lower.endswith(".yml") + or lower == ".env" + or lower.endswith(".env") + ) + + +def scan_directory(dir_fd, relative_dir, directories, files, state, sensitive_names): + with os.scandir(dir_fd) as entries: + for entry in entries: + name = validate_name(entry.name) + state["entries"] += 1 + if state["entries"] > MAX_ENTRIES: + fail("snapshot tree exceeds the sanitization entry limit") + try: + observed = os.stat(name, dir_fd=dir_fd, follow_symlinks=False) + except FileNotFoundError: + fail("snapshot entry changed during sanitization") + relative_path = name if not relative_dir else relative_dir + "/" + name + if stat.S_ISLNK(observed.st_mode): + continue + if stat.S_ISDIR(observed.st_mode): + child_fd = os.open(name, dir_flags(), dir_fd=dir_fd) + try: + opened = verify_opened_at(dir_fd, name, child_fd, metadata(observed)) + opened_metadata = metadata(opened) + directories[relative_path] = opened_metadata + scan_directory( + child_fd, + relative_path, + directories, + files, + state, + sensitive_names, + ) + final = os.fstat(child_fd) + current = os.stat(name, dir_fd=dir_fd, follow_symlinks=False) + if not same_version(opened_metadata, final) or not same_version( + opened_metadata, current + ): + fail("snapshot directory changed while it was scanned") + finally: + os.close(child_fd) + continue + if not stat.S_ISREG(observed.st_mode) or not should_read(name, sensitive_names): + continue + lower = name.lower() + if lower in sensitive_names: + files.append({"path": relative_path, "metadata": metadata(observed)}) + continue + payload, file_metadata = read_regular_file_at(dir_fd, name, observed) + state["bytes"] += len(payload) + if state["bytes"] > MAX_TOTAL_BYTES: + fail("snapshot artifacts exceed the sanitization size limit") + files.append( + { + "path": relative_path, + "metadata": file_metadata, + "content": base64.b64encode(payload).decode("ascii"), + } + ) + + +def scan(root_path, expected_root, target_name, sensitive_names): + root_fd = open_absolute_dir_no_follow(root_path) + try: + root_metadata = metadata(os.fstat(root_fd)) + if root_metadata != expected_root: + fail("snapshot root changed before sanitization") + directories = {} + files = [] + state = {"entries": 0, "bytes": 0} + if target_name is None: + scan_directory(root_fd, "", directories, files, state, sensitive_names) + else: + name = validate_name(target_name) + observed = os.stat(name, dir_fd=root_fd, follow_symlinks=False) + if stat.S_ISLNK(observed.st_mode) or not stat.S_ISREG(observed.st_mode): + fail("required snapshot artifact is not a regular file") + payload, file_metadata = read_regular_file_at(root_fd, name, observed) + files.append( + { + "path": name, + "metadata": file_metadata, + "content": base64.b64encode(payload).decode("ascii"), + } + ) + if not same_version(root_metadata, os.fstat(root_fd)): + fail("snapshot root changed while it was scanned") + print( + json.dumps( + { + "root": root_metadata, + "directories": directories, + "files": files, + }, + separators=(",", ":"), + ) + ) + finally: + os.close(root_fd) + + +def read_plan(): + payload = sys.stdin.buffer.read(MAX_TOTAL_BYTES * 2 + 1) + if len(payload) > MAX_TOTAL_BYTES * 2: + fail("snapshot sanitization plan exceeds the size limit") + try: + parsed = json.loads(payload) + except (TypeError, ValueError, UnicodeDecodeError): + fail("snapshot sanitization plan is invalid") + if not isinstance(parsed, dict): + fail("snapshot sanitization plan is invalid") + return parsed + + +def open_parent(root_fd, relative_path, directories): + parts = validate_relative_path(relative_path) + fd = os.dup(root_fd) + try: + traversed = [] + for part in parts[:-1]: + traversed.append(part) + key = "/".join(traversed) + expected = directories.get(key) + if not isinstance(expected, dict): + fail("snapshot sanitization plan omits a parent identity") + next_fd = os.open(part, dir_flags(), dir_fd=fd) + verify_opened_at(fd, part, next_fd, expected) + os.close(fd) + fd = next_fd + return fd, parts[-1] + except Exception: + os.close(fd) + raise + + +def create_staged_file(parent_fd): + flags = os.O_WRONLY | os.O_CREAT | os.O_EXCL | O_NOFOLLOW | O_CLOEXEC + for _attempt in range(100): + name = ".nemoclaw-sanitize." + secrets.token_hex(16) + try: + fd = os.open(name, flags, 0o600, dir_fd=parent_fd) + except FileExistsError: + continue + opened = os.fstat(fd) + if not stat.S_ISREG(opened.st_mode) or opened.st_nlink != 1: + os.close(fd) + fail("snapshot staging file is unsafe") + return name, fd, metadata(opened) + fail("snapshot staging file could not be created") + + +def unlink_staged_if_owned(parent_fd, name, expected): + if not name or expected is None: + return + try: + current = os.stat(name, dir_fd=parent_fd, follow_symlinks=False) + if same_version(expected, current): + os.unlink(name, dir_fd=parent_fd) + except OSError: + pass + + +def verify_current_file(parent_fd, name, expected): + fd = os.open(name, file_flags(), dir_fd=parent_fd) + try: + opened = verify_opened_at(parent_fd, name, fd, expected) + if not stat.S_ISREG(opened.st_mode) or opened.st_nlink != 1: + fail("snapshot artifact is no longer a single regular file") + if not same_version(expected, opened): + fail("snapshot artifact changed before sanitization") + return metadata(opened) + finally: + os.close(fd) + + +def replace_file(parent_fd, name, expected, payload): + verify_current_file(parent_fd, name, expected) + staged_name = "" + staged_fd = -1 + staged_metadata = None + installed = False + try: + staged_name, staged_fd, staged_metadata = create_staged_file(parent_fd) + written = 0 + while written < len(payload): + written += os.write(staged_fd, payload[written:]) + os.fchmod(staged_fd, 0o600) + os.fsync(staged_fd) + staged_metadata = metadata(os.fstat(staged_fd)) + verify_current_file(parent_fd, name, expected) + staged_current = os.stat(staged_name, dir_fd=parent_fd, follow_symlinks=False) + if not same_version(staged_metadata, staged_current): + fail("snapshot staging file changed before replacement") + os.rename(staged_name, name, src_dir_fd=parent_fd, dst_dir_fd=parent_fd) + installed = True + os.fsync(parent_fd) + finally: + if staged_fd >= 0: + os.close(staged_fd) + if not installed: + unlink_staged_if_owned(parent_fd, staged_name, staged_metadata) + + +def remove_file(parent_fd, name, expected): + verify_current_file(parent_fd, name, expected) + current = os.stat(name, dir_fd=parent_fd, follow_symlinks=False) + if not same_version(expected, current): + fail("snapshot artifact changed before removal") + os.unlink(name, dir_fd=parent_fd) + os.fsync(parent_fd) + + +def apply(root_path, plan): + root = plan.get("root") + directories = plan.get("directories") + actions = plan.get("actions") + if not isinstance(root, dict) or not isinstance(directories, dict) or not isinstance(actions, list): + fail("snapshot sanitization plan is invalid") + root_fd = open_absolute_dir_no_follow(root_path) + try: + if not same_version(root, os.fstat(root_fd)): + fail("snapshot root changed before sanitized output was installed") + for action in actions: + if not isinstance(action, dict): + fail("snapshot sanitization action is invalid") + expected = action.get("metadata") + if not isinstance(expected, dict): + fail("snapshot sanitization action omits a file identity") + parent_fd, name = open_parent(root_fd, action.get("path"), directories) + try: + kind = action.get("kind") + if kind == "remove": + remove_file(parent_fd, name, expected) + elif kind == "replace": + raw = action.get("content") + if not isinstance(raw, str): + fail("snapshot replacement content is invalid") + try: + payload = base64.b64decode(raw, validate=True) + except ValueError: + fail("snapshot replacement content is invalid") + if len(payload) > MAX_FILE_BYTES: + fail("snapshot replacement content exceeds the size limit") + replace_file(parent_fd, name, expected, payload) + else: + fail("snapshot sanitization action is invalid") + finally: + os.close(parent_fd) + finally: + os.close(root_fd) + + +def main(): + require_descriptor_support() + if len(sys.argv) < 3: + fail("snapshot sanitizer arguments are invalid") + mode = sys.argv[1] + root_path = sys.argv[2] + if mode in ("scan-tree", "scan-file"): + if len(sys.argv) != 6: + fail("snapshot sanitizer scan arguments are invalid") + try: + expected_root = json.loads(sys.argv[3]) + sensitive_names = set(json.loads(sys.argv[5])) + except (TypeError, ValueError): + fail("snapshot sanitizer scan arguments are invalid") + target_name = None if mode == "scan-tree" else sys.argv[4] + scan(root_path, expected_root, target_name, sensitive_names) + return + if mode == "apply" and len(sys.argv) == 3: + apply(root_path, read_plan()) + return + fail("snapshot sanitizer mode is invalid") + + +if __name__ == "__main__": + try: + main() + except Exception as error: + fail(str(error)) +`.trim(); diff --git a/nemoclaw/src/security/snapshot-sanitizer.ts b/nemoclaw/src/security/snapshot-sanitizer.ts index 9895dae7ea6..288e68a41a1 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.ts @@ -1,33 +1,61 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { randomUUID } from "node:crypto"; -import { - chmodSync, - closeSync, - constants, - existsSync, - fstatSync, - lstatSync, - openSync, - readdirSync, - readFileSync, - renameSync, - rmSync, - writeFileSync, -} from "node:fs"; +import { spawnSync } from "node:child_process"; +import { lstatSync, realpathSync } from "node:fs"; import path from "node:path"; import JSON5 from "json5"; import { parse as parseYaml, stringify as stringifyYaml } from "yaml"; +import { buildSubprocessEnv } from "../lib/subprocess-env.js"; import { isObjectRecord } from "../shared/object-record.js"; import { CREDENTIAL_PLACEHOLDER, + CREDENTIAL_SENSITIVE_BASENAMES, isSafeCredentialPlaceholder, isSensitiveFile, sanitizeEnvFileContent, stripCredentials, valueLooksLikeSecret, } from "./credential-filter.js"; +import { SNAPSHOT_SANITIZER_PYTHON } from "./snapshot-sanitizer-python.js"; + +const HELPER_TIMEOUT_MS = 60_000; +const HELPER_MAX_BUFFER_BYTES = 48 * 1024 * 1024; +const MAX_SANITIZATION_PASSES = 3; + +interface FileIdentity { + readonly dev: string; + readonly ino: string; + readonly mode: string; + readonly nlink: string; + readonly size: string; + readonly mtimeNs: string; + readonly ctimeNs: string; +} + +interface ScannedFile { + readonly path: string; + readonly metadata: FileIdentity; + readonly content?: string; +} + +interface ScanResult { + readonly root: FileIdentity; + readonly directories: Readonly>; + readonly files: readonly ScannedFile[]; +} + +interface SanitizationAction { + readonly kind: "remove" | "replace"; + readonly path: string; + readonly metadata: FileIdentity; + readonly content?: string; +} + +interface SnapshotRoot { + readonly canonicalPath: string; + readonly identity: FileIdentity; +} function sanitizeTopLevelValue(value: unknown): unknown { if (typeof value !== "string" || isSafeCredentialPlaceholder(value)) { @@ -42,91 +70,178 @@ function withoutGateway(value: unknown): unknown { return config; } -function writeSnapshotFile(filePath: string, contents: string): boolean { - const temporaryPath = `${filePath}.${String(process.pid)}.${randomUUID()}.tmp`; +function isFileIdentity(value: unknown): value is FileIdentity { + if (!isObjectRecord(value)) return false; + return ["dev", "ino", "mode", "nlink", "size", "mtimeNs", "ctimeNs"].every( + (key) => typeof value[key] === "string", + ); +} + +function parseScanResult(stdout: string): ScanResult | null { try { - writeFileSync(temporaryPath, contents, { mode: 0o600 }); - renameSync(temporaryPath, filePath); - chmodSync(filePath, 0o600); - return true; + const parsed: unknown = JSON.parse(stdout); + if (!isObjectRecord(parsed) || !isFileIdentity(parsed.root)) return null; + if (!isObjectRecord(parsed.directories) || !Array.isArray(parsed.files)) return null; + + const directories: Record = {}; + for (const [relativePath, identity] of Object.entries(parsed.directories)) { + if (!isSafeRelativePath(relativePath) || !isFileIdentity(identity)) return null; + directories[relativePath] = identity; + } + + const files: ScannedFile[] = []; + for (const value of parsed.files) { + if (!isObjectRecord(value) || !isSafeRelativePath(value.path)) return null; + if (!isFileIdentity(value.metadata)) return null; + if (value.content !== undefined && typeof value.content !== "string") return null; + files.push({ + path: value.path, + metadata: value.metadata, + ...(typeof value.content === "string" ? { content: value.content } : {}), + }); + } + return { root: parsed.root, directories, files }; } catch { - rmSync(temporaryPath, { force: true }); - return false; + return null; } } -function readRegularSnapshotFile(filePath: string): string | null { - const noFollowFlag = constants.O_NOFOLLOW; - if (typeof noFollowFlag !== "number") return null; +function isSafeRelativePath(value: unknown): value is string { + if (typeof value !== "string" || value === "" || path.isAbsolute(value)) return false; + if (value.includes("\\")) return false; + return value.split("/").every((part) => part !== "" && part !== "." && part !== ".."); +} - let fd: number; +function rootIdentity(rootPath: string): SnapshotRoot | null { + let observed: ReturnType; try { - fd = openSync(filePath, constants.O_RDONLY | noFollowFlag); - } catch { - return null; + observed = lstatSync(rootPath, { bigint: true }); + } catch (error: unknown) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return null; + throw error; } - try { - if (!fstatSync(fd).isFile()) return null; - return String(readFileSync(fd, "utf-8")); - } finally { - closeSync(fd); + if (!observed.isDirectory() || observed.isSymbolicLink()) { + throw new Error(`Migration snapshot root is not a safe directory: ${rootPath}`); } + const canonicalPath = realpathSync(rootPath); + const canonical = lstatSync(canonicalPath, { bigint: true }); + if (canonical.dev !== observed.dev || canonical.ino !== observed.ino) { + throw new Error(`Migration snapshot root changed while it was resolved: ${rootPath}`); + } + return { + canonicalPath, + identity: { + dev: String(observed.dev), + ino: String(observed.ino), + mode: String(observed.mode), + nlink: String(observed.nlink), + size: String(observed.size), + mtimeNs: String(observed.mtimeNs), + ctimeNs: String(observed.ctimeNs), + }, + }; } -function sanitizeJsonFile(filePath: string): boolean { - const raw = readRegularSnapshotFile(filePath); - if (raw === null) return false; - try { - const parsed: unknown = JSON5.parse(raw); - const sanitized = sanitizeTopLevelValue(withoutGateway(parsed)); - return writeSnapshotFile(filePath, JSON.stringify(sanitized, null, 2)); - } catch { - return false; - } +function scanSnapshot(root: SnapshotRoot, targetName?: string): ScanResult | null { + const mode = targetName === undefined ? "scan-tree" : "scan-file"; + const result = spawnSync( + "python3", + [ + "-I", + "-c", + SNAPSHOT_SANITIZER_PYTHON, + mode, + root.canonicalPath, + JSON.stringify(root.identity), + targetName ?? "", + JSON.stringify([...CREDENTIAL_SENSITIVE_BASENAMES]), + ], + { + encoding: "utf-8", + env: buildSubprocessEnv(), + maxBuffer: HELPER_MAX_BUFFER_BYTES, + timeout: HELPER_TIMEOUT_MS, + }, + ); + if (result.status !== 0 || result.error) return null; + return parseScanResult(result.stdout); } -function sanitizeYamlFile(filePath: string): boolean { - const raw = readRegularSnapshotFile(filePath); - if (raw === null) return false; - try { - const parsed: unknown = parseYaml(raw); - if (parsed === null || parsed === undefined) return true; - const sanitized = sanitizeTopLevelValue(withoutGateway(parsed)); - return writeSnapshotFile(filePath, stringifyYaml(sanitized)); - } catch { - return false; - } +function applyActions( + canonicalPath: string, + scan: ScanResult, + actions: readonly SanitizationAction[], +): boolean { + if (actions.length === 0) return true; + const result = spawnSync( + "python3", + ["-I", "-c", SNAPSHOT_SANITIZER_PYTHON, "apply", canonicalPath], + { + encoding: "utf-8", + env: buildSubprocessEnv(), + input: JSON.stringify({ root: scan.root, directories: scan.directories, actions }), + maxBuffer: HELPER_MAX_BUFFER_BYTES, + timeout: HELPER_TIMEOUT_MS, + }, + ); + return result.status === 0 && !result.error; } -function sanitizeEnvFile(filePath: string): boolean { - const raw = readRegularSnapshotFile(filePath); - if (raw === null) return false; - return writeSnapshotFile(filePath, sanitizeEnvFileContent(raw)); +function decodeScannedContent(content: string | undefined): string | null { + if ( + content === undefined || + !/^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/u.test(content) + ) { + return null; + } + const decoded = Buffer.from(content, "base64"); + if (decoded.toString("base64") !== content) return null; + const utf8 = decoded.toString("utf-8"); + if (!Buffer.from(utf8, "utf-8").equals(decoded)) return null; + return utf8; } -function removeUnsafeArtifact(filePath: string): void { - rmSync(filePath, { force: true }); - if (existsSync(filePath)) { - throw new Error(`Unable to remove unsanitizable migration artifact: ${filePath}`); +function sanitizedContents(name: string, raw: string): string | null | undefined { + try { + if (name.endsWith(".json")) { + const parsed: unknown = JSON5.parse(raw); + return JSON.stringify(sanitizeTopLevelValue(withoutGateway(parsed)), null, 2); + } + if (name.endsWith(".yaml") || name.endsWith(".yml")) { + const parsed: unknown = parseYaml(raw); + if (parsed === null || parsed === undefined) return undefined; + return stringifyYaml(sanitizeTopLevelValue(withoutGateway(parsed))); + } + if (name === ".env" || name.endsWith(".env")) { + return sanitizeEnvFileContent(raw); + } + } catch { + return null; } + return undefined; } -function sanitizeFile(filePath: string): void { - const name = path.basename(filePath).toLowerCase(); +function actionForScannedFile(file: ScannedFile): SanitizationAction | null { + const name = path.posix.basename(file.path).toLowerCase(); if (isSensitiveFile(name)) { - removeUnsafeArtifact(filePath); - return; + return { kind: "remove", path: file.path, metadata: file.metadata }; } - - let sanitized = true; - if (name.endsWith(".json")) { - sanitized = sanitizeJsonFile(filePath); - } else if (name.endsWith(".yaml") || name.endsWith(".yml")) { - sanitized = sanitizeYamlFile(filePath); - } else if (name === ".env" || name.endsWith(".env")) { - sanitized = sanitizeEnvFile(filePath); + const raw = decodeScannedContent(file.content); + if (raw === null) { + return { kind: "remove", path: file.path, metadata: file.metadata }; + } + const sanitized = sanitizedContents(name, raw); + if (sanitized === undefined) return null; + if (sanitized === null) { + return { kind: "remove", path: file.path, metadata: file.metadata }; } - if (!sanitized) removeUnsafeArtifact(filePath); + if (sanitized === raw) return null; + return { + kind: "replace", + path: file.path, + metadata: file.metadata, + content: Buffer.from(sanitized, "utf-8").toString("base64"), + }; } /** @@ -135,22 +250,63 @@ function sanitizeFile(filePath: string): void { * existing manifest audit and are never followed by this walk. */ export function sanitizeMigrationDirectory(rootPath: string): void { - if (!existsSync(rootPath)) return; - for (const entry of readdirSync(rootPath)) { - const fullPath = path.join(rootPath, entry); - const stat = lstatSync(fullPath); - if (stat.isSymbolicLink()) continue; - if (stat.isDirectory()) { - sanitizeMigrationDirectory(fullPath); - } else if (stat.isFile()) { - sanitizeFile(fullPath); + for (let pass = 0; pass < MAX_SANITIZATION_PASSES; pass += 1) { + const root = rootIdentity(rootPath); + if (root === null) { + if (pass === 0) return; + throw new Error(`Failed to inspect migration artifacts safely: ${rootPath}`); + } + const scan = scanSnapshot(root); + if (scan === null) { + throw new Error(`Failed to inspect migration artifacts safely: ${rootPath}`); + } + const actions = scan.files + .map((file) => actionForScannedFile(file)) + .filter((action): action is SanitizationAction => action !== null); + if (actions.length === 0) return; + if (!applyActions(root.canonicalPath, scan, actions)) { + throw new Error(`Failed to sanitize migration artifacts safely: ${rootPath}`); } } + throw new Error(`Migration artifacts did not reach a stable sanitized state: ${rootPath}`); } /** * Sanitize a required OpenClaw configuration copy. */ export function sanitizeOpenClawConfigFile(configPath: string): boolean { - return sanitizeJsonFile(configPath); + const parentPath = path.dirname(configPath); + const targetName = path.basename(configPath); + if (targetName === "" || targetName === "." || targetName === "..") return false; + for (let pass = 0; pass < MAX_SANITIZATION_PASSES; pass += 1) { + let root: SnapshotRoot | null; + try { + root = rootIdentity(parentPath); + } catch { + return false; + } + if (root === null) return false; + const scan = scanSnapshot(root, targetName); + if (scan === null || scan.files.length !== 1) return false; + const file = scan.files[0]; + if (!file || file.path !== targetName) return false; + const raw = decodeScannedContent(file.content); + if (raw === null) return false; + const sanitized = sanitizedContents(targetName.toLowerCase(), raw); + if (typeof sanitized !== "string") return false; + if (sanitized === raw) return true; + if ( + !applyActions(root.canonicalPath, scan, [ + { + kind: "replace", + path: file.path, + metadata: file.metadata, + content: Buffer.from(sanitized, "utf-8").toString("base64"), + }, + ]) + ) { + return false; + } + } + return false; } From 015fee1b55d8c6126be2a75809582d81f06448dc Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 06:13:32 -0700 Subject: [PATCH 14/19] test(security): cover sanitizer path races Signed-off-by: Apurv Kumaria --- ci/test-file-size-budget.json | 2 +- .../migration-state-sanitizer-test-fixture.ts | 44 +++++ nemoclaw/src/commands/migration-state.test.ts | 16 +- .../snapshot-sanitizer-failure.test.ts | 160 ++++++++---------- .../src/security/snapshot-sanitizer.test.ts | 69 +++++++- 5 files changed, 193 insertions(+), 98 deletions(-) create mode 100644 nemoclaw/src/commands/migration-state-sanitizer-test-fixture.ts diff --git a/ci/test-file-size-budget.json b/ci/test-file-size-budget.json index 1b0ad7132ca..5452bd9c9d8 100644 --- a/ci/test-file-size-budget.json +++ b/ci/test-file-size-budget.json @@ -2,7 +2,7 @@ "$comment": "SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.\nSPDX-License-Identifier: Apache-2.0", "defaultMaxLines": 1500, "legacyMaxLines": { - "nemoclaw/src/commands/migration-state.test.ts": 1564, + "nemoclaw/src/commands/migration-state.test.ts": 1562, "src/lib/inference/nim.test.ts": 2068, "src/lib/onboard/preflight.test.ts": 1904, "test/generate-openclaw-config.test.ts": 1941, diff --git a/nemoclaw/src/commands/migration-state-sanitizer-test-fixture.ts b/nemoclaw/src/commands/migration-state-sanitizer-test-fixture.ts new file mode 100644 index 00000000000..5cb0c6232fa --- /dev/null +++ b/nemoclaw/src/commands/migration-state-sanitizer-test-fixture.ts @@ -0,0 +1,44 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { isSensitiveFile, stripCredentials } from "../security/credential-filter.js"; + +interface TestFsEntry { + type: "file" | "dir" | "symlink"; + content?: string; +} + +/** Model the sanitizer's public data contract for migration-state's in-memory filesystem tests. */ +export function buildMigrationStateSanitizerMock(store: Map) { + function sanitizeJsonAt(filePath: string): boolean { + const entry = store.get(filePath); + if (entry?.type !== "file") return false; + try { + const parsed: unknown = JSON.parse(entry.content ?? ""); + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { + delete (parsed as Record)["gateway"]; + } + store.set(filePath, { + type: "file", + content: JSON.stringify(stripCredentials(parsed), null, 2), + }); + return true; + } catch { + return false; + } + } + + return { + sanitizeMigrationDirectory: (rootPath: string) => { + for (const [filePath, entry] of [...store.entries()]) { + if (entry.type !== "file" || !filePath.startsWith(`${rootPath}/`)) continue; + if (isSensitiveFile(filePath.split("/").at(-1) ?? "")) { + store.delete(filePath); + } else if (filePath.toLowerCase().endsWith(".json") && !sanitizeJsonAt(filePath)) { + store.delete(filePath); + } + } + }, + sanitizeOpenClawConfigFile: sanitizeJsonAt, + }; +} diff --git a/nemoclaw/src/commands/migration-state.test.ts b/nemoclaw/src/commands/migration-state.test.ts index 59c772a5dd5..268f43b01b1 100644 --- a/nemoclaw/src/commands/migration-state.test.ts +++ b/nemoclaw/src/commands/migration-state.test.ts @@ -7,7 +7,7 @@ import { setConfigValue } from "./migration-state.js"; // fs mock — thin in-memory store keyed by absolute path type FsEntry = { type: "file" | "dir" | "symlink"; content?: string }; -const store = new Map(); +const { store } = vi.hoisted(() => ({ store: new Map() })); const descriptors = new Map(); const addDir = (p: string): void => void store.set(p, { type: "dir" }); const addFile = (p: string, content: string): void => void store.set(p, { type: "file", content }); @@ -93,6 +93,12 @@ vi.mock("node:fs", async (importOriginal) => { }; }); +vi.mock("../security/snapshot-sanitizer.js", async () => + (await import("./migration-state-sanitizer-test-fixture.js")).buildMigrationStateSanitizerMock( + store, + ), +); + // Mock tar to avoid real archive creation vi.mock("tar", () => ({ create: vi.fn(async () => {}), @@ -125,9 +131,7 @@ describe("commands/migration-state", () => { vi.clearAllMocks(); }); - // ------------------------------------------------------------------------- // detectHostOpenClaw - // ------------------------------------------------------------------------- describe("detectHostOpenClaw", () => { it("returns exists=false when no state dir or config", () => { @@ -445,9 +449,7 @@ describe("commands/migration-state", () => { }); }); - // ------------------------------------------------------------------------- // createSnapshotBundle - // ------------------------------------------------------------------------- describe("createSnapshotBundle", () => { it("returns null when stateDir is missing", () => { @@ -863,9 +865,7 @@ describe("commands/migration-state", () => { }); }); - // ------------------------------------------------------------------------- // cleanupSnapshotBundle - // ------------------------------------------------------------------------- describe("cleanupSnapshotBundle", () => { it("removes temporary snapshot directory", async () => { @@ -897,9 +897,7 @@ describe("commands/migration-state", () => { }); }); - // ------------------------------------------------------------------------- // createArchiveFromDirectory - // ------------------------------------------------------------------------- describe("createArchiveFromDirectory", () => { it("calls tar.create with correct options", async () => { diff --git a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts index fe9cb29686b..611b1b441a3 100644 --- a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts @@ -1,57 +1,11 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { spawnSync } from "node:child_process"; +import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; - -const fsControl = vi.hoisted(() => ({ - failOpenNames: [] as string[], - failWriteNames: [] as string[], - noFollowUnavailable: false, - preventRemoval: false, -})); - -vi.mock("node:fs", async (importOriginal) => { - const original = await importOriginal(); - return { - ...original, - constants: { - ...original.constants, - get O_NOFOLLOW(): number | undefined { - return fsControl.noFollowUnavailable ? undefined : original.constants.O_NOFOLLOW; - }, - }, - openSync: (target: Parameters[0], flags: number) => { - switch (fsControl.failOpenNames.some((name) => String(target).endsWith(name))) { - case true: - throw new Error(`simulated open failure: ${String(target)}`); - default: - return original.openSync(target, flags); - } - }, - rmSync: (...args: Parameters) => { - const [target] = args; - switch (fsControl.preventRemoval && String(target).endsWith("auth.json")) { - case true: - return; - default: - return original.rmSync(...args); - } - }, - writeFileSync: (...args: Parameters) => { - const [target] = args; - switch (fsControl.failWriteNames.some((name) => String(target).includes(name))) { - case true: - throw new Error(`simulated write failure: ${String(target)}`); - default: - return original.writeFileSync(...args); - } - }, - }; -}); - import { sanitizeMigrationDirectory, sanitizeOpenClawConfigFile } from "./snapshot-sanitizer.js"; const roots: string[] = []; @@ -62,72 +16,106 @@ function makeRoot(): string { return root; } +function shellQuote(value: string): string { + return `'${value.replaceAll("'", `'"'"'`)}'`; +} + +function writePythonWrapper(lines: readonly string[]): string { + const wrapperRoot = makeRoot(); + const wrapper = path.join(wrapperRoot, "python3"); + writeFileSync(wrapper, ["#!/bin/sh", ...lines].join("\n")); + chmodSync(wrapper, 0o755); + vi.stubEnv("PATH", `${wrapperRoot}:${process.env.PATH ?? ""}`); + return wrapper; +} + afterEach(() => { - fsControl.failOpenNames = []; - fsControl.failWriteNames = []; - fsControl.noFollowUnavailable = false; - fsControl.preventRemoval = false; + vi.unstubAllEnvs(); for (const root of roots.splice(0)) rmSync(root, { force: true, recursive: true }); }); describe("migration snapshot sanitizer fallbacks", () => { - it("fails closed for a regular file when O_NOFOLLOW is unavailable", () => { + it("fails closed when the descriptor helper is unavailable", () => { const configPath = path.join(makeRoot(), "openclaw.json"); - writeFileSync(configPath, JSON.stringify({ apiKey: "sk-secret-value" })); - fsControl.noFollowUnavailable = true; + const original = JSON.stringify({ apiKey: "sk-secret-value" }); + writeFileSync(configPath, original); + vi.stubEnv("PATH", makeRoot()); expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); - expect(JSON.parse(readFileSync(configPath, "utf-8"))).toEqual({ - apiKey: "sk-secret-value", - }); + expect(readFileSync(configPath, "utf-8")).toBe(original); }); - it.runIf(process.platform !== "win32")("rejects a symlink when O_NOFOLLOW is unavailable", () => { - const root = makeRoot(); - const targetPath = path.join(root, "target.json"); - const configPath = path.join(root, "openclaw.json"); - writeFileSync(targetPath, JSON.stringify({ apiKey: "sk-secret-value" })); - symlinkSync(targetPath, configPath); - fsControl.noFollowUnavailable = true; + it("rejects invalid output from the descriptor helper", () => { + const configPath = path.join(makeRoot(), "openclaw.json"); + const original = JSON.stringify({ apiKey: "sk-secret-value" }); + writeFileSync(configPath, original); + writePythonWrapper(["printf '%s\\n' '{}'", "exit 0"]); expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); - expect(JSON.parse(readFileSync(targetPath, "utf-8"))).toEqual({ - apiKey: "sk-secret-value", - }); + expect(readFileSync(configPath, "utf-8")).toBe(original); }); - it("fails closed when a required sanitized file cannot be written", () => { + it("fails closed when sanitized output cannot be installed", () => { const configPath = path.join(makeRoot(), "openclaw.json"); - writeFileSync(configPath, JSON.stringify({ apiKey: "sk-secret-value" })); - fsControl.failWriteNames = ["openclaw.json"]; + const original = JSON.stringify({ apiKey: "sk-secret-value" }); + writeFileSync(configPath, original); + const python = spawnSync( + "python3", + ["-I", "-c", "import os, sys; print(os.path.realpath(sys.executable))"], + { encoding: "utf-8" }, + ); + expect(python.status, python.stderr).toBe(0); + writePythonWrapper([ + 'if [ "${4-}" = apply ]; then exit 1; fi', + `exec ${shellQuote(python.stdout.trim())} "$@"`, + ]); expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); - expect(JSON.parse(readFileSync(configPath, "utf-8"))).toEqual({ - apiKey: "sk-secret-value", - }); + expect(readFileSync(configPath, "utf-8")).toBe(original); }); - it("fails closed when a sensitive artifact cannot be removed", () => { + it("aborts optional-artifact sanitization when inspection fails", () => { const root = makeRoot(); - writeFileSync(path.join(root, "auth.json"), JSON.stringify({ token: "raw" })); - fsControl.preventRemoval = true; + writeFileSync(path.join(root, "config.json"), JSON.stringify({ token: "raw" })); + writePythonWrapper(["exit 1"]); expect(() => sanitizeMigrationDirectory(root)).toThrow( - /Unable to remove unsanitizable migration artifact/u, + /Failed to inspect migration artifacts safely/u, ); }); - it("omits YAML and env artifacts that cannot be opened safely", () => { + it("removes optional artifacts that are not valid UTF-8", () => { const root = makeRoot(); - const yamlPath = path.join(root, "blocked.yaml"); - const envPath = path.join(root, "blocked.env"); - writeFileSync(yamlPath, "model: keep-me\n"); - writeFileSync(envPath, "MODEL=keep-me\n"); - fsControl.failOpenNames = ["blocked.yaml", "blocked.env"]; + const artifact = path.join(root, "config.json"); + writeFileSync(artifact, Buffer.from([0xff, 0xfe, 0xfd])); sanitizeMigrationDirectory(root); - expect(() => readFileSync(yamlPath)).toThrow(); - expect(() => readFileSync(envPath)).toThrow(); + expect(() => readFileSync(artifact)).toThrow(); }); + + it.runIf(process.platform !== "win32")( + "fails closed when the snapshot root disappears after identity validation", + () => { + const root = makeRoot(); + const movedRoot = `${root}-moved`; + roots.push(movedRoot); + writeFileSync(path.join(root, "config.json"), JSON.stringify({ token: "raw" })); + const python = spawnSync( + "python3", + ["-I", "-c", "import os, sys; print(os.path.realpath(sys.executable))"], + { encoding: "utf-8" }, + ); + expect(python.status, python.stderr).toBe(0); + writePythonWrapper([ + `if [ "\${4-}" = scan-tree ]; then mv ${shellQuote(root)} ${shellQuote(movedRoot)}; fi`, + `exec ${shellQuote(python.stdout.trim())} "$@"`, + ]); + + expect(() => sanitizeMigrationDirectory(root)).toThrow( + /Failed to inspect migration artifacts safely/u, + ); + expect(readFileSync(path.join(movedRoot, "config.json"), "utf-8")).toContain("raw"); + }, + ); }); diff --git a/nemoclaw/src/security/snapshot-sanitizer.test.ts b/nemoclaw/src/security/snapshot-sanitizer.test.ts index 94695fa85b4..00a226a2ba9 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.test.ts @@ -1,10 +1,19 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { spawnSync } from "node:child_process"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { sanitizeMigrationDirectory, sanitizeOpenClawConfigFile } from "./snapshot-sanitizer.js"; const temporaryRoots: string[] = []; @@ -16,9 +25,14 @@ function makeRoot(): string { } afterEach(() => { + vi.unstubAllEnvs(); for (const root of temporaryRoots.splice(0)) rmSync(root, { force: true, recursive: true }); }); +function shellQuote(value: string): string { + return `'${value.replaceAll("'", `'"'"'`)}'`; +} + describe("migration snapshot sanitizer", () => { it("sanitizes credential-shaped values in every supported external artifact", () => { const root = makeRoot(); @@ -130,4 +144,55 @@ describe("migration snapshot sanitizer", () => { expect(readFileSync(path.join(root, "empty.yaml"), "utf-8")).toBe(""); expect(readFileSync(path.join(root, "comments.yaml"), "utf-8")).toBe("# retained context\n"); }); + + it.runIf(process.platform !== "win32")( + "fails closed when a parent directory is swapped after the secure read", + () => { + const root = makeRoot(); + const outside = makeRoot(); + const wrapperRoot = makeRoot(); + const nested = path.join(root, "nested"); + const movedNested = path.join(root, "nested-before-swap"); + const marker = path.join(wrapperRoot, "swapped"); + const wrapper = path.join(wrapperRoot, "python3"); + const outsideConfig = path.join(outside, "config.json"); + mkdirSync(nested); + writeFileSync( + path.join(nested, "config.json"), + JSON.stringify({ apiKey: "sk-secret-value" }), + ); + writeFileSync(outsideConfig, JSON.stringify({ apiKey: "outside-must-not-change" })); + + const python = spawnSync( + "python3", + ["-I", "-c", "import os, sys; print(os.path.realpath(sys.executable))"], + { encoding: "utf-8" }, + ); + expect(python.status, python.stderr).toBe(0); + writeFileSync( + wrapper, + [ + "#!/bin/sh", + `if [ \"\${4-}\" = apply ] && [ ! -e ${shellQuote(marker)} ]; then`, + ` mv ${shellQuote(nested)} ${shellQuote(movedNested)}`, + ` ln -s ${shellQuote(outside)} ${shellQuote(nested)}`, + ` : > ${shellQuote(marker)}`, + "fi", + `exec ${shellQuote(python.stdout.trim())} \"$@\"`, + ].join("\n"), + ); + chmodSync(wrapper, 0o755); + vi.stubEnv("PATH", `${wrapperRoot}:${process.env.PATH ?? ""}`); + + expect(() => sanitizeMigrationDirectory(root)).toThrow( + /Failed to sanitize migration artifacts safely/u, + ); + expect(readFileSync(outsideConfig, "utf-8")).toBe( + JSON.stringify({ apiKey: "outside-must-not-change" }), + ); + expect(readFileSync(path.join(movedNested, "config.json"), "utf-8")).toContain( + "sk-secret-value", + ); + }, + ); }); From 9aea329ebae7582294b4465378d4c9b883434927 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 06:14:11 -0700 Subject: [PATCH 15/19] docs(security): document snapshot sanitizer boundary Signed-off-by: Apurv Kumaria --- docs/get-started/prerequisites.mdx | 5 +++++ docs/manage-sandboxes/backup-restore.mdx | 2 ++ docs/reference/host-files-and-state.mdx | 6 ++++-- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/docs/get-started/prerequisites.mdx b/docs/get-started/prerequisites.mdx index 41859d63a6d..1a071cb5aac 100644 --- a/docs/get-started/prerequisites.mdx +++ b/docs/get-started/prerequisites.mdx @@ -31,6 +31,7 @@ If you cannot add memory, configure at least 8 GB of swap to work around the iss |------------|----------------------------------| | Node.js | 22.19 or later | | npm | 10 or later | +| Python | `python3` with POSIX descriptor-relative filesystem support | | Docker | Docker Engine, Docker Desktop, or Colima on a tested platform | | Platform | Refer to [Platforms](#platforms) below | @@ -40,6 +41,10 @@ When Docker is missing, the installer downloads Docker's official convenience sc In a non-interactive run, the installer can reactivate the group through `sg docker` and continue onboarding. If that path is unavailable or does not restore Docker access, the installer exits with `newgrp docker` guidance before it starts onboarding. +NemoClaw uses an isolated `python3` helper for descriptor-relative migration snapshot sanitization and deletion. +Supported Linux, macOS, and WSL environments provide the required POSIX filesystem operations. +Native Windows is not a supported execution path; use WSL. + If you choose the native Linux Ollama install path, the onboard wizard also requires `zstd` for Ollama archive extraction. The installer also requires `strings` from `binutils` to verify the OpenShell binary before it continues with OpenShell install work. diff --git a/docs/manage-sandboxes/backup-restore.mdx b/docs/manage-sandboxes/backup-restore.mdx index b7728f8285a..10f2d02d2d4 100644 --- a/docs/manage-sandboxes/backup-restore.mdx +++ b/docs/manage-sandboxes/backup-restore.mdx @@ -43,6 +43,8 @@ It preserves OpenShell credential placeholders so rebuild can reattach the host- If NemoClaw cannot sanitize a copied configuration or environment file, it omits that file from the snapshot. If it cannot remove the unsafe file, snapshot creation returns an error. It deletes the incomplete backup when cleanup succeeds and reports when the backup remains. +This sanitization uses an isolated `python3` helper on POSIX hosts to keep reads, replacements, and removals anchored to opened directory descriptors. +If a copied file or parent directory changes identity during the operation, snapshot creation fails closed instead of following the changed path. Hermes snapshots include `SOUL.md`, the Web Dashboard profile under `.hermes/dashboard-home/`, the SQLite database behind `.hermes/state.db`, and the default kanban board in `.hermes/kanban.db`. diff --git a/docs/reference/host-files-and-state.mdx b/docs/reference/host-files-and-state.mdx index 95892c1852e..db37110da7e 100644 --- a/docs/reference/host-files-and-state.mdx +++ b/docs/reference/host-files-and-state.mdx @@ -56,6 +56,8 @@ If you see `registry.json` in older tests, notes, or discussions, treat it as le Before NemoClaw retains a migration snapshot, it recursively sanitizes the copied OpenClaw state and every configured external root. It preserves empty or comment-only YAML files and omits copied JSON, YAML, or `.env` files that it cannot sanitize. If NemoClaw cannot remove an unsafe copied artifact, snapshot creation fails and attempts to delete the incomplete snapshot directory. +Sanitization requires `python3` on a POSIX host so every traversal and mutation can remain anchored to opened directory descriptors. +It fails closed if a copied file or parent directory changes identity during sanitization. The direct blueprint runner accepts these action arguments for migration snapshots: @@ -71,8 +73,8 @@ An integration that invokes the direct runner can use `snapshots list` first to `snapshots delete` accepts only one timestamped directory directly under `~/.nemoclaw/snapshots/`. Both deletion commands are irreversible: they do not modify a running sandbox, but they remove host state that could otherwise be used for rollback or restore. -Snapshot deletion requires `python3` on a POSIX host. -Listing works on native Windows, but deletion does not; use WSL to prune or delete snapshots on Windows. +Snapshot sanitization and deletion require `python3` on a POSIX host. +Listing works on native Windows, but migration snapshot creation and deletion do not; use WSL for those operations on Windows. ## Uninstall Behavior From 32c80651b6596acc7563debb562d4f368bfe6805 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 06:40:03 -0700 Subject: [PATCH 16/19] fix(security): pin rebuild backup traversal Signed-off-by: Apurv Kumaria --- nemoclaw/src/security/snapshot-sanitizer.ts | 202 ++---------------- .../snapshot-sanitizer-boundary.cts} | 192 ++++++++++++++++- nemoclaw/tsconfig.shared.json | 6 +- nemoclaw/vitest.project.ts | 8 + src/lib/security/credential-filter.ts | 88 +++++--- src/lib/security/snapshot-sanitizer.ts | 82 +++++++ .../state/sandbox-backup-sanitization.test.ts | 58 ++++- src/lib/state/sandbox.ts | 49 +---- test/plugin-vitest-project.test.ts | 7 + vitest.config.ts | 7 + 10 files changed, 433 insertions(+), 266 deletions(-) rename nemoclaw/src/{security/snapshot-sanitizer-python.ts => shared/snapshot-sanitizer-boundary.cts} (71%) create mode 100644 src/lib/security/snapshot-sanitizer.ts diff --git a/nemoclaw/src/security/snapshot-sanitizer.ts b/nemoclaw/src/security/snapshot-sanitizer.ts index 288e68a41a1..25ee876e5da 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.ts @@ -1,13 +1,19 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { spawnSync } from "node:child_process"; -import { lstatSync, realpathSync } from "node:fs"; import path from "node:path"; import JSON5 from "json5"; import { parse as parseYaml, stringify as stringifyYaml } from "yaml"; -import { buildSubprocessEnv } from "../lib/subprocess-env.js"; import { isObjectRecord } from "../shared/object-record.js"; +import { + applyDescriptorSnapshotActions, + type DescriptorSnapshotRoot, + decodeDescriptorSnapshotContent, + inspectDescriptorSnapshotRoot, + type SnapshotSanitizationAction, + type SnapshotScannedFile, + scanDescriptorSnapshot, +} from "../shared/snapshot-sanitizer-boundary.cjs"; import { CREDENTIAL_PLACEHOLDER, CREDENTIAL_SENSITIVE_BASENAMES, @@ -17,46 +23,9 @@ import { stripCredentials, valueLooksLikeSecret, } from "./credential-filter.js"; -import { SNAPSHOT_SANITIZER_PYTHON } from "./snapshot-sanitizer-python.js"; -const HELPER_TIMEOUT_MS = 60_000; -const HELPER_MAX_BUFFER_BYTES = 48 * 1024 * 1024; const MAX_SANITIZATION_PASSES = 3; -interface FileIdentity { - readonly dev: string; - readonly ino: string; - readonly mode: string; - readonly nlink: string; - readonly size: string; - readonly mtimeNs: string; - readonly ctimeNs: string; -} - -interface ScannedFile { - readonly path: string; - readonly metadata: FileIdentity; - readonly content?: string; -} - -interface ScanResult { - readonly root: FileIdentity; - readonly directories: Readonly>; - readonly files: readonly ScannedFile[]; -} - -interface SanitizationAction { - readonly kind: "remove" | "replace"; - readonly path: string; - readonly metadata: FileIdentity; - readonly content?: string; -} - -interface SnapshotRoot { - readonly canonicalPath: string; - readonly identity: FileIdentity; -} - function sanitizeTopLevelValue(value: unknown): unknown { if (typeof value !== "string" || isSafeCredentialPlaceholder(value)) { return stripCredentials(value); @@ -70,137 +39,6 @@ function withoutGateway(value: unknown): unknown { return config; } -function isFileIdentity(value: unknown): value is FileIdentity { - if (!isObjectRecord(value)) return false; - return ["dev", "ino", "mode", "nlink", "size", "mtimeNs", "ctimeNs"].every( - (key) => typeof value[key] === "string", - ); -} - -function parseScanResult(stdout: string): ScanResult | null { - try { - const parsed: unknown = JSON.parse(stdout); - if (!isObjectRecord(parsed) || !isFileIdentity(parsed.root)) return null; - if (!isObjectRecord(parsed.directories) || !Array.isArray(parsed.files)) return null; - - const directories: Record = {}; - for (const [relativePath, identity] of Object.entries(parsed.directories)) { - if (!isSafeRelativePath(relativePath) || !isFileIdentity(identity)) return null; - directories[relativePath] = identity; - } - - const files: ScannedFile[] = []; - for (const value of parsed.files) { - if (!isObjectRecord(value) || !isSafeRelativePath(value.path)) return null; - if (!isFileIdentity(value.metadata)) return null; - if (value.content !== undefined && typeof value.content !== "string") return null; - files.push({ - path: value.path, - metadata: value.metadata, - ...(typeof value.content === "string" ? { content: value.content } : {}), - }); - } - return { root: parsed.root, directories, files }; - } catch { - return null; - } -} - -function isSafeRelativePath(value: unknown): value is string { - if (typeof value !== "string" || value === "" || path.isAbsolute(value)) return false; - if (value.includes("\\")) return false; - return value.split("/").every((part) => part !== "" && part !== "." && part !== ".."); -} - -function rootIdentity(rootPath: string): SnapshotRoot | null { - let observed: ReturnType; - try { - observed = lstatSync(rootPath, { bigint: true }); - } catch (error: unknown) { - if ((error as NodeJS.ErrnoException).code === "ENOENT") return null; - throw error; - } - if (!observed.isDirectory() || observed.isSymbolicLink()) { - throw new Error(`Migration snapshot root is not a safe directory: ${rootPath}`); - } - const canonicalPath = realpathSync(rootPath); - const canonical = lstatSync(canonicalPath, { bigint: true }); - if (canonical.dev !== observed.dev || canonical.ino !== observed.ino) { - throw new Error(`Migration snapshot root changed while it was resolved: ${rootPath}`); - } - return { - canonicalPath, - identity: { - dev: String(observed.dev), - ino: String(observed.ino), - mode: String(observed.mode), - nlink: String(observed.nlink), - size: String(observed.size), - mtimeNs: String(observed.mtimeNs), - ctimeNs: String(observed.ctimeNs), - }, - }; -} - -function scanSnapshot(root: SnapshotRoot, targetName?: string): ScanResult | null { - const mode = targetName === undefined ? "scan-tree" : "scan-file"; - const result = spawnSync( - "python3", - [ - "-I", - "-c", - SNAPSHOT_SANITIZER_PYTHON, - mode, - root.canonicalPath, - JSON.stringify(root.identity), - targetName ?? "", - JSON.stringify([...CREDENTIAL_SENSITIVE_BASENAMES]), - ], - { - encoding: "utf-8", - env: buildSubprocessEnv(), - maxBuffer: HELPER_MAX_BUFFER_BYTES, - timeout: HELPER_TIMEOUT_MS, - }, - ); - if (result.status !== 0 || result.error) return null; - return parseScanResult(result.stdout); -} - -function applyActions( - canonicalPath: string, - scan: ScanResult, - actions: readonly SanitizationAction[], -): boolean { - if (actions.length === 0) return true; - const result = spawnSync( - "python3", - ["-I", "-c", SNAPSHOT_SANITIZER_PYTHON, "apply", canonicalPath], - { - encoding: "utf-8", - env: buildSubprocessEnv(), - input: JSON.stringify({ root: scan.root, directories: scan.directories, actions }), - maxBuffer: HELPER_MAX_BUFFER_BYTES, - timeout: HELPER_TIMEOUT_MS, - }, - ); - return result.status === 0 && !result.error; -} - -function decodeScannedContent(content: string | undefined): string | null { - if ( - content === undefined || - !/^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/u.test(content) - ) { - return null; - } - const decoded = Buffer.from(content, "base64"); - if (decoded.toString("base64") !== content) return null; - const utf8 = decoded.toString("utf-8"); - if (!Buffer.from(utf8, "utf-8").equals(decoded)) return null; - return utf8; -} - function sanitizedContents(name: string, raw: string): string | null | undefined { try { if (name.endsWith(".json")) { @@ -221,12 +59,12 @@ function sanitizedContents(name: string, raw: string): string | null | undefined return undefined; } -function actionForScannedFile(file: ScannedFile): SanitizationAction | null { +function actionForScannedFile(file: SnapshotScannedFile): SnapshotSanitizationAction | null { const name = path.posix.basename(file.path).toLowerCase(); if (isSensitiveFile(name)) { return { kind: "remove", path: file.path, metadata: file.metadata }; } - const raw = decodeScannedContent(file.content); + const raw = decodeDescriptorSnapshotContent(file.content); if (raw === null) { return { kind: "remove", path: file.path, metadata: file.metadata }; } @@ -251,20 +89,20 @@ function actionForScannedFile(file: ScannedFile): SanitizationAction | null { */ export function sanitizeMigrationDirectory(rootPath: string): void { for (let pass = 0; pass < MAX_SANITIZATION_PASSES; pass += 1) { - const root = rootIdentity(rootPath); + const root = inspectDescriptorSnapshotRoot(rootPath); if (root === null) { if (pass === 0) return; throw new Error(`Failed to inspect migration artifacts safely: ${rootPath}`); } - const scan = scanSnapshot(root); + const scan = scanDescriptorSnapshot(root, CREDENTIAL_SENSITIVE_BASENAMES); if (scan === null) { throw new Error(`Failed to inspect migration artifacts safely: ${rootPath}`); } const actions = scan.files .map((file) => actionForScannedFile(file)) - .filter((action): action is SanitizationAction => action !== null); + .filter((action): action is SnapshotSanitizationAction => action !== null); if (actions.length === 0) return; - if (!applyActions(root.canonicalPath, scan, actions)) { + if (!applyDescriptorSnapshotActions(root, scan, actions)) { throw new Error(`Failed to sanitize migration artifacts safely: ${rootPath}`); } } @@ -279,24 +117,24 @@ export function sanitizeOpenClawConfigFile(configPath: string): boolean { const targetName = path.basename(configPath); if (targetName === "" || targetName === "." || targetName === "..") return false; for (let pass = 0; pass < MAX_SANITIZATION_PASSES; pass += 1) { - let root: SnapshotRoot | null; + let root: DescriptorSnapshotRoot | null; try { - root = rootIdentity(parentPath); + root = inspectDescriptorSnapshotRoot(parentPath); } catch { return false; } if (root === null) return false; - const scan = scanSnapshot(root, targetName); + const scan = scanDescriptorSnapshot(root, CREDENTIAL_SENSITIVE_BASENAMES, targetName); if (scan === null || scan.files.length !== 1) return false; const file = scan.files[0]; if (!file || file.path !== targetName) return false; - const raw = decodeScannedContent(file.content); + const raw = decodeDescriptorSnapshotContent(file.content); if (raw === null) return false; const sanitized = sanitizedContents(targetName.toLowerCase(), raw); if (typeof sanitized !== "string") return false; if (sanitized === raw) return true; if ( - !applyActions(root.canonicalPath, scan, [ + !applyDescriptorSnapshotActions(root, scan, [ { kind: "replace", path: file.path, diff --git a/nemoclaw/src/security/snapshot-sanitizer-python.ts b/nemoclaw/src/shared/snapshot-sanitizer-boundary.cts similarity index 71% rename from nemoclaw/src/security/snapshot-sanitizer-python.ts rename to nemoclaw/src/shared/snapshot-sanitizer-boundary.cts index f390132b05a..ce9a0e80025 100644 --- a/nemoclaw/src/security/snapshot-sanitizer-python.ts +++ b/nemoclaw/src/shared/snapshot-sanitizer-boundary.cts @@ -1,8 +1,53 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import { spawnSync } from "node:child_process"; +import { lstatSync, realpathSync } from "node:fs"; +import path from "node:path"; + +const HELPER_TIMEOUT_MS = 60_000; +const HELPER_MAX_BUFFER_BYTES = 48 * 1024 * 1024; + +function snapshotSanitizerEnvironment(): Record { + return typeof process.env.PATH === "string" ? { PATH: process.env.PATH } : {}; +} + +export interface SnapshotFileIdentity { + readonly dev: string; + readonly ino: string; + readonly mode: string; + readonly nlink: string; + readonly size: string; + readonly mtimeNs: string; + readonly ctimeNs: string; +} + +export interface SnapshotScannedFile { + readonly path: string; + readonly metadata: SnapshotFileIdentity; + readonly content?: string; +} + +export interface DescriptorSnapshotScan { + readonly root: SnapshotFileIdentity; + readonly directories: Readonly>; + readonly files: readonly SnapshotScannedFile[]; +} + +export interface SnapshotSanitizationAction { + readonly kind: "remove" | "replace"; + readonly path: string; + readonly metadata: SnapshotFileIdentity; + readonly content?: string; +} + +export interface DescriptorSnapshotRoot { + readonly canonicalPath: string; + readonly identity: SnapshotFileIdentity; +} + /** - * Descriptor-relative filesystem helper for migration snapshot sanitization. + * Descriptor-relative filesystem helper for copied snapshot sanitization. * * The plugin package publishes compiled JavaScript only, so the helper is * passed as immutable source to an isolated Python interpreter. Every path @@ -10,7 +55,7 @@ * and every mutation revalidates the exact inode version observed by the * read pass before replacing or unlinking it. */ -export const SNAPSHOT_SANITIZER_PYTHON = String.raw` +const SNAPSHOT_SANITIZER_PYTHON = String.raw` import base64 import json import os @@ -437,3 +482,146 @@ if __name__ == "__main__": except Exception as error: fail(str(error)) `.trim(); + +function isObjectRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function isFileIdentity(value: unknown): value is SnapshotFileIdentity { + if (!isObjectRecord(value)) return false; + return ["dev", "ino", "mode", "nlink", "size", "mtimeNs", "ctimeNs"].every( + (key) => typeof value[key] === "string", + ); +} + +function isSafeRelativePath(value: unknown): value is string { + if (typeof value !== "string" || value === "" || path.isAbsolute(value)) return false; + if (value.includes("\\")) return false; + return value.split("/").every((part) => part !== "" && part !== "." && part !== ".."); +} + +function parseScanResult(stdout: string): DescriptorSnapshotScan | null { + try { + const parsed: unknown = JSON.parse(stdout); + if (!isObjectRecord(parsed) || !isFileIdentity(parsed.root)) return null; + if (!isObjectRecord(parsed.directories) || !Array.isArray(parsed.files)) return null; + + const directories: Record = {}; + for (const [relativePath, identity] of Object.entries(parsed.directories)) { + if (!isSafeRelativePath(relativePath) || !isFileIdentity(identity)) return null; + directories[relativePath] = identity; + } + + const files: SnapshotScannedFile[] = []; + for (const value of parsed.files) { + if (!isObjectRecord(value) || !isSafeRelativePath(value.path)) return null; + if (!isFileIdentity(value.metadata)) return null; + if (value.content !== undefined && typeof value.content !== "string") return null; + files.push({ + path: value.path, + metadata: value.metadata, + ...(typeof value.content === "string" ? { content: value.content } : {}), + }); + } + return { root: parsed.root, directories, files }; + } catch { + return null; + } +} + +/** Resolve and pin one snapshot root without accepting a final-component symlink. */ +export function inspectDescriptorSnapshotRoot(rootPath: string): DescriptorSnapshotRoot | null { + let observed: ReturnType; + try { + observed = lstatSync(rootPath, { bigint: true }); + } catch (error: unknown) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return null; + throw error; + } + if (!observed.isDirectory() || observed.isSymbolicLink()) { + throw new Error(`Snapshot root is not a safe directory: ${rootPath}`); + } + const canonicalPath = realpathSync(rootPath); + const canonical = lstatSync(canonicalPath, { bigint: true }); + if (canonical.dev !== observed.dev || canonical.ino !== observed.ino) { + throw new Error(`Snapshot root changed while it was resolved: ${rootPath}`); + } + return { + canonicalPath, + identity: { + dev: String(observed.dev), + ino: String(observed.ino), + mode: String(observed.mode), + nlink: String(observed.nlink), + size: String(observed.size), + mtimeNs: String(observed.mtimeNs), + ctimeNs: String(observed.ctimeNs), + }, + }; +} + +/** Read a bounded snapshot tree through pinned directory descriptors. */ +export function scanDescriptorSnapshot( + root: DescriptorSnapshotRoot, + sensitiveNames: ReadonlySet, + targetName?: string, +): DescriptorSnapshotScan | null { + const mode = targetName === undefined ? "scan-tree" : "scan-file"; + const result = spawnSync( + "python3", + [ + "-I", + "-c", + SNAPSHOT_SANITIZER_PYTHON, + mode, + root.canonicalPath, + JSON.stringify(root.identity), + targetName ?? "", + JSON.stringify([...sensitiveNames]), + ], + { + encoding: "utf-8", + env: snapshotSanitizerEnvironment(), + maxBuffer: HELPER_MAX_BUFFER_BYTES, + timeout: HELPER_TIMEOUT_MS, + }, + ); + if (result.status !== 0 || result.error) return null; + return parseScanResult(result.stdout); +} + +/** Install or remove sanitized artifacts through their pinned parent descriptors. */ +export function applyDescriptorSnapshotActions( + root: DescriptorSnapshotRoot, + scan: DescriptorSnapshotScan, + actions: readonly SnapshotSanitizationAction[], +): boolean { + if (actions.length === 0) return true; + const result = spawnSync( + "python3", + ["-I", "-c", SNAPSHOT_SANITIZER_PYTHON, "apply", root.canonicalPath], + { + encoding: "utf-8", + env: snapshotSanitizerEnvironment(), + input: JSON.stringify({ root: scan.root, directories: scan.directories, actions }), + maxBuffer: HELPER_MAX_BUFFER_BYTES, + timeout: HELPER_TIMEOUT_MS, + }, + ); + return result.status === 0 && !result.error; +} + +/** Decode one helper payload and reject non-canonical base64 or invalid UTF-8. */ +export function decodeDescriptorSnapshotContent(content: string | undefined): string | null { + if ( + content === undefined || + !/^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$/u.test(content) + ) { + return null; + } + const decoded = Buffer.from(content, "base64"); + if (decoded.toString("base64") !== content) return null; + const utf8 = decoded.toString("utf-8"); + if (!Buffer.from(utf8, "utf-8").equals(decoded)) return null; + return utf8; +} diff --git a/nemoclaw/tsconfig.shared.json b/nemoclaw/tsconfig.shared.json index 6545b3e953c..68578e9e315 100644 --- a/nemoclaw/tsconfig.shared.json +++ b/nemoclaw/tsconfig.shared.json @@ -4,6 +4,10 @@ "outDir": "dist", "rootDir": "src" }, - "include": ["src/shared/openshell-policy-boundary.cts", "src/shared/sandbox-name.cts"], + "include": [ + "src/shared/openshell-policy-boundary.cts", + "src/shared/sandbox-name.cts", + "src/shared/snapshot-sanitizer-boundary.cts" + ], "exclude": ["node_modules", "dist"] } diff --git a/nemoclaw/vitest.project.ts b/nemoclaw/vitest.project.ts index 0ba1d08b5aa..1ee0e02bea5 100644 --- a/nemoclaw/vitest.project.ts +++ b/nemoclaw/vitest.project.ts @@ -9,6 +9,10 @@ const canonicalOpenShellPolicyBoundary = path.resolve( "src/shared/openshell-policy-boundary.cts", ); const canonicalSandboxName = path.resolve(import.meta.dirname, "src/shared/sandbox-name.cts"); +const canonicalSnapshotSanitizerBoundary = path.resolve( + import.meta.dirname, + "src/shared/snapshot-sanitizer-boundary.cts", +); type PluginVitestProjectOptions = { root: string; @@ -47,6 +51,10 @@ const pluginVitestProjectOptions = { find: /^.*sandbox-name\.cjs$/, replacement: canonicalSandboxName, }, + { + find: /^.*snapshot-sanitizer-boundary\.cjs$/, + replacement: canonicalSnapshotSanitizerBoundary, + }, ], env: { NEMOCLAW_DISABLE_GATEWAY_DRIFT_PREFLIGHT: "1", diff --git a/src/lib/security/credential-filter.ts b/src/lib/security/credential-filter.ts index ae27d4da48d..0dd121e745b 100644 --- a/src/lib/security/credential-filter.ts +++ b/src/lib/security/credential-filter.ts @@ -427,6 +427,56 @@ function toConfigValue(value: unknown): ConfigValue | typeof UNREPRESENTABLE_CON return result; } +/** + * Strip credential fields from a Hermes YAML config body. + * + * Returns null when the document cannot be represented safely. Empty and + * comment-only documents are returned unchanged because they contain no + * credential material. + */ +export function sanitizeYamlConfigContent(rawConfig: string): string | null { + try { + const parsed = parseYaml(rawConfig); + if (parsed === null || parsed === undefined) return rawConfig; + const configValue = toConfigValue(parsed); + if (configValue === UNREPRESENTABLE_CONFIG_VALUE || !isConfigObject(configValue)) { + return null; + } + + const { gateway: _gateway, ...config } = configValue; + return stringifyYaml(stripCredentials(config)); + } catch { + return null; + } +} + +/** + * Strip credential fields from a JSON or YAML config body. + * + * The filename is used only to decide whether a non-JSON document is an + * allowed YAML target. Returns null when the input cannot be sanitized. + */ +export function sanitizeConfigFileContent(configName: string, rawConfig: string): string | null { + try { + const parsed = parseJson(rawConfig); + if (!isConfigValue(parsed)) return null; + let config = parsed; + if (isConfigObject(parsed)) { + const { gateway: _gateway, ...withoutGateway } = parsed; + config = withoutGateway; + } + return JSON.stringify(stripCredentials(config), null, 2); + } catch { + // Fall through to YAML for Hermes and other non-JSON configs. + } + + const normalized = basename(configName).toLowerCase(); + if (normalized.endsWith(".yaml") || normalized.endsWith(".yml")) { + return sanitizeYamlConfigContent(rawConfig); + } + return null; +} + /** * Strip credential fields from a Hermes YAML config file in-place. * Removes the "gateway" section when present (auth tokens — regenerated @@ -438,19 +488,11 @@ export function sanitizeYamlConfigFile( ): boolean { const rawConfig = readRegularFileNoFollow(configPath); if (rawConfig === null) return false; + const sanitized = sanitizeYamlConfigContent(rawConfig); + if (sanitized === null) return false; + if (sanitized === rawConfig) return true; try { - const parsed = parseYaml(rawConfig); - // Empty and comment-only YAML documents contain no credentials. Preserve - // them instead of misclassifying the parser's null result as unsafe. - if (parsed === null || parsed === undefined) return true; - const configValue = toConfigValue(parsed); - if (configValue === UNREPRESENTABLE_CONFIG_VALUE || !isConfigObject(configValue)) { - return false; - } - - const { gateway: _gateway, ...config } = configValue; - const sanitized = stripCredentials(config); - writeSanitized(configPath, stringifyYaml(sanitized)); + writeSanitized(configPath, sanitized); return true; } catch { return false; @@ -468,27 +510,15 @@ export function sanitizeYamlConfigFile( export function sanitizeConfigFile(configPath: string): boolean { const rawConfig = readRegularFileNoFollow(configPath); if (rawConfig === null) return false; - + const sanitized = sanitizeConfigFileContent(configPath, rawConfig); + if (sanitized === null) return false; + if (sanitized === rawConfig) return true; try { - const parsed = parseJson(rawConfig); - if (!isConfigValue(parsed)) return false; - let config = parsed; - if (isConfigObject(parsed)) { - const { gateway: _gateway, ...withoutGateway } = parsed; - config = withoutGateway; - } - const sanitized = stripCredentials(config); - writeFileAtomically(configPath, JSON.stringify(sanitized, null, 2)); + writeFileAtomically(configPath, sanitized); return true; } catch { - // Fall through to YAML for Hermes and other non-JSON configs. - } - - const normalized = basename(configPath).toLowerCase(); - if (normalized.endsWith(".yaml") || normalized.endsWith(".yml")) { - return sanitizeYamlConfigFile(configPath); + return false; } - return false; } /** diff --git a/src/lib/security/snapshot-sanitizer.ts b/src/lib/security/snapshot-sanitizer.ts new file mode 100644 index 00000000000..5424ef93f34 --- /dev/null +++ b/src/lib/security/snapshot-sanitizer.ts @@ -0,0 +1,82 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import path from "node:path"; + +import { + applyDescriptorSnapshotActions, + decodeDescriptorSnapshotContent, + inspectDescriptorSnapshotRoot, + type SnapshotSanitizationAction, + type SnapshotScannedFile, + scanDescriptorSnapshot, +} from "../../../nemoclaw/dist/shared/snapshot-sanitizer-boundary.cjs"; + +import { + CREDENTIAL_SENSITIVE_BASENAMES, + isSensitiveFile, + sanitizeConfigFileContent, + sanitizeEnvFileContent, +} from "./credential-filter"; + +const MAX_SANITIZATION_PASSES = 3; + +function actionForScannedFile(file: SnapshotScannedFile): SnapshotSanitizationAction | null { + const name = path.posix.basename(file.path).toLowerCase(); + if (isSensitiveFile(name)) { + return { kind: "remove", path: file.path, metadata: file.metadata }; + } + + const raw = decodeDescriptorSnapshotContent(file.content); + if (raw === null) { + return { kind: "remove", path: file.path, metadata: file.metadata }; + } + + let sanitized: string | null; + if (name === ".env" || name.endsWith(".env")) { + sanitized = sanitizeEnvFileContent(raw); + } else { + sanitized = sanitizeConfigFileContent(name, raw); + } + if (sanitized === null) { + return { kind: "remove", path: file.path, metadata: file.metadata }; + } + const hasRestrictedMode = (Number(file.metadata.mode) & 0o777) === 0o600; + if (sanitized === raw && hasRestrictedMode) return null; + return { + kind: "replace", + path: file.path, + metadata: file.metadata, + content: Buffer.from(sanitized, "utf-8").toString("base64"), + }; +} + +/** + * Sanitize every credential-bearing artifact beneath a copied snapshot root. + * + * Both discovery and mutation use the shared descriptor-relative helper. A + * directory or file that changes after inspection therefore fails closed + * instead of redirecting the sanitizer outside the snapshot root. + */ +export function sanitizeSnapshotDirectory(rootPath: string): void { + for (let pass = 0; pass < MAX_SANITIZATION_PASSES; pass += 1) { + const root = inspectDescriptorSnapshotRoot(rootPath); + if (root === null) { + if (pass === 0) return; + throw new Error(`Failed to inspect snapshot artifacts safely: ${rootPath}`); + } + + const scan = scanDescriptorSnapshot(root, CREDENTIAL_SENSITIVE_BASENAMES); + if (scan === null) { + throw new Error(`Failed to inspect snapshot artifacts safely: ${rootPath}`); + } + const actions = scan.files + .map((file) => actionForScannedFile(file)) + .filter((action): action is SnapshotSanitizationAction => action !== null); + if (actions.length === 0) return; + if (!applyDescriptorSnapshotActions(root, scan, actions)) { + throw new Error(`Failed to sanitize snapshot artifacts safely: ${rootPath}`); + } + } + throw new Error(`Snapshot artifacts did not reach a stable sanitized state: ${rootPath}`); +} diff --git a/src/lib/state/sandbox-backup-sanitization.test.ts b/src/lib/state/sandbox-backup-sanitization.test.ts index 45a7260ee10..62c626883a1 100644 --- a/src/lib/state/sandbox-backup-sanitization.test.ts +++ b/src/lib/state/sandbox-backup-sanitization.test.ts @@ -1,7 +1,9 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import { execFileSync } from "node:child_process"; import { + chmodSync, existsSync, mkdirSync, mkdtempSync, @@ -12,7 +14,7 @@ import { } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { sanitizeBackupDirectory } from "./sandbox.js"; @@ -26,6 +28,7 @@ function createBackup(): string { } afterEach(() => { + vi.unstubAllEnvs(); for (const testDirectory of testDirectories.splice(0)) { rmSync(testDirectory, { recursive: true, force: true }); } @@ -45,6 +48,18 @@ describe("rebuild backup credential sanitization", () => { expect(statSync(envPath).mode & 0o777).toBe(0o600); }); + it("restricts an already-safe config artifact without changing its content", () => { + const backupPath = createBackup(); + const envPath = join(backupPath, "state", ".env"); + const contents = "LOG_LEVEL=info\n"; + writeFileSync(envPath, contents, { mode: 0o644 }); + + sanitizeBackupDirectory(backupPath); + + expect(readFileSync(envPath, "utf-8")).toBe(contents); + expect(statSync(envPath).mode & 0o777).toBe(0o600); + }); + it("omits unsanitizable config and env artifacts", () => { const backupPath = createBackup(); const yamlPath = join(backupPath, "state", "config.yaml"); @@ -53,12 +68,10 @@ describe("rebuild backup credential sanitization", () => { const safePath = join(backupPath, "state", "notes.txt"); writeFileSync(yamlPath, "api_key: [unclosed\n"); writeFileSync(jsonPath, '{"apiKey":'); - writeFileSync(envPath, "DB_PASS=raw-secret\n"); + writeFileSync(envPath, Buffer.from([0xff])); writeFileSync(safePath, "safe"); - sanitizeBackupDirectory(backupPath, { - sanitizeEnvFile: () => false, - }); + sanitizeBackupDirectory(backupPath); expect(existsSync(yamlPath)).toBe(false); expect(existsSync(jsonPath)).toBe(false); @@ -73,7 +86,7 @@ describe("rebuild backup credential sanitization", () => { expect(() => sanitizeBackupDirectory(backupPath, { - unlinkFile: () => { + sanitizeDirectory: () => { throw new Error("injected unlink failure"); }, }), @@ -88,7 +101,7 @@ describe("rebuild backup credential sanitization", () => { expect(() => sanitizeBackupDirectory(backupPath, { - unlinkFile: () => { + sanitizeDirectory: () => { throw new Error("injected unlink failure"); }, removeBackup: () => undefined, @@ -97,4 +110,35 @@ describe("rebuild backup credential sanitization", () => { ).toThrow("Credential sanitization failed and the incomplete backup remains"); expect(existsSync(backupPath)).toBe(true); }); + + it("fails closed when a scanned parent directory is swapped before apply", () => { + const backupPath = createBackup(); + const nestedPath = join(backupPath, "state", "nested"); + const movedPath = join(backupPath, "state", "nested-original"); + mkdirSync(nestedPath); + writeFileSync(join(nestedPath, "config.json"), '{"apiKey":"sk-inside-secret"}'); + + const outsidePath = mkdtempSync(join(tmpdir(), "nemoclaw-sanitize-outside-")); + const wrapperPath = mkdtempSync(join(tmpdir(), "nemoclaw-sanitize-python-")); + testDirectories.push(outsidePath, wrapperPath); + const outsideConfigPath = join(outsidePath, "config.json"); + const outsideContents = '{"apiKey":"sk-outside-secret"}'; + writeFileSync(outsideConfigPath, outsideContents); + + const realPython = execFileSync("which", ["python3"], { encoding: "utf-8" }).trim(); + const shellQuote = (value: string): string => `'${value.replaceAll("'", `'\\''`)}'`; + const pythonWrapper = join(wrapperPath, "python3"); + writeFileSync( + pythonWrapper, + `#!/bin/sh\nif [ "$4" = "apply" ]; then\n mv ${shellQuote(nestedPath)} ${shellQuote(movedPath)}\n ln -s ${shellQuote(outsidePath)} ${shellQuote(nestedPath)}\nfi\nexec ${shellQuote(realPython)} "$@"\n`, + ); + chmodSync(pythonWrapper, 0o755); + vi.stubEnv("PATH", `${wrapperPath}:${process.env.PATH ?? ""}`); + + expect(() => sanitizeBackupDirectory(backupPath)).toThrow( + "Credential sanitization failed; removed the incomplete backup", + ); + expect(existsSync(backupPath)).toBe(false); + expect(readFileSync(outsideConfigPath, "utf-8")).toBe(outsideContents); + }); }); diff --git a/src/lib/state/sandbox.ts b/src/lib/state/sandbox.ts index 17d370e64e1..72e3ef9028b 100644 --- a/src/lib/state/sandbox.ts +++ b/src/lib/state/sandbox.ts @@ -24,7 +24,6 @@ import { renameSync, rmSync, statSync, - unlinkSync, writeFileSync, } from "node:fs"; import os from "node:os"; @@ -44,11 +43,7 @@ import { } from "../domain/backup-failure.js"; import { shellQuote } from "../runner.js"; import { createTempSshConfig } from "../sandbox/temp-ssh-config.js"; -import { - isSensitiveFile, - sanitizeConfigFile, - sanitizeEnvFile, -} from "../security/credential-filter.js"; +import { sanitizeSnapshotDirectory } from "../security/snapshot-sanitizer.js"; import { buildRestoreCleanupCommand, buildRestoreTarArgs, @@ -626,22 +621,14 @@ function computeBlueprintDigest(): string | null { return null; } -/** - * Walk a local directory and sanitize any JSON config files found. - * Also removes files that match CREDENTIAL_SENSITIVE_BASENAMES. - */ export interface BackupSanitizationOperations { - sanitizeConfigFile: (filePath: string) => boolean; - sanitizeEnvFile: (filePath: string) => boolean; - unlinkFile: (filePath: string) => void; + sanitizeDirectory: (backupPath: string) => void; removeBackup: (backupPath: string) => void; backupExists: (backupPath: string) => boolean; } const DEFAULT_BACKUP_SANITIZATION_OPERATIONS: BackupSanitizationOperations = { - sanitizeConfigFile, - sanitizeEnvFile, - unlinkFile: unlinkSync, + sanitizeDirectory: sanitizeSnapshotDirectory, removeBackup: (backupPath) => rmSync(backupPath, { recursive: true, force: true }), backupExists: existsSync, }; @@ -651,38 +638,10 @@ export function sanitizeBackupDirectory( dirPath: string, overrides: Partial = {}, ): void { - if (!existsSync(dirPath)) return; const operations = { ...DEFAULT_BACKUP_SANITIZATION_OPERATIONS, ...overrides }; - const walk = (current: string): void => { - for (const entry of readdirSync(current, { withFileTypes: true })) { - const fullPath = path.join(current, entry.name); - if (entry.isDirectory()) { - walk(fullPath); - } else if (entry.isFile()) { - const name = entry.name.toLowerCase(); - if (isSensitiveFile(entry.name)) { - operations.unlinkFile(fullPath); - } else if (name.endsWith(".json") || name.endsWith(".yaml") || name.endsWith(".yml")) { - // JSON (OpenClaw) and YAML (Hermes config.yaml) both carry secrets. - // Fail closed: omit the artifact when sanitization cannot run. - if (!operations.sanitizeConfigFile(fullPath)) { - operations.unlinkFile(fullPath); - } - } else if (name === ".env" || name.endsWith(".env")) { - // Hermes stores API keys in .env alongside config.yaml. - if (operations.sanitizeEnvFile(fullPath)) { - chmodSync(fullPath, 0o600); - } else { - operations.unlinkFile(fullPath); - } - } - } - } - }; - try { - walk(dirPath); + operations.sanitizeDirectory(dirPath); } catch (error) { try { operations.removeBackup(dirPath); diff --git a/test/plugin-vitest-project.test.ts b/test/plugin-vitest-project.test.ts index 1a81e61b07c..40b37af1829 100644 --- a/test/plugin-vitest-project.test.ts +++ b/test/plugin-vitest-project.test.ts @@ -65,6 +65,13 @@ describe("plugin Vitest project contract", () => { find: /^.*sandbox-name\.cjs$/, replacement: path.join(repositoryRoot, "nemoclaw/src/shared/sandbox-name.cts"), }, + { + find: /^.*snapshot-sanitizer-boundary\.cjs$/, + replacement: path.join( + repositoryRoot, + "nemoclaw/src/shared/snapshot-sanitizer-boundary.cts", + ), + }, ]); expect(pluginVitestProjectOptions.test).not.toHaveProperty("globalSetup"); expect(rootPluginProjects).toEqual([pluginVitestProjectOptions]); diff --git a/vitest.config.ts b/vitest.config.ts index 151ed56ac6c..b21fbd483a5 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -24,6 +24,9 @@ const canonicalOpenShellPolicyBoundary = path.resolve( "nemoclaw/src/shared/openshell-policy-boundary.cts", ); const canonicalSandboxName = path.resolve("nemoclaw/src/shared/sandbox-name.cts"); +const canonicalSnapshotSanitizerBoundary = path.resolve( + "nemoclaw/src/shared/snapshot-sanitizer-boundary.cts", +); // Map the generated shared .cjs specifiers back to their .cts source so // source-mode test projects exercise the single source of truth rather than a // possibly-stale build artifact. @@ -36,6 +39,10 @@ const canonicalSourceAliases = [ find: /^.*sandbox-name\.cjs$/, replacement: canonicalSandboxName, }, + { + find: /^.*snapshot-sanitizer-boundary\.cjs$/, + replacement: canonicalSnapshotSanitizerBoundary, + }, ]; const e2ePhaseCollectionAlias = process.env.NEMOCLAW_E2E_PHASE_COLLECTION === "1" From 157595093eefcc320b1f18b08d96052df543a1c3 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 07:01:39 -0700 Subject: [PATCH 17/19] test(security): cover sanitizer boundary failures Signed-off-by: Apurv Kumaria --- .../src/security/credential-filter.test.ts | 39 +++++++++++ .../snapshot-sanitizer-failure.test.ts | 65 +++++++++++++++++++ .../src/security/snapshot-sanitizer.test.ts | 19 ++++++ 3 files changed, 123 insertions(+) diff --git a/nemoclaw/src/security/credential-filter.test.ts b/nemoclaw/src/security/credential-filter.test.ts index b3a27deab52..a2466e512ff 100644 --- a/nemoclaw/src/security/credential-filter.test.ts +++ b/nemoclaw/src/security/credential-filter.test.ts @@ -90,7 +90,10 @@ describe("plugin credential-filter", () => { it("preserves safe placeholders and detects secret-shaped values", () => { expect(isSafeCredentialPlaceholder("unused")).toBe(true); + expect(isSafeCredentialPlaceholder("Bearer unused")).toBe(true); expect(isSafeCredentialPlaceholder("openshell:resolve:env:TOKEN")).toBe(true); + expect(isSafeCredentialPlaceholder("xoxb-OPENSHELL-RESOLVE-ENV-SLACK_TOKEN")).toBe(true); + expect(isSafeCredentialPlaceholder(null)).toBe(false); expect(valueLooksLikeSecret("sk-abcdefghijklmnopqrstuvwxyz")).toBe(true); expect(valueLooksLikeSecret("glpat-abcdefghijklmnopqrst")).toBe(true); expect(valueLooksLikeSecret("nvcf-abcdefghij")).toBe(true); @@ -107,6 +110,29 @@ describe("plugin credential-filter", () => { expect(result.model).toBe("keep"); }); + it("strips inline CLI credentials while preserving explicit safe placeholders", () => { + expect( + stripCredentials([ + "--api-key=opaque-value", + "--token=openshell:resolve:env:SAFE_TOKEN", + { password: "opaque-password" }, + 42, + "--verbose", + "keep-me", + ]), + ).toEqual([ + `--api-key=${CREDENTIAL_PLACEHOLDER}`, + "--token=openshell:resolve:env:SAFE_TOKEN", + { password: CREDENTIAL_PLACEHOLDER }, + 42, + "--verbose", + "keep-me", + ]); + expect(stripCredentials(null)).toBeNull(); + expect(stripCredentials(undefined)).toBeUndefined(); + expect(stripCredentials("keep-me")).toBe("keep-me"); + }); + it("strips secret-shaped env values stored under benign keys", () => { const result = sanitizeEnvFileContent( [ @@ -124,6 +150,19 @@ describe("plugin credential-filter", () => { expect(result).toContain("SAFE=openshell:resolve:env:SAFE"); }); + it("preserves comments, malformed assignments, and values without credential signals", () => { + const content = [ + "# comment", + "NO_EQUALS", + "=missing-key", + "export MODEL=keep-me", + "TOKEN=unused", + "", + ].join("\n"); + + expect(sanitizeEnvFileContent(content)).toBe(content); + }); + it("excludes auth state basenames from migration copies", () => { expect(isSensitiveFile("auth-profiles.json")).toBe(true); expect(isSensitiveFile("auth.json")).toBe(true); diff --git a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts index 611b1b441a3..5f52794a5a7 100644 --- a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts @@ -6,6 +6,13 @@ import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "nod import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { + applyDescriptorSnapshotActions, + decodeDescriptorSnapshotContent, + inspectDescriptorSnapshotRoot, + type SnapshotFileIdentity, + scanDescriptorSnapshot, +} from "../shared/snapshot-sanitizer-boundary.cjs"; import { sanitizeMigrationDirectory, sanitizeOpenClawConfigFile } from "./snapshot-sanitizer.js"; const roots: string[] = []; @@ -35,6 +42,16 @@ afterEach(() => { }); describe("migration snapshot sanitizer fallbacks", () => { + const identity: SnapshotFileIdentity = { + dev: "1", + ino: "2", + mode: "16832", + nlink: "1", + size: "0", + mtimeNs: "3", + ctimeNs: "4", + }; + it("fails closed when the descriptor helper is unavailable", () => { const configPath = path.join(makeRoot(), "openclaw.json"); const original = JSON.stringify({ apiKey: "sk-secret-value" }); @@ -55,6 +72,54 @@ describe("migration snapshot sanitizer fallbacks", () => { expect(readFileSync(configPath, "utf-8")).toBe(original); }); + it("rejects every malformed descriptor scan boundary", () => { + const root = { canonicalPath: makeRoot(), identity }; + const malformedOutputs = [ + "not-json", + JSON.stringify({ root: identity, directories: [], files: [] }), + JSON.stringify({ root: identity, directories: { "nested\\escape": identity }, files: [] }), + JSON.stringify({ root: identity, directories: {}, files: [null] }), + JSON.stringify({ + root: identity, + directories: {}, + files: [{ path: "/escape", metadata: identity }], + }), + JSON.stringify({ + root: identity, + directories: {}, + files: [{ path: "config.json", metadata: null }], + }), + JSON.stringify({ + root: identity, + directories: {}, + files: [{ path: "config.json", metadata: identity, content: 42 }], + }), + ]; + + for (const output of malformedOutputs) { + writePythonWrapper([`printf '%s\\n' ${shellQuote(output)}`]); + expect(scanDescriptorSnapshot(root, new Set())).toBeNull(); + } + }); + + it("rejects unsafe roots and non-canonical helper payloads", () => { + const root = makeRoot(); + const filePath = path.join(root, "not-a-directory"); + writeFileSync(filePath, "content"); + + expect(() => inspectDescriptorSnapshotRoot(filePath)).toThrow(/not a safe directory/u); + expect(decodeDescriptorSnapshotContent(undefined)).toBeNull(); + expect(decodeDescriptorSnapshotContent("not-base64!")).toBeNull(); + expect(decodeDescriptorSnapshotContent("AB==")).toBeNull(); + expect( + applyDescriptorSnapshotActions( + { canonicalPath: root, identity }, + { root: identity, directories: {}, files: [] }, + [], + ), + ).toBe(true); + }); + it("fails closed when sanitized output cannot be installed", () => { const configPath = path.join(makeRoot(), "openclaw.json"); const original = JSON.stringify({ apiKey: "sk-secret-value" }); diff --git a/nemoclaw/src/security/snapshot-sanitizer.test.ts b/nemoclaw/src/security/snapshot-sanitizer.test.ts index 00a226a2ba9..0366e7bf6c1 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.test.ts @@ -138,11 +138,30 @@ describe("migration snapshot sanitizer", () => { const root = makeRoot(); writeFileSync(path.join(root, "empty.yaml"), ""); writeFileSync(path.join(root, "comments.yaml"), "# retained context\n"); + writeFileSync(path.join(root, "notes.txt"), "retained context\n"); sanitizeMigrationDirectory(root); expect(readFileSync(path.join(root, "empty.yaml"), "utf-8")).toBe(""); expect(readFileSync(path.join(root, "comments.yaml"), "utf-8")).toBe("# retained context\n"); + expect(readFileSync(path.join(root, "notes.txt"), "utf-8")).toBe("retained context\n"); + }); + + it("handles required-config path, presence, format, and stable-content boundaries", () => { + const root = makeRoot(); + expect(sanitizeOpenClawConfigFile(".")).toBe(false); + expect(sanitizeOpenClawConfigFile(path.join(root, "missing", "openclaw.json"))).toBe(false); + expect(sanitizeOpenClawConfigFile(path.join(root, "openclaw.json"))).toBe(false); + + const textPath = path.join(root, "openclaw.txt"); + writeFileSync(textPath, "not a supported config format\n"); + expect(sanitizeOpenClawConfigFile(textPath)).toBe(false); + + const safePath = path.join(root, "openclaw.json"); + const safeContent = JSON.stringify({ model: "keep-me" }, null, 2); + writeFileSync(safePath, safeContent); + expect(sanitizeOpenClawConfigFile(safePath)).toBe(true); + expect(readFileSync(safePath, "utf-8")).toBe(safeContent); }); it.runIf(process.platform !== "win32")( From d59a0e3b4f33b15f20b0fe6ec0d3862fe6118251 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 15:49:00 -0700 Subject: [PATCH 18/19] fix(security): pin snapshot sanitizer interpreter Signed-off-by: Apurv Kumaria --- docs/get-started/prerequisites.mdx | 5 +- .../snapshot-sanitizer-failure.test.ts | 87 +++++++++++++++---- .../src/security/snapshot-sanitizer.test.ts | 18 ++-- .../shared/snapshot-sanitizer-boundary.cts | 78 +++++++++++++++-- .../state/sandbox-backup-sanitization.test.ts | 11 ++- 5 files changed, 162 insertions(+), 37 deletions(-) diff --git a/docs/get-started/prerequisites.mdx b/docs/get-started/prerequisites.mdx index 1a071cb5aac..e9db60e9caf 100644 --- a/docs/get-started/prerequisites.mdx +++ b/docs/get-started/prerequisites.mdx @@ -31,7 +31,7 @@ If you cannot add memory, configure at least 8 GB of swap to work around the iss |------------|----------------------------------| | Node.js | 22.19 or later | | npm | 10 or later | -| Python | `python3` with POSIX descriptor-relative filesystem support | +| Python | Python 3 at a trusted system location, with POSIX descriptor-relative filesystem support | | Docker | Docker Engine, Docker Desktop, or Colima on a tested platform | | Platform | Refer to [Platforms](#platforms) below | @@ -42,6 +42,9 @@ In a non-interactive run, the installer can reactivate the group through `sg doc If that path is unavailable or does not restore Docker access, the installer exits with `newgrp docker` guidance before it starts onboarding. NemoClaw uses an isolated `python3` helper for descriptor-relative migration snapshot sanitization and deletion. +NemoClaw does not resolve this credential-bearing helper through the host `PATH`. +It accepts a verified executable at `/usr/bin/python3`, `/usr/local/bin/python3`, `/opt/homebrew/bin/python3`, `/opt/local/bin/python3`, or beside the Node.js executable. +The sanitizer fails closed before it reads snapshot content if no candidate passes its ownership, permission, and executable checks. Supported Linux, macOS, and WSL environments provide the required POSIX filesystem operations. Native Windows is not a supported execution path; use WSL. diff --git a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts index 5f52794a5a7..11dc87d3a6d 100644 --- a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts @@ -1,7 +1,6 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { spawnSync } from "node:child_process"; import { chmodSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; @@ -10,8 +9,10 @@ import { applyDescriptorSnapshotActions, decodeDescriptorSnapshotContent, inspectDescriptorSnapshotRoot, + resolveTrustedSnapshotSanitizerPythonPath, type SnapshotFileIdentity, scanDescriptorSnapshot, + setSnapshotSanitizerPythonPathForTest, } from "../shared/snapshot-sanitizer-boundary.cjs"; import { sanitizeMigrationDirectory, sanitizeOpenClawConfigFile } from "./snapshot-sanitizer.js"; @@ -32,11 +33,18 @@ function writePythonWrapper(lines: readonly string[]): string { const wrapper = path.join(wrapperRoot, "python3"); writeFileSync(wrapper, ["#!/bin/sh", ...lines].join("\n")); chmodSync(wrapper, 0o755); - vi.stubEnv("PATH", `${wrapperRoot}:${process.env.PATH ?? ""}`); + setSnapshotSanitizerPythonPathForTest(wrapper); return wrapper; } +function requireTrustedPython(): string { + const python = resolveTrustedSnapshotSanitizerPythonPath(); + if (python === null) throw new Error("A trusted Python 3 interpreter is required for this test"); + return python; +} + afterEach(() => { + setSnapshotSanitizerPythonPathForTest(undefined); vi.unstubAllEnvs(); for (const root of roots.splice(0)) rmSync(root, { force: true, recursive: true }); }); @@ -56,12 +64,67 @@ describe("migration snapshot sanitizer fallbacks", () => { const configPath = path.join(makeRoot(), "openclaw.json"); const original = JSON.stringify({ apiKey: "sk-secret-value" }); writeFileSync(configPath, original); - vi.stubEnv("PATH", makeRoot()); + setSnapshotSanitizerPythonPathForTest(null); expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); expect(readFileSync(configPath, "utf-8")).toBe(original); }); + it("fails closed when the descriptor apply helper is unavailable", () => { + const root = { canonicalPath: makeRoot(), identity }; + setSnapshotSanitizerPythonPathForTest(null); + + expect( + applyDescriptorSnapshotActions(root, { root: identity, directories: {}, files: [] }, [ + { kind: "remove", path: "config.json", metadata: identity }, + ]), + ).toBe(false); + }); + + it("accepts only absolute helper substitutions under Vitest", () => { + expect(() => setSnapshotSanitizerPythonPathForTest("python3")).toThrow( + /test Python path must be absolute/u, + ); + + const originalVitest = process.env.VITEST; + try { + process.env.VITEST = "false"; + expect(() => setSnapshotSanitizerPythonPathForTest(null)).toThrow( + /only available under Vitest/u, + ); + } finally { + process.env.VITEST = originalVitest; + } + }); + + it.runIf(process.platform !== "win32")( + "ignores a PATH-preceding helper before it can read unsanitized credentials", + () => { + const root = makeRoot(); + const configPath = path.join(root, "openclaw.json"); + const attackerRoot = makeRoot(); + const stolen = path.join(attackerRoot, "stolen-config"); + const wrapper = path.join(attackerRoot, "python3"); + writeFileSync(configPath, JSON.stringify({ apiKey: "sk-secret-value" })); + writeFileSync( + wrapper, + [ + "#!/bin/sh", + 'if [ "${4-}" = scan-file ]; then', + ` cp "$5/$7" ${shellQuote(stolen)}`, + "fi", + "exit 1", + ].join("\n"), + ); + chmodSync(wrapper, 0o755); + vi.stubEnv("PATH", `${attackerRoot}:${process.env.PATH ?? ""}`); + + expect(sanitizeOpenClawConfigFile(configPath)).toBe(true); + expect(() => readFileSync(stolen)).toThrow(); + expect(readFileSync(configPath, "utf-8")).not.toContain("sk-secret-value"); + }, + ); + it("rejects invalid output from the descriptor helper", () => { const configPath = path.join(makeRoot(), "openclaw.json"); const original = JSON.stringify({ apiKey: "sk-secret-value" }); @@ -124,15 +187,10 @@ describe("migration snapshot sanitizer fallbacks", () => { const configPath = path.join(makeRoot(), "openclaw.json"); const original = JSON.stringify({ apiKey: "sk-secret-value" }); writeFileSync(configPath, original); - const python = spawnSync( - "python3", - ["-I", "-c", "import os, sys; print(os.path.realpath(sys.executable))"], - { encoding: "utf-8" }, - ); - expect(python.status, python.stderr).toBe(0); + const python = requireTrustedPython(); writePythonWrapper([ 'if [ "${4-}" = apply ]; then exit 1; fi', - `exec ${shellQuote(python.stdout.trim())} "$@"`, + `exec ${shellQuote(python)} "$@"`, ]); expect(sanitizeOpenClawConfigFile(configPath)).toBe(false); @@ -166,15 +224,10 @@ describe("migration snapshot sanitizer fallbacks", () => { const movedRoot = `${root}-moved`; roots.push(movedRoot); writeFileSync(path.join(root, "config.json"), JSON.stringify({ token: "raw" })); - const python = spawnSync( - "python3", - ["-I", "-c", "import os, sys; print(os.path.realpath(sys.executable))"], - { encoding: "utf-8" }, - ); - expect(python.status, python.stderr).toBe(0); + const python = requireTrustedPython(); writePythonWrapper([ `if [ "\${4-}" = scan-tree ]; then mv ${shellQuote(root)} ${shellQuote(movedRoot)}; fi`, - `exec ${shellQuote(python.stdout.trim())} "$@"`, + `exec ${shellQuote(python)} "$@"`, ]); expect(() => sanitizeMigrationDirectory(root)).toThrow( diff --git a/nemoclaw/src/security/snapshot-sanitizer.test.ts b/nemoclaw/src/security/snapshot-sanitizer.test.ts index 0366e7bf6c1..8b87b14da3d 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.test.ts @@ -1,7 +1,6 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { spawnSync } from "node:child_process"; import { chmodSync, mkdirSync, @@ -14,6 +13,10 @@ import { import { tmpdir } from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { + resolveTrustedSnapshotSanitizerPythonPath, + setSnapshotSanitizerPythonPathForTest, +} from "../shared/snapshot-sanitizer-boundary.cjs"; import { sanitizeMigrationDirectory, sanitizeOpenClawConfigFile } from "./snapshot-sanitizer.js"; const temporaryRoots: string[] = []; @@ -25,6 +28,7 @@ function makeRoot(): string { } afterEach(() => { + setSnapshotSanitizerPythonPathForTest(undefined); vi.unstubAllEnvs(); for (const root of temporaryRoots.splice(0)) rmSync(root, { force: true, recursive: true }); }); @@ -182,12 +186,8 @@ describe("migration snapshot sanitizer", () => { ); writeFileSync(outsideConfig, JSON.stringify({ apiKey: "outside-must-not-change" })); - const python = spawnSync( - "python3", - ["-I", "-c", "import os, sys; print(os.path.realpath(sys.executable))"], - { encoding: "utf-8" }, - ); - expect(python.status, python.stderr).toBe(0); + const python = resolveTrustedSnapshotSanitizerPythonPath(); + if (python === null) throw new Error("A trusted Python 3 interpreter is required"); writeFileSync( wrapper, [ @@ -197,11 +197,11 @@ describe("migration snapshot sanitizer", () => { ` ln -s ${shellQuote(outside)} ${shellQuote(nested)}`, ` : > ${shellQuote(marker)}`, "fi", - `exec ${shellQuote(python.stdout.trim())} \"$@\"`, + `exec ${shellQuote(python)} \"$@\"`, ].join("\n"), ); chmodSync(wrapper, 0o755); - vi.stubEnv("PATH", `${wrapperRoot}:${process.env.PATH ?? ""}`); + setSnapshotSanitizerPythonPathForTest(wrapper); expect(() => sanitizeMigrationDirectory(root)).toThrow( /Failed to sanitize migration artifacts safely/u, diff --git a/nemoclaw/src/shared/snapshot-sanitizer-boundary.cts b/nemoclaw/src/shared/snapshot-sanitizer-boundary.cts index ce9a0e80025..8adfe1e41b1 100644 --- a/nemoclaw/src/shared/snapshot-sanitizer-boundary.cts +++ b/nemoclaw/src/shared/snapshot-sanitizer-boundary.cts @@ -2,14 +2,74 @@ // SPDX-License-Identifier: Apache-2.0 import { spawnSync } from "node:child_process"; -import { lstatSync, realpathSync } from "node:fs"; +import { accessSync, constants, lstatSync, realpathSync, statSync } from "node:fs"; import path from "node:path"; const HELPER_TIMEOUT_MS = 60_000; const HELPER_MAX_BUFFER_BYTES = 48 * 1024 * 1024; +const TRUSTED_PYTHON_LOCATIONS = [ + "/usr/bin/python3", + "/usr/local/bin/python3", + "/opt/homebrew/bin/python3", + "/opt/local/bin/python3", +] as const; +let snapshotSanitizerPythonPathForTest: string | null | undefined; + +function isTrustedAbsoluteExecutable(candidate: string): string | null { + try { + const canonical = realpathSync(candidate); + const currentUid = typeof process.getuid === "function" ? process.getuid() : null; + let inspected = canonical; + while (true) { + const metadata = statSync(inspected); + if ((metadata.mode & 0o022) !== 0) return null; + if (currentUid !== null && metadata.uid !== 0 && metadata.uid !== currentUid) return null; + const parent = path.dirname(inspected); + if (parent === inspected) break; + inspected = parent; + } + const executable = statSync(canonical); + if (!executable.isFile()) return null; + accessSync(canonical, constants.R_OK | constants.X_OK); + return canonical; + } catch { + return null; + } +} -function snapshotSanitizerEnvironment(): Record { - return typeof process.env.PATH === "string" ? { PATH: process.env.PATH } : {}; +/** Resolve a verified interpreter without consulting attacker-controlled PATH entries. */ +export function resolveTrustedSnapshotSanitizerPythonPath(): string | null { + const candidates: string[] = [...TRUSTED_PYTHON_LOCATIONS]; + try { + candidates.push(path.join(path.dirname(realpathSync(process.execPath)), "python3")); + } catch { + // The fixed system locations remain authoritative when Node cannot be canonicalized. + } + for (const candidate of new Set(candidates)) { + const trusted = isTrustedAbsoluteExecutable(candidate); + if (trusted !== null) return trusted; + } + return null; +} + +/** @visibleForTesting Install an explicit helper substitute without weakening production lookup. */ +export function setSnapshotSanitizerPythonPathForTest( + pythonPath: string | null | undefined, +): void { + if (process.env.VITEST !== "true") { + throw new Error("Snapshot sanitizer Python substitution is only available under Vitest"); + } + if (typeof pythonPath === "string" && !path.isAbsolute(pythonPath)) { + throw new Error("Snapshot sanitizer test Python path must be absolute"); + } + snapshotSanitizerPythonPathForTest = pythonPath; +} + +function snapshotSanitizerPythonPath(): string | null { + if (process.env.VITEST === "true" && snapshotSanitizerPythonPathForTest !== undefined) { + return snapshotSanitizerPythonPathForTest; + } + return resolveTrustedSnapshotSanitizerPythonPath(); } export interface SnapshotFileIdentity { @@ -567,8 +627,10 @@ export function scanDescriptorSnapshot( targetName?: string, ): DescriptorSnapshotScan | null { const mode = targetName === undefined ? "scan-tree" : "scan-file"; + const pythonPath = snapshotSanitizerPythonPath(); + if (pythonPath === null) return null; const result = spawnSync( - "python3", + pythonPath, [ "-I", "-c", @@ -581,7 +643,7 @@ export function scanDescriptorSnapshot( ], { encoding: "utf-8", - env: snapshotSanitizerEnvironment(), + env: {}, maxBuffer: HELPER_MAX_BUFFER_BYTES, timeout: HELPER_TIMEOUT_MS, }, @@ -597,12 +659,14 @@ export function applyDescriptorSnapshotActions( actions: readonly SnapshotSanitizationAction[], ): boolean { if (actions.length === 0) return true; + const pythonPath = snapshotSanitizerPythonPath(); + if (pythonPath === null) return false; const result = spawnSync( - "python3", + pythonPath, ["-I", "-c", SNAPSHOT_SANITIZER_PYTHON, "apply", root.canonicalPath], { encoding: "utf-8", - env: snapshotSanitizerEnvironment(), + env: {}, input: JSON.stringify({ root: scan.root, directories: scan.directories, actions }), maxBuffer: HELPER_MAX_BUFFER_BYTES, timeout: HELPER_TIMEOUT_MS, diff --git a/src/lib/state/sandbox-backup-sanitization.test.ts b/src/lib/state/sandbox-backup-sanitization.test.ts index 62c626883a1..0651b41d082 100644 --- a/src/lib/state/sandbox-backup-sanitization.test.ts +++ b/src/lib/state/sandbox-backup-sanitization.test.ts @@ -1,7 +1,6 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { execFileSync } from "node:child_process"; import { chmodSync, existsSync, @@ -16,6 +15,10 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; +import { + resolveTrustedSnapshotSanitizerPythonPath, + setSnapshotSanitizerPythonPathForTest, +} from "../../../nemoclaw/dist/shared/snapshot-sanitizer-boundary.cjs"; import { sanitizeBackupDirectory } from "./sandbox.js"; const testDirectories: string[] = []; @@ -28,6 +31,7 @@ function createBackup(): string { } afterEach(() => { + setSnapshotSanitizerPythonPathForTest(undefined); vi.unstubAllEnvs(); for (const testDirectory of testDirectories.splice(0)) { rmSync(testDirectory, { recursive: true, force: true }); @@ -125,7 +129,8 @@ describe("rebuild backup credential sanitization", () => { const outsideContents = '{"apiKey":"sk-outside-secret"}'; writeFileSync(outsideConfigPath, outsideContents); - const realPython = execFileSync("which", ["python3"], { encoding: "utf-8" }).trim(); + const realPython = resolveTrustedSnapshotSanitizerPythonPath(); + if (realPython === null) throw new Error("A trusted Python 3 interpreter is required"); const shellQuote = (value: string): string => `'${value.replaceAll("'", `'\\''`)}'`; const pythonWrapper = join(wrapperPath, "python3"); writeFileSync( @@ -133,7 +138,7 @@ describe("rebuild backup credential sanitization", () => { `#!/bin/sh\nif [ "$4" = "apply" ]; then\n mv ${shellQuote(nestedPath)} ${shellQuote(movedPath)}\n ln -s ${shellQuote(outsidePath)} ${shellQuote(nestedPath)}\nfi\nexec ${shellQuote(realPython)} "$@"\n`, ); chmodSync(pythonWrapper, 0o755); - vi.stubEnv("PATH", `${wrapperPath}:${process.env.PATH ?? ""}`); + setSnapshotSanitizerPythonPathForTest(pythonWrapper); expect(() => sanitizeBackupDirectory(backupPath)).toThrow( "Credential sanitization failed; removed the incomplete backup", From 5b8dd525f6297ca6fe1f3b669801c464824e5704 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Fri, 31 Jul 2026 16:13:25 -0700 Subject: [PATCH 19/19] test(security): keep sanitizer fixtures linear Signed-off-by: Apurv Kumaria --- nemoclaw/src/security/snapshot-sanitizer-failure.test.ts | 4 ++-- nemoclaw/src/security/snapshot-sanitizer.test.ts | 4 ++-- src/lib/state/sandbox-backup-sanitization.test.ts | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts index 11dc87d3a6d..95dc6c747ea 100644 --- a/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer-failure.test.ts @@ -39,8 +39,8 @@ function writePythonWrapper(lines: readonly string[]): string { function requireTrustedPython(): string { const python = resolveTrustedSnapshotSanitizerPythonPath(); - if (python === null) throw new Error("A trusted Python 3 interpreter is required for this test"); - return python; + expect(python).toEqual(expect.any(String)); + return python as string; } afterEach(() => { diff --git a/nemoclaw/src/security/snapshot-sanitizer.test.ts b/nemoclaw/src/security/snapshot-sanitizer.test.ts index 8b87b14da3d..ae7c7d3e40e 100644 --- a/nemoclaw/src/security/snapshot-sanitizer.test.ts +++ b/nemoclaw/src/security/snapshot-sanitizer.test.ts @@ -187,7 +187,7 @@ describe("migration snapshot sanitizer", () => { writeFileSync(outsideConfig, JSON.stringify({ apiKey: "outside-must-not-change" })); const python = resolveTrustedSnapshotSanitizerPythonPath(); - if (python === null) throw new Error("A trusted Python 3 interpreter is required"); + expect(python).toEqual(expect.any(String)); writeFileSync( wrapper, [ @@ -197,7 +197,7 @@ describe("migration snapshot sanitizer", () => { ` ln -s ${shellQuote(outside)} ${shellQuote(nested)}`, ` : > ${shellQuote(marker)}`, "fi", - `exec ${shellQuote(python)} \"$@\"`, + `exec ${shellQuote(python as string)} \"$@\"`, ].join("\n"), ); chmodSync(wrapper, 0o755); diff --git a/src/lib/state/sandbox-backup-sanitization.test.ts b/src/lib/state/sandbox-backup-sanitization.test.ts index 0651b41d082..10cc63dbd7b 100644 --- a/src/lib/state/sandbox-backup-sanitization.test.ts +++ b/src/lib/state/sandbox-backup-sanitization.test.ts @@ -130,12 +130,12 @@ describe("rebuild backup credential sanitization", () => { writeFileSync(outsideConfigPath, outsideContents); const realPython = resolveTrustedSnapshotSanitizerPythonPath(); - if (realPython === null) throw new Error("A trusted Python 3 interpreter is required"); + expect(realPython).toEqual(expect.any(String)); const shellQuote = (value: string): string => `'${value.replaceAll("'", `'\\''`)}'`; const pythonWrapper = join(wrapperPath, "python3"); writeFileSync( pythonWrapper, - `#!/bin/sh\nif [ "$4" = "apply" ]; then\n mv ${shellQuote(nestedPath)} ${shellQuote(movedPath)}\n ln -s ${shellQuote(outsidePath)} ${shellQuote(nestedPath)}\nfi\nexec ${shellQuote(realPython)} "$@"\n`, + `#!/bin/sh\nif [ "$4" = "apply" ]; then\n mv ${shellQuote(nestedPath)} ${shellQuote(movedPath)}\n ln -s ${shellQuote(outsidePath)} ${shellQuote(nestedPath)}\nfi\nexec ${shellQuote(realPython as string)} "$@"\n`, ); chmodSync(pythonWrapper, 0o755); setSnapshotSanitizerPythonPathForTest(pythonWrapper);