diff --git a/CHANGELOG.md b/CHANGELOG.md index 348e55682f9..e54aceb118f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ Docs: https://docs.openclaw.ai - Sessions/Store: canonicalize inbound mixed-case session keys for metadata and route updates, and migrate legacy case-variant entries to a single lowercase key to prevent duplicate sessions and missing TUI/WebUI history. (#9561) Thanks @hillghost86. - Security/CI: add pre-commit security hook coverage for private-key detection and production dependency auditing, and enforce those checks in CI alongside baseline secret scanning. Thanks @vincentkoc. - Skills/Python: harden skill script packaging and validation edge cases (self-including `.skill` outputs, CRLF frontmatter parsing, strict `--days` validation, and safer image file loading), with expanded Python regression coverage. Thanks @vincentkoc. +- Config/Write: apply `unsetPaths` with immutable path-copy updates so config writes never mutate caller-provided objects, and harden `openclaw config get/set/unset` path traversal by rejecting prototype-key segments and inherited-property traversal. (#24134) thanks @frankekn. ## 2026.2.23 diff --git a/src/cli/config-cli.test.ts b/src/cli/config-cli.test.ts index 392be2ad0cc..a1735449736 100644 --- a/src/cli/config-cli.test.ts +++ b/src/cli/config-cli.test.ts @@ -204,6 +204,35 @@ describe("config cli", () => { }); }); + describe("path hardening", () => { + it("rejects blocked prototype-key segments for config get", async () => { + await expect(runConfigCommand(["config", "get", "gateway.__proto__.token"])).rejects.toThrow( + "Invalid path segment: __proto__", + ); + + expect(mockReadConfigFileSnapshot).not.toHaveBeenCalled(); + expect(mockWriteConfigFile).not.toHaveBeenCalled(); + }); + + it("rejects blocked prototype-key segments for config set", async () => { + await expect( + runConfigCommand(["config", "set", "tools.constructor.profile", '"sandbox"']), + ).rejects.toThrow("Invalid path segment: constructor"); + + expect(mockReadConfigFileSnapshot).not.toHaveBeenCalled(); + expect(mockWriteConfigFile).not.toHaveBeenCalled(); + }); + + it("rejects blocked prototype-key segments for config unset", async () => { + await expect( + runConfigCommand(["config", "unset", "channels.prototype.enabled"]), + ).rejects.toThrow("Invalid path segment: prototype"); + + expect(mockReadConfigFileSnapshot).not.toHaveBeenCalled(); + expect(mockWriteConfigFile).not.toHaveBeenCalled(); + }); + }); + describe("config unset - issue #6070", () => { it("preserves existing config keys when unsetting a value", async () => { const resolved: OpenClawConfig = { diff --git a/src/cli/config-cli.ts b/src/cli/config-cli.ts index c9fb6e33520..3893aa1d020 100644 --- a/src/cli/config-cli.ts +++ b/src/cli/config-cli.ts @@ -1,6 +1,7 @@ import type { Command } from "commander"; import JSON5 from "json5"; import { readConfigFileSnapshot, writeConfigFile } from "../config/config.js"; +import { isBlockedObjectKey } from "../config/prototype-keys.js"; import { redactConfigObject } from "../config/redact-snapshot.js"; import { danger, info } from "../globals.js"; import type { RuntimeEnv } from "../runtime.js"; @@ -88,6 +89,18 @@ function parseValue(raw: string, opts: ConfigSetParseOpts): unknown { } } +function hasOwnPathKey(value: Record, key: string): boolean { + return Object.prototype.hasOwnProperty.call(value, key); +} + +function validatePathSegments(path: PathSegment[]): void { + for (const segment of path) { + if (!isIndexSegment(segment) && isBlockedObjectKey(segment)) { + throw new Error(`Invalid path segment: ${segment}`); + } + } +} + function getAtPath(root: unknown, path: PathSegment[]): { found: boolean; value?: unknown } { let current: unknown = root; for (const segment of path) { @@ -106,7 +119,7 @@ function getAtPath(root: unknown, path: PathSegment[]): { found: boolean; value? continue; } const record = current as Record; - if (!(segment in record)) { + if (!hasOwnPathKey(record, segment)) { return { found: false }; } current = record[segment]; @@ -136,7 +149,7 @@ function setAtPath(root: Record, path: PathSegment[], value: un throw new Error(`Cannot traverse into "${segment}" (not an object)`); } const record = current as Record; - const existing = record[segment]; + const existing = hasOwnPathKey(record, segment) ? record[segment] : undefined; if (!existing || typeof existing !== "object") { record[segment] = nextIsIndex ? [] : {}; } @@ -177,7 +190,7 @@ function unsetAtPath(root: Record, path: PathSegment[]): boolea continue; } const record = current as Record; - if (!(segment in record)) { + if (!hasOwnPathKey(record, segment)) { return false; } current = record[segment]; @@ -199,7 +212,7 @@ function unsetAtPath(root: Record, path: PathSegment[]): boolea return false; } const record = current as Record; - if (!(last in record)) { + if (!hasOwnPathKey(record, last)) { return false; } delete record[last]; @@ -225,6 +238,7 @@ function parseRequiredPath(path: string): PathSegment[] { if (parsedPath.length === 0) { throw new Error("Path is empty."); } + validatePathSegments(parsedPath); return parsedPath; } @@ -322,10 +336,7 @@ export function registerConfigCli(program: Command) { .option("--json", "Legacy alias for --strict-json", false) .action(async (path: string, value: string, opts) => { try { - const parsedPath = parsePath(path); - if (parsedPath.length === 0) { - throw new Error("Path is empty."); - } + const parsedPath = parseRequiredPath(path); const parsedValue = parseValue(value, { strictJson: Boolean(opts.strictJson || opts.json), }); diff --git a/src/config/io.ts b/src/config/io.ts index fba3be8d63f..574b52ee293 100644 --- a/src/config/io.ts +++ b/src/config/io.ts @@ -39,6 +39,7 @@ import { applyMergePatch } from "./merge-patch.js"; import { normalizeExecSafeBinProfilesInConfig } from "./normalize-exec-safe-bin.js"; import { normalizeConfigPaths } from "./normalize-paths.js"; import { resolveConfigPath, resolveDefaultConfigCandidates, resolveStateDir } from "./paths.js"; +import { isBlockedObjectKey } from "./prototype-keys.js"; import { applyConfigOverrides } from "./runtime-overrides.js"; import type { OpenClawConfig, ConfigFileSnapshot, LegacyConfigIssue } from "./types.js"; import { @@ -143,76 +144,104 @@ function isWritePlainObject(value: unknown): value is Record { return Boolean(value) && typeof value === "object" && !Array.isArray(value); } -function unsetPathForWrite(root: Record, pathSegments: string[]): boolean { - if (pathSegments.length === 0) { - return false; +function hasOwnObjectKey(value: Record, key: string): boolean { + return Object.prototype.hasOwnProperty.call(value, key); +} + +const WRITE_PRUNED_OBJECT = Symbol("write-pruned-object"); + +type UnsetPathWriteResult = { + changed: boolean; + value: unknown; +}; + +function unsetPathForWriteAt( + value: unknown, + pathSegments: string[], + depth: number, +): UnsetPathWriteResult { + if (depth >= pathSegments.length) { + return { changed: false, value }; } + const segment = pathSegments[depth]; + const isLeaf = depth === pathSegments.length - 1; - const traversal: Array<{ container: unknown; key: string | number }> = []; - let cursor: unknown = root; - - for (let i = 0; i < pathSegments.length - 1; i += 1) { - const segment = pathSegments[i]; - if (Array.isArray(cursor)) { - if (!isNumericPathSegment(segment)) { - return false; - } - const index = Number.parseInt(segment, 10); - if (!Number.isFinite(index) || index < 0 || index >= cursor.length) { - return false; - } - traversal.push({ container: cursor, key: index }); - cursor = cursor[index]; - continue; + if (Array.isArray(value)) { + if (!isNumericPathSegment(segment)) { + return { changed: false, value }; } - if (!isWritePlainObject(cursor) || !(segment in cursor)) { - return false; + const index = Number.parseInt(segment, 10); + if (!Number.isFinite(index) || index < 0 || index >= value.length) { + return { changed: false, value }; } - traversal.push({ container: cursor, key: segment }); - cursor = cursor[segment]; - } - - const leaf = pathSegments[pathSegments.length - 1]; - if (Array.isArray(cursor)) { - if (!isNumericPathSegment(leaf)) { - return false; + if (isLeaf) { + const next = value.slice(); + next.splice(index, 1); + return { changed: true, value: next }; } - const index = Number.parseInt(leaf, 10); - if (!Number.isFinite(index) || index < 0 || index >= cursor.length) { - return false; + const child = unsetPathForWriteAt(value[index], pathSegments, depth + 1); + if (!child.changed) { + return { changed: false, value }; } - cursor.splice(index, 1); - } else { - if (!isWritePlainObject(cursor) || !(leaf in cursor)) { - return false; - } - delete cursor[leaf]; - } - - // Prune now-empty object branches after unsetting to avoid dead config scaffolding. - for (let i = traversal.length - 1; i >= 0; i -= 1) { - const { container, key } = traversal[i]; - let child: unknown; - if (Array.isArray(container)) { - child = typeof key === "number" ? container[key] : undefined; - } else if (isWritePlainObject(container)) { - child = container[String(key)]; + const next = value.slice(); + if (child.value === WRITE_PRUNED_OBJECT) { + next.splice(index, 1); } else { - break; - } - if (!isWritePlainObject(child) || Object.keys(child).length > 0) { - break; - } - if (Array.isArray(container) && typeof key === "number") { - if (key >= 0 && key < container.length) { - container.splice(key, 1); - } - } else if (isWritePlainObject(container)) { - delete container[String(key)]; + next[index] = child.value; } + return { changed: true, value: next }; } - return true; + if ( + isBlockedObjectKey(segment) || + !isWritePlainObject(value) || + !hasOwnObjectKey(value, segment) + ) { + return { changed: false, value }; + } + if (isLeaf) { + const next: Record = { ...value }; + delete next[segment]; + return { + changed: true, + value: Object.keys(next).length === 0 ? WRITE_PRUNED_OBJECT : next, + }; + } + + const child = unsetPathForWriteAt(value[segment], pathSegments, depth + 1); + if (!child.changed) { + return { changed: false, value }; + } + const next: Record = { ...value }; + if (child.value === WRITE_PRUNED_OBJECT) { + delete next[segment]; + } else { + next[segment] = child.value; + } + return { + changed: true, + value: Object.keys(next).length === 0 ? WRITE_PRUNED_OBJECT : next, + }; +} + +function unsetPathForWrite( + root: OpenClawConfig, + pathSegments: string[], +): { changed: boolean; next: OpenClawConfig } { + if (pathSegments.length === 0) { + return { changed: false, next: root }; + } + const result = unsetPathForWriteAt(root, pathSegments, 0); + if (!result.changed) { + return { changed: false, next: root }; + } + if (result.value === WRITE_PRUNED_OBJECT) { + return { changed: true, next: {} }; + } + if (isWritePlainObject(result.value)) { + return { changed: true, next: coerceConfig(result.value) }; + } + return { changed: false, next: root }; } export function resolveConfigSnapshotHash(snapshot: { @@ -1011,16 +1040,20 @@ export function createConfigIO(overrides: ConfigIoDeps = {}) { const dir = path.dirname(configPath); await deps.fs.promises.mkdir(dir, { recursive: true, mode: 0o700 }); - const outputConfig = + const outputConfigBase = envRefMap && changedPaths ? (restoreEnvRefsFromMap(cfgToWrite, "", envRefMap, changedPaths) as OpenClawConfig) : cfgToWrite; + let outputConfig = outputConfigBase; if (options.unsetPaths?.length) { for (const unsetPath of options.unsetPaths) { if (!Array.isArray(unsetPath) || unsetPath.length === 0) { continue; } - unsetPathForWrite(outputConfig as Record, unsetPath); + const unsetResult = unsetPathForWrite(outputConfig, unsetPath); + if (unsetResult.changed) { + outputConfig = unsetResult.next; + } } } // Do NOT apply runtime defaults when writing — user config should only contain diff --git a/src/config/io.write-config.test.ts b/src/config/io.write-config.test.ts index 110d81ef61e..17f1951de33 100644 --- a/src/config/io.write-config.test.ts +++ b/src/config/io.write-config.test.ts @@ -124,6 +124,130 @@ describe("config io write", () => { }); }); + it("does not mutate caller config when unsetPaths is applied on first write", async () => { + await withTempHome("openclaw-config-io-", async (home) => { + const configPath = path.join(home, ".openclaw", "openclaw.json"); + const io = createConfigIO({ + env: {} as NodeJS.ProcessEnv, + homedir: () => home, + logger: silentLogger, + }); + + const input: Record = { + gateway: { mode: "local" }, + commands: { ownerDisplay: "hash" }, + }; + + await io.writeConfigFile(input, { unsetPaths: [["commands", "ownerDisplay"]] }); + + expect(input).toEqual({ + gateway: { mode: "local" }, + commands: { ownerDisplay: "hash" }, + }); + expect((input.commands as Record).ownerDisplay).toBe("hash"); + const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as { + commands?: Record; + }; + expect(persisted.commands ?? {}).not.toHaveProperty("ownerDisplay"); + }); + }); + + it("does not mutate caller config when unsetPaths is applied on existing files", async () => { + await withTempHome("openclaw-config-io-", async (home) => { + const { configPath, io, snapshot } = await writeConfigAndCreateIo({ + home, + initialConfig: { + gateway: { mode: "local" }, + commands: { ownerDisplay: "hash" }, + }, + }); + + const input = structuredClone(snapshot.config) as Record; + await io.writeConfigFile(input, { unsetPaths: [["commands", "ownerDisplay"]] }); + + expect((input.commands as Record).ownerDisplay).toBe("hash"); + const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as { + commands?: Record; + }; + expect(persisted.commands ?? {}).not.toHaveProperty("ownerDisplay"); + }); + }); + + it("keeps caller arrays immutable when unsetting array entries", async () => { + await withTempHome("openclaw-config-io-", async (home) => { + const { configPath, io, snapshot } = await writeConfigAndCreateIo({ + home, + initialConfig: { + gateway: { mode: "local" }, + tools: { alsoAllow: ["exec", "fetch", "read"] }, + }, + }); + + const input = structuredClone(snapshot.config) as Record; + await io.writeConfigFile(input, { unsetPaths: [["tools", "alsoAllow", "1"]] }); + + expect((input.tools as { alsoAllow: string[] }).alsoAllow).toEqual(["exec", "fetch", "read"]); + const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as { + tools?: { alsoAllow?: string[] }; + }; + expect(persisted.tools?.alsoAllow).toEqual(["exec", "read"]); + }); + }); + + it("treats missing unset paths as no-op without mutating caller config", async () => { + await withTempHome("openclaw-config-io-", async (home) => { + const { configPath, io } = await writeConfigAndCreateIo({ + home, + initialConfig: { + gateway: { mode: "local" }, + commands: { ownerDisplay: "hash" }, + }, + }); + + const input: Record = { + gateway: { mode: "local" }, + commands: { ownerDisplay: "hash" }, + }; + await io.writeConfigFile(input, { unsetPaths: [["commands", "missingKey"]] }); + + expect((input.commands as Record).ownerDisplay).toBe("hash"); + const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as { + commands?: Record; + }; + expect(persisted.commands?.ownerDisplay).toBe("hash"); + }); + }); + + it("ignores blocked prototype-key unset path segments", async () => { + await withTempHome("openclaw-config-io-", async (home) => { + const { configPath, io } = await writeConfigAndCreateIo({ + home, + initialConfig: { + gateway: { mode: "local" }, + commands: { ownerDisplay: "hash" }, + }, + }); + + const input: Record = { + gateway: { mode: "local" }, + commands: { ownerDisplay: "hash" }, + }; + await io.writeConfigFile(input, { + unsetPaths: [ + ["commands", "__proto__"], + ["commands", "constructor"], + ["commands", "prototype"], + ], + }); + + expect((input.commands as Record).ownerDisplay).toBe("hash"); + const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as { + commands?: Record; + }; + expect(persisted.commands?.ownerDisplay).toBe("hash"); + }); + }); + it("preserves env var references when writing", async () => { await withTempHome("openclaw-config-io-", async (home) => { const { configPath, io, snapshot } = await writeConfigAndCreateIo({