fix(HTTP Request Node): Fix prototype pollution vulnerability (#15463)

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);
 			}
 		}

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<T = unknown>(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));
 		}
 	}
 

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;

packages/workflow/src/index.ts

@@ -35,6 +35,8 @@ export {
 	updateDisplayOptions,
 	randomInt,
 	randomString,
+	isSafeObjectProperty,
+	setSafeObjectProperty,
 } from './utils';
 export {
 	isINodeProperties,

packages/workflow/src/utils.ts

@@ -283,3 +283,30 @@ export function randomString(minLength: number, maxLength?: number): string {
 export function hasKey<T extends PropertyKey>(value: unknown, key: T): value is Record<T, unknown> {
 	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<string, unknown>,
+	property: string,
+	value: unknown,
+) {
+	if (isSafeObjectProperty(property)) {
+		target[property] = value;
+	}
+}

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<string, unknown> = {};
+		setSafeObjectProperty(obj, key, value);
+		expect(obj).toEqual(expected);
+	});
+});