From 323e26acfd0fe07b754c95d765a62481593472f5 Mon Sep 17 00:00:00 2001 From: Val <68596159+valya@users.noreply.github.com> Date: Wed, 12 Apr 2023 09:18:26 +0100 Subject: [PATCH] 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 | 18 ++++- .../core/src/WorkflowExecutionMetadata.ts | 49 +++++++++++-- .../test/WorkflowExecutionMetadata.test.ts | 71 ++++++++++++++++++- 3 files changed, 129 insertions(+), 9 deletions(-) diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index 681d2228f0..e5db3e8d6c 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/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): 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 146d5418d2..a6c186a20d 100644 --- a/packages/core/src/WorkflowExecutionMetadata.ts +++ b/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, ) { - 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 1c1ee49bf2..dc99effede 100644 --- a/packages/core/test/WorkflowExecutionMetadata.test.ts +++ b/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', () => {