mirror of
https://github.com/n8n-io/n8n.git
synced 2026-09-24 23:22:38 +08:00
feat(core): Add no-silent-error-swallowing community node lint rule (no-changelog) (#34780)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
7040b69102
commit
b7dcfc84cb
@@ -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 | ✅ ☑️ | | 🔧 | | |
|
||||
|
||||
@@ -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`.
|
||||
|
||||
<!-- end auto-generated rule header -->
|
||||
|
||||
## 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<boolean> {
|
||||
try {
|
||||
return await this.helpers.httpRequest({ url });
|
||||
} catch (error) {}
|
||||
},
|
||||
async create(this: IHookFunctions): Promise<boolean> {
|
||||
try {
|
||||
return await this.helpers.httpRequest({ url });
|
||||
} catch (error) {
|
||||
return false;
|
||||
}
|
||||
},
|
||||
},
|
||||
};
|
||||
```
|
||||
|
||||
### ✅ Correct
|
||||
|
||||
```typescript
|
||||
webhookMethods = {
|
||||
default: {
|
||||
async checkExists(this: IHookFunctions): Promise<boolean> {
|
||||
try {
|
||||
return await this.helpers.httpRequest({ url });
|
||||
} catch (error) {
|
||||
this.logger.error('checkExists failed', { error });
|
||||
return false;
|
||||
}
|
||||
},
|
||||
async create(this: IHookFunctions): Promise<boolean> {
|
||||
try {
|
||||
return await this.helpers.httpRequest({ url });
|
||||
} catch (error) {
|
||||
throw error;
|
||||
}
|
||||
},
|
||||
},
|
||||
};
|
||||
```
|
||||
@@ -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',
|
||||
|
||||
@@ -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,
|
||||
|
||||
+245
@@ -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<boolean> { return true; },
|
||||
async create(this: IHookFunctions): Promise<boolean> { return true; },
|
||||
async delete(this: IHookFunctions): Promise<boolean> { return true; },
|
||||
},
|
||||
}`),
|
||||
},
|
||||
{
|
||||
name: 'catch block that logs and returns',
|
||||
code: createTriggerNode(`{
|
||||
default: {
|
||||
async checkExists(this: IHookFunctions): Promise<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
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<boolean> => {
|
||||
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<boolean> {
|
||||
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<boolean> {
|
||||
try { return await this.helpers.httpRequest({ url: 'https://example.com' }); } catch (error) {}
|
||||
},
|
||||
async create(this: IHookFunctions): Promise<boolean> {
|
||||
try { return await this.helpers.httpRequest({ url: 'https://example.com' }); } catch (error) { return false; }
|
||||
},
|
||||
async delete(this: IHookFunctions): Promise<boolean> {
|
||||
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' } },
|
||||
],
|
||||
},
|
||||
],
|
||||
});
|
||||
@@ -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<TSESTree.Node, string>();
|
||||
|
||||
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 },
|
||||
});
|
||||
}
|
||||
},
|
||||
};
|
||||
},
|
||||
});
|
||||
@@ -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)),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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<WebhookSetupMethodNames, true>` 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<WebhookSetupMethodNames, true>;
|
||||
|
||||
/** The webhook trigger lifecycle methods, in the order n8n invokes them. */
|
||||
export const WEBHOOK_LIFECYCLE_METHODS = Object.keys(
|
||||
LIFECYCLE_METHOD_SET,
|
||||
) as readonly WebhookLifecycleMethod[];
|
||||
@@ -1,3 +1,4 @@
|
||||
export * from './ast-utils.js';
|
||||
export * from './constants.js';
|
||||
export * from './file-utils.js';
|
||||
export * from './rule-creator.js';
|
||||
|
||||
Reference in New Issue
Block a user