diff --git a/packages/openapi/src/bracket-notation.test.ts b/packages/openapi/src/bracket-notation.test.ts index e6e9af973..6281418aa 100644 --- a/packages/openapi/src/bracket-notation.test.ts +++ b/packages/openapi/src/bracket-notation.test.ts @@ -410,6 +410,27 @@ describe('bracket notation serializer', () => { expect(result.constructor.prototype.polluted).toBe('x') expect(result.a.constructor.prototype.polluted).toBe('y') }) + + it('can prevent prototype pollution attack through object values', () => { + /* eslint-disable no-proto, no-restricted-properties */ + const result = serializer.deserialize([ + ['a', {}], + ['a[__proto__][polluted]', '1'], + ['b', { c: '2' }], + ['b[constructor][prototype][polluted]', '3'], + ]) as any + + expect(({} as any).polluted).toBeUndefined() + expect((Object.prototype as any).polluted).toBeUndefined() + + // dangerous keys are stored as plain data on the object value, not as real prototype links + expect(result.a.__proto__).toEqual({ polluted: '1' }) + expect(result.a.polluted).toBeUndefined() + expect(result.b.c).toBe('2') + expect(result.b.constructor.prototype.polluted).toBe('3') + expect(result.b.polluted).toBeUndefined() + /* eslint-enable no-proto, no-restricted-properties */ + }) }) it.each([ diff --git a/packages/openapi/src/bracket-notation.ts b/packages/openapi/src/bracket-notation.ts index 6fc8b9503..de47ecde9 100644 --- a/packages/openapi/src/bracket-notation.ts +++ b/packages/openapi/src/bracket-notation.ts @@ -1,5 +1,5 @@ import type { Segment } from '@orpc/shared' -import { isPlainObject, NullProtoObj } from '@orpc/shared' +import { getOwn, isPlainObject, NullProtoObj, setOwn } from '@orpc/shared' export type BracketNotationSerializeResult = [string, unknown][] @@ -52,58 +52,46 @@ export class BracketNotationSerializer { } deserialize(serialized: BracketNotationSerializeResult): Record { - if (serialized.length === 0) { - return new NullProtoObj() // Prevent Prototype Pollution with NullProtoObj - } - + // A caller-supplied object value can become a container for deeper paths, and unlike + // `NullProtoObj` it carries a real prototype, so accesses below stay own-property only. const arrayPushStyles = new WeakSet() - const ref: { value: Record } = { value: new NullProtoObj() } // Prevent Prototype Pollution with NullProtoObj + const root: Record = new NullProtoObj() for (const [path, value] of serialized) { const segments = this.parsePath(path) - let currentRef: any = ref - let nextSegment: string = 'value' + let currentRef: any = root + let nextSegment: string = segments[0]! - for (let i = 0; i < segments.length; i++) { + for (let i = 1; i < segments.length; i++) { const segment = segments[i]! + const isLast = i === segments.length - 1 + + const existing: any = getOwn(currentRef, nextSegment) + let child: any = existing - if (!Array.isArray(currentRef[nextSegment]) && !isPlainObject(currentRef[nextSegment])) { - currentRef[nextSegment] = [] + if (!Array.isArray(child) && !isPlainObject(child)) { + child = [] } - if (i !== segments.length - 1) { - if (Array.isArray(currentRef[nextSegment]) && !internalIsValidArrayIndex(segment, this.maxExplicitDeserializingArrayIndex)) { - if (arrayPushStyles.has(currentRef[nextSegment])) { - arrayPushStyles.delete(currentRef[nextSegment]) - currentRef[nextSegment] = internalPushStyleArrayToObject(currentRef[nextSegment]) - } - else { - currentRef[nextSegment] = internalArrayToObject(currentRef[nextSegment]) - } + if (Array.isArray(child)) { + const isPushStyle = arrayPushStyles.has(child) + + const canStayArray = segment === '' + ? isLast && (isPushStyle || child.length === 0) + : internalIsValidArrayIndex(segment, this.maxExplicitDeserializingArrayIndex) && !(isLast && isPushStyle) + + if (!canStayArray) { + arrayPushStyles.delete(child) + child = isPushStyle ? internalPushStyleArrayToObject(child) : internalArrayToObject(child) } } - else { - if (Array.isArray(currentRef[nextSegment])) { - if (segment === '') { - if (currentRef[nextSegment].length && !arrayPushStyles.has(currentRef[nextSegment])) { - currentRef[nextSegment] = internalArrayToObject(currentRef[nextSegment]) - } - } - else { - if (arrayPushStyles.has(currentRef[nextSegment])) { - arrayPushStyles.delete(currentRef[nextSegment]) - currentRef[nextSegment] = internalPushStyleArrayToObject(currentRef[nextSegment]) - } - - else if (!internalIsValidArrayIndex(segment, this.maxExplicitDeserializingArrayIndex)) { - currentRef[nextSegment] = internalArrayToObject(currentRef[nextSegment]) - } - } - } + + if (child !== existing) { + setOwn(currentRef, nextSegment, child) } - currentRef = currentRef[nextSegment] + currentRef = child nextSegment = segment } @@ -111,20 +99,22 @@ export class BracketNotationSerializer { arrayPushStyles.add(currentRef) currentRef.push(value) } - else if (nextSegment in currentRef) { - if (Array.isArray(currentRef[nextSegment])) { - currentRef[nextSegment].push(value) + else if (Object.hasOwn(currentRef, nextSegment)) { + const current = currentRef[nextSegment] + + if (Array.isArray(current)) { + current.push(value) } else { - currentRef[nextSegment] = [currentRef[nextSegment], value] + setOwn(currentRef, nextSegment, [current, value]) } } else { - currentRef[nextSegment] = value + setOwn(currentRef, nextSegment, value) } } - return ref.value + return root } stringifyPath(segments: readonly Segment[]): string { diff --git a/packages/shared/src/object.test.ts b/packages/shared/src/object.test.ts index 83639b88e..4b47c8c88 100644 --- a/packages/shared/src/object.test.ts +++ b/packages/shared/src/object.test.ts @@ -1,7 +1,7 @@ import * as a from 'arktype' import * as v from 'valibot' import z from 'zod' -import { bindMethods, clone, findDeepMatches, get, getConstructor, getConstructors, getOwn, isPlainObject, isPropertyKey, mergeTwoLevels, NullProtoObj, omit, set } from './object' +import { bindMethods, clone, findDeepMatches, get, getConstructor, getConstructors, getOwn, isPlainObject, isPropertyKey, mergeTwoLevels, NullProtoObj, omit, set, setOwn } from './object' it('findDeepMatches', () => { const { maps, values } = findDeepMatches(v => typeof v === 'string', { @@ -16,6 +16,7 @@ it('findDeepMatches', () => { 'v4', ], }, + ignored: 42, }) expect(maps).toEqual([ @@ -239,6 +240,84 @@ describe('set', () => { }) }) +describe('setOwn', () => { + it('sets a value', () => { + const root: Record = {} + setOwn(root, 'a', 1) + expect(root).toEqual({ a: 1 }) + }) + + it('overwrites an existing value', () => { + const root: Record = { a: 1 } + setOwn(root, 'a', 2) + expect(root).toEqual({ a: 2 }) + }) + + it('supports symbol and number keys', () => { + const root: Record = {} + const sym = Symbol('key') + + setOwn(root, sym, 'sym-value') + setOwn(root, 0, 'number-value') + + expect(root[sym]).toBe('sym-value') + expect(root[0]).toBe('number-value') + }) + + it('writes through to an array index', () => { + const root: unknown[] = [] + setOwn(root, 0, 'value') + + expect(root).toEqual(['value']) + expect(root.length).toBe(1) + }) + + it('sets __proto__ as an own property instead of changing the prototype', () => { + const root: Record = {} + setOwn(root, '__proto__', 'value') + + expect(Object.getPrototypeOf(root)).toBe(Object.prototype) + expect(getOwn(root, '__proto__')).toBe('value') + // eslint-disable-next-line no-proto, no-restricted-properties + expect((root as any).__proto__).toBe('value') + }) + + it('does not pollute the prototype via __proto__', () => { + const root: Record = {} + setOwn(root, '__proto__', { polluted: 'yes' }) + + expect(({} as Record).polluted).toBeUndefined() + expect((Object.prototype as any).polluted).toBeUndefined() + expect(Object.getPrototypeOf(root)).toBe(Object.prototype) + expect(root.polluted).toBeUndefined() + }) + + it('sets __proto__ on a null prototype object', () => { + const root = new NullProtoObj>() + setOwn(root, '__proto__', 'value') + + expect(Object.getPrototypeOf(root)).toBe(Object.getPrototypeOf(new NullProtoObj())) + expect(getOwn(root, '__proto__')).toBe('value') + }) + + it('defines __proto__ as a writable, enumerable and configurable property', () => { + const root: Record = {} + setOwn(root, '__proto__', 'value') + + expect(Object.getOwnPropertyDescriptor(root, '__proto__')).toEqual({ + value: 'value', + writable: true, + enumerable: true, + configurable: true, + }) + + expect(Object.keys(root)).toEqual(['__proto__']) + + setOwn(root, '__proto__', 'updated') + expect(getOwn(root, '__proto__')).toBe('updated') + }) +}) + describe('mergeTwoLevels', () => { it('merges the root level', () => { expect(mergeTwoLevels({ a: 1, shared: 'first' }, { b: 2, shared: 'second' })).toEqual({ diff --git a/packages/shared/src/object.ts b/packages/shared/src/object.ts index 8f26148fa..a1617ea79 100644 --- a/packages/shared/src/object.ts +++ b/packages/shared/src/object.ts @@ -121,7 +121,7 @@ export function set( /** * Sets `object[key]`, defining `__proto__` as an own property instead of re-parenting the object. */ -function setOwn(object: object, key: PropertyKey, value: unknown): void { +export function setOwn(object: object, key: PropertyKey, value: unknown): void { if (key === '__proto__') { Object.defineProperty(object, key, { value,