From 1ffc33dcc63dfcc0dc27905a9c0a01de33da4160 Mon Sep 17 00:00:00 2001 From: Elias Meire Date: Tue, 20 May 2025 15:39:33 +0200 Subject: [PATCH] fix(HTTP Request Node): Fix prototype pollution vulnerability (#15463) --- .../CloudFirestore/GenericFunctions.ts | 6 ++-- .../nodes/HttpRequest/GenericFunctions.ts | 3 +- packages/workflow/src/ExpressionSandboxing.ts | 8 +++--- packages/workflow/src/index.ts | 2 ++ packages/workflow/src/utils.ts | 27 ++++++++++++++++++ packages/workflow/test/utils.test.ts | 28 +++++++++++++++++++ 6 files changed, 65 insertions(+), 9 deletions(-) diff --git a/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/GenericFunctions.ts b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/GenericFunctions.ts index e32fcaab58..7a4ee7b4d4 100644 --- a/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/GenericFunctions.ts @@ -7,7 +7,7 @@ import type { IHttpRequestMethods, IRequestOptions, } from 'n8n-workflow'; -import { NodeApiError } from 'n8n-workflow'; +import { isSafeObjectProperty, NodeApiError } from 'n8n-workflow'; import { getGoogleAccessToken } from '../../GenericFunctions'; @@ -82,8 +82,6 @@ export async function googleApiRequestAllItems( const isValidDate = (str: string) => moment(str, ['YYYY-MM-DD HH:mm:ss Z', moment.ISO_8601], true).isValid(); -const protoKeys = ['__proto__', 'prototype', 'constructor']; - // Both functions below were taken from Stack Overflow jsonToDocument was fixed as it was unable to handle null values correctly // https://stackoverflow.com/questions/62246410/how-to-convert-a-firestore-document-to-plain-json-and-vice-versa // Great thanks to https://stackoverflow.com/users/3915246/mahindar @@ -108,7 +106,7 @@ export function jsonToDocument(value: string | number | IDataObject | IDataObjec } else if (typeof value === 'object') { const obj: IDataObject = {}; for (const key of Object.keys(value)) { - if (value.hasOwnProperty(key) && !protoKeys.includes(key)) { + if (value.hasOwnProperty(key) && isSafeObjectProperty(key)) { obj[key] = jsonToDocument((value as IDataObject)[key] as IDataObject); } } diff --git a/packages/nodes-base/nodes/HttpRequest/GenericFunctions.ts b/packages/nodes-base/nodes/HttpRequest/GenericFunctions.ts index 286ce435b5..338b471967 100644 --- a/packages/nodes-base/nodes/HttpRequest/GenericFunctions.ts +++ b/packages/nodes-base/nodes/HttpRequest/GenericFunctions.ts @@ -4,6 +4,7 @@ import isPlainObject from 'lodash/isPlainObject'; import set from 'lodash/set'; import { deepCopy, + setSafeObjectProperty, type ICredentialDataDecryptedObject, type IDataObject, type INodeExecutionData, @@ -48,7 +49,7 @@ function redact(obj: T, secrets: string[]): T { return obj.map((item) => redact(item, secrets)) as T; } else if (isObject(obj)) { for (const [key, value] of Object.entries(obj)) { - (obj as IDataObject)[key] = redact(value, secrets); + setSafeObjectProperty(obj, key, redact(value, secrets)); } } diff --git a/packages/workflow/src/ExpressionSandboxing.ts b/packages/workflow/src/ExpressionSandboxing.ts index 7562e8efa2..c21156d3fc 100644 --- a/packages/workflow/src/ExpressionSandboxing.ts +++ b/packages/workflow/src/ExpressionSandboxing.ts @@ -1,8 +1,8 @@ import { type ASTAfterHook, astBuilders as b, astVisit } from '@n8n/tournament'; import { ExpressionError } from './errors'; +import { isSafeObjectProperty } from './utils'; -const forbiddenMembers = ['__proto__', 'prototype', 'constructor', 'getPrototypeOf']; export const sanitizerName = '__sanitize'; const sanitizerIdentifier = b.identifier(sanitizerName); @@ -20,14 +20,14 @@ export const PrototypeSanitizer: ASTAfterHook = (ast, dataNode) => { ); } - if (forbiddenMembers.includes(node.property.name)) { + if (!isSafeObjectProperty(node.property.name)) { throw new ExpressionError( `Cannot access "${node.property.name}" due to security concerns`, ); } } else if (node.property.type === 'StringLiteral' || node.property.type === 'Literal') { // Check any static strings against our forbidden list - if (forbiddenMembers.includes(node.property.value as string)) { + if (!isSafeObjectProperty(node.property.value as string)) { throw new ExpressionError( `Cannot access "${node.property.value as string}" due to security concerns`, ); @@ -52,7 +52,7 @@ export const PrototypeSanitizer: ASTAfterHook = (ast, dataNode) => { }; export const sanitizer = (value: unknown): unknown => { - if (forbiddenMembers.includes(value as string)) { + if (!isSafeObjectProperty(value as string)) { throw new ExpressionError(`Cannot access "${value as string}" due to security concerns`); } return value; diff --git a/packages/workflow/src/index.ts b/packages/workflow/src/index.ts index 0ed7a2c93e..ef2ff18ed4 100644 --- a/packages/workflow/src/index.ts +++ b/packages/workflow/src/index.ts @@ -35,6 +35,8 @@ export { updateDisplayOptions, randomInt, randomString, + isSafeObjectProperty, + setSafeObjectProperty, } from './utils'; export { isINodeProperties, diff --git a/packages/workflow/src/utils.ts b/packages/workflow/src/utils.ts index d758e17324..c3daea7685 100644 --- a/packages/workflow/src/utils.ts +++ b/packages/workflow/src/utils.ts @@ -283,3 +283,30 @@ export function randomString(minLength: number, maxLength?: number): string { export function hasKey(value: unknown, key: T): value is Record { return value !== null && typeof value === 'object' && value.hasOwnProperty(key); } + +const unsafeObjectProperties = new Set(['__proto__', 'prototype', 'constructor', 'getPrototypeOf']); + +/** + * Checks if a property key is safe to use on an object, preventing prototype pollution. + * setting untrusted properties can alter the object's prototype chain and introduce vulnerabilities. + * + * @see setSafeObjectProperty + */ +export function isSafeObjectProperty(property: string) { + return !unsafeObjectProperties.has(property); +} + +/** + * Safely sets a property on an object, preventing prototype pollution. + * + * @see isSafeObjectProperty + */ +export function setSafeObjectProperty( + target: Record, + property: string, + value: unknown, +) { + if (isSafeObjectProperty(property)) { + target[property] = value; + } +} diff --git a/packages/workflow/test/utils.test.ts b/packages/workflow/test/utils.test.ts index 3723f846b0..20bd1360a6 100644 --- a/packages/workflow/test/utils.test.ts +++ b/packages/workflow/test/utils.test.ts @@ -9,6 +9,8 @@ import { randomInt, randomString, hasKey, + isSafeObjectProperty, + setSafeObjectProperty, } from '@/utils'; describe('isObjectEmpty', () => { @@ -366,3 +368,29 @@ describe('hasKey', () => { } }); }); + +describe('isSafeObjectProperty', () => { + it.each([ + ['__proto__', false], + ['prototype', false], + ['constructor', false], + ['getPrototypeOf', false], + ['safeKey', true], + ['anotherKey', true], + ['toString', true], + ])('should return %s for key "%s"', (key, expected) => { + expect(isSafeObjectProperty(key)).toBe(expected); + }); +}); + +describe('setSafeObjectProperty', () => { + it.each([ + ['safeKey', 123, { safeKey: 123 }], + ['__proto__', 456, {}], + ['constructor', 'test', {}], + ])('should set property "%s" safely', (key, value, expected) => { + const obj: Record = {}; + setSafeObjectProperty(obj, key, value); + expect(obj).toEqual(expected); + }); +});