fix(core): Avoid using Object.keys on Buffer and other non-plain objects (#6131)

* create a unified way to check if an object is empty

* avoid running `Object.keys` on Buffer objects, to avoid unnecessary memory usage

packages/core/src/NodeExecuteFunctions.ts

@@ -64,6 +64,7 @@ import type {
 } from 'n8n-workflow';
 import {
 	createDeferredPromise,
+	isObjectEmpty,
 	NodeApiError,
 	NodeHelpers,
 	NodeOperationError,
@@ -727,10 +728,6 @@ export async function proxyRequestToAxios(
 	}
 }
 
-function isIterator(obj: unknown): boolean {
-	return obj instanceof Object && Symbol.iterator in obj;
-}
-
 function convertN8nRequestToAxios(n8nRequest: IHttpRequestOptions): AxiosRequestConfig {
 	// Destructure properties with the same name first.
 	const { headers, method, timeout, auth, proxy, url } = n8nRequest;
@@ -794,7 +791,7 @@ function convertN8nRequestToAxios(n8nRequest: IHttpRequestOptions): AxiosRequest
 		// if there is a body and it's empty (does not have properties),
 		// make sure not to send anything in it as some services fail when
 		// sending GET request with empty body.
-		if (isIterator(body) || Object.keys(body).length > 0) {
+		if (typeof body === 'object' && !isObjectEmpty(body)) {
 			axiosRequest.data = body;
 		}
 	}

packages/nodes-base/credentials/Aws.credentials.ts

@@ -9,6 +9,7 @@ import type {
 	IHttpRequestOptions,
 	INodeProperties,
 } from 'n8n-workflow';
+import { isObjectEmpty } from 'n8n-workflow';
 import type { OptionsWithUri } from 'request';
 
 export const regions = [
@@ -353,7 +354,7 @@ export class Aws implements ICredentialType {
 			});
 		}
 
-		if (body && Object.keys(body).length === 0) {
+		if (body && typeof body === 'object' && !isObjectEmpty(body)) {
 			body = '';
 		}
 

packages/workflow/src/index.ts

@@ -23,7 +23,15 @@ export * from './WorkflowErrors';
 export * from './WorkflowHooks';
 export * from './VersionedNodeType';
 export { LoggerProxy, NodeHelpers, ObservableObject, TelemetryHelpers };
-export { deepCopy, jsonParse, jsonStringify, sleep, fileTypeFromMimeType, assert } from './utils';
+export {
+	isObjectEmpty,
+	deepCopy,
+	jsonParse,
+	jsonStringify,
+	sleep,
+	fileTypeFromMimeType,
+	assert,
+} from './utils';
 export {
 	isINodeProperties,
 	isINodePropertyOptions,

packages/workflow/src/utils.ts

@@ -1,5 +1,19 @@
 import type { BinaryFileType } from './Interfaces';
 
+const readStreamClasses = new Set(['ReadStream', 'Readable', 'ReadableStream']);
+
+export const isObjectEmpty = (obj: object | null | undefined): boolean => {
+	if (obj === undefined || obj === null) return true;
+	if (typeof obj === 'object') {
+		if (Array.isArray(obj)) return obj.length === 0;
+		if (obj instanceof Set || obj instanceof Map) return obj.size === 0;
+		if (ArrayBuffer.isView(obj) || obj instanceof ArrayBuffer) return obj.byteLength === 0;
+		if (Symbol.iterator in obj || readStreamClasses.has(obj.constructor.name)) return false;
+		return Object.keys(obj).length === 0;
+	}
+	return true;
+};
+
 export type Primitives = string | number | boolean | bigint | symbol | null | undefined;
 
 /* eslint-disable @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-argument */

packages/workflow/test/utils.test.ts

@@ -1,4 +1,77 @@
-import { jsonParse, jsonStringify, deepCopy } from '@/utils';
+import { jsonParse, jsonStringify, deepCopy, isObjectEmpty } from '@/utils';
+
+describe('isObjectEmpty', () => {
+	it('should handle null and undefined', () => {
+		expect(isObjectEmpty(null)).toEqual(true);
+		expect(isObjectEmpty(undefined)).toEqual(true);
+	});
+
+	it('should handle arrays', () => {
+		expect(isObjectEmpty([])).toEqual(true);
+		expect(isObjectEmpty([1, 2, 3])).toEqual(false);
+	});
+
+	it('should handle Set and Map', () => {
+		expect(isObjectEmpty(new Set())).toEqual(true);
+		expect(isObjectEmpty(new Set([1, 2, 3]))).toEqual(false);
+
+		expect(isObjectEmpty(new Map())).toEqual(true);
+		expect(
+			isObjectEmpty(
+				new Map([
+					['a', 1],
+					['b', 2],
+				]),
+			),
+		).toEqual(false);
+	});
+
+	it('should handle Buffer, ArrayBuffer, and Uint8Array', () => {
+		expect(isObjectEmpty(Buffer.from(''))).toEqual(true);
+		expect(isObjectEmpty(Buffer.from('abcd'))).toEqual(false);
+
+		expect(isObjectEmpty(Uint8Array.from([]))).toEqual(true);
+		expect(isObjectEmpty(Uint8Array.from([1, 2, 3]))).toEqual(false);
+
+		expect(isObjectEmpty(new ArrayBuffer(0))).toEqual(true);
+		expect(isObjectEmpty(new ArrayBuffer(1))).toEqual(false);
+	});
+
+	it('should handle plain objects', () => {
+		expect(isObjectEmpty({})).toEqual(true);
+		expect(isObjectEmpty({ a: 1, b: 2 })).toEqual(false);
+	});
+
+	it('should handle instantiated classes', () => {
+		expect(isObjectEmpty(new (class Test {})())).toEqual(true);
+		expect(
+			isObjectEmpty(
+				new (class Test {
+					prop = 123;
+				})(),
+			),
+		).toEqual(false);
+	});
+
+	it('should not call Object.keys unless a plain object', () => {
+		const keySpy = jest.spyOn(Object, 'keys');
+		const { calls } = keySpy.mock;
+
+		const assertCalls = (count: number) => {
+			if (calls.length !== count) throw new Error(`Object.keys was called ${calls.length} times`);
+		};
+
+		assertCalls(0);
+		isObjectEmpty(null);
+		assertCalls(0);
+		isObjectEmpty([1, 2, 3]);
+		assertCalls(0);
+		isObjectEmpty(Buffer.from('123'));
+		assertCalls(0);
+		isObjectEmpty({});
+		assertCalls(1);
+	});
+});
 
 describe('jsonParse', () => {
 	it('parses JSON', () => {