diff --git a/packages/@n8n/api-types/src/__tests__/chat-hub-tool-types.test.ts b/packages/@n8n/api-types/src/__tests__/chat-hub-tool-types.test.ts new file mode 100644 index 00000000000..4bebc0d67df --- /dev/null +++ b/packages/@n8n/api-types/src/__tests__/chat-hub-tool-types.test.ts @@ -0,0 +1,68 @@ +import { isChatHubToolTypeAllowed } from '../chat-hub'; + +describe('isChatHubToolTypeAllowed', () => { + it.each(['@n8n/n8n-nodes-langchain.chat', '@n8n/n8n-nodes-langchain.chatHitlTool'])( + 'blocks the %s variant of the Chat tool for all users', + (type) => { + expect(isChatHubToolTypeAllowed(type, { isChatUser: false })).toBe(false); + }, + ); + + it('blocks the Data Table HITL tool for chat users', () => { + expect(isChatHubToolTypeAllowed('n8n-nodes-base.dataTableHitlTool', { isChatUser: true })).toBe( + false, + ); + }); + + it('blocks the Chat tool for all users', () => { + expect( + isChatHubToolTypeAllowed('@n8n/n8n-nodes-langchain.chatTool', { isChatUser: false }), + ).toBe(false); + expect( + isChatHubToolTypeAllowed('@n8n/n8n-nodes-langchain.chatTool', { isChatUser: true }), + ).toBe(false); + }); + + it.each([ + '@n8n/n8n-nodes-langchain.toolWorkflow', + 'n8n-nodes-base.dataTableTool', + 'n8n-nodes-base.executeCommand', + 'n8n-nodes-base.executeCommandTool', + 'n8n-nodes-base.code', + '@n8n/n8n-nodes-langchain.toolCode', + 'n8n-nodes-base.git', + 'n8n-nodes-base.gitTool', + 'n8n-nodes-base.messageAnAgentTool', + 'n8n-nodes-base.executeCommandHitlTool', + 'n8n-nodes-base.codeHitlTool', + 'n8n-nodes-base.readWriteFileHitlTool', + 'n8n-nodes-base.executeWorkflowHitlTool', + 'n8n-nodes-base.n8nHitlTool', + ])('blocks %s for chat users', (type) => { + expect(isChatHubToolTypeAllowed(type, { isChatUser: true })).toBe(false); + }); + + it.each(['n8n-nodes-base.executeCommandTool', '@n8n/n8n-nodes-langchain.toolCode'])( + 'allows %s for users who can build workflows', + (type) => { + expect(isChatHubToolTypeAllowed(type, { isChatUser: false })).toBe(true); + }, + ); + + it.each([ + 'n8n-nodes-base.slackTool', + 'n8n-nodes-base.gmailTool', + 'n8n-nodes-base.httpRequestTool', + '@n8n/n8n-nodes-langchain.toolHttpRequest', + 'n8n-nodes-base.graphqlTool', + 'n8n-nodes-base.rssFeedReadTool', + '@n8n/n8n-nodes-langchain.mcpClientTool', + 'n8n-nodes-base.sshTool', + 'n8n-nodes-base.ftpHitlTool', + 'n8n-nodes-base.jenkinsTool', + 'n8n-nodes-base.rundeckTool', + 'n8n-nodes-base.awsLambdaTool', + ])('allows %s for chat users', (type) => { + expect(isChatHubToolTypeAllowed(type, { isChatUser: true })).toBe(true); + }); +}); diff --git a/packages/@n8n/api-types/src/chat-hub.ts b/packages/@n8n/api-types/src/chat-hub.ts index c6df723c46f..95f82fcdf01 100644 --- a/packages/@n8n/api-types/src/chat-hub.ts +++ b/packages/@n8n/api-types/src/chat-hub.ts @@ -2,7 +2,10 @@ import type { Scope } from '@n8n/permissions'; import { CHAT_TOOL_NODE_TYPE, type ChunkType, + CODE_NODE_TYPE, + CODE_TOOL_LANGCHAIN_NODE_TYPE, DATA_TABLE_TOOL_NODE_TYPE, + EXECUTE_WORKFLOW_NODE_TYPE, type INode, INodeSchema, WORKFLOW_TOOL_LANGCHAIN_NODE_TYPE, @@ -709,12 +712,46 @@ export interface ChatHubToolDto { /** Tool types blocked for ALL users in the Chat Hub. */ export const ALWAYS_BLOCKED_CHAT_HUB_TOOL_TYPES: string[] = [CHAT_TOOL_NODE_TYPE]; -/** Additional tool types blocked for chat-only users (global:chatUser). */ +/** + * Additional tool types blocked for chat-only users (global:chatUser). + * These tools run code or commands on the n8n instance, or access its files, + * workflows, data tables or agents. Tools that act on a remote system through + * a credential stay allowed: the credential limits what they can do. + * Chat users cannot build workflows, so they cannot add these tools either. + * A `*Tool` or `*HitlTool` variant of a listed node type is blocked too. + * Any node can have a `*HitlTool` variant, so the list also holds node types + * that are not usable as tools. + */ export const CHAT_USER_BLOCKED_CHAT_HUB_TOOL_TYPES: string[] = [ WORKFLOW_TOOL_LANGCHAIN_NODE_TYPE, DATA_TABLE_TOOL_NODE_TYPE, + CODE_NODE_TYPE, + CODE_TOOL_LANGCHAIN_NODE_TYPE, + EXECUTE_WORKFLOW_NODE_TYPE, + 'n8n-nodes-base.executeCommand', + 'n8n-nodes-base.messageAnAgent', + 'n8n-nodes-base.git', + 'n8n-nodes-base.readWriteFile', + 'n8n-nodes-base.n8n', ]; +/** Matches the node type registry, which resolves both suffixes to the base node type. */ +function withoutToolSuffix(type: string): string { + return type.replace(/HitlTool$/, '').replace(/Tool$/, ''); +} + +/** Compares base node types, so an entry also covers the base, `*Tool` and `*HitlTool` forms. */ +function isListed(list: string[], type: string): boolean { + const baseType = withoutToolSuffix(type); + return list.some((entry) => withoutToolSuffix(entry) === baseType); +} + +/** Returns whether a user can add or run a tool of this type in the Chat Hub. */ +export function isChatHubToolTypeAllowed(type: string, user: { isChatUser: boolean }): boolean { + if (isListed(ALWAYS_BLOCKED_CHAT_HUB_TOOL_TYPES, type)) return false; + return !(user.isChatUser && isListed(CHAT_USER_BLOCKED_CHAT_HUB_TOOL_TYPES, type)); +} + /** * Request schema for creating a chat hub tool */ diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 6ef7a82275f..d7f879bebae 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -246,6 +246,11 @@ export { GetWorkflowQueryDto } from './workflows/get-workflow-query.dto'; export { UpdateWorkflowPublicDto } from './workflows/update-workflow-public.dto'; export { UpdateWorkflowQueryDto } from './workflows/update-workflow-query.dto'; export { ListWorkflowsQueryDto } from './workflows/list-workflows-query.dto'; +export { + ListCallableSubworkflowsQueryDto, + type CallableSubworkflow, + type CallableSubworkflowsResponse, +} from './workflows/list-callable-subworkflows-query.dto'; export { WorkflowPublicDto, CreatedWorkflowPublicDto, diff --git a/packages/@n8n/api-types/src/dto/workflows/__tests__/create-workflow.dto.test.ts b/packages/@n8n/api-types/src/dto/workflows/__tests__/create-workflow.dto.test.ts index 38d63ebbbd0..d8223b0bf7c 100644 --- a/packages/@n8n/api-types/src/dto/workflows/__tests__/create-workflow.dto.test.ts +++ b/packages/@n8n/api-types/src/dto/workflows/__tests__/create-workflow.dto.test.ts @@ -212,6 +212,11 @@ describe('CreateWorkflowDto', () => { request: { name: 'a'.repeat(129), nodes: [], connections: {} }, expectedErrorPath: ['name'], }, + ...['__proto__', 'constructor', 'prototype'].map((id) => ({ + name: `reserved id ${id}`, + request: { id, name: 'My Workflow', nodes: [], connections: {} }, + expectedErrorPath: ['id'], + })), { name: 'missing nodes', request: { name: 'Test', connections: {} }, diff --git a/packages/@n8n/api-types/src/dto/workflows/create-workflow.dto.ts b/packages/@n8n/api-types/src/dto/workflows/create-workflow.dto.ts index 13b7eb6b61f..3f03be46a27 100644 --- a/packages/@n8n/api-types/src/dto/workflows/create-workflow.dto.ts +++ b/packages/@n8n/api-types/src/dto/workflows/create-workflow.dto.ts @@ -1,9 +1,13 @@ import { z } from 'zod'; import { baseWorkflowShape } from './base-workflow.dto'; +import { isSafeObjectKey } from '../../schemas/instance-ai.schema'; import { Z } from '../../zod-class'; -export const workflowIdSchema = z.string(); +// Workflow ids are used as object keys, so `__proto__`, `constructor` and `prototype` are not allowed +export const workflowIdSchema = z + .string() + .refine(isSafeObjectKey, { message: 'Invalid workflow id' }); export class CreateWorkflowDto extends Z.class({ // Spread base fields (name, nodes, connections, settings, etc.) diff --git a/packages/@n8n/api-types/src/dto/workflows/list-callable-subworkflows-query.dto.ts b/packages/@n8n/api-types/src/dto/workflows/list-callable-subworkflows-query.dto.ts new file mode 100644 index 00000000000..f8eefaa0bd7 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/workflows/list-callable-subworkflows-query.dto.ts @@ -0,0 +1,28 @@ +import { z } from 'zod'; + +import { Z } from '../../zod-class'; +import { paginationSchema } from '../pagination/pagination.dto'; + +/** + * Query for the list of workflows that a parent workflow can call as + * sub-workflows. Only name search and pagination are accepted, so callers + * cannot probe workflow contents. + */ +export class ListCallableSubworkflowsQueryDto extends Z.class({ + ...paginationSchema, + query: z.string().trim().max(256).optional(), +}) {} + +export type CallableSubworkflow = { + id: string; + name: string; + active: boolean; + isArchived: boolean; + /** Set only when the workflow belongs to a team project. */ + projectName?: string; +}; + +export type CallableSubworkflowsResponse = { + count: number; + data: CallableSubworkflow[]; +}; diff --git a/packages/@n8n/api-types/src/index.ts b/packages/@n8n/api-types/src/index.ts index 9a9626aabcf..c30c3a6e3aa 100644 --- a/packages/@n8n/api-types/src/index.ts +++ b/packages/@n8n/api-types/src/index.ts @@ -96,6 +96,7 @@ export { ChatHubUpdateToolRequest, ALWAYS_BLOCKED_CHAT_HUB_TOOL_TYPES, CHAT_USER_BLOCKED_CHAT_HUB_TOOL_TYPES, + isChatHubToolTypeAllowed, chatHubVectorStoreProviderSchema, type ChatHubVectorStoreProvider, VECTOR_STORE_PROVIDER_CREDENTIAL_TYPE_MAP, diff --git a/packages/@n8n/backend-network/src/http/__tests__/connection-guards.test.ts b/packages/@n8n/backend-network/src/http/__tests__/connection-guards.test.ts index dd7fcb84b4b..84e079e425a 100644 --- a/packages/@n8n/backend-network/src/http/__tests__/connection-guards.test.ts +++ b/packages/@n8n/backend-network/src/http/__tests__/connection-guards.test.ts @@ -1,8 +1,8 @@ import type { HttpsProxyAgent } from 'https-proxy-agent'; -import type http from 'node:http'; +import http from 'node:http'; import { makeSsrfBridge } from '../../ssrf/__tests__/mock-ssrf-bridge'; -import { installProxyConnectionGuard } from '../connection-guards'; +import { installConnectionGuard, installProxyConnectionGuard } from '../connection-guards'; type ConnectOpts = Parameters['connect']>[1]; @@ -76,3 +76,78 @@ describe('installProxyConnectionGuard', () => { expect(bridge.validateConnectionHost).not.toHaveBeenCalled(); }); }); + +describe('installConnectionGuard', () => { + function guarded() { + const createConnection = vi.fn().mockReturnValue('SOCKET'); + const target = { createConnection }; + return { target, createConnection }; + } + + it('validates the target host and delegates when the policy allows it', () => { + const bridge = makeSsrfBridge(); + const { target, createConnection } = guarded(); + installConnectionGuard(target, bridge); + + expect(target.createConnection({ host: 'target.example.com' })).toBe('SOCKET'); + + expect(bridge.validateConnectionHost).toHaveBeenCalledWith('target.example.com'); + expect(createConnection).toHaveBeenCalled(); + }); + + it('rejects a connection to a socket path without validating the unused host', () => { + const bridge = makeSsrfBridge(); + const { target, createConnection } = guarded(); + installConnectionGuard(target, bridge); + + expect(() => + target.createConnection({ host: 'target.example.com', socketPath: '/var/run/docker.sock' }), + ).toThrow('Cannot open a connection to a socket path'); + + expect(createConnection).not.toHaveBeenCalled(); + expect(bridge.validateConnectionHost).not.toHaveBeenCalled(); + }); + + it('reports a socket path through the callback when the caller passes one', () => { + const bridge = makeSsrfBridge(); + const { target, createConnection } = guarded(); + installConnectionGuard(target, bridge); + const onConnect = vi.fn(); + + expect( + target.createConnection({ socketPath: '/var/run/docker.sock' }, onConnect), + ).toBeUndefined(); + + expect(onConnect).toHaveBeenCalledWith( + expect.objectContaining({ message: 'Cannot open a connection to a socket path' }), + ); + expect(createConnection).not.toHaveBeenCalled(); + }); +}); + +describe('installConnectionGuard on a real http.Agent', () => { + async function get(agent: http.Agent): Promise { + return await new Promise((resolve, reject) => { + const req = http.get('http://target.example.com/x', { agent, timeout: 3000 }, (res) => { + let data = ''; + res.on('data', (chunk) => (data += String(chunk))); + res.on('end', () => resolve(data)); + }); + req.on('error', reject); + req.on('timeout', () => { + req.destroy(); + reject(new Error('timeout')); + }); + }); + } + + it.each([ + ['ssrf active', () => makeSsrfBridge()], + ['ssrf disabled', () => 'disabled' as const], + ])('refuses a socket path Node puts in the connect options (%s)', async (_label, ssrf) => { + const agent = new http.Agent({ socketPath: '/tmp/backend-network.sock' } as http.AgentOptions); + installConnectionGuard(agent as unknown as { createConnection: never }, ssrf()); + + await expect(get(agent)).rejects.toThrow('Cannot open a connection to a socket path'); + }); +}); diff --git a/packages/@n8n/backend-network/src/http/__tests__/node-agents.test.ts b/packages/@n8n/backend-network/src/http/__tests__/node-agents.test.ts index 287e69394c2..034c29c4ad8 100644 --- a/packages/@n8n/backend-network/src/http/__tests__/node-agents.test.ts +++ b/packages/@n8n/backend-network/src/http/__tests__/node-agents.test.ts @@ -15,7 +15,7 @@ import { installConnectionGuard, UnknownConnectionHostError } from '../connectio import { EnvProxyHttpAgent } from '../env-proxy-http-agent'; import { EnvProxyHttpsAgent } from '../env-proxy-https-agent'; import { type LocalServer, startServer } from '../local-server'; -import { buildNodeAgents, type ProxyUrl } from '../node-agents'; +import { buildNodeAgents, type NodeAgentOptions, type ProxyUrl } from '../node-agents'; // HttpsProxyAgent stores `lookup` in `connectOpts` rather than `options` // (unlike http.Agent and HttpProxyAgent which use `options`). @@ -32,6 +32,10 @@ function getAgentOptions(agent: http.Agent | https.Agent): { keepAlive?: boolean return (agent as unknown as { options?: { keepAlive?: boolean } }).options ?? {}; } +function withSocketPath(socketPath: string): NodeAgentOptions { + return { socketPath } as unknown as NodeAgentOptions; +} + // --------------------------------------------------------------------------- // buildNodeAgents — shared builder (single source of truth for the undici // factory, the axios transport layer, and the global proxy agents) @@ -111,6 +115,29 @@ describe('buildNodeAgents', () => { }); }); + describe('rejects a caller-provided socketPath (not a validated connection target)', () => { + const socketPath = '/tmp/backend-network.sock'; + + it.each([ + ['ssrf disabled', 'disabled' as const], + ['ssrf active', makeSsrfBridge()], + ])('throws when agentOptions.socketPath is set (%s)', (_label, ssrf) => { + expect(() => buildNodeAgents(false, ssrf, withSocketPath(socketPath))).toThrow( + '`agentOptions.socketPath` is not supported', + ); + }); + + it.each([ + ['direct', false as const], + ['env proxy', 'env' as const], + ['explicit proxy', 'http://proxy.internal:3128' as ProxyUrl], + ])('throws on the %s path too', (_label, proxy) => { + expect(() => buildNodeAgents(proxy, makeSsrfBridge(), withSocketPath(socketPath))).toThrow( + '`agentOptions.socketPath` is not supported', + ); + }); + }); + describe('SSRF lookup placement', () => { const modes: Array<[string, false | 'env' | ProxyUrl]> = [ ['proxy: false', false], diff --git a/packages/@n8n/backend-network/src/http/connection-guards.ts b/packages/@n8n/backend-network/src/http/connection-guards.ts index 169634890bc..27fcaf4aea6 100644 --- a/packages/@n8n/backend-network/src/http/connection-guards.ts +++ b/packages/@n8n/backend-network/src/http/connection-guards.ts @@ -14,8 +14,18 @@ export class UnknownConnectionHostError extends UnexpectedError { } } +export class SocketPathConnectionError extends UnexpectedError { + constructor() { + super('Cannot open a connection to a socket path'); + } +} + /** Subset of an agent's connection options we read to find the target host. */ -type ConnectionOptions = { host?: string | null; hostname?: string | null }; +type ConnectionOptions = { + host?: string | null; + hostname?: string | null; + socketPath?: string; +}; /** * Runtime-only `createConnection` method of Node's http(s) agents. @@ -59,6 +69,16 @@ export function installProxyConnectionGuard(agent: ProxyConnectionAgent, ssrf: S }; } +function connectionError( + ssrf: SsrfBridge | 'disabled', + options: ConnectionOptions, +): Error | undefined { + if (options.socketPath) { + return new SocketPathConnectionError(); + } + return ssrf === 'disabled' ? undefined : connectionHostError(ssrf, options); +} + /** * Wraps an agent's `createConnection` to validate the connection target before the socket opens. * @@ -68,11 +88,11 @@ export function installProxyConnectionGuard(agent: ProxyConnectionAgent, ssrf: S */ export function installConnectionGuard( target: { createConnection: CreateConnection }, - ssrf: SsrfBridge, + ssrf: SsrfBridge | 'disabled', ): void { const createConnection = target.createConnection.bind(target); target.createConnection = (options, onConnect) => { - const error = connectionHostError(ssrf, options); + const error = connectionError(ssrf, options); if (error) { if (onConnect) { onConnect(error); diff --git a/packages/@n8n/backend-network/src/http/node-agents.ts b/packages/@n8n/backend-network/src/http/node-agents.ts index 632b5ca790a..8852c3bac1b 100644 --- a/packages/@n8n/backend-network/src/http/node-agents.ts +++ b/packages/@n8n/backend-network/src/http/node-agents.ts @@ -1,4 +1,4 @@ -import { UnexpectedError } from 'n8n-workflow'; +import { UserError } from 'n8n-workflow'; import http from 'node:http'; import https from 'node:https'; import type { LookupFunction } from 'node:net'; @@ -81,6 +81,10 @@ export type NodeAgentOptions = https.AgentOptions; * therefore has no effect and is rejected to avoid a false sense of control over * DNS resolution. * + * `agentOptions.socketPath` is rejected for a related reason: it replaces the + * connection target, leaving the request's host and port unused, so nothing the + * policy decides applies to the socket that opens. + * * `agentOptions` describes the connection to the **target**. Behind a proxy its TLS * options travel with the tunnelled session rather than the proxy handshake * (see `proxy/proxied-agents.ts`). @@ -91,11 +95,17 @@ export function buildNodeAgents( agentOptions?: NodeAgentOptions, ): { httpAgent: http.Agent; httpsAgent: https.Agent } { if (agentOptions?.lookup) { - throw new UnexpectedError( + throw new UserError( '`agentOptions.lookup` is not supported: DNS resolution is managed by the SSRF policy. Remove it from `agentOptions`.', ); } + if (agentOptions !== undefined && 'socketPath' in agentOptions && agentOptions.socketPath) { + throw new UserError( + '`agentOptions.socketPath` is not supported. Remove it from `agentOptions`.', + ); + } + const lookup: LookupFunction | undefined = ssrf !== 'disabled' ? ssrf.createSecureLookup() : undefined; @@ -132,15 +142,16 @@ export function buildNodeAgents( } /** - * Installs {@link installConnectionGuard} on a direct-path agent pair when SSRF protection is active. + * Installs {@link installConnectionGuard} on a direct-path agent pair. + * + * It goes on whatever the SSRF setting, because it also refuses a socket path, which + * is not an SSRF decision. */ function applyConnectionGuard( agents: { httpAgent: http.Agent; httpsAgent: https.Agent }, ssrf: SsrfOption, ): { httpAgent: http.Agent; httpsAgent: https.Agent } { - if (ssrf !== 'disabled') { - installConnectionGuard(agents.httpAgent, ssrf); - installConnectionGuard(agents.httpsAgent, ssrf); - } + installConnectionGuard(agents.httpAgent, ssrf); + installConnectionGuard(agents.httpsAgent, ssrf); return agents; } diff --git a/packages/@n8n/backend-services/src/services/__tests__/workflow-project.service.test.ts b/packages/@n8n/backend-services/src/services/__tests__/workflow-project.service.test.ts index 9606bdb17f2..29236e09882 100644 --- a/packages/@n8n/backend-services/src/services/__tests__/workflow-project.service.test.ts +++ b/packages/@n8n/backend-services/src/services/__tests__/workflow-project.service.test.ts @@ -35,14 +35,17 @@ describe('WorkflowProjectService', () => { }); }); - it('loads the owner project when the cached value is malformed', async () => { - const project = { id: 'project-1' } as Project; - cacheService.getHashValue.mockResolvedValue('invalid' as never); - sharedWorkflowRepository.findOneOrFail.mockResolvedValue({ project } as never); + it.each(['invalid', {}])( + 'loads the owner project when the cached value is malformed', + async (cached) => { + const project = { id: 'project-1' } as Project; + cacheService.getHashValue.mockResolvedValue(cached as never); + sharedWorkflowRepository.findOneOrFail.mockResolvedValue({ project } as never); - await expect(service.getWorkflowProjectCached('workflow-1')).resolves.toBe(project); - expect(sharedWorkflowRepository.findOneOrFail).toHaveBeenCalledOnce(); - }); + await expect(service.getWorkflowProjectCached('workflow-1')).resolves.toBe(project); + expect(sharedWorkflowRepository.findOneOrFail).toHaveBeenCalledOnce(); + }, + ); it('sets the cached owner project', async () => { const project = { id: 'project-1' } as Project; diff --git a/packages/@n8n/backend-services/src/services/cache/__tests__/cache.service.test.ts b/packages/@n8n/backend-services/src/services/cache/__tests__/cache.service.test.ts index 6b07ee11a99..1394c322d77 100644 --- a/packages/@n8n/backend-services/src/services/cache/__tests__/cache.service.test.ts +++ b/packages/@n8n/backend-services/src/services/cache/__tests__/cache.service.test.ts @@ -338,6 +338,27 @@ for (const backend of ['memory', 'redis'] as const) { await expect(cacheService.getHashValue('key', 'field1')).resolves.toBe('value1'); }); + + if (backend === 'memory') { + test('should not return inherited properties as hash field values', async () => { + await cacheService.setHash('key', { field1: { id: 'value1' } }); + + await expect(cacheService.getHashValue('key', '__proto__')).resolves.toBeUndefined(); + await expect(cacheService.getHashValue('key', 'constructor')).resolves.toBeUndefined(); + }); + + test('should not store a `__proto__` hash field', async () => { + await cacheService.setHash('key', { field1: { id: 'value1' } }); + // A computed key creates an own `__proto__` field, as a hash keyed by a workflow id does + await cacheService.setHash('key', { ['__proto__']: { id: 'value2' } }); + + expect(Object.getPrototypeOf(await cacheService.get('key'))).toBe(Object.prototype); + await expect(cacheService.getHashValue('key', '__proto__')).resolves.toBeUndefined(); + await expect(cacheService.getHashValue('key', 'field1')).resolves.toEqual({ + id: 'value1', + }); + }); + } }); }); } diff --git a/packages/@n8n/backend-services/src/services/cache/cache.service.ts b/packages/@n8n/backend-services/src/services/cache/cache.service.ts index f562eed9edf..c4c195a227a 100644 --- a/packages/@n8n/backend-services/src/services/cache/cache.service.ts +++ b/packages/@n8n/backend-services/src/services/cache/cache.service.ts @@ -151,7 +151,15 @@ export class CacheService extends TypedEmitter { const hashObject: Hash = (await this.get(key)) ?? {}; - Object.assign(hashObject, hash); + for (const [field, value] of Object.entries(hash)) { + // Skip `__proto__`. On a plain object, `hash['__proto__'] = value` does not add a field. + // It changes the object's prototype. + if (field === '__proto__') { + continue; + } + + hashObject[field] = value; + } await this.set(key, hashObject); } @@ -309,7 +317,10 @@ export class CacheService extends TypedEmitter { } else { const hashObject = await this.cache.store.get>(cacheKey); - hashValue = hashObject?.[hashKey] as MaybeHash; + hashValue = + hashObject && Object.hasOwn(hashObject, hashKey) + ? (hashObject[hashKey] as MaybeHash) + : undefined; } if (hashValue !== undefined) { diff --git a/packages/@n8n/backend-services/src/services/workflow-project.service.ts b/packages/@n8n/backend-services/src/services/workflow-project.service.ts index f6a9eaa46dc..b1f52e53de5 100644 --- a/packages/@n8n/backend-services/src/services/workflow-project.service.ts +++ b/packages/@n8n/backend-services/src/services/workflow-project.service.ts @@ -10,7 +10,9 @@ export class WorkflowProjectService { ) {} private reconstructProject(project: Partial): Project | undefined { - if (typeof project !== 'object' || project === null) return undefined; + if (typeof project !== 'object' || project === null || typeof project.id !== 'string') { + return undefined; + } return Object.assign(new Project(), project); } diff --git a/packages/@n8n/db/src/repositories/__tests__/workflow.repository.test.ts b/packages/@n8n/db/src/repositories/__tests__/workflow.repository.test.ts index 2db05a45e69..ad91175d441 100644 --- a/packages/@n8n/db/src/repositories/__tests__/workflow.repository.test.ts +++ b/packages/@n8n/db/src/repositories/__tests__/workflow.repository.test.ts @@ -1,5 +1,6 @@ import { GlobalConfig } from '@n8n/config'; import { In, IsNull, MoreThan, Not, type SelectQueryBuilder } from '@n8n/typeorm'; +import type { IWorkflowSettings } from 'n8n-workflow'; import type { Mock, Mocked } from 'vitest'; import { mock } from 'vitest-mock-extended'; @@ -840,4 +841,270 @@ describe('WorkflowRepository', () => { expect(mockExecute).toHaveBeenCalled(); }); }); + + describe('caller allowlist purge', () => { + let writes: Array<{ id: string; callerIds: string }>; + let writtenSettings: Array; + let selectQb: { where: Mock; andWhere: Mock; setLock: Mock }; + + const stubCandidates = (candidates: Array>) => { + writes = []; + writtenSettings = []; + + const select = { + select: vi.fn(), + where: vi.fn(), + andWhere: vi.fn(), + setLock: vi.fn(), + getMany: vi.fn().mockResolvedValue(candidates), + }; + select.select.mockReturnValue(select); + select.where.mockReturnValue(select); + select.andWhere.mockReturnValue(select); + select.setLock.mockReturnValue(select); + + let pendingId = ''; + let pendingCallerIds = ''; + let pendingSettings: IWorkflowSettings | undefined; + const update = { + update: vi.fn(), + set: vi.fn(), + where: vi.fn(), + setParameter: vi.fn(), + execute: vi.fn(async () => { + writes.push({ id: pendingId, callerIds: pendingCallerIds }); + writtenSettings.push(pendingSettings); + }), + }; + update.update.mockReturnValue(update); + // Postgres writes the whole document, SQLite the one key through a parameter. + update.set.mockImplementation((values: { settings: unknown }) => { + if (typeof values.settings === 'object' && values.settings !== null) { + pendingSettings = values.settings as IWorkflowSettings; + pendingCallerIds = pendingSettings.callerIds ?? ''; + } + return update; + }); + update.where.mockImplementation((_sql: string, parameters: { workflowId: string }) => { + pendingId = parameters.workflowId; + return update; + }); + update.setParameter.mockImplementation((_name: string, value: string) => { + pendingCallerIds = value; + return update; + }); + + vi.mocked(entityManager.createQueryBuilder).mockImplementation(((...args: unknown[]) => + args.length > 0 ? select : update) as never); + + transactionRunner.run.mockImplementation( + async (ctx: unknown, fn: (c: unknown) => Promise) => await fn(ctx), + ); + + selectQb = select; + return select; + }; + + it('removes the entry and keeps the rest in order, whatever whitespace separates them', async () => { + stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'first,\n second , ,\tthird' }, + }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['second'], {}); + + expect(writes).toEqual([{ id: 'subworkflow-1', callerIds: 'first,third' }]); + }); + + it('rewrites only the allowlists that hold the id as an entry of its own', async () => { + stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'abc,abcdef' }, + }, + { + id: 'subworkflow-2', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'abcdef' }, + }, + { id: 'subworkflow-3', settings: { callerPolicy: 'workflowsFromAList' } }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['abc'], {}); + + expect(writes).toEqual([{ id: 'subworkflow-1', callerIds: 'abcdef' }]); + }); + + it('leaves an allowlist alone when the id only appears inside another entry', async () => { + stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'abc-1, abc-2' }, + }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['abc'], {}); + + expect(writes).toEqual([]); + }); + + it('rewrites every workflow that names the id', async () => { + stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'first,second' }, + }, + { + id: 'subworkflow-2', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'second,third' }, + }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['second'], {}); + + expect(writes).toEqual([ + { id: 'subworkflow-1', callerIds: 'first' }, + { id: 'subworkflow-2', callerIds: 'third' }, + ]); + }); + + it('takes every id out of one allowlist in a single write', async () => { + stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'first,second,third' }, + }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['first', 'third'], {}); + + expect(writes).toEqual([{ id: 'subworkflow-1', callerIds: 'second' }]); + }); + + it('reads the candidates once, whatever the batch size, and locks only the ones it rewrites', async () => { + const select = stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'one,first' }, + }, + { + id: 'subworkflow-2', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'first' }, + }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['one', 'two'], {}); + + expect(select.where).toHaveBeenCalledTimes(2); + expect(select.where).toHaveBeenNthCalledWith(1, expect.stringContaining('callerIds')); + expect(select.where).toHaveBeenNthCalledWith(2, 'workflow.id IN (:...ids)', { + ids: ['subworkflow-1'], + }); + expect(select.setLock).toHaveBeenCalledTimes(1); + expect(select.setLock).toHaveBeenCalledWith('pessimistic_write'); + expect(writes).toEqual([{ id: 'subworkflow-1', callerIds: 'first' }]); + }); + + it('locks nothing when no allowlist names a removed id', async () => { + const select = stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'first' }, + }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['one'], {}); + + expect(select.setLock).not.toHaveBeenCalled(); + expect(writes).toEqual([]); + }); + + it('matches candidates on the stored document, never on a value read out of it', async () => { + const select = stubCandidates([]); + + await workflowRepository.removeFromCallerAllowlists(['one'], {}); + + expect(select.where).toHaveBeenCalledWith('workflow.settings::text LIKE \'%"callerIds"%\''); + }); + + it('keeps a value the binary JSON form has no room for', async () => { + const noBinaryJsonForm = String.fromCharCode(0); + stubCandidates([ + { + id: 'subworkflow-1', + settings: { + callerPolicy: 'workflowsFromAList', + callerIds: 'first,second', + errorWorkflow: `error-workflow${noBinaryJsonForm}`, + }, + }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['second'], {}); + + expect(writtenSettings).toEqual([ + { + callerPolicy: 'workflowsFromAList', + callerIds: 'first', + errorWorkflow: `error-workflow${noBinaryJsonForm}`, + }, + ]); + }); + + it('purges a row that exists at an id being removed', async () => { + stubCandidates([ + { + id: 'recreated-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'first,second' }, + }, + ]); + + await workflowRepository.removeFromCallerAllowlists(['recreated-1', 'second'], {}); + + expect(writes).toEqual([{ id: 'recreated-1', callerIds: 'first' }]); + }); + + it('writes nothing when given no ids', async () => { + stubCandidates([]); + + await workflowRepository.removeFromCallerAllowlists([], {}); + + expect(selectQb.where).not.toHaveBeenCalled(); + expect(writes).toEqual([]); + }); + + it('removes only the entries naming an id without a row', async () => { + stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'deleted,survivor,other' }, + }, + ]); + vi.mocked(entityManager.find).mockResolvedValue([{ id: 'survivor' }, { id: 'other' }]); + + await workflowRepository.removeMissingFromCallerAllowlists({}); + + expect(entityManager.find).toHaveBeenCalledWith(WorkflowEntity, { + select: ['id'], + where: { id: In(['deleted', 'survivor', 'other']) }, + }); + expect(writes).toEqual([{ id: 'subworkflow-1', callerIds: 'survivor,other' }]); + }); + + it('locks and writes nothing when every named id has a row', async () => { + const select = stubCandidates([ + { + id: 'subworkflow-1', + settings: { callerPolicy: 'workflowsFromAList', callerIds: 'first,second' }, + }, + ]); + vi.mocked(entityManager.find).mockResolvedValue([{ id: 'first' }, { id: 'second' }]); + + await workflowRepository.removeMissingFromCallerAllowlists({}); + + expect(select.setLock).not.toHaveBeenCalled(); + expect(writes).toEqual([]); + }); + }); }); diff --git a/packages/@n8n/db/src/repositories/workflow.repository.ts b/packages/@n8n/db/src/repositories/workflow.repository.ts index 1155e2551e5..3bb7f29d9bd 100644 --- a/packages/@n8n/db/src/repositories/workflow.repository.ts +++ b/packages/@n8n/db/src/repositories/workflow.repository.ts @@ -454,6 +454,136 @@ export class WorkflowRepository extends BaseRepository { } } + /** Remove every id in `workflowIds` from the `callerIds` allowlist of every other workflow. */ + async removeFromCallerAllowlists(workflowIds: string[], ctx: OperationContext): Promise { + if (workflowIds.length === 0) return; + + await this.runInTransaction(ctx, async (tx) => { + const candidates = await this.findCallerAllowlistCandidates(tx); + await this.purgeCallerAllowlists(candidates, new Set(workflowIds), tx); + }); + } + + /** Remove every id that has no workflow row from every `callerIds` allowlist. */ + async removeMissingFromCallerAllowlists(ctx: OperationContext): Promise { + await this.runInTransaction(ctx, async (tx) => { + const candidates = await this.findCallerAllowlistCandidates(tx); + const namedIds = [ + ...new Set(candidates.flatMap((workflow) => this.callerIdEntries(workflow))), + ]; + + const existingIds = new Set(); + for (const chunk of chunkIds(namedIds)) { + const existing = await tx.find(WorkflowEntity, { + select: ['id'], + where: { id: In(chunk) }, + }); + for (const { id } of existing) existingIds.add(id); + } + + const missingIds = new Set(namedIds.filter((id) => !existingIds.has(id))); + if (missingIds.size > 0) await this.purgeCallerAllowlists(candidates, missingIds, tx); + }); + } + + /** Rewrite the allowlists among `candidates` that name any of `removedIds`, under a row lock. */ + private async purgeCallerAllowlists( + candidates: WorkflowEntity[], + removedIds: Set, + tx: EntityManager, + ): Promise { + const affected = candidates.filter( + (workflow) => this.callerIdsWithout(workflow, removedIds) !== undefined, + ); + + for (const workflow of await this.lockCallerAllowlists(affected, tx)) { + const remaining = this.callerIdsWithout(workflow, removedIds); + if (remaining !== undefined) await this.writeCallerIds(workflow, remaining, tx); + } + } + + /** The non-empty, trimmed entries of the `callerIds` allowlist of `workflow`. */ + private callerIdEntries(workflow: WorkflowEntity): string[] { + const callerIds = workflow.settings?.callerIds; + if (typeof callerIds !== 'string') return []; + + return callerIds + .split(',') + .map((callerId) => callerId.trim()) + .filter((callerId) => callerId !== ''); + } + + /** The allowlist of `workflow` without `removedIds`, or `undefined` when it names none of them. */ + private callerIdsWithout(workflow: WorkflowEntity, removedIds: Set): string | undefined { + const entries = this.callerIdEntries(workflow); + if (!entries.some((callerId) => removedIds.has(callerId))) return undefined; + + return entries.filter((callerId) => !removedIds.has(callerId)).join(','); + } + + /** Find every workflow that has a `callerIds` allowlist. */ + private async findCallerAllowlistCandidates(tx: EntityManager): Promise { + // On Postgres, match the raw text rather than extract: the `json` column accepts characters, + // such as an escaped NUL, that extracting `callerIds` rejects with `unsupported Unicode + // escape sequence`, which fails the query for every row. + const hasAllowlist = + this.globalConfig.database.type === 'postgresdb' + ? 'workflow.settings::text LIKE \'%"callerIds"%\'' + : `${this.settingsTextValue('workflow.settings', 'callerIds')} IS NOT NULL`; + + return await tx + .createQueryBuilder(WorkflowEntity, 'workflow') + .select(['workflow.id', 'workflow.settings']) + .where(hasAllowlist) + .getMany(); + } + + /** Lock `workflows` until the transaction ends and re-read their settings under the lock. */ + private async lockCallerAllowlists( + workflows: WorkflowEntity[], + tx: EntityManager, + ): Promise { + // SQLite serialises writes, so what it read is already current. + if (this.globalConfig.database.type !== 'postgresdb') return workflows; + + const locked: WorkflowEntity[] = []; + for (const chunk of chunkIds(workflows.map(({ id }) => id))) { + locked.push( + ...(await tx + .createQueryBuilder(WorkflowEntity, 'workflow') + .select(['workflow.id', 'workflow.settings']) + .where('workflow.id IN (:...ids)', { ids: chunk }) + .setLock('pessimistic_write') + .getMany()), + ); + } + return locked; + } + + /** Store `callerIds` back on one workflow, keeping its other settings. */ + private async writeCallerIds( + workflow: WorkflowEntity, + callerIds: string, + tx: EntityManager, + ): Promise { + const update = tx + .createQueryBuilder() + .update(WorkflowEntity) + .where('id = :workflowId', { workflowId: workflow.id }); + + if (this.globalConfig.database.type === 'postgresdb') { + // `jsonb_set` rejects characters the `json` column accepts, so rewrite the whole document + // read under the row lock. + await update.set({ settings: { ...workflow.settings, callerIds } }).execute(); + return; + } + + await update + .set({ settings: () => "JSON_SET(COALESCE(settings, '{}'), '$.callerIds', :callerIds)" }) + .setParameter('callerIds', callerIds) + .execute(); + } + async findById(workflowId: string) { return await this.findOne({ where: { id: workflowId }, @@ -864,7 +994,6 @@ export class WorkflowRepository extends BaseRepository { onlySharedWithMe?: boolean; }, options: WorkflowListQueryOptions = {}, - callableForParentWorkflowId?: string, ) { if ( options.filter?.parentFolderId && @@ -884,18 +1013,8 @@ export class WorkflowRepository extends BaseRepository { } const [workflowsAndFolders, count] = await Promise.all([ - this.getWorkflowsAndFoldersUnionWithSharingSubquery( - user, - sharingOptions, - options, - callableForParentWorkflowId, - ), - this.getWorkflowsAndFoldersCountWithSharingSubquery( - user, - sharingOptions, - options, - callableForParentWorkflowId, - ), + this.getWorkflowsAndFoldersUnionWithSharingSubquery(user, sharingOptions, options), + this.getWorkflowsAndFoldersCountWithSharingSubquery(user, sharingOptions, options), ]); const isArchived = @@ -922,15 +1041,9 @@ export class WorkflowRepository extends BaseRepository { onlySharedWithMe?: boolean; }, options: WorkflowListQueryOptions = {}, - callableForParentWorkflowId?: string, ) { const { baseQuery, sortByColumn, sortByDirection } = - this.buildBaseUnionQueryWithSharingSubquery( - user, - sharingOptions, - options, - callableForParentWorkflowId, - ); + this.buildBaseUnionQueryWithSharingSubquery(user, sharingOptions, options); const query = this.buildUnionQuery(baseQuery, { sortByColumn, @@ -956,7 +1069,6 @@ export class WorkflowRepository extends BaseRepository { onlySharedWithMe?: boolean; }, options: WorkflowListQueryOptions = {}, - callableForParentWorkflowId?: string, ) { const { skip, take, ...baseQueryParameters } = options; @@ -964,7 +1076,6 @@ export class WorkflowRepository extends BaseRepository { user, sharingOptions, baseQueryParameters, - callableForParentWorkflowId, ); const response = await baseQuery @@ -986,7 +1097,6 @@ export class WorkflowRepository extends BaseRepository { onlySharedWithMe?: boolean; }, options: WorkflowListQueryOptions = {}, - callableForParentWorkflowId?: string, ) { // Common fields for both folders and workflows const commonFields = { @@ -1036,7 +1146,6 @@ export class WorkflowRepository extends BaseRepository { user, sharingOptions, workflowQueryParameters, - callableForParentWorkflowId, ).addSelect("'workflow'", 'resource'); const qb = this.manager.createQueryBuilder(); @@ -1155,14 +1264,8 @@ export class WorkflowRepository extends BaseRepository { onlySharedWithMe?: boolean; }, options: WorkflowListQueryOptions = {}, - callableForParentWorkflowId?: string, ): Promise { - const query = this.getManyQueryWithSharingSubquery( - user, - sharingOptions, - options, - callableForParentWorkflowId, - ); + const query = this.getManyQueryWithSharingSubquery(user, sharingOptions, options); const [workflows, count] = await query.getManyAndCount(); return { workflows, count }; @@ -1182,7 +1285,6 @@ export class WorkflowRepository extends BaseRepository { onlySharedWithMe?: boolean; }, options: WorkflowListQueryOptions = {}, - callableForParentWorkflowId?: string, ): SelectQueryBuilder { const qb = this.createQueryBuilder('workflow'); @@ -1200,21 +1302,9 @@ export class WorkflowRepository extends BaseRepository { sharingOptionsWithProjectId, ); - if (callableForParentWorkflowId) { - // Union: workflows readable by the user OR callable by the parent workflow. - const callableSubquery = this.buildCallablePolicySubquery(callableForParentWorkflowId); - qb.andWhere( - `(workflow.id IN (${sharedWorkflowSubquery.getQuery()}) OR workflow.id IN (${callableSubquery.getQuery()}))`, - ); - qb.setParameters({ - ...sharedWorkflowSubquery.getParameters(), - ...callableSubquery.getParameters(), - }); - } else { - // Apply the sharing filter using the subquery - qb.andWhere(`workflow.id IN (${sharedWorkflowSubquery.getQuery()})`); - qb.setParameters(sharedWorkflowSubquery.getParameters()); - } + // Apply the sharing filter using the subquery + qb.andWhere(`workflow.id IN (${sharedWorkflowSubquery.getQuery()})`); + qb.setParameters(sharedWorkflowSubquery.getParameters()); // Apply other filters // For personal project and shared-with-me cases, projectId is already handled in the subquery @@ -1238,70 +1328,98 @@ export class WorkflowRepository extends BaseRepository { } /** - * Build a subquery returning IDs of workflows whose callerPolicy permits the - * given `parentWorkflowId` to call them. + * List the workflows that `parentWorkflowId` can call as sub-workflows. The + * list holds the workflows the user can read, plus the workflows whose + * caller policy names the parent explicitly (`any`, or an allowlist that + * contains the parent id). + * + * Only a minimal set of columns is loaded, because the callable rows can + * belong to projects that the user has no access to. + */ + async findCallableSubworkflows( + user: User, + sharingOptions: { scopes: Scope[]; projectRoles: string[]; workflowRoles: string[] }, + parentWorkflowId: string, + options: { query?: string; triggerNodeTypes: string[]; skip: number; take: number }, + ): Promise<{ workflows: WorkflowEntity[]; count: number }> { + const sharedWorkflowSubquery = this.buildSharedWorkflowIdsSubquery(user, sharingOptions); + const callableSubquery = this.buildCallablePolicySubquery(parentWorkflowId); + + const qb = this.createQueryBuilder('workflow') + .select([ + 'workflow.id', + 'workflow.name', + 'workflow.activeVersionId', + 'workflow.isArchived', + 'workflow.updatedAt', + ]) + .leftJoin('workflow.shared', 'owner', 'owner.role = :callableOwnerRole', { + callableOwnerRole: 'workflow:owner', + }) + .leftJoin('owner.project', 'ownerProject') + .addSelect([ + 'owner.workflowId', + 'owner.projectId', + 'owner.role', + 'ownerProject.id', + 'ownerProject.type', + 'ownerProject.name', + ]) + .where( + `(workflow.id IN (${sharedWorkflowSubquery.getQuery()}) OR workflow.id IN (${callableSubquery.getQuery()}))`, + ) + .setParameters({ + ...sharedWorkflowSubquery.getParameters(), + ...callableSubquery.getParameters(), + }); + + // Match names only: the list can hold workflows the user cannot read, so + // their descriptions must not affect the result. + const searchWords = this.parseSearchWords(options.query); + if (searchWords.length > 0) { + const { conditions, parameters } = this.buildSearchConditions( + searchWords, + 'LOWER(workflow.name)', + ); + qb.andWhere(`(${conditions.join(' AND ')})`, parameters); + } + this.applyTriggerNodeTypesFilter(qb, options.triggerNodeTypes); + qb.orderBy('workflow.updatedAt', 'DESC').skip(options.skip).take(options.take); + + const [workflows, count] = await qb.getManyAndCount(); + return { workflows, count }; + } + + /** + * Build a subquery returning IDs of workflows whose callerPolicy names the + * given `parentWorkflowId` explicitly. + * + * The `workflowsFromSameOwner` policy is left out on purpose: for a user who + * is not the owner, it would list workflows that the owner did not share. + * The owner can already read those workflows through the sharing subquery. */ // eslint-disable-next-line @typescript-eslint/no-explicit-any private buildCallablePolicySubquery(parentWorkflowId: string): SelectQueryBuilder { - const subquery = this.manager - .createQueryBuilder() - .select('cpw.id') - .from(WorkflowEntity, 'cpw') - .leftJoin( - SharedWorkflow, - 'sw_sub', - "sw_sub.workflowId = cpw.id AND sw_sub.role = 'workflow:owner'", - ) - .leftJoin( - SharedWorkflow, - 'sw_par', - "sw_par.workflowId = :cpParentWorkflowId AND sw_par.role = 'workflow:owner'", - ); - - const { conditions, params } = this.buildCallablePolicyConditions(parentWorkflowId); - - subquery.where('(' + conditions.join(' OR ') + ')', params); - - return subquery; - } - - private buildCallablePolicyConditions(parentWorkflowId: string): { - conditions: string[]; - params: Record; - } { const defaultPolicy = this.globalConfig.workflows.callerPolicyDefaultOption; const callerPolicy = this.settingsTextValue('cpw.settings', 'callerPolicy'); const callerIds = this.settingsTextValue('cpw.settings', 'callerIds'); - const conditions: string[] = []; - const params: Record = { - cpParentWorkflowId: parentWorkflowId, - cpCallerIdMembership: `%,${escapeLike(parentWorkflowId)},%`, - }; - - // Branch 1: callerPolicy = 'any' - conditions.push(`${callerPolicy} = 'any'`); - - // Branch 2: callerPolicy = 'workflowsFromAList' and the allowlist contains parentWorkflowId as a whole ID. - conditions.push( + const conditions = [ + `${callerPolicy} = 'any'`, + // The allowlist must contain parentWorkflowId as a whole ID. `(${callerPolicy} = 'workflowsFromAList' AND (',' || REPLACE(${callerIds}, ' ', '') || ',') LIKE :cpCallerIdMembership ${LIKE_ESCAPE_CLAUSE})`, - ); - - // Branch 3: callerPolicy = 'workflowsFromSameOwner' (or NULL when default is 'workflowsFromSameOwner'). - const sameOwnerPolicyClauses = [`${callerPolicy} = 'workflowsFromSameOwner'`]; - if (defaultPolicy === 'workflowsFromSameOwner') { - sameOwnerPolicyClauses.push(`${callerPolicy} IS NULL`); - } - conditions.push( - `((${sameOwnerPolicyClauses.join(' OR ')}) AND sw_sub.projectId = sw_par.projectId AND sw_par.projectId IS NOT NULL)`, - ); - - // Handle NULL callerPolicy when default is 'any' + ]; if (defaultPolicy === 'any') { conditions.push(`${callerPolicy} IS NULL`); } - return { conditions, params }; + return this.manager + .createQueryBuilder() + .select('cpw.id') + .from(WorkflowEntity, 'cpw') + .where(`(${conditions.join(' OR ')})`, { + cpCallerIdMembership: `%,${escapeLike(parentWorkflowId)},%`, + }); } private settingsTextValue(field: string, key: string): string { @@ -1467,14 +1585,15 @@ export class WorkflowRepository extends BaseRepository { /** * Builds search conditions and parameters for matching all of the search words */ - private buildSearchConditions(searchWords: string[]): { + private buildSearchConditions( + searchWords: string[], + fieldExpression = this.getFieldConcatExpression(), + ): { conditions: string[]; parameters: Record; } { - const concatExpression = this.getFieldConcatExpression(); - const conditions = searchWords.map((_, index) => { - return `${concatExpression} LIKE :searchWord${index}`; + return `${fieldExpression} LIKE :searchWord${index}`; }); const parameters: Record = {}; diff --git a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/ChatTrigger.node.ts b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/ChatTrigger.node.ts index 331c585d79a..2c07a7253b7 100644 --- a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/ChatTrigger.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/ChatTrigger.node.ts @@ -1098,6 +1098,10 @@ export class ChatTrigger extends Node { // By header as well as by the iframe's attribute, so the document has no // origin even if the attribute is ever stripped. res.setHeader('Content-Security-Policy', `sandbox ${CHAT_FRAME_SANDBOX}`); + } else { + // The webhook route sandboxes every response. This page keeps its origin, + // because the widget keeps the session in `localStorage`. + res.removeHeader('Content-Security-Policy'); } const page = createPage({ diff --git a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/ChatTrigger.node.test.ts b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/ChatTrigger.node.test.ts index 51f1ed98758..6ee3e008814 100644 --- a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/ChatTrigger.node.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/ChatTrigger.node.test.ts @@ -984,6 +984,17 @@ describe('ChatTrigger Node', () => { expect(renderedView().sandbox).not.toContain('allow-popups-to-escape-sandbox'); }); + it('removes the route sandbox from the public chat page', async () => { + await renderSetupPage('none'); + + expect(mockResponse.removeHeader).toHaveBeenCalledWith('Content-Security-Policy'); + expect(mockResponse.setHeader).not.toHaveBeenCalledWith( + 'Content-Security-Policy', + expect.anything(), + ); + expect(renderedPage()).toContain('createChat'); + }); + it('renders the author chat for the frame own request', async () => { mockRequest.query = { n8nShellInner: '1' }; mockRequest.headers = { @@ -998,6 +1009,7 @@ describe('ChatTrigger Node', () => { 'Content-Security-Policy', 'sandbox allow-scripts allow-forms allow-modals allow-popups', ); + expect(mockResponse.removeHeader).not.toHaveBeenCalled(); expect(renderedPage()).toContain('createChat'); // Identity injected server-side, and a header token for the messages that follow. expect(renderedPage()).toContain('"email":"visitor@example.com"'); diff --git a/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreMongoDBAtlas/VectorStoreMongoDBAtlas.node.test.ts b/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreMongoDBAtlas/VectorStoreMongoDBAtlas.node.test.ts index ec48bbde6be..79bb26eecab 100644 --- a/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreMongoDBAtlas/VectorStoreMongoDBAtlas.node.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreMongoDBAtlas/VectorStoreMongoDBAtlas.node.test.ts @@ -1,5 +1,6 @@ import { MongoClient } from 'mongodb'; import type { ILoadOptionsFunctions, ISupplyDataFunctions } from 'n8n-workflow'; +import { NodeOperationError } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; import { @@ -144,6 +145,30 @@ describe('VectorStoreMongoDBAtlas', () => { expect(mockClient.connect).toHaveBeenCalledTimes(1); expect(client).toBe(mockClient); }); + it('should not include the connection string in the error when the client cannot be created', async () => { + MockMongoClient.mockImplementation(function () { + throw new Error( + 'Protocol and host list are required in "mongodb://testuser:secret-pass@?x:27017"', + ); + }); + mockContext.getCredentials.mockResolvedValue({ + configurationType: 'values', + host: '?x', + user: 'testuser', + password: 'secret-pass', + port: 27017, + database: 'testdb', + }); + + const error = await createMongoClient(mockContext, 1.1).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(NodeOperationError); + expect((error as Error).message).toBe( + 'MongoDB connection error: Protocol and host list are required in "mongodb://[REDACTED]"', + ); + expect(JSON.stringify(error)).not.toContain('secret-pass'); + expect((error as Error).stack).not.toContain('secret-pass'); + }); }); describe('.getCollections', () => { diff --git a/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreMongoDBAtlas/VectorStoreMongoDBAtlas.node.ts b/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreMongoDBAtlas/VectorStoreMongoDBAtlas.node.ts index 7c9a0134dea..d6774b61683 100644 --- a/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreMongoDBAtlas/VectorStoreMongoDBAtlas.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreMongoDBAtlas/VectorStoreMongoDBAtlas.node.ts @@ -11,7 +11,10 @@ import { } from 'n8n-workflow'; import { metadataFilterField, createVectorStoreNode } from '@n8n/ai-utilities'; -import { validateAndResolveMongoCredentials } from 'n8n-nodes-base/dist/nodes/MongoDb/GenericFunctions'; +import { + sanitizeMongoUriInMessage, + validateAndResolveMongoCredentials, +} from 'n8n-nodes-base/dist/nodes/MongoDb/GenericFunctions'; /** * Constants for the name of the credentials and Node parameters. @@ -169,15 +172,22 @@ export async function createMongoClient( const node = context.getNode(); const { connectionString } = validateAndResolveMongoCredentials(node, credentials); - const client = new MongoClient(connectionString, { - appName: 'devrel.integration.n8n_vector_integ', - driverInfo: { - name: 'n8n_vector', - version: version.toString(), - }, - }); - await client.connect(); - return client; + try { + const client = new MongoClient(connectionString, { + appName: 'devrel.integration.n8n_vector_integ', + driverInfo: { + name: 'n8n_vector', + version: version.toString(), + }, + }); + await client.connect(); + return client; + } catch (error) { + throw new NodeOperationError( + node, + `MongoDB connection error: ${sanitizeMongoUriInMessage(error, connectionString)}`, + ); + } } /** diff --git a/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreOracleDB/VectorStoreOracleDB.node.test.ts b/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreOracleDB/VectorStoreOracleDB.node.test.ts index 3ba380b489f..a684256e819 100644 --- a/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreOracleDB/VectorStoreOracleDB.node.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreOracleDB/VectorStoreOracleDB.node.test.ts @@ -4,7 +4,7 @@ import type { Embeddings } from '@langchain/core/embeddings'; import type { VectorStore } from '@langchain/core/vectorstores'; import type { VectorStoreNodeConstructorArgs } from '@n8n/ai-utilities'; import { ConnectionPoolManager } from 'n8n-nodes-base/dist/utils/connection-pool-manager'; -import type { IExecuteFunctions } from 'n8n-workflow'; +import type { IExecuteFunctions, INode } from 'n8n-workflow'; import type { Pool } from 'oracledb'; import type { Mocked } from 'vitest'; @@ -323,9 +323,12 @@ vi.mock('@n8n/ai-utilities', () => { import { VectorStoreOracleDB } from './VectorStoreOracleDB.node'; describe('VectorStoreOracleDB.node', () => { + const node = { name: 'Oracle Database Vector Store', type: 'vectorStoreOracleDBVector' } as INode; + const context = { getNodeParameter: vi.fn(), getCredentials: vi.fn(), + getNode: vi.fn(() => node), logger: { debug: vi.fn(), error: vi.fn(), @@ -594,4 +597,104 @@ describe('VectorStoreOracleDB.node', () => { expect(connectionCloseMocks[1]).toHaveBeenCalledTimes(1); expect(poolCloseMocks[0]).not.toHaveBeenCalled(); }); + + describe('parameter validation', () => { + const withTableName = (tableName: string) => (name: string) => { + if (name === 'tableName') return tableName; + if (name === 'options.distanceStrategy') return DistanceStrategyMock.DOT_PRODUCT; + return undefined; + }; + + it('rejects a metadata filter name that is not a plain identifier', async () => { + const node = createNode(); + + await expect( + node.getVectorStoreClient(context, { "userId'": 'alice' } as OracleFilter, embeddings, 0), + ).rejects.toThrow('Invalid metadata filter key'); + expect(initializeSpy).not.toHaveBeenCalled(); + }); + + it('rejects a metadata filter name supplied at search time', async () => { + const node = createNode(); + const vectorStore = await node.getVectorStoreClient(context, undefined, embeddings, 0); + + await expect( + vectorStore.similaritySearchVectorWithScore([0.1], 3, { + 'userId?(@ == "bob")': 'alice', + } as OracleFilter), + ).rejects.toThrow('Invalid metadata filter key'); + expect(similaritySearchSpy).not.toHaveBeenCalled(); + }); + + it('rejects a metadata filter name nested under $and at search time', async () => { + const node = createNode(); + const vectorStore = await node.getVectorStoreClient(context, undefined, embeddings, 0); + + await expect( + vectorStore.similaritySearchVectorWithScore([0.1], 3, { + $and: [{ project: 'n8n' }, { 'a.b': 'x' }], + } as OracleFilter), + ).rejects.toThrow('Invalid metadata filter key'); + expect(similaritySearchSpy).not.toHaveBeenCalled(); + }); + + it('accepts a filter whose names are plain identifiers', async () => { + const node = createNode(); + const filter = { userId: 'alice', user_id: 'alice' } as OracleFilter; + + const vectorStore = await node.getVectorStoreClient(context, filter, embeddings, 0); + await vectorStore.similaritySearchVectorWithScore([0.1], 3, undefined); + + expect(similaritySearchSpy).toHaveBeenCalledWith([0.1], 3, filter); + }); + + it('rejects a table name that is not a plain identifier', async () => { + const node = createNode(); + context.getNodeParameter.mockImplementation(withTableName('n8n_vectors; SELECT 1')); + + await expect(node.getVectorStoreClient(context, undefined, embeddings, 0)).rejects.toThrow( + 'Invalid table name', + ); + + context.getNodeParameter.mockImplementation(defaultGetNodeParameter); + }); + + it('rejects a table name that is not a plain identifier when inserting', async () => { + const node = createNode(); + context.getNodeParameter.mockImplementation(withTableName('n8n_vectors; SELECT 1')); + + await expect(node.populateVectorStore(context, embeddings, documents, 0)).rejects.toThrow( + 'Invalid table name', + ); + expect(fromDocumentsSpy).not.toHaveBeenCalled(); + + context.getNodeParameter.mockImplementation(defaultGetNodeParameter); + }); + + it('accepts a schema-qualified table name', async () => { + const node = createNode(); + context.getNodeParameter.mockImplementation(withTableName('app.n8n_vectors')); + + await node.getVectorStoreClient(context, undefined, embeddings, 0); + + const lastCall = initializeSpy.mock.calls.at(-1); + if (!lastCall) throw new Error('initializeSpy should have been called'); + expect(lastCall[1]?.tableName).toBe('app.n8n_vectors'); + + context.getNodeParameter.mockImplementation(defaultGetNodeParameter); + }); + + it('falls back to the default table name when the parameter resolves to empty', async () => { + const node = createNode(); + context.getNodeParameter.mockImplementation(withTableName('')); + + await node.getVectorStoreClient(context, undefined, embeddings, 0); + + const lastCall = initializeSpy.mock.calls.at(-1); + if (!lastCall) throw new Error('initializeSpy should have been called'); + expect(lastCall[1]?.tableName).toBe('n8n_vectors'); + + context.getNodeParameter.mockImplementation(defaultGetNodeParameter); + }); + }); }); diff --git a/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreOracleDB/VectorStoreOracleDB.node.ts b/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreOracleDB/VectorStoreOracleDB.node.ts index c3f62415fb1..a56bfd33432 100644 --- a/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreOracleDB/VectorStoreOracleDB.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/vector_store/VectorStoreOracleDB/VectorStoreOracleDB.node.ts @@ -4,9 +4,15 @@ import { createVectorStoreNode, metadataFilterField } from '@n8n/ai-utilities'; import { DistanceStrategy, OracleVS, type OracleDBVSArgs } from '@oracle/langchain-oracledb'; import type { OracleDBNodeCredentials } from 'n8n-nodes-base/dist/nodes/Oracle/Sql/helpers/interfaces'; import { configureOracleDB } from 'n8n-nodes-base/dist/nodes/Oracle/Sql/transport'; -import type { IExecuteFunctions, INodeProperties, ISupplyDataFunctions } from 'n8n-workflow'; +import type { INode, IExecuteFunctions, INodeProperties, ISupplyDataFunctions } from 'n8n-workflow'; +import { NodeOperationError } from 'n8n-workflow'; import type oracledb from 'oracledb'; +import { + findUnsupportedJsonPathFilterKey, + isSafeQualifiedSqlIdentifier, +} from '@utils/sqlIdentifier'; + const sharedFields: INodeProperties[] = [ { displayName: 'Table Name', @@ -20,6 +26,40 @@ const sharedFields: INodeProperties[] = [ const DEFAULT_INITIALIZATION_TEXT = 'n8n vector store initialization text'; +const DEFAULT_TABLE_NAME = 'n8n_vectors'; + +/** Reads the Table Name, falling back to the default when an expression resolves to empty. */ +function getTableName( + context: IExecuteFunctions | ISupplyDataFunctions, + itemIndex: number, +): string { + const tableName = + (context.getNodeParameter('tableName', itemIndex, '', { extractValue: true }) as string) || + DEFAULT_TABLE_NAME; + + if (!isSafeQualifiedSqlIdentifier(tableName)) { + throw new NodeOperationError(context.getNode(), `Invalid table name \"${tableName}\"`, { + itemIndex, + description: + 'The table name must be a plain name, or a schema and name separated by a dot. Letters, digits and underscores only.', + }); + } + + return tableName; +} + +/** Rejects a metadata filter the Oracle store cannot resolve into a SQL/JSON path. */ +function assertSupportedFilter(node: INode, filter: unknown, itemIndex?: number): void { + const unsupported = findUnsupportedJsonPathFilterKey(filter); + if (unsupported === undefined) return; + + throw new NodeOperationError(node, `Invalid metadata filter key \"${unsupported}\"`, { + itemIndex, + description: + 'A metadata filter name must start with a letter or underscore and contain only letters, digits and underscores.', + }); +} + const NO_ROWS_FOUND_ERROR_MESSAGE = 'No rows found.'; const isNoRowsFoundError = (error: unknown): error is Error => @@ -146,11 +186,15 @@ const createLazyOraclePool = ( * with any ad-hoc filter provided at runtime. */ class ExtendedOracleDBVectorStore extends OracleVS { + n8nNode!: INode; + static async initialize( embeddings: EmbeddingsInterface, - args: OracleDBVSArgs, + args: OracleDBVSArgs & { n8nNode: INode }, ): Promise { - const oracleDBVectorStore = new this(embeddings, args); + const { n8nNode, ...rest } = args; + const oracleDBVectorStore = new this(embeddings, rest); + oracleDBVectorStore.n8nNode = n8nNode; await oracleDBVectorStore.initialize(); return oracleDBVectorStore; @@ -162,6 +206,9 @@ class ExtendedOracleDBVectorStore extends OracleVS { filter?: OracleVS['FilterType'], ) { const mergedFilter = mergeFilters(this.filter, filter); + // Checked here as well as at construction, because load and both + // retrieve-as-tool modes supply the filter at search time. + assertSupportedFilter(this.n8nNode, mergedFilter); try { return await super.similaritySearchVectorWithScore(query, k, mergedFilter); @@ -199,17 +246,17 @@ export class VectorStoreOracleDB extends createVectorStoreNode { - const tableName = context.getNodeParameter('tableName', itemIndex, '', { - extractValue: true, - }) as string; + const tableName = getTableName(context, itemIndex); + assertSupportedFilter(context.getNode(), filter, itemIndex); const credentials = (await context.getCredentials('oracleDBApi')) as OracleDBNodeCredentials; const client = createLazyOraclePool(context, credentials); const query = DEFAULT_INITIALIZATION_TEXT; - const config: OracleDBVSArgs = { + const config: OracleDBVSArgs & { n8nNode: INode } = { client, tableName, query, filter: filter as ExtendedOracleDBVectorStore['FilterType'] | undefined, + n8nNode: context.getNode(), }; config.distanceStrategy = context.getNodeParameter( @@ -227,9 +274,7 @@ export class VectorStoreOracleDB extends createVectorStoreNode>>, itemIndex: number, ): Promise { - const tableName = context.getNodeParameter('tableName', itemIndex, '', { - extractValue: true, - }) as string; + const tableName = getTableName(context, itemIndex); const credentials = (await context.getCredentials('oracleDBApi')) as OracleDBNodeCredentials; const client = createLazyOraclePool(context, credentials); const query = DEFAULT_INITIALIZATION_TEXT; diff --git a/packages/@n8n/nodes-langchain/utils/sqlIdentifier.test.ts b/packages/@n8n/nodes-langchain/utils/sqlIdentifier.test.ts index 037d8151e89..f797e1b56db 100644 --- a/packages/@n8n/nodes-langchain/utils/sqlIdentifier.test.ts +++ b/packages/@n8n/nodes-langchain/utils/sqlIdentifier.test.ts @@ -2,6 +2,7 @@ import { escapeQualifiedSqlIdentifier, escapeSqlIdentifier, findUnsafeMetadataFilterKey, + findUnsupportedJsonPathFilterKey, isSafeQualifiedSqlIdentifier, isSafeSqlIdentifier, } from './sqlIdentifier'; @@ -97,4 +98,56 @@ describe('sqlIdentifier', () => { expect(findUnsafeMetadataFilterKey({ ok: '1', 'a\\b': '2' })).toBe('a\\b'); }); }); + + describe('findUnsupportedJsonPathFilterKey', () => { + it.each([ + ['a plain name', { userId: 'alice' }], + ['an underscored name', { user_id: 'alice' }], + ['a leading underscore', { _internal: true }], + ['mixed case', { UserID: 'alice' }], + ['trailing digits', { field2: 'x' }], + ['several names at once', { project: 'n8n', category: 'movies', author: 'a' }], + ['an array value', { author: ['a', 'b'] }], + ['an operator in value position', { rating: { $gte: 4.5 } }], + ['an operator the store does not know', { nested: { flag: true } }], + ['names nested under $and', { $and: [{ project: 'n8n' }, { language: 'en' }] }], + ['names nested under $or', { $or: [{ project: 'n8n' }] }], + ['no filter at all', undefined], + ])('accepts %s', (_name, filter) => { + expect(findUnsupportedJsonPathFilterKey(filter)).toBeUndefined(); + }); + + it.each([ + ['a quote', "x'"], + ['a backslash', 'a\\b'], + ['a double quote', 'a"b'], + ['a path predicate', 'userId?(@ == "bob")'], + ['a dot', 'user.id'], + ['a hyphen', 'file-name'], + ['a space', 'my key'], + ['a dollar sign', 'a$b'], + ['a leading digit', '1field'], + ['an empty name', ''], + ])('rejects %s', (_name, key) => { + expect(findUnsupportedJsonPathFilterKey({ [key]: 'value' })).toBe(key); + }); + + it('reports a name nested under $and', () => { + const filter = { $and: [{ project: 'n8n' }, { "x'": 'alice' }] }; + + expect(findUnsupportedJsonPathFilterKey(filter)).toBe("x'"); + }); + + it('reports a name nested under $or inside $and', () => { + const filter = { $and: [{ $or: [{ ok: 1 }, { 'a.b': 2 }] }] }; + + expect(findUnsupportedJsonPathFilterKey(filter)).toBe('a.b'); + }); + + it('reports the first unsupported name when several are present', () => { + const filter = { good: 1, "bad'one": 2, 'bad.two': 3 }; + + expect(findUnsupportedJsonPathFilterKey(filter)).toBe("bad'one"); + }); + }); }); diff --git a/packages/@n8n/nodes-langchain/utils/sqlIdentifier.ts b/packages/@n8n/nodes-langchain/utils/sqlIdentifier.ts index e58ee0d4905..0a8df0aed1d 100644 --- a/packages/@n8n/nodes-langchain/utils/sqlIdentifier.ts +++ b/packages/@n8n/nodes-langchain/utils/sqlIdentifier.ts @@ -1,3 +1,4 @@ +import { isRecord } from '@n8n/utils/is-record'; import pg from 'pg'; /** @@ -64,3 +65,39 @@ export function findUnsafeMetadataFilterKey( if (!filter) return undefined; return Object.keys(filter).find((key) => key.includes("'") || key.includes('\\')); } + +/** + * Matches a field name that Oracle accepts as an unquoted SQL/JSON path step. + * Oracle documents the unquoted form as a leading letter followed by letters and + * digits; underscore is allowed here because it is inert in both the path + * grammar and the surrounding string literal, and is common in existing keys. + */ +const SAFE_JSON_PATH_FIELD = /^[A-Za-z_][A-Za-z0-9_]*$/; + +/** + * Returns the first metadata-filter key that the Oracle store cannot resolve as + * an unquoted SQL/JSON path step, or undefined when every key is usable. + * + * Walks `$and` and `$or` because the store expands them before building the + * path, so a nested key reaches the same place a top-level one does. + */ +export function findUnsupportedJsonPathFilterKey(filter: unknown): string | undefined { + if (!isRecord(filter)) return undefined; + + for (const [key, value] of Object.entries(filter)) { + if (key === '$and' || key === '$or') { + if (!Array.isArray(value)) continue; + for (const condition of value) { + const unsupported = findUnsupportedJsonPathFilterKey(condition); + if (unsupported !== undefined) return unsupported; + } + continue; + } + + // Only the field name reaches the path. Operator names in value position + // are matched against a fixed set by the store, and values are bound. + if (!SAFE_JSON_PATH_FIELD.test(key)) return key; + } + + return undefined; +} diff --git a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts index 6c31a61c19d..707ad8c978d 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/__tests__/js-task-runner.test.ts @@ -193,6 +193,34 @@ describe('JsTaskRunner', () => { expect(outcome.result).toEqual([wrapIntoJson({ allZeros: true })]); }); + it('should give small buffers their own backing memory', async () => { + const outcome = await executeForAllItems({ + code: ` + const fromString = Buffer.from('.'); + const fromArray = Buffer.from([1, 2]); + return [{ json: { + fromString: fromString.buffer.byteLength, + fromArray: fromArray.buffer.byteLength, + } }] + `, + inputItems: [{ a: 1 }], + }); + + expect(outcome.result).toEqual([wrapIntoJson({ fromString: 1, fromArray: 2 })]); + }); + + it('should prevent guest code from re-enabling the buffer pool', async () => { + const outcome = await executeForAllItems({ + code: ` + Buffer.poolSize = 8192; + return [{ json: { poolSize: Buffer.poolSize } }] + `, + inputItems: [{ a: 1 }], + }); + + expect(outcome.result).toEqual([wrapIntoJson({ poolSize: 0 })]); + }); + it('should support Buffer.from and Buffer.concat', async () => { const outcome = await executeForAllItems({ code: ` diff --git a/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts b/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts index 4cd16c6e028..78387155ac0 100644 --- a/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts +++ b/packages/@n8n/task-runner/src/js-task-runner/js-task-runner.ts @@ -186,6 +186,9 @@ export class JsTaskRunner extends TaskRunner { const safeAlloc = Buffer.alloc.bind(Buffer); Buffer.allocUnsafe = safeAlloc; Buffer.allocUnsafeSlow = safeAlloc; + // Disable the shared buffer pool so small buffers get their own backing memory. + // Use defineProperty so the value is read-only: sandboxed code must not re-enable the pool. + Object.defineProperty(Buffer, 'poolSize', { value: 0, writable: false, configurable: false }); if (process.env.NODE_ENV !== 'test') { freezeGlobals(); diff --git a/packages/cli/src/__tests__/abstract-server.test.ts b/packages/cli/src/__tests__/abstract-server.test.ts index 321bf20f4e1..deb112bb591 100644 --- a/packages/cli/src/__tests__/abstract-server.test.ts +++ b/packages/cli/src/__tests__/abstract-server.test.ts @@ -7,6 +7,16 @@ import { mock } from 'vitest-mock-extended'; import { AbstractServer } from '@/abstract-server'; import { ExternalHooks } from '@/external-hooks'; +import { LiveWebhooks } from '@/webhooks/live-webhooks'; +import { SlackInteractionWebhooks } from '@/webhooks/slack-interaction-webhooks'; +import { TelegramInteractionWebhooks } from '@/webhooks/telegram-interaction-webhooks'; +import { WaitingForms } from '@/webhooks/waiting-forms'; +import { WaitingWebhooks } from '@/webhooks/waiting-webhooks'; +import { createWebhookHandlerFor } from '@/webhooks/webhook-request-handler'; + +vi.mock('@/webhooks/webhook-request-handler', () => ({ + createWebhookHandlerFor: vi.fn(), +})); const mockServer = mock(); @@ -87,6 +97,21 @@ describe('AbstractServer health endpoints', () => { await testServer.init(); }); + describe('webhook route registration', () => { + it('should register the waiting-forms route with the form sandbox family', async () => { + const waitingForms = mock(); + Container.set(LiveWebhooks, mock()); + Container.set(WaitingForms, waitingForms); + Container.set(WaitingWebhooks, mock()); + Container.set(SlackInteractionWebhooks, mock()); + Container.set(TelegramInteractionWebhooks, mock()); + + await testServer.start(); + + expect(createWebhookHandlerFor).toHaveBeenCalledWith(waitingForms, undefined, 'form'); + }); + }); + describe('/healthz (liveness)', () => { it('should return 200 regardless of fullyReady flag', () => { const res = mock(); diff --git a/packages/cli/src/__tests__/response-helper.test.ts b/packages/cli/src/__tests__/response-helper.test.ts index 4954207f0bd..e98e910402b 100644 --- a/packages/cli/src/__tests__/response-helper.test.ts +++ b/packages/cli/src/__tests__/response-helper.test.ts @@ -1,4 +1,6 @@ import { mockInstance } from '@n8n/backend-test-utils'; +import { SecurityConfig } from '@n8n/config'; +import { Container } from '@n8n/di'; import type { Request, Response } from 'express'; import { ErrorReporter } from 'n8n-core'; import { UserError } from 'n8n-workflow'; @@ -8,6 +10,7 @@ import { ConflictError, ForbiddenError, InternalServerError, NotFoundError } fro import { LicenseEulaRequiredError } from '@/errors/response-errors/license-eula-required.error'; import { PolicyViolationError } from '@/policy/policy-violation.error'; import { reportError, sendErrorResponse } from '@/response-helper'; +import { setRouteSandboxSetting } from '@/webhooks/webhook-response-headers'; describe('sendErrorResponse', () => { let mockResponse: Response; @@ -56,10 +59,34 @@ describe('sendErrorResponse', () => { ); }); + it('should declare a JSON content type on error responses', () => { + // Simulate an earlier handler having declared an HTML content type. + const res = mock({ + status: vi.fn().mockReturnThis(), + json: vi.fn().mockReturnThis(), + get: vi.fn().mockReturnValue('text/html; charset=utf-8'), + }); + + sendErrorResponse(res, new Error('Regular error')); + + expect(res.setHeader).toHaveBeenCalledWith( + 'Content-Type', + expect.stringContaining('application/json'), + ); + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + code: 0, + message: 'Regular error', + }), + ); + }); + describe('form pages', () => { const responseFor = (originalUrl: string) => mock({ req: mock({ originalUrl }), + locals: {}, status: vi.fn().mockReturnThis(), json: vi.fn().mockReturnThis(), render: vi.fn().mockReturnThis(), @@ -88,6 +115,45 @@ describe('sendErrorResponse', () => { expect.stringContaining('sandbox'), ); }); + + describe('when only form sandboxing is disabled', () => { + const securityConfig = Container.get(SecurityConfig); + + beforeEach(() => { + securityConfig.disableFormHtmlSandboxing = true; + }); + + afterEach(() => { + securityConfig.disableFormHtmlSandboxing = false; + }); + + it('should keep the webhook sandbox on a webhook path that looks like a form path', () => { + const res = responseFor('/webhook/n8n-form-demo'); + setRouteSandboxSetting(res, 'webhook'); + + sendErrorResponse(res, new NotFoundError('not found')); + + expect(res.render).toHaveBeenCalledWith('form-trigger-404', { isTestWebhook: false }); + expect(res.setHeader).toHaveBeenCalledWith( + 'Content-Security-Policy', + expect.stringContaining('sandbox'), + ); + expect(res.removeHeader).not.toHaveBeenCalled(); + }); + + it('should not sandbox the form 404 page on a form route', () => { + const res = responseFor('/form/does-not-exist'); + setRouteSandboxSetting(res, 'form'); + + sendErrorResponse(res, new NotFoundError('not found')); + + expect(res.render).toHaveBeenCalledWith('form-trigger-404', { isTestWebhook: false }); + expect(res.setHeader).not.toHaveBeenCalledWith( + 'Content-Security-Policy', + expect.stringContaining('sandbox'), + ); + }); + }); }); }); 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 daed589d85d..d1569107dc6 100644 --- a/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts +++ b/packages/cli/src/__tests__/workflow-execute-additional-data.test.ts @@ -379,15 +379,16 @@ describe('WorkflowExecuteAdditionalData', () => { parameters: {}, }, ]; - const subWorkflowData = () => - mock({ + // Plain object: inline code is structure-validated, which a deep mock fails. + const subWorkflowData = (): IWorkflowBase => + ({ id: 'sub-id', name: 'Sub Workflow', nodes: structuredClone(nodes), connections: {}, staticData: {}, settings: {}, - }); + }) as IWorkflowBase; beforeEach(() => { nodeTypes.getByNameAndVersion.mockReturnValue(nodeType); @@ -398,7 +399,7 @@ describe('WorkflowExecuteAdditionalData', () => { it('checks credentials against the triggering user for an inline sub-workflow', async () => { await executeWorkflow( - mock({ id: undefined, code: subWorkflowData() }), + { code: subWorkflowData() }, // `rootExecutionMode` explicit: a deep mock auto-stubs every // property, so leaving it out would hand `??` a truthy proxy. mock({ @@ -415,11 +416,24 @@ describe('WorkflowExecuteAdditionalData', () => { expect(credentialsPermissionChecker.check).not.toHaveBeenCalled(); }); + it('rejects an inline sub-workflow with a non-boolean disabled flag', async () => { + const code = subWorkflowData(); + code.nodes = code.nodes.map((n) => ({ ...n, disabled: 1 as unknown as boolean })); + + await expect( + executeWorkflow({ code }, mock({ userId: 'user-1' }), { + parentWorkflowId: 'parent-1', + executionMode: 'manual', + }), + ).rejects.toThrow('Workflow structure is invalid'); + expect(credentialsPermissionChecker.checkForUser).not.toHaveBeenCalled(); + }); + // A sub-workflow's own WorkflowExecute runs as 'integrated', so the // original mode has to come from the root. it('checks against the triggering user through a nested inline sub-workflow', async () => { await executeWorkflow( - mock({ id: undefined, code: subWorkflowData() }), + { code: subWorkflowData() }, mock({ userId: 'user-1', rootExecutionMode: 'manual', @@ -442,7 +456,7 @@ describe('WorkflowExecuteAdditionalData', () => { // access to it has since changed. it('keeps the project check for an inline sub-workflow in a triggered run', async () => { await executeWorkflow( - mock({ id: undefined, code: subWorkflowData() }), + { code: subWorkflowData() }, mock({ userId: 'publisher-1', rootExecutionMode: undefined, @@ -467,7 +481,7 @@ describe('WorkflowExecuteAdditionalData', () => { 'checks against the acting user for an inline sub-workflow in a %s run', async (executionMode) => { await executeWorkflow( - mock({ id: undefined, code: subWorkflowData() }), + { code: subWorkflowData() }, mock({ userId: 'user-1', rootExecutionMode: undefined, @@ -502,7 +516,7 @@ describe('WorkflowExecuteAdditionalData', () => { it('falls back to the project check for an inline sub-workflow without a triggering user', async () => { const workflowData = subWorkflowData(); await executeWorkflow( - mock({ id: undefined, code: workflowData }), + { code: workflowData }, mock({ userId: undefined }), { parentWorkflowId: 'parent-1', executionMode: 'webhook' }, ); @@ -555,7 +569,7 @@ describe('WorkflowExecuteAdditionalData', () => { it('preserves parent static-data persistence for inline definitions without an id', async () => { await executeWorkflow( - { code: mock({ ...subWorkflowData(), id: undefined }) }, + { code: { ...subWorkflowData(), id: undefined } as unknown as IWorkflowBase }, mock({ userId: undefined }), { parentWorkflowId: 'parent-1', executionMode: 'webhook' }, ); @@ -579,7 +593,7 @@ describe('WorkflowExecuteAdditionalData', () => { 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 () => { await executeWorkflow( - mock({ id: undefined, code: subWorkflowData() }), + { code: subWorkflowData() }, mock({ userId: 'user-1' }), mock({ loadedWorkflowData: subWorkflowData(), @@ -1352,22 +1366,22 @@ describe('WorkflowExecuteAdditionalData', () => { }); it('should use provided workflow code when id is not provided', async () => { - const workflowCode = mock({ + const workflowCode = { id: 'code-workflow', name: 'Code Workflow', active: false, nodes: [ - mock({ + { id: 'node1', type: 'n8n-nodes-base.set', name: 'Node 1', typeVersion: 1, parameters: {}, position: [250, 300], - }), + } as INode, ], connections: {}, - }); + } as IWorkflowBase; const result = await getPublishedWorkflowData({ code: workflowCode }, 'parent-workflow-id'); diff --git a/packages/cli/src/abstract-server.ts b/packages/cli/src/abstract-server.ts index 4f003949994..8339890f670 100644 --- a/packages/cli/src/abstract-server.ts +++ b/packages/cli/src/abstract-server.ts @@ -269,7 +269,7 @@ export abstract class AbstractServer { // Register a handler for waiting forms (excluded from metrics to avoid double-counting) this.app.all( `/${this.endpointFormWaiting}/:path{/:suffix}`, - createWebhookHandlerFor(Container.get(WaitingForms)), + createWebhookHandlerFor(Container.get(WaitingForms), undefined, 'form'), ); // Register a handler for waiting webhooks (excluded from metrics to avoid double-counting) diff --git a/packages/cli/src/errors/uninspectable-subworkflow-source.error.ts b/packages/cli/src/errors/uninspectable-subworkflow-source.error.ts new file mode 100644 index 00000000000..ba686feb263 --- /dev/null +++ b/packages/cli/src/errors/uninspectable-subworkflow-source.error.ts @@ -0,0 +1,11 @@ +import type { INode } from 'n8n-workflow'; +import { NodeOperationError } from 'n8n-workflow'; + +export class UninspectableSubworkflowSourceError extends NodeOperationError { + constructor(node: INode) { + super( + node, + `The '${node.name}' node loads its sub-workflow from a source that is resolved only when the workflow runs, so its credential access cannot be checked on save. Use the "Database" or "Define Below" source instead.`, + ); + } +} diff --git a/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts b/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts index 8d1e382398f..4160cfb32d8 100644 --- a/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts +++ b/packages/cli/src/executions/pre-execution-checks/__tests__/credentials-permission-checker.test.ts @@ -810,6 +810,77 @@ describe('CredentialsPermissionChecker', () => { }); }); + describe('with a default-valued authentication parameter', () => { + const credentialId = 'password-cred'; + const description = { + credentials: [ + { + name: 'passwordApi', + required: true, + displayOptions: { show: { authentication: ['password'] } }, + }, + { + name: 'tokenApi', + required: true, + displayOptions: { show: { authentication: ['token'] } }, + }, + ], + properties: [ + { + displayName: 'Authentication', + name: 'authentication', + type: 'options', + options: [ + { name: 'Password', value: 'password' }, + { name: 'Token', value: 'token' }, + ], + default: 'password', + }, + ], + }; + + // Saved nodes omit parameters left at their default value + const node: INode = { + id: 'node-6', + name: 'Service', + type: 'n8n-nodes-base.service', + typeVersion: 1, + position: [0, 0], + parameters: { resource: 'entry' }, + credentials: { passwordApi: { id: credentialId, name: 'Password' } }, + }; + + beforeEach(() => { + nodeTypes.getByNameAndVersion.mockReturnValue({ description } as never); + credentialsRepository.findGlobalProjectCredentialIds.mockResolvedValue([]); + }); + + it('should check the credential selected by the default value', async () => { + sharedCredentialsRepository.getFilteredAccessibleCredentials.mockResolvedValue([]); + + await expect(permissionChecker.check(workflowId, [node])).rejects.toThrow( + 'Node "Service" does not have access to the credential', + ); + + expect(sharedCredentialsRepository.getFilteredAccessibleCredentials).toHaveBeenCalledWith( + [teamProject.id], + [credentialId], + ); + }); + + it('should let the run through when the project can use the credential', async () => { + sharedCredentialsRepository.getFilteredAccessibleCredentials.mockResolvedValue([ + credentialId, + ]); + + await expect(permissionChecker.check(workflowId, [node])).resolves.not.toThrow(); + }); + + it('should return the credential id for the node', () => { + expect(permissionChecker.getCredentialIdsForNodes([node])).toEqual([credentialId]); + }); + }); + it('should fall back to checking all credentials if node type cannot be resolved', async () => { nodeTypes.getByNameAndVersion.mockImplementation(() => { throw new Error('Unknown node type'); @@ -874,6 +945,12 @@ describe('CredentialsPermissionChecker', () => { expect(permissionChecker.getCredentialIdsForNodes([disabledNode])).toEqual([]); }); + it.each([1, 'true', {}])('does not skip a node whose disabled flag is %p', (disabled) => { + const flaggedNode = { ...node, disabled } as unknown as INode; + + expect(permissionChecker.getCredentialIdsForNodes([flaggedNode])).toEqual([credentialId]); + }); + it('skips a credential reference with no id instead of throwing', () => { const nodeWithNoId: INode = { ...node, diff --git a/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts b/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts index 785f6e7445e..bcbb5a20ec7 100644 --- a/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts +++ b/packages/cli/src/executions/pre-execution-checks/credentials-permission-checker.ts @@ -480,7 +480,7 @@ export class CredentialsPermissionChecker { getCredentialIdsForNodes(nodes: INode[]): string[] { const ids = new Set(); for (const node of nodes) { - if (node.disabled || !node.credentials) continue; + if (node.disabled === true || !node.credentials) continue; const activeCredTypes = this.getActiveCredentialTypes(node); @@ -495,7 +495,7 @@ export class CredentialsPermissionChecker { private mapCredIdsToNodes(nodes: INode[]) { return nodes.reduce<{ [credentialId: string]: INode[] }>((map, node) => { - if (node.disabled || !node.credentials) return map; + if (node.disabled === true || !node.credentials) return map; const activeCredTypes = this.getActiveCredentialTypes(node); diff --git a/packages/cli/src/middlewares/list-query/__tests__/list-query.test.ts b/packages/cli/src/middlewares/list-query/__tests__/list-query.test.ts index 2aafe4509dd..41571966b96 100644 --- a/packages/cli/src/middlewares/list-query/__tests__/list-query.test.ts +++ b/packages/cli/src/middlewares/list-query/__tests__/list-query.test.ts @@ -23,7 +23,7 @@ describe('List query middleware', () => { vi.clearAllMocks(); mockReq = { baseUrl: '/rest/workflows' } as ListQuery.Request; - mockRes = { status: () => ({ json: vi.fn() }) } as unknown as Response; + mockRes = { status: () => ({ json: vi.fn() }), setHeader: vi.fn() } as unknown as Response; args = [mockReq, mockRes, nextFn]; sendErrorResponse = vi.spyOn(ResponseHelper, 'sendErrorResponse'); diff --git a/packages/cli/src/middlewares/list-query/dtos/workflow.filter.dto.ts b/packages/cli/src/middlewares/list-query/dtos/workflow.filter.dto.ts index a2103b47909..02d31fe26fd 100644 --- a/packages/cli/src/middlewares/list-query/dtos/workflow.filter.dto.ts +++ b/packages/cli/src/middlewares/list-query/dtos/workflow.filter.dto.ts @@ -76,25 +76,6 @@ export class WorkflowFilter extends BaseFilter { @Expose() executionBlockedBy?: WorkflowExecutionBlockCause[]; - /** - * When `true`, the list is widened to include workflows that the workflow - * identified by `parentWorkflowId` is permitted to call, even if those - * workflows are not directly shared with the requesting user. - */ - @IsBoolean() - @IsOptional() - @Expose() - includeCallableSubworkflows?: boolean; - - /** - * ID of the workflow that will act as the caller. - * Used together with `includeCallableSubworkflows` to evaluate `callerPolicy`. - */ - @IsString() - @IsOptional() - @Expose() - parentWorkflowId?: string; - static async fromString(rawFilter: string) { return await this.toFilter(rawFilter, WorkflowFilter); } diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-eval.service.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-eval.service.test.ts index 5c539ac9c4d..c83945583c5 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-eval.service.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-eval.service.test.ts @@ -43,6 +43,9 @@ vi.mock('../agent-eval-instructions-rewrite', () => ({ rewriteAgentInstructions: vi.mock('../agent-eval-case-generation.service', () => ({ AgentEvalCaseGenerationService: class AgentEvalCaseGenerationService {}, })); +vi.mock('@/permissions.ee/check-access', () => ({ + userHasScopes: vi.fn().mockResolvedValue(true), +})); const AGENT_ID = 'agent-1'; const PROJECT_ID = 'proj-1'; @@ -132,6 +135,7 @@ describe('AgentEvalService', () => { runRepository.findByIdAndAgentId.mockResolvedValue(makeRun()); runRepository.findAndCountByDatasetIdAndAgentId.mockResolvedValue([[], 0]); resultRepository.findAndCountByRunId.mockResolvedValue([[], 0]); + vi.mocked(userHasScopes).mockReset().mockResolvedValue(true); resultRepository.findById.mockResolvedValue(makeResult()); service = new AgentEvalService( @@ -181,7 +185,10 @@ describe('AgentEvalService', () => { ['previewRun', async () => await service.previewRun(user, AGENT_ID, PROJECT_ID, {})], ['startRun', async () => await service.startRun(user, AGENT_ID, PROJECT_ID, 'ds-1', {})], ['listRuns', async () => await service.listRuns(AGENT_ID, PROJECT_ID, 'ds-1', PAGE)], - ['getRunDetail', async () => await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1', PAGE)], + [ + 'getRunDetail', + async () => await service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-1', PAGE), + ], ['getRunSummary', async () => await service.getRunSummary(AGENT_ID, PROJECT_ID, 'run-1')], ['cancelRun', async () => await service.cancelRun(AGENT_ID, PROJECT_ID, 'run-1')], [ @@ -255,9 +262,9 @@ describe('AgentEvalService', () => { it('404s a run belonging to another agent, and reads no results for it', async () => { runRepository.findByIdAndAgentId.mockResolvedValue(null); - await expect(service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-other', PAGE)).rejects.toThrow( - NotFoundError, - ); + await expect( + service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-other', PAGE), + ).rejects.toThrow(NotFoundError); expect(resultRepository.findAndCountByRunId).not.toHaveBeenCalled(); }); @@ -956,6 +963,77 @@ describe('AgentEvalService', () => { createdById: 'user-1', }); }); + + it('checks the creating user can read the referenced data table', async () => { + datasetRepository.createDataset.mockResolvedValue(makeDataset()); + + await service.createDataset(user, AGENT_ID, PROJECT_ID, { + name: 'cases', + agentId: AGENT_ID, + datasetSource: 'data_table', + datasetRef: { dataTableId: 'dt-1' }, + }); + + expect(userHasScopes).toHaveBeenCalledWith(user, ['dataTable:readRow'], false, { + dataTableId: 'dt-1', + }); + }); + + it('refuses a data table the creating user cannot read', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + + await expect( + service.createDataset(user, AGENT_ID, PROJECT_ID, { + name: 'cases', + agentId: AGENT_ID, + datasetSource: 'data_table', + datasetRef: { dataTableId: 'dt-other' }, + }), + ).rejects.toThrow(ForbiddenError); + expect(datasetRepository.createDataset).not.toHaveBeenCalled(); + }); + }); + + // Results hold copies of the source rows, and the table can live in another + // project than the agent, so `agent:read` alone must not open them. + describe('getRunDetail data table access', () => { + it('checks the reader can read the run dataset’s data table', async () => { + await service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-1', PAGE); + + expect(datasetRepository.findByIdAndAgentId).toHaveBeenCalledWith('ds-1', AGENT_ID); + expect(userHasScopes).toHaveBeenCalledWith(user, ['dataTable:readRow'], false, { + dataTableId: 'dt-1', + }); + }); + + it('refuses a reader without access to the data table, and reads no results', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + + await expect(service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-1', PAGE)).rejects.toThrow( + ForbiddenError, + ); + expect(resultRepository.findAndCountByRunId).not.toHaveBeenCalled(); + }); + + it('reads no results when the run’s dataset does not resolve', async () => { + datasetRepository.findByIdAndAgentId.mockResolvedValue(null); + + await expect(service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-1', PAGE)).rejects.toThrow( + NotFoundError, + ); + expect(resultRepository.findAndCountByRunId).not.toHaveBeenCalled(); + }); + + it('refuses a data-table dataset whose ref carries no table id', async () => { + datasetRepository.findByIdAndAgentId.mockResolvedValue( + makeDataset({ datasetRef: { credentialId: 'c-1', spreadsheetId: 's-1', sheetName: 'S' } }), + ); + + await expect(service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-1', PAGE)).rejects.toThrow( + /without a table id/, + ); + expect(resultRepository.findAndCountByRunId).not.toHaveBeenCalled(); + }); }); describe('deleteDataset', () => { @@ -1039,7 +1117,7 @@ describe('AgentEvalService', () => { describe('response mapping', () => { it('serializes dates as ISO strings and keeps run coordination columns off the wire', async () => { - const detail = await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1', PAGE); + const detail = await service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-1', PAGE); expect(detail.runAt).toBe('2026-01-03T00:00:00.000Z'); expect(detail.createdAt).toBe('2026-01-03T00:00:00.000Z'); @@ -1106,7 +1184,7 @@ describe('AgentEvalService', () => { 1, ]); - const detail = await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1', PAGE); + const detail = await service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-1', PAGE); expect(detail.results.data).toHaveLength(1); expect(detail.results.data[0]).toMatchObject({ @@ -1138,7 +1216,7 @@ describe('AgentEvalService', () => { it('passes the run-detail window to the repository and reports the case total', async () => { resultRepository.findAndCountByRunId.mockResolvedValue([[], 500]); - const detail = await service.getRunDetail(AGENT_ID, PROJECT_ID, 'run-1', { + const detail = await service.getRunDetail(user, AGENT_ID, PROJECT_ID, 'run-1', { take: 25, skip: 50, }); diff --git a/packages/cli/src/modules/agent-evals/__tests__/agent-evals.controller.test.ts b/packages/cli/src/modules/agent-evals/__tests__/agent-evals.controller.test.ts index 3f58ed59bac..9906c8655ba 100644 --- a/packages/cli/src/modules/agent-evals/__tests__/agent-evals.controller.test.ts +++ b/packages/cli/src/modules/agent-evals/__tests__/agent-evals.controller.test.ts @@ -485,7 +485,7 @@ describe('AgentEvalsController', () => { await controller.getRun(runReq(), undefined, query); - expect(service.getRunDetail).toHaveBeenCalledWith(AGENT_ID, PROJECT_ID, 'run-1', { + expect(service.getRunDetail).toHaveBeenCalledWith(user, AGENT_ID, PROJECT_ID, 'run-1', { take: 25, skip: 50, }); @@ -507,7 +507,7 @@ describe('AgentEvalsController', () => { it('defaults the run-detail window wider than the shared list default', async () => { await controller.getRun(runReq(), undefined, AgentEvalRunDetailQueryDto.parse({})); - expect(service.getRunDetail).toHaveBeenCalledWith(AGENT_ID, PROJECT_ID, 'run-1', { + expect(service.getRunDetail).toHaveBeenCalledWith(user, AGENT_ID, PROJECT_ID, 'run-1', { take: AGENT_EVAL_RESULTS_DEFAULT_TAKE, skip: 0, }); diff --git a/packages/cli/src/modules/agent-evals/agent-eval.service.ts b/packages/cli/src/modules/agent-evals/agent-eval.service.ts index dc2f6124f39..b3ff8f2e415 100644 --- a/packages/cli/src/modules/agent-evals/agent-eval.service.ts +++ b/packages/cli/src/modules/agent-evals/agent-eval.service.ts @@ -27,6 +27,7 @@ import { AgentEvalRunRepository, } from '@n8n/db'; import { Service } from '@n8n/di'; +import { UnexpectedError } from 'n8n-workflow'; import pLimit from 'p-limit'; import { BadRequestError, ForbiddenError, NotFoundError, OperationalError } from '@n8n/errors'; @@ -110,6 +111,10 @@ export class AgentEvalService { ); } + if (payload.datasetSource === 'data_table') { + await this.assertDataTableReadable(user, payload.datasetRef.dataTableId); + } + const dataset = await this.datasetRepository.createDataset({ name: payload.name, description: payload.description ?? null, @@ -323,8 +328,13 @@ export class AgentEvalService { return { count, data: runs.map(toRunRecord) }; } - /** A run with a page of its per-case results — the "open a run" view. */ + /** + * A run with a page of its per-case results — the "open a run" view. Results + * hold copies of the source rows, so the reader needs the same table access + * the runner required of whoever started the run. + */ async getRunDetail( + user: User, agentId: string, projectId: string, runId: string, @@ -332,6 +342,7 @@ export class AgentEvalService { ): Promise { await this.assertAgentInProject(agentId, projectId); const run = await this.resolveRun(agentId, runId); + await this.assertDatasetSourceReadable(user, await this.resolveDataset(agentId, run.datasetId)); const [results, count] = await this.resultRepository.findAndCountByRunId(runId, page); return { ...toRunRecord(run), results: { count, data: results.map(toResultRecord) } }; } @@ -541,6 +552,24 @@ export class AgentEvalService { return dataset; } + // The table may live in another project than the agent, so `@ProjectScope` on + // the agent's project says nothing about it. Only data-table datasets are + // runnable today, so no other source has stored rows to protect. + private async assertDatasetSourceReadable(user: User, dataset: AgentEvalDataset): Promise { + if (dataset.datasetSource !== 'data_table') return; + if (!('dataTableId' in dataset.datasetRef)) { + throw new UnexpectedError( + `Agent eval dataset ${dataset.id} has a 'data_table' source without a table id.`, + ); + } + await this.assertDataTableReadable(user, dataset.datasetRef.dataTableId); + } + + private async assertDataTableReadable(user: User, dataTableId: string): Promise { + const allowed = await userHasScopes(user, ['dataTable:readRow'], false, { dataTableId }); + if (!allowed) throw new ForbiddenError('You do not have access to this dataset.'); + } + private async resolveRun(agentId: string, runId: string): Promise { const run = await this.runRepository.findByIdAndAgentId(runId, agentId); if (!run) throw new NotFoundError(`Agent eval run ${runId} not found.`); diff --git a/packages/cli/src/modules/agent-evals/agent-evals.controller.ts b/packages/cli/src/modules/agent-evals/agent-evals.controller.ts index da24502fe09..5d970aa4f38 100644 --- a/packages/cli/src/modules/agent-evals/agent-evals.controller.ts +++ b/packages/cli/src/modules/agent-evals/agent-evals.controller.ts @@ -255,7 +255,7 @@ export class AgentEvalsController { ): Promise { await this.flagGate.assertEnabled(req.user); const { agentId, projectId, runId } = req.params; - return await this.service.getRunDetail(agentId, projectId, runId, query); + return await this.service.getRunDetail(req.user, agentId, projectId, runId, query); } /** Counts only — cheap enough for the UI to poll while a run is in flight. */ diff --git a/packages/cli/src/modules/agents/__tests__/agents-list.controller.test.ts b/packages/cli/src/modules/agents/__tests__/agents-list.controller.test.ts index 5eadcbdb795..9217f21b7c4 100644 --- a/packages/cli/src/modules/agents/__tests__/agents-list.controller.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agents-list.controller.test.ts @@ -21,11 +21,11 @@ describe('AgentsListController', () => { const res = mock(); const query = { skip: 0, take: 50, sortBy: 'updatedAt:desc' } as never; - agentsService.findByUserMembershipPaginated.mockResolvedValue(response); + agentsService.findListableByUserPaginated.mockResolvedValue(response); await controller.list(req, res, query); - expect(agentsService.findByUserMembershipPaginated).toHaveBeenCalledWith(user, query); + expect(agentsService.findListableByUserPaginated).toHaveBeenCalledWith(user, query); expect(agentsService.findChatReachableByUserPaginated).not.toHaveBeenCalled(); expect(res.json).toHaveBeenCalledWith(response); }); @@ -35,11 +35,11 @@ describe('AgentsListController', () => { const res = mock(); const query = { skip: 0, take: 10, filter: { query: 'support' } } as never; - agentsService.findByUserMembershipPaginated.mockResolvedValue({ count: 0, data: [] } as never); + agentsService.findListableByUserPaginated.mockResolvedValue({ count: 0, data: [] } as never); await controller.list(req, res, query); - expect(agentsService.findByUserMembershipPaginated).toHaveBeenCalledWith(user, query); + expect(agentsService.findListableByUserPaginated).toHaveBeenCalledWith(user, query); }); it.each([true, false])('serves the chat list when availableInChat is %s', async (value) => { @@ -53,7 +53,7 @@ describe('AgentsListController', () => { await controller.list(req, res, query); expect(agentsService.findChatReachableByUserPaginated).toHaveBeenCalledWith(user, query); - expect(agentsService.findByUserMembershipPaginated).not.toHaveBeenCalled(); + expect(agentsService.findListableByUserPaginated).not.toHaveBeenCalled(); expect(res.json).toHaveBeenCalledWith(response); }); @@ -63,7 +63,7 @@ describe('AgentsListController', () => { const query = { skip: 0, take: 50 } as never; const emptyResponse = { count: 0, data: [] } as never; - agentsService.findByUserMembershipPaginated.mockResolvedValue(emptyResponse); + agentsService.findListableByUserPaginated.mockResolvedValue(emptyResponse); await controller.list(req, res, query); diff --git a/packages/cli/src/modules/agents/__tests__/agents.service.test.ts b/packages/cli/src/modules/agents/__tests__/agents.service.test.ts index fb4c10fa964..c798ee434be 100644 --- a/packages/cli/src/modules/agents/__tests__/agents.service.test.ts +++ b/packages/cli/src/modules/agents/__tests__/agents.service.test.ts @@ -1148,28 +1148,36 @@ describe('AgentsService', () => { }); }); - describe('findByUserMembershipPaginated', () => { + describe('findListableByUserPaginated', () => { const user = { id: 'user-1' } as unknown as User; - it('uses bare project membership and does not consult the scope service', async () => { + it('scopes to agent:list, the scope the project list route requires', async () => { const { service, agentRepository, projectRelationRepository, projectScopeService } = makeService(); - projectRelationRepository.findAllByUser.mockResolvedValue([ - { projectId: 'project-1' }, - { projectId: 'project-2' }, - ] as never); + projectScopeService.getProjectIds.mockResolvedValue(['project-1', 'project-2']); agentRepository.findByProjectIdsPaginated.mockResolvedValue({ count: 0, data: [] }); const options = { skip: 0, take: 10 } as never; - await service.findByUserMembershipPaginated(user, options); + await service.findListableByUserPaginated(user, options); - expect(projectRelationRepository.findAllByUser).toHaveBeenCalledWith('user-1'); - expect(projectScopeService.getProjectIds).not.toHaveBeenCalled(); + expect(projectScopeService.getProjectIds).toHaveBeenCalledWith(user, ['agent:list']); + expect(projectRelationRepository.findAllByUser).not.toHaveBeenCalled(); expect(agentRepository.findByProjectIdsPaginated).toHaveBeenCalledWith( ['project-1', 'project-2'], options, ); }); + + it('passes a null project scope through, so a global role sees every project', async () => { + const { service, agentRepository, projectScopeService } = makeService(); + projectScopeService.getProjectIds.mockResolvedValue(null); + agentRepository.findByProjectIdsPaginated.mockResolvedValue({ count: 0, data: [] }); + const options = { skip: 0, take: 10 } as never; + + await service.findListableByUserPaginated(user, options); + + expect(agentRepository.findByProjectIdsPaginated).toHaveBeenCalledWith(null, options); + }); }); describe('getCapabilitySummary', () => { diff --git a/packages/cli/src/modules/agents/agents-list.controller.ts b/packages/cli/src/modules/agents/agents-list.controller.ts index 5c1f7e712de..42615872d5a 100644 --- a/packages/cli/src/modules/agents/agents-list.controller.ts +++ b/packages/cli/src/modules/agents/agents-list.controller.ts @@ -11,8 +11,8 @@ import { parseThreadListLimit } from './utils/parse-thread-list-limit'; * Global (cross-project) agents list endpoint. * * It serves two audiences, and `availableInChat` picks between them: - * - the overview page, which wants every agent in the user's projects and the - * full entity to render its cards; + * - the overview page, which wants every agent in the projects where the user + * holds `agent:list` and the full entity to render its cards; * - the n8n chat page, which wants the agents whose published config carries * the n8n Chat channel, scoped to `agent:execute` and answered with the * narrow `AgentChatListItem` so a chat-only member gets no config. @@ -30,7 +30,7 @@ export class AgentsListController { async list(req: AuthenticatedRequest, res: Response, @Query query: ListAgentsQueryDto) { res.json( query.filter?.availableInChat === undefined - ? await this.agentsService.findByUserMembershipPaginated(req.user, query) + ? await this.agentsService.findListableByUserPaginated(req.user, query) : await this.agentsService.findChatReachableByUserPaginated(req.user, query), ); } diff --git a/packages/cli/src/modules/agents/agents.service.ts b/packages/cli/src/modules/agents/agents.service.ts index 75775c7bb5f..83291e5b795 100644 --- a/packages/cli/src/modules/agents/agents.service.ts +++ b/packages/cli/src/modules/agents/agents.service.ts @@ -470,13 +470,12 @@ export class AgentsService { return await this.agentExecutionService.findN8nChatThreadForAgents(user.id, agentIds, threadId); } - /** The agents overview list: every project the user belongs to, no scope check. */ - async findByUserMembershipPaginated( + /** The agents overview list: every project where the user holds `agent:list`. */ + async findListableByUserPaginated( user: User, options: ListAgentsQueryDto, ): Promise { - const projectRelations = await this.projectRelationRepository.findAllByUser(user.id); - const projectIds = projectRelations.map((pr) => pr.projectId); + const projectIds = await this.projectScopeService.getProjectIds(user, ['agent:list']); return await this.agentRepository.findByProjectIdsPaginated(projectIds, options); } diff --git a/packages/cli/src/modules/chat-hub/__tests__/chat-hub-agent.service.test.ts b/packages/cli/src/modules/chat-hub/__tests__/chat-hub-agent.service.test.ts index 09abac33f19..b10186d1eca 100644 --- a/packages/cli/src/modules/chat-hub/__tests__/chat-hub-agent.service.test.ts +++ b/packages/cli/src/modules/chat-hub/__tests__/chat-hub-agent.service.test.ts @@ -1,6 +1,7 @@ import type { ChatHubAgentKnowledgeItem } from '@n8n/api-types'; import type { Logger } from '@n8n/backend-common'; import type { User } from '@n8n/db'; +import type { EntityManager } from '@n8n/typeorm'; import type { IRunExecutionData, IWorkflowBase } from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; import { mock } from 'vitest-mock-extended'; @@ -80,6 +81,7 @@ describe('ChatHubAgentService', () => { const dynamicNodeParametersService = mock(); const toolService = mock(); const mockUser = mock({ id: mockUserId }); + const mockTrx = mock(); let service: ChatHubAgentService; @@ -99,6 +101,11 @@ describe('ChatHubAgentService', () => { dynamicNodeParametersService, toolService, ); + + agentRepository.runInTransaction.mockImplementation( + async (ctx: unknown, fn: (em: never, ctx: unknown) => Promise) => + await fn(mockTrx as never, ctx), + ); }); describe('createAgent', () => { @@ -129,6 +136,7 @@ describe('ChatHubAgentService', () => { model: 'gpt-4', ownerId: mockUserId, }), + mockTrx, ); expect(dto.id).toBe(agent.id); expect(dto.name).toBe('Test Agent'); @@ -153,6 +161,7 @@ describe('ChatHubAgentService', () => { expect(agentRepository.createAgent).toHaveBeenCalledWith( expect.objectContaining({ description: null }), + mockTrx, ); }); @@ -171,10 +180,34 @@ describe('ChatHubAgentService', () => { toolIds, }); - expect(toolService.setAgentTools).toHaveBeenCalledWith(expect.any(String), toolIds); + expect(toolService.setAgentTools).toHaveBeenCalledWith( + expect.any(String), + mockUserId, + toolIds, + mockTrx, + ); expect(dto.toolIds).toEqual(toolIds); }); + it('should create the agent and set its tools in one transaction', async () => { + agentRepository.createAgent.mockResolvedValue(makeAgent()); + toolService.setAgentTools.mockRejectedValue(new NotFoundError('Chat hub tool not found')); + + await expect( + service.createAgent(mockUser, { + name: 'Test Agent', + systemPrompt: 'You are helpful', + icon: { type: 'emoji', value: '🤖' }, + credentialId: 'cred-1', + provider: 'openai', + model: 'gpt-4', + toolIds: [uuid()], + }), + ).rejects.toThrow(NotFoundError); + + expect(agentRepository.createAgent).toHaveBeenCalledWith(expect.anything(), mockTrx); + }); + it('should not call setAgentTools when toolIds is empty', async () => { const agent = makeAgent(); agentRepository.createAgent.mockResolvedValue(agent); @@ -210,6 +243,7 @@ describe('ChatHubAgentService', () => { expect(agentRepository.createAgent).toHaveBeenCalledWith( expect.objectContaining({ suggestedPrompts: prompts }), + mockTrx, ); expect(dto.suggestedPrompts).toEqual(prompts); }); @@ -230,6 +264,7 @@ describe('ChatHubAgentService', () => { expect(agentRepository.createAgent).toHaveBeenCalledWith( expect.objectContaining({ suggestedPrompts: [] }), + mockTrx, ); }); }); @@ -273,7 +308,11 @@ describe('ChatHubAgentService', () => { await service.updateAgent(agent.id, mockUser, { name: 'New Name' }); - expect(agentRepository.updateAgent).toHaveBeenCalledWith(agent.id, { name: 'New Name' }); + expect(agentRepository.updateAgent).toHaveBeenCalledWith( + agent.id, + { name: 'New Name' }, + mockTrx, + ); }); it('should update multiple fields at once', async () => { @@ -294,11 +333,15 @@ describe('ChatHubAgentService', () => { systemPrompt: 'New prompt', }); - expect(agentRepository.updateAgent).toHaveBeenCalledWith(agent.id, { - name: 'Updated', - description: 'New desc', - systemPrompt: 'New prompt', - }); + expect(agentRepository.updateAgent).toHaveBeenCalledWith( + agent.id, + { + name: 'Updated', + description: 'New desc', + systemPrompt: 'New prompt', + }, + mockTrx, + ); expect(dto.name).toBe('Updated'); }); @@ -311,7 +354,24 @@ describe('ChatHubAgentService', () => { await service.updateAgent(agent.id, mockUser, { toolIds }); - expect(toolService.setAgentTools).toHaveBeenCalledWith(agent.id, toolIds); + expect(toolService.setAgentTools).toHaveBeenCalledWith( + agent.id, + mockUserId, + toolIds, + mockTrx, + ); + }); + + it('should not update the agent fields when the tools are rejected', async () => { + const agent = makeAgent(); + agentRepository.getOneById.mockResolvedValue(agent); + toolService.setAgentTools.mockRejectedValue(new NotFoundError('Chat hub tool not found')); + + await expect( + service.updateAgent(agent.id, mockUser, { name: 'New Name', toolIds: [uuid()] }), + ).rejects.toThrow(NotFoundError); + + expect(agentRepository.updateAgent).not.toHaveBeenCalled(); }); it('should not set tools when toolIds is not provided', async () => { @@ -339,6 +399,7 @@ describe('ChatHubAgentService', () => { expect(agentRepository.updateAgent).toHaveBeenCalledWith( agent.id, expect.objectContaining({ suggestedPrompts: prompts }), + mockTrx, ); expect(dto.suggestedPrompts).toEqual(prompts); }); @@ -354,6 +415,7 @@ describe('ChatHubAgentService', () => { expect(agentRepository.updateAgent).toHaveBeenCalledWith( agent.id, expect.not.objectContaining({ suggestedPrompts: expect.anything() }), + mockTrx, ); }); }); diff --git a/packages/cli/src/modules/chat-hub/__tests__/chat-hub-tool-references.integration.test.ts b/packages/cli/src/modules/chat-hub/__tests__/chat-hub-tool-references.integration.test.ts new file mode 100644 index 00000000000..8689af4d153 --- /dev/null +++ b/packages/cli/src/modules/chat-hub/__tests__/chat-hub-tool-references.integration.test.ts @@ -0,0 +1,307 @@ +import { mockInstance, testDb, testModules } from '@n8n/backend-test-utils'; +import type { User } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { NotFoundError } from '@n8n/errors'; +import { BinaryDataService } from 'n8n-core'; +import type { INode } from 'n8n-workflow'; + +import { saveCredential } from '@test-integration/db/credentials'; +import { createMember } from '@test-integration/db/users'; + +import { WorkflowExecutionService } from '../../../workflows/workflow-execution.service'; +import { ChatHubAgent } from '../chat-hub-agent.entity'; +import { ChatHubAgentRepository } from '../chat-hub-agent.repository'; +import { ChatHubAgentService } from '../chat-hub-agent.service'; +import { ChatHubCredentialsService } from '../chat-hub-credentials.service'; +import { ChatHubSession } from '../chat-hub-session.entity'; +import type { ChatHubTool } from '../chat-hub-tool.entity'; +import { ChatHubToolRepository } from '../chat-hub-tool.repository'; +import { ChatHubToolService } from '../chat-hub-tool.service'; +import { ChatHubService } from '../chat-hub.service'; +import { ChatHubSessionRepository } from '../chat-session.repository'; + +mockInstance(BinaryDataService); +mockInstance(WorkflowExecutionService); +mockInstance(ChatHubCredentialsService); + +beforeAll(async () => { + await testModules.loadModules(['chat-hub']); + await testDb.init(); +}); + +beforeEach(async () => { + await testDb.truncate([ + 'ChatHubSession', + 'ChatHubAgent', + 'ChatHubTool', + 'SharedCredentials', + 'CredentialsEntity', + ]); +}); + +afterAll(async () => { + await testDb.terminate(); +}); + +describe('chat hub tool references', () => { + let chatHubService: ChatHubService; + let agentService: ChatHubAgentService; + let toolService: ChatHubToolService; + let toolRepository: ChatHubToolRepository; + let sessionRepository: ChatHubSessionRepository; + let agentRepository: ChatHubAgentRepository; + + let owner: User; + let otherUser: User; + + beforeAll(() => { + chatHubService = Container.get(ChatHubService); + agentService = Container.get(ChatHubAgentService); + toolService = Container.get(ChatHubToolService); + toolRepository = Container.get(ChatHubToolRepository); + sessionRepository = Container.get(ChatHubSessionRepository); + agentRepository = Container.get(ChatHubAgentRepository); + }); + + beforeEach(async () => { + owner = await createMember(); + otherUser = await createMember(); + }); + + async function createTool(ownerId: string) { + const id = crypto.randomUUID(); + const definition: INode = { + id, + name: `Tool ${id}`, + type: 'n8n-nodes-base.httpRequestTool', + typeVersion: 4.4, + position: [0, 0], + parameters: {}, + }; + return await toolRepository.createTool({ + id, + name: definition.name, + type: definition.type, + typeVersion: definition.typeVersion, + ownerId, + definition, + enabled: true, + }); + } + + async function createSession(ownerId: string) { + return await sessionRepository.createChatSession({ + id: crypto.randomUUID(), + ownerId, + title: 'session', + lastMessageAt: new Date(), + }); + } + + async function createAgent(ownerId: string) { + return await agentRepository.createAgent({ + id: crypto.randomUUID(), + name: 'agent', + systemPrompt: 'prompt', + ownerId, + credentialId: null, + provider: 'openai', + model: 'gpt-4', + files: [], + }); + } + + /** Reads every stored link row, without the owner filter that the services apply */ + async function storedLinkIds(entity: typeof ChatHubSession | typeof ChatHubAgent, id: string) { + const tools = await toolRepository.manager + .createQueryBuilder() + .relation(entity, 'tools') + .of(id) + .loadMany(); + return tools.map((t) => t.id).sort(); + } + + describe('updateSession', () => { + it('should set tools that the user owns', async () => { + const session = await createSession(owner.id); + const tool = await createTool(owner.id); + + await chatHubService.updateSession(owner, session.id, { toolIds: [tool.id] }); + + expect(await toolService.getToolIdsForSession(session.id)).toEqual([tool.id]); + }); + + it('should reject tools that another user owns and keep the session unchanged', async () => { + const session = await createSession(owner.id); + const ownTool = await createTool(owner.id); + const foreignTool = await createTool(otherUser.id); + await chatHubService.updateSession(owner, session.id, { toolIds: [ownTool.id] }); + + await expect( + chatHubService.updateSession(owner, session.id, { + title: 'renamed', + toolIds: [ownTool.id, foreignTool.id], + }), + ).rejects.toThrow(NotFoundError); + + expect(await toolService.getToolIdsForSession(session.id)).toEqual([ownTool.id]); + const stored = await sessionRepository.findOneByOrFail({ id: session.id }); + expect(stored.title).toBe('session'); + }); + + it('should accept duplicate IDs of an owned tool', async () => { + const session = await createSession(owner.id); + const tool = await createTool(owner.id); + + await chatHubService.updateSession(owner, session.id, { toolIds: [tool.id, tool.id] }); + + expect(await toolService.getToolIdsForSession(session.id)).toEqual([tool.id]); + }); + + it('should keep the tool links unchanged when the session update fails', async () => { + const session = await createSession(owner.id); + const tool = await createTool(owner.id); + + await expect( + chatHubService.updateSession(owner, session.id, { + credentialId: crypto.randomUUID(), + toolIds: [tool.id], + }), + ).rejects.toThrow(); + + expect(await storedLinkIds(ChatHubSession, session.id)).toEqual([]); + }); + + it('should only return owned tool IDs and remove other links on the next save', async () => { + const session = await createSession(owner.id); + const ownTool = await createTool(owner.id); + const foreignTool = await createTool(otherUser.id); + // Write the link rows directly to simulate rows stored before validation existed + await toolRepository.setSessionTools(session.id, [ownTool.id, foreignTool.id]); + + const { session: dto } = await chatHubService.getConversation(owner.id, session.id); + expect(dto.toolIds).toEqual([ownTool.id]); + + await chatHubService.updateSession(owner, session.id, { toolIds: dto.toolIds }); + + expect(await storedLinkIds(ChatHubSession, session.id)).toEqual([ownTool.id]); + }); + }); + + describe('createAgent', () => { + async function agentRequest() { + const credential = await saveCredential( + { name: 'OpenAI', type: 'openAiApi', data: { apiKey: 'test-api-key' } }, + { user: owner, role: 'credential:owner' }, + ); + return { + name: 'agent', + systemPrompt: 'prompt', + icon: { type: 'emoji' as const, value: '🤖' }, + credentialId: credential.id, + provider: 'openai' as const, + model: 'gpt-4', + }; + } + + it('should set tools that the user owns', async () => { + const tool = await createTool(owner.id); + + const agent = await agentService.createAgent(owner, { + ...(await agentRequest()), + toolIds: [tool.id], + }); + + expect(await toolService.getToolIdsForAgent(agent.id)).toEqual([tool.id]); + }); + + it('should reject tools that another user owns and not create the agent', async () => { + const foreignTool = await createTool(otherUser.id); + + await expect( + agentService.createAgent(owner, { ...(await agentRequest()), toolIds: [foreignTool.id] }), + ).rejects.toThrow(NotFoundError); + + expect(await agentRepository.getManyByUserId(owner.id)).toHaveLength(0); + }); + + it('should return each tool ID once when the request repeats an ID', async () => { + const tool = await createTool(owner.id); + + const agent = await agentService.createAgent(owner, { + ...(await agentRequest()), + toolIds: [tool.id, tool.id], + }); + + expect(agent.toolIds).toEqual([tool.id]); + }); + }); + + describe('updateAgent', () => { + it('should reject tools that another user owns and keep the agent unchanged', async () => { + const agent = await createAgent(owner.id); + const foreignTool = await createTool(otherUser.id); + + await expect( + agentService.updateAgent(agent.id, owner, { name: 'renamed', toolIds: [foreignTool.id] }), + ).rejects.toThrow(NotFoundError); + + expect(await toolService.getToolIdsForAgent(agent.id)).toEqual([]); + const stored = await agentRepository.findOneByOrFail({ id: agent.id }); + expect(stored.name).toBe('agent'); + }); + + it('should keep the tool links unchanged when the agent update fails', async () => { + const agent = await createAgent(owner.id); + const tool = await createTool(owner.id); + + await expect( + agentService.updateAgent(agent.id, owner, { + credentialId: crypto.randomUUID(), + toolIds: [tool.id], + }), + ).rejects.toThrow(); + + expect(await storedLinkIds(ChatHubAgent, agent.id)).toEqual([]); + }); + + it('should only return owned tool IDs and remove other links on the next save', async () => { + const agent = await createAgent(owner.id); + const ownTool = await createTool(owner.id); + const foreignTool = await createTool(otherUser.id); + await toolRepository.setAgentTools(agent.id, [ownTool.id, foreignTool.id]); + + const dto = await agentService.getAgentByIdAsDto(agent.id, owner.id); + expect(dto.toolIds).toEqual([ownTool.id]); + + await agentService.updateAgent(agent.id, owner, { toolIds: dto.toolIds }); + + expect(await storedLinkIds(ChatHubAgent, agent.id)).toEqual([ownTool.id]); + }); + }); + + describe('tool definitions at run time', () => { + it('should only return session tools that the session owner owns', async () => { + const session = await createSession(owner.id); + const ownTool = await createTool(owner.id); + const foreignTool = await createTool(otherUser.id); + // Write the link rows directly to simulate rows stored before validation existed + await toolRepository.setSessionTools(session.id, [ownTool.id, foreignTool.id]); + + const definitions = await toolService.getToolDefinitionsForSession(session.id); + + expect(definitions.map((d) => d.id)).toEqual([ownTool.id]); + }); + + it('should only return agent tools that the agent owner owns', async () => { + const agent = await createAgent(owner.id); + const ownTool = await createTool(owner.id); + const foreignTool = await createTool(otherUser.id); + await toolRepository.setAgentTools(agent.id, [ownTool.id, foreignTool.id]); + + const definitions = await toolService.getToolDefinitionsForAgent(agent.id); + + expect(definitions.map((d) => d.id)).toEqual([ownTool.id]); + }); + }); +}); diff --git a/packages/cli/src/modules/chat-hub/__tests__/chat-hub-tool.service.test.ts b/packages/cli/src/modules/chat-hub/__tests__/chat-hub-tool.service.test.ts index ed464b82a84..abf961c3db4 100644 --- a/packages/cli/src/modules/chat-hub/__tests__/chat-hub-tool.service.test.ts +++ b/packages/cli/src/modules/chat-hub/__tests__/chat-hub-tool.service.test.ts @@ -136,6 +136,85 @@ describe('ChatHubToolService', () => { }); }); + describe('setSessionTools', () => { + it('should set tools that the user owns', async () => { + const tool = makeTool(); + chatToolRepository.getByIds.mockResolvedValue([tool]); + + await service.setSessionTools(mockSessionId, mockUserId, [tool.id]); + + expect(chatToolRepository.setSessionTools).toHaveBeenCalledWith( + mockSessionId, + [tool.id], + undefined, + ); + }); + + it('should not set tools when the user does not own one of them', async () => { + chatToolRepository.getByIds.mockResolvedValue([]); + + await expect(service.setSessionTools(mockSessionId, mockUserId, [uuid()])).rejects.toThrow( + NotFoundError, + ); + expect(chatToolRepository.setSessionTools).not.toHaveBeenCalled(); + }); + + it('should not set tools when the user owns only some of them', async () => { + const ownTool = makeTool(); + chatToolRepository.getByIds.mockResolvedValue([ownTool]); + + await expect( + service.setSessionTools(mockSessionId, mockUserId, [ownTool.id, uuid()]), + ).rejects.toThrow(NotFoundError); + expect(chatToolRepository.setSessionTools).not.toHaveBeenCalled(); + }); + + it('should check and store each tool once when IDs repeat', async () => { + const tool = makeTool(); + chatToolRepository.getByIds.mockResolvedValue([tool]); + + await service.setSessionTools(mockSessionId, mockUserId, [tool.id, tool.id]); + + expect(chatToolRepository.getByIds).toHaveBeenCalledWith([tool.id], mockUserId, undefined); + expect(chatToolRepository.setSessionTools).toHaveBeenCalledWith( + mockSessionId, + [tool.id], + undefined, + ); + }); + + it('should clear the tools without a lookup when the list is empty', async () => { + await service.setSessionTools(mockSessionId, mockUserId, []); + + expect(chatToolRepository.getByIds).not.toHaveBeenCalled(); + expect(chatToolRepository.setSessionTools).toHaveBeenCalledWith(mockSessionId, [], undefined); + }); + }); + + describe('setAgentTools', () => { + it('should set tools that the user owns', async () => { + const tool = makeTool(); + chatToolRepository.getByIds.mockResolvedValue([tool]); + + await service.setAgentTools(mockAgentId, mockUserId, [tool.id]); + + expect(chatToolRepository.setAgentTools).toHaveBeenCalledWith( + mockAgentId, + [tool.id], + undefined, + ); + }); + + it('should not set tools when the user does not own one of them', async () => { + chatToolRepository.getByIds.mockResolvedValue([]); + + await expect(service.setAgentTools(mockAgentId, mockUserId, [uuid()])).rejects.toThrow( + NotFoundError, + ); + expect(chatToolRepository.setAgentTools).not.toHaveBeenCalled(); + }); + }); + const deprecatedCodeTool: INode = { ...mockDefinition, type: '@n8n/n8n-nodes-langchain.code', diff --git a/packages/cli/src/modules/chat-hub/__tests__/chat-hub-workflow.service.test.ts b/packages/cli/src/modules/chat-hub/__tests__/chat-hub-workflow.service.test.ts index 86d98215bf4..19617de66de 100644 --- a/packages/cli/src/modules/chat-hub/__tests__/chat-hub-workflow.service.test.ts +++ b/packages/cli/src/modules/chat-hub/__tests__/chat-hub-workflow.service.test.ts @@ -1,6 +1,7 @@ +import type { ChatHubConversationModel, ChatProviderSettingsDto } from '@n8n/api-types'; import type { PolicyCleared } from '@n8n/decorators'; import type { Logger } from '@n8n/backend-common'; -import type { WorkflowRepository, SharedWorkflowRepository, User } from '@n8n/db'; +import type { Project, WorkflowRepository, SharedWorkflowRepository, User } from '@n8n/db'; import type { EntityManager } from '@n8n/typeorm'; import type { Cipher, BinaryDataService } from 'n8n-core'; import { type IBinaryData, type INode, CHAT_TRIGGER_NODE_TYPE } from 'n8n-workflow'; @@ -1409,7 +1410,7 @@ describe('ChatHubWorkflowService', () => { vi.spyOn(serviceWithRepo, 'getSystemMessageMetadata').mockReturnValue('__metadata__'); const result = await serviceWithRepo.prepareReplyWorkflow( - { id: 'user-123' } as User, + { id: 'user-123', role: { slug: 'global:member' } } as User, 'session-456' as any, {}, { provider: 'custom-agent', agentId: 'agent-1' } as any, @@ -1508,4 +1509,78 @@ __metadata__" `); }); }); + + describe('tool type authorization', () => { + const toolNode = (name: string, type: string): INode => ({ + id: `id-${name}`, + name, + type, + typeVersion: 1, + position: [0, 0], + parameters: {}, + }); + + const storedTools = [ + toolNode('Shell', 'n8n-nodes-base.executeCommandTool'), + toolNode('Code', '@n8n/n8n-nodes-langchain.toolCode'), + toolNode('Slack', 'n8n-nodes-base.slackTool'), + ]; + + beforeEach(() => { + chatHubSettingsService.ensureModelIsAllowed.mockResolvedValue(undefined); + chatHubSettingsService.getProviderSettings.mockResolvedValue(mock()); + chatHubSettingsService.getSemanticSearchOptions.mockResolvedValue(null); + chatHubCredentialsService.findPersonalProject.mockResolvedValue( + mock({ id: 'project-789' }), + ); + }); + + async function getToolNodeNames(user: User, model: ChatHubConversationModel) { + const result = await service.prepareReplyWorkflow( + user, + 'session-456', + { openAiApi: { id: 'cred-1', name: '' } }, + model, + [], + 'Hello', + storedTools, + [], + 'UTC', + {}, + defaultExecutionMetadata, + ); + const toolNames = new Set(storedTools.map((t) => t.name)); + return result.workflowData.nodes.map((n) => n.name).filter((name) => toolNames.has(name)); + } + + const chatUser = { id: 'user-123', role: { slug: 'global:chatUser' } } as User; + const member = { id: 'user-123', role: { slug: 'global:member' } } as User; + const baseModel: ChatHubConversationModel = { provider: 'openai', model: 'gpt-4' }; + + it('should drop stored tools that a chat user is not allowed to run', async () => { + expect(await getToolNodeNames(chatUser, baseModel)).toEqual(['Slack']); + }); + + it('should keep all stored tools for a member', async () => { + expect(await getToolNodeNames(member, baseModel)).toEqual(['Shell', 'Code', 'Slack']); + }); + + it('should drop blocked tools from a chat user agent', async () => { + chatHubAgentRepository.getOneById.mockResolvedValue( + mock({ + id: 'agent-1', + provider: 'openai', + model: 'gpt-4', + credentialId: 'cred-1', + systemPrompt: '', + files: [], + }), + ); + chatHubToolService.getToolDefinitionsForAgent.mockResolvedValue(storedTools); + + expect( + await getToolNodeNames(chatUser, { provider: 'custom-agent', agentId: 'agent-1' }), + ).toEqual(['Slack']); + }); + }); }); diff --git a/packages/cli/src/modules/chat-hub/__tests__/chat-hub.controller.test.ts b/packages/cli/src/modules/chat-hub/__tests__/chat-hub.controller.test.ts index fd04514a378..366653849f7 100644 --- a/packages/cli/src/modules/chat-hub/__tests__/chat-hub.controller.test.ts +++ b/packages/cli/src/modules/chat-hub/__tests__/chat-hub.controller.test.ts @@ -1,11 +1,15 @@ +import type { ChatHubCreateToolRequest, ChatHubUpdateToolRequest } from '@n8n/api-types'; import type { ModuleRegistry } from '@n8n/backend-common'; +import type { AuthenticatedRequest } from '@n8n/db'; import type { ModuleSettings } from '@n8n/decorators'; import type { NextFunction, Request, Response } from 'express'; +import type { INode } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; -import { ForbiddenError } from '@n8n/errors'; +import { BadRequestError, ForbiddenError } from '@n8n/errors'; import { sendErrorResponse } from '@/response-helper'; +import type { ChatHubToolService } from '../chat-hub-tool.service'; import { ChatHubController } from '../chat-hub.controller'; vi.mock('@/response-helper'); @@ -13,7 +17,15 @@ vi.mock('@/response-helper'); describe('ChatHubController', () => { const settingsMap = new Map(); const moduleRegistry = { settings: settingsMap } as unknown as ModuleRegistry; - const controller = new ChatHubController(mock(), mock(), mock(), mock(), mock(), moduleRegistry); + const chatToolService = mock(); + const controller = new ChatHubController( + mock(), + mock(), + mock(), + chatToolService, + mock(), + moduleRegistry, + ); const req = mock(); const res = mock(); @@ -51,4 +63,81 @@ describe('ChatHubController', () => { expect(sendErrorResponse).toHaveBeenCalledWith(res, expect.any(ForbiddenError)); }); }); + + describe('tool type authorization', () => { + const chatUserReq = mock({ + user: { id: 'chat-user', role: { slug: 'global:chatUser' } }, + }); + const memberReq = mock({ + user: { id: 'member', role: { slug: 'global:member' } }, + }); + + const toolDefinition = (type: string): INode => ({ + id: '00000000-0000-4000-8000-000000000001', + name: 'Tool', + type, + typeVersion: 1, + position: [0, 0], + parameters: {}, + }); + + const createPayload = (type: string) => + ({ definition: toolDefinition(type) }) as ChatHubCreateToolRequest; + const updatePayload = (type: string) => + ({ definition: toolDefinition(type) }) as ChatHubUpdateToolRequest; + + beforeEach(() => { + chatToolService.createTool.mockResolvedValue(mock({ definition: toolDefinition('x') })); + chatToolService.updateTool.mockResolvedValue(mock({ definition: toolDefinition('x') })); + }); + + it.each([ + 'n8n-nodes-base.executeCommandTool', + 'n8n-nodes-base.gitTool', + '@n8n/n8n-nodes-langchain.toolCode', + 'n8n-nodes-base.messageAnAgentTool', + ])('should reject creating a %s tool for a chat user', async (type) => { + await expect(controller.createTool(chatUserReq, res, createPayload(type))).rejects.toThrow( + BadRequestError, + ); + expect(chatToolService.createTool).not.toHaveBeenCalled(); + }); + + it('should reject updating a tool to a blocked type for a chat user', async () => { + await expect( + controller.updateTool( + chatUserReq, + res, + 'tool-1', + updatePayload('n8n-nodes-base.executeCommandTool'), + ), + ).rejects.toThrow(BadRequestError); + expect(chatToolService.updateTool).not.toHaveBeenCalled(); + }); + + it.each(['n8n-nodes-base.slackTool', 'n8n-nodes-base.httpRequestTool'])( + 'should allow creating a %s tool for a chat user', + async (type) => { + await controller.createTool(chatUserReq, res, createPayload(type)); + + expect(chatToolService.createTool).toHaveBeenCalled(); + }, + ); + + it('should allow creating a Code tool for a member', async () => { + await controller.createTool( + memberReq, + res, + createPayload('@n8n/n8n-nodes-langchain.toolCode'), + ); + + expect(chatToolService.createTool).toHaveBeenCalled(); + }); + + it('should reject creating a Chat tool for a member', async () => { + await expect( + controller.createTool(memberReq, res, createPayload('@n8n/n8n-nodes-langchain.chatTool')), + ).rejects.toThrow(BadRequestError); + }); + }); }); diff --git a/packages/cli/src/modules/chat-hub/chat-hub-agent.service.ts b/packages/cli/src/modules/chat-hub/chat-hub-agent.service.ts index 49e8580ed04..4b9f6e39bea 100644 --- a/packages/cli/src/modules/chat-hub/chat-hub-agent.service.ts +++ b/packages/cli/src/modules/chat-hub/chat-hub-agent.service.ts @@ -98,26 +98,34 @@ export class ChatHubAgentService { const id = uuidv4(); - const agent = await this.chatAgentRepository.createAgent({ - id, - name: data.name, - description: data.description ?? null, - icon: data.icon, - suggestedPrompts: data.suggestedPrompts ?? [], - systemPrompt: data.systemPrompt, - ownerId: user.id, - credentialId: data.credentialId, - provider: data.provider, - model: data.model, - files: [], + // One transaction: a rejected tool removes the new agent again + const agent = await this.chatAgentRepository.runInTransaction({}, async (trx) => { + const created = await this.chatAgentRepository.createAgent( + { + id, + name: data.name, + description: data.description ?? null, + icon: data.icon, + suggestedPrompts: data.suggestedPrompts ?? [], + systemPrompt: data.systemPrompt, + ownerId: user.id, + credentialId: data.credentialId, + provider: data.provider, + model: data.model, + files: [], + }, + trx, + ); + + if (data.toolIds.length > 0) { + await this.chatHubToolService.setAgentTools(id, user.id, data.toolIds, trx); + } + + return created; }); - if (data.toolIds.length > 0) { - await this.chatHubToolService.setAgentTools(id, data.toolIds); - } - this.logger.debug(`Chat agent created: ${id} by user ${user.id}`); - return this.toDto(agent, data.toolIds); + return this.toDto(agent, [...new Set(data.toolIds)]); } async updateAgent( @@ -148,11 +156,14 @@ export class ChatHubAgentService { if (updates.provider !== undefined) updateData.provider = updates.provider; if (updates.model !== undefined) updateData.model = updates.model ?? null; - const agent = await this.chatAgentRepository.updateAgent(id, updateData); - - if (updates.toolIds !== undefined) { - await this.chatHubToolService.setAgentTools(id, updates.toolIds); - } + // One transaction: a rejected tool or a failed field update leaves the agent unchanged + const { toolIds: newToolIds } = updates; + const agent = await this.chatAgentRepository.runInTransaction({}, async (trx) => { + if (newToolIds !== undefined) { + await this.chatHubToolService.setAgentTools(id, user.id, newToolIds, trx); + } + return await this.chatAgentRepository.updateAgent(id, updateData, trx); + }); this.logger.debug(`Chat agent updated: ${id} by user ${user.id}`); const toolIds = await this.chatHubToolService.getToolIdsForAgent(agent.id); diff --git a/packages/cli/src/modules/chat-hub/chat-hub-tool.repository.ts b/packages/cli/src/modules/chat-hub/chat-hub-tool.repository.ts index a357ae04971..50990b799f1 100644 --- a/packages/cli/src/modules/chat-hub/chat-hub-tool.repository.ts +++ b/packages/cli/src/modules/chat-hub/chat-hub-tool.repository.ts @@ -69,7 +69,7 @@ export class ChatHubToolRepository extends Repository { const em = trx ?? this.manager; const session = await em .createQueryBuilder(ChatHubSession, 'session') - .innerJoinAndSelect('session.tools', 'tool') + .innerJoinAndSelect('session.tools', 'tool', 'tool.ownerId = session.ownerId') .where('session.id = :sessionId', { sessionId }) .orderBy('tool.createdAt', 'ASC') .getOne(); @@ -80,7 +80,7 @@ export class ChatHubToolRepository extends Repository { const em = trx ?? this.manager; const agent = await em .createQueryBuilder(ChatHubAgent, 'agent') - .innerJoinAndSelect('agent.tools', 'tool') + .innerJoinAndSelect('agent.tools', 'tool', 'tool.ownerId = agent.ownerId') .where('agent.id = :agentId', { agentId }) .orderBy('tool.createdAt', 'ASC') .getOne(); @@ -91,7 +91,7 @@ export class ChatHubToolRepository extends Repository { const em = trx ?? this.manager; const rows = await em .createQueryBuilder(ChatHubSession, 'session') - .innerJoin('session.tools', 'tool') + .innerJoin('session.tools', 'tool', 'tool.ownerId = session.ownerId') .select('tool.id', 'toolId') .where('session.id = :sessionId', { sessionId }) .getRawMany<{ toolId: string }>(); @@ -102,7 +102,7 @@ export class ChatHubToolRepository extends Repository { const em = trx ?? this.manager; const rows = await em .createQueryBuilder(ChatHubAgent, 'agent') - .innerJoin('agent.tools', 'tool') + .innerJoin('agent.tools', 'tool', 'tool.ownerId = agent.ownerId') .select('tool.id', 'toolId') .where('agent.id = :agentId', { agentId }) .getRawMany<{ toolId: string }>(); @@ -110,22 +110,28 @@ export class ChatHubToolRepository extends Repository { } async setSessionTools(sessionId: string, toolIds: string[], trx?: EntityManager): Promise { - const em = trx ?? this.manager; - const currentToolIds = await this.getToolIdsForSession(sessionId, em); - await em - .createQueryBuilder() - .relation(ChatHubSession, 'tools') - .of(sessionId) - .addAndRemove(toolIds, currentToolIds); + await this.replaceLinkedTools(ChatHubSession, sessionId, toolIds, trx ?? this.manager); } async setAgentTools(agentId: string, toolIds: string[], trx?: EntityManager): Promise { - const em = trx ?? this.manager; - const currentToolIds = await this.getToolIdsForAgent(agentId, em); - await em - .createQueryBuilder() - .relation(ChatHubAgent, 'tools') - .of(agentId) - .addAndRemove(toolIds, currentToolIds); + await this.replaceLinkedTools(ChatHubAgent, agentId, toolIds, trx ?? this.manager); + } + + /** + * Replaces every link row, including rows to tools that the owner does not own, + * so that a save also removes those rows. + */ + private async replaceLinkedTools( + entity: typeof ChatHubSession | typeof ChatHubAgent, + id: string, + toolIds: string[], + em: EntityManager, + ): Promise { + const relation = em.createQueryBuilder().relation(entity, 'tools').of(id); + const currentTools = await relation.loadMany(); + await relation.addAndRemove( + toolIds, + currentTools.map((t) => t.id), + ); } } diff --git a/packages/cli/src/modules/chat-hub/chat-hub-tool.service.ts b/packages/cli/src/modules/chat-hub/chat-hub-tool.service.ts index 4291a553a88..85ae6018783 100644 --- a/packages/cli/src/modules/chat-hub/chat-hub-tool.service.ts +++ b/packages/cli/src/modules/chat-hub/chat-hub-tool.service.ts @@ -53,12 +53,41 @@ export class ChatHubToolService { return await this.chatToolRepository.getToolIdsForAgent(agentId, trx); } - async setSessionTools(sessionId: string, toolIds: string[], trx?: EntityManager): Promise { - await this.chatToolRepository.setSessionTools(sessionId, toolIds, trx); + async setSessionTools( + sessionId: string, + ownerId: string, + toolIds: string[], + trx?: EntityManager, + ): Promise { + await this.ensureToolsOwnedBy(ownerId, toolIds, trx); + await this.chatToolRepository.setSessionTools(sessionId, [...new Set(toolIds)], trx); } - async setAgentTools(agentId: string, toolIds: string[], trx?: EntityManager): Promise { - await this.chatToolRepository.setAgentTools(agentId, toolIds, trx); + async setAgentTools( + agentId: string, + ownerId: string, + toolIds: string[], + trx?: EntityManager, + ): Promise { + await this.ensureToolsOwnedBy(ownerId, toolIds, trx); + await this.chatToolRepository.setAgentTools(agentId, [...new Set(toolIds)], trx); + } + + /** + * Throws `NotFoundError` unless the user owns every tool in `toolIds`. + */ + private async ensureToolsOwnedBy( + userId: string, + toolIds: string[], + trx?: EntityManager, + ): Promise { + const uniqueIds = [...new Set(toolIds)]; + if (uniqueIds.length === 0) return; + + const ownedTools = await this.chatToolRepository.getByIds(uniqueIds, userId, trx); + if (ownedTools.length !== uniqueIds.length) { + throw new NotFoundError('Chat hub tool not found'); + } } private validateToolExpressions(definition: INode) { diff --git a/packages/cli/src/modules/chat-hub/chat-hub-workflow.service.ts b/packages/cli/src/modules/chat-hub/chat-hub-workflow.service.ts index bc804c08f61..4e714db8c49 100644 --- a/packages/cli/src/modules/chat-hub/chat-hub-workflow.service.ts +++ b/packages/cli/src/modules/chat-hub/chat-hub-workflow.service.ts @@ -5,6 +5,7 @@ import { type ChatHubBaseLLMModel, type ChatProviderSettingsDto, type ChatHubAgentKnowledgeItem, + isChatHubToolTypeAllowed, } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; import { @@ -1361,7 +1362,7 @@ Respond the title only:`, credentials, model, systemMessage, - tools, + this.filterAllowedTools(user, tools), timeZone, vectorStoreSearch, executionMetadata, @@ -1371,6 +1372,22 @@ Respond the title only:`, }); } + /** + * Stored tools can predate a tool type rule or a role change, so check them + * again before each run. + */ + private filterAllowedTools(user: User, tools: INode[]): INode[] { + const isChatUser = user.role.slug === 'global:chatUser'; + return tools.filter((tool) => { + if (isChatHubToolTypeAllowed(tool.type, { isChatUser })) return true; + this.logger.warn('Skipped a Chat Hub tool that the user is not allowed to run', { + userId: user.id, + toolType: tool.type, + }); + return false; + }); + } + private async prepareChatAgentWorkflow( agentId: string, user: User, diff --git a/packages/cli/src/modules/chat-hub/chat-hub.controller.ts b/packages/cli/src/modules/chat-hub/chat-hub.controller.ts index d77ed8bf615..d4c5cc0f61f 100644 --- a/packages/cli/src/modules/chat-hub/chat-hub.controller.ts +++ b/packages/cli/src/modules/chat-hub/chat-hub.controller.ts @@ -20,8 +20,7 @@ import { type ChatSendMessageResponse, type ChatReconnectResponse, ChatReconnectRequest, - ALWAYS_BLOCKED_CHAT_HUB_TOOL_TYPES, - CHAT_USER_BLOCKED_CHAT_HUB_TOOL_TYPES, + isChatHubToolTypeAllowed, } from '@n8n/api-types'; import { ModuleRegistry } from '@n8n/backend-common'; import { AuthenticatedRequest } from '@n8n/db'; @@ -518,13 +517,10 @@ export class ChatHubController { } private assertToolTypeAllowed(type: string, user: AuthenticatedRequest['user']) { - if (ALWAYS_BLOCKED_CHAT_HUB_TOOL_TYPES.includes(type)) { + if (!isChatHubToolTypeAllowed(type, { isChatUser: false })) { throw new BadRequestError(`Tool type "${type}" is not supported in the Chat Hub`); } - if ( - user.role.slug === 'global:chatUser' && - CHAT_USER_BLOCKED_CHAT_HUB_TOOL_TYPES.includes(type) - ) { + if (!isChatHubToolTypeAllowed(type, { isChatUser: user.role.slug === 'global:chatUser' })) { throw new BadRequestError(`Tool type "${type}" is not available for your role`); } } diff --git a/packages/cli/src/modules/chat-hub/chat-hub.service.ts b/packages/cli/src/modules/chat-hub/chat-hub.service.ts index e3111197e1b..93bf9df951c 100644 --- a/packages/cli/src/modules/chat-hub/chat-hub.service.ts +++ b/packages/cli/src/modules/chat-hub/chat-hub.service.ts @@ -240,7 +240,7 @@ export class ChatHubService { const enabledTools = await this.chatHubToolService.getEnabledTools(user.id, trx); const toolIds = enabledTools.map((t) => t.id); if (toolIds.length > 0) { - await this.chatHubToolService.setSessionTools(sessionId, toolIds, trx); + await this.chatHubToolService.setSessionTools(sessionId, user.id, toolIds, trx); } return session; @@ -378,11 +378,14 @@ export class ChatHubService { if (updates.title !== undefined) sessionUpdates.title = updates.title; if (updates.credentialId !== undefined) sessionUpdates.credentialId = updates.credentialId; - await this.sessionRepository.updateChatSession(sessionId, sessionUpdates); - - if (updates.toolIds !== undefined) { - await this.chatHubToolService.setSessionTools(sessionId, updates.toolIds); - } + // One transaction: a rejected tool or a failed field update leaves the session unchanged + const { toolIds } = updates; + await this.sessionRepository.runInTransaction({}, async (trx) => { + if (toolIds !== undefined) { + await this.chatHubToolService.setSessionTools(sessionId, user.id, toolIds, trx); + } + await this.sessionRepository.updateChatSession(sessionId, sessionUpdates, trx); + }); } /** diff --git a/packages/cli/src/modules/data-table/__tests__/data-table-proxy.service.integration.test.ts b/packages/cli/src/modules/data-table/__tests__/data-table-proxy.service.integration.test.ts index 107420f6a4b..257750d7004 100644 --- a/packages/cli/src/modules/data-table/__tests__/data-table-proxy.service.integration.test.ts +++ b/packages/cli/src/modules/data-table/__tests__/data-table-proxy.service.integration.test.ts @@ -1,6 +1,7 @@ import type { ListDataTableQueryDto } from '@n8n/api-types'; import type { Logger } from '@n8n/backend-common'; -import { createTeamProject, testDb, testModules } from '@n8n/backend-test-utils'; +import { CacheService } from '@n8n/backend-services'; +import { createTeamProject, createWorkflow, testDb, testModules } from '@n8n/backend-test-utils'; import type { Project, User } from '@n8n/db'; import { Container } from '@n8n/di'; import type { @@ -17,7 +18,7 @@ import { mock } from 'vitest-mock-extended'; import type { InstanceWriteAccessService } from '@n8n/backend-services'; import * as checkAccess from '@/permissions.ee/check-access'; -import type { OwnershipService } from '@/services/ownership.service'; +import { OwnershipService } from '@/services/ownership.service'; import type { DataTableAggregateService } from '../data-table-aggregate.service'; import { DataTableProxyService } from '../data-table-proxy.service'; @@ -299,7 +300,15 @@ describe('DataTableProxyService (with database)', () => { const workflow = mock({ id: 'workflow-id' }); afterEach(async () => { - await testDb.truncate(['DataTable', 'DataTableColumn', 'Project', 'ProjectRelation']); + await testDb.truncate([ + 'DataTable', + 'DataTableColumn', + 'SharedWorkflow', + 'WorkflowEntity', + 'Project', + 'ProjectRelation', + ]); + await Container.get(CacheService).delete('workflow-project'); }); it('resolves the proxy when the data table exists in the project', async () => { @@ -332,6 +341,29 @@ describe('DataTableProxyService (with database)', () => { await expect(promise).rejects.toBeInstanceOf(NodeOperationError); await expect(promise).rejects.toThrow(table.id); }); + + it('rejects a table from another project for a workflow with the id `__proto__`', async () => { + const workflowProject = await createTeamProject(); + const otherProject = await createTeamProject(); + const table = await Container.get(DataTableService).createDataTable(otherProject.id, { + name: 'Other Project Table', + columns: [], + }); + await createWorkflow({ id: '__proto__' }, workflowProject); + // The ownership cache must already exist, as it does on any instance that ran a workflow + await Container.get(CacheService).setHash('workflow-project', { + 'other-workflow': Container.get(OwnershipService).copyProject(workflowProject), + }); + + const promise = Container.get(DataTableProxyService).getDataTableProxy( + mock({ id: '__proto__' }), + node, + table.id, + ); + + await expect(promise).rejects.toBeInstanceOf(NodeOperationError); + await expect(promise).rejects.toThrow(table.id); + }); }); describe('makeDataTableOperationsForUser', () => { diff --git a/packages/cli/src/modules/mcp/__tests__/execution-tools.scopes.integration.test.ts b/packages/cli/src/modules/mcp/__tests__/execution-tools.scopes.integration.test.ts new file mode 100644 index 00000000000..a0b0ba6673f --- /dev/null +++ b/packages/cli/src/modules/mcp/__tests__/execution-tools.scopes.integration.test.ts @@ -0,0 +1,127 @@ +import { + createTeamProject, + createWorkflow, + linkUserToProject, + testDb, +} from '@n8n/backend-test-utils'; +import { ExecutionRepository } from '@n8n/db'; +import type { User, WorkflowEntity } from '@n8n/db'; +import { Container } from '@n8n/di'; +import { mock } from 'vitest-mock-extended'; + +import { ExecutionListService } from '@/executions/execution-list.service'; +import { ExecutionRedactionServiceProxy } from '@/executions/execution-redaction-proxy.service'; +import type { Telemetry } from '@/telemetry'; +import { WorkflowFinderService } from '@/workflows/workflow-finder.service'; +import { createSuccessfulExecution } from '@test-integration/db/executions'; +import { createCustomRoleWithScopeSlugs } from '@test-integration/db/roles'; +import { createMember, createOwner } from '@test-integration/db/users'; + +import { createGetExecutionTool } from '../tools/get-execution.tool'; +import { createSearchExecutionsTool } from '../tools/search-executions.tool'; + +let workflow: WorkflowEntity; +let executionId: string; +let workflowReader: User; +let executionReader: User; + +beforeAll(async () => { + await testDb.init(); + + const owner = await createOwner(); + const project = await createTeamProject('MCP execution scopes', owner); + workflow = await createWorkflow({ settings: { availableInMCP: true } }, project); + executionId = (await createSuccessfulExecution(workflow)).id; + + const workflowOnlyRole = await createCustomRoleWithScopeSlugs(['workflow:read'], { + roleType: 'project', + }); + workflowReader = await createMember(); + await linkUserToProject(workflowReader, project, workflowOnlyRole.slug); + + executionReader = await createMember(); + await linkUserToProject(executionReader, project, 'project:viewer'); +}); + +afterAll(async () => { + await testDb.terminate(); +}); + +const getExecution = async (user: User, includeData: boolean) => + await createGetExecutionTool( + user, + Container.get(ExecutionRepository), + Container.get(WorkflowFinderService), + mock(), + Container.get(ExecutionRedactionServiceProxy), + ).handler( + { + workflowId: workflow.id, + executionId, + includeData, + nodeNames: undefined, + truncateData: undefined, + }, + {} as never, + ); + +const searchExecutions = async (user: User, workflowId?: string) => + await createSearchExecutionsTool( + user, + Container.get(ExecutionListService), + Container.get(WorkflowFinderService), + mock(), + ).handler({ workflowId }, {} as never); + +describe('get_workflow_execution project role scopes', () => { + test.each([true, false])( + 'returns no execution for a role without execution:read (includeData: %s)', + async (includeData) => { + const result = await getExecution(workflowReader, includeData); + + expect(result.structuredContent).toMatchObject({ + execution: null, + error: "Workflow not found or you don't have permission to access it.", + }); + expect(result.structuredContent).not.toHaveProperty('data'); + }, + ); + + test('returns the execution for a role with execution:read', async () => { + const result = await getExecution(executionReader, false); + + expect(result.structuredContent).toMatchObject({ + execution: { id: executionId, workflowId: workflow.id }, + }); + }); +}); + +describe('search_workflow_executions project role scopes', () => { + test('returns an access error for a role without execution:read when filtering by workflow', async () => { + const result = await searchExecutions(workflowReader, workflow.id); + + expect(result.isError).toBe(true); + expect(result.structuredContent).toMatchObject({ + data: [], + error: "Workflow not found or you don't have permission to access it.", + }); + }); + + test('returns no executions for a role without execution:read', async () => { + const result = await searchExecutions(workflowReader); + + expect(result.structuredContent).toMatchObject({ data: [], count: 0 }); + expect(result.structuredContent).not.toHaveProperty('error'); + }); + + test.each([undefined, 'workflow'] as const)( + 'returns the execution for a role with execution:read (filter: %s)', + async (filter) => { + const result = await searchExecutions(executionReader, filter ? workflow.id : undefined); + + expect(result.structuredContent).toMatchObject({ + data: [expect.objectContaining({ id: executionId, workflowId: workflow.id })], + }); + }, + ); +}); 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 935e1ab81aa..6b4c6568b8d 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 @@ -57,6 +57,74 @@ describe('get-execution MCP tool', () => { }); describe('handler tests', () => { + describe('access checks', () => { + test.each([true, false])( + 'requires workflow:read and execution:read on the workflow (includeData: %s)', + async (includeData) => { + const tool = createGetExecutionTool( + user, + executionRepository, + workflowFinderService, + telemetry, + executionRedactionServiceProxy, + ); + + (workflowFinderService.findWorkflowForUser as Mock).mockResolvedValue(mockWorkflow); + (executionPersistence.findWithUnflattenedData as Mock).mockResolvedValue(null); + (executionRepository.findIfAccessible as Mock).mockResolvedValue(null); + + await tool.handler( + { + workflowId: 'workflow-1', + executionId: 'execution-1', + includeData, + nodeNames: undefined, + truncateData: undefined, + }, + {} as never, + ); + + expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith( + 'workflow-1', + user, + ['workflow:read', 'execution:read'], + {}, + ); + }, + ); + + test.each([true, false])( + 'does not load the execution when the workflow lookup fails (includeData: %s)', + async (includeData) => { + const tool = createGetExecutionTool( + user, + executionRepository, + workflowFinderService, + telemetry, + executionRedactionServiceProxy, + ); + + (workflowFinderService.findWorkflowForUser as Mock).mockResolvedValue(null); + + const result = await tool.handler( + { + workflowId: 'workflow-1', + executionId: 'execution-1', + includeData, + nodeNames: undefined, + truncateData: undefined, + }, + {} as never, + ); + + expect(executionPersistence.findWithUnflattenedData).not.toHaveBeenCalled(); + expect(executionRepository.findIfAccessible).not.toHaveBeenCalled(); + expect(result.structuredContent).toMatchObject({ execution: null }); + expect(result.structuredContent).not.toHaveProperty('data'); + }, + ); + }); + describe('execution retrieval', () => { test('returns generic error when execution is not found for workflow', async () => { const tool = createGetExecutionTool( diff --git a/packages/cli/src/modules/mcp/__tests__/search-executions.tool.test.ts b/packages/cli/src/modules/mcp/__tests__/search-executions.tool.test.ts index bec1d288817..07a25a37863 100644 --- a/packages/cli/src/modules/mcp/__tests__/search-executions.tool.test.ts +++ b/packages/cli/src/modules/mcp/__tests__/search-executions.tool.test.ts @@ -35,7 +35,7 @@ describe('search-executions MCP tool', () => { beforeEach(() => { executionListService = mockInstance(ExecutionListService, { buildSharingOptions: vi.fn().mockResolvedValue({ - scopes: ['workflow:read'], + scopes: ['execution:read'], projectRoles: ['project:editor'], workflowRoles: ['workflow:editor'], }), @@ -123,7 +123,7 @@ describe('search-executions MCP tool', () => { expect(workflowFinderService.findWorkflowForUser).toHaveBeenCalledWith( 'wf-1', user, - ['workflow:read'], + ['workflow:read', 'execution:read'], { includeActiveVersion: undefined }, ); @@ -203,10 +203,32 @@ describe('search-executions MCP tool', () => { }); }); + test('returns an access error and runs no query when the workflow lookup fails', async () => { + (workflowFinderService.findWorkflowForUser as Mock).mockResolvedValue(null); + + const result = await createTool().handler({ workflowId: 'wf-1' } as never, {} as never); + + expect(result.isError).toBe(true); + expect(result.structuredContent).toEqual({ + data: [], + count: 0, + estimated: false, + nextCursor: null, + error: "Workflow not found or you don't have permission to access it.", + }); + expect(executionListService.findPageWithCount).not.toHaveBeenCalled(); + }); + test('delegates sharing options to the list service', async () => { await createTool().handler({} as never, {} as never); - expect(executionListService.buildSharingOptions).toHaveBeenCalledWith('workflow:read'); + expect(executionListService.buildSharingOptions).toHaveBeenCalledWith('execution:read'); + const query = (executionListService.findPageWithCount as Mock).mock.calls[0][0]; + expect(query.sharingOptions).toEqual({ + scopes: ['execution:read'], + projectRoles: ['project:editor'], + workflowRoles: ['workflow:editor'], + }); }); test('tracks telemetry on success', async () => { 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 ae47d464b1e..916e5dbb931 100644 --- a/packages/cli/src/modules/mcp/tools/get-execution.tool.ts +++ b/packages/cli/src/modules/mcp/tools/get-execution.tool.ts @@ -98,7 +98,12 @@ export const createGetExecutionTool = ( }; try { - await getMcpWorkflow(workflowId, user, ['workflow:read'], workflowFinderService); + await getMcpWorkflow( + workflowId, + user, + ['workflow:read', 'execution:read'], + workflowFinderService, + ); // Use lightweight metadata query when data isn't needed; // split into branches so TypeScript can narrow each return type. diff --git a/packages/cli/src/modules/mcp/tools/search-executions.tool.ts b/packages/cli/src/modules/mcp/tools/search-executions.tool.ts index 8b47b195ddb..81bcadb84b5 100644 --- a/packages/cli/src/modules/mcp/tools/search-executions.tool.ts +++ b/packages/cli/src/modules/mcp/tools/search-executions.tool.ts @@ -113,11 +113,16 @@ export const createSearchExecutionsTool = ( try { // Validate workflow access if workflowId is provided if (workflowId) { - await getMcpWorkflow(workflowId, user, ['workflow:read'], workflowFinderService); + await getMcpWorkflow( + workflowId, + user, + ['workflow:read', 'execution:read'], + workflowFinderService, + ); } const safeLimit = Math.min(Math.max(1, limit), MAX_RESULTS); - const sharingOptions = await executionListService.buildSharingOptions('workflow:read'); + const sharingOptions = await executionListService.buildSharingOptions('execution:read'); const parsedCursor = parseExecutionCursor(cursor); const query = { diff --git a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts index 741509ae3f3..105dc3d0de3 100644 --- a/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts +++ b/packages/cli/src/modules/source-control.ee/__tests__/source-control-import.service.ee.test.ts @@ -4,6 +4,7 @@ import type { PolicyCleared, PolicyViolation } from '@n8n/decorators'; import { type Variables, type VariablesRepository, + Folder, type FolderRepository, CredentialsEntity, type CredentialsRepository, @@ -99,6 +100,7 @@ describe('SourceControlImportService', () => { const executionPersistence = mock(); const credentialsService = mock(); const transactionManager = mock(); + const cascadeManager = mock(); const globalAdminContext = new SourceControlContext( Object.assign(new User(), { role: GLOBAL_ADMIN_ROLE }), @@ -166,6 +168,14 @@ describe('SourceControlImportService', () => { transactionManager.delete.mockImplementation( async (_entity, criteria) => await sharedCredentialsRepository.delete(criteria as never), ); + cascadeManager.delete.mockImplementation(async (entity, criteria) => + entity === Folder + ? await folderRepository.delete(criteria as never) + : await projectRepository.delete(criteria as never), + ); + workflowRepository.runInTransaction.mockImplementation( + async (ctx, fn) => await fn(cascadeManager, ctx), + ); credentialsRepositoryManager.transaction.mockImplementation(async (...args) => { const callback = args.find((arg) => typeof arg === 'function'); if (!callback) throw new Error('Transaction callback is required'); @@ -3463,6 +3473,48 @@ describe('SourceControlImportService', () => { ).toBeGreaterThan(folderRepository.delete.mock.invocationCallOrder[0]); }); + it('should purge caller allowlists in the folder delete transaction', async () => { + const candidates = [mock({ id: 'folder1' })]; + const straggler = Object.assign(new WorkflowEntity(), { + id: 'wf-1', + activeVersionId: null, + }); + folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]); + workflowRepository.find.mockResolvedValueOnce([straggler]); + workflowRepository.findOne.mockResolvedValueOnce(straggler); + + await service.deleteFoldersNotInWorkfolder(candidates); + + expect(workflowRepository.runInTransaction).toHaveBeenCalledTimes(1); + expect(workflowRepository.removeMissingFromCallerAllowlists).toHaveBeenCalledWith({}); + expect( + workflowRepository.removeMissingFromCallerAllowlists.mock.invocationCallOrder[0], + ).toBeGreaterThan(folderRepository.delete.mock.invocationCallOrder[0]); + expect( + workflowRepository.removeMissingFromCallerAllowlists.mock.invocationCallOrder[0], + ).toBeLessThan(workflowMutationHooks.afterWorkflowsDeleted.mock.invocationCallOrder[0]); + }); + + it('should fail the folder deletion when the caller allowlist purge fails', async () => { + const candidates = [mock({ id: 'folder1' })]; + const straggler = Object.assign(new WorkflowEntity(), { + id: 'wf-1', + activeVersionId: null, + }); + folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]); + workflowRepository.find.mockResolvedValueOnce([straggler]); + workflowRepository.findOne.mockResolvedValueOnce(straggler); + workflowRepository.removeMissingFromCallerAllowlists.mockRejectedValueOnce( + new Error('statement timeout'), + ); + + await expect(service.deleteFoldersNotInWorkfolder(candidates)).rejects.toThrow( + 'statement timeout', + ); + + expect(workflowMutationHooks.afterWorkflowsDeleted).not.toHaveBeenCalled(); + }); + it('should not fire the sweep when the deleted folders contained no workflows', async () => { const candidates = [mock({ id: 'folder1' })]; folderRepository.getAllFolderIdsInHierarchy.mockResolvedValueOnce([]); @@ -4132,6 +4184,32 @@ describe('SourceControlImportService', () => { ).toBeGreaterThan(projectRepository.delete.mock.invocationCallOrder[0]); }); + it('should purge caller allowlists in the project delete transaction', async () => { + const candidates = [mock({ id: 'project-1' })]; + sharedWorkflowRepository.find.mockResolvedValueOnce([ + { workflowId: 'wf-in-folder' }, + { workflowId: 'wf-at-project-root' }, + ] as SharedWorkflow[]); + workflowRepository.findOne + .mockResolvedValueOnce( + Object.assign(new WorkflowEntity(), { id: 'wf-in-folder', activeVersionId: null }), + ) + .mockResolvedValueOnce( + Object.assign(new WorkflowEntity(), { + id: 'wf-at-project-root', + activeVersionId: null, + }), + ); + + await service.deleteTeamProjectsNotInWorkfolder(user, candidates, []); + + expect(workflowRepository.runInTransaction).toHaveBeenCalledTimes(1); + expect(workflowRepository.removeMissingFromCallerAllowlists).toHaveBeenCalledWith({}); + expect( + workflowRepository.removeMissingFromCallerAllowlists.mock.invocationCallOrder[0], + ).toBeGreaterThan(projectRepository.delete.mock.invocationCallOrder[0]); + }); + it('should not fire the sweep when the deleted projects contained no workflows', async () => { const candidates = [mock({ id: 'project-1' })]; sharedWorkflowRepository.find.mockResolvedValueOnce([]); diff --git a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts index a5988d2e939..b3fd2dcd451 100644 --- a/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts +++ b/packages/cli/src/modules/source-control.ee/source-control-import.service.ee.ts @@ -1,16 +1,11 @@ import type { SourceControlledFile, WorkflowPublishBlockedDetails } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; -import type { - FindOptionsWhere, - Project, - TagEntity, - User, - Variables, - WorkflowEntity, -} from '@n8n/db'; +import type { FindOptionsWhere, TagEntity, User, Variables, WorkflowEntity } from '@n8n/db'; import { CredentialsRepository, + Folder, FolderRepository, + Project, ProjectRelationRepository, ProjectRepository, SharedCredentialsRepository, @@ -1916,8 +1911,8 @@ export class SourceControlImportService { workflows.map((workflow) => workflow.id), ); - await this.folderRepository.delete({ - id: In(candidateIds), + await this.deleteWithCallerAllowlistPurge(async (trx) => { + await trx.delete(Folder, { id: In(candidateIds) }); }); await this.sweepAfterWorkflowCascade(cascadedWorkflowIds); @@ -1967,8 +1962,8 @@ export class SourceControlImportService { ownedWorkflows.map((sw) => sw.workflowId), ); - await this.projectRepository.delete({ - id: In(candidateIds), + await this.deleteWithCallerAllowlistPurge(async (trx) => { + await trx.delete(Project, { id: In(candidateIds) }); }); await this.sweepAfterWorkflowCascade(cascadedWorkflowIds); @@ -2080,6 +2075,19 @@ export class SourceControlImportService { await this.workflowMutationHooks.afterWorkflowsDeleted(cascadedWorkflowIds); } + /** + * Run a folder/project row delete and drop every id left without a row from the `callerIds` + * allowlists, in one transaction, so no new workflow can take a freed id first. + */ + private async deleteWithCallerAllowlistPurge(deleteRows: (trx: EntityManager) => Promise) { + await this.workflowRepository.runInTransaction({}, async (trx, ctx) => { + await deleteRows(trx); + // Purge by what is gone rather than by the ids collected before the delete: the cascade + // also removes a workflow moved into a deleted folder since. + await this.workflowRepository.removeMissingFromCallerAllowlists(ctx); + }); + } + /** Contextual error for a failed deletion during pull, so the operator learns which resource to look at. */ private deletionError( type: 'workflow' | 'folder' | 'project', diff --git a/packages/cli/src/response-helper.ts b/packages/cli/src/response-helper.ts index 92726737720..ffca1dc10ec 100644 --- a/packages/cli/src/response-helper.ts +++ b/packages/cli/src/response-helper.ts @@ -14,7 +14,7 @@ import { FORM_TRIGGER_PATH_IDENTIFIER, NodeApiError } from 'n8n-workflow'; import { Readable } from 'node:stream'; import picocolors from 'picocolors'; -import { applyFormSandboxCSP } from './webhooks/webhook-response-headers'; +import { applySandboxCSP } from './webhooks/webhook-response-headers'; export function sendSuccessResponse( res: Response, @@ -65,7 +65,8 @@ export function sendErrorResponse(res: Response, error: Error) { if (isFormTrigger || isLegacyFormTrigger) { const isTestWebhook = basePath.includes('test'); res.status(404); - applyFormSandboxCSP(res); + // The setting of the route decides: a webhook path can look like a form path. + applySandboxCSP(res, 'form'); return res.render('form-trigger-404', { isTestWebhook }); } } @@ -73,7 +74,7 @@ export function sendErrorResponse(res: Response, error: Error) { if (error.errorCode === 409 && originalUrl && originalUrl.includes('form-waiting')) { //codes other than 200 breaks redirection to form-waiting page from form trigger //render form page instead of json - applyFormSandboxCSP(res); + applySandboxCSP(res, 'form'); return res.render('form-trigger-409', { message: error.message, }); @@ -95,6 +96,8 @@ export function sendErrorResponse(res: Response, error: Error) { response.stacktrace = error.stack; } + // res.json() does not override a Content-Type set earlier; force JSON here. + res.setHeader('Content-Type', 'application/json'); res.status(status).json(response); } diff --git a/packages/cli/src/services/__tests__/ownership.service.test.ts b/packages/cli/src/services/__tests__/ownership.service.test.ts index db116cf4894..d9194907637 100644 --- a/packages/cli/src/services/__tests__/ownership.service.test.ts +++ b/packages/cli/src/services/__tests__/ownership.service.test.ts @@ -70,6 +70,20 @@ describe('OwnershipService', () => { expect(returnedProject).toBe(mockProject); }); + test('should ignore a cached project without an id and read the project from the database', async () => { + const mockProject = Object.assign(new Project(), { id: 'project-id' }); + // A cached value without an id, as a lookup of the key `__proto__` used to return + cacheService.getHashValue.mockResolvedValueOnce({}); + sharedWorkflowRepository.findOneOrFail.mockResolvedValueOnce( + Object.assign(new SharedWorkflow(), { role: 'workflow:owner', project: mockProject }), + ); + + const returnedProject = await ownershipService.getWorkflowProjectCached('some-workflow-id'); + + expect(returnedProject).toBe(mockProject); + expect(sharedWorkflowRepository.findOneOrFail).toHaveBeenCalled(); + }); + test('should throw if no workflow owner project found', async () => { sharedWorkflowRepository.findOneOrFail.mockRejectedValue(new Error()); diff --git a/packages/cli/src/services/ownership.service.ts b/packages/cli/src/services/ownership.service.ts index f772df8d066..0288167fa49 100644 --- a/packages/cli/src/services/ownership.service.ts +++ b/packages/cli/src/services/ownership.service.ts @@ -34,6 +34,23 @@ export class OwnershipService { private settingsRepository: SettingsRepository, ) {} + // To make use of the cache service we should store POJOs, these + // methods should be used to create copies of the entities + // converting them into plain JavaScript objects + // Ideally our data entities wouldn't be classes, but plain + // interfaces and therefore pojos adhering to these interfaces + copyProject(project: Project): Partial { + return { + ...project, + }; + } + + reconstructProject(project: Partial): Project | undefined { + if (typeof project?.id !== 'string') { + return undefined; + } + return Object.assign(new Project(), project); + } copyUser(user: User): Partial { return { ...user, diff --git a/packages/cli/src/webhooks/__tests__/webhook-request-handler.test.ts b/packages/cli/src/webhooks/__tests__/webhook-request-handler.test.ts index 63e4aef1258..36dcefcbe88 100644 --- a/packages/cli/src/webhooks/__tests__/webhook-request-handler.test.ts +++ b/packages/cli/src/webhooks/__tests__/webhook-request-handler.test.ts @@ -1,7 +1,11 @@ import { Logger } from '@n8n/backend-common'; import { mockInstance } from '@n8n/backend-test-utils'; import { type Response } from 'express'; -import { isWebhookHtmlSandboxingDisabled, getHtmlSandboxCSP } from 'n8n-core'; +import { + isFormHtmlSandboxingDisabled, + isWebhookHtmlSandboxingDisabled, + getHtmlSandboxCSP, +} from 'n8n-core'; import { OperationalError, randomString } from 'n8n-workflow'; import type { IHttpRequestMethods } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; @@ -18,6 +22,7 @@ import type { vi.mock('n8n-core', async () => ({ ...(await vi.importActual('n8n-core')), isWebhookHtmlSandboxingDisabled: vi.fn().mockReturnValue(false), + isFormHtmlSandboxingDisabled: vi.fn().mockReturnValue(false), getHtmlSandboxCSP: vi.fn().mockReturnValue('sandbox allow-downloads allow-forms allow-modals'), })); @@ -335,7 +340,7 @@ describe('WebhookRequestHandler', () => { }); it('should not set CSP sandbox header when sandboxing is disabled', async () => { - vi.mocked(isWebhookHtmlSandboxingDisabled).mockReturnValueOnce(true); + vi.mocked(isWebhookHtmlSandboxingDisabled).mockReturnValue(true); const req = mock({ path: '/', @@ -354,7 +359,174 @@ describe('WebhookRequestHandler', () => { await handler(req, res); - expect(res.setHeader).not.toHaveBeenCalledWith('Content-Security-Policy', expect.anything()); + expect(res.setHeader).not.toHaveBeenCalledWith( + 'Content-Security-Policy', + getHtmlSandboxCSP(), + ); + }); + + describe('on every exit path', () => { + const sandboxCSP = 'sandbox allow-scripts'; + + const cspSetBefore = (res: ReturnType>, writer: 'json' | 'send') => { + const cspCall = res.setHeader.mock.calls.findIndex( + ([name]) => name === 'Content-Security-Policy', + ); + expect(cspCall).not.toBe(-1); + expect(res.setHeader.mock.calls[cspCall][1]).toBe(sandboxCSP); + expect(res.setHeader.mock.invocationCallOrder[cspCall]).toBeLessThan( + res[writer].mock.invocationCallOrder[0], + ); + }; + + beforeEach(() => { + vi.mocked(getHtmlSandboxCSP).mockReturnValue(sandboxCSP); + }); + + it('should set the sandbox header before an error response is sent', async () => { + const req = mock({ path: '/', method: 'GET', params: { path: 'test' } }); + const res = mock(); + res.status.mockReturnValue(res); + webhookManager.executeWebhook.mockRejectedValueOnce(new OperationalError('Test error')); + + await handler(req, res); + + expect(res.status).toHaveBeenCalledWith(500); + cspSetBefore(res, 'json'); + }); + + it('should set the sandbox header on a legacy response without headers', async () => { + const req = mock({ path: '/', method: 'GET', params: { path: 'test' } }); + const res = mock(); + // A node can pass a plain string body to `sendResponse()`, without a headers object. + webhookManager.executeWebhook.mockResolvedValueOnce({ + responseCode: 200, + data: '

text

', + } as unknown as IWebhookResponseCallbackData); + + await handler(req, res); + + expect(res.send).toHaveBeenCalledWith('

text

'); + cspSetBefore(res, 'send'); + }); + + it('should set the sandbox header for an unsupported method', async () => { + const req = mock({ + path: '/', + method: 'CONNECT' as IHttpRequestMethods, + }); + const res = mock(); + res.status.mockReturnValue(res); + + await handler(req, res); + + cspSetBefore(res, 'json'); + }); + + it('should set the sandbox header when the node writes the response', async () => { + const req = mock({ path: '/', method: 'GET', params: { path: 'test' } }); + const res = mock(); + webhookManager.executeWebhook.mockImplementationOnce(async (_req, response) => { + response.send('

text

'); + return { noWebhookResponse: true }; + }); + + await handler(req, res); + + cspSetBefore(res, 'send'); + }); + }); + + describe('sandbox setting of the route', () => { + const formHandlers = [ + ['a form route', createWebhookHandlerFor(webhookManager, 'form')], + [ + 'a route that selects the form setting', + createWebhookHandlerFor(webhookManager, undefined, 'form'), + ], + ] as const; + + test.each(formHandlers)('should follow the form setting on %s', async (_, formHandler) => { + vi.mocked(isFormHtmlSandboxingDisabled).mockReturnValue(true); + vi.mocked(isWebhookHtmlSandboxingDisabled).mockReturnValue(false); + vi.mocked(getHtmlSandboxCSP).mockReturnValue('sandbox allow-scripts'); + + const req = mock({ path: '/', method: 'GET', params: { path: 'test' } }); + const res = mock(); + webhookManager.executeWebhook.mockResolvedValueOnce({ noWebhookResponse: true }); + + await (formHandler as typeof handler)(req, res); + + expect(res.setHeader).not.toHaveBeenCalledWith( + 'Content-Security-Policy', + 'sandbox allow-scripts', + ); + }); + + it('should not sandbox a form response when only form sandboxing is disabled', async () => { + vi.mocked(isFormHtmlSandboxingDisabled).mockReturnValue(true); + vi.mocked(isWebhookHtmlSandboxingDisabled).mockReturnValue(false); + vi.mocked(getHtmlSandboxCSP).mockReturnValue('sandbox allow-scripts'); + const formHandler = createWebhookHandlerFor(webhookManager, 'form') as typeof handler; + + const req = mock({ path: '/', method: 'GET', params: { path: 'test' } }); + const res = mock(); + webhookManager.executeWebhook.mockResolvedValueOnce({ + responseCode: 200, + data: '

text

', + headers: { 'content-type': 'text/html' }, + } as unknown as IWebhookResponseCallbackData); + + await formHandler(req, res); + + expect(res.setHeaders).toHaveBeenCalled(); + expect(res.setHeader).not.toHaveBeenCalledWith( + 'Content-Security-Policy', + 'sandbox allow-scripts', + ); + expect(res.send).toHaveBeenCalledWith('

text

'); + }); + + it('should keep the form sandbox on a form response when webhook sandboxing is disabled', async () => { + vi.mocked(isFormHtmlSandboxingDisabled).mockReturnValue(false); + vi.mocked(isWebhookHtmlSandboxingDisabled).mockReturnValue(true); + vi.mocked(getHtmlSandboxCSP).mockReturnValue('sandbox allow-scripts'); + const formHandler = createWebhookHandlerFor(webhookManager, 'form') as typeof handler; + + const req = mock({ path: '/', method: 'GET', params: { path: 'test' } }); + const res = mock(); + webhookManager.executeWebhook.mockResolvedValueOnce({ + responseCode: 200, + data: { ok: true }, + headers: { 'content-type': 'text/html' }, + }); + + await formHandler(req, res); + + expect(res.setHeader).toHaveBeenCalledWith( + 'Content-Security-Policy', + 'sandbox allow-scripts', + ); + expect(res.removeHeader).not.toHaveBeenCalled(); + expect(res.json).toHaveBeenCalledWith({ ok: true }); + }); + + it('should follow the webhook setting on other routes', async () => { + vi.mocked(isFormHtmlSandboxingDisabled).mockReturnValue(true); + vi.mocked(isWebhookHtmlSandboxingDisabled).mockReturnValue(false); + vi.mocked(getHtmlSandboxCSP).mockReturnValue('sandbox allow-scripts'); + + const req = mock({ path: '/', method: 'GET', params: { path: 'test' } }); + const res = mock(); + webhookManager.executeWebhook.mockResolvedValueOnce({ noWebhookResponse: true }); + + await handler(req, res); + + expect(res.setHeader).toHaveBeenCalledWith( + 'Content-Security-Policy', + 'sandbox allow-scripts', + ); + }); }); }); }); diff --git a/packages/cli/src/webhooks/__tests__/webhook-response-headers.test.ts b/packages/cli/src/webhooks/__tests__/webhook-response-headers.test.ts index 17ee7619248..6750ca7b098 100644 --- a/packages/cli/src/webhooks/__tests__/webhook-response-headers.test.ts +++ b/packages/cli/src/webhooks/__tests__/webhook-response-headers.test.ts @@ -6,7 +6,12 @@ import type { Response } from 'express'; import { getHtmlSandboxCSP } from 'n8n-core'; import { mock } from 'vitest-mock-extended'; -import { applyFormSandboxCSP, WebhookResponseHeaders } from '@/webhooks/webhook-response-headers'; +import { + applyFormSandboxCSP, + applySandboxCSP, + setRouteSandboxSetting, + WebhookResponseHeaders, +} from '@/webhooks/webhook-response-headers'; describe('WebhookResponseHeaders', () => { const logger = mockInstance(Logger); @@ -353,13 +358,75 @@ describe('WebhookResponseHeaders', () => { expect(res.setHeader).toHaveBeenCalledWith('Content-Security-Policy', getHtmlSandboxCSP()); }); - it('should set no policy when form sandboxing is disabled', () => { + it('should remove the policy when form sandboxing is disabled', () => { securityConfig.disableFormHtmlSandboxing = true; const res = mock(); applyFormSandboxCSP(res); expect(res.setHeader).not.toHaveBeenCalled(); + expect(res.removeHeader).toHaveBeenCalledWith('Content-Security-Policy'); + }); + }); + + describe('applySandboxCSP', () => { + const securityConfig = Container.get(SecurityConfig); + + afterEach(() => { + securityConfig.disableWebhookHtmlSandboxing = false; + }); + + it('should set the sandbox policy', () => { + const res = mock(); + + applySandboxCSP(res); + + expect(res.setHeader).toHaveBeenCalledWith('Content-Security-Policy', getHtmlSandboxCSP()); + }); + + it('should leave the policy unchanged when webhook sandboxing is disabled', () => { + securityConfig.disableWebhookHtmlSandboxing = true; + const res = mock(); + + applySandboxCSP(res); + + expect(res.setHeader).not.toHaveBeenCalled(); + expect(res.removeHeader).not.toHaveBeenCalled(); + }); + + describe('on a form route', () => { + afterEach(() => { + securityConfig.disableFormHtmlSandboxing = false; + }); + + it('should follow the form setting instead of the webhook setting', () => { + securityConfig.disableFormHtmlSandboxing = true; + const res = mock({ locals: {} }); + setRouteSandboxSetting(res, 'form'); + + applySandboxCSP(res); + + expect(res.setHeader).not.toHaveBeenCalled(); + }); + + it('should set the policy when only webhook sandboxing is disabled', () => { + securityConfig.disableWebhookHtmlSandboxing = true; + const res = mock({ locals: {} }); + setRouteSandboxSetting(res, 'form'); + + applySandboxCSP(res); + + expect(res.setHeader).toHaveBeenCalledWith('Content-Security-Policy', getHtmlSandboxCSP()); + }); + }); + + it('should use the fallback setting outside webhook and form routes', () => { + securityConfig.disableWebhookHtmlSandboxing = true; + const res = mock({ locals: {} }); + + applySandboxCSP(res, 'form'); + + expect(res.setHeader).toHaveBeenCalledWith('Content-Security-Policy', getHtmlSandboxCSP()); }); }); }); diff --git a/packages/cli/src/webhooks/webhook-request-handler.ts b/packages/cli/src/webhooks/webhook-request-handler.ts index c15ff0803df..5db87425445 100644 --- a/packages/cli/src/webhooks/webhook-request-handler.ts +++ b/packages/cli/src/webhooks/webhook-request-handler.ts @@ -21,7 +21,12 @@ import { isWebhookResponse, isWebhookStreamResponse, } from '@/webhooks/webhook-response'; -import { applySandboxCSP, WebhookResponseHeaders } from '@/webhooks/webhook-response-headers'; +import { + applySandboxCSP, + setRouteSandboxSetting, + WebhookResponseHeaders, + type WebhookSandboxSetting, +} from '@/webhooks/webhook-response-headers'; import type { IWebhookManager, WebhookOptionsRequest, @@ -34,6 +39,7 @@ class WebhookRequestHandler { constructor( private readonly webhookManager: IWebhookManager, private readonly expectedNodeType?: ExpectedWebhookNodeType, + private readonly sandboxSetting: WebhookSandboxSetting = 'webhook', ) {} /** @@ -43,6 +49,12 @@ class WebhookRequestHandler { async handleRequest(req: WebhookRequest | WebhookOptionsRequest, res: express.Response) { const method = req.method; + // Set the sandbox before any writer runs, so that every response of the route has it, + // for all status codes and content types. Only an n8n page that must keep its origin + // replaces or removes it. Later writers follow the same setting through `res.locals`. + setRouteSandboxSetting(res, this.sandboxSetting); + applySandboxCSP(res); + if (method !== 'OPTIONS' && !WEBHOOK_METHODS.includes(method)) { return ResponseHelper.sendErrorResponse( res, @@ -280,12 +292,16 @@ function trackWebhookMetrics( * recorded on each response: webhook requests via `observeWebhookRequest`, * form submissions (POST only) via `observeFormSubmission`. No metrics are * emitted for other node types. + * + * `sandboxSetting` selects the operator setting for the sandbox CSP. It defaults to the + * form setting for form routes and to the webhook setting for all other routes. */ export function createWebhookHandlerFor( webhookManager: IWebhookManager, expectedNodeType?: ExpectedWebhookNodeType, + sandboxSetting: WebhookSandboxSetting = expectedNodeType === 'form' ? 'form' : 'webhook', ): express.RequestHandler { - const handler = new WebhookRequestHandler(webhookManager, expectedNodeType); + const handler = new WebhookRequestHandler(webhookManager, expectedNodeType, sandboxSetting); const metricsService = Container.get(PrometheusWebhookAndFormMetricsService); return async (req, res) => { diff --git a/packages/cli/src/webhooks/webhook-response-headers.ts b/packages/cli/src/webhooks/webhook-response-headers.ts index 05b459ae0ed..76400b92207 100644 --- a/packages/cli/src/webhooks/webhook-response-headers.ts +++ b/packages/cli/src/webhooks/webhook-response-headers.ts @@ -86,17 +86,43 @@ export class WebhookResponseHeaders { } } -/** Set the sandbox CSP header on a webhook response, unless explicitly disabled. */ -export function applySandboxCSP(res: Response): void { - if (isWebhookHtmlSandboxingDisabled()) return; +/** The operator setting that controls the sandbox CSP on the responses of a route. */ +export type WebhookSandboxSetting = 'webhook' | 'form'; + +const SANDBOX_SETTING_LOCAL = 'webhookSandboxSetting'; + +/** Record which operator setting controls the sandbox CSP on this response. */ +export function setRouteSandboxSetting(res: Response, setting: WebhookSandboxSetting): void { + res.locals[SANDBOX_SETTING_LOCAL] = setting; +} + +function getRouteSandboxSetting(res: Response): WebhookSandboxSetting | undefined { + const setting: unknown = res.locals?.[SANDBOX_SETTING_LOCAL]; + return setting === 'webhook' || setting === 'form' ? setting : undefined; +} + +/** + * Set the sandbox CSP header on a webhook or form route response, unless an operator + * disabled it. The setting of the route decides, so every writer of a route follows the + * same setting. `fallback` applies when the response is not on a webhook or form route. + */ +export function applySandboxCSP(res: Response, fallback: WebhookSandboxSetting = 'webhook'): void { + const setting = getRouteSandboxSetting(res) ?? fallback; + const disabled = + setting === 'form' ? isFormHtmlSandboxingDisabled() : isWebhookHtmlSandboxingDisabled(); + if (disabled) return; res.setHeader('Content-Security-Policy', getHtmlSandboxCSP()); } /** - * Set the sandbox CSP header on a form page response, unless an operator disabled it. + * Set the sandbox CSP header on a form page response. If an operator disabled it, remove + * the header instead, because the request handler sets it for the whole route. * Call this for every HTML page a form endpoint serves, error and status pages included. */ export function applyFormSandboxCSP(res: Response): void { - if (isFormHtmlSandboxingDisabled()) return; + if (isFormHtmlSandboxingDisabled()) { + res.removeHeader('Content-Security-Policy'); + return; + } res.setHeader('Content-Security-Policy', getHtmlSandboxCSP()); } diff --git a/packages/cli/src/workflow-execute-additional-data.ts b/packages/cli/src/workflow-execute-additional-data.ts index dc83a5a6e72..fb013a83045 100644 --- a/packages/cli/src/workflow-execute-additional-data.ts +++ b/packages/cli/src/workflow-execute-additional-data.ts @@ -162,6 +162,7 @@ async function fetchWorkflowData( workflowData.nodes ?? [], parentWorkflowId, ); + WorkflowHelpers.validateWorkflowStructure(workflowData); // 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; diff --git a/packages/cli/src/workflows/__tests__/workflow-creation.service.test.ts b/packages/cli/src/workflows/__tests__/workflow-creation.service.test.ts index e63439fb5db..ce6f00b1f96 100644 --- a/packages/cli/src/workflows/__tests__/workflow-creation.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow-creation.service.test.ts @@ -10,11 +10,13 @@ import type { } from '@n8n/db'; import { WorkflowEntity } from '@n8n/db'; import type { PolicyCleared } from '@n8n/decorators'; +import type { INode } from 'n8n-workflow'; import type { MockProxy } from 'vitest-mock-extended'; import { mock } from 'vitest-mock-extended'; import type { CredentialsFinderService } from '@n8n/backend-services'; import { BadRequestError, ForbiddenError, NotFoundError } from '@n8n/errors'; +import { UninspectableSubworkflowSourceError } from '@/errors/uninspectable-subworkflow-source.error'; import type { ExternalHooks, WorkflowLifecycleHookActor } from '@/external-hooks'; import type { McpSettingsService } from '@/modules/mcp/mcp.settings.service'; import type { InstanceRedactionEnforcementService } from '@/modules/redaction/instance-redaction-enforcement.service'; @@ -81,6 +83,7 @@ describe('WorkflowCreationService', () => { enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ ids: new Set(), hasUnresolved: false, + hasUninspectableSource: false, }); // Default: no active floor. Tests opt into a floor explicitly. @@ -139,6 +142,7 @@ describe('WorkflowCreationService', () => { enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ ids: new Set(['source-credential']), hasUnresolved: false, + hasUninspectableSource: false, }); const context = await workflowCreationService.prepareBatchContext( @@ -328,6 +332,7 @@ describe('WorkflowCreationService', () => { enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ ids: new Set(['credential-1']), hasUnresolved: false, + hasUninspectableSource: false, }); credentialsFinderServiceMock.findCredentialIdsWithScopeForUser.mockResolvedValue( new Set(['credential-1']), @@ -384,6 +389,7 @@ describe('WorkflowCreationService', () => { enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ ids: new Set(['credential-1']), hasUnresolved: true, + hasUninspectableSource: false, }); credentialsFinderServiceMock.findCredentialIdsWithScopeForUser.mockResolvedValue( new Set(['credential-1']), @@ -403,6 +409,58 @@ describe('WorkflowCreationService', () => { 'The workflow you are trying to save contains credentials that are not shared with you', ); }); + + it('should pass full credential access to the source check', async () => { + const user = mock(); + const newWorkflow = new WorkflowEntity(); + licenseStateMock.isSharingLicensed.mockReturnValue(true); + projectServiceMock.getProjectWithScope.mockResolvedValue({ id: 'project-1' } as never); + enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ + ids: new Set(), + hasUnresolved: false, + hasUninspectableSource: true, + }); + enterpriseWorkflowServiceMock.canUseEveryCredential.mockReturnValue(true); + enterpriseWorkflowServiceMock.validateCredentialPermissionsToUser.mockImplementation(() => { + throw new Error('Stopping for test'); + }); + + await expect( + workflowCreationService.createWorkflow(user, newWorkflow, { projectId: 'project-1' }), + ).rejects.toThrow(); + + expect(enterpriseWorkflowServiceMock.canUseEveryCredential).toHaveBeenCalledWith(user); + expect( + enterpriseWorkflowServiceMock.validateCredentialPermissionsToUser, + ).toHaveBeenCalledWith(newWorkflow, new Set(), { allowUninspectableSource: true }); + }); + + it('should preserve the source error message for a restricted user', async () => { + const user = mock(); + const newWorkflow = new WorkflowEntity(); + const node = { name: 'Sub' } as INode; + licenseStateMock.isSharingLicensed.mockReturnValue(true); + projectServiceMock.getProjectWithScope.mockResolvedValue({ id: 'project-1' } as never); + enterpriseWorkflowServiceMock.collectCredentialReferences.mockReturnValue({ + ids: new Set(), + hasUnresolved: false, + hasUninspectableSource: true, + }); + enterpriseWorkflowServiceMock.canUseEveryCredential.mockReturnValue(false); + enterpriseWorkflowServiceMock.validateCredentialPermissionsToUser.mockImplementation(() => { + throw new UninspectableSubworkflowSourceError(node); + }); + + await expect( + workflowCreationService.createWorkflow(user, newWorkflow, { projectId: 'project-1' }), + ).rejects.toThrow( + /'Sub' node loads its sub-workflow from a source that is resolved only when the workflow runs/, + ); + + expect( + enterpriseWorkflowServiceMock.validateCredentialPermissionsToUser, + ).toHaveBeenCalledWith(newWorkflow, new Set(), { allowUninspectableSource: false }); + }); }); it('should throw BadRequestError when user lacks access to credentials in workflow', async () => { diff --git a/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts index d5efbd912dc..013490a720d 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts @@ -17,6 +17,7 @@ import { mock } from 'vitest-mock-extended'; import type { ActiveWorkflowManager } from '@/active-workflow-manager'; import type { CredentialsFinderService } from '@n8n/backend-services'; import type { CredentialsService } from '@/credentials/credentials.service'; +import type { NodeTypes } from '@/node-types'; import type { CredentialsPermissionChecker } from '@/executions/pre-execution-checks/credentials-permission-checker'; import type { PolicyEnforcementService } from '@/policy/policy-enforcement.service'; import type { OwnershipService } from '@/services/ownership.service'; @@ -44,10 +45,12 @@ describe('EnterpriseWorkflowService', () => { const credentialsService = mock(); const ownershipService = mock(); const credentialsFinderService = mock(); + const nodeTypes = mock(); const credentialsPermissionChecker = mock(); beforeEach(() => { vi.clearAllMocks(); + nodeTypes.getByNameAndVersion.mockReset(); flags.credSharingEnabled = false; service = new EnterpriseWorkflowService( mock(), // logger @@ -65,6 +68,7 @@ describe('EnterpriseWorkflowService', () => { workflowPublishHistoryRepository, workflowMutationHooks, policyEnforcementService, + nodeTypes, credentialsPermissionChecker, ); }); @@ -401,6 +405,216 @@ describe('EnterpriseWorkflowService', () => { }); }); + describe('validateWorkflowCredentialUsage() - sub-workflow source', () => { + const URL_SOURCE = { source: 'url', workflowUrl: 'https://example.com/workflow.json' }; + const subworkflowNode = (parameters: Record, overrides: Partial = {}) => + ({ + id: 'sub-1', + name: 'Sub', + type: 'n8n-nodes-base.executeWorkflow', + typeVersion: 1.2, + position: [0, 0], + parameters, + ...overrides, + }) as unknown as INode; + const save = ( + newNodes: INode[], + previousNodes: INode[] = [], + options?: { allowUninspectableSource?: boolean }, + ) => + service.validateWorkflowCredentialUsage( + { nodes: newNodes } as unknown as IWorkflowBase, + { nodes: previousNodes } as unknown as IWorkflowBase, + [], + [], + options, + ); + + it.each([ + ['"url"', URL_SOURCE], + ['"localFile"', { source: 'localFile', workflowPath: '/data/workflow.json' }], + ['an expression', { source: '={{ $json.source }}' }], + ['not a string', { source: 42 }], + ])('rejects a new Execute Sub-workflow node whose source is %s', (_label, parameters) => { + expect(() => save([subworkflowNode(parameters)])).toThrow(/cannot be checked/); + }); + + it.each([ + ['absent', {}], + ['"database"', { source: 'database', workflowId: { __rl: true, value: 'wf-1', mode: 'id' } }], + [ + '"parameter"', + { source: 'parameter', workflowJson: JSON.stringify({ nodes: [], connections: {} }) }, + ], + ])('accepts a new Execute Sub-workflow node whose source is %s', (_label, parameters) => { + expect(() => save([subworkflowNode(parameters)])).not.toThrow(); + }); + + it('rejects a Workflow Tool node whose source is neither "database" nor "parameter"', () => { + const node = subworkflowNode(URL_SOURCE, { + type: '@n8n/n8n-nodes-langchain.toolWorkflow', + typeVersion: 2, + }); + expect(() => save([node])).toThrow(/cannot be checked/); + }); + + it('rejects the synthetic HITL tool variant of a url-sourced Execute Sub-workflow node', () => { + const node = subworkflowNode(URL_SOURCE, { type: 'n8n-nodes-base.executeWorkflowHitlTool' }); + expect(() => save([node])).toThrow(/cannot be checked/); + }); + + it('rejects a url-sourced node nested inside an inline sub-workflow', () => { + const inner = subworkflowNode(URL_SOURCE, { id: 'inner', name: 'Inner' }); + const outer = subworkflowNode({ + source: 'parameter', + workflowJson: JSON.stringify({ nodes: [inner], connections: {} }), + }); + expect(() => save([outer])).toThrow(/cannot be checked/); + }); + + it('rejects a url-sourced Execute Sub-workflow tool inside an inline agent parameter', () => { + const agent = { + id: 'agent-1', + name: 'Agent', + type: 'n8n-nodes-base.messageAnAgent', + typeVersion: 2, + position: [0, 0], + parameters: { + agentSource: 'inline', + inlineAgent: { + config: { + tools: [ + { nodeType: 'n8n-nodes-base.executeWorkflowTool', nodeParameters: URL_SOURCE }, + ], + }, + }, + }, + } as unknown as INode; + expect(() => save([agent])).toThrow(/cannot be checked/); + }); + + it('rejects an existing editable node that is changed to a url source', () => { + const previous = subworkflowNode({ source: 'database', workflowId: 'wf-1' }); + expect(() => save([subworkflowNode(URL_SOURCE)], [previous])).toThrow(/cannot be checked/); + }); + + it('allows renaming, moving and disabling a stored URL node', () => { + const previous = subworkflowNode(URL_SOURCE); + const edited = { ...previous, name: 'Renamed', position: [10, 10], disabled: true } as INode; + + const result = save([edited], [previous]); + + expect(result.nodes[0]).toEqual(edited); + }); + + it('rejects URL changes when the node description is unavailable', () => { + const previous = subworkflowNode(URL_SOURCE); + const changed = subworkflowNode({ + source: 'url', + workflowUrl: 'https://example.com/other.json', + }); + expect(() => save([changed], [previous])).toThrow(/cannot be checked/); + }); + + it('rejects retry setting changes on a stored URL node', () => { + const previous = subworkflowNode(URL_SOURCE); + const changed = { ...previous, retryOnFail: true } as INode; + expect(() => save([changed], [previous])).toThrow(/cannot be checked/); + }); + + describe('with the node type description available', () => { + beforeEach(() => { + nodeTypes.getByNameAndVersion.mockReturnValue({ + description: { + properties: [ + { displayName: 'Source', name: 'source', type: 'options', default: 'database' }, + { + displayName: 'Workflow', + name: 'workflowId', + type: 'string', + default: '', + displayOptions: { show: { source: ['database'] } }, + }, + { + displayName: 'URL', + name: 'workflowUrl', + type: 'string', + default: '', + displayOptions: { show: { source: ['url'] } }, + }, + { displayName: 'Mode', name: 'mode', type: 'options', default: 'once' }, + ], + }, + } as never); + }); + + it('accepts omitted parameter defaults', () => { + // A node stored through the API may carry a default the editor strips on save. + const stored = subworkflowNode({ ...URL_SOURCE, mode: 'once' }); + const fromEditor = { ...subworkflowNode(URL_SOURCE), name: 'Renamed' } as INode; + + const result = save([fromEditor], [stored]); + + expect(result.nodes[0]).toEqual({ ...stored, name: 'Renamed' }); + }); + + it('accepts omitted hidden parameters', () => { + // The editor drops the old database workflow ID when the source is URL. + const stored = subworkflowNode({ ...URL_SOURCE, workflowId: 'old-workflow' }); + const fromEditor = { ...subworkflowNode(URL_SOURCE), name: 'Renamed' } as INode; + + const result = save([fromEditor], [stored]); + + expect(result.nodes[0]).toEqual({ ...stored, name: 'Renamed' }); + }); + + it('rejects URL changes after parameter normalization', () => { + const stored = subworkflowNode({ ...URL_SOURCE, mode: 'once' }); + const changed = subworkflowNode({ + source: 'url', + workflowUrl: 'https://example.com/other.json', + }); + + expect(() => save([changed], [stored])).toThrow(/cannot be checked/); + }); + }); + + it('accepts url-sourced nodes when the caller allows uninspectable sources', () => { + const previous = subworkflowNode(URL_SOURCE); + const changed = subworkflowNode({ + source: 'url', + workflowUrl: 'https://example.com/other.json', + }); + const options = { allowUninspectableSource: true }; + + expect(() => save([subworkflowNode(URL_SOURCE)], [], options)).not.toThrow(); + expect(save([changed], [previous], options).nodes[0]).toEqual(changed); + }); + + it('checks source permissions when validating a new workflow', () => { + const workflow = { nodes: [subworkflowNode(URL_SOURCE)] } as unknown as IWorkflowBase; + + expect(() => service.validateCredentialPermissionsToUser(workflow, [])).toThrow( + /cannot be checked/, + ); + expect(() => + service.validateCredentialPermissionsToUser(workflow, [], { + allowUninspectableSource: true, + }), + ).not.toThrow(); + }); + + it('reports runtime-loaded sources even when there are no credential IDs', () => { + const workflow = { nodes: [subworkflowNode(URL_SOURCE)] } as unknown as IWorkflowBase; + + expect(service.collectCredentialReferences(workflow)).toEqual({ + ids: new Set(), + hasUnresolved: false, + hasUninspectableSource: true, + }); + }); + }); + describe('validateWorkflowCredentialUsage() - credential added to an existing node', () => { const httpNode = ( credentials?: INode['credentials'], diff --git a/packages/cli/src/workflows/__tests__/workflow.service.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.test.ts index fc60722f7bf..75ac0037829 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.test.ts @@ -58,7 +58,7 @@ import type { WorkflowPublicationStatusService } from '@/workflows/publication/w import type { WorkflowMutationHooksProxy } from '@/workflows/workflow-mutation-hooks-proxy.service'; import type { WorkflowPublishGuardProxy } from '@/workflows/workflow-publish-guard-proxy.service'; import type { WorkflowValidationService } from '@/workflows/workflow-validation.service'; -import { NodeGroupRulesFlagGate } from '@/workflows/node-group-rules-flag-gate'; +import type { NodeGroupRulesFlagGate } from '@/workflows/node-group-rules-flag-gate'; import type { RestrictedNodeTypesProviderProxy } from '@/workflows/restricted-node-types-provider-proxy.service'; import { WorkflowService } from '@/workflows/workflow.service'; import { ALL_RULES_RELAXED, NO_RULES_RELAXED } from './node-group-rules.test-data'; @@ -73,6 +73,7 @@ describe('WorkflowService', () => { let workflowRepositoryMock: MockProxy<{ getManyAndCountWithSharingSubquery: Mock; getWorkflowsAndFoldersWithCountWithSharingSubquery: Mock; + findCallableSubworkflows: Mock; }>; let roleServiceMock: MockProxy; let webhookServiceMock: MockProxy; @@ -94,8 +95,6 @@ describe('WorkflowService', () => { webhookServiceMock = mock(); workflowFinderServiceMock = mock(); - // By default the requester can read the supplied parent workflow. - workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(mock()); globalConfigMock = mock({ workflows: mock({ useWorkflowPublicationService: false }), @@ -163,7 +162,6 @@ describe('WorkflowService', () => { workflowRoles: expect.any(Array), }), undefined, - undefined, // callableForParentWorkflowId ); }); @@ -183,7 +181,6 @@ describe('WorkflowService', () => { workflowRoles: expect.any(Array), }), undefined, - undefined, // callableForParentWorkflowId ); }); @@ -203,7 +200,6 @@ describe('WorkflowService', () => { workflowRoles: expect.any(Array), }), undefined, - undefined, // callableForParentWorkflowId ); }); @@ -223,110 +219,82 @@ describe('WorkflowService', () => { workflowRoles: expect.any(Array), }), undefined, - undefined, // callableForParentWorkflowId ); }); - describe('callableForParentWorkflowId', () => { - test('should pass parentWorkflowId when includeCallableSubworkflows is true', async () => { + describe('getCallableSubworkflows', () => { + test('should query with read roles and the Execute Workflow Trigger node type', async () => { const user = mock(); - const options = { - filter: { - includeCallableSubworkflows: true, - parentWorkflowId: 'parent-wf-id', - }, - }; + workflowRepositoryMock.findCallableSubworkflows.mockResolvedValue({ + workflows: [], + count: 0, + }); - await workflowService.getMany(user, options); + await workflowService.getCallableSubworkflows(user, 'parent-wf-id', { + query: 'search', + skip: 10, + take: 5, + }); - expect(workflowRepositoryMock.getManyAndCountWithSharingSubquery).toHaveBeenCalledWith( + expect(roleServiceMock.rolesWithScope).toHaveBeenCalledWith('project', ['workflow:read']); + expect(roleServiceMock.rolesWithScope).toHaveBeenCalledWith('workflow', ['workflow:read']); + expect(workflowRepositoryMock.findCallableSubworkflows).toHaveBeenCalledWith( user, - expect.any(Object), - options, + { + scopes: ['workflow:read'], + projectRoles: ['project:viewer'], + workflowRoles: ['project:viewer'], + }, 'parent-wf-id', + { + query: 'search', + triggerNodeTypes: ['n8n-nodes-base.executeWorkflowTrigger'], + skip: 10, + take: 5, + }, ); }); - test('should pass undefined when includeCallableSubworkflows is false', async () => { - const user = mock(); - const options = { - filter: { - includeCallableSubworkflows: false, - parentWorkflowId: 'parent-wf-id', - }, - }; + test('should return minimal rows with the project name for team projects only', async () => { + workflowRepositoryMock.findCallableSubworkflows.mockResolvedValue({ + count: 2, + workflows: [ + { + id: 'team-wf', + name: 'Team', + activeVersionId: 'version-1', + isArchived: false, + nodes: [{ type: 'n8n-nodes-base.code' }], + shared: [{ project: { type: 'team', name: 'Team Project' } }], + }, + { + id: 'personal-wf', + name: 'Personal', + activeVersionId: null, + isArchived: true, + shared: [{ project: { type: 'personal', name: 'Jane ' } }], + }, + ], + }); - await workflowService.getMany(user, options); + const result = await workflowService.getCallableSubworkflows(mock(), 'parent-wf-id', { + skip: 0, + take: 10, + }); - expect(workflowRepositoryMock.getManyAndCountWithSharingSubquery).toHaveBeenCalledWith( - user, - expect.any(Object), - options, - undefined, - ); - }); - - test('should pass undefined when includeCallableSubworkflows is true but parentWorkflowId is missing', async () => { - const user = mock(); - const options = { - filter: { - includeCallableSubworkflows: true, - }, - }; - - await workflowService.getMany(user, options); - - expect(workflowRepositoryMock.getManyAndCountWithSharingSubquery).toHaveBeenCalledWith( - user, - expect.any(Object), - options, - undefined, - ); - }); - - test('should pass undefined when includeCallableSubworkflows is true but parentWorkflowId is not a string', async () => { - const user = mock(); - const options = { - filter: { - includeCallableSubworkflows: true, - parentWorkflowId: 123, // invalid type - }, - }; - - await workflowService.getMany(user, options); - - expect(workflowRepositoryMock.getManyAndCountWithSharingSubquery).toHaveBeenCalledWith( - user, - expect.any(Object), - options, - undefined, - ); - }); - - test('should pass undefined when the requester cannot read the parent workflow', async () => { - const user = mock(); - const options = { - filter: { - includeCallableSubworkflows: true, - parentWorkflowId: 'parent-wf-id', - }, - }; - // Requester has no read access to the supplied parent workflow. - workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(null); - - await workflowService.getMany(user, options); - - expect(workflowFinderServiceMock.findWorkflowForUser).toHaveBeenCalledWith( - 'parent-wf-id', - user, - ['workflow:read'], - ); - expect(workflowRepositoryMock.getManyAndCountWithSharingSubquery).toHaveBeenCalledWith( - user, - expect.any(Object), - options, - undefined, - ); + expect(result).toEqual({ + count: 2, + data: [ + { + id: 'team-wf', + name: 'Team', + active: true, + isArchived: false, + projectName: 'Team Project', + }, + { id: 'personal-wf', name: 'Personal', active: false, isArchived: true }, + ], + }); }); }); @@ -461,12 +429,10 @@ describe('WorkflowService', () => { expect(result).toMatchObject({ workflows: [folder], count: 1 }); expect( workflowRepositoryMock.getWorkflowsAndFoldersWithCountWithSharingSubquery, - ).toHaveBeenCalledWith( - expect.anything(), - expect.anything(), - { ...restrictedNodeFilter, workflowIdsIn: null }, - undefined, - ); + ).toHaveBeenCalledWith(expect.anything(), expect.anything(), { + ...restrictedNodeFilter, + workflowIdsIn: null, + }); }); test('limits the list query to the restricted workflow ids', async () => { @@ -479,7 +445,6 @@ describe('WorkflowService', () => { expect.anything(), expect.anything(), { ...restrictedNodeFilter, workflowIdsIn }, - undefined, ); }); }); diff --git a/packages/cli/src/workflows/workflow-creation.service.ts b/packages/cli/src/workflows/workflow-creation.service.ts index 825ca3e30f3..86c553ad78f 100644 --- a/packages/cli/src/workflows/workflow-creation.service.ts +++ b/packages/cli/src/workflows/workflow-creation.service.ts @@ -17,6 +17,7 @@ import { v4 as uuid } from 'uuid'; import { BadRequestError, ForbiddenError, InternalServerError, NotFoundError } from '@n8n/errors'; import { WorkflowValidationError } from '@/errors/response-errors/workflow-validation.error'; +import { UninspectableSubworkflowSourceError } from '@/errors/uninspectable-subworkflow-source.error'; import type { WorkflowActionSource } from '@/events/maps/relay.event-map'; import { ExternalHooks, toWorkflowLifecycleHookActor } from '@/external-hooks'; import { validateEntity } from '@/generic-helpers'; @@ -239,8 +240,11 @@ export class WorkflowCreationService { if (this.licenseState.isSharingLicensed()) { // This is a new workflow, so we simply check if the user has access to // all used credentials - const { ids: credentialIds, hasUnresolved } = - this.enterpriseWorkflowService.collectCredentialReferences(newWorkflow); + const { + ids: credentialIds, + hasUnresolved, + hasUninspectableSource, + } = this.enterpriseWorkflowService.collectCredentialReferences(newWorkflow); if (batchContext) { const uncheckedIds = [...credentialIds].filter( (id) => !batchContext.checkedCredentialIds.has(id), @@ -270,8 +274,15 @@ export class WorkflowCreationService { this.enterpriseWorkflowService.validateCredentialPermissionsToUser( newWorkflow, hasUnresolved ? new Set() : accessibleCredentialIds, + { + allowUninspectableSource: + hasUninspectableSource && this.enterpriseWorkflowService.canUseEveryCredential(user), + }, ); } catch (error) { + if (error instanceof UninspectableSubworkflowSourceError) { + throw new BadRequestError(error.message); + } throw new BadRequestError( 'The workflow you are trying to save contains credentials that are not shared with you', ); diff --git a/packages/cli/src/workflows/workflow.service.ee.ts b/packages/cli/src/workflows/workflow.service.ee.ts index c9d5385afd5..16433b6059e 100644 --- a/packages/cli/src/workflows/workflow.service.ee.ts +++ b/packages/cli/src/workflows/workflow.service.ee.ts @@ -21,10 +21,12 @@ import { hasGlobalScope } from '@n8n/permissions'; import { isRecord } from '@n8n/utils/is-record'; import { In, type EntityManager } from '@n8n/typeorm'; import isEqual from 'lodash/isEqual'; -import type { INode, IWorkflowBase, WorkflowId } from 'n8n-workflow'; +import omit from 'lodash/omit'; +import type { INode, INodeTypeDescription, IWorkflowBase, WorkflowId } from 'n8n-workflow'; import { isNodeWithWorkflowSelector, jsonParse, + NodeHelpers, NodeOperationError, PROJECT_ROOT, UserError, @@ -40,14 +42,46 @@ import { CredentialsPermissionChecker } from '@/executions/pre-execution-checks/ import { FolderNotFoundError } from '@/errors/folder-not-found.error'; import { BadRequestError, NotFoundError } from '@n8n/errors'; import { TransferWorkflowError } from '@/errors/response-errors/transfer-workflow.error'; +import { UninspectableSubworkflowSourceError } from '@/errors/uninspectable-subworkflow-source.error'; import { extractAgentCredentialIds } from '@/modules/agents/utils/extract-agent-credential-ids'; +import { NodeTypes } from '@/node-types'; import { PolicyEnforcementService } from '@/policy/policy-enforcement.service'; import { OwnershipService } from '@/services/ownership.service'; import { ProjectService } from '@/services/project.service.ee'; +import { stripToolSuffix } from '@/utils'; import { WorkflowFinderService } from './workflow-finder.service'; import { WorkflowMutationHooksProxy } from './workflow-mutation-hooks-proxy.service'; +/** + * Database workflows use their own project permissions at runtime. + * Parameter workflows contain JSON we can inspect before saving. + */ +const INSPECTABLE_SUBWORKFLOW_SOURCES = new Set(['database', 'parameter']); + +/** Exclude allowed edits and temporary editor state from the definition comparison. */ +const FREELY_EDITABLE_NODE_FIELDS = ['name', 'position', 'disabled', 'color', 'issues', 'status']; + +/** Tool and HITL tool variants use the same source rules as their base node. */ +function isWorkflowSelectorNode(node: INode): boolean { + if (typeof node.type !== 'string') return false; + return isNodeWithWorkflowSelector({ ...node, type: stripToolSuffix(node.type) }); +} + +type CredentialReferences = { + /** Resolvable credential ids. */ + ids: string[]; + /** A non-managed credential carrying no id, i.e. only a name. */ + hasUnresolved: boolean; + /** A sub-workflow source whose content is not available at save time. */ + hasUninspectableSource: boolean; +}; + +export type CredentialGuardOptions = { + /** Allow content loaded at runtime only for users who can use every credential. */ + allowUninspectableSource?: boolean; +}; + @Service() export class EnterpriseWorkflowService { constructor( @@ -66,6 +100,7 @@ export class EnterpriseWorkflowService { private readonly workflowPublishHistoryRepository: WorkflowPublishHistoryRepository, private readonly workflowMutationHooks: WorkflowMutationHooksProxy, private readonly policyEnforcementService: PolicyEnforcementService, + private readonly nodeTypes: NodeTypes, private readonly credentialsPermissionChecker: CredentialsPermissionChecker, ) {} @@ -185,6 +220,7 @@ export class EnterpriseWorkflowService { validateCredentialPermissionsToUser( workflow: IWorkflowBase, allowedCredentials: Array<{ id: string }> | ReadonlySet, + options: CredentialGuardOptions = {}, ) { // Reuse the shared collector so inline sub-workflow credentials (Execute // Sub-workflow, Workflow Tool, Workflow Retriever) and unresolved name-only @@ -192,8 +228,16 @@ export class EnterpriseWorkflowService { const allowedCredentialIds: ReadonlySet = Array.isArray(allowedCredentials) ? new Set(allowedCredentials.map(({ id }) => id)) : allowedCredentials; - const inaccessibleNodes = this.getNodesWithInaccessibleCreds(workflow, allowedCredentialIds); + const inaccessibleNodes = this.getNodesWithInaccessibleCreds( + workflow, + allowedCredentialIds, + options, + ); if (inaccessibleNodes.length > 0) { + const uninspectableNode = inaccessibleNodes.find((node) => + this.isBlockedByUninspectableSource(node, options), + ); + if (uninspectableNode) throw new UninspectableSubworkflowSourceError(uninspectableNode); throw new UserError('The workflow contains credentials that you do not have access to'); } } @@ -224,6 +268,7 @@ export class EnterpriseWorkflowService { previousVersion, allCredentials, restoredNodes, + { allowUninspectableSource: this.canUseEveryCredential(user) }, ); } catch (error) { if (error instanceof NodeOperationError) { @@ -240,20 +285,12 @@ export class EnterpriseWorkflowService { previousWorkflowVersion: IWorkflowBase, credentialsUserHasAccessTo: Array<{ id: string }>, restoredNodes: INode[] = [], + options: CredentialGuardOptions = {}, ) { - /** - * We only need to check nodes that use credentials the current user cannot access, - * since these can be 2 possibilities: - * - It matches exactly one previous node that already used such a credential: it's a - * read only node and therefore cannot be changed - * - It's a new node, or an editable node that newly references such a credential, - * which indicates tampering and therefore must fail saving - */ - const allowedCredentialIds = credentialsUserHasAccessTo.map((cred) => cred.id); const nodesWithCredentialsUserDoesNotHaveAccessTo = new Set( - this.getNodesWithInaccessibleCreds(newWorkflowVersion, allowedCredentialIds), + this.getNodesWithInaccessibleCreds(newWorkflowVersion, allowedCredentialIds, options), ); // If there are no nodes with credentials the user does not have access to we can skip the rest @@ -265,9 +302,11 @@ export class EnterpriseWorkflowService { // the user could edit that now carries a credential they cannot use fails the // save like a new node does; restoring it would drop the other edits silently. const readOnlyNodeIds = new Set( - this.getNodesWithInaccessibleCreds(previousWorkflowVersion, allowedCredentialIds).map( - (node) => node.id, - ), + this.getNodesWithInaccessibleCreds( + previousWorkflowVersion, + allowedCredentialIds, + options, + ).map((node) => node.id), ); // Node ids are meant to be unique. Edge case: where stored data still repeats one, match the @@ -295,6 +334,7 @@ export class EnterpriseWorkflowService { // ambiguous, and an ambiguous match is not a proof, so no claimant is trusted. const previousNode = previousNodesById.get(node.id); const idClaimedOnce = submittedIdCounts.get(node.id) === 1; + const blockedBySource = this.isBlockedByUninspectableSource(node, options); // A restore brings back a node exactly as a version from this workflow's history // stored it. Any change to it makes it a new edit, which the checks below judge. @@ -308,12 +348,22 @@ export class EnterpriseWorkflowService { nodeId: node.id, nodeCredentials: node.credentials, }); + if (blockedBySource) throw new UninspectableSubworkflowSourceError(node); throw new NodeOperationError( node, `You don't have access to the credentials in the '${node.name}' node. Ask the owner to share them with you.`, ); } + // The editor allows these source settings to be edited. Reject changes + // instead of silently discarding them when we restore the stored node. + if ( + blockedBySource && + !isEqual(this.getNodeDefinition(node), this.getNodeDefinition(previousNode)) + ) { + throw new UninspectableSubworkflowSourceError(node); + } + this.logger.debug('Replacing node with previous version when saving updated workflow', { nodeType: node.type, nodeName: node.name, @@ -335,62 +385,120 @@ export class EnterpriseWorkflowService { } /** - * Get all nodes in a workflow whose credentials the user cannot use: either a - * referenced credential id is not accessible, or the node carries an unresolved - * non-managed reference (id null/empty) that could resolve by name to a - * credential the user does not own. Inline sub-workflow credentials are included. + * Find nodes with inaccessible credentials, unresolved references, or blocked sources. + * Include credentials and sources in nested workflows and agents. */ - getNodesWithInaccessibleCreds(workflow: IWorkflowBase, userCredIds: Iterable) { + getNodesWithInaccessibleCreds( + workflow: IWorkflowBase, + userCredIds: Iterable, + options: CredentialGuardOptions = {}, + ) { if (!workflow.nodes) { return []; } const allowedCredentialIds = userCredIds instanceof Set ? userCredIds : new Set(userCredIds); return workflow.nodes.filter((node) => { - const { ids, hasUnresolved } = this.getNodeCredentialRefs(node); - return hasUnresolved || ids.some((credId) => !allowedCredentialIds.has(credId)); + const { ids, hasUnresolved, hasUninspectableSource } = this.getNodeCredentialRefs(node); + return ( + hasUnresolved || + (hasUninspectableSource && !options.allowUninspectableSource) || + ids.some((credId) => !allowedCredentialIds.has(credId)) + ); }); } collectCredentialReferences(workflow: IWorkflowBase): { ids: Set; hasUnresolved: boolean; + hasUninspectableSource: boolean; } { const ids = new Set(); let hasUnresolved = false; + let hasUninspectableSource = false; for (const node of workflow.nodes ?? []) { const references = this.getNodeCredentialRefs(node); for (const id of references.ids) ids.add(id); hasUnresolved ||= references.hasUnresolved; + hasUninspectableSource ||= references.hasUninspectableSource; } - return { ids, hasUnresolved }; + return { ids, hasUnresolved, hasUninspectableSource }; + } + + canUseEveryCredential(user: User): boolean { + return hasGlobalScope(user, 'credential:use'); + } + + private isBlockedByUninspectableSource(node: INode, options: CredentialGuardOptions): boolean { + if (options.allowUninspectableSource) return false; + return this.getNodeCredentialRefs(node).hasUninspectableSource; } /** - * Collect the credential references a node uses. Besides the node's own - * `credentials`, a node with an inline workflow selector (Execute - * Sub-workflow, Workflow Tool, Workflow Retriever) embeds a whole workflow — - * including its nodes' credentials — inside its `workflowJson` string - * parameter, so those references are walked as well. - * - * Returns `ids` (resolvable credential ids) and `hasUnresolved` (a non-managed - * credential carrying no id, i.e. only a name). A name-only reference can be - * resolved by name to a credential the user cannot access, so callers gating a - * save must reject it rather than treat the node as credential-free. - * `__aiGatewayManaged` credentials with a null id are resolved at execution and - * are exempt. - * - * An inline sub-workflow may itself contain inline sub-workflows, so the walk - * is iterative over an explicit stack: every referenced credential is - * inspected regardless of nesting depth, with no silent cut-off that could let - * a deeply nested reference escape validation. Nesting is inherently bounded by - * the request size (each level embeds its child as literal escaped JSON) and - * cannot cycle, so the stack cannot grow unbounded. + * Compare node behavior without treating editor formatting or allowed edits as changes. */ - private getNodeCredentialRefs(node: INode): { ids: string[]; hasUnresolved: boolean } { + private getNodeDefinition(node: INode): Record { + const definition: Record = omit(node, FREELY_EDITABLE_NODE_FIELDS); + definition.parameters = this.getDisplayedParameters(node); + + const credentials = node.credentials ?? {}; + if (Object.keys(credentials).length === 0) delete definition.credentials; + // The editor persists these only when they differ from their default. + if (!node.continueOnFail) delete definition.continueOnFail; + if (!node.onError || node.onError === 'stopWorkflow') delete definition.onError; + if (!node.notes) delete definition.notes; + + return definition; + } + + /** + * The editor omits default values and hidden parameters on save. + * Compare displayed parameters with defaults filled in so an unchanged node still matches. + */ + private getDisplayedParameters(node: INode) { + const parameters = node.parameters ?? {}; + const description = this.getNodeTypeDescription(node); + if (!description) return parameters; + return ( + NodeHelpers.getNodeParameters( + description.properties, + parameters, + true, + false, + node, + description, + ) ?? {} + ); + } + + private getNodeTypeDescription(node: INode): INodeTypeDescription | null { + try { + return this.nodeTypes.getByNameAndVersion(node.type, node.typeVersion)?.description ?? null; + } catch { + return null; + } + } + + /** + * A missing source defaults to Database. Expressions are not resolved until runtime. + */ + private hasUninspectableSubworkflowSource(node: INode): boolean { + const source = node.parameters?.source; + if (source === undefined) return false; + return !(typeof source === 'string' && INSPECTABLE_SUBWORKFLOW_SOURCES.has(source)); + } + + /** + * Inspect the node and its embedded workflows and agents at every nesting level. + * Report name-only credentials and runtime-loaded sources separately from known IDs; + * neither means the node uses no credentials. Managed Gateway credentials are exempt. + * Use a stack to avoid a call-stack limit. Serialized input bounds the nesting depth. + */ + private getNodeCredentialRefs(node: INode): CredentialReferences { const ids: string[] = []; let hasUnresolved = false; + let hasUninspectableSource = false; const stack: INode[] = [node]; while (stack.length > 0) { @@ -409,7 +517,8 @@ export class EnterpriseWorkflowService { } } - if (isNodeWithWorkflowSelector(current)) { + if (isWorkflowSelectorNode(current)) { + hasUninspectableSource ||= this.hasUninspectableSubworkflowSource(current); stack.push(...this.getInlineWorkflowNodes(current.parameters?.workflowJson)); } @@ -420,7 +529,7 @@ export class EnterpriseWorkflowService { } } - return { ids, hasUnresolved }; + return { ids, hasUnresolved, hasUninspectableSource }; } /** @@ -452,9 +561,8 @@ export class EnterpriseWorkflowService { const nested = Object.values(config).flatMap((value) => this.getAgentToolNodes(value)); if (typeof config.nodeType !== 'string') return nested; - // A node runs as a tool under its `…Tool` variant, which carries the same - // parameters as the node itself. - const type = config.nodeType.replace(/Tool$/, ''); + // Tool variants keep the base node's parameters. + const type = stripToolSuffix(config.nodeType); const parameters = isRecord(config.nodeParameters) ? config.nodeParameters : {}; return [...nested, { type, parameters } as INode]; diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index 744856d2d60..6c05db4dc80 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -1,5 +1,10 @@ import { UpdateWorkflowHistoryVersionDto } from '@n8n/api-types'; -import type { WorkflowListPublicationStatus, WorkflowExecutionBlockCause } from '@n8n/api-types'; +import type { + CallableSubworkflowsResponse, + ListCallableSubworkflowsQueryDto, + WorkflowExecutionBlockCause, + WorkflowListPublicationStatus, +} from '@n8n/api-types'; import { LicenseState, Logger } from '@n8n/backend-common'; import { EventService, RoleService } from '@n8n/backend-services'; import { GlobalConfig } from '@n8n/config'; @@ -38,7 +43,13 @@ import { ensureError } from '@n8n/utils/errors/ensure-error'; import isEqual from 'lodash/isEqual'; import pick from 'lodash/pick'; import type { INode, INodes, IWorkflowSettings, JsonValue, IConnections } from 'n8n-workflow'; -import { PROJECT_ROOT, Workflow, assert, calculateWorkflowChecksum } from 'n8n-workflow'; +import { + EXECUTE_WORKFLOW_TRIGGER_NODE_TYPE, + PROJECT_ROOT, + Workflow, + assert, + calculateWorkflowChecksum, +} from 'n8n-workflow'; import { v4 as uuid } from 'uuid'; import { @@ -303,11 +314,6 @@ export class WorkflowService { sharingOptions.workflowRoles = workflowRoles; } - const callableForParentWorkflowId = await this.resolveCallableForParentWorkflowId( - user, - options, - ); - const workflowIdsIn = await this.resolveRestrictedWorkflowIds(options); const listOptions = workflowIdsIn === undefined ? options : { ...options, workflowIdsIn }; @@ -318,7 +324,6 @@ export class WorkflowService { user, sharingOptions, listOptions, - callableForParentWorkflowId, ); workflows = workflowsAndFolders.filter((wf) => wf.resource === 'workflow'); @@ -327,7 +332,6 @@ export class WorkflowService { user, sharingOptions, listOptions, - callableForParentWorkflowId, )); } @@ -380,25 +384,42 @@ export class WorkflowService { }; } - private async resolveCallableForParentWorkflowId( + /** + * List the workflows that `parentWorkflowId` can call as sub-workflows, for + * the sub-workflow picker. The caller must check that the user can update + * the parent workflow. Each row holds only the fields the picker shows. + */ + async getCallableSubworkflows( user: User, - options?: ListQuery.Options, - ): Promise { - if (options?.filter?.includeCallableSubworkflows !== true) return undefined; + parentWorkflowId: string, + { query, skip, take }: ListCallableSubworkflowsQueryDto, + ): Promise { + const requiredScopes: Scope[] = ['workflow:read']; + const [projectRoles, workflowRoles] = await Promise.all([ + this.roleService.rolesWithScope('project', requiredScopes), + this.roleService.rolesWithScope('workflow', requiredScopes), + ]); - const parentWorkflowId = - typeof options.filter.parentWorkflowId === 'string' - ? options.filter.parentWorkflowId - : undefined; - if (!parentWorkflowId) return undefined; - - const parentWorkflow = await this.workflowFinderService.findWorkflowForUser( - parentWorkflowId, + const { workflows, count } = await this.workflowRepository.findCallableSubworkflows( user, - ['workflow:read'], + { scopes: requiredScopes, projectRoles, workflowRoles }, + parentWorkflowId, + { query, triggerNodeTypes: [EXECUTE_WORKFLOW_TRIGGER_NODE_TYPE], skip, take }, ); - return parentWorkflow ? parentWorkflowId : undefined; + const data = workflows.map((workflow) => { + const project = workflow.shared?.[0]?.project; + return { + id: workflow.id, + name: workflow.name, + active: workflow.activeVersionId !== null, + isArchived: workflow.isArchived, + // Personal project names hold user names and emails, so only team names are returned. + ...(project?.type === 'team' ? { projectName: project.name } : {}), + }; + }); + + return { count, data }; } private async resolveRestrictedWorkflowIds( @@ -1731,14 +1752,18 @@ export class WorkflowService { // workflow cannot be deleted and unpublishing deprovisions them. This covers // the paths that never published and anything an interrupted unpublish left. // - // Both writes commit together. A deprovision of its own would leave a workflow + // All three writes commit together. A deprovision of its own would leave a workflow // whose delete then failed alive with its schedules stripped. - await this.workflowRepository.runInTransaction({}, async (trx) => { + await this.workflowRepository.runInTransaction({}, async (trx, ctx) => { await this.durableJobProvisioner.deprovisionOwnerInTransaction( trx, this.workflowScheduledJobOwner.ref(workflowId), ); + // A new workflow can be created at this id once the row is gone, so allowlists lose + // the entry in the same transaction. + await this.workflowRepository.removeFromCallerAllowlists([workflowId], ctx); + await trx.delete(WorkflowEntity, { id: workflowId }); }); diff --git a/packages/cli/src/workflows/workflows.controller.ts b/packages/cli/src/workflows/workflows.controller.ts index 846a0082f30..7c8a6fb8b49 100644 --- a/packages/cli/src/workflows/workflows.controller.ts +++ b/packages/cli/src/workflows/workflows.controller.ts @@ -5,9 +5,11 @@ import { DeactivateWorkflowDto, ExecutionRedactionQueryDtoSchema, ImportWorkflowFromUrlDto, + ListCallableSubworkflowsQueryDto, ManualRunDto, TransferWorkflowBodyDto, UpdateWorkflowDto, + type CallableSubworkflowsResponse, type WorkflowPublicationStatus, } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; @@ -717,6 +719,22 @@ export class WorkflowsController { return await this.workflowPublicationStatusService.getStatus(workflowId); } + /** + * Lists the workflows that this workflow can call as sub-workflows. Only + * editors of the parent can add a sub-workflow call, so this requires + * `workflow:update` on the parent. + */ + @Get('/:workflowId/callable-subworkflows') + @ProjectScope('workflow:update') + async getCallableSubworkflows( + req: AuthenticatedRequest, + _res: unknown, + @Param('workflowId') workflowId: string, + @Query query: ListCallableSubworkflowsQueryDto, + ): Promise { + return await this.workflowService.getCallableSubworkflows(req.user, workflowId, query); + } + @Post('/with-node-types') async getWorkflowsWithNodesIncluded(req: AuthenticatedRequest, res: express.Response) { try { diff --git a/packages/cli/test/integration/dynamic-credentials.ee/dynamic-credentials.auth.api.test.ts b/packages/cli/test/integration/dynamic-credentials.ee/dynamic-credentials.auth.api.test.ts index 573327a2e92..0b1dadb8605 100644 --- a/packages/cli/test/integration/dynamic-credentials.ee/dynamic-credentials.auth.api.test.ts +++ b/packages/cli/test/integration/dynamic-credentials.ee/dynamic-credentials.auth.api.test.ts @@ -280,6 +280,9 @@ describe('Dynamic Credentials API', () => { }); }); + // `authSource=cookie` makes the caller's own n8n session token the identity the + // resolver is asked to validate. The resolver here keys on an external subject + // (`credential-resolver.oauth2-1.0`), so it must not be handed that identity. describe('when the identity is taken from the session cookie', () => { it('should refuse to authorize against a resolver that does not resolve n8n users', async () => { const response = await testServer diff --git a/packages/cli/test/integration/shared/utils/index.ts b/packages/cli/test/integration/shared/utils/index.ts index b236195ec45..937b453490d 100644 --- a/packages/cli/test/integration/shared/utils/index.ts +++ b/packages/cli/test/integration/shared/utils/index.ts @@ -112,7 +112,7 @@ function minimalNodeType(name: string, properties: INodeProperties[] = []): INod }; } -function buildDefaultNodes(): INodeTypeData { +export function buildDefaultNodes(): INodeTypeData { ScheduleTrigger.prototype.trigger = async () => ({}); return { 'n8n-nodes-base.manualTrigger': { diff --git a/packages/cli/test/integration/workflows/workflow.service.ee.test.ts b/packages/cli/test/integration/workflows/workflow.service.ee.test.ts index 2ea048c719c..ff3f76e42b9 100644 --- a/packages/cli/test/integration/workflows/workflow.service.ee.test.ts +++ b/packages/cli/test/integration/workflows/workflow.service.ee.test.ts @@ -43,6 +43,7 @@ describe('EnterpriseWorkflowService', () => { mock(), // workflowPublishHistoryRepository mock(), // workflowMutationHooks mock(), // policyEnforcementService + mock(), // nodeTypes mock(), // credentialsPermissionChecker ); }); diff --git a/packages/cli/test/integration/workflows/workflow.service.test.ts b/packages/cli/test/integration/workflows/workflow.service.test.ts index 99b92824676..6b09fadbff3 100644 --- a/packages/cli/test/integration/workflows/workflow.service.test.ts +++ b/packages/cli/test/integration/workflows/workflow.service.test.ts @@ -29,6 +29,8 @@ import { v4 as uuid } from 'uuid'; import { mock } from 'vitest-mock-extended'; import { ActiveWorkflowManager } from '@/active-workflow-manager'; +import { SubworkflowPolicyDenialError } from '@/errors/subworkflow-policy-denial.error'; +import { SubworkflowPolicyChecker } from '@/executions/pre-execution-checks/subworkflow-policy-checker'; import type { ExternalHooks } from '@/external-hooks'; import { MessageEventBus } from '@/eventbus/message-event-bus/message-event-bus'; import { NodeTypes } from '@/node-types'; @@ -1459,3 +1461,227 @@ describe('publishAsSystem()', () => { expect(workflowPublicationNotifier.requestDrain).not.toHaveBeenCalled(); }); }); + +describe('delete() and the workflowsFromAList caller policy', () => { + test('denies a caller whose allowlist entry no longer refers to a live workflow', async () => { + const owner = await createOwner(); + const member = await createMember(); + + const allowlistedCaller = await createWorkflow({}, owner); + const subworkflow = await createWorkflow( + { + settings: { callerPolicy: 'workflowsFromAList', callerIds: allowlistedCaller.id }, + }, + owner, + ); + + await workflowService.archive(owner, allowlistedCaller.id); + await workflowService.delete(owner, allowlistedCaller.id); + + const reusedIdWorkflow = await createWorkflow({ id: allowlistedCaller.id }, member); + + const storedSubworkflow = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + + await expect( + Container.get(SubworkflowPolicyChecker).check( + { id: storedSubworkflow.id, settings: storedSubworkflow.settings }, + reusedIdWorkflow.id, + ), + ).rejects.toThrowError(SubworkflowPolicyDenialError); + }); + + test('keeps the allowlist entry while the caller is only archived', async () => { + const owner = await createOwner(); + + const allowlistedCaller = await createWorkflow({}, owner); + const callerIds = `keepMe,\n${allowlistedCaller.id}`; + const subworkflow = await createWorkflow( + { settings: { callerPolicy: 'workflowsFromAList', callerIds } }, + owner, + ); + + await workflowService.archive(owner, allowlistedCaller.id); + + const afterArchive = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + expect(afterArchive.settings).toEqual({ callerPolicy: 'workflowsFromAList', callerIds }); + + await workflowService.delete(owner, allowlistedCaller.id); + + const afterDelete = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + expect(afterDelete.settings).toEqual({ + callerPolicy: 'workflowsFromAList', + callerIds: 'keepMe', + }); + }); + + test('allows a live allowlisted caller owned by another project', async () => { + const owner = await createOwner(); + const member = await createMember(); + + const allowlistedCaller = await createWorkflow({}, member); + const subworkflow = await createWorkflow( + { + settings: { callerPolicy: 'workflowsFromAList', callerIds: allowlistedCaller.id }, + }, + owner, + ); + + const storedSubworkflow = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + + await expect( + Container.get(SubworkflowPolicyChecker).check( + { id: storedSubworkflow.id, settings: storedSubworkflow.settings }, + allowlistedCaller.id, + ), + ).resolves.toBeUndefined(); + }); + + test('keeps the other settings of an allowlist it rewrites', async () => { + const owner = await createOwner(); + + const caller = await createWorkflow({}, owner); + const subworkflow = await createWorkflow( + { + settings: { + callerPolicy: 'workflowsFromAList', + callerIds: `${caller.id},another-caller`, + executionOrder: 'v1', + timezone: 'Europe/Berlin', + }, + }, + owner, + ); + + await workflowService.archive(owner, caller.id); + await workflowService.delete(owner, caller.id); + + const after = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + expect(after.settings).toEqual({ + callerPolicy: 'workflowsFromAList', + callerIds: 'another-caller', + executionOrder: 'v1', + timezone: 'Europe/Berlin', + }); + }); + + test('leaves an allowlist alone when the deleted id only appears inside another entry', async () => { + const owner = await createOwner(); + + const caller = await createWorkflow({}, owner); + const subworkflow = await createWorkflow( + { + settings: { + callerPolicy: 'workflowsFromAList', + callerIds: `${caller.id}-suffix, ${caller.id}-other`, + }, + }, + owner, + ); + const before = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + + await workflowService.archive(owner, caller.id); + await workflowService.delete(owner, caller.id); + + const after = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + expect(after.settings).toEqual(before.settings); + expect(after.updatedAt).toEqual(before.updatedAt); + }); + + test('takes several deleted ids out of one allowlist', async () => { + const owner = await createOwner(); + + const first = await createWorkflow({}, owner); + const second = await createWorkflow({}, owner); + const subworkflow = await createWorkflow( + { + settings: { + callerPolicy: 'workflowsFromAList', + callerIds: `${first.id},keepMe,${second.id}`, + }, + }, + owner, + ); + + await workflowRepository.removeFromCallerAllowlists([first.id, second.id], {}); + + const after = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + expect(after.settings).toEqual({ callerPolicy: 'workflowsFromAList', callerIds: 'keepMe' }); + }); + + test('leaves every allowlist untouched when the deleted workflow is on none of them', async () => { + const owner = await createOwner(); + + const allowlistedCaller = await createWorkflow({}, owner); + const subworkflow = await createWorkflow( + { + settings: { + callerPolicy: 'workflowsFromAList', + callerIds: `${allowlistedCaller.id},another-caller`, + }, + }, + owner, + ); + const before = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + + const unrelatedWorkflow = await createWorkflow({}, owner); + await workflowService.archive(owner, unrelatedWorkflow.id); + await workflowService.delete(owner, unrelatedWorkflow.id); + + const after = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + expect(after.settings).toEqual(before.settings); + expect(after.updatedAt).toEqual(before.updatedAt); + }); + + // A character the `json` column keeps but the binary JSON form cannot hold. + const NO_BINARY_JSON_FORM = String.fromCharCode(0); + + test('deletes a workflow while an allowlist elsewhere holds a value json alone can carry', async () => { + const owner = await createOwner(); + + const subworkflow = await createWorkflow( + { + settings: { + callerPolicy: 'workflowsFromAList', + callerIds: `another-caller${NO_BINARY_JSON_FORM}`, + }, + }, + owner, + ); + const before = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + + const unrelatedWorkflow = await createWorkflow({}, owner); + await workflowService.archive(owner, unrelatedWorkflow.id); + await workflowService.delete(owner, unrelatedWorkflow.id); + + await expect(workflowRepository.findOneBy({ id: unrelatedWorkflow.id })).resolves.toBeNull(); + + const after = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + expect(after.settings).toEqual(before.settings); + }); + + test('rewrites an allowlist that holds a value json alone can carry', async () => { + const owner = await createOwner(); + + const caller = await createWorkflow({}, owner); + const subworkflow = await createWorkflow( + { + settings: { + callerPolicy: 'workflowsFromAList', + callerIds: `${caller.id},another-caller`, + errorWorkflow: `error-workflow${NO_BINARY_JSON_FORM}`, + }, + }, + owner, + ); + + await workflowService.archive(owner, caller.id); + await workflowService.delete(owner, caller.id); + + const after = await workflowRepository.findOneByOrFail({ id: subworkflow.id }); + expect(after.settings).toEqual({ + callerPolicy: 'workflowsFromAList', + callerIds: 'another-caller', + errorWorkflow: `error-workflow${NO_BINARY_JSON_FORM}`, + }); + }); +}); diff --git a/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts b/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts index 7735d13f068..5dd1907f9f9 100644 --- a/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts +++ b/packages/cli/test/integration/workflows/workflows.controller.ee.test.ts @@ -38,6 +38,7 @@ import { calculateWorkflowChecksum, type INode, } from 'n8n-workflow'; +import { ExecuteWorkflow } from 'n8n-nodes-base/nodes/ExecuteWorkflow/ExecuteWorkflow/ExecuteWorkflow.node'; import { v4 as uuid } from 'uuid'; import { ActiveWorkflowManager } from '@/active-workflow-manager'; @@ -103,7 +104,11 @@ beforeAll(async () => { saveCredential = affixRoleToSaveCredential('credential:owner'); - await utils.initNodeTypes(); + // Use the real node description to test parameter defaults and visibility. + await utils.initNodeTypes({ + ...utils.buildDefaultNodes(), + 'n8n-nodes-base.executeWorkflow': { type: new ExecuteWorkflow(), sourcePath: '' }, + }); }); beforeEach(async () => { @@ -1359,6 +1364,131 @@ describe('PATCH /workflows/:workflowId', () => { }, ); + describe('sub-workflow loaded from a URL or file', () => { + const UNINSPECTABLE_SOURCE_MESSAGE = + 'The \'Sub\' node loads its sub-workflow from a source that is resolved only when the workflow runs, so its credential access cannot be checked on save. Use the "Database" or "Define Below" source instead.'; + + const remoteSubworkflowNode = (workflowUrl = 'https://example.com/workflow.json') => ({ + id: 'uuid-sub-1', + name: 'Sub', + type: 'n8n-nodes-base.executeWorkflow', + typeVersion: 1.1, + position: [0, 0] as [number, number], + parameters: { source: 'url', workflowUrl }, + }); + + it('rejects a member adding such a node to a shared workflow', async () => { + const workflow = await createWorkflow({}, owner); + await shareWorkflowWithUsers(workflow, [member]); + + const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({ + versionId: workflow.versionId, + nodes: [remoteSubworkflowNode()], + }); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toBe(UNINSPECTABLE_SOURCE_MESSAGE); + }); + + it('rejects a member creating a workflow with such a node', async () => { + const response = await authMemberAgent.post('/workflows').send({ + name: 'test', + active: false, + connections: {}, + nodes: [remoteSubworkflowNode()], + }); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toBe(UNINSPECTABLE_SOURCE_MESSAGE); + }); + + it('rejects a member adding the HITL tool variant of such a node', async () => { + const workflow = await createWorkflow({}, owner); + await shareWorkflowWithUsers(workflow, [member]); + + const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({ + versionId: workflow.versionId, + nodes: [{ ...remoteSubworkflowNode(), type: 'n8n-nodes-base.executeWorkflowHitlTool' }], + }); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toBe(UNINSPECTABLE_SOURCE_MESSAGE); + }); + + it('rejects a member changing an execution setting of an existing node', async () => { + const workflow = await createWorkflow({ nodes: [remoteSubworkflowNode()] }, owner); + await shareWorkflowWithUsers(workflow, [member]); + + const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({ + versionId: workflow.versionId, + nodes: [{ ...remoteSubworkflowNode(), retryOnFail: true }], + }); + + expect(response.statusCode).toBe(400); + expect(response.body.message).toBe(UNINSPECTABLE_SOURCE_MESSAGE); + }); + + it('preserves a renamed URL node when the editor omits default and hidden parameters', async () => { + // The editor omits the default mode and the old database workflow ID. + const stored = { + ...remoteSubworkflowNode(), + parameters: { + ...remoteSubworkflowNode().parameters, + mode: 'once', + workflowId: { __rl: true, value: 'old-workflow', mode: 'id' }, + }, + }; + const workflow = await createWorkflow({ nodes: [stored] }, owner); + await shareWorkflowWithUsers(workflow, [member]); + + const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({ + versionId: workflow.versionId, + nodes: [{ ...remoteSubworkflowNode(), name: 'Renamed' }], + }); + + expect(response.statusCode).toBe(200); + const saved = await workflowRepository.findOneByOrFail({ id: workflow.id }); + expect(saved.nodes).toEqual([{ ...stored, name: 'Renamed' }]); + }); + + it('rejects a member changing a stored URL and leaves the node unchanged', async () => { + const workflow = await createWorkflow({ nodes: [remoteSubworkflowNode()] }, owner); + await shareWorkflowWithUsers(workflow, [member]); + + const changed = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({ + versionId: workflow.versionId, + nodes: [remoteSubworkflowNode('https://example.com/other.json')], + }); + + expect(changed.statusCode).toBe(400); + expect(changed.body.message).toBe(UNINSPECTABLE_SOURCE_MESSAGE); + + const saved = await workflowRepository.findOneByOrFail({ id: workflow.id }); + expect(saved.nodes).toEqual([remoteSubworkflowNode()]); + }); + + it('lets the owner create and change such a node', async () => { + const createResponse = await authOwnerAgent.post('/workflows').send({ + name: 'test', + active: false, + connections: {}, + nodes: [remoteSubworkflowNode()], + }); + expect(createResponse.statusCode).toBe(200); + const { id, versionId } = createResponse.body.data; + + const response = await authOwnerAgent.patch(`/workflows/${id}`).send({ + versionId, + nodes: [remoteSubworkflowNode('https://example.com/other.json')], + }); + + expect(response.statusCode).toBe(200); + expect(response.body.data.nodes[0].parameters.workflowUrl).toBe( + 'https://example.com/other.json', + ); + }); + }); + describe('restoring a history version', () => { const nodeWith = (credential: { id: string; name: string }, url = 'https://v1.test') => ({ id: 'uuid-restore-1', @@ -1468,6 +1598,60 @@ describe('PATCH /workflows/:workflowId', () => { }); }); + describe('sub-workflow with an inspectable source', () => { + const databaseSubworkflowNode = (workflowId: string) => ({ + id: 'uuid-sub-db', + name: 'Sub', + type: 'n8n-nodes-base.executeWorkflow', + typeVersion: 1.2, + position: [0, 0] as [number, number], + parameters: { + source: 'database', + workflowId: { __rl: true, value: workflowId, mode: 'id' }, + }, + }); + + it('lets a member change a stored node that loads its sub-workflow from the database', async () => { + const workflow = await createWorkflow( + { nodes: [databaseSubworkflowNode('wf-old')] }, + owner, + ); + await shareWorkflowWithUsers(workflow, [member]); + + const response = await authMemberAgent.patch(`/workflows/${workflow.id}`).send({ + versionId: workflow.versionId, + nodes: [databaseSubworkflowNode('wf-new')], + }); + + expect(response.statusCode).toBe(200); + expect(response.body.data.nodes[0].parameters.workflowId.value).toBe('wf-new'); + }); + + it('lets a member create a workflow with a node that defines its sub-workflow inline', async () => { + const response = await authMemberAgent.post('/workflows').send({ + name: 'test', + active: false, + connections: {}, + nodes: [ + { + id: 'uuid-sub-inline', + name: 'Sub', + type: 'n8n-nodes-base.executeWorkflow', + typeVersion: 1.2, + position: [0, 0], + parameters: { + source: 'parameter', + workflowJson: JSON.stringify({ nodes: [], connections: {} }), + }, + }, + ], + }); + + expect(response.statusCode).toBe(200); + expect(response.body.data.nodes[0].parameters.source).toBe('parameter'); + }); + }); + describe('credentials in an agent node parameter', () => { // The agent node keeps the model credential and its tool credentials in // the hidden `inlineAgent` parameter, not in `node.credentials`. diff --git a/packages/cli/test/integration/workflows/workflows.controller.test.ts b/packages/cli/test/integration/workflows/workflows.controller.test.ts index 27192c0e080..078559c1a20 100644 --- a/packages/cli/test/integration/workflows/workflows.controller.test.ts +++ b/packages/cli/test/integration/workflows/workflows.controller.test.ts @@ -23,6 +23,7 @@ import type { Role, WorkflowHistory, WorkflowEntity, + Project, } from '@n8n/db'; import { ProjectRepository, @@ -1968,185 +1969,26 @@ describe('GET /workflows', () => { expect(response.body.data).toHaveLength(1); // Should return all workflows when nodeTypes is empty }); - describe('includeCallableSubworkflows', () => { - const executeWorkflowTriggerNode = (id = uuid()) => ({ - id, - name: 'When Executed by Another Workflow', - type: 'n8n-nodes-base.executeWorkflowTrigger', - parameters: {}, - typeVersion: 1, - position: [0, 0] as [number, number], + test('should ignore callable sub-workflow filter keys', async () => { + const parentWorkflow = await createWorkflow({ name: 'Parent' }, member); + const anyPolicyWorkflow = await createWorkflow( + { name: 'Any Policy', settings: { callerPolicy: 'any' } }, + owner, + ); + + const filter = JSON.stringify({ + includeCallableSubworkflows: true, + parentWorkflowId: parentWorkflow.id, }); - test('should include workflows callable by the parent workflow based on callerPolicy', async () => { - const parentWorkflow = await createWorkflow({ name: 'Parent' }, member); + const response = await authMemberAgent + .get('/workflows') + .query(`filter=${filter}`) + .expect(200); - // callerPolicy: 'any' — callable by everyone - const anyPolicyWorkflow = await createWorkflow( - { - name: 'Any Policy', - nodes: [executeWorkflowTriggerNode()], - settings: { callerPolicy: 'any' }, - }, - owner, - ); - - // callerPolicy: 'workflowsFromAList' — parentWorkflow is explicitly listed - const fromListWorkflow = await createWorkflow( - { - name: 'From List Policy', - nodes: [executeWorkflowTriggerNode()], - settings: { - callerPolicy: 'workflowsFromAList', - callerIds: parentWorkflow.id, - }, - }, - owner, - ); - - // callerPolicy: 'none' — callable by nobody - await createWorkflow( - { - name: 'None Policy', - nodes: [executeWorkflowTriggerNode()], - settings: { callerPolicy: 'none' }, - }, - owner, - ); - - const filter = JSON.stringify({ - triggerNodeTypes: ['n8n-nodes-base.executeWorkflowTrigger'], - includeCallableSubworkflows: true, - parentWorkflowId: parentWorkflow.id, - }); - - const response = await authMemberAgent - .get('/workflows') - .query(`filter=${filter}`) - .expect(200); - - const returnedIds = response.body.data.map((w: { id: string }) => w.id) as string[]; - expect(returnedIds).toContain(anyPolicyWorkflow.id); - expect(returnedIds).toContain(fromListWorkflow.id); - expect(returnedIds).not.toContain( - ( - await createWorkflow( - { - name: 'None Policy Check', - nodes: [executeWorkflowTriggerNode()], - settings: { callerPolicy: 'none' }, - }, - owner, - ) - ).id, - ); - }); - - test('should not include callable workflows when includeCallableSubworkflows is false', async () => { - const parentWorkflow = await createWorkflow({ name: 'Parent' }, member); - - const anyPolicyWorkflow = await createWorkflow( - { - name: 'Any Policy', - nodes: [executeWorkflowTriggerNode()], - settings: { callerPolicy: 'any' }, - }, - owner, - ); - - const filter = JSON.stringify({ - triggerNodeTypes: ['n8n-nodes-base.executeWorkflowTrigger'], - includeCallableSubworkflows: false, - parentWorkflowId: parentWorkflow.id, - }); - - const response = await authMemberAgent - .get('/workflows') - .query(`filter=${filter}`) - .expect(200); - - const returnedIds = response.body.data.map((w: { id: string }) => w.id) as string[]; - // member doesn't own anyPolicyWorkflow, so without expansion it should not be visible - expect(returnedIds).not.toContain(anyPolicyWorkflow.id); - }); - - test('should not expand the list when the requester cannot read the parent workflow', async () => { - // Parent workflow owned by `owner`, inaccessible to `member`. - const parentWorkflow = await createWorkflow({ name: 'Owner Parent' }, owner); - - // Subworkflow owned by the same owner, callable by same-owner workflows. - const sameOwnerSubworkflow = await createWorkflow( - { - name: 'Same Owner Sub', - nodes: [executeWorkflowTriggerNode()], - settings: { callerPolicy: 'workflowsFromSameOwner' }, - }, - owner, - ); - - // Subworkflow that explicitly allowlists the inaccessible parent id. - const fromListSubworkflow = await createWorkflow( - { - name: 'From List Sub', - nodes: [executeWorkflowTriggerNode()], - settings: { - callerPolicy: 'workflowsFromAList', - callerIds: parentWorkflow.id, - }, - }, - owner, - ); - - const filter = JSON.stringify({ - triggerNodeTypes: ['n8n-nodes-base.executeWorkflowTrigger'], - includeCallableSubworkflows: true, - parentWorkflowId: parentWorkflow.id, - }); - - const response = await authMemberAgent - .get('/workflows') - .query(`filter=${filter}`) - .expect(200); - - const returnedIds = response.body.data.map((w: { id: string }) => w.id) as string[]; - // member cannot read the supplied parent workflow, so callable - // subworkflows must not be exposed regardless of caller policy. - expect(returnedIds).not.toContain(sameOwnerSubworkflow.id); - expect(returnedIds).not.toContain(fromListSubworkflow.id); - }); - - test('should match callerIds by whole id, not by substring', async () => { - const parentWorkflow = await createWorkflow({ name: 'Parent' }, member); - - // Allowlist contains the parent id only as a substring of a longer id. - const substringSubworkflow = await createWorkflow( - { - name: 'Substring Allowlist', - nodes: [executeWorkflowTriggerNode()], - settings: { - callerPolicy: 'workflowsFromAList', - callerIds: `${parentWorkflow.id}-extra-suffix`, - }, - }, - owner, - ); - - const filter = JSON.stringify({ - triggerNodeTypes: ['n8n-nodes-base.executeWorkflowTrigger'], - includeCallableSubworkflows: true, - parentWorkflowId: parentWorkflow.id, - }); - - const response = await authMemberAgent - .get('/workflows') - .query(`filter=${filter}`) - .expect(200); - - const returnedIds = response.body.data.map((w: { id: string }) => w.id) as string[]; - // parentWorkflow.id is only a substring of the allowlisted id, so it must - // not be treated as an allowed caller. - expect(returnedIds).not.toContain(substringSubworkflow.id); - }); + const returnedIds = response.body.data.map((w: { id: string }) => w.id) as string[]; + expect(returnedIds).toEqual([parentWorkflow.id]); + expect(returnedIds).not.toContain(anyPolicyWorkflow.id); }); }); @@ -5851,6 +5693,157 @@ describe('GET /workflows/:workflowId/executions/last-successful', () => { }); }); +describe('GET /workflows/:workflowId/callable-subworkflows', () => { + const executeWorkflowTriggerNode = () => ({ + id: uuid(), + name: 'When Executed by Another Workflow', + type: 'n8n-nodes-base.executeWorkflowTrigger', + parameters: {}, + typeVersion: 1, + position: [0, 0] as [number, number], + }); + + const createSubworkflow = async ( + name: string, + settings: WorkflowEntity['settings'], + userOrProject: User | Project, + ) => + await createWorkflow({ name, nodes: [executeWorkflowTriggerNode()], settings }, userOrProject); + + const getCallableIds = async (agent: SuperAgentTest, parentWorkflowId: string) => { + const response = await agent + .get(`/workflows/${parentWorkflowId}/callable-subworkflows`) + .expect(200); + return (response.body.data.data as Array<{ id: string }>).map((w) => w.id); + }; + + test('should list readable workflows and workflows that allow the parent explicitly', async () => { + const parentWorkflow = await createWorkflow({ name: 'Parent' }, member); + const ownSubworkflow = await createSubworkflow('Own', undefined, member); + const anyPolicyWorkflow = await createSubworkflow('Any', { callerPolicy: 'any' }, owner); + const fromListWorkflow = await createSubworkflow( + 'From List', + { callerPolicy: 'workflowsFromAList', callerIds: `other-id, ${parentWorkflow.id}` }, + owner, + ); + const nonePolicyWorkflow = await createSubworkflow('None', { callerPolicy: 'none' }, owner); + const substringListWorkflow = await createSubworkflow( + 'Substring List', + { callerPolicy: 'workflowsFromAList', callerIds: `${parentWorkflow.id}-extra-suffix` }, + owner, + ); + // Readable, but has no Execute Workflow Trigger node. + await createWorkflow({ name: 'No Trigger' }, member); + // Callable by the parent, but has no Execute Workflow Trigger node. + const callableNoTriggerWorkflow = await createWorkflow( + { name: 'Any Policy No Trigger', settings: { callerPolicy: 'any' } }, + owner, + ); + + const returnedIds = await getCallableIds(authMemberAgent, parentWorkflow.id); + + expect(returnedIds.sort()).toEqual( + [ownSubworkflow.id, anyPolicyWorkflow.id, fromListWorkflow.id].sort(), + ); + expect(returnedIds).not.toContain(nonePolicyWorkflow.id); + expect(returnedIds).not.toContain(substringListWorkflow.id); + expect(returnedIds).not.toContain(callableNoTriggerWorkflow.id); + }); + + test('should not list unshared workflows of the user who shared the parent', async () => { + const sharedParent = await createWorkflow({ name: 'Shared Parent' }, owner); + await shareWorkflowWithUsers(sharedParent, [member]); + const sameOwnerWorkflow = await createSubworkflow( + 'Same Owner', + { callerPolicy: 'workflowsFromSameOwner' }, + owner, + ); + const defaultPolicyWorkflow = await createSubworkflow('Default Policy', undefined, owner); + + const returnedIds = await getCallableIds(authMemberAgent, sharedParent.id); + + expect(returnedIds).not.toContain(sameOwnerWorkflow.id); + expect(returnedIds).not.toContain(defaultPolicyWorkflow.id); + }); + + test('should return only the minimal fields and the project name for team projects', async () => { + const parentWorkflow = await createWorkflow({ name: 'Parent' }, owner); + const teamProject = await createTeamProject('Team Project', owner); + const teamWorkflow = await createSubworkflow('Team Sub', undefined, teamProject); + const personalWorkflow = await createSubworkflow('Personal Sub', undefined, owner); + + const response = await authOwnerAgent + .get(`/workflows/${parentWorkflow.id}/callable-subworkflows`) + .query({ select: '["nodes"]', filter: '{"nodeTypes":["n8n-nodes-base.code"]}' }) + .expect(200); + + expect(response.body.data.count).toBe(2); + expect(response.body.data.data).toEqual( + arrayContaining([ + { + id: teamWorkflow.id, + name: 'Team Sub', + active: false, + isArchived: false, + projectName: 'Team Project', + }, + { id: personalWorkflow.id, name: 'Personal Sub', active: false, isArchived: false }, + ]), + ); + }); + + test('should filter by name and paginate', async () => { + const parentWorkflow = await createWorkflow({ name: 'Parent' }, member); + await createSubworkflow('Alpha One', undefined, member); + await createSubworkflow('Alpha Two', undefined, member); + await createSubworkflow('Beta', undefined, member); + + const response = await authMemberAgent + .get(`/workflows/${parentWorkflow.id}/callable-subworkflows`) + .query({ query: 'alpha', take: 1 }) + .expect(200); + + expect(response.body.data.count).toBe(2); + expect(response.body.data.data).toHaveLength(1); + expect(response.body.data.data[0].name).toMatch(/^Alpha/); + }); + + test('should match the search query against workflow names only', async () => { + const parentWorkflow = await createWorkflow({ name: 'Parent' }, member); + await createWorkflow( + { + name: 'Callable', + description: 'contains uniqueterm', + nodes: [executeWorkflowTriggerNode()], + settings: { callerPolicy: 'any' }, + }, + owner, + ); + + const response = await authMemberAgent + .get(`/workflows/${parentWorkflow.id}/callable-subworkflows`) + .query({ query: 'uniqueterm' }) + .expect(200); + + expect(response.body.data).toEqual({ count: 0, data: [] }); + }); + + test('should forbid users who cannot update the parent workflow', async () => { + const teamProject = await createTeamProject('Team Project', owner); + await linkUserToProject(member, teamProject, 'project:viewer'); + const parentWorkflow = await createWorkflow({ name: 'Parent' }, teamProject); + + await authMemberAgent.get(`/workflows/${parentWorkflow.id}/callable-subworkflows`).expect(403); + }); + + test('should forbid users who cannot access the parent workflow', async () => { + const parentWorkflow = await createWorkflow({ name: 'Parent' }, owner); + await createSubworkflow('Allows Parent', { callerPolicy: 'any' }, owner); + + await authMemberAgent.get(`/workflows/${parentWorkflow.id}/callable-subworkflows`).expect(403); + }); +}); + describe('POST /workflows/with-node-types', () => { const NODE_TYPE = 'n8n-nodes-base.set'; diff --git a/packages/core/src/execution-engine/__tests__/routing-node.test.ts b/packages/core/src/execution-engine/__tests__/routing-node.test.ts index 222905ca2e0..7dc77695fc4 100644 --- a/packages/core/src/execution-engine/__tests__/routing-node.test.ts +++ b/packages/core/src/execution-engine/__tests__/routing-node.test.ts @@ -1,5 +1,11 @@ +import { AxiosError, AxiosHeaders } from 'axios'; import get from 'lodash/get'; -import { Workflow, createEmptyRunExecutionData } from 'n8n-workflow'; +import { + NodeApiError, + NodeOperationError, + Workflow, + createEmptyRunExecutionData, +} from 'n8n-workflow'; import type { DeclarativeRestApiSettings, ICredentialDataDecryptedObject, @@ -2428,9 +2434,11 @@ describe('RoutingNode', () => { const createRoutingNode = ({ continueOnFail, requestErrorUrl, + requestError = new Error('Request failed'), }: { continueOnFail: boolean; requestErrorUrl?: string; + requestError?: Error; }) => { const items: INodeExecutionData[] = [ { json: { id: 'first' } }, @@ -2483,7 +2491,7 @@ describe('RoutingNode', () => { context.helpers.httpRequest = vi.fn(async (requestOptions: IHttpRequestOptions) => { requestUrls.push(requestOptions.url); if (requestOptions.url === requestErrorUrl) { - throw new Error('Request failed'); + throw requestError; } return { body: { url: requestOptions.url } }; }); @@ -2541,6 +2549,41 @@ describe('RoutingNode', () => { expect(result?.[0]?.[1]?.error).toMatchObject({ message: 'Request failed' }); expect(result?.[0]?.[2]?.json).toEqual({ url: '/items/third' }); }); + + test('returns a request error without request headers when continue on fail is enabled', async () => { + const requestError = new AxiosError( + 'Request failed with status code 500', + 'ERR_BAD_RESPONSE', + { headers: new AxiosHeaders({ Authorization: 'Bearer secret-token' }) }, + ); + const { run } = createRoutingNode({ + continueOnFail: true, + requestErrorUrl: '/items/second', + requestError, + }); + + const result = await run; + const error = result?.[0]?.[1]?.error; + + expect(error).toBeInstanceOf(NodeApiError); + expect(error).toMatchObject({ context: { itemIndex: 1, runIndex: 0 } }); + expect(JSON.stringify(error)).not.toContain('secret-token'); + }); + + test('keeps a node operation error when continue on fail is enabled', async () => { + const requestError = new NodeOperationError(mock(), 'Post-receive action failed'); + const { run } = createRoutingNode({ + continueOnFail: true, + requestErrorUrl: '/items/second', + requestError, + }); + + const result = await run; + const error = result?.[0]?.[1]?.error; + + expect(error).toBe(requestError); + expect(error).toMatchObject({ context: { itemIndex: 1, runIndex: 0 } }); + }); }); describe('itemIndex', () => { diff --git a/packages/core/src/execution-engine/routing-node.ts b/packages/core/src/execution-engine/routing-node.ts index f2250c2b9e5..e87c467669c 100644 --- a/packages/core/src/execution-engine/routing-node.ts +++ b/packages/core/src/execution-engine/routing-node.ts @@ -311,24 +311,27 @@ export class RoutingNode { const error = responseData.reason; + const nodeError = + error instanceof NodeApiError || error instanceof NodeOperationError + ? error + : new NodeApiError(node, error as JsonObject, { + runIndex, + itemIndex, + message: error?.message, + description: error?.description, + httpCode: + error.isAxiosError && error.response ? String(error.response?.status) : 'none', + }); + + set(nodeError, 'context.itemIndex', itemIndex); + set(nodeError, 'context.runIndex', runIndex); + if (itemContext[itemIndex].thisArgs?.continueOnFail()) { - returnData.push({ json: {}, error: error as NodeApiError }); + returnData.push({ json: {}, error: nodeError }); continue; } - if (error instanceof NodeApiError) { - set(error, 'context.itemIndex', itemIndex); - set(error, 'context.runIndex', runIndex); - throw error; - } - - throw new NodeApiError(node, error as JsonObject, { - runIndex, - itemIndex, - message: error?.message, - description: error?.description, - httpCode: error.isAxiosError && error.response ? String(error.response?.status) : 'none', - }); + throw nodeError; } if (itemContext[itemIndex].requestData.maxResults) { diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index c93cabbccc4..5ed9fc065ab 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -681,6 +681,7 @@ "chatHub.toolsManager.add": "Add", "chatHub.toolsManager.addTool": "Add tool", "chatHub.toolsManager.remove": "Remove", + "chatHub.toolsManager.roleBlocked": "Your role cannot use this tool. Remove it, or ask an admin for a different role.", "chatHub.toolsManager.configure": "Configure", "chatHub.toolsManager.manageTools": "Manage tools", "chatHub.toolsManager.enableTool": "Enable tool", diff --git a/packages/frontend/editor-ui/src/app/api/workflows.test.ts b/packages/frontend/editor-ui/src/app/api/workflows.test.ts index ab97bbf61b8..b56a3450f9e 100644 --- a/packages/frontend/editor-ui/src/app/api/workflows.test.ts +++ b/packages/frontend/editor-ui/src/app/api/workflows.test.ts @@ -1,4 +1,9 @@ -import { getLastSuccessfulExecution, getNewWorkflowData, getWorkflows } from './workflows'; +import { + getCallableSubworkflows, + getLastSuccessfulExecution, + getNewWorkflowData, + getWorkflows, +} from './workflows'; import { DEFAULT_NEW_WORKFLOW_NAME, DEFAULT_SETTINGS } from '@/app/constants/workflows'; import * as apiUtils from '@n8n/rest-api-client'; import type { IRestApiContext } from '@n8n/rest-api-client'; @@ -46,6 +51,38 @@ describe('API: workflows', () => { }); }); + describe('getCallableSubworkflows', () => { + const mockContext = { + baseUrl: 'http://test-base-url', + sessionId: 'test-session', + pushRef: 'test-ref', + } as IRestApiContext; + + it('should call the parent-specific endpoint with the search and page params', async () => { + const response = { + count: 1, + data: [{ id: 'sub-1', name: 'Sub', active: false, isArchived: false }], + }; + const makeRestApiRequestSpy = vi + .spyOn(apiUtils, 'makeRestApiRequest') + .mockResolvedValue(response); + + const result = await getCallableSubworkflows(mockContext, 'parent-id', { + query: 'sub', + skip: 40, + take: 20, + }); + + expect(makeRestApiRequestSpy).toHaveBeenCalledWith( + mockContext, + 'GET', + '/workflows/parent-id/callable-subworkflows', + { query: 'sub', skip: 40, take: 20 }, + ); + expect(result).toEqual(response); + }); + }); + describe('getLastSuccessfulExecution', () => { let mockContext: IRestApiContext; let makeRestApiRequestSpy: MockInstance; diff --git a/packages/frontend/editor-ui/src/app/api/workflows.ts b/packages/frontend/editor-ui/src/app/api/workflows.ts index 6cb992573dc..d17669edda8 100644 --- a/packages/frontend/editor-ui/src/app/api/workflows.ts +++ b/packages/frontend/editor-ui/src/app/api/workflows.ts @@ -12,7 +12,7 @@ import type { } from '@/features/execution/executions/executions.types'; import { DEFAULT_NEW_WORKFLOW_NAME, DEFAULT_SETTINGS } from '@/app/constants'; import { isEmpty } from '@/app/utils/typesUtils'; -import type { ExecutionRedactionQueryDto } from '@n8n/api-types'; +import type { CallableSubworkflowsResponse, ExecutionRedactionQueryDto } from '@n8n/api-types'; import type { IRestApiContext } from '@n8n/rest-api-client'; import type { ExecutionFilters, @@ -120,6 +120,19 @@ export async function getWorkflowsAndFolders( }); } +export async function getCallableSubworkflows( + context: IRestApiContext, + parentWorkflowId: string, + params: { query?: string; skip: number; take: number }, +) { + return await makeRestApiRequest( + context, + 'GET', + `/workflows/${parentWorkflowId}/callable-subworkflows`, + params, + ); +} + export async function getActiveWorkflows(context: IRestApiContext) { return await makeRestApiRequest(context, 'GET', '/active-workflows'); } diff --git a/packages/frontend/editor-ui/src/app/stores/workflowsList.store.test.ts b/packages/frontend/editor-ui/src/app/stores/workflowsList.store.test.ts index 1d4f707daf4..ecd51abce9c 100644 --- a/packages/frontend/editor-ui/src/app/stores/workflowsList.store.test.ts +++ b/packages/frontend/editor-ui/src/app/stores/workflowsList.store.test.ts @@ -4,12 +4,14 @@ import { useWorkflowsListStore } from '@/app/stores/workflowsList.store'; import type { IWorkflowDb, WorkflowListResource, WorkflowResource } from '@/Interface'; import * as apiUtils from '@n8n/rest-api-client'; import { createTestWorkflow } from '@/__tests__/mocks'; +import { DEFAULT_WORKFLOW_PAGE_SIZE } from '@/app/constants'; import type { WorkflowHistory } from '@n8n/rest-api-client'; vi.mock('@/app/api/workflows', () => ({ getWorkflows: vi.fn(), getWorkflow: vi.fn(), getWorkflowsAndFolders: vi.fn(), + getCallableSubworkflows: vi.fn(), getWorkflowsWithNodesIncluded: vi.fn(), getActiveWorkflows: vi.fn(), workflowExists: vi.fn(), @@ -328,6 +330,44 @@ describe('useWorkflowsListStore', () => { }); }); + describe('fetchCallableSubworkflowsPage', () => { + it('should request the page and keep the rows out of the workflows cache', async () => { + const response = { + count: 1, + data: [{ id: 'sub-1', name: 'Sub', active: false, isArchived: false }], + }; + vi.mocked(workflowsApi).getCallableSubworkflows.mockResolvedValue(response); + + const result = await workflowsListStore.fetchCallableSubworkflowsPage( + 'parent-id', + 2, + 10, + 'sub', + ); + + expect(workflowsApi.getCallableSubworkflows).toHaveBeenCalledWith( + expect.anything(), + 'parent-id', + { query: 'sub', skip: 10, take: 10 }, + ); + expect(result).toEqual(response); + expect(workflowsListStore.workflowsById).toEqual({}); + expect(workflowsListStore.totalWorkflowCount).toBe(0); + }); + + it('should request the first page with the default page size and no query', async () => { + vi.mocked(workflowsApi).getCallableSubworkflows.mockResolvedValue({ count: 0, data: [] }); + + await workflowsListStore.fetchCallableSubworkflowsPage('parent-id'); + + expect(workflowsApi.getCallableSubworkflows).toHaveBeenCalledWith( + expect.anything(), + 'parent-id', + { query: undefined, skip: 0, take: DEFAULT_WORKFLOW_PAGE_SIZE }, + ); + }); + }); + describe('searchWorkflows', () => { it('should search workflows with filters', async () => { const mockWorkflows = [{ id: '1', name: 'Test Workflow' }] as IWorkflowDb[]; diff --git a/packages/frontend/editor-ui/src/app/stores/workflowsList.store.ts b/packages/frontend/editor-ui/src/app/stores/workflowsList.store.ts index cbfcfdf383e..98744309854 100644 --- a/packages/frontend/editor-ui/src/app/stores/workflowsList.store.ts +++ b/packages/frontend/editor-ui/src/app/stores/workflowsList.store.ts @@ -18,8 +18,6 @@ export type WorkflowListFilters = { parentFolderId?: string; availableInMCP?: boolean; triggerNodeTypes?: string[]; - includeCallableSubworkflows?: boolean; - parentWorkflowId?: string; }; const ALL_PROJECTS_KEY = '__all_projects__'; @@ -184,6 +182,21 @@ export const useWorkflowsListStore = defineStore(STORES.WORKFLOWS_LIST, () => { return data; } + // Callable rows can belong to workflows the user cannot read, so they are not + // added to the workflows cache. + async function fetchCallableSubworkflowsPage( + parentWorkflowId: string, + page = 1, + pageSize = DEFAULT_WORKFLOW_PAGE_SIZE, + query?: string, + ) { + return await workflowsApi.getCallableSubworkflows(rootStore.restApiContext, parentWorkflowId, { + query, + skip: (page - 1) * pageSize, + take: pageSize, + }); + } + async function searchWorkflows({ projectId, ids, @@ -329,6 +342,7 @@ export const useWorkflowsListStore = defineStore(STORES.WORKFLOWS_LIST, () => { // Fetching fetchWorkflowsPageWithCount, fetchWorkflowsPage, + fetchCallableSubworkflowsPage, searchWorkflows, fetchAllWorkflows, fetchWorkflow, diff --git a/packages/frontend/editor-ui/src/features/ai/chatHub/components/ToolListItem.vue b/packages/frontend/editor-ui/src/features/ai/chatHub/components/ToolListItem.vue index 4a66a84ed05..0d5fdfaab52 100644 --- a/packages/frontend/editor-ui/src/features/ai/chatHub/components/ToolListItem.vue +++ b/packages/frontend/editor-ui/src/features/ai/chatHub/components/ToolListItem.vue @@ -18,6 +18,8 @@ const props = defineProps<{ installing?: boolean; /** Non-admin cannot install; button is disabled with contact-admin tooltip. */ installDisabled?: boolean; + /** The user's role cannot use this tool type, so only removal is possible. */ + roleBlocked?: boolean; }>(); const emit = defineEmits<{ @@ -62,7 +64,10 @@ const restriction = computed(() => getNodeItemRestriction(props.nodeType.name)); ref="rowRef" :class="[ $style.item, - { [$style.configured]: mode === 'configured', [$style.restricted]: !!restriction }, + { + [$style.configured]: mode === 'configured', + [$style.restricted]: !!restriction || roleBlocked, + }, ]" >
@@ -101,8 +106,23 @@ const restriction = computed(() => getNodeItemRestriction(props.nodeType.name)); :anchor="rowRef" /> + + + + + +