mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-25 12:31:15 +00:00
* fix(sdk): add prototype-pollution guard to migration config merge mergeMigrationConfigValue and writeMigrationConfigPath had no isBlockedObjectKey guard, allowing __proto__/constructor/prototype keys from imported config files to trigger prototype pollution. Add the same guard used by the sibling config-path writer (setConfigValueAtPath/parseConfigPath) to reject blocked object keys before they reach a bracket assignment. Fixes #103059 * fix(sdk): harden migration config patches --------- Co-authored-by: Peter Steinberger <steipete@gmail.com>
This commit is contained in:
@@ -187,6 +187,58 @@ describe("Hermes migration config mapping", () => {
|
||||
expect(config.skills?.entries?.["ship-it"]?.config?.mode).toBe("fast");
|
||||
});
|
||||
|
||||
it("drops prototype-bearing provider, MCP, and skill keys during apply", async () => {
|
||||
const root = await makeTempRoot();
|
||||
const source = path.join(root, "hermes");
|
||||
const workspaceDir = path.join(root, "workspace");
|
||||
const stateDir = path.join(root, "state");
|
||||
const config = {
|
||||
agents: { defaults: { workspace: workspaceDir } },
|
||||
} as OpenClawConfig;
|
||||
await writeFile(
|
||||
path.join(source, "config.yaml"),
|
||||
[
|
||||
"providers:",
|
||||
' "__proto__":',
|
||||
" base_url: https://untrusted.example/v1",
|
||||
" models: [untrusted-model]",
|
||||
"mcp_servers:",
|
||||
" constructor:",
|
||||
" command: untrusted-command",
|
||||
" safe-server:",
|
||||
" command: safe-command",
|
||||
"skills:",
|
||||
" config:",
|
||||
" prototype:",
|
||||
" mode: untrusted",
|
||||
" safe-skill:",
|
||||
" mode: safe",
|
||||
"",
|
||||
].join("\n"),
|
||||
);
|
||||
|
||||
const result = await buildHermesMigrationProvider().apply(
|
||||
makeContext({
|
||||
source,
|
||||
stateDir,
|
||||
workspaceDir,
|
||||
runtime: makeConfigRuntime(config),
|
||||
}),
|
||||
);
|
||||
|
||||
expect(result.summary.errors).toBe(0);
|
||||
const providers = config.models?.providers as Record<string, unknown>;
|
||||
const servers = config.mcp?.servers as Record<string, unknown>;
|
||||
const skills = config.skills?.entries as Record<string, unknown>;
|
||||
expect(Object.hasOwn(providers, "__proto__")).toBe(false);
|
||||
expect(Object.hasOwn(servers, "constructor")).toBe(false);
|
||||
expect(Object.hasOwn(skills, "prototype")).toBe(false);
|
||||
expect(Object.getPrototypeOf(providers)).toBe(Object.prototype);
|
||||
expect((servers["safe-server"] as { command?: string }).command).toBe("safe-command");
|
||||
expect((skills["safe-skill"] as { config?: { mode?: string } }).config?.mode).toBe("safe");
|
||||
expect(({} as { polluted?: boolean }).polluted).toBeUndefined();
|
||||
});
|
||||
|
||||
it("uses the provider runtime for CLI-applied config items", async () => {
|
||||
const root = await makeTempRoot();
|
||||
const source = path.join(root, "hermes");
|
||||
|
||||
78
src/plugin-sdk/migration-config.test.ts
Normal file
78
src/plugin-sdk/migration-config.test.ts
Normal file
@@ -0,0 +1,78 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import type { MigrationProviderContext } from "../plugins/types.js";
|
||||
import {
|
||||
applyMigrationConfigPatchItem,
|
||||
createMigrationConfigPatchItem,
|
||||
hasMigrationConfigPatchConflict,
|
||||
mergeMigrationConfigValue,
|
||||
writeMigrationConfigPath,
|
||||
} from "./migration.js";
|
||||
|
||||
describe("migration config patches", () => {
|
||||
it("drops blocked keys recursively from objects and arrays", () => {
|
||||
const patch = JSON.parse(
|
||||
'{"__proto__":{"polluted":true},"safe":{"prototype":{"polluted":true},"next":true},"list":[{"constructor":{"polluted":true},"kept":true}]}',
|
||||
);
|
||||
|
||||
const merged = mergeMigrationConfigValue({ safe: { keep: true } }, patch) as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
|
||||
expect(merged).toEqual({
|
||||
safe: { keep: true, next: true },
|
||||
list: [{ kept: true }],
|
||||
});
|
||||
expect(Object.getPrototypeOf(merged)).toBe(Object.prototype);
|
||||
expect(Object.getPrototypeOf(merged.safe as object)).toBe(Object.prototype);
|
||||
expect(({} as { polluted?: boolean }).polluted).toBeUndefined();
|
||||
});
|
||||
|
||||
it("rejects blocked path segments before mutating the target", () => {
|
||||
const config: Record<string, unknown> = {};
|
||||
|
||||
expect(() =>
|
||||
writeMigrationConfigPath(config, ["models", "__proto__", "polluted"], true),
|
||||
).toThrow("unsafe config patch path");
|
||||
expect(config).toEqual({});
|
||||
expect(({} as { polluted?: boolean }).polluted).toBeUndefined();
|
||||
});
|
||||
|
||||
it("reports unsafe migration items as errors without calling the config writer", async () => {
|
||||
const mutateConfigFile = vi.fn();
|
||||
const config: MigrationProviderContext["config"] = {};
|
||||
const ctx = {
|
||||
config,
|
||||
stateDir: "/tmp/openclaw-migration-test",
|
||||
logger: {},
|
||||
runtime: {
|
||||
config: {
|
||||
current: () => config,
|
||||
mutateConfigFile,
|
||||
},
|
||||
},
|
||||
} as unknown as MigrationProviderContext;
|
||||
const item = createMigrationConfigPatchItem({
|
||||
id: "config:unsafe",
|
||||
target: "models.__proto__.polluted",
|
||||
path: ["models", "__proto__", "polluted"],
|
||||
value: true,
|
||||
message: "unsafe patch",
|
||||
});
|
||||
|
||||
await expect(applyMigrationConfigPatchItem(ctx, item)).resolves.toEqual(
|
||||
expect.objectContaining({ status: "error", reason: "unsafe config patch path" }),
|
||||
);
|
||||
expect(mutateConfigFile).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("ignores blocked and inherited keys during conflict checks", () => {
|
||||
const patch = JSON.parse(
|
||||
'{"__proto__":{"polluted":true},"toString":{"command":"safe-own-value"}}',
|
||||
);
|
||||
|
||||
expect(
|
||||
hasMigrationConfigPatchConflict({ mcp: { servers: {} } }, ["mcp", "servers"], patch),
|
||||
).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -1,6 +1,7 @@
|
||||
// Shared migration-provider helpers for plan/apply item bookkeeping.
|
||||
|
||||
import { isRecord } from "../../packages/normalization-core/src/record-coerce.js";
|
||||
import { isBlockedObjectKey } from "../infra/prototype-keys.js";
|
||||
import type {
|
||||
MigrationDetection,
|
||||
MigrationItem,
|
||||
@@ -115,11 +116,40 @@ class MigrationConfigPatchConflictError extends Error {
|
||||
}
|
||||
}
|
||||
|
||||
const MIGRATION_REASON_UNSAFE_CONFIG_PATCH_PATH = "unsafe config patch path";
|
||||
|
||||
function isSafeMigrationConfigPath(path: readonly string[]): boolean {
|
||||
return (
|
||||
path.length > 0 && path.every((segment) => segment.length > 0 && !isBlockedObjectKey(segment))
|
||||
);
|
||||
}
|
||||
|
||||
function cloneMigrationConfigValue(value: unknown): unknown {
|
||||
if (Array.isArray(value)) {
|
||||
return value.map((entry) => cloneMigrationConfigValue(entry));
|
||||
}
|
||||
if (!isRecord(value)) {
|
||||
return structuredClone(value);
|
||||
}
|
||||
const next: Record<string, unknown> = {};
|
||||
for (const [key, entry] of Object.entries(value)) {
|
||||
// Migration patches come from external tools. Drop prototype-bearing keys
|
||||
// recursively before any value reaches a live config object.
|
||||
if (!isBlockedObjectKey(key)) {
|
||||
next[key] = cloneMigrationConfigValue(entry);
|
||||
}
|
||||
}
|
||||
return next;
|
||||
}
|
||||
|
||||
/** Reads a nested config value, returning undefined when a parent is not an object. */
|
||||
export function readMigrationConfigPath(
|
||||
root: Record<string, unknown>,
|
||||
path: readonly string[],
|
||||
): unknown {
|
||||
if (!isSafeMigrationConfigPath(path)) {
|
||||
return undefined;
|
||||
}
|
||||
let current: unknown = root;
|
||||
for (const segment of path) {
|
||||
if (!isRecord(current)) {
|
||||
@@ -133,10 +163,13 @@ export function readMigrationConfigPath(
|
||||
/** Deep-merges object patches and replaces scalar/array values with a cloned target value. */
|
||||
export function mergeMigrationConfigValue(left: unknown, right: unknown): unknown {
|
||||
if (!isRecord(left) || !isRecord(right)) {
|
||||
return structuredClone(right);
|
||||
return cloneMigrationConfigValue(right);
|
||||
}
|
||||
const next: Record<string, unknown> = { ...left };
|
||||
for (const [key, value] of Object.entries(right)) {
|
||||
if (isBlockedObjectKey(key)) {
|
||||
continue;
|
||||
}
|
||||
next[key] = mergeMigrationConfigValue(next[key], value);
|
||||
}
|
||||
return next;
|
||||
@@ -148,6 +181,9 @@ export function writeMigrationConfigPath(
|
||||
path: readonly string[],
|
||||
value: unknown,
|
||||
): void {
|
||||
if (!isSafeMigrationConfigPath(path)) {
|
||||
throw new Error(MIGRATION_REASON_UNSAFE_CONFIG_PATCH_PATH);
|
||||
}
|
||||
let current = root;
|
||||
for (const segment of path.slice(0, -1)) {
|
||||
const existing = current[segment];
|
||||
@@ -158,7 +194,7 @@ export function writeMigrationConfigPath(
|
||||
}
|
||||
const leaf = path.at(-1);
|
||||
if (!leaf) {
|
||||
return;
|
||||
throw new Error(MIGRATION_REASON_UNSAFE_CONFIG_PATCH_PATH);
|
||||
}
|
||||
current[leaf] = mergeMigrationConfigValue(current[leaf], value);
|
||||
}
|
||||
@@ -177,7 +213,10 @@ export function hasMigrationConfigPatchConflict(
|
||||
if (!isRecord(existing)) {
|
||||
return false;
|
||||
}
|
||||
return Object.keys(value).some((key) => existing[key] !== undefined);
|
||||
return Object.keys(value).some(
|
||||
(key) =>
|
||||
!isBlockedObjectKey(key) && Object.hasOwn(existing, key) && existing[key] !== undefined,
|
||||
);
|
||||
}
|
||||
|
||||
/** Builds a planned or conflicting config-merge migration item. */
|
||||
@@ -251,6 +290,9 @@ export async function applyMigrationConfigPatchItem(
|
||||
if (!details) {
|
||||
return markMigrationItemError(item, "missing config patch");
|
||||
}
|
||||
if (!isSafeMigrationConfigPath(details.path)) {
|
||||
return markMigrationItemError(item, MIGRATION_REASON_UNSAFE_CONFIG_PATCH_PATH);
|
||||
}
|
||||
const configApi = ctx.runtime?.config;
|
||||
if (!configApi?.current || !configApi.mutateConfigFile) {
|
||||
return markMigrationItemError(item, "config runtime unavailable");
|
||||
|
||||
Reference in New Issue
Block a user