From 9790834cf4c2bca75db00e588e58056dacaf602f Mon Sep 17 00:00:00 2001 From: Daniel Thoma Date: Thu, 2 Nov 2023 23:26:43 +0100 Subject: [PATCH 1/2] Add guards against prototype pollution Add guards in setByPath and getByPath to prevent prototype pollution. Fixes CVE-2023-45827 (https://github.com/clickbar/dot-diver/security/advisories/GHSA-9w5f-mw3p-pj47) Co-authored-by: saibotk --- CHANGELOG.md | 1 + src/index.ts | 47 ++++++++++++++++++++++++++++----------- test/index.test.ts | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca89e8a..5ea5554 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes to **dot-diver** will be documented here. Inspired by [keep - Updated dependencies - Formatted code with new lint rules - Fixed testcase for new TypeScript behavior +- Added guards against prototype pollution, thanks to @d3ng03 () ## [1.0.1](https://github.com/clickbar/dot-diver/tree/1.0.1) (2023-03-26) diff --git a/src/index.ts b/src/index.ts index 7f0733d..6b2ec3e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -218,8 +218,12 @@ type PathValueEntry, Depth extends number = 10> type SearchableObject = Record | unknown[] +// eslint-disable-next-line @typescript-eslint/unbound-method +const hasOwnProperty = Object.prototype.hasOwnProperty + /** - * Retrives a value from an object by dot notation + * Retrives a value from an object by dot notation. The value is received by optional chaining, + * therefore this function returns undefined if a intermediate property is undefined. * * @param object - object to get value from * @param path - path to value @@ -236,16 +240,27 @@ function getByPath & string>( ): PathValueEntry { const pathArray = (path as string).split('.') - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access,@typescript-eslint/no-unsafe-return - return pathArray.reduce((accumulator: any, current) => accumulator?.[current], object) + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return pathArray.reduce((current: any, pathPart) => { + if (typeof current !== 'object' || !hasOwnProperty.call(current, pathPart)) { + return undefined + } + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access,@typescript-eslint/no-unsafe-return + return current?.[pathPart] + }, object) } /** - * Sets a value in an object by dot notation + * Sets a value in an object by dot notation. If a intermediate property is undefined, + * this function will throw an error. + * * @param object - object to set value in * @param path - path to value * @param value - value to set * + * @throws {Error} - if a intermediate property is undefined + * * @privateRemarks * The intersection between PathEntry and string is necessary for TypeScript to successfully narrow down the type of P based on the user-provided path input. * Without the intersection, the path would just be of type PathEntry and PathValueEntry would be a union of all possible return types. @@ -264,18 +279,24 @@ function setByPath< } // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const objectToSet = pathArray.reduce( - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access,@typescript-eslint/no-unsafe-return - (accumulator: any, current) => accumulator?.[current], - object, - ) + const parentObject = pathArray.reduce((current: any, pathPart) => { + if (typeof current !== 'object' || !hasOwnProperty.call(current, pathPart)) { + throw new Error(`Property ${pathPart} is undefined`) + } - if (objectToSet === undefined) { - throw new Error('Path is invalid') - } + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access + const next = current?.[pathPart] + + if (next === undefined || next === null) { + throw new Error(`Property ${pathPart} is undefined`) + } + + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return next + }, object) // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - objectToSet[lastKey] = value + parentObject[lastKey] = value } export type { PathEntry as Path, PathValueEntry as PathValue, SearchableObject } diff --git a/test/index.test.ts b/test/index.test.ts index 5665692..d08ac09 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -160,3 +160,58 @@ it('Test readme usage example: ⚙️ Customizing the Depth Limit', () => { setByPathDepth5(object, 'f.1.g', 'new array-item-2') expect(object.f[1].g).toBe('new array-item-2') }) + +it('Test for prototype pollution', () => { + const object = {} + + expect(() => { + // @ts-expect-error - this is not a valid path for the object + setByPath(object, '__proto__.polluted', true) + }).toThrowError('__proto__') + + // @ts-expect-error - this is not a valid path for the object + expect(getByPath(object, '__proto__')).toBe(undefined) + + expect(() => { + // @ts-expect-error - this is not a valid path for the object + setByPath(object, 'constructor.polluted', true) + }).toThrowError('constructor') + + // @ts-expect-error - this is not a valid path for the object + expect(getByPath(object, 'constructor')).toBe(undefined) + + // @ts-expect-error - this is should not be defined on the object + expect(object.polluted).toBe(undefined) + + const object2 = { constructor: { prototype: { polluted: true } } } + + expect(getByPath(object2, 'constructor.prototype.polluted')).toBe(true) + + setByPath(object2, 'constructor.prototype.polluted', false) + + expect(object2.constructor.prototype.polluted).toBe(false) + + // eslint-disable-next-line @typescript-eslint/no-extraneous-class + const testClass = class TestClass { + // eslint-disable-next-line @typescript-eslint/no-useless-constructor, @typescript-eslint/no-empty-function + constructor() {} + } + + const object3 = new testClass() + + // @ts-expect-error - this is not a valid path for the object + expect(getByPath(object3, 'constructor.prototype')).toBe(undefined) + + // @ts-expect-error - this is not a valid path for the object + expect(getByPath(object3, 'constructor')).toBe(undefined) + + expect(() => { + // @ts-expect-error - this is not a valid path for the object + setByPath(object3, 'constructor.polluted', true) + }).toThrowError('constructor') + + expect(() => { + // @ts-expect-error - this is not a valid path for the object + setByPath(object3, '__proto__.polluted', true) + }).toThrowError('__proto__') +}) From 56c64dd8bda7c52b698f4fd2ff26c3ee58a9a2a4 Mon Sep 17 00:00:00 2001 From: Daniel Thoma Date: Thu, 2 Nov 2023 23:36:11 +0100 Subject: [PATCH 2/2] fix various typos --- src/index.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/index.ts b/src/index.ts index 6b2ec3e..845a0e6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -222,8 +222,8 @@ type SearchableObject = Record | unknown[] const hasOwnProperty = Object.prototype.hasOwnProperty /** - * Retrives a value from an object by dot notation. The value is received by optional chaining, - * therefore this function returns undefined if a intermediate property is undefined. + * Retrieves a value from an object by dot notation. The value is received by optional chaining, + * therefore this function returns undefined if an intermediate property is undefined. * * @param object - object to get value from * @param path - path to value @@ -252,14 +252,14 @@ function getByPath & string>( } /** - * Sets a value in an object by dot notation. If a intermediate property is undefined, + * Sets a value in an object by dot notation. If an intermediate property is undefined, * this function will throw an error. * * @param object - object to set value in * @param path - path to value * @param value - value to set * - * @throws {Error} - if a intermediate property is undefined + * @throws {Error} - if an intermediate property is undefined * * @privateRemarks * The intersection between PathEntry and string is necessary for TypeScript to successfully narrow down the type of P based on the user-provided path input.