From eeaf84f75651cd6a44b2c2121a0ec54ca96baefd Mon Sep 17 00:00:00 2001 From: "n8n-assistant[bot]" <100856346+n8n-assistant[bot]@users.noreply.github.com> Date: Thu, 1 Oct 2026 09:50:06 +0300 Subject: [PATCH] chore: Bundle/2.x (#40030) Co-authored-by: Matsu --- .../community-package-request.dto.test.ts | 29 ++ .../community-package-public.dto.ts | 2 - .../community-package-public.openapi.ts | 10 +- .../community-package-request.dto.ts | 9 + packages/@n8n/api-types/src/dto/index.ts | 1 + .../test/LmChatDatabricks.test.ts | 12 + .../LmChatOpenRouter/LmChatOpenRouter.node.ts | 3 +- .../test/LmChatOpenRouter.test.ts | 19 +- .../utils/databricks/gateway-config.ts | 4 +- .../src/__tests__/credentials-helper.test.ts | 332 +++++++++++------- .../workflow-execute-additional-data.test.ts | 104 +++++- packages/cli/src/credentials-helper.ts | 13 +- .../__tests__/credentials.service.test.ts | 41 +++ .../src/credentials/credentials.service.ts | 2 +- .../__tests__/execute-error-workflow.test.ts | 69 ++++ .../execute-error-workflow.ts | 3 +- ...unity-packages.instance-settings-loader.ts | 17 +- .../community-packages.controller.test.ts | 97 +++-- .../community-packages.integration.test.ts | 2 + ...mmunity-packages.lifecycle.service.test.ts | 255 +++++++++++--- .../community-packages.service.test.ts | 14 + .../community-node-types-utils.ts | 17 + .../community-packages.controller.ts | 30 +- .../community-packages.lifecycle.service.ts | 89 ++--- .../mcp/__tests__/get-execution.tool.test.ts | 229 +++++++++++- ...nstall-community-node.registration.test.ts | 6 +- .../install-community-node.tool.test.ts | 20 +- .../modules/mcp/__tests__/mcp-scopes.test.ts | 6 +- .../modules/mcp/__tests__/mcp.service.test.ts | 21 +- .../__tests__/update-workflow.tool.test.ts | 50 ++- packages/cli/src/modules/mcp/mcp.service.ts | 10 +- .../modules/mcp/tools/get-execution.tool.ts | 10 +- .../install-community-node.tool.ts | 5 +- .../workflow-builder/update-workflow.tool.ts | 169 +++------ .../workflow-builder/workflow-operations.ts | 2 +- .../import-subworkflows.integration.test.ts | 41 ++- .../__tests__/workflow-importer.test.ts | 8 +- .../entities/workflow/workflow-importer.ts | 5 + .../__tests__/oauth-server.service.test.ts | 67 ++++ .../oauth-server/oauth-server.service.ts | 8 +- .../community-packages.public.controller.ts | 4 +- .../spec/paths/installPackage.generated.yml | 5 +- .../spec/paths/updatePackage.generated.yml | 5 +- packages/cli/src/requests.ts | 8 - .../src/workflow-execute-additional-data.ts | 6 +- .../error-workflow-validation.service.test.ts | 186 ++++++++++ .../__tests__/workflow.service.test.ts | 185 ++++++++++ .../error-workflow-validation.service.ts | 167 +++++++++ .../cli/src/workflows/workflow.service.ts | 78 ++++ .../mcp/get-execution.tool.test.ts | 151 ++++++++ .../public-api/community-packages.test.ts | 59 +++- .../workflows/workflow.service.test.ts | 2 + .../workflows/workflows.controller.test.ts | 124 +++++++ .../__tests__/execution-context.test.ts | 171 ++++++++- .../src/execution-engine/execution-context.ts | 79 ++++- .../rest-api-client/src/api/communityNodes.ts | 10 +- .../communityNodes/communityNodes.store.ts | 20 +- ...CommunityPackageManageConfirmModal.test.ts | 6 +- .../CommunityPackageManageConfirmModal.vue | 6 +- .../nodeCreator/CommunityNodeDetails.test.ts | 2 +- .../composables/useInstallNode.test.ts | 30 +- .../composables/useInstallNode.ts | 1 - packages/workflow/src/execution-context.ts | 39 ++ .../workflow/test/execution-context.test.ts | 76 +++- 64 files changed, 2648 insertions(+), 603 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/community-packages/__tests__/community-package-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/community-packages/community-package-request.dto.ts create mode 100644 packages/cli/src/workflows/__tests__/error-workflow-validation.service.test.ts create mode 100644 packages/cli/src/workflows/error-workflow-validation.service.ts create mode 100644 packages/cli/test/integration/mcp/get-execution.tool.test.ts diff --git a/packages/@n8n/api-types/src/dto/community-packages/__tests__/community-package-request.dto.test.ts b/packages/@n8n/api-types/src/dto/community-packages/__tests__/community-package-request.dto.test.ts new file mode 100644 index 00000000000..c7783607f03 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/community-packages/__tests__/community-package-request.dto.test.ts @@ -0,0 +1,29 @@ +import { CommunityPackageRequestDto } from '../community-package-request.dto'; + +describe('CommunityPackageRequestDto', () => { + it.each([ + { name: 'n8n-nodes-test' }, + { name: 'n8n-nodes-test', version: '1.0.0' }, + { name: '@scope/n8n-nodes-test@1.0.0' }, + ])('accepts %o', (request) => { + expect(CommunityPackageRequestDto.safeParse(request).success).toBe(true); + }); + + it.each([ + { name: 'missing name', request: {} }, + { name: 'empty name', request: { name: ' ' } }, + { name: 'empty version', request: { name: 'n8n-nodes-test', version: '' } }, + ])('rejects $name', ({ request }) => { + expect(CommunityPackageRequestDto.safeParse(request).success).toBe(false); + }); + + it('drops unknown fields', () => { + const result = CommunityPackageRequestDto.safeParse({ + name: 'n8n-nodes-test', + checksum: 'sha512-abc', + verify: true, + }); + expect(result.success).toBe(true); + expect(result.success && result.data).toEqual({ name: 'n8n-nodes-test' }); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/community-packages/community-package-public.dto.ts b/packages/@n8n/api-types/src/dto/community-packages/community-package-public.dto.ts index f2fef0c1965..a58bed624a6 100644 --- a/packages/@n8n/api-types/src/dto/community-packages/community-package-public.dto.ts +++ b/packages/@n8n/api-types/src/dto/community-packages/community-package-public.dto.ts @@ -47,10 +47,8 @@ export class ListCommunityPackagesQueryDto extends Z.class({}, { strict: true }) export class InstallCommunityPackagePublicDto extends Z.class({ name: z.string().openapi(installCommunityPackageFieldDocs.name), version: z.string().optional().openapi(installCommunityPackageFieldDocs.version), - verify: z.boolean().optional().openapi(installCommunityPackageFieldDocs.verify), }) {} export class UpdateCommunityPackagePublicDto extends Z.class({ version: z.string().optional().openapi(updateCommunityPackageFieldDocs.version), - verify: z.boolean().optional().openapi(updateCommunityPackageFieldDocs.verify), }) {} diff --git a/packages/@n8n/api-types/src/dto/community-packages/community-package-public.openapi.ts b/packages/@n8n/api-types/src/dto/community-packages/community-package-public.openapi.ts index a19a217db2c..83f4406e313 100644 --- a/packages/@n8n/api-types/src/dto/community-packages/community-package-public.openapi.ts +++ b/packages/@n8n/api-types/src/dto/community-packages/community-package-public.openapi.ts @@ -6,20 +6,14 @@ export const installCommunityPackageFieldDocs = { 'npm package name. Unscoped names start with n8n-nodes-. Scoped names use the @[author]/n8n-nodes- form.', }, version: { - description: 'Semver version or npm dist-tag, such as latest or beta', - }, - verify: { description: - 'Whether to verify the package against the n8n-vetted package list. Required when the instance has N8N_UNVERIFIED_PACKAGES_ENABLED=false.', + 'Semver version or npm dist-tag, such as latest or beta. When unverified packages are disabled, only versions verified by n8n can be installed.', }, } as const satisfies Record; export const updateCommunityPackageFieldDocs = { version: { - description: 'Semver version or npm dist-tag to update to, such as latest or beta', - }, - verify: { description: - 'Whether to verify the package against the n8n-vetted package list. Setting to false will allow installing or updating to an unverified version. Default is true.', + 'Semver version or npm dist-tag to update to, such as latest or beta. When unverified packages are disabled, only versions verified by n8n can be installed.', }, } as const satisfies Record; diff --git a/packages/@n8n/api-types/src/dto/community-packages/community-package-request.dto.ts b/packages/@n8n/api-types/src/dto/community-packages/community-package-request.dto.ts new file mode 100644 index 00000000000..42754b581d7 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/community-packages/community-package-request.dto.ts @@ -0,0 +1,9 @@ +import { z } from 'zod'; + +import { Z } from '../../zod-class'; + +/** Body of `POST` and `PATCH /community-packages`. */ +export class CommunityPackageRequestDto extends Z.class({ + name: z.string().trim().min(1), + version: z.string().trim().min(1).optional(), +}) {} diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 8e978cc7046..b6fb1ed43ac 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -465,6 +465,7 @@ export { IMPORT_PACKAGE_SELECTION_REQUEST_FORM_FIELDS, } from './packages/import-package-request.dto'; export { ExportPackageRequestDto } from './packages/export-package-request.dto'; +export { CommunityPackageRequestDto } from './community-packages/community-package-request.dto'; export * from './evaluations'; diff --git a/packages/@n8n/nodes-langchain/nodes/llms/LmChatDatabricks/test/LmChatDatabricks.test.ts b/packages/@n8n/nodes-langchain/nodes/llms/LmChatDatabricks/test/LmChatDatabricks.test.ts index 0dc1fd91b81..c2bca6aa9e6 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/LmChatDatabricks/test/LmChatDatabricks.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/LmChatDatabricks/test/LmChatDatabricks.test.ts @@ -9,6 +9,7 @@ import { createRefreshingAuthFetch, makeN8nLlmFailedAttemptHandler, getProxyAgent, + aiClientFetch, } from '@n8n/ai-utilities'; import { DATABRICKS_PARTNER_USER_AGENT } from 'n8n-nodes-base/dist/nodes/Databricks/constants'; import { createMockExecuteFunction } from 'n8n-nodes-base/test/nodes/Helpers'; @@ -28,6 +29,7 @@ const mockedMakeN8nLlmFailedAttemptHandler = vi.mocked(makeN8nLlmFailedAttemptHa const mockedGetProxyAgent = vi.mocked(getProxyAgent); const mockedGetDatabricksTokenProvider = vi.mocked(getDatabricksTokenProvider); const mockedCreateRefreshingAuthFetch = vi.mocked(createRefreshingAuthFetch); +const mockedAiClientFetch = vi.mocked(aiClientFetch); const mockTokenProvider = { getToken: vi.fn(async () => 'test-token'), @@ -132,6 +134,16 @@ describe('LmChatDatabricks', () => { ); }); + it('should build the refreshing fetch on top of the bounded aiClientFetch', async () => { + const ctx = setupMockContext(); + + await node.supplyData.call(ctx, 0); + + expect(mockedCreateRefreshingAuthFetch).toHaveBeenCalledWith( + expect.objectContaining({ baseFetch: mockedAiClientFetch }), + ); + }); + it('should wire the refreshing fetch into ChatOpenAI', async () => { const ctx = setupMockContext(); diff --git a/packages/@n8n/nodes-langchain/nodes/llms/LmChatOpenRouter/LmChatOpenRouter.node.ts b/packages/@n8n/nodes-langchain/nodes/llms/LmChatOpenRouter/LmChatOpenRouter.node.ts index 4fdcfaaeb06..2fd5c4f64b5 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/LmChatOpenRouter/LmChatOpenRouter.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/LmChatOpenRouter/LmChatOpenRouter.node.ts @@ -1,6 +1,7 @@ import { ChatOpenAI, type ClientOptions } from '@langchain/openai'; import { getProxyAgent, + aiClientFetch, makeN8nLlmFailedAttemptHandler, N8nLlmTracing, getConnectionHintNoticeField, @@ -397,7 +398,7 @@ export class LmChatOpenRouter implements INodeType { const timeout = options.timeout; const configuration: ClientOptions = { baseURL: credentials.url, - fetch: createOpenRouterFetch(globalThis.fetch), + fetch: createOpenRouterFetch(aiClientFetch), fetchOptions: { dispatcher: getProxyAgent( credentials.url, diff --git a/packages/@n8n/nodes-langchain/nodes/llms/LmChatOpenRouter/test/LmChatOpenRouter.test.ts b/packages/@n8n/nodes-langchain/nodes/llms/LmChatOpenRouter/test/LmChatOpenRouter.test.ts index ec102bda183..d482f25c777 100644 --- a/packages/@n8n/nodes-langchain/nodes/llms/LmChatOpenRouter/test/LmChatOpenRouter.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/llms/LmChatOpenRouter/test/LmChatOpenRouter.test.ts @@ -4,7 +4,7 @@ /* eslint-disable @typescript-eslint/unbound-method */ import { ChatOpenAI } from '@langchain/openai'; -import { makeN8nLlmFailedAttemptHandler, getProxyAgent } from '@n8n/ai-utilities'; +import { makeN8nLlmFailedAttemptHandler, getProxyAgent, aiClientFetch } from '@n8n/ai-utilities'; import { createMockExecuteFunction } from 'n8n-nodes-base/test/nodes/Helpers'; import type { INode, ISupplyDataFunctions } from 'n8n-workflow'; import type { Mock, Mocked } from 'vitest'; @@ -17,6 +17,7 @@ vi.mock('@n8n/ai-utilities'); const MockedChatOpenAI = vi.mocked(ChatOpenAI); const mockedMakeN8nLlmFailedAttemptHandler = vi.mocked(makeN8nLlmFailedAttemptHandler); const mockedGetProxyAgent = vi.mocked(getProxyAgent); +const mockedAiClientFetch = vi.mocked(aiClientFetch); describe('LmChatOpenRouter', () => { let node: LmChatOpenRouter; @@ -50,6 +51,11 @@ describe('LmChatOpenRouter', () => { mockedMakeN8nLlmFailedAttemptHandler.mockReturnValue(vi.fn()); mockedGetProxyAgent.mockReturnValue({} as any); + // aiClientFetch wraps global fetch in production; mirror that so the + // OpenRouter adapter drives the test's globalThis.fetch stub. + mockedAiClientFetch.mockImplementation( + async (...args) => await (globalThis.fetch as typeof fetch)(...args), + ); return ctx; }; @@ -303,6 +309,17 @@ describe('LmChatOpenRouter', () => { return MockedChatOpenAI.mock.calls[0][0]?.configuration?.fetch as typeof fetch; } + it('should delegate to the bounded aiClientFetch under the wrapper', async () => { + const mockFetch = vi.fn().mockResolvedValue(jsonResponse({ ok: true })); + + const wrappedFetch = await setupFetchWrapper(mockFetch); + await wrappedFetch('https://openrouter.ai/api/v1/models'); + + // The wrapper must route through aiClientFetch, not call fetch directly, + // so the outbound response stays bounded. + expect(mockedAiClientFetch).toHaveBeenCalled(); + }); + it.each<{ input: unknown; expected: string; label: string }>([ { input: '', expected: '{}', label: 'empty string' }, { input: ' ', expected: '{}', label: 'whitespace-only string' }, diff --git a/packages/@n8n/nodes-langchain/utils/databricks/gateway-config.ts b/packages/@n8n/nodes-langchain/utils/databricks/gateway-config.ts index fb90606b195..c7fc0fd1285 100644 --- a/packages/@n8n/nodes-langchain/utils/databricks/gateway-config.ts +++ b/packages/@n8n/nodes-langchain/utils/databricks/gateway-config.ts @@ -1,5 +1,5 @@ import type { ClientOptions } from '@langchain/openai'; -import { getProxyAgent } from '@n8n/ai-utilities'; +import { aiClientFetch, getProxyAgent } from '@n8n/ai-utilities'; import type { ISupplyDataFunctions } from 'n8n-workflow'; import type { RefreshingTokenSource } from '../oauth2-token-provider'; @@ -29,7 +29,7 @@ export function createDatabricksGatewayConfig( const { fetch: authFetch, tokenSource } = createDatabricksAuthFetch(ctx, credential, { endpointUrl: baseURL, egressFilter, - baseFetch: fetch, + baseFetch: aiClientFetch, }); const configuration: ClientOptions = { diff --git a/packages/cli/src/__tests__/credentials-helper.test.ts b/packages/cli/src/__tests__/credentials-helper.test.ts index 5b173415856..8ce2940928b 100644 --- a/packages/cli/src/__tests__/credentials-helper.test.ts +++ b/packages/cli/src/__tests__/credentials-helper.test.ts @@ -3002,7 +3002,7 @@ describe('CredentialsHelper', () => { }); }); - describe('getDecrypted - credentialDecrypt policy enforcement', () => { + describe('getDecrypted', () => { const nodeCredentials: INodeCredentialsDetails = { id: 'cred-policy', name: 'Policy Test Credential', @@ -3017,6 +3017,19 @@ describe('CredentialsHelper', () => { usageScope: 'project', } as CredentialsEntity; + const executeDataFor = (nodeType: string): IExecuteData => ({ + node: { + id: nodeType, + name: nodeType, + type: nodeType, + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + data: {}, + source: null, + }); + let helper: CredentialsHelper; beforeEach(() => { @@ -3036,161 +3049,222 @@ describe('CredentialsHelper', () => { ); }); - test('calls enforceCredentialDecrypt with the credential, consumer and project context', async () => { - const executeData = { - node: { - name: 'Slack1', - type: 'n8n-nodes-base.slack', - typeVersion: 1, - position: [0, 0], - parameters: {}, - }, - data: {}, - source: null, - } as IExecuteData; + describe('managed credential access', () => { + const useManagedCredential = (type: string) => { + credentialsRepository.findOneByOrFail.mockResolvedValue( + mock({ + ...credentialEntity, + type, + isManaged: true, + }), + ); + }; - const additionalData = mock({ - projectId: 'proj-1', - executionId: 'exec-1', - userId: 'user-1', + test('does not resolve managed OpenAI credentials for full-access nodes', async () => { + useManagedCredential('openAiApi'); + + await expect( + helper.getDecrypted( + mock(), + nodeCredentials, + 'openAiApi', + 'manual', + executeDataFor('n8n-nodes-base.httpRequest'), + true, + ), + ).rejects.toThrow('Managed credentials are not supported by this node'); + expect(policyEnforcementService.enforceCredentialDecrypt).not.toHaveBeenCalled(); }); - await helper.getDecrypted( - additionalData, - nodeCredentials, - 'testApi', - 'manual', - executeData, - true, - ); + test.each([ + ['other managed credential types', 'slackApi', 'n8n-nodes-base.httpRequest'], + ['managed OpenAI credentials in regular nodes', 'openAiApi', 'n8n-nodes-base.openAi'], + ])('resolves %s', async (_scenario, credentialType, nodeType) => { + useManagedCredential(credentialType); - expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( - { - credentialType: 'testApi', - credentialId: 'cred-policy', - consumer: { nodeType: 'n8n-nodes-base.slack' }, + const result = await helper.getDecrypted( + mock({ + projectId: 'proj-1', + executionId: undefined, + userId: undefined, + }), + nodeCredentials, + credentialType, + 'manual', + executeDataFor(nodeType), + true, + ); + + expect(result).toEqual({ apiKey: 'test' }); + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( + { + credentialType, + credentialId: 'cred-policy', + consumer: { nodeType }, + projectId: 'proj-1', + }, + { kind: 'system', reason: 'execution' }, + ); + }); + }); + + describe('credentialDecrypt policy enforcement', () => { + test('calls enforceCredentialDecrypt with the credential, consumer and project context', async () => { + const executeData = { + node: { + name: 'Slack1', + type: 'n8n-nodes-base.slack', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + data: {}, + source: null, + } as IExecuteData; + + const additionalData = mock({ projectId: 'proj-1', - }, - { kind: 'system', reason: 'execution', executionId: 'exec-1' }, - ); - }); + executionId: 'exec-1', + userId: 'user-1', + }); - test('names the user when the decrypt is outside a run, e.g. an OAuth flow', async () => { - const additionalData = mock({ - projectId: undefined, - executionId: undefined, - userId: 'user-1', + await helper.getDecrypted( + additionalData, + nodeCredentials, + 'testApi', + 'manual', + executeData, + true, + ); + + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( + { + credentialType: 'testApi', + credentialId: 'cred-policy', + consumer: { nodeType: 'n8n-nodes-base.slack' }, + projectId: 'proj-1', + }, + { kind: 'system', reason: 'execution', executionId: 'exec-1' }, + ); }); - await helper.getDecrypted( - additionalData, - nodeCredentials, - 'testApi', - 'internal', - undefined, - true, - ); + test('names the user when the decrypt is outside a run, e.g. an OAuth flow', async () => { + const additionalData = mock({ + projectId: undefined, + executionId: undefined, + userId: 'user-1', + }); - expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( - expect.anything(), - { kind: 'user', user: { id: 'user-1' } }, - ); - }); + await helper.getDecrypted( + additionalData, + nodeCredentials, + 'testApi', + 'internal', + undefined, + true, + ); - test('uses the actor the caller names over the derived one', async () => { - const additionalData = mock({ - projectId: undefined, - executionId: undefined, - userId: undefined, + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( + expect.anything(), + { kind: 'user', user: { id: 'user-1' } }, + ); }); - await helper.getDecrypted( - additionalData, - nodeCredentials, - 'testApi', - 'internal', - undefined, - true, - undefined, - { actor: { kind: 'system', reason: 'log-streaming' } }, - ); + test('uses the actor the caller names over the derived one', async () => { + const additionalData = mock({ + projectId: undefined, + executionId: undefined, + userId: undefined, + }); - expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( - expect.anything(), - { kind: 'system', reason: 'log-streaming' }, - ); - }); + await helper.getDecrypted( + additionalData, + nodeCredentials, + 'testApi', + 'internal', + undefined, + true, + undefined, + { actor: { kind: 'system', reason: 'log-streaming' } }, + ); - test('passes a null consumer when no node is asking, e.g. a credential test', async () => { - const additionalData = mock({ - projectId: undefined, - executionId: undefined, - userId: undefined, + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( + expect.anything(), + { kind: 'system', reason: 'log-streaming' }, + ); }); - await helper.getDecrypted( - additionalData, - nodeCredentials, - 'testApi', - 'manual', - undefined, - true, - ); + test('passes a null consumer when no node is asking, e.g. a credential test', async () => { + const additionalData = mock({ + projectId: undefined, + executionId: undefined, + userId: undefined, + }); - expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( - expect.objectContaining({ consumer: null, projectId: null }), - { kind: 'system', reason: 'execution' }, - ); - }); + await helper.getDecrypted( + additionalData, + nodeCredentials, + 'testApi', + 'manual', + undefined, + true, + ); - test('resolves the credential before enforcing the policy check', async () => { - const callOrder: string[] = []; - credentialsRepository.findOneByOrFail.mockImplementation(async () => { - callOrder.push('findOneByOrFail'); - return credentialEntity; - }); - policyEnforcementService.enforceCredentialDecrypt.mockImplementation(async () => { - callOrder.push('enforceCredentialDecrypt'); - return await mock(); + expect(policyEnforcementService.enforceCredentialDecrypt).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ consumer: null, projectId: null }), + { kind: 'system', reason: 'execution' }, + ); }); - await helper.getDecrypted( - mock(), - nodeCredentials, - 'testApi', - 'manual', - undefined, - true, - ); + test('resolves the credential before enforcing the policy check', async () => { + const callOrder: string[] = []; + credentialsRepository.findOneByOrFail.mockImplementation(async () => { + callOrder.push('findOneByOrFail'); + return credentialEntity; + }); + policyEnforcementService.enforceCredentialDecrypt.mockImplementation(async () => { + callOrder.push('enforceCredentialDecrypt'); + return await mock(); + }); - expect(callOrder).toEqual(['findOneByOrFail', 'enforceCredentialDecrypt']); - }); - - test('blocks decryption when the policy check throws', async () => { - const violation = new Error('blocked by policy'); - policyEnforcementService.enforceCredentialDecrypt.mockRejectedValueOnce(violation); - - await expect( - helper.getDecrypted( + await helper.getDecrypted( mock(), nodeCredentials, 'testApi', 'manual', - ), - ).rejects.toThrow(violation); - }); + undefined, + true, + ); - test('decryption behavior is unchanged when the policy check clears', async () => { - const result = await helper.getDecrypted( - mock(), - nodeCredentials, - 'testApi', - 'manual', - undefined, - true, - ); + expect(callOrder).toEqual(['findOneByOrFail', 'enforceCredentialDecrypt']); + }); - expect(result).toEqual({ apiKey: 'test' }); + test('blocks decryption when the policy check throws', async () => { + const violation = new Error('blocked by policy'); + policyEnforcementService.enforceCredentialDecrypt.mockRejectedValueOnce(violation); + + await expect( + helper.getDecrypted( + mock(), + nodeCredentials, + 'testApi', + 'manual', + ), + ).rejects.toThrow(violation); + }); + + test('decryption behavior is unchanged when the policy check clears', async () => { + const result = await helper.getDecrypted( + mock(), + nodeCredentials, + 'testApi', + 'manual', + undefined, + true, + ); + + expect(result).toEqual({ apiKey: 'test' }); + }); }); }); }); diff --git a/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts b/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts index a6a8c2284da..89c4f322905 100644 --- a/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts +++ b/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts @@ -18,6 +18,7 @@ import type { IRun, INodeExecutionData, INode, + INodeType, ITaskData, WorkflowExecuteMode, ExecuteAgentWorkflowContext, @@ -42,6 +43,7 @@ import { ExternalHooks } from '@/external-hooks'; import { hashAgentSandboxPrincipal } from '@/modules/agents/agent-sandbox-principal'; import { AgentWorkflowExecutionService } from '@/modules/agents/agent-workflow-execution.service'; import { DataTableProxyService } from '@/modules/data-table/data-table-proxy.service'; +import { NodeTypes } from '@/node-types'; import { OwnershipService } from '@/services/ownership.service'; import { WorkflowStatisticsService } from '@/services/workflow-statistics.service'; import { Telemetry } from '@/telemetry'; @@ -58,6 +60,7 @@ import { import * as WorkflowHelpers from '@/workflow-helpers'; import { WorkflowHookContextService } from '@/workflow-hook-context.service'; import { WorkflowPublishedDataService } from '@/workflows/workflow-published-data.service'; +import { WorkflowStaticDataService } from '@/workflows/workflow-static-data.service'; const EXECUTION_ID = '123'; const LAST_NODE_EXECUTED = 'Last node executed'; @@ -344,17 +347,31 @@ describe('WorkflowExecuteAdditionalData', () => { }); describe('credential permission check routing', () => { + const nodeTypes = mockInstance(NodeTypes); + const workflowStaticDataService = mockInstance(WorkflowStaticDataService); + const nodeType = mock({ description: { properties: [] } }); + const nodes: INode[] = [ + { + id: 'trigger', + name: 'Trigger', + type: 'n8n-nodes-base.executeWorkflowTrigger', + typeVersion: 1.1, + position: [0, 0], + parameters: {}, + }, + ]; const subWorkflowData = () => mock({ id: 'sub-id', name: 'Sub Workflow', - nodes: [], + nodes, connections: {}, staticData: {}, settings: {}, }); beforeEach(() => { + nodeTypes.getByNameAndVersion.mockReturnValue(nodeType); vi.mocked(credentialsPermissionChecker.check).mockClear(); vi.mocked(credentialsPermissionChecker.checkForUser).mockClear(); vi.mocked(WorkflowExecute).mockClear(); @@ -369,12 +386,7 @@ describe('WorkflowExecuteAdditionalData', () => { userId: 'user-1', rootExecutionMode: undefined, }), - mock({ - loadedWorkflowData: subWorkflowData(), - doNotWaitToFinish: false, - parentWorkflowId: 'parent-1', - executionMode: 'manual', - }), + { parentWorkflowId: 'parent-1', executionMode: 'manual' }, ); expect(credentialsPermissionChecker.checkForUser).toHaveBeenCalledTimes(1); @@ -472,15 +484,61 @@ describe('WorkflowExecuteAdditionalData', () => { await executeWorkflow( mock({ id: undefined, code: subWorkflowData() }), mock({ userId: undefined }), - mock({ - loadedWorkflowData: subWorkflowData(), - doNotWaitToFinish: false, - parentWorkflowId: 'parent-1', - }), + { parentWorkflowId: 'parent-1', executionMode: 'webhook' }, ); - expect(credentialsPermissionChecker.check).toHaveBeenCalled(); + expect(credentialsPermissionChecker.check).toHaveBeenCalledWith('parent-1', nodes); expect(credentialsPermissionChecker.checkForUser).not.toHaveBeenCalled(); + expect(activeExecutions.add).toHaveBeenCalledWith( + expect.objectContaining({ + workflowData: expect.objectContaining({ id: 'parent-1' }), + }), + ); + const [integratedAdditionalData] = vi.mocked(WorkflowExecute).mock.calls[0]; + expect(integratedAdditionalData.workflowId).toBe('parent-1'); + }); + + it('keeps the parent identity for nested inline sub-workflows', async () => { + await executeWorkflow( + { code: subWorkflowData() }, + mock({ userId: undefined }), + { parentWorkflowId: 'parent-1', executionMode: 'webhook' }, + ); + const [integratedAdditionalData] = vi.mocked(WorkflowExecute).mock.calls[0]; + const parentWorkflowId = integratedAdditionalData.workflowId; + if (!parentWorkflowId) throw new Error('Expected a parent workflow ID'); + + await executeWorkflow({ code: subWorkflowData() }, integratedAdditionalData, { + parentWorkflowId, + executionMode: 'webhook', + }); + + expect(credentialsPermissionChecker.check).toHaveBeenNthCalledWith(1, 'parent-1', nodes); + expect(credentialsPermissionChecker.check).toHaveBeenNthCalledWith(2, 'parent-1', nodes); + }); + + it('preserves parent static-data persistence for inline definitions without an id', async () => { + await executeWorkflow( + { code: mock({ ...subWorkflowData(), id: undefined }) }, + mock({ userId: undefined }), + { parentWorkflowId: 'parent-1', executionMode: 'webhook' }, + ); + const [integratedAdditionalData, , runExecutionData] = + vi.mocked(WorkflowExecute).mock.calls[0]; + if (!integratedAdditionalData.hooks || !runExecutionData) { + throw new Error('Expected sub-workflow hooks and run data'); + } + const staticData = { global: { lastProcessedId: 123 } }; + + await integratedAdditionalData.hooks.runHook('workflowExecuteAfter', [ + { ...runWithData, data: runExecutionData, mode: 'integrated' }, + staticData, + ]); + + expect(workflowStaticDataService.saveStaticDataById).toHaveBeenCalledWith( + 'parent-1', + staticData, + ); }); it('preserves the triggering user in the sub-workflow additional data for inline sub-workflows so nested inline calls stay scoped to that user', async () => { @@ -1103,6 +1161,23 @@ describe('WorkflowExecuteAdditionalData', () => { }); }); + describe.each([ + ['draft', getDraftWorkflowData], + ['published', getPublishedWorkflowData], + ])('inline %s workflow data', (_version, loadWorkflow) => { + // Regression guard for inline workflow identity (IAM-1105). + it.each(['embedded-id', undefined])( + 'uses the parent identity with inline id %s', + async (id) => { + const workflowCode = mock({ id, nodes: [], connections: {} }); + + const result = await loadWorkflow({ code: workflowCode }, 'parent-workflow-id'); + + expect(result.id).toBe('parent-workflow-id'); + }, + ); + }); + describe('getPublishedWorkflowData', () => { beforeEach(() => { workflowRepository.get.mockClear(); @@ -1249,7 +1324,8 @@ describe('WorkflowExecuteAdditionalData', () => { const result = await getPublishedWorkflowData({ code: workflowCode }, 'parent-workflow-id'); - expect(result).toEqual(workflowCode); + expect(result.id).toBe('parent-workflow-id'); + expect(result.nodes).toEqual(workflowCode.nodes); expect(workflowRepository.get).not.toHaveBeenCalled(); }); diff --git a/packages/cli/src/credentials-helper.ts b/packages/cli/src/credentials-helper.ts index e73a7547a91..b807d33c832 100644 --- a/packages/cli/src/credentials-helper.ts +++ b/packages/cli/src/credentials-helper.ts @@ -7,7 +7,7 @@ import { SecretsProviderConnectionRepository, } from '@n8n/db'; import { Service } from '@n8n/di'; -import { Credentials, getAdditionalKeys } from 'n8n-core'; +import { Credentials, FULL_ACCESS_NODE_TYPES, getAdditionalKeys } from 'n8n-core'; import type { CredentialInformation, ICredentialDataDecryptedObject, @@ -30,6 +30,7 @@ import type { import { ICredentialsHelper, NodeHelpers, + OPEN_AI_API_CREDENTIAL_TYPE, Workflow, UnexpectedError, UserError, @@ -585,6 +586,16 @@ export class CredentialsHelper extends ICredentialsHelper { const credentialsEntity = await this.getCredentialsEntity(nodeCredentials, type); + // Managed OpenAI credentials are unavailable to nodes that can request undeclared types. + if ( + credentialsEntity.isManaged && + type === OPEN_AI_API_CREDENTIAL_TYPE && + consumerNode && + FULL_ACCESS_NODE_TYPES.has(consumerNode.type) + ) { + throw new UserError('Managed credentials are not supported by this node'); + } + // Validate against the executing project's policy before any decryption happens. await this.policyEnforcementService.enforceCredentialDecrypt( { diff --git a/packages/cli/src/credentials/__tests__/credentials.service.test.ts b/packages/cli/src/credentials/__tests__/credentials.service.test.ts index dc43d7bc674..fe0e033e6f7 100644 --- a/packages/cli/src/credentials/__tests__/credentials.service.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.service.test.ts @@ -2035,6 +2035,7 @@ describe('CredentialsService', () => { id: 'credential-id', name: 'Stored Credential', type: 'githubApi', + isManaged: false, }); const decryptedData = { accessToken: 'stored-token' } as ICredentialDataDecryptedObject; const unredactedData = { accessToken: 'live-token' } as ICredentialDataDecryptedObject; @@ -2071,11 +2072,50 @@ describe('CredentialsService', () => { expect(result).toEqual(testResult); }); + it('uses stored values when testing a managed credential', async () => { + const storedCredential = mock({ + id: 'credential-id', + name: 'Stored Credential', + type: 'openAiApi', + isManaged: true, + }); + const decryptedData = { + apiKey: 'stored-key', + url: 'https://saved.example/v1', + } as ICredentialDataDecryptedObject; + const testResult = { status: 'OK', message: 'Credential tested successfully' } as const; + + credentialsFinderService.findCredentialForUser.mockResolvedValue(storedCredential); + sharedCredentialsRepository.findCredentialOwningProject.mockResolvedValue(undefined); + vi.spyOn(service, 'decrypt').mockResolvedValue(decryptedData); + credentialsTester.testCredentials.mockResolvedValue(testResult); + + const result = await service.testWithCredentials(ownerUser, { + id: storedCredential.id, + name: 'Draft Credential', + type: 'githubApi', + data: { + apiKey: CREDENTIAL_BLANKING_VALUE, + url: 'https://draft.example/v1', + }, + }); + + expect(credentialsTester.testCredentials).toHaveBeenCalledWith(ownerUser.id, 'openAiApi', { + id: storedCredential.id, + name: storedCredential.name, + type: storedCredential.type, + data: decryptedData, + homeProject: undefined, + }); + expect(result).toEqual(testResult); + }); + it('discards a caller-supplied homeProject and resolves the owning project from storage', async () => { const storedCredential = mock({ id: 'credential-id', name: 'Stored Credential', type: 'githubApi', + isManaged: false, }); const decryptedData = { accessToken: 'stored-token' } as ICredentialDataDecryptedObject; const testResult = { status: 'OK', message: 'Credential tested successfully' } as const; @@ -2131,6 +2171,7 @@ describe('CredentialsService', () => { id: 'credential-id', name: 'Stored Credential', type: 'githubApi', + isManaged: false, }); const decryptedData = { accessToken: 'stored-token' } as ICredentialDataDecryptedObject; const testResult = { status: 'OK', message: 'Credential tested successfully' } as const; diff --git a/packages/cli/src/credentials/credentials.service.ts b/packages/cli/src/credentials/credentials.service.ts index 728b123f380..c36ba6e9fb9 100644 --- a/packages/cli/src/credentials/credentials.service.ts +++ b/packages/cli/src/credentials/credentials.service.ts @@ -1451,7 +1451,7 @@ export class CredentialsService { const mergedCredentials = await this.prepareCredentialsForTest({ storedCredential, user, - credentialsToTest: credentials, + credentialsToTest: storedCredential.isManaged ? undefined : credentials, }); return await this.test(user.id, mergedCredentials); diff --git a/packages/cli/src/execution-lifecycle/__tests__/execute-error-workflow.test.ts b/packages/cli/src/execution-lifecycle/__tests__/execute-error-workflow.test.ts index 1a73bf7cd1c..77b9beaccb6 100644 --- a/packages/cli/src/execution-lifecycle/__tests__/execute-error-workflow.test.ts +++ b/packages/cli/src/execution-lifecycle/__tests__/execute-error-workflow.test.ts @@ -208,4 +208,73 @@ describe('executeErrorWorkflow', () => { expect(workflowExecutionService.executeErrorWorkflow).toHaveBeenCalled(); }); }); + + describe('execution context handed to the error workflow', () => { + const runtimeData = { + version: 1 as const, + establishedAt: 2000000000, + source: 'webhook' as const, + credentials: 'sealed-identity-carrier', + secureArtifacts: 'stripped-trigger-values', + redaction: { version: 2 as const, production: true, manual: false }, + executedByUserId: 'user-who-ran-the-failed-workflow', + usesDynamicCredentials: true, + }; + + const runFailedWorkflow = async () => { + const workflowData = mock({ + id: 'workflow-123', + name: 'Failing workflow', + settings: { errorWorkflow: 'error-workflow-456' }, + nodes: [], + }); + const data = createRunExecutionData({ + resultData: { + error: new NodeOperationError( + mockNode, + 'Test error', + ) as IRun['data']['resultData']['error'], + lastNodeExecuted: 'TestNode', + runData: {}, + }, + executionData: { nodeExecutionStack: [] }, + }); + data.executionData!.runtimeData = runtimeData; + + ownershipService.getWorkflowProjectCached.mockResolvedValue({ id: 'project-1' } as never); + + executeErrorWorkflow( + workflowData, + { data, mode: 'trigger', startedAt: new Date(), storedAt: 'db', status: 'error' }, + 'trigger', + 'execution-1', + ); + await new Promise(process.nextTick); + + const [, workflowErrorData] = + workflowExecutionService.executeErrorWorkflow.mock.calls.at(0) ?? []; + return workflowErrorData!.execution!.executionContext; + }; + + it('omits the identity carrier and the secure artifacts', async () => { + const executionContext = await runFailedWorkflow(); + + expect(executionContext).toBeDefined(); + expect(executionContext).not.toHaveProperty('credentials'); + expect(executionContext).not.toHaveProperty('secureArtifacts'); + }); + + // This same payload becomes the error workflow's start-item `$json`, so a + // carrier left on it would be readable workflow data, not just inherited state. + it('passes only the non-identity fields', async () => { + expect(await runFailedWorkflow()).toEqual({ + version: 1, + establishedAt: 2000000000, + source: 'webhook', + redaction: { version: 2, production: true, manual: false }, + executedByUserId: 'user-who-ran-the-failed-workflow', + usesDynamicCredentials: true, + }); + }); + }); }); diff --git a/packages/cli/src/execution-lifecycle/execute-error-workflow.ts b/packages/cli/src/execution-lifecycle/execute-error-workflow.ts index 8997f2d4468..1f60e08adb2 100644 --- a/packages/cli/src/execution-lifecycle/execute-error-workflow.ts +++ b/packages/cli/src/execution-lifecycle/execute-error-workflow.ts @@ -3,6 +3,7 @@ import { GlobalConfig } from '@n8n/config'; import { Container } from '@n8n/di'; import { ErrorReporter } from 'n8n-core'; import type { IRun, IWorkflowBase, WorkflowExecuteMode } from 'n8n-workflow'; +import { toErrorWorkflowContext } from 'n8n-workflow'; import type { IWorkflowErrorData } from '@/interfaces'; import { isPolicyRefusal } from '@/policy/policy-violation.error'; @@ -74,7 +75,7 @@ export function executeErrorWorkflow( lastNodeExecuted: fullRunData.data.resultData.lastNodeExecuted!, mode, retryOf, - executionContext: fullRunData.data.executionData?.runtimeData, + executionContext: toErrorWorkflowContext(fullRunData.data.executionData?.runtimeData), }, workflow: { id: workflowId, diff --git a/packages/cli/src/instance-settings-loader/loaders/community-packages.instance-settings-loader.ts b/packages/cli/src/instance-settings-loader/loaders/community-packages.instance-settings-loader.ts index 4774888400d..c5cfb9b9d3b 100644 --- a/packages/cli/src/instance-settings-loader/loaders/community-packages.instance-settings-loader.ts +++ b/packages/cli/src/instance-settings-loader/loaders/community-packages.instance-settings-loader.ts @@ -5,6 +5,7 @@ import { Service } from '@n8n/di'; import { ensureError } from '@n8n/utils/errors/ensure-error'; import { z } from 'zod'; +import { selectVettedVersion } from '@/modules/community-packages/community-node-types-utils'; import { CommunityNodeTypesService } from '@/modules/community-packages/community-node-types.service'; import { CommunityPackagesConfig } from '@/modules/community-packages/community-packages.config'; import { @@ -200,19 +201,9 @@ export class CommunityPackagesInstanceSettingsLoader { ); } - let resolvedVersion = item.version; - let checksum: string | undefined; - - if (vetted) { - if (item.version === undefined) { - resolvedVersion = vetted.npmVersion; - checksum = vetted.checksum; - } else if (vetted.npmVersion === item.version) { - checksum = vetted.checksum; - } else { - checksum = vetted.nodeVersions?.find((v) => v.npmVersion === item.version)?.checksum; - } - } + const { version: resolvedVersion, checksum } = vetted + ? selectVettedVersion(vetted, item.version) + : { version: item.version, checksum: undefined }; if (!checksum && !this.communityPackagesConfig.unverifiedEnabled) { const ref = resolvedVersion ? `'${item.name}@${resolvedVersion}'` : `'${item.name}'`; diff --git a/packages/cli/src/modules/community-packages/__tests__/community-packages.controller.test.ts b/packages/cli/src/modules/community-packages/__tests__/community-packages.controller.test.ts index 5f74cc680a7..ce281b94f5c 100644 --- a/packages/cli/src/modules/community-packages/__tests__/community-packages.controller.test.ts +++ b/packages/cli/src/modules/community-packages/__tests__/community-packages.controller.test.ts @@ -1,4 +1,4 @@ -import type { CommunityNodeType } from '@n8n/api-types'; +import type { CommunityNodeType, CommunityPackageRequestDto } from '@n8n/api-types'; import type { Logger } from '@n8n/backend-common'; import type { EventService } from '@n8n/backend-services'; import type { InstanceSettingsLoaderConfig } from '@n8n/config'; @@ -6,7 +6,7 @@ import type { InstanceSettings } from 'n8n-core'; import { mock } from 'vitest-mock-extended'; import type { Push } from '@/push'; -import type { NodeRequest } from '@/requests'; +import type { AuthenticatedRequest } from '@n8n/db'; import type { CommunityNodeTypesService } from '../community-node-types.service'; import type { CommunityPackagesConfig } from '../community-packages.config'; @@ -23,7 +23,7 @@ describe('CommunityPackagesController', () => { const communityNodeTypesService = mock(); const instanceSettings = mock(); (instanceSettings as any).nodesDownloadDir = '/tmp/n8n-nodes-download'; - const communityPackagesConfig = mock(); + const communityPackagesConfig = mock({ unverifiedEnabled: true }); const instanceSettingsLoaderConfig = mock({ communityPackagesManagedByEnv: false, }); @@ -40,19 +40,24 @@ describe('CommunityPackagesController', () => { ); const controller = new CommunityPackagesController(lifecycle); + const req = mock({ user: { id: 'user1' } }); beforeEach(() => { vi.clearAllMocks(); + communityPackagesConfig.unverifiedEnabled = true; }); describe('installPackage', () => { - it('should throw error if verify in options but no checksum', async () => { - const request = mock({ - user: { id: 'user123' }, - body: { name: 'n8n-nodes-test', verify: true, version: '1.0.0' }, + it('should reject an install when unverified packages are disabled and the package is not vetted', async () => { + communityPackagesConfig.unverifiedEnabled = false; + const body = { name: 'n8n-nodes-test', version: '1.0.0' }; + communityPackagesService.parseNpmPackageName.mockReturnValue({ + rawString: 'n8n-nodes-test', + packageName: 'n8n-nodes-test', + version: undefined, }); communityNodeTypesService.findVetted.mockResolvedValue(undefined); - await expect(controller.installPackage(request)).rejects.toThrow( + await expect(controller.installPackage(req, {}, body)).rejects.toThrow( 'Package n8n-nodes-test is not vetted for installation', ); }); @@ -60,21 +65,16 @@ describe('CommunityPackagesController', () => { it.each(['echo "hello"', '1.a.b', '0.1.29#;ls'])( 'should throw error if version is invalid', async (version) => { - const request = mock({ - user: { id: 'user123' }, - body: { name: 'n8n-nodes-test', verify: true, version }, - }); - await expect(controller.installPackage(request)).rejects.toThrow( + const body = { name: 'n8n-nodes-test', version }; + await expect(controller.installPackage(req, {}, body)).rejects.toThrow( `Invalid version: ${version}`, ); }, ); it('should have correct version', async () => { - const request = mock({ - user: { id: 'user123' }, - body: { name: 'n8n-nodes-test', verify: true, version: '1.0.0' }, - }); + communityPackagesConfig.unverifiedEnabled = false; + const body = { name: 'n8n-nodes-test', version: '1.0.0' }; communityNodeTypesService.findVetted.mockResolvedValue( mock({ npmVersion: '1.1.1', @@ -99,7 +99,7 @@ describe('CommunityPackagesController', () => { }), ); - await controller.installPackage(request); + await controller.installPackage(req, {}, body); expect(communityPackagesService.installPackage).toHaveBeenCalledWith( 'n8n-nodes-test', @@ -116,15 +116,12 @@ describe('CommunityPackagesController', () => { }); describe('updatePackage', () => { - it('should use the version from the request body when updating a package', async () => { - const req = mock({ - body: { - name: 'n8n-nodes-test', - version: '2.0.0', - checksum: 'a893hfdsy7399', - }, - user: { id: 'user1' }, - }); + it('should use the version from the request body and ignore a checksum in the body', async () => { + const body = { + name: 'n8n-nodes-test', + version: '2.0.0', + checksum: 'a893hfdsy7399', + } as CommunityPackageRequestDto; const previouslyInstalledPackage = mock({ installedNodes: [{ type: 'testNode', latestVersion: 1, name: 'testNode' }], @@ -147,25 +144,59 @@ describe('CommunityPackagesController', () => { version: undefined, }); - const result = await controller.updatePackage(req); + const result = await controller.updatePackage(req, {}, body); expect(communityPackagesService.updatePackage).toHaveBeenCalledWith( 'n8n-nodes-test', previouslyInstalledPackage, '2.0.0', - 'a893hfdsy7399', + undefined, ); expect(result).toBe(newInstalledPackage); }); + it('should resolve the checksum from the vetted list when unverified packages are disabled', async () => { + communityPackagesConfig.unverifiedEnabled = false; + const body = { + name: 'n8n-nodes-test', + version: '2.0.0', + checksum: 'a893hfdsy7399', + } as CommunityPackageRequestDto; + communityNodeTypesService.findVetted.mockResolvedValue( + mock({ npmVersion: '2.0.0', checksum: 'vetted-checksum' }), + ); + const previouslyInstalledPackage = mock({ + installedNodes: [], + installedVersion: '1.0.0', + }); + communityPackagesService.findInstalledPackage.mockResolvedValue(previouslyInstalledPackage); + communityPackagesService.updatePackage.mockResolvedValue( + mock({ installedNodes: [], installedVersion: '2.0.0' }), + ); + communityPackagesService.parseNpmPackageName.mockReturnValue({ + rawString: 'n8n-nodes-test', + packageName: 'n8n-nodes-test', + version: undefined, + }); + + await controller.updatePackage(req, {}, body); + + expect(communityPackagesService.updatePackage).toHaveBeenCalledWith( + 'n8n-nodes-test', + previouslyInstalledPackage, + '2.0.0', + 'vetted-checksum', + ); + }); + it.each(['echo "hello"', '1.a.b', '0.1.29#;ls'])( 'should throw error if version is invalid', async (version) => { - const req = mock({ - body: { name: 'n8n-nodes-test', version, checksum: 'a893hfdsy7399' }, - }); - await expect(controller.updatePackage(req)).rejects.toThrow(`Invalid version: ${version}`); + const body = { name: 'n8n-nodes-test', version }; + await expect(controller.updatePackage(req, {}, body)).rejects.toThrow( + `Invalid version: ${version}`, + ); }, ); }); diff --git a/packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts b/packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts index 76773ff9ac2..07b2cc240fc 100644 --- a/packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts +++ b/packages/cli/src/modules/community-packages/__tests__/community-packages.integration.test.ts @@ -249,6 +249,8 @@ describe('PATCH /community-packages', () => { }); test('should reject if package is not installed', async () => { + communityPackagesService.parseNpmPackageName.mockReturnValue(parsedNpmPackageName); + const { body: { message }, } = await authAgent.patch('/community-packages').send({ name: mockPackageName() }).expect(400); diff --git a/packages/cli/src/modules/community-packages/__tests__/community-packages.lifecycle.service.test.ts b/packages/cli/src/modules/community-packages/__tests__/community-packages.lifecycle.service.test.ts index f1fc0a5eac6..e0c9dc05e63 100644 --- a/packages/cli/src/modules/community-packages/__tests__/community-packages.lifecycle.service.test.ts +++ b/packages/cli/src/modules/community-packages/__tests__/community-packages.lifecycle.service.test.ts @@ -63,15 +63,22 @@ describe('CommunityPackagesLifecycleService', () => { beforeEach(() => { vi.clearAllMocks(); + communityPackagesConfig.unverifiedEnabled = true; communityPackagesService.withLoadStatus.mockImplementation((packages) => packages); }); describe('install', () => { - it('should throw error if verify in options but no checksum', async () => { + it('should reject an install when unverified packages are disabled and the package is not vetted', async () => { + communityPackagesConfig.unverifiedEnabled = false; + communityPackagesService.parseNpmPackageName.mockReturnValue({ + rawString: 'n8n-nodes-test', + packageName: 'n8n-nodes-test', + version: undefined, + }); communityNodeTypesService.findVetted.mockResolvedValue(undefined); await expect( - lifecycle.install({ name: 'n8n-nodes-test', verify: true, version: '1.0.0' }, user, 'ui'), + lifecycle.install({ name: 'n8n-nodes-test', version: '1.0.0' }, user, 'ui'), ).rejects.toThrow('Package n8n-nodes-test is not vetted for installation'); expect(communityNodeTypesService.findVetted).toHaveBeenCalledWith('n8n-nodes-test'); @@ -81,12 +88,35 @@ describe('CommunityPackagesLifecycleService', () => { 'should throw error if version is invalid', async (version) => { await expect( - lifecycle.install({ name: 'n8n-nodes-test', verify: true, version }, user, 'ui'), + lifecycle.install({ name: 'n8n-nodes-test', version }, user, 'ui'), ).rejects.toThrow(`Invalid version: ${version}`); }, ); - it('should install with checksum when verify is true', async () => { + it('should install the requested version without a checksum when unverified packages are enabled', async () => { + communityPackagesService.parseNpmPackageName.mockReturnValue({ + rawString: 'n8n-nodes-test', + packageName: 'n8n-nodes-test', + version: undefined, + }); + communityPackagesService.findInstalledPackage.mockResolvedValue(null); + communityPackagesService.checkNpmPackageStatus.mockResolvedValue({ status: 'OK' }); + communityPackagesService.installPackage.mockResolvedValue( + mock({ installedNodes: [] }), + ); + + await lifecycle.install({ name: 'n8n-nodes-test', version: '1.5.0' }, user, 'ui'); + + expect(communityNodeTypesService.findVetted).not.toHaveBeenCalled(); + expect(communityPackagesService.installPackage).toHaveBeenCalledWith( + 'n8n-nodes-test', + '1.5.0', + undefined, + ); + }); + + it('should install a vetted version with its checksum when unverified packages are disabled', async () => { + communityPackagesConfig.unverifiedEnabled = false; communityNodeTypesService.findVetted.mockResolvedValue( mock({ npmVersion: '1.1.1', @@ -111,11 +141,7 @@ describe('CommunityPackagesLifecycleService', () => { }), ); - await lifecycle.install( - { name: 'n8n-nodes-test', verify: true, version: '1.0.0' }, - user, - 'ui', - ); + await lifecycle.install({ name: 'n8n-nodes-test', version: '1.0.0' }, user, 'ui'); expect(communityPackagesService.installPackage).toHaveBeenCalledWith( 'n8n-nodes-test', @@ -130,6 +156,85 @@ describe('CommunityPackagesLifecycleService', () => { ); }); + it('should install the latest vetted version when unverified packages are disabled and no version is given', async () => { + communityPackagesConfig.unverifiedEnabled = false; + communityNodeTypesService.findVetted.mockResolvedValue( + mock({ checksum: 'vetted-checksum', npmVersion: '1.2.3' }), + ); + communityPackagesService.parseNpmPackageName.mockReturnValue({ + rawString: 'n8n-nodes-test', + packageName: 'n8n-nodes-test', + version: undefined, + }); + communityPackagesService.findInstalledPackage.mockResolvedValue(null); + communityPackagesService.checkNpmPackageStatus.mockResolvedValue({ status: 'OK' }); + communityPackagesService.installPackage.mockResolvedValue( + mock({ installedNodes: [] }), + ); + + await lifecycle.install({ name: 'n8n-nodes-test' }, user, 'ui'); + + expect(communityPackagesService.installPackage).toHaveBeenCalledWith( + 'n8n-nodes-test', + '1.2.3', + 'vetted-checksum', + ); + }); + + it('should resolve a scoped package with a version suffix against the vetted list', async () => { + communityPackagesConfig.unverifiedEnabled = false; + communityPackagesService.parseNpmPackageName.mockReturnValue({ + rawString: '@scope/n8n-nodes-test@1.0.0', + packageName: '@scope/n8n-nodes-test', + scope: '@scope', + version: '1.0.0', + }); + communityNodeTypesService.findVetted.mockResolvedValue( + mock({ + checksum: 'latest-checksum', + npmVersion: '2.0.0', + nodeVersions: [{ npmVersion: '1.0.0', checksum: 'v1-checksum' }], + }), + ); + communityPackagesService.findInstalledPackage.mockResolvedValue(null); + communityPackagesService.checkNpmPackageStatus.mockResolvedValue({ status: 'OK' }); + communityPackagesService.installPackage.mockResolvedValue( + mock({ installedNodes: [] }), + ); + + await lifecycle.install({ name: '@scope/n8n-nodes-test@1.0.0' }, user, 'ui'); + + expect(communityNodeTypesService.findVetted).toHaveBeenCalledWith('@scope/n8n-nodes-test'); + expect(communityPackagesService.installPackage).toHaveBeenCalledWith( + '@scope/n8n-nodes-test', + '1.0.0', + 'v1-checksum', + ); + }); + + it('should reject an install of a version that is not in the vetted list when unverified packages are disabled', async () => { + communityPackagesConfig.unverifiedEnabled = false; + communityPackagesService.parseNpmPackageName.mockReturnValue({ + rawString: 'n8n-nodes-test', + packageName: 'n8n-nodes-test', + version: undefined, + }); + communityNodeTypesService.findVetted.mockResolvedValue( + mock({ + checksum: 'latest-checksum', + npmVersion: '2.0.0', + nodeVersions: [{ npmVersion: '1.0.0', checksum: 'v1-checksum' }], + }), + ); + + await expect( + lifecycle.install({ name: 'n8n-nodes-test', version: '1.5.0' }, user, 'ui'), + ).rejects.toThrow( + 'Version 1.5.0 of n8n-nodes-test is not verified by n8n. Latest verified version is 2.0.0', + ); + expect(communityPackagesService.installPackage).not.toHaveBeenCalled(); + }); + it('should reject install when the package is already installed and loaded', async () => { communityPackagesService.parseNpmPackageName.mockReturnValue({ rawString: 'n8n-nodes-test', @@ -207,7 +312,7 @@ describe('CommunityPackagesLifecycleService', () => { ), ); - const promise = lifecycle.install({ name: 'n8n-nodes-test', verify: true }, user, 'ui'); + const promise = lifecycle.install({ name: 'n8n-nodes-test' }, user, 'ui'); await expect(promise).rejects.toBeInstanceOf(IncompatibleNodesApiVersionError); await expect(promise).rejects.toMatchObject({ httpStatusCode: 400, @@ -334,7 +439,7 @@ describe('CommunityPackagesLifecycleService', () => { }); describe('update', () => { - it('should use the version from the request when updating a package', async () => { + it('should update without a checksum when unverified packages are enabled', async () => { const previouslyInstalledPackage = mockPackage('1.0.0'); const newInstalledPackage = mockPackage('2.0.0'); @@ -347,20 +452,17 @@ describe('CommunityPackagesLifecycleService', () => { }); const result = await lifecycle.update( - { - name: 'n8n-nodes-test', - version: '2.0.0', - checksum: 'a893hfdsy7399', - }, + { name: 'n8n-nodes-test', version: '2.0.0' }, user, 'badRequest', ); + expect(communityNodeTypesService.findVetted).not.toHaveBeenCalled(); expect(communityPackagesService.updatePackage).toHaveBeenCalledWith( 'n8n-nodes-test', previouslyInstalledPackage, '2.0.0', - 'a893hfdsy7399', + undefined, ); expect(result).toBe(newInstalledPackage); @@ -370,28 +472,13 @@ describe('CommunityPackagesLifecycleService', () => { 'should throw error if version is invalid', async (version) => { await expect( - lifecycle.update( - { name: 'n8n-nodes-test', version, checksum: 'a893hfdsy7399' }, - user, - 'badRequest', - ), + lifecycle.update({ name: 'n8n-nodes-test', version }, user, 'badRequest'), ).rejects.toThrow(`Invalid version: ${version}`); }, ); - it('should throw error if verify is true but package is not vetted', async () => { - communityNodeTypesService.findVetted.mockResolvedValue(undefined); - - await expect( - lifecycle.update( - { name: 'n8n-nodes-test', version: '2.0.0', verify: true }, - user, - 'badRequest', - ), - ).rejects.toThrow('Package n8n-nodes-test is not vetted for installation'); - }); - - it('should update with checksum when verify is true and version matches latest', async () => { + it('should update with the vetted checksum when unverified packages are disabled and the version matches latest', async () => { + communityPackagesConfig.unverifiedEnabled = false; communityNodeTypesService.findVetted.mockResolvedValue( mock({ checksum: 'vetted-checksum', @@ -409,11 +496,7 @@ describe('CommunityPackagesLifecycleService', () => { version: undefined, }); - await lifecycle.update( - { name: 'n8n-nodes-test', version: '2.0.0', verify: true }, - user, - 'badRequest', - ); + await lifecycle.update({ name: 'n8n-nodes-test', version: '2.0.0' }, user, 'badRequest'); expect(communityPackagesService.updatePackage).toHaveBeenCalledWith( 'n8n-nodes-test', @@ -424,6 +507,7 @@ describe('CommunityPackagesLifecycleService', () => { }); it('should use version-specific checksum from nodeVersions when targeting older version', async () => { + communityPackagesConfig.unverifiedEnabled = false; communityNodeTypesService.findVetted.mockResolvedValue( mock({ checksum: 'latest-checksum', @@ -441,11 +525,7 @@ describe('CommunityPackagesLifecycleService', () => { version: undefined, }); - await lifecycle.update( - { name: 'n8n-nodes-test', version: '1.0.0', verify: true }, - user, - 'badRequest', - ); + await lifecycle.update({ name: 'n8n-nodes-test', version: '1.0.0' }, user, 'badRequest'); expect(communityPackagesService.updatePackage).toHaveBeenCalledWith( 'n8n-nodes-test', @@ -455,6 +535,91 @@ describe('CommunityPackagesLifecycleService', () => { ); }); + it('should update to the latest vetted version when unverified packages are disabled and no version is given', async () => { + communityPackagesConfig.unverifiedEnabled = false; + communityNodeTypesService.findVetted.mockResolvedValue( + mock({ checksum: 'vetted-checksum', npmVersion: '2.0.0' }), + ); + const previouslyInstalledPackage = mockPackage('1.0.0'); + communityPackagesService.findInstalledPackage.mockResolvedValue(previouslyInstalledPackage); + communityPackagesService.updatePackage.mockResolvedValue(mockPackage('2.0.0')); + communityPackagesService.parseNpmPackageName.mockReturnValue({ + rawString: 'n8n-nodes-test', + packageName: 'n8n-nodes-test', + version: undefined, + }); + + await lifecycle.update({ name: 'n8n-nodes-test' }, user, 'badRequest'); + + expect(communityPackagesService.updatePackage).toHaveBeenCalledWith( + 'n8n-nodes-test', + previouslyInstalledPackage, + '2.0.0', + 'vetted-checksum', + ); + }); + + it('should resolve a scoped package with a version suffix against the vetted list', async () => { + communityPackagesConfig.unverifiedEnabled = false; + communityPackagesService.parseNpmPackageName.mockReturnValueOnce({ + rawString: '@scope/n8n-nodes-test@1.0.0', + packageName: '@scope/n8n-nodes-test', + scope: '@scope', + version: '1.0.0', + }); + communityNodeTypesService.findVetted.mockResolvedValue( + mock({ + checksum: 'latest-checksum', + npmVersion: '2.0.0', + nodeVersions: [{ npmVersion: '1.0.0', checksum: 'v1-checksum' }], + }), + ); + const previouslyInstalledPackage = mockPackage('0.5.0'); + communityPackagesService.findInstalledPackage.mockResolvedValue(previouslyInstalledPackage); + communityPackagesService.updatePackage.mockResolvedValue(mockPackage('1.0.0')); + + await lifecycle.update({ name: '@scope/n8n-nodes-test@1.0.0' }, user, 'badRequest'); + + expect(communityNodeTypesService.findVetted).toHaveBeenCalledWith('@scope/n8n-nodes-test'); + expect(communityPackagesService.findInstalledPackage).toHaveBeenCalledWith( + '@scope/n8n-nodes-test', + ); + expect(communityPackagesService.updatePackage).toHaveBeenCalledWith( + '@scope/n8n-nodes-test', + previouslyInstalledPackage, + '1.0.0', + 'v1-checksum', + ); + }); + + it('should reject an update when unverified packages are disabled and the package is not vetted', async () => { + communityPackagesConfig.unverifiedEnabled = false; + communityNodeTypesService.findVetted.mockResolvedValue(undefined); + + await expect( + lifecycle.update({ name: 'n8n-nodes-test', version: '2.0.0' }, user, 'badRequest'), + ).rejects.toThrow('Package n8n-nodes-test is not vetted for installation'); + expect(communityPackagesService.updatePackage).not.toHaveBeenCalled(); + }); + + it('should reject an update to a version that is not in the vetted list when unverified packages are disabled', async () => { + communityPackagesConfig.unverifiedEnabled = false; + communityNodeTypesService.findVetted.mockResolvedValue( + mock({ + checksum: 'latest-checksum', + npmVersion: '2.0.0', + nodeVersions: [{ npmVersion: '1.0.0', checksum: 'v1-checksum' }], + }), + ); + + await expect( + lifecycle.update({ name: 'n8n-nodes-test', version: '1.5.0' }, user, 'badRequest'), + ).rejects.toThrow( + 'Version 1.5.0 of n8n-nodes-test is not verified by n8n. Latest verified version is 2.0.0', + ); + expect(communityPackagesService.updatePackage).not.toHaveBeenCalled(); + }); + it('should throw NotFoundError when package missing and whenMissing is notFound', async () => { communityPackagesService.findInstalledPackage.mockResolvedValue(null); diff --git a/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts b/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts index 199b798338c..13a043b1e2f 100644 --- a/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts +++ b/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts @@ -132,6 +132,20 @@ describe('CommunityPackagesService', () => { }, ); + test.each([ + ['n8n-nodes-test', 'n8n-nodes-test', undefined, undefined], + ['n8n-nodes-test@1.2.3', 'n8n-nodes-test', undefined, '1.2.3'], + ['@scope/n8n-nodes-test', '@scope/n8n-nodes-test', '@scope', undefined], + ['@scope/n8n-nodes-test@1.2.3', '@scope/n8n-nodes-test', '@scope', '1.2.3'], + ])('should parse "%s"', (rawString, packageName, scope, version) => { + expect(communityPackagesService.parseNpmPackageName(rawString)).toEqual({ + rawString, + packageName, + scope, + version, + }); + }); + test.each(['n8n-nodes-base', '@n8n/n8n-nodes-langchain'])( 'should reject reserved package name %s', (name) => { diff --git a/packages/cli/src/modules/community-packages/community-node-types-utils.ts b/packages/cli/src/modules/community-packages/community-node-types-utils.ts index eb1027c4483..8aca512b496 100644 --- a/packages/cli/src/modules/community-packages/community-node-types-utils.ts +++ b/packages/cli/src/modules/community-packages/community-node-types-utils.ts @@ -23,6 +23,23 @@ export type StrapiCommunityNodeType = { aiNodeSdkVersion?: number; }; +/** + * Picks the version to install from a vetted entry and the checksum that belongs to it. + * No requested version means the latest vetted one. `checksum` is undefined when the + * requested version is not vetted. + */ +export function selectVettedVersion( + vetted: Pick, + requestedVersion: string | undefined, +): { version: string; checksum: string | undefined } { + const version = requestedVersion ?? vetted.npmVersion; + const checksum = + version === vetted.npmVersion + ? vetted.checksum + : vetted.nodeVersions?.find((v) => v.npmVersion === version)?.checksum; + return { version, checksum }; +} + export type CommunityNodesMetadata = Pick< StrapiCommunityNodeType, 'id' | 'name' | 'npmVersion' | 'updatedAt' diff --git a/packages/cli/src/modules/community-packages/community-packages.controller.ts b/packages/cli/src/modules/community-packages/community-packages.controller.ts index 746eb968cad..efb63f5ae8a 100644 --- a/packages/cli/src/modules/community-packages/community-packages.controller.ts +++ b/packages/cli/src/modules/community-packages/community-packages.controller.ts @@ -1,6 +1,8 @@ -import { Delete, Get, Patch, Post, RestController, GlobalScope } from '@n8n/decorators'; +import { CommunityPackageRequestDto } from '@n8n/api-types'; +import type { AuthenticatedRequest } from '@n8n/db'; +import { Body, Delete, Get, Patch, Post, RestController, GlobalScope } from '@n8n/decorators'; -import { NodeRequest } from '@/requests'; +import type { NodeRequest } from '@/requests'; import { CommunityPackagesLifecycleService } from './community-packages.lifecycle.service'; @@ -10,10 +12,12 @@ export class CommunityPackagesController { @Post('/') @GlobalScope('communityPackage:install') - async installPackage(req: NodeRequest.Post) { - const { name, verify, version } = req.body; - - return await this.communityPackagesLifecycle.install({ name, verify, version }, req.user, 'ui'); + async installPackage( + req: AuthenticatedRequest, + _res: unknown, + @Body body: CommunityPackageRequestDto, + ) { + return await this.communityPackagesLifecycle.install(body, req.user, 'ui'); } @Get('/') @@ -32,13 +36,11 @@ export class CommunityPackagesController { @Patch('/') @GlobalScope('communityPackage:update') - async updatePackage(req: NodeRequest.Update) { - const { name, version, checksum } = req.body; - - return await this.communityPackagesLifecycle.update( - { name, version, checksum }, - req.user, - 'badRequest', - ); + async updatePackage( + req: AuthenticatedRequest, + _res: unknown, + @Body body: CommunityPackageRequestDto, + ) { + return await this.communityPackagesLifecycle.update(body, req.user, 'badRequest'); } } diff --git a/packages/cli/src/modules/community-packages/community-packages.lifecycle.service.ts b/packages/cli/src/modules/community-packages/community-packages.lifecycle.service.ts index 68e733a4334..f05b360f4b0 100644 --- a/packages/cli/src/modules/community-packages/community-packages.lifecycle.service.ts +++ b/packages/cli/src/modules/community-packages/community-packages.lifecycle.service.ts @@ -16,6 +16,7 @@ import { IncompatibleNodesApiVersionError } from '@/errors/response-errors/incom import type { UserLike } from '@/events/maps/relay.event-map'; import { Push } from '@/push'; +import { selectVettedVersion } from './community-node-types-utils'; import { CommunityNodeTypesService } from './community-node-types.service'; import { CommunityPackagesConfig } from './community-packages.config'; import { CommunityPackagesService, isValidVersionSpecifier } from './community-packages.service'; @@ -63,34 +64,31 @@ export class CommunityPackagesLifecycleService { } /** - * Version and checksum the registry vetted for this package, in one lookup. - * The pair must come from the same catalog read: two separate reads can - * straddle a catalog refresh and pair a pinned version with the checksum of - * a newer release, failing a legitimate install. No requested version means - * the latest vetted one. + * Resolves the version and checksum to install. In verified-only mode both come from the + * vetted list. The checksum is what lets a package through `checkInstallPermissions`, so + * only the instance config decides this, never the caller. */ private async resolveVetted( name: string, - version?: string, - ): Promise<{ version: string; checksum: string }> { + requestedVersion: string | undefined, + ): Promise<{ version: string | undefined; checksum: string | undefined }> { + if (this.communityPackagesConfig.unverifiedEnabled) { + return { version: requestedVersion, checksum: undefined }; + } + const vettedPackage = await this.communityNodeTypesService.findVetted(name); if (!vettedPackage) { throw new BadRequestError(`Package ${name} is not vetted for installation`); } - const resolvedVersion = version ?? vettedPackage.npmVersion; - const checksum = - resolvedVersion === vettedPackage.npmVersion - ? vettedPackage.checksum - : vettedPackage.nodeVersions?.find((v) => v.npmVersion === resolvedVersion)?.checksum; - + const { version, checksum } = selectVettedVersion(vettedPackage, requestedVersion); if (!checksum) { throw new BadRequestError( - `Version ${resolvedVersion} of ${name} is not verified by n8n. Latest verified version is ${vettedPackage.npmVersion}`, + `Version ${version} of ${name} is not verified by n8n. Latest verified version is ${vettedPackage.npmVersion}`, ); } - return { version: resolvedVersion, checksum }; + return { version, checksum }; } async listInstalledPackages(): Promise { @@ -130,29 +128,19 @@ export class CommunityPackagesLifecycleService { } async install( - args: { name: string | undefined; version?: string; verify?: boolean }, + args: { name: string | undefined; version?: string }, user: UserLike, presentation: CommunityPackageInstallPresentation, ): Promise { this.assertNotManagedByEnv(); - const { name, verify, version } = args; + const { name } = args; if (!name) { throw new BadRequestError(PACKAGE_NAME_NOT_PROVIDED); } - if (version && !isValidVersionSpecifier(version)) { - throw new BadRequestError(`Invalid version: ${version}`); - } - - let checksum: string | undefined; - let resolvedVersion = version; - - if (verify) { - // Pins the vetted version too: without it, npm would install its own - // `latest` while the checksum describes the registry's latest, and the - // two can differ while a new release awaits vetting. - ({ version: resolvedVersion, checksum } = await this.resolveVetted(name, version)); + if (args.version && !isValidVersionSpecifier(args.version)) { + throw new BadRequestError(`Invalid version: ${args.version}`); } let parsed: CommunityPackages.ParsedPackageName; @@ -165,6 +153,12 @@ export class CommunityPackagesLifecycleService { ); } + // The vetted list is keyed by bare package name; `name` may carry a `@version` suffix. + const { version: packageVersion, checksum } = await this.resolveVetted( + parsed.packageName, + args.version ?? parsed.version, + ); + if (parsed.packageName === STARTER_TEMPLATE_NAME) { const templateMessage = presentation === 'ui' @@ -197,7 +191,6 @@ export class CommunityPackagesLifecycleService { throw new BadRequestError(`Package "${name}" is banned so it cannot be installed`); } - const packageVersion = resolvedVersion ?? parsed.version; let installedPackage: InstalledPackages; try { @@ -256,29 +249,39 @@ export class CommunityPackagesLifecycleService { } async update( - args: { name: string | undefined; version?: string; checksum?: string; verify?: boolean }, + args: { name: string | undefined; version?: string }, user: UserLike, whenMissing: MissingInstalledPackageBehavior, ): Promise { this.assertNotManagedByEnv(); - const { name, version, verify } = args; - - let checksum = args.checksum; - - if (verify) { - ({ checksum } = await this.resolveVetted(name ?? '', version)); - } + const { name } = args; if (!name) { throw new BadRequestError(PACKAGE_NAME_NOT_PROVIDED); } - if (version && !isValidVersionSpecifier(version)) { - throw new BadRequestError(`Invalid version: ${version}`); + if (args.version && !isValidVersionSpecifier(args.version)) { + throw new BadRequestError(`Invalid version: ${args.version}`); } + let parsed: CommunityPackages.ParsedPackageName; + + try { + parsed = this.communityPackagesService.parseNpmPackageName(name); + } catch (error) { + throw new BadRequestError( + error instanceof Error ? error.message : 'Failed to parse package name', + ); + } + + const { packageName } = parsed; + const { version, checksum } = await this.resolveVetted( + packageName, + args.version ?? parsed.version, + ); + const previouslyInstalledPackage = - await this.communityPackagesService.findInstalledPackage(name); + await this.communityPackagesService.findInstalledPackage(packageName); if (!previouslyInstalledPackage) { if (whenMissing === 'notFound') { @@ -289,7 +292,7 @@ export class CommunityPackagesLifecycleService { try { const newInstalledPackage = await this.communityPackagesService.updatePackage( - this.communityPackagesService.parseNpmPackageName(name).packageName, + packageName, previouslyInstalledPackage, version, checksum, @@ -317,7 +320,7 @@ export class CommunityPackagesLifecycleService { this.eventService.emit('community-package-updated', { user, - packageName: name, + packageName, packageVersionCurrent: previouslyInstalledPackage.installedVersion, packageVersionNew: newInstalledPackage.installedVersion, packageNodeNames: newInstalledPackage.installedNodes.map((n) => n.name), diff --git a/packages/cli/src/modules/mcp/__tests__/get-execution.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/get-execution.tool.test.ts index 4bb7474911a..935e1ab81aa 100644 --- a/packages/cli/src/modules/mcp/__tests__/get-execution.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/get-execution.tool.test.ts @@ -1,21 +1,23 @@ import { mockInstance } from '@n8n/backend-test-utils'; import { ExecutionRepository, User } from '@n8n/db'; -import { createEmptyRunExecutionData } from 'n8n-workflow'; +import { createEmptyRunExecutionData, type IRunExecutionData } from 'n8n-workflow'; import type { Mock } from 'vitest'; -import { ExecutionPersistence } from '@/executions/execution-persistence'; -import { Telemetry } from '@/telemetry'; -import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; - import { USER_CALLED_MCP_TOOL_EVENT } from '../mcp.constants'; import { createGetExecutionTool } from '../tools/get-execution.tool'; +import { ExecutionPersistence } from '@/executions/execution-persistence'; +import { ExecutionRedactionServiceProxy } from '@/executions/execution-redaction-proxy.service'; +import { Telemetry } from '@/telemetry'; +import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; + describe('get-execution MCP tool', () => { const user = Object.assign(new User(), { id: 'user-1' }); let executionRepository: ExecutionRepository; let executionPersistence: ExecutionPersistence; let workflowFinderService: WorkflowFinderService; let telemetry: Telemetry; + let executionRedactionServiceProxy: ExecutionRedactionServiceProxy; const mockWorkflow = { id: 'workflow-1', @@ -31,6 +33,7 @@ describe('get-execution MCP tool', () => { telemetry = mockInstance(Telemetry, { track: vi.fn(), }); + executionRedactionServiceProxy = mockInstance(ExecutionRedactionServiceProxy); }); describe('smoke tests', () => { @@ -40,6 +43,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); expect(tool.name).toBe('get_workflow_execution'); @@ -60,6 +64,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); (workflowFinderService.findWorkflowForUser as Mock).mockResolvedValue(mockWorkflow); @@ -92,6 +97,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecution = { @@ -137,6 +143,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecutionData = createEmptyRunExecutionData(); @@ -210,6 +217,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecutionData = createEmptyRunExecutionData(); @@ -282,6 +290,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecutionData = createEmptyRunExecutionData(); @@ -337,6 +346,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecutionData = createEmptyRunExecutionData(); @@ -402,6 +412,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecutionData = createEmptyRunExecutionData(); @@ -456,6 +467,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecutionData = createEmptyRunExecutionData(); @@ -514,6 +526,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecutionData = createEmptyRunExecutionData(); @@ -582,6 +595,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecution = { @@ -623,6 +637,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecution = { @@ -663,6 +678,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecution = { @@ -706,6 +722,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const mockExecutionData = createEmptyRunExecutionData(); @@ -768,6 +785,7 @@ describe('get-execution MCP tool', () => { executionRepository, workflowFinderService, telemetry, + executionRedactionServiceProxy, ); const waitTillDate = new Date('2025-01-01T12:00:00.000Z'); @@ -806,5 +824,206 @@ describe('get-execution MCP tool', () => { }); }); }); + + describe('redaction', () => { + const mockExecution = () => ({ + id: 'execution-1', + workflowId: 'workflow-1', + mode: 'manual' as const, + status: 'success' as const, + startedAt: new Date('2025-01-01T00:00:00.000Z'), + stoppedAt: new Date('2025-01-01T00:01:00.000Z'), + retryOf: null, + retrySuccessId: null, + waitTill: null, + data: { + ...createEmptyRunExecutionData(), + resultData: { + runData: { + 'HTTP Request': [ + { + startTime: 1, + executionIndex: 0, + executionTime: 1, + source: [], + data: { main: [[{ json: { secret: 'leaked' } }]] }, + }, + ], + }, + }, + } as IRunExecutionData, + }); + + test('passes the execution through redaction when includeData is true', async () => { + const tool = createGetExecutionTool( + user, + executionRepository, + workflowFinderService, + telemetry, + executionRedactionServiceProxy, + ); + + const execution = mockExecution(); + + (workflowFinderService.findWorkflowForUser as Mock).mockResolvedValue(mockWorkflow); + (executionPersistence.findWithUnflattenedData as Mock).mockResolvedValue(execution); + + await tool.handler( + { + workflowId: 'workflow-1', + executionId: 'execution-1', + includeData: true, + nodeNames: undefined, + truncateData: undefined, + }, + {} as never, + ); + + expect(executionRedactionServiceProxy.processExecution).toHaveBeenCalledTimes(1); + expect(executionRedactionServiceProxy.processExecution).toHaveBeenCalledWith(execution, { + user, + }); + }); + + test('does not pass redactExecutionData, so the workflow policy applies', async () => { + const tool = createGetExecutionTool( + user, + executionRepository, + workflowFinderService, + telemetry, + executionRedactionServiceProxy, + ); + + (workflowFinderService.findWorkflowForUser as Mock).mockResolvedValue(mockWorkflow); + (executionPersistence.findWithUnflattenedData as Mock).mockResolvedValue(mockExecution()); + + await tool.handler( + { + workflowId: 'workflow-1', + executionId: 'execution-1', + includeData: true, + nodeNames: undefined, + truncateData: undefined, + }, + {} as never, + ); + + // `true` would redact regardless of policy and `false` is the reveal path, + // which the MCP surface must never take. + const [, options] = (executionRedactionServiceProxy.processExecution as Mock).mock.calls[0]; + expect(options).not.toHaveProperty('redactExecutionData'); + }); + + test('returns the redacted data', async () => { + const tool = createGetExecutionTool( + user, + executionRepository, + workflowFinderService, + telemetry, + executionRedactionServiceProxy, + ); + + const execution = mockExecution(); + + (workflowFinderService.findWorkflowForUser as Mock).mockResolvedValue(mockWorkflow); + (executionPersistence.findWithUnflattenedData as Mock).mockResolvedValue(execution); + // The real service redacts in place, so mirror that here. + (executionRedactionServiceProxy.processExecution as Mock).mockImplementation( + async (target: typeof execution) => { + target.data.resultData.runData = { + 'HTTP Request': [ + { + startTime: 1, + executionIndex: 0, + executionTime: 1, + source: [], + data: { + main: [ + [ + { + json: {}, + redaction: { redacted: true, reason: 'workflow_redaction_policy' }, + }, + ], + ], + }, + }, + ], + }; + target.data.redactionInfo = { + isRedacted: true, + reason: 'workflow_redaction_policy', + canReveal: false, + }; + return target; + }, + ); + + const result = await tool.handler( + { + workflowId: 'workflow-1', + executionId: 'execution-1', + includeData: true, + nodeNames: undefined, + truncateData: undefined, + }, + {} as never, + ); + + expect(result.structuredContent).toMatchObject({ + data: { + resultData: { + runData: { + 'HTTP Request': [ + { + data: { + main: [ + [ + { + json: {}, + redaction: { redacted: true, reason: 'workflow_redaction_policy' }, + }, + ], + ], + }, + }, + ], + }, + }, + redactionInfo: { + isRedacted: true, + reason: 'workflow_redaction_policy', + canReveal: false, + }, + }, + }); + }); + + test('skips redaction when includeData is false', async () => { + const tool = createGetExecutionTool( + user, + executionRepository, + workflowFinderService, + telemetry, + executionRedactionServiceProxy, + ); + + (workflowFinderService.findWorkflowForUser as Mock).mockResolvedValue(mockWorkflow); + (executionRepository.findIfAccessible as Mock).mockResolvedValue(mockExecution()); + + await tool.handler( + { + workflowId: 'workflow-1', + executionId: 'execution-1', + includeData: false, + nodeNames: undefined, + truncateData: undefined, + }, + {} as never, + ); + + expect(executionRedactionServiceProxy.processExecution).not.toHaveBeenCalled(); + }); + }); }); }); diff --git a/packages/cli/src/modules/mcp/__tests__/install-community-node.registration.test.ts b/packages/cli/src/modules/mcp/__tests__/install-community-node.registration.test.ts index b67a72f3823..fd4125d6668 100644 --- a/packages/cli/src/modules/mcp/__tests__/install-community-node.registration.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/install-community-node.registration.test.ts @@ -10,7 +10,7 @@ import { CollaborationService } from '@/collaboration/collaboration.service'; import { CredentialsService } from '@/credentials/credentials.service'; import { ExecutionService } from '@/executions/execution.service'; import { ExecutionListService } from '@/executions/execution-list.service'; -import { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks/subworkflow-policy-checker'; +import { ExecutionRedactionServiceProxy } from '@/executions/execution-redaction-proxy.service'; import { DataTableProxyService } from '@/modules/data-table/data-table-proxy.service'; import { NodeCatalogService } from '@/node-catalog'; import { NodeTypes } from '@/node-types'; @@ -26,6 +26,7 @@ import { TagService } from '@/services/tag.service'; import { Telemetry } from '@/telemetry'; import { WorkflowRunner } from '@/workflow-runner'; import { WorkflowCreationService } from '@/workflows/workflow-creation.service'; +import { ErrorWorkflowValidationService } from '@/workflows/error-workflow-validation.service'; import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; import { WorkflowPublishedDataService } from '@/workflows/workflow-published-data.service'; @@ -135,7 +136,7 @@ describe('install_community_node registration', () => { mockInstance(WorkflowHistoryService), mockInstance(WorkflowsConfig), mockInstance(WorkflowPublishedDataService), - mockInstance(SubworkflowPolicyChecker), + mockInstance(ErrorWorkflowValidationService), mockInstance(AiGatewayService, { isAvailable: vi.fn().mockResolvedValue({ available: false }), }), @@ -147,6 +148,7 @@ describe('install_community_node registration', () => { mockInstance(FolderService), mockInstance(AiPreferenceService), mockInstance(McpConfig, { communityNodeDiscoveryEnabled: true }), + mockInstance(ExecutionRedactionServiceProxy), ); beforeEach(() => { diff --git a/packages/cli/src/modules/mcp/__tests__/install-community-node.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/install-community-node.tool.test.ts index 03839f669f4..3fbcffb671f 100644 --- a/packages/cli/src/modules/mcp/__tests__/install-community-node.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/install-community-node.tool.test.ts @@ -98,11 +98,7 @@ describe('install_community_node MCP tool', () => { test('installs the package the node type belongs to', async () => { const structured = await call(); - expect(lifecycleService.install).toHaveBeenCalledWith( - { name: PACKAGE, verify: true }, - user, - 'mcp', - ); + expect(lifecycleService.install).toHaveBeenCalledWith({ name: PACKAGE }, user, 'mcp'); expect(structured).toMatchObject({ installed: true, packageName: PACKAGE, @@ -116,15 +112,11 @@ describe('install_community_node MCP tool', () => { expect(structured.nodeTypes).toEqual([NODE_TYPE, `${PACKAGE}.firecrawlTool`]); }); - test('delegates version pinning to install(), which always verifies the checksum', async () => { - // No version is passed on purpose: install() resolves the latest vetted - // version and its checksum from one catalog lookup, so the pair can - // never straddle a catalog refresh. + test('delegates version and verification policy to install()', async () => { await call(); const [args] = lifecycleService.install.mock.calls[0]; - expect(args.version).toBeUndefined(); - expect(args.verify).toBe(true); + expect(args).toEqual({ name: PACKAGE }); expect(communityNodeTypesService.findVetted).not.toHaveBeenCalled(); }); @@ -299,11 +291,7 @@ describe('install_community_node MCP tool', () => { await call(DOTTED_NODE); - expect(lifecycleService.install).toHaveBeenCalledWith( - { name: DOTTED_PACKAGE, verify: true }, - user, - 'mcp', - ); + expect(lifecycleService.install).toHaveBeenCalledWith({ name: DOTTED_PACKAGE }, user, 'mcp'); }); }); diff --git a/packages/cli/src/modules/mcp/__tests__/mcp-scopes.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp-scopes.test.ts index 587a6f26ba4..3a7e3144b30 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp-scopes.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp-scopes.test.ts @@ -30,8 +30,8 @@ import { ActiveExecutions } from '@/active-executions'; import { CollaborationService } from '@/collaboration/collaboration.service'; import { CredentialsService } from '@/credentials/credentials.service'; import { ExecutionListService } from '@/executions/execution-list.service'; +import { ExecutionRedactionServiceProxy } from '@/executions/execution-redaction-proxy.service'; import { ExecutionService } from '@/executions/execution.service'; -import { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks/subworkflow-policy-checker'; import { DataTableProxyService } from '@/modules/data-table/data-table-proxy.service'; import { InstanceContextService } from '@/modules/instance-ai/instance-context.service'; import { @@ -56,6 +56,7 @@ import { WorkflowRunner } from '@/workflow-runner'; import { WorkflowCreationService } from '@/workflows/workflow-creation.service'; import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; +import { ErrorWorkflowValidationService } from '@/workflows/error-workflow-validation.service'; import { WorkflowPublishedDataService } from '@/workflows/workflow-published-data.service'; import { WorkflowService } from '@/workflows/workflow.service'; @@ -203,7 +204,7 @@ describe('McpService scope enforcement', () => { mockInstance(WorkflowHistoryService), mockInstance(WorkflowsConfig), mockInstance(WorkflowPublishedDataService), - mockInstance(SubworkflowPolicyChecker), + mockInstance(ErrorWorkflowValidationService), mockInstance(AiGatewayService, { isAvailable: vi.fn().mockResolvedValue({ available: false }), }), @@ -217,6 +218,7 @@ describe('McpService scope enforcement', () => { mockInstance(FolderService), mockInstance(AiPreferenceService), mockInstance(McpConfig), + mockInstance(ExecutionRedactionServiceProxy), ); beforeEach(() => { diff --git a/packages/cli/src/modules/mcp/__tests__/mcp.service.test.ts b/packages/cli/src/modules/mcp/__tests__/mcp.service.test.ts index c67a218eefc..c9757f7bdbe 100644 --- a/packages/cli/src/modules/mcp/__tests__/mcp.service.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/mcp.service.test.ts @@ -30,8 +30,8 @@ import { ActiveExecutions } from '@/active-executions'; import { CollaborationService } from '@/collaboration/collaboration.service'; import { CredentialsService } from '@/credentials/credentials.service'; import { ExecutionListService } from '@/executions/execution-list.service'; +import { ExecutionRedactionServiceProxy } from '@/executions/execution-redaction-proxy.service'; import { ExecutionService } from '@/executions/execution.service'; -import { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks/subworkflow-policy-checker'; import { DataTableProxyService } from '@/modules/data-table/data-table-proxy.service'; import { NodeCatalogService } from '@/node-catalog'; import { NodeTypes } from '@/node-types'; @@ -49,6 +49,7 @@ import { WorkflowRunner } from '@/workflow-runner'; import { WorkflowCreationService } from '@/workflows/workflow-creation.service'; import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; +import { ErrorWorkflowValidationService } from '@/workflows/error-workflow-validation.service'; import { WorkflowPublishedDataService } from '@/workflows/workflow-published-data.service'; import { WorkflowService } from '@/workflows/workflow.service'; @@ -134,7 +135,7 @@ describe('McpService', () => { mockInstance(WorkflowHistoryService), mockInstance(WorkflowsConfig), mockInstance(WorkflowPublishedDataService), - mockInstance(SubworkflowPolicyChecker), + mockInstance(ErrorWorkflowValidationService), mockAiGatewayService(), mockInstance(McpPostSaveMetricsService), mockInstance(ModuleRegistry), @@ -142,6 +143,7 @@ describe('McpService', () => { mockInstance(FolderService), aiPreferenceService, mockInstance(McpConfig), + mockInstance(ExecutionRedactionServiceProxy), ); }); @@ -190,7 +192,7 @@ describe('McpService', () => { mockInstance(WorkflowHistoryService), mockInstance(WorkflowsConfig), mockInstance(WorkflowPublishedDataService), - mockInstance(SubworkflowPolicyChecker), + mockInstance(ErrorWorkflowValidationService), mockAiGatewayService(), mockInstance(McpPostSaveMetricsService), mockInstance(ModuleRegistry), @@ -198,6 +200,7 @@ describe('McpService', () => { mockInstance(FolderService), mockInstance(AiPreferenceService), mockInstance(McpConfig), + mockInstance(ExecutionRedactionServiceProxy), ); expect(queueMcpService.isQueueMode).toBe(true); @@ -400,7 +403,7 @@ describe('McpService', () => { mockInstance(WorkflowHistoryService), mockInstance(WorkflowsConfig), mockInstance(WorkflowPublishedDataService), - mockInstance(SubworkflowPolicyChecker), + mockInstance(ErrorWorkflowValidationService), mockAiGatewayService(), mockInstance(McpPostSaveMetricsService), mockInstance(ModuleRegistry), @@ -408,6 +411,7 @@ describe('McpService', () => { mockInstance(FolderService), mockInstance(AiPreferenceService), mockInstance(McpConfig), + mockInstance(ExecutionRedactionServiceProxy), ); const user = Object.assign(new User(), { id: 'user-1', role: GLOBAL_MEMBER_ROLE }); @@ -1202,7 +1206,7 @@ describe('McpService', () => { mockInstance(WorkflowHistoryService), mockInstance(WorkflowsConfig), mockInstance(WorkflowPublishedDataService), - mockInstance(SubworkflowPolicyChecker), + mockInstance(ErrorWorkflowValidationService), mockAiGatewayService(), mockInstance(McpPostSaveMetricsService), mockInstance(ModuleRegistry), @@ -1210,6 +1214,7 @@ describe('McpService', () => { mockInstance(FolderService), mockInstance(AiPreferenceService), mockInstance(McpConfig), + mockInstance(ExecutionRedactionServiceProxy), ); const server = await service.getServer(user, mcpFeatureFlags()); @@ -1260,7 +1265,7 @@ describe('McpService', () => { mockInstance(WorkflowHistoryService), mockInstance(WorkflowsConfig), mockInstance(WorkflowPublishedDataService), - mockInstance(SubworkflowPolicyChecker), + mockInstance(ErrorWorkflowValidationService), mockAiGatewayService(), mockInstance(McpPostSaveMetricsService), mockInstance(ModuleRegistry), @@ -1268,6 +1273,7 @@ describe('McpService', () => { mockInstance(FolderService), mockInstance(AiPreferenceService), mockInstance(McpConfig), + mockInstance(ExecutionRedactionServiceProxy), ); const server = await service.getServer(user, mcpFeatureFlags()); @@ -1343,7 +1349,7 @@ describe('McpService', () => { mockInstance(WorkflowHistoryService), mockInstance(WorkflowsConfig), mockInstance(WorkflowPublishedDataService), - mockInstance(SubworkflowPolicyChecker), + mockInstance(ErrorWorkflowValidationService), mockAiGatewayService(), mockInstance(McpPostSaveMetricsService), mockInstance(ModuleRegistry), @@ -1351,6 +1357,7 @@ describe('McpService', () => { mockInstance(FolderService), mockInstance(AiPreferenceService), mockInstance(McpConfig), + mockInstance(ExecutionRedactionServiceProxy), ); }; diff --git a/packages/cli/src/modules/mcp/__tests__/update-workflow.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/update-workflow.tool.test.ts index a3726b86109..4a8266b7637 100644 --- a/packages/cli/src/modules/mcp/__tests__/update-workflow.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/update-workflow.tool.test.ts @@ -27,6 +27,7 @@ import { TagService } from '@/services/tag.service'; import { UrlService } from '@n8n/backend-services'; import { Telemetry } from '@/telemetry'; import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; +import { ErrorWorkflowValidationService } from '@/workflows/error-workflow-validation.service'; import { WorkflowPublishedDataService } from '@/workflows/workflow-published-data.service'; import { WorkflowService } from '@/workflows/workflow.service'; @@ -201,6 +202,20 @@ describe('update-workflow MCP tool', () => { incrementPostSaveFailure: vi.fn(), }); + /** + * Built per tool, not once in `beforeEach`: a test may swap `globalConfig` + * (custom error-trigger type) before building its tool, and the service reads + * the config it was constructed with. + */ + const buildErrorWorkflowValidationService = () => + new ErrorWorkflowValidationService( + globalConfig, + nodeTypes, + workflowFinderService, + workflowPublishedDataService, + subworkflowPolicyChecker, + ); + const createTool = () => createUpdateWorkflowTool( user, @@ -215,8 +230,7 @@ describe('update-workflow MCP tool', () => { dataTableOps as never, tagService, globalConfig, - subworkflowPolicyChecker, - workflowPublishedDataService, + buildErrorWorkflowValidationService(), aiGatewayService, {}, logger, @@ -2347,6 +2361,26 @@ describe('update-workflow MCP tool', () => { expect(saved.settings).toEqual(expect.objectContaining({ errorWorkflow: 'DEFAULT' })); }); + // Nothing evaluates `settings.errorWorkflow`; `executeErrorWorkflow` uses it + // as a literal workflow id. Saving an expression would look like it worked + // and then silently never run a handler. + test('rejects an expression instead of saving a reference that never resolves', async () => { + const result = await callHandler({ + workflowId: 'wf-1', + operations: [ + { + type: 'setWorkflowSettings', + settings: { errorWorkflow: '={{ $json.handlerId }}' }, + }, + ], + }); + + const response = parseResult(result); + expect(result.isError).toBe(true); + expect(response.error).toContain('does not accept expressions'); + expect(workflowService.update).not.toHaveBeenCalled(); + }); + test('does not attach settings for node-only edits', async () => { await callHandler({ workflowId: 'wf-1', @@ -2525,8 +2559,7 @@ describe('update-workflow MCP tool', () => { dataTableOps as never, tagService, globalConfig, - subworkflowPolicyChecker, - workflowPublishedDataService, + buildErrorWorkflowValidationService(), aiGatewayService, {}, logger, @@ -3887,8 +3920,7 @@ describe('update-workflow MCP tool', () => { dataTableOps as never, tagService, globalConfig, - subworkflowPolicyChecker, - workflowPublishedDataService, + buildErrorWorkflowValidationService(), aiGatewayService, {}, logger, @@ -3927,8 +3959,7 @@ describe('update-workflow MCP tool', () => { dataTableOps as never, tagService, globalConfig, - subworkflowPolicyChecker, - workflowPublishedDataService, + buildErrorWorkflowValidationService(), aiGatewayService, {}, logger, @@ -3967,8 +3998,7 @@ describe('update-workflow MCP tool', () => { dataTableOps as never, tagService, globalConfig, - subworkflowPolicyChecker, - workflowPublishedDataService, + buildErrorWorkflowValidationService(), aiGatewayService, {}, logger, diff --git a/packages/cli/src/modules/mcp/mcp.service.ts b/packages/cli/src/modules/mcp/mcp.service.ts index f58854a3e0d..9180c974f51 100644 --- a/packages/cli/src/modules/mcp/mcp.service.ts +++ b/packages/cli/src/modules/mcp/mcp.service.ts @@ -28,8 +28,8 @@ import { CollaborationService } from '@/collaboration/collaboration.service'; import { N8N_VERSION } from '@/constants'; import { CredentialsService } from '@/credentials/credentials.service'; import { ExecutionListService } from '@/executions/execution-list.service'; +import { ExecutionRedactionServiceProxy } from '@/executions/execution-redaction-proxy.service'; import { ExecutionService } from '@/executions/execution.service'; -import { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks/subworkflow-policy-checker'; import { DataTableProxyService } from '@/modules/data-table/data-table-proxy.service'; import { NodeCatalogService } from '@/node-catalog'; import { NodeTypes } from '@/node-types'; @@ -44,6 +44,7 @@ import { RoleService } from '@/services/role.service'; import { TagService } from '@/services/tag.service'; import { Telemetry } from '@/telemetry'; import { WorkflowRunner } from '@/workflow-runner'; +import { ErrorWorkflowValidationService } from '@/workflows/error-workflow-validation.service'; import { WorkflowCreationService } from '@/workflows/workflow-creation.service'; import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; @@ -262,7 +263,7 @@ export class McpService { private readonly workflowHistoryService: WorkflowHistoryService, private readonly workflowsConfig: WorkflowsConfig, private readonly workflowPublishedDataService: WorkflowPublishedDataService, - private readonly subworkflowPolicyChecker: SubworkflowPolicyChecker, + private readonly errorWorkflowValidationService: ErrorWorkflowValidationService, private readonly aiGatewayService: AiGatewayService, private readonly postSaveMetrics: McpPostSaveMetricsService, private readonly moduleRegistry: ModuleRegistry, @@ -270,6 +271,7 @@ export class McpService { private readonly folderService: FolderService, private readonly aiPreferenceService: AiPreferenceService, private readonly mcpConfig: McpConfig, + private readonly executionRedactionServiceProxy: ExecutionRedactionServiceProxy, ) {} /** Resolves user experience flags and the shared activity gate. */ @@ -521,6 +523,7 @@ export class McpService { this.executionRepository, this.workflowFinderService, this.telemetry, + this.executionRedactionServiceProxy, ); registerIfAllowed(getExecutionTool); @@ -1055,8 +1058,7 @@ export class McpService { dataTableOps, this.tagService, this.globalConfig, - this.subworkflowPolicyChecker, - this.workflowPublishedDataService, + this.errorWorkflowValidationService, this.aiGatewayService, uninstalledNodeOptions, this.logger, diff --git a/packages/cli/src/modules/mcp/tools/get-execution.tool.ts b/packages/cli/src/modules/mcp/tools/get-execution.tool.ts index 659ba740b2b..ae47d464b1e 100644 --- a/packages/cli/src/modules/mcp/tools/get-execution.tool.ts +++ b/packages/cli/src/modules/mcp/tools/get-execution.tool.ts @@ -1,7 +1,7 @@ import { type ExecutionRepository, type User } from '@n8n/db'; import { Container } from '@n8n/di'; -import type { IRunExecutionData, IRunData, ITaskDataConnections, IPinData } from 'n8n-workflow'; import { ensureError } from '@n8n/utils/errors/ensure-error'; +import type { IPinData, IRunData, IRunExecutionData, ITaskDataConnections } from 'n8n-workflow'; import { jsonStringify, replaceCircularReferences } from 'n8n-workflow'; import z from 'zod'; @@ -11,6 +11,7 @@ import type { ToolDefinition, UserCalledMCPToolEventPayload } from '../mcp.types import { getMcpWorkflow } from './workflow-validation.utils'; import { ExecutionPersistence } from '@/executions/execution-persistence'; +import type { ExecutionRedactionServiceProxy } from '@/executions/execution-redaction-proxy.service'; import type { Telemetry } from '@/telemetry'; import type { WorkflowFinderService } from '@/workflows/workflow-finder.service'; @@ -67,6 +68,7 @@ export const createGetExecutionTool = ( executionRepository: ExecutionRepository, workflowFinderService: WorkflowFinderService, telemetry: Telemetry, + executionRedactionServiceProxy: ExecutionRedactionServiceProxy, ): ToolDefinition => ({ name: 'get_workflow_execution', config: { @@ -107,6 +109,12 @@ export const createGetExecutionTool = ( executionId, [workflowId], ); + + if (fullExecution) { + // Redacts in place, modifying fullExecution internally + await executionRedactionServiceProxy.processExecution(fullExecution, { user }); + } + execution = fullExecution; executionData = fullExecution?.data ?? null; } else { diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/install-community-node.tool.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/install-community-node.tool.ts index 4ac973d7b9a..49752a5cb39 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/install-community-node.tool.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/install-community-node.tool.ts @@ -200,11 +200,8 @@ export const createInstallCommunityNodeTool = ( let installedPackage: Awaited>; try { - // No version: install() pins the latest vetted version and its - // checksum from a single catalog lookup, so the pair can never - // straddle a catalog refresh. installedPackage = await communityPackagesLifecycleService.install( - { name: packageName, verify: true }, + { name: packageName }, user, 'mcp', ); diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/update-workflow.tool.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/update-workflow.tool.ts index 34ee919c4b2..5243cabae6a 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/update-workflow.tool.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/update-workflow.tool.ts @@ -4,13 +4,11 @@ import type { GlobalConfig } from '@n8n/config'; import { type User, type SharedWorkflowRepository, WorkflowEntity } from '@n8n/db'; import { hasGlobalScope } from '@n8n/permissions'; import isEqual from 'lodash/isEqual'; -import { Workflow, type INode, type IWorkflowSettings } from 'n8n-workflow'; +import type { IWorkflowSettings } from 'n8n-workflow'; import { z } from 'zod'; import type { CollaborationService } from '@/collaboration/collaboration.service'; import type { CredentialsService } from '@/credentials/credentials.service'; -import { SubworkflowPolicyDenialError } from '@/errors/subworkflow-policy-denial.error'; -import type { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks/subworkflow-policy-checker'; import type { DataTableUserOperations } from '@/modules/data-table/data-table-proxy.service'; import type { McpPostSaveMetricsService } from '@/modules/mcp/mcp-post-save-metrics.service'; import type { NodeTypes } from '@/node-types'; @@ -24,8 +22,12 @@ import { removeDefaultValues, resolveNodeWebhookIds, } from '@/workflow-helpers'; +import { + isExpressionErrorWorkflowId, + staticErrorWorkflowId, + type ErrorWorkflowValidationService, +} from '@/workflows/error-workflow-validation.service'; import type { WorkflowFinderService } from '@/workflows/workflow-finder.service'; -import type { WorkflowPublishedDataService } from '@/workflows/workflow-published-data.service'; import type { WorkflowService } from '@/workflows/workflow.service'; import { buildInvalidAiToolSourceErrorResponse } from './connection-structure-check'; @@ -366,119 +368,67 @@ type UpdateWorkflowOutput = z.infer>; /** * Validates a freshly-set `errorWorkflow` reference. Throws a teaching-oriented - * error when the target does not exist / is inaccessible, has no active Error - * Trigger node, or cannot be called by this workflow due to its sub-workflow - * caller policy — each of which would otherwise silently prevent the error - * workflow from running on failure. A 'DEFAULT' / cleared value skips the check. - * An archived target, or one that is not available in MCP, is rejected like in - * the by-id MCP tools. + * error when the value is an expression, or when the target does not exist / is + * inaccessible, has no active Error Trigger node, or cannot be called by this + * workflow due to its sub-workflow caller policy — each of which would otherwise + * silently prevent the error workflow from running on failure. A 'DEFAULT' / + * cleared value skips the check. */ async function assertErrorWorkflowIsUsable({ errorWorkflowId, parentWorkflowId, user, workflowFinderService, - workflowPublishedDataService, - useWorkflowPublicationService, - nodeTypes, - subworkflowPolicyChecker, - errorTriggerType, + errorWorkflowValidationService, }: { errorWorkflowId: string | undefined; parentWorkflowId: string; user: User; workflowFinderService: WorkflowFinderService; - workflowPublishedDataService: WorkflowPublishedDataService; - useWorkflowPublicationService: boolean; - nodeTypes: NodeTypes; - subworkflowPolicyChecker: SubworkflowPolicyChecker; - errorTriggerType: string; + errorWorkflowValidationService: ErrorWorkflowValidationService; }): Promise { - if (!errorWorkflowId || errorWorkflowId === 'DEFAULT') { - return; + // The setting is never evaluated — an expression is used as a literal workflow + // id, so the handler would never run. Caught before `staticErrorWorkflowId`, + // which reports "no id to validate" for an expression and a cleared value alike. + if (isExpressionErrorWorkflowId(errorWorkflowId)) { + throw new Error( + `errorWorkflow does not accept expressions, and '${errorWorkflowId}' would be stored as a literal workflow ID that never matches, so the error workflow would never run. Pass a plain workflow ID (find one with search_workflows), or "DEFAULT" to clear it.`, + ); } - // Read access is required intentionally, mirroring the editor UI (the error - // workflow picker only lists workflows the user can read). Resolving the - // target without an access check would let callers probe arbitrary workflow - // IDs and learn their name / published / trigger / policy state from the - // validation errors below. Runtime not requiring read access is separate: it - // runs the error workflow under the owner project's context, gated by caller - // policy, which is about execution — not about who may configure the link. - const errorWorkflow = await workflowFinderService.findWorkflowForUser( - errorWorkflowId, + const staticId = staticErrorWorkflowId(errorWorkflowId); + if (!staticId) return; + + const errorWorkflow = await workflowFinderService.findWorkflowForUser(staticId, user, [ + 'workflow:read', + ]); + if (errorWorkflow) validateMcpWorkflow(errorWorkflow); + + const problem = await errorWorkflowValidationService.findProblem({ + errorWorkflowId: staticId, + parentWorkflowId, user, - ['workflow:read'], - // activeVersion is only the published source of truth when the publication - // service is off; otherwise we read it from the service below. - { includeActiveVersion: !useWorkflowPublicationService }, - ); - - if (!errorWorkflow) { - throw new Error( - `Error workflow '${errorWorkflowId}' was not found or you do not have access to it. Find a valid workflow ID with search_workflows, or create an error-handler workflow first.`, - ); - } - validateMcpWorkflow(errorWorkflow); - - // Runtime runs the PUBLISHED version of the error workflow, not its draft, and - // resolves it differently depending on the publication service flag — mirror - // WorkflowExecutionService.loadErrorWorkflowData exactly so we neither reject a - // workflow runtime would run nor accept a version runtime will not use. - let publishedNodes: INode[] | undefined; - - if (useWorkflowPublicationService) { - const published = await workflowPublishedDataService.getPublishedWorkflowData(errorWorkflowId); - publishedNodes = published?.publishedVersion.nodes; - } else if (errorWorkflow.activeVersionId && errorWorkflow.activeVersion) { - publishedNodes = errorWorkflow.activeVersion.nodes ?? []; - } - - if (!publishedNodes) { - throw new Error( - `Error workflow '${errorWorkflow.name}' (${errorWorkflowId}) has no published version, so n8n cannot run it when this workflow fails. Publish that workflow first (publish_workflow), then set it as the error workflow.`, - ); - } - - const hasErrorTrigger = publishedNodes.some( - (node) => node.type === errorTriggerType && node.disabled !== true, - ); - - if (!hasErrorTrigger) { - throw new Error( - `The published version of workflow '${errorWorkflow.name}' (${errorWorkflowId}) has no active Error Trigger node, so it would never run when this workflow fails. Add an Error Trigger node (${errorTriggerType}) and publish it, pick a different error workflow, or create a new error-handler workflow.`, - ); - } - - // Runtime blocks the error workflow if this workflow may not call it as a - // sub-workflow (see WorkflowExecutionService.executeErrorWorkflow). The - // policy checker only reads the target's id + settings, so an empty-node - // Workflow instance is sufficient. - const errorWorkflowInstance = new Workflow({ - id: errorWorkflow.id, - name: errorWorkflow.name, - nodeTypes, - nodes: [], - connections: {}, - active: false, - settings: errorWorkflow.settings ?? {}, }); - try { - await subworkflowPolicyChecker.check( - errorWorkflowInstance, - parentWorkflowId, - undefined, - user.id, - ); - } catch (error) { - if (error instanceof SubworkflowPolicyDenialError) { - throw new Error( - `Error workflow '${errorWorkflow.name}' (${errorWorkflowId}) cannot be called by this workflow because of its caller policy, so n8n would block it at runtime. Update that workflow's settings ("This workflow can be called by …") to allow this one — set it to any workflow, or add this workflow to its allowlist — or pick a different error workflow.`, - ); - } + if (!problem) return; - throw error; + switch (problem.reason) { + case 'not-found': + throw new Error( + `Error workflow '${staticId}' was not found or you do not have access to it. Find a valid workflow ID with search_workflows, or create an error-handler workflow first.`, + ); + case 'not-published': + throw new Error( + `Error workflow '${problem.name}' (${staticId}) has no published version, so n8n cannot run it when this workflow fails. Publish that workflow first (publish_workflow), then set it as the error workflow.`, + ); + case 'no-error-trigger': + throw new Error( + `The published version of workflow '${problem.name}' (${staticId}) has no active Error Trigger node, so it would never run when this workflow fails. Add an Error Trigger node (${problem.errorTriggerType}) and publish it, pick a different error workflow, or create a new error-handler workflow.`, + ); + case 'caller-policy': + throw new Error( + `Error workflow '${problem.name}' (${staticId}) cannot be called by this workflow because of its caller policy, so n8n would block it at runtime. Update that workflow's settings ("This workflow can be called by …") to allow this one — set it to any workflow, or add this workflow to its allowlist — or pick a different error workflow.`, + ); } } @@ -536,11 +486,9 @@ function assertExecutionTimeoutWithinMax( */ type WorkflowSettingsGuardDependencies = { user: User; - nodeTypes: NodeTypes; globalConfig: GlobalConfig; workflowFinderService: WorkflowFinderService; - workflowPublishedDataService: WorkflowPublishedDataService; - subworkflowPolicyChecker: SubworkflowPolicyChecker; + errorWorkflowValidationService: ErrorWorkflowValidationService; }; /** @@ -565,11 +513,9 @@ async function assertWorkflowSettingsValid( }, { user, - nodeTypes, globalConfig, workflowFinderService, - workflowPublishedDataService, - subworkflowPolicyChecker, + errorWorkflowValidationService, }: WorkflowSettingsGuardDependencies, ): Promise { // Validate a freshly-set error workflow so the agent can self-correct in @@ -585,11 +531,7 @@ async function assertWorkflowSettingsValid( parentWorkflowId: workflowId, user, workflowFinderService, - workflowPublishedDataService, - useWorkflowPublicationService: globalConfig.workflows.useWorkflowPublicationService, - nodeTypes, - subworkflowPolicyChecker, - errorTriggerType: globalConfig.nodes.errorTriggerType, + errorWorkflowValidationService, }); } @@ -1116,8 +1058,7 @@ export const createUpdateWorkflowTool = ( dataTableOps: DataTableUserOperations, tagService: TagService, globalConfig: GlobalConfig, - subworkflowPolicyChecker: SubworkflowPolicyChecker, - workflowPublishedDataService: WorkflowPublishedDataService, + errorWorkflowValidationService: ErrorWorkflowValidationService, aiGatewayService: AiGatewayService, options: { /** @@ -1136,11 +1077,9 @@ export const createUpdateWorkflowTool = ( // what is being validated. const settingsGuardDependencies: WorkflowSettingsGuardDependencies = { user, - nodeTypes, globalConfig, workflowFinderService, - workflowPublishedDataService, - subworkflowPolicyChecker, + errorWorkflowValidationService, }; return { diff --git a/packages/cli/src/modules/mcp/tools/workflow-builder/workflow-operations.ts b/packages/cli/src/modules/mcp/tools/workflow-builder/workflow-operations.ts index eba298fcb1e..008e2f454b6 100644 --- a/packages/cli/src/modules/mcp/tools/workflow-builder/workflow-operations.ts +++ b/packages/cli/src/modules/mcp/tools/workflow-builder/workflow-operations.ts @@ -51,7 +51,7 @@ export const workflowSettingsObjectSchema = z.object({ errorWorkflow: z .string() .describe( - 'ID of a SEPARATE workflow to run whenever THIS workflow fails — the common best-practice way to send failure alerts (email, Slack, etc.) or log errors via a shared, reusable handler. The referenced workflow must contain an Error Trigger node; find its ID with search_workflows. Pass "DEFAULT" to clear it. There are two ways to handle failures: (a) a dedicated/shared error workflow set here, or (b) an Error Trigger node placed directly inside THIS workflow (n8n fires it automatically on failure, no setting needed). When the user asks for error handling, ask which pattern they prefer before choosing. When errorWorkflow is set, it takes precedence over a same-workflow Error Trigger for the failing run. Failure handling fires for production executions only, not manual/test runs. Distinct from per-node onError/retry (setNodeSettings).', + 'ID of a SEPARATE workflow to run whenever THIS workflow fails — the common best-practice way to send failure alerts (email, Slack, etc.) or log errors via a shared, reusable handler. The referenced workflow must contain an Error Trigger node; find its ID with search_workflows. Pass "DEFAULT" to clear it. Must be a plain ID — this setting is never evaluated, so an expression would be stored as a literal ID and the handler would never run. There are two ways to handle failures: (a) a dedicated/shared error workflow set here, or (b) an Error Trigger node placed directly inside THIS workflow (n8n fires it automatically on failure, no setting needed). When the user asks for error handling, ask which pattern they prefer before choosing. When errorWorkflow is set, it takes precedence over a same-workflow Error Trigger for the failing run. Failure handling fires for production executions only, not manual/test runs. Distinct from per-node onError/retry (setNodeSettings).', ) .optional(), timezone: z diff --git a/packages/cli/src/modules/n8n-packages/__tests__/import-subworkflows.integration.test.ts b/packages/cli/src/modules/n8n-packages/__tests__/import-subworkflows.integration.test.ts index 8c13db5b9ee..2a4817640c4 100644 --- a/packages/cli/src/modules/n8n-packages/__tests__/import-subworkflows.integration.test.ts +++ b/packages/cli/src/modules/n8n-packages/__tests__/import-subworkflows.integration.test.ts @@ -27,7 +27,12 @@ import { import { executeWorkflowNode } from './utils/test-builders'; type ImportPackageParams = Pick & - Partial>; + Partial< + Pick< + ImportPackageRequest, + 'workflowIdPolicy' | 'workflowPublishingPolicy' | 'workflowConflictPolicy' + > + >; async function importPackage(params: ImportPackageParams) { return await Container.get(N8nPackagesService).importPackage(importPackageRequest(params)); @@ -191,6 +196,40 @@ describe('Package import of workflows with sub-workflows', () => { expect(importedMain.settings?.errorWorkflow).toBe(importedError.id); }); + // A later package version can add an error handler for a workflow that is + // already imported. The parent is then an update that points at a handler this + // same import creates, so the handler is neither present nor published while + // the parent is written — publishing is a package-wide sweep that runs after. + it('remaps `settings.errorWorkflow` when the parent is an update and the handler is new', async () => { + const owner = await createOwner(); + + const v1 = serializedWorkflow({ id: 'CHEDDAR', name: 'Main' }); + await importPackage({ + user: owner, + packageBuffer: await buildSubWorkflowPackage([v1]), + workflowIdPolicy: WorkflowIdPolicy.Source, + }); + + const v2 = serializedWorkflow({ + id: 'CHEDDAR', + name: 'Main', + settings: { errorWorkflow: 'BRIE' }, + }); + const errorHandler = serializedWorkflow({ id: 'BRIE', name: 'Error handler' }); + + const result = await importPackage({ + user: owner, + packageBuffer: await buildSubWorkflowPackage([v2, errorHandler]), + workflowIdPolicy: WorkflowIdPolicy.Source, + workflowConflictPolicy: 'new-version', + }); + + const importedMain = await findImported(result, 'CHEDDAR'); + const importedError = await findImported(result, 'BRIE'); + + expect(importedMain.settings?.errorWorkflow).toBe(importedError.id); + }); + it('does not crash when a package carries a non-string settings.callerIds', async () => { const owner = await createOwner(); diff --git a/packages/cli/src/modules/n8n-packages/entities/workflow/__tests__/workflow-importer.test.ts b/packages/cli/src/modules/n8n-packages/entities/workflow/__tests__/workflow-importer.test.ts index 30d4435f22c..01fbb745d82 100644 --- a/packages/cli/src/modules/n8n-packages/entities/workflow/__tests__/workflow-importer.test.ts +++ b/packages/cli/src/modules/n8n-packages/entities/workflow/__tests__/workflow-importer.test.ts @@ -123,7 +123,12 @@ describe('WorkflowImporter.apply', () => { user, expect.any(WorkflowEntity), 'existing-update', - { publicApi: true, source: 'import', allowArchivedUpdate: false }, + { + publicApi: true, + source: 'import', + allowArchivedUpdate: false, + allowUnresolvedErrorWorkflow: true, + }, ); }); @@ -291,6 +296,7 @@ describe('WorkflowImporter.apply', () => { publicApi: true, source: 'import', allowArchivedUpdate: true, + allowUnresolvedErrorWorkflow: true, }); expect(archive).not.toHaveBeenCalled(); expect(unarchive).not.toHaveBeenCalled(); diff --git a/packages/cli/src/modules/n8n-packages/entities/workflow/workflow-importer.ts b/packages/cli/src/modules/n8n-packages/entities/workflow/workflow-importer.ts index f3d6ba55b69..6207232a5c6 100644 --- a/packages/cli/src/modules/n8n-packages/entities/workflow/workflow-importer.ts +++ b/packages/cli/src/modules/n8n-packages/entities/workflow/workflow-importer.ts @@ -286,6 +286,11 @@ export class WorkflowImporter { publicApi: true, source: 'import', allowArchivedUpdate: item.existing.isArchived, + // `settings.errorWorkflow` is a package reference this importer rebinds. The + // handler it now points at can still be unwritten (later in this loop) and is + // unpublished until the package-wide sweep, so the write-time check on that + // reference cannot apply here. + allowUnresolvedErrorWorkflow: true, ...(tagIds !== undefined ? { tagIds } : {}), }); diff --git a/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts b/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts index ae770f00f7b..06e0fb4713f 100644 --- a/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts +++ b/packages/cli/src/modules/oauth-server/__tests__/oauth-server.service.test.ts @@ -298,6 +298,73 @@ describe('OAuthServerService', () => { }); }); + const buildServiceWithQueryIgnoringResolver = () => { + const registry = new ProtectedResourceRegistry(mock()); + // A static resource would not reproduce the bug. Needs Resolver + registry.registerResolver({ + id: 'form-path-only', + scopes: [], + resolveByUrl: async (url) => + new URL(url).pathname.replace(/\/$/, '') === '/form/abc' + ? { + id: 'form-abc', + isFirstParty: true, + getResourceUrl: () => FIRST_PARTY_URL, + getAudiences: () => [FIRST_PARTY_URL], + scopes: [], + authorize: async () => true, + } + : undefined, + resolveByPath: async () => undefined, + }); + + return new OAuthServerService( + logger, + mockInstance(GlobalConfig), + oauthSessionService, + oauthClientRepository, + tokenService, + authorizationCodeService, + userConsentRepository, + registry, + mailer, + urlServiceMock, + mock(), + mock(), + mock(), + ); + }; + + it.each([ + ['a query string', `${FIRST_PARTY_URL}?x=1`], + ['a trailing slash', `${FIRST_PARTY_URL}/`], + ['an oversized query string', `${FIRST_PARTY_URL}?z=${'a'.repeat(2048)}`], + ])( + 'returns undefined and does not upsert when the client_id is the resource URL with %s', + async (_, clientId) => { + oauthClientRepository.findOneBy.mockResolvedValue(null); + + const result = + await buildServiceWithQueryIgnoringResolver().clientsStore.getClient(clientId); + + expect(result).toBeUndefined(); + expect(oauthClientRepository.upsert).not.toHaveBeenCalled(); + }, + ); + + it('upserts the virtual client when the client_id is the canonical resource URL', async () => { + oauthClientRepository.findOneBy.mockResolvedValue(null); + + const result = + await buildServiceWithQueryIgnoringResolver().clientsStore.getClient(FIRST_PARTY_URL); + + expect(result).toMatchObject({ client_id: FIRST_PARTY_URL }); + expect(oauthClientRepository.upsert).toHaveBeenCalledWith( + expect.objectContaining({ id: FIRST_PARTY_URL, redirectUris: [FIRST_PARTY_URL] }), + ['id'], + ); + }); + it('returns undefined and does not upsert when the resolved resource is not first-party', async () => { oauthClientRepository.findOneBy.mockResolvedValue(null); diff --git a/packages/cli/src/modules/oauth-server/oauth-server.service.ts b/packages/cli/src/modules/oauth-server/oauth-server.service.ts index 3ae38b85755..776fa66f138 100644 --- a/packages/cli/src/modules/oauth-server/oauth-server.service.ts +++ b/packages/cli/src/modules/oauth-server/oauth-server.service.ts @@ -266,7 +266,7 @@ export class OAuthServerService implements OAuthServerProvider { // base URL, so a client_id that isn't can never resolve to one. Skip the resolver // sweep + lazy upsert for anything else, so the unauthenticated /authorize path // can't be used to fan out DB lookups on arbitrary client_ids. - if (!this.isTriggerResourceClientId(clientId)) { + if (clientId.length > MAX_REDIRECT_URI_LENGTH || !this.isTriggerResourceClientId(clientId)) { return undefined; } @@ -275,6 +275,12 @@ export class OAuthServerService implements OAuthServerProvider { return undefined; } + // The lookup ignores the query string, so without this check each `?x=N` variant adds a new row. + const resourceUrls = resource.getResourceUrls?.() ?? [resource.getResourceUrl()]; + if (!resourceUrls.includes(clientId)) { + return undefined; + } + await this.oauthClientRepository.upsert( { id: clientId, diff --git a/packages/cli/src/public-api/v1/controllers/community-packages.public.controller.ts b/packages/cli/src/public-api/v1/controllers/community-packages.public.controller.ts index e893f8ebd62..0afc9c9888b 100644 --- a/packages/cli/src/public-api/v1/controllers/community-packages.public.controller.ts +++ b/packages/cli/src/public-api/v1/controllers/community-packages.public.controller.ts @@ -62,7 +62,7 @@ export class CommunityPackagesPublicController { @Body body: InstallCommunityPackagePublicDto, ): Promise { const installedPackage = await this.communityPackagesLifecycleService.install( - { name: body.name, version: body.version, verify: body.verify ?? true }, + { name: body.name, version: body.version }, req.user, 'publicApi', ); @@ -84,7 +84,7 @@ export class CommunityPackagesPublicController { @Body body: UpdateCommunityPackagePublicDto, ): Promise { const updated = await this.communityPackagesLifecycleService.update( - { name, version: body.version, verify: body.verify ?? true }, + { name, version: body.version }, req.user, 'notFound', ); diff --git a/packages/cli/src/public-api/v1/handlers/community-packages/spec/paths/installPackage.generated.yml b/packages/cli/src/public-api/v1/handlers/community-packages/spec/paths/installPackage.generated.yml index 8dac4ea86f9..8ef12043332 100644 --- a/packages/cli/src/public-api/v1/handlers/community-packages/spec/paths/installPackage.generated.yml +++ b/packages/cli/src/public-api/v1/handlers/community-packages/spec/paths/installPackage.generated.yml @@ -19,10 +19,7 @@ requestBody: description: npm package name. Unscoped names start with n8n-nodes-. Scoped names use the @[author]/n8n-nodes- form. version: type: string - description: Semver version or npm dist-tag, such as latest or beta - verify: - type: boolean - description: Whether to verify the package against the n8n-vetted package list. Required when the instance has N8N_UNVERIFIED_PACKAGES_ENABLED=false. + description: Semver version or npm dist-tag, such as latest or beta. When unverified packages are disabled, only versions verified by n8n can be installed. required: - name responses: diff --git a/packages/cli/src/public-api/v1/handlers/community-packages/spec/paths/updatePackage.generated.yml b/packages/cli/src/public-api/v1/handlers/community-packages/spec/paths/updatePackage.generated.yml index 6dd7d8d60a2..38d457cce4c 100644 --- a/packages/cli/src/public-api/v1/handlers/community-packages/spec/paths/updatePackage.generated.yml +++ b/packages/cli/src/public-api/v1/handlers/community-packages/spec/paths/updatePackage.generated.yml @@ -22,10 +22,7 @@ requestBody: properties: version: type: string - description: Semver version or npm dist-tag to update to, such as latest or beta - verify: - type: boolean - description: Whether to verify the package against the n8n-vetted package list. Setting to false will allow installing or updating to an unverified version. Default is true. + description: Semver version or npm dist-tag to update to, such as latest or beta. When unverified packages are disabled, only versions verified by n8n can be installed. responses: '200': description: Operation successful. diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 2ecd943c82c..b36f775287e 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -211,15 +211,7 @@ export declare namespace AnnotationTagsRequest { export declare namespace NodeRequest { type GetAll = AuthenticatedRequest; - type Post = AuthenticatedRequest< - {}, - {}, - { name?: string; verify?: boolean; version?: string; checksum?: string } - >; - type Delete = AuthenticatedRequest<{}, {}, {}, { name: string }>; - - type Update = Post; } // ---------------------------------- diff --git a/packages/cli/src/workflow-execute-additional-data.ts b/packages/cli/src/workflow-execute-additional-data.ts index 4272c1b77e2..4f26c8df0e1 100644 --- a/packages/cli/src/workflow-execute-additional-data.ts +++ b/packages/cli/src/workflow-execute-additional-data.ts @@ -156,9 +156,9 @@ async function fetchWorkflowData( } else { const workflowData = workflowInfo.code; if (workflowData) { - if (!workflowData.id) { - workflowData.id = parentWorkflowId; - } + // An inline sub-workflow is part of the parent that embeds it, not a + // workflow of its own, so it runs under the parent workflow's id. + workflowData.id = parentWorkflowId; workflowData.settings ??= parentWorkflowSettings; } return workflowData; diff --git a/packages/cli/src/workflows/__tests__/error-workflow-validation.service.test.ts b/packages/cli/src/workflows/__tests__/error-workflow-validation.service.test.ts new file mode 100644 index 00000000000..05142857884 --- /dev/null +++ b/packages/cli/src/workflows/__tests__/error-workflow-validation.service.test.ts @@ -0,0 +1,186 @@ +import { mockInstance } from '@n8n/backend-test-utils'; +import type { GlobalConfig } from '@n8n/config'; +import type { User, WorkflowEntity, WorkflowHistory } from '@n8n/db'; +import type { INode } from 'n8n-workflow'; +import { mock } from 'vitest-mock-extended'; + +import { SubworkflowPolicyDenialError } from '@/errors/subworkflow-policy-denial.error'; +import type { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks/subworkflow-policy-checker'; +import { NodeTypes } from '@/node-types'; +import { + ErrorWorkflowValidationService, + staticErrorWorkflowId, +} from '@/workflows/error-workflow-validation.service'; +import type { WorkflowFinderService } from '@/workflows/workflow-finder.service'; +import type { + PublishedWorkflowData, + WorkflowPublishedDataService, +} from '@/workflows/workflow-published-data.service'; + +const ERROR_TRIGGER_TYPE = 'n8n-nodes-base.errorTrigger'; + +describe('staticErrorWorkflowId', () => { + it('returns the id for a concrete reference', () => { + expect(staticErrorWorkflowId('wf-1')).toBe('wf-1'); + }); + + it.each([undefined, '', 'DEFAULT', '={{ $json.handler }}'])( + 'returns undefined for %s, which names no concrete workflow', + (value) => { + expect(staticErrorWorkflowId(value)).toBeUndefined(); + }, + ); +}); + +describe('ErrorWorkflowValidationService', () => { + const user = mock({ id: 'user-1' }); + const nodeTypes = mockInstance(NodeTypes); + + let workflowFinderService: WorkflowFinderService; + let workflowPublishedDataService: WorkflowPublishedDataService; + let subworkflowPolicyChecker: SubworkflowPolicyChecker; + let service: ErrorWorkflowValidationService; + + const buildErrorWorkflow = (overrides: Partial = {}) => + mock({ + id: 'err-wf', + name: 'Error Handler', + settings: {}, + activeVersionId: 'err-wf-v1', + activeVersion: mock({ + nodes: [{ type: ERROR_TRIGGER_TYPE, disabled: false }] as INode[], + }), + ...overrides, + }); + + const buildService = (useWorkflowPublicationService = false) => { + const globalConfig = mock({ + nodes: { errorTriggerType: ERROR_TRIGGER_TYPE }, + workflows: { useWorkflowPublicationService }, + }); + return new ErrorWorkflowValidationService( + globalConfig, + nodeTypes, + workflowFinderService, + workflowPublishedDataService, + subworkflowPolicyChecker, + ); + }; + + const findProblem = async (svc = service) => + await svc.findProblem({ errorWorkflowId: 'err-wf', parentWorkflowId: 'wf-1', user }); + + beforeEach(() => { + workflowFinderService = mock(); + workflowPublishedDataService = mock(); + subworkflowPolicyChecker = mock(); + service = buildService(); + }); + + it('accepts a published, callable workflow with an active Error Trigger', async () => { + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue(buildErrorWorkflow()); + + expect(await findProblem()).toBeUndefined(); + }); + + it('requires read access, so a caller cannot probe ids outside their projects', async () => { + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue(null); + + expect(await findProblem()).toEqual({ reason: 'not-found' }); + expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith( + 'err-wf', + user, + ['workflow:read'], + expect.anything(), + ); + }); + + it('reports a workflow with no published version', async () => { + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue( + buildErrorWorkflow({ activeVersionId: null, activeVersion: null }), + ); + + expect(await findProblem()).toEqual({ reason: 'not-published', name: 'Error Handler' }); + }); + + it('reports a published version whose Error Trigger is disabled', async () => { + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue( + buildErrorWorkflow({ + activeVersion: mock({ + nodes: [{ type: ERROR_TRIGGER_TYPE, disabled: true }] as INode[], + }), + }), + ); + + expect(await findProblem()).toEqual({ + reason: 'no-error-trigger', + name: 'Error Handler', + errorTriggerType: ERROR_TRIGGER_TYPE, + }); + }); + + it('reports a target whose caller policy excludes the parent workflow', async () => { + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue(buildErrorWorkflow()); + vi.mocked(subworkflowPolicyChecker.check).mockRejectedValue( + new SubworkflowPolicyDenialError({ + subworkflowId: 'err-wf', + subworkflowProject: mock(), + instanceUrl: 'http://localhost:5678', + hasReadAccess: false, + }), + ); + + expect(await findProblem()).toEqual({ reason: 'caller-policy', name: 'Error Handler' }); + }); + + it('checks the policy against the parent workflow and the acting user', async () => { + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue(buildErrorWorkflow()); + + await findProblem(); + + expect(subworkflowPolicyChecker.check).toHaveBeenCalledWith( + expect.objectContaining({ id: 'err-wf' }), + 'wf-1', + undefined, + 'user-1', + ); + }); + + it('does not swallow an unexpected failure from the policy checker', async () => { + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue(buildErrorWorkflow()); + vi.mocked(subworkflowPolicyChecker.check).mockRejectedValue(new Error('db is down')); + + await expect(findProblem()).rejects.toThrow('db is down'); + }); + + describe('with the workflow publication service on', () => { + // Runtime reads the published version from the service in this mode, so + // validation must read it from the same place or it would accept a version + // runtime will never run. + it('reads the published nodes from the publication service', async () => { + const withPublicationService = buildService(true); + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue(buildErrorWorkflow()); + vi.mocked(workflowPublishedDataService.getPublishedWorkflowData).mockResolvedValue( + mock({ + publishedVersion: mock({ + nodes: [{ type: ERROR_TRIGGER_TYPE }] as INode[], + }), + }), + ); + + expect(await findProblem(withPublicationService)).toBeUndefined(); + expect(workflowPublishedDataService.getPublishedWorkflowData).toHaveBeenCalledWith('err-wf'); + }); + + it('reports not-published when the service has no published version', async () => { + const withPublicationService = buildService(true); + vi.mocked(workflowFinderService.findWorkflowForUser).mockResolvedValue(buildErrorWorkflow()); + vi.mocked(workflowPublishedDataService.getPublishedWorkflowData).mockResolvedValue(null); + + expect(await findProblem(withPublicationService)).toEqual({ + reason: 'not-published', + name: 'Error Handler', + }); + }); + }); +}); diff --git a/packages/cli/src/workflows/__tests__/workflow.service.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.test.ts index 0b316a53735..f83c9b83d73 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.test.ts @@ -37,6 +37,7 @@ import type { ExternalHooks, WorkflowLifecycleHookActor } from '@/external-hooks import type { RedactionEnforcementService } from '@/modules/redaction/redaction-enforcement.service'; import type { PolicyCleared } from '@n8n/decorators'; import { userHasScopes } from '@/permissions.ee/check-access'; +import type { ErrorWorkflowValidationService } from '@/workflows/error-workflow-validation.service'; import type { PolicyEnforcementService } from '@/policy/policy-enforcement.service'; import { PolicyViolationError } from '@/policy/policy-violation.error'; import type { DurableJobProvisioner } from '@/scheduling/durable-job-provisioner'; @@ -136,6 +137,7 @@ describe('WorkflowService', () => { mock(), // policyEnforcementService workflowPublicationStatusServiceMock, // workflowPublicationStatusService mock(), // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); }); @@ -510,6 +512,7 @@ describe('WorkflowService', () => { mock(), // policyEnforcementService mock(), // workflowPublicationStatusService nodeGroupRulesFlagGateMock, // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); vi.clearAllMocks(); @@ -1354,6 +1357,7 @@ describe('WorkflowService', () => { policyEnforcementServiceMock, // policyEnforcementService mock(), // workflowPublicationStatusService mock(), // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); // Bypass validation internals @@ -2114,6 +2118,7 @@ describe('WorkflowService', () => { mock(), // policyEnforcementService mock(), // workflowPublicationStatusService mock(), // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); }); @@ -2255,6 +2260,7 @@ describe('WorkflowService', () => { mock(), // policyEnforcementService mock(), // workflowPublicationStatusService mock(), // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); }); @@ -2561,6 +2567,7 @@ describe('WorkflowService', () => { mock(), // policyEnforcementService mock(), // workflowPublicationStatusService mock(), // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); }); @@ -2731,6 +2738,7 @@ describe('WorkflowService', () => { policyEnforcementServiceMock, // policyEnforcementService mock(), // workflowPublicationStatusService mock(), // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); }); @@ -2918,6 +2926,7 @@ describe('WorkflowService', () => { mock(), // policyEnforcementService mock(), // workflowPublicationStatusService mock(), // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); }); @@ -3021,6 +3030,7 @@ describe('WorkflowService', () => { mock(), // policyEnforcementService mock(), // workflowPublicationStatusService mock(), // nodeGroupRulesFlagGate + mock(), // errorWorkflowValidationService ); }); @@ -3064,4 +3074,179 @@ describe('WorkflowService', () => { ).rejects.toThrow('Some tags not found'); }); }); + + describe('update() error workflow reference', () => { + let workflowService: WorkflowService; + let workflowFinderServiceMock: MockProxy; + let errorWorkflowValidationServiceMock: MockProxy; + let workflowRepositoryMock: MockProxy<{ update: Mock; updateContent: Mock; findOne: Mock }>; + + const user = mock({ id: 'user-1' }); + + beforeEach(() => { + workflowFinderServiceMock = mock(); + errorWorkflowValidationServiceMock = mock(); + errorWorkflowValidationServiceMock.findProblem.mockResolvedValue(undefined); + workflowRepositoryMock = mock(); + + const ownershipServiceMock = mock(); + ownershipServiceMock.getWorkflowProjectCached.mockResolvedValue( + mock({ id: 'project-1' }), + ); + + workflowService = new WorkflowService( + mock(), // logger + mock(), // sharedWorkflowRepository + workflowRepositoryMock as never, // workflowRepository + mock(), // workflowTagMappingRepository + ownershipServiceMock, // ownershipService + mock(), // tagService + mock(), // workflowHistoryService + mock(), // externalHooks + mock(), // activeWorkflowManager + mock(), // roleService + mock(), // projectService + mock(), // executionPersistence + mock(), // eventService + mock(), // globalConfig + mock(), // folderRepository + workflowFinderServiceMock, // workflowFinderService + mock(), // workflowPublishHistoryRepository + mock(), // outboxRepository + Object.assign(mock(), { + validateCredentialNodeRestrictions: () => ({ isValid: true }), + }), // workflowValidationService + mock(), // nodeTypes + mock(), // webhookService + mock(), // licenseState + mock(), // projectRepository + mock(), // redactionEnforcementService + mock(), // workflowPublicationNotifier + mock(), // scheduleTriggerJobRegistrar + mock(), // pollTriggerJobRegistrar + mock(), // workflowScheduledJobOwner + mock(), // durableJobProvisioner + mock(), // workflowPublishedVersionRepository + mock(), // workflowHookContextService + mock(), // workflowPublishGuard + mock(), // workflowMutationHooks + mock(), // policyEnforcementService + mock(), // workflowPublicationStatusService + mock(), // nodeGroupRulesFlagGate + errorWorkflowValidationServiceMock, // errorWorkflowValidationService + ); + + vi.mocked(WorkflowHelpers.removeDefaultValues).mockImplementation((settings) => settings); + }); + + const setupExistingWorkflow = (settings: Record = {}) => { + const existingWorkflow = mock({ + id: 'workflow-1', + isArchived: false, + versionId: 'v1', + nodes: [], + connections: {}, + settings, + activeVersionId: undefined as unknown as string, + tags: [], + }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(existingWorkflow); + workflowRepositoryMock.findOne.mockResolvedValue(existingWorkflow); + }; + + const update = async ( + settings: Record, + options: { allowUnresolvedErrorWorkflow?: boolean } = {}, + ) => + await workflowService.update(user, { settings } as unknown as WorkflowEntity, 'workflow-1', { + forceSave: true, + ...options, + }); + + it('validates a newly set reference against the acting user', async () => { + setupExistingWorkflow(); + + await update({ errorWorkflow: 'err-wf' }); + + expect(errorWorkflowValidationServiceMock.findProblem).toHaveBeenCalledWith({ + errorWorkflowId: 'err-wf', + parentWorkflowId: 'workflow-1', + user, + }); + }); + + it.each([ + ['not-found', 'does not exist or you do not have access to it'], + ['not-published', 'has no published version'], + ['no-error-trigger', 'no active Error Trigger node'], + ['caller-policy', 'does not allow this workflow to call it'], + ] as const)('rejects the write when the reference is %s', async (reason, message) => { + setupExistingWorkflow(); + errorWorkflowValidationServiceMock.findProblem.mockResolvedValue({ + reason, + name: 'Error Handler', + errorTriggerType: 'n8n-nodes-base.errorTrigger', + } as never); + + await expect(update({ errorWorkflow: 'err-wf' })).rejects.toThrow(BadRequestError); + await expect(update({ errorWorkflow: 'err-wf' })).rejects.toThrow(message); + expect(workflowRepositoryMock.update).not.toHaveBeenCalled(); + }); + + // The message must read the same whether the workflow is missing or merely + // unreadable, otherwise the endpoint reports which ids exist. + it('does not distinguish a missing workflow from an unreadable one', async () => { + setupExistingWorkflow(); + errorWorkflowValidationServiceMock.findProblem.mockResolvedValue({ reason: 'not-found' }); + + await expect(update({ errorWorkflow: 'err-wf' })).rejects.toThrow( + 'The selected error workflow does not exist or you do not have access to it.', + ); + }); + + // The setting is unversioned, so an existing workflow whose handler was since + // archived or restricted must stay saveable. + it('does not re-check a reference the write leaves unchanged', async () => { + setupExistingWorkflow({ errorWorkflow: 'err-wf' }); + + await update({ errorWorkflow: 'err-wf', timezone: 'UTC' }); + + expect(errorWorkflowValidationServiceMock.findProblem).not.toHaveBeenCalled(); + }); + + it.each([ + ['no settings key', {}], + ['a cleared reference', { errorWorkflow: 'DEFAULT' }], + ['an expression', { errorWorkflow: '={{ $json.handler }}' }], + ])('skips validation for %s', async (_label, settings) => { + setupExistingWorkflow(); + + await update(settings); + + expect(errorWorkflowValidationServiceMock.findProblem).not.toHaveBeenCalled(); + }); + + it('validates a reference that replaces a different one', async () => { + setupExistingWorkflow({ errorWorkflow: 'old-err-wf' }); + + await update({ errorWorkflow: 'new-err-wf' }); + + expect(errorWorkflowValidationServiceMock.findProblem).toHaveBeenCalledWith( + expect.objectContaining({ errorWorkflowId: 'new-err-wf' }), + ); + }); + + // A package import points at a handler the same import still has to create and + // publish, so none of the four problems is decidable while it writes. + it('skips validation for a package import', async () => { + setupExistingWorkflow(); + errorWorkflowValidationServiceMock.findProblem.mockResolvedValue({ reason: 'not-found' }); + + await expect( + update({ errorWorkflow: 'err-wf' }, { allowUnresolvedErrorWorkflow: true }), + ).resolves.toBeDefined(); + + expect(errorWorkflowValidationServiceMock.findProblem).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/cli/src/workflows/error-workflow-validation.service.ts b/packages/cli/src/workflows/error-workflow-validation.service.ts new file mode 100644 index 00000000000..86d6271f9bc --- /dev/null +++ b/packages/cli/src/workflows/error-workflow-validation.service.ts @@ -0,0 +1,167 @@ +import { GlobalConfig } from '@n8n/config'; +import type { User } from '@n8n/db'; +import { Service } from '@n8n/di'; +import type { IWorkflowSettings } from 'n8n-workflow'; +import { Workflow } from 'n8n-workflow'; + +import { WorkflowFinderService } from './workflow-finder.service'; +import { WorkflowPublishedDataService } from './workflow-published-data.service'; + +import { SubworkflowPolicyDenialError } from '@/errors/subworkflow-policy-denial.error'; +import { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks/subworkflow-policy-checker'; +import { NodeTypes } from '@/node-types'; + +/** + * Why a workflow cannot serve as another workflow's error handler. Callers turn + * this into their own copy — the MCP tool teaches an agent how to self-correct, + * the REST path states the problem — so the rules stay in one place while the + * wording stays fit for each audience. + */ +export type ErrorWorkflowProblem = + | { reason: 'not-found' } + | { reason: 'not-published'; name: string } + | { reason: 'no-error-trigger'; name: string; errorTriggerType: string } + | { reason: 'caller-policy'; name: string }; + +/** + * True when the setting holds an n8n expression. + * + * Nothing ever evaluates it. `executeErrorWorkflow` reads `settings.errorWorkflow` + * and passes it to the error-workflow lookup verbatim, so a stored `=…` string is + * used as a literal workflow id and can never match one — the handler silently + * never runs. The editor's setting is a workflow picker and cannot produce this; + * only a caller that writes settings directly (API, MCP) can. Such callers should + * reject it rather than save a reference that is dead on arrival. + */ +export function isExpressionErrorWorkflowId( + errorWorkflow: IWorkflowSettings['errorWorkflow'], +): boolean { + return typeof errorWorkflow === 'string' && errorWorkflow.startsWith('='); +} + +/** + * The workflow id `settings.errorWorkflow` actually points at, or undefined when + * the setting names no concrete workflow: cleared (`DEFAULT`), or an expression. + * + * An expression yields undefined because there is no id to validate, NOT because + * it is a usable value — see {@link isExpressionErrorWorkflowId}. + */ +export function staticErrorWorkflowId( + errorWorkflow: IWorkflowSettings['errorWorkflow'], +): string | undefined { + if ( + typeof errorWorkflow !== 'string' || + errorWorkflow === '' || + errorWorkflow === 'DEFAULT' || + isExpressionErrorWorkflowId(errorWorkflow) + ) { + return undefined; + } + return errorWorkflow; +} + +/** + * Validates a `settings.errorWorkflow` reference at write time. + * + * Runtime runs the named workflow with the failed run's context and checks only + * the target's own caller policy, so nothing downstream establishes that the + * person who wrote the setting may point at that workflow at all. Checking on + * write also catches the three ways a link silently never fires: the target is + * gone, unpublished, or has no Error Trigger. + * + * Callers must only run this when the reference actually changes, so a workflow + * that already carries a stale link stays saveable. + */ +@Service() +export class ErrorWorkflowValidationService { + constructor( + private readonly globalConfig: GlobalConfig, + private readonly nodeTypes: NodeTypes, + private readonly workflowFinderService: WorkflowFinderService, + private readonly workflowPublishedDataService: WorkflowPublishedDataService, + private readonly subworkflowPolicyChecker: SubworkflowPolicyChecker, + ) {} + + /** The problem with the reference, or undefined when it is usable. */ + async findProblem({ + errorWorkflowId, + parentWorkflowId, + user, + }: { + errorWorkflowId: string; + parentWorkflowId: string; + user: User; + }): Promise { + const { useWorkflowPublicationService } = this.globalConfig.workflows; + const { errorTriggerType } = this.globalConfig.nodes; + + // Read access is required intentionally, mirroring the editor UI (the error + // workflow picker only lists workflows the user can read). Resolving the + // target without an access check would let callers probe arbitrary workflow + // IDs and learn their name / published / trigger / policy state from the + // problems below. Runtime not requiring read access is separate: it runs the + // error workflow under the owner project's context, gated by caller policy, + // which is about execution — not about who may configure the link. + const errorWorkflow = await this.workflowFinderService.findWorkflowForUser( + errorWorkflowId, + user, + ['workflow:read'], + // activeVersion is only the published source of truth when the publication + // service is off; otherwise we read it from the service below. + { includeActiveVersion: !useWorkflowPublicationService }, + ); + + if (!errorWorkflow) return { reason: 'not-found' }; + + // Runtime runs the PUBLISHED version of the error workflow, not its draft, and + // resolves it differently depending on the publication service flag — mirror + // WorkflowExecutionService.loadErrorWorkflowData exactly so we neither reject a + // workflow runtime would run nor accept a version runtime will not use. + const publishedNodes = useWorkflowPublicationService + ? (await this.workflowPublishedDataService.getPublishedWorkflowData(errorWorkflowId)) + ?.publishedVersion.nodes + : errorWorkflow.activeVersionId && errorWorkflow.activeVersion + ? (errorWorkflow.activeVersion.nodes ?? []) + : undefined; + + if (!publishedNodes) return { reason: 'not-published', name: errorWorkflow.name }; + + const hasErrorTrigger = publishedNodes.some( + (node) => node.type === errorTriggerType && node.disabled !== true, + ); + + if (!hasErrorTrigger) { + return { reason: 'no-error-trigger', name: errorWorkflow.name, errorTriggerType }; + } + + // Runtime blocks the error workflow if this workflow may not call it as a + // sub-workflow (see WorkflowExecutionService.executeErrorWorkflow). The + // policy checker only reads the target's id + settings, so an empty-node + // Workflow instance is sufficient. + const errorWorkflowInstance = new Workflow({ + id: errorWorkflow.id, + name: errorWorkflow.name, + nodeTypes: this.nodeTypes, + nodes: [], + connections: {}, + active: false, + settings: errorWorkflow.settings ?? {}, + }); + + try { + await this.subworkflowPolicyChecker.check( + errorWorkflowInstance, + parentWorkflowId, + undefined, + user.id, + ); + } catch (error) { + if (error instanceof SubworkflowPolicyDenialError) { + return { reason: 'caller-policy', name: errorWorkflow.name }; + } + throw error; + } + + return undefined; + } +} diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index 5acd1fa2b40..3529d52e40c 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -30,6 +30,11 @@ import type { INode, INodes, IWorkflowSettings, JsonValue, IConnections } from ' import { PROJECT_ROOT, Workflow, assert, calculateWorkflowChecksum } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; +import { + ErrorWorkflowValidationService, + staticErrorWorkflowId, + type ErrorWorkflowProblem, +} from './error-workflow-validation.service'; import { WorkflowPublicationNotifier } from './publication/workflow-publication-notifier'; import { WorkflowPublicationStatusService } from './publication/workflow-publication-status.service'; import { NodeGroupRulesFlagGate } from './node-group-rules-flag-gate'; @@ -129,8 +134,66 @@ export class WorkflowService { private readonly policyEnforcementService: PolicyEnforcementService, private readonly workflowPublicationStatusService: WorkflowPublicationStatusService, private readonly nodeGroupRulesFlagGate: NodeGroupRulesFlagGate, + private readonly errorWorkflowValidationService: ErrorWorkflowValidationService, ) {} + /** + * Rejects a write that points `settings.errorWorkflow` at a workflow the + * acting user may not link to, or one that could never handle the failure. + * + * Only a *changed* reference is checked: the setting is unversioned and takes + * effect on the next failure, so this is the one moment authorization can be + * applied — but re-checking an unchanged value would make a workflow whose + * handler was since archived or restricted impossible to save at all. + * + * This is a check on *authoring* a reference (UI, API, MCP), which is why a + * package import opts out with `allowUnresolvedErrorWorkflow`. An import writes + * package content whose references it rebinds itself, and mid-import none of + * these four problems is decidable: the handler may be created later in the same + * apply loop, it is published only by the package-wide sweep that runs after every + * write, and its own `callerIds` are rebound as part of that same content. Nor + * would enforcing it here buy anything — the create path has no such check, so a + * package reaches the same end state by importing the parent as a new workflow. + */ + private async assertErrorWorkflowChangeAllowed( + user: User, + workflowId: string, + currentSettings: IWorkflowSettings | undefined, + updateSettings: IWorkflowSettings | undefined, + ) { + const nextId = staticErrorWorkflowId(updateSettings?.errorWorkflow); + if (!nextId || nextId === staticErrorWorkflowId(currentSettings?.errorWorkflow)) return; + + const problem = await this.errorWorkflowValidationService.findProblem({ + errorWorkflowId: nextId, + parentWorkflowId: workflowId, + user, + }); + + if (!problem) return; + + this.logger.warn('Rejected an error workflow reference the user may not set', { + workflowId, + errorWorkflowId: nextId, + userId: user.id, + reason: problem.reason, + }); + + // `not-found` covers "no such workflow" and "you cannot read it" alike, so + // the message must not distinguish them — telling them apart would turn + // this into a probe for workflow ids outside the user's projects. + const messages: Record = { + 'not-found': 'The selected error workflow does not exist or you do not have access to it.', + 'not-published': 'The selected error workflow has no published version.', + 'no-error-trigger': + 'The published version of the selected error workflow has no active Error Trigger node.', + 'caller-policy': + 'The selected error workflow does not allow this workflow to call it. Update its "This workflow can be called by" setting.', + }; + + throw new BadRequestError(messages[problem.reason]); + } + async getMany( user: User, options?: ListQuery.Options, @@ -412,6 +475,11 @@ export class WorkflowService { versionDescription?: string; /** Allows a package import to update archived content. */ allowArchivedUpdate?: boolean; + /** + * Skips the `settings.errorWorkflow` write-time check for a package import. + * See {@link assertErrorWorkflowChangeAllowed}. + */ + allowUnresolvedErrorWorkflow?: boolean; } = {}, ): Promise { const { @@ -428,6 +496,7 @@ export class WorkflowService { versionName, versionDescription, allowArchivedUpdate = false, + allowUnresolvedErrorWorkflow = false, } = options; const workflow = await this.workflowFinderService.findWorkflowForUser(workflowId, user, [ 'workflow:update', @@ -452,6 +521,15 @@ export class WorkflowService { workflowUpdateData.settings?.redactionPolicy, ); + if (!allowUnresolvedErrorWorkflow) { + await this.assertErrorWorkflowChangeAllowed( + user, + workflowId, + workflow.settings, + workflowUpdateData.settings, + ); + } + if (!forceSave && expectedChecksum) { await this._detectConflicts(workflow, expectedChecksum); } diff --git a/packages/cli/test/integration/mcp/get-execution.tool.test.ts b/packages/cli/test/integration/mcp/get-execution.tool.test.ts new file mode 100644 index 00000000000..702f7fd9f33 --- /dev/null +++ b/packages/cli/test/integration/mcp/get-execution.tool.test.ts @@ -0,0 +1,151 @@ +import { LicenseState } from '@n8n/backend-common'; +import { + createWorkflow, + shareWorkflowWithUsers, + mockInstance, + testDb, + testModules, +} from '@n8n/backend-test-utils'; +import { ExecutionRepository, type User } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { stringify } from 'flatted'; +import { createEmptyRunExecutionData, type IRunExecutionData } from 'n8n-workflow'; + +import { createExecution } from '../shared/db/executions'; +import { createMember, createOwner } from '../shared/db/users'; + +import { ExecutionRedactionServiceProxy } from '@/executions/execution-redaction-proxy.service'; +import { createGetExecutionTool } from '@/modules/mcp/tools/get-execution.tool'; +import { RedactionModule } from '@/modules/redaction/redaction.module'; +import { Telemetry } from '@/telemetry'; +import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; + +beforeAll(async () => { + await testModules.loadModules(['mcp', 'redaction']); + await testDb.init(); + // `loadModules` registers modules but never runs their `init`, which is what + // hands the real implementation to `ExecutionRedactionServiceProxy`. + await Container.get(RedactionModule).init(); +}); + +afterAll(async () => { + await testDb.terminate(); +}); + +const telemetry = mockInstance(Telemetry, { track: vi.fn() }); + +/** + * Data redaction is licensed-gated, and `resolvePolicy` returns 'none' without it. + */ +const licenseState = mockInstance(LicenseState, { + isDataRedactionLicensed: () => true, +}); + +const SECRET = 'super-secret-value'; + +const runExecutionData = (): IRunExecutionData => ({ + ...createEmptyRunExecutionData(), + resultData: { + runData: { + 'HTTP Request': [ + { + startTime: Date.now(), + executionIndex: 0, + executionTime: 1, + executionStatus: 'success', + source: [], + data: { main: [[{ json: { secret: SECRET } }]] }, + }, + ], + }, + }, +}); + +const callTool = async (user: User, workflowId: string, executionId: string) => { + const tool = createGetExecutionTool( + user, + Container.get(ExecutionRepository), + Container.get(WorkflowFinderService), + telemetry, + Container.get(ExecutionRedactionServiceProxy), + ); + + return await tool.handler( + { + workflowId, + executionId, + includeData: true, + nodeNames: undefined, + truncateData: undefined, + }, + {} as never, + ); +}; + +const itemOf = (result: Awaited>) => { + const data = (result.structuredContent as { data: IRunExecutionData }).data; + return data.resultData.runData['HTTP Request'][0].data?.main[0]?.[0]; +}; + +const redactionInfoOf = (result: Awaited>) => + (result.structuredContent as { data: IRunExecutionData }).data.redactionInfo; + +const setup = async ({ redact, owner }: { redact: boolean; owner: User }) => { + const workflow = await createWorkflow( + { + settings: { + availableInMCP: true, + ...(redact && { redactionPolicy: 'all' as const }), + }, + }, + owner, + ); + + const execution = await createExecution( + { status: 'success', data: stringify(runExecutionData()) }, + workflow, + ); + + return { workflow, execution }; +}; + +describe('get_workflow_execution applies the workflow redaction policy', () => { + let owner: User; + let member: User; + + beforeEach(async () => { + await testDb.truncate(['ExecutionEntity', 'WorkflowEntity', 'SharedWorkflow', 'User']); + owner = await createOwner(); + member = await createMember(); + expect(licenseState.isDataRedactionLicensed()).toBe(true); + }); + + test('redacts for the owner, who may still reveal', async () => { + const { workflow, execution } = await setup({ redact: true, owner }); + + const result = await callTool(owner, workflow.id, execution.id); + + expect(itemOf(result)?.json).toEqual({}); + expect(redactionInfoOf(result)).toMatchObject({ isRedacted: true, canReveal: true }); + }); + + test('redacts for a shared editor, who may not reveal', async () => { + const { workflow, execution } = await setup({ redact: true, owner }); + await shareWorkflowWithUsers(workflow, [member]); + + const result = await callTool(member, workflow.id, execution.id); + + // The `workflow:editor` sharing role carries no `execution:reveal` scope. + expect(itemOf(result)?.json).toEqual({}); + expect(redactionInfoOf(result)).toMatchObject({ isRedacted: true, canReveal: false }); + }); + + test('leaves data untouched when the workflow has no redaction policy', async () => { + const { workflow, execution } = await setup({ redact: false, owner }); + + const result = await callTool(owner, workflow.id, execution.id); + + expect(itemOf(result)?.json).toEqual({ secret: SECRET }); + expect(redactionInfoOf(result)).toBeUndefined(); + }); +}); diff --git a/packages/cli/test/integration/public-api/community-packages.test.ts b/packages/cli/test/integration/public-api/community-packages.test.ts index 04f1e29f9f5..506f2b2498b 100644 --- a/packages/cli/test/integration/public-api/community-packages.test.ts +++ b/packages/cli/test/integration/public-api/community-packages.test.ts @@ -9,12 +9,14 @@ vi.mock('@/modules/community-packages/npm-utils', async () => ({ import type { CommunityNodeType } from '@n8n/api-types'; import { mockInstance, testDb } from '@n8n/backend-test-utils'; import type { User } from '@n8n/db'; +import { Container } from '@n8n/di'; import type { ApiKeyScope } from '@n8n/permissions'; import { OWNER_API_KEY_SCOPES } from '@n8n/permissions'; import { mock } from 'vitest-mock-extended'; import { LoadNodesAndCredentials } from '@/load-nodes-and-credentials'; import { CommunityNodeTypesService } from '@/modules/community-packages/community-node-types.service'; +import { CommunityPackagesConfig } from '@/modules/community-packages/community-packages.config'; import { CommunityPackagesService } from '@/modules/community-packages/community-packages.service'; import { executeNpmCommand } from '@/modules/community-packages/npm-utils'; @@ -52,6 +54,7 @@ const parsedNpmPackageName = { }; let owner: User; +const communityPackagesConfig = Container.get(CommunityPackagesConfig); describe('Community packages (Public API)', () => { beforeAll(async () => { @@ -60,6 +63,7 @@ describe('Community packages (Public API)', () => { beforeEach(async () => { vi.resetAllMocks(); + communityPackagesConfig.unverifiedEnabled = true; communityPackagesService.withLoadStatus.mockImplementation((packages) => packages); communityNodeTypesService.findVetted.mockResolvedValue(mockedVettedPackage); await testDb.truncate(['User']); @@ -248,12 +252,10 @@ describe('Community packages (Public API)', () => { createdAt: pkg.createdAt.toISOString(), updatedAt: pkg.updatedAt.toISOString(), }); - // No version requested, so install() pins the latest vetted version: - // the same catalog lookup provides both it and the checksum. expect(communityPackagesService.installPackage).toHaveBeenCalledWith( parsedNpmPackageName.packageName, - mockedVettedPackage.npmVersion, - mockedVettedPackage.checksum, + undefined, + undefined, ); }); @@ -272,12 +274,13 @@ describe('Community packages (Public API)', () => { expect(response.status).toBe(200); expect(communityPackagesService.installPackage).toHaveBeenCalledWith( parsedNpmPackageName.packageName, - mockedVettedPackage.npmVersion, - mockedVettedPackage.checksum, + undefined, + undefined, ); }); - it('should install with verification disabled when verify is false', async () => { + it('should install the latest vetted version when unverified packages are disabled', async () => { + communityPackagesConfig.unverifiedEnabled = false; const pkg = mockPackage(); communityPackagesService.parseNpmPackageName.mockReturnValue(parsedNpmPackageName); communityPackagesService.findInstalledPackage.mockResolvedValue(null); @@ -287,18 +290,18 @@ describe('Community packages (Public API)', () => { const response = await testServer .publicApiAgentFor(owner) .post('/community-packages') - .send({ name: mockPackageName(), verify: false }); + .send({ name: mockPackageName() }); expect(response.status).toBe(200); - expect(communityNodeTypesService.findVetted).not.toHaveBeenCalled(); expect(communityPackagesService.installPackage).toHaveBeenCalledWith( parsedNpmPackageName.packageName, - undefined, - undefined, + mockedVettedPackage.npmVersion, + mockedVettedPackage.checksum, ); }); - it('should return 400 when package is not vetted', async () => { + it('should return 400 when unverified packages are disabled and the package is not vetted', async () => { + communityPackagesConfig.unverifiedEnabled = false; communityNodeTypesService.findVetted.mockResolvedValue(undefined); communityPackagesService.parseNpmPackageName.mockReturnValue(parsedNpmPackageName); @@ -344,18 +347,32 @@ describe('Community packages (Public API)', () => { expect(communityPackagesService.updatePackage).not.toHaveBeenCalled(); }); - it('should return 400 when update options are invalid', async () => { + it('should update without a checksum and ignore a verify field when unverified packages are enabled', async () => { + const pkg = mockPackage(); + communityPackagesService.findInstalledPackage.mockResolvedValue(pkg); + communityPackagesService.parseNpmPackageName.mockReturnValue({ + packageName: pkg.packageName, + rawString: pkg.packageName, + }); + communityPackagesService.updatePackage.mockResolvedValue(pkg); + const response = await testServer .publicApiAgentFor(owner) - .patch(`/community-packages/${encodeURIComponent(mockPackageName())}`) - .send({ verify: 'false' }); + .patch(`/community-packages/${encodeURIComponent(pkg.packageName)}`) + .send({ version: COMMUNITY_PACKAGE_VERSION.UPDATED, verify: true }); - expect(response.status).toBe(400); - expect(response.body.message).toContain('verify'); - expect(communityPackagesService.updatePackage).not.toHaveBeenCalled(); + expect(response.status).toBe(200); + expect(communityNodeTypesService.findVetted).not.toHaveBeenCalled(); + expect(communityPackagesService.updatePackage).toHaveBeenCalledWith( + pkg.packageName, + pkg, + COMMUNITY_PACKAGE_VERSION.UPDATED, + undefined, + ); }); - it('should return 200 when package is updated successfully', async () => { + it('should update with the vetted checksum when unverified packages are disabled', async () => { + communityPackagesConfig.unverifiedEnabled = false; const packageName = mockPackageName(); const pkg = mockPackage({ packageName }); const updatedNode = mockNode(packageName); @@ -406,6 +423,10 @@ describe('Community packages (Public API)', () => { it('should return 404 when package is not installed', async () => { const name = mockPackageName(); communityPackagesService.findInstalledPackage.mockResolvedValue(null); + communityPackagesService.parseNpmPackageName.mockReturnValue({ + packageName: name, + rawString: name, + }); const response = await testServer .publicApiAgentFor(owner) diff --git a/packages/cli/test/integration/workflows/workflow.service.test.ts b/packages/cli/test/integration/workflows/workflow.service.test.ts index b8c6dcf2096..cee67a2f2cc 100644 --- a/packages/cli/test/integration/workflows/workflow.service.test.ts +++ b/packages/cli/test/integration/workflows/workflow.service.test.ts @@ -44,6 +44,7 @@ import { WorkflowPublicationStatusService } from '@/workflows/publication/workfl import { WorkflowHistoryService } from '@/workflows/workflow-history/workflow-history.service'; import type { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service'; import { WorkflowValidationService } from '@/workflows/workflow-validation.service'; +import { ErrorWorkflowValidationService } from '@/workflows/error-workflow-validation.service'; import { WorkflowService } from '@/workflows/workflow.service'; import { createCustomRoleWithScopeSlugs, cleanupRolesAndScopes } from '../shared/db/roles'; @@ -129,6 +130,7 @@ beforeAll(async () => { Container.get(PolicyEnforcementService), // policyEnforcementService Container.get(WorkflowPublicationStatusService), // workflowPublicationStatusService Container.get(NodeGroupRulesFlagGate), // nodeGroupRulesFlagGate + Container.get(ErrorWorkflowValidationService), // errorWorkflowValidationService ); }); diff --git a/packages/cli/test/integration/workflows/workflows.controller.test.ts b/packages/cli/test/integration/workflows/workflows.controller.test.ts index 0fa9c344035..545d9aa59a1 100644 --- a/packages/cli/test/integration/workflows/workflows.controller.test.ts +++ b/packages/cli/test/integration/workflows/workflows.controller.test.ts @@ -7,6 +7,7 @@ import { createActiveWorkflow, setActiveVersion, createWorkflowWithHistory, + createWorkflowHistory, shareWorkflowWithProjects, shareWorkflowWithUsers, randomCredentialPayload, @@ -5732,3 +5733,126 @@ describe('POST /workflows/with-node-types', () => { expect(response.body.count).toBe(1); }); }); + +describe('PATCH /workflows/:id - error workflow reference', () => { + const ERROR_TRIGGER_TYPE = 'n8n-nodes-base.errorTrigger'; + + /** A published handler with an active Error Trigger, owned by `ownerUser`. */ + const createErrorHandler = async (ownerUser: User, name: string) => { + const handler = await createWorkflow( + { + name, + nodes: [ + { + id: uuid(), + name: 'Error Trigger', + type: ERROR_TRIGGER_TYPE, + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + ], + connections: {}, + settings: { callerPolicy: 'any' }, + }, + ownerUser, + ); + await createWorkflowHistory(handler, ownerUser); + await setActiveVersion(handler.id, handler.versionId); + return handler; + }; + + /** A victim workflow the member may edit but does not own. */ + const createSharedVictimWorkflow = async () => { + const victim = await createWorkflowWithHistory({ name: 'Victim' }, owner); + await shareWorkflowWithUsers(victim, [member]); + return victim; + }; + + test('rejects pointing a workflow at an error workflow the caller cannot read', async () => { + const victim = await createSharedVictimWorkflow(); + const foreignHandler = await createErrorHandler(anotherMember, 'Foreign Handler'); + + const response = await authMemberAgent + .patch(`/workflows/${victim.id}`) + .send({ settings: { errorWorkflow: foreignHandler.id } }); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toContain('does not exist or you do not have access to it'); + + const stored = await workflowRepository.findOneBy({ id: victim.id }); + expect(stored?.settings?.errorWorkflow).toBeUndefined(); + }); + + test('rejects an error workflow that has no published version', async () => { + const victim = await createSharedVictimWorkflow(); + const draftHandler = await createWorkflow({ name: 'Draft Handler' }, member); + + const response = await authMemberAgent + .patch(`/workflows/${victim.id}`) + .send({ settings: { errorWorkflow: draftHandler.id } }); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toContain('has no published version'); + }); + + test('rejects a published error workflow with no Error Trigger', async () => { + const victim = await createSharedVictimWorkflow(); + const handler = await createWorkflowWithHistory({ name: 'Plain Handler' }, member); + await setActiveVersion(handler.id, handler.versionId); + + const response = await authMemberAgent + .patch(`/workflows/${victim.id}`) + .send({ settings: { errorWorkflow: handler.id } }); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toContain('no active Error Trigger node'); + }); + + test('accepts a published, readable error workflow with an Error Trigger', async () => { + const victim = await createSharedVictimWorkflow(); + const handler = await createErrorHandler(member, 'Own Handler'); + + const response = await authMemberAgent + .patch(`/workflows/${victim.id}`) + .send({ settings: { errorWorkflow: handler.id } }); + + expect(response.statusCode).toBe(200); + + const stored = await workflowRepository.findOneBy({ id: victim.id }); + expect(stored?.settings?.errorWorkflow).toBe(handler.id); + }); + + // The setting is unversioned, so re-checking an unchanged value would make a + // workflow whose handler was since archived or restricted impossible to save. + test('still saves a workflow whose existing error workflow is no longer valid', async () => { + const victim = await createWorkflowWithHistory( + { name: 'Victim', settings: { errorWorkflow: 'long-gone-workflow-id' } }, + owner, + ); + await shareWorkflowWithUsers(victim, [member]); + + const response = await authMemberAgent + .patch(`/workflows/${victim.id}`) + .send({ settings: { errorWorkflow: 'long-gone-workflow-id', timezone: 'UTC' } }); + + expect(response.statusCode).toBe(200); + }); + + test('allows clearing the reference', async () => { + const victim = await createWorkflowWithHistory( + { name: 'Victim', settings: { errorWorkflow: 'long-gone-workflow-id' } }, + owner, + ); + await shareWorkflowWithUsers(victim, [member]); + + const response = await authMemberAgent + .patch(`/workflows/${victim.id}`) + .send({ settings: { errorWorkflow: 'DEFAULT' } }); + + expect(response.statusCode).toBe(200); + + const stored = await workflowRepository.findOneBy({ id: victim.id }); + expect(stored?.settings?.errorWorkflow).toBeUndefined(); + }); +}); diff --git a/packages/core/src/execution-engine/__tests__/execution-context.test.ts b/packages/core/src/execution-engine/__tests__/execution-context.test.ts index 2d41b12db61..02f82b7fe9b 100644 --- a/packages/core/src/execution-engine/__tests__/execution-context.test.ts +++ b/packages/core/src/execution-engine/__tests__/execution-context.test.ts @@ -937,8 +937,9 @@ describe('establishExecutionContext', () => { const errorContext = runExecutionData.executionData!.runtimeData!; - // Should inherit credentials from failed workflow - expect(errorContext.credentials).toBe('original-workflow-credentials'); + // Must NOT inherit the failed run's identity carrier: the error workflow + // reports the failure, it does not act as that run's user. + expect(errorContext.credentials).toBeUndefined(); // Should have fresh establishedAt expect(errorContext.establishedAt).toBeGreaterThanOrEqual(beforeTimestamp); @@ -1039,8 +1040,9 @@ describe('establishExecutionContext', () => { const errorContext = runExecutionData.executionData!.runtimeData!; - // Should inherit root credentials - expect(errorContext.credentials).toBe('root-workflow-credentials'); + // The carrier is dropped at every depth, so a failure nested under a + // sub-workflow cannot hand the root run's identity on either. + expect(errorContext.credentials).toBeUndefined(); // Should track the failed sub-workflow expect(errorContext.parentExecutionId).toBe('failed-sub-workflow-id'); @@ -1051,6 +1053,167 @@ describe('establishExecutionContext', () => { }); }); + describe('error workflow identity isolation', () => { + // `executeErrorWorkflow` populates BOTH `runExecutionData.parentExecution` + // and the start item's metadata, and the first wins. Each case below drives + // one of the two branches so neither can regress on its own. + const sealedParentContext: IExecutionContext = { + version: 1, + establishedAt: 2000000000, + source: 'webhook', + credentials: 'sealed-identity-carrier', + secureArtifacts: 'stripped-trigger-values', + redaction: { version: 2, production: true, manual: false }, + executedByUserId: 'user-who-ran-the-failed-workflow', + usesDynamicCredentials: true, + }; + + const errorTriggerNode = mock({ + name: 'ErrorTrigger', + type: 'n8n-nodes-base.errorTrigger', + }); + + const buildErrorRun = (source: 'parentExecution' | 'startItemMetadata') => { + const parentExecution: RelatedExecution = { + executionId: 'failed-execution-id', + workflowId: 'failed-workflow-id', + executionContext: sealedParentContext, + }; + + return createRunExecutionData({ + startData: {}, + resultData: { runData: {} }, + executionData: { + contextData: {}, + nodeExecutionStack: [ + { + node: errorTriggerNode, + data: { main: [[{ json: { error: 'boom' } }]] }, + source: null, + ...(source === 'startItemMetadata' && { metadata: { parentExecution } }), + }, + ], + metadata: {}, + waitingExecution: {}, + waitingExecutionSource: {}, + }, + ...(source === 'parentExecution' && { parentExecution }), + }); + }; + + it.each(['parentExecution', 'startItemMetadata'] as const)( + 'does not pass the identity carrier to an error workflow (via %s)', + async (source) => { + const runExecutionData = buildErrorRun(source); + + await establishExecutionContext( + mockWorkflow, + runExecutionData, + mockAdditionalData, + 'error', + ); + + const errorContext = runExecutionData.executionData!.runtimeData!; + + expect(errorContext.credentials).toBeUndefined(); + expect(errorContext.secureArtifacts).toBeUndefined(); + }, + ); + + it.each(['parentExecution', 'startItemMetadata'] as const)( + 'still inherits the fields the error execution needs (via %s)', + async (source) => { + const runExecutionData = buildErrorRun(source); + + await establishExecutionContext( + mockWorkflow, + runExecutionData, + mockAdditionalData, + 'error', + ); + + const errorContext = runExecutionData.executionData!.runtimeData!; + + // Redaction escalates top-down, so a failure of a redacted run must not + // produce an unredacted error record. + expect(errorContext.redaction).toEqual(sealedParentContext.redaction); + expect(errorContext.usesDynamicCredentials).toBe(true); + expect(errorContext.executedByUserId).toBe('user-who-ran-the-failed-workflow'); + expect(errorContext.parentExecutionId).toBe('failed-execution-id'); + expect(errorContext.source).toBe('error'); + }, + ); + + it('keeps passing the carrier to a sub-workflow, which is a different relationship', async () => { + const runExecutionData = buildErrorRun('parentExecution'); + + // Same shape, but an 'integrated' run: a sub-workflow the parent's own + // graph calls, not a handler named in an unversioned setting. + await establishExecutionContext( + mockWorkflow, + runExecutionData, + mockAdditionalData, + 'integrated', + ); + + const childContext = runExecutionData.executionData!.runtimeData!; + + expect(childContext.credentials).toBe('sealed-identity-carrier'); + expect(childContext.secureArtifacts).toBe('stripped-trigger-values'); + }); + + // The two branches above are skipped when the run data already carries a + // context — a persisted error run that resumes after a Wait, or the second + // establish call once the main process has populated `runtimeData`. + it('strips a carrier from a context the error run already carries', async () => { + const runExecutionData = buildErrorRun('parentExecution'); + runExecutionData.executionData!.runtimeData = { ...sealedParentContext }; + + await establishExecutionContext(mockWorkflow, runExecutionData, mockAdditionalData, 'error'); + + const errorContext = runExecutionData.executionData!.runtimeData!; + + expect(errorContext.credentials).toBeUndefined(); + expect(errorContext.secureArtifacts).toBeUndefined(); + // The rest of the established context survives untouched — this is the + // error run's own context, not an inheritance point. + expect(errorContext.establishedAt).toBe(sealedParentContext.establishedAt); + expect(errorContext.source).toBe('webhook'); + expect(errorContext.redaction).toEqual(sealedParentContext.redaction); + expect(errorContext.executedByUserId).toBe('user-who-ran-the-failed-workflow'); + expect(errorContext.usesDynamicCredentials).toBe(true); + }); + + it('leaves an already-established context alone for a non-error resume', async () => { + const runExecutionData = buildErrorRun('parentExecution'); + runExecutionData.executionData!.runtimeData = { ...sealedParentContext }; + + await establishExecutionContext( + mockWorkflow, + runExecutionData, + mockAdditionalData, + 'webhook', + ); + + expect(runExecutionData.executionData!.runtimeData).toEqual(sealedParentContext); + }); + + // `encryptedRunnerIdentity` is a carrier, so it cannot be filtered — and it + // is spread onto the context AFTER the inheritance gate, so refusing it at + // the source is the only place the gate holds. + it('does not take the runner identity from additionalData on an error run', async () => { + const runExecutionData = buildErrorRun('parentExecution'); + const additionalData = mock({ + encryptedRunnerIdentity: 'encrypted-credential-blob', + executionId: undefined, + }); + + await establishExecutionContext(mockWorkflow, runExecutionData, additionalData, 'error'); + + expect(runExecutionData.executionData!.runtimeData!.credentials).toBeUndefined(); + }); + }); + describe('context propagation priority and edge cases', () => { it('should prefer runExecutionData.parentExecution over startItem.metadata', async () => { // Both sources provide context, runExecutionData.parentExecution should win diff --git a/packages/core/src/execution-engine/execution-context.ts b/packages/core/src/execution-engine/execution-context.ts index 76ec3f8eccf..23ce717eb7e 100644 --- a/packages/core/src/execution-engine/execution-context.ts +++ b/packages/core/src/execution-engine/execution-context.ts @@ -1,6 +1,12 @@ import { Logger } from '@n8n/backend-common'; import { Container } from '@n8n/di'; -import { type IRunExecutionData, type Workflow, type WorkflowExecuteMode } from 'n8n-workflow'; +import { + toErrorWorkflowContext, + type IExecutionContext, + type IRunExecutionData, + type Workflow, + type WorkflowExecuteMode, +} from 'n8n-workflow'; import { assertExecutionDataExists, type PreExecutionAdditionalData } from '@/utils/assertions'; @@ -27,9 +33,10 @@ import { ExecutionContextService } from './execution-context.service'; * The function follows a priority-based approach to establish execution context: * * ### 1. Preserve Existing Context (Webhook Resume) - * If `executionData.runtimeData` already exists, the function returns immediately without - * modification. This preserves context when workflows resume from database (e.g., after - * waiting for a webhook or manual continuation). + * If `executionData.runtimeData` already exists, the function returns immediately, keeping + * that context. This preserves context when workflows resume from database (e.g., after + * waiting for a webhook or manual continuation). An error run still passes its context + * through {@link contextAllowedFor}, which the branches below cannot reach. * * ### 2. Inherit from Parent Execution (Sub-workflows) * If `runExecutionData.parentExecution` exists, creates a new context by inheriting all @@ -38,12 +45,12 @@ import { ExecutionContextService } from './execution-context.service'; * - `source`: Set to current execution mode * - `parentExecutionId`: Tracks the parent execution ID * - * This applies to sub-workflows invoked via "Execute Workflow" node. + * This applies to sub-workflows invoked via "Execute Workflow" node, and to error + * workflows — `executeErrorWorkflow` populates `parentExecution` as well. * - * ### 3. Inherit from Start Node Metadata (Error Workflows) + * ### 3. Inherit from Start Node Metadata * If `startItem.metadata.parentExecution.executionContext` exists, creates a new context - * by inheriting from the parent context. This applies to error workflows that need to - * preserve the original workflow's context. + * by inheriting from the parent context. Reached only when branch 2 did not apply. * * ### 4. Create Fresh Context (New Executions) * For new root executions, creates a fresh context with: @@ -56,7 +63,8 @@ import { ExecutionContextService } from './execution-context.service'; * * ## Context Inheritance Pattern * When inheriting context, the strategy is: - * 1. Spread all parent context fields (credentials, custom fields, etc.) + * 1. Spread the inheritable parent context fields (see {@link contextAllowedFor}: + * everything for a sub-workflow, non-identity fields only for an error workflow) * 2. Override `establishedAt` with current timestamp * 3. Override `source` with current execution mode * 4. Add `parentExecutionId` to track lineage @@ -101,6 +109,35 @@ import { ExecutionContextService } from './execution-context.service'; * @see IWorkflowExecuteAdditionalData for additional execution data * @see RelatedExecution for parent execution context propagation */ +/** + * Second gate on the identity carrier reaching an error workflow. + * + * `executeErrorWorkflow` already strips the carrier when it builds the error + * payload. This refuses one even if a caller passes it, so the boundary does not + * depend on every present and future call site getting it right. Every way a + * context enters an error run passes through here: the two inheritance branches + * below (`executeErrorWorkflow` populates both `parentExecution` and the start + * item's metadata) and a context the run data already carries, which the early + * return would otherwise hand on unfiltered. + * + * The remaining source, `additionalData.encryptedRunnerIdentity`, cannot be + * filtered — it IS a carrier — so an error run must not read it at all. + */ +function contextAllowedFor( + context: IExecutionContext, + mode: WorkflowExecuteMode, +): IExecutionContext; +function contextAllowedFor( + context: IExecutionContext | undefined, + mode: WorkflowExecuteMode, +): IExecutionContext | undefined; +function contextAllowedFor( + context: IExecutionContext | undefined, + mode: WorkflowExecuteMode, +): IExecutionContext | undefined { + return mode === 'error' ? toErrorWorkflowContext(context) : context; +} + export const establishExecutionContext = async ( workflow: Workflow, runExecutionData: IRunExecutionData, @@ -123,8 +160,12 @@ export const establishExecutionContext = async ( // A retry reloads the original run's data under a NEW executionId, so its // id must join the sealed carrier's path (allowInherit) or resolution would // reject it. Resume keeps the same id, so the bind stays a no-op regardless. + // + // Filter first: an error run that already has a context (resumed from the + // database, or established by an earlier call on the main process) skips + // the inheritance branches below, so this is where its gate has to sit. executionData.runtimeData = await executionContextService.maybeBindExecutionId( - executionData.runtimeData, + contextAllowedFor(executionData.runtimeData, mode), additionalData?.executionId, { allowInherit: mode === 'retry' }, ); @@ -140,7 +181,10 @@ export const establishExecutionContext = async ( source: mode, }; - if (additionalData?.encryptedRunnerIdentity) { + // An error workflow reports a failure; it never acts as the failed run's user. + // A carrier handed in here would land on the context after the inheritance + // branches spread it, so it has to be refused at the source. + if (mode !== 'error' && additionalData?.encryptedRunnerIdentity) { executionData.runtimeData.credentials = additionalData.encryptedRunnerIdentity; if (executionData.runtimeData.credentials) { executionData.runtimeData = await executionContextService.maybeBindExecutionId( @@ -157,7 +201,7 @@ export const establishExecutionContext = async ( // and the mode in which it is running, while still retaining all other contextual information // from the parent execution. executionData.runtimeData = { - ...(runExecutionData.parentExecution.executionContext ?? {}), + ...(contextAllowedFor(runExecutionData.parentExecution.executionContext, mode) ?? {}), ...executionData.runtimeData, parentExecutionId: runExecutionData.parentExecution.executionId, }; @@ -210,7 +254,7 @@ export const establishExecutionContext = async ( // and can inherit context from there if (startItem.metadata?.parentExecution?.executionContext) { executionData.runtimeData = { - ...startItem.metadata.parentExecution.executionContext, + ...contextAllowedFor(startItem.metadata.parentExecution.executionContext, mode), ...executionData.runtimeData, parentExecutionId: startItem.metadata.parentExecution.executionId, }; @@ -225,11 +269,10 @@ export const establishExecutionContext = async ( // Re-run the global sub-execution hooks against the child workflow so its own // record reflects context derived from itself (redaction policy, private- - // credential flag), merged with the inherited parent context. An error - // workflow reaches inheritance through this branch, so without this a - // policy'd or private-credential error workflow called by a policy-less - // parent would not redact its own record. Mirrors the - // `runExecutionData.parentExecution` branch above. + // credential flag), merged with the inherited parent context. Without this a + // policy'd or private-credential child called by a policy-less parent would + // not redact its own record. Mirrors the `runExecutionData.parentExecution` + // branch above. executionData.runtimeData = await executionContextService.augmentSubExecutionContext( workflow, startItem, diff --git a/packages/frontend/@n8n/rest-api-client/src/api/communityNodes.ts b/packages/frontend/@n8n/rest-api-client/src/api/communityNodes.ts index d4013fc371f..ac7ebfc5701 100644 --- a/packages/frontend/@n8n/rest-api-client/src/api/communityNodes.ts +++ b/packages/frontend/@n8n/rest-api-client/src/api/communityNodes.ts @@ -14,10 +14,9 @@ export async function getInstalledCommunityNodes( export async function installNewPackage( context: IRestApiContext, name: string, - verify?: boolean, version?: string, ): Promise { - return await post(context.baseUrl, '/community-packages', { name, verify, version }); + return await post(context.baseUrl, '/community-packages', { name, version }); } export async function uninstallPackage(context: IRestApiContext, name: string): Promise { @@ -28,13 +27,8 @@ export async function updatePackage( context: IRestApiContext, name: string, version?: string, - checksum?: string, ): Promise { - return await makeRestApiRequest(context, 'PATCH', '/community-packages', { - name, - version, - checksum, - }); + return await makeRestApiRequest(context, 'PATCH', '/community-packages', { name, version }); } export async function getAvailableCommunityPackageCount(): Promise { diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/communityNodes.store.ts b/packages/frontend/editor-ui/src/features/settings/communityNodes/communityNodes.store.ts index 2c39a7703e6..1cd2ab5ff84 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/communityNodes.store.ts +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/communityNodes.store.ts @@ -57,17 +57,8 @@ export const useCommunityNodesStore = defineStore(STORES.COMMUNITY_NODES, () => }, timeout); }; - const installPackage = async ( - packageName: string, - verify?: boolean, - version?: string, - ): Promise => { - await communityNodesApi.installNewPackage( - rootStore.restApiContext, - packageName, - verify, - version, - ); + const installPackage = async (packageName: string, version?: string): Promise => { + await communityNodesApi.installNewPackage(rootStore.restApiContext, packageName, version); await fetchInstalledPackages(); }; @@ -85,17 +76,12 @@ export const useCommunityNodesStore = defineStore(STORES.COMMUNITY_NODES, () => installedPackages.value[newPackage.packageName] = newPackage; }; - const updatePackage = async ( - packageName: string, - version?: string, - checksum?: string, - ): Promise => { + const updatePackage = async (packageName: string, version?: string): Promise => { const packageToUpdate = installedPackages.value[packageName]; const updatedPackage = await communityNodesApi.updatePackage( rootStore.restApiContext, packageToUpdate.packageName, version, - checksum, ); updatePackageObject(updatedPackage); }; diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageManageConfirmModal.test.ts b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageManageConfirmModal.test.ts index c89d5c89b09..6d780820c76 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageManageConfirmModal.test.ts +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageManageConfirmModal.test.ts @@ -122,11 +122,7 @@ describe('CommunityPackageManageConfirmModal', () => { await flushPromises(); await fireEvent.click(getByRole('button', { name: 'Confirm update' })); - expect(communityNodesStore.updatePackage).toHaveBeenCalledWith( - 'n8n-nodes-test', - '2.0.5', - 'correct-checksum', - ); + expect(communityNodesStore.updatePackage).toHaveBeenCalledWith('n8n-nodes-test', '2.0.5'); }); it('should call nodeTypesStore methods and update latestVerifiedVersion on mount', async () => { diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageManageConfirmModal.vue b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageManageConfirmModal.vue index b62e96a6e10..df1aad5aa93 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageManageConfirmModal.vue +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/CommunityPackageManageConfirmModal.vue @@ -160,11 +160,7 @@ const onUpdate = async () => { if (settingsStore.isUnverifiedPackagesEnabled) { await communityNodesStore.updatePackage(props.activePackageName); } else if (settingsStore.isCommunityNodesFeatureEnabled) { - await communityNodesStore.updatePackage( - props.activePackageName, - updateVersion.value, - nodeTypeStorePackage.value?.checksum, - ); + await communityNodesStore.updatePackage(props.activePackageName, updateVersion.value); } else { throw new Error('Community nodes feature is not correctly enabled.'); } diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/nodeCreator/CommunityNodeDetails.test.ts b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/nodeCreator/CommunityNodeDetails.test.ts index 21804a695dd..9b69d40da97 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/components/nodeCreator/CommunityNodeDetails.test.ts +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/components/nodeCreator/CommunityNodeDetails.test.ts @@ -163,7 +163,7 @@ describe('CommunityNodeDetails', () => { await waitFor(() => expect(removeNodeFromMergedNodes).toHaveBeenCalled()); expect(getCommunityNodeAttributes).toHaveBeenCalledWith('n8n-nodes-preview-test.OtherNode'); - expect(installPackage).toHaveBeenCalledWith('n8n-nodes-test', true, '1.0.0'); + expect(installPackage).toHaveBeenCalledWith('n8n-nodes-test', '1.0.0'); expect(fetchCredentialTypes).toHaveBeenCalledWith(true); expect(getAllNodeCreateElements).toHaveBeenCalled(); expect(popViewStack).toHaveBeenCalled(); diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.test.ts b/packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.test.ts index 2eec3b60138..cf648838c00 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.test.ts +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.test.ts @@ -204,11 +204,7 @@ describe('useInstallNode', () => { }); expect(result.success).toBe(true); - expect(communityNodesStore.installPackage).toHaveBeenCalledWith( - 'test-package', - true, - '1.0.0', - ); + expect(communityNodesStore.installPackage).toHaveBeenCalledWith('test-package', '1.0.0'); }); it('should install verified node with npm version', async () => { @@ -221,11 +217,7 @@ describe('useInstallNode', () => { }); expect(result.success).toBe(true); - expect(communityNodesStore.installPackage).toHaveBeenCalledWith( - 'test-package', - true, - '1.0.0', - ); + expect(communityNodesStore.installPackage).toHaveBeenCalledWith('test-package', '1.0.0'); expect(nodeTypesStore.getNodeTypes).toHaveBeenCalled(); expect(nodeTypesStore.fetchCommunityNodePreviews).toHaveBeenCalled(); expect(credentialsStore.fetchCredentialTypes).toHaveBeenCalledWith(true); @@ -251,11 +243,7 @@ describe('useInstallNode', () => { expect(result.success).toBe(true); expect(nodeTypesStore.getCommunityNodeAttributes).toHaveBeenCalledWith('test-node'); - expect(communityNodesStore.installPackage).toHaveBeenCalledWith( - 'test-package', - true, - '1.0.0', - ); + expect(communityNodesStore.installPackage).toHaveBeenCalledWith('test-package', '1.0.0'); }); it('should install verified node as latest when unverified packages are enabled', async () => { @@ -444,11 +432,7 @@ describe('useInstallNode', () => { }); expect(result.success).toBe(true); - expect(communityNodesStore.installPackage).toHaveBeenCalledWith( - 'test-package', - true, - undefined, - ); + expect(communityNodesStore.installPackage).toHaveBeenCalledWith('test-package', undefined); }); it('should handle getNpmVersion errors', async () => { @@ -570,11 +554,7 @@ describe('useInstallNode', () => { }); expect(nodeTypesStore.getCommunityNodeAttributes).toHaveBeenCalledWith('test-node'); - expect(communityNodesStore.installPackage).toHaveBeenCalledWith( - 'test-package', - true, - '2.0.0', - ); + expect(communityNodesStore.installPackage).toHaveBeenCalledWith('test-package', '2.0.0'); }); }); diff --git a/packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.ts b/packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.ts index 0e993c8a104..c6d8cd3a45c 100644 --- a/packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.ts +++ b/packages/frontend/editor-ui/src/features/settings/communityNodes/composables/useInstallNode.ts @@ -80,7 +80,6 @@ export function useInstallNode() { if (props.type === 'verified' && !settingsStore.isUnverifiedPackagesEnabled) { await communityNodesStore.installPackage( props.packageName, - true, await getNpmVersion(props.nodeType), ); } else { diff --git a/packages/workflow/src/execution-context.ts b/packages/workflow/src/execution-context.ts index ee634451565..d4178551e7b 100644 --- a/packages/workflow/src/execution-context.ts +++ b/packages/workflow/src/execution-context.ts @@ -266,6 +266,45 @@ export const ExecutionContextSchema = z */ export type IExecutionContext = z.output; +/** + * The subset of a failed run's context that an error workflow may inherit. + * + * An error workflow reports a failure; it does not continue the failed run's + * session. So it must not act as that run's user: the failing workflow names its + * handler in `settings.errorWorkflow`, and whoever can write that setting would + * otherwise choose which workflow gets to borrow the identity. + * + * Built as an allow-list rather than by deleting known-secret keys, so a field + * added to `IExecutionContext` later has to be listed here before it can cross + * the boundary. + */ +export function toErrorWorkflowContext(context: IExecutionContext): IExecutionContext; +export function toErrorWorkflowContext( + context: IExecutionContext | undefined, +): IExecutionContext | undefined; +export function toErrorWorkflowContext( + context: IExecutionContext | undefined, +): IExecutionContext | undefined { + if (!context) return undefined; + + return { + version: context.version, + establishedAt: context.establishedAt, + source: context.source, + ...(context.triggerNode !== undefined && { triggerNode: context.triggerNode }), + ...(context.parentExecutionId !== undefined && { + parentExecutionId: context.parentExecutionId, + }), + ...(context.redaction !== undefined && { redaction: context.redaction }), + ...(context.executedByUserId !== undefined && { + executedByUserId: context.executedByUserId, + }), + ...(context.usesDynamicCredentials !== undefined && { + usesDynamicCredentials: context.usesDynamicCredentials, + }), + }; +} + /** * Metadata shape for the `n8n-oauth` credential-context source. * diff --git a/packages/workflow/test/execution-context.test.ts b/packages/workflow/test/execution-context.test.ts index 697e650263e..e45fe9518f9 100644 --- a/packages/workflow/test/execution-context.test.ts +++ b/packages/workflow/test/execution-context.test.ts @@ -1,4 +1,8 @@ -import { toExecutionContext } from '../src/execution-context'; +import { + toErrorWorkflowContext, + toExecutionContext, + type IExecutionContext, +} from '../src/execution-context'; describe('toExecutionContext — redaction snapshot', () => { const baseContext = { @@ -72,3 +76,73 @@ describe('toExecutionContext — redaction snapshot', () => { ).toThrow(); }); }); + +describe('toErrorWorkflowContext', () => { + const fullContext: IExecutionContext = { + version: 1, + establishedAt: 1234567890, + source: 'webhook', + triggerNode: { name: 'Webhook', type: 'n8n-nodes-base.webhook' }, + parentExecutionId: 'parent-1', + credentials: 'encrypted-identity-carrier', + secureArtifacts: 'encrypted-secure-artifacts', + redaction: { version: 2, production: true, manual: false }, + executedByUserId: 'user-1', + usesDynamicCredentials: true, + }; + + it('drops the identity carrier and the secure artifacts', () => { + const result = toErrorWorkflowContext(fullContext); + + expect(result).not.toHaveProperty('credentials'); + expect(result).not.toHaveProperty('secureArtifacts'); + }); + + it('keeps the fields the error execution needs for its own record', () => { + expect(toErrorWorkflowContext(fullContext)).toEqual({ + version: 1, + establishedAt: 1234567890, + source: 'webhook', + triggerNode: { name: 'Webhook', type: 'n8n-nodes-base.webhook' }, + parentExecutionId: 'parent-1', + redaction: { version: 2, production: true, manual: false }, + executedByUserId: 'user-1', + usesDynamicCredentials: true, + }); + }); + + it('carries no key that the allow-list does not name', () => { + // The point of the allow-list: a field added to IExecutionContext must be + // listed before it can reach an error workflow. A deny-list would leak it. + const withUnknownField = { + ...fullContext, + someFutureSecret: 'must-not-cross', + } as IExecutionContext; + + expect(toErrorWorkflowContext(withUnknownField)).not.toHaveProperty('someFutureSecret'); + }); + + it('omits optional fields the parent did not set, rather than setting them undefined', () => { + const result = toErrorWorkflowContext({ + version: 1, + establishedAt: 1234567890, + source: 'trigger', + }); + + expect(Object.keys(result!)).toEqual(['version', 'establishedAt', 'source']); + }); + + it('keeps `usesDynamicCredentials: false` rather than dropping it', () => { + const result = toErrorWorkflowContext({ ...fullContext, usesDynamicCredentials: false }); + + expect(result).toHaveProperty('usesDynamicCredentials', false); + }); + + it('returns undefined when there is no context', () => { + expect(toErrorWorkflowContext(undefined)).toBeUndefined(); + }); + + it('produces a context that still parses as an execution context', () => { + expect(() => toExecutionContext(toErrorWorkflowContext(fullContext)!)).not.toThrow(); + }); +});