From 00aad642c27e6b3fd1d5560669ee183656f08e44 Mon Sep 17 00:00:00 2001 From: "n8n-assistant[bot]" <100856346+n8n-assistant[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 08:33:35 +0300 Subject: [PATCH] chore: Bundle/2.x (backport to release-candidate/2.35.x) (#36561) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: n8n-assistant[bot] <100856346+n8n-assistant[bot]@users.noreply.github.com> Co-authored-by: Matsu Co-authored-by: Sam Wooler Co-authored-by: Dimitri Lavrenük <20122620+dlavrenuek@users.noreply.github.com> Co-authored-by: Bernhard Wittmann Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Lorent Lempereur Co-authored-by: yehorkardash Co-authored-by: Danny Martini Co-authored-by: mfsiega <93014743+mfsiega@users.noreply.github.com> Co-authored-by: phyllis-noester <102315132+phyllis-noester@users.noreply.github.com> Co-authored-by: n8n-assistant[bot] Co-authored-by: Emilia <100027345+sovietspaceship@users.noreply.github.com> Co-authored-by: Robin Braumann <50590409+bjorger@users.noreply.github.com> Co-authored-by: Cursor Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- .../http/__tests__/connection-guards.test.ts | 78 +++ .../__tests__/env-proxy-http-agent.test.ts | 61 ++- .../__tests__/env-proxy-https-agent.test.ts | 61 ++- .../src/http/__tests__/node-agents.test.ts | 169 ++++++- .../outbound-http.node-agent.test.ts | 92 +--- .../http/__tests__/outbound-http.ssrf.test.ts | 138 +++++- .../__tests__/transport-integration.test.ts | 43 +- .../axios/__tests__/ssrf-redirect.test.ts | 190 +++++++- .../src/http/axios/__tests__/utils.test.ts | 61 +++ .../backend-network/src/http/axios/request.ts | 2 + .../backend-network/src/http/axios/utils.ts | 14 +- .../src/http/connection-guards.ts | 85 ++++ .../src/http/env-proxy-http-agent.ts | 6 +- .../src/http/legacy-request.ts | 8 +- .../backend-network/src/http/node-agents.ts | 72 ++- .../backend-network/src/http/outbound-http.ts | 9 +- .../src/http/undici/transport.ts | 55 ++- .../src/ssrf/__tests__/mock-ssrf-bridge.ts | 47 +- .../src/bridge/isolated-vm-bridge.ts | 32 +- .../src/runtime/__tests__/context.test.ts | 4 +- .../src/runtime/__tests__/lazy-proxy.test.ts | 26 +- .../expression-runtime/src/runtime/context.ts | 95 ++-- .../src/runtime/lazy-proxy.ts | 25 +- .../expression-runtime/src/types/evaluator.ts | 8 +- packages/@n8n/tournament/src/Constants.ts | 8 +- .../@n8n/tournament/src/VariablePolyfill.ts | 44 +- .../tournament/test/VariablePolyfill.test.ts | 107 ++++ .../__tests__/insights.controller.test.ts | 9 +- .../insights.service.integration.test.ts | 461 +++++++++++++++++- .../__tests__/insights.service.test.ts | 281 +++++++++-- ...s-by-period.repository.integration.test.ts | 161 +++++- .../insights-by-period.repository.ts | 86 +++- .../modules/insights/insights.controller.ts | 9 +- .../src/modules/insights/insights.service.ts | 54 +- .../v1/handlers/insights/insights.handler.ts | 1 + .../__tests__/workflow.service.ee.test.ts | 185 ++++++- .../__tests__/workflow.service.test.ts | 23 +- .../src/workflows/workflow-sharing.service.ts | 23 + .../cli/src/workflows/workflow.service.ee.ts | 60 ++- .../cli/src/workflows/workflow.service.ts | 2 + .../integration/insights/insights.api.test.ts | 323 +++++++++++- .../integration/public-api/insights.test.ts | 77 ++- .../workflow-sharing.service.test.ts | 22 + ...kflow-execute-node-error-reporting.test.ts | 53 +- .../src/execution-engine/workflow-execute.ts | 13 +- .../frontend/@n8n/i18n/src/locales/en.json | 2 + .../components/InsightsDashboard.test.ts | 90 ++++ .../insights/components/InsightsDashboard.vue | 41 +- .../nodes/Brevo/GenericFunctions.ts | 6 +- .../nodes/Brevo/test/GenericFunctions.test.ts | 43 ++ .../Elasticsearch/Elasticsearch.node.ts | 13 +- .../descriptions/DocumentDescription.ts | 17 +- .../descriptions/placeholders.ts | 2 +- ...ument-getAll-queryParameters.workflow.json | 69 +++ .../tests/document.getAll.test.ts | 41 ++ .../nodes/Elastic/Elasticsearch/types.ts | 1 + .../nodes-base/nodes/Git/GenericFunctions.ts | 64 +++ packages/nodes-base/nodes/Git/Git.node.ts | 58 ++- .../Git/__test__/GenericFunctions.test.ts | 79 ++- .../nodes/Git/test/Git.node.security.test.ts | 134 ++++- .../nodes/Git/test/Git.node.test.ts | 125 ++++- .../CloudFirestore/DocumentDescription.ts | 26 +- .../GoogleFirebaseCloudFirestore.node.ts | 13 +- .../document-query-parameters.workflow.json | 65 +++ .../document.query-parameters.test.ts | 48 ++ .../nodes/Google/Gmail/GenericFunctions.ts | 3 + .../Gmail/test/GenericFunctions.test.ts | 40 ++ .../nodes/MongoDb/GenericFunctions.test.ts | 8 +- .../nodes/MongoDb/GenericFunctions.ts | 113 +---- .../nodes-base/nodes/MongoDb/MongoDb.node.ts | 3 +- .../utils/__tests__/query-parameters.test.ts | 94 ++++ packages/nodes-base/utils/query-parameters.ts | 120 +++++ .../expression-engine/micro.bench.ts | 11 +- .../src/expression-evaluator-proxy.ts | 12 +- .../workflow/src/expression-sandboxing.ts | 165 +++---- packages/workflow/src/expression.ts | 13 +- packages/workflow/src/workflow-data-proxy.ts | 32 +- .../test/expression-nested-data.test.ts | 18 + .../test/expression-sandboxing.test.ts | 299 +++++++----- .../workflow/test/workflow-data-proxy.test.ts | 78 +++ scripts/build-n8n.mjs | 7 +- 81 files changed, 4505 insertions(+), 831 deletions(-) create mode 100644 packages/@n8n/backend-network/src/http/__tests__/connection-guards.test.ts create mode 100644 packages/@n8n/backend-network/src/http/connection-guards.ts create mode 100644 packages/@n8n/tournament/test/VariablePolyfill.test.ts create mode 100644 packages/nodes-base/nodes/Elastic/Elasticsearch/tests/document-getAll-queryParameters.workflow.json create mode 100644 packages/nodes-base/nodes/Elastic/Elasticsearch/tests/document.getAll.test.ts create mode 100644 packages/nodes-base/nodes/Google/Firebase/CloudFirestore/__test__/document-query-parameters.workflow.json create mode 100644 packages/nodes-base/nodes/Google/Firebase/CloudFirestore/__test__/document.query-parameters.test.ts create mode 100644 packages/nodes-base/nodes/Google/Gmail/test/GenericFunctions.test.ts create mode 100644 packages/nodes-base/utils/__tests__/query-parameters.test.ts create mode 100644 packages/nodes-base/utils/query-parameters.ts 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 new file mode 100644 index 00000000000..dd7fcb84b4b --- /dev/null +++ b/packages/@n8n/backend-network/src/http/__tests__/connection-guards.test.ts @@ -0,0 +1,78 @@ +import type { HttpsProxyAgent } from 'https-proxy-agent'; +import type http from 'node:http'; + +import { makeSsrfBridge } from '../../ssrf/__tests__/mock-ssrf-bridge'; +import { installProxyConnectionGuard } from '../connection-guards'; + +type ConnectOpts = Parameters['connect']>[1]; + +describe('installProxyConnectionGuard', () => { + function guarded(connectOpts: { host?: string | null; hostname?: string | null }) { + const connect = vi.fn().mockResolvedValue('SOCKET'); + const agent = { connectOpts, connect }; + return { agent, connect }; + } + + it.each([ + ['host', { host: 'proxy.internal' }], + ['hostname', { hostname: 'proxy.internal' }], + ])( + 'reads the proxy host from `%s` and delegates when the policy allows it', + async (_label, connectOpts) => { + const bridge = makeSsrfBridge(); + const { agent, connect } = guarded(connectOpts); + installProxyConnectionGuard(agent, bridge); + const req = {} as http.ClientRequest; + const opts = {} as ConnectOpts; + + await expect(agent.connect(req, opts)).resolves.toBe('SOCKET'); + + expect(bridge.validateConnectionHost).toHaveBeenCalledWith('proxy.internal'); + expect(connect).toHaveBeenCalledWith(req, opts); + }, + ); + + it('rejects the connection when the policy denies the proxy host', async () => { + const error = new Error('blocked'); + const bridge = makeSsrfBridge({ + validateConnectionHost: vi.fn().mockReturnValue({ ok: false, error }), + }); + const { agent, connect } = guarded({ host: '169.254.169.254' }); + installProxyConnectionGuard(agent, bridge); + + await expect(agent.connect({} as http.ClientRequest, {} as ConnectOpts)).rejects.toBe(error); + + expect(connect).not.toHaveBeenCalled(); + }); + + it('validates the proxy host on every connection, not once at installation', async () => { + const bridge = makeSsrfBridge(); + const { agent } = guarded({ host: 'proxy.internal' }); + installProxyConnectionGuard(agent, bridge); + + await agent.connect({} as http.ClientRequest, {} as ConnectOpts); + agent.connectOpts.host = 'other-proxy.internal'; + await agent.connect({} as http.ClientRequest, {} as ConnectOpts); + + expect(bridge.validateConnectionHost).toHaveBeenNthCalledWith(1, 'proxy.internal'); + expect(bridge.validateConnectionHost).toHaveBeenNthCalledWith(2, 'other-proxy.internal'); + }); + + it.each([ + ['missing', {}], + ['null', { host: null, hostname: null }], + ['empty', { host: '' }], + ['empty while `hostname` is set', { host: '', hostname: 'proxy.internal' }], + ])('rejects the connection when the proxy host is %s', async (_label, connectOpts) => { + const bridge = makeSsrfBridge(); + const { agent, connect } = guarded(connectOpts); + installProxyConnectionGuard(agent, bridge); + + await expect(agent.connect({} as http.ClientRequest, {} as ConnectOpts)).rejects.toThrow( + 'Cannot determine the host for this connection', + ); + + expect(connect).not.toHaveBeenCalled(); + expect(bridge.validateConnectionHost).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/@n8n/backend-network/src/http/__tests__/env-proxy-http-agent.test.ts b/packages/@n8n/backend-network/src/http/__tests__/env-proxy-http-agent.test.ts index a6563ba34a4..79719d5e31f 100644 --- a/packages/@n8n/backend-network/src/http/__tests__/env-proxy-http-agent.test.ts +++ b/packages/@n8n/backend-network/src/http/__tests__/env-proxy-http-agent.test.ts @@ -1,24 +1,47 @@ import http from 'node:http'; +import type { LookupFunction } from 'node:net'; import type { MockInstance } from 'vitest'; +import { makeLookupFn } from '../../ssrf/__tests__/mock-ssrf-bridge'; import { EnvProxyHttpAgent } from '../env-proxy-http-agent'; // Routing/caching is covered in env-proxy-router.test.ts; here we only assert // the agent's wiring: delegate to the resolved proxy agent, else dispatch // directly via `super.addRequest`. The proxy agent is mocked and `getProxyForUrl` // drives which branch runs, so nothing hits the network. -const { getProxyForUrl, proxyAddRequest } = vi.hoisted(() => ({ +const { getProxyForUrl, proxyAddRequest, proxyConnect, proxyAgents } = vi.hoisted(() => ({ getProxyForUrl: vi.fn<(url: string) => string>(), proxyAddRequest: vi.fn(), + proxyConnect: vi.fn(), + proxyAgents: [] as unknown[], })); vi.mock('proxy-from-env', () => ({ getProxyForUrl })); vi.mock('http-proxy-agent', () => ({ HttpProxyAgent: class { + connectOpts: { host: string }; + addRequest = proxyAddRequest; + + constructor( + proxyUrl: string, + readonly agentOptions?: { lookup?: unknown }, + ) { + this.connectOpts = { host: new URL(proxyUrl).hostname }; + proxyAgents.push(this); + } + + async connect(...args: unknown[]): Promise { + return await proxyConnect(...args); + } }, })); +type CreatedProxyAgent = { + agentOptions?: { lookup?: unknown }; + connect: (req: unknown, opts: unknown) => Promise; +}; + const req = {} as http.ClientRequest; const options = (o: Partial): http.RequestOptions => o as http.RequestOptions; @@ -28,6 +51,9 @@ describe('EnvProxyHttpAgent', () => { beforeEach(() => { getProxyForUrl.mockReset(); proxyAddRequest.mockReset(); + proxyConnect.mockReset(); + proxyConnect.mockResolvedValue('SOCKET'); + proxyAgents.length = 0; // `super.addRequest` is the only path that would open a real socket. // `addRequest` is an internal Agent method untyped on the public types. superAddRequest = vi @@ -57,4 +83,37 @@ describe('EnvProxyHttpAgent', () => { expect(proxyAddRequest).not.toHaveBeenCalled(); expect(superAddRequest).toHaveBeenCalledWith(req, opts); }); + + describe('SSRF scope of the resolved proxy agent', () => { + beforeEach(() => { + getProxyForUrl.mockReturnValue('http://proxy.internal:3128'); + }); + + function resolvedProxyAgent(lookup?: LookupFunction): CreatedProxyAgent { + new EnvProxyHttpAgent(lookup).addRequest(req, options({ host: 'a.example', port: 80 })); + return proxyAgents[0] as CreatedProxyAgent; + } + + it.each([ + ['a lookup is given', () => makeLookupFn()], + ['no lookup is given', () => undefined], + ])('opens the connection to the proxy unchecked when %s', async (_label, makeLookup) => { + const lookup = makeLookup(); + const proxyAgent = resolvedProxyAgent(lookup); + + await expect(proxyAgent.connect(req, {})).resolves.toBe('SOCKET'); + + expect(proxyAgent.agentOptions?.lookup).toBeUndefined(); + }); + + it('keeps the secure lookup on its own direct pool', () => { + const lookupFn = makeLookupFn(); + + const agent = new EnvProxyHttpAgent(lookupFn) as unknown as { + options: { lookup?: unknown }; + }; + + expect(agent.options.lookup).toBe(lookupFn); + }); + }); }); diff --git a/packages/@n8n/backend-network/src/http/__tests__/env-proxy-https-agent.test.ts b/packages/@n8n/backend-network/src/http/__tests__/env-proxy-https-agent.test.ts index 6ab3d7bdaf6..92f711b30b0 100644 --- a/packages/@n8n/backend-network/src/http/__tests__/env-proxy-https-agent.test.ts +++ b/packages/@n8n/backend-network/src/http/__tests__/env-proxy-https-agent.test.ts @@ -1,25 +1,48 @@ import type http from 'node:http'; import https from 'node:https'; +import type { LookupFunction } from 'node:net'; import type { MockInstance } from 'vitest'; +import { makeLookupFn } from '../../ssrf/__tests__/mock-ssrf-bridge'; import { EnvProxyHttpsAgent } from '../env-proxy-https-agent'; // Routing/caching is covered in env-proxy-router.test.ts; here we only assert // the agent's wiring: delegate to the resolved proxy agent, else dispatch // directly via `super.addRequest`. The proxy agent is mocked and `getProxyForUrl` // drives which branch runs, so nothing hits the network. -const { getProxyForUrl, proxyAddRequest } = vi.hoisted(() => ({ +const { getProxyForUrl, proxyAddRequest, proxyConnect, proxyAgents } = vi.hoisted(() => ({ getProxyForUrl: vi.fn<(url: string) => string>(), proxyAddRequest: vi.fn(), + proxyConnect: vi.fn(), + proxyAgents: [] as unknown[], })); vi.mock('proxy-from-env', () => ({ getProxyForUrl })); vi.mock('https-proxy-agent', () => ({ HttpsProxyAgent: class { + connectOpts: { host: string }; + addRequest = proxyAddRequest; + + constructor( + proxyUrl: string, + readonly agentOptions?: { lookup?: unknown }, + ) { + this.connectOpts = { host: new URL(proxyUrl).hostname }; + proxyAgents.push(this); + } + + async connect(...args: unknown[]): Promise { + return await proxyConnect(...args); + } }, })); +type CreatedProxyAgent = { + agentOptions?: { lookup?: unknown }; + connect: (req: unknown, opts: unknown) => Promise; +}; + const req = {} as http.ClientRequest; const options = (o: Partial): https.RequestOptions => o as https.RequestOptions; @@ -30,6 +53,9 @@ describe('EnvProxyHttpsAgent', () => { beforeEach(() => { getProxyForUrl.mockReset(); proxyAddRequest.mockReset(); + proxyConnect.mockReset(); + proxyConnect.mockResolvedValue('SOCKET'); + proxyAgents.length = 0; // `super.addRequest` is the only path that would open a real socket. // `addRequest` is an internal Agent method untyped on the public types. superAddRequest = vi @@ -59,4 +85,37 @@ describe('EnvProxyHttpsAgent', () => { expect(proxyAddRequest).not.toHaveBeenCalled(); expect(superAddRequest).toHaveBeenCalledWith(req, opts); }); + + describe('SSRF scope of the resolved proxy agent', () => { + beforeEach(() => { + getProxyForUrl.mockReturnValue('http://proxy.internal:3128'); + }); + + function resolvedProxyAgent(lookup?: LookupFunction): CreatedProxyAgent { + new EnvProxyHttpsAgent(lookup).addRequest(req, options({ host: 'a.example', port: 443 })); + return proxyAgents[0] as CreatedProxyAgent; + } + + it.each([ + ['a lookup is given', () => makeLookupFn()], + ['no lookup is given', () => undefined], + ])('opens the connection to the proxy unchecked when %s', async (_label, makeLookup) => { + const lookup = makeLookup(); + const proxyAgent = resolvedProxyAgent(lookup); + + await expect(proxyAgent.connect(req, {})).resolves.toBe('SOCKET'); + + expect(proxyAgent.agentOptions?.lookup).toBeUndefined(); + }); + + it('keeps the secure lookup on its own direct pool', () => { + const lookupFn = makeLookupFn(); + + const agent = new EnvProxyHttpsAgent(lookupFn) as unknown as { + options: { lookup?: unknown }; + }; + + expect(agent.options.lookup).toBe(lookupFn); + }); + }); }); 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 fdf3778e1cc..287e69394c2 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 @@ -1,13 +1,21 @@ import { HttpProxyAgent } from 'http-proxy-agent'; import { HttpsProxyAgent } from 'https-proxy-agent'; +import dns from 'node:dns'; import http from 'node:http'; import https from 'node:https'; +import type { LookupFunction } from 'node:net'; import type { SsrfBridge } from '../../ssrf'; -import { makeLookupFn, makeSsrfBridge } from '../../ssrf/__tests__/mock-ssrf-bridge'; +import { + makeDenyingLookup, + makeLookupFn, + makeSsrfBridge, +} from '../../ssrf/__tests__/mock-ssrf-bridge'; +import { installConnectionGuard, UnknownConnectionHostError } from '../connection-guards'; import { EnvProxyHttpAgent } from '../env-proxy-http-agent'; import { EnvProxyHttpsAgent } from '../env-proxy-https-agent'; -import { buildNodeAgents, installConnectionGuard } from '../node-agents'; +import { type LocalServer, startServer } from '../local-server'; +import { buildNodeAgents, type ProxyUrl } from '../node-agents'; // HttpsProxyAgent stores `lookup` in `connectOpts` rather than `options` // (unlike http.Agent and HttpProxyAgent which use `options`). @@ -103,39 +111,25 @@ describe('buildNodeAgents', () => { }); }); - describe('SSRF lookup placement (direct connections only)', () => { - it('proxy: false → injects the secure lookup on both agents', () => { + describe('SSRF lookup placement', () => { + const modes: Array<[string, false | 'env' | ProxyUrl]> = [ + ['proxy: false', false], + ['proxy: env', 'env'], + ['proxy: explicit URL', 'http://proxy.internal:3128'], + ]; + + it.each(modes)('%s → injects the secure lookup on both agents', (_label, proxy) => { const lookupFn = makeLookupFn(); const bridge = makeSsrfBridge({ createSecureLookup: vi.fn().mockReturnValue(lookupFn) }); - const { httpAgent, httpsAgent } = buildNodeAgents(false, bridge); + const { httpAgent, httpsAgent } = buildNodeAgents(proxy, bridge); expect(getAgentLookup(httpAgent)).toBe(lookupFn); expect(getAgentLookup(httpsAgent)).toBe(lookupFn); }); - it('proxy: env → injects the secure lookup for the direct path', () => { - const lookupFn = makeLookupFn(); - const bridge = makeSsrfBridge({ createSecureLookup: vi.fn().mockReturnValue(lookupFn) }); - - const { httpAgent, httpsAgent } = buildNodeAgents('env', bridge); - - expect(getAgentLookup(httpAgent)).toBe(lookupFn); - expect(getAgentLookup(httpsAgent)).toBe(lookupFn); - }); - - it('proxy: explicit URL → does NOT inject the lookup (proxy validates the target)', () => { - const lookupFn = makeLookupFn(); - const bridge = makeSsrfBridge({ createSecureLookup: vi.fn().mockReturnValue(lookupFn) }); - - const { httpAgent, httpsAgent } = buildNodeAgents('http://proxy.internal:3128', bridge); - - expect(getAgentLookup(httpAgent)).toBeUndefined(); - expect(getAgentLookup(httpsAgent)).toBeUndefined(); - }); - - it('ssrf disabled → no lookup on the direct path', () => { - const { httpAgent, httpsAgent } = buildNodeAgents('env', 'disabled'); + it.each(modes)('%s with SSRF disabled → no lookup on either agent', (_label, proxy) => { + const { httpAgent, httpsAgent } = buildNodeAgents(proxy, 'disabled'); expect(getAgentLookup(httpAgent)).toBeUndefined(); expect(getAgentLookup(httpsAgent)).toBeUndefined(); @@ -185,6 +179,24 @@ describe('buildNodeAgents', () => { expect(socket).toBe('SOCKET'); }); + it.each([ + ['missing', {}], + ['null', { host: null, hostname: null }], + ['empty', { host: '' }], + ['empty while `hostname` is set', { host: '', hostname: 'target.example' }], + ])('rejects the connection when the host is %s', (_label, options) => { + const bridge = makeSsrfBridge(); + const { createConnection, original } = guarded(bridge); + const onCreate = vi.fn(); + + const result = createConnection(options, onCreate); + + expect(onCreate).toHaveBeenCalledWith(expect.any(UnknownConnectionHostError)); + expect(original).not.toHaveBeenCalled(); + expect(result).toBeUndefined(); + expect(bridge.validateConnectionHost).not.toHaveBeenCalled(); + }); + it('passes the raw host through to the bridge (normalization is the service’s job)', () => { const bridge = makeSsrfBridge(); const { createConnection } = guarded(bridge); @@ -214,4 +226,107 @@ describe('buildNodeAgents', () => { }, ); }); + + describe('proxy host validation (explicit proxy URL)', () => { + let proxyServer: LocalServer; + let proxyByIp: ProxyUrl; + let proxyByHostname: ProxyUrl; + + const realLookup = () => dns.lookup as unknown as LookupFunction; + + async function getThroughAgent(agent: http.Agent): Promise { + return await new Promise((resolve, reject) => { + const req = http.get('http://proxied-target.invalid/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')); + }); + }); + } + + beforeEach(async () => { + proxyServer = await startServer((_req, res) => { + res.setHeader('Content-Type', 'text/plain'); + res.end('proxied'); + }); + const { port } = new URL(proxyServer.url); + proxyByIp = `http://127.0.0.1:${port}`; + proxyByHostname = `http://localhost:${port}`; + }); + + afterEach(async () => { + await proxyServer.close(); + }); + + it('rejects a proxy host the SSRF policy denies, without reaching the proxy', async () => { + const error = new Error( + 'The request was blocked because it resolves to a restricted IP address', + ); + const bridge = makeSsrfBridge({ + createSecureLookup: realLookup, + validateConnectionHost: vi.fn().mockReturnValue({ ok: false, error }), + }); + const { httpAgent } = buildNodeAgents(proxyByIp, bridge); + + await expect(getThroughAgent(httpAgent)).rejects.toThrow(error.message); + + expect(bridge.validateConnectionHost).toHaveBeenCalledWith('127.0.0.1'); + expect(proxyServer.captured).toEqual([]); + }); + + it.each(['httpAgent', 'httpsAgent'] as const)( + 'validates the proxy host before the %s opens its connection', + async (which) => { + const error = new Error('blocked'); + const bridge = makeSsrfBridge({ + validateConnectionHost: vi.fn().mockReturnValue({ ok: false, error }), + }); + const agents = buildNodeAgents('http://proxy.internal:3128', bridge); + const agent = agents[which] as unknown as { + connect: (req: unknown, opts: unknown) => Promise; + }; + + await expect(agent.connect({}, {})).rejects.toBe(error); + + expect(bridge.validateConnectionHost).toHaveBeenCalledWith('proxy.internal'); + }, + ); + + it('rejects a proxy hostname that resolves to a restricted address', async () => { + const error = new Error( + 'The request was blocked because it resolves to a restricted IP address', + ); + const lookup = makeDenyingLookup(error); + const bridge = makeSsrfBridge({ createSecureLookup: () => lookup }); + const { httpAgent } = buildNodeAgents(proxyByHostname, bridge); + + await expect(getThroughAgent(httpAgent)).rejects.toThrow(error.message); + + expect(lookup).toHaveBeenCalledWith('localhost', expect.any(Object), expect.any(Function)); + expect(proxyServer.captured).toEqual([]); + }); + + it('connects through a proxy host the SSRF policy allows', async () => { + const bridge = makeSsrfBridge({ createSecureLookup: realLookup }); + const { httpAgent } = buildNodeAgents(proxyByIp, bridge); + + await expect(getThroughAgent(httpAgent)).resolves.toBe('proxied'); + + expect(bridge.validateConnectionHost).toHaveBeenCalledWith('127.0.0.1'); + expect(proxyServer.captured).toEqual(['http://proxied-target.invalid/x']); + }); + + it('connects through the proxy unchanged when SSRF protection is disabled', async () => { + const { httpAgent } = buildNodeAgents(proxyByHostname, 'disabled'); + + await expect(getThroughAgent(httpAgent)).resolves.toBe('proxied'); + + expect(proxyServer.captured).toEqual(['http://proxied-target.invalid/x']); + }); + }); }); diff --git a/packages/@n8n/backend-network/src/http/__tests__/outbound-http.node-agent.test.ts b/packages/@n8n/backend-network/src/http/__tests__/outbound-http.node-agent.test.ts index fc735f214b6..e60febf52b1 100644 --- a/packages/@n8n/backend-network/src/http/__tests__/outbound-http.node-agent.test.ts +++ b/packages/@n8n/backend-network/src/http/__tests__/outbound-http.node-agent.test.ts @@ -1,8 +1,8 @@ import type { Logger } from '@n8n/backend-common'; import { HttpProxyAgent } from 'http-proxy-agent'; import { HttpsProxyAgent } from 'https-proxy-agent'; -import http from 'node:http'; -import https from 'node:https'; +import type http from 'node:http'; +import type https from 'node:https'; import { mock } from 'vitest-mock-extended'; import type { SsrfProtectionService } from '../../ssrf'; @@ -30,15 +30,9 @@ function getAgentLookup(agent: http.Agent | https.Agent): unknown { // --------------------------------------------------------------------------- describe('getNodeAgent', () => { - it('proxy: false → plain http/https.Agent (no proxy class)', () => { - const { httpAgent, httpsAgent } = makeFacade().transport({ proxy: false }).getNodeAgent(); - - expect(httpAgent).toBeInstanceOf(http.Agent); - expect(httpsAgent).toBeInstanceOf(https.Agent); - expect(httpAgent).not.toBeInstanceOf(HttpProxyAgent); - expect(httpsAgent).not.toBeInstanceOf(HttpsProxyAgent); - }); - + // Proxy-mode → agent-class matrix is owned by `node-agents.test.ts` + // (`buildNodeAgents` → `describe('agent classes per proxy mode')`); this is a + // representative case proving the facade forwards to it correctly. it('proxy: explicit URL → HttpProxyAgent / HttpsProxyAgent', () => { const { httpAgent, httpsAgent } = makeFacade() .transport({ proxy: 'http://proxy.internal:3128' }) @@ -48,13 +42,6 @@ describe('getNodeAgent', () => { expect(httpsAgent).toBeInstanceOf(HttpsProxyAgent); }); - it('proxy: env → custom env-routing agents (http/https.Agent subclasses)', () => { - const { httpAgent, httpsAgent } = makeFacade().transport({ proxy: 'env' }).getNodeAgent(); - - expect(httpAgent).toBeInstanceOf(http.Agent); - expect(httpsAgent).toBeInstanceOf(https.Agent); - }); - it('returns the same agent instances on repeated calls', () => { const client = makeFacade().transport(); const a1 = client.getNodeAgent(); @@ -79,62 +66,29 @@ describe('getNodeAgent', () => { // --------------------------------------------------------------------------- describe('getNodeAgent SSRF lookup injection', () => { - describe('proxy: false', () => { - it('injects createSecureLookup when SSRF is enabled', () => { - const lookupFn = makeLookupFn(); - const bridge = makeSsrfBridge({ - createSecureLookup: vi.fn().mockReturnValue(lookupFn), - }); - const { httpAgent, httpsAgent } = makeFacade() - .transport({ ssrf: bridge, proxy: false }) - .getNodeAgent(); - - expect(bridge.createSecureLookup).toHaveBeenCalledTimes(1); - expect(getAgentLookup(httpAgent)).toBe(lookupFn); - expect(getAgentLookup(httpsAgent)).toBe(lookupFn); + // The full proxy-mode matrix is owned by `node-agents.test.ts` + // (`buildNodeAgents` → `describe('SSRF lookup placement')`); this is a + // representative case proving the facade forwards to it correctly. + it('the agents carry the lookup created by the SSRF bridge', () => { + const lookupFn = makeLookupFn(); + const bridge = makeSsrfBridge({ + createSecureLookup: vi.fn().mockReturnValue(lookupFn), }); - it('does NOT inject lookup when SSRF is disabled', () => { - const { httpAgent, httpsAgent } = makeFacade() - .transport({ ssrf: 'disabled', proxy: false }) - .getNodeAgent(); + const { httpAgent, httpsAgent } = makeFacade() + .transport({ ssrf: bridge, proxy: false }) + .getNodeAgent(); - expect(getAgentLookup(httpAgent)).toBeUndefined(); - expect(getAgentLookup(httpsAgent)).toBeUndefined(); - }); + expect(getAgentLookup(httpAgent)).toBe(lookupFn); + expect(getAgentLookup(httpsAgent)).toBe(lookupFn); }); - describe('proxy: explicit URL', () => { - it('does NOT inject lookup behind an explicit proxy (proxy validates the target)', () => { - const lookupFn = makeLookupFn(); - const bridge = makeSsrfBridge({ - createSecureLookup: vi.fn().mockReturnValue(lookupFn), - }); - const { httpAgent, httpsAgent } = makeFacade() - .transport({ ssrf: bridge, proxy: 'http://proxy.internal:3128' }) - .getNodeAgent(); + it('no lookup when SSRF is disabled', () => { + const { httpAgent, httpsAgent } = makeFacade() + .transport({ ssrf: 'disabled', proxy: false }) + .getNodeAgent(); - // SSRF lookup is applied to direct connections only. Behind a proxy it - // would resolve the proxy host, not the final target, so it is omitted. - expect(getAgentLookup(httpAgent)).toBeUndefined(); - expect(getAgentLookup(httpsAgent)).toBeUndefined(); - }); - }); - - describe('proxy: env', () => { - it('injects createSecureLookup when SSRF is enabled', () => { - const lookupFn = makeLookupFn(); - const bridge = makeSsrfBridge({ - createSecureLookup: vi.fn().mockReturnValue(lookupFn), - }); - const { httpAgent, httpsAgent } = makeFacade() - .transport({ ssrf: bridge, proxy: 'env' }) - .getNodeAgent(); - - expect(bridge.createSecureLookup).toHaveBeenCalledTimes(1); - // EnvProxy* agents inherit from http/https.Agent and pass lookup to super() - expect(getAgentLookup(httpAgent)).toBe(lookupFn); - expect(getAgentLookup(httpsAgent)).toBe(lookupFn); - }); + expect(getAgentLookup(httpAgent)).toBeUndefined(); + expect(getAgentLookup(httpsAgent)).toBeUndefined(); }); }); diff --git a/packages/@n8n/backend-network/src/http/__tests__/outbound-http.ssrf.test.ts b/packages/@n8n/backend-network/src/http/__tests__/outbound-http.ssrf.test.ts index 0505da567d1..d4a1408bc7f 100644 --- a/packages/@n8n/backend-network/src/http/__tests__/outbound-http.ssrf.test.ts +++ b/packages/@n8n/backend-network/src/http/__tests__/outbound-http.ssrf.test.ts @@ -5,13 +5,18 @@ import type { Dispatcher } from 'undici'; import { mock } from 'vitest-mock-extended'; import type { SsrfBridge, SsrfProtectionService } from '../../ssrf'; -import { makeLookupFn, makeSsrfBridge } from '../../ssrf/__tests__/mock-ssrf-bridge'; +import { + makeDenyingLookup, + makeSsrfBridge, + useCleanProxyEnv, +} from '../../ssrf/__tests__/mock-ssrf-bridge'; import { type LocalServer, startServer } from '../local-server'; import { OutboundHttp } from '../outbound-http'; import { createSsrfInterceptor } from '../undici/transport'; -// SSRF enforcement lives in a single place: the dispatcher interceptor. -// This file proves it at two levels: +// SSRF enforcement on the dispatch path lives in the dispatcher interceptor +// (an explicit proxy URI is additionally checked when the dispatcher is built). +// This file proves the interceptor at two levels: // (a) a direct unit test of `createSsrfInterceptor`, and // (b) end-to-end tests against a real local server (no mocked `fetch`), so the // interceptor actually runs and we assert that a 30x cannot smuggle a @@ -49,6 +54,8 @@ function makeOpts(path: string, origin?: string) { return { path, origin } as unknown as Dispatcher.DispatchOptions; } +useCleanProxyEnv(); + describe('createSsrfInterceptor', () => { it('validates the reconstructed target URL and dispatches when allowed', async () => { const bridge = makeSsrfBridge(); @@ -89,7 +96,7 @@ describe('createSsrfInterceptor', () => { expect(bridge.validateUrl).not.toHaveBeenCalled(); expect(innerDispatch).not.toHaveBeenCalled(); - expect(handler.onResponseError).toHaveBeenCalled(); + expect(handler.onResponseError).toHaveBeenCalledWith(null, expect.any(TypeError)); }); it('falls back to onError when onResponseError is unavailable', async () => { @@ -309,14 +316,123 @@ describe('connect-time secure lookup (DNS rebinding)', () => { await expect(fetchFn('http://rebind.example/')).rejects.toThrow(); }); - it('does not derive a connect-time lookup behind an explicit proxy', () => { - const createSecureLookup = vi.fn(makeLookupFn); - const bridge = makeSsrfBridge({ createSecureLookup }); + it('resolves the proxy host through the secure lookup behind an explicit proxy', async () => { + const denied = new Error('blocked: restricted IP address'); + const lookupSpy = makeDenyingLookup(denied); + const bridge = makeSsrfBridge({ createSecureLookup: () => lookupSpy }); + const fetchFn = makeTransport({ + ssrf: bridge, + proxy: 'http://proxy.internal:3128', + }).asCustomFetch(); - // Forces the lazy dispatcher to build. The proxy resolves the target, so - // the secure lookup must not be consulted on our side. - makeTransport({ ssrf: bridge, proxy: 'http://proxy.invalid:3128' }).getDispatcher(); + const rejection = await fetchFn('http://target.invalid/x').catch((e: unknown) => e); - expect(createSecureLookup).not.toHaveBeenCalled(); + expect(lookupSpy).toHaveBeenCalledWith('proxy.internal', expect.anything(), expect.anything()); + expect(rootCauseMessage(rejection)).toBe(denied.message); + }); +}); + +describe('proxy host validation', () => { + const realLookup = () => dns.lookup as unknown as LookupFunction; + + function denyingBridge(error: Error) { + return makeSsrfBridge({ + createSecureLookup: realLookup, + validateConnectionHost: vi.fn().mockReturnValue({ ok: false, error }), + }); + } + + function bridgeDenyingProxyHost(deniedHostname: string) { + const error = new Error('The proxy host is not permitted by policy'); + const bridge = makeSsrfBridge({ + createSecureLookup: realLookup, + validateUrl: vi.fn(async (url: string | URL) => { + const hostname = typeof url === 'string' ? new URL(url).hostname : url.hostname; + return await Promise.resolve( + hostname === deniedHostname + ? { ok: false as const, error } + : { ok: true as const, result: undefined }, + ); + }), + }); + return { bridge, error }; + } + + it('rejects an explicit proxy host the policy denies', () => { + const error = new Error('The proxy host is not permitted by policy'); + const bridge = denyingBridge(error); + + expect(() => + makeTransport({ ssrf: bridge, proxy: 'http://127.0.0.1:3128' }).getDispatcher(), + ).toThrow(error.message); + expect(bridge.validateConnectionHost).toHaveBeenCalledWith('127.0.0.1'); + }); + + it('surfaces the rejection of an explicit proxy host through fetch', async () => { + const error = new Error('The proxy host is not permitted by policy'); + const fetchFn = makeTransport({ + ssrf: denyingBridge(error), + proxy: 'http://127.0.0.1:3128', + }).asCustomFetch(); + + await expect(fetchFn('http://target.invalid/x')).rejects.toThrow(error.message); + }); + + // The environment's proxies describe the deployment, so the policy that decides + // which targets a workflow may reach does not decide them. + it.each([ + ['HTTP_PROXY', 'http://target.invalid/x'], + ['HTTPS_PROXY', 'https://target.invalid/x'], + ] as const)('leaves a proxy configured through %s unchecked', async (envKey, target) => { + process.env[envKey] = 'http://127.0.0.1:3128'; + const lookupSpy = makeDenyingLookup(new Error('blocked: restricted IP address')); + const { bridge } = bridgeDenyingProxyHost('127.0.0.1'); + bridge.createSecureLookup = () => lookupSpy; + const transport = makeTransport({ ssrf: bridge, proxy: 'env' }); + + expect(() => transport.getDispatcher()).not.toThrow(); + await transport + .asCustomFetch()(target) + .catch(() => undefined); + + const validated = vi.mocked(bridge.validateUrl).mock.calls.map(([url]) => String(url)); + expect(validated).toEqual([target]); + expect(bridge.validateConnectionHost).not.toHaveBeenCalled(); + expect(lookupSpy).not.toHaveBeenCalled(); + }); + + it('leaves an explicit proxy unchecked when SSRF protection is disabled', () => { + const bridge = denyingBridge(new Error('The proxy host is not permitted by policy')); + + expect(() => + makeTransport({ ssrf: 'disabled', proxy: 'http://127.0.0.1:3128' }).getDispatcher(), + ).not.toThrow(); + expect(bridge.validateConnectionHost).not.toHaveBeenCalled(); + }); + + it('serves a target the environment exempts from the direct path', async () => { + const server = await startServer((_req, res) => { + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end('ok'); + }); + process.env.HTTP_PROXY = 'http://proxy.internal:3128'; + process.env.NO_PROXY = '127.0.0.1'; + const { bridge } = bridgeDenyingProxyHost('proxy.internal'); + const fetchFn = makeTransport({ ssrf: bridge, proxy: 'env' }).asCustomFetch(); + + try { + const res = await fetchFn(`${server.url}/x`); + + expect(res.status).toBe(200); + } finally { + await server.close(); + } + }); + + it('leaves the dispatcher untouched when no proxy is configured in the environment', () => { + const bridge = makeSsrfBridge({ createSecureLookup: realLookup }); + + expect(() => makeTransport({ ssrf: bridge, proxy: 'env' }).getDispatcher()).not.toThrow(); + expect(bridge.validateConnectionHost).not.toHaveBeenCalled(); }); }); diff --git a/packages/@n8n/backend-network/src/http/__tests__/transport-integration.test.ts b/packages/@n8n/backend-network/src/http/__tests__/transport-integration.test.ts index 00cc1c2f3fb..212ce4d520b 100644 --- a/packages/@n8n/backend-network/src/http/__tests__/transport-integration.test.ts +++ b/packages/@n8n/backend-network/src/http/__tests__/transport-integration.test.ts @@ -4,7 +4,7 @@ import dns from 'node:dns'; import http from 'node:http'; import type { LookupFunction } from 'node:net'; -import { makeSsrfBridge } from '../../ssrf/__tests__/mock-ssrf-bridge'; +import { makeSsrfBridge, useCleanProxyEnv } from '../../ssrf/__tests__/mock-ssrf-bridge'; import { getBeforeRedirectFn, setAxiosAgents } from '../axios/utils'; import { type LocalServer, startServer } from '../local-server'; import { buildNodeAgents } from '../node-agents'; @@ -31,9 +31,10 @@ async function httpGetWithAgent(url: string, agent: http.Agent): Promise } describe('outbound transport integration', () => { + useCleanProxyEnv(); + let target: LocalServer; let proxy: LocalServer; - const ORIGINAL_ENV = { ...process.env }; beforeAll(async () => { target = await startServer((req, res) => { @@ -58,18 +59,10 @@ describe('outbound transport integration', () => { }); beforeEach(() => { - delete process.env.HTTP_PROXY; - delete process.env.HTTPS_PROXY; - delete process.env.NO_PROXY; - delete process.env.ALL_PROXY; target.clear(); proxy.clear(); }); - afterEach(() => { - process.env = { ...ORIGINAL_ENV }; - }); - describe('setAxiosAgents routing', () => { it('routes through an explicit custom proxy', async () => { const config: AxiosRequestConfig = { url: `${target.url}/x`, method: 'GET', proxy: false }; @@ -120,7 +113,7 @@ describe('outbound transport integration', () => { }); }); - describe('SSRF secure lookup is applied to direct connections only', () => { + describe('SSRF secure lookup placement', () => { it('invokes the secure lookup for a direct (hostname) connection', async () => { const lookupSpy = vi.fn((hostname: string, options: dns.LookupOptions, onResult: unknown) => dns.lookup(hostname, options, onResult as never), @@ -142,8 +135,17 @@ describe('outbound transport integration', () => { expect(lookupSpy).toHaveBeenCalledWith('localhost', expect.anything(), expect.anything()); }); - it('does NOT invoke the secure lookup when routed through a proxy', async () => { - process.env.HTTP_PROXY = proxy.url; + // An explicit proxy URL can come from a request, so its host is resolved + // through the policy; the environment's is part of the deployment and is not. + it.each([ + ['leaves the env proxy host to the platform resolver', true], + ['invokes the secure lookup for an explicit proxy host', false], + ])('%s', async (_case, fromEnv) => { + const { port } = new URL(proxy.url); + const proxyUrl = `http://localhost:${port}`; + if (fromEnv) { + process.env.HTTP_PROXY = proxyUrl; + } const lookupSpy = vi.fn((hostname: string, options: dns.LookupOptions, onResult: unknown) => dns.lookup(hostname, options, onResult as never), ); @@ -156,14 +158,21 @@ describe('outbound transport integration', () => { method: 'GET', proxy: false, }; - setAxiosAgents(config, undefined, undefined, bridge); + setAxiosAgents(config, undefined, fromEnv ? undefined : proxyUrl, bridge); const res = await axios<{ message: string }>(config); expect(res.data.message).toBe('proxied'); - // The proxy host is an IP (no lookup) and the proxy resolves the final - // target, so the secure lookup is never consulted on our side. - expect(lookupSpy).not.toHaveBeenCalled(); + if (fromEnv) { + expect(lookupSpy).not.toHaveBeenCalled(); + } else { + expect(lookupSpy).toHaveBeenCalledWith('localhost', expect.anything(), expect.anything()); + } + expect(lookupSpy).not.toHaveBeenCalledWith( + 'proxied-target.invalid', + expect.anything(), + expect.anything(), + ); }); }); diff --git a/packages/@n8n/backend-network/src/http/axios/__tests__/ssrf-redirect.test.ts b/packages/@n8n/backend-network/src/http/axios/__tests__/ssrf-redirect.test.ts index f2405ec2a4e..4b37e953b51 100644 --- a/packages/@n8n/backend-network/src/http/axios/__tests__/ssrf-redirect.test.ts +++ b/packages/@n8n/backend-network/src/http/axios/__tests__/ssrf-redirect.test.ts @@ -1,8 +1,10 @@ import type { Logger } from '@n8n/backend-common'; +import dns from 'node:dns'; +import type { LookupFunction } from 'node:net'; import { mock } from 'vitest-mock-extended'; import type { SsrfBridge } from '../../../ssrf'; -import { makeSsrfBridge } from '../../../ssrf/__tests__/mock-ssrf-bridge'; +import { makeSsrfBridge, useCleanProxyEnv } from '../../../ssrf/__tests__/mock-ssrf-bridge'; import { executeLegacyRequest } from '../../legacy-request'; import { type LocalServer, startServer } from '../../local-server'; import { configureGlobalAxiosDefaults } from '../config'; @@ -16,8 +18,6 @@ import { httpRequest } from '../request'; configureGlobalAxiosDefaults(); -const PROXY_ENV_KEYS = ['HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY'] as const; - async function startRedirectServer(): Promise { let serverUrl = ''; const server = await startServer((req, res) => { @@ -67,13 +67,11 @@ function makeBridge(blockedPath: string): { bridge: SsrfBridge; error: Error } { } describe('httpRequest manual redirect following with SSRF + proxy', () => { + useCleanProxyEnv(); + let server: LocalServer; - const savedEnv: Record = {}; beforeEach(async () => { - for (const key of PROXY_ENV_KEYS) { - savedEnv[key] = process.env[key]; - } // A proxy is configured (engages the manual follower), but loopback is // exempt so the request connects directly to the local server. process.env.HTTP_PROXY = 'http://127.0.0.1:1'; @@ -84,13 +82,6 @@ describe('httpRequest manual redirect following with SSRF + proxy', () => { }); afterEach(async () => { - for (const key of PROXY_ENV_KEYS) { - if (savedEnv[key] === undefined) { - delete process.env[key]; - } else { - process.env[key] = savedEnv[key]; - } - } await server.close(); }); @@ -218,6 +209,177 @@ describe('httpRequest manual redirect following with SSRF + proxy', () => { }); }); + describe('proxy host validation across redirect hops', () => { + let proxyServer: LocalServer; + const START_URL = 'http://redirect-target.invalid/start'; + const INTERNAL_URL = 'http://redirect-target.invalid/internal'; + + beforeEach(async () => { + proxyServer = await startServer((req, res) => { + if (req.url === START_URL) { + res.writeHead(302, { Location: INTERNAL_URL }); + res.end(); + return; + } + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end('reached:proxied'); + }); + }); + + afterEach(async () => { + await proxyServer.close(); + }); + + const proxyConfig = () => ({ + host: 'localhost', + port: Number(new URL(proxyServer.url).port), + protocol: 'http', + }); + + it('follows the redirect through a proxy the policy allows', async () => { + const bridge = makeSsrfBridge({ + createSecureLookup: () => dns.lookup as unknown as LookupFunction, + }); + + const response = await httpRequest( + { method: 'GET', url: START_URL, proxy: proxyConfig() }, + bridge, + ); + + expect(response).toBe('reached:proxied'); + expect(proxyServer.captured).toEqual([START_URL, INTERNAL_URL]); + }); + + it('blocks the redirect hop when the proxy host is no longer allowed', async () => { + const denied = new Error( + 'The request was blocked because it resolves to a restricted IP address', + ); + const lookup = ((hostname: string, options: dns.LookupOptions, onResult: unknown) => { + if (proxyServer.captured.length > 0) { + (onResult as (error: Error | null, address?: unknown, family?: number) => void)( + denied, + options.all ? [] : '', + undefined, + ); + return; + } + dns.lookup(hostname, options, onResult as never); + }) as unknown as LookupFunction; + const bridge = makeSsrfBridge({ createSecureLookup: () => lookup }); + + await expect( + httpRequest({ method: 'GET', url: START_URL, proxy: proxyConfig() }, bridge), + ).rejects.toThrow(denied.message); + + expect(proxyServer.captured).toEqual([START_URL]); + }); + }); + + describe('proxy host validation before the request is sent', () => { + let proxyServer: LocalServer; + const TARGET_URL = 'http://proxied-target.invalid/x'; + + beforeEach(async () => { + proxyServer = await startServer((_req, res) => { + res.writeHead(200, { 'content-type': 'text/plain' }); + res.end('reached:proxied'); + }); + }); + + afterEach(async () => { + await proxyServer.close(); + }); + + const proxyUrl = () => `http://localhost:${new URL(proxyServer.url).port}`; + + const proxyConfig = () => ({ + host: 'localhost', + port: Number(new URL(proxyServer.url).port), + protocol: 'http', + }); + + function bridgeDenying(hostname: string, error: Error): SsrfBridge { + return makeSsrfBridge({ + createSecureLookup: () => dns.lookup as unknown as LookupFunction, + validateUrl: vi.fn(async (url: string | URL) => { + const host = typeof url === 'string' ? new URL(url).hostname : url.hostname; + return await Promise.resolve( + host === hostname + ? { ok: false as const, error } + : { ok: true as const, result: undefined }, + ); + }), + }); + } + + const requestPaths = [ + [ + 'httpRequest', + async (bridge: SsrfBridge) => + await httpRequest({ method: 'GET', url: TARGET_URL, proxy: proxyConfig() }, bridge), + ], + [ + 'executeLegacyRequest', + async (bridge: SsrfBridge) => + await executeLegacyRequest( + { uri: TARGET_URL, proxy: proxyUrl() }, + bridge, + mock(), + ), + ], + ] as const; + + it.each(requestPaths)( + 'blocks %s when the policy denies the proxy host', + async (_name, send) => { + const error = new Error('The proxy host is not permitted by policy'); + + await expect(send(bridgeDenying('localhost', error))).rejects.toBe(error); + + expect(proxyServer.captured).toEqual([]); + }, + ); + + it.each(requestPaths)('sends %s when the policy allows the proxy host', async (_name, send) => { + const bridge = bridgeDenying('never-matches.invalid', new Error('unused')); + + expect(await send(bridge)).toBe('reached:proxied'); + + expect(bridge.validateUrl).toHaveBeenCalledWith( + expect.objectContaining({ hostname: 'localhost' }), + ); + }); + + it('reports the target URL denial before checking the proxy host', async () => { + const targetError = new Error('The target host is not permitted by policy'); + const bridge = makeSsrfBridge({ + createSecureLookup: () => dns.lookup as unknown as LookupFunction, + validateUrl: vi.fn(async (url: string | URL) => { + const host = typeof url === 'string' ? new URL(url).hostname : url.hostname; + return await Promise.resolve({ + ok: false as const, + error: host === 'localhost' ? new Error('proxy denied') : targetError, + }); + }), + }); + + await expect( + httpRequest({ method: 'GET', url: TARGET_URL, proxy: proxyConfig() }, bridge), + ).rejects.toBe(targetError); + }); + + it('sends the request through the proxy when SSRF protection is disabled', async () => { + const response = await httpRequest({ + method: 'GET', + url: TARGET_URL, + proxy: proxyConfig(), + }); + + expect(response).toBe('reached:proxied'); + expect(proxyServer.captured).toEqual([TARGET_URL]); + }); + }); + describe('credential headers across redirects', () => { let origin: LocalServer; let crossOrigin: LocalServer; diff --git a/packages/@n8n/backend-network/src/http/axios/__tests__/utils.test.ts b/packages/@n8n/backend-network/src/http/axios/__tests__/utils.test.ts index 94f57ce63d2..50bb81fb386 100644 --- a/packages/@n8n/backend-network/src/http/axios/__tests__/utils.test.ts +++ b/packages/@n8n/backend-network/src/http/axios/__tests__/utils.test.ts @@ -24,6 +24,7 @@ import { searchForHeader, setAxiosAgents, tryParseUrl, + validateProxySsrf, } from '../utils'; // Agent construction is owned by `buildNodeAgents` (./factory). @@ -431,6 +432,66 @@ describe('getUrlFromProxyConfig', () => { }); }); +describe('validateProxySsrf', () => { + const denyingBridge = (deniedHostname: string, error: Error) => + makeSsrfBridge({ + validateUrl: vi.fn(async (url: string | URL) => { + const hostname = typeof url === 'string' ? new URL(url).hostname : url.hostname; + return await Promise.resolve( + hostname === deniedHostname + ? { ok: false as const, error } + : { ok: true as const, result: undefined }, + ); + }), + }); + + it.each([ + ['a proxy URL string', 'http://proxy.example.com:8080'], + ['a proxy config object', { host: 'proxy.example.com', port: 8080 }], + ])('throws when the policy denies the host of %s', async (_label, proxyConfig) => { + const error = new Error('The proxy host is not permitted by policy'); + + await expect( + validateProxySsrf(proxyConfig, denyingBridge('proxy.example.com', error)), + ).rejects.toBe(error); + }); + + it('validates the URL composed from a proxy config object', async () => { + const bridge = makeSsrfBridge(); + + await validateProxySsrf( + { + protocol: 'https', + host: 'proxy.example.com', + port: 9443, + auth: { username: 'user', password: 'pass' }, + }, + bridge, + ); + + expect(bridge.validateUrl).toHaveBeenCalledWith( + expect.objectContaining({ href: 'https://user:pass@proxy.example.com:9443/' }), + ); + }); + + it.each([ + ['no proxy is configured', undefined], + ['the proxy config has no host', { host: '', port: 8080 }], + ['the proxy scheme is not supported', 'socks5://proxy.example.com:1080'], + ['the proxy value is not a URL', 'not-a-url'], + ])('does not consult the policy when %s', async (_label, proxyConfig) => { + const bridge = makeSsrfBridge(); + + await validateProxySsrf(proxyConfig, bridge); + + expect(bridge.validateUrl).not.toHaveBeenCalled(); + }); + + it('does not consult the policy when no bridge is provided', async () => { + await expect(validateProxySsrf('http://proxy.example.com:8080')).resolves.toBeUndefined(); + }); +}); + describe('buildTargetUrl', () => { it('should return url as-is when no baseURL', () => { expect(buildTargetUrl('https://example.com/path')).toBe('https://example.com/path'); diff --git a/packages/@n8n/backend-network/src/http/axios/request.ts b/packages/@n8n/backend-network/src/http/axios/request.ts index 3f440eb7380..6ca54a0eb7b 100644 --- a/packages/@n8n/backend-network/src/http/axios/request.ts +++ b/packages/@n8n/backend-network/src/http/axios/request.ts @@ -20,6 +20,7 @@ import { searchForHeader, setAxiosAgents, throwIfDomainNotAllowed, + validateProxySsrf, validateUrlSsrf, } from './utils'; import type { SsrfBridge } from '../../ssrf'; @@ -187,6 +188,7 @@ export async function httpRequest( const url = buildTargetUrl(requestOptions.url, requestOptions.baseURL) ?? requestOptions.url; await validateUrlSsrf(url, ssrfBridge); + await validateProxySsrf(requestOptions.proxy, ssrfBridge); const axiosRequest = convertN8nRequestToAxios(requestOptions, ssrfBridge); if ( diff --git a/packages/@n8n/backend-network/src/http/axios/utils.ts b/packages/@n8n/backend-network/src/http/axios/utils.ts index 32d9686d2e6..b64184fd839 100644 --- a/packages/@n8n/backend-network/src/http/axios/utils.ts +++ b/packages/@n8n/backend-network/src/http/axios/utils.ts @@ -139,8 +139,6 @@ export const getBeforeRedirectFn = const customProxyUrl = proxyConfig ? getUrlFromProxyConfig(proxyConfig) : null; const proxy = resolveProxyOption(customProxyUrl); - // SSRF lookup is applied to direct connections only; behind a proxy the - // proxy validates the final target. const targetUrl = redirectedRequest.href; const { httpAgent, httpsAgent } = buildNodeAgents(proxy, ssrf, redirectAgentOptions); @@ -397,8 +395,6 @@ export function setAxiosAgents( const customProxyUrl = proxyConfig ? getUrlFromProxyConfig(proxyConfig) : null; const proxy = resolveProxyOption(customProxyUrl); - // SSRF lookup is applied to direct connections only; behind a proxy the - // proxy validates the final target. const { httpAgent, httpsAgent } = buildNodeAgents(proxy, ssrf, agentOptions); config.httpAgent = httpAgent; config.httpsAgent = httpsAgent; @@ -420,6 +416,16 @@ export async function validateUrlSsrf( } } +export async function validateProxySsrf( + proxyConfig: IHttpRequestOptions['proxy'] | string | undefined, + ssrfBridge?: SsrfBridge, +): Promise { + const proxyUrl = getUrlFromProxyConfig(proxyConfig); + if (!isSupportedProxyUrl(proxyUrl)) return; + + await validateUrlSsrf(proxyUrl, ssrfBridge); +} + /** * Resolves the raw target of a legacy request object, i.e. the value the axios * config carries as its `url`. diff --git a/packages/@n8n/backend-network/src/http/connection-guards.ts b/packages/@n8n/backend-network/src/http/connection-guards.ts new file mode 100644 index 00000000000..169634890bc --- /dev/null +++ b/packages/@n8n/backend-network/src/http/connection-guards.ts @@ -0,0 +1,85 @@ +import type { HttpsProxyAgent } from 'https-proxy-agent'; +import { UnexpectedError } from 'n8n-workflow'; +import type http from 'node:http'; +import type net from 'node:net'; + +import type { SsrfBridge } from '../ssrf'; + +/** Per-hop connect options of a proxy agent, as `agent-base` hands them to `connect()`. */ +type ProxyConnectOpts = Parameters['connect']>[1]; + +export class UnknownConnectionHostError extends UnexpectedError { + constructor() { + super('Cannot determine the host for this connection'); + } +} + +/** Subset of an agent's connection options we read to find the target host. */ +type ConnectionOptions = { host?: string | null; hostname?: string | null }; + +/** + * Runtime-only `createConnection` method of Node's http(s) agents. + */ +type CreateConnection = ( + options: ConnectionOptions, + onConnect?: (error: Error | null, stream?: unknown) => void, +) => unknown; + +interface ProxyConnectionAgent { + connectOpts: ConnectionOptions; + connect(req: http.ClientRequest, opts: ProxyConnectOpts): Promise; +} + +function connectionHostError(ssrf: SsrfBridge, options: ConnectionOptions): Error | undefined { + const host = options.host ?? options.hostname ?? undefined; + if (typeof host !== 'string' || host.length === 0) { + return new UnknownConnectionHostError(); + } + const result = ssrf.validateConnectionHost(host); + return result.ok ? undefined : result.error; +} + +/** + * Validates the proxy's own host before the socket to it opens. + * + * `agent-base` opens that socket inside `connect()`, from `connectOpts`. + * The agent's `createConnection` receives the target's options and never sees the proxy. + * This guard decides IP literals only. + * A hostname proxy relies on the secure lookup in the agent's options, which validates + * the address that hostname resolves to. + */ +export function installProxyConnectionGuard(agent: ProxyConnectionAgent, ssrf: SsrfBridge): void { + const connect = agent.connect.bind(agent); + agent.connect = async (req, opts) => { + const error = connectionHostError(ssrf, agent.connectOpts); + if (error) { + throw error; + } + return await connect(req, opts); + }; +} + +/** + * Wraps an agent's `createConnection` to validate the connection target before the socket opens. + * + * Node invokes the custom `lookup` to resolve hostnames only. + * An IP-literal target therefore reaches the socket unresolved, and needs this check + * at connection time. + */ +export function installConnectionGuard( + target: { createConnection: CreateConnection }, + ssrf: SsrfBridge, +): void { + const createConnection = target.createConnection.bind(target); + target.createConnection = (options, onConnect) => { + const error = connectionHostError(ssrf, options); + if (error) { + if (onConnect) { + onConnect(error); + return undefined; + } + throw error; + } + return createConnection(options, onConnect); + }; +} diff --git a/packages/@n8n/backend-network/src/http/env-proxy-http-agent.ts b/packages/@n8n/backend-network/src/http/env-proxy-http-agent.ts index 516cd5376c5..54b281ce231 100644 --- a/packages/@n8n/backend-network/src/http/env-proxy-http-agent.ts +++ b/packages/@n8n/backend-network/src/http/env-proxy-http-agent.ts @@ -13,8 +13,10 @@ type HttpProxyReqOpts = HttpAddRequestArgs[1]; /** * `http.Agent` that delegates per-request env-proxy routing and caching to a shared {@link EnvProxyRouter}. * - * The optional SSRF `lookup` is applied to the direct path only - * (behind a proxy it would resolve the proxy host, so the proxy validates the target). + * The optional SSRF `lookup` is applied to the direct path only. + * A proxy named by the environment belongs to the deployment rather than to a request. + * The policy that decides which targets a workflow may reach does not decide such a + * proxy (see `buildNodeAgents`). * * Also backs `installGlobalProxyAgent` (http-proxy.ts), keeping a single env-proxy agent implementation. */ diff --git a/packages/@n8n/backend-network/src/http/legacy-request.ts b/packages/@n8n/backend-network/src/http/legacy-request.ts index 971f64cee8c..36222b9f9db 100644 --- a/packages/@n8n/backend-network/src/http/legacy-request.ts +++ b/packages/@n8n/backend-network/src/http/legacy-request.ts @@ -15,7 +15,12 @@ import type { SsrfBridge } from '../ssrf'; import { invokeAxios } from './axios/invoke'; import { buildAxiosConfigFromLegacyRequest, buildLegacyAgentOptions } from './axios/legacy'; import { followSsrfRedirects, shouldFollowRedirectsManually } from './axios/redirect'; -import { resolveLegacyRequestUrl, throwIfDomainNotAllowed, validateUrlSsrf } from './axios/utils'; +import { + resolveLegacyRequestUrl, + throwIfDomainNotAllowed, + validateProxySsrf, + validateUrlSsrf, +} from './axios/utils'; import { binaryToString } from './binary-string'; import { parseIncomingMessage } from './parse-incoming-message'; @@ -56,6 +61,7 @@ export async function executeLegacyRequest( const url = resolveLegacyRequestUrl(requestObject); await validateUrlSsrf(url, ssrfBridge); + await validateProxySsrf(requestObject.proxy, ssrfBridge); axiosConfig = Object.assign( axiosConfig, diff --git a/packages/@n8n/backend-network/src/http/node-agents.ts b/packages/@n8n/backend-network/src/http/node-agents.ts index a0f48e8583e..7f799ea08b6 100644 --- a/packages/@n8n/backend-network/src/http/node-agents.ts +++ b/packages/@n8n/backend-network/src/http/node-agents.ts @@ -3,6 +3,7 @@ import http from 'node:http'; import https from 'node:https'; import type { LookupFunction } from 'node:net'; +import { installConnectionGuard, installProxyConnectionGuard } from './connection-guards'; import { EnvProxyHttpAgent } from './env-proxy-http-agent'; import { EnvProxyHttpsAgent } from './env-proxy-https-agent'; import { createProxiedHttpAgent, createProxiedHttpsAgent } from '../proxy/proxied-agents'; @@ -54,9 +55,25 @@ export type NodeAgentOptions = https.AgentOptions; * undici factory (`undici/factory.ts`), the axios transport layer * (`axios/utils.ts`) and the global proxy agents (`http-proxy.ts`). * - * SSRF lookup is injected only for **direct** connections. Behind a proxy the - * lookup resolves the proxy host, not the final target, so it is omitted there - * and the proxy validates the final target. + * The SSRF policy covers every host this builder opens a socket to, with one + * exception: a proxy named by the environment. + * `HTTP_PROXY` / `HTTPS_PROXY` describe the deployment rather than a request, and + * such a proxy commonly sits on a private address the policy blocks for targets. + * Setting them already requires control of the process. + * An explicit {@link ProxyUrl} can come from a request, so the policy decides its + * host like any other. + * + * The lookup resolves the target on a direct connection, and the proxy's own host + * behind an explicit proxy. + * Behind any proxy the final target never reaches these agents, so validating it + * belongs to the caller: + * - the axios entry points (`httpRequest`, `executeLegacyRequest`) check it before + * the request, and on each hop with the manual redirect follower + * - callers taking the agents on their own (`HttpTransport.getNodeAgent`) get + * neither check, and validate the targets they send through them + * + * A pre-request check does not pin an address to the socket, so a target the proxy + * resolves stays open to a rebind between the check and the connection. * * The `lookup` is owned by this builder: it is derived from the SSRF policy and * always overrides anything in `agentOptions`. Passing `agentOptions.lookup` @@ -101,24 +118,18 @@ export function buildNodeAgents( ); } - // Explicit proxy URL. No direct path, so no SSRF lookup is injected. - return { - httpAgent: createProxiedHttpAgent(proxy, agentOptions), - httpsAgent: createProxiedHttpsAgent(proxy, agentOptions), + // Explicit proxy URL. No direct path, so the lookup only guards the proxy host. + const agents = { + httpAgent: createProxiedHttpAgent(proxy, { ...agentOptions, lookup }), + httpsAgent: createProxiedHttpsAgent(proxy, { ...agentOptions, lookup }), }; + if (ssrf !== 'disabled') { + installProxyConnectionGuard(agents.httpAgent, ssrf); + installProxyConnectionGuard(agents.httpsAgent, ssrf); + } + return agents; } -/** Subset of an agent's connection options we read to find the target host. */ -type ConnectionOptions = { host?: string | null; hostname?: string | null }; - -/** - * Runtime-only `createConnection` method of Node's http(s) agents. - */ -type CreateConnection = ( - options: ConnectionOptions, - onConnect?: (error: Error | null, stream?: unknown) => void, -) => unknown; - /** * Installs {@link installConnectionGuard} on a direct-path agent pair when SSRF protection is active. */ @@ -132,28 +143,3 @@ function applyConnectionGuard( } return agents; } - -/** - * Wraps an agent's `createConnection` to validate the connection target before the socket opens. - * Node invokes the custom `lookup` only to resolve hostnames, so it also requires a check at connection time. - */ -export function installConnectionGuard( - target: { createConnection: CreateConnection }, - ssrf: SsrfBridge, -): void { - const createConnection = target.createConnection.bind(target); - target.createConnection = (options, onConnect) => { - const host = options.host ?? options.hostname ?? undefined; - if (typeof host === 'string') { - const result = ssrf.validateConnectionHost(host); - if (!result.ok) { - if (onConnect) { - onConnect(result.error); - return undefined; - } - throw result.error; - } - } - return createConnection(options, onConnect); - }; -} diff --git a/packages/@n8n/backend-network/src/http/outbound-http.ts b/packages/@n8n/backend-network/src/http/outbound-http.ts index 7f22b39ae73..38d1be809c8 100644 --- a/packages/@n8n/backend-network/src/http/outbound-http.ts +++ b/packages/@n8n/backend-network/src/http/outbound-http.ts @@ -139,8 +139,13 @@ export interface HttpRequestClient { * SSRF coverage is identical for `asCustomFetch()` and `getDispatcher()` (same * underlying dispatcher): every dispatched request — initial and each redirect * hop — is validated, and direct connections also carry a connect-time secure - * DNS lookup that defeats DNS-rebinding (TOCTOU). `getNodeAgent()` enforces the - * same connect-time secure lookup for direct connections. + * DNS lookup that defeats DNS-rebinding (TOCTOU). `getNodeAgent()` carries that + * same lookup, applied to whichever host its socket opens to: the target on a + * direct connection, the proxy behind an explicit one. + * It does **not** validate the targets sent through it. + * Behind any proxy the proxy resolves the final target, so no client-side + * connect-time check of that target is possible. + * Callers taking the agents validate those targets themselves (see `buildNodeAgents`). */ export interface HttpTransport { asCustomFetch(): CustomFetch; diff --git a/packages/@n8n/backend-network/src/http/undici/transport.ts b/packages/@n8n/backend-network/src/http/undici/transport.ts index 110b2f4f6b7..04890cfaa7d 100644 --- a/packages/@n8n/backend-network/src/http/undici/transport.ts +++ b/packages/@n8n/backend-network/src/http/undici/transport.ts @@ -108,11 +108,53 @@ function buildDispatcherFromProxy( return new Agent({ ...agentOptions, ...secureConnect(ssrf) }); } if (proxy === 'env') { + // The environment's proxies are part of the deployment, not of a request, + // so the policy does not decide them (see `buildNodeAgents`). return new EnvHttpProxyAgent({ ...agentOptions, ...secureConnect(ssrf) }); } - // Explicit proxy URL: no direct path, so no connect-time lookup is injected — - // the proxy resolves the target. Mirrors `buildNodeAgents`. - return new ProxyAgent({ uri: proxy, ...agentOptions }); + assertProxyHostAllowed(ssrf, proxy); + return new ProxyAgent({ + uri: proxy, + ...agentOptions, + ...secureProxyConnect(ssrf), + }); +} + +/** + * A connect-time secure DNS lookup for the socket opened to a proxy. + * + * undici builds the connector for the proxy socket from `proxyTls`. + * A caller's `connect` does not reach it: `ProxyAgent` overwrites that with its own + * tunnel handshake. + */ +function secureProxyConnect(ssrf: SsrfOption) { + return ssrf === 'disabled' ? {} : { proxyTls: { lookup: ssrf.createSecureLookup() } }; +} + +/** + * Catches an IP-literal proxy host, which no lookup sees. + * Applies to an explicit proxy URI, the one form that can come from a request. + */ +function assertProxyHostAllowed(ssrf: SsrfOption, proxyUri: string): void { + if (ssrf === 'disabled') { + return; + } + const hostname = proxyHostname(proxyUri); + if (hostname === undefined) { + return; + } + const result = ssrf.validateConnectionHost(hostname); + if (!result.ok) { + throw result.error; + } +} + +function proxyHostname(uri: string): string | undefined { + try { + return new URL(uri).hostname; + } catch { + return undefined; + } } /** @@ -174,8 +216,11 @@ function lazyValue(factory: () => T): () => T { * `fetch` re-dispatches through this dispatcher for each redirect hop, so this * validates the initial request **and** every redirect target (both hostname * and direct-IP targets), unlike a connect-time DNS lookup which never fires for - * IP-literal targets. Validation runs against the request target, never the - * proxy, so it is proxy-agnostic. + * IP-literal targets. + * + * This interceptor does not validate a proxy's own host. + * That host is fixed for the dispatcher rather than per request, so + * {@link buildDispatcherFromProxy} decides it where the dispatcher is built. */ export function createSsrfInterceptor(bridge: SsrfBridge): Dispatcher.DispatcherComposeInterceptor { return (dispatch) => (opts, handler) => { diff --git a/packages/@n8n/backend-network/src/ssrf/__tests__/mock-ssrf-bridge.ts b/packages/@n8n/backend-network/src/ssrf/__tests__/mock-ssrf-bridge.ts index 1667bd95ab5..154557128ff 100644 --- a/packages/@n8n/backend-network/src/ssrf/__tests__/mock-ssrf-bridge.ts +++ b/packages/@n8n/backend-network/src/ssrf/__tests__/mock-ssrf-bridge.ts @@ -1,5 +1,6 @@ +import type dns from 'node:dns'; import type { LookupFunction } from 'node:net'; -import { vi } from 'vitest'; +import { afterEach, beforeEach, vi, type Mock } from 'vitest'; import type { SsrfBridge } from '..'; @@ -7,6 +8,50 @@ export function makeLookupFn(): LookupFunction { return vi.fn() as unknown as LookupFunction; } +/** A `lookup` that fails every resolution with `error`. */ +export function makeDenyingLookup(error: Error): LookupFunction & Mock { + return vi.fn((_hostname: string, options: dns.LookupOptions, onResult: unknown) => { + (onResult as (error: Error | null, address?: unknown, family?: number) => void)( + error, + options.all ? [] : '', + undefined, + ); + }) as unknown as LookupFunction & Mock; +} + +const PROXY_ENV_KEYS = [ + 'HTTP_PROXY', + 'http_proxy', + 'HTTPS_PROXY', + 'https_proxy', + 'NO_PROXY', + 'no_proxy', + 'ALL_PROXY', + 'all_proxy', +] as const; + +/** Clears the proxy environment around each test and restores it afterwards. */ +export function useCleanProxyEnv(): void { + const savedEnv: Record = {}; + + beforeEach(() => { + for (const key of PROXY_ENV_KEYS) { + savedEnv[key] = process.env[key]; + delete process.env[key]; + } + }); + + afterEach(() => { + for (const key of PROXY_ENV_KEYS) { + if (savedEnv[key] === undefined) { + delete process.env[key]; + } else { + process.env[key] = savedEnv[key]; + } + } + }); +} + export function makeSsrfBridge(overrides?: Partial): SsrfBridge { return { validateUrl: vi.fn().mockResolvedValue({ ok: true, result: undefined }), diff --git a/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts b/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts index 2886952b22e..36df22e251b 100644 --- a/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts +++ b/packages/@n8n/expression-runtime/src/bridge/isolated-vm-bridge.ts @@ -277,7 +277,7 @@ export class IsolatedVmBridge implements RuntimeBridge { } /** - * Create an ivm.Reference callback for getting value/metadata at a path. + * Create an ivm.Callback for getting value/metadata at a path. * * Used by createDeepLazyProxy when accessing properties. Returns metadata * markers for arrays and objects, or the primitive value directly. @@ -291,8 +291,8 @@ export class IsolatedVmBridge implements RuntimeBridge { * @param data - Current workflow data to use for callback responses * @private */ - private createGetValueAtPathRef(data: WorkflowData): ivm.Reference { - return new (getIvm().Reference)((path: string[]) => { + private createGetValueAtPathRef(data: WorkflowData): ivm.Callback { + return new (getIvm().Callback)((path: string[]) => { try { // Navigate to value // Special-case: paths starting with ['$item', index] call data.$item(index) @@ -361,15 +361,15 @@ export class IsolatedVmBridge implements RuntimeBridge { } /** - * Create an ivm.Reference callback for getting array elements at an index. + * Create an ivm.Callback for getting array elements at an index. * * Used by array proxy when accessing numeric indices. * * @param data - Current workflow data to use for callback responses * @private */ - private createGetArrayElementRef(data: WorkflowData): ivm.Reference { - return new (getIvm().Reference)((path: string[], index: number) => { + private createGetArrayElementRef(data: WorkflowData): ivm.Callback { + return new (getIvm().Callback)((path: string[], index: number) => { try { // Navigate to array // Special-case: paths starting with ['$item', index] call data.$item(index) @@ -462,11 +462,17 @@ export class IsolatedVmBridge implements RuntimeBridge { * in this switch; the `type` field selects a static branch in source, * not a property lookup on a runtime object. * + * Return-value note: handlers must return plain, structured-clone-able + * data. Results cross into the isolate through an ivm.Callback, which copies + * them via the structured-clone algorithm — return JSON-shaped values, not + * isolated-vm objects (`Reference`/`ExternalCopy`) or other non-cloneable + * values. + * * @param data - Current workflow data * @private */ - private createCallHostRef(data: WorkflowData): ivm.Reference { - return new (getIvm().Reference)((rawMsg: unknown) => { + private createCallHostRef(data: WorkflowData): ivm.Callback { + return new (getIvm().Callback)((rawMsg: unknown) => { try { const msg = bridgeMessageSchema.parse(rawMsg); switch (msg.type) { @@ -695,7 +701,7 @@ export class IsolatedVmBridge implements RuntimeBridge { * Execute JavaScript code in the isolated context. * * Flow: - * 1. Create three ivm.Reference callbacks scoped to the current data: + * 1. Create three ivm.Callback instances scoped to the current data: * `getValueAtPath`, `getArrayElement`, `callHost`. * 2. Use evalClosureSync to run the code in a closure where `$0`/`$1`/`$2` * are the callback references — no global mutable state. @@ -715,6 +721,10 @@ export class IsolatedVmBridge implements RuntimeBridge { throw new Error('Bridge not initialized. Call initialize() first.'); } + // Host callbacks are ivm.Callback instances: inside the isolate they + // arrive as plain functions with structured-clone marshaling, so the + // runtime invokes them directly. Callbacks are GC-managed; there is no + // release() to call in `finally`. const getValueAtPath = this.createGetValueAtPathRef(data); const getArrayElement = this.createGetArrayElementRef(data); const callHost = this.createCallHostRef(data); @@ -796,10 +806,6 @@ try { ); } throw new Error(`Expression evaluation failed: ${errorMessage}`); - } finally { - getValueAtPath.release(); - getArrayElement.release(); - callHost.release(); } } diff --git a/packages/@n8n/expression-runtime/src/runtime/__tests__/context.test.ts b/packages/@n8n/expression-runtime/src/runtime/__tests__/context.test.ts index 465ba4f4dd5..6e028b0f6c6 100644 --- a/packages/@n8n/expression-runtime/src/runtime/__tests__/context.test.ts +++ b/packages/@n8n/expression-runtime/src/runtime/__tests__/context.test.ts @@ -1,9 +1,7 @@ import { buildContext } from '../context'; function makeRef(impl: (args: unknown[]) => unknown) { - return { - applySync: (_thisArg: unknown, args: unknown[]) => impl(args), - }; + return (...args: unknown[]) => impl(args); } describe('buildContext proxy', () => { diff --git a/packages/@n8n/expression-runtime/src/runtime/__tests__/lazy-proxy.test.ts b/packages/@n8n/expression-runtime/src/runtime/__tests__/lazy-proxy.test.ts index 82573d917ab..abefab80b0d 100644 --- a/packages/@n8n/expression-runtime/src/runtime/__tests__/lazy-proxy.test.ts +++ b/packages/@n8n/expression-runtime/src/runtime/__tests__/lazy-proxy.test.ts @@ -5,8 +5,6 @@ import { createDeepLazyProxy, isLazyProxy, getProxyPath } from '../lazy-proxy'; // Helpers // --------------------------------------------------------------------------- -const ivmCallOpts = { arguments: { copy: true }, result: { copy: true } }; - function mockApplySync(returnValue: unknown = undefined) { return vi.fn().mockReturnValue(returnValue); } @@ -22,8 +20,8 @@ function createMockCallbacks( const getArrayElement = overrides.getArrayElement ?? mockApplySync(); const callbacks = { - getValueAtPath: { applySync: getValueAtPath }, - getArrayElement: { applySync: getArrayElement }, + getValueAtPath, + getArrayElement, }; return { getValueAtPath, getArrayElement, callbacks }; @@ -231,7 +229,7 @@ describe('createDeepLazyProxy', () => { const p = proxy(['data']); mocks.getArrayElement.mockReturnValue('val'); p.list[3]; - expect(mocks.getArrayElement).toHaveBeenCalledWith(null, [['data', 'list'], 3], ivmCallOpts); + expect(mocks.getArrayElement).toHaveBeenCalledWith(['data', 'list'], 3); }); it('returns undefined for non-numeric non-length properties', () => { @@ -272,15 +270,15 @@ describe('createDeepLazyProxy', () => { // Each level triggers __getValueAtPath and creates a nested proxy // a -> returns object metadata const a = p.a; - expect(mocks.getValueAtPath).toHaveBeenLastCalledWith(null, [['a']], ivmCallOpts); + expect(mocks.getValueAtPath).toHaveBeenLastCalledWith(['a']); // a.b -> returns object metadata const b = a.b; - expect(mocks.getValueAtPath).toHaveBeenLastCalledWith(null, [['a', 'b']], ivmCallOpts); + expect(mocks.getValueAtPath).toHaveBeenLastCalledWith(['a', 'b']); // a.b.c -> returns object metadata const c = b.c; - expect(mocks.getValueAtPath).toHaveBeenLastCalledWith(null, [['a', 'b', 'c']], ivmCallOpts); + expect(mocks.getValueAtPath).toHaveBeenLastCalledWith(['a', 'b', 'c']); expect(getProxyPath(c)).toEqual(['a', 'b', 'c']); }); @@ -303,7 +301,7 @@ describe('createDeepLazyProxy', () => { mocks.getValueAtPath.mockReturnValue('val'); const p = proxy(['$json']); p.user; - expect(mocks.getValueAtPath).toHaveBeenCalledWith(null, [['$json', 'user']], ivmCallOpts); + expect(mocks.getValueAtPath).toHaveBeenCalledWith(['$json', 'user']); }); it('nested proxies inherit full path', () => { @@ -315,11 +313,7 @@ describe('createDeepLazyProxy', () => { // Accessing a property on the nested proxy should build the full path mocks.getValueAtPath.mockReturnValue('Alice'); user.name; - expect(mocks.getValueAtPath).toHaveBeenLastCalledWith( - null, - [['$json', 'user', 'name']], - ivmCallOpts, - ); + expect(mocks.getValueAtPath).toHaveBeenLastCalledWith(['$json', 'user', 'name']); }); }); @@ -369,7 +363,7 @@ describe('createDeepLazyProxy', () => { mocks.getValueAtPath.mockReturnValue('val'); const p = proxy(['$json']); 'foo' in p; - expect(mocks.getValueAtPath).toHaveBeenCalledWith(null, [['$json', 'foo']], ivmCallOpts); + expect(mocks.getValueAtPath).toHaveBeenCalledWith(['$json', 'foo']); }); }); @@ -461,7 +455,7 @@ describe('createDeepLazyProxy', () => { const p = arrayProxy(['arr'], 3); mocks.getArrayElement.mockReturnValue('first'); expect(p[0]).toBe('first'); - expect(mocks.getArrayElement).toHaveBeenCalledWith(null, [['arr'], 0], ivmCallOpts); + expect(mocks.getArrayElement).toHaveBeenCalledWith(['arr'], 0); }); it('nested array element returns an array-shaped child proxy', () => { diff --git a/packages/@n8n/expression-runtime/src/runtime/context.ts b/packages/@n8n/expression-runtime/src/runtime/context.ts index ba0429dfc72..0d374a1ddb2 100644 --- a/packages/@n8n/expression-runtime/src/runtime/context.ts +++ b/packages/@n8n/expression-runtime/src/runtime/context.ts @@ -52,30 +52,23 @@ type InputRpcType = (typeof INPUT_RPC_TYPES)[keyof typeof INPUT_RPC_TYPES]; // ============================================================================ /** - * The subset of `ivm.Reference` shape the in-isolate runtime relies on. - * Declared locally rather than importing from `isolated-vm` because this - * module is bundled into the isolate IIFE, where the native module is - * unavailable. The host wires real `ivm.Reference` instances which - * structurally satisfy this interface. + * A bridge callback as seen from inside the isolate: a plain function. + * The host wires these as `ivm.Callback` instances, which arrive in the + * isolate as ordinary functions whose arguments and return value are copied + * via the structured-clone algorithm. */ -interface BridgeCallback { - applySync( - thisArg: unknown, - args: unknown[], - options?: { arguments?: { copy?: boolean }; result?: { copy?: boolean } }, - ): unknown; -} +type BridgeCallback = (...args: unknown[]) => unknown; /** - * Bridge callbacks the in-isolate runtime can invoke synchronously via - * `ivm.Reference.applySync`. + * Bridge callbacks the in-isolate runtime invokes synchronously as plain + * functions (host-side `ivm.Callback` instances). * * - `getValueAtPath`, `getArrayElement`: data-access primitives used by the - * lazy-proxy system. Hot path; one ivm.Reference each for minimum overhead. + * lazy-proxy system. Hot path; one ivm.Callback each for minimum overhead. * - `callHost`: typed-RPC dispatcher. The in-isolate runtime constructs * an envelope (e.g. `{ type: 'getNodeFirst', nodeName, ... }`) and the * host-side dispatcher validates it with zod before routing to a handler. - * A single ivm.Reference covers every typed operation; new operations + * A single ivm.Callback covers every typed operation; new operations * are new schemas in `bridge/bridge-messages.ts` + new cases in the * dispatcher switch. The name reflects what this is: a synchronous * host RPC, not a postMessage-style async send. @@ -198,11 +191,7 @@ export function buildContext( const lazyProxy = createDeepLazyProxy(['$', nodeName], undefined, callbacks); const sendNodeMethod = (type: NodeRpcType) => { return (branchIndex?: number, runIndex?: number) => { - const result = callbacks.callHost.applySync( - null, - [{ type, nodeName, branchIndex, runIndex }], - { arguments: { copy: true }, result: { copy: true } }, - ); + const result = callbacks.callHost({ type, nodeName, branchIndex, runIndex }); throwIfErrorSentinel(result); return result; }; @@ -217,18 +206,12 @@ export function buildContext( type: 'getNodePairedItem' | 'getNodeItemMatching', itemIndex?: number, ) => { - const result = callbacks.callHost.applySync(null, [{ type, nodeName, itemIndex }], { - arguments: { copy: true }, - result: { copy: true }, - }); + const result = callbacks.callHost({ type, nodeName, itemIndex }); throwIfErrorSentinel(result); return result; }; const sendGetNodeItem = () => { - const result = callbacks.callHost.applySync(null, [{ type: 'getNodeItem', nodeName }], { - arguments: { copy: true }, - result: { copy: true }, - }); + const result = callbacks.callHost({ type: 'getNodeItem', nodeName }); throwIfErrorSentinel(result); return result; }; @@ -271,10 +254,7 @@ export function buildContext( const lazyInputProxy = createDeepLazyProxy(['$input'], undefined, callbacks); const sendInputMethod = (type: InputRpcType) => { return () => { - const result = callbacks.callHost.applySync(null, [{ type }], { - arguments: { copy: true }, - result: { copy: true }, - }); + const result = callbacks.callHost({ type }); throwIfErrorSentinel(result); return result; }; @@ -295,11 +275,7 @@ export function buildContext( // args, and the host's `WorkflowDataProxy.$items` applies its own // defaults when fields are undefined. target.$items = (nodeName?: string, outputIndex?: number, runIndex?: number) => { - const result = callbacks.callHost.applySync( - null, - [{ type: 'getItems', nodeName, outputIndex, runIndex }], - { arguments: { copy: true }, result: { copy: true } }, - ); + const result = callbacks.callHost({ type: 'getItems', nodeName, outputIndex, runIndex }); throwIfErrorSentinel(result); return result; }; @@ -316,11 +292,13 @@ export function buildContext( valueType?: string, defaultValue?: unknown, ) => { - const result = callbacks.callHost.applySync( - null, - [{ type: 'fromAi', name, description, valueType, defaultValue }], - { arguments: { copy: true }, result: { copy: true } }, - ); + const result = callbacks.callHost({ + type: 'fromAi', + name, + description, + valueType, + defaultValue, + }); throwIfErrorSentinel(result); return result; }; @@ -333,11 +311,7 @@ export function buildContext( // Under the VM engine this re-enters the bridge on a fresh evaluation; // the legacy engine handles it inline. target.$evaluateExpression = (expression: string, itemIndex?: number) => { - const result = callbacks.callHost.applySync( - null, - [{ type: 'evaluateExpression', expression, itemIndex }], - { arguments: { copy: true }, result: { copy: true } }, - ); + const result = callbacks.callHost({ type: 'evaluateExpression', expression, itemIndex }); throwIfErrorSentinel(result); return result; }; @@ -352,18 +326,12 @@ export function buildContext( incomingSourceData: unknown, initialPairedItem: unknown, ) => { - const result = callbacks.callHost.applySync( - null, - [ - { - type: 'getPairedItem', - destinationNodeName, - incomingSourceData, - initialPairedItem, - }, - ], - { arguments: { copy: true }, result: { copy: true } }, - ); + const result = callbacks.callHost({ + type: 'getPairedItem', + destinationNodeName, + incomingSourceData, + initialPairedItem, + }); throwIfErrorSentinel(result); return result; }; @@ -374,7 +342,7 @@ export function buildContext( // so each key is fetched at most once per evaluation. // ------------------------------------------------------------------------- - // Track keys we've already probed so we never call applySync twice + // Track keys we've already probed so we never call getValueAtPath twice // for the same key — even if the host returned undefined. const probedKeys = new Set(); @@ -383,10 +351,7 @@ export function buildContext( let value: unknown; try { - value = callbacks.getValueAtPath.applySync(null, [[key]], { - arguments: { copy: true }, - result: { copy: true }, - }); + value = callbacks.getValueAtPath([key]); } catch { // Don't mark as probed — the throw may be transient // (e.g. host data not yet available) and a retry should be allowed. diff --git a/packages/@n8n/expression-runtime/src/runtime/lazy-proxy.ts b/packages/@n8n/expression-runtime/src/runtime/lazy-proxy.ts index 1f5d9ab5bca..470d3ba89f3 100644 --- a/packages/@n8n/expression-runtime/src/runtime/lazy-proxy.ts +++ b/packages/@n8n/expression-runtime/src/runtime/lazy-proxy.ts @@ -106,7 +106,7 @@ export type ProxyMeta = { kind: 'object'; keys?: string[] } | { kind: 'array'; l * * @param basePath - Current path in object tree (e.g., ['$json', 'user']) * @param meta - Optional shape descriptor (object/array + known keys/length) - * @param callbacks - ivm.Reference callbacks for cross-isolate communication + * @param callbacks - ivm.Callback functions for cross-isolate communication * @returns Proxy object with lazy loading behavior */ export function createDeepLazyProxy( @@ -133,10 +133,7 @@ export function createDeepLazyProxy( function resolveObjectKeys(): string[] { if (objectKeys) return objectKeys; if (fetchedKeys) return fetchedKeys; - const value = getValueAtPath.applySync(null, [basePath], { - arguments: { copy: true }, - result: { copy: true }, - }); + const value = getValueAtPath(basePath); throwIfErrorSentinel(value); if (isObjectMetadata(value)) { fetchedKeys = value.__keys; @@ -252,10 +249,7 @@ export function createDeepLazyProxy( if (isArray) { const idx = isInArrayBounds(prop); if (idx === undefined) return undefined; - const element = getArrayElement.applySync(null, [basePath, idx], { - arguments: { copy: true }, - result: { copy: true }, - }); + const element = getArrayElement(basePath, idx); // Primitives (and null) skip `materializeChild`'s metadata checks. if (element === null || typeof element !== 'object') { targetObj[prop] = element; @@ -269,12 +263,8 @@ export function createDeepLazyProxy( // materializeChild rebuilds it only inside metadata branches. const path = [...basePath, prop]; - // Call back to host to get metadata/value - // Note: getValueAtPath is an ivm.Reference passed via callbacks - const value = getValueAtPath.applySync(null, [path], { - arguments: { copy: true }, - result: { copy: true }, - }); + // Call back to host to get metadata/value. + const value = getValueAtPath(path); targetObj[prop] = materializeChild(basePath, prop, value); return targetObj[prop]; @@ -301,10 +291,7 @@ export function createDeepLazyProxy( // Build path and check existence via callback const path = [...basePath, prop]; - const value = getValueAtPath.applySync(null, [path], { - arguments: { copy: true }, - result: { copy: true }, - }); + const value = getValueAtPath(path); // Handle errors serialized by host-side callbacks — reconstruct and throw // so the isolate's outer try-catch can serialize them back via __reportError diff --git a/packages/@n8n/expression-runtime/src/types/evaluator.ts b/packages/@n8n/expression-runtime/src/types/evaluator.ts index e9f5b350e76..0aa093d2c16 100644 --- a/packages/@n8n/expression-runtime/src/types/evaluator.ts +++ b/packages/@n8n/expression-runtime/src/types/evaluator.ts @@ -137,10 +137,10 @@ export interface NodeProxy { * type. * * Return types are `unknown` rather than `INodeExecutionData` / `[]`: - * results cross the isolate boundary via `applySync({ result: { copy: true } })`, - * which structured-clones the value and erases nominal types. The handlers - * pass the clone through verbatim, so a precise return type would be - * misleading. Matches the `NodeProxy` return type for the same reason. + * `ivm.Callback` structured-clones results as they pass between isolates, + * which erases nominal types. The handlers pass the clone through verbatim, + * so a precise return type would be misleading. Matches the `NodeProxy` + * return type for the same reason. */ export interface InputProxy { first?: () => unknown; diff --git a/packages/@n8n/tournament/src/Constants.ts b/packages/@n8n/tournament/src/Constants.ts index 935ba9f6eb9..d6fbae869cf 100644 --- a/packages/@n8n/tournament/src/Constants.ts +++ b/packages/@n8n/tournament/src/Constants.ts @@ -3,7 +3,10 @@ import type { ExpressionKind, PatternKind, PropertyKind, + SpreadElementKind, + SpreadPropertyKind, StatementKind, + SwitchCaseKind, VariableDeclaratorKind, } from 'ast-types/lib/gen/kinds'; @@ -23,4 +26,7 @@ export type ParentKind = | PropertyKind | PatternKind | VariableDeclaratorKind - | CatchClauseKind; + | CatchClauseKind + | SpreadElementKind + | SpreadPropertyKind + | SwitchCaseKind; diff --git a/packages/@n8n/tournament/src/VariablePolyfill.ts b/packages/@n8n/tournament/src/VariablePolyfill.ts index 35935480d54..e716ec9e446 100644 --- a/packages/@n8n/tournament/src/VariablePolyfill.ts +++ b/packages/@n8n/tournament/src/VariablePolyfill.ts @@ -80,6 +80,10 @@ const customPatches: Partial> = { } }, Property(path, parent: namedTypes.Property, dataNode) { + if (parent.computed && parent.key === path.node) { + polyfillVar(path, dataNode); + return; + } if (path.node !== parent.value) { return; } @@ -117,6 +121,36 @@ const customPatches: Partial> = { polyfillVar(path, dataNode); } }, + SpreadElement(path, parent: namedTypes.SpreadElement, dataNode) { + if (parent.argument === path.node) { + polyfillVar(path, dataNode); + } + }, + SpreadProperty(path, parent: namedTypes.SpreadProperty, dataNode) { + if (parent.argument === path.node) { + polyfillVar(path, dataNode); + } + }, + MethodDefinition(path, parent: namedTypes.MethodDefinition, dataNode) { + if (parent.computed && parent.key === path.node) { + polyfillVar(path, dataNode); + } + }, + SwitchCase(path, parent: namedTypes.SwitchCase, dataNode) { + if (parent.test === path.node) { + polyfillVar(path, dataNode); + } + }, + ClassDeclaration(path, parent: namedTypes.ClassDeclaration, dataNode) { + if (parent.superClass === path.node) { + polyfillVar(path, dataNode); + } + }, + ClassExpression(path, parent: namedTypes.ClassExpression, dataNode) { + if (parent.superClass === path.node) { + polyfillVar(path, dataNode); + } + }, }; export const jsVariablePolyfill = ( @@ -143,6 +177,12 @@ export const jsVariablePolyfill = ( case 'OptionalMemberExpression': case 'VariableDeclarator': case 'ArrowFunctionExpression': + case 'SpreadElement': + case 'SpreadProperty': + case 'MethodDefinition': + case 'SwitchCase': + case 'ClassDeclaration': + case 'ClassExpression': if (!customPatches[parent.type]) { throw new Error(`Couldn't find custom patcher for parent type: ${parent.type}`); } @@ -211,12 +251,9 @@ export const jsVariablePolyfill = ( case 'RestElement': case 'ArrayPattern': case 'ObjectPattern': - case 'ClassExpression': case 'RecordExpression': case 'V8IntrinsicIdentifier': case 'TopicReference': - case 'MethodDefinition': - case 'ClassDeclaration': case 'ClassProperty': case 'StaticBlock': case 'ClassBody': @@ -314,6 +351,7 @@ export const jsVariablePolyfill = ( // This is a simple type guard that guarantees we haven't missed // a case. It'll result in a type error at compile time. assertNever(parent); + polyfillVar(path, dataNode); break; } }, diff --git a/packages/@n8n/tournament/test/VariablePolyfill.test.ts b/packages/@n8n/tournament/test/VariablePolyfill.test.ts new file mode 100644 index 00000000000..c1367c7a2db --- /dev/null +++ b/packages/@n8n/tournament/test/VariablePolyfill.test.ts @@ -0,0 +1,107 @@ +import { Tournament } from '../src/index'; + +const evaluator = new Tournament((e) => { + throw e; +}); + +/** + * Every identifier that is a free read has to be resolved through the data + * context. One that is skipped resolves against the host scope instead, which + * is how `process` and friends become reachable from an expression. + */ +describe('jsVariablePolyfill', () => { + describe('free reads resolve through the data context', () => { + it.each([ + ['object spread', '{{ ({...value}).a }}'], + ['nested object spread', '{{ ({...({...value})}).a }}'], + ['computed object key', '{{ ({[key]: 1}).a }}'], + ['computed class field', '{{ (() => { class X { [key] = 1; } return new X().a; })() }}'], + [ + 'computed class method', + '{{ (() => { class X { [key]() { return 1; } } return new X().a(); })() }}', + ], + ['switch case', '{{ (() => { switch (1) { case one: return 1; } })() }}'], + ])('%s', (_, expression) => { + expect(evaluator.execute(expression, { value: { a: 1 }, key: 'a', one: 1 })).toBe(1); + }); + + it('array spread', () => { + expect(evaluator.execute('{{ [...value].length }}', { value: [1, 2, 3] })).toBe(3); + }); + + it('call argument spread', () => { + expect(evaluator.execute('{{ Math.max(...value) }}', { value: [1, 5, 3] })).toBe(5); + }); + + it('base class', () => { + class Base { + greet() { + return 'hello'; + } + } + + expect( + evaluator.execute('{{ (() => { class X extends Base {} return new X().greet(); })() }}', { + Base, + }), + ).toBe('hello'); + }); + }); + + describe('unresolved free reads do not fall through to the host scope', () => { + it.each([ + ['object spread', '{{ ({...process}) }}'], + ['nested object spread', '{{ ({...({...process})}) }}'], + ['spread inside a function', '{{ (() => ({...process}))() }}'], + ])('%s', (_, expression) => { + expect(evaluator.execute(expression, {})).toEqual({}); + }); + + it.each([ + ['array spread', '{{ [...process] }}'], + ['call argument spread', '{{ ((a) => a)(...process) }}'], + ])('%s', (_, expression) => { + expect(() => evaluator.execute(expression, {})).toThrow(/is not iterable/); + }); + + it('base class', () => { + expect(() => evaluator.execute('{{ (() => { class X extends Buffer {} })() }}', {})).toThrow( + /is not a constructor or null/, + ); + }); + + it('switch case', () => { + expect( + evaluator.execute( + '{{ (() => { switch (1) { case process: return "host"; } return "safe"; })() }}', + {}, + ), + ).toBe('safe'); + }); + + it.each([ + ['computed object key', '{{ Object.keys({[process]: 1})[0] }}'], + [ + 'computed class method key', + '{{ (() => { class X { [process]() {} } return Object.getOwnPropertyNames(X.prototype)[1]; })() }}', + ], + ])('%s', (_, expression) => { + expect(evaluator.execute(expression, { Object })).toBe('undefined'); + }); + }); + + describe('bindings are left alone', () => { + it.each([ + ['rest property', '{{ (() => { const {...rest} = value; return rest.a; })() }}'], + ['rest element', '{{ (() => { const [...rest] = [1]; return rest[0]; })() }}'], + ['rest parameter', '{{ ((...rest) => rest[0])(1) }}'], + [ + 'local shadowing a host global', + '{{ (() => { const process = value; return {...process}.a; })() }}', + ], + ['parameter shadowing a host global', '{{ ((process) => ({...process}).a)(value) }}'], + ])('%s', (_, expression) => { + expect(evaluator.execute(expression, { value: { a: 1 } })).toBe(1); + }); + }); +}); diff --git a/packages/cli/src/modules/insights/__tests__/insights.controller.test.ts b/packages/cli/src/modules/insights/__tests__/insights.controller.test.ts index 5fc58cab4bd..a5365661ab6 100644 --- a/packages/cli/src/modules/insights/__tests__/insights.controller.test.ts +++ b/packages/cli/src/modules/insights/__tests__/insights.controller.test.ts @@ -35,7 +35,7 @@ afterAll(async () => { describe('InsightsController', () => { const insightsByPeriodRepository = mockInstance(InsightsByPeriodRepository); - const workflowSharingService = mockInstance(WorkflowSharingService); + mockInstance(WorkflowSharingService); let controller: InsightsController; const sevenDaysAgo = DateTime.now().minus({ days: 7 }).toJSDate(); const today = DateTime.now().toJSDate(); @@ -365,13 +365,6 @@ describe('InsightsController', () => { }, ]; - beforeEach(() => { - // getSharedWorkflowIds returns all workflow IDs for the owner-like user - workflowSharingService.getSharedWorkflowIds.mockResolvedValue( - mockRows.map((row) => row.workflowId), - ); - }); - it('should return empty insights by workflow if no data', async () => { // ARRANGE insightsByPeriodRepository.getInsightsByWorkflow.mockResolvedValue({ count: 0, rows: [] }); diff --git a/packages/cli/src/modules/insights/__tests__/insights.service.integration.test.ts b/packages/cli/src/modules/insights/__tests__/insights.service.integration.test.ts index fede7bf063c..8bb6f03427a 100644 --- a/packages/cli/src/modules/insights/__tests__/insights.service.integration.test.ts +++ b/packages/cli/src/modules/insights/__tests__/insights.service.integration.test.ts @@ -2,12 +2,14 @@ import type { LicenseState } from '@n8n/backend-common'; import { createTeamProject, createWorkflow, + linkUserToProject, mockLogger, testDb, testModules, } from '@n8n/backend-test-utils'; import type { InstanceType } from '@n8n/constants'; import type { IWorkflowDb, Project, User, WorkflowEntity } from '@n8n/db'; +import { WorkflowRepository } from '@n8n/db'; import { Container } from '@n8n/di'; import { DateTime } from 'luxon'; import type { InstanceSettings } from 'n8n-core'; @@ -16,9 +18,12 @@ import type { MockInstance, Mocked } from 'vitest'; import type { MockProxy } from 'vitest-mock-extended'; import { mock } from 'vitest-mock-extended'; +import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; import type { WorkflowSharingService } from '@/workflows/workflow-sharing.service'; +import { createMember } from '@test-integration/db/users'; import { createCompactedInsightsEvent } from '../database/entities/__tests__/db-utils'; +import type { InsightsByPeriod } from '../database/entities/insights-by-period'; import type { InsightsByPeriodRepository } from '../database/repositories/insights-by-period.repository'; import { InsightsCollectionService } from '../insights-collection.service'; import type { InsightsCompactionService } from '../insights-compaction.service'; @@ -36,8 +41,11 @@ describe('InsightsService (Integration)', () => { 'InsightsRaw', 'InsightsByPeriod', 'InsightsMetadata', + 'SharedWorkflow', 'WorkflowEntity', + 'ProjectRelation', 'Project', + 'User', ]); }); @@ -144,6 +152,10 @@ describe('InsightsService (Integration)', () => { let project: Project; let workflow: IWorkflowDb & WorkflowEntity; + const globalWorkflowReadUser = { + role: { scopes: [{ slug: 'workflow:read' }] }, + } as unknown as User; + beforeEach(async () => { project = await createTeamProject(); workflow = await createWorkflow({}, project); @@ -198,6 +210,7 @@ describe('InsightsService (Integration)', () => { // ACT const summary = await insightsService.getInsightsSummary({ + user: globalWorkflowReadUser, startDate: startDate.toJSDate(), endDate: endDate.toJSDate(), }); @@ -227,6 +240,7 @@ describe('InsightsService (Integration)', () => { // ACT const summary = await insightsService.getInsightsSummary({ + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), }); @@ -294,6 +308,7 @@ describe('InsightsService (Integration)', () => { // ACT const summary = await insightsService.getInsightsSummary({ + user: globalWorkflowReadUser, startDate: startDate.toJSDate(), endDate: endDate.toJSDate(), projectId: project.id, @@ -308,13 +323,157 @@ describe('InsightsService (Integration)', () => { total: { value: 10, unit: 'count', deviation: -5 }, }); }); + + describe('project scoping', () => { + let member: User; + let otherProject: Project; + let startDate: Date; + let endDate: Date; + + let workflowInsights: InsightsByPeriod; + let otherWorkflowInsights: InsightsByPeriod; + + beforeEach(async () => { + member = await createMember(); + otherProject = await createTeamProject(); + const otherWorkflow = await createWorkflow({}, otherProject); + + const now = DateTime.utc(); + startDate = now.minus({ days: 6 }).toJSDate(); + endDate = now.toJSDate(); + + // 4 successes in `project`, 10 in `otherProject` + [workflowInsights, otherWorkflowInsights] = await Promise.all([ + createCompactedInsightsEvent(workflow, { + type: 'success', + value: 4, + periodUnit: 'day', + periodStart: now.minus({ days: 1 }), + }), + createCompactedInsightsEvent(otherWorkflow, { + type: 'success', + value: 10, + periodUnit: 'day', + periodStart: now.minus({ days: 1 }), + }), + ]); + }); + + test('should aggregate only accessible projects when no project is requested', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + const summary = await insightsService.getInsightsSummary({ + user: member, + startDate, + endDate, + }); + + expect(summary.total.value).toBe(workflowInsights.value); + }); + + test('should return no results for a user with no accessible projects', async () => { + const summary = await insightsService.getInsightsSummary({ + user: member, + startDate, + endDate, + }); + + expect(summary.total.value).toBe(0); + expect(summary.failed.value).toBe(0); + }); + + test('should aggregate all projects for users with the global workflow read scope', async () => { + const summary = await insightsService.getInsightsSummary({ + user: globalWorkflowReadUser, + startDate, + endDate, + }); + + expect(summary.total.value).toBe(workflowInsights.value + otherWorkflowInsights.value); + }); + + test('should aggregate the requested project when it is accessible', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + const summary = await insightsService.getInsightsSummary({ + user: member, + startDate, + endDate, + projectId: project.id, + }); + + expect(summary.total.value).toBe(workflowInsights.value); + }); + + test('should throw a forbidden error when the requested project is not accessible', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + await expect( + insightsService.getInsightsSummary({ + user: member, + startDate, + endDate, + projectId: otherProject.id, + }), + ).rejects.toThrow(ForbiddenError); + }); + + test('should throw a forbidden error when the requested project does not exist', async () => { + await expect( + insightsService.getInsightsSummary({ + user: member, + startDate, + endDate, + projectId: 'non-existing-project-id', + }), + ).rejects.toThrow(ForbiddenError); + }); + + test('should retain history from deleted workflows for users with the global workflow read scope', async () => { + // Deleting a workflow nulls the insights metadata FK but keeps the row + await Container.get(WorkflowRepository).delete({ id: workflow.id }); + + const summary = await insightsService.getInsightsSummary({ + user: globalWorkflowReadUser, + startDate, + endDate, + }); + + expect(summary.total.value).toBe(workflowInsights.value + otherWorkflowInsights.value); + }); + + test('should exclude deleted workflow history when scoped to the requested project', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + const deletedWorkflow = await createWorkflow({}, project); + await createCompactedInsightsEvent(deletedWorkflow, { + type: 'success', + value: 5, + periodUnit: 'day', + periodStart: DateTime.utc().minus({ days: 1 }), + }); + + await Container.get(WorkflowRepository).delete({ id: deletedWorkflow.id }); + + const summary = await insightsService.getInsightsSummary({ + user: member, + startDate, + endDate, + projectId: project.id, + }); + + // Only the live workflow's 4 successes; the deleted workflows are excluded + expect(summary.total.value).toBe(workflowInsights.value); + }); + }); }); describe('getInsightsByWorkflow', () => { let insightsService: InsightsService; - // Owner-like user with the global `workflow:read` scope, so every workflow is accessible - const owner = { role: { scopes: [{ slug: 'workflow:read' }] } } as unknown as User; + const globalWorkflowReadUser = { + role: { scopes: [{ slug: 'workflow:read' }] }, + } as unknown as User; beforeAll(() => { insightsService = Container.get(InsightsService); @@ -415,7 +574,7 @@ describe('InsightsService (Integration)', () => { // ACT const byWorkflow = await insightsService.getInsightsByWorkflow({ - user: owner, + user: globalWorkflowReadUser, startDate, endDate, }); @@ -482,7 +641,7 @@ describe('InsightsService (Integration)', () => { // ACT const byWorkflow = await insightsService.getInsightsByWorkflow({ - user: owner, + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), sortBy: 'runTime:desc', @@ -510,7 +669,7 @@ describe('InsightsService (Integration)', () => { // ACT const byWorkflow = await insightsService.getInsightsByWorkflow({ - user: owner, + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), sortBy: 'succeeded:desc', @@ -538,7 +697,7 @@ describe('InsightsService (Integration)', () => { const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); const byWorkflow = await insightsService.getInsightsByWorkflow({ - user: owner, + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), skip: 10, @@ -608,7 +767,7 @@ describe('InsightsService (Integration)', () => { // ACT const byWorkflow = await insightsService.getInsightsByWorkflow({ - user: owner, + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), projectId: project.id, @@ -655,7 +814,7 @@ describe('InsightsService (Integration)', () => { // ACT const byWorkflow = await insightsService.getInsightsByWorkflow({ - user: owner, + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), }); @@ -664,6 +823,148 @@ describe('InsightsService (Integration)', () => { expect(byWorkflow.count).toEqual(0); expect(byWorkflow.data).toHaveLength(0); }); + + describe('project scoping', () => { + let member: User; + let startDate: Date; + let endDate: Date; + + beforeEach(async () => { + member = await createMember(); + + const now = DateTime.utc(); + startDate = now.minus({ days: 6 }).toJSDate(); + endDate = now.toJSDate(); + + // workflow1 (in `project`) is accessible; workflow4 (in `project2`) is not + await createCompactedInsightsEvent(workflow1, { + type: 'success', + value: 4, + periodUnit: 'day', + periodStart: now.minus({ days: 1 }), + }); + await createCompactedInsightsEvent(workflow4, { + type: 'success', + value: 10, + periodUnit: 'day', + periodStart: now.minus({ days: 1 }), + }); + }); + + test('should list only workflows in projects the caller can read when no project is requested', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + const byWorkflow = await insightsService.getInsightsByWorkflow({ + user: member, + startDate, + endDate, + }); + + expect(byWorkflow.count).toBe(1); + expect(byWorkflow.data.map((row) => row.workflowId)).toEqual([workflow1.id]); + }); + + test('should list no workflows for a user with no accessible projects', async () => { + const byWorkflow = await insightsService.getInsightsByWorkflow({ + user: member, + startDate, + endDate, + }); + + expect(byWorkflow.count).toBe(0); + expect(byWorkflow.data).toEqual([]); + }); + + test('should list workflows for the requested project when it is accessible', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + const byWorkflow = await insightsService.getInsightsByWorkflow({ + user: member, + startDate, + endDate, + projectId: project.id, + }); + + expect(byWorkflow.count).toBe(1); + expect(byWorkflow.data.map((row) => row.workflowId)).toEqual([workflow1.id]); + }); + + test('should throw a forbidden error when the requested project is not accessible', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + await expect( + insightsService.getInsightsByWorkflow({ + user: member, + startDate, + endDate, + projectId: project2.id, + }), + ).rejects.toThrow(ForbiddenError); + }); + + test('should throw a forbidden error when the requested project does not exist', async () => { + await expect( + insightsService.getInsightsByWorkflow({ + user: member, + startDate, + endDate, + projectId: 'non-existing-project-id', + }), + ).rejects.toThrow(ForbiddenError); + }); + + test('should report a count consistent with the returned rows when results are scoped', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + // A second accessible workflow, so pagination has something to page through + await createCompactedInsightsEvent(workflow2, { + type: 'success', + value: 1, + periodUnit: 'day', + periodStart: DateTime.utc().minus({ days: 1 }), + }); + + const byWorkflow = await insightsService.getInsightsByWorkflow({ + user: member, + startDate, + endDate, + take: 1, + }); + + // The inaccessible workflow4 must not inflate the count + expect(byWorkflow.count).toBe(2); + expect(byWorkflow.data).toHaveLength(1); + }); + + test('should retain a row for a deleted workflow when the caller has the global workflow read scope', async () => { + await Container.get(WorkflowRepository).delete({ id: workflow4.id }); + + const byWorkflow = await insightsService.getInsightsByWorkflow({ + user: globalWorkflowReadUser, + startDate, + endDate, + }); + + expect(byWorkflow.data.find((row) => row.workflowId === null)).toMatchObject({ + total: 10, + hasReadAccess: false, + }); + }); + + test('should exclude a row for a deleted workflow when the caller is scoped to its project', async () => { + await linkUserToProject(member, project2, 'project:viewer'); + await Container.get(WorkflowRepository).delete({ id: workflow4.id }); + + const byWorkflow = await insightsService.getInsightsByWorkflow({ + user: member, + startDate, + endDate, + projectId: project2.id, + }); + + expect(byWorkflow.data).toHaveLength(0); + }); + }); }); describe('getInsightsByTime', () => { @@ -672,6 +973,10 @@ describe('InsightsService (Integration)', () => { insightsService = Container.get(InsightsService); }); + const globalWorkflowReadUser = { + role: { scopes: [{ slug: 'workflow:read' }] }, + } as unknown as User; + let project: Project; let otherProject: Project; let workflow1: IWorkflowDb & WorkflowEntity; @@ -691,6 +996,7 @@ describe('InsightsService (Integration)', () => { const now = DateTime.utc(); const startDate = now.minus({ days: 14 }).toJSDate(); const byTime = await insightsService.getInsightsByTime({ + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), }); @@ -709,6 +1015,7 @@ describe('InsightsService (Integration)', () => { const startDate = now.minus({ days: 14 }).startOf('day').toJSDate(); const byTime = await insightsService.getInsightsByTime({ + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), }); @@ -773,6 +1080,7 @@ describe('InsightsService (Integration)', () => { // ACT const byTime = await insightsService.getInsightsByTime({ + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), }); @@ -851,6 +1159,7 @@ describe('InsightsService (Integration)', () => { // ACT const byTime = await insightsService.getInsightsByTime({ + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), insightTypes: ['time_saved_min', 'failure'], @@ -931,6 +1240,7 @@ describe('InsightsService (Integration)', () => { // ACT const byTime = await insightsService.getInsightsByTime({ + user: globalWorkflowReadUser, startDate, endDate: now.toJSDate(), projectId: project.id, @@ -985,6 +1295,141 @@ describe('InsightsService (Integration)', () => { ]), ); }); + + describe('project scoping', () => { + let member: User; + let startDate: Date; + let endDate: Date; + + let workflowInsights: InsightsByPeriod; + let otherWorkflowInsights: InsightsByPeriod; + + beforeEach(async () => { + member = await createMember(); + + const now = DateTime.utc(); + startDate = now.minus({ days: 6 }).toJSDate(); + endDate = now.toJSDate(); + + // 4 successes in `project`, 10 in `otherProject` + [workflowInsights, otherWorkflowInsights] = await Promise.all([ + createCompactedInsightsEvent(workflow1, { + type: 'success', + value: 4, + periodUnit: 'day', + periodStart: now.minus({ days: 1 }), + }), + createCompactedInsightsEvent(workflow3, { + type: 'success', + value: 10, + periodUnit: 'day', + periodStart: now.minus({ days: 1 }), + }), + ]); + }); + + test('should aggregate only accessible projects when no project is requested', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + const byTime = await insightsService.getInsightsByTime({ + user: member, + startDate, + endDate, + }); + + expect(byTime).toHaveLength(1); + expect(byTime[0].values.succeeded).toBe(workflowInsights.value); + }); + + test('should return no results for a user with no accessible projects', async () => { + const byTime = await insightsService.getInsightsByTime({ + user: member, + startDate, + endDate, + }); + + expect(byTime).toHaveLength(0); + }); + + test('should aggregate the requested project when it is accessible', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + const byTime = await insightsService.getInsightsByTime({ + user: member, + startDate, + endDate, + projectId: project.id, + }); + + expect(byTime).toHaveLength(1); + expect(byTime[0].values.succeeded).toBe(workflowInsights.value); + }); + + test('should throw a forbidden error when the requested project is not accessible', async () => { + await linkUserToProject(member, project, 'project:viewer'); + + await expect( + insightsService.getInsightsByTime({ + user: member, + startDate, + endDate, + projectId: otherProject.id, + }), + ).rejects.toThrow(ForbiddenError); + }); + + test('should throw a forbidden error when the requested project does not exist', async () => { + await expect( + insightsService.getInsightsByTime({ + user: member, + startDate, + endDate, + projectId: 'non-existing-project-id', + }), + ).rejects.toThrow(ForbiddenError); + }); + + test('should aggregate all projects for users with the global workflow read scope', async () => { + const byTime = await insightsService.getInsightsByTime({ + user: globalWorkflowReadUser, + startDate, + endDate, + }); + + expect(byTime).toHaveLength(1); + expect(byTime[0].values.succeeded).toBe( + workflowInsights.value + otherWorkflowInsights.value, + ); + }); + + test('should retain deleted workflow history for users with the global workflow read scope', async () => { + await Container.get(WorkflowRepository).delete({ id: workflow3.id }); + + const byTime = await insightsService.getInsightsByTime({ + user: globalWorkflowReadUser, + startDate, + endDate, + }); + + expect(byTime[0].values.succeeded).toBe( + workflowInsights.value + otherWorkflowInsights.value, + ); + }); + + test('should exclude deleted workflow history when scoped to the requested project', async () => { + await linkUserToProject(member, otherProject, 'project:viewer'); + await Container.get(WorkflowRepository).delete({ id: workflow3.id }); + + const byTime = await insightsService.getInsightsByTime({ + user: member, + startDate, + endDate, + projectId: otherProject.id, + }); + + expect(byTime).toHaveLength(0); + }); + }); }); describe('validateDateFiltersLicense', () => { diff --git a/packages/cli/src/modules/insights/__tests__/insights.service.test.ts b/packages/cli/src/modules/insights/__tests__/insights.service.test.ts index f453d0b1c00..d8b9258b385 100644 --- a/packages/cli/src/modules/insights/__tests__/insights.service.test.ts +++ b/packages/cli/src/modules/insights/__tests__/insights.service.test.ts @@ -5,6 +5,7 @@ import type { InstanceSettings } from 'n8n-core'; import type { MockProxy } from 'vitest-mock-extended'; import { mock } from 'vitest-mock-extended'; +import { userHasScopes } from '@/permissions.ee/check-access'; import type { WorkflowSharingService } from '@/workflows/workflow-sharing.service'; import { TypeToNumber } from '../database/entities/insights-shared'; @@ -13,6 +14,12 @@ import type { InsightsCompactionService } from '../insights-compaction.service'; import type { InsightsPruningService } from '../insights-pruning.service'; import { InsightsService } from '../insights.service'; +vi.mock('@/permissions.ee/check-access', () => ({ + userHasScopes: vi.fn(), +})); + +const user = mock({ id: 'user-1' }); + describe('InsightsService', () => { let insightsService: InsightsService; @@ -32,6 +39,7 @@ describe('InsightsService', () => { mockLicenseState = mock(); mockInstanceSettings = mock(); mockWorkflowSharingService = mock(); + vi.mocked(userHasScopes).mockResolvedValue(true); insightsService = new InsightsService( mockInsightsByPeriodRepository, @@ -61,6 +69,7 @@ describe('InsightsService', () => { ]); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -194,6 +203,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -216,6 +226,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -238,6 +249,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -259,6 +271,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -283,6 +296,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -303,6 +317,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -324,6 +339,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -348,6 +364,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -374,6 +391,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -396,6 +414,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -420,6 +439,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -442,6 +462,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -464,6 +485,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -483,6 +505,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -505,6 +528,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -527,6 +551,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -550,6 +575,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -572,6 +598,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -594,6 +621,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -616,6 +644,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -639,6 +668,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -661,6 +691,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -683,6 +714,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -705,6 +737,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -728,6 +761,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -750,6 +784,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -771,6 +806,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -793,6 +829,7 @@ describe('InsightsService', () => { ); const result = await insightsService.getInsightsSummary({ + user, startDate, endDate, }); @@ -802,6 +839,79 @@ describe('InsightsService', () => { }); }); }); + + describe('project access', () => { + beforeEach(() => { + mockInsightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates.mockResolvedValue( + [], + ); + }); + + it('should not check project access when no project is requested', async () => { + await insightsService.getInsightsSummary({ user, startDate, endDate }); + + expect(userHasScopes).not.toHaveBeenCalled(); + }); + + it('should throw a forbidden error when the requested project is not accessible', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + + await expect( + insightsService.getInsightsSummary({ user, startDate, endDate, projectId: 'project-1' }), + ).rejects.toThrow('You do not have access to insights for this project.'); + + expect( + mockInsightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates, + ).not.toHaveBeenCalled(); + }); + + it('should query the requested project when it is accessible', async () => { + await insightsService.getInsightsSummary({ + user, + startDate, + endDate, + projectId: 'project-1', + }); + + expect(userHasScopes).toHaveBeenCalledWith(user, ['workflow:read'], false, { + projectId: 'project-1', + }); + expect( + mockInsightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates, + ).toHaveBeenCalledWith(expect.objectContaining({ projectId: 'project-1' })); + }); + + it('should query without an access filter for users with the global workflow read scope', async () => { + mockWorkflowSharingService.rolesGrantingScope.mockResolvedValue(undefined); + + await insightsService.getInsightsSummary({ user, startDate, endDate }); + + expect( + mockInsightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates, + ).toHaveBeenCalledWith(expect.objectContaining({ accessFilter: undefined })); + }); + + it('should query with an access filter for users without the global workflow read scope', async () => { + mockWorkflowSharingService.rolesGrantingScope.mockResolvedValue({ + projectRoles: ['project:viewer'], + workflowRoles: ['workflow:owner'], + }); + + await insightsService.getInsightsSummary({ user, startDate, endDate }); + + expect( + mockInsightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates, + ).toHaveBeenCalledWith( + expect.objectContaining({ + accessFilter: { + user, + projectRoles: ['project:viewer'], + workflowRoles: ['workflow:owner'], + }, + }), + ); + }); + }); }); describe('timeZone forwarding', () => { @@ -813,7 +923,12 @@ describe('InsightsService', () => { [], ); - await insightsService.getInsightsSummary({ startDate, endDate, timeZone: 'Europe/Berlin' }); + await insightsService.getInsightsSummary({ + user, + startDate, + endDate, + timeZone: 'Europe/Berlin', + }); expect( mockInsightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates, @@ -842,7 +957,12 @@ describe('InsightsService', () => { it('forwards timeZone to getInsightsByTime', async () => { mockInsightsByPeriodRepository.getInsightsByTime.mockResolvedValue([]); - await insightsService.getInsightsByTime({ startDate, endDate, timeZone: 'Europe/Berlin' }); + await insightsService.getInsightsByTime({ + user, + startDate, + endDate, + timeZone: 'Europe/Berlin', + }); expect(mockInsightsByPeriodRepository.getInsightsByTime).toHaveBeenCalledWith( expect.objectContaining({ timeZone: 'Europe/Berlin' }), @@ -871,55 +991,148 @@ describe('InsightsService', () => { const makeUser = (scopes: string[]) => ({ role: { scopes: scopes.map((slug) => ({ slug })) } }) as unknown as User; - it('sets hasReadAccess only for workflows in the shared set for non-admins', async () => { + beforeEach(() => { + mockInsightsByPeriodRepository.getInsightsByWorkflow.mockResolvedValue({ + count: 0, + rows: [], + }); + }); + + it('sets hasReadAccess true for a row with a workflowId', async () => { const user = makeUser([]); mockInsightsByPeriodRepository.getInsightsByWorkflow.mockResolvedValue({ - count: 2, - rows: [makeRow('wf-accessible'), makeRow('wf-inaccessible')], + count: 1, + rows: [makeRow('wf-1')], }); - mockWorkflowSharingService.getSharedWorkflowIds.mockResolvedValue(['wf-accessible']); const result = await insightsService.getInsightsByWorkflow({ user, startDate, endDate }); - expect(mockWorkflowSharingService.getSharedWorkflowIds).toHaveBeenCalledWith(user, { - scopes: ['workflow:read'], - projectId: undefined, - }); - expect(result.data.find((r) => r.workflowId === 'wf-accessible')?.hasReadAccess).toBe(true); - expect(result.data.find((r) => r.workflowId === 'wf-inaccessible')?.hasReadAccess).toBe( - false, - ); + expect(result.data[0].hasReadAccess).toBe(true); }); - it('sets hasReadAccess true for all rows for admins', async () => { + it('sets hasReadAccess false for a row with a null workflowId (deleted workflow)', async () => { const user = makeUser(['workflow:read']); - mockInsightsByPeriodRepository.getInsightsByWorkflow.mockResolvedValue({ - count: 2, - rows: [makeRow('wf-1'), makeRow('wf-2')], - }); - // getSharedWorkflowIds returns all workflow IDs for users with the global scope - mockWorkflowSharingService.getSharedWorkflowIds.mockResolvedValue(['wf-1', 'wf-2']); - - const result = await insightsService.getInsightsByWorkflow({ user, startDate, endDate }); - - expect(mockWorkflowSharingService.getSharedWorkflowIds).toHaveBeenCalledWith(user, { - scopes: ['workflow:read'], - projectId: undefined, - }); - expect(result.data.every((r) => r.hasReadAccess)).toBe(true); - }); - - it('sets hasReadAccess false for rows with a null workflowId (deleted workflow)', async () => { - const user = makeUser([]); mockInsightsByPeriodRepository.getInsightsByWorkflow.mockResolvedValue({ count: 1, rows: [makeRow(null)], }); - mockWorkflowSharingService.getSharedWorkflowIds.mockResolvedValue([]); const result = await insightsService.getInsightsByWorkflow({ user, startDate, endDate }); expect(result.data[0].hasReadAccess).toBe(false); }); + + describe('project access', () => { + it('should not check project access when no project is requested', async () => { + await insightsService.getInsightsByWorkflow({ user, startDate, endDate }); + + expect(userHasScopes).not.toHaveBeenCalled(); + }); + + it('should throw a forbidden error when the requested project is not accessible', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + + await expect( + insightsService.getInsightsByWorkflow({ + user, + startDate, + endDate, + projectId: 'project-1', + }), + ).rejects.toThrow('You do not have access to insights for this project.'); + + expect(mockInsightsByPeriodRepository.getInsightsByWorkflow).not.toHaveBeenCalled(); + }); + + it('should query without an access filter for users with the global workflow read scope', async () => { + mockWorkflowSharingService.rolesGrantingScope.mockResolvedValue(undefined); + + await insightsService.getInsightsByWorkflow({ user, startDate, endDate }); + + expect(mockInsightsByPeriodRepository.getInsightsByWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ accessFilter: undefined }), + ); + }); + + it('should query with an access filter for users without the global workflow read scope', async () => { + mockWorkflowSharingService.rolesGrantingScope.mockResolvedValue({ + projectRoles: ['project:viewer'], + workflowRoles: ['workflow:owner'], + }); + + await insightsService.getInsightsByWorkflow({ user, startDate, endDate }); + + expect(mockInsightsByPeriodRepository.getInsightsByWorkflow).toHaveBeenCalledWith( + expect.objectContaining({ + accessFilter: { + user, + projectRoles: ['project:viewer'], + workflowRoles: ['workflow:owner'], + }, + }), + ); + }); + }); + }); + + describe('getInsightsByTime', () => { + const startDate = new Date('2024-01-01'); + const endDate = new Date('2024-01-07'); + + beforeEach(() => { + mockInsightsByPeriodRepository.getInsightsByTime.mockResolvedValue([]); + }); + + describe('project access', () => { + it('should not check project access when no project is requested', async () => { + await insightsService.getInsightsByTime({ user, startDate, endDate }); + + expect(userHasScopes).not.toHaveBeenCalled(); + }); + + it('should throw a forbidden error when the requested project is not accessible', async () => { + vi.mocked(userHasScopes).mockResolvedValue(false); + + await expect( + insightsService.getInsightsByTime({ + user, + startDate, + endDate, + projectId: 'project-1', + }), + ).rejects.toThrow('You do not have access to insights for this project.'); + + expect(mockInsightsByPeriodRepository.getInsightsByTime).not.toHaveBeenCalled(); + }); + + it('should query without an access filter for users with the global workflow read scope', async () => { + mockWorkflowSharingService.rolesGrantingScope.mockResolvedValue(undefined); + + await insightsService.getInsightsByTime({ user, startDate, endDate }); + + expect(mockInsightsByPeriodRepository.getInsightsByTime).toHaveBeenCalledWith( + expect.objectContaining({ accessFilter: undefined }), + ); + }); + + it('should query with an access filter for users without the global workflow read scope', async () => { + mockWorkflowSharingService.rolesGrantingScope.mockResolvedValue({ + projectRoles: ['project:viewer'], + workflowRoles: ['workflow:owner'], + }); + + await insightsService.getInsightsByTime({ user, startDate, endDate }); + + expect(mockInsightsByPeriodRepository.getInsightsByTime).toHaveBeenCalledWith( + expect.objectContaining({ + accessFilter: { + user, + projectRoles: ['project:viewer'], + workflowRoles: ['workflow:owner'], + }, + }), + ); + }); + }); }); }); diff --git a/packages/cli/src/modules/insights/database/repositories/__tests__/insights-by-period.repository.integration.test.ts b/packages/cli/src/modules/insights/database/repositories/__tests__/insights-by-period.repository.integration.test.ts index 5279693fb92..8dc628f266e 100644 --- a/packages/cli/src/modules/insights/database/repositories/__tests__/insights-by-period.repository.integration.test.ts +++ b/packages/cli/src/modules/insights/database/repositories/__tests__/insights-by-period.repository.integration.test.ts @@ -1,11 +1,22 @@ -import { createTeamProject, createWorkflow, testDb, testModules } from '@n8n/backend-test-utils'; import { GlobalConfig } from '@n8n/config'; +import { + createTeamProject, + createWorkflow, + linkUserToProject, + testDb, + testModules, +} from '@n8n/backend-test-utils'; +import type { Project, User, WorkflowEntity } from '@n8n/db'; import { Container } from '@n8n/di'; import { DateTime } from 'luxon'; import { InsightsConfig } from '@/modules/insights/insights.config'; +import { createMember } from '@test-integration/db/users'; import { createCompactedInsightsEvent, createMetadata } from '../../entities/__tests__/db-utils'; +import type { InsightsByPeriod } from '../../entities/insights-by-period'; +import { TypeToNumber } from '../../entities/insights-shared'; +import type { InsightsAccessFilter } from '../insights-by-period.repository'; import { InsightsByPeriodRepository } from '../insights-by-period.repository'; const isPostgres = Container.get(GlobalConfig).database.type === 'postgresdb'; @@ -268,4 +279,152 @@ describe('InsightsByPeriodRepository', () => { expect(transactionSpy).toHaveBeenCalledTimes(1); }); }); + + describe('access filter', () => { + let member: User; + let accessibleProject: Project; + let accessibleWorkflow: WorkflowEntity; + let accessibleInsight: InsightsByPeriod; + let inaccessibleProject: Project; + let inaccessibleWorkflow: WorkflowEntity; + let inaccessibleInsight: InsightsByPeriod; + let accessFilter: InsightsAccessFilter; + let startDate: Date; + let endDate: Date; + + beforeAll(async () => { + member = await createMember(); + + accessibleProject = await createTeamProject(); + await linkUserToProject(member, accessibleProject, 'project:viewer'); + accessibleWorkflow = await createWorkflow({}, accessibleProject); + + inaccessibleProject = await createTeamProject(); + inaccessibleWorkflow = await createWorkflow({}, inaccessibleProject); + + const now = DateTime.utc(); + startDate = now.minus({ days: 7 }).toJSDate(); + endDate = now.toJSDate(); + + [accessibleInsight, inaccessibleInsight] = await Promise.all([ + createCompactedInsightsEvent(accessibleWorkflow, { + type: 'success', + value: 4, + periodUnit: 'day', + periodStart: now.minus({ days: 1 }), + }), + createCompactedInsightsEvent(inaccessibleWorkflow, { + type: 'success', + value: 10, + periodUnit: 'day', + periodStart: now.minus({ days: 1 }), + }), + ]); + + accessFilter = { + user: member, + projectRoles: ['project:viewer'], + workflowRoles: ['workflow:owner'], + }; + }); + + describe('getPreviousAndCurrentPeriodTypeAggregates', () => { + test('should aggregate both workflows when no access filter is applied', async () => { + const insightsByPeriodRepository = Container.get(InsightsByPeriodRepository); + + const rows = await insightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates({ + startDate, + endDate, + }); + + const currentSuccessTotal = rows.find( + (row) => row.period === 'current' && row.type === TypeToNumber.success, + )?.total_value; + + expect(Number(currentSuccessTotal)).toBe( + accessibleInsight.value + inaccessibleInsight.value, + ); + }); + + test('should exclude workflows outside the access filter', async () => { + const insightsByPeriodRepository = Container.get(InsightsByPeriodRepository); + + const rows = await insightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates({ + startDate, + endDate, + accessFilter, + }); + + const currentSuccessTotal = rows.find( + (row) => row.period === 'current' && row.type === TypeToNumber.success, + )?.total_value; + + expect(Number(currentSuccessTotal)).toBe(accessibleInsight.value); + }); + }); + + describe('getInsightsByWorkflow', () => { + test('should return both workflows when no access filter is applied', async () => { + const insightsByPeriodRepository = Container.get(InsightsByPeriodRepository); + + const { count, rows } = await insightsByPeriodRepository.getInsightsByWorkflow({ + startDate, + endDate, + }); + + expect(count).toBe(2); + expect(rows.map((row) => row.workflowId).sort()).toEqual( + [accessibleWorkflow.id, inaccessibleWorkflow.id].sort(), + ); + }); + + test('should exclude workflows outside the access filter', async () => { + const insightsByPeriodRepository = Container.get(InsightsByPeriodRepository); + + const { count, rows } = await insightsByPeriodRepository.getInsightsByWorkflow({ + startDate, + endDate, + accessFilter, + }); + + expect(count).toBe(1); + expect(rows).toHaveLength(1); + expect(rows[0].workflowId).toBe(accessibleWorkflow.id); + expect(rows[0].succeeded).toBe(accessibleInsight.value); + }); + }); + + describe('getInsightsByTime', () => { + test('should aggregate both workflows when no access filter is applied', async () => { + const insightsByPeriodRepository = Container.get(InsightsByPeriodRepository); + + const rows = await insightsByPeriodRepository.getInsightsByTime({ + startDate, + endDate, + periodUnit: 'day', + insightTypes: ['success'], + }); + + const totalSucceeded = rows.reduce((sum, row) => sum + (row.succeeded ?? 0), 0); + + expect(totalSucceeded).toBe(accessibleInsight.value + inaccessibleInsight.value); + }); + + test('should exclude workflows outside the access filter', async () => { + const insightsByPeriodRepository = Container.get(InsightsByPeriodRepository); + + const rows = await insightsByPeriodRepository.getInsightsByTime({ + startDate, + endDate, + periodUnit: 'day', + insightTypes: ['success'], + accessFilter, + }); + + const totalSucceeded = rows.reduce((sum, row) => sum + (row.succeeded ?? 0), 0); + + expect(totalSucceeded).toBe(accessibleInsight.value); + }); + }); + }); }); diff --git a/packages/cli/src/modules/insights/database/repositories/insights-by-period.repository.ts b/packages/cli/src/modules/insights/database/repositories/insights-by-period.repository.ts index d27493825c5..f54b051d656 100644 --- a/packages/cli/src/modules/insights/database/repositories/insights-by-period.repository.ts +++ b/packages/cli/src/modules/insights/database/repositories/insights-by-period.repository.ts @@ -1,10 +1,12 @@ import { isValidTimeZone } from '@n8n/api-types'; import { GlobalConfig } from '@n8n/config'; -import { sql } from '@n8n/db'; +import type { User } from '@n8n/db'; +import { sql, SharedWorkflowRepository } from '@n8n/db'; import { Container, Service } from '@n8n/di'; import type { SelectQueryBuilder } from '@n8n/typeorm'; import { DataSource, LessThanOrEqual, Repository } from '@n8n/typeorm'; import { DateTime } from 'luxon'; +import { UnexpectedError } from 'n8n-workflow'; import { z } from 'zod'; import { getDateRangesCommonTableExpressionQuery } from './insights-by-period-query.helper'; @@ -73,11 +75,24 @@ const aggregatedInsightsByTimeParser = z }) .array(); +/** + * Identifies a caller whose insights must be limited to the workflows they can + * read. + */ +export type InsightsAccessFilter = { + user: User; + projectRoles: string[]; + workflowRoles: string[]; +}; + @Service() export class InsightsByPeriodRepository extends Repository { private isRunningCompaction = false; - constructor(dataSource: DataSource) { + constructor( + dataSource: DataSource, + private readonly sharedWorkflowRepository: SharedWorkflowRepository, + ) { super(InsightsByPeriod, dataSource.manager); } @@ -85,6 +100,30 @@ export class InsightsByPeriodRepository extends Repository { return this.manager.connection.driver.escape(fieldName); } + /** + * Limits a query to insights for workflows the caller can read, by + * correlating against their workflow shares. + */ + private applyAccessFilter( + qb: SelectQueryBuilder, + accessFilter: InsightsAccessFilter, + ) { + // Ensure the metadata relation is joined, so we can correlate against workflowId + if (!qb.expressionMap.joinAttributes.some((join) => join.alias.name === 'metadata')) { + throw new UnexpectedError('The metadata relation must be joined before the access filter'); + } + + const subquery = this.sharedWorkflowRepository.buildSharedWorkflowIdsSubquery( + accessFilter.user, + { + projectRoles: accessFilter.projectRoles, + workflowRoles: accessFilter.workflowRoles, + }, + ); + subquery.andWhere('"sw"."workflowId" = metadata."workflowId"'); + qb.andWhere(`EXISTS (${subquery.getQuery()})`).setParameters(subquery.getParameters()); + } + private getPeriodFilterExpr(maxAgeInDays = 0) { // Database-specific period start expression to filter out data to compact by days matching the periodUnit let periodStartExpr = `date('now', '-${maxAgeInDays} days')`; @@ -305,7 +344,14 @@ export class InsightsByPeriodRepository extends Repository { endDate, projectId, timeZone, - }: { projectId?: string; startDate: Date; endDate: Date; timeZone?: string }): Promise< + accessFilter, + }: { + projectId?: string; + startDate: Date; + endDate: Date; + accessFilter?: InsightsAccessFilter; + timeZone?: string; + }): Promise< Array<{ period: 'previous' | 'current'; type: 0 | 1 | 2 | 3; @@ -336,10 +382,17 @@ export class InsightsByPeriodRepository extends Repository { .groupBy('period') .addGroupBy('insights.type'); + // If we're filtering by projectId or accessFilter, we need a metadata join to access projectId and workflowId. + if (projectId || accessFilter) { + rawRowsQuery.innerJoin('insights.metadata', 'metadata'); + } + if (projectId) { - rawRowsQuery - .innerJoin('insights.metadata', 'metadata') - .andWhere('metadata.projectId = :projectId', { projectId }); + rawRowsQuery.andWhere('metadata.projectId = :projectId', { projectId }); + } + + if (accessFilter) { + this.applyAccessFilter(rawRowsQuery, accessFilter); } const rawRows = await rawRowsQuery.getRawMany(); @@ -373,6 +426,7 @@ export class InsightsByPeriodRepository extends Repository { sortBy = 'total:desc', projectId, timeZone, + accessFilter, }: { skip?: number; take?: number; @@ -381,6 +435,7 @@ export class InsightsByPeriodRepository extends Repository { startDate: Date; endDate: Date; timeZone?: string; + accessFilter?: InsightsAccessFilter; }) { const [sortField, sortOrder] = this.parseSortingParams(sortBy); const sumOfExecutions = sql`SUM(CASE WHEN insights.type IN (${TypeToNumber.success.toString()}, ${TypeToNumber.failure.toString()}) THEN value ELSE 0 END)`; @@ -422,6 +477,10 @@ export class InsightsByPeriodRepository extends Repository { rawRowsQuery.andWhere('metadata.projectId = :projectId', { projectId }); } + if (accessFilter) { + this.applyAccessFilter(rawRowsQuery, accessFilter); + } + const paginatedQuery = rawRowsQuery .clone() .orderBy(this.escapeField(sortField), sortOrder) @@ -443,6 +502,7 @@ export class InsightsByPeriodRepository extends Repository { startDate, endDate, timeZone, + accessFilter, }: { periodUnit: PeriodUnit; insightTypes: TypeUnit[]; @@ -450,6 +510,7 @@ export class InsightsByPeriodRepository extends Repository { startDate: Date; endDate: Date; timeZone?: string; + accessFilter?: InsightsAccessFilter; }) { const cte = getDateRangesCommonTableExpressionQuery({ dbType, startDate, endDate, timeZone }); @@ -474,10 +535,17 @@ export class InsightsByPeriodRepository extends Repository { .groupBy(periodStartExpr) .orderBy(periodStartExpr, 'ASC'); + // If we're filtering by projectId or accessFilter, we need a metadata join to access projectId and workflowId. + if (projectId || accessFilter) { + rawRowsQuery.innerJoin('insights.metadata', 'metadata'); + } + if (projectId) { - rawRowsQuery - .innerJoin('insights.metadata', 'metadata') - .andWhere('metadata.projectId = :projectId', { projectId }); + rawRowsQuery.andWhere('metadata.projectId = :projectId', { projectId }); + } + + if (accessFilter) { + this.applyAccessFilter(rawRowsQuery, accessFilter); } const rawRows = await rawRowsQuery.getRawMany(); diff --git a/packages/cli/src/modules/insights/insights.controller.ts b/packages/cli/src/modules/insights/insights.controller.ts index 937d11e0bc3..9192206985e 100644 --- a/packages/cli/src/modules/insights/insights.controller.ts +++ b/packages/cli/src/modules/insights/insights.controller.ts @@ -24,13 +24,14 @@ export class InsightsController { @Get('/summary') @GlobalScope('insights:list') async getInsightsSummary( - _req: AuthenticatedRequest, + req: AuthenticatedRequest, _res: Response, @Query query: InsightsDateFilterDto = {}, ): Promise { const { startDate, endDate, timeZone } = this.prepareDateFilters(query); return await this.insightsService.getInsightsSummary({ + user: req.user, startDate, endDate, timeZone, @@ -64,7 +65,7 @@ export class InsightsController { @GlobalScope('insights:list') @Licensed('feat:insights:viewDashboard') async getInsightsByTime( - _req: AuthenticatedRequest, + req: AuthenticatedRequest, _res: Response, @Query query: InsightsDateFilterDto, ): Promise { @@ -73,6 +74,7 @@ export class InsightsController { // Cast to full insights by time type // as the service returns all types by default return (await this.insightsService.getInsightsByTime({ + user: req.user, projectId: query.projectId, startDate, endDate, @@ -87,7 +89,7 @@ export class InsightsController { @Get('/by-time/time-saved') @GlobalScope('insights:list') async getTimeSavedInsightsByTime( - _req: AuthenticatedRequest, + req: AuthenticatedRequest, _res: Response, @Query query: InsightsDateFilterDto, ): Promise { @@ -96,6 +98,7 @@ export class InsightsController { // Cast to restricted insights by time type // as the service returns only time saved data return (await this.insightsService.getInsightsByTime({ + user: req.user, insightTypes: ['time_saved_min'], projectId: query.projectId, startDate, diff --git a/packages/cli/src/modules/insights/insights.service.ts b/packages/cli/src/modules/insights/insights.service.ts index d639b698fa3..f43a1910ab7 100644 --- a/packages/cli/src/modules/insights/insights.service.ts +++ b/packages/cli/src/modules/insights/insights.service.ts @@ -7,10 +7,13 @@ import { DateTime } from 'luxon'; import { InstanceSettings } from 'n8n-core'; import { UserError } from 'n8n-workflow'; +import { ForbiddenError } from '@/errors/response-errors/forbidden.error'; +import { userHasScopes } from '@/permissions.ee/check-access'; import { WorkflowSharingService } from '@/workflows/workflow-sharing.service'; import type { PeriodUnit, TypeUnit } from './database/entities/insights-shared'; import { NumberToType, TypeToNumber } from './database/entities/insights-shared'; +import type { InsightsAccessFilter } from './database/repositories/insights-by-period.repository'; import { InsightsByPeriodRepository } from './database/repositories/insights-by-period.repository'; import { InsightsCompactionService } from './insights-compaction.service'; import { InsightsPruningService } from './insights-pruning.service'; @@ -70,22 +73,56 @@ export class InsightsService { await this.stopCompactionAndPruningTimers(); } + /** + * Resolves what insights the caller may read. A requested project must be + * readable by them. When no specific project is requested, results are limited to the + * workflows they can read. + * + * Returns the filter to apply, or `undefined` when the caller's global role + * already grants access to every workflow. + */ + private async resolveAccessFilter( + user: User, + projectId?: string, + ): Promise { + if (projectId) { + const userHasRequiredProjectScopes = await userHasScopes(user, ['workflow:read'], false, { + projectId, + }); + if (!userHasRequiredProjectScopes) { + throw new ForbiddenError('You do not have access to insights for this project.'); + } + } + + const workflowReadRoles = await this.workflowSharingService.rolesGrantingScope( + user, + 'workflow:read', + ); + + return workflowReadRoles && { user, ...workflowReadRoles }; + } + async getInsightsSummary({ + user, startDate, endDate, projectId, timeZone, }: { + user: User; projectId?: string; startDate: Date; endDate: Date; timeZone?: string; }): Promise { + const accessFilter = await this.resolveAccessFilter(user, projectId); + const rows = await this.insightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates({ startDate, endDate, projectId, timeZone, + accessFilter, }); // Initialize data structures for both periods @@ -186,6 +223,8 @@ export class InsightsService { endDate: Date; timeZone?: string; }) { + const accessFilter = await this.resolveAccessFilter(user, projectId); + const { count, rows } = await this.insightsByPeriodRepository.getInsightsByWorkflow({ startDate, endDate, @@ -194,18 +233,13 @@ export class InsightsService { sortBy, projectId, timeZone, + accessFilter, }); - const accessibleWorkflowIds = new Set( - await this.workflowSharingService.getSharedWorkflowIds(user, { - scopes: ['workflow:read'], - projectId, - }), - ); - + // A non-null means the caller can read it; null means the workflow has since been deleted. const data = rows.map((row) => ({ ...row, - hasReadAccess: row.workflowId !== null && accessibleWorkflowIds.has(row.workflowId), + hasReadAccess: row.workflowId !== null, })); return { @@ -215,6 +249,7 @@ export class InsightsService { } async getInsightsByTime({ + user, // Default to all insight types insightTypes = Object.keys(TypeToNumber) as TypeUnit[], projectId, @@ -222,12 +257,14 @@ export class InsightsService { endDate, timeZone, }: { + user: User; insightTypes?: TypeUnit[]; projectId?: string; startDate: Date; endDate: Date; timeZone?: string; }) { + const accessFilter = await this.resolveAccessFilter(user, projectId); const periodUnit = this.getDateFiltersGranularity({ startDate, endDate }); const rows = await this.insightsByPeriodRepository.getInsightsByTime({ periodUnit, @@ -236,6 +273,7 @@ export class InsightsService { startDate, endDate, timeZone, + accessFilter, }); return rows.map((r) => { diff --git a/packages/cli/src/public-api/v1/handlers/insights/insights.handler.ts b/packages/cli/src/public-api/v1/handlers/insights/insights.handler.ts index 1ce089ec427..8ec0e47ba08 100644 --- a/packages/cli/src/public-api/v1/handlers/insights/insights.handler.ts +++ b/packages/cli/src/public-api/v1/handlers/insights/insights.handler.ts @@ -67,6 +67,7 @@ const insightsHandlers: InsightsHandlers = { } const summary = await Container.get(InsightsService).getInsightsSummary({ + user: req.user, startDate, endDate, projectId: query.data.projectId, 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 12c6896fef9..4839c7e2b6d 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.ee.test.ts @@ -7,7 +7,7 @@ import type { WorkflowRepository, } from '@n8n/db'; import type { EntityManager, UpdateResult } from '@n8n/typeorm'; -import type { IWorkflowBase } from 'n8n-workflow'; +import type { INode, IWorkflowBase } from 'n8n-workflow'; import { WorkflowActivationError } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; @@ -115,6 +115,189 @@ describe('EnterpriseWorkflowService', () => { expect(() => service.validateCredentialPermissionsToUser(workflow, [])).not.toThrow(); }); + + it('should inspect credentials referenced inside an inline sub-workflow', () => { + const workflow = mock({ + nodes: [ + { + type: 'n8n-nodes-base.executeWorkflow', + parameters: { + source: 'parameter', + workflowJson: JSON.stringify({ + nodes: [{ credentials: { spotifyApi: { id: 'cred-unknown', name: 'x' } } }], + connections: {}, + }), + }, + }, + ], + }); + + expect(() => + service.validateCredentialPermissionsToUser(workflow, [ + mock({ id: 'cred-1' }), + ]), + ).toThrow(); + }); + + it('should pass when an inline sub-workflow credential is in the allowed list', () => { + const workflow = mock({ + nodes: [ + { + type: 'n8n-nodes-base.executeWorkflow', + parameters: { + source: 'parameter', + workflowJson: JSON.stringify({ + nodes: [{ credentials: { spotifyApi: { id: 'cred-1', name: 'x' } } }], + connections: {}, + }), + }, + }, + ], + }); + + expect(() => + service.validateCredentialPermissionsToUser(workflow, [ + mock({ id: 'cred-1' }), + ]), + ).not.toThrow(); + }); + + it('should reject a non-managed credential with a null id (name-only reference)', () => { + // Built as a real object: mock strips an explicit null id. + const workflow = { + nodes: [{ credentials: { spotifyApi: { id: null, name: 'Someone else prod' } } }], + } as unknown as IWorkflowBase; + + expect(() => + service.validateCredentialPermissionsToUser(workflow, [ + mock({ id: 'cred-1' }), + ]), + ).toThrow(); + }); + + it('should reject a non-managed credential with an empty-string id', () => { + const workflow = mock({ + nodes: [{ credentials: { spotifyApi: { id: '', name: 'Someone else prod' } } }], + }); + + expect(() => + service.validateCredentialPermissionsToUser(workflow, [ + mock({ id: 'cred-1' }), + ]), + ).toThrow(); + }); + + it('should reject a null-id credential hidden inside an inline sub-workflow', () => { + const workflow = mock({ + nodes: [ + { + type: 'n8n-nodes-base.executeWorkflow', + parameters: { + source: 'parameter', + workflowJson: JSON.stringify({ + nodes: [{ credentials: { spotifyApi: { id: null, name: 'Someone else prod' } } }], + connections: {}, + }), + }, + }, + ], + }); + + expect(() => + service.validateCredentialPermissionsToUser(workflow, [ + mock({ id: 'cred-1' }), + ]), + ).toThrow(); + }); + + it('should inspect credentials nested in a deeper inline sub-workflow', () => { + const inner = JSON.stringify({ + nodes: [{ credentials: { spotifyApi: { id: 'cred-unknown', name: 'x' } } }], + connections: {}, + }); + const workflow = mock({ + nodes: [ + { + type: 'n8n-nodes-base.executeWorkflow', + parameters: { + source: 'parameter', + workflowJson: JSON.stringify({ + nodes: [ + { type: 'n8n-nodes-base.executeWorkflow', parameters: { workflowJson: inner } }, + ], + connections: {}, + }), + }, + }, + ], + }); + + expect(() => + service.validateCredentialPermissionsToUser(workflow, [ + mock({ id: 'cred-1' }), + ]), + ).toThrow(); + }); + }); + + describe('validateWorkflowCredentialUsage() - unresolved credentials', () => { + // Real objects, not mock, so the explicit null id survives. + const nodeWithNullCred = (id: string, name: string) => + ({ + id, + name, + type: 'n8n-nodes-base.httpRequest', + typeVersion: 4.2, + position: [0, 0], + parameters: {}, + credentials: { httpHeaderAuth: { id: null, name: 'some name' } }, + }) as unknown as INode; + + it('rejects a new node carrying an unresolved (name-only) credential as tampering', () => { + const newVersion = { + nodes: [nodeWithNullCred('new-1', 'Steal')], + } as unknown as IWorkflowBase; + const previousVersion = { nodes: [] } as unknown as IWorkflowBase; + + expect(() => + service.validateWorkflowCredentialUsage(newVersion, previousVersion, []), + ).toThrow(); + }); + + it('rejects an unresolved credential smuggled into a new inline sub-workflow node', () => { + const inlineNode = { + id: 'new-inline', + name: 'Sub', + type: 'n8n-nodes-base.executeWorkflow', + typeVersion: 1.2, + position: [0, 0], + parameters: { + source: 'parameter', + workflowJson: JSON.stringify({ + nodes: [{ credentials: { httpHeaderAuth: { id: null, name: 'y' } } }], + connections: {}, + }), + }, + } as unknown as INode; + const newVersion = { nodes: [inlineNode] } as unknown as IWorkflowBase; + const previousVersion = { nodes: [] } as unknown as IWorkflowBase; + + expect(() => + service.validateWorkflowCredentialUsage(newVersion, previousVersion, []), + ).toThrow(); + }); + + it('keeps an unresolved credential on a pre-existing (read-only) node without throwing', () => { + const existing = nodeWithNullCred('existing-1', 'Call'); + const newVersion = { + nodes: [{ ...existing, name: 'Renamed' }], + } as unknown as IWorkflowBase; + const previousVersion = { nodes: [existing] } as unknown as IWorkflowBase; + + expect(() => + service.validateWorkflowCredentialUsage(newVersion, previousVersion, []), + ).not.toThrow(); + }); }); describe('attemptWorkflowReactivation', () => { diff --git a/packages/cli/src/workflows/__tests__/workflow.service.test.ts b/packages/cli/src/workflows/__tests__/workflow.service.test.ts index 84bc9257738..a91c077410b 100644 --- a/packages/cli/src/workflows/__tests__/workflow.service.test.ts +++ b/packages/cli/src/workflows/__tests__/workflow.service.test.ts @@ -1686,6 +1686,7 @@ describe('WorkflowService', () => { let externalHooksMock: MockProxy; let workflowPublishedVersionRepositoryMock: MockProxy; let workflowMutationHooksMock: MockProxy; + let ownershipServiceMock: MockProxy; const WORKFLOW_ID = 'workflow-1'; @@ -1702,6 +1703,7 @@ describe('WorkflowService', () => { beforeEach(() => { workflowFinderServiceMock = mock(); + ownershipServiceMock = mock(); workflowRepositoryMock = mock(); executionPersistenceMock = mock(); activeWorkflowManagerMock = mock(); @@ -1718,7 +1720,7 @@ describe('WorkflowService', () => { mock(), // sharedWorkflowRepository workflowRepositoryMock, // workflowRepository mock(), // workflowTagMappingRepository - mock(), // ownershipService + ownershipServiceMock, // ownershipService mock(), // tagService mock(), // workflowHistoryService externalHooksMock, // externalHooks @@ -1886,6 +1888,25 @@ describe('WorkflowService', () => { ).toBeLessThan(workflowRepositoryMock.delete.mock.invocationCallOrder[0]); }); + test('invalidates the cached project for the deleted workflow', async () => { + const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null }); + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); + + await workflowService.delete(mock(), WORKFLOW_ID, true); + + expect(ownershipServiceMock.invalidateWorkflowProjectCacheByIds).toHaveBeenCalledWith([ + WORKFLOW_ID, + ]); + }); + + test('does not invalidate the cached project when the workflow is not found', async () => { + workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(null); + + await workflowService.delete(mock(), WORKFLOW_ID, true); + + expect(ownershipServiceMock.invalidateWorkflowProjectCacheByIds).not.toHaveBeenCalled(); + }); + test('forwards the acting user to the delete and afterDelete hooks', async () => { const workflow = makeWorkflowEntity({ isArchived: true, activeVersionId: null }); workflowFinderServiceMock.findWorkflowForUser.mockResolvedValue(workflow); diff --git a/packages/cli/src/workflows/workflow-sharing.service.ts b/packages/cli/src/workflows/workflow-sharing.service.ts index 215b02b947d..202a918af29 100644 --- a/packages/cli/src/workflows/workflow-sharing.service.ts +++ b/packages/cli/src/workflows/workflow-sharing.service.ts @@ -146,4 +146,27 @@ export class WorkflowSharingService { }); return sharedWorkflows.map(({ workflowId }) => workflowId); } + + /** + * Resolve the roles granting `scope`. Returns `undefined` when the user's + * global role already grants the scope, meaning no filtering is needed. + */ + async rolesGrantingScope( + user: User, + scope: Scope, + ): Promise<{ projectRoles: string[]; workflowRoles: string[] } | undefined> { + if (hasGlobalScope(user, scope)) { + return undefined; + } + + const [projectRoles, workflowRoles] = await Promise.all([ + this.roleService.rolesWithScope('project', [scope]), + this.roleService.rolesWithScope('workflow', [scope]), + ]); + + return { + projectRoles, + workflowRoles, + }; + } } diff --git a/packages/cli/src/workflows/workflow.service.ee.ts b/packages/cli/src/workflows/workflow.service.ee.ts index 6c4f02283fb..fd33394c9a0 100644 --- a/packages/cli/src/workflows/workflow.service.ee.ts +++ b/packages/cli/src/workflows/workflow.service.ee.ts @@ -154,21 +154,14 @@ export class EnterpriseWorkflowService { workflow: IWorkflowBase, allowedCredentials: CredentialsEntity[], ) { - workflow.nodes.forEach((node) => { - if (!node.credentials) { - return; - } - Object.keys(node.credentials).forEach((credentialType) => { - const credential = node.credentials?.[credentialType]; - if (credential?.__aiGatewayManaged && credential?.id === null) return; - const credentialId = credential?.id; - if (credentialId === undefined) return; - const matchedCredential = allowedCredentials.find(({ id }) => id === credentialId); - if (!matchedCredential) { - throw new UserError('The workflow contains credentials that you do not have access to'); - } - }); - }); + // Reuse the shared collector so inline sub-workflow credentials (Execute + // Sub-workflow, Workflow Tool, Workflow Retriever) and unresolved name-only + // references are inspected too, keeping this check aligned with the update path. + const allowedCredentialIds = allowedCredentials.map(({ id }) => id); + const inaccessibleNodes = this.getNodesWithInaccessibleCreds(workflow, allowedCredentialIds); + if (inaccessibleNodes.length > 0) { + throw new UserError('The workflow contains credentials that you do not have access to'); + } } async preventTampering(workflow: T, workflowId: string, user: User) { @@ -264,23 +257,35 @@ export class EnterpriseWorkflowService { return newWorkflowVersion; } - /** Get all nodes in a workflow where the node credential is not accessible to the user. */ + /** + * 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. + */ getNodesWithInaccessibleCreds(workflow: IWorkflowBase, userCredIds: string[]) { if (!workflow.nodes) { return []; } return workflow.nodes.filter((node) => { - const usedCredentialIds = this.getCredentialIdsUsedByNode(node); - return usedCredentialIds.some((credId) => !userCredIds.includes(credId)); + const { ids, hasUnresolved } = this.getNodeCredentialRefs(node); + return hasUnresolved || ids.some((credId) => !userCredIds.includes(credId)); }); } /** - * Collect every credential id a node references. Besides the node's own + * 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 parsed out and returned as well. + * 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 @@ -289,8 +294,9 @@ export class EnterpriseWorkflowService { * the request size (each level embeds its child as literal escaped JSON) and * cannot cycle, so the stack cannot grow unbounded. */ - private getCredentialIdsUsedByNode(node: INode): string[] { - const credentialIds: string[] = []; + private getNodeCredentialRefs(node: INode): { ids: string[]; hasUnresolved: boolean } { + const ids: string[] = []; + let hasUnresolved = false; const stack: INode[] = [node]; while (stack.length > 0) { @@ -299,7 +305,13 @@ export class EnterpriseWorkflowService { if (current.credentials) { for (const nodeCred of Object.values(current.credentials)) { const id = nodeCred.id?.toString(); - if (id) credentialIds.push(id); + if (id) { + ids.push(id); + } else if (nodeCred.__aiGatewayManaged && nodeCred.id === null) { + // Managed credential, resolved at execution — exempt. + } else if (nodeCred.id === null || nodeCred.id === '') { + hasUnresolved = true; + } } } @@ -308,7 +320,7 @@ export class EnterpriseWorkflowService { } } - return credentialIds; + return { ids, hasUnresolved }; } /** diff --git a/packages/cli/src/workflows/workflow.service.ts b/packages/cli/src/workflows/workflow.service.ts index 5c741ba3de3..f86a013697a 100644 --- a/packages/cli/src/workflows/workflow.service.ts +++ b/packages/cli/src/workflows/workflow.service.ts @@ -1197,6 +1197,8 @@ export class WorkflowService { await this.workflowRepository.delete(workflowId); + await this.ownershipService.invalidateWorkflowProjectCacheByIds([workflowId]); + // After the cascade, so it can see the rows the delete orphaned. Observes a // committed delete, so it must not throw — the module swallows its own errors. await this.workflowMutationHooks.afterWorkflowsDeleted([workflowId]); diff --git a/packages/cli/test/integration/insights/insights.api.test.ts b/packages/cli/test/integration/insights/insights.api.test.ts index 4cce7e460f7..8f198042a01 100644 --- a/packages/cli/test/integration/insights/insights.api.test.ts +++ b/packages/cli/test/integration/insights/insights.api.test.ts @@ -1,11 +1,20 @@ import type { InsightsDateRange } from '@n8n/api-types'; -import { mockInstance, createWorkflow, createTeamProject, testDb } from '@n8n/backend-test-utils'; +import { + mockInstance, + createWorkflow, + createTeamProject, + linkUserToProject, + testDb, +} from '@n8n/backend-test-utils'; +import type { Project } from '@n8n/db'; import { GLOBAL_ADMIN_ROLE, GLOBAL_MEMBER_ROLE, GLOBAL_OWNER_ROLE } from '@n8n/db'; import { DateTime } from 'luxon'; import { createCompactedInsightsEvent } from '@/modules/insights/database/entities/__tests__/db-utils'; +import type { InsightsByPeriod } from '@/modules/insights/database/entities/insights-by-period'; import { Telemetry } from '@/telemetry'; +import { createCustomRoleWithScopeSlugs } from '../shared/db/roles'; import { createUser } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; import * as utils from '../shared/utils'; @@ -20,6 +29,17 @@ const testServer = utils.setupTestServer({ modules: ['insights'], }); +const truncateDatabase = async () => { + await testDb.truncate([ + 'InsightsRaw', + 'InsightsByPeriod', + 'InsightsMetadata', + 'SharedWorkflow', + 'WorkflowEntity', + 'Project', + ]); +}; + beforeAll(async () => { const owner = await createUser({ role: GLOBAL_OWNER_ROLE }); const admin = await createUser({ role: GLOBAL_ADMIN_ROLE }); @@ -113,6 +133,87 @@ describe('GET /insights routes return 200 if date range inside license limits', }); }); +describe('GET /insights/summary', () => { + describe('scopes results by project access', () => { + let insightsViewer: SuperAgentTest; + let accessibleProject: Project; + let inaccessibleProject: Project; + + let accessibleWorkflowInsights: InsightsByPeriod; + + beforeAll(async () => { + testServer.license.setDefaults({ + features: ['feat:insights:viewSummary', 'feat:insights:viewDashboard'], + quotas: { 'quota:insights:maxHistoryDays': 365 }, + }); + + // A global role granting only the insights view scopes, and no workflow access + const insightsRole = await createCustomRoleWithScopeSlugs( + ['insights:list', 'insights:read'], + { + roleType: 'global', + }, + ); + const viewer = await createUser({ role: insightsRole }); + insightsViewer = testServer.authAgentFor(viewer); + + accessibleProject = await createTeamProject(); + inaccessibleProject = await createTeamProject(); + await linkUserToProject(viewer, accessibleProject, 'project:viewer'); + + const accessibleWorkflow = await createWorkflow({}, accessibleProject); + const inaccessibleWorkflow = await createWorkflow({}, inaccessibleProject); + + const periodStart = DateTime.utc().startOf('day'); + accessibleWorkflowInsights = await createCompactedInsightsEvent(accessibleWorkflow, { + type: 'success', + value: 1, + periodUnit: 'day', + periodStart, + }); + await createCompactedInsightsEvent(inaccessibleWorkflow, { + type: 'success', + value: 5, + periodUnit: 'day', + periodStart, + }); + }); + + test('should return the summary for a project the user can read', async () => { + const response = await insightsViewer + .get('/insights/summary') + .query({ projectId: accessibleProject.id }) + .expect(200); + + expect(response.body.data.total.value).toBe(accessibleWorkflowInsights.value); + }); + + test('should return 403 for a project the user cannot read', async () => { + await insightsViewer + .get('/insights/summary') + .query({ projectId: inaccessibleProject.id }) + .expect(403); + }); + + test('should respond the same for an unknown project id as for an inaccessible one', async () => { + await insightsViewer + .get('/insights/summary') + .query({ projectId: 'non-existing-project-id' }) + .expect(403); + }); + + test('should return the summary when no project is requested', async () => { + const response = await insightsViewer.get('/insights/summary').expect(200); + + expect(response.body.data.total.value).toBe(accessibleWorkflowInsights.value); + }); + + afterAll(async () => { + await truncateDatabase(); + }); + }); +}); + describe('GET /insights/by-workflow', () => { beforeAll(() => { testServer.license.setDefaults({ @@ -189,14 +290,7 @@ describe('GET /insights/by-workflow', () => { describe('sorting order verification', () => { afterEach(async () => { - await testDb.truncate([ - 'InsightsRaw', - 'InsightsByPeriod', - 'InsightsMetadata', - 'SharedWorkflow', - 'WorkflowEntity', - 'Project', - ]); + await truncateDatabase(); }); test('should return workflows sorted by total:desc', async () => { @@ -325,4 +419,215 @@ describe('GET /insights/by-workflow', () => { }); }); }); + + describe('scopes results by project access', () => { + let insightsViewer: SuperAgentTest; + let accessibleProject: Project; + let inaccessibleProject: Project; + let inaccessibleWorkflowName: string; + + let accessibleWorkflowInsights: InsightsByPeriod; + + beforeAll(async () => { + testServer.license.setDefaults({ + features: ['feat:insights:viewSummary', 'feat:insights:viewDashboard'], + quotas: { 'quota:insights:maxHistoryDays': 365 }, + }); + + // A global role granting only the insights view scopes, and no workflow access + const insightsRole = await createCustomRoleWithScopeSlugs( + ['insights:list', 'insights:read'], + { + roleType: 'global', + }, + ); + const viewer = await createUser({ role: insightsRole }); + insightsViewer = testServer.authAgentFor(viewer); + + accessibleProject = await createTeamProject(); + inaccessibleProject = await createTeamProject(); + await linkUserToProject(viewer, accessibleProject, 'project:viewer'); + + const accessibleWorkflow = await createWorkflow({}, accessibleProject); + const inaccessibleWorkflow = await createWorkflow( + { name: 'TOPSECRET Payroll Sync' }, + inaccessibleProject, + ); + inaccessibleWorkflowName = inaccessibleWorkflow.name; + + const periodStart = DateTime.utc().startOf('day'); + accessibleWorkflowInsights = await createCompactedInsightsEvent(accessibleWorkflow, { + type: 'success', + value: 2, + periodUnit: 'day', + periodStart, + }); + await createCompactedInsightsEvent(inaccessibleWorkflow, { + type: 'success', + value: 6, + periodUnit: 'day', + periodStart, + }); + }); + + test('should not include workflows from inaccessible projects when no project is requested', async () => { + const response = await insightsViewer.get('/insights/by-workflow').expect(200); + + expect(response.body.data.count).toBe(1); + expect(response.body.data.data).toHaveLength(1); + + const workflowNames = response.body.data.data.map( + (row: { workflowName: string }) => row.workflowName, + ); + expect(workflowNames).not.toContain(inaccessibleWorkflowName); + expect(response.body.data.data[0].total).toBe(accessibleWorkflowInsights.value); + }); + + test('should return 403 for a project the user cannot read', async () => { + await insightsViewer + .get('/insights/by-workflow') + .query({ projectId: inaccessibleProject.id }) + .expect(403); + }); + + test('should return workflows for a project the user can read', async () => { + const response = await insightsViewer + .get('/insights/by-workflow') + .query({ projectId: accessibleProject.id }) + .expect(200); + + expect(response.body.data.data).toHaveLength(1); + expect(response.body.data.data[0].total).toBe(accessibleWorkflowInsights.value); + }); + + afterAll(async () => { + await truncateDatabase(); + }); + }); +}); + +describe('GET /insights/by-time', () => { + describe('scopes results by project access', () => { + let insightsViewer: SuperAgentTest; + let accessibleProject: Project; + let inaccessibleProject: Project; + + let accessibleWorkflowInsights: InsightsByPeriod; + + beforeAll(async () => { + testServer.license.setDefaults({ + features: ['feat:insights:viewSummary', 'feat:insights:viewDashboard'], + quotas: { 'quota:insights:maxHistoryDays': 365 }, + }); + + // A global role granting only the insights view scopes, and no workflow access + const insightsRole = await createCustomRoleWithScopeSlugs( + ['insights:list', 'insights:read'], + { + roleType: 'global', + }, + ); + const viewer = await createUser({ role: insightsRole }); + insightsViewer = testServer.authAgentFor(viewer); + + accessibleProject = await createTeamProject(); + inaccessibleProject = await createTeamProject(); + await linkUserToProject(viewer, accessibleProject, 'project:viewer'); + + const accessibleWorkflow = await createWorkflow({}, accessibleProject); + const inaccessibleWorkflow = await createWorkflow({}, inaccessibleProject); + + const periodStart = DateTime.utc().startOf('day'); + accessibleWorkflowInsights = await createCompactedInsightsEvent(accessibleWorkflow, { + type: 'success', + value: 3, + periodUnit: 'day', + periodStart, + }); + await createCompactedInsightsEvent(inaccessibleWorkflow, { + type: 'success', + value: 7, + periodUnit: 'day', + periodStart, + }); + }); + + test('returns 403 for an inaccessible project and scoped totals otherwise', async () => { + await insightsViewer + .get('/insights/by-time') + .query({ projectId: inaccessibleProject.id }) + .expect(403); + + const response = await insightsViewer.get('/insights/by-time').expect(200); + + expect(response.body.data[0].values.succeeded).toBe(accessibleWorkflowInsights.value); + }); + + afterAll(async () => { + await truncateDatabase(); + }); + }); +}); + +describe('GET /insights/by-time/time-saved', () => { + describe('scopes results by project access', () => { + let insightsViewer: SuperAgentTest; + let accessibleProject: Project; + let inaccessibleProject: Project; + + let accessibleWorkflowInsights: InsightsByPeriod; + + beforeAll(async () => { + testServer.license.setDefaults({ + features: ['feat:insights:viewSummary', 'feat:insights:viewDashboard'], + quotas: { 'quota:insights:maxHistoryDays': 365 }, + }); + + // A global role granting only the insights view scopes, and no workflow access + const insightsRole = await createCustomRoleWithScopeSlugs( + ['insights:list', 'insights:read'], + { + roleType: 'global', + }, + ); + const viewer = await createUser({ role: insightsRole }); + insightsViewer = testServer.authAgentFor(viewer); + + accessibleProject = await createTeamProject(); + inaccessibleProject = await createTeamProject(); + await linkUserToProject(viewer, accessibleProject, 'project:viewer'); + + const accessibleWorkflow = await createWorkflow({}, accessibleProject); + const inaccessibleWorkflow = await createWorkflow({}, inaccessibleProject); + + const periodStart = DateTime.utc().startOf('day'); + accessibleWorkflowInsights = await createCompactedInsightsEvent(accessibleWorkflow, { + type: 'time_saved_min', + value: 4, + periodUnit: 'day', + periodStart, + }); + await createCompactedInsightsEvent(inaccessibleWorkflow, { + type: 'time_saved_min', + value: 8, + periodUnit: 'day', + periodStart, + }); + }); + + afterAll(async () => { + await truncateDatabase(); + }); + + test('returns 403 for an inaccessible project and scoped totals otherwise', async () => { + await insightsViewer + .get('/insights/by-time/time-saved') + .query({ projectId: inaccessibleProject.id }) + .expect(403); + + const response = await insightsViewer.get('/insights/by-time/time-saved').expect(200); + + expect(response.body.data[0].values.timeSaved).toBe(accessibleWorkflowInsights.value); + }); + }); }); diff --git a/packages/cli/test/integration/public-api/insights.test.ts b/packages/cli/test/integration/public-api/insights.test.ts index 6b2bd35e447..6a3d61698ac 100644 --- a/packages/cli/test/integration/public-api/insights.test.ts +++ b/packages/cli/test/integration/public-api/insights.test.ts @@ -1,11 +1,17 @@ import { insightsSummarySchema } from '@n8n/api-types'; -import { createTeamProject, createWorkflow, testDb } from '@n8n/backend-test-utils'; -import { type User } from '@n8n/db'; +import { + createTeamProject, + createWorkflow, + linkUserToProject, + testDb, +} from '@n8n/backend-test-utils'; +import { type Project, type User } from '@n8n/db'; import { DateTime } from 'luxon'; import { createCompactedInsightsEvent } from '@/modules/insights/database/entities/__tests__/db-utils'; -import { createOwnerWithApiKey } from '../shared/db/users'; +import { createCustomRoleWithScopeSlugs } from '../shared/db/roles'; +import { addApiKey, createOwnerWithApiKey, createUser } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; import * as utils from '../shared/utils'; @@ -174,4 +180,69 @@ describe('GET /insights/summary', () => { expect(response.body.total.value).toBe(4); expect(response.body.failed.value).toBe(1); }); + + describe('project access', () => { + let viewer: User; + let viewerAgent: SuperAgentTest; + let accessibleProject: Project; + let inaccessibleProject: Project; + let accessibleWorkflow: Awaited>; + let inaccessibleWorkflow: Awaited>; + + beforeAll(async () => { + // A global role granting only the insights view scopes, and no workflow access + const insightsRole = await createCustomRoleWithScopeSlugs( + ['insights:list', 'insights:read'], + { + roleType: 'global', + }, + ); + viewer = await createUser({ role: insightsRole }); + viewer.apiKeys = [await addApiKey(viewer, { scopes: ['insights:read'] })]; + + accessibleProject = await createTeamProject(); + inaccessibleProject = await createTeamProject(); + await linkUserToProject(viewer, accessibleProject, 'project:viewer'); + accessibleWorkflow = await createWorkflow({}, accessibleProject); + inaccessibleWorkflow = await createWorkflow({}, inaccessibleProject); + }); + + beforeEach(() => { + viewerAgent = testServer.publicApiAgentFor(viewer); + }); + + test('returns 403 for a project the API key holder cannot read', async () => { + await viewerAgent + .get('/insights/summary') + .query({ projectId: inaccessibleProject.id }) + .expect(403); + }); + + test('returns the summary for a project the API key holder can read', async () => { + await viewerAgent + .get('/insights/summary') + .query({ projectId: accessibleProject.id }) + .expect(200); + }); + + test('aggregates only accessible projects when no project is requested', async () => { + const accessibleSuccessfulExecutions = 3; + const inaccessibleSuccessfulExecutions = 5; + + await createSummaryMetrics(accessibleWorkflow, { success: accessibleSuccessfulExecutions }); + await createSummaryMetrics(inaccessibleWorkflow, { + success: inaccessibleSuccessfulExecutions, + }); + + const response = await viewerAgent + .get('/insights/summary') + .query({ + startDate: DateTime.utc().minus({ days: 2 }).toISO(), + endDate: DateTime.utc().plus({ days: 1 }).toISO(), + }) + .expect(200); + + expect(response.body.total.value).toBe(accessibleSuccessfulExecutions); + }); + }); }); diff --git a/packages/cli/test/integration/workflows/workflow-sharing.service.test.ts b/packages/cli/test/integration/workflows/workflow-sharing.service.test.ts index 0d1729c23ef..87b91b4d8cd 100644 --- a/packages/cli/test/integration/workflows/workflow-sharing.service.test.ts +++ b/packages/cli/test/integration/workflows/workflow-sharing.service.test.ts @@ -116,4 +116,26 @@ describe('WorkflowSharingService', () => { expect(sharedWorkflowIds).not.toContain(workflow2.id); }); }); + + describe('rolesGrantingScope', () => { + it('should return no options for users holding the scope globally', async () => { + const options = await workflowSharingService.rolesGrantingScope(owner, 'workflow:read'); + + expect(options).toBeUndefined(); + }); + + it('should return the roles granting the scope for other users', async () => { + const options = await workflowSharingService.rolesGrantingScope(member, 'workflow:read'); + + expect(options?.projectRoles).toContain('project:viewer'); + expect(options?.workflowRoles).toContain('workflow:owner'); + }); + + it('should return only the roles granting the requested scope', async () => { + const options = await workflowSharingService.rolesGrantingScope(member, 'workflow:update'); + + expect(options?.projectRoles).not.toContain('project:viewer'); + expect(options?.projectRoles).toContain('project:admin'); + }); + }); }); diff --git a/packages/core/src/execution-engine/__tests__/workflow-execute-node-error-reporting.test.ts b/packages/core/src/execution-engine/__tests__/workflow-execute-node-error-reporting.test.ts index 7958c3bdfcf..457ded8a0b2 100644 --- a/packages/core/src/execution-engine/__tests__/workflow-execute-node-error-reporting.test.ts +++ b/packages/core/src/execution-engine/__tests__/workflow-execute-node-error-reporting.test.ts @@ -80,7 +80,7 @@ describe('WorkflowExecute node error forwarding to ErrorReporter', () => { async function runWorkflowThatThrows( error: unknown, additionalDataOverrides: Partial = {}, - ): Promise { + ): Promise { const nodeType = mock({ description: { name: 'manualTrigger', @@ -116,7 +116,23 @@ describe('WorkflowExecute node error forwarding to ErrorReporter', () => { ).mockImplementation(() => {}); await workflowExecute.run({ workflow, startNode: triggerNode }); - await waitPromise.promise; + return await waitPromise.promise; + } + + function createAxiosError() { + return Object.assign(new Error('connect ECONNREFUSED 127.0.0.1:443'), { + name: 'AxiosError', + isAxiosError: true, + code: 'ECONNREFUSED', + config: { + headers: { 'x-request-header': 'payload' }, + data: '{"requestField":"payload"}', + }, + options: { + headers: { 'x-request-header': 'payload' }, + data: '{"requestField":"payload"}', + }, + }); } it('should report a Error instance as class + stack only, without its message', async () => { @@ -174,6 +190,39 @@ describe('WorkflowExecute node error forwarding to ErrorReporter', () => { expect(mockErrorReporter.error).not.toHaveBeenCalled(); }); + it('should store unhandled axios errors as NodeApiError', async () => { + const run = await runWorkflowThatThrows(createAxiosError()); + const nodeError = run.data.resultData.runData.ThrowingNode[0].error; + const resultError = run.data.resultData.error; + + expect(nodeError?.name).toBe('NodeApiError'); + expect(resultError?.name).toBe('NodeApiError'); + }); + + it('should omit axios config from stored errors', async () => { + const run = await runWorkflowThatThrows(createAxiosError()); + const nodeError = run.data.resultData.runData.ThrowingNode[0].error; + const resultError = run.data.resultData.error; + + expect(nodeError).not.toHaveProperty('config'); + expect(resultError).not.toHaveProperty('config'); + }); + + it('should omit legacy request options from stored errors', async () => { + const run = await runWorkflowThatThrows(createAxiosError()); + const nodeError = run.data.resultData.runData.ThrowingNode[0].error; + const resultError = run.data.resultData.error; + + expect(nodeError).not.toHaveProperty('options'); + expect(resultError).not.toHaveProperty('options'); + }); + + it('should omit request values from serialized execution errors', async () => { + const run = await runWorkflowThatThrows(createAxiosError()); + + expect(JSON.stringify(run.data.resultData)).not.toContain('payload'); + }); + it('should not report an ApplicationError with no cause', async () => { const plainAppError = new ApplicationError('plain operational error', { level: 'error' }); diff --git a/packages/core/src/execution-engine/workflow-execute.ts b/packages/core/src/execution-engine/workflow-execute.ts index cc797c7ffa9..21919415cc8 100644 --- a/packages/core/src/execution-engine/workflow-execute.ts +++ b/packages/core/src/execution-engine/workflow-execute.ts @@ -27,7 +27,6 @@ import type { ITaskData, ITaskDataConnections, ITaskMetadata, - NodeApiError, NodeOperationError, Workflow, IRunExecutionData, @@ -39,6 +38,7 @@ import type { INodeIssues, INodeType, ITaskStartedData, + JsonObject, AiAgentRequest, IWorkflowExecutionDataProcess, EngineRequest, @@ -55,6 +55,7 @@ import { UnexpectedError, UserError, OperationalError, + NodeApiError, TimeoutExecutionCancelledError, ManualExecutionCancelledError, createRunExecutionData, @@ -105,6 +106,14 @@ interface RunWorkflowOptions { additionalRunFilterNodes?: string[]; } +function normalizeUnhandledAxiosError(error: unknown, node: INode): ExecutionBaseError { + if (isAxiosError(error)) { + return new NodeApiError(node, error as JsonObject); + } + + return error as ExecutionBaseError; +} + export class WorkflowExecute { private status: ExecutionStatus = 'new'; @@ -2010,7 +2019,7 @@ export class WorkflowExecute { }); } - const e = error as unknown as ExecutionBaseError; + const e = normalizeUnhandledAxiosError(error, executionNode); executionError = { ...e, message: e.message, stack: e.stack }; diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 99c3ad32635..b249434e33f 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -6483,6 +6483,8 @@ "insights.dashboard.dataRangeAlert.dismiss": "Dismiss", "insights.dashboard.paywall.title": "Upgrade to access more detailed insights", "insights.dashboard.paywall.description": "Gain access to more granular, per-workflow insights and visual breakdown of production executions over different time periods.", + "insights.dashboard.error.forbidden.title": "Couldn't load insights", + "insights.dashboard.error.forbidden.message": "You don't have access to insights for this project", "insights.banner.title.timeSaved.tooltip": "Total time saved calculated from your estimated time savings per execution across all workflows", "insights.banner.queueMode.warning": "We identified and fixed an issue where insights execution counts were duplicated for queue mode users.", "insights.banner.queueMode.warning.link.text": "Learn more", diff --git a/packages/frontend/editor-ui/src/features/execution/insights/components/InsightsDashboard.test.ts b/packages/frontend/editor-ui/src/features/execution/insights/components/InsightsDashboard.test.ts index 635a6fd6068..6c69a33af75 100644 --- a/packages/frontend/editor-ui/src/features/execution/insights/components/InsightsDashboard.test.ts +++ b/packages/frontend/editor-ui/src/features/execution/insights/components/InsightsDashboard.test.ts @@ -23,6 +23,7 @@ import type { } from '@n8n/api-types'; import { INSIGHT_TYPES } from '@/features/execution/insights/insights.constants'; import type { InsightsSummaryDisplay } from '@/features/execution/insights/insights.types'; +import { ResponseError } from '@n8n/rest-api-client/utils'; import { vi } from 'vitest'; const { emitters, addEmitter } = useEmitters<'n8nDataTableServer'>(); @@ -71,6 +72,12 @@ const mockTelemetry = { track: vi.fn(), }; +const showError = vi.fn(); + +vi.mock('@n8n/composables/useToast', () => ({ + useToast: () => ({ showError }), +})); + vi.mock('@n8n/composables/useTelemetry', () => ({ useTelemetry: () => mockTelemetry, })); @@ -726,6 +733,89 @@ describe('InsightsDashboard', () => { }, }); }); + + it('should show an error and revert to all projects when the selected project is forbidden', async () => { + insightsStore.charts.execute = vi.fn().mockImplementation(async (_delay, params) => { + insightsStore.charts.error = params?.projectId + ? new ResponseError('You do not have access to insights for this project.', { + httpStatusCode: 403, + }) + : null; + }); + + renderComponent({ + props: { insightType: INSIGHT_TYPES.TOTAL }, + }); + + await waitFor(() => { + expect(screen.getByTestId('project-sharing-select')).toBeInTheDocument(); + }); + + await selectProject(teamProjects[0].name); + await waitAllPromises(); + + expect(showError).toHaveBeenCalledWith( + expect.objectContaining({ + httpStatusCode: 403, + message: 'You do not have access to insights for this project.', + }), + "Couldn't load insights", + { message: "You don't have access to insights for this project" }, + ); + + await waitFor(() => { + expect(insightsStore.charts.execute).toHaveBeenLastCalledWith(0, { + ...DEFAULT_DATE_RANGE, + projectId: undefined, + }); + }); + }); + + it('should not revert the current selection when a stale forbidden request resolves after a newer one', async () => { + let resolveForbiddenFetch: () => void = () => {}; + const forbiddenFetchGate = new Promise((resolve) => { + resolveForbiddenFetch = resolve; + }); + + insightsStore.charts.execute = vi.fn().mockImplementation(async (_delay, params) => { + if (params?.projectId === teamProjects[0].id) { + await forbiddenFetchGate; + insightsStore.charts.error = new ResponseError( + 'You do not have access to insights for this project.', + { httpStatusCode: 403 }, + ); + return; + } + insightsStore.charts.error = null; + }); + + renderComponent({ + props: { insightType: INSIGHT_TYPES.TOTAL }, + }); + + await waitFor(() => { + expect(screen.getByTestId('project-sharing-select')).toBeInTheDocument(); + }); + + // Select the forbidden project — its request is held open below. + await selectProject(teamProjects[0].name); + + // Switch to an accessible project before the first request resolves. + await selectProject(teamProjects[1].name); + await waitAllPromises(); + + vi.clearAllMocks(); + + // Let the stale (forbidden) request resolve now that a newer selection is active. + resolveForbiddenFetch(); + await waitAllPromises(); + + expect(showError).not.toHaveBeenCalled(); + expect(insightsStore.charts.execute).not.toHaveBeenCalledWith( + 0, + expect.objectContaining({ projectId: undefined }), + ); + }); }); describe('Default date range initialization', () => { diff --git a/packages/frontend/editor-ui/src/features/execution/insights/components/InsightsDashboard.vue b/packages/frontend/editor-ui/src/features/execution/insights/components/InsightsDashboard.vue index 925c808413e..2fd675560c2 100644 --- a/packages/frontend/editor-ui/src/features/execution/insights/components/InsightsDashboard.vue +++ b/packages/frontend/editor-ui/src/features/execution/insights/components/InsightsDashboard.vue @@ -10,7 +10,9 @@ import { useInsightsStore } from '@/features/execution/insights/insights.store'; import type { DateValue } from '@internationalized/date'; import { getLocalTimeZone, parseDate, today } from '@internationalized/date'; import type { InsightsSummaryType } from '@n8n/api-types'; +import { useToast } from '@n8n/composables/useToast'; import { useI18n } from '@n8n/i18n'; +import { ResponseError } from '@n8n/rest-api-client/utils'; import { computed, defineAsyncComponent, @@ -61,6 +63,7 @@ const props = defineProps<{ const route = useRoute(); const i18n = useI18n(); +const toast = useToast(); const insightsStore = useInsightsStore(); const projectsStore = useProjectsStore(); @@ -167,33 +170,47 @@ const fetchPaginatedTableData = ({ }); }; +let latestFetchId = 0; + watch( () => [props.insightType, selectedProject.value, range.value], - () => { + async () => { + const fetchId = ++latestFetchId; + sortTableBy.value = [{ id: props.insightType, desc: true }]; const { startDate, endDate } = getFilteredRange(); + const projectId = selectedProject.value?.id; if (insightsStore.isSummaryEnabled) { - void insightsStore.summary.execute(0, { - startDate, - endDate, - projectId: selectedProject.value?.id, - }); + void insightsStore.summary.execute(0, { startDate, endDate, projectId }); } - void insightsStore.charts.execute(0, { - startDate, - endDate, - projectId: selectedProject.value?.id, - }); + const chartsPromise = insightsStore.charts.execute(0, { startDate, endDate, projectId }); if (insightsStore.isDashboardEnabled) { fetchPaginatedTableData({ sortBy: sortTableBy.value, - projectId: selectedProject.value?.id, + projectId, }); } + + await chartsPromise; + + // A newer selection/range change has superseded this request. + if (fetchId !== latestFetchId) { + return; + } + + // Callers may receive HTTP 403 if they have no `workflow:read` permission for a project. + // Revert to "All projects" instead of leaving the dashboard stuck on an all-zero, misleading state. + const chartsError = insightsStore.charts.error; + if (projectId && chartsError instanceof ResponseError && chartsError.httpStatusCode === 403) { + toast.showError(chartsError, i18n.baseText('insights.dashboard.error.forbidden.title'), { + message: i18n.baseText('insights.dashboard.error.forbidden.message'), + }); + selectedProject.value = null; + } }, { immediate: true, diff --git a/packages/nodes-base/nodes/Brevo/GenericFunctions.ts b/packages/nodes-base/nodes/Brevo/GenericFunctions.ts index 6977d0dff37..38918f1cb6d 100644 --- a/packages/nodes-base/nodes/Brevo/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Brevo/GenericFunctions.ts @@ -131,7 +131,11 @@ export namespace BrevoNode { } function validateEmailStrings(input: ValidEmailFields): ValidatedEmail { - const composer = new MailComposer({ ...input }); + const composer = new MailComposer({ + ...input, + disableFileAccess: true, + disableUrlAccess: true, + }); const addressFields = composer.compile().getAddresses(); const fieldFetcher = new Map Email[] | Email>([ diff --git a/packages/nodes-base/nodes/Brevo/test/GenericFunctions.test.ts b/packages/nodes-base/nodes/Brevo/test/GenericFunctions.test.ts index d0df5a70fde..34efc1f0c61 100644 --- a/packages/nodes-base/nodes/Brevo/test/GenericFunctions.test.ts +++ b/packages/nodes-base/nodes/Brevo/test/GenericFunctions.test.ts @@ -2,6 +2,26 @@ import type { IBinaryData, IExecuteSingleFunctions, IHttpRequestOptions } from ' import { BrevoNode } from '../GenericFunctions'; +const { mailComposerOptions } = vi.hoisted(() => ({ + mailComposerOptions: vi.fn(), +})); + +vi.mock('nodemailer/lib/mail-composer', () => ({ + default: class MailComposer { + constructor(options: unknown) { + mailComposerOptions(options); + } + + compile() { + return { + getAddresses: () => ({ + to: [{ address: 'recipient@example.com' }], + }), + }; + } + }, +})); + type AttachmentEntry = { content: string; name: string }; function makeContext(overrides: { @@ -37,6 +57,29 @@ function makeContext(overrides: { } as unknown as IExecuteSingleFunctions; } +describe('Brevo - email validation', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('applies content access restrictions when validating addresses', async () => { + const context = { + getNodeParameter: vi.fn().mockReturnValue('recipient@example.com'), + } as unknown as IExecuteSingleFunctions; + + await BrevoNode.Validators.validateAndCompileRecipientEmails.call(context, { + url: '', + body: {}, + }); + + expect(mailComposerOptions).toHaveBeenCalledWith({ + to: 'recipient@example.com', + disableFileAccess: true, + disableUrlAccess: true, + }); + }); +}); + describe('Brevo - validateAndCompileAttachmentsData', () => { const validate = BrevoNode.Validators.validateAndCompileAttachmentsData; diff --git a/packages/nodes-base/nodes/Elastic/Elasticsearch/Elasticsearch.node.ts b/packages/nodes-base/nodes/Elastic/Elasticsearch/Elasticsearch.node.ts index 44a78d1eef4..b423ee0163c 100644 --- a/packages/nodes-base/nodes/Elastic/Elasticsearch/Elasticsearch.node.ts +++ b/packages/nodes-base/nodes/Elastic/Elasticsearch/Elasticsearch.node.ts @@ -7,7 +7,9 @@ import type { INodeTypeDescription, JsonObject, } from 'n8n-workflow'; -import { NodeConnectionTypes, jsonParse, NodeApiError } from 'n8n-workflow'; +import { NodeConnectionTypes, NodeApiError } from 'n8n-workflow'; + +import { parseAndResolveQueryParameters } from '@utils/query-parameters'; import { documentFields, documentOperations, indexFields, indexOperations } from './descriptions'; import { @@ -146,11 +148,16 @@ export class Elasticsearch implements INodeType { // const paginate = this.getNodeParameter('paginate', i) as boolean; if (Object.keys(options).length) { - const { query, ...rest } = options; + const { query, queryParameters, ...rest } = options; if (query) { Object.assign( body, - jsonParse(query, { errorMessage: "Invalid JSON in 'Query' option" }), + parseAndResolveQueryParameters( + query, + queryParameters ?? '[]', + this.getNode(), + i, + ) as IDataObject, ); } Object.assign(qs, rest); diff --git a/packages/nodes-base/nodes/Elastic/Elasticsearch/descriptions/DocumentDescription.ts b/packages/nodes-base/nodes/Elastic/Elasticsearch/descriptions/DocumentDescription.ts index 3a446243ace..d9f4e97abc6 100644 --- a/packages/nodes-base/nodes/Elastic/Elasticsearch/descriptions/DocumentDescription.ts +++ b/packages/nodes-base/nodes/Elastic/Elasticsearch/descriptions/DocumentDescription.ts @@ -395,13 +395,28 @@ export const documentFields: INodeProperties[] = [ displayName: 'Query', name: 'query', description: - 'Query in the Elasticsearch Query DSL', + 'Query in the Elasticsearch Query DSL. Use $1, $2, and so on as complete values to reference Query Parameters below.', type: 'json', typeOptions: { alwaysOpenEditWindow: true, }, default: '', placeholder: placeholders.query, + hint: 'Use query parameters for dynamic values instead of embedding expressions in the query', + }, + { + displayName: 'Query Parameters', + name: 'queryParameters', + description: + 'Array of values to use for $1, $2, and so on, in order. Values can be strings, numbers, booleans, null, or arrays of these values.', + type: 'json', + typeOptions: { + rows: 2, + }, + default: '={{ [] }}', + placeholder: '{{ [$json.name, 30] }}', + validateType: 'array', + hint: 'Build the array inside the expression, for example {{ [$json.name, 30] }} sets $1 to the name and $2 to 30', }, { displayName: 'Request Cache', diff --git a/packages/nodes-base/nodes/Elastic/Elasticsearch/descriptions/placeholders.ts b/packages/nodes-base/nodes/Elastic/Elasticsearch/descriptions/placeholders.ts index bda913525a9..75f1acc6447 100644 --- a/packages/nodes-base/nodes/Elastic/Elasticsearch/descriptions/placeholders.ts +++ b/packages/nodes-base/nodes/Elastic/Elasticsearch/descriptions/placeholders.ts @@ -30,7 +30,7 @@ export const aliases = `{ export const query = `{ "query": { "term": { - "user.id": "john" + "user.id": "$1" } } }`; diff --git a/packages/nodes-base/nodes/Elastic/Elasticsearch/tests/document-getAll-queryParameters.workflow.json b/packages/nodes-base/nodes/Elastic/Elasticsearch/tests/document-getAll-queryParameters.workflow.json new file mode 100644 index 00000000000..c5c1eaa124c --- /dev/null +++ b/packages/nodes-base/nodes/Elastic/Elasticsearch/tests/document-getAll-queryParameters.workflow.json @@ -0,0 +1,69 @@ +{ + "name": "Elasticsearch Document GetAll Query Parameters Test", + "nodes": [ + { + "parameters": {}, + "type": "n8n-nodes-base.manualTrigger", + "typeVersion": 1, + "position": [0, 0], + "id": "trigger-id", + "name": "When clicking 'Execute Workflow'" + }, + { + "parameters": { + "resource": "document", + "operation": "getAll", + "indexId": "my-index", + "returnAll": false, + "limit": 10, + "simple": false, + "options": { + "query": "{\"query\": {\"term\": {\"user.id\": \"$1\"}}}", + "queryParameters": "={{ ['john\", \"boost\": \"2'] }}" + } + }, + "type": "n8n-nodes-base.elasticsearch", + "typeVersion": 1, + "position": [200, 0], + "id": "elasticsearch-getAll", + "name": "Elasticsearch", + "credentials": { + "elasticsearchApi": { + "id": "elasticsearch-cred-id", + "name": "Elasticsearch account" + } + } + } + ], + "pinData": { + "Elasticsearch": [ + { + "json": { + "_index": "my-index", + "_id": "doc1", + "_score": 1, + "_source": { + "user.id": "john" + } + } + } + ] + }, + "connections": { + "When clicking 'Execute Workflow'": { + "main": [ + [ + { + "node": "Elasticsearch", + "type": "main", + "index": 0 + } + ] + ] + } + }, + "active": false, + "settings": { + "executionOrder": "v1" + } +} diff --git a/packages/nodes-base/nodes/Elastic/Elasticsearch/tests/document.getAll.test.ts b/packages/nodes-base/nodes/Elastic/Elasticsearch/tests/document.getAll.test.ts new file mode 100644 index 00000000000..7c5a8980a3f --- /dev/null +++ b/packages/nodes-base/nodes/Elastic/Elasticsearch/tests/document.getAll.test.ts @@ -0,0 +1,41 @@ +import { NodeTestHarness } from '@nodes-testing/node-test-harness'; +import nock from 'nock'; + +describe('Elasticsearch', () => { + const credentials = { + elasticsearchApi: { + username: 'user', + password: 'password', + baseUrl: 'https://elastic.local', + ignoreSSLIssues: false, + }, + }; + + beforeAll(() => { + // The interceptor only matches if the expression-supplied value lands as a single + // plain string value in the body, quotes and all, and if `queryParameters` is kept + // out of the request's query string. + nock('https://elastic.local') + .post('/my-index/_search', { + query: { term: { 'user.id': 'john", "boost": "2' } }, + }) + .query({ _source: 'true', size: '10' }) + .reply(200, { + hits: { + hits: [ + { + _index: 'my-index', + _id: 'doc1', + _score: 1, + _source: { 'user.id': 'john' }, + }, + ], + }, + }); + }); + + new NodeTestHarness().setupTests({ + credentials, + workflowFiles: ['document-getAll-queryParameters.workflow.json'], + }); +}); diff --git a/packages/nodes-base/nodes/Elastic/Elasticsearch/types.ts b/packages/nodes-base/nodes/Elastic/Elasticsearch/types.ts index 05c6c58d307..388f281cef5 100644 --- a/packages/nodes-base/nodes/Elastic/Elasticsearch/types.ts +++ b/packages/nodes-base/nodes/Elastic/Elasticsearch/types.ts @@ -18,6 +18,7 @@ export type DocumentGetAllOptions = Partial<{ max_concurrent_shard_requests: number; pre_filter_shard_size: number; query: string; + queryParameters: unknown; request_cache: boolean; routing: string; search_type: 'query_then_fetch' | 'dfs_query_then_fetch'; diff --git a/packages/nodes-base/nodes/Git/GenericFunctions.ts b/packages/nodes-base/nodes/Git/GenericFunctions.ts index 21a6aef01a3..0298d576914 100644 --- a/packages/nodes-base/nodes/Git/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Git/GenericFunctions.ts @@ -2,6 +2,70 @@ import type { INode } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; import type { ConfigListSummary } from 'simple-git'; +const FILTER_COMMAND_CONFIG_KEY_PATTERN = /^filter\.(.*)\.(?:clean|smudge|process)$/i; + +const MERGE_DRIVER_CONFIG_KEY_PATTERN = /^merge\.(.*)\.driver$/i; + +const REMOTE_PACK_COMMAND_CONFIG_KEY_PATTERN = /^remote\.(.*)\.(?:uploadpack|receivepack)$/i; + +const GPG_FORMAT_PROGRAM_CONFIG_KEY_PATTERN = /^gpg\.(.*)\.program$/i; + +const CORE_ASKPASS_CONFIG_KEY_PATTERN = /^core\.askpass$/i; + +const CORE_EDITOR_CONFIG_KEY_PATTERN = /^core\.editor$/i; + +const CORE_ALTERNATE_REFS_COMMAND_CONFIG_KEY_PATTERN = /^core\.alternaterefscommand$/i; + +const GC_RECENT_OBJECTS_HOOK_CONFIG_KEY_PATTERN = /^gc\.recentobjectshook$/i; + +const CONFIGURED_HOOK_COMMAND_CONFIG_KEY_PATTERN = /^hook\.(.*)\.command$/i; + +const SEQUENCE_EDITOR_CONFIG_KEY_PATTERN = /^sequence\.editor$/i; + +const GPG_SSH_DEFAULT_KEY_COMMAND_CONFIG_KEY_PATTERN = /^gpg\.ssh\.defaultkeycommand$/i; + +const URL_REWRITE_CONFIG_KEY_PATTERN = /^url\.(.*)\.(?:insteadof|pushinsteadof)$/i; + +const KEY_BLACKLIST = [ + FILTER_COMMAND_CONFIG_KEY_PATTERN, + MERGE_DRIVER_CONFIG_KEY_PATTERN, + REMOTE_PACK_COMMAND_CONFIG_KEY_PATTERN, + GPG_FORMAT_PROGRAM_CONFIG_KEY_PATTERN, + CORE_ASKPASS_CONFIG_KEY_PATTERN, + CORE_EDITOR_CONFIG_KEY_PATTERN, + CORE_ALTERNATE_REFS_COMMAND_CONFIG_KEY_PATTERN, + GC_RECENT_OBJECTS_HOOK_CONFIG_KEY_PATTERN, + CONFIGURED_HOOK_COMMAND_CONFIG_KEY_PATTERN, + SEQUENCE_EDITOR_CONFIG_KEY_PATTERN, + GPG_SSH_DEFAULT_KEY_COMMAND_CONFIG_KEY_PATTERN, + URL_REWRITE_CONFIG_KEY_PATTERN, +]; + +export function findBlacklistedKeys( + config: ConfigListSummary, + localConfigFiles: string[], +): string[] { + const localConfigFileSet = new Set(localConfigFiles); + const localConfigIndex = config.files.findIndex((file) => localConfigFileSet.has(file)); + if (localConfigIndex === -1) { + return []; + } + + // Scan config sources from repository-local config onward. + const repositoryConfigFiles = config.files.slice(localConfigIndex); + const forbiddenKeys = new Set(); + + for (const file of repositoryConfigFiles) { + for (const key of Object.keys(config.values[file] ?? {})) { + if (KEY_BLACKLIST.some((pattern) => pattern.test(key))) { + forbiddenKeys.add(key); + } + } + } + + return Array.from(forbiddenKeys); +} + /** * Shared safeguards for git references: block argument injection, path * traversal, and control characters. The caller supplies the allowed-character diff --git a/packages/nodes-base/nodes/Git/Git.node.ts b/packages/nodes-base/nodes/Git/Git.node.ts index ca384bda6d2..52b02b0cc24 100644 --- a/packages/nodes-base/nodes/Git/Git.node.ts +++ b/packages/nodes-base/nodes/Git/Git.node.ts @@ -16,7 +16,7 @@ import { import { randomBytes } from 'node:crypto'; import { mkdir, rename, rm } from 'node:fs/promises'; import { basename, dirname, isAbsolute, join, resolve } from 'path'; -import type { LogOptions, SimpleGit, SimpleGitOptions } from 'simple-git'; +import type { ConfigListSummary, LogOptions, SimpleGit, SimpleGitOptions } from 'simple-git'; import simpleGit from 'simple-git'; import { URL, fileURLToPath } from 'url'; @@ -35,6 +35,7 @@ import { import { type ConfiguredRemoteRepositories, getConfiguredRemoteRepositories, + findBlacklistedKeys, getRepositoryTypeForRemoteConfigKey, mapGitConfigList, validateGitReference, @@ -411,11 +412,10 @@ export class Git implements INodeType { }; const validateConfiguredRemoteRepositories = async ( - git: SimpleGit, repositoryType: 'source' | 'target', baseDir: string, + config: ConfigListSummary, ): Promise => { - const config = await git.listConfig(); const remoteRepositories = getConfiguredRemoteRepositories(config.values, this.getNode()); const validationTargets = repositoryType === 'source' @@ -540,7 +540,7 @@ export class Git implements INodeType { ...(Object.keys(unsafe).length > 0 && { unsafe }), }; - const cleanEnv = Object.create(null) as Record; + const cleanEnv = Object.create(null) as Record; const isWriteOperation = operation === 'push' || operation === 'pushTags'; // Tell git not to ask for any information via the terminal like for // example the username. As nobody will be able to answer it would @@ -550,6 +550,25 @@ export class Git implements INodeType { isWriteOperation && !enableHooks ? 'git:http:https:ssh' : 'file:git:http:https:ssh'; const git: SimpleGit = simpleGit(gitOptions).env(cleanEnv); + const validateGitConfig = async (): Promise => { + const repositoryConfig = await git.listConfig(); + if (securityConfig.enableGitNodeAllConfigKeys) { + return repositoryConfig; + } + + const localConfig = await git.listConfig('local'); + + const blacklistedConfigKeys = findBlacklistedKeys(repositoryConfig, localConfig.files); + if (blacklistedConfigKeys.length > 0) { + throw new NodeOperationError( + this.getNode(), + `Repository Git config key '${blacklistedConfigKeys[0]}' is not allowed`, + ); + } + + return repositoryConfig; + }; + if (operation === 'add') { // ---------------------------------- // add @@ -562,6 +581,7 @@ export class Git implements INodeType { .filter((p) => p.length > 0); // Use -- separator to prevent argument injection + await validateGitConfig(); await git.add(['--', ...paths]); returnItems.push({ @@ -658,6 +678,7 @@ export class Git implements INodeType { // ---------------------------------- const message = this.getNodeParameter('message', itemIndex, '') as string; + await validateGitConfig(); const branch = options.branch; if (branch !== undefined && branch !== '') { assertParamIsString('branch', branch, this.getNode()); @@ -694,8 +715,12 @@ export class Git implements INodeType { // ---------------------------------- // fetch // ---------------------------------- - - await validateConfiguredRemoteRepositories(git, 'source', resolvedRepositoryPath); + const repositoryConfig = await validateGitConfig(); + await validateConfiguredRemoteRepositories( + 'source', + resolvedRepositoryPath, + repositoryConfig, + ); await git.fetch(); returnItems.push({ json: { @@ -736,7 +761,12 @@ export class Git implements INodeType { // pull // ---------------------------------- - await validateConfiguredRemoteRepositories(git, 'source', resolvedRepositoryPath); + const repositoryConfig = await validateGitConfig(); + await validateConfiguredRemoteRepositories( + 'source', + resolvedRepositoryPath, + repositoryConfig, + ); await git.pull(); returnItems.push({ json: { @@ -751,6 +781,7 @@ export class Git implements INodeType { // push // ---------------------------------- + const repositoryConfig = await validateGitConfig(); const branch = options.branch; if (branch !== undefined && branch !== '') { assertParamIsString('branch', branch, this.getNode()); @@ -770,9 +801,9 @@ export class Git implements INodeType { } else { const authentication = this.getNodeParameter('authentication', 0) as string; const { pushTarget } = await validateConfiguredRemoteRepositories( - git, 'target', resolvedRepositoryPath, + repositoryConfig, ); if (authentication === 'gitPassword') { @@ -799,8 +830,12 @@ export class Git implements INodeType { // ---------------------------------- // pushTags // ---------------------------------- - - await validateConfiguredRemoteRepositories(git, 'target', resolvedRepositoryPath); + const repositoryConfig = await validateGitConfig(); + await validateConfiguredRemoteRepositories( + 'target', + resolvedRepositoryPath, + repositoryConfig, + ); await git.pushTags(); returnItems.push({ json: { @@ -883,6 +918,7 @@ export class Git implements INodeType { // status // ---------------------------------- + await validateGitConfig(); const status = await git.status(); returnItems.push( @@ -926,6 +962,7 @@ export class Git implements INodeType { assertParamIsBoolean('force', force, this.getNode()); } + await validateGitConfig(); await checkoutBranch(git, { branchName, createBranch, @@ -952,6 +989,7 @@ export class Git implements INodeType { const name = this.getNodeParameter('name', itemIndex, '') as string; validateGitTag(name, this.getNode()); + await validateGitConfig(); await git.addTag(name); returnItems.push({ json: { diff --git a/packages/nodes-base/nodes/Git/__test__/GenericFunctions.test.ts b/packages/nodes-base/nodes/Git/__test__/GenericFunctions.test.ts index 285beaa6525..546a05a005b 100644 --- a/packages/nodes-base/nodes/Git/__test__/GenericFunctions.test.ts +++ b/packages/nodes-base/nodes/Git/__test__/GenericFunctions.test.ts @@ -2,9 +2,86 @@ import { mockDeep } from 'vitest-mock-extended'; import type { ConfigListSummary } from 'simple-git'; import type { INode } from 'n8n-workflow'; -import { getConfiguredRemoteRepositories, mapGitConfigList } from '../GenericFunctions'; +import { + getConfiguredRemoteRepositories, + findBlacklistedKeys, + mapGitConfigList, +} from '../GenericFunctions'; describe('GenericFunctions', () => { + describe('findBlacklistedKeys', () => { + it('should reject filter and merge commands from repository config only', () => { + const config = mockDeep({ + files: ['global-config', '.git/config', 'command line:'], + values: { + 'global-config': { 'filter.lfs.process': 'git-lfs filter-process' }, + '.git/config': { + 'filter.poc.clean': 'command', + 'merge.poc.driver': 'command', + }, + 'command line:': { 'core.sshcommand': 'ssh' }, + }, + }); + + const result = findBlacklistedKeys(config, ['.git/config']); + + expect(result).toEqual(['filter.poc.clean', 'merge.poc.driver']); + }); + + it('should reject other command-bearing repository config keys', () => { + const blacklistedKeys = [ + 'core.askPass', + 'core.editor', + 'core.alternateRefsCommand', + 'gc.recentObjectsHook', + 'hook.pre-commit.command', + 'sequence.editor', + 'remote.origin.uploadpack', + 'remote.origin.receivepack', + 'gpg.openpgp.program', + 'gpg.x509.program', + 'gpg.ssh.program', + 'gpg.ssh.defaultKeyCommand', + ]; + const config = mockDeep({ + files: ['.git/config'], + values: { + '.git/config': Object.fromEntries(blacklistedKeys.map((key) => [key, 'command'])), + }, + }); + + expect(findBlacklistedKeys(config, ['.git/config'])).toEqual(blacklistedKeys); + }); + + it('should include config files included from repository config', () => { + const config = mockDeep({ + files: ['global-config', '.git/config', 'included-config', 'command line:'], + values: { + 'global-config': {}, + '.git/config': { 'include.path': 'included-config' }, + 'included-config': { 'FILTER.POC.PROCESS': 'command' }, + 'command line:': {}, + }, + }); + + const result = findBlacklistedKeys(config, ['.git/config']); + + expect(result).toEqual(['FILTER.POC.PROCESS']); + }); + + it('should return no keys when repository config origin is absent', () => { + const config = mockDeep({ + files: ['global-config', 'command line:'], + values: { + 'global-config': { 'filter.lfs.clean': 'git-lfs clean' }, + 'command line:': {}, + }, + }); + + expect(findBlacklistedKeys(config, ['.git/config'])).toEqual([]); + }); + }); + describe('mapGitConfigList', () => { it('should map the git config list', () => { const config = mockDeep({ diff --git a/packages/nodes-base/nodes/Git/test/Git.node.security.test.ts b/packages/nodes-base/nodes/Git/test/Git.node.security.test.ts index 20c7ad07354..6247ce1221d 100644 --- a/packages/nodes-base/nodes/Git/test/Git.node.security.test.ts +++ b/packages/nodes-base/nodes/Git/test/Git.node.security.test.ts @@ -1,6 +1,6 @@ import { DeploymentConfig, SecurityConfig } from '@n8n/config'; import { Container } from '@n8n/di'; -import type { IExecuteFunctions, ResolvedFilePath } from 'n8n-workflow'; +import type { IExecuteFunctions, NodeParameterValueType, ResolvedFilePath } from 'n8n-workflow'; import { execFileSync } from 'node:child_process'; import { existsSync, writeFileSync } from 'node:fs'; import { mkdtemp, rm } from 'node:fs/promises'; @@ -22,8 +22,13 @@ describe('Git Node command-config handling', () => { let gitNode: Git; let repoDir: string; let marker: string; + let additionalDirs: string[]; - const buildContext = (operation: string, repositoryPath: string): Mocked => { + const buildContext = ( + operation: string, + repositoryPath: string, + parameters: Record = {}, + ): Mocked => { const ctx = mock({ getInputData: vi.fn(() => [{ json: {} }]), // Swallow the expected network failure so we can assert on the side effect. @@ -36,15 +41,22 @@ describe('Git Node command-config handling', () => { }, }); ctx.getNodeParameter.mockImplementation( - (name: string, _itemIndex: number, fallbackValue?: unknown) => { + ( + name: string, + _itemIndex: number, + fallbackValue?: NodeParameterValueType, + ): NodeParameterValueType | object => { switch (name) { case 'operation': return operation; case 'repositoryPath': return repositoryPath; case 'options': - return {}; + return parameters.options ?? {}; default: + if (Object.hasOwn(parameters, name)) { + return parameters[name]; + } return fallbackValue ?? ''; } }, @@ -65,15 +77,25 @@ describe('Git Node command-config handling', () => { ); repoDir = await mkdtemp(join(tmpdir(), 'n8n-git-cfg-')); marker = join(repoDir, 'command-ran'); - execFileSync('git', ['init', '-q', repoDir]); + additionalDirs = []; + execFileSync('git', ['init', '-q', '-b', 'main', repoDir]); gitConfig(repoDir, 'user.email', 'test@example.com'); gitConfig(repoDir, 'user.name', 'Test'); }); afterEach(async () => { - await rm(repoDir, { recursive: true, force: true }); + await Promise.all( + [repoDir, ...additionalDirs].map( + async (dir) => await rm(dir, { recursive: true, force: true }), + ), + ); }); + const git = (...args: string[]) => execFileSync('git', ['-C', repoDir, ...args]); + + const markerCommand = () => + `node -e "require('node:fs').writeFileSync(process.argv[1], '')" ${JSON.stringify(marker)}`; + it('does not run command-bearing repo-local git config on fetch', async () => { // A repository-local sshCommand that git would otherwise run when talking to an // ssh remote, plus an ssh remote to trigger it. @@ -117,4 +139,104 @@ describe('Git Node command-config handling', () => { // status succeeded (no error), proving git ran and would have queried fsmonitor. expect((result[0][0].json as { error?: unknown }).error).toBeUndefined(); }); + + it('rejects a repo-local clean filter before add', async () => { + writeFileSync(join(repoDir, '.gitattributes'), '*.txt filter=poc\n'); + writeFileSync(join(repoDir, 'payload.txt'), 'content'); + gitConfig(repoDir, 'filter.poc.clean', `${markerCommand()}; cat`); + + const result = await gitNode.execute.call( + buildContext('add', repoDir, { pathsToAdd: 'payload.txt' }), + ); + + expect(existsSync(marker)).toBe(false); + expect((result[0][0].json as { error?: unknown }).error).toBeDefined(); + }); + + it('rejects a clean filter included from repository config', async () => { + const includedConfig = join(repoDir, 'included-config'); + writeFileSync(join(repoDir, '.gitattributes'), '*.txt filter=poc\n'); + writeFileSync(join(repoDir, 'payload.txt'), 'content'); + execFileSync('git', [ + 'config', + '--file', + includedConfig, + 'filter.poc.clean', + `${markerCommand()}; cat`, + ]); + gitConfig(repoDir, 'include.path', includedConfig); + + const result = await gitNode.execute.call( + buildContext('add', repoDir, { pathsToAdd: 'payload.txt' }), + ); + + expect(existsSync(marker)).toBe(false); + expect((result[0][0].json as { error?: unknown }).error).toBeDefined(); + }); + + it('rejects a repo-local smudge filter before switching branch', async () => { + writeFileSync(join(repoDir, '.gitattributes'), '*.txt filter=poc\n'); + writeFileSync(join(repoDir, 'payload.txt'), 'main'); + git('add', '.'); + git('commit', '-q', '-m', 'main'); + git('checkout', '-q', '-b', 'other'); + writeFileSync(join(repoDir, 'payload.txt'), 'other'); + git('commit', '-q', '-am', 'other'); + git('checkout', '-q', 'main'); + gitConfig(repoDir, 'filter.poc.smudge', `${markerCommand()}; cat`); + + const result = await gitNode.execute.call( + buildContext('switchBranch', repoDir, { + branchName: 'other', + options: { createBranch: false }, + }), + ); + + expect(existsSync(marker)).toBe(false); + expect((result[0][0].json as { error?: unknown }).error).toBeDefined(); + }); + + it('rejects a repo-local process filter before add', async () => { + writeFileSync(join(repoDir, '.gitattributes'), '*.txt filter=poc\n'); + writeFileSync(join(repoDir, 'payload.txt'), 'content'); + gitConfig(repoDir, 'filter.poc.process', markerCommand()); + + const result = await gitNode.execute.call( + buildContext('add', repoDir, { pathsToAdd: 'payload.txt' }), + ); + + expect(existsSync(marker)).toBe(false); + expect((result[0][0].json as { error?: unknown }).error).toBeDefined(); + }); + + it('rejects a repo-local merge driver before pull', async () => { + writeFileSync(join(repoDir, '.gitattributes'), '*.txt merge=poc\n'); + writeFileSync(join(repoDir, 'payload.txt'), 'base'); + git('add', '.'); + git('commit', '-q', '-m', 'base'); + + const remoteDir = await mkdtemp(join(tmpdir(), 'n8n-git-remote-')); + const otherDir = await mkdtemp(join(tmpdir(), 'n8n-git-other-')); + additionalDirs.push(remoteDir, otherDir); + execFileSync('git', ['init', '--bare', '-q', '-b', 'main', remoteDir]); + git('remote', 'add', 'origin', remoteDir); + git('push', '-q', '-u', 'origin', 'main'); + + execFileSync('git', ['clone', '-q', remoteDir, otherDir]); + gitConfig(otherDir, 'user.email', 'test@example.com'); + gitConfig(otherDir, 'user.name', 'Test'); + writeFileSync(join(otherDir, 'payload.txt'), 'remote'); + execFileSync('git', ['-C', otherDir, 'commit', '-q', '-am', 'remote']); + execFileSync('git', ['-C', otherDir, 'push', '-q']); + + writeFileSync(join(repoDir, 'payload.txt'), 'local'); + git('commit', '-q', '-am', 'local'); + gitConfig(repoDir, 'pull.rebase', 'false'); + gitConfig(repoDir, 'merge.poc.driver', `${markerCommand()}; exit 1`); + + const result = await gitNode.execute.call(buildContext('pull', repoDir)); + + expect(existsSync(marker)).toBe(false); + expect((result[0][0].json as { error?: unknown }).error).toBeDefined(); + }); }); diff --git a/packages/nodes-base/nodes/Git/test/Git.node.test.ts b/packages/nodes-base/nodes/Git/test/Git.node.test.ts index 78be8b07a6e..d81a345b903 100644 --- a/packages/nodes-base/nodes/Git/test/Git.node.test.ts +++ b/packages/nodes-base/nodes/Git/test/Git.node.test.ts @@ -105,8 +105,13 @@ describe('Git Node', () => { } }, ); - mockGit.listConfig.mockResolvedValue({ values: {} } as any); + mockGit.listConfig.mockResolvedValue({ + files: ['.git/config', 'command line:'], + values: { '.git/config': {}, 'command line:': {} }, + all: {}, + } as any); mockGit.log.mockResolvedValue({ all: [] } as any); + mockGit.raw.mockResolvedValue(''); mockMkdir.mockResolvedValue(undefined); mockRename.mockResolvedValue(undefined); mockRm.mockResolvedValue(undefined); @@ -439,6 +444,25 @@ describe('Git Node', () => { expect(mockGit.push).not.toHaveBeenCalled(); }); + it('should validate repository config before pushing to a custom target', async () => { + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce('push') + .mockReturnValueOnce('/repo') + .mockReturnValueOnce({ + repository: true, + targetRepository: 'https://github.com/example/repo.git', + }); + mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], + values: { '.git/config': { 'core.askPass': 'command' } }, + } as any); + + await expect(gitNode.execute.call(mockExecuteFunctions)).rejects.toThrow( + "Repository Git config key 'core.askPass' is not allowed", + ); + expect(mockGit.push).not.toHaveBeenCalled(); + }); + it('should reject push target repositories starting with a hyphen', async () => { mockExecuteFunctions.getNodeParameter .mockReturnValueOnce('push') @@ -464,6 +488,7 @@ describe('Git Node', () => { // Mock git config for push operation mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': 'https://github.com/test/repo.git' } }, } as any); @@ -480,6 +505,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}) .mockReturnValueOnce('gitPassword'); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': '/blocked/target-repo' } }, } as any); mockExecuteFunctions.helpers.isFilePathBlocked = vi.fn( @@ -500,6 +526,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}) .mockReturnValueOnce('none'); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': '/blocked/target-repo' } }, } as any); mockExecuteFunctions.helpers.isFilePathBlocked = vi.fn( @@ -520,6 +547,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}) .mockReturnValueOnce('none'); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': 'https://github.com/test/repo.git', @@ -572,6 +600,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}) .mockReturnValueOnce('none'); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': [ @@ -596,6 +625,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}) .mockReturnValueOnce('none'); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': 'https://github.com/test/repo.git', @@ -621,6 +651,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}) .mockReturnValueOnce('none'); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config', '.git/config.worktree'], values: { '.git/config': { 'remote.origin.url': 'https://github.com/test/repo.git', @@ -1043,6 +1074,7 @@ describe('Git Node', () => { .mockReturnValueOnce('/repo') .mockReturnValueOnce({}); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': 'https://github.com/test/repo.git', @@ -1070,6 +1102,7 @@ describe('Git Node', () => { .mockReturnValueOnce('/repo') .mockReturnValueOnce({}); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': 'https://github.com/test/repo.git', @@ -1098,6 +1131,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}) .mockReturnValueOnce('none'); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': 'https://github.com/test/repo.git', @@ -1123,6 +1157,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}) .mockReturnValueOnce('none'); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'remote.origin.url': '/outside/source-repo', @@ -1612,6 +1647,7 @@ describe('Git Node', () => { .mockReturnValueOnce({}); mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], values: { '.git/config': { 'user.name': 'test' } }, } as any); @@ -1782,6 +1818,23 @@ invalid line without proper format`; expect(mockGit.addTag).toHaveBeenCalledWith('v1.2.3+build.1'); }); + it('should reject signing program config before creating a tag', async () => { + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce('tag') + .mockReturnValueOnce('/repo') + .mockReturnValueOnce({}) + .mockReturnValueOnce('v1.0.0'); + mockGit.listConfig.mockResolvedValueOnce({ + files: ['.git/config'], + values: { '.git/config': { 'gpg.ssh.program': 'command' } }, + } as any); + + await expect(gitNode.execute.call(mockExecuteFunctions)).rejects.toThrow( + "Repository Git config key 'gpg.ssh.program' is not allowed", + ); + expect(mockGit.addTag).not.toHaveBeenCalled(); + }); + it('should reject tag operation when name starts with a hyphen', async () => { mockExecuteFunctions.getNodeParameter .mockReturnValueOnce('tag') @@ -1846,7 +1899,7 @@ invalid line without proper format`; }); }); - describe('Command config neutralization', () => { + describe('Command config protection', () => { const expectedOverrides = [ 'core.sshCommand=ssh', 'core.fsmonitor=false', @@ -1883,6 +1936,73 @@ invalid line without proper format`; } }); + it.each([ + 'filter.poc.clean', + 'merge.poc.driver', + 'core.askPass', + 'core.editor', + 'core.alternateRefsCommand', + 'gc.recentObjectsHook', + 'hook.pre-commit.command', + 'sequence.editor', + 'remote.origin.uploadpack', + 'remote.origin.receivepack', + 'gpg.openpgp.program', + 'gpg.ssh.defaultKeyCommand', + ])("rejects repository Git config key '%s'", async (configKey) => { + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce('add') + .mockReturnValueOnce('/repo') + .mockReturnValueOnce({}) + .mockReturnValueOnce('file.txt'); + mockGit.listConfig.mockResolvedValueOnce({ + files: ['global-config', '.git/config', 'command line:'], + values: { + 'global-config': { 'filter.lfs.process': 'git-lfs filter-process' }, + '.git/config': { [configKey]: 'command' }, + 'command line:': {}, + }, + } as any); + + await expect(gitNode.execute.call(mockExecuteFunctions)).rejects.toThrow( + `Repository Git config key '${configKey}' is not allowed`, + ); + expect(mockGit.add).not.toHaveBeenCalled(); + }); + + it.each([ + { operation: 'add', params: ['add', '/repo', {}, 'file.txt'], guard: 'add' }, + { operation: 'commit', params: ['commit', '/repo', {}, 'repro commit'], guard: 'commit' }, + { operation: 'fetch', params: ['fetch', '/repo', {}], guard: 'fetch' }, + { operation: 'pull', params: ['pull', '/repo', {}], guard: 'pull' }, + { operation: 'push', params: ['push', '/repo', {}], guard: 'push' }, + { operation: 'pushTags', params: ['pushTags', '/repo', {}], guard: 'pushTags' }, + { operation: 'status', params: ['status', '/repo', {}], guard: 'status' }, + { + operation: 'switchBranch', + params: ['switchBranch', '/repo', {}, 'main'], + guard: 'checkout', + }, + { operation: 'tag', params: ['tag', '/repo', {}, 'v1.0.0'], guard: 'addTag' }, + ])('rejects repository Git config key before $operation', async ({ params, guard }) => { + for (const value of params) { + mockExecuteFunctions.getNodeParameter.mockReturnValueOnce(value); + } + mockGit.listConfig.mockResolvedValueOnce({ + files: ['global-config', '.git/config', 'command line:'], + values: { + 'global-config': { 'filter.lfs.process': 'git-lfs filter-process' }, + '.git/config': { 'filter.poc.clean': 'command' }, + 'command line:': {}, + }, + } as any); + + await expect(gitNode.execute.call(mockExecuteFunctions)).rejects.toThrow( + "Repository Git config key 'filter.poc.clean' is not allowed", + ); + expect((mockGit as any)[guard]).not.toHaveBeenCalled(); + }); + it('does not pin command config when enableGitNodeAllConfigKeys is true', async () => { securityConfig.enableGitNodeAllConfigKeys = true; @@ -1895,6 +2015,7 @@ invalid line without proper format`; for (const flag of expectedFlags) { expect(unsafe?.[flag]).toBeUndefined(); } + expect(mockGit.listConfig).not.toHaveBeenCalled(); }); }); }); diff --git a/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/DocumentDescription.ts b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/DocumentDescription.ts index 950892afff6..8f74cd1cf2b 100644 --- a/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/DocumentDescription.ts +++ b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/DocumentDescription.ts @@ -623,10 +623,32 @@ export const documentFields: INodeProperties[] = [ operation: ['query'], }, }, - description: 'JSON query to execute', + description: + 'JSON query to execute. Use $1, $2, and so on as complete values to reference Query Parameters below.', + hint: 'Use query parameters for dynamic values instead of embedding expressions in the query', required: true, placeholder: - '{"structuredQuery": {"where": {"fieldFilter": {"field": {"fieldPath": "age"},"op": "EQUAL", "value": {"integerValue": 28}}}, "from": [{"collectionId": "users-collection"}]}}', + '{"structuredQuery": {"where": {"fieldFilter": {"field": {"fieldPath": "age"},"op": "EQUAL", "value": {"integerValue": "$1"}}}, "from": [{"collectionId": "users-collection"}]}}', + }, + { + displayName: 'Query Parameters', + name: 'queryParameters', + type: 'json', + typeOptions: { + rows: 2, + }, + default: '={{ [] }}', + displayOptions: { + show: { + resource: ['document'], + operation: ['query'], + }, + }, + description: + 'Array of values to use for $1, $2, and so on, in order. Values can be strings, numbers, booleans, null, or arrays of these values.', + hint: 'Build the array inside the expression, for example {{ [$json.name, 30] }} sets $1 to the name and $2 to 30', + placeholder: '{{ [$json.name, 30] }}', + validateType: 'array', }, { displayName: 'Simplify', diff --git a/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/GoogleFirebaseCloudFirestore.node.ts b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/GoogleFirebaseCloudFirestore.node.ts index 7f05761e374..d655ea7d2ef 100644 --- a/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/GoogleFirebaseCloudFirestore.node.ts +++ b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/GoogleFirebaseCloudFirestore.node.ts @@ -7,7 +7,9 @@ import type { INodeType, INodeTypeDescription, } from 'n8n-workflow'; -import { NodeConnectionTypes, jsonParse } from 'n8n-workflow'; +import { NodeConnectionTypes } from 'n8n-workflow'; + +import { parseAndResolveQueryParameters } from '@utils/query-parameters'; import { collectionFields, collectionOperations } from './CollectionDescription'; import { documentFields, documentOperations } from './DocumentDescription'; @@ -363,11 +365,18 @@ export class GoogleFirebaseCloudFirestore implements INodeType { await Promise.all( items.map(async (_: IDataObject, i: number) => { const query = this.getNodeParameter('query', i) as string; + const queryParameters = this.getNodeParameter('queryParameters', i, '[]'); + const body = parseAndResolveQueryParameters( + query, + queryParameters, + this.getNode(), + i, + ) as IDataObject; responseData = await googleApiRequest.call( this, 'POST', `/${projectId}/databases/${database}/documents:runQuery`, - jsonParse(query), + body, ); responseData = responseData.map( diff --git a/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/__test__/document-query-parameters.workflow.json b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/__test__/document-query-parameters.workflow.json new file mode 100644 index 00000000000..d8f8af62436 --- /dev/null +++ b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/__test__/document-query-parameters.workflow.json @@ -0,0 +1,65 @@ +{ + "name": "CloudFirestore Document Query Parameters Test", + "nodes": [ + { + "parameters": {}, + "type": "n8n-nodes-base.manualTrigger", + "typeVersion": 1, + "position": [0, 0], + "id": "trigger-id", + "name": "When clicking 'Execute Workflow'" + }, + { + "parameters": { + "authentication": "googleFirebaseCloudFirestoreOAuth2Api", + "resource": "document", + "operation": "query", + "projectId": "test-project", + "database": "(default)", + "query": "{\"structuredQuery\": {\"where\": {\"fieldFilter\": {\"field\": {\"fieldPath\": \"name\"}, \"op\": \"EQUAL\", \"value\": {\"stringValue\": \"$1\"}}}, \"from\": [{\"collectionId\": \"users\"}]}}", + "queryParameters": "={{ ['hello \"there\", \"op\": \"GREATER_THAN'] }}" + }, + "type": "n8n-nodes-base.googleFirebaseCloudFirestore", + "typeVersion": 1.1, + "position": [200, 0], + "id": "cloudFirestore-query", + "name": "Query Documents", + "credentials": { + "googleFirebaseCloudFirestoreOAuth2Api": { + "id": "cloudFirestore-oauth-id", + "name": "CloudFirestore OAuth2" + } + } + } + ], + "pinData": { + "Query Documents": [ + { + "json": { + "_name": "projects/test-project/databases/(default)/documents/users/user1", + "_id": "user1", + "_createTime": "2023-01-01T10:00:00.000Z", + "_updateTime": "2023-01-01T10:00:00.000Z", + "name": "John Doe" + } + } + ] + }, + "connections": { + "When clicking 'Execute Workflow'": { + "main": [ + [ + { + "node": "Query Documents", + "type": "main", + "index": 0 + } + ] + ] + } + }, + "active": false, + "settings": { + "executionOrder": "v1" + } +} diff --git a/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/__test__/document.query-parameters.test.ts b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/__test__/document.query-parameters.test.ts new file mode 100644 index 00000000000..999af02c369 --- /dev/null +++ b/packages/nodes-base/nodes/Google/Firebase/CloudFirestore/__test__/document.query-parameters.test.ts @@ -0,0 +1,48 @@ +import { NodeTestHarness } from '@nodes-testing/node-test-harness'; +import nock from 'nock'; + +describe('GoogleFirebaseCloudFirestore', () => { + const credentials = { + googleFirebaseCloudFirestoreOAuth2Api: { + oauthTokenData: { + access_token: 'test-access-token', + }, + }, + }; + + beforeAll(() => { + // The interceptor only matches if the expression-supplied value lands as a single + // plain string value, quotes and all, leaving the query structure untouched. + nock('https://firestore.googleapis.com') + .post('/v1/projects/test-project/databases/(default)/documents:runQuery', { + structuredQuery: { + where: { + fieldFilter: { + field: { fieldPath: 'name' }, + op: 'EQUAL', + value: { stringValue: 'hello "there", "op": "GREATER_THAN' }, + }, + }, + from: [{ collectionId: 'users' }], + }, + }) + .reply(200, [ + { + document: { + name: 'projects/test-project/databases/(default)/documents/users/user1', + id: 'user1', + fields: { + name: { stringValue: 'John Doe' }, + }, + createTime: '2023-01-01T10:00:00.000Z', + updateTime: '2023-01-01T10:00:00.000Z', + }, + }, + ]); + }); + + new NodeTestHarness().setupTests({ + credentials, + workflowFiles: ['document-query-parameters.workflow.json'], + }); +}); diff --git a/packages/nodes-base/nodes/Google/Gmail/GenericFunctions.ts b/packages/nodes-base/nodes/Google/Gmail/GenericFunctions.ts index e01338b5c28..c443602ffda 100644 --- a/packages/nodes-base/nodes/Google/Gmail/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Google/Gmail/GenericFunctions.ts @@ -240,6 +240,9 @@ export async function encodeEmail(email: IEmail) { mailOptions.attachments = attachments; } + mailOptions.disableFileAccess = true; + mailOptions.disableUrlAccess = true; + const mail = new MailComposer(mailOptions).compile(); // by default the bcc headers are deleted when the mail is built. diff --git a/packages/nodes-base/nodes/Google/Gmail/test/GenericFunctions.test.ts b/packages/nodes-base/nodes/Google/Gmail/test/GenericFunctions.test.ts new file mode 100644 index 00000000000..89981e0603f --- /dev/null +++ b/packages/nodes-base/nodes/Google/Gmail/test/GenericFunctions.test.ts @@ -0,0 +1,40 @@ +import { encodeEmail } from '../GenericFunctions'; + +const { mailComposerOptions } = vi.hoisted(() => ({ + mailComposerOptions: vi.fn(), +})); + +vi.mock('nodemailer/lib/mail-composer', () => ({ + default: class MailComposer { + constructor(options: unknown) { + mailComposerOptions(options); + } + + compile() { + return { + keepBcc: false, + build: vi.fn().mockResolvedValue(Buffer.from('email')), + }; + } + }, +})); + +describe('Gmail email encoding', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('applies content access restrictions to generated emails', async () => { + await encodeEmail({ + subject: 'Test subject', + body: 'Test body', + }); + + expect(mailComposerOptions).toHaveBeenCalledWith( + expect.objectContaining({ + disableFileAccess: true, + disableUrlAccess: true, + }), + ); + }); +}); diff --git a/packages/nodes-base/nodes/MongoDb/GenericFunctions.test.ts b/packages/nodes-base/nodes/MongoDb/GenericFunctions.test.ts index 580059d58fd..a65fc12acaa 100644 --- a/packages/nodes-base/nodes/MongoDb/GenericFunctions.test.ts +++ b/packages/nodes-base/nodes/MongoDb/GenericFunctions.test.ts @@ -1,11 +1,9 @@ import type { INode, IExecuteFunctions } from 'n8n-workflow'; import { Binary, ObjectId } from 'mongodb'; -import { - parseAndResolveQueryParameters, - prepareItems, - serializeMongoItems, -} from './GenericFunctions'; +import { parseAndResolveQueryParameters } from '@utils/query-parameters'; + +import { prepareItems, serializeMongoItems } from './GenericFunctions'; const mockNode = { name: 'MongoDB', type: 'n8n-nodes-base.mongoDb' } as INode; diff --git a/packages/nodes-base/nodes/MongoDb/GenericFunctions.ts b/packages/nodes-base/nodes/MongoDb/GenericFunctions.ts index 89b05a7e41b..fd9c27fb495 100644 --- a/packages/nodes-base/nodes/MongoDb/GenericFunctions.ts +++ b/packages/nodes-base/nodes/MongoDb/GenericFunctions.ts @@ -2,7 +2,7 @@ import { formatPemBlock } from '@n8n/utils/format-pem-block'; import get from 'lodash/get'; import set from 'lodash/set'; import { Binary, MongoClient, ObjectId } from 'mongodb'; -import { jsonParse, NodeOperationError } from 'n8n-workflow'; +import { NodeOperationError } from 'n8n-workflow'; import type { ICredentialDataDecryptedObject, IDataObject, @@ -13,6 +13,7 @@ import type { import { createSecureContext } from 'tls'; import { routeBinaryProperties } from '@utils/binary'; +import { isScalarValue } from '@utils/query-parameters'; import type { IMongoCredentials, @@ -83,116 +84,6 @@ export function validateAndResolveMongoCredentials( } } -type MongoQueryParameterScalar = string | number | boolean | bigint | Date | null; -type MongoQueryParameter = MongoQueryParameterScalar | MongoQueryParameterScalar[]; - -function isScalarValue(value: unknown): value is MongoQueryParameterScalar { - return ( - value === null || - typeof value === 'string' || - typeof value === 'number' || - typeof value === 'boolean' || - typeof value === 'bigint' || - value instanceof Date - ); -} - -function parseQueryParameters( - rawParameters: unknown, - node: INode, - itemIndex: number, -): MongoQueryParameter[] { - let parameters: unknown = rawParameters; - - if (typeof parameters === 'string') { - try { - parameters = JSON.parse(parameters) as unknown; - } catch (error) { - throw new NodeOperationError(node, error as Error, { - itemIndex, - message: 'Query Parameters must be valid JSON', - description: 'Enter the parameters as a JSON array', - }); - } - } - - if (!Array.isArray(parameters)) { - throw new NodeOperationError(node, 'Query Parameters must be a JSON array', { - itemIndex, - description: 'Enter the parameters as a JSON array', - }); - } - - return parameters.map((parameter, index) => { - if (isScalarValue(parameter) || (Array.isArray(parameter) && parameter.every(isScalarValue))) { - return parameter; - } - - throw new NodeOperationError( - node, - `Query parameter ${index + 1} must be a scalar or an array of scalars`, - { - itemIndex, - description: 'Objects and nested arrays are not supported', - }, - ); - }); -} - -export function parseAndResolveQueryParameters( - query: string, - rawParameters: unknown, - node: INode, - itemIndex: number, -): unknown { - const parsedQuery = jsonParse(query); - const parameters = parseQueryParameters(rawParameters, node, itemIndex); - - if (parameters.length === 0) return parsedQuery; - - const usedParameters = new Set(); - - const resolveValue = (value: unknown): unknown => { - if (typeof value === 'string') { - const match = /^\$(\d+)$/.exec(value); - if (!match) return value; - - const parameterIndex = Number(match[1]) - 1; - if (parameterIndex < 0 || parameterIndex >= parameters.length) { - throw new NodeOperationError(node, `Query placeholder ${value} has no matching value`, { - itemIndex, - description: `Add a value for ${value} to Query Parameters`, - }); - } - - usedParameters.add(parameterIndex); - return parameters[parameterIndex]; - } - - if (Array.isArray(value)) return value.map(resolveValue); - - if (value !== null && typeof value === 'object') { - return Object.fromEntries( - Object.entries(value).map(([key, entry]) => [key, resolveValue(entry)]), - ); - } - - return value; - }; - - const resolvedQuery = resolveValue(parsedQuery); - const unusedParameter = parameters.findIndex((_, index) => !usedParameters.has(index)); - - if (unusedParameter !== -1) { - throw new NodeOperationError(node, `Query parameter ${unusedParameter + 1} is not used`, { - itemIndex, - description: `Add $${unusedParameter + 1} to the query or remove the unused parameter`, - }); - } - - return resolvedQuery; -} - function describeUpdateKeyValueType(value: unknown): string { if (value === null) return 'null'; if (Array.isArray(value)) return 'array'; diff --git a/packages/nodes-base/nodes/MongoDb/MongoDb.node.ts b/packages/nodes-base/nodes/MongoDb/MongoDb.node.ts index f846647e5d3..78a26f879bf 100644 --- a/packages/nodes-base/nodes/MongoDb/MongoDb.node.ts +++ b/packages/nodes-base/nodes/MongoDb/MongoDb.node.ts @@ -19,10 +19,11 @@ import type { IPairedItemData, } from 'n8n-workflow'; +import { parseAndResolveQueryParameters } from '@utils/query-parameters'; + import { buildParameterizedConnString, connectMongoClient, - parseAndResolveQueryParameters, prepareFields, prepareItems, serializeMongoItems, diff --git a/packages/nodes-base/utils/__tests__/query-parameters.test.ts b/packages/nodes-base/utils/__tests__/query-parameters.test.ts new file mode 100644 index 00000000000..14e86d0c581 --- /dev/null +++ b/packages/nodes-base/utils/__tests__/query-parameters.test.ts @@ -0,0 +1,94 @@ +import type { INode } from 'n8n-workflow'; + +import { parseAndResolveQueryParameters } from '../query-parameters'; + +const mockNode = { name: 'Test Node', type: 'n8n-nodes-base.test' } as INode; + +describe('parseAndResolveQueryParameters', () => { + it('replaces placeholders with scalars and scalar arrays', () => { + const query = JSON.stringify({ + name: '$1', + age: { gte: '$2' }, + tags: { in: '$3' }, + }); + + const result = parseAndResolveQueryParameters( + query, + '["Alice", 30, ["active", "admin"]]', + mockNode, + 0, + ); + + expect(result).toEqual({ + name: 'Alice', + age: { gte: 30 }, + tags: { in: ['active', 'admin'] }, + }); + }); + + it('only replaces complete values, not keys or parts of strings', () => { + const query = JSON.stringify({ $1: 'key', exact: '$1', partial: 'user-$1' }); + + const result = parseAndResolveQueryParameters(query, ['Alice'], mockNode, 0); + + expect(result).toEqual({ $1: 'key', exact: 'Alice', partial: 'user-$1' }); + }); + + it('treats a parameter containing JSON as a plain string value', () => { + const query = JSON.stringify({ term: { user: '$1' } }); + + const result = parseAndResolveQueryParameters( + query, + ['zzz", "match_all": {}, "boost": "2'], + mockNode, + 0, + ); + + expect(result).toEqual({ term: { user: 'zzz", "match_all": {}, "boost": "2' } }); + }); + + it('does not replace placeholders when parameters are empty', () => { + const result = parseAndResolveQueryParameters('{ "name": "$1" }', [], mockNode, 0); + + expect(result).toEqual({ name: '$1' }); + }); + + it('throws when a placeholder has no matching parameter', () => { + expect(() => + parseAndResolveQueryParameters('{ "a": "$1", "b": "$2" }', ['Alice'], mockNode, 0), + ).toThrow('Query placeholder $2 has no matching value'); + }); + + it.each([{ parameters: [{ name: 'Alice' }] }, { parameters: [[['nested']]] }])( + 'throws for unsupported parameter value $parameters', + ({ parameters }) => { + expect(() => + parseAndResolveQueryParameters('{ "name": "$1" }', parameters, mockNode, 0), + ).toThrow(/must be a scalar or an array of scalars/); + }, + ); + + it('throws when a parameter is not used', () => { + expect(() => + parseAndResolveQueryParameters('{ "name": "$1" }', ['Alice', 30], mockNode, 0), + ).toThrow('Query parameter 2 is not used'); + }); + + it('throws when the parameters are not a JSON array', () => { + expect(() => + parseAndResolveQueryParameters('{ "name": "$1" }', '{ "name": "Alice" }', mockNode, 0), + ).toThrow('Query Parameters must be a JSON array'); + }); + + it('throws when the parameters are not valid JSON', () => { + expect(() => parseAndResolveQueryParameters('{ "name": "$1" }', '[', mockNode, 0)).toThrow( + 'Query Parameters must be valid JSON', + ); + }); + + it('throws when the query is not valid JSON', () => { + expect(() => parseAndResolveQueryParameters('{ "name": ', ['Alice'], mockNode, 0)).toThrow( + "Invalid JSON in 'Query'", + ); + }); +}); diff --git a/packages/nodes-base/utils/query-parameters.ts b/packages/nodes-base/utils/query-parameters.ts new file mode 100644 index 00000000000..512295ac69a --- /dev/null +++ b/packages/nodes-base/utils/query-parameters.ts @@ -0,0 +1,120 @@ +import { jsonParse, NodeOperationError } from 'n8n-workflow'; +import type { INode } from 'n8n-workflow'; + +type QueryParameterScalar = string | number | boolean | bigint | Date | null; +type QueryParameter = QueryParameterScalar | QueryParameterScalar[]; + +export function isScalarValue(value: unknown): value is QueryParameterScalar { + return ( + value === null || + typeof value === 'string' || + typeof value === 'number' || + typeof value === 'boolean' || + typeof value === 'bigint' || + value instanceof Date + ); +} + +function parseQueryParameters( + rawParameters: unknown, + node: INode, + itemIndex: number, +): QueryParameter[] { + let parameters: unknown = rawParameters; + + if (typeof parameters === 'string') { + try { + parameters = JSON.parse(parameters) as unknown; + } catch (error) { + throw new NodeOperationError(node, error as Error, { + itemIndex, + message: 'Query Parameters must be valid JSON', + description: 'Enter the parameters as a JSON array', + }); + } + } + + if (!Array.isArray(parameters)) { + throw new NodeOperationError(node, 'Query Parameters must be a JSON array', { + itemIndex, + description: 'Enter the parameters as a JSON array', + }); + } + + return parameters.map((parameter, index) => { + if (isScalarValue(parameter) || (Array.isArray(parameter) && parameter.every(isScalarValue))) { + return parameter; + } + + throw new NodeOperationError( + node, + `Query parameter ${index + 1} must be a scalar or an array of scalars`, + { + itemIndex, + description: 'Objects and nested arrays are not supported', + }, + ); + }); +} + +/** + * Parses a JSON query and substitutes `$1`, `$2`, ... placeholders with the given parameters. + * + * Placeholders are only substituted when they make up a complete string value, so a parameter + * can never contribute structure (keys, operators, extra clauses) to the resulting query. + */ +export function parseAndResolveQueryParameters( + query: string, + rawParameters: unknown, + node: INode, + itemIndex: number, +): unknown { + const parsedQuery = jsonParse(query, { + errorMessage: "Invalid JSON in 'Query'", + }); + const parameters = parseQueryParameters(rawParameters, node, itemIndex); + + if (parameters.length === 0) return parsedQuery; + + const usedParameters = new Set(); + + const resolveValue = (value: unknown): unknown => { + if (typeof value === 'string') { + const match = /^\$(\d+)$/.exec(value); + if (!match) return value; + + const parameterIndex = Number(match[1]) - 1; + if (parameterIndex < 0 || parameterIndex >= parameters.length) { + throw new NodeOperationError(node, `Query placeholder ${value} has no matching value`, { + itemIndex, + description: `Add a value for ${value} to Query Parameters`, + }); + } + + usedParameters.add(parameterIndex); + return parameters[parameterIndex]; + } + + if (Array.isArray(value)) return value.map(resolveValue); + + if (value !== null && typeof value === 'object') { + return Object.fromEntries( + Object.entries(value).map(([key, entry]) => [key, resolveValue(entry)]), + ); + } + + return value; + }; + + const resolvedQuery = resolveValue(parsedQuery); + const unusedParameter = parameters.findIndex((_, index) => !usedParameters.has(index)); + + if (unusedParameter !== -1) { + throw new NodeOperationError(node, `Query parameter ${unusedParameter + 1} is not used`, { + itemIndex, + description: `Add $${unusedParameter + 1} to the query or remove the unused parameter`, + }); + } + + return resolvedQuery; +} diff --git a/packages/testing/performance/benchmarks/expression-engine/micro.bench.ts b/packages/testing/performance/benchmarks/expression-engine/micro.bench.ts index 613165767c3..4eda0060610 100644 --- a/packages/testing/performance/benchmarks/expression-engine/micro.bench.ts +++ b/packages/testing/performance/benchmarks/expression-engine/micro.bench.ts @@ -12,11 +12,7 @@ */ import { bench } from 'vitest'; import { ExpressionEvaluator, IsolatedVmBridge } from '@n8n/expression-runtime'; -import { - DollarSignValidator, - PrototypeSanitizer, - ThisSanitizer, -} from 'n8n-workflow/expression-sandboxing'; +import { expressionSandboxHooks } from 'n8n-workflow/expression-sandboxing'; import { BENCH_OPTIONS } from '../bench-options'; @@ -24,10 +20,7 @@ import { BENCH_OPTIONS } from '../bench-options'; const evaluator = new ExpressionEvaluator({ createBridge: () => new IsolatedVmBridge({ timeout: 5000 }), maxCodeCacheSize: 1024, - hooks: { - before: [ThisSanitizer], - after: [PrototypeSanitizer, DollarSignValidator], - }, + hooks: expressionSandboxHooks, }); await evaluator.initialize(); const caller = {}; diff --git a/packages/workflow/src/expression-evaluator-proxy.ts b/packages/workflow/src/expression-evaluator-proxy.ts index 2c5db7a6efc..5292a41a8f5 100644 --- a/packages/workflow/src/expression-evaluator-proxy.ts +++ b/packages/workflow/src/expression-evaluator-proxy.ts @@ -1,15 +1,17 @@ import { Tournament } from '@n8n/tournament'; -import { DollarSignValidator, ThisSanitizer, PrototypeSanitizer } from './expression-sandboxing'; +import { expressionSandboxHooks } from './expression-sandboxing'; type Evaluator = (expr: string, data: unknown) => string | null | (() => unknown); type ErrorHandler = (error: Error) => void; const errorHandler: ErrorHandler = () => {}; -const tournamentEvaluator = new Tournament(errorHandler, undefined, undefined, { - before: [ThisSanitizer], - after: [PrototypeSanitizer, DollarSignValidator], -}); +const tournamentEvaluator = new Tournament( + errorHandler, + undefined, + undefined, + expressionSandboxHooks, +); const evaluator: Evaluator = tournamentEvaluator.execute.bind(tournamentEvaluator); export const setErrorHandler = (handler: ErrorHandler) => { diff --git a/packages/workflow/src/expression-sandboxing.ts b/packages/workflow/src/expression-sandboxing.ts index f7e8ef1a410..2b0cbe869c3 100644 --- a/packages/workflow/src/expression-sandboxing.ts +++ b/packages/workflow/src/expression-sandboxing.ts @@ -1,4 +1,10 @@ -import { type ASTAfterHook, type ASTBeforeHook, astBuilders as b, astVisit } from '@n8n/tournament'; +import { + type ASTAfterHook, + type ASTBeforeHook, + type TournamentHooks, + astBuilders as b, + astVisit, +} from '@n8n/tournament'; import { ExpressionClassExtensionError, @@ -129,7 +135,51 @@ const isValidDollarPropertyAccess = (expr: unknown): boolean => { const GLOBAL_IDENTIFIERS = new Set(['globalThis']); -const BLOCKED_SPREAD_GLOBALS = new Set(['process', 'global', 'globalThis', 'Buffer']); +/** + * Backstop, not the containment: the polyfill already resolves a free base class + * identifier through the data context. Listed here because a subclass inherits + * the static side of its base, so these are the costliest to ever let through. + */ +const blockedBaseClasses = new Set([ + 'Function', + 'GeneratorFunction', + 'AsyncFunction', + 'AsyncGeneratorFunction', + 'Buffer', +]); + +/** + * Rejects `class X extends ` unless the base class is a plain identifier + * that is not one of {@link blockedBaseClasses}. + * + * Must stay a before hook: the polyfill rewrites a free base class identifier + * into a data-context lookup, after which the two cases are indistinguishable. + */ +export const ClassExtensionValidator: ASTBeforeHook = (ast, _dataNode) => { + const validate = (superClass: unknown) => { + if (isAstNode(superClass)) { + if (superClass.type !== 'Identifier') { + throw new ExpressionError('Cannot use dynamic class extension due to security concerns'); + } + + if (typeof superClass.name === 'string' && blockedBaseClasses.has(superClass.name)) { + throw new ExpressionClassExtensionError(superClass.name); + } + } + }; + + astVisit(ast, { + visitClassDeclaration(path) { + this.traverse(path); + validate(path.node.superClass); + }, + + visitClassExpression(path) { + this.traverse(path); + validate(path.node.superClass); + }, + }); +}; /** * Prevents regular functions from binding their `this` to the Node.js global. @@ -301,58 +351,6 @@ export const DollarSignValidator: ASTAfterHook = (ast, _dataNode) => { }); }; -const blockedBaseClasses = new Set([ - 'Function', - 'GeneratorFunction', - 'AsyncFunction', - 'AsyncGeneratorFunction', -]); - -/** - * Builds an AST node that safely resolves a spread argument like `...process`. - * - * Tournament's VariablePolyfill rewrites plain identifiers (e.g. `process`) - * to look them up from the data context, but it does NOT handle identifiers - * inside SpreadElement / SpreadProperty nodes. Without this fix, `{...process}` - * would resolve to the real Node.js `process` object. - * - * The generated code checks the data context first, falling back to a throw: - * - * ("process" in data) ? data.process : (() => { throw new Error("...") })() - * - * - If the workflow has a variable called "process" → spread that (safe, user-defined) - * - Otherwise → throw at runtime, blocking access to the real global - */ -const buildSafeSpreadArg = (name: string, dataNode: Parameters[1]) => { - // "process" in ___n8n_data - const isInDataContext = b.binaryExpression('in', b.literal(name), dataNode); - - // ___n8n_data.process - const readFromDataContext = b.memberExpression(dataNode, b.identifier(name)); - - // (() => { throw new Error('Cannot spread "process" ...') })() - // - // This is an IIFE because `throw` is a statement, not an expression, - // so it cannot appear directly inside a ternary's falsy branch. - const throwSecurityError = b.callExpression( - b.arrowFunctionExpression( - [], - b.blockStatement([ - b.throwStatement( - b.newExpression(b.identifier('Error'), [ - b.literal(`Cannot spread "${name}" due to security concerns`), - ]), - ), - ]), - ), - [], - ); - - // Full result: - // ("process" in ___n8n_data) ? ___n8n_data.process : (() => { throw ... })() - return b.conditionalExpression(isInDataContext, readFromDataContext, throwSecurityError); -}; - export const PrototypeSanitizer: ASTAfterHook = (ast, dataNode) => { astVisit(ast, { visitVariableDeclarator(path) { @@ -392,42 +390,19 @@ export const PrototypeSanitizer: ASTAfterHook = (ast, dataNode) => { visitClassDeclaration(path) { this.traverse(path); - const node = path.node; - const className = getReservedIdentifier(node.id); - if (className !== undefined) { - throw new ExpressionReservedVariableError(className); - } - - if (node.superClass) { - if (node.superClass.type === 'Identifier') { - if (blockedBaseClasses.has(node.superClass.name)) { - throw new ExpressionClassExtensionError(node.superClass.name); - } - } else { - throw new ExpressionError('Cannot use dynamic class extension due to security concerns'); - } - } + const className = getReservedIdentifier(path.node.id); + if (className === undefined) return; + throw new ExpressionReservedVariableError(className); }, visitClassExpression(path) { this.traverse(path); - const node = path.node; - const className = getReservedIdentifier(node.id); + const className = getReservedIdentifier(path.node.id); if (className !== undefined) { throw new ExpressionReservedVariableError(className); } - - if (node.superClass) { - if (node.superClass.type === 'Identifier') { - if (blockedBaseClasses.has(node.superClass.name)) { - throw new ExpressionClassExtensionError(node.superClass.name); - } - } else { - throw new ExpressionError('Cannot use dynamic class extension due to security concerns'); - } - } }, visitAssignmentExpression(path) { @@ -530,30 +505,22 @@ export const PrototypeSanitizer: ASTAfterHook = (ast, dataNode) => { } }, - visitSpreadElement(path) { - this.traverse(path); - const { argument } = path.node; - if (argument.type === 'Identifier' && BLOCKED_SPREAD_GLOBALS.has(argument.name)) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any - (path.node as any).argument = buildSafeSpreadArg(argument.name, dataNode); - } - }, - - visitSpreadProperty(path) { - this.traverse(path); - const { argument } = path.node; - if (argument.type === 'Identifier' && BLOCKED_SPREAD_GLOBALS.has(argument.name)) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any - (path.node as any).argument = buildSafeSpreadArg(argument.name, dataNode); - } - }, - visitWithStatement() { throw new ExpressionWithStatementError(); }, }); }; +/** + * The complete set of AST hooks an expression evaluator must run. Evaluators take + * this object rather than assembling their own, so a hook added here cannot be + * missed by one of them. + */ +export const expressionSandboxHooks: TournamentHooks = { + before: [ClassExtensionValidator, ThisSanitizer], + after: [PrototypeSanitizer, DollarSignValidator], +}; + export const sanitizer = (value: unknown): unknown => { const propertyKey = String(value); if (!isSafeObjectProperty(propertyKey)) { diff --git a/packages/workflow/src/expression.ts b/packages/workflow/src/expression.ts index f688a7d358e..f968439dd68 100644 --- a/packages/workflow/src/expression.ts +++ b/packages/workflow/src/expression.ts @@ -6,13 +6,7 @@ import { UnexpectedError, UserError } from './errors'; import { ExpressionExtensionError } from './errors/expression-extension.error'; import { ExpressionError } from './errors/expression.error'; import { evaluateExpression, setErrorHandler } from './expression-evaluator-proxy'; -import { - DollarSignValidator, - PrototypeSanitizer, - ThisSanitizer, - sanitizer, - sanitizerName, -} from './expression-sandboxing'; +import { expressionSandboxHooks, sanitizer, sanitizerName } from './expression-sandboxing'; import { isExpression } from './expressions/expression-helpers'; import * as LoggerProxy from './logger-proxy'; import { extend, extendOptional } from './extensions'; @@ -268,10 +262,7 @@ export class Expression { maxCodeCacheSize: options.maxCodeCacheSize, poolSize: options.poolSize, idleTimeoutMs: options.idleTimeoutMs, - hooks: { - before: [ThisSanitizer], - after: [PrototypeSanitizer, DollarSignValidator], - }, + hooks: expressionSandboxHooks, logger: LoggerProxy, observability: options.observability, }); diff --git a/packages/workflow/src/workflow-data-proxy.ts b/packages/workflow/src/workflow-data-proxy.ts index bff21cba176..86b436d767d 100644 --- a/packages/workflow/src/workflow-data-proxy.ts +++ b/packages/workflow/src/workflow-data-proxy.ts @@ -33,7 +33,12 @@ import { createResultError, createResultOk } from '@n8n/utils/result'; import type { IRunExecutionData } from './run-execution-data/run-execution-data'; import { safeRegex } from './safe-regex'; import { isResourceLocatorValue } from './type-guards'; -import { containsUnsafeObjectPropertyToken, deepCopy, isObjectEmpty } from './utils'; +import { + containsUnsafeObjectPropertyToken, + deepCopy, + isObjectEmpty, + isSafeObjectProperty, +} from './utils'; import type { Workflow } from './workflow'; import type { EnvProviderState } from './workflow-data-proxy-env-provider'; import { createEnvProvider, createEnvProviderState } from './workflow-data-proxy-env-provider'; @@ -61,6 +66,14 @@ type PairedItemMethod = (typeof PAIRED_ITEM_METHOD)[keyof typeof PAIRED_ITEM_MET * This is a process-wide invariant, so we probe once and cache the result. */ let codeGenerationAllowed: boolean | undefined; +// Reads a key from a placeholder source only when it is the source's own +// property, so a lookup can never resolve to a value reached through the +// prototype chain (e.g. `constructor` / `__proto__`). +const readOwnKey = (source: unknown, key: string): unknown => { + if (source === null || typeof source !== 'object') return undefined; + return Object.hasOwn(source, key) ? (source as Record)[key] : undefined; +}; + const isCodeGenerationAllowed = (): boolean => { if (codeGenerationAllowed === undefined) { try { @@ -1116,6 +1129,15 @@ export class WorkflowDataProxy { }, ); } + // Reserved keys resolve to inherited members (e.g. the object's + // constructor or prototype) rather than a placeholder value, so they + // are never valid placeholder names. + if (!isSafeObjectProperty(name)) { + throw new ExpressionError('Invalid parameter key', { + runIndex, + itemIndex, + }); + } const resultData = that.runExecutionData?.resultData?.runData?.[that.activeNodeName]?.[runIndex]; @@ -1138,10 +1160,12 @@ export class WorkflowDataProxy { type: 'no_execution_data', }); } + // Resolve only own placeholder keys, never values reached through the + // prototype chain of the input data — including the `query` container + // itself, which is read as an own key for the same reason. return ( - // TS does not know that the key exists, we need to address this in refactor - (placeholdersDataInputData?.query as Record)?.[name] ?? - placeholdersDataInputData?.[name] ?? + readOwnKey(readOwnKey(placeholdersDataInputData, 'query'), name) ?? + readOwnKey(placeholdersDataInputData, name) ?? defaultValue ); }; diff --git a/packages/workflow/test/expression-nested-data.test.ts b/packages/workflow/test/expression-nested-data.test.ts index f0089121aac..91ff93f4f27 100644 --- a/packages/workflow/test/expression-nested-data.test.ts +++ b/packages/workflow/test/expression-nested-data.test.ts @@ -87,4 +87,22 @@ describe('Expression — nested $json shapes (engine parity)', () => { list: [[1, 2], { k: 'v' }], }); }); + + it('spreads $json into an object literal', () => { + expect(evaluate('={{ {...$json} }}', { name: 'alice', age: 30 })).toEqual({ + name: 'alice', + age: 30, + }); + }); + + it('overrides a property of a spread $json', () => { + expect(evaluate('={{ {...$json, age: 31} }}', { name: 'alice', age: 30 })).toEqual({ + name: 'alice', + age: 31, + }); + }); + + it('spreads a nested array from $json', () => { + expect(evaluate('={{ [...$json.rows] }}', { rows: [1, 2, 3] })).toEqual([1, 2, 3]); + }); }); diff --git a/packages/workflow/test/expression-sandboxing.test.ts b/packages/workflow/test/expression-sandboxing.test.ts index c36a508560e..673a0e26fad 100644 --- a/packages/workflow/test/expression-sandboxing.test.ts +++ b/packages/workflow/test/expression-sandboxing.test.ts @@ -10,13 +10,7 @@ import { ExpressionError, ExpressionWithStatementError, } from '../src/errors'; -import { - DollarSignValidator, - ThisSanitizer, - PrototypeSanitizer, - sanitizer, - DOLLAR_SIGN_ERROR, -} from '../src/expression-sandboxing'; +import { expressionSandboxHooks, sanitizer, DOLLAR_SIGN_ERROR } from '../src/expression-sandboxing'; const tournament = new Tournament( (e) => { @@ -24,10 +18,7 @@ const tournament = new Tournament( }, undefined, undefined, - { - before: [ThisSanitizer], - after: [PrototypeSanitizer, DollarSignValidator], - }, + expressionSandboxHooks, ); const errorRegex = /^Cannot access ".*" due to security concerns$/; @@ -477,6 +468,15 @@ describe('PrototypeSanitizer', () => { }).toThrowError(ExpressionClassExtensionError); }); + it('should not allow class extending Buffer', () => { + expect(() => { + tournament.execute( + '{{ (() => { class Z extends Buffer {} return Z.allocUnsafe(32).length; })() }}', + { __sanitize: sanitizer }, + ); + }).toThrow(ExpressionClassExtensionError); + }); + it('should allow class extending safe classes', () => { expect(() => { tournament.execute( @@ -647,98 +647,179 @@ describe('PrototypeSanitizer', () => { }); }); - describe('Spread-based global access', () => { - it('should not allow spreading process', () => { + describe('Spread of host globals', () => { + it.each([ + 'process', + 'global', + 'globalThis', + 'Buffer', + 'console', + 'Error', + 'crypto', + 'navigator', + 'performance', + 'Intl', + 'Atomics', + 'URL', + 'TextEncoder', + 'TextDecoder', + 'fetch', + 'Headers', + 'Request', + 'Response', + 'Blob', + 'File', + 'FormData', + 'AbortController', + 'Event', + 'EventTarget', + 'MessageChannel', + 'SharedArrayBuffer', + 'ReadableStream', + 'WritableStream', + 'WeakRef', + 'FinalizationRegistry', + 'AggregateError', + ])('should not expose the host %s through a spread', (name) => { + expect(tournament.execute(`{{ ({...${name}}) }}`, { __sanitize: sanitizer })).toEqual({}); + }); + + it.each([ + ['nested spread', '{{ ({...({...process})}) }}'], + ['spread inside an arrow function', '{{ (() => ({...process}))() }}'], + ['spread among other spreads', '{{ ({...{}, ...process}) }}'], + ])('should not expose a host global through a %s', (_, expression) => { + expect(tournament.execute(expression, { __sanitize: sanitizer })).toEqual({}); + }); + + it.each([ + ['process.env', '{{ typeof ({...process}).env }}'], + ['process.pid', '{{ typeof ({...process}).pid }}'], + ['Buffer.allocUnsafe', '{{ typeof ({...Buffer}).allocUnsafe }}'], + ['console.log', '{{ typeof ({...console}).log }}'], + ])('should not expose the host %s through a spread', (_, expression) => { + expect(tournament.execute(expression, { __sanitize: sanitizer })).toBe('undefined'); + }); + + it.each([ + ['an array literal', '{{ [...process] }}'], + ['call arguments', '{{ ((a) => a)(...process) }}'], + ])('should not iterate the host process in %s', (_, expression) => { + expect(() => tournament.execute(expression, { __sanitize: sanitizer })).toThrow( + /is not iterable/, + ); + }); + + it('should not hand out a reference to a host function', () => { expect(() => { - tournament.execute('{{ ((g) => g.getBuiltinModule)(({...process})) }}', { + tournament.execute('{{ ({...console}).log.toString() }}', { __sanitize: sanitizer }); + }).toThrow(/Cannot read properties of undefined/); + }); + + it('should not write to a host function', () => { + expect(() => { + tournament.execute('{{ Object.assign(({...console}).log, {written: 1}).name }}', { __sanitize: sanitizer, + Object, }); - }).toThrowError(/due to security concerns/); + }).toThrow(/Cannot convert undefined or null to object/); + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any + expect((console.log as any).written).toBeUndefined(); }); - it('should not allow spreading process in object literal', () => { - expect(() => { - tournament.execute('{{ ({...process}) }}', { __sanitize: sanitizer }); - }).toThrowError(/Cannot spread "process" due to security concerns/); + it.each([ + ['an array literal', '{{ [...arguments][0] }}'], + ['an object literal', '{{ ({...arguments}) }}'], + ['a function body', '{{ (() => [...arguments][0])() }}'], + ])("should not expose the evaluator's own arguments in %s", (_, expression) => { + expect(() => tournament.execute(expression, { __sanitize: sanitizer })).toThrow(errorRegex); }); - it('should not allow spreading process in array', () => { - expect(() => { - tournament.execute('{{ [...process] }}', { __sanitize: sanitizer }); - }).toThrowError(/Cannot spread "process" due to security concerns/); - }); - - it('should not allow spreading global', () => { - expect(() => { - tournament.execute('{{ ({...global}) }}', { __sanitize: sanitizer }); - }).toThrowError(/Cannot spread "global" due to security concerns/); - }); - - it('should not allow spreading Buffer', () => { - expect(() => { - tournament.execute('{{ ({...Buffer}) }}', { __sanitize: sanitizer }); - }).toThrowError(/Cannot spread "Buffer" due to security concerns/); - }); - - it('should not allow the exact RCE PoC payload', () => { + it('should not reach a built-in module through a spread', () => { expect(() => { tournament.execute( "{{ ((g) => g.getBuiltinModule('child_process').execSync('id').toString())({...process}) }}", { __sanitize: sanitizer }, ); - }).toThrowError(/due to security concerns/); + }).toThrow(errorRegex); }); - it('should not allow spreading process in function call arguments', () => { - expect(() => { - tournament.execute('{{ ((a, b) => a)(...process) }}', { __sanitize: sanitizer }); - }).toThrowError(/Cannot spread "process" due to security concerns/); - }); - - it('should not allow spreading process inside arrow function', () => { - expect(() => { - tournament.execute('{{ (() => ({...process}))() }}', { __sanitize: sanitizer }); - }).toThrowError(/Cannot spread "process" due to security concerns/); - }); - - it('should not allow spreading process in nested spread', () => { - expect(() => { - tournament.execute('{{ ({...({...process})}) }}', { __sanitize: sanitizer }); - }).toThrowError(/Cannot spread "process" due to security concerns/); - }); - - it('should not allow spreading process in template expression', () => { - expect(() => { - // eslint-disable-next-line n8n-local-rules/no-interpolation-in-regular-string - tournament.execute('{{ `${JSON.stringify({...process})}` }}', { - __sanitize: sanitizer, - }); - }).toThrow(); - }); - - it('should not allow spreading process among other spreads', () => { - expect(() => { - tournament.execute('{{ ({...{a:1}, ...process}) }}', { __sanitize: sanitizer }); - }).toThrowError(/Cannot spread "process" due to security concerns/); - }); - - it('should resolve spread from data context process, not the real one', () => { - const result = tournament.execute('{{ ({...process}).safe }}', { + it('should not expose the host process through a template expression', () => { + // eslint-disable-next-line n8n-local-rules/no-interpolation-in-regular-string + const result = tournament.execute('{{ `${JSON.stringify({...process})}` }}', { __sanitize: sanitizer, - process: { safe: true }, + JSON, }); - expect(result).toBe(true); + expect(result).toBe('{}'); }); - it('should not expose real process.version via spread', () => { - const result = tournament.execute('{{ typeof ({...process}).version }}', { + it('should not expose the host process as a computed key', () => { + const result = tournament.execute('{{ Object.keys({[process]: 1})[0] }}', { __sanitize: sanitizer, - process: {}, + Object, }); expect(result).toBe('undefined'); }); - it('should use data context pid via spread, not real pid', () => { + /** + * `Buffer` is additionally named in `blockedBaseClasses`, so these use a + * global that is not, to show that containment comes from the polyfill + * rather than from that list. + */ + it.each([ + ['Error', '{{ (() => { class X extends Error {} return X.captureStackTrace; })() }}'], + ['Array', '{{ (() => { class X extends Array {} return X.from; })() }}'], + ])('should not expose the host %s as a base class', (_, expression) => { + expect(() => tournament.execute(expression, { __sanitize: sanitizer })).toThrow( + /is not a constructor or null/, + ); + }); + + it('should not expose the host process as a switch case', () => { + const result = tournament.execute( + '{{ (() => { switch (1) { case process: return "host"; } return "safe"; })() }}', + { __sanitize: sanitizer }, + ); + expect(result).toBe('safe'); + }); + }); + + describe('Spread of data context values', () => { + it('should spread an object from the data context', () => { + const result = tournament.execute('{{ ({...$json}).greeting }}', { + __sanitize: sanitizer, + $json: { greeting: 'hello' }, + }); + expect(result).toBe('hello'); + }); + + it('should merge a spread data context object with additional properties', () => { + const result = tournament.execute('{{ JSON.stringify({...$json, b: 2}) }}', { + __sanitize: sanitizer, + $json: { a: 1 }, + JSON, + }); + expect(result).toBe('{"a":1,"b":2}'); + }); + + it('should spread an array from the data context', () => { + const result = tournament.execute('{{ [...$arr].length }}', { + __sanitize: sanitizer, + $arr: [1, 2, 3], + }); + expect(result).toBe(3); + }); + + it('should spread a data context value into call arguments', () => { + const result = tournament.execute('{{ Math.max(...$arr) }}', { + __sanitize: sanitizer, + $arr: [1, 5, 3], + }); + expect(result).toBe(5); + }); + + it('should prefer a data context value over the host global of the same name', () => { const result = tournament.execute('{{ ({...process}).pid }}', { __sanitize: sanitizer, process: { pid: -1 }, @@ -746,42 +827,42 @@ describe('PrototypeSanitizer', () => { expect(result).toBe(-1); }); - it('should use data context when spread is wrapped in arrow function', () => { - const result = tournament.execute('{{ ((g) => g.pid)({...process}) }}', { - __sanitize: sanitizer, - process: { pid: -1 }, - }); - expect(result).toBe(-1); + it.each([ + ['const', '{{ (() => { const process = { a: 1 }; return {...process}.a; })() }}'], + ['function scope', '{{ (function(){ const Buffer = { a: 1 }; return {...Buffer}.a; })() }}'], + ['parameter', '{{ ((process) => ({...process}).a)({ a: 1 }) }}'], + ])('should spread a local variable declared in %s scope', (_, expression) => { + expect(tournament.execute(expression, { __sanitize: sanitizer })).toBe(1); }); - it('should not give access to real process.exit via spread', () => { - const result = tournament.execute('{{ typeof ({...process}).exit }}', { + it('should resolve a computed key from the data context', () => { + const result = tournament.execute('{{ ({[$key]: 1}).dynamic }}', { __sanitize: sanitizer, - process: {}, + $key: 'dynamic', }); - expect(result).not.toBe('function'); + expect(result).toBe(1); }); - it('should not give access to real process.env via spread', () => { - const result = tournament.execute('{{ typeof ({...process}).env }}', { - __sanitize: sanitizer, - process: {}, - }); - expect(result).not.toBe('object'); - }); - - it('should not give access to getBuiltinModule via spread', () => { - let result: unknown; - try { - result = tournament.execute('{{ typeof ({...process}).getBuiltinModule }}', { - __sanitize: sanitizer, - process: {}, - }); - } catch { - // Blocked by PrototypeSanitizer — also a valid outcome - return; + it('should resolve a base class from the data context', () => { + class Base { + greet() { + return 'hello'; + } } - expect(result).not.toBe('function'); + + const result = tournament.execute( + '{{ (() => { class X extends Base {} return new X().greet(); })() }}', + { __sanitize: sanitizer, Base }, + ); + expect(result).toBe('hello'); + }); + + it('should resolve a switch case from the data context', () => { + const result = tournament.execute( + '{{ (() => { switch ($key) { case $key: return "matched"; } return "unmatched"; })() }}', + { __sanitize: sanitizer, $key: 'a' }, + ); + expect(result).toBe('matched'); }); }); diff --git a/packages/workflow/test/workflow-data-proxy.test.ts b/packages/workflow/test/workflow-data-proxy.test.ts index ac3ba9ded1b..f66837e811a 100644 --- a/packages/workflow/test/workflow-data-proxy.test.ts +++ b/packages/workflow/test/workflow-data-proxy.test.ts @@ -1052,6 +1052,84 @@ describe('WorkflowDataProxy', () => { expect(() => proxy.$fromAI('some_key')).toThrow(ExpressionError); }); + + describe('key resolution is limited to own placeholder keys', () => { + const buildProxy = (json: unknown) => { + const workflow = new Workflow({ + id: '123', + name: 'test workflow', + nodes: [ + { + id: 'aiNode', + name: 'AI Node', + type: 'n8n-nodes-base.aiAgent', + typeVersion: 1, + position: [0, 0], + parameters: {}, + }, + ], + connections: {}, + active: false, + nodeTypes: Helpers.NodeTypes(), + }); + const connectionInputData = [{ json: json as IDataObject, pairedItem: { item: 0 } }]; + const dataProxy = new WorkflowDataProxy( + workflow, + null, + 0, + 0, + 'AI Node', + connectionInputData, + {}, + 'manual', + {}, + undefined, + ); + return dataProxy.getDataProxy(); + }; + + test('rejects reserved object keys (__proto__, constructor, prototype)', () => { + const proxy = buildProxy({ safe: 'ok' }); + expect(proxy.$fromAI('safe')).toBe('ok'); + expect(() => proxy.$fromAI('__proto__')).toThrow(ExpressionError); + expect(() => proxy.$fromAI('constructor')).toThrow(ExpressionError); + expect(() => proxy.$fromAI('prototype')).toThrow(ExpressionError); + }); + + test('ignores inherited (non-own) keys and returns the default', () => { + const proxy = buildProxy({ safe: 'ok' }); + expect(proxy.$fromAI('toString', '', 'string', 'fallback')).toBe('fallback'); + expect(proxy.$fromAI('hasOwnProperty', '', 'string', 'fallback')).toBe('fallback'); + expect(proxy.$fromAI('valueOf', '', 'string', 'fallback')).toBe('fallback'); + }); + + test('rejects a reserved key even when present as an own key from parsed JSON', () => { + const json = JSON.parse('{"__proto__": {"polluted": true}, "safe": "ok"}'); + const proxy = buildProxy(json); + expect(() => proxy.$fromAI('__proto__')).toThrow(ExpressionError); + expect(proxy.$fromAI('safe')).toBe('ok'); + }); + + test('returns the default when input json is a bare primitive', () => { + const proxy = buildProxy(1); + expect(() => proxy.$fromAI('constructor')).toThrow(ExpressionError); + expect(() => proxy.$fromAI('__proto__')).toThrow(ExpressionError); + expect(proxy.$fromAI('any_key', '', 'string', 'fallback')).toBe('fallback'); + }); + + test('resolves a placeholder from an own query container', () => { + const proxy = buildProxy({ query: { full_name: 'Alice' } }); + expect(proxy.$fromAI('full_name')).toBe('Alice'); + }); + + test('does not read the query container through the prototype chain', () => { + const json = Object.create({ query: { evil: 'from-prototype' } }); + json.safe = 'ok'; + const proxy = buildProxy(json); + expect(proxy.$fromAI('safe')).toBe('ok'); + expect(proxy.$fromAI('evil', '', 'string', 'fallback')).toBe('fallback'); + }); + }); }); describe('$tool', () => { diff --git a/scripts/build-n8n.mjs b/scripts/build-n8n.mjs index e21d731582d..df7829ea050 100755 --- a/scripts/build-n8n.mjs +++ b/scripts/build-n8n.mjs @@ -35,7 +35,12 @@ const config = { }; // Define backend patches to keep during deployment -const PATCHES_TO_KEEP = ['pdfjs-dist', 'pkce-challenge', 'bull']; +const PATCHES_TO_KEEP = [ + 'pdfjs-dist', + 'pkce-challenge', + 'bull', + 'lodash' +]; // #endregion ===== Configuration =====