From 6782e551f965407aad730e36b0c0a1a056e87967 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Thu, 15 Oct 2026 15:50:56 +0000 Subject: [PATCH] fix(security): harden tool argument sanitization against special keys Copy sanitized object properties with defineProperty instead of direct assignment. Assignment on an ordinary object invokes inherited accessors for special names such as __proto__, letting untrusted input change the prototype of the sanitized result. defineProperty always creates a plain own data property, so __proto__, constructor and prototype are handled safely as ordinary keys. Throw a Duplicate key error when stripping control characters maps two different input keys onto the same output key, instead of silently dropping one of the values. Add direct and nested regression coverage (including objects inside arrays) for special property names, a key that sanitizes into a special name, and key collisions. --- README.md | 2 + src/security.ts | 24 +++++++++- test/tool-argument-limits.test.ts | 78 +++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index bfd15b3..d3c27a0 100644 --- a/README.md +++ b/README.md @@ -2442,6 +2442,8 @@ The plugin automatically sanitizes tool parameters to prevent common attacks: - **Array length limits:** Unlimited by default - **Control character removal:** Strips null bytes and control characters - **Circular reference detection:** Prevents infinite loops +- **Prototype pollution protection:** Special key names (`__proto__`, `constructor`, `prototype`) are copied as plain own data properties, so untrusted input cannot change the prototype of the sanitized arguments +- **Key collision detection:** If stripping control characters maps two input keys onto the same output key, the request is refused instead of silently dropping one of the values The limits are configurable with the `toolArgumentLimits` option. Pass `Infinity` to disable one; Fastify's `bodyLimit` still caps the total request size. Arguments over a limit are refused with an diff --git a/src/security.ts b/src/security.ts index 1a3b00f..c085bbc 100644 --- a/src/security.ts +++ b/src/security.ts @@ -101,6 +101,13 @@ export function validateObjectDepth ( * once up front; every string reached afterwards, including object keys and * strings nested inside arrays at any depth, is length-checked and stripped of * control characters. + * + * Objects are copied by defining plain own data properties instead of direct + * assignment, so special key names such as `__proto__`, `constructor` and + * `prototype` cannot invoke inherited accessors and change the prototype of + * the sanitized result. When stripping control characters maps two different + * input keys onto the same output key, a `Duplicate key` error is thrown rather + * than silently dropping one of the values. */ export function sanitizeToolParams (params: Record, limits: ToolArgumentLimits = {}): Record { const maxStringLength = limits.maxStringLength ?? MAX_STRING_LENGTH @@ -123,7 +130,22 @@ export function sanitizeToolParams (params: Record, limits: ToolArg if (typeof value === 'object' && value !== null) { const sanitized: Record = {} for (const [key, item] of Object.entries(value)) { - sanitized[sanitizeString(key, maxStringLength)] = sanitizeValue(item) + const sanitizedKey = sanitizeString(key, maxStringLength) + // Copy with defineProperty rather than assignment: assignment on an + // ordinary object invokes inherited accessors for special names + // (notably `__proto__`), which lets untrusted input change the + // prototype of the sanitized result. defineProperty always creates a + // plain own data property and never triggers a setter, so `__proto__`, + // `constructor` and `prototype` are handled safely as ordinary keys. + if (Object.prototype.hasOwnProperty.call(sanitized, sanitizedKey)) { + throw new Error(`Duplicate key ${JSON.stringify(sanitizedKey)} after key sanitization`) + } + Object.defineProperty(sanitized, sanitizedKey, { + value: sanitizeValue(item), + enumerable: true, + writable: true, + configurable: true + }) } return sanitized } diff --git a/test/tool-argument-limits.test.ts b/test/tool-argument-limits.test.ts index 9cc94ee..527698e 100644 --- a/test/tool-argument-limits.test.ts +++ b/test/tool-argument-limits.test.ts @@ -195,4 +195,82 @@ describe('tool argument limits', () => { /depth of 3/ ) }) + + test('sanitizeToolParams does not let a direct __proto__ key change the result prototype', (t: TestContext) => { + const result = sanitizeToolParams(JSON.parse('{"__proto__": {"polluted": true}}')) + + t.assert.strictEqual(Object.getPrototypeOf(result), Object.prototype) + t.assert.strictEqual(Object.prototype.hasOwnProperty.call(result, '__proto__'), true) + const descriptor = Object.getOwnPropertyDescriptor(result, '__proto__') + t.assert.ok(descriptor) + t.assert.strictEqual(descriptor.value.polluted, true) + }) + + test('sanitizeToolParams does not let __proto__ pollute when nested in objects and arrays', (t: TestContext) => { + const result = sanitizeToolParams({ + obj: JSON.parse('{"__proto__": {"a": 1}}'), + xs: [JSON.parse('{"__proto__": {"b": 2}}')] + }) + + t.assert.strictEqual(Object.getPrototypeOf(result.obj), Object.prototype) + t.assert.strictEqual(Object.getPrototypeOf(result.xs[0]), Object.prototype) + t.assert.deepStrictEqual(Object.keys(result.obj), ['__proto__']) + t.assert.deepStrictEqual(Object.keys(result.xs[0]), ['__proto__']) + const objDescriptor = Object.getOwnPropertyDescriptor(result.obj, '__proto__') + const xsDescriptor = Object.getOwnPropertyDescriptor(result.xs[0], '__proto__') + t.assert.ok(objDescriptor) + t.assert.ok(xsDescriptor) + t.assert.strictEqual(objDescriptor.value.a, 1) + t.assert.strictEqual(xsDescriptor.value.b, 2) + }) + + test('sanitizeToolParams copies constructor and prototype keys as ordinary own properties', (t: TestContext) => { + const result = sanitizeToolParams(JSON.parse('{"constructor": 1, "prototype": 2}')) + + t.assert.strictEqual(Object.getPrototypeOf(result), Object.prototype) + t.assert.strictEqual(Object.prototype.hasOwnProperty.call(result, 'constructor'), true) + t.assert.strictEqual(Object.prototype.hasOwnProperty.call(result, 'prototype'), true) + t.assert.strictEqual(result.constructor, 1) + t.assert.strictEqual(result.prototype, 2) + }) + + test('sanitizeToolParams safely copies a key that sanitizes to a special property name', (t: TestContext) => { + // '__pro\u0000to__' strips the null character back to '__proto__' + const result = sanitizeToolParams({ '__pro\u0000to__': { polluted: true } }) + + t.assert.strictEqual(Object.getPrototypeOf(result), Object.prototype) + t.assert.strictEqual(Object.prototype.hasOwnProperty.call(result, '__proto__'), true) + const entries = Object.entries(result) + t.assert.strictEqual(entries.length, 1) + t.assert.strictEqual(entries[0][0], '__proto__') + t.assert.strictEqual((entries[0][1] as Record).polluted, true) + }) + + test('sanitizeToolParams detects when key sanitization maps two keys onto the same output key', (t: TestContext) => { + t.assert.throws( + () => sanitizeToolParams({ 'a\u0000b': 1, ab: 2 }), + /Duplicate key "ab" after key sanitization/ + ) + }) + + test('sanitizeToolParams detects key collisions nested inside objects and arrays', (t: TestContext) => { + t.assert.throws( + () => sanitizeToolParams({ o: { 'a\u0000b': 1, ab: 2 } }), + /Duplicate key "ab" after key sanitization/ + ) + t.assert.throws( + () => sanitizeToolParams({ xs: [{ 'a\u0000b': 1, ab: 2 }] }), + /Duplicate key "ab" after key sanitization/ + ) + }) + + test('refuses a tool call whose arguments collide after key sanitization', async (t: TestContext) => { + const { app, getInvocations } = await buildApp(t) + + const result = await callTool(app, { 'a\u0000b': 1, ab: 2 }) + + t.assert.strictEqual(result.isError, true) + t.assert.match((result.content[0] as { text: string }).text, /Duplicate key "ab" after key sanitization/) + t.assert.strictEqual(getInvocations(), 0) + }) })