From 7f16040253dc26fc0170ae8e98e74ab22a4a5298 Mon Sep 17 00:00:00 2001 From: Stephen Wright Date: Fri, 16 Oct 2026 14:42:03 +0000 Subject: [PATCH 1/7] feat(Databricks Node): Add the Lakebase Delete operation (no-changelog) (#40543) --- .../actions/lakebase/Lakebase.resource.ts | 1 + .../Databricks/actions/lakebase/conditions.ts | 22 ++- .../actions/lakebase/deleteRows.operation.ts | 40 +++++ .../resources/lakebase/operations.ts | 6 + .../resources/lakebase/parameters.ts | 8 +- .../Databricks/test/lakebaseDelete.test.ts | 144 ++++++++++++++++++ .../Databricks/transport/lakebaseErrors.ts | 4 + 7 files changed, 215 insertions(+), 10 deletions(-) create mode 100644 packages/nodes-base/nodes/Databricks/actions/lakebase/deleteRows.operation.ts create mode 100644 packages/nodes-base/nodes/Databricks/test/lakebaseDelete.test.ts diff --git a/packages/nodes-base/nodes/Databricks/actions/lakebase/Lakebase.resource.ts b/packages/nodes-base/nodes/Databricks/actions/lakebase/Lakebase.resource.ts index 6edd16edcf9..4f6dd78d3d1 100644 --- a/packages/nodes-base/nodes/Databricks/actions/lakebase/Lakebase.resource.ts +++ b/packages/nodes-base/nodes/Databricks/actions/lakebase/Lakebase.resource.ts @@ -1,2 +1,3 @@ +export * as deleteRows from './deleteRows.operation'; export * as getAll from './getAll.operation'; export * as insert from './insert.operation'; diff --git a/packages/nodes-base/nodes/Databricks/actions/lakebase/conditions.ts b/packages/nodes-base/nodes/Databricks/actions/lakebase/conditions.ts index c83c804296b..9acbaa9c502 100644 --- a/packages/nodes-base/nodes/Databricks/actions/lakebase/conditions.ts +++ b/packages/nodes-base/nodes/Databricks/actions/lakebase/conditions.ts @@ -44,14 +44,17 @@ export function quotePostgrestComponent(value: unknown): string { } /** - * Turns the Select Rows, Sort and Output Columns fields into Data API query parameters. + * Turns the Select Rows conditions into the one query parameter that narrows a + * request. Returns an empty object when nothing survives, which is how a caller + * that must not run unfiltered tells the difference. * * Conditions always go into a single `and=()` or `or=()` group rather than one * parameter per column, so two conditions on the same column both survive. */ -export function buildLakebaseQuery(input: LakebaseQueryInput): IDataObject { - const qs: IDataObject = {}; - +export function buildLakebaseFilter(input: { + where: LakebaseWhereRule[]; + combineConditions: 'AND' | 'OR'; +}): IDataObject { const terms = input.where .filter((rule) => rule.column) .map((rule) => { @@ -75,9 +78,14 @@ export function buildLakebaseQuery(input: LakebaseQueryInput): IDataObject { return `${column}.${condition}.${quotePostgrestComponent(rule.value)}`; }); - if (terms.length > 0) { - qs[input.combineConditions === 'OR' ? 'or' : 'and'] = `(${terms.join(',')})`; - } + if (terms.length === 0) return {}; + + return { [input.combineConditions === 'OR' ? 'or' : 'and']: `(${terms.join(',')})` }; +} + +/** Adds the read-only concerns, sorting and column selection, on top of the filter */ +export function buildLakebaseQuery(input: LakebaseQueryInput): IDataObject { + const qs: IDataObject = buildLakebaseFilter(input); const order = input.sort .filter((rule) => rule.column) diff --git a/packages/nodes-base/nodes/Databricks/actions/lakebase/deleteRows.operation.ts b/packages/nodes-base/nodes/Databricks/actions/lakebase/deleteRows.operation.ts new file mode 100644 index 00000000000..6fd08f005e7 --- /dev/null +++ b/packages/nodes-base/nodes/Databricks/actions/lakebase/deleteRows.operation.ts @@ -0,0 +1,40 @@ +import type { IDataObject, IExecuteFunctions, INodeExecutionData } from 'n8n-workflow'; +import { NodeOperationError } from 'n8n-workflow'; + +import { lakebaseApiRequest } from '../../transport'; +import { buildLakebaseFilter, type LakebaseWhereRule } from './conditions'; +import { resolveLakebaseTableUrl } from './helpers'; + +export async function execute(this: IExecuteFunctions, i: number): Promise { + const url = await resolveLakebaseTableUrl(this, i); + + const where = this.getNodeParameter('where.values', i, []) as LakebaseWhereRule[]; + const combineConditions = this.getNodeParameter('combineConditions', i, 'AND') as 'AND' | 'OR'; + const qs = buildLakebaseFilter({ where, combineConditions }); + + // The Data API applies no filter of its own, so an unfiltered delete empties + // the table and there is nothing to undo it. A condition row left blank + // builds nothing, which is why this tests the filter rather than the rows. + if (Object.keys(qs).length === 0) { + throw new NodeOperationError(this.getNode(), 'At least one condition is required', { + itemIndex: i, + description: 'Add a condition under Select Rows so the node knows which rows to delete.', + }); + } + + const response = await lakebaseApiRequest(this, { + method: 'DELETE', + url, + qs, + json: true, + headers: { Accept: 'application/json', Prefer: 'return=representation' }, + }); + + // `return=representation` echoes the removed rows. An empty answer cannot tell + // "nothing matched" from "the project ignores the header and rows went", so report + // success as Insert does rather than return nothing and stop the branch. + const deleted = Array.isArray(response) ? (response as IDataObject[]) : []; + if (deleted.length === 0) return [{ json: { success: true }, pairedItem: { item: i } }]; + + return deleted.map((row) => ({ json: row, pairedItem: { item: i } })); +} diff --git a/packages/nodes-base/nodes/Databricks/resources/lakebase/operations.ts b/packages/nodes-base/nodes/Databricks/resources/lakebase/operations.ts index 0e139f68f6d..215c9d7e007 100644 --- a/packages/nodes-base/nodes/Databricks/resources/lakebase/operations.ts +++ b/packages/nodes-base/nodes/Databricks/resources/lakebase/operations.ts @@ -11,6 +11,12 @@ export const lakebaseOperations: INodeProperties = { }, }, options: [ + { + name: 'Delete', + value: 'deleteRows', + description: 'Delete the rows that match the conditions', + action: 'Delete rows', + }, { name: 'Get Many', value: 'getAll', diff --git a/packages/nodes-base/nodes/Databricks/resources/lakebase/parameters.ts b/packages/nodes-base/nodes/Databricks/resources/lakebase/parameters.ts index eea048e831f..764eb2ae8ac 100644 --- a/packages/nodes-base/nodes/Databricks/resources/lakebase/parameters.ts +++ b/packages/nodes-base/nodes/Databricks/resources/lakebase/parameters.ts @@ -2,6 +2,7 @@ import type { INodeProperties } from 'n8n-workflow'; const showForLakebase = { resource: ['lakebase'] }; const showForGetAll = { resource: ['lakebase'], operation: ['getAll'] }; +const showForFilter = { resource: ['lakebase'], operation: ['getAll', 'deleteRows'] }; export const lakebaseParameters: INodeProperties[] = [ { @@ -200,9 +201,10 @@ export const lakebaseParameters: INodeProperties[] = [ }, placeholder: 'Add Condition', default: {}, - description: 'If not set, the node returns all rows', + description: + 'Which rows to act on. Get Many returns rows from the full table if you set no conditions. Delete needs at least one condition.', displayOptions: { - show: showForGetAll, + show: showForFilter, }, options: [ { @@ -280,7 +282,7 @@ export const lakebaseParameters: INodeProperties[] = [ ], default: 'AND', displayOptions: { - show: showForGetAll, + show: showForFilter, }, }, { diff --git a/packages/nodes-base/nodes/Databricks/test/lakebaseDelete.test.ts b/packages/nodes-base/nodes/Databricks/test/lakebaseDelete.test.ts new file mode 100644 index 00000000000..a5ffadc330e --- /dev/null +++ b/packages/nodes-base/nodes/Databricks/test/lakebaseDelete.test.ts @@ -0,0 +1,144 @@ +import type { IDataObject, IExecuteFunctions, INode, NodeParameterValueType } from 'n8n-workflow'; +import { mockDeep } from 'vitest-mock-extended'; + +import { execute as deleteRows } from '../actions/lakebase/deleteRows.operation'; + +const TABLE_URL = 'https://host.example/api/2.0/workspace/7/rest/app/public/orders'; + +// The URL assembly belongs to the picker PR, so the operation tests pin a fixed table URL +vi.mock('../actions/lakebase/helpers', () => ({ + resolveLakebaseTableUrl: vi.fn(async () => TABLE_URL), + resolveLakebaseSchemaUrl: vi.fn(async () => TABLE_URL.replace(/\/orders$/, '')), +})); + +const node: INode = { + id: '1', + name: 'Databricks', + type: 'n8n-nodes-base.databricks', + typeVersion: 1, + position: [0, 0], + parameters: {}, +}; + +const matchSku = [{ column: 'sku', condition: 'eq', value: 'WF-1' }]; + +describe('Lakebase -> Delete', () => { + const setupContext = ( + overrides: Record = {}, + itemIndex = 0, + ) => { + const parameters: Record = { + authentication: 'oAuth2', + 'where.values': matchSku, + ...overrides, + }; + const context = mockDeep(); + context.getNode.mockReturnValue(node); + context.getInputData.mockReturnValue([]); + context.getExecutionCancelSignal.mockReturnValue(undefined); + context.getNodeParameter.mockImplementation((name, index, fallback) => + index === itemIndex || name === 'authentication' ? (parameters[name] ?? fallback) : fallback, + ); + context.getCredentials.mockResolvedValue({ host: 'https://host.example' }); + return context; + }; + const apiMock = (context: ReturnType) => + context.helpers.httpRequestWithAuthentication; + const run = ( + response: unknown, + overrides: Record = {}, + itemIndex = 0, + ) => { + const context = setupContext(overrides, itemIndex); + apiMock(context).mockResolvedValue(response); + return { context, result: deleteRows.call(context, itemIndex) }; + }; + + it('deletes the rows the conditions select', async () => { + const { context, result } = run([{ id: 1, sku: 'WF-1' }]); + + expect(await result).toEqual([{ json: { id: 1, sku: 'WF-1' }, pairedItem: { item: 0 } }]); + expect(apiMock(context).mock.calls[0][0]).toBe('databricksOAuth2Api'); + expect(apiMock(context).mock.calls[0][1]).toEqual( + expect.objectContaining({ + method: 'DELETE', + url: TABLE_URL, + qs: { and: '(sku.eq.WF-1)' }, + }), + ); + }); + + it('asks the Data API to echo what it removed', async () => { + const { context, result } = run([{ id: 1 }]); + await result; + + const options = apiMock(context).mock.calls[0][1] as { headers: Record }; + expect(options.headers.Prefer).toBe('return=representation'); + }); + + it('combines conditions with OR when asked', async () => { + const { context, result } = run([{ id: 1 }], { + 'where.values': [ + { column: 'sku', condition: 'eq', value: 'a' }, + { column: 'sku', condition: 'eq', value: 'b' }, + ], + combineConditions: 'OR', + }); + await result; + + expect((apiMock(context).mock.calls[0][1] as { qs: IDataObject }).qs).toEqual({ + or: '(sku.eq.a,sku.eq.b)', + }); + }); + + it('sends no sort or column selection', async () => { + const { context, result } = run([{ id: 1 }], { + 'sort.values': [{ column: 'sku', direction: 'asc' }], + 'options.outputColumns': ['sku'], + }); + await result; + + const { qs } = apiMock(context).mock.calls[0][1] as { qs: IDataObject }; + expect(qs).toEqual({ and: '(sku.eq.WF-1)' }); + }); + + it.each([ + ['no conditions at all', []], + ['a condition row with no column', [{ condition: 'eq', value: 'WF-1' }]], + ['several rows, none with a column', [{ condition: 'eq' }, { value: 'x' }]], + ])('refuses to run with %s', async (_name, where) => { + const { context, result } = run([{ id: 1 }], { 'where.values': where }); + + await expect(result).rejects.toThrow('At least one condition is required'); + expect(apiMock(context)).not.toHaveBeenCalled(); + }); + + it('refuses an OR filter that builds nothing', async () => { + const { context, result } = run([{ id: 1 }], { + 'where.values': [{ condition: 'eq', value: 'x' }], + combineConditions: 'OR', + }); + + await expect(result).rejects.toThrow('At least one condition is required'); + expect(apiMock(context)).not.toHaveBeenCalled(); + }); + + it.each([ + ['nothing matched', []], + ['the project does not echo rows', undefined], + ['the response is not an array', { ok: true }], + ])('reports success when %s', async (_name, response) => { + const { result } = run(response); + + expect(await result).toEqual([{ json: { success: true }, pairedItem: { item: 0 } }]); + }); + + it('pairs each removed row to the item it came from', async () => { + const { result } = run([{ id: 1 }, { id: 2 }], {}, 3); + + expect(await result).toEqual([ + { json: { id: 1 }, pairedItem: { item: 3 } }, + { json: { id: 2 }, pairedItem: { item: 3 } }, + ]); + }); +}); diff --git a/packages/nodes-base/nodes/Databricks/transport/lakebaseErrors.ts b/packages/nodes-base/nodes/Databricks/transport/lakebaseErrors.ts index 01f5d7fa19f..71d21fb9b34 100644 --- a/packages/nodes-base/nodes/Databricks/transport/lakebaseErrors.ts +++ b/packages/nodes-base/nodes/Databricks/transport/lakebaseErrors.ts @@ -36,6 +36,10 @@ export const LAKEBASE_ERROR_HINTS = new Map([ '42501', 'The Postgres role is not granted to the Data API. Run `GRANT "your-role" TO authenticator` as the role\'s creator. A project owner cannot use the Data API at all, so use a non-owner identity.', ], + [ + '23503', + 'If you insert or update a row, the row it points at must exist. If you delete a row, delete the rows that point at it first, or set the foreign key to cascade.', + ], ['23505', 'A row with the same key already exists.'], ['23502', 'A column that cannot be null has no value.'], ]); From 8ee5cf184574194c27f91293f6cc2b3d149403b2 Mon Sep 17 00:00:00 2001 From: "n8n-cat-bot[bot]" <283985454+n8n-cat-bot[bot]@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:44:01 +0000 Subject: [PATCH 2/7] chore: Flaky test: frontend.service.test.ts (3 master or merge-queue commits in 72 h) (#41279) Co-authored-by: n8n-cat-bot[bot] --- .../__tests__/frontend.service.test.ts | 61 +++++++++++-------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/packages/cli/src/services/__tests__/frontend.service.test.ts b/packages/cli/src/services/__tests__/frontend.service.test.ts index 40fbb156634..4e9d6a03c7e 100644 --- a/packages/cli/src/services/__tests__/frontend.service.test.ts +++ b/packages/cli/src/services/__tests__/frontend.service.test.ts @@ -220,31 +220,40 @@ describe('FrontendService', () => { // constructor-injected mock above must also be registered here. Container.set(License, license); - return { - service: new FrontendService( - globalConfig, - logger, - loadNodesAndCredentials, - credentialTypes, - credentialsOverwrites, + // Suppress the constructor's background write so it cannot outlive the test. + // Restore the method so tests and reload handlers can call it explicitly. + const generateTypes = vi + .spyOn(FrontendService.prototype, 'generateTypes') + .mockResolvedValueOnce(); + try { + return { + service: new FrontendService( + globalConfig, + logger, + loadNodesAndCredentials, + credentialTypes, + credentialsOverwrites, + license, + mailer, + instanceSettings, + urlService, + securityConfig, + pushConfig, + binaryDataConfig, + licenseState, + moduleRegistry, + mfaService, + ownershipService, + aiUsageService, + workflowRepository, + workflowReviewPolicyService, + inboxService, + ), license, - mailer, - instanceSettings, - urlService, - securityConfig, - pushConfig, - binaryDataConfig, - licenseState, - moduleRegistry, - mfaService, - ownershipService, - aiUsageService, - workflowRepository, - workflowReviewPolicyService, - inboxService, - ), - license, - }; + }; + } finally { + generateTypes.mockRestore(); + } }; beforeEach(() => { @@ -1368,9 +1377,9 @@ describe('FrontendService', () => { const largeNodes = createNodes(5000); const smallNodes = createNodes(1); const collectTypes = loadNodesAndCredentials.collectTypes as Mock; - collectTypes.mockReturnValueOnce(new Promise(() => {})); const { service } = createMockService(); + expect(collectTypes).not.toHaveBeenCalled(); for (let attempt = 0; attempt < 10; attempt++) { collectTypes @@ -1383,6 +1392,8 @@ describe('FrontendService', () => { expect(readTypesFile('credentials')).toEqual([]); expect(readTypesFile('node-versions')).toEqual(expect.any(Array)); } + + expect(collectTypes).toHaveBeenCalledTimes(20); }); }); }); From 24621ad4caa4a8ea76bb8d1d45c884fbc9cdc1c4 Mon Sep 17 00:00:00 2001 From: "n8n-cat-bot[bot]" <283985454+n8n-cat-bot[bot]@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:47:15 +0000 Subject: [PATCH 3/7] chore: Add review/lint rule to discourage open-ended fields in nodes (warn level) (#39959) Co-authored-by: n8n-cat-bot[bot] --- .../eslint-plugin-community-nodes/README.md | 1 + .../docs/rules/open-ended-node-fields.md | 35 +++++ .../src/plugin.ts | 2 + .../src/rules/index.ts | 2 + .../src/rules/open-ended-node-fields.test.ts | 122 ++++++++++++++++++ .../src/rules/open-ended-node-fields.ts | 55 ++++++++ 6 files changed, 217 insertions(+) create mode 100644 packages/@n8n/eslint-plugin-community-nodes/docs/rules/open-ended-node-fields.md create mode 100644 packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.test.ts create mode 100644 packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.ts diff --git a/packages/@n8n/eslint-plugin-community-nodes/README.md b/packages/@n8n/eslint-plugin-community-nodes/README.md index db539c78ff6..96a5fd63178 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/README.md +++ b/packages/@n8n/eslint-plugin-community-nodes/README.md @@ -77,6 +77,7 @@ export default [ | [node-connection-type-literal](docs/rules/node-connection-type-literal.md) | Disallow string literals in node description `inputs`/`outputs` — use `NodeConnectionTypes` enum instead | ✅ ☑️ | | 🔧 | | | | [node-registration-complete](docs/rules/node-registration-complete.md) | Ensure every `.node.ts` file in the `nodes/` directory is registered in the "n8n.nodes" array of package.json | | ✅ ☑️ | | | | | [node-usable-as-tool](docs/rules/node-usable-as-tool.md) | Ensure node classes have usableAsTool property | ✅ ☑️ | | 🔧 | | | +| [open-ended-node-fields](docs/rules/open-ended-node-fields.md) | Discourage optional open-ended JSON fields in node properties | | ✅ ☑️ | | | | | [package-name-convention](docs/rules/package-name-convention.md) | Enforce correct package naming convention for n8n community nodes | ✅ ☑️ | | | 💡 | | | [require-files-array](docs/rules/require-files-array.md) | Require a non-empty "files" array in package.json | | ✅ ☑️ | | | | | [require-homepage](docs/rules/require-homepage.md) | Require a "homepage" field with a valid URL in package.json | | ✅ ☑️ | | | | diff --git a/packages/@n8n/eslint-plugin-community-nodes/docs/rules/open-ended-node-fields.md b/packages/@n8n/eslint-plugin-community-nodes/docs/rules/open-ended-node-fields.md new file mode 100644 index 00000000000..5907ae66bdc --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/docs/rules/open-ended-node-fields.md @@ -0,0 +1,35 @@ +# Discourage optional open-ended JSON fields in node properties (`@n8n/community-nodes/open-ended-node-fields`) + +⚠️ This rule _warns_ in the following configs: ✅ `recommended`, ☑️ `recommendedWithoutN8nCloudSupport`. + + + +## Rule Details + +This rule warns about optional top-level JSON properties in node descriptions. A JSON configuration field can hide parameters from the editor. Typed parameters make configuration easier to find, validate, and change. + +Place optional parameters under Additional Options or Additional Fields. The rule does not report required JSON properties or typed collection options. + +## Examples + +### Incorrect + +```typescript +properties: [ + { displayName: 'Configuration', name: 'configuration', type: 'json', default: '{}' }, +]; +``` + +### Correct + +```typescript +properties: [ + { + displayName: 'Additional Fields', + name: 'additionalFields', + type: 'collection', + default: {}, + options: [{ displayName: 'Limit', name: 'limit', type: 'number', default: 10 }], + }, +]; +``` diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts b/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts index 622119334fb..049c9234493 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/plugin.ts @@ -29,6 +29,7 @@ const configs = { '@n8n/community-nodes/no-deprecated-workflow-functions': 'error', '@n8n/community-nodes/no-emoji-in-options': 'error', '@n8n/community-nodes/node-usable-as-tool': 'error', + '@n8n/community-nodes/open-ended-node-fields': 'warn', '@n8n/community-nodes/package-name-convention': 'error', '@n8n/community-nodes/credential-test-required': 'error', '@n8n/community-nodes/no-credential-reuse': 'error', @@ -79,6 +80,7 @@ const configs = { '@n8n/community-nodes/no-deprecated-workflow-functions': 'error', '@n8n/community-nodes/no-emoji-in-options': 'error', '@n8n/community-nodes/node-usable-as-tool': 'error', + '@n8n/community-nodes/open-ended-node-fields': 'warn', '@n8n/community-nodes/package-name-convention': 'error', '@n8n/community-nodes/credential-test-required': 'error', '@n8n/community-nodes/no-credential-reuse': 'error', 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 d178a210639..cb51527cb18 100644 --- a/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/index.ts @@ -31,6 +31,7 @@ import { NodeClassDescriptionIconMissingRule } from './node-class-description-ic import { NodeConnectionTypeLiteralRule } from './node-connection-type-literal.js'; import { NodeRegistrationCompleteRule } from './node-registration-complete.js'; import { NodeUsableAsToolRule } from './node-usable-as-tool.js'; +import { OpenEndedNodeFieldsRule } from './open-ended-node-fields.js'; import { PackageNameConventionRule } from './package-name-convention.js'; import { RequireFilesArrayRule } from './require-files-array.js'; import { RequireHomepageRule } from './require-homepage.js'; @@ -54,6 +55,7 @@ export const rules = { 'no-deprecated-workflow-functions': NoDeprecatedWorkflowFunctionsRule, 'no-emoji-in-options': NoEmojiInOptionsRule, 'node-usable-as-tool': NodeUsableAsToolRule, + 'open-ended-node-fields': OpenEndedNodeFieldsRule, 'package-name-convention': PackageNameConventionRule, 'credential-test-required': CredentialTestRequiredRule, 'credential-unnecessary-password': CredentialUnnecessaryPasswordRule, diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.test.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.test.ts new file mode 100644 index 00000000000..5b50fe308cc --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.test.ts @@ -0,0 +1,122 @@ +import { Linter } from 'eslint'; +import { describe, expect, it } from 'vitest'; + +import { configs } from '../plugin.js'; + +const linter = new Linter(); + +function createNodeCode(property: string): string { + return ` +export class TestNode extends Node { + description = { + displayName: 'Test Node', + name: 'testNode', + group: ['transform'], + version: 1, + inputs: ['main'], + outputs: ['main'], + properties: [${property}], + }; +}`; +} + +function lintNode(code: string, config: keyof typeof configs) { + return linter.verify(code, [{ ...configs[config], files: ['**/*.node.ts'] }], { + filename: 'src/rules/TestNode.node.ts', + }); +} + +describe.each(['recommended', 'recommendedWithoutN8nCloudSupport'] as const)( + '%s node property review', + (config) => { + // CE-1879: Warn when an optional JSON field replaces typed node parameters. + it('warns about an optional JSON configuration catch-all', () => { + const code = createNodeCode(`{ + displayName: 'Configuration', + name: 'configuration', + type: 'json', + required: false, + default: '{}', + }`); + + const warnings = lintNode(code, config).filter( + ({ severity, message }) => severity === 1 && /Additional (Options|Fields)/i.test(message), + ); + + expect(warnings).toEqual([ + expect.objectContaining({ + severity: 1, + message: expect.stringMatching(/Additional (Options|Fields)/i), + }), + ]); + }); + + it('does not warn about typed parameters inside Additional Fields', () => { + const code = createNodeCode(`{ + displayName: 'Additional Fields', + name: 'additionalFields', + type: 'collection', + default: {}, + options: [{ displayName: 'Limit', name: 'limit', type: 'number', default: 10 }], + }`); + + const messages = lintNode(code, config); + expect(messages).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + ruleId: '@n8n/community-nodes/require-node-description-fields', + }), + ]), + ); + + const warnings = messages.filter( + ({ severity, message }) => severity === 1 && /Additional (Options|Fields)/i.test(message), + ); + + expect(warnings).toEqual([]); + }); + + it('warns when an open-ended JSON field does not declare required', () => { + const code = createNodeCode(`{ + displayName: 'Configuration', + name: 'configuration', + type: 'json', + default: '{}', + }`); + + expect( + lintNode(code, config).filter( + ({ ruleId }) => ruleId === '@n8n/community-nodes/open-ended-node-fields', + ), + ).toHaveLength(1); + }); + + it('does not warn about required JSON fields', () => { + const code = createNodeCode(`{ + displayName: 'Request Body', + name: 'body', + type: 'json', + required: true, + default: '{}', + }`); + + expect( + lintNode(code, config).filter( + ({ ruleId }) => ruleId === '@n8n/community-nodes/open-ended-node-fields', + ), + ).toEqual([]); + }); + + it('does not inspect classes that are not nodes', () => { + const code = `class Helper { + description = { properties: [{ type: 'json', required: false }] }; + }`; + + expect( + lintNode(code, config).filter( + ({ ruleId }) => ruleId === '@n8n/community-nodes/open-ended-node-fields', + ), + ).toEqual([]); + }); + }, +); diff --git a/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.ts b/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.ts new file mode 100644 index 00000000000..58e99caca0e --- /dev/null +++ b/packages/@n8n/eslint-plugin-community-nodes/src/rules/open-ended-node-fields.ts @@ -0,0 +1,55 @@ +import { AST_NODE_TYPES } from '@typescript-eslint/utils'; + +import { + createRule, + findArrayLiteralProperty, + findNodeDescriptionObject, + findObjectProperty, + getBooleanLiteralValue, + getStringLiteralValue, + isFileType, + isNodeTypeClass, +} from '../utils/index.js'; + +export const OpenEndedNodeFieldsRule = createRule({ + name: 'open-ended-node-fields', + meta: { + type: 'suggestion', + docs: { + description: 'Discourage optional open-ended JSON fields in node properties', + }, + messages: { + optionalJson: + 'Optional JSON fields can hide configuration. Use typed parameters under Additional Options or Additional Fields instead.', + }, + schema: [], + }, + defaultOptions: [], + create(context) { + if (!isFileType(context.filename, '.node.ts')) return {}; + + return { + ClassDeclaration(node) { + if (!isNodeTypeClass(node)) return; + + const description = findNodeDescriptionObject(node); + if (!description) return; + + const properties = findArrayLiteralProperty(description, 'properties'); + if (!properties) return; + + for (const property of properties.elements) { + if (property?.type !== AST_NODE_TYPES.ObjectExpression) continue; + + const type = findObjectProperty(property, 'type'); + if (getStringLiteralValue(type?.value ?? null) !== 'json') continue; + + const required = findObjectProperty(property, 'required'); + if (getBooleanLiteralValue(required?.value ?? null) === true) continue; + + context.report({ node: property, messageId: 'optionalJson' }); + } + }, + }; + }, +}); From 532e8ef93058af3f33eb0fef06c286b19639945e Mon Sep 17 00:00:00 2001 From: Guillaume Jacquart Date: Fri, 16 Oct 2026 14:48:56 +0000 Subject: [PATCH 4/7] fix(core): Save policy rules that name uninstalled packages (no-changelog) (#41210) Co-authored-by: Claude Opus 5.5 (1M context) Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> --- .../node-type-policy-public.dto.ts | 22 +- .../src/events/type-availability-policies.ts | 3 + .../__tests__/telemetry-event-relay.test.ts | 37 ++-- .../cli/src/events/maps/relay.event-map.ts | 3 +- .../events/relays/telemetry.event-relay.ts | 6 +- .../type-availability-policies/README.md | 11 +- .../__tests__/package-resolver.test.ts | 120 +++++++++++ .../__tests__/policy-rule-lint.test.ts | 98 +++++++++ .../__tests__/policy-shadow-lint.test.ts | 24 +-- .../type-availability-policy.service.test.ts | 192 ++++++++++++++---- .../package-resolver.ts | 33 ++- .../policy-rule-lint.ts | 48 +++++ .../policy-shadow-lint.ts | 3 +- .../type-availability-policy.service.ts | 125 ++++-------- ...al-type-policies.public.controller.test.ts | 2 +- ...de-type-policies.public.controller.test.ts | 2 +- ...DocumentWriteResultPublicDto.generated.yml | 34 +++- ...ffectiveWriteResultPublicDto.generated.yml | 34 +++- ...DocumentWriteResultPublicDto.generated.yml | 34 +++- ...ffectiveWriteResultPublicDto.generated.yml | 34 +++- .../credential-type-policy.extends.test.ts | 16 +- ...ability-policy-instance.controller.test.ts | 26 ++- .../credential-type-policies.test.ts | 11 +- .../public-api/node-type-policies.test.ts | 12 +- 24 files changed, 726 insertions(+), 204 deletions(-) create mode 100644 packages/cli/src/modules/type-availability-policies/__tests__/policy-rule-lint.test.ts create mode 100644 packages/cli/src/modules/type-availability-policies/policy-rule-lint.ts diff --git a/packages/@n8n/api-types/src/dto/type-availability-policies/node-type-policy-public.dto.ts b/packages/@n8n/api-types/src/dto/type-availability-policies/node-type-policy-public.dto.ts index afa1b99555c..caafed5d525 100644 --- a/packages/@n8n/api-types/src/dto/type-availability-policies/node-type-policy-public.dto.ts +++ b/packages/@n8n/api-types/src/dto/type-availability-policies/node-type-policy-public.dto.ts @@ -14,11 +14,15 @@ import { publicApiPaginationSchema } from '../pagination/pagination.dto'; * `PutCredentialTypeInstancePolicyDto`, …), so validation is identical on both surfaces. */ -/** A rule that never matches because an earlier rule in the same list already covers it. */ -const shadowWarningSchema = z.object({ - ruleId: z.string(), - shadowedByRuleId: z.string(), -}); +/** + * A saved rule worth a second look: `shadowed` when an earlier rule in the same list already + * covers it, so it never matches. `notLoaded` when it names a package or type this instance + * has not loaded, so nothing here uses it yet. `NODES_EXCLUDE` and `NODES_INCLUDE` count too. + */ +const ruleWarningSchema = z.discriminatedUnion('kind', [ + z.object({ kind: z.literal('shadowed'), ruleId: z.string(), shadowedByRuleId: z.string() }), + z.object({ kind: z.literal('notLoaded'), ruleId: z.string() }), +]); /** Plain rule arrays on purpose: a response never re-runs the duplicate-id refinement. */ function publicSchemasFor(rule: Rule) { @@ -52,9 +56,9 @@ function publicSchemasFor(rule: Rule) { return { effective: effectivePolicy.shape, - effectiveWriteResult: { ...effectivePolicy.shape, warnings: z.array(shadowWarningSchema) }, + effectiveWriteResult: { ...effectivePolicy.shape, warnings: z.array(ruleWarningSchema) }, document: policyDocument.shape, - documentWriteResult: { policy: policyDocument, warnings: z.array(shadowWarningSchema) }, + documentWriteResult: { policy: policyDocument, warnings: z.array(ruleWarningSchema) }, documentList: { data: z.array(policyDocument), nextCursor: z.string().nullable() }, // The scope's version after a replace, which always bumps it. attachments: { attachments: z.array(attachment), version: z.number().int().positive() }, @@ -67,13 +71,13 @@ const credentialType = publicSchemasFor(credentialTypePolicyRuleSchemas.rule); /** `GET` of a scope's composed node type policy. */ export class PolicyEffectivePublicDto extends Z.class(nodeType.effective) {} -/** `PUT` of a scope's composed node type policy: the new state plus shadowing warnings. */ +/** `PUT` of a scope's composed node type policy: the new state plus rule warnings. */ export class PolicyEffectiveWriteResultPublicDto extends Z.class(nodeType.effectiveWriteResult) {} /** One reusable node type policy document. */ export class PolicyDocumentPublicDto extends Z.class(nodeType.document) {} -/** Create or update of a node type policy document, plus shadowing warnings. */ +/** Create or update of a node type policy document, plus rule warnings. */ export class PolicyDocumentWriteResultPublicDto extends Z.class(nodeType.documentWriteResult) {} export class PolicyDocumentListPublicDto extends Z.class(nodeType.documentList) {} diff --git a/packages/@n8n/telemetry/src/events/type-availability-policies.ts b/packages/@n8n/telemetry/src/events/type-availability-policies.ts index 0b18c02d6b6..fd312994ca8 100644 --- a/packages/@n8n/telemetry/src/events/type-availability-policies.ts +++ b/packages/@n8n/telemetry/src/events/type-availability-policies.ts @@ -90,6 +90,9 @@ export const TYPE_AVAILABILITY_POLICIES_TELEMETRY = defineTelemetryEvents({ shadow_warning_count: z .number() .describe('Saved rules that can never match, because an earlier rule already covers them'), + not_loaded_warning_count: z + .number() + .describe('Saved rules that name a package or type this instance has not loaded'), version: z.number().describe('The scope version after the write'), }), }, diff --git a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts index b47831baec2..68840f645c1 100644 --- a/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts +++ b/packages/cli/src/events/__tests__/telemetry-event-relay.test.ts @@ -758,7 +758,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [denyRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( @@ -780,7 +781,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [denyRule, allowPackageRule, delegateRule], - warningCount: 2, + shadowWarningCount: 2, + notLoadedWarningCount: 1, }; eventService.emit('node-type-policy-saved', event); @@ -810,6 +812,7 @@ describe('TelemetryEventRelay', () => { allowed_types: ['n8n-nodes-base.code'], previous_rule_count: null, shadow_warning_count: 2, + not_loaded_warning_count: 1, version: 1, }, ); @@ -831,7 +834,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [credentialDenyRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -871,7 +875,8 @@ describe('TelemetryEventRelay', () => { rulesAfter: [ { id: 'rule-1', action: 'deny', selector: { kind: 'extends', value: 'oAuth2Api' } }, ], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( @@ -900,7 +905,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [credentialPackageDenyRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -927,7 +933,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -945,7 +952,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 5 }, rulesBefore: [denyRule], rulesAfter: [allowPackageRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -975,7 +983,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }; eventService.emit('node-type-policy-saved', event); @@ -1097,7 +1106,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [denyRule], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( @@ -1121,7 +1131,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'allow', version: 1 }, rulesBefore: null, rulesAfter: [{ ...allowPackageRule, action: 'deny' }], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( @@ -1151,7 +1162,8 @@ describe('TelemetryEventRelay', () => { action: 'allow' as const, selector: { kind: 'name' as const, value: name }, })), - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); const properties = vi.mocked(telemetry.track).mock.calls.at(-1)?.[1]; @@ -1174,7 +1186,8 @@ describe('TelemetryEventRelay', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); expect(telemetry.track).toHaveBeenCalledWith( diff --git a/packages/cli/src/events/maps/relay.event-map.ts b/packages/cli/src/events/maps/relay.event-map.ts index 6c96d23ada2..c72226d7603 100644 --- a/packages/cli/src/events/maps/relay.event-map.ts +++ b/packages/cli/src/events/maps/relay.event-map.ts @@ -1450,7 +1450,8 @@ export type RelayEventMap = { after: { defaultAction: PolicyAction; version: number }; rulesBefore: readonly PolicyRule[] | null; rulesAfter: readonly PolicyRule[]; - warningCount: number; + shadowWarningCount: number; + notLoadedWarningCount: number; }; 'node-type-policy-attachments-updated': { diff --git a/packages/cli/src/events/relays/telemetry.event-relay.ts b/packages/cli/src/events/relays/telemetry.event-relay.ts index 8930d2a6408..c0e7c33984e 100644 --- a/packages/cli/src/events/relays/telemetry.event-relay.ts +++ b/packages/cli/src/events/relays/telemetry.event-relay.ts @@ -595,7 +595,8 @@ export class TelemetryEventRelay extends EventRelay { after, rulesBefore, rulesAfter, - warningCount, + shadowWarningCount, + notLoadedWarningCount, }: RelayEventMap['node-type-policy-saved']) { if (!isPolicyKind(kind)) return; @@ -619,7 +620,8 @@ export class TelemetryEventRelay extends EventRelay { ...countSelectorKinds(rulesAfter), ...summarizeTypeAvailability(rulesAfter, after.defaultAction, types, resolvePackage), previous_rule_count: rulesBefore?.length ?? null, - shadow_warning_count: warningCount, + shadow_warning_count: shadowWarningCount, + not_loaded_warning_count: notLoadedWarningCount, version: after.version, }, ); diff --git a/packages/cli/src/modules/type-availability-policies/README.md b/packages/cli/src/modules/type-availability-policies/README.md index edff49d7cde..a1624018073 100644 --- a/packages/cli/src/modules/type-availability-policies/README.md +++ b/packages/cli/src/modules/type-availability-policies/README.md @@ -220,7 +220,15 @@ covers `googleOAuth2Api` and `googleSheetsOAuth2Api`. A `name` rule stays exact, `name oAuth2Api` blocks only the generic OAuth2 credential. Rule order decides as usual: an `allow name googleSheetsOAuth2Api` placed before `deny extends oAuth2Api` keeps Sheets usable, and placed after it, the shadow lint warns that it can never match. A write rejects an `extends` -rule in a node type policy, and one that names a credential type that is not installed. +rule in a node type policy. + +A rule can name a package or type that this instance has not loaded. The write saves it and +returns a `notLoaded` warning for it, next to the `shadowed` warnings. This applies to all +three selectors. Thus, when you uninstall a package, its rules do not block later writes to the +policy. An installed node that `NODES_EXCLUDE` or `NODES_INCLUDE` filters out is not loaded +either. The policy still enforces its rule, because evaluation matches type names, but a policy +cannot make a filtered-out node available. One exception: a write rejects a `package` rule on `n8n-creds-base`, because no package +can ever provide it. ### Reading it on the execution path @@ -324,6 +332,7 @@ through a sealed repository method, and the lint rule that guards that has no al | `credential-type-policy.check.ts` | Credential types: all eight points, including `credentialSave` | | `policy-evaluator.ts` | Pure evaluation: first match per scope, then the instance ∩ project composition | | `policy-shadow-lint.ts` | Warns at write time about rules an earlier rule already covers | +| `policy-rule-lint.ts` | Adds `notLoaded` warnings and merges all write-time warnings in rule order | | `package-resolver.ts` | Resolves a type's package per `kind`, for the `package` selector | | `type-availability-policy.service.ts` | Reads and writes the store, with versioning and row locks | | `type-availability-policy-instance.controller.ts` | Instance scope, documents and attachments, `node-types` | diff --git a/packages/cli/src/modules/type-availability-policies/__tests__/package-resolver.test.ts b/packages/cli/src/modules/type-availability-policies/__tests__/package-resolver.test.ts index 72940387314..4b5b8db1473 100644 --- a/packages/cli/src/modules/type-availability-policies/__tests__/package-resolver.test.ts +++ b/packages/cli/src/modules/type-availability-policies/__tests__/package-resolver.test.ts @@ -3,6 +3,7 @@ import type { NodeTypes } from '@/node-types'; import { CREDENTIAL_TYPES_KIND, NODE_TYPES_KIND } from '../constants'; import { + loadedCheckFor, isCredentialTypeKnown, isPackageInstalled, packageResolverFor, @@ -168,3 +169,122 @@ describe('isCredentialTypeKnown', () => { expect(isCredentialTypeKnown(registry, 'constructor')).toBe(false); }); }); + +describe('loadedCheckFor', () => { + /** Stands in for `NodeTypes.resolveBaseName`: `slackTool` is a synthetic variant of `slack`. */ + const nodeTypes = { + resolveBaseName: (name: string) => ({ + baseName: name === 'n8n-nodes-base.slackTool' ? 'n8n-nodes-base.slack' : name, + isSyntheticTool: name === 'n8n-nodes-base.slackTool', + }), + } as unknown as NodeTypes; + + /** A stand-in for `LoadNodesAndCredentials`, built from just the fields this check reads. */ + function makeFullRegistry(overrides: { + loaders?: LoadNodesAndCredentials['loaders']; + knownNodes?: LoadNodesAndCredentials['knownNodes']; + knownCredentials?: LoadNodesAndCredentials['knownCredentials']; + }): LoadNodesAndCredentials { + return { + loaders: overrides.loaders ?? {}, + knownNodes: overrides.knownNodes ?? {}, + knownCredentials: overrides.knownCredentials ?? {}, + } as unknown as LoadNodesAndCredentials; + } + + describe('package selector', () => { + it('is true for a package that has a registered loader', () => { + const registry = makeFullRegistry({ + loaders: { 'n8n-nodes-base': {} as LoadNodesAndCredentials['loaders'][string] }, + }); + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, registry); + + expect(isLoaded({ kind: 'package', value: 'n8n-nodes-base' })).toBe(true); + }); + + it('is false for a package with no registered loader', () => { + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'package', value: 'n8n-nodes-not-installed' })).toBe(false); + }); + + it('is false for a package named after an inherited Object.prototype property', () => { + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'package', value: 'constructor' })).toBe(false); + }); + }); + + describe('extends selector', () => { + it('is true for a known credential type, checked the same way for either kind', () => { + const registry = makeFullRegistry({ + knownCredentials: { oAuth2Api: { className: '', sourcePath: '' } }, + }); + const selector = { kind: 'extends', value: 'oAuth2Api' } as const; + + expect(loadedCheckFor(NODE_TYPES_KIND, nodeTypes, registry)(selector)).toBe(true); + expect(loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, registry)(selector)).toBe(true); + }); + + it('is false for an unknown credential type', () => { + const isLoaded = loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'extends', value: 'unknownApi' })).toBe(false); + }); + + it('is false for a value named after an inherited Object.prototype property', () => { + const isLoaded = loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'extends', value: 'constructor' })).toBe(false); + }); + }); + + describe('name selector, node-types kind', () => { + it('is true when the resolved base name is a known node', () => { + const registry = makeFullRegistry({ + knownNodes: { 'n8n-nodes-base.slack': { className: '', sourcePath: '' } }, + }); + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, registry); + + expect(isLoaded({ kind: 'name', value: 'n8n-nodes-base.slack' })).toBe(true); + }); + + it('is false when the resolved base name is not a known node', () => { + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'name', value: 'n8n-nodes-base.slack' })).toBe(false); + }); + + it('treats a synthetic tool variant as installed when its base node is known', () => { + const registry = makeFullRegistry({ + knownNodes: { 'n8n-nodes-base.slack': { className: '', sourcePath: '' } }, + }); + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, registry); + + expect(isLoaded({ kind: 'name', value: 'n8n-nodes-base.slackTool' })).toBe(true); + }); + + it('is false for a name resolving to an inherited Object.prototype property', () => { + const isLoaded = loadedCheckFor(NODE_TYPES_KIND, nodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'name', value: 'constructor' })).toBe(false); + }); + }); + + describe('name selector, credential-types kind', () => { + it('is true when the value is a known credential type', () => { + const registry = makeFullRegistry({ + knownCredentials: { slackApi: { className: '', sourcePath: '' } }, + }); + const isLoaded = loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, registry); + + expect(isLoaded({ kind: 'name', value: 'slackApi' })).toBe(true); + }); + + it('is false when the value is not a known credential type', () => { + const isLoaded = loadedCheckFor(CREDENTIAL_TYPES_KIND, {} as NodeTypes, makeFullRegistry({})); + + expect(isLoaded({ kind: 'name', value: 'unknownApi' })).toBe(false); + }); + }); +}); diff --git a/packages/cli/src/modules/type-availability-policies/__tests__/policy-rule-lint.test.ts b/packages/cli/src/modules/type-availability-policies/__tests__/policy-rule-lint.test.ts new file mode 100644 index 00000000000..3a52ef67640 --- /dev/null +++ b/packages/cli/src/modules/type-availability-policies/__tests__/policy-rule-lint.test.ts @@ -0,0 +1,98 @@ +import { lintPolicyRules, lintRulesForMissingTypes } from '../policy-rule-lint'; +import type { PolicyRule, PolicySelector } from '../policy-rule.types'; + +type RuleOverrides = Partial & Pick; + +const rule = (overrides: RuleOverrides): PolicyRule => ({ + action: 'allow', + ...overrides, +}); + +describe('lintRulesForMissingTypes', () => { + it('returns no warnings when every rule is installed', () => { + const rules = [ + rule({ id: 'a', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'b', selector: { kind: 'package', value: 'n8n-nodes-base' } }), + ]; + + expect(lintRulesForMissingTypes(rules, () => true)).toEqual([]); + }); + + it('flags each rule whose selector is not installed, carrying its id', () => { + const rules = [ + rule({ id: 'installed', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'missing-one', selector: { kind: 'name', value: 'typeB' } }), + rule({ id: 'missing-two', selector: { kind: 'package', value: 'n8n-nodes-missing' } }), + ]; + const isLoaded = (selector: PolicySelector) => selector.value === 'typeA'; + + expect(lintRulesForMissingTypes(rules, isLoaded)).toEqual([ + { kind: 'notLoaded', ruleId: 'missing-one' }, + { kind: 'notLoaded', ruleId: 'missing-two' }, + ]); + }); + + it('returns no warnings for an empty rule list', () => { + expect(lintRulesForMissingTypes([], () => false)).toEqual([]); + }); +}); + +describe('lintPolicyRules', () => { + const policedType = (name: string) => ({ name, baseName: name }); + const resolvePackage = () => null; + + it('returns no warnings when every rule is installed and none are shadowed', () => { + const rules = [ + rule({ id: 'a', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'b', selector: { kind: 'name', value: 'typeB' } }), + ]; + + expect(lintPolicyRules(rules, { resolvePackage, policedType, isLoaded: () => true })).toEqual( + [], + ); + }); + + it('flags not-installed rules that are not shadowed, in rule order', () => { + const rules = [ + rule({ id: 'a', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'b', selector: { kind: 'name', value: 'typeB' } }), + ]; + const isLoaded = (selector: PolicySelector) => selector.value !== 'typeB'; + + expect(lintPolicyRules(rules, { resolvePackage, policedType, isLoaded })).toEqual([ + { kind: 'notLoaded', ruleId: 'b' }, + ]); + }); + + it('merges shadow and not-installed warnings, sorted by rule position rather than warning kind', () => { + const rules = [ + rule({ id: 'a', selector: { kind: 'name', value: 'typeA' } }), + rule({ id: 'b', selector: { kind: 'name', value: 'typeB' } }), + // identical selector to `a`, so shadowed by it + rule({ id: 'c', selector: { kind: 'name', value: 'typeA' } }), + ]; + const isLoaded = (selector: PolicySelector) => selector.value !== 'typeB'; + + expect(lintPolicyRules(rules, { resolvePackage, policedType, isLoaded })).toEqual([ + { kind: 'notLoaded', ruleId: 'b' }, + { kind: 'shadowed', ruleId: 'c', shadowedByRuleId: 'a' }, + ]); + }); + + it('lists the shadowed warning before the notLoaded warning for the same rule', () => { + const ruleX = rule({ id: 'x', selector: { kind: 'name', value: 'typeX' } }); + // identical selector to `ruleX`, so shadowed by it, and separately stubbed as not installed + const ruleY = rule({ id: 'y', selector: { kind: 'name', value: 'typeX' } }); + const notLoadedSelectors = new Set([ruleY.selector]); + const isLoaded = (selector: PolicySelector) => !notLoadedSelectors.has(selector); + + expect(lintPolicyRules([ruleX, ruleY], { resolvePackage, policedType, isLoaded })).toEqual([ + { kind: 'shadowed', ruleId: 'y', shadowedByRuleId: 'x' }, + { kind: 'notLoaded', ruleId: 'y' }, + ]); + }); + + it('returns no warnings for an empty rule list', () => { + expect(lintPolicyRules([], { resolvePackage, policedType, isLoaded: () => true })).toEqual([]); + }); +}); diff --git a/packages/cli/src/modules/type-availability-policies/__tests__/policy-shadow-lint.test.ts b/packages/cli/src/modules/type-availability-policies/__tests__/policy-shadow-lint.test.ts index 046ebead2ee..fa7eb81e590 100644 --- a/packages/cli/src/modules/type-availability-policies/__tests__/policy-shadow-lint.test.ts +++ b/packages/cli/src/modules/type-availability-policies/__tests__/policy-shadow-lint.test.ts @@ -26,7 +26,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules)).toEqual([ - { ruleId: 'deny-slack', shadowedByRuleId: 'allow-package' }, + { kind: 'shadowed', ruleId: 'deny-slack', shadowedByRuleId: 'allow-package' }, ]); }); @@ -66,7 +66,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules, undefined, policedType)).toEqual([ - { ruleId: 'deny-gmail-tool', shadowedByRuleId: 'allow-gmail' }, + { kind: 'shadowed', ruleId: 'deny-gmail-tool', shadowedByRuleId: 'allow-gmail' }, ]); }); @@ -109,7 +109,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules)).toEqual([ - { ruleId: 'deny-slack', shadowedByRuleId: 'allow-slack' }, + { kind: 'shadowed', ruleId: 'deny-slack', shadowedByRuleId: 'allow-slack' }, ]); }); @@ -121,8 +121,8 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules)).toEqual([ - { ruleId: 'second-package', shadowedByRuleId: 'first-package' }, - { ruleId: 'later-name', shadowedByRuleId: 'first-package' }, + { kind: 'shadowed', ruleId: 'second-package', shadowedByRuleId: 'first-package' }, + { kind: 'shadowed', ruleId: 'later-name', shadowedByRuleId: 'first-package' }, ]); }); @@ -137,7 +137,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(rules, resolvePackage)).toEqual([ - { ruleId: 'deny-slack-api', shadowedByRuleId: 'allow-package' }, + { kind: 'shadowed', ruleId: 'deny-slack-api', shadowedByRuleId: 'allow-package' }, ]); }); @@ -162,7 +162,7 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(nameFirst)).toEqual([ - { ruleId: 'later-name', shadowedByRuleId: 'earlier-name' }, + { kind: 'shadowed', ruleId: 'later-name', shadowedByRuleId: 'earlier-name' }, ]); const packageFirst = [ @@ -172,8 +172,8 @@ describe('lintRulesForShadowing', () => { ]; expect(lintRulesForShadowing(packageFirst)).toEqual([ - { ruleId: 'later-name-1', shadowedByRuleId: 'earlier-package' }, - { ruleId: 'later-name-2', shadowedByRuleId: 'earlier-package' }, + { kind: 'shadowed', ruleId: 'later-name-1', shadowedByRuleId: 'earlier-package' }, + { kind: 'shadowed', ruleId: 'later-name-2', shadowedByRuleId: 'earlier-package' }, ]); }); @@ -196,18 +196,18 @@ describe('lintRulesForShadowing', () => { it('flags a later name rule for a type built on an earlier family rule', () => { expect( lint([family('oauth2', 'oAuth2Api'), name('sheets', 'googleSheetsOAuth2Api')]), - ).toEqual([{ ruleId: 'sheets', shadowedByRuleId: 'oauth2' }]); + ).toEqual([{ kind: 'shadowed', ruleId: 'sheets', shadowedByRuleId: 'oauth2' }]); }); it('flags a later name rule for the family base itself', () => { expect(lint([family('oauth2', 'oAuth2Api'), name('generic', 'oAuth2Api')])).toEqual([ - { ruleId: 'generic', shadowedByRuleId: 'oauth2' }, + { kind: 'shadowed', ruleId: 'generic', shadowedByRuleId: 'oauth2' }, ]); }); it('flags a later family rule nested inside an earlier one', () => { expect(lint([family('oauth2', 'oAuth2Api'), family('google', 'googleOAuth2Api')])).toEqual([ - { ruleId: 'google', shadowedByRuleId: 'oauth2' }, + { kind: 'shadowed', ruleId: 'google', shadowedByRuleId: 'oauth2' }, ]); }); diff --git a/packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts b/packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts index 0b8252a6c30..da61853dc85 100644 --- a/packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts +++ b/packages/cli/src/modules/type-availability-policies/__tests__/type-availability-policy.service.test.ts @@ -46,6 +46,8 @@ const RULE: PolicyRule = { selector: { kind: 'name', value: 'n8n-nodes-base.slack' }, }; +const RULE_2: PolicyRule = { ...RULE, id: 'r2' }; + const DELEGATE_RULE: PolicyRule = { id: 'r-delegate', action: 'delegate', @@ -113,6 +115,11 @@ describe('TypeAvailabilityPolicyService', () => { scopeRepository.findScopeKeysByIds.mockResolvedValue([]); // Every fixture rule names `n8n-nodes-base`, so it must resolve as an installed package. loadNodesAndCredentials.loaders = { 'n8n-nodes-base': makeLoader('n8n-nodes-base') }; + // Fixture `name` rules name Slack, so it must resolve as a loaded node. + Object.defineProperty(loadNodesAndCredentials, 'knownNodes', { + configurable: true, + value: { 'n8n-nodes-base.slack': { className: '', sourcePath: '' } }, + }); nodeTypes.resolveBaseName.mockImplementation((name) => ({ baseName: name, isSyntheticTool: false, @@ -315,21 +322,72 @@ describe('TypeAvailabilityPolicyService', () => { const { warnings } = await service.createPolicyDocument(KIND, [RULE, shadowed], 'user-1'); - expect(warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(warnings).toEqual([{ kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }]); expect(policyRepository.createPolicy).toHaveBeenCalled(); }); - it('rejects a package rule naming a package that is not installed', async () => { + it('saves a package rule naming a package that is not installed, with a warning', async () => { + policyRepository.createPolicy.mockResolvedValue(makePolicy()); const rule: PolicyRule = { id: 'r1', action: 'deny', selector: { kind: 'package', value: 'n8n-nodes-not-installed' }, }; - await expect(service.createPolicyDocument(KIND, [rule], 'user-1')).rejects.toThrow( - 'Package rule names a package that is not installed: n8n-nodes-not-installed', + const { warnings } = await service.createPolicyDocument(KIND, [rule], 'user-1'); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); + expect(policyRepository.createPolicy).toHaveBeenCalled(); + }); + + it('saves a name rule naming a node type that is not loaded, with a warning', async () => { + policyRepository.createPolicy.mockResolvedValue(makePolicy()); + const rule: PolicyRule = { + id: 'r1', + action: 'deny', + selector: { kind: 'name', value: 'n8n-nodes-base.notLoaded' }, + }; + + const { warnings } = await service.createPolicyDocument(KIND, [rule], 'user-1'); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); + }); + + it('checks a synthetic tool variant rule against its base node', async () => { + policyRepository.createPolicy.mockResolvedValue(makePolicy()); + nodeTypes.resolveBaseName.mockImplementation((name) => ({ + baseName: name.replace(/Tool$/, ''), + isSyntheticTool: name.endsWith('Tool'), + })); + const rules: PolicyRule[] = [ + { id: 'r1', action: 'deny', selector: { kind: 'name', value: 'n8n-nodes-base.slackTool' } }, + { id: 'r2', action: 'deny', selector: { kind: 'name', value: 'n8n-nodes-base.fooTool' } }, + ]; + + const { warnings } = await service.createPolicyDocument(KIND, rules, 'user-1'); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r2' }]); + }); + + it('lists a shadowed warning before a notLoaded warning for the same rule, in rule order', async () => { + policyRepository.createPolicy.mockResolvedValue(makePolicy()); + const missing = (id: string): PolicyRule => ({ + id, + action: 'deny', + selector: { kind: 'name', value: 'n8n-nodes-base.notLoaded' }, + }); + + const { warnings } = await service.createPolicyDocument( + KIND, + [missing('r1'), RULE_2, missing('r3')], + 'user-1', ); - expect(policyRepository.createPolicy).not.toHaveBeenCalled(); + + expect(warnings).toEqual([ + { kind: 'notLoaded', ruleId: 'r1' }, + { kind: 'shadowed', ruleId: 'r3', shadowedByRuleId: 'r1' }, + { kind: 'notLoaded', ruleId: 'r3' }, + ]); }); describe('extends rules', () => { @@ -360,17 +418,37 @@ describe('TypeAvailabilityPolicyService', () => { expect(policyRepository.createPolicy).not.toHaveBeenCalled(); }); - it('rejects an extends rule naming a credential type that is not installed', async () => { - await expect( - service.createPolicyDocument( - CREDENTIAL_TYPES_KIND, - [familyRule('r1', 'notInstalledApi')], - 'user-1', - ), - ).rejects.toThrow( - 'Extends rule names a credential type that is not installed: notInstalledApi', + it('saves an extends rule naming a credential type that is not installed, with a warning', async () => { + policyRepository.createPolicy.mockResolvedValue( + makePolicy({ kind: CREDENTIAL_TYPES_KIND }), ); - expect(policyRepository.createPolicy).not.toHaveBeenCalled(); + + const { warnings } = await service.createPolicyDocument( + CREDENTIAL_TYPES_KIND, + [familyRule('r1', 'notLoadedApi')], + 'user-1', + ); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); + }); + + it('saves a credential name rule naming a type that is not installed, with a warning', async () => { + policyRepository.createPolicy.mockResolvedValue( + makePolicy({ kind: CREDENTIAL_TYPES_KIND }), + ); + const rule: PolicyRule = { + id: 'r1', + action: 'deny', + selector: { kind: 'name', value: 'notLoadedApi' }, + }; + + const { warnings } = await service.createPolicyDocument( + CREDENTIAL_TYPES_KIND, + [rule], + 'user-1', + ); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); }); it('warns about a derived-type rule placed after the family rule that covers it', async () => { @@ -389,7 +467,7 @@ describe('TypeAvailabilityPolicyService', () => { 'user-1', ); - expect(warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(warnings).toEqual([{ kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }]); }); }); @@ -438,7 +516,7 @@ describe('TypeAvailabilityPolicyService', () => { }; await expect(service.createPolicyDocument(KIND, [rule], 'user-1')).rejects.toThrow( - 'Package rule names a package that is not installed: n8n-creds-base', + 'Package rule names "n8n-creds-base", which no package provides. Write a credential type rule instead.', ); expect(policyRepository.createPolicy).not.toHaveBeenCalled(); }); @@ -451,19 +529,29 @@ describe('TypeAvailabilityPolicyService', () => { scopeRepository.containsProjectScope.mockResolvedValue(false); }); - it('rejects a package rule naming a package that is not installed, before opening a transaction', async () => { - const rule: PolicyRule = { - id: 'r1', + it('keeps accepting edits to a document whose package rule names an uninstalled package', async () => { + const missingPackage: PolicyRule = { + id: 'r-missing', action: 'deny', - selector: { kind: 'package', value: 'n8n-nodes-not-installed' }, + selector: { kind: 'package', value: 'n8n-nodes-mcp' }, }; - - await expect( - service.updatePolicyDocument(KIND, 'policy-1', [rule], 1, 'user-1'), - ).rejects.toThrow( - 'Package rule names a package that is not installed: n8n-nodes-not-installed', + policyRepository.findByIdAndKind.mockResolvedValue( + makePolicy({ rules: [missingPackage], version: 1 }), ); - expect(transactionRunner.run).not.toHaveBeenCalled(); + policyRepository.updateRules.mockResolvedValue( + makePolicy({ rules: [missingPackage, RULE], version: 2 }), + ); + + const { warnings } = await service.updatePolicyDocument( + KIND, + 'policy-1', + [missingPackage, RULE], + 1, + 'user-1', + ); + + expect(warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r-missing' }]); + expect(policyRepository.updateRules).toHaveBeenCalled(); }); it('rejects a node type rule naming a credential-only node, before opening a transaction', async () => { @@ -868,11 +956,11 @@ describe('TypeAvailabilityPolicyService', () => { attachmentRepository.listScopeIdsAttachedToPolicy.mockResolvedValue(['scope-1']); }); - it('rejects a package rule naming a package that is not installed, before opening a transaction', async () => { + it('rejects a package rule on the credential-only prefix, before opening a transaction', async () => { const rule: PolicyRule = { id: 'r1', action: 'deny', - selector: { kind: 'package', value: 'n8n-nodes-not-installed' }, + selector: { kind: 'package', value: 'n8n-creds-base' }, }; await expect( @@ -883,9 +971,7 @@ describe('TypeAvailabilityPolicyService', () => { 0, 'user-1', ), - ).rejects.toThrow( - 'Package rule names a package that is not installed: n8n-nodes-not-installed', - ); + ).rejects.toThrow('Package rule names "n8n-creds-base"'); expect(transactionRunner.run).not.toHaveBeenCalled(); }); @@ -1137,7 +1223,8 @@ describe('TypeAvailabilityPolicyService', () => { after: { defaultAction: 'deny', version: 1 }, rulesBefore: null, rulesAfter: [RULE], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); }); @@ -1188,10 +1275,47 @@ describe('TypeAvailabilityPolicyService', () => { after: { defaultAction: 'deny', version: 2 }, rulesBefore: [], rulesAfter: [RULE], - warningCount: 0, + shadowWarningCount: 0, + notLoadedWarningCount: 0, }); }); + it('saves a rule for an uninstalled package, returns its warning and counts it apart', async () => { + const missingPackage: PolicyRule = { + id: 'r-missing', + action: 'deny', + selector: { kind: 'package', value: 'n8n-nodes-mcp' }, + }; + const scope = makeScope({ defaultAction: 'allow', version: 1 }); + scopeRepository.findScopeByKindAndProject.mockResolvedValue(scope); + const existingPolicy = makePolicy({ rules: [missingPackage], version: 1 }); + attachmentRepository.listAttachmentsForScope.mockResolvedValue([ + { policyId: existingPolicy.id, rules: [missingPackage], priority: 0, isFloor: false }, + ]); + policyRepository.findByIdAndKind.mockResolvedValue(existingPolicy); + policyRepository.updateRules.mockResolvedValue( + makePolicy({ rules: [missingPackage, RULE, RULE_2], version: 2 }), + ); + scopeRepository.findScopeById.mockResolvedValue(makeScope({ version: 2 })); + + const result = await service.setEffectivePolicy( + KIND, + null, + { rules: [missingPackage, RULE, RULE_2], defaultAction: 'allow' }, + 1, + 'user-2', + ); + + expect(result.warnings).toEqual([ + { kind: 'notLoaded', ruleId: 'r-missing' }, + { kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }, + ]); + expect(eventService.emit).toHaveBeenCalledWith( + 'node-type-policy-saved', + expect.objectContaining({ shadowWarningCount: 1, notLoadedWarningCount: 1 }), + ); + }); + it('throws NotFoundError when the document update unexpectedly finds no row', async () => { const scope = makeScope({ defaultAction: 'allow', version: 1 }); scopeRepository.findScopeByKindAndProject.mockResolvedValue(scope); diff --git a/packages/cli/src/modules/type-availability-policies/package-resolver.ts b/packages/cli/src/modules/type-availability-policies/package-resolver.ts index f0dfe5707a9..30f6f7dffce 100644 --- a/packages/cli/src/modules/type-availability-policies/package-resolver.ts +++ b/packages/cli/src/modules/type-availability-policies/package-resolver.ts @@ -7,6 +7,7 @@ import { type PackageResolver, type PolicedType, } from './policy-evaluator'; +import type { PolicySelector } from './policy-rule.types'; /** * A credential type name (e.g. `slackApi`) carries no package prefix, unlike a node type @@ -93,7 +94,7 @@ export function policedTypeFor( }); } -/** Whether a credential type is known, checked at write time for an `extends` rule's value. */ +/** Whether a credential type is known, checked at write time for a rule's value. */ export function isCredentialTypeKnown( loadNodesAndCredentials: LoadNodesAndCredentials, typeName: string, @@ -102,8 +103,7 @@ export function isCredentialTypeKnown( } /** - * Whether `packageName` names a package this instance actually has loaded — checked at write - * time so a `package` rule can never be saved unable to match anything. + * Whether `packageName` names a package this instance actually has loaded. * * `Object.hasOwn`, not `in`: `in` also matches an inherited property, so a rule naming * `toString` or `constructor` would pass this check through `Object.prototype` even though @@ -115,3 +115,30 @@ export function isPackageInstalled( ): boolean { return Object.hasOwn(loadNodesAndCredentials.loaders, packageName); } + +/** + * Whether a selector names something this instance has loaded, for the write-time + * `notLoaded` warning. A `name` rule on a synthetic tool variant checks its base node. + */ +export function loadedCheckFor( + kind: string, + nodeTypes: NodeTypes, + loadNodesAndCredentials: LoadNodesAndCredentials, +): (selector: PolicySelector) => boolean { + const policedType = policedTypeFor(kind, nodeTypes, loadNodesAndCredentials); + const isNameKnown = + kind === NODE_TYPES_KIND + ? (name: string) => Object.hasOwn(loadNodesAndCredentials.knownNodes, name) + : (name: string) => isCredentialTypeKnown(loadNodesAndCredentials, name); + + return (selector) => { + switch (selector.kind) { + case 'package': + return isPackageInstalled(loadNodesAndCredentials, selector.value); + case 'extends': + return isCredentialTypeKnown(loadNodesAndCredentials, selector.value); + case 'name': + return isNameKnown(policedType(selector.value).baseName); + } + }; +} diff --git a/packages/cli/src/modules/type-availability-policies/policy-rule-lint.ts b/packages/cli/src/modules/type-availability-policies/policy-rule-lint.ts new file mode 100644 index 00000000000..03d639e43f7 --- /dev/null +++ b/packages/cli/src/modules/type-availability-policies/policy-rule-lint.ts @@ -0,0 +1,48 @@ +import type { PackageResolver, PolicedType } from './policy-evaluator'; +import type { PolicyRule, PolicySelector } from './policy-rule.types'; +import { lintRulesForShadowing, type ShadowWarning } from './policy-shadow-lint'; + +/** + * `ruleId` names a package or type this instance has not loaded. The rule still applies by + * name, for example to an imported workflow. `NODES_EXCLUDE` and `NODES_INCLUDE` count too. + */ +export type NotLoadedWarning = { + readonly kind: 'notLoaded'; + readonly ruleId: string; +}; + +export type PolicyRuleWarning = ShadowWarning | NotLoadedWarning; + +/** + * Warns instead of rejecting: a package can be uninstalled after its rule is saved, and the + * rule must not block later, unrelated writes to the same policy. + */ +export function lintRulesForMissingTypes( + rules: readonly PolicyRule[], + isLoaded: (selector: PolicySelector) => boolean, +): NotLoadedWarning[] { + return rules + .filter((rule) => !isLoaded(rule.selector)) + .map((rule) => ({ kind: 'notLoaded', ruleId: rule.id })); +} + +/** + * Every warning for a rule list, in rule order. A rule with both kinds lists `shadowed` first, + * since the sort is stable. + */ +export function lintPolicyRules( + rules: readonly PolicyRule[], + resolvers: { + resolvePackage: PackageResolver; + policedType: (name: string) => PolicedType; + isLoaded: (selector: PolicySelector) => boolean; + }, +): PolicyRuleWarning[] { + const position = new Map(rules.map((rule, index) => [rule.id, index])); + const warnings: PolicyRuleWarning[] = [ + ...lintRulesForShadowing(rules, resolvers.resolvePackage, resolvers.policedType), + ...lintRulesForMissingTypes(rules, resolvers.isLoaded), + ]; + + return warnings.sort((a, b) => (position.get(a.ruleId) ?? 0) - (position.get(b.ruleId) ?? 0)); +} diff --git a/packages/cli/src/modules/type-availability-policies/policy-shadow-lint.ts b/packages/cli/src/modules/type-availability-policies/policy-shadow-lint.ts index e28f7c25abe..f882a824a3a 100644 --- a/packages/cli/src/modules/type-availability-policies/policy-shadow-lint.ts +++ b/packages/cli/src/modules/type-availability-policies/policy-shadow-lint.ts @@ -10,6 +10,7 @@ import type { PolicyRule, PolicySelector } from './policy-rule.types'; * appears earlier in the same rule list and matches every type that `ruleId` would match. */ export type ShadowWarning = { + readonly kind: 'shadowed'; readonly ruleId: string; readonly shadowedByRuleId: string; }; @@ -113,7 +114,7 @@ export function lintRulesForShadowing( } if (shadowedBy) { - warnings.push({ ruleId: rule.id, shadowedByRuleId: shadowedBy.rule.id }); + warnings.push({ kind: 'shadowed', ruleId: rule.id, shadowedByRuleId: shadowedBy.rule.id }); } const firstOccurrenceByValue = firstByKind[selector.kind]; diff --git a/packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts b/packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts index bae30f94d28..5d9b21db26f 100644 --- a/packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts +++ b/packages/cli/src/modules/type-availability-policies/type-availability-policy.service.ts @@ -6,6 +6,7 @@ import { TransactionRunner, type OperationContext } from '@n8n/db'; import { Service } from '@n8n/di'; import { LRUCache } from 'lru-cache'; import { + CREDENTIAL_ONLY_NODE_PREFIX, getCredentialOnlyNodeCredentialType, isCredentialOnlyNodeType, OperationalError, @@ -22,12 +23,7 @@ import { TypeAvailabilityPolicyRepository } from './database/repositories/type-a import type { TypeAvailabilityPolicy } from './database/entities/type-availability-policy.entity'; import type { TypeAvailabilityPolicyScope } from './database/entities/type-availability-policy-scope.entity'; import { CREDENTIAL_TYPES_KIND, NODE_TYPES_KIND } from './constants'; -import { - isCredentialTypeKnown, - isPackageInstalled, - packageResolverFor, - policedTypeFor, -} from './package-resolver'; +import { loadedCheckFor, packageResolverFor, policedTypeFor } from './package-resolver'; import { evaluateComposedType, orderedAttachments, type ComposedVerdict } from './policy-evaluator'; import type { PolicyAction, @@ -35,7 +31,7 @@ import type { PolicyRule, PolicyScopeKey, } from './policy-rule.types'; -import { lintRulesForShadowing, type ShadowWarning } from './policy-shadow-lint'; +import { lintPolicyRules, type PolicyRuleWarning } from './policy-rule-lint'; /** * A scope that has never been written has no row. That "unconfigured" state behaves as @@ -187,7 +183,7 @@ export type ComposedTypeEvaluation = { type PolicyDocumentWrite = { readonly policy: TypeAvailabilityPolicy; - readonly warnings: readonly ShadowWarning[]; + readonly warnings: readonly PolicyRuleWarning[]; }; function flattenRules(attachments: readonly PolicyAttachment[]): PolicyRule[] { @@ -226,33 +222,6 @@ function assertNoDelegateAtProjectScope( } } -/** - * A `package` rule that names a package this instance never loaded can never match anything — - * worse than not supporting the rule at all, since it looks like it works. Checked at every - * write so a policy is never saved with one. - */ -function assertPackagesInstalled( - rules: readonly PolicyRule[], - loadNodesAndCredentials: LoadNodesAndCredentials, -): void { - const missing = new Set(); - - for (const rule of rules) { - if ( - rule.selector.kind === 'package' && - !isPackageInstalled(loadNodesAndCredentials, rule.selector.value) - ) { - missing.add(rule.selector.value); - } - } - - if (missing.size > 0) { - throw new UserError( - `Package rule names a package that is not installed: ${[...missing].join(', ')}`, - ); - } -} - /** * A credential-only node (`n8n-creds-base.`) exists only in the editor. It is stored and * executed as `n8n-nodes-base.httpRequest`, so a node type rule on the generated name would @@ -273,30 +242,25 @@ function assertNoCredentialOnlyNodeRules(kind: string, rules: readonly PolicyRul } /** - * Only credential types declare `extends`, so the selector means nothing for another kind. An - * unknown base would match nothing, the same trap `assertPackagesInstalled` closes. + * No loader provides the credential-only prefix, so a `package` rule on it can never match on + * any instance. Unlike a package that is merely not installed, nothing can fix it later. */ -function assertExtendsSelectorsValid( - kind: string, - rules: readonly PolicyRule[], - loadNodesAndCredentials: LoadNodesAndCredentials, -): void { - const values = rules.flatMap((rule) => - rule.selector.kind === 'extends' ? [rule.selector.value] : [], - ); - if (values.length === 0) return; - - if (kind !== CREDENTIAL_TYPES_KIND) { - throw new UserError('An "extends" rule is only valid in a credential type policy'); +function assertNoCredentialOnlyPackageRules(rules: readonly PolicyRule[]): void { + for (const rule of rules) { + if (rule.selector.kind === 'package' && rule.selector.value === CREDENTIAL_ONLY_NODE_PREFIX) { + throw new UserError( + `Package rule names "${CREDENTIAL_ONLY_NODE_PREFIX}", which no package provides. Write a credential type rule instead.`, + ); + } } +} - const unknown = new Set( - values.filter((value) => !isCredentialTypeKnown(loadNodesAndCredentials, value)), - ); - if (unknown.size > 0) { - throw new UserError( - `Extends rule names a credential type that is not installed: ${[...unknown].join(', ')}`, - ); +/** Only credential types declare `extends`, so the selector means nothing for another kind. */ +function assertExtendsSelectorsValid(kind: string, rules: readonly PolicyRule[]): void { + if (kind === CREDENTIAL_TYPES_KIND) return; + + if (rules.some((rule) => rule.selector.kind === 'extends')) { + throw new UserError('An "extends" rule is only valid in a credential type policy'); } } @@ -490,19 +454,28 @@ export class TypeAvailabilityPolicyService { return scope; } + /** + * Rejects a rule that is malformed for this kind. A rule that only names something not + * loaded saves, and comes back as a `notLoaded` warning. + */ + private validateAndLintRules(kind: string, rules: readonly PolicyRule[]): PolicyRuleWarning[] { + assertNoCredentialOnlyPackageRules(rules); + assertNoCredentialOnlyNodeRules(kind, rules); + assertExtendsSelectorsValid(kind, rules); + + return lintPolicyRules(rules, { + resolvePackage: packageResolverFor(kind, this.loadNodesAndCredentials), + policedType: policedTypeFor(kind, this.nodeTypes, this.loadNodesAndCredentials), + isLoaded: loadedCheckFor(kind, this.nodeTypes, this.loadNodesAndCredentials), + }); + } + async createPolicyDocument( kind: string, rules: readonly PolicyRule[], updatedBy: string, ): Promise { - assertPackagesInstalled(rules, this.loadNodesAndCredentials); - assertNoCredentialOnlyNodeRules(kind, rules); - assertExtendsSelectorsValid(kind, rules, this.loadNodesAndCredentials); - const warnings = lintRulesForShadowing( - rules, - packageResolverFor(kind, this.loadNodesAndCredentials), - policedTypeFor(kind, this.nodeTypes, this.loadNodesAndCredentials), - ); + const warnings = this.validateAndLintRules(kind, rules); const policy = await this.policyRepository.createPolicy({ kind, rules, updatedBy }, {}); @@ -535,14 +508,7 @@ export class TypeAvailabilityPolicyService { expectedVersion: number, updatedBy: string, ): Promise { - assertPackagesInstalled(rules, this.loadNodesAndCredentials); - assertNoCredentialOnlyNodeRules(kind, rules); - assertExtendsSelectorsValid(kind, rules, this.loadNodesAndCredentials); - const warnings = lintRulesForShadowing( - rules, - packageResolverFor(kind, this.loadNodesAndCredentials), - policedTypeFor(kind, this.nodeTypes, this.loadNodesAndCredentials), - ); + const warnings = this.validateAndLintRules(kind, rules); const result = await this.transactionRunner.run({}, async (ctx) => { let invalidated: PolicyScopeKey[] = []; @@ -791,18 +757,10 @@ export class TypeAvailabilityPolicyService { defaultAction: PolicyAction; version: number; rules: PolicyRule[]; - warnings: readonly ShadowWarning[]; + warnings: readonly PolicyRuleWarning[]; }> { assertNoDelegateAtProjectScope(projectId, input.defaultAction, input.rules); - assertPackagesInstalled(input.rules, this.loadNodesAndCredentials); - assertNoCredentialOnlyNodeRules(kind, input.rules); - assertExtendsSelectorsValid(kind, input.rules, this.loadNodesAndCredentials); - - const warnings = lintRulesForShadowing( - input.rules, - packageResolverFor(kind, this.loadNodesAndCredentials), - policedTypeFor(kind, this.nodeTypes, this.loadNodesAndCredentials), - ); + const warnings = this.validateAndLintRules(kind, input.rules); const result = await this.transactionRunner.run({}, async (ctx) => { const scope = await this.scopeRepository.findScopeByKindAndProject( @@ -991,7 +949,8 @@ export class TypeAvailabilityPolicyService { after: result.scopeAfter, rulesBefore: result.documentBefore?.rules ?? null, rulesAfter: result.documentAfter.rules, - warningCount: warnings.length, + shadowWarningCount: warnings.filter((warning) => warning.kind === 'shadowed').length, + notLoadedWarningCount: warnings.filter((warning) => warning.kind === 'notLoaded').length, }); return { diff --git a/packages/cli/src/public-api/v1/controllers/__tests__/credential-type-policies.public.controller.test.ts b/packages/cli/src/public-api/v1/controllers/__tests__/credential-type-policies.public.controller.test.ts index e562e12c7c3..07aae9803fb 100644 --- a/packages/cli/src/public-api/v1/controllers/__tests__/credential-type-policies.public.controller.test.ts +++ b/packages/cli/src/public-api/v1/controllers/__tests__/credential-type-policies.public.controller.test.ts @@ -162,7 +162,7 @@ describe('CredentialTypePoliciesPublicController handler bodies', () => { defaultAction: 'deny' as const, version: 4, rules, - warnings: [{ ruleId: 'r2', shadowedByRuleId: 'r1' }], + warnings: [{ kind: 'shadowed' as const, ruleId: 'r2', shadowedByRuleId: 'r1' }], }; it('getCredentialTypeInstancePolicy reads the null-project scope and maps the response', async () => { diff --git a/packages/cli/src/public-api/v1/controllers/__tests__/node-type-policies.public.controller.test.ts b/packages/cli/src/public-api/v1/controllers/__tests__/node-type-policies.public.controller.test.ts index 62cf37e7d18..11b27368a0b 100644 --- a/packages/cli/src/public-api/v1/controllers/__tests__/node-type-policies.public.controller.test.ts +++ b/packages/cli/src/public-api/v1/controllers/__tests__/node-type-policies.public.controller.test.ts @@ -157,7 +157,7 @@ describe('NodeTypePoliciesPublicController handler bodies', () => { defaultAction: 'deny' as const, version: 4, rules, - warnings: [{ ruleId: 'r2', shadowedByRuleId: 'r1' }], + warnings: [{ kind: 'shadowed' as const, ruleId: 'r2', shadowedByRuleId: 'r1' }], }; it('fails loudly instead of returning an extends rule, which no node type write accepts', async () => { diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyDocumentWriteResultPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyDocumentWriteResultPublicDto.generated.yml index 094930d6fec..ae8d9b803d7 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyDocumentWriteResultPublicDto.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyDocumentWriteResultPublicDto.generated.yml @@ -88,15 +88,33 @@ properties: warnings: type: array items: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - shadowed + ruleId: + type: string + shadowedByRuleId: + type: string + required: + - kind + - ruleId + - shadowedByRuleId + - type: object + properties: + kind: + type: string + enum: + - notLoaded + ruleId: + type: string + required: + - kind + - ruleId type: object - properties: - ruleId: - type: string - shadowedByRuleId: - type: string - required: - - ruleId - - shadowedByRuleId required: - policy - warnings diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyEffectiveWriteResultPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyEffectiveWriteResultPublicDto.generated.yml index 8c3b41c4f29..88dcc1b54b0 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyEffectiveWriteResultPublicDto.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/credentialTypePolicyEffectiveWriteResultPublicDto.generated.yml @@ -73,15 +73,33 @@ properties: warnings: type: array items: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - shadowed + ruleId: + type: string + shadowedByRuleId: + type: string + required: + - kind + - ruleId + - shadowedByRuleId + - type: object + properties: + kind: + type: string + enum: + - notLoaded + ruleId: + type: string + required: + - kind + - ruleId type: object - properties: - ruleId: - type: string - shadowedByRuleId: - type: string - required: - - ruleId - - shadowedByRuleId required: - scopeId - rules diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/policyDocumentWriteResultPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/policyDocumentWriteResultPublicDto.generated.yml index 421646a75f1..f92f77e2be8 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/policyDocumentWriteResultPublicDto.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/policyDocumentWriteResultPublicDto.generated.yml @@ -75,15 +75,33 @@ properties: warnings: type: array items: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - shadowed + ruleId: + type: string + shadowedByRuleId: + type: string + required: + - kind + - ruleId + - shadowedByRuleId + - type: object + properties: + kind: + type: string + enum: + - notLoaded + ruleId: + type: string + required: + - kind + - ruleId type: object - properties: - ruleId: - type: string - shadowedByRuleId: - type: string - required: - - ruleId - - shadowedByRuleId required: - policy - warnings diff --git a/packages/cli/src/public-api/v1/shared/spec/schemas/policyEffectiveWriteResultPublicDto.generated.yml b/packages/cli/src/public-api/v1/shared/spec/schemas/policyEffectiveWriteResultPublicDto.generated.yml index 93194bd4e1c..97bb09c5ef2 100644 --- a/packages/cli/src/public-api/v1/shared/spec/schemas/policyEffectiveWriteResultPublicDto.generated.yml +++ b/packages/cli/src/public-api/v1/shared/spec/schemas/policyEffectiveWriteResultPublicDto.generated.yml @@ -60,15 +60,33 @@ properties: warnings: type: array items: + oneOf: + - type: object + properties: + kind: + type: string + enum: + - shadowed + ruleId: + type: string + shadowedByRuleId: + type: string + required: + - kind + - ruleId + - shadowedByRuleId + - type: object + properties: + kind: + type: string + enum: + - notLoaded + ruleId: + type: string + required: + - kind + - ruleId type: object - properties: - ruleId: - type: string - shadowedByRuleId: - type: string - required: - - ruleId - - shadowedByRuleId required: - scopeId - rules diff --git a/packages/cli/test/integration/policy/credential-type-policy.extends.test.ts b/packages/cli/test/integration/policy/credential-type-policy.extends.test.ts index 73a34fd05f1..d00bc771428 100644 --- a/packages/cli/test/integration/policy/credential-type-policy.extends.test.ts +++ b/packages/cli/test/integration/policy/credential-type-policy.extends.test.ts @@ -183,19 +183,19 @@ describe('a rule on a base credential type', () => { }); describe('writing an extends rule', () => { - test('rejects a base credential type that is not installed', async () => { + test('saves a base credential type that is not installed, with a warning', async () => { const response = await ownerAgent.put('/credential-type-policies/instance').send({ - rules: [ - { id: 'r1', action: 'deny', selector: { kind: 'extends', value: 'notInstalledApi' } }, - ], + rules: [{ id: 'r1', action: 'deny', selector: { kind: 'extends', value: 'notLoadedApi' } }], defaultAction: 'allow', version: 0, }); - expect(response.statusCode).toBe(400); - expect(response.body.message).toBe( - 'Extends rule names a credential type that is not installed: notInstalledApi', - ); + expect(response.statusCode).toBe(200); + const persisted = await ownerAgent.get('/credential-type-policies/instance'); + expect(persisted.body.data.rules).toEqual([ + { id: 'r1', action: 'deny', selector: { kind: 'extends', value: 'notLoadedApi' } }, + ]); + expect(response.body.data.warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r1' }]); }); test('rejects the rule in a node type policy request', async () => { diff --git a/packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts b/packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts index ab51a01ae98..8e6976ca3cc 100644 --- a/packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts +++ b/packages/cli/test/integration/policy/type-availability-policy-instance.controller.test.ts @@ -32,8 +32,7 @@ beforeAll(async () => { owner = await createOwner(); member = await createMember(); - // A `package` rule is rejected at write time unless the package is actually loaded — see - // `n8n-nodes-base` used as a stand-in for "an installed package" below. + // `n8n-nodes-base` stands in for an installed package, so its rules save without a warning. Container.get(LoadNodesAndCredentials).loaders = { 'n8n-nodes-base': mock({ known: { nodes: {}, credentials: {} } }), }; @@ -236,6 +235,29 @@ describe('node type availability policy instance controller admin happy path', ( expect(persisted.body.data.rules).toEqual([rule('r2', 'c.d')]); }); + test('a package rule for an uninstalled package saves with a warning, and does not block later writes', async () => { + const agent = testServer.authAgentFor(owner); + const missingPackage = { + id: 'r-mcp', + action: 'deny' as const, + selector: { kind: 'package' as const, value: 'n8n-nodes-mcp' }, + }; + + const first = await agent + .put('/node-type-policies/instance') + .send({ rules: [missingPackage], defaultAction: 'allow', version: 0 }); + expect(first.statusCode).toBe(200); + expect(first.body.data.warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r-mcp' }]); + + const unrelatedChange = await agent.put('/node-type-policies/instance').send({ + rules: [missingPackage], + defaultAction: 'deny', + version: first.body.data.version, + }); + expect(unrelatedChange.statusCode).toBe(200); + expect(unrelatedChange.body.data.warnings).toEqual([{ kind: 'notLoaded', ruleId: 'r-mcp' }]); + }); + test('a PUT /instance that changes the rules and the default action together moves the version by one', async () => { const agent = testServer.authAgentFor(owner); diff --git a/packages/cli/test/integration/public-api/credential-type-policies.test.ts b/packages/cli/test/integration/public-api/credential-type-policies.test.ts index b17e811bab9..afa32491bf1 100644 --- a/packages/cli/test/integration/public-api/credential-type-policies.test.ts +++ b/packages/cli/test/integration/public-api/credential-type-policies.test.ts @@ -61,6 +61,13 @@ afterEach(async () => { ]); }); +// Fixture rules name `slackApi`; without it loaded, every write would also warn `notLoaded`. +beforeEach(() => { + vi.spyOn(Container.get(LoadNodesAndCredentials), 'knownCredentials', 'get').mockReturnValue({ + slackApi: { className: '', sourcePath: '' }, + }); +}); + /** * Least privilege first: the API-key scope check and the RBAC check are independent, so a key * that lacks the scope and a key whose user lacks the RBAC grant are both rejected with 403. @@ -425,7 +432,9 @@ describe('credential type policies public API policy documents', () => { .send({ rules: [DENY_RULE, { ...DENY_RULE, id: 'r2', action: 'allow' }] }); expect(response.statusCode).toBe(201); - expect(response.body.warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(response.body.warnings).toEqual([ + { kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }, + ]); }); test('POST rejects duplicate rule ids with 400', async () => { diff --git a/packages/cli/test/integration/public-api/node-type-policies.test.ts b/packages/cli/test/integration/public-api/node-type-policies.test.ts index be6ce527447..7076f8d939f 100644 --- a/packages/cli/test/integration/public-api/node-type-policies.test.ts +++ b/packages/cli/test/integration/public-api/node-type-policies.test.ts @@ -4,6 +4,7 @@ import { LICENSE_FEATURES } from '@n8n/constants'; import type { Project, User } from '@n8n/db'; import { Container } from '@n8n/di'; +import { LoadNodesAndCredentials } from '@/load-nodes-and-credentials'; import { TypeAvailabilityPolicyRepository } from '@/modules/type-availability-policies/database/repositories/type-availability-policy.repository'; import { createMemberWithApiKey, createOwnerWithApiKey } from '@test-integration/db/users'; import * as utils from '@test-integration/utils'; @@ -56,6 +57,13 @@ afterEach(async () => { ]); }); +// Fixture rules name `a.b`; without it loaded, every write would also warn `notLoaded`. +beforeEach(() => { + vi.spyOn(Container.get(LoadNodesAndCredentials), 'knownNodes', 'get').mockReturnValue({ + 'a.b': { className: '', sourcePath: '' }, + }); +}); + /** * Least privilege first: the API-key scope check and the RBAC check are independent, so a key * that lacks the scope and a key whose user lacks the RBAC grant are both rejected with 403. @@ -416,7 +424,9 @@ describe('node type policies public API policy documents', () => { .send({ rules: [DENY_RULE, { ...DENY_RULE, id: 'r2', action: 'allow' }] }); expect(response.statusCode).toBe(201); - expect(response.body.warnings).toEqual([{ ruleId: 'r2', shadowedByRuleId: 'r1' }]); + expect(response.body.warnings).toEqual([ + { kind: 'shadowed', ruleId: 'r2', shadowedByRuleId: 'r1' }, + ]); }); test('POST rejects duplicate rule ids with 400', async () => { From 21221adc78ff3bb5695c96b487418329e75484be Mon Sep 17 00:00:00 2001 From: "n8n-cat-bot[bot]" <283985454+n8n-cat-bot[bot]@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:56:23 +0000 Subject: [PATCH 5/7] fix: Validate attendee values (#40953) Co-authored-by: n8n-cat-bot[bot] --- .../nodes/Google/Calendar/GenericFunctions.ts | 25 +++ .../Google/Calendar/GoogleCalendar.node.ts | 34 +--- .../Calendar/test/node/event.create.test.ts | 175 ++++++++++++++++++ .../Calendar/test/node/event.update.test.ts | 48 ++++- 4 files changed, 250 insertions(+), 32 deletions(-) create mode 100644 packages/nodes-base/nodes/Google/Calendar/test/node/event.create.test.ts diff --git a/packages/nodes-base/nodes/Google/Calendar/GenericFunctions.ts b/packages/nodes-base/nodes/Google/Calendar/GenericFunctions.ts index ce7dadd4f9e..d542208ecbb 100644 --- a/packages/nodes-base/nodes/Google/Calendar/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Google/Calendar/GenericFunctions.ts @@ -19,6 +19,31 @@ import { sleep } from '@n8n/utils/sleep'; import type { RecurringEventInstance } from './EventInterface'; +export function parseAttendees( + this: IExecuteFunctions, + attendees: unknown, + itemIndex: number, +): Array<{ email: string }> { + if ( + !Array.isArray(attendees) || + !attendees.every( + (attendee: unknown): attendee is string => + typeof attendee === 'string' && + attendee.split(',').every((email) => email.trim().length > 0), + ) + ) { + throw new NodeOperationError(this.getNode(), 'Invalid attendee value', { + description: + 'Set each attendee to an email address string. To include multiple addresses in one entry, separate them with commas.', + itemIndex, + }); + } + + return attendees.flatMap((attendee) => + attendee.split(',').map((email) => ({ email: email.trim() })), + ); +} + export async function googleApiRequest( this: IExecuteFunctions | ILoadOptionsFunctions | IPollFunctions, method: IHttpRequestMethods, diff --git a/packages/nodes-base/nodes/Google/Calendar/GoogleCalendar.node.ts b/packages/nodes-base/nodes/Google/Calendar/GoogleCalendar.node.ts index 787c9f6abe9..2a3a281e002 100644 --- a/packages/nodes-base/nodes/Google/Calendar/GoogleCalendar.node.ts +++ b/packages/nodes-base/nodes/Google/Calendar/GoogleCalendar.node.ts @@ -27,6 +27,7 @@ import { googleApiRequest, googleApiRequestAllItems, googleApiRequestWithRetries, + parseAttendees, type RecurrentEvent, } from './GenericFunctions'; import { sortItemKeysByPriorityList } from '../../../utils/utilities'; @@ -241,16 +242,7 @@ export class GoogleCalendar implements INodeType { }, }; if (additionalFields.attendees) { - body.attendees = []; - (additionalFields.attendees as string[]).forEach((attendee) => { - body.attendees!.push.apply( - body.attendees, - attendee - .split(',') - .map((a) => a.trim()) - .map((email) => ({ email })), - ); - }); + body.attendees = parseAttendees.call(this, additionalFields.attendees, i); } if (additionalFields.color) { body.colorId = additionalFields.color as string; @@ -642,16 +634,7 @@ export class GoogleCalendar implements INodeType { } // nodeVersion < 1.2 if (updateFields.attendees) { - body.attendees = []; - (updateFields.attendees as string[]).forEach((attendee) => { - body.attendees!.push.apply( - body.attendees, - attendee - .split(',') - .map((a) => a.trim()) - .map((email) => ({ email })), - ); - }); + body.attendees = parseAttendees.call(this, updateFields.attendees, i); } // nodeVersion >= 1.2 if (updateFields.attendeesUi) { @@ -663,6 +646,7 @@ export class GoogleCalendar implements INodeType { }; } ).values; + const parsedAttendees = parseAttendees.call(this, attendees, i); body.attendees = []; if (mode === 'add') { const event = await googleApiRequest.call( @@ -674,15 +658,7 @@ export class GoogleCalendar implements INodeType { body.attendees?.push(attendee); }); } - attendees.forEach((attendee) => { - body.attendees!.push.apply( - body.attendees, - attendee - .split(',') - .map((a) => a.trim()) - .map((email) => ({ email })), - ); - }); + body.attendees.push(...parsedAttendees); } if (updateFields.color) { body.colorId = updateFields.color as string; diff --git a/packages/nodes-base/nodes/Google/Calendar/test/node/event.create.test.ts b/packages/nodes-base/nodes/Google/Calendar/test/node/event.create.test.ts new file mode 100644 index 00000000000..c3a6f6ffc5a --- /dev/null +++ b/packages/nodes-base/nodes/Google/Calendar/test/node/event.create.test.ts @@ -0,0 +1,175 @@ +import type { IDataObject, IExecuteFunctions, INode } from 'n8n-workflow'; +import { NodeOperationError } from 'n8n-workflow'; +import { mock, mockDeep } from 'vitest-mock-extended'; + +import * as genericFunctions from '../../GenericFunctions'; +import { GoogleCalendar } from '../../GoogleCalendar.node'; + +describe('Google Calendar > Event > Create', () => { + const googleCalendar = new GoogleCalendar(); + const createdEvent = { id: 'test-event' }; + let executeFunctions: ReturnType>; + + beforeEach(() => { + vi.spyOn(genericFunctions, 'googleApiRequest').mockResolvedValue(createdEvent); + executeFunctions = mockDeep(); + executeFunctions.getInputData.mockReturnValue([{ json: {} }]); + executeFunctions.getNode.mockReturnValue( + mock({ + name: 'Google Calendar', + type: 'n8n-nodes-base.googleCalendar', + typeVersion: 1.3, + }), + ); + executeFunctions.getTimezone.mockReturnValue('UTC'); + executeFunctions.continueOnFail.mockReturnValue(false); + executeFunctions.helpers.returnJsonArray.mockReturnValue([{ json: createdEvent }]); + executeFunctions.helpers.constructExecutionMetaData.mockImplementation((data, { itemData }) => + data.map((item) => ({ ...item, pairedItem: itemData })), + ); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + const setupCreate = (additionalFields: IDataObject) => { + executeFunctions.getNodeParameter + .mockReturnValueOnce('event') + .mockReturnValueOnce('create') + .mockReturnValueOnce('primary') + .mockReturnValueOnce('2026-07-18T09:00:00Z') + .mockReturnValueOnce('2026-07-18T10:00:00Z') + .mockReturnValueOnce(true) + .mockReturnValueOnce(additionalFields); + }; + + it('should create an event with email strings and comma-separated attendees', async () => { + setupCreate({ + attendees: ['alice@example.com, bob@example.com', 'carol@example.com'], + }); + + await expect(googleCalendar.execute.call(executeFunctions)).resolves.toEqual([ + [{ json: createdEvent, pairedItem: { item: 0 } }], + ]); + expect(genericFunctions.googleApiRequest).toHaveBeenCalledWith( + 'POST', + '/calendar/v3/calendars/primary/events', + expect.objectContaining({ + attendees: [ + { email: 'alice@example.com' }, + { email: 'bob@example.com' }, + { email: 'carol@example.com' }, + ], + }), + {}, + ); + }); + + it('should create an event with an empty attendee list', async () => { + setupCreate({ attendees: [] }); + + await googleCalendar.execute.call(executeFunctions); + + expect(genericFunctions.googleApiRequest).toHaveBeenCalledWith( + 'POST', + '/calendar/v3/calendars/primary/events', + expect.objectContaining({ attendees: [] }), + {}, + ); + }); + + it.each([ + { attendees: [''] }, + { attendees: [' \t\n '] }, + { attendees: ['alice@example.com,'] }, + { attendees: [',alice@example.com'] }, + { attendees: ['alice@example.com,,bob@example.com'] }, + { attendees: ['alice@example.com, \t ,bob@example.com'] }, + { attendees: ['alice@example.com', ''] }, + ])('should report a node error for blank attendee entries: $attendees', async ({ attendees }) => { + setupCreate({ attendees }); + + const execution = googleCalendar.execute.call(executeFunctions); + + await expect(execution).rejects.toBeInstanceOf(NodeOperationError); + await expect(execution).rejects.toMatchObject({ + message: 'Invalid attendee value', + context: { itemIndex: 0 }, + }); + expect(genericFunctions.googleApiRequest).not.toHaveBeenCalled(); + }); + + it('should report a node error when an attendee expression returns an object', async () => { + // NODE-5494: An attendee expression can return an object instead of an email string. + setupCreate({ attendees: [{ email: 'alice@example.com' }] }); + + const execution = googleCalendar.execute.call(executeFunctions); + + await expect(execution).rejects.toBeInstanceOf(NodeOperationError); + await expect(execution).rejects.toMatchObject({ + message: expect.stringMatching(/attendee/i), + context: { itemIndex: 0 }, + }); + expect(genericFunctions.googleApiRequest).not.toHaveBeenCalled(); + }); + + it.each([null, 42, false, ['alice@example.com']])( + 'should report a node error for a non-string attendee: %j', + async (attendee) => { + setupCreate({ attendees: [attendee] }); + + const execution = googleCalendar.execute.call(executeFunctions); + + await expect(execution).rejects.toBeInstanceOf(NodeOperationError); + await expect(execution).rejects.toMatchObject({ context: { itemIndex: 0 } }); + expect(genericFunctions.googleApiRequest).not.toHaveBeenCalled(); + }, + ); + + it.each([{ email: 'alice@example.com' }, 'alice@example.com', 42, true])( + 'should report a node error when the attendee list is not an array: %j', + async (attendees) => { + setupCreate({ attendees }); + + await expect(googleCalendar.execute.call(executeFunctions)).rejects.toBeInstanceOf( + NodeOperationError, + ); + expect(genericFunctions.googleApiRequest).not.toHaveBeenCalled(); + }, + ); + + it('should identify the input item with an invalid attendee', async () => { + executeFunctions.getInputData.mockReturnValue([{ json: {} }, { json: {} }]); + setupCreate({ attendees: ['alice@example.com'] }); + executeFunctions.getNodeParameter + .mockReturnValueOnce('primary') + .mockReturnValueOnce('2026-07-18T09:00:00Z') + .mockReturnValueOnce('2026-07-18T10:00:00Z') + .mockReturnValueOnce(true) + .mockReturnValueOnce({ attendees: [{ email: 'bob@example.com' }] }); + + await expect(googleCalendar.execute.call(executeFunctions)).rejects.toMatchObject({ + context: { itemIndex: 1 }, + }); + expect(genericFunctions.googleApiRequest).toHaveBeenCalledTimes(1); + }); + + it('should return the attendee error when continue on fail is enabled', async () => { + setupCreate({ attendees: [{ email: 'alice@example.com' }] }); + executeFunctions.continueOnFail.mockReturnValue(true); + executeFunctions.helpers.returnJsonArray.mockImplementation((data) => [ + { json: data as IDataObject }, + ]); + + await expect(googleCalendar.execute.call(executeFunctions)).resolves.toEqual([ + [ + { + json: { error: 'Invalid attendee value' }, + pairedItem: { item: 0 }, + }, + ], + ]); + expect(genericFunctions.googleApiRequest).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/nodes-base/nodes/Google/Calendar/test/node/event.update.test.ts b/packages/nodes-base/nodes/Google/Calendar/test/node/event.update.test.ts index 6c309c13330..26cf34efcaf 100644 --- a/packages/nodes-base/nodes/Google/Calendar/test/node/event.update.test.ts +++ b/packages/nodes-base/nodes/Google/Calendar/test/node/event.update.test.ts @@ -1,6 +1,7 @@ import type { MockProxy } from 'vitest-mock-extended'; import { mock } from 'vitest-mock-extended'; import type { IDataObject, INode, IExecuteFunctions } from 'n8n-workflow'; +import { NodeOperationError } from 'n8n-workflow'; import * as genericFunctions from '../../GenericFunctions'; import { GoogleCalendar } from '../../GoogleCalendar.node'; @@ -62,7 +63,10 @@ describe('Google Calendar Node', () => { ); }); - it('should update replace attendees', async () => { + it.each([ + { attendees: ['email1@mail.com'], expectedAttendees: [{ email: 'email1@mail.com' }] }, + { attendees: [], expectedAttendees: [] }, + ])('should update replace attendees: $attendees', async ({ attendees, expectedAttendees }) => { mockExecuteFunctions.getInputData.mockReturnValue([{ json: {} }]); mockExecuteFunctions.getNodeParameter.mockReturnValueOnce('event'); mockExecuteFunctions.getNodeParameter.mockReturnValueOnce('update'); @@ -75,7 +79,7 @@ describe('Google Calendar Node', () => { attendeesUi: { values: { mode: 'replace', - attendees: ['email1@mail.com'], + attendees, }, }, }); @@ -85,7 +89,7 @@ describe('Google Calendar Node', () => { expect(genericFunctions.googleApiRequest).toHaveBeenCalledWith( 'PATCH', '/calendar/v3/calendars/undefined/events/myEvent', - { attendees: [{ email: 'email1@mail.com' }] }, + { attendees: expectedAttendees }, {}, ); }); @@ -127,6 +131,44 @@ describe('Google Calendar Node', () => { ); }); + describe.each([ + { version: 1.1, mode: 'legacy' }, + { version: 1.2, mode: 'replace' }, + { version: 1.3, mode: 'add' }, + ])('attendee validation in version $version ($mode)', ({ version, mode }) => { + it.each([ + { attendees: [{ email: 'alice@example.com' }] }, + { attendees: [''] }, + { attendees: [' \t '] }, + { attendees: ['alice@example.com,'] }, + ])('should report a node error for invalid attendees: $attendees', async ({ attendees }) => { + const updateFields = + mode === 'legacy' ? { attendees } : { attendeesUi: { values: { mode, attendees } } }; + mockExecuteFunctions.getInputData.mockReturnValue([{ json: {} }]); + mockExecuteFunctions.getNode.mockReturnValue(mock({ typeVersion: version })); + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce('event') + .mockReturnValueOnce('update') + .mockReturnValueOnce('myCalendar') + .mockReturnValueOnce('myEvent'); + if (version >= 1.3) { + mockExecuteFunctions.getNodeParameter.mockReturnValueOnce('instance'); + } + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce(true) + .mockReturnValueOnce(updateFields); + + const execution = googleCalendar.execute.call(mockExecuteFunctions); + + await expect(execution).rejects.toBeInstanceOf(NodeOperationError); + await expect(execution).rejects.toMatchObject({ + message: expect.stringMatching(/attendee/i), + context: { itemIndex: 0 }, + }); + expect(genericFunctions.googleApiRequest).not.toHaveBeenCalled(); + }); + }); + describe('all-day <-> timed conversion', () => { const setupUpdate = (updateFields: IDataObject) => { mockExecuteFunctions.getInputData.mockReturnValue([{ json: {} }]); From 6f3b397a1e76b79b69ebb22e0f0a92b0093ef508 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=BAben=20Castro?= Date: Fri, 16 Oct 2026 15:43:39 +0000 Subject: [PATCH 6/7] feat(editor): Mark community packages with blocked node types (no-changelog) (#41105) Co-authored-by: Claude Sonnet 5 Co-authored-by: Guillaume Jacquart --- .../frontend/@n8n/i18n/src/locales/en.json | 2 + .../components/CommunityPackageCard.test.ts | 71 +++++++++++++++++++ .../components/CommunityPackageCard.vue | 42 ++++++++++- .../views/SettingsCommunityNodesView.test.ts | 25 +++++++ .../views/SettingsCommunityNodesView.vue | 16 ++++- 5 files changed, 154 insertions(+), 2 deletions(-) diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 8d3601b36fe..766587e622f 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -3558,6 +3558,8 @@ "settings.communityNodes.uninstallAction.label": "Uninstall package", "settings.communityNodes.upToDate.tooltip": "You are up to date", "settings.communityNodes.failedToLoad.tooltip": "This package failed to load. It may be corrupted, or it may need a newer version of n8n. Try uninstalling and reinstalling it, or upgrade n8n.", + "settings.communityNodes.restricted.badge.label": "Restricted", + "settings.communityNodes.restricted.tooltip.heading": "Blocked by an instance policy", "settings.communityNodes.managedByEnv": "Community packages are managed via environment variables and cannot be modified from the UI.", "settings.communityNodes.fetchError.title": "Problem fetching installed packages", "settings.communityNodes.fetchError.message": "There may be a problem with your internet connection or your n8n instance", diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.test.ts b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.test.ts index 5c81e9c4ff1..4b2d47bb600 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.test.ts +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.test.ts @@ -5,6 +5,9 @@ import { STORES } from '@n8n/stores'; import CommunityPackageCard from './CommunityPackageCard.vue'; import { createComponentRenderer } from '@/__tests__/render'; import type { PublicInstalledNode, PublicInstalledPackage } from 'n8n-workflow'; +import { useTypeAvailabilityPoliciesStore } from '@n8n/frontend-module-type-availability-policies'; +import type { NodeTypeAvailability } from '@n8n/api-types'; +import { waitFor } from '@testing-library/vue'; const communityPackage: PublicInstalledPackage = { packageName: 'n8n-nodes-test', @@ -113,4 +116,72 @@ describe('CommunityPackageCard', () => { expect(queryByText('Uninstall package')).not.toBeInTheDocument(); }); }); + + describe('restricted badge', () => { + const mockNodeAvailability = ( + unavailable: Record>, + ) => { + vi.spyOn(useTypeAvailabilityPoliciesStore(), 'getNodeTypeAvailability').mockImplementation( + (name: string) => + name in unavailable + ? { name, available: false, ...unavailable[name] } + : { name, available: true }, + ); + }; + + // Reka UI only mounts tooltip content once the trigger is hovered. + function hoverTrigger(element: Element) { + element.dispatchEvent( + new PointerEvent('pointermove', { + bubbles: true, + cancelable: true, + pointerType: 'mouse', + clientX: 100, + clientY: 100, + }), + ); + } + + it('shows the badge and lists instance-blocked node types in the tooltip', async () => { + mockNodeAvailability({ 'n8n-nodes-test.oldNode': { scope: 'instance' } }); + + const { getByText } = renderComponent({ props: { communityPackage } }); + + const badge = getByText('Restricted'); + expect(badge).toBeInTheDocument(); + + hoverTrigger(badge); + + await waitFor(() => { + expect(document.querySelector('[data-dismissable-layer]')).toHaveTextContent('OldNode'); + }); + expect(document.querySelector('[data-dismissable-layer]')).not.toHaveTextContent('TestNode'); + }); + + it('does not show the badge when no node type is blocked', () => { + mockNodeAvailability({}); + + const { queryByText } = renderComponent({ props: { communityPackage } }); + + expect(queryByText('Restricted')).not.toBeInTheDocument(); + }); + + it('ignores a project-scoped restriction', () => { + mockNodeAvailability({ 'n8n-nodes-test.oldNode': { scope: 'project' } }); + + const { queryByText } = renderComponent({ props: { communityPackage } }); + + expect(queryByText('Restricted')).not.toBeInTheDocument(); + }); + + it('ignores an instance restriction that projects can opt into', () => { + mockNodeAvailability({ + 'n8n-nodes-test.oldNode': { scope: 'instance', optInAvailable: true }, + }); + + const { queryByText } = renderComponent({ props: { communityPackage } }); + + expect(queryByText('Restricted')).not.toBeInTheDocument(); + }); + }); }); diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.vue b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.vue index 3db55978b42..3244b65268f 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.vue +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageCard.vue @@ -1,6 +1,6 @@