From 4247477b3d6b6de63615c263b03ebde1add4a630 Mon Sep 17 00:00:00 2001 From: Elias Meire Date: Sat, 2 Aug 2025 13:40:47 +0200 Subject: [PATCH] build: Add lint rule to ban argument spreads to prevent stack overflow (#17493) --- packages/@n8n/eslint-config/package.json | 12 +-- .../@n8n/eslint-config/src/configs/base.ts | 2 +- packages/@n8n/eslint-config/src/plugin.ts | 1 + .../@n8n/eslint-config/src/rules/index.ts | 2 + .../src/rules/no-argument-spread.test.ts | 47 ++++++++++ .../src/rules/no-argument-spread.ts | 87 +++++++++++++++++++ 6 files changed, 144 insertions(+), 7 deletions(-) create mode 100644 packages/@n8n/eslint-config/src/rules/no-argument-spread.test.ts create mode 100644 packages/@n8n/eslint-config/src/rules/no-argument-spread.ts diff --git a/packages/@n8n/eslint-config/package.json b/packages/@n8n/eslint-config/package.json index 6bbd089d51..38fbd88ed6 100644 --- a/packages/@n8n/eslint-config/package.json +++ b/packages/@n8n/eslint-config/package.json @@ -5,16 +5,16 @@ "version": "0.0.1", "exports": { "./base": { - "default": "./dist/configs/base.js", - "types": "./dist/configs/base.d.js" + "types": "./dist/configs/base.d.js", + "default": "./dist/configs/base.js" }, "./frontend": { - "default": "./dist/configs/frontend.js", - "types": "./dist/configs/frontend.d.js" + "types": "./dist/configs/frontend.d.js", + "default": "./dist/configs/frontend.js" }, "./node": { - "default": "./dist/configs/node.js", - "types": "./dist/configs/node.d.js" + "types": "./dist/configs/node.d.js", + "default": "./dist/configs/node.js" } }, "scripts": { diff --git a/packages/@n8n/eslint-config/src/configs/base.ts b/packages/@n8n/eslint-config/src/configs/base.ts index fda6f5f53b..dbd5522673 100644 --- a/packages/@n8n/eslint-config/src/configs/base.ts +++ b/packages/@n8n/eslint-config/src/configs/base.ts @@ -369,7 +369,7 @@ export const baseConfig = tseslint.config( /** * https://eslint.org/docs/rules/prefer-spread */ - 'prefer-spread': 'error', + 'prefer-spread': 'off', // These are tuned off since we use `noUnusedLocals` and `noUnusedParameters` now 'no-unused-vars': 'off', diff --git a/packages/@n8n/eslint-config/src/plugin.ts b/packages/@n8n/eslint-config/src/plugin.ts index da81092122..1da4a84c3f 100644 --- a/packages/@n8n/eslint-config/src/plugin.ts +++ b/packages/@n8n/eslint-config/src/plugin.ts @@ -24,6 +24,7 @@ export const localRulesPlugin = { 'n8n-local-rules/no-interpolation-in-regular-string': 'error', 'n8n-local-rules/no-unused-param-in-catch-clause': 'error', 'n8n-local-rules/no-useless-catch-throw': 'error', + 'n8n-local-rules/no-argument-spread': 'warn', // TODO: mark error }, }, }, diff --git a/packages/@n8n/eslint-config/src/rules/index.ts b/packages/@n8n/eslint-config/src/rules/index.ts index 2a28310cc5..f68f469cc3 100644 --- a/packages/@n8n/eslint-config/src/rules/index.ts +++ b/packages/@n8n/eslint-config/src/rules/index.ts @@ -13,6 +13,7 @@ import { NoUntypedConfigClassFieldRule } from './no-untyped-config-class-field.j import { NoTopLevelRelativeImportsInBackendModuleRule } from './no-top-level-relative-imports-in-backend-module.js'; import { NoConstructorInBackendModuleRule } from './no-constructor-in-backend-module.js'; import type { AnyRuleModule } from '@typescript-eslint/utils/ts-eslint'; +import { NoArgumentSpreadRule } from './no-argument-spread.js'; export const rules = { 'no-uncaught-json-parse': NoUncaughtJsonParseRule, @@ -29,4 +30,5 @@ export const rules = { 'no-untyped-config-class-field': NoUntypedConfigClassFieldRule, 'no-top-level-relative-imports-in-backend-module': NoTopLevelRelativeImportsInBackendModuleRule, 'no-constructor-in-backend-module': NoConstructorInBackendModuleRule, + 'no-argument-spread': NoArgumentSpreadRule, } satisfies Record; diff --git a/packages/@n8n/eslint-config/src/rules/no-argument-spread.test.ts b/packages/@n8n/eslint-config/src/rules/no-argument-spread.test.ts new file mode 100644 index 0000000000..f3f43358a5 --- /dev/null +++ b/packages/@n8n/eslint-config/src/rules/no-argument-spread.test.ts @@ -0,0 +1,47 @@ +import { RuleTester } from '@typescript-eslint/rule-tester'; +import { NoArgumentSpreadRule } from './no-argument-spread.js'; + +const ruleTester = new RuleTester(); + +ruleTester.run('no-unbounded-argument-spread', NoArgumentSpreadRule, { + valid: [ + { code: 'fn(1, 2, 3)' }, + { code: 'fn(...[1, 2, 3])' }, + { code: 'new Foo(...[1, 2])' }, + { code: 'fn.apply(null, deps)' }, + { code: 'Reflect.construct(Foo, deps)' }, + ], + + invalid: [ + { + code: 'fn(...deps)', + output: 'fn.apply(undefined, deps)', + errors: [{ messageId: 'replaceWithApply' }], + }, + { + code: 'obj.fn(...deps)', + output: 'obj.fn.apply(obj, deps)', + errors: [{ messageId: 'replaceWithApply' }], + }, + { + code: 'instance = metadata.factory(...dependencies);', + output: 'instance = metadata.factory.apply(metadata, dependencies);', + errors: [{ messageId: 'replaceWithApply' }], + }, + { + code: 'new Foo(...deps)', + output: 'Reflect.construct(Foo, deps)', + errors: [{ messageId: 'replaceWithReflect' }], + }, + { + code: 'someFunction(a, ...deps)', + output: null, // multiple args — no fix + errors: [{ messageId: 'replaceWithApply' }], + }, + { + code: 'new Bar(a, ...deps)', + output: null, + errors: [{ messageId: 'replaceWithReflect' }], + }, + ], +}); diff --git a/packages/@n8n/eslint-config/src/rules/no-argument-spread.ts b/packages/@n8n/eslint-config/src/rules/no-argument-spread.ts new file mode 100644 index 0000000000..623617d83d --- /dev/null +++ b/packages/@n8n/eslint-config/src/rules/no-argument-spread.ts @@ -0,0 +1,87 @@ +import { ESLintUtils } from '@typescript-eslint/utils'; + +export const NoArgumentSpreadRule = ESLintUtils.RuleCreator.withoutDocs({ + meta: { + type: 'problem', + docs: { + description: + 'Avoid spreading potentially large arrays in function or constructor calls — can cause stack overflows. Use `.apply` or `Reflect.construct` instead.', + }, + fixable: 'code', + messages: { + noUnboundedSpread: + 'Avoid spreading an array in function or constructor calls unless known to be small.', + replaceWithApply: + 'Replace `array.push(...largeArray)` with `array.push.apply(array, largeArray)` to avoid potential stack overflows.', + replaceWithReflect: + 'Replace `new Constructor(...args)` with `Reflect.construct(Constructor, args)` to avoid potential stack overflows.', + }, + schema: [], + }, + defaultOptions: [], + create(context) { + return { + CallExpression(node) { + for (const arg of node.arguments) { + if (arg.type !== 'SpreadElement') continue; + + const spreadArg = arg.argument; + + // Allow spread of inline arrays + if (spreadArg.type === 'ArrayExpression') return; + + // Only autofix if it's the sole argument + const canFix = node.arguments.length === 1; + + context.report({ + node, + messageId: 'replaceWithApply', + fix: canFix + ? (fixer) => { + const source = context.sourceCode; + + if (node.callee.type === 'MemberExpression') { + // Preserve `this` + const thisText = source.getText(node.callee.object); + const calleeText = source.getText(node.callee); + const argText = source.getText(spreadArg); + return fixer.replaceText(node, `${calleeText}.apply(${thisText}, ${argText})`); + } else { + // Not a memberexpression, use undefined as thisArg + const calleeText = source.getText(node.callee); + const argText = source.getText(spreadArg); + return fixer.replaceText(node, `${calleeText}.apply(undefined, ${argText})`); + } + } + : null, + }); + } + }, + + NewExpression(node) { + for (const arg of node.arguments || []) { + if (arg.type !== 'SpreadElement') continue; + + const spreadArg = arg.argument; + + if (spreadArg.type === 'ArrayExpression') return; + + const canFix = node.arguments.length === 1; + + context.report({ + node, + messageId: 'replaceWithReflect', + fix: canFix + ? (fixer) => { + const source = context.sourceCode; + const ctorText = source.getText(node.callee); + const argText = source.getText(spreadArg); + return fixer.replaceText(node, `Reflect.construct(${ctorText}, ${argText})`); + } + : null, + }); + } + }, + }; + }, +});