From 1197811a1e3bc4ad7464d53d7e4860d0e62335a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Thu, 8 Jun 2023 11:06:09 +0200 Subject: [PATCH] fix(core)!: Allow syntax errors and expression errors to fail executions (#6352) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: Unify expression error behavior for v1 * fix: Add `package.json` to `tsconfig.build.json` * fix: Make `isFrontend` a constant * fix: Use CommonJS require to read version * fix: Use `JSON.parse()` and `fs.readFileSync()` * feat(editor): Make WF name a link on /executions (#6354) * make wf name a link in exec view * link color * make wf name a link in exec view * link color --------- Co-authored-by: Alex Grozav * fix: Try restoring inclusions in tsconfig files * fix: Try with copy * refactor: Switch base branch and remove global toggle * chore: Remove unrelated changes * chore: Restore lockfile * fix: Ensure all expression errors fail executions * uncaught ExpressionErrors should not fail e2e tests --------- Co-authored-by: romainminaud Co-authored-by: Alex Grozav Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- cypress/e2e/11-inline-expression-editor.cy.ts | 2 + cypress/e2e/9-expression-editor-modal.cy.ts | 2 + packages/core/src/NodeExecuteFunctions.ts | 1 - packages/workflow/src/Expression.ts | 42 +++---------------- packages/workflow/src/ExpressionError.ts | 17 +------- packages/workflow/src/WorkflowDataProxy.ts | 7 ---- packages/workflow/src/utils.ts | 18 ++++++++ packages/workflow/test/Expression.test.ts | 18 +++++++- 8 files changed, 45 insertions(+), 62 deletions(-) diff --git a/cypress/e2e/11-inline-expression-editor.cy.ts b/cypress/e2e/11-inline-expression-editor.cy.ts index 929c0721df..4a4e3794ef 100644 --- a/cypress/e2e/11-inline-expression-editor.cy.ts +++ b/cypress/e2e/11-inline-expression-editor.cy.ts @@ -21,6 +21,8 @@ describe('Inline expression editor', () => { WorkflowPage.actions.addNodeToCanvas('Hacker News'); WorkflowPage.actions.openNode('Hacker News'); WorkflowPage.actions.openInlineExpressionEditor(); + + cy.on('uncaught:exception', (err) => err.name !== 'ExpressionError'); }); it('should resolve primitive resolvables', () => { diff --git a/cypress/e2e/9-expression-editor-modal.cy.ts b/cypress/e2e/9-expression-editor-modal.cy.ts index 6f5ad04b63..c50309ae12 100644 --- a/cypress/e2e/9-expression-editor-modal.cy.ts +++ b/cypress/e2e/9-expression-editor-modal.cy.ts @@ -21,6 +21,8 @@ describe('Expression editor modal', () => { WorkflowPage.actions.addNodeToCanvas('Hacker News'); WorkflowPage.actions.openNode('Hacker News'); WorkflowPage.actions.openExpressionEditorModal(); + + cy.on('uncaught:exception', (err) => err.name !== 'ExpressionError'); }); it('should resolve primitive resolvables', () => { diff --git a/packages/core/src/NodeExecuteFunctions.ts b/packages/core/src/NodeExecuteFunctions.ts index 16f8f7b691..2835045425 100644 --- a/packages/core/src/NodeExecuteFunctions.ts +++ b/packages/core/src/NodeExecuteFunctions.ts @@ -1982,7 +1982,6 @@ const validateValueAgainstSchema = ( }' [item ${itemIndex}]`, { description: validationResult.errorMessage, - failExecution: true, runIndex, itemIndex, nodeCause: node.name, diff --git a/packages/workflow/src/Expression.ts b/packages/workflow/src/Expression.ts index 2d6aa4e14f..0e8842cf76 100644 --- a/packages/workflow/src/Expression.ts +++ b/packages/workflow/src/Expression.ts @@ -22,21 +22,14 @@ import type { Workflow } from './Workflow'; import { extend, extendOptional } from './Extensions'; import { extendedFunctions } from './Extensions/ExtendedFunctions'; import { extendSyntax } from './Extensions/ExpressionExtension'; +import { isExpressionError, IS_FRONTEND, isSyntaxError, isTypeError } from './utils'; // Set it to use double curly brackets instead of single ones tmpl.brackets.set('{{ }}'); // Make sure that error get forwarded tmpl.tmpl.errorHandler = (error: Error) => { - if (error instanceof ExpressionError) { - if (error.context.failExecution) { - throw error; - } - - if (typeof process === 'undefined' && error.clientOnly) { - throw error; - } - } + if (isExpressionError(error)) throw error; }; // eslint-disable-next-line @typescript-eslint/naming-convention @@ -332,35 +325,11 @@ export class Expression { ); return tmpl.tmpl(expression, data); } catch (error) { - if (error instanceof ExpressionError) { - // Ignore all errors except if they are ExpressionErrors and they are supposed - // to fail the execution - if (error.context.failExecution) { - throw error; - } + if (isExpressionError(error)) throw error; - if (typeof process === 'undefined' && error.clientOnly) { - throw error; - } - } + if (isSyntaxError(error)) throw new Error('invalid syntax'); - // Syntax errors resolve to `Error` on the frontend and `null` on the backend. - // This is a temporary divergence in evaluation behavior until we make the - // breaking change to allow syntax errors to fail executions. - if ( - typeof process === 'undefined' && - error instanceof Error && - error.name === 'SyntaxError' - ) { - throw new Error('invalid syntax'); - } - - if ( - typeof process === 'undefined' && - error instanceof Error && - error.name === 'TypeError' && - error.message.endsWith('is not a function') - ) { + if (isTypeError(error) && IS_FRONTEND && error.message.endsWith('is not a function')) { const match = error.message.match(/(?[^.]+is not a function)/); if (!match?.groups?.msg) return null; @@ -373,6 +342,7 @@ export class Expression { value: fnConstructors.async, }); } + return null; } diff --git a/packages/workflow/src/ExpressionError.ts b/packages/workflow/src/ExpressionError.ts index c7540ccd6f..7e06065424 100644 --- a/packages/workflow/src/ExpressionError.ts +++ b/packages/workflow/src/ExpressionError.ts @@ -5,8 +5,6 @@ import { ExecutionBaseError } from './NodeErrors'; * Class for instantiating an expression error */ export class ExpressionError extends ExecutionBaseError { - clientOnly = false; - constructor( message: string, options?: { @@ -14,8 +12,6 @@ export class ExpressionError extends ExecutionBaseError { causeDetailed?: string; description?: string; descriptionTemplate?: string; - failExecution?: boolean; - clientOnly?: boolean; // whether to throw error only on frontend functionality?: 'pairedItem'; itemIndex?: number; messageTemplate?: string; @@ -31,12 +27,6 @@ export class ExpressionError extends ExecutionBaseError { this.description = options.description; } - if (options?.clientOnly) { - this.clientOnly = options.clientOnly; - } - - this.context.failExecution = !!options?.failExecution; - const allowedKeys = [ 'causeDetailed', 'descriptionTemplate', @@ -58,9 +48,4 @@ export class ExpressionError extends ExecutionBaseError { } } -export class ExpressionExtensionError extends ExpressionError { - constructor(message: string) { - super(message); - this.context.failExecution = true; - } -} +export class ExpressionExtensionError extends ExpressionError {} diff --git a/packages/workflow/src/WorkflowDataProxy.ts b/packages/workflow/src/WorkflowDataProxy.ts index 24527df845..913dda6da7 100644 --- a/packages/workflow/src/WorkflowDataProxy.ts +++ b/packages/workflow/src/WorkflowDataProxy.ts @@ -468,7 +468,6 @@ export class WorkflowDataProxy { throw new ExpressionError('not accessible via UI, please run node', { runIndex: that.runIndex, itemIndex: that.itemIndex, - failExecution: true, }); } if (process.env.N8N_BLOCK_ENV_ACCESS_IN_NODE === 'true') { @@ -477,7 +476,6 @@ export class WorkflowDataProxy { 'If you need access please contact the administrator to remove the environment variable ‘N8N_BLOCK_ENV_ACCESS_IN_NODE‘', runIndex: that.runIndex, itemIndex: that.itemIndex, - failExecution: true, }); } return process.env[name.toString()]; @@ -560,7 +558,6 @@ export class WorkflowDataProxy { description: 'Please save the workflow first to use $workflow', runIndex: that.runIndex, itemIndex: that.itemIndex, - failExecution: true, }); } @@ -592,8 +589,6 @@ export class WorkflowDataProxy { throw new ExpressionError(`"${nodeName}" node doesn't exist`, { runIndex: that.runIndex, itemIndex: that.itemIndex, - // TODO: re-enable this for v1.0.0 release - // failExecution: true, }); } @@ -630,7 +625,6 @@ export class WorkflowDataProxy { throw new ExpressionError('expected two arguments (Object, string) for this function', { runIndex: that.runIndex, itemIndex: that.itemIndex, - clientOnly: true, }); } @@ -696,7 +690,6 @@ export class WorkflowDataProxy { return new ExpressionError(message, { runIndex: that.runIndex, itemIndex: that.itemIndex, - failExecution: true, ...context, }); }; diff --git a/packages/workflow/src/utils.ts b/packages/workflow/src/utils.ts index 4db97aac10..ad4b7750c1 100644 --- a/packages/workflow/src/utils.ts +++ b/packages/workflow/src/utils.ts @@ -1,3 +1,4 @@ +import { ExpressionError, ExpressionExtensionError } from './ExpressionError'; import type { BinaryFileType } from './Interfaces'; const readStreamClasses = new Set(['ReadStream', 'Readable', 'ReadableStream']); @@ -127,3 +128,20 @@ export function assert(condition: T, msg?: string): asserts condition { throw error; } } + +const IS_FRONTEND_IN_DEV_MODE = + typeof process === 'object' && + Object.keys(process).length === 1 && + 'env' in process && + Object.keys(process.env).length === 0; + +export const IS_FRONTEND = typeof process === 'undefined' || IS_FRONTEND_IN_DEV_MODE; + +export const isSyntaxError = (error: unknown): error is SyntaxError => + error instanceof SyntaxError || (error instanceof Error && error.name === 'SyntaxError'); + +export const isExpressionError = (error: unknown): error is ExpressionError => + error instanceof ExpressionError || error instanceof ExpressionExtensionError; + +export const isTypeError = (error: unknown): error is TypeError => + error instanceof TypeError || (error instanceof Error && error.name === 'TypeError'); diff --git a/packages/workflow/test/Expression.test.ts b/packages/workflow/test/Expression.test.ts index 5229361440..bcfc44f613 100644 --- a/packages/workflow/test/Expression.test.ts +++ b/packages/workflow/test/Expression.test.ts @@ -10,6 +10,7 @@ import type { ExpressionTestEvaluation, ExpressionTestTransform } from './Expres import { baseFixtures } from './ExpressionFixtures/base'; import type { INodeExecutionData } from '@/Interfaces'; import { extendSyntax } from '@/Extensions/ExpressionExtension'; +import { ExpressionError } from '@/ExpressionError'; describe('Expression', () => { describe('getParameterValue()', () => { @@ -154,7 +155,9 @@ describe('Expression', () => { it('should not able to do arbitrary code execution', () => { const testFn = jest.fn(); Object.assign(global, { testFn }); - evaluate("={{ Date['constructor']('testFn()')()}}"); + expect(() => evaluate("={{ Date['constructor']('testFn()')()}}")).toThrowError( + new ExpressionError('Arbitrary code execution detected'), + ); expect(testFn).not.toHaveBeenCalled(); }); }); @@ -180,7 +183,18 @@ describe('Expression', () => { const expression = new Expression(workflow); const evaluate = (value: string, data: INodeExecutionData[]) => { - return expression.getParameterValue(value, null, 0, 0, 'node', data, 'manual', '', {}); + const itemIndex = data.length === 0 ? -1 : 0; + return expression.getParameterValue( + value, + null, + 0, + itemIndex, + 'node', + data, + 'manual', + '', + {}, + ); }; for (const t of baseFixtures) {