mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-12 07:20:45 +00:00
fix(config): keep write inputs immutable when using unsetPaths (#24134)
Merged via /review-pr -> /prepare-pr -> /merge-pr.
Prepared head SHA: 951f8480c3
Co-authored-by: frankekn <4488090+frankekn@users.noreply.github.com>
Co-authored-by: gumadeiras <5599352+gumadeiras@users.noreply.github.com>
Reviewed-by: @gumadeiras
This commit is contained in:
@@ -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
|
||||
|
||||
|
||||
@@ -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 = {
|
||||
|
||||
@@ -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<string, unknown>, 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<string, unknown>;
|
||||
if (!(segment in record)) {
|
||||
if (!hasOwnPathKey(record, segment)) {
|
||||
return { found: false };
|
||||
}
|
||||
current = record[segment];
|
||||
@@ -136,7 +149,7 @@ function setAtPath(root: Record<string, unknown>, path: PathSegment[], value: un
|
||||
throw new Error(`Cannot traverse into "${segment}" (not an object)`);
|
||||
}
|
||||
const record = current as Record<string, unknown>;
|
||||
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<string, unknown>, path: PathSegment[]): boolea
|
||||
continue;
|
||||
}
|
||||
const record = current as Record<string, unknown>;
|
||||
if (!(segment in record)) {
|
||||
if (!hasOwnPathKey(record, segment)) {
|
||||
return false;
|
||||
}
|
||||
current = record[segment];
|
||||
@@ -199,7 +212,7 @@ function unsetAtPath(root: Record<string, unknown>, path: PathSegment[]): boolea
|
||||
return false;
|
||||
}
|
||||
const record = current as Record<string, unknown>;
|
||||
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),
|
||||
});
|
||||
|
||||
157
src/config/io.ts
157
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<string, unknown> {
|
||||
return Boolean(value) && typeof value === "object" && !Array.isArray(value);
|
||||
}
|
||||
|
||||
function unsetPathForWrite(root: Record<string, unknown>, pathSegments: string[]): boolean {
|
||||
if (pathSegments.length === 0) {
|
||||
return false;
|
||||
function hasOwnObjectKey(value: Record<string, unknown>, 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<string, unknown> = { ...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<string, unknown> = { ...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<string, unknown>, unsetPath);
|
||||
const unsetResult = unsetPathForWrite(outputConfig, unsetPath);
|
||||
if (unsetResult.changed) {
|
||||
outputConfig = unsetResult.next;
|
||||
}
|
||||
}
|
||||
}
|
||||
// Do NOT apply runtime defaults when writing — user config should only contain
|
||||
|
||||
@@ -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<string, unknown> = {
|
||||
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<string, unknown>).ownerDisplay).toBe("hash");
|
||||
const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as {
|
||||
commands?: Record<string, unknown>;
|
||||
};
|
||||
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<string, unknown>;
|
||||
await io.writeConfigFile(input, { unsetPaths: [["commands", "ownerDisplay"]] });
|
||||
|
||||
expect((input.commands as Record<string, unknown>).ownerDisplay).toBe("hash");
|
||||
const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as {
|
||||
commands?: Record<string, unknown>;
|
||||
};
|
||||
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<string, unknown>;
|
||||
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<string, unknown> = {
|
||||
gateway: { mode: "local" },
|
||||
commands: { ownerDisplay: "hash" },
|
||||
};
|
||||
await io.writeConfigFile(input, { unsetPaths: [["commands", "missingKey"]] });
|
||||
|
||||
expect((input.commands as Record<string, unknown>).ownerDisplay).toBe("hash");
|
||||
const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as {
|
||||
commands?: Record<string, unknown>;
|
||||
};
|
||||
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<string, unknown> = {
|
||||
gateway: { mode: "local" },
|
||||
commands: { ownerDisplay: "hash" },
|
||||
};
|
||||
await io.writeConfigFile(input, {
|
||||
unsetPaths: [
|
||||
["commands", "__proto__"],
|
||||
["commands", "constructor"],
|
||||
["commands", "prototype"],
|
||||
],
|
||||
});
|
||||
|
||||
expect((input.commands as Record<string, unknown>).ownerDisplay).toBe("hash");
|
||||
const persisted = JSON.parse(await fs.readFile(configPath, "utf-8")) as {
|
||||
commands?: Record<string, unknown>;
|
||||
};
|
||||
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({
|
||||
|
||||
Reference in New Issue
Block a user