fix(core): Validate customData keys and values (#5920) (no-changelog)

* fix(core): Validate customData keys and values

Throws errors in manual mode and ignores and logs values in production

* fix: validate customData key characters

* refactor: review changes

* fix: logger not initialised for metadata tests

* fix: allow numbers for values

packages/core/src/NodeExecuteFunctions.ts

@@ -1653,10 +1653,24 @@ export function getAdditionalKeys(
 			customData: runExecutionData
 				? {
 						set(key: string, value: string): void {
-							setWorkflowExecutionMetadata(runExecutionData, key, value);
+							try {
+								setWorkflowExecutionMetadata(runExecutionData, key, value);
+							} catch (e) {
+								if (mode === 'manual') {
+									throw e;
+								}
+								Logger.verbose(e.message);
+							}
 						},
 						setAll(obj: Record<string, string>): void {
-							setAllWorkflowExecutionMetadata(runExecutionData, obj);
+							try {
+								setAllWorkflowExecutionMetadata(runExecutionData, obj);
+							} catch (e) {
+								if (mode === 'manual') {
+									throw e;
+								}
+								Logger.verbose(e.message);
+							}
 						},
 						get(key: string): string {
 							return getWorkflowExecutionMetadata(runExecutionData, key);

packages/core/src/WorkflowExecutionMetadata.ts

@@ -1,7 +1,20 @@
 import type { IRunExecutionData } from 'n8n-workflow';
+import { LoggerProxy as Logger } from 'n8n-workflow';
 
 export const KV_LIMIT = 10;
 
+export class ExecutionMetadataValidationError extends Error {
+	constructor(
+		public type: 'key' | 'value',
+		key: unknown,
+		message?: string,
+		options?: ErrorOptions,
+	) {
+		// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
+		super(message ?? `Custom data ${type}s must be a string (key "${key}")`, options);
+	}
+}
+
 export function setWorkflowExecutionMetadata(
 	executionData: IRunExecutionData,
 	key: string,
@@ -17,16 +30,44 @@ export function setWorkflowExecutionMetadata(
 	) {
 		return;
 	}
-	executionData.resultData.metadata[String(key).slice(0, 50)] = String(value).slice(0, 255);
+	if (typeof key !== 'string') {
+		throw new ExecutionMetadataValidationError('key', key);
+	}
+	if (key.replace(/[A-Za-z0-9_]/g, '').length !== 0) {
+		throw new ExecutionMetadataValidationError(
+			'key',
+			key,
+			`Custom date key can only contain characters "A-Za-z0-9_" (key "${key}")`,
+		);
+	}
+	if (typeof value !== 'string' && typeof value !== 'number' && typeof value !== 'bigint') {
+		throw new ExecutionMetadataValidationError('value', key);
+	}
+	const val = String(value);
+	if (key.length > 50) {
+		Logger.error('Custom data key over 50 characters long. Truncating to 50 characters.');
+	}
+	if (val.length > 255) {
+		Logger.error('Custom data value over 255 characters long. Truncating to 255 characters.');
+	}
+	executionData.resultData.metadata[key.slice(0, 50)] = val.slice(0, 255);
 }
 
 export function setAllWorkflowExecutionMetadata(
 	executionData: IRunExecutionData,
 	obj: Record<string, string>,
 ) {
-	Object.entries(obj).forEach(([key, value]) =>
-		setWorkflowExecutionMetadata(executionData, key, value),
-	);
+	const errors: Error[] = [];
+	Object.entries(obj).forEach(([key, value]) => {
+		try {
+			setWorkflowExecutionMetadata(executionData, key, value);
+		} catch (e) {
+			errors.push(e as Error);
+		}
+	});
+	if (errors.length) {
+		throw errors[0];
+	}
 }
 
 export function getAllWorkflowExecutionMetadata(

packages/core/test/WorkflowExecutionMetadata.test.ts

@@ -4,8 +4,22 @@ import {
 	KV_LIMIT,
 	setAllWorkflowExecutionMetadata,
 	setWorkflowExecutionMetadata,
+	ExecutionMetadataValidationError,
 } from '@/WorkflowExecutionMetadata';
-import type { IRunExecutionData } from 'n8n-workflow';
+import { LoggerProxy } from 'n8n-workflow';
+import type { ILogger, IRunExecutionData } from 'n8n-workflow';
+
+beforeAll(() => {
+	const fakeLogger = {
+		log: () => {},
+		debug: () => {},
+		verbose: () => {},
+		info: () => {},
+		warn: () => {},
+		error: () => {},
+	} as ILogger;
+	LoggerProxy.init(fakeLogger);
+});
 
 describe('Execution Metadata functions', () => {
 	test('setWorkflowExecutionMetadata will set a value', () => {
@@ -42,7 +56,7 @@ describe('Execution Metadata functions', () => {
 		});
 	});
 
-	test('setWorkflowExecutionMetadata should convert values to strings', () => {
+	test('setWorkflowExecutionMetadata should only convert numbers to strings', () => {
 		const metadata = {};
 		const executionData = {
 			resultData: {
@@ -50,11 +64,62 @@ describe('Execution Metadata functions', () => {
 			},
 		} as IRunExecutionData;
 
-		setWorkflowExecutionMetadata(executionData, 'test1', 1234);
+		expect(() => setWorkflowExecutionMetadata(executionData, 'test1', 1234)).not.toThrow(
+			ExecutionMetadataValidationError,
+		);
 
 		expect(metadata).toEqual({
 			test1: '1234',
 		});
+
+		expect(() => setWorkflowExecutionMetadata(executionData, 'test2', {})).toThrow(
+			ExecutionMetadataValidationError,
+		);
+
+		expect(metadata).not.toEqual({
+			test1: '1234',
+			test2: {},
+		});
+	});
+
+	test('setAllWorkflowExecutionMetadata should not convert values to strings and should set other values correctly', () => {
+		const metadata = {};
+		const executionData = {
+			resultData: {
+				metadata,
+			},
+		} as IRunExecutionData;
+
+		expect(() =>
+			setAllWorkflowExecutionMetadata(executionData, {
+				test1: {} as unknown as string,
+				test2: [] as unknown as string,
+				test3: 'value3',
+				test4: 'value4',
+			}),
+		).toThrow(ExecutionMetadataValidationError);
+
+		expect(metadata).toEqual({
+			test3: 'value3',
+			test4: 'value4',
+		});
+	});
+
+	test('setWorkflowExecutionMetadata should validate key characters', () => {
+		const metadata = {};
+		const executionData = {
+			resultData: {
+				metadata,
+			},
+		} as IRunExecutionData;
+
+		expect(() => setWorkflowExecutionMetadata(executionData, 'te$t1$', 1234)).toThrow(
+			ExecutionMetadataValidationError,
+		);
+
+		expect(metadata).not.toEqual({
+			test1: '1234',
+		});
 	});
 
 	test('setWorkflowExecutionMetadata should limit the number of metadata entries', () => {