From e810131c20922fd917342a2cb4af33e7218dc398 Mon Sep 17 00:00:00 2001 From: Valya Bullions Date: Thu, 6 Apr 2023 11:44:21 +0100 Subject: [PATCH 1/5] fix(core): Validate customData keys and values Throws errors in manual mode and ignores and logs values in production --- packages/core/src/NodeExecuteFunctions.ts | 18 ++++++++-- .../core/src/WorkflowExecutionMetadata.ts | 34 ++++++++++++++++--- .../test/WorkflowExecutionMetadata.test.ts | 32 +++++++++++++++-- 3 files changed, 75 insertions(+), 9 deletions(-) diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index 846338e6f1a59..0315a6999cd95 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/packages/core/src/NodeExecuteFunctions.ts @@ -1644,10 +1644,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): 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); diff --git a/packages/core/src/WorkflowExecutionMetadata.ts b/packages/core/src/WorkflowExecutionMetadata.ts index 146d5418d20d3..a3c616470715e 100644 --- a/packages/core/src/WorkflowExecutionMetadata.ts +++ b/packages/core/src/WorkflowExecutionMetadata.ts @@ -2,6 +2,18 @@ import type { IRunExecutionData } from 'n8n-workflow'; export const KV_LIMIT = 10; +export class WorkflowMetadataValidationError extends Error { + constructor( + public type: 'key' | 'value', + key: unknown, + message?: string, + options?: ErrorOptions, + ) { + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + super(`Custom data ${type}s must be a string (key "${key}")`, options); + } +} + export function setWorkflowExecutionMetadata( executionData: IRunExecutionData, key: string, @@ -17,16 +29,30 @@ export function setWorkflowExecutionMetadata( ) { return; } - executionData.resultData.metadata[String(key).slice(0, 50)] = String(value).slice(0, 255); + if (typeof key !== 'string') { + throw new WorkflowMetadataValidationError('key', key); + } + if (typeof value !== 'string') { + throw new WorkflowMetadataValidationError('value', key); + } + executionData.resultData.metadata[key.slice(0, 50)] = value.slice(0, 255); } export function setAllWorkflowExecutionMetadata( executionData: IRunExecutionData, obj: Record, ) { - 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( diff --git a/packages/core/test/WorkflowExecutionMetadata.test.ts b/packages/core/test/WorkflowExecutionMetadata.test.ts index 1c1ee49bf288b..8429a01f1aa2f 100644 --- a/packages/core/test/WorkflowExecutionMetadata.test.ts +++ b/packages/core/test/WorkflowExecutionMetadata.test.ts @@ -4,6 +4,7 @@ import { KV_LIMIT, setAllWorkflowExecutionMetadata, setWorkflowExecutionMetadata, + WorkflowMetadataValidationError, } from '@/WorkflowExecutionMetadata'; import type { IRunExecutionData } from 'n8n-workflow'; @@ -42,7 +43,7 @@ describe('Execution Metadata functions', () => { }); }); - test('setWorkflowExecutionMetadata should convert values to strings', () => { + test('setWorkflowExecutionMetadata should not convert values to strings', () => { const metadata = {}; const executionData = { resultData: { @@ -50,13 +51,38 @@ describe('Execution Metadata functions', () => { }, } as IRunExecutionData; - setWorkflowExecutionMetadata(executionData, 'test1', 1234); + expect(() => setWorkflowExecutionMetadata(executionData, 'test1', 1234)).toThrow( + WorkflowMetadataValidationError, + ); - expect(metadata).toEqual({ + expect(metadata).not.toEqual({ test1: '1234', }); }); + 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: 1234 as unknown as string, + test2: [] as unknown as string, + test3: 'value3', + test4: 'value4', + }), + ).toThrow(WorkflowMetadataValidationError); + + expect(metadata).toEqual({ + test3: 'value3', + test4: 'value4', + }); + }); + test('setWorkflowExecutionMetadata should limit the number of metadata entries', () => { const metadata = {}; const executionData = { From 2168560f5e99a7da8596c8e0676c6415aeee478c Mon Sep 17 00:00:00 2001 From: Valya Bullions Date: Thu, 6 Apr 2023 12:25:30 +0100 Subject: [PATCH 2/5] fix: validate customData key characters --- packages/core/src/WorkflowExecutionMetadata.ts | 9 ++++++++- .../core/test/WorkflowExecutionMetadata.test.ts | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/core/src/WorkflowExecutionMetadata.ts b/packages/core/src/WorkflowExecutionMetadata.ts index a3c616470715e..a0f433152f4be 100644 --- a/packages/core/src/WorkflowExecutionMetadata.ts +++ b/packages/core/src/WorkflowExecutionMetadata.ts @@ -10,7 +10,7 @@ export class WorkflowMetadataValidationError extends Error { options?: ErrorOptions, ) { // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - super(`Custom data ${type}s must be a string (key "${key}")`, options); + super(message ?? `Custom data ${type}s must be a string (key "${key}")`, options); } } @@ -32,6 +32,13 @@ export function setWorkflowExecutionMetadata( if (typeof key !== 'string') { throw new WorkflowMetadataValidationError('key', key); } + if (key.replace(/[A-Za-z0-9_]/g, '').length !== 0) { + throw new WorkflowMetadataValidationError( + 'key', + key, + `Custom date key can only contain characters "A-Za-z0-9_" (key "${key}")`, + ); + } if (typeof value !== 'string') { throw new WorkflowMetadataValidationError('value', key); } diff --git a/packages/core/test/WorkflowExecutionMetadata.test.ts b/packages/core/test/WorkflowExecutionMetadata.test.ts index 8429a01f1aa2f..24f1b844574eb 100644 --- a/packages/core/test/WorkflowExecutionMetadata.test.ts +++ b/packages/core/test/WorkflowExecutionMetadata.test.ts @@ -83,6 +83,23 @@ describe('Execution Metadata functions', () => { }); }); + test('setWorkflowExecutionMetadata should validate key characters', () => { + const metadata = {}; + const executionData = { + resultData: { + metadata, + }, + } as IRunExecutionData; + + expect(() => setWorkflowExecutionMetadata(executionData, 'te$t1$', 1234)).toThrow( + WorkflowMetadataValidationError, + ); + + expect(metadata).not.toEqual({ + test1: '1234', + }); + }); + test('setWorkflowExecutionMetadata should limit the number of metadata entries', () => { const metadata = {}; const executionData = { From d2219b28583479cb6f484713553f7705f5f64fdd Mon Sep 17 00:00:00 2001 From: Valya Bullions Date: Thu, 6 Apr 2023 14:43:22 +0100 Subject: [PATCH 3/5] refactor: review changes --- packages/core/src/WorkflowExecutionMetadata.ts | 15 +++++++++++---- .../core/test/WorkflowExecutionMetadata.test.ts | 8 ++++---- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/packages/core/src/WorkflowExecutionMetadata.ts b/packages/core/src/WorkflowExecutionMetadata.ts index a0f433152f4be..8c5b43edaa44e 100644 --- a/packages/core/src/WorkflowExecutionMetadata.ts +++ b/packages/core/src/WorkflowExecutionMetadata.ts @@ -1,8 +1,9 @@ import type { IRunExecutionData } from 'n8n-workflow'; +import { LoggerProxy as Logger } from 'n8n-workflow'; export const KV_LIMIT = 10; -export class WorkflowMetadataValidationError extends Error { +export class ExecutionMetadataValidationError extends Error { constructor( public type: 'key' | 'value', key: unknown, @@ -30,17 +31,23 @@ export function setWorkflowExecutionMetadata( return; } if (typeof key !== 'string') { - throw new WorkflowMetadataValidationError('key', key); + throw new ExecutionMetadataValidationError('key', key); } if (key.replace(/[A-Za-z0-9_]/g, '').length !== 0) { - throw new WorkflowMetadataValidationError( + throw new ExecutionMetadataValidationError( 'key', key, `Custom date key can only contain characters "A-Za-z0-9_" (key "${key}")`, ); } if (typeof value !== 'string') { - throw new WorkflowMetadataValidationError('value', key); + throw new ExecutionMetadataValidationError('value', key); + } + if (key.length > 50) { + Logger.error('Custom data key over 50 characters long. Truncating to 50 characters.'); + } + if (value.length > 255) { + Logger.error('Custom data value over 255 characters long. Truncating to 255 characters.'); } executionData.resultData.metadata[key.slice(0, 50)] = value.slice(0, 255); } diff --git a/packages/core/test/WorkflowExecutionMetadata.test.ts b/packages/core/test/WorkflowExecutionMetadata.test.ts index 24f1b844574eb..5340f4143fe55 100644 --- a/packages/core/test/WorkflowExecutionMetadata.test.ts +++ b/packages/core/test/WorkflowExecutionMetadata.test.ts @@ -4,7 +4,7 @@ import { KV_LIMIT, setAllWorkflowExecutionMetadata, setWorkflowExecutionMetadata, - WorkflowMetadataValidationError, + ExecutionMetadataValidationError, } from '@/WorkflowExecutionMetadata'; import type { IRunExecutionData } from 'n8n-workflow'; @@ -52,7 +52,7 @@ describe('Execution Metadata functions', () => { } as IRunExecutionData; expect(() => setWorkflowExecutionMetadata(executionData, 'test1', 1234)).toThrow( - WorkflowMetadataValidationError, + ExecutionMetadataValidationError, ); expect(metadata).not.toEqual({ @@ -75,7 +75,7 @@ describe('Execution Metadata functions', () => { test3: 'value3', test4: 'value4', }), - ).toThrow(WorkflowMetadataValidationError); + ).toThrow(ExecutionMetadataValidationError); expect(metadata).toEqual({ test3: 'value3', @@ -92,7 +92,7 @@ describe('Execution Metadata functions', () => { } as IRunExecutionData; expect(() => setWorkflowExecutionMetadata(executionData, 'te$t1$', 1234)).toThrow( - WorkflowMetadataValidationError, + ExecutionMetadataValidationError, ); expect(metadata).not.toEqual({ From 6c6f58e36034bbe846b0b8dd2f32ea97cb025c8f Mon Sep 17 00:00:00 2001 From: Valya Bullions Date: Thu, 6 Apr 2023 14:45:27 +0100 Subject: [PATCH 4/5] fix: logger not initialised for metadata tests --- .../core/test/WorkflowExecutionMetadata.test.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/core/test/WorkflowExecutionMetadata.test.ts b/packages/core/test/WorkflowExecutionMetadata.test.ts index 5340f4143fe55..ce368171cf241 100644 --- a/packages/core/test/WorkflowExecutionMetadata.test.ts +++ b/packages/core/test/WorkflowExecutionMetadata.test.ts @@ -6,7 +6,20 @@ import { 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', () => { From 86561728f99f63be98e5399082d6aaaf54497633 Mon Sep 17 00:00:00 2001 From: Valya Bullions Date: Tue, 11 Apr 2023 16:53:17 +0100 Subject: [PATCH 5/5] fix: allow numbers for values --- packages/core/src/WorkflowExecutionMetadata.ts | 7 ++++--- .../core/test/WorkflowExecutionMetadata.test.ts | 15 ++++++++++++--- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/packages/core/src/WorkflowExecutionMetadata.ts b/packages/core/src/WorkflowExecutionMetadata.ts index 8c5b43edaa44e..a6c186a20dd89 100644 --- a/packages/core/src/WorkflowExecutionMetadata.ts +++ b/packages/core/src/WorkflowExecutionMetadata.ts @@ -40,16 +40,17 @@ export function setWorkflowExecutionMetadata( `Custom date key can only contain characters "A-Za-z0-9_" (key "${key}")`, ); } - if (typeof value !== 'string') { + 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 (value.length > 255) { + if (val.length > 255) { Logger.error('Custom data value over 255 characters long. Truncating to 255 characters.'); } - executionData.resultData.metadata[key.slice(0, 50)] = value.slice(0, 255); + executionData.resultData.metadata[key.slice(0, 50)] = val.slice(0, 255); } export function setAllWorkflowExecutionMetadata( diff --git a/packages/core/test/WorkflowExecutionMetadata.test.ts b/packages/core/test/WorkflowExecutionMetadata.test.ts index ce368171cf241..dc99effede13a 100644 --- a/packages/core/test/WorkflowExecutionMetadata.test.ts +++ b/packages/core/test/WorkflowExecutionMetadata.test.ts @@ -56,7 +56,7 @@ describe('Execution Metadata functions', () => { }); }); - test('setWorkflowExecutionMetadata should not convert values to strings', () => { + test('setWorkflowExecutionMetadata should only convert numbers to strings', () => { const metadata = {}; const executionData = { resultData: { @@ -64,12 +64,21 @@ describe('Execution Metadata functions', () => { }, } as IRunExecutionData; - expect(() => setWorkflowExecutionMetadata(executionData, 'test1', 1234)).toThrow( + 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: {}, }); }); @@ -83,7 +92,7 @@ describe('Execution Metadata functions', () => { expect(() => setAllWorkflowExecutionMetadata(executionData, { - test1: 1234 as unknown as string, + test1: {} as unknown as string, test2: [] as unknown as string, test3: 'value3', test4: 'value4',