From ab0b5351ec6b331c8a7255b6927ecce86df62c11 Mon Sep 17 00:00:00 2001 From: "n8n-assistant[bot]" <100856346+n8n-assistant[bot]@users.noreply.github.com> Date: Wed, 30 Sep 2026 07:44:17 +0300 Subject: [PATCH] chore: Bundle/1.x (#39936) Co-authored-by: Matsu --- package.json | 3 +- .../src/test/workflow-builder-agent.test.ts | 72 +++++- .../src/tools/utils/connection.utils.ts | 13 +- .../tools/utils/test/connection.utils.test.ts | 70 ++++++ .../src/utils/operations-processor.ts | 49 +++- .../utils/test/operations-processor.test.ts | 193 +++++++++++++++ .../utils/test/workflow-sanitization.test.ts | 215 +++++++++++++++++ .../src/utils/workflow-sanitization.ts | 72 ++++++ .../src/workflow-builder-agent.ts | 14 +- packages/@n8n/expression-runtime/package.json | 2 +- .../ChatTrigger/__test__/custom-css.test.ts | 188 +++++++++++++++ .../ChatTrigger/__test__/templates.test.ts | 108 --------- .../nodes/trigger/ChatTrigger/templates.ts | 34 ++- .../task-runner/src/health-check-server.ts | 20 ++ .../__tests__/prototype-hardening.test.ts | 224 ++++++++++++++++++ .../src/js-task-runner/prototype-hardening.ts | 102 ++++++-- .../src/__tests__/credentials-helper.test.ts | 186 +++++++++++++++ .../workflow-execute-additional-data.test.ts | 91 ++++++- .../src/auth/__tests__/auth.service.test.ts | 204 ++++++++++++++++ packages/cli/src/auth/auth.service.ts | 51 +++- packages/cli/src/credentials-helper.ts | 52 +++- ...ce-control-git.service.integration.test.ts | 146 ++++++++++++ .../source-control-git.service.test.ts | 216 ++++++++++++++++- .../source-control-import.service.ee.test.ts | 30 +++ .../source-control/constants.ts | 16 ++ .../source-control-git.service.ee.ts | 172 ++++++++++++-- .../source-control-import.service.ee.ts | 6 + packages/cli/src/server.ts | 28 ++- .../__tests__/waiting-webhooks.test.ts | 109 ++++++++- .../__tests__/webhook.service.test.ts | 50 ++++ packages/cli/src/webhooks/waiting-webhooks.ts | 14 ++ packages/cli/src/webhooks/webhook.service.ts | 13 +- .../src/workflow-execute-additional-data.ts | 6 +- .../cli/src/workflows/workflow.service.ee.ts | 53 +++-- .../integration/server-static-assets.test.ts | 168 ++++++++++++- .../cli/test/integration/shared/workflow.ts | 137 ++++++++++- .../workflows/workflow.service.ee.test.ts | 56 +++++ .../components/BinaryDataDisplayEmbed.test.ts | 123 ++++++++++ .../components/BinaryDataDisplayEmbed.vue | 46 ++-- .../nodes/Slack/SlackTriggerHelpers.ts | 22 +- .../Slack/test/SlackTriggerHelper.test.ts | 43 +++- packages/workflow/package.json | 2 +- packages/workflow/src/constants.ts | 1 + packages/workflow/src/index.ts | 1 + packages/workflow/src/node-helpers.ts | 12 +- packages/workflow/src/utils.ts | 18 ++ patches/@n8n__tournament@1.9.1.patch | 164 ------------- pnpm-lock.yaml | 33 ++- 48 files changed, 3221 insertions(+), 427 deletions(-) create mode 100644 packages/@n8n/ai-workflow-builder.ee/src/utils/test/workflow-sanitization.test.ts create mode 100644 packages/@n8n/ai-workflow-builder.ee/src/utils/workflow-sanitization.ts create mode 100644 packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/custom-css.test.ts create mode 100644 packages/@n8n/task-runner/src/js-task-runner/__tests__/prototype-hardening.test.ts create mode 100644 packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.integration.test.ts create mode 100644 packages/frontend/editor-ui/src/features/ndv/runData/components/BinaryDataDisplayEmbed.test.ts delete mode 100644 patches/@n8n__tournament@1.9.1.patch diff --git a/package.json b/package.json index c8c4379e460..5a2024c2831 100644 --- a/package.json +++ b/package.json @@ -195,8 +195,7 @@ "z-vue-scan": "patches/z-vue-scan.patch", "@lezer/highlight": "patches/@lezer__highlight.patch", "v-code-diff": "patches/v-code-diff.patch", - "lodash@4.18.0": "patches/lodash@4.18.0.patch", - "@n8n/tournament@1.9.1": "patches/@n8n__tournament@1.9.1.patch" + "lodash@4.18.0": "patches/lodash@4.18.0.patch" } } } diff --git a/packages/@n8n/ai-workflow-builder.ee/src/test/workflow-builder-agent.test.ts b/packages/@n8n/ai-workflow-builder.ee/src/test/workflow-builder-agent.test.ts index 80c9384cb5f..ce2e5e0f8c9 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/test/workflow-builder-agent.test.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/test/workflow-builder-agent.test.ts @@ -5,8 +5,8 @@ import type { MemorySaver } from '@langchain/langgraph'; import { GraphRecursionError } from '@langchain/langgraph'; import type { Logger } from '@n8n/backend-common'; import { mock } from 'jest-mock-extended'; -import type { INodeTypeDescription } from 'n8n-workflow'; -import { ApplicationError } from 'n8n-workflow'; +import type { INodeTypeDescription, IWorkflowBase } from 'n8n-workflow'; +import { ApplicationError, jsonParse } from 'n8n-workflow'; jest.mock('@/tools/add-node.tool', () => ({ createAddNodeTool: jest.fn().mockReturnValue({ tool: { name: 'add_node' } }), @@ -69,6 +69,7 @@ import { MAX_AI_BUILDER_PROMPT_LENGTH } from '@/constants'; import { ValidationError } from '@/errors'; import { createMainAgentPrompt } from '@/tools/prompts/main-agent.prompt'; import type { StreamOutput } from '@/types/streaming'; +import type { SimpleWorkflow } from '@/types/workflow'; import { createStreamProcessor } from '@/utils/stream-processor'; import { WorkflowBuilderAgent, @@ -141,6 +142,73 @@ describe('WorkflowBuilderAgent', () => { agent = new WorkflowBuilderAgent(config); }); + describe('workflow entry point', () => { + // getDefaultWorkflowJSON is the only producer of the workflowJSON the graph is started + // with, so it is the choke point where a stored workflow is made safe to key by node + // name. Exercised directly: the call sites below it sit behind the LangGraph stream. + function getDefaultWorkflowJSON(payload: ChatPayload): SimpleWorkflow { + return ( + agent as unknown as { + getDefaultWorkflowJSON: (p: ChatPayload) => SimpleWorkflow; + } + ).getDefaultWorkflowJSON(payload); + } + + function payloadWith(currentWorkflow: unknown): ChatPayload { + return { + message: 'hi', + workflowContext: { currentWorkflow: currentWorkflow as IWorkflowBase }, + }; + } + + it('should pass an ordinary workflow through unchanged', () => { + const currentWorkflow = { + name: 'w', + nodes: [{ id: '1', name: 'Manual' }], + connections: { Manual: { main: [[{ node: 'Sink', type: 'main', index: 0 }]] } }, + }; + + const result = getDefaultWorkflowJSON(payloadWith(currentWorkflow)); + + expect(result).toBe(currentWorkflow); + }); + + it('should return an empty workflow when no workflow is supplied', () => { + expect(getDefaultWorkflowJSON({ message: 'hi' })).toEqual({ + nodes: [], + connections: {}, + name: '', + }); + }); + + it('should drop a node whose name cannot be an object key before the graph starts', () => { + const result = getDefaultWorkflowJSON( + payloadWith({ + name: 'w', + nodes: [ + { id: '1', name: '__proto__' }, + { id: '2', name: 'Keep' }, + ], + connections: {}, + }), + ); + + expect(result.nodes.map((n) => n.name)).toEqual(['Keep']); + }); + + it('should drop a connection field that cannot be an object key before the graph starts', () => { + const result = getDefaultWorkflowJSON( + payloadWith( + jsonParse( + '{"name":"w","nodes":[],"connections":{"A":{"main":[[{"node":["__proto__"],"type":"main","index":0}]]}}}', + ), + ), + ); + + expect(result.connections.A?.main?.[0]).toEqual([]); + }); + }); + describe('chat method', () => { let mockPayload: ChatPayload; diff --git a/packages/@n8n/ai-workflow-builder.ee/src/tools/utils/connection.utils.ts b/packages/@n8n/ai-workflow-builder.ee/src/tools/utils/connection.utils.ts index 9d379af3213..0ce9e0fd38d 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/tools/utils/connection.utils.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/tools/utils/connection.utils.ts @@ -5,7 +5,7 @@ import type { IConnection, NodeConnectionType, } from 'n8n-workflow'; -import { NodeConnectionTypes } from 'n8n-workflow'; +import { isUsableObjectKey, NodeConnectionTypes } from 'n8n-workflow'; import type { ConnectionValidationResult, @@ -186,6 +186,13 @@ export function createConnection( sourceOutputIndex: number = 0, targetInputIndex: number = 0, ): IConnections { + // Connection maps are keyed by node name and connection type. A key that names an + // inherited object property resolves to a slot that is not the map's own, so refuse + // it instead of writing through it. + if (!isUsableObjectKey(sourceNodeName) || !isUsableObjectKey(connectionType)) { + return connections; + } + // Ensure source node exists in connections if (!connections[sourceNodeName]) { connections[sourceNodeName] = {}; @@ -243,6 +250,10 @@ export function removeConnection( sourceOutputIndex?: number, targetInputIndex?: number, ): IConnections { + if (!isUsableObjectKey(sourceNodeName) || !isUsableObjectKey(connectionType)) { + return connections; + } + if (!connections[sourceNodeName]?.[connectionType]) { return connections; } diff --git a/packages/@n8n/ai-workflow-builder.ee/src/tools/utils/test/connection.utils.test.ts b/packages/@n8n/ai-workflow-builder.ee/src/tools/utils/test/connection.utils.test.ts index b09e45351f5..a5e6dd7b85f 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/tools/utils/test/connection.utils.test.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/tools/utils/test/connection.utils.test.ts @@ -987,4 +987,74 @@ describe('connection.utils', () => { expect(nodeHasOutputType(returnExprNode, 'ai_tool')).toBe(true); }); }); + + describe('reserved connection keys', () => { + const plantedKeys = ['main', 'ai_tool']; + + afterEach(() => { + for (const key of plantedKeys) { + delete (Object.prototype as Record)[key]; + } + }); + + it('createConnection should refuse a source name that is not a usable object key', () => { + const connections: IConnections = {}; + + const result = createConnection(connections, '__proto__', 'Sink', NodeConnectionTypes.Main); + + expect(({} as Record).main).toBeUndefined(); + expect(Object.keys(result)).toEqual([]); + }); + + it('createConnection should refuse a connection type that is not a usable object key', () => { + const connections: IConnections = {}; + + const result = createConnection( + connections, + 'Source', + 'Sink', + 'constructor' as NodeConnectionType, + ); + + expect(Object.keys(result)).toEqual([]); + }); + + it('createConnection should still create a connection for usable keys', () => { + const connections: IConnections = {}; + + const result = createConnection(connections, 'Source', 'Sink', NodeConnectionTypes.Main); + + expect(result.Source?.main?.[0]).toEqual([{ node: 'Sink', type: 'main', index: 0 }]); + }); + + it('removeConnection should not write through a reserved source name to a shared slot', () => { + // Start from a shared slot that already holds a connection-shaped value, so the + // unguarded lookup finds something to read, rewrite and store back. One entry is + // kept by the filter, which keeps the unguarded path away from its delete branch. + const seeded = [[{ node: 'Other', type: 'ai_tool', index: 0 }]]; + (Object.prototype as Record).ai_tool = seeded; + + const connections: IConnections = {}; + + const result = removeConnection(connections, '__proto__', 'Sink', 'ai_tool'); + + // Identity, not deep equality: the unguarded path stores back a filtered copy + // whose contents happen to match. + expect(({} as Record).ai_tool).toBe(seeded); + expect(seeded).toEqual([[{ node: 'Other', type: 'ai_tool', index: 0 }]]); + expect(Object.keys(result)).toEqual([]); + }); + + it('removeConnection should refuse a connection type that resolves through the prototype', () => { + const connections: IConnections = { + Source: { main: [[{ node: 'Sink', type: 'main', index: 0 }]] }, + }; + + // 'constructor' reads back a function rather than undefined, so the unguarded + // path gets past its early return and then calls an array method on it. + expect(() => removeConnection(connections, 'Source', 'Sink', 'constructor')).not.toThrow(); + + expect(Object.keys(connections.Source)).toEqual(['main']); + }); + }); }); diff --git a/packages/@n8n/ai-workflow-builder.ee/src/utils/operations-processor.ts b/packages/@n8n/ai-workflow-builder.ee/src/utils/operations-processor.ts index d69f0f8eb75..f66f511e9ce 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/utils/operations-processor.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/utils/operations-processor.ts @@ -1,4 +1,5 @@ import type { INode, IConnections } from 'n8n-workflow'; +import { isUsableObjectKey } from 'n8n-workflow'; import type { SimpleWorkflow, WorkflowOperation } from '../types/workflow'; @@ -36,10 +37,14 @@ function applyRemoveNodeOperation( // Copy connections, excluding those from/to removed nodes for (const [sourceId, nodeConnections] of Object.entries(workflow.connections)) { + if (!isUsableObjectKey(sourceId)) continue; + if (!nodesToRemove.has(sourceId)) { cleanedConnections[sourceId] = {}; for (const [connectionType, outputs] of Object.entries(nodeConnections)) { + if (!isUsableObjectKey(connectionType)) continue; + if (Array.isArray(outputs)) { cleanedConnections[sourceId][connectionType] = outputs.map((outputConnections) => { if (Array.isArray(outputConnections)) { @@ -105,6 +110,36 @@ function applyUpdateNodeOperation( }; } +/** + * Keep only the connection types that can be own keys of a plain object. + */ +function pickSafeConnectionTypes(nodeConnections: IConnections[string]): IConnections[string] { + return Object.fromEntries( + Object.entries(nodeConnections).filter(([connectionType]) => isUsableObjectKey(connectionType)), + ); +} + +/** + * Keep only the connection entries whose node-name and connection-type keys can be + * own keys of a plain object. Used where a whole connection map arrives from outside + * and is stored as-is. + */ +function pickSafeConnections(connections: IConnections): IConnections { + const safe: IConnections = {}; + + for (const [sourceName, nodeConnections] of Object.entries(connections)) { + if (!isUsableObjectKey(sourceName)) continue; + + const safeTypes = pickSafeConnectionTypes(nodeConnections); + + if (Object.keys(safeTypes).length > 0) { + safe[sourceName] = safeTypes; + } + } + + return safe; +} + /** * Handle 'setConnections' operation - replace all connections */ @@ -116,7 +151,7 @@ function applySetConnectionsOperation( return { ...workflow, - connections: operation.connections, + connections: pickSafeConnections(operation.connections), }; } @@ -133,11 +168,17 @@ function applyMergeConnectionsOperation( // Merge connections additively for (const [sourceId, nodeConnections] of Object.entries(operation.connections)) { + // A node name that is not usable as an own key of the map cannot take part in a + // connection, so drop the entry rather than resolve it against the prototype chain. + if (!isUsableObjectKey(sourceId)) continue; + if (!connections[sourceId]) { - connections[sourceId] = nodeConnections; + connections[sourceId] = pickSafeConnectionTypes(nodeConnections); } else { // Merge connections for this source node for (const [connectionType, newOutputs] of Object.entries(nodeConnections)) { + if (!isUsableObjectKey(connectionType)) continue; + if (!connections[sourceId][connectionType]) { connections[sourceId][connectionType] = newOutputs; } else { @@ -194,6 +235,10 @@ function applyRemoveConnectionOperation( const { sourceNode, targetNode, connectionType, sourceOutputIndex, targetInputIndex } = operation; + if (!isUsableObjectKey(sourceNode) || !isUsableObjectKey(connectionType)) { + return workflow; + } + const connections = { ...workflow.connections }; // Check if source node has connections diff --git a/packages/@n8n/ai-workflow-builder.ee/src/utils/test/operations-processor.test.ts b/packages/@n8n/ai-workflow-builder.ee/src/utils/test/operations-processor.test.ts index 73424e7cc8b..3476dac09dc 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/utils/test/operations-processor.test.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/utils/test/operations-processor.test.ts @@ -1,4 +1,5 @@ import type { INode, IConnections } from 'n8n-workflow'; +import { jsonParse } from 'n8n-workflow'; import { createNode, createWorkflow } from '../../../test/test-utils'; import type { SimpleWorkflow, WorkflowOperation } from '../../types/workflow'; @@ -656,4 +657,196 @@ describe('operations-processor', () => { expect(result.workflowOperations).toBeNull(); }); }); + + describe('reserved connection keys', () => { + const plantedKeys = ['main', 'shell', 'ai_tool']; + + afterEach(() => { + for (const key of plantedKeys) { + delete (Object.prototype as Record)[key]; + } + }); + + it('should build a fixture whose reserved key is an own property', () => { + // Guards the tests below: a plain object literal would set the prototype instead + // of adding the key, which would make the cases vacuous. + const connections = { ['__proto__']: { main: [[]] } } as unknown as IConnections; + + expect(Object.prototype.hasOwnProperty.call(connections, '__proto__')).toBe(true); + }); + + describe('mergeConnections', () => { + it('should ignore a source key that is not a usable object key', () => { + const workflow = createWorkflow([createNode({ id: 'node1', name: 'Sink' })]); + const operations: WorkflowOperation[] = [ + { + type: 'mergeConnections', + connections: { + ['__proto__']: { main: [[{ node: 'Sink', type: 'main', index: 0 }]] }, + } as unknown as IConnections, + }, + ]; + + const result = applyOperations(workflow, operations); + + expect(({} as Record).main).toBeUndefined(); + expect(Object.keys(result.connections)).toEqual([]); + }); + + it('should ignore a connection type that is not a usable object key', () => { + const workflow: SimpleWorkflow = { + name: 'Test Workflow', + nodes: [createNode({ id: 'node1', name: 'Source' })], + connections: { Source: { main: [[]] } }, + }; + const operations: WorkflowOperation[] = [ + { + type: 'mergeConnections', + connections: { + Source: { ['__proto__']: [[{ node: 'Sink', type: 'main', index: 0 }]] }, + } as unknown as IConnections, + }, + ]; + + const result = applyOperations(workflow, operations); + + // Object.keys alone cannot see this: the unguarded write lands on the entry's + // prototype, so assert the prototype itself is intact. + expect(Object.getPrototypeOf(result.connections.Source)).toBe(Object.prototype); + expect(Object.keys(result.connections.Source)).toEqual(['main']); + }); + + it('should still merge connections for usable keys', () => { + const workflow: SimpleWorkflow = { + name: 'Test Workflow', + nodes: [createNode({ id: 'node1', name: 'Source' })], + connections: { Source: { main: [[{ node: 'A', type: 'main', index: 0 }]] } }, + }; + const operations: WorkflowOperation[] = [ + { + type: 'mergeConnections', + connections: { + ['__proto__']: { main: [[{ node: 'Ignored', type: 'main', index: 0 }]] }, + Source: { main: [[{ node: 'B', type: 'main', index: 0 }]] }, + } as unknown as IConnections, + }, + ]; + + const result = applyOperations(workflow, operations); + + expect(result.connections.Source?.main?.[0]).toEqual([ + { node: 'A', type: 'main', index: 0 }, + { node: 'B', type: 'main', index: 0 }, + ]); + expect(result.connections).not.toHaveProperty('Ignored'); + }); + }); + + it('mergeConnections should filter types on a first-time source entry', () => { + const workflow = createWorkflow([createNode({ id: 'node1', name: 'Source' })]); + const operations: WorkflowOperation[] = [ + { + type: 'mergeConnections', + connections: jsonParse( + '{"Source":{"__proto__":[[]],"main":[[{"node":"Sink","type":"main","index":0}]]}}', + ), + }, + ]; + + const result = applyOperations(workflow, operations); + + expect(Object.keys(result.connections.Source)).toEqual(['main']); + expect(Object.getPrototypeOf(result.connections.Source)).toBe(Object.prototype); + }); + + describe('removeNode', () => { + it('should not reparent the rebuilt connections map', () => { + const workflow: SimpleWorkflow = { + name: 'Test Workflow', + nodes: [createNode({ id: 'node1', name: 'Keep' }), createNode({ id: 'node2' })], + connections: jsonParse( + '{"__proto__":{"shell":[[{"node":"Keep","type":"main","index":0}]]},"Keep":{"main":[[]]}}', + ), + }; + const operations: WorkflowOperation[] = [{ type: 'removeNode', nodeIds: ['node2'] }]; + + const result = applyOperations(workflow, operations); + + expect(Object.getPrototypeOf(result.connections)).toBe(Object.prototype); + expect(Object.keys(result.connections)).toEqual(['Keep']); + expect(({} as Record).shell).toBeUndefined(); + }); + }); + + describe('removeConnection', () => { + it('should not write through a reserved source node to a shared slot', () => { + // Start from a shared slot that already holds a connection-shaped value, so the + // unguarded lookup finds something to rewrite and then delete. + (Object.prototype as Record).main = [ + [{ node: 'Sink', type: 'main', index: 0 }], + ]; + + const workflow = createWorkflow([createNode({ id: 'node1', name: 'Sink' })]); + const operations: WorkflowOperation[] = [ + { + type: 'removeConnection', + sourceNode: '__proto__', + targetNode: 'Sink', + connectionType: 'main', + sourceOutputIndex: 0, + targetInputIndex: 0, + }, + ]; + + const result = applyOperations(workflow, operations); + + expect(({} as Record).main).toEqual([ + [{ node: 'Sink', type: 'main', index: 0 }], + ]); + expect(result.connections).toEqual({}); + }); + + it('should leave the workflow unchanged for a reserved connection type', () => { + const workflow: SimpleWorkflow = { + name: 'Test Workflow', + nodes: [createNode({ id: 'node1', name: 'Source' })], + connections: { Source: { main: [[{ node: 'Sink', type: 'main', index: 0 }]] } }, + }; + const operations: WorkflowOperation[] = [ + { + type: 'removeConnection', + sourceNode: 'Source', + targetNode: 'Sink', + connectionType: 'constructor', + sourceOutputIndex: 0, + targetInputIndex: 0, + }, + ]; + + const result = applyOperations(workflow, operations); + + expect(result.connections.Source?.main?.[0]).toHaveLength(1); + }); + }); + + describe('setConnections', () => { + it('should keep only entries whose keys are usable object keys', () => { + const workflow = createWorkflow([createNode({ id: 'node1', name: 'Source' })]); + const operations: WorkflowOperation[] = [ + { + type: 'setConnections', + connections: jsonParse( + '{"__proto__":{"ai_tool":[[]]},"Source":{"main":[[{"node":"Sink","type":"main","index":0}]],"constructor":[[]]}}', + ), + }, + ]; + + const result = applyOperations(workflow, operations); + + expect(Object.keys(result.connections)).toEqual(['Source']); + expect(Object.keys(result.connections.Source)).toEqual(['main']); + expect(({} as Record).ai_tool).toBeUndefined(); + }); + }); + }); }); diff --git a/packages/@n8n/ai-workflow-builder.ee/src/utils/test/workflow-sanitization.test.ts b/packages/@n8n/ai-workflow-builder.ee/src/utils/test/workflow-sanitization.test.ts new file mode 100644 index 00000000000..981ae0095c6 --- /dev/null +++ b/packages/@n8n/ai-workflow-builder.ee/src/utils/test/workflow-sanitization.test.ts @@ -0,0 +1,215 @@ +import type { IConnection, IConnections, INode, NodeConnectionType } from 'n8n-workflow'; +import { jsonParse, mapConnectionsByDestination } from 'n8n-workflow'; + +import type { SimpleWorkflow } from '@/types/workflow'; + +import { sanitizeWorkflowForBuilder } from '../workflow-sanitization'; + +/** + * A persisted workflow reaches the builder as parsed JSON, so a reserved key arrives as an + * own property of the connections map. An object literal cannot express that (`__proto__` + * in a literal sets the prototype instead), so build these fixtures with JSON.parse. + */ +function parseConnections(json: string): IConnections { + return jsonParse(json); +} + +function createNode(name: string): INode { + return { + id: `id-${name}`, + name, + type: 'n8n-nodes-base.noOp', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }; +} + +function createWorkflow(nodes: INode[], connections: IConnections = {}): SimpleWorkflow { + return { name: 'Test Workflow', nodes, connections }; +} + +describe('sanitizeWorkflowForBuilder', () => { + const reservedKeys = ['main', 'ai_languageModel', 'shell']; + + afterEach(() => { + for (const key of reservedKeys) { + delete (Object.prototype as Record)[key]; + } + }); + + it('should return the same workflow when every name and connection key is usable', () => { + const workflow = createWorkflow([createNode('Manual'), createNode('Sink')], { + Manual: { main: [[{ node: 'Sink', type: 'main', index: 0 }]] }, + }); + + const result = sanitizeWorkflowForBuilder(workflow); + + // Unchanged input is returned as-is, so the common case allocates nothing. + expect(result).toBe(workflow); + }); + + it('should drop a node whose name is not a usable object key', () => { + const workflow = createWorkflow([createNode('__proto__'), createNode('Sink')]); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(result.nodes.map((n) => n.name)).toEqual(['Sink']); + }); + + it('should drop connections that start from a node with an unusable name', () => { + const workflow = createWorkflow( + [createNode('__proto__'), createNode('Sink')], + parseConnections('{"__proto__":{"main":[[{"node":"Sink","type":"main","index":0}]]}}'), + ); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(Object.keys(result.connections)).toEqual([]); + }); + + it('should drop connections that point at a node with an unusable name', () => { + const workflow = createWorkflow([createNode('Manual'), createNode('__proto__')], { + Manual: { + main: [ + [ + { node: '__proto__', type: 'main', index: 0 }, + { node: 'Sink', type: 'main', index: 0 }, + ], + ], + }, + }); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(result.connections.Manual?.main?.[0]).toEqual([ + { node: 'Sink', type: 'main', index: 0 }, + ]); + }); + + it('should drop a connection type that is not a usable object key', () => { + const workflow = createWorkflow( + [createNode('Manual'), createNode('Sink')], + parseConnections( + '{"Manual":{"__proto__":[[{"node":"Sink","type":"main","index":0}]],"main":[[{"node":"Sink","type":"main","index":0}]]}}', + ), + ); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(Object.keys(result.connections.Manual)).toEqual(['main']); + }); + + it('should leave the shared object prototype untouched for a reserved source key', () => { + const workflow = createWorkflow( + [createNode('Sink')], + parseConnections('{"__proto__":{"shell":[[{"node":"Sink","type":"main","index":0}]]}}'), + ); + + sanitizeWorkflowForBuilder(workflow); + + expect(({} as Record).shell).toBeUndefined(); + }); + + it('should drop a connection whose target name has no node entry', () => { + // A stored payload can name a target that is absent from `nodes`, so filtering by the + // set of known unsafe node names is not enough on its own. + const workflow = createWorkflow([createNode('Manual')], { + Manual: { + main: [ + [ + { node: '__proto__', type: 'main', index: 0 }, + { node: 'Sink', type: 'main', index: 0 }, + ], + ], + }, + }); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(result.connections.Manual?.main?.[0]).toEqual([ + { node: 'Sink', type: 'main', index: 0 }, + ]); + }); + + it('should drop a connection whose type field is not a usable object key', () => { + // Traversal helpers key the inverted graph by this field as well as by `node`. + const workflow = createWorkflow([createNode('Manual'), createNode('Sink')], { + Manual: { + main: [ + [ + { node: 'Sink', type: '__proto__' as NodeConnectionType, index: 0 }, + { node: 'Sink', type: 'main', index: 0 }, + ], + ], + }, + }); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(result.connections.Manual?.main?.[0]).toEqual([ + { node: 'Sink', type: 'main', index: 0 }, + ]); + }); + + // A request body is JSON, so these fields are only strings by convention. The key check + // has to reject a non-string outright: bracket access would coerce it back to the very + // name being guarded against ( obj[['__proto__']] writes to obj['__proto__'] ). + const coercibleKeys: Array<[string, unknown]> = [ + ['a string', '__proto__'], + ['an array', ['__proto__']], + ['a nested array', [['__proto__']]], + ['an object with toString', { toString: (): string => '__proto__' }], + ]; + + it.each(coercibleKeys)('should drop a connection whose target is %s', (_label, node) => { + const workflow = createWorkflow([createNode('Manual')], { + Manual: { + main: [[{ node, type: 'main', index: 0 } as unknown as IConnection]], + }, + }); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(result.connections.Manual?.main?.[0]).toEqual([]); + expect(({} as Record).main).toBeUndefined(); + }); + + it.each(coercibleKeys)('should drop a node whose name is %s', (_label, name) => { + const workflow = createWorkflow([ + { ...createNode('placeholder'), name } as unknown as INode, + createNode('Keep'), + ]); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(result.nodes.map((n) => n.name)).toEqual(['Keep']); + }); + + it('should leave the inverted graph intact for a non-string target', () => { + // End to end on the choke-point property: whatever the sanitizer returns must be + // safe to invert, which is where a surviving field would be used as a key. + const workflow = jsonParse( + '{"name":"w","nodes":[],"connections":{"A":{"main":[[{"node":["__proto__"],"type":"main","index":0}]]}}}', + ); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(result.connections.A?.main?.[0]).toEqual([]); + expect(Object.getPrototypeOf(mapConnectionsByDestination(result.connections))).toBe( + Object.prototype, + ); + }); + + it('should keep the workflow name and unrelated nodes', () => { + const workflow = createWorkflow([createNode('__proto__'), createNode('Keep')], { + Keep: { main: [[]] }, + }); + + const result = sanitizeWorkflowForBuilder(workflow); + + expect(result.name).toBe('Test Workflow'); + expect(result.nodes.map((n) => n.name)).toEqual(['Keep']); + expect(result.connections.Keep).toBeDefined(); + }); +}); diff --git a/packages/@n8n/ai-workflow-builder.ee/src/utils/workflow-sanitization.ts b/packages/@n8n/ai-workflow-builder.ee/src/utils/workflow-sanitization.ts new file mode 100644 index 00000000000..9114dcff1d0 --- /dev/null +++ b/packages/@n8n/ai-workflow-builder.ee/src/utils/workflow-sanitization.ts @@ -0,0 +1,72 @@ +import type { IConnection, IConnections } from 'n8n-workflow'; +import { isUsableObjectKey } from 'n8n-workflow'; + +import type { SimpleWorkflow } from '../types/workflow'; + +/** + * A connection can only be kept if both of its node-name and connection-type fields can be + * own keys of an object. Traversal helpers invert the graph and key the result by these two + * fields, so an unusable value there is as bad as an unusable key in the stored map — and a + * stored payload can name a target that has no node entry at all. + */ +function isUsableConnection(connection: IConnection): boolean { + return isUsableObjectKey(connection.node) && isUsableObjectKey(connection.type); +} + +/** + * Remove nodes whose name cannot be an own key of a plain object, together with every + * connection that references them or that is itself keyed by such a name. + * + * The builder keys its connection maps by node name. A name such as `__proto__` resolves + * to an inherited slot instead of an own key, so a node with that name can never take part + * in a valid graph. The write paths guard their own keys; this runs once when a stored + * workflow enters builder state, so no later step has to reason about the name at all. + */ +export function sanitizeWorkflowForBuilder(workflow: SimpleWorkflow): SimpleWorkflow { + const nodes = workflow.nodes ?? []; + + // Set when anything is dropped, so an untouched workflow is returned as-is. + let changed = false; + + const keptNodes = nodes.filter((node) => { + if (isUsableObjectKey(node.name)) return true; + changed = true; + return false; + }); + + const connections: IConnections = {}; + + for (const [sourceName, nodeConnections] of Object.entries(workflow.connections ?? {})) { + if (!isUsableObjectKey(sourceName)) { + changed = true; + continue; + } + + const keptTypes: IConnections[string] = {}; + + for (const [connectionType, outputs] of Object.entries(nodeConnections)) { + if (!isUsableObjectKey(connectionType) || !Array.isArray(outputs)) { + changed = true; + continue; + } + + keptTypes[connectionType] = outputs.map((outputConnections) => { + if (!Array.isArray(outputConnections)) return outputConnections; + + const kept = outputConnections.filter(isUsableConnection); + if (kept.length !== outputConnections.length) changed = true; + return kept; + }); + } + + connections[sourceName] = keptTypes; + } + + if (!changed) return workflow; + + return { + ...workflow, + nodes: keptNodes, + connections, + }; +} diff --git a/packages/@n8n/ai-workflow-builder.ee/src/workflow-builder-agent.ts b/packages/@n8n/ai-workflow-builder.ee/src/workflow-builder-agent.ts index fda2b4baa5b..0c1729a1715 100644 --- a/packages/@n8n/ai-workflow-builder.ee/src/workflow-builder-agent.ts +++ b/packages/@n8n/ai-workflow-builder.ee/src/workflow-builder-agent.ts @@ -43,6 +43,7 @@ import { } from './utils/stream-processor'; import { estimateTokenCountFromMessages } from './utils/token-usage'; import { executeToolsInParallel } from './utils/tool-executor'; +import { sanitizeWorkflowForBuilder } from './utils/workflow-sanitization'; import { WorkflowState } from './workflow-state'; /** @@ -459,12 +460,13 @@ export class WorkflowBuilderAgent { } private getDefaultWorkflowJSON(payload: ChatPayload): SimpleWorkflow { - return ( - (payload.workflowContext?.currentWorkflow as SimpleWorkflow) ?? { - nodes: [], - connections: {}, - } - ); + const currentWorkflow = payload.workflowContext?.currentWorkflow as SimpleWorkflow | undefined; + + if (!currentWorkflow) { + return { nodes: [], connections: {}, name: '' }; + } + + return sanitizeWorkflowForBuilder(currentWorkflow); } async *chat(payload: ChatPayload, userId?: string, abortSignal?: AbortSignal) { diff --git a/packages/@n8n/expression-runtime/package.json b/packages/@n8n/expression-runtime/package.json index 59a077dbe06..88f1d6b92e8 100644 --- a/packages/@n8n/expression-runtime/package.json +++ b/packages/@n8n/expression-runtime/package.json @@ -32,7 +32,7 @@ "license": "SEE LICENSE IN LICENSE.md", "dependencies": { "@n8n/errors": "workspace:*", - "@n8n/tournament": "1.9.1", + "@n8n/tournament": "1.11.1", "isolated-vm": "catalog:", "jmespath": "0.16.0", "js-base64": "catalog:", diff --git a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/custom-css.test.ts b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/custom-css.test.ts new file mode 100644 index 00000000000..af3b0f3e2dc --- /dev/null +++ b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/custom-css.test.ts @@ -0,0 +1,188 @@ +import { JSDOM } from 'jsdom'; + +import { createPage } from '../templates'; + +/** + * The custom-CSS sink, tested through a real HTML parser and a real JS engine. + * + * The page ships an empty ``) are exactly where such a model is most likely to be wrong. JSDOM + * replaces the model with the parsers themselves. + */ + +const LINE_SEPARATOR = String.fromCharCode(0x2028); +const PARAGRAPH_SEPARATOR = String.fromCharCode(0x2029); + +/** Ties the assertions to the element `templates.ts` fills. */ +const CUSTOM_CSS_ELEMENT_ID = 'n8n-chat-custom-css'; + +// Inputs a browser resolves as closing a style element, including the spellings an HTML +// parser disagrees on. The `<<` and `', + '', + '', + '', + '', + '', + '', + '', + '', + '-->', +]; + +// Inputs aimed at the assignment the CSS travels in rather than at the style element: +// closing the JS string literal, closing the script element, and the two line +// terminators that a JSON string may hold but a pre-ES2019 string literal could not. +const SCRIPT_CONTEXT_INPUTS = [ + '"; alert("foobar"); //', + '"; alert(1); "', + '\\"; alert(1); //', + '\\\\"; alert(1); //', + '"}); alert(1); ({"', + '', + '', + '', + `";${LINE_SEPARATOR}alert(1);//`, + `";${PARAGRAPH_SEPARATOR}alert(1);//`, + '\n"; alert(1); //', + '\u0000"; alert(1); //', +]; + +// Legal CSS that a filter would damage. The range syntax of Media Queries Level 4 holds +// a bare `<`, so any rule that removes or escapes `<` in the markup drops it silently. +// These are the reason the CSS travels as a string instead. +const LEGAL_CSS = [ + ['plain rules', '.chat { color: red; font-size: 14px; }'], + ['combinators', 'div > span + p ~ .class:hover { background: #fff; }'], + ['media query range', '@media (width < 600px) { .chat { display: none; } }'], + ['container query range', '@container (width < 400px) { .chat { gap: 0; } }'], + ['a literal < in a value', '.chat::after { content: "<3"; }'], + ['an escaped quote', ".chat::after { content: '\\\\'; }"], + ['a line separator', `.chat { color: red; }${LINE_SEPARATOR}`], +] as const; + +const params = { + instanceId: 'test-instance', + webhookUrl: 'http://test.com/webhook', + showWelcomeScreen: false, + loadPreviousSession: 'notSupported' as const, + i18n: { en: {} }, + mode: 'test' as const, + authentication: 'none' as const, + allowFileUploads: false, + allowedFilesMimeTypes: '', + customCss: '', + enableStreaming: false, + initialMessages: '', +}; + +/** + * Parses the rendered page the way a browser does, with its classic scripts running. + * + * `beforeParse` is where the recorders go: JSDOM runs scripts inside the constructor, so + * anything assigned to `dom.window` afterwards arrives too late and the test silently + * stops watching for execution. The widget bootstrap is a module script, which JSDOM + * skips, and `resources` defaults to loading nothing, so no test reaches the CDN. + */ +function renderPage(customCss: string) { + const executed: string[] = []; + + const dom = new JSDOM(createPage({ ...params, customCss }), { + runScripts: 'dangerously', + url: 'http://test.local/chat', + beforeParse(window) { + window.alert = (message?: string) => executed.push(`alert(${String(message)})`); + window.addEventListener('error', (event) => + executed.push(`error(${(event as ErrorEvent).message})`), + ); + }, + }); + + return { document: dom.window.document, executed }; +} + +/** The classic scripts in the page. The module bootstrap never runs under JSDOM. */ +function executableScripts(document: Document) { + return [...document.querySelectorAll('script')].filter((script) => { + const type = script.getAttribute('type'); + return type === null || type === 'text/javascript'; + }); +} + +/** + * The whole contract, asserted against the parsed page. Every input goes through this: + * a hostile one must not behave differently from a benign one, which is the point of + * moving the value out of the markup. + */ +function expectDeliveredAsCssText(customCss: string) { + const { document, executed } = renderPage(customCss); + + // Execution comes first. A payload that ran is the finding; asserting the delivered + // text before this would report a mismatched string and hide it. + expect(executed).toEqual([]); + // The input contributed no tag - neither one it spelled out nor one spliced together + // from the text on either side of a removed match. + expect(document.querySelectorAll('img, svg, iframe, object, embed')).toHaveLength(0); + // The page's own base block and the custom one. A third means the input opened it. + expect(document.querySelectorAll('style')).toHaveLength(2); + // The injector is the only script the page can ever execute, and only when there is + // CSS to apply. + expect(executableScripts(document)).toHaveLength(customCss ? 1 : 0); + + const element = document.getElementById(CUSTOM_CSS_ELEMENT_ID); + expect(element?.tagName).toBe('STYLE'); + // The CSS arrives byte for byte. Nothing is removed and nothing is escaped, which is + // what no HTML context can offer: reaching this element through one would cost the + // legal CSS below, and a NUL would arrive as U+FFFD. + expect(element?.textContent).toBe(customCss); +} + +describe('ChatTrigger custom CSS', () => { + it.each(STYLE_CLOSING_INPUTS)('does not let %j close the style element', (customCss) => { + expectDeliveredAsCssText(customCss); + }); + + it.each(SCRIPT_CONTEXT_INPUTS)('does not let %j escape the assignment', (customCss) => { + expectDeliveredAsCssText(customCss); + }); + + it.each(LEGAL_CSS)('delivers %s unchanged', (_label, customCss) => { + expectDeliveredAsCssText(customCss); + }); + + it('leaves the element empty and adds no script when there is no custom CSS', () => { + expectDeliveredAsCssText(''); + }); + + // The one property the parsed DOM cannot state: that the value reached a single place + // in the page. The DOM shows where it landed, not that it landed nowhere else. + it('writes the CSS to exactly one place in the page', () => { + // Anchored at both ends, so the only part that may vary between two pages is the + // string literal. An input that added a statement, or escaped the literal or the + // script element, cannot still match. + const injector = new RegExp( + '\\s*', + ); + const pageWithoutCustomCss = createPage({ ...params, customCss: '' }); + + for (const customCss of [...STYLE_CLOSING_INPUTS, ...SCRIPT_CONTEXT_INPUTS]) { + const page = createPage({ ...params, customCss }); + + expect(page).toMatch(injector); + // Everything outside the literal is fixed text, so dropping the injector must + // give back the page built with no custom CSS at all. + expect(page.replace(injector, '')).toBe(pageWithoutCustomCss); + } + }); +}); diff --git a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/templates.test.ts b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/templates.test.ts index 63d2cb0410f..c28028b189f 100644 --- a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/templates.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/__test__/templates.test.ts @@ -1,7 +1,6 @@ import { createPage, escapeForScriptContext, - getSanitizedCustomCss, getSanitizedInitialMessages, getSanitizedI18nConfig, } from '../templates'; @@ -126,76 +125,6 @@ describe('ChatTrigger Templates Security', () => { }); }); - describe('XSS Prevention in customCss', () => { - it('should strip { - const result = createPage({ - ...defaultParams, - customCss: ' { - const result = createPage({ - ...defaultParams, - customCss: '', - }); - - expect(result).not.toContain(' to prevent parser differential XSS', () => { - const result = createPage({ - ...defaultParams, - customCss: '', - }); - - // is recognized as a closing tag by browsers but not sanitize-html - expect(result).not.toContain(''); - expect(result).not.toContain(' variant', () => { - const result = createPage({ - ...defaultParams, - customCss: '', - }); - - expect(result).not.toContain(' { - const result = createPage({ - ...defaultParams, - customCss: '', - }); - - expect(result).not.toContain(''); - }); - - it('should preserve legitimate CSS', () => { - const css = '.chat { color: red; font-size: 14px; }'; - const result = createPage({ - ...defaultParams, - customCss: css, - }); - - expect(result).toContain(css); - }); - - it('should preserve CSS with special characters', () => { - const css = 'div > span + p ~ .class:hover { background: #fff; }'; - const result = createPage({ - ...defaultParams, - customCss: css, - }); - - expect(result).toContain(css); - }); - }); - describe('General Security', () => { it('should not expose raw user input in HTML comments or other locations', () => { const maliciousInput = ''; @@ -456,43 +385,6 @@ describe('ChatTrigger Templates Security', () => { }); }); - describe('getSanitizedCustomCss function', () => { - it('should strip { - expect(getSanitizedCustomCss('')).toBe( - '>', - ); - }); - - it('should strip parser differential variant', () => { - expect(getSanitizedCustomCss('')).toBe( - '/>', - ); - }); - - it('should strip { - expect(getSanitizedCustomCss('')).toBe('>'); - expect(getSanitizedCustomCss('')).toBe('>'); - expect(getSanitizedCustomCss('')).toBe('>'); - }); - - it('should strip multiple { - expect(getSanitizedCustomCss('x')).toBe('>x>'); - }); - - it('should strip partial ', () => { - expect(getSanitizedCustomCss(' { - expect(getSanitizedCustomCss('')).toBe(''); - }); - - it('should preserve legitimate CSS', () => { - const css = '.chat { color: red; } div > span + p ~ .class:hover { background: #fff; }'; - expect(getSanitizedCustomCss(css)).toBe(css); - }); - }); - describe('getSanitizedInitialMessages function', () => { it('should sanitize XSS payloads', () => { const maliciousInput = '"%09'; diff --git a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/templates.ts b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/templates.ts index aa9c76a150c..cdb1ef611a7 100644 --- a/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/templates.ts +++ b/packages/@n8n/nodes-langchain/nodes/trigger/ChatTrigger/templates.ts @@ -52,11 +52,31 @@ export function getSanitizedI18nConfig(config: Record): Record context. - // Browsers treat as a closing tag, - // so we remove all ...` put it one `` was, which keeps the cascade order. + */ +function buildCustomCssScript(customCss: string): string { + if (!customCss) return ''; + + return ` + `; } export function createPage({ @@ -104,7 +124,7 @@ export function createPage({ const sanitizedShowWelcomeScreen = !!showWelcomeScreen; const sanitizedAllowFileUploads = !!allowFileUploads; const sanitizedAllowedFilesMimeTypes = sanitizeUserInput(allowedFilesMimeTypes?.toString() ?? ''); - const sanitizedCustomCss = getSanitizedCustomCss(customCss?.toString() ?? ''); + const customCssScript = buildCustomCssScript(customCss?.toString() ?? ''); const sanitizedLoadPreviousSession = validLoadPreviousSessionOptions.includes( loadPreviousSession as LoadPreviousSessionChatOption, @@ -131,7 +151,7 @@ export function createPage({ height: 100%; } - + ${customCssScript}