diff --git a/.changeset/merge-dict.md b/.changeset/merge-dict.md new file mode 100644 index 000000000..73db3bf77 --- /dev/null +++ b/.changeset/merge-dict.md @@ -0,0 +1,5 @@ +--- +"braintrust": patch +--- + +fix(security): reject `__proto__`, `constructor`, and `prototype` keys in `mergeDicts` / `mergeDictsWithPaths` to prevent prototype pollution from untrusted merge sources diff --git a/js/util/object_util.test.ts b/js/util/object_util.test.ts index e22ddc63f..794f601d6 100644 --- a/js/util/object_util.test.ts +++ b/js/util/object_util.test.ts @@ -1,4 +1,4 @@ -import { expect, test, describe } from "vitest"; +import { expect, test, describe, afterEach } from "vitest"; import { mapAt, forEachMissingKey, @@ -242,3 +242,52 @@ describe("tags set-union merge", () => { expect(a.metadata.tags).toEqual(["c", "d"]); }); }); + +describe("mergeDicts prototype pollution resistance", () => { + const POLLUTION_KEY = "polluted"; + + afterEach(() => { + Reflect.deleteProperty(Object.prototype, POLLUTION_KEY); + }); + + test.each([ + { + name: "__proto__ at top level", + payload: '{"__proto__":{"polluted":"yes"}}', + }, + { + name: "nested __proto__", + payload: '{"a":{"__proto__":{"polluted":"yes"}}}', + }, + { + name: "constructor.prototype", + payload: '{"constructor":{"prototype":{"polluted":"yes"}}}', + }, + { + name: "top-level prototype", + payload: '{"prototype":{"polluted":"yes"}}', + }, + ])("mergeDicts ignores $name", ({ payload }) => { + const target: Record = {}; + mergeDicts(target, JSON.parse(payload)); + expect(Object.prototype).not.toHaveProperty(POLLUTION_KEY); + expect(target).not.toHaveProperty(POLLUTION_KEY); + }); + + test("nested __proto__ leaves the legitimate subtree intact", () => { + const target: Record = { a: { b: 1 } }; + mergeDicts(target, JSON.parse('{"a":{"__proto__":{"polluted":"yes"}}}')); + expect(target).toEqual({ a: { b: 1 } }); + }); + + test("forbidden key alongside safe key only drops the forbidden one", () => { + const target: Record = {}; + mergeDictsWithPaths({ + mergeInto: target, + mergeFrom: JSON.parse('{"safe":1,"__proto__":{"polluted":"yes"}}'), + mergePaths: [], + }); + expect(target).toEqual({ safe: 1 }); + expect(Object.prototype).not.toHaveProperty(POLLUTION_KEY); + }); +}); diff --git a/js/util/object_util.ts b/js/util/object_util.ts index 7925eac06..607bacbb8 100644 --- a/js/util/object_util.ts +++ b/js/util/object_util.ts @@ -3,6 +3,13 @@ import { isArray, isObject, isObjectOrArray } from "./type_util"; // Fields that automatically use set-union merge semantics (unless in mergePaths). const SET_UNION_FIELDS = new Set(["tags"]); +// Block certain keys from being merged to prevent prototype pollution and other unsafe merges. +const FORBIDDEN_MERGE_KEYS: ReadonlySet = new Set([ + "__proto__", + "constructor", + "prototype", +]); + // Mutably updates `mergeInto` with the contents of `mergeFrom`, merging objects // deeply. Does not merge any further than `merge_paths`. // For fields in SET_UNION_FIELDS (like "tags"), arrays are merged as sets (union) @@ -39,6 +46,7 @@ function mergeDictsWithPathsHelper({ mergePaths: Set; }) { Object.entries(mergeFrom).forEach(([k, mergeFromV]) => { + if (FORBIDDEN_MERGE_KEYS.has(k)) return; const fullPath = path.concat([k]); const fullPathSerialized = JSON.stringify(fullPath); const mergeIntoV = recordFind(mergeInto, k);