From beda4218af32f482daa3c5e0fa3057b5b898ea88 Mon Sep 17 00:00:00 2001 From: Yuval Dinodia <102706514+yetval@users.noreply.github.com> Date: Thu, 9 Jul 2026 19:17:31 -0400 Subject: [PATCH] fix(secrets): reject prototype-polluting mutation paths (#102840) * fix(secrets): reject prototype-polluting mutation paths Co-authored-by: yetval * docs(secrets): document blocked mutation segments * chore(secrets): keep release notes in PR metadata --------- Co-authored-by: Peter Steinberger --- docs/plugins/manifest.md | 2 +- src/secrets/path-utils.test.ts | 34 +++++++++++++++++++++++ src/secrets/path-utils.ts | 19 ++++++++----- src/secrets/plan.ts | 9 ++----- src/wizard/setup.plugin-config.test.ts | 37 ++++++++++++++++++++++++++ 5 files changed, 87 insertions(+), 14 deletions(-) diff --git a/docs/plugins/manifest.md b/docs/plugins/manifest.md index 48938c8115bb..eec98f9134cc 100644 --- a/docs/plugins/manifest.md +++ b/docs/plugins/manifest.md @@ -511,7 +511,7 @@ Supported evidence entries: ## uiHints reference -`uiHints` is a map from config field names to small rendering hints. +`uiHints` is a map from config field names to small rendering hints. Keys can use dots for nested config fields, but no path segment may be `__proto__`, `constructor`, or `prototype`; setup rejects those names. ```json { diff --git a/src/secrets/path-utils.test.ts b/src/secrets/path-utils.test.ts index fa439c8a0f76..bfac82b3e0bd 100644 --- a/src/secrets/path-utils.test.ts +++ b/src/secrets/path-utils.test.ts @@ -20,6 +20,9 @@ function createAgentListConfig(): OpenClawConfig { }); } +const BLOCKED_PATH_SEGMENTS = ["__proto__", "constructor", "prototype"]; +const POLLUTION_PROBE = "openclawPathPollutionProbe"; + describe("secrets path utils", () => { it("deletePathStrict compacts arrays via splice", () => { const config = asConfig({}); @@ -56,6 +59,37 @@ describe("secrets path utils", () => { ); }); + it.each(BLOCKED_PATH_SEGMENTS)( + "setPathCreateStrict rejects %s before creating partial containers", + (blockedSegment) => { + const config = asConfig({}); + + expect(() => + setPathCreateStrict(config, ["safe", blockedSegment, POLLUTION_PROBE], "yes"), + ).toThrow(/prototype-polluting/); + expect(config).toEqual({}); + expect(Object.hasOwn(Object.prototype, POLLUTION_PROBE)).toBe(false); + }, + ); + + it.each([ + ["leading", (blockedSegment: string) => [blockedSegment, POLLUTION_PROBE]], + ["middle", (blockedSegment: string) => ["safe", blockedSegment, POLLUTION_PROBE]], + ["leaf", (blockedSegment: string) => ["safe", "value", blockedSegment]], + ] as const)("all mutation helpers reject blocked segments at the %s", (_position, pathFor) => { + for (const blockedSegment of BLOCKED_PATH_SEGMENTS) { + const segments = pathFor(blockedSegment); + const config = asConfig({ safe: { value: "kept" } }); + + expect(() => setPathCreateStrict(config, segments, "changed")).toThrow(/prototype-polluting/); + expect(() => setPathExistingStrict(config, segments, "changed")).toThrow( + /prototype-polluting/, + ); + expect(() => deletePathStrict(config, segments)).toThrow(/prototype-polluting/); + expect(config).toEqual({ safe: { value: "kept" } }); + } + }); + it("setPathExistingStrict throws when path does not already exist", () => { const config = createAgentListConfig(); expect(() => diff --git a/src/secrets/path-utils.ts b/src/secrets/path-utils.ts index 7e96c1cab771..409c89df7892 100644 --- a/src/secrets/path-utils.ts +++ b/src/secrets/path-utils.ts @@ -1,5 +1,6 @@ /** Strict dotted-path get/set/delete helpers for secrets migration targets. */ import { isDeepStrictEqual } from "node:util"; +import { isBlockedObjectKey } from "../infra/prototype-keys.js"; import { parseConfigPathArrayIndex } from "../shared/path-array-index.js"; import { isRecord } from "./shared.js"; @@ -23,6 +24,16 @@ function expectedContainer(nextSegment: string): "array" | "object" { return looksLikeArrayIndexSegment(nextSegment) ? "array" : "object"; } +function assertSafeMutationPath(segments: string[]): void { + if (segments.length === 0) { + throw new Error("Target path is empty."); + } + const blockedSegment = segments.find(isBlockedObjectKey); + if (blockedSegment) { + throw new Error(`Refusing to mutate prototype-polluting path segment "${blockedSegment}".`); + } +} + function parseArrayLeafTarget( cursor: unknown, leaf: string, @@ -39,9 +50,7 @@ function traverseToLeafParent(params: { segments: string[]; requireExistingSegment: boolean; }): unknown { - if (params.segments.length === 0) { - throw new Error("Target path is empty."); - } + assertSafeMutationPath(params.segments); let cursor: unknown = params.root; for (let index = 0; index < params.segments.length - 1; index += 1) { @@ -108,9 +117,7 @@ export function setPathCreateStrict( segments: string[], value: unknown, ): boolean { - if (segments.length === 0) { - throw new Error("Target path is empty."); - } + assertSafeMutationPath(segments); let cursor: unknown = root; let changed = false; diff --git a/src/secrets/plan.ts b/src/secrets/plan.ts index 91ff3760eac8..f4865629e4cd 100644 --- a/src/secrets/plan.ts +++ b/src/secrets/plan.ts @@ -3,6 +3,7 @@ import { isRecord as isObjectRecord } from "@openclaw/normalization-core/record- import { normalizeStringEntries } from "@openclaw/normalization-core/string-normalization"; import type { SecretProviderConfig, SecretRef } from "../config/types.secrets.js"; import { SecretProviderSchema } from "../config/zod-schema.core.js"; +import { isBlockedObjectKey } from "../infra/prototype-keys.js"; import { isValidSecretProviderAlias, isValidSecretRef } from "./ref-contract.js"; import { parseDotPath, toDotPath } from "./shared.js"; import { resolvePlanTargetAgainstRegistry, type ResolvedPlanTarget } from "./target-registry.js"; @@ -60,16 +61,10 @@ export type SecretsApplyPlan = { }; }; -const FORBIDDEN_PATH_SEGMENTS = new Set(["__proto__", "prototype", "constructor"]); - function isSecretProviderConfigShape(value: unknown): value is SecretProviderConfig { return SecretProviderSchema.safeParse(value).success; } -function hasForbiddenPathSegment(segments: string[]): boolean { - return segments.some((segment) => FORBIDDEN_PATH_SEGMENTS.has(segment)); -} - /** Resolves a user-supplied plan target through the registry after path safety checks. */ export function resolveValidatedPlanTarget(candidate: { type?: SecretsPlanTargetType; @@ -91,7 +86,7 @@ export function resolveValidatedPlanTarget(candidate: { Array.isArray(candidate.pathSegments) && candidate.pathSegments.length > 0 ? normalizeStringEntries(candidate.pathSegments) : parseDotPath(path); - if (segments.length === 0 || hasForbiddenPathSegment(segments) || path !== toDotPath(segments)) { + if (segments.length === 0 || segments.some(isBlockedObjectKey) || path !== toDotPath(segments)) { return null; } // Registry resolution is the ownership gate; caller-provided paths must map to a known diff --git a/src/wizard/setup.plugin-config.test.ts b/src/wizard/setup.plugin-config.test.ts index 2f5f7875e816..cb4c7add2887 100644 --- a/src/wizard/setup.plugin-config.test.ts +++ b/src/wizard/setup.plugin-config.test.ts @@ -357,6 +357,43 @@ describe("setupPluginConfig", () => { expect(result.plugins?.entries?.brave?.config?.["webSearch.mode"]).toBeUndefined(); }); + it("rejects prototype-polluting dotted uiHint paths without mutating config", async () => { + const pollutionProbe = "openclawPluginPollutionProbe"; + loadPluginManifestRegistry.mockReturnValue({ + plugins: [ + { + ...makeManifestPlugin("unsafe-plugin", { + [`safe.__proto__.${pollutionProbe}`]: { label: "Unsafe field" }, + }), + enabledByDefault: true, + }, + ], + }); + const config: OpenClawConfig = { + plugins: { entries: { "unsafe-plugin": { enabled: true } } }, + }; + + await expect( + setupPluginConfig({ + config, + prompter: { + intro: vi.fn(async () => {}), + outro: vi.fn(async () => {}), + note: vi.fn(async () => {}), + select: vi.fn(async () => "") as unknown as WizardPrompter["select"], + multiselect: vi.fn(async () => [ + "unsafe-plugin", + ]) as unknown as WizardPrompter["multiselect"], + text: vi.fn(async () => "owned") as unknown as WizardPrompter["text"], + confirm: vi.fn(async () => true), + progress: vi.fn(() => ({ update: vi.fn(), stop: vi.fn() })), + }, + }), + ).rejects.toThrow(/prototype-polluting/); + expect(config.plugins?.entries?.["unsafe-plugin"]?.config).toBeUndefined(); + expect(({} as Record)[pollutionProbe]).toBeUndefined(); + }); + it("coerces integer schema fields from text input", async () => { loadPluginManifestRegistry.mockReturnValue({ plugins: [