From b7dcfc84cb9b19cd5b1f3190202cb37ea9d79079 Mon Sep 17 00:00:00 2001 From: "Guillaume F." Date: Fri, 24 Jul 2026 11:13:19 +0200 Subject: [PATCH] feat(core): Add no-silent-error-swallowing community node lint rule (no-changelog) (#34780) Co-authored-by: Claude Opus 4.8 (1M context) --- .../eslint-plugin-community-nodes/README.md | 1 + .../docs/rules/no-silent-error-swallowing.md | 72 +++++ .../src/plugin.ts | 2 + .../src/rules/index.ts | 2 + .../rules/no-silent-error-swallowing.test.ts | 245 ++++++++++++++++++ .../src/rules/no-silent-error-swallowing.ts | 127 +++++++++ .../src/rules/webhook-lifecycle-complete.ts | 9 +- .../src/utils/constants.ts | 21 ++ .../src/utils/index.ts | 1 + 9 files changed, 475 insertions(+), 5 deletions(-) create mode 100644 packages/@n8n/eslint-plugin-community-nodes/docs/rules/no-silent-error-swallowing.md create mode 100644 packages/@n8n/eslint-plugin-community-nodes/src/rules/no-silent-error-swallowing.test.ts create mode 100644 packages/@n8n/eslint-plugin-community-nodes/src/rules/no-silent-error-swallowing.ts create mode 100644 packages/@n8n/eslint-plugin-community-nodes/src/utils/constants.ts diff --git a/packages/@n8n/eslint-plugin-community-nodes/README.md b/packages/@n8n/eslint-plugin-community-nodes/README.md index 57d51e0162a..1ce8f1e0cdc 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/README.md +++ b/packages/@n8n/eslint-plugin-community-nodes/README.md @@ -67,6 +67,7 @@ export default [ | [no-restricted-globals](docs/rules/no-restricted-globals.md) | Disallow usage of restricted global variables in community nodes. | ✅ | | | | | | [no-restricted-imports](docs/rules/no-restricted-imports.md) | Disallow usage of restricted imports in community nodes. | ✅ | | | | | | [no-runtime-dependencies](docs/rules/no-runtime-dependencies.md) | Disallow non-empty "dependencies" in community node package.json | ✅ ☑️ | | | | | +| [no-silent-error-swallowing](docs/rules/no-silent-error-swallowing.md) | Disallow webhook lifecycle methods (checkExists, create, delete) from silently swallowing errors in catch blocks | ✅ ☑️ | | | | | | [no-template-placeholders](docs/rules/no-template-placeholders.md) | Disallow unresolved template placeholders in package.json | ✅ ☑️ | | | | | | [node-class-description-icon-missing](docs/rules/node-class-description-icon-missing.md) | Node class description must have an `icon` property defined. Deprecated: use `require-node-description-fields` instead. | | | | 💡 | ❌ | | [node-connection-type-literal](docs/rules/node-connection-type-literal.md) | Disallow string literals in node description `inputs`/`outputs` — use `NodeConnectionTypes` enum instead | ✅ ☑️ | | 🔧 | | | diff --git a/packages/@n8n/eslint-plugin-community-nodes/docs/rules/no-silent-error-swallowing.md b/packages/@n8n/eslint-plugin-community-nodes/docs/rules/no-silent-error-swallowing.md new file mode 100644 index 00000000000..ec4d7013009 --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/docs/rules/no-silent-error-swallowing.md @@ -0,0 +1,72 @@ +# Disallow webhook lifecycle methods (checkExists, create, delete) from silently swallowing errors in catch blocks (`@n8n/community-nodes/no-silent-error-swallowing`) + +💼 This rule is enabled in the following configs: ✅ `recommended`, ☑️ `recommendedWithoutN8nCloudSupport`. + + + +## Rule Details + +Webhook trigger nodes register, verify, and clean up webhooks on a third-party +service through the `webhookMethods` lifecycle (`checkExists`, `create`, +`delete`). When these methods catch an error and silently discard it — an empty +`catch` block, or one that only returns `true`/`false` without logging or +rethrowing — real failures become invisible. The user sees a webhook that +"succeeded" while it was never registered, a stale webhook that is never cleaned +up, or a workflow that quietly stops firing with no diagnostic to explain why. + +This rule flags `catch` blocks inside the `checkExists`, `create`, and `delete` +methods of a node's `webhookMethods` that: + +- have an empty body, or +- contain only a `return true`, `return false`, or bare `return` statement. + +Handle the error instead: log it (so it surfaces in execution logs) and/or +rethrow it. A `catch` that logs before returning, rethrows, or returns a +computed value is allowed. + +## Examples + +### ❌ Incorrect + +```typescript +webhookMethods = { + default: { + async checkExists(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url }); + } catch (error) {} + }, + async create(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url }); + } catch (error) { + return false; + } + }, + }, +}; +``` + +### ✅ Correct + +```typescript +webhookMethods = { + default: { + async checkExists(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url }); + } catch (error) { + this.logger.error('checkExists failed', { error }); + return false; + } + }, + async create(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url }); + } catch (error) { + throw error; + } + }, + }, +}; +``` diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts b/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts index e797390b465..a1d8ffc53dc 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts @@ -36,6 +36,7 @@ const configs = { '@n8n/community-nodes/no-http-request-with-manual-auth': 'error', '@n8n/community-nodes/no-overrides-field': 'error', '@n8n/community-nodes/no-runtime-dependencies': 'error', + '@n8n/community-nodes/no-silent-error-swallowing': 'error', '@n8n/community-nodes/no-template-placeholders': 'error', '@n8n/community-nodes/icon-validation': 'error', '@n8n/community-nodes/icon-prefer-themed-variants': 'warn', @@ -78,6 +79,7 @@ const configs = { '@n8n/community-nodes/no-http-request-with-manual-auth': 'error', '@n8n/community-nodes/no-overrides-field': 'error', '@n8n/community-nodes/no-runtime-dependencies': 'error', + '@n8n/community-nodes/no-silent-error-swallowing': 'error', '@n8n/community-nodes/no-template-placeholders': 'error', '@n8n/community-nodes/icon-validation': 'error', '@n8n/community-nodes/icon-prefer-themed-variants': 'warn', diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts index a3bd8b5c158..4432af214fb 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts @@ -21,6 +21,7 @@ import { NoOverridesFieldRule } from './no-overrides-field.js'; import { NoRestrictedGlobalsRule } from './no-restricted-globals.js'; import { NoRestrictedImportsRule } from './no-restricted-imports.js'; import { NoRuntimeDependenciesRule } from './no-runtime-dependencies.js'; +import { NoSilentErrorSwallowingRule } from './no-silent-error-swallowing.js'; import { NoTemplatePlaceholdersRule } from './no-template-placeholders.js'; import { NodeClassDescriptionIconMissingRule } from './node-class-description-icon-missing.js'; import { NodeConnectionTypeLiteralRule } from './node-connection-type-literal.js'; @@ -54,6 +55,7 @@ export const rules = { 'no-http-request-with-manual-auth': NoHttpRequestWithManualAuthRule, 'no-overrides-field': NoOverridesFieldRule, 'no-runtime-dependencies': NoRuntimeDependenciesRule, + 'no-silent-error-swallowing': NoSilentErrorSwallowingRule, 'no-template-placeholders': NoTemplatePlaceholdersRule, 'icon-validation': IconValidationRule, 'icon-prefer-themed-variants': IconPreferThemedVariantsRule, diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/no-silent-error-swallowing.test.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/no-silent-error-swallowing.test.ts new file mode 100644 index 00000000000..95ee4062fc8 --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/no-silent-error-swallowing.test.ts @@ -0,0 +1,245 @@ +import { RuleTester } from '@typescript-eslint/rule-tester'; + +import { NoSilentErrorSwallowingRule } from './no-silent-error-swallowing.js'; + +const ruleTester = new RuleTester(); + +function createTriggerNode(webhookMethods: string): string { + return ` +import type { INodeType, INodeTypeDescription, IHookFunctions } from 'n8n-workflow'; + +export class TestTrigger implements INodeType { + description: INodeTypeDescription = { + displayName: 'Test Trigger', + name: 'testTrigger', + group: ['trigger'], + version: 1, + description: 'A test trigger', + defaults: { name: 'Test Trigger' }, + inputs: [], + outputs: ['main'], + webhooks: [{ name: 'default', httpMethod: 'POST', responseMode: 'onReceived', path: 'webhook' }], + properties: [], + }; + + webhookMethods = ${webhookMethods}; +}`; +} + +ruleTester.run('no-silent-error-swallowing', NoSilentErrorSwallowingRule, { + valid: [ + { + name: 'lifecycle methods with no catch blocks', + code: createTriggerNode(`{ + default: { + async checkExists(this: IHookFunctions): Promise { return true; }, + async create(this: IHookFunctions): Promise { return true; }, + async delete(this: IHookFunctions): Promise { return true; }, + }, + }`), + }, + { + name: 'catch block that logs and returns', + code: createTriggerNode(`{ + default: { + async checkExists(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) { + this.logger.error('checkExists failed', { error }); + return false; + } + }, + }, + }`), + }, + { + name: 'catch block that rethrows', + code: createTriggerNode(`{ + default: { + async create(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) { + throw error; + } + }, + }, + }`), + }, + { + name: 'catch block that returns a non-boolean expression', + code: createTriggerNode(`{ + default: { + async delete(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) { + return this.recover(error); + } + }, + }, + }`), + }, + { + name: 'silent catch inside a non-lifecycle method is ignored', + code: createTriggerNode(`{ + default: { + async someHelper(this: IHookFunctions): Promise { + try { + return true; + } catch (error) { + return false; + } + }, + }, + }`), + }, + { + name: 'silent catch inside a nested callback within a lifecycle method is ignored', + code: createTriggerNode(`{ + default: { + async create(this: IHookFunctions): Promise { + const results = items.map((item) => { + try { + return normalize(item); + } catch (error) { + return false; + } + }); + return results.length > 0; + }, + }, + }`), + }, + { + name: 'silent catch in a non-node-type class is ignored', + code: ` +export class RegularClass { + webhookMethods = { + default: { + async checkExists() { + try { + return true; + } catch (error) { + return false; + } + }, + }, + }; +}`, + }, + ], + invalid: [ + { + name: 'empty catch block in checkExists', + code: createTriggerNode(`{ + default: { + async checkExists(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) {} + }, + }, + }`), + errors: [{ messageId: 'emptyCatch', data: { method: 'checkExists' } }], + }, + { + name: 'catch block that only returns false in create', + code: createTriggerNode(`{ + default: { + async create(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) { + return false; + } + }, + }, + }`), + errors: [{ messageId: 'silentReturn', data: { method: 'create' } }], + }, + { + name: 'catch block that only returns true in delete', + code: createTriggerNode(`{ + default: { + async delete(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) { + return true; + } + }, + }, + }`), + errors: [{ messageId: 'silentReturn', data: { method: 'delete' } }], + }, + { + name: 'catch block with a bare return', + code: createTriggerNode(`{ + default: { + async checkExists(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) { + return; + } + }, + }, + }`), + errors: [{ messageId: 'silentReturn', data: { method: 'checkExists' } }], + }, + { + name: 'silent catch defined via arrow function method', + code: createTriggerNode(`{ + default: { + checkExists: async (this: IHookFunctions): Promise => { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) { + return false; + } + }, + }, + }`), + errors: [{ messageId: 'silentReturn', data: { method: 'checkExists' } }], + }, + { + name: 'flagged regardless of file name (webhookMethods is the signal)', + filename: 'GenericFunctions.ts', + code: createTriggerNode(`{ + default: { + async create(this: IHookFunctions): Promise { + try { + return await this.helpers.httpRequest({ url: 'https://example.com' }); + } catch (error) { + return false; + } + }, + }, + }`), + errors: [{ messageId: 'silentReturn', data: { method: 'create' } }], + }, + { + name: 'multiple silent lifecycle methods each flagged', + code: createTriggerNode(`{ + default: { + async checkExists(this: IHookFunctions): Promise { + try { return await this.helpers.httpRequest({ url: 'https://example.com' }); } catch (error) {} + }, + async create(this: IHookFunctions): Promise { + try { return await this.helpers.httpRequest({ url: 'https://example.com' }); } catch (error) { return false; } + }, + async delete(this: IHookFunctions): Promise { + try { return await this.helpers.httpRequest({ url: 'https://example.com' }); } catch (error) { return true; } + }, + }, + }`), + errors: [ + { messageId: 'emptyCatch', data: { method: 'checkExists' } }, + { messageId: 'silentReturn', data: { method: 'create' } }, + { messageId: 'silentReturn', data: { method: 'delete' } }, + ], + }, + ], +}); diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/no-silent-error-swallowing.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/no-silent-error-swallowing.ts new file mode 100644 index 00000000000..4978761538d --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/no-silent-error-swallowing.ts @@ -0,0 +1,127 @@ +import { AST_NODE_TYPES, type TSESTree } from '@typescript-eslint/utils'; + +import { + createRule, + findClassProperty, + isNodeTypeClass, + WEBHOOK_LIFECYCLE_METHODS, +} from '../utils/index.js'; + +const LIFECYCLE_METHODS: readonly string[] = WEBHOOK_LIFECYCLE_METHODS; + +/** Returns the static name of a non-computed object property key, or null. */ +function getPropertyName(property: TSESTree.Property): string | null { + if (property.computed) return null; + if (property.key.type === AST_NODE_TYPES.Identifier) return property.key.name; + if (property.key.type === AST_NODE_TYPES.Literal) return String(property.key.value); + return null; +} + +function isFunctionNode(node: TSESTree.Node): boolean { + return ( + node.type === AST_NODE_TYPES.FunctionDeclaration || + node.type === AST_NODE_TYPES.FunctionExpression || + node.type === AST_NODE_TYPES.ArrowFunctionExpression + ); +} + +/** Walks up to the function that lexically encloses `node`, or null at module scope. */ +function getEnclosingFunction(node: TSESTree.Node): TSESTree.Node | null { + let current = node.parent; + while (current) { + if (isFunctionNode(current)) return current; + current = current.parent; + } + return null; +} + +/** A bare `return;` or `return true/false;` with no other work in the block. */ +function isSilentReturn(statement: TSESTree.Statement): boolean { + if (statement.type !== AST_NODE_TYPES.ReturnStatement) return false; + const argument = statement.argument; + if (argument === null) return true; + return argument.type === AST_NODE_TYPES.Literal && typeof argument.value === 'boolean'; +} + +export const NoSilentErrorSwallowingRule = createRule({ + name: 'no-silent-error-swallowing', + meta: { + type: 'problem', + docs: { + description: + 'Disallow webhook lifecycle methods (checkExists, create, delete) from silently swallowing errors in catch blocks', + }, + messages: { + emptyCatch: + 'Empty catch block in webhook lifecycle method `{{method}}` silently swallows the error. Log the error or rethrow it so failures surface instead of being hidden.', + silentReturn: + 'Catch block in webhook lifecycle method `{{method}}` swallows the error and only returns without logging. Log the error (or rethrow) so failures surface instead of being hidden.', + }, + schema: [], + }, + defaultOptions: [], + create(context) { + // Maps each lifecycle method's function node to its method name. Populated + // when the enclosing class is visited (before its `catch` clauses), so a + // `catch` is only flagged when its *nearest* enclosing function is a + // lifecycle method — catches inside nested callbacks are left alone. + const lifecycleMethodByFn = new WeakMap(); + + return { + ClassDeclaration(node) { + if (!isNodeTypeClass(node)) return; + + const webhookMethodsProperty = findClassProperty(node, 'webhookMethods'); + if (webhookMethodsProperty?.value?.type !== AST_NODE_TYPES.ObjectExpression) return; + + for (const groupProperty of webhookMethodsProperty.value.properties) { + if (groupProperty.type !== AST_NODE_TYPES.Property) continue; + if (groupProperty.value.type !== AST_NODE_TYPES.ObjectExpression) continue; + + for (const methodProperty of groupProperty.value.properties) { + if (methodProperty.type !== AST_NODE_TYPES.Property) continue; + + const methodName = getPropertyName(methodProperty); + if (methodName === null || !LIFECYCLE_METHODS.includes(methodName)) continue; + + const fn = methodProperty.value; + if ( + fn.type === AST_NODE_TYPES.FunctionExpression || + fn.type === AST_NODE_TYPES.ArrowFunctionExpression + ) { + lifecycleMethodByFn.set(fn, methodName); + } + } + } + }, + + CatchClause(node) { + const enclosingFn = getEnclosingFunction(node); + if (enclosingFn === null) return; + + const methodName = lifecycleMethodByFn.get(enclosingFn); + if (methodName === undefined) return; + + const statements = node.body.body; + + if (statements.length === 0) { + context.report({ + node: node.body, + messageId: 'emptyCatch', + data: { method: methodName }, + }); + return; + } + + const onlyStatement = statements.length === 1 ? statements[0] : undefined; + if (onlyStatement && isSilentReturn(onlyStatement)) { + context.report({ + node: onlyStatement, + messageId: 'silentReturn', + data: { method: methodName }, + }); + } + }, + }; + }, +}); diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/webhook-lifecycle-complete.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/webhook-lifecycle-complete.ts index f36dffe54ca..bddd5c2c301 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/rules/webhook-lifecycle-complete.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/webhook-lifecycle-complete.ts @@ -5,11 +5,10 @@ import { findClassProperty, findObjectProperty, isNodeTypeClass, + WEBHOOK_LIFECYCLE_METHODS, + type WebhookLifecycleMethod, } from '../utils/index.js'; -const REQUIRED_METHODS = ['checkExists', 'create', 'delete'] as const; -type RequiredMethod = (typeof REQUIRED_METHODS)[number]; - /** * Returns true if the description declares webhook endpoints, indicating the * node is a webhook-based trigger that needs a complete lifecycle. @@ -39,8 +38,8 @@ function isMethodProperty(property: TSESTree.ObjectLiteralElement, name: string) ); } -function findMissingMethods(group: TSESTree.ObjectExpression): RequiredMethod[] { - return REQUIRED_METHODS.filter( +function findMissingMethods(group: TSESTree.ObjectExpression): WebhookLifecycleMethod[] { + return WEBHOOK_LIFECYCLE_METHODS.filter( (method) => !group.properties.some((property) => isMethodProperty(property, method)), ); } diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/utils/constants.ts b/packages/@n8n/eslint-plugin-community-nodes/src/utils/constants.ts new file mode 100644 index 00000000000..79dc13d8f31 --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/src/utils/constants.ts @@ -0,0 +1,21 @@ +import type { WebhookSetupMethodNames } from 'n8n-workflow'; + +export type WebhookLifecycleMethod = WebhookSetupMethodNames; + +/** + * n8n's canonical webhook lifecycle method names (`WebhookSetupMethodNames`) are + * a type, so they can't be iterated at lint time. This object mirrors them as a + * runtime value; the `satisfies Record` ties it to + * the source of truth — adding, removing, or renaming a method upstream breaks + * type-checking here until this list is updated. + */ +const LIFECYCLE_METHOD_SET = { + checkExists: true, + create: true, + delete: true, +} as const satisfies Record; + +/** The webhook trigger lifecycle methods, in the order n8n invokes them. */ +export const WEBHOOK_LIFECYCLE_METHODS = Object.keys( + LIFECYCLE_METHOD_SET, +) as readonly WebhookLifecycleMethod[]; diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/utils/index.ts b/packages/@n8n/eslint-plugin-community-nodes/src/utils/index.ts index 622daee5834..147b4e4f090 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/utils/index.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/utils/index.ts @@ -1,3 +1,4 @@ export * from './ast-utils.js'; +export * from './constants.js'; export * from './file-utils.js'; export * from './rule-creator.js';