diff --git a/packages/@n8n/api-types/src/dto/credentials/test-credential-request.dto.ts b/packages/@n8n/api-types/src/dto/credentials/test-credential-request.dto.ts new file mode 100644 index 00000000000..823fed0110c --- /dev/null +++ b/packages/@n8n/api-types/src/dto/credentials/test-credential-request.dto.ts @@ -0,0 +1,11 @@ +import { z } from 'zod'; +import { Z } from 'zod-class'; + +export class TestCredentialRequestDto extends Z.class({ + credentials: z.object({ + id: z.string(), + name: z.string().default(''), + type: z.string(), + data: z.record(z.string(), z.unknown()).optional(), + }), +}) {} diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index f54a4341ab3..f11a878580b 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -49,6 +49,7 @@ export { PullWorkFolderRequestDto } from './source-control/pull-work-folder-requ export { PushWorkFolderRequestDto } from './source-control/push-work-folder-request.dto'; export { CreateCredentialDto } from './credentials/create-credential.dto'; +export { TestCredentialRequestDto } from './credentials/test-credential-request.dto'; export { VariableListRequestDto } from './variables/variables-list-request.dto'; export { CreateVariableRequestDto } from './variables/create-variable-request.dto'; export { UpdateVariableRequestDto } from './variables/update-variable-request.dto'; diff --git a/packages/@n8n/expression-runtime/src/extensions/__tests__/function-extensions.test.ts b/packages/@n8n/expression-runtime/src/extensions/__tests__/function-extensions.test.ts index 5eb25c32c19..9a98b00a9f5 100644 --- a/packages/@n8n/expression-runtime/src/extensions/__tests__/function-extensions.test.ts +++ b/packages/@n8n/expression-runtime/src/extensions/__tests__/function-extensions.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect } from 'vitest'; import { extendedFunctions } from '../function-extensions'; const zip = extendedFunctions.zip; +const toPathSegment = extendedFunctions.toPathSegment; describe('zip', () => { it('should pair keys with values', () => { @@ -21,3 +22,13 @@ describe('zip', () => { expect(({} as Record).marker).toBeUndefined(); }); }); + +describe('toPathSegment', () => { + it('encodes a value as one path segment', () => { + expect(toPathSegment('a/b c')).toBe('a%2Fb%20c'); + }); + + it.each(['', '.', '..', null, undefined])('rejects %s', (input) => { + expect(() => toPathSegment(input)).toThrow('Invalid identifier'); + }); +}); diff --git a/packages/@n8n/expression-runtime/src/extensions/function-extensions.ts b/packages/@n8n/expression-runtime/src/extensions/function-extensions.ts index 6b2636fd170..161f9e8b79d 100644 --- a/packages/@n8n/expression-runtime/src/extensions/function-extensions.ts +++ b/packages/@n8n/expression-runtime/src/extensions/function-extensions.ts @@ -52,6 +52,19 @@ const not = (value: unknown): boolean => { return !value; }; +function toPathSegment(input: unknown): string { + if (input === null || input === undefined) { + throw new ExpressionExtensionError('Invalid identifier: a value is required'); + } + + const value = String(input); + if (value === '' || value === '.' || value === '..') { + throw new ExpressionExtensionError(`Invalid identifier: "${value}" is not allowed`); + } + + return encodeURIComponent(value); +} + function ifEmpty(value: V, defaultValue: T) { if (arguments.length !== 2) { // DIVERGENCE from packages/workflow/src/extensions/extended-functions.ts: @@ -95,6 +108,7 @@ export const extendedFunctions = { average, numberList, zip, + toPathSegment, $min: min, $max: max, $average: average, diff --git a/packages/@n8n/nodes-langchain/nodes/memory/MemoryBufferWindow/MemoryBufferWindow.node.ts b/packages/@n8n/nodes-langchain/nodes/memory/MemoryBufferWindow/MemoryBufferWindow.node.ts index 78d11d8e77c..97739f5f242 100644 --- a/packages/@n8n/nodes-langchain/nodes/memory/MemoryBufferWindow/MemoryBufferWindow.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/memory/MemoryBufferWindow/MemoryBufferWindow.node.ts @@ -8,7 +8,7 @@ import { type SupplyData, } from 'n8n-workflow'; -import { getSessionId } from '@utils/helpers'; +import { coerceSessionIdToString, getSessionId } from '@utils/helpers'; import { logWrapper } from '@utils/logWrapper'; import { getConnectionHintNoticeField } from '@utils/sharedFields'; @@ -155,7 +155,11 @@ export class MemoryBufferWindow implements INodeType { if (nodeVersion >= 1.2) { sessionId = getSessionId(this, itemIndex); } else { - sessionId = this.getNodeParameter('sessionKey', itemIndex) as string; + sessionId = coerceSessionIdToString( + this, + this.getNodeParameter('sessionKey', itemIndex), + itemIndex, + ); } const memory = await memoryInstance.getMemory(`${workflowId}__${sessionId}`, { diff --git a/packages/@n8n/nodes-langchain/nodes/memory/MemoryMotorhead/MemoryMotorhead.node.ts b/packages/@n8n/nodes-langchain/nodes/memory/MemoryMotorhead/MemoryMotorhead.node.ts index 8d48535d185..bb99fa969c9 100644 --- a/packages/@n8n/nodes-langchain/nodes/memory/MemoryMotorhead/MemoryMotorhead.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/memory/MemoryMotorhead/MemoryMotorhead.node.ts @@ -7,7 +7,7 @@ import { type SupplyData, } from 'n8n-workflow'; -import { getSessionId } from '@utils/helpers'; +import { coerceSessionIdToString, getSessionId } from '@utils/helpers'; import { logWrapper } from '@utils/logWrapper'; import { getConnectionHintNoticeField } from '@utils/sharedFields'; @@ -98,7 +98,11 @@ export class MemoryMotorhead implements INodeType { if (nodeVersion >= 1.2) { sessionId = getSessionId(this, itemIndex); } else { - sessionId = this.getNodeParameter('sessionId', itemIndex) as string; + sessionId = coerceSessionIdToString( + this, + this.getNodeParameter('sessionId', itemIndex), + itemIndex, + ); } const memory = new MotorheadMemory({ diff --git a/packages/@n8n/nodes-langchain/nodes/memory/MemoryRedisChat/MemoryRedisChat.node.ts b/packages/@n8n/nodes-langchain/nodes/memory/MemoryRedisChat/MemoryRedisChat.node.ts index 51de637d9ba..414aa2df9bb 100644 --- a/packages/@n8n/nodes-langchain/nodes/memory/MemoryRedisChat/MemoryRedisChat.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/memory/MemoryRedisChat/MemoryRedisChat.node.ts @@ -12,7 +12,7 @@ import { import type { RedisClientOptions } from 'redis'; import { createClient } from 'redis'; -import { getSessionId } from '@utils/helpers'; +import { coerceSessionIdToString, getSessionId } from '@utils/helpers'; import { logWrapper } from '@utils/logWrapper'; import { getConnectionHintNoticeField } from '@utils/sharedFields'; @@ -121,7 +121,11 @@ export class MemoryRedisChat implements INodeType { if (nodeVersion >= 1.2) { sessionId = getSessionId(this, itemIndex); } else { - sessionId = this.getNodeParameter('sessionKey', itemIndex) as string; + sessionId = coerceSessionIdToString( + this, + this.getNodeParameter('sessionKey', itemIndex), + itemIndex, + ); } const redisOptions: RedisClientOptions = { diff --git a/packages/@n8n/nodes-langchain/nodes/memory/MemoryXata/MemoryXata.node.ts b/packages/@n8n/nodes-langchain/nodes/memory/MemoryXata/MemoryXata.node.ts index f06051da3c8..dadc866abf7 100644 --- a/packages/@n8n/nodes-langchain/nodes/memory/MemoryXata/MemoryXata.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/memory/MemoryXata/MemoryXata.node.ts @@ -9,7 +9,7 @@ import type { SupplyData, } from 'n8n-workflow'; -import { getSessionId } from '@utils/helpers'; +import { coerceSessionIdToString, getSessionId } from '@utils/helpers'; import { logWrapper } from '@utils/logWrapper'; import { getConnectionHintNoticeField } from '@utils/sharedFields'; @@ -110,7 +110,11 @@ export class MemoryXata implements INodeType { if (nodeVersion >= 1.2) { sessionId = getSessionId(this, itemIndex); } else { - sessionId = this.getNodeParameter('sessionId', itemIndex) as string; + sessionId = coerceSessionIdToString( + this, + this.getNodeParameter('sessionId', itemIndex), + itemIndex, + ); } const xataClient = new BaseClient({ diff --git a/packages/@n8n/nodes-langchain/nodes/memory/MemoryZep/MemoryZep.node.ts b/packages/@n8n/nodes-langchain/nodes/memory/MemoryZep/MemoryZep.node.ts index 8e13570488c..e21012008d1 100644 --- a/packages/@n8n/nodes-langchain/nodes/memory/MemoryZep/MemoryZep.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/memory/MemoryZep/MemoryZep.node.ts @@ -12,7 +12,7 @@ import { NodeOperationError, } from 'n8n-workflow'; -import { getSessionId } from '@utils/helpers'; +import { coerceSessionIdToString, getSessionId } from '@utils/helpers'; import { logWrapper } from '@utils/logWrapper'; import { getConnectionHintNoticeField } from '@utils/sharedFields'; @@ -132,7 +132,11 @@ export class MemoryZep implements INodeType { if (nodeVersion >= 1.2) { sessionId = getSessionId(this, itemIndex); } else { - sessionId = this.getNodeParameter('sessionId', itemIndex) as string; + sessionId = coerceSessionIdToString( + this, + this.getNodeParameter('sessionId', itemIndex), + itemIndex, + ); } let memory: BaseChatMemory; diff --git a/packages/@n8n/nodes-langchain/utils/helpers.ts b/packages/@n8n/nodes-langchain/utils/helpers.ts index 5cca97ae4c1..118978f97fd 100644 --- a/packages/@n8n/nodes-langchain/utils/helpers.ts +++ b/packages/@n8n/nodes-langchain/utils/helpers.ts @@ -101,6 +101,26 @@ export function getPromptInputByType(options: { return input; } +// The session key can arrive straight from an untrusted request body (e.g. a +// public chat trigger). It is used to address stored data in the memory +// backend, so it must be a primitive value: an object or array would be +// interpreted as query structure by backends like MongoDB. Coerce numbers and +// booleans to strings and reject anything non-primitive. +export function coerceSessionIdToString( + ctx: ISupplyDataFunctions | IWebhookFunctions, + sessionId: unknown, + itemIndex: number, +): string { + if (typeof sessionId === 'string') return sessionId; + if (typeof sessionId === 'number' || typeof sessionId === 'boolean') { + return String(sessionId); + } + throw new NodeOperationError(ctx.getNode(), 'Invalid session ID', { + description: 'The session ID must be a text value', + itemIndex, + }); +} + export function getSessionId( ctx: ISupplyDataFunctions | IWebhookFunctions, itemIndex: number, @@ -114,43 +134,48 @@ export function getSessionId( if (selectorType === autoSelect) { // If memory node is used in webhook like node(like chat trigger node), it doesn't have access to evaluateExpression // so we try to extract sessionId from the bodyData + let rawSessionId: unknown; if ('getBodyData' in ctx) { const bodyData = ctx.getBodyData() ?? {}; - sessionId = bodyData.sessionId as string; + rawSessionId = bodyData.sessionId; } else { - sessionId = ctx.evaluateExpression('{{ $json.sessionId }}', itemIndex) as string; + rawSessionId = ctx.evaluateExpression('{{ $json.sessionId }}', itemIndex); // try to get sessionId from chat trigger - if (!sessionId || sessionId === undefined) { + if (rawSessionId === '' || rawSessionId === undefined || rawSessionId === null) { try { const chatTrigger = ctx.getChatTrigger(); if (chatTrigger) { - sessionId = ctx.evaluateExpression( + rawSessionId = ctx.evaluateExpression( `{{ $('${chatTrigger.name}').first().json.sessionId }}`, itemIndex, - ) as string; + ); } } catch (error) {} } } - if (sessionId === '' || sessionId === undefined) { + if (rawSessionId === '' || rawSessionId === undefined || rawSessionId === null) { throw new NodeOperationError(ctx.getNode(), 'No session ID found', { description: "Expected to find the session ID in an input field called 'sessionId' (this is what the chat trigger node outputs). To use something else, change the 'Session ID' parameter", itemIndex, }); } + + sessionId = coerceSessionIdToString(ctx, rawSessionId, itemIndex); } else { - sessionId = ctx.getNodeParameter(customKey, itemIndex, '') as string; - if (sessionId === '' || sessionId === undefined) { + const rawSessionId = ctx.getNodeParameter(customKey, itemIndex, ''); + if (rawSessionId === '' || rawSessionId === undefined || rawSessionId === null) { throw new NodeOperationError(ctx.getNode(), 'Key parameter is empty', { description: "Provide a key to use as session ID in the 'Key' parameter or use the 'Connected Chat Trigger Node' option to use the session ID from your Chat Trigger", itemIndex, }); } + + sessionId = coerceSessionIdToString(ctx, rawSessionId, itemIndex); } return sessionId; diff --git a/packages/@n8n/nodes-langchain/utils/tests/helpers.test.ts b/packages/@n8n/nodes-langchain/utils/tests/helpers.test.ts index 01b80044eca..5cb941c92be 100644 --- a/packages/@n8n/nodes-langchain/utils/tests/helpers.test.ts +++ b/packages/@n8n/nodes-langchain/utils/tests/helpers.test.ts @@ -486,6 +486,55 @@ describe('getSessionId', () => { expect(sessionId).toBe('12345'); }); + it('should reject a non-primitive sessionId from bodyData', () => { + mockCtx.getBodyData = jest.fn(); + mockCtx.getNodeParameter.mockReturnValue('fromInput'); + mockCtx.getBodyData.mockReturnValue({ sessionId: { $ne: null } }); + + expect(() => getSessionId(mockCtx, 0)).toThrow(NodeOperationError); + }); + + it('should reject an array sessionId from bodyData', () => { + mockCtx.getBodyData = jest.fn(); + mockCtx.getNodeParameter.mockReturnValue('fromInput'); + mockCtx.getBodyData.mockReturnValue({ sessionId: [{ $regex: '^a' }] }); + + expect(() => getSessionId(mockCtx, 0)).toThrow(NodeOperationError); + }); + + it('should coerce a numeric sessionId from bodyData to a string', () => { + mockCtx.getBodyData = jest.fn(); + mockCtx.getNodeParameter.mockReturnValue('fromInput'); + mockCtx.getBodyData.mockReturnValue({ sessionId: 12345 }); + + const sessionId = getSessionId(mockCtx, 0); + expect(sessionId).toBe('12345'); + }); + + it('should coerce a boolean sessionId from bodyData to a string', () => { + mockCtx.getBodyData = jest.fn(); + mockCtx.getNodeParameter.mockReturnValue('fromInput'); + mockCtx.getBodyData.mockReturnValue({ sessionId: false }); + + const sessionId = getSessionId(mockCtx, 0); + expect(sessionId).toBe('false'); + }); + + it('should throw "No session ID found" for a null sessionId from bodyData', () => { + mockCtx.getBodyData = jest.fn(); + mockCtx.getNodeParameter.mockReturnValue('fromInput'); + mockCtx.getBodyData.mockReturnValue({ sessionId: null }); + + expect(() => getSessionId(mockCtx, 0)).toThrow(NodeOperationError); + }); + + it('should reject a non-primitive sessionId resolved from an expression', () => { + mockCtx.getNodeParameter.mockReturnValue('fromInput'); + mockCtx.evaluateExpression.mockReturnValue({ $ne: null }); + + expect(() => getSessionId(mockCtx, 0)).toThrow(NodeOperationError); + }); + it('should retrieve sessionId from chat trigger', () => { mockCtx.getNodeParameter.mockReturnValue('fromInput'); mockCtx.evaluateExpression.mockReturnValueOnce(undefined); diff --git a/packages/@n8n/task-runner-python/src/task_analyzer.py b/packages/@n8n/task-runner-python/src/task_analyzer.py index b347a404193..a7f14446537 100644 --- a/packages/@n8n/task-runner-python/src/task_analyzer.py +++ b/packages/@n8n/task-runner-python/src/task_analyzer.py @@ -52,6 +52,10 @@ class SecurityValidator(ast.NodeVisitor): self._add_violation( node.lineno, ERROR_DANGEROUS_NAME.format(name=alias.asname) ) + elif alias.asname and alias.asname in BLOCKED_ATTRIBUTES: + self._add_violation( + node.lineno, ERROR_DANGEROUS_ATTRIBUTE.format(attr=alias.asname) + ) self.generic_visit(node) def visit_ImportFrom(self, node: ast.ImportFrom) -> None: @@ -67,10 +71,18 @@ class SecurityValidator(ast.NodeVisitor): self._add_violation( node.lineno, ERROR_DANGEROUS_NAME.format(name=alias.asname) ) + elif alias.asname and alias.asname in BLOCKED_ATTRIBUTES: + self._add_violation( + node.lineno, ERROR_DANGEROUS_ATTRIBUTE.format(attr=alias.asname) + ) elif alias.name in BLOCKED_NAMES: self._add_violation( node.lineno, ERROR_DANGEROUS_NAME.format(name=alias.name) ) + elif alias.name in BLOCKED_ATTRIBUTES: + self._add_violation( + node.lineno, ERROR_DANGEROUS_ATTRIBUTE.format(attr=alias.name) + ) self.generic_visit(node) diff --git a/packages/@n8n/task-runner-python/tests/unit/test_task_analyzer.py b/packages/@n8n/task-runner-python/tests/unit/test_task_analyzer.py index 40b20e453a8..295615e8a3e 100644 --- a/packages/@n8n/task-runner-python/tests/unit/test_task_analyzer.py +++ b/packages/@n8n/task-runner-python/tests/unit/test_task_analyzer.py @@ -342,6 +342,33 @@ class TestImportAliasValidation(TestTaskAnalyzer): analyzer.validate(code) assert name in exc_info.value.description + def test_import_alias_using_blocked_attribute_rejected( + self, analyzer: TaskAnalyzer + ) -> None: + for name in BLOCKED_ATTRIBUTES: + code = f"import json as {name}" + with pytest.raises(SecurityViolationError) as exc_info: + analyzer.validate(code) + assert name in exc_info.value.description + + def test_from_import_alias_using_blocked_attribute_rejected( + self, analyzer: TaskAnalyzer + ) -> None: + for name in BLOCKED_ATTRIBUTES: + code = f"from json import dumps as {name}" + with pytest.raises(SecurityViolationError) as exc_info: + analyzer.validate(code) + assert name in exc_info.value.description + + def test_from_import_name_using_blocked_attribute_rejected( + self, analyzer: TaskAnalyzer + ) -> None: + for name in BLOCKED_ATTRIBUTES: + code = f"from math import {name}" + with pytest.raises(SecurityViolationError) as exc_info: + analyzer.validate(code) + assert name in exc_info.value.description + def test_safe_import_aliases_allowed(self, analyzer: TaskAnalyzer) -> None: safe = [ "import json as j", diff --git a/packages/cli/src/credentials/__tests__/credentials.controller.test.ts b/packages/cli/src/credentials/__tests__/credentials.controller.test.ts index 6befa6109ef..fd87f3b394d 100644 --- a/packages/cli/src/credentials/__tests__/credentials.controller.test.ts +++ b/packages/cli/src/credentials/__tests__/credentials.controller.test.ts @@ -1,3 +1,4 @@ +import type { TestCredentialRequestDto } from '@n8n/api-types'; import type { AuthenticatedRequest, CredentialsEntity, @@ -131,6 +132,90 @@ describe('CredentialsController', () => { }); }); + describe('testCredentials', () => { + const storedCredential = mock({ + id: 'credential-id', + name: 'Stored Credential', + type: 'githubApi', + }); + const decryptedData = { accessToken: 'stored-token' }; + const testResult = { status: 'OK', message: 'Credential tested successfully' } as any; + + beforeEach(() => { + credentialsFinderService.findCredentialForUser.mockResolvedValue(storedCredential); + jest.mocked(credentialsService.decrypt).mockReturnValue(decryptedData); + jest + .spyOn(credentialsService, 'replaceCredentialContentsForSharee') + .mockResolvedValue(undefined); + jest.spyOn(credentialsService, 'unredact').mockReturnValue(decryptedData); + jest.spyOn(credentialsService, 'test').mockResolvedValue(testResult); + }); + + it('discards a caller-supplied homeProject and resolves the owning project from storage', async () => { + const owningProject = createRawProjectData({ id: 'real-owning-project-id', type: 'team' }); + sharedCredentialsRepository.findCredentialOwningProject.mockResolvedValue(owningProject); + + const payload = { + credentials: { + id: 'credential-id', + name: 'Stored Credential', + type: 'githubApi', + // A caller naming a project it doesn't own — this must never + // reach the credential test. + homeProject: { + id: 'attacker-chosen-project-id', + name: 'Attacker Project', + icon: null, + type: 'team', + createdAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-01T00:00:00.000Z', + }, + data: { accessToken: 'live-token' }, + }, + } as unknown as TestCredentialRequestDto; + + await credentialsController.testCredentials(req, res, payload); + + expect(sharedCredentialsRepository.findCredentialOwningProject).toHaveBeenCalledWith( + 'credential-id', + ); + expect(credentialsService.test).toHaveBeenCalledWith( + req.user.id, + expect.objectContaining({ + homeProject: expect.objectContaining({ id: 'real-owning-project-id' }), + }), + ); + }); + + it('does not fall back to a caller-supplied homeProject when no owning project is found', async () => { + sharedCredentialsRepository.findCredentialOwningProject.mockResolvedValue(undefined); + + const payload = { + credentials: { + id: 'credential-id', + name: 'Stored Credential', + type: 'githubApi', + homeProject: { + id: 'attacker-chosen-project-id', + name: 'Attacker Project', + icon: null, + type: 'team', + createdAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-01T00:00:00.000Z', + }, + data: { accessToken: 'live-token' }, + }, + } as unknown as TestCredentialRequestDto; + + await credentialsController.testCredentials(req, res, payload); + + expect(credentialsService.test).toHaveBeenCalledWith( + req.user.id, + expect.objectContaining({ homeProject: undefined }), + ); + }); + }); + describe('updateCredentials', () => { const credentialId = 'cred-123'; const existingCredential = mock({ diff --git a/packages/cli/src/credentials/credentials.controller.ts b/packages/cli/src/credentials/credentials.controller.ts index 3ddcc5c8bc2..669307355b7 100644 --- a/packages/cli/src/credentials/credentials.controller.ts +++ b/packages/cli/src/credentials/credentials.controller.ts @@ -3,6 +3,7 @@ import { CredentialsGetManyRequestQuery, CredentialsGetOneRequestQuery, GenerateCredentialNameRequestQuery, + TestCredentialRequestDto, } from '@n8n/api-types'; import { Logger } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; @@ -29,7 +30,7 @@ import { hasGlobalScope, PROJECT_OWNER_ROLE_SLUG } from '@n8n/permissions'; // eslint-disable-next-line n8n-local-rules/misplaced-n8n-typeorm-import import { In } from '@n8n/typeorm'; import { deepCopy } from 'n8n-workflow'; -import type { ICredentialDataDecryptedObject } from 'n8n-workflow'; +import type { ICredentialDataDecryptedObject, ICredentialsDecrypted } from 'n8n-workflow'; import { z } from 'zod'; import { CredentialsFinderService } from './credentials-finder.service'; @@ -135,8 +136,12 @@ export class CredentialsController { // TODO: Write at least test cases for the failure paths. @Post('/test') - async testCredentials(req: CredentialRequest.Test) { - const { credentials } = req.body; + async testCredentials( + req: AuthenticatedRequest, + _res: unknown, + @Body payload: TestCredentialRequestDto, + ) { + const { credentials } = payload; const storedCredential = await this.credentialsFinderService.findCredentialForUser( credentials.id, @@ -148,9 +153,29 @@ export class CredentialsController { throw new ForbiddenError(); } - const mergedCredentials = deepCopy(credentials); + const mergedCredentials: ICredentialsDecrypted = deepCopy({ + ...credentials, + data: credentials.data as ICredentialDataDecryptedObject | undefined, + }); const decryptedData = this.credentialsService.decrypt(storedCredential, true); + // Find the owning project to prevent leakage of other project data. + const owningProject = await this.sharedCredentialsRepository.findCredentialOwningProject( + storedCredential.id, + ); + if (!owningProject) { + mergedCredentials.homeProject = undefined; + } else { + mergedCredentials.homeProject = { + id: owningProject.id, + name: owningProject.name, + icon: owningProject.icon, + type: owningProject.type, + createdAt: owningProject.createdAt.toISOString(), + updatedAt: owningProject.updatedAt.toISOString(), + }; + } + // When a sharee (or project viewer) opens a credential, the fields and the // credential data are missing so the payload will be empty // We need to replace the credential contents with the db version if that's the case diff --git a/packages/cli/src/modules/community-packages/__tests__/community-packages.service.pubsub.test.ts b/packages/cli/src/modules/community-packages/__tests__/community-packages.service.pubsub.test.ts new file mode 100644 index 00000000000..6690144be3c --- /dev/null +++ b/packages/cli/src/modules/community-packages/__tests__/community-packages.service.pubsub.test.ts @@ -0,0 +1,411 @@ +import type { Logger } from '@n8n/backend-common'; +import { mockInstance } from '@n8n/backend-test-utils'; +import axios from 'axios'; +import { mocked } from 'jest-mock'; +import { mock } from 'jest-mock-extended'; +import type { InstanceSettings, PackageDirectoryLoader } from 'n8n-core'; +import { execFile } from 'node:child_process'; +import { readFile, rm, writeFile } from 'node:fs/promises'; +import path from 'node:path'; + +import { NPM_PACKAGE_STATUS_GOOD } from '@/constants'; +import type { License } from '@/license'; +import type { LoadNodesAndCredentials } from '@/load-nodes-and-credentials'; +import type { Publisher } from '@/scaling/pubsub/publisher.service'; + +import type { CommunityPackagesConfig } from '../community-packages.config'; +import { CommunityPackagesService } from '../community-packages.service'; +import { InstalledNodesRepository } from '../installed-nodes.repository'; +import type { InstalledPackages } from '../installed-packages.entity'; +import { InstalledPackagesRepository } from '../installed-packages.repository'; +import { checkIfVersionExistsOrThrow, verifyIntegrity } from '../npm-utils'; + +jest.mock('node:fs/promises'); +jest.mock('node:child_process'); +jest.mock('axios'); +jest.mock('../community-node-types-utils', () => ({ + getCommunityNodeTypes: jest.fn().mockResolvedValue([]), +})); +jest.mock('../npm-utils', () => ({ + checkIfVersionExistsOrThrow: jest.fn().mockResolvedValue(true), + verifyIntegrity: jest.fn().mockResolvedValue(undefined), +})); + +type ExecFileCallback = NonNullable[3]>; + +describe('CommunityPackagesService pubsub handlers', () => { + const PACKAGE_NAME = 'n8n-nodes-test'; + const PACKAGE_VERSION = '1.0.0'; + const REGISTRY = 'https://registry.npmjs.org'; + const TARBALL_NAME = `${PACKAGE_NAME}-1.0.0.tgz`; + + const nodesDownloadDir = path.join('tmp', 'n8n-jest-pubsub-downloads'); + const instanceSettings = mock({ nodesDownloadDir }); + const logger = mock(); + const publisher = mock(); + const license = mock(); + const loadNodesAndCredentials = mock(); + + mockInstance(InstalledNodesRepository); + const installedPackageRepository = mockInstance(InstalledPackagesRepository); + + const createService = (configOverrides: Partial = {}) => { + const config = mock({ + enabled: true, + preventLoading: false, + reinstallMissing: false, + registry: REGISTRY, + unverifiedEnabled: true, + ...configOverrides, + }); + + return new CommunityPackagesService( + instanceSettings, + logger, + installedPackageRepository, + loadNodesAndCredentials, + publisher, + license, + config, + ); + }; + + const execMock = ((...args: Parameters) => { + const command = args[0]; + const cmdArgs = args[1]; + const callback = args[args.length - 1] as ExecFileCallback; + + if (command === 'npm' && cmdArgs?.[0] === 'pack') { + callback(null, { stdout: TARBALL_NAME } as never, ''); + } else { + callback(null, 'Done', ''); + } + }) as typeof execFile; + + beforeEach(() => { + jest.resetAllMocks(); + + mocked(execFile).mockImplementation(execMock); + mocked(checkIfVersionExistsOrThrow).mockResolvedValue(true); + mocked(verifyIntegrity).mockResolvedValue(undefined); + + mocked(readFile).mockResolvedValue( + JSON.stringify({ name: PACKAGE_NAME, version: PACKAGE_VERSION, dependencies: {} }), + ); + mocked(writeFile).mockResolvedValue(undefined); + mocked(rm).mockResolvedValue(undefined); + + mocked(axios.post).mockResolvedValue({ data: { status: NPM_PACKAGE_STATUS_GOOD } }); + + license.isCustomNpmRegistryEnabled.mockReturnValue(true); + loadNodesAndCredentials.loadPackage.mockResolvedValue( + mock({ + loadedNodes: [{ name: 'a-node-from-the-loader', version: 1 }], + }), + ); + loadNodesAndCredentials.unloadPackage.mockResolvedValue(undefined); + loadNodesAndCredentials.postProcessLoaders.mockResolvedValue(undefined); + + installedPackageRepository.findOne.mockResolvedValue(null); + }); + + const arrangeInstalledPackage = ( + overrides: Partial> = {}, + ) => { + installedPackageRepository.findOne.mockResolvedValue( + mock({ + packageName: PACKAGE_NAME, + installedVersion: PACKAGE_VERSION, + ...overrides, + }), + ); + }; + + const expectPack = (packageName: string, packageVersion: string) => { + expect(execFile).toHaveBeenCalledWith( + 'npm', + ['pack', `${packageName}@${packageVersion}`, `--registry=${REGISTRY}`, '--quiet'], + { cwd: nodesDownloadDir }, + expect.any(Function), + ); + }; + + const expectNoDownloadOrLoad = () => { + expect(execFile).not.toHaveBeenCalled(); + expect(loadNodesAndCredentials.loadPackage).not.toHaveBeenCalled(); + }; + + const expectDownloadAndLoad = (packageName: string) => { + expectPack(packageName, PACKAGE_VERSION); + expect(loadNodesAndCredentials.loadPackage).toHaveBeenCalledWith(packageName); + expect(loadNodesAndCredentials.postProcessLoaders).toHaveBeenCalled(); + }; + + test.each([PACKAGE_NAME, `@scope/${PACKAGE_NAME}`])( + 'should download and load "%s" for a valid install event', + async (packageName) => { + const service = createService(); + arrangeInstalledPackage({ packageName }); + + await service.handleInstallEvent({ packageName, packageVersion: PACKAGE_VERSION }); + + expectDownloadAndLoad(packageName); + }, + ); + + test('should install a checksum-verified package when unverified packages are disabled', async () => { + const service = createService({ unverifiedEnabled: false }); + arrangeInstalledPackage(); + + await service.handleInstallEvent({ + packageName: PACKAGE_NAME, + packageVersion: PACKAGE_VERSION, + checksum: 'sha512-abc123', + }); + + expect(verifyIntegrity).toHaveBeenCalledWith( + PACKAGE_NAME, + PACKAGE_VERSION, + REGISTRY, + 'sha512-abc123', + ); + expectDownloadAndLoad(PACKAGE_NAME); + }); + + test('should not install a package whose checksum does not match', async () => { + const service = createService(); + arrangeInstalledPackage(); + mocked(verifyIntegrity).mockRejectedValue(new Error('Checksum mismatch')); + + await expect( + service.handleInstallEvent({ + packageName: PACKAGE_NAME, + packageVersion: PACKAGE_VERSION, + checksum: 'sha512-abc123', + }), + ).resolves.toBeUndefined(); + + expectNoDownloadOrLoad(); + }); + + test.each([ + ['leading segment is not a valid scope', `../${PACKAGE_NAME}`], + ['required prefix is missing', 'some-other-package'], + ['name is not a single word', 'n8n-nodes-With Space'], + ])('should not install a package when the %s', async (_label, packageName) => { + const service = createService(); + // Arranged so the stored-record guard passes and only the name guard can refuse. + arrangeInstalledPackage({ packageName }); + + await expect( + service.handleInstallEvent({ packageName, packageVersion: PACKAGE_VERSION }), + ).resolves.toBeUndefined(); + + expectNoDownloadOrLoad(); + }); + + test('should not install a package whose stored version specifier is invalid', async () => { + const service = createService(); + arrangeInstalledPackage({ installedVersion: '1.a.b' }); + + await expect( + service.handleInstallEvent({ packageName: PACKAGE_NAME, packageVersion: '1.a.b' }), + ).resolves.toBeUndefined(); + + expectNoDownloadOrLoad(); + }); + + test('should not install an unverified package when unverified packages are disabled', async () => { + const service = createService({ unverifiedEnabled: false }); + arrangeInstalledPackage(); + + await expect( + service.handleInstallEvent({ packageName: PACKAGE_NAME, packageVersion: PACKAGE_VERSION }), + ).resolves.toBeUndefined(); + + expectNoDownloadOrLoad(); + }); + + test('should not install a package rejected by the package status check', async () => { + const service = createService(); + arrangeInstalledPackage(); + mocked(axios.post).mockResolvedValue({ data: { status: 'Banned', reason: 'not allowed' } }); + + await expect( + service.handleInstallEvent({ packageName: PACKAGE_NAME, packageVersion: PACKAGE_VERSION }), + ).resolves.toBeUndefined(); + + expectNoDownloadOrLoad(); + }); + + test('should not install a version that does not exist in the registry', async () => { + const service = createService(); + arrangeInstalledPackage({ installedVersion: '9.9.9' }); + mocked(checkIfVersionExistsOrThrow).mockRejectedValue(new Error('Version not found')); + + await expect( + service.handleInstallEvent({ packageName: PACKAGE_NAME, packageVersion: '9.9.9' }), + ).resolves.toBeUndefined(); + + expectNoDownloadOrLoad(); + }); + + test('should not install a package without a stored record', async () => { + const service = createService(); + + await expect( + service.handleInstallEvent({ packageName: PACKAGE_NAME, packageVersion: PACKAGE_VERSION }), + ).resolves.toBeUndefined(); + + expectNoDownloadOrLoad(); + }); + + test('should install the version from the stored record when the payload version differs', async () => { + const service = createService(); + arrangeInstalledPackage({ installedVersion: '2.5.0' }); + + await service.handleInstallEvent({ packageName: PACKAGE_NAME, packageVersion: '9.9.9' }); + + expect(checkIfVersionExistsOrThrow).toHaveBeenCalledWith(PACKAGE_NAME, '2.5.0', REGISTRY); + expectPack(PACKAGE_NAME, '2.5.0'); + expect(loadNodesAndCredentials.loadPackage).toHaveBeenCalledWith(PACKAGE_NAME); + }); + + test('should install without the payload checksum when the payload version differs', async () => { + const service = createService(); + arrangeInstalledPackage({ installedVersion: '2.5.0' }); + + await service.handleInstallEvent({ + packageName: PACKAGE_NAME, + packageVersion: '9.9.9', + checksum: 'sha512-abc123', + }); + + expect(verifyIntegrity).not.toHaveBeenCalled(); + expectPack(PACKAGE_NAME, '2.5.0'); + expect(loadNodesAndCredentials.loadPackage).toHaveBeenCalledWith(PACKAGE_NAME); + }); + + const disabledInstanceCases: Array<[string, Partial]> = [ + ['community packages are disabled', { enabled: false }], + ['package loading is disabled', { preventLoading: true }], + ]; + + test.each(disabledInstanceCases)( + 'should not install a package when %s', + async (_label, configOverrides) => { + const service = createService(configOverrides); + arrangeInstalledPackage(); + + await expect( + service.handleInstallEvent({ packageName: PACKAGE_NAME, packageVersion: PACKAGE_VERSION }), + ).resolves.toBeUndefined(); + + expectNoDownloadOrLoad(); + }, + ); + + const expectNoRemoval = () => { + expect(rm).not.toHaveBeenCalled(); + expect(loadNodesAndCredentials.unloadPackage).not.toHaveBeenCalled(); + }; + + test('should delete and unload a package for a valid uninstall event', async () => { + const service = createService(); + + await service.handleUninstallEvent({ packageName: PACKAGE_NAME }); + + expect(rm).toHaveBeenCalledWith(`${nodesDownloadDir}/node_modules/${PACKAGE_NAME}`, { + recursive: true, + force: true, + }); + expect(loadNodesAndCredentials.unloadPackage).toHaveBeenCalledWith(PACKAGE_NAME); + expect(loadNodesAndCredentials.postProcessLoaders).toHaveBeenCalled(); + }); + + test.each(disabledInstanceCases)( + 'should not uninstall a package when %s', + async (_label, configOverrides) => { + const service = createService(configOverrides); + + await expect( + service.handleUninstallEvent({ packageName: PACKAGE_NAME }), + ).resolves.toBeUndefined(); + + expectNoRemoval(); + }, + ); + + test('should not uninstall a package while a stored record exists', async () => { + const service = createService(); + arrangeInstalledPackage(); + + await expect( + service.handleUninstallEvent({ packageName: PACKAGE_NAME }), + ).resolves.toBeUndefined(); + + expectNoRemoval(); + }); + + test.each([ + `../${PACKAGE_NAME}`, + '../../etc', + 'some-other-package', + `@scope/${PACKAGE_NAME}/..`, + `${PACKAGE_NAME}/../../node_modules/n8n-nodes-other`, + ])('should not uninstall the package "%s"', async (packageName) => { + const service = createService(); + + await expect(service.handleUninstallEvent({ packageName })).resolves.toBeUndefined(); + + expectNoRemoval(); + }); + + describe('logging', () => { + test('should log a refused command as skipped', async () => { + const service = createService(); + + await service.handleInstallEvent({ + packageName: PACKAGE_NAME, + packageVersion: PACKAGE_VERSION, + }); + + expect(logger.warn).toHaveBeenCalledWith( + 'Skipped installing community package', + expect.objectContaining({ packageName: PACKAGE_NAME }), + ); + expect(logger.error).not.toHaveBeenCalled(); + }); + + test('should log a command that failed while installing as an error', async () => { + const service = createService(); + arrangeInstalledPackage(); + mocked(execFile).mockImplementation(((...args: Parameters) => { + const callback = args[args.length - 1] as ExecFileCallback; + callback(new Error('npm exploded'), '', ''); + }) as typeof execFile); + + await service.handleInstallEvent({ + packageName: PACKAGE_NAME, + packageVersion: PACKAGE_VERSION, + }); + + expect(logger.error).toHaveBeenCalledWith( + 'Failed to install community package', + expect.objectContaining({ packageName: PACKAGE_NAME }), + ); + }); + + test('should log a command that failed while uninstalling as an error', async () => { + const service = createService(); + mocked(rm).mockRejectedValue(new Error('permission denied')); + + await service.handleUninstallEvent({ packageName: PACKAGE_NAME }); + + expect(logger.error).toHaveBeenCalledWith( + 'Failed to uninstall community package', + expect.objectContaining({ packageName: PACKAGE_NAME }), + ); + }); + }); +}); diff --git a/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts b/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts index b31adc177d9..4663643c554 100644 --- a/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts +++ b/packages/cli/src/modules/community-packages/__tests__/community-packages.service.test.ts @@ -402,6 +402,7 @@ describe('CommunityPackagesService', () => { const PACKAGE_NAME = 'n8n-nodes-test'; const installedPackageForUpdateTest = mock({ packageName: PACKAGE_NAME, + installedVersion: '1.0.0', }); const packageDirectoryLoader = mock({ @@ -533,7 +534,7 @@ describe('CommunityPackagesService', () => { expect(publisher.publishCommand).toHaveBeenCalledWith({ command: 'community-package-update', - payload: { packageName: PACKAGE_NAME, packageVersion: 'latest' }, + payload: { packageName: PACKAGE_NAME, packageVersion: '1.0.0', checksum: undefined }, }); }); @@ -554,6 +555,8 @@ describe('CommunityPackagesService', () => { }); describe('installPackage', () => { + const PACKAGE_NAME = 'n8n-nodes-test'; + test('should throw when installation of not vetted packages is forbidden', async () => { config.unverifiedEnabled = false; config.registry = 'https://registry.npmjs.org'; @@ -561,6 +564,67 @@ describe('CommunityPackagesService', () => { 'Installation of unverified community packages is forbidden!', ); }); + + test('should throw when the package is rejected by the package status check', async () => { + config.unverifiedEnabled = true; + config.registry = 'https://registry.npmjs.org'; + mocked(axios.post).mockResolvedValue({ data: { status: 'Banned', reason: 'not allowed' } }); + + await expect(communityPackagesService.installPackage(PACKAGE_NAME, '1.0.0')).rejects.toThrow( + `Package "${PACKAGE_NAME}" is not allowed`, + ); + + expect(execFile).not.toHaveBeenCalled(); + }); + + test('should throw for a package name that resolves outside its own directory', async () => { + config.unverifiedEnabled = true; + config.registry = 'https://registry.npmjs.org'; + + // `installPackage` does not parse the name, which is why the resolved directory is + // asserted to be the named one. + await expect( + communityPackagesService.installPackage(`${PACKAGE_NAME}/..`, '1.0.0'), + ).rejects.toThrow('Invalid package name'); + + expect(mkdir).not.toHaveBeenCalled(); + expect(rm).not.toHaveBeenCalled(); + }); + + test('should publish the installed version rather than the requested specifier', async () => { + config.unverifiedEnabled = true; + config.registry = 'https://registry.npmjs.org'; + + mocked(execFile).mockImplementation(((...args: Parameters) => { + const cmdArgs = args[1]; + const actualCallback = args[args.length - 1] as ExecFileCallback; + + if (cmdArgs?.[0] === 'pack') { + actualCallback(null, { stdout: `${PACKAGE_NAME}-latest.tgz` } as never, ''); + } else { + actualCallback(null, 'Done', ''); + } + }) as typeof execFile); + + mocked(readFile).mockResolvedValue( + JSON.stringify({ name: PACKAGE_NAME, version: '1.2.3', dependencies: {} }), + ); + loadNodesAndCredentials.loadPackage.mockResolvedValue( + mock({ + loadedNodes: [{ name: 'a-node-from-the-loader', version: 1 }], + }), + ); + installedPackageRepository.saveInstalledPackageWithNodes.mockResolvedValue( + mock({ packageName: PACKAGE_NAME, installedVersion: '1.2.3' }), + ); + + await communityPackagesService.installPackage(PACKAGE_NAME); + + expect(publisher.publishCommand).toHaveBeenCalledWith({ + command: 'community-package-install', + payload: { packageName: PACKAGE_NAME, packageVersion: '1.2.3', checksum: undefined }, + }); + }); }); describe('ensurePackageJson', () => { diff --git a/packages/cli/src/modules/community-packages/community-packages.service.ts b/packages/cli/src/modules/community-packages/community-packages.service.ts index ecbbcce4b17..fbdd0c85e89 100644 --- a/packages/cli/src/modules/community-packages/community-packages.service.ts +++ b/packages/cli/src/modules/community-packages/community-packages.service.ts @@ -14,7 +14,7 @@ import { } from 'n8n-workflow'; import { execFile } from 'node:child_process'; import { access, constants, mkdir, readFile, rm, writeFile } from 'node:fs/promises'; -import { join } from 'node:path'; +import { join, relative, resolve, sep } from 'node:path'; import { promisify } from 'node:util'; import { @@ -70,6 +70,11 @@ const INVALID_OR_SUSPICIOUS_PACKAGE_NAME = /[^0-9a-z@\-._/]/; /** Built-in package names cannot be installed as community packages. */ const RESERVED_PACKAGE_NAMES = new Set(BUILTIN_NODES_PACKAGES); +const NPM_PACKAGE_SCOPE_PATTERN = /^@[a-z0-9][a-z0-9\-._]*$/; + +/** A package name holds no path separator, so it can never widen the resolved directory. */ +const NPM_PACKAGE_NAME_PATTERN = /^[a-z0-9][a-z0-9\-._]*$/; + type PackageJson = { name: 'installed-nodes'; private: true; @@ -146,6 +151,10 @@ export class CommunityPackagesService { const scope = rawString.includes('/') ? rawString.split('/')[0] : undefined; + if (scope && !NPM_PACKAGE_SCOPE_PATTERN.test(scope)) { + throw new UnexpectedError(`Invalid package scope: ${scope}`); + } + const packageNameWithoutScope = scope ? rawString.replace(`${scope}/`, '') : rawString; if (!packageNameWithoutScope.startsWith(NODE_PACKAGE_PREFIX)) { @@ -162,6 +171,12 @@ export class CommunityPackagesService { const packageName = version ? rawString.replace(`@${version}`, '') : rawString; + const nameWithoutScopeOrVersion = scope ? packageName.replace(`${scope}/`, '') : packageName; + + if (!NPM_PACKAGE_NAME_PATTERN.test(nameWithoutScopeOrVersion)) { + throw new UnexpectedError(`Invalid package name: ${packageName}`); + } + if (RESERVED_PACKAGE_NAMES.has(packageName)) { throw new UserError(`Package name "${packageName}" is reserved for n8n built-in packages`); } @@ -416,6 +431,24 @@ export class CommunityPackagesService { } } + private async runRegistryChecks(packageName: string, packageVersion: string, checksum?: string) { + this.checkInstallPermissions(Boolean(checksum)); + + const packageStatus = await this.checkNpmPackageStatus(packageName); + + if (packageStatus.status !== NPM_PACKAGE_STATUS_GOOD) { + throw new UnexpectedError(`Package "${packageName}" is not allowed`); + } + + const registry = this.getNpmRegistry(); + + if (checksum) { + await verifyIntegrity(packageName, packageVersion, registry, checksum); + } + + await checkIfVersionExistsOrThrow(packageName, packageVersion, registry); + } + private async installOrUpdatePackage( packageName: string, options: @@ -425,14 +458,7 @@ export class CommunityPackagesService { const isUpdate = 'installedPackage' in options; const packageVersion = !options.version ? 'latest' : options.version; - const shouldValidateChecksum = 'checksum' in options && Boolean(options.checksum); - this.checkInstallPermissions(shouldValidateChecksum); - - if (options.checksum) { - await verifyIntegrity(packageName, packageVersion, this.getNpmRegistry(), options.checksum); - } - - await checkIfVersionExistsOrThrow(packageName, packageVersion, this.getNpmRegistry()); + await this.runRegistryChecks(packageName, packageVersion, options.checksum); try { await this.downloadPackage(packageName, packageVersion); @@ -464,7 +490,11 @@ export class CommunityPackagesService { const installedPackage = await this.persistInstalledPackage(loader); void this.publisher.publishCommand({ command: isUpdate ? 'community-package-update' : 'community-package-install', - payload: { packageName, packageVersion }, + payload: { + packageName, + packageVersion: installedPackage.installedVersion, + checksum: options.checksum, + }, }); await this.loadNodesAndCredentials.postProcessLoaders(); this.logger.info(`Community package installed: ${packageName}`); @@ -484,18 +514,107 @@ export class CommunityPackagesService { } } + private assertPackageChangesAllowed() { + if (!this.config.enabled) { + throw new UserError('Community packages are disabled on this instance'); + } + + if (this.config.preventLoading) { + throw new UserError('Community package loading is disabled on this instance'); + } + } + + // Payloads from other instances pass the same gates as a local install request. Not + // `communityPackagesManagedByEnv`: only the main runs the env loader, and it publishes + // these commands, so a follower that refused them would never get the packages. + // Failures are logged rather than rethrown, so one message cannot break the subscriber. @OnPubSubEvent('community-package-install') @OnPubSubEvent('community-package-update') async handleInstallEvent({ packageName, packageVersion, - }: { packageName: string; packageVersion: string }) { - await this.installOrUpdateNpmPackage(packageName, packageVersion); + checksum, + }: { packageName: string; packageVersion: string; checksum?: string }) { + let accepted = false; + + try { + this.assertPackageChangesAllowed(); + + const parsed = this.parseNpmPackageName(packageName); + + // The payload only says that something needs converging. What gets installed comes + // from the record the publishing instance persisted before it published. + const installedPackage = await this.findInstalledPackage(parsed.packageName); + + if (!installedPackage) { + throw new UnexpectedError('No installed package record found', { + extra: { packageName: parsed.packageName }, + }); + } + + const resolvedVersion = installedPackage.installedVersion; + + if (!valid(resolvedVersion)) { + throw new UnexpectedError(`Invalid version: ${resolvedVersion}`); + } + + const versionMatches = resolvedVersion === packageVersion; + + if (!versionMatches) { + this.logger.warn('Community package version differs from the stored record', { + packageName: installedPackage.packageName, + packageVersion, + installedVersion: resolvedVersion, + }); + } + + // Integrity is recorded per version, so the payload checksum only applies when the + // payload names the version that is about to be installed. + await this.runRegistryChecks( + installedPackage.packageName, + resolvedVersion, + versionMatches ? checksum : undefined, + ); + + accepted = true; + + await this.installOrUpdateNpmPackage(installedPackage.packageName, resolvedVersion); + } catch (error) { + const details = { packageName, reason: ensureError(error).message }; + + if (accepted) this.logger.error('Failed to install community package', details); + else this.logger.warn('Skipped installing community package', details); + } } @OnPubSubEvent('community-package-uninstall') async handleUninstallEvent({ packageName }: { packageName: string }) { - await this.removeNpmPackage(packageName); + let accepted = false; + + try { + this.assertPackageChangesAllowed(); + + const parsed = this.parseNpmPackageName(packageName); + + // The mirror of the install path: the removed record is what says the package is + // meant to be gone, so a payload on its own never removes anything. + const installedPackage = await this.findInstalledPackage(parsed.packageName); + + if (installedPackage) { + throw new UnexpectedError('Installed package record still present', { + extra: { packageName: parsed.packageName }, + }); + } + + accepted = true; + + await this.removeNpmPackage(parsed.packageName); + } catch (error) { + const details = { packageName, reason: ensureError(error).message }; + + if (accepted) this.logger.error('Failed to uninstall community package', details); + else this.logger.warn('Skipped uninstalling community package', details); + } } private async installOrUpdateNpmPackage(packageName: string, packageVersion: string) { @@ -513,7 +632,25 @@ export class CommunityPackagesService { } private resolvePackageDirectory(packageName: string) { - return `${this.downloadFolder}/node_modules/${packageName}`; + const nodeModulesDirectory = `${this.downloadFolder}/node_modules`; + const packageDirectory = `${nodeModulesDirectory}/${packageName}`; + + // Not every caller validates the name, so confirm the resolved path is exactly the + // named directory. A prefix check alone still accepts a name like `@scope/pkg/..`, + // which resolves to the scope directory and stays under node_modules. + const relativePath = relative(resolve(nodeModulesDirectory), resolve(packageDirectory)); + + // An empty relative path is the download folder itself, and a leading `..` is outside + // it. Both of those compare equal to the name that produced them, so reject them first. + if ( + relativePath === '' || + relativePath.startsWith('..') || + relativePath.split(sep).join('/') !== packageName + ) { + throw new UserError('Invalid package name', { extra: { packageName } }); + } + + return packageDirectory; } private async downloadPackage(packageName: string, packageVersion: string): Promise { diff --git a/packages/cli/src/scaling/pubsub/pubsub.event-map.ts b/packages/cli/src/scaling/pubsub/pubsub.event-map.ts index b6629cec11e..3b5161028b9 100644 --- a/packages/cli/src/scaling/pubsub/pubsub.event-map.ts +++ b/packages/cli/src/scaling/pubsub/pubsub.event-map.ts @@ -33,11 +33,13 @@ export type PubSubCommandMap = { 'community-package-install': { packageName: string; packageVersion: string; + checksum?: string; }; 'community-package-update': { packageName: string; packageVersion: string; + checksum?: string; }; 'community-package-uninstall': { diff --git a/packages/cli/src/workflows/workflow.service.ee.ts b/packages/cli/src/workflows/workflow.service.ee.ts index 8ac98fd3cf6..4235adf1cc7 100644 --- a/packages/cli/src/workflows/workflow.service.ee.ts +++ b/packages/cli/src/workflows/workflow.service.ee.ts @@ -20,7 +20,6 @@ import { import { Service } from '@n8n/di'; // eslint-disable-next-line n8n-local-rules/misplaced-n8n-typeorm-import import { In, type EntityManager } from '@n8n/typeorm'; -import omit from 'lodash/omit'; import type { INode, IWorkflowBase, WorkflowId } from 'n8n-workflow'; import { EXECUTE_WORKFLOW_NODE_TYPE, @@ -200,62 +199,70 @@ export class EnterpriseWorkflowService { /** * We only need to check nodes that use credentials the current user cannot access, * since these can be 2 possibilities: - * - Same ID already exist: it's a read only node and therefore cannot be changed + * - It matches exactly one previous node: it's a read only node and therefore cannot be changed * - It's a new node which indicates tampering and therefore must fail saving */ const allowedCredentialIds = credentialsUserHasAccessTo.map((cred) => cred.id); - const nodesWithCredentialsUserDoesNotHaveAccessTo = this.getNodesWithInaccessibleCreds( - newWorkflowVersion, - allowedCredentialIds, + const nodesWithCredentialsUserDoesNotHaveAccessTo = new Set( + this.getNodesWithInaccessibleCreds(newWorkflowVersion, allowedCredentialIds), ); // If there are no nodes with credentials the user does not have access to we can skip the rest - if (nodesWithCredentialsUserDoesNotHaveAccessTo.length === 0) { + if (nodesWithCredentialsUserDoesNotHaveAccessTo.size === 0) { return newWorkflowVersion; } - const previouslyExistingNodeIds = previousWorkflowVersion.nodes.map((node) => node.id); + // Node ids are meant to be unique. Edge case: where stored data still repeats one, match the + // first node. + const previousNodesById = new Map(); + for (const node of previousWorkflowVersion.nodes) { + if (!previousNodesById.has(node.id)) previousNodesById.set(node.id, node); + } - // If it's a new node we can't allow it to be saved - // since it uses creds the node doesn't have access - const isTamperingAttempt = (inaccessibleCredNodeId: string) => - !previouslyExistingNodeIds.includes(inaccessibleCredNodeId); + const submittedIdCounts = new Map(); + for (const node of newWorkflowVersion.nodes) { + submittedIdCounts.set(node.id, (submittedIdCounts.get(node.id) ?? 0) + 1); + } - nodesWithCredentialsUserDoesNotHaveAccessTo.forEach((node) => { - if (isTamperingAttempt(node.id)) { + newWorkflowVersion.nodes = newWorkflowVersion.nodes.map((node) => { + if (!nodesWithCredentialsUserDoesNotHaveAccessTo.has(node)) return node; + + // Only a node we can prove is the stored one may keep credentials the user cannot + // access. A missing id means the node is new; a repeated id makes the match + // ambiguous, and an ambiguous match is not a proof, so no claimant is trusted. + const previousNode = previousNodesById.get(node.id); + const idClaimedOnce = submittedIdCounts.get(node.id) === 1; + + if (!previousNode || !idClaimedOnce) { this.logger.warn('Blocked workflow update due to tampering attempt', { nodeType: node.type, nodeName: node.name, nodeId: node.id, nodeCredentials: node.credentials, }); - // Node is new, so this is probably a tampering attempt. Throw an error throw new NodeOperationError( node, `You don't have access to the credentials in the '${node.name}' node. Ask the owner to share them with you.`, ); } - // Replace the node with the previous version of the node - // Since it cannot be modified (read only node) - const nodeIdx = newWorkflowVersion.nodes.findIndex( - (newWorkflowNode) => newWorkflowNode.id === node.id, - ); this.logger.debug('Replacing node with previous version when saving updated workflow', { nodeType: node.type, nodeName: node.name, nodeId: node.id, }); - const previousNodeVersion = previousWorkflowVersion.nodes.find( - (previousNode) => previousNode.id === node.id, - ); + + // Replace the whole node. Patching the previous fields over the submitted node + // would keep every field the previous node does not have. // Allow changing only name, position and disabled status for read-only nodes - Object.assign( - newWorkflowVersion.nodes[nodeIdx], - omit(previousNodeVersion, ['name', 'position', 'disabled']), - ); + const restoredNode = { ...previousNode }; + if ('name' in node) restoredNode.name = node.name; + if ('position' in node) restoredNode.position = node.position; + if ('disabled' in node) restoredNode.disabled = node.disabled; + else delete restoredNode.disabled; + return restoredNode; }); return newWorkflowVersion; diff --git a/packages/cli/test/integration/credentials/credentials.api.test.ts b/packages/cli/test/integration/credentials/credentials.api.test.ts index d413d3d72ab..49b7d779e2e 100644 --- a/packages/cli/test/integration/credentials/credentials.api.test.ts +++ b/packages/cli/test/integration/credentials/credentials.api.test.ts @@ -1622,8 +1622,10 @@ describe('POST /credentials/test', () => { expect(mockCredentialsTester.testCredentials.mock.calls[0][1]).toBe(savedCredential.type); expect(mockCredentialsTester.testCredentials.mock.calls[0][2]).toEqual({ id: savedCredential.id, + name: '', type: savedCredential.type, data: credential.data, + homeProject: expect.objectContaining({ id: ownerPersonalProject.id }), }); }); @@ -1652,8 +1654,10 @@ describe('POST /credentials/test', () => { expect(mockCredentialsTester.testCredentials.mock.calls[0][1]).toBe(savedCredential.type); expect(mockCredentialsTester.testCredentials.mock.calls[0][2]).toEqual({ id: savedCredential.id, + name: '', type: savedCredential.type, data: credential.data, + homeProject: expect.objectContaining({ id: ownerPersonalProject.id }), }); }); diff --git a/packages/cli/test/integration/workflows/workflow.service.ee.test.ts b/packages/cli/test/integration/workflows/workflow.service.ee.test.ts index 849ceb4a682..35545572751 100644 --- a/packages/cli/test/integration/workflows/workflow.service.ee.test.ts +++ b/packages/cli/test/integration/workflows/workflow.service.ee.test.ts @@ -3,10 +3,12 @@ import { CredentialsEntity, CredentialsRepository, SharedWorkflowRepository, + WorkflowEntity, WorkflowRepository, } from '@n8n/db'; import { Container } from '@n8n/di'; import { mock } from 'jest-mock-extended'; +import type { INode } from 'n8n-workflow'; import { Telemetry } from '@/telemetry'; import { EnterpriseWorkflowService } from '@/workflows/workflow.service.ee'; @@ -59,6 +61,28 @@ describe('EnterpriseWorkflowService', () => { return credentialEntity; } + const STORED_NODE_ID = '4673f869-f2dc-4a33-b053-ca3193bc5226'; + + const inaccessibleCredential = { + test: { id: FIRST_CREDENTIAL_ID, name: 'First fake credential' }, + }; + + const makeNode = (overrides: Partial): INode => ({ + id: STORED_NODE_ID, + name: 'Node', + type: 'n8n-nodes-base.httpRequest', + typeVersion: 1, + position: [0, 0], + parameters: {}, + ...overrides, + }); + + const workflowWithNodes = (nodes: INode[]) => { + const workflow = new WorkflowEntity(); + workflow.nodes = nodes; + return workflow; + }; + it('Should throw error saving a workflow using credential without access', () => { const newWorkflowVersion = getWorkflow({ addNodeWithOneCred: true }); const previousWorkflowVersion = getWorkflow(); @@ -116,6 +140,59 @@ describe('EnterpriseWorkflowService', () => { service.validateWorkflowCredentialUsage(newWorkflowVersion, previousWorkflowVersion, []); }).toThrow(); }); + + it('Should reject a repeated node id when both claimants use the credential', () => { + const previousWorkflowVersion = workflowWithNodes([ + makeNode({ id: STORED_NODE_ID, name: 'First', credentials: inaccessibleCredential }), + ]); + const newWorkflowVersion = workflowWithNodes([ + makeNode({ id: STORED_NODE_ID, name: 'First', credentials: inaccessibleCredential }), + makeNode({ + id: STORED_NODE_ID, + name: 'Second', + credentials: inaccessibleCredential, + parameters: { url: 'https://example.com/' }, + }), + ]); + + expect(() => { + service.validateWorkflowCredentialUsage(newWorkflowVersion, previousWorkflowVersion, []); + }).toThrow(); + }); + + // Only one claimant is checked here, so the id count has to cover every submitted node. + it('Should reject a repeated node id when only one claimant uses the credential', () => { + const previousWorkflowVersion = workflowWithNodes([ + makeNode({ id: STORED_NODE_ID, name: 'First', credentials: inaccessibleCredential }), + ]); + const newWorkflowVersion = workflowWithNodes([ + makeNode({ id: STORED_NODE_ID, name: 'First' }), + makeNode({ + id: STORED_NODE_ID, + name: 'Second', + credentials: inaccessibleCredential, + parameters: { url: 'https://example.com/' }, + }), + ]); + + expect(() => { + service.validateWorkflowCredentialUsage(newWorkflowVersion, previousWorkflowVersion, []); + }).toThrow(); + }); + + it('Should not keep submitted fields that the stored node does not have', () => { + const storedNode = makeNode({ id: STORED_NODE_ID, credentials: inaccessibleCredential }); + delete (storedNode as Partial).parameters; + + const previousWorkflowVersion = workflowWithNodes([storedNode]); + const newWorkflowVersion = workflowWithNodes([ + { ...storedNode, parameters: { url: 'https://example.com/' } }, + ]); + + service.validateWorkflowCredentialUsage(newWorkflowVersion, previousWorkflowVersion, []); + + expect(newWorkflowVersion.nodes[0].parameters).toBeUndefined(); + }); }); describe('getNodesWithInaccessibleCreds', () => { diff --git a/packages/core/src/execution-engine/__tests__/routing-node.test.ts b/packages/core/src/execution-engine/__tests__/routing-node.test.ts index 0ccf2e0e967..3f573a214d2 100644 --- a/packages/core/src/execution-engine/__tests__/routing-node.test.ts +++ b/packages/core/src/execution-engine/__tests__/routing-node.test.ts @@ -2256,6 +2256,125 @@ describe('RoutingNode', () => { } }); + describe('per-item errors', () => { + const createRoutingNode = ({ + continueOnFail, + requestErrorUrl, + }: { + continueOnFail: boolean; + requestErrorUrl?: string; + }) => { + const items: INodeExecutionData[] = [ + { json: { id: 'first' } }, + { json: requestErrorUrl ? { id: 'second' } : {} }, + { json: { id: 'third' } }, + ]; + const node: INode = { + parameters: {}, + name: 'test', + type: 'test.set', + typeVersion: 1, + id: 'uuid-1234', + position: [0, 0], + }; + const nodeType = mock(); + nodeType.description = { + requestDefaults: { + baseURL: 'http://127.0.0.1:5678', + url: '=/items/{{ toPathSegment($json.id) }}', + }, + properties: [], + } as unknown as INodeTypeDescription; + const runExecutionData = createEmptyRunExecutionData(); + const workflow = new Workflow({ + nodes: [node], + connections: {}, + active: false, + nodeTypes, + }); + const executeFunctions = mock(); + Object.assign(executeFunctions, { + executeData: { data: {}, node, source: null } as IExecuteData, + inputData: { main: [items] } as ITaskDataConnections, + runIndex: 0, + additionalData, + workflow, + node, + mode: 'internal', + connectionInputData: items, + runExecutionData, + }); + executeFunctions.getNodeParameter.mockReturnValue({}); + + const requestUrls: string[] = []; + const contexts = items.map((_, itemIndex) => { + const context = getExecuteSingleFunctions(workflow, runExecutionData, 0, node, itemIndex); + // @ts-expect-error overwriting a method + context.getNodeParameter = () => ({}); + context.continueOnFail.mockReturnValue(continueOnFail); + context.helpers.httpRequest = jest.fn(async (requestOptions: IHttpRequestOptions) => { + requestUrls.push(requestOptions.url); + if (requestOptions.url === requestErrorUrl) { + throw new Error('Request failed'); + } + return { body: { url: requestOptions.url } }; + }); + return context; + }); + let contextIndex = 0; + jest.spyOn(executionContexts, 'ExecuteSingleContext').mockImplementation(function ( + this: executionContexts.ExecuteSingleContext, + ) { + return contexts[contextIndex++] as never; + } as never); + + return { + requestUrls, + run: new RoutingNode(executeFunctions, nodeType).runNode(), + }; + }; + + test('returns a preparation error and processes later items when continue on fail is enabled', async () => { + const { requestUrls, run } = createRoutingNode({ continueOnFail: true }); + + const result = await run; + + expect(requestUrls).toEqual(['/items/first', '/items/third']); + expect(result?.[0]?.[0]?.json).toEqual({ url: '/items/first' }); + expect(result?.[0]?.[1]?.error).toMatchObject({ + name: 'NodeOperationError', + message: 'Invalid identifier: a value is required', + context: { itemIndex: 1, runIndex: 0 }, + }); + expect(result?.[0]?.[2]?.json).toEqual({ url: '/items/third' }); + }); + + test('throws a preparation error and does not process later items when continue on fail is disabled', async () => { + const { requestUrls, run } = createRoutingNode({ continueOnFail: false }); + + await expect(run).rejects.toMatchObject({ + name: 'NodeOperationError', + message: 'Invalid identifier: a value is required', + context: { itemIndex: 1, runIndex: 0 }, + }); + expect(requestUrls).toEqual(['/items/first']); + }); + + test('returns a request error and processes later items when continue on fail is enabled', async () => { + const { requestUrls, run } = createRoutingNode({ + continueOnFail: true, + requestErrorUrl: '/items/second', + }); + + const result = await run; + + expect(requestUrls).toEqual(['/items/first', '/items/second', '/items/third']); + expect(result?.[0]?.[0]?.json).toEqual({ url: '/items/first' }); + expect(result?.[0]?.[1]?.error).toMatchObject({ message: 'Request failed' }); + expect(result?.[0]?.[2]?.json).toEqual({ url: '/items/third' }); + }); + }); + describe('itemIndex', () => { const tests: Array<{ description: string; diff --git a/packages/core/src/execution-engine/node-execution-context/__tests__/node-execution-context.test.ts b/packages/core/src/execution-engine/node-execution-context/__tests__/node-execution-context.test.ts index 876d5d9e3b6..bc12ea53ecf 100644 --- a/packages/core/src/execution-engine/node-execution-context/__tests__/node-execution-context.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/__tests__/node-execution-context.test.ts @@ -12,7 +12,9 @@ import type { } from 'n8n-workflow'; import { CHAT_TRIGGER_NODE_TYPE, createRunExecutionData, NodeConnectionTypes } from 'n8n-workflow'; +import { WAITING_TOKEN_QUERY_PARAM } from '@/constants'; import { InstanceSettings } from '@/instance-settings'; +import { generateUrlSignature, prepareUrlForSigning } from '@/utils/signature-helpers'; import { NodeExecutionContext } from '../node-execution-context'; @@ -403,5 +405,50 @@ describe('NodeExecutionContext', () => { 'http://localhost/waiting-webhook/123/node456?approved=true&signature=11c5efc97a0d6f2ea9045dba6e397596cba29dc24adb44a9ebd3d1272c991e9b', ); }); + + describe('when the node id is not a plain path segment', () => { + const NODE_ID = '../43/dddd2020-0000-4000-8000-000000002099'; + + const signedUrlForNodeId = (nodeId: string) => + new URL( + new TestContext( + workflow, + mock({ id: nodeId }), + mock({ + executionId: '123', + webhookWaitingBaseUrl: 'http://localhost/waiting-webhook', + formWaitingBaseUrl: 'http://localhost/form-waiting', + }), + mode, + createRunExecutionData({ resultData: { runData: {} } }), + ).getSignedResumeUrl({ approved: 'true' }), + ); + + it('should keep the node id inside its own path segment', () => { + expect(signedUrlForNodeId(NODE_ID).pathname).toBe( + '/waiting-webhook/123/..%2F43%2Fdddd2020-0000-4000-8000-000000002099', + ); + }); + + it('should keep the signature scoped to the escaped path', () => { + const signature = signedUrlForNodeId(NODE_ID).searchParams.get('signature')!; + + // Unescaped, the same ids normalise to a different path, so the signature over the + // escaped one must not validate against it. + const normalisedUrl = new URL( + `http://localhost/waiting-webhook/123/${NODE_ID}?approved=true&signature=${signature}`, + ); + expect(normalisedUrl.pathname).toBe( + '/waiting-webhook/43/dddd2020-0000-4000-8000-000000002099', + ); + + normalisedUrl.searchParams.delete(WAITING_TOKEN_QUERY_PARAM); + const expectedSignature = generateUrlSignature( + prepareUrlForSigning(normalisedUrl), + instanceSettings.hmacSignatureSecret, + ); + expect(signature).not.toBe(expectedSignature); + }); + }); }); }); diff --git a/packages/core/src/execution-engine/node-execution-context/node-execution-context.ts b/packages/core/src/execution-engine/node-execution-context/node-execution-context.ts index 766bdf9b1b8..b5205267d42 100644 --- a/packages/core/src/execution-engine/node-execution-context/node-execution-context.ts +++ b/packages/core/src/execution-engine/node-execution-context/node-execution-context.ts @@ -46,7 +46,11 @@ import { ensureType } from './utils/ensure-type'; import { extractValue } from './utils/extract-value'; import { getAdditionalKeys } from './utils/get-additional-keys'; import { validateValueAgainstSchema } from './utils/validate-value-against-schema'; -import { generateUrlSignature, prepareUrlForSigning } from '../../utils/signature-helpers'; +import { + buildResumeUrlSuffix, + generateUrlSignature, + prepareUrlForSigning, +} from '../../utils/signature-helpers'; export abstract class NodeExecutionContext implements Omit { protected readonly instanceSettings = Container.get(InstanceSettings); @@ -238,7 +242,9 @@ export abstract class NodeExecutionContext implements Omit)[key]; + if (nodeType.description.requestOperations) { + itemContext[itemIndex].requestData.requestOperations = { + ...nodeType.description.requestOperations, + }; + } + + if (nodeType.description.requestDefaults) { + for (const key of Object.keys(nodeType.description.requestDefaults)) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let value = (nodeType.description.requestDefaults as Record)[key]; + // If the value is an expression resolve it + value = this.getParameterValue( + value, + itemIndex, + runIndex, + executeData, + { $credentials: credentials, $version: node.typeVersion }, + false, + ) as string; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (itemContext[itemIndex].requestData.options as Record)[key] = value; + } + } + + for (const property of nodeType.description.properties) { + let value = get(node.parameters, property.name, []) as string | NodeParameterValue; // If the value is an expression resolve it value = this.getParameterValue( value, @@ -145,109 +162,113 @@ export class RoutingNode { executeData, { $credentials: credentials, $version: node.typeVersion }, false, - ) as string; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (itemContext[itemIndex].requestData.options as Record)[key] = value; + ) as string | NodeParameterValue; + + const tempOptions = this.getRequestOptionsFromParameters( + itemContext[itemIndex].thisArgs, + property, + itemIndex, + runIndex, + '', + { $credentials: credentials, $value: value, $version: node.typeVersion }, + ); + + this.mergeOptions(itemContext[itemIndex].requestData, tempOptions); } - } - for (const property of nodeType.description.properties) { - let value = get(node.parameters, property.name, []) as string | NodeParameterValue; - // If the value is an expression resolve it - value = this.getParameterValue( - value, - itemIndex, - runIndex, - executeData, - { $credentials: credentials, $version: node.typeVersion }, - false, - ) as string | NodeParameterValue; + if (proxy) { + const proxyParsed = url.parse(proxy); + const proxyProperties = ['host', 'port']; - const tempOptions = this.getRequestOptionsFromParameters( - itemContext[itemIndex].thisArgs, - property, - itemIndex, - runIndex, - '', - { $credentials: credentials, $value: value, $version: node.typeVersion }, - ); + for (const property of proxyProperties) { + if ( + !(property in proxyParsed) || + proxyParsed[property as keyof typeof proxyParsed] === null + ) { + throw new NodeOperationError(node, 'The proxy is not value', { + runIndex, + itemIndex, + description: `The proxy URL does not contain a valid value for "${property}"`, + }); + } + } - this.mergeOptions(itemContext[itemIndex].requestData, tempOptions); - } + itemContext[itemIndex].requestData.options.proxy = { + host: proxyParsed.hostname as string, + port: parseInt(proxyParsed.port!), + protocol: proxyParsed.protocol?.replace(/:$/, '') || undefined, + }; - if (proxy) { - const proxyParsed = url.parse(proxy); - const proxyProperties = ['host', 'port']; - - for (const property of proxyProperties) { - if ( - !(property in proxyParsed) || - proxyParsed[property as keyof typeof proxyParsed] === null - ) { - throw new NodeOperationError(node, 'The proxy is not value', { - runIndex, - itemIndex, - description: `The proxy URL does not contain a valid value for "${property}"`, - }); + if (proxyParsed.auth) { + const [username, password] = proxyParsed.auth.split(':'); + itemContext[itemIndex].requestData.options.proxy!.auth = { + username, + password, + }; } } - itemContext[itemIndex].requestData.options.proxy = { - host: proxyParsed.hostname as string, - port: parseInt(proxyParsed.port!), - protocol: proxyParsed.protocol?.replace(/:$/, '') || undefined, - }; - - if (proxyParsed.auth) { - const [username, password] = proxyParsed.auth.split(':'); - itemContext[itemIndex].requestData.options.proxy!.auth = { - username, - password, - }; + if (allowUnauthorizedCerts) { + itemContext[itemIndex].requestData.options.skipSslCertificateValidation = + allowUnauthorizedCerts; } - } - if (allowUnauthorizedCerts) { - itemContext[itemIndex].requestData.options.skipSslCertificateValidation = - allowUnauthorizedCerts; - } + if (timeout) { + itemContext[itemIndex].requestData.options.timeout = timeout; + } else { + // set default timeout to 5 minutes + itemContext[itemIndex].requestData.options.timeout = 300_000; + } - if (timeout) { - itemContext[itemIndex].requestData.options.timeout = timeout; - } else { - // set default timeout to 5 minutes - itemContext[itemIndex].requestData.options.timeout = 300_000; - } + if (credentials?.allowedHttpRequestDomains === 'none') { + throw new NodeOperationError( + node, + 'This credential is configured to prevent use within an HTTP Request node', + ); + } - if (credentials?.allowedHttpRequestDomains === 'none') { - throw new NodeOperationError( - node, - 'This credential is configured to prevent use within an HTTP Request node', + const allowedDomains = getCredentialAllowedDomains(credentials); + if (credentials?.allowedHttpRequestDomains === 'domains' && !allowedDomains) { + throw new NodeOperationError( + node, + 'No allowed domains specified. Configure allowed domains or change restriction setting.', + ); + } + if (allowedDomains) { + itemContext[itemIndex].requestData.options.allowedDomains = allowedDomains; + } + + requestPromises.push( + this.makeRequest( + itemContext[itemIndex].requestData, + itemContext[itemIndex].thisArgs, + itemIndex, + runIndex, + credentialDescription?.name, + itemContext[itemIndex].requestData.requestOperations, + credentialsDecrypted, + ), ); - } + } catch (error) { + const nodeError = + error instanceof NodeApiError || error instanceof NodeOperationError + ? error + : new NodeOperationError( + node, + error instanceof Error ? error : new Error(String(error)), + { itemIndex, runIndex }, + ); - const allowedDomains = getCredentialAllowedDomains(credentials); - if (credentials?.allowedHttpRequestDomains === 'domains' && !allowedDomains) { - throw new NodeOperationError( - node, - 'No allowed domains specified. Configure allowed domains or change restriction setting.', - ); - } - if (allowedDomains) { - itemContext[itemIndex].requestData.options.allowedDomains = allowedDomains; - } + set(nodeError, 'context.itemIndex', itemIndex); + set(nodeError, 'context.runIndex', runIndex); - requestPromises.push( - this.makeRequest( - itemContext[itemIndex].requestData, - itemContext[itemIndex].thisArgs, - itemIndex, - runIndex, - credentialDescription?.name, - itemContext[itemIndex].requestData.requestOperations, - credentialsDecrypted, - ), - ); + if (itemContext[itemIndex].thisArgs.continueOnFail()) { + requestPromises.push(Promise.resolve([{ json: {}, error: nodeError }])); + continue; + } + + throw nodeError; + } } const promisesResponses = await Promise.allSettled(requestPromises); diff --git a/packages/core/src/utils/__tests__/signature-helpers.test.ts b/packages/core/src/utils/__tests__/signature-helpers.test.ts index cc98c5d45f7..ed9a80654b5 100644 --- a/packages/core/src/utils/__tests__/signature-helpers.test.ts +++ b/packages/core/src/utils/__tests__/signature-helpers.test.ts @@ -1,4 +1,4 @@ -import { generateUrlSignature } from '../signature-helpers'; +import { buildResumeUrlSuffix, generateUrlSignature } from '../signature-helpers'; describe('signature-helpers', () => { const secret = 'test-secret'; @@ -24,3 +24,32 @@ describe('signature-helpers', () => { }); }); }); + +describe('buildResumeUrlSuffix', () => { + it('should leave a uuid node id unchanged', () => { + expect(buildResumeUrlSuffix('123', 'dddd2020-0000-4000-8000-000000002099')).toBe( + '/123/dddd2020-0000-4000-8000-000000002099', + ); + }); + + it('should confine a node id to a single path segment', () => { + expect(buildResumeUrlSuffix('123', '../43/other')).toBe('/123/..%2F43%2Fother'); + }); + + it('should confine a node id that opens a query string', () => { + expect(buildResumeUrlSuffix('123', 'x?approved=true')).toBe('/123/x%3Fapproved%3Dtrue'); + }); + + describe('when the node id is a bare dot segment', () => { + // Encoding leaves `.` and `..` untouched, so the documented outcome is a link that + // collapses on parsing and no longer resolves. Pinned so the limitation stays visible. + it.each([ + { nodeId: '.', suffix: '/123/.', pathname: '/waiting-webhook/123/' }, + { nodeId: '..', suffix: '/123/..', pathname: '/waiting-webhook/' }, + ])('should collapse the path for a node id of "$nodeId"', ({ nodeId, suffix, pathname }) => { + expect(buildResumeUrlSuffix('123', nodeId)).toBe(suffix); + + expect(new URL(`http://localhost/waiting-webhook${suffix}`).pathname).toBe(pathname); + }); + }); +}); diff --git a/packages/core/src/utils/signature-helpers.ts b/packages/core/src/utils/signature-helpers.ts index fa8d8081a75..ef378158156 100644 --- a/packages/core/src/utils/signature-helpers.ts +++ b/packages/core/src/utils/signature-helpers.ts @@ -8,6 +8,18 @@ export function generateUrlSignature(url: string, secret: string) { return token; } +/** + * Builds the `//` suffix of a waiting-webhook resume URL. + * + * The node id is client-supplied and stored verbatim, so it is percent-encoded to keep it + * within one path segment of the signed URL. Ids of exactly `.` or `..` are the exception — + * no encoding of a dot segment survives URL normalisation, so those collapse the path and + * yield a link that does not resolve. The execution id is server-generated and already URL-safe. + */ +export function buildResumeUrlSuffix(executionId: string, nodeId: string): string { + return `/${executionId}/${encodeURIComponent(nodeId)}`; +} + /** * Prepare url for signing */ diff --git a/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocatorDropdown.test.ts b/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocatorDropdown.test.ts index 21ef1e710b1..5dcad8ff2b4 100644 --- a/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocatorDropdown.test.ts +++ b/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocatorDropdown.test.ts @@ -100,6 +100,62 @@ describe('ResourceLocatorDropdown', () => { }); }); + describe('opening a result link', () => { + async function clickCachedResultLink(cachedResultUrl: string) { + vi.useFakeTimers(); + + const cachedModelValue: INodeParameterResourceLocator = { + __rl: true, + value: 'workflow-cached', + mode: 'list', + cachedResultName: 'Cached Workflow', + cachedResultUrl, + }; + + renderComponent({ + props: { + show: true, + resources: mockResources, // doesn't contain workflow-cached + modelValue: cachedModelValue, + }, + }); + + const cachedItem = screen.getByText('Cached Workflow').closest('[data-test-id="rlc-item"]'); + + await fireEvent.mouseEnter(cachedItem as Element); + await vi.advanceTimersByTimeAsync(250); + + const linkIcon = cachedItem?.querySelector('svg[data-icon="external-link"]'); + expect(linkIcon).toBeInTheDocument(); + + await fireEvent.click(linkIcon as Element); + } + + it('should open an http(s) result url with noopener,noreferrer', async () => { + const windowOpenSpy = vi.spyOn(window, 'open').mockImplementation(() => null); + + await clickCachedResultLink('https://example.com/resource'); + + expect(windowOpenSpy).toHaveBeenCalledWith( + 'https://example.com/resource', + '_blank', + 'noopener,noreferrer', + ); + + windowOpenSpy.mockRestore(); + }); + + it('should not open a result url with a disallowed scheme', async () => { + const windowOpenSpy = vi.spyOn(window, 'open').mockImplementation(() => null); + + await clickCachedResultLink('javascript:alert(document.domain)'); + + expect(windowOpenSpy).not.toHaveBeenCalled(); + + windowOpenSpy.mockRestore(); + }); + }); + describe('model value handling', () => { it('should compare values correctly for selection highlighting', () => { const modelValue: INodeParameterResourceLocator = { diff --git a/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocatorDropdown.vue b/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocatorDropdown.vue index 1f472a9bfd9..d6c0a9141f7 100644 --- a/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocatorDropdown.vue +++ b/packages/frontend/editor-ui/src/features/ndv/parameters/components/ResourceLocator/ResourceLocatorDropdown.vue @@ -7,6 +7,7 @@ import type { EventBus } from '@n8n/utils/event-bus'; import { createEventBus } from '@n8n/utils/event-bus'; import type { INodeParameterResourceLocator } from 'n8n-workflow'; import { computed, onBeforeUnmount, onMounted, ref, useCssModule, watch } from 'vue'; +import { openSafeUrl } from '@/app/utils/htmlUtils'; const SEARCH_BAR_HEIGHT_PX = 40; const SCROLL_MARGIN_PX = 10; @@ -135,7 +136,7 @@ function openUrl(event: MouseEvent, url: string) { event.preventDefault(); event.stopPropagation(); - window.open(url, '_blank'); + openSafeUrl(url); } function onKeyDown(e: KeyboardEvent) { diff --git a/packages/frontend/editor-ui/src/features/settings/sso/components/SamlSettingsForm.vue b/packages/frontend/editor-ui/src/features/settings/sso/components/SamlSettingsForm.vue index a1ff3961e96..fb43d741814 100644 --- a/packages/frontend/editor-ui/src/features/settings/sso/components/SamlSettingsForm.vue +++ b/packages/frontend/editor-ui/src/features/settings/sso/components/SamlSettingsForm.vue @@ -16,6 +16,7 @@ import { useRootStore } from '@n8n/stores/useRootStore'; import { useTelemetry } from '@/app/composables/useTelemetry'; import ConfirmProvisioningDialog from '../provisioning/components/ConfirmProvisioningDialog.vue'; import { MODAL_CONFIRM } from '@/app/constants/modals'; +import { openSafeUrl } from '@/app/utils/htmlUtils'; const i18n = useI18n(); const ssoStore = useSSOStore(); @@ -245,7 +246,7 @@ const onTest = async () => { const url = await ssoStore.testSamlConfig(); if (typeof window !== 'undefined') { - window.open(url, '_blank'); + openSafeUrl(url); } } catch (error) { toast.showError(error, 'error'); diff --git a/packages/nodes-base/nodes/Adalo/Adalo.node.test.ts b/packages/nodes-base/nodes/Adalo/Adalo.node.test.ts new file mode 100644 index 00000000000..18cbbebb019 --- /dev/null +++ b/packages/nodes-base/nodes/Adalo/Adalo.node.test.ts @@ -0,0 +1,30 @@ +import { Adalo } from './Adalo.node'; + +function collectUrls(value: unknown): string[] { + if (Array.isArray(value)) return value.flatMap(collectUrls); + if (value === null || typeof value !== 'object') return []; + + return Object.entries(value).flatMap(([key, child]) => { + if (key === 'url' && typeof child === 'string') return [child]; + return collectUrls(child); + }); +} + +describe('Adalo node', () => { + it('encodes every dynamic request URL path segment', () => { + const dynamicUrls = collectUrls(new Adalo().description.properties).filter((url) => + url.includes('$parameter'), + ); + const dynamicExpressions = dynamicUrls.flatMap((url) => + [...url.matchAll(/{{\s*(.*?)\s*}}/g)] + .map((match) => match[1]) + .filter((expression) => expression.includes('$parameter')), + ); + + expect(dynamicUrls).toHaveLength(5); + expect(dynamicExpressions).toHaveLength(8); + for (const expression of dynamicExpressions) { + expect(expression).toMatch(/^toPathSegment\(.+\)$/); + } + }); +}); diff --git a/packages/nodes-base/nodes/Adalo/Adalo.node.ts b/packages/nodes-base/nodes/Adalo/Adalo.node.ts index 66540bbdcda..574a83c7fd9 100644 --- a/packages/nodes-base/nodes/Adalo/Adalo.node.ts +++ b/packages/nodes-base/nodes/Adalo/Adalo.node.ts @@ -75,7 +75,7 @@ export class Adalo implements INodeType { }, request: { method: 'POST', - url: '=/collections/{{$parameter["collectionId"]}}', + url: '=/collections/{{toPathSegment($parameter["collectionId"])}}', }, }, action: 'Create a row', @@ -87,7 +87,7 @@ export class Adalo implements INodeType { routing: { request: { method: 'DELETE', - url: '=/collections/{{$parameter["collectionId"]}}/{{$parameter["rowId"]}}', + url: '=/collections/{{toPathSegment($parameter["collectionId"])}}/{{toPathSegment($parameter["rowId"])}}', }, output: { postReceive: [ @@ -109,7 +109,7 @@ export class Adalo implements INodeType { routing: { request: { method: 'GET', - url: '=/collections/{{$parameter["collectionId"]}}/{{$parameter["rowId"]}}', + url: '=/collections/{{toPathSegment($parameter["collectionId"])}}/{{toPathSegment($parameter["rowId"])}}', }, }, action: 'Retrieve a row', @@ -121,7 +121,7 @@ export class Adalo implements INodeType { routing: { request: { method: 'GET', - url: '=/collections/{{$parameter["collectionId"]}}', + url: '=/collections/{{toPathSegment($parameter["collectionId"])}}', qs: { limit: '={{$parameter["limit"]}}', }, @@ -152,7 +152,7 @@ export class Adalo implements INodeType { }, request: { method: 'PUT', - url: '=/collections/{{$parameter["collectionId"]}}/{{$parameter["rowId"]}}', + url: '=/collections/{{toPathSegment($parameter["collectionId"])}}/{{toPathSegment($parameter["rowId"])}}', }, }, action: 'Update a row', diff --git a/packages/nodes-base/nodes/Elastic/ElasticSecurity/ElasticSecurity.node.ts b/packages/nodes-base/nodes/Elastic/ElasticSecurity/ElasticSecurity.node.ts index c16347c0987..b513a70b108 100644 --- a/packages/nodes-base/nodes/Elastic/ElasticSecurity/ElasticSecurity.node.ts +++ b/packages/nodes-base/nodes/Elastic/ElasticSecurity/ElasticSecurity.node.ts @@ -7,9 +7,7 @@ import type { INodeType, INodeTypeDescription, } from 'n8n-workflow'; -import { NodeConnectionTypes, NodeOperationError } from 'n8n-workflow'; - -import { toPathSegment } from '@utils/url'; +import { toPathSegment, NodeConnectionTypes, NodeOperationError } from 'n8n-workflow'; import { caseCommentFields, diff --git a/packages/nodes-base/nodes/Elastic/ElasticSecurity/GenericFunctions.ts b/packages/nodes-base/nodes/Elastic/ElasticSecurity/GenericFunctions.ts index a841135b757..db889cf4c40 100644 --- a/packages/nodes-base/nodes/Elastic/ElasticSecurity/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Elastic/ElasticSecurity/GenericFunctions.ts @@ -6,9 +6,7 @@ import type { IRequestOptions, IHttpRequestMethods, } from 'n8n-workflow'; -import { NodeApiError, NodeOperationError } from 'n8n-workflow'; - -import { toPathSegment } from '@utils/url'; +import { toPathSegment, NodeApiError, NodeOperationError } from 'n8n-workflow'; import type { Connector, ElasticSecurityApiCredentials } from './types'; diff --git a/packages/nodes-base/nodes/Elastic/Elasticsearch/Elasticsearch.node.ts b/packages/nodes-base/nodes/Elastic/Elasticsearch/Elasticsearch.node.ts index 0888587066f..d6eea63f125 100644 --- a/packages/nodes-base/nodes/Elastic/Elasticsearch/Elasticsearch.node.ts +++ b/packages/nodes-base/nodes/Elastic/Elasticsearch/Elasticsearch.node.ts @@ -7,9 +7,7 @@ import type { INodeTypeDescription, JsonObject, } from 'n8n-workflow'; -import { NodeConnectionTypes, jsonParse, NodeApiError } from 'n8n-workflow'; - -import { toPathSegment } from '@utils/url'; +import { NodeConnectionTypes, jsonParse, NodeApiError, toPathSegment } from 'n8n-workflow'; import { documentFields, documentOperations, indexFields, indexOperations } from './descriptions'; import { diff --git a/packages/nodes-base/nodes/Elastic/Elasticsearch/GenericFunctions.ts b/packages/nodes-base/nodes/Elastic/Elasticsearch/GenericFunctions.ts index 97625574d5d..43325ab7ad5 100644 --- a/packages/nodes-base/nodes/Elastic/Elasticsearch/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Elastic/Elasticsearch/GenericFunctions.ts @@ -5,9 +5,7 @@ import type { IHttpRequestOptions, IHttpRequestMethods, } from 'n8n-workflow'; -import { NodeApiError } from 'n8n-workflow'; - -import { toPathSegment } from '@utils/url'; +import { toPathSegment, NodeApiError } from 'n8n-workflow'; import type { ElasticsearchApiCredentials } from './types'; diff --git a/packages/nodes-base/nodes/Git/GenericFunctions.ts b/packages/nodes-base/nodes/Git/GenericFunctions.ts index 694f915855f..e966c3ac0c4 100644 --- a/packages/nodes-base/nodes/Git/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Git/GenericFunctions.ts @@ -27,6 +27,8 @@ const GPG_SSH_DEFAULT_KEY_COMMAND_CONFIG_KEY_PATTERN = /^gpg\.ssh\.defaultkeycom const URL_REWRITE_CONFIG_KEY_PATTERN = /^url\.(.*)\.(?:insteadof|pushinsteadof)$/i; +const CREDENTIAL_URL_HELPER_CONFIG_KEY_PATTERN = /^credential\.(.*)\.helper$/i; + const KEY_BLACKLIST = [ FILTER_COMMAND_CONFIG_KEY_PATTERN, MERGE_DRIVER_CONFIG_KEY_PATTERN, @@ -40,6 +42,7 @@ const KEY_BLACKLIST = [ SEQUENCE_EDITOR_CONFIG_KEY_PATTERN, GPG_SSH_DEFAULT_KEY_COMMAND_CONFIG_KEY_PATTERN, URL_REWRITE_CONFIG_KEY_PATTERN, + CREDENTIAL_URL_HELPER_CONFIG_KEY_PATTERN, ]; export function findBlacklistedKeys( diff --git a/packages/nodes-base/nodes/Git/Git.node.ts b/packages/nodes-base/nodes/Git/Git.node.ts index d72e3dea4ed..58db52137f1 100644 --- a/packages/nodes-base/nodes/Git/Git.node.ts +++ b/packages/nodes-base/nodes/Git/Git.node.ts @@ -830,6 +830,7 @@ export class Git implements INodeType { logOptions.file = options.file as string; } + await validateGitConfig(); const log = await git.log(logOptions); returnItems.push( 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 d15027ae4d3..870299a5f49 100644 --- a/packages/nodes-base/nodes/Git/__test__/Git.node.test.ts +++ b/packages/nodes-base/nodes/Git/__test__/Git.node.test.ts @@ -11,6 +11,7 @@ import { Git } from '../Git.node'; const mockGit = { log: jest.fn(), raw: jest.fn(), + listConfig: jest.fn(), env: jest.fn().mockReturnThis(), }; @@ -71,6 +72,11 @@ describe('Git Node', () => { mockGit.raw.mockImplementation(async (args: string[]) => args[0] === 'rev-parse' ? revParseOutput : '', ); + mockGit.listConfig.mockResolvedValue({ + files: ['.git/config', 'command line:'], + values: { '.git/config': {}, 'command line:': {} }, + all: {}, + }); gitNode = new Git(); }); @@ -84,7 +90,7 @@ describe('Git Node', () => { expect(mockSimpleGit).toHaveBeenCalledWith( expect.objectContaining({ - config: ['safe.bareRepository=explicit'], + config: expect.arrayContaining(['safe.bareRepository=explicit']), }), ); }); @@ -97,7 +103,7 @@ describe('Git Node', () => { expect(mockSimpleGit).toHaveBeenCalledWith( expect.objectContaining({ - config: ['safe.bareRepository=explicit'], + config: expect.arrayContaining(['safe.bareRepository=explicit']), }), ); }); @@ -110,7 +116,7 @@ describe('Git Node', () => { expect(mockSimpleGit).toHaveBeenCalledWith( expect.objectContaining({ - config: ['safe.bareRepository=explicit'], + config: expect.arrayContaining(['safe.bareRepository=explicit']), }), ); }); @@ -121,11 +127,8 @@ describe('Git Node', () => { await gitNode.execute.call(executeFunctions); - expect(mockSimpleGit).toHaveBeenCalledWith( - expect.objectContaining({ - config: [], - }), - ); + const options = mockSimpleGit.mock.calls[0][0]; + expect(options.config).not.toContain('safe.bareRepository=explicit'); }); }); @@ -137,7 +140,7 @@ describe('Git Node', () => { expect(mockSimpleGit).toHaveBeenCalledWith( expect.objectContaining({ - config: ['core.hooksPath=/dev/null'], + config: expect.arrayContaining(['core.hooksPath=/dev/null']), }), ); }); @@ -147,11 +150,8 @@ describe('Git Node', () => { await gitNode.execute.call(executeFunctions); - expect(mockSimpleGit).toHaveBeenCalledWith( - expect.objectContaining({ - config: [], - }), - ); + const options = mockSimpleGit.mock.calls[0][0]; + expect(options.config).not.toContain('core.hooksPath=/dev/null'); }); }); 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 939f4d08566..597a1c69c53 100644 --- a/packages/nodes-base/nodes/Git/test/Git.node.test.ts +++ b/packages/nodes-base/nodes/Git/test/Git.node.test.ts @@ -1976,6 +1976,7 @@ describe('Git Node', () => { 'remote.origin.receivepack', 'gpg.openpgp.program', 'gpg.ssh.defaultKeyCommand', + 'credential.https://example.invalid.helper', ])("rejects repository Git config key '%s'", async (configKey) => { mockExecuteFunctions.getNodeParameter .mockReturnValueOnce('add') @@ -2001,6 +2002,7 @@ describe('Git Node', () => { { 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: 'log', params: ['log', '/repo', {}], guard: 'log' }, { operation: 'pull', params: ['pull', '/repo', {}], guard: 'pull' }, { operation: 'push', params: ['push', '/repo', {}], guard: 'push' }, { operation: 'pushTags', params: ['pushTags', '/repo', {}], guard: 'pushTags' }, @@ -2042,7 +2044,9 @@ describe('Git Node', () => { for (const flag of expectedFlags) { expect(unsafe?.[flag]).toBeUndefined(); } - expect(mockGit.listConfig).not.toHaveBeenCalled(); + // The guard reads the merged config, then returns before the denylist check, so it + // never looks up the repository-local scope. + expect(mockGit.listConfig).not.toHaveBeenCalledWith('local'); }); }); }); diff --git a/packages/nodes-base/nodes/HighLevel/HighLevel.structure.test.ts b/packages/nodes-base/nodes/HighLevel/HighLevel.structure.test.ts new file mode 100644 index 00000000000..7bad89a729a --- /dev/null +++ b/packages/nodes-base/nodes/HighLevel/HighLevel.structure.test.ts @@ -0,0 +1,53 @@ +import type { INodeTypeBaseDescription } from 'n8n-workflow'; + +import { HighLevelV1 } from './v1/HighLevelV1.node'; +import { HighLevelV2 } from './v2/HighLevelV2.node'; + +const baseDescription: INodeTypeBaseDescription = { + displayName: 'HighLevel', + name: 'highLevel', + icon: 'file:highLevel.svg', + group: ['transform'], + description: 'Consume HighLevel API', + defaultVersion: 2, +}; + +function collectUrls(value: unknown): string[] { + if (Array.isArray(value)) return value.flatMap(collectUrls); + if (value === null || typeof value !== 'object') return []; + + return Object.entries(value).flatMap(([key, child]) => { + if (key === 'url' && typeof child === 'string') return [child]; + return collectUrls(child); + }); +} + +describe('HighLevel node', () => { + it.each([ + { + version: 1, + description: new HighLevelV1(baseDescription).description, + }, + { + version: 2, + description: new HighLevelV2(baseDescription).description, + }, + ])('encodes every dynamic request URL path segment in v$version', ({ description }) => { + const dynamicUrls = collectUrls(description.properties).filter((url) => + url.includes('$parameter'), + ); + + expect(dynamicUrls.length).toBeGreaterThan(0); + for (const url of dynamicUrls) { + const dynamicExpressions = [...url.matchAll(/{{\s*(.*?)\s*}}/g)] + .map((match) => match[1]) + .filter((expression) => expression.includes('$parameter')); + + expect(url.replace(/{{\s*(.*?)\s*}}/g, '')).not.toContain('$parameter'); + expect(dynamicExpressions.length).toBeGreaterThan(0); + for (const expression of dynamicExpressions) { + expect(expression).toMatch(/^toPathSegment\(.+\)$/); + } + } + }); +}); diff --git a/packages/nodes-base/nodes/HighLevel/v1/GenericFunctions.ts b/packages/nodes-base/nodes/HighLevel/v1/GenericFunctions.ts index c5e08c9142d..9fb62def01e 100644 --- a/packages/nodes-base/nodes/HighLevel/v1/GenericFunctions.ts +++ b/packages/nodes-base/nodes/HighLevel/v1/GenericFunctions.ts @@ -17,7 +17,7 @@ import type { IRequestOptions, IWebhookFunctions, } from 'n8n-workflow'; -import { NodeApiError } from 'n8n-workflow'; +import { NodeApiError, toPathSegment } from 'n8n-workflow'; const VALID_EMAIL_REGEX = /^(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/; @@ -162,7 +162,7 @@ export async function opportunityUpdatePreSendAction( if (!body.status || !body.title) { const pipelineId = this.getNodeParameter('pipelineId'); const opportunityId = this.getNodeParameter('opportunityId'); - const resource = `/pipelines/${pipelineId}/opportunities/${opportunityId}`; + const resource = `/pipelines/${toPathSegment(pipelineId)}/opportunities/${toPathSegment(opportunityId)}`; const responseData = await highLevelApiRequest.call(this, 'GET', resource); body.status = body.status || responseData.status; body.title = body.title || responseData.name; @@ -179,7 +179,7 @@ export async function taskUpdatePreSendAction( if (!body.title || !body.dueDate) { const contactId = this.getNodeParameter('contactId'); const taskId = this.getNodeParameter('taskId'); - const resource = `/contacts/${contactId}/tasks/${taskId}`; + const resource = `/contacts/${toPathSegment(contactId)}/tasks/${toPathSegment(taskId)}`; const responseData = await highLevelApiRequest.call(this, 'GET', resource); body.title = body.title || responseData.title; // the api response dueDate has to be formatted or it will error on update diff --git a/packages/nodes-base/nodes/HighLevel/v1/description/ContactDescription.ts b/packages/nodes-base/nodes/HighLevel/v1/description/ContactDescription.ts index 0b29b34f3a0..0d7d5acb7a3 100644 --- a/packages/nodes-base/nodes/HighLevel/v1/description/ContactDescription.ts +++ b/packages/nodes-base/nodes/HighLevel/v1/description/ContactDescription.ts @@ -44,7 +44,7 @@ export const contactOperations: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/contacts/{{$parameter.contactId}}', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}', }, output: { postReceive: [ @@ -65,7 +65,7 @@ export const contactOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/contacts/{{$parameter.contactId}}', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}', }, output: { postReceive: [ @@ -100,7 +100,7 @@ export const contactOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/contacts/lookup?email={{$parameter.email}}&phone={{$parameter.phone}}', + url: '=/contacts/lookup?email={{toPathSegment($parameter.email)}}&phone={{toPathSegment($parameter.phone)}}', }, output: { postReceive: [ @@ -121,7 +121,7 @@ export const contactOperations: INodeProperties[] = [ routing: { request: { method: 'PUT', - url: '=/contacts/{{$parameter.contactId}}', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}', }, send: { preSend: [validEmailAndPhonePreSendAction, splitTagsPreSendAction], diff --git a/packages/nodes-base/nodes/HighLevel/v1/description/OpportunityDescription.ts b/packages/nodes-base/nodes/HighLevel/v1/description/OpportunityDescription.ts index 12c86911d32..37bdf45e5ad 100644 --- a/packages/nodes-base/nodes/HighLevel/v1/description/OpportunityDescription.ts +++ b/packages/nodes-base/nodes/HighLevel/v1/description/OpportunityDescription.ts @@ -28,7 +28,7 @@ export const opportunityOperations: INodeProperties[] = [ }, request: { method: 'POST', - url: '=/pipelines/{{$parameter.pipelineId}}/opportunities', + url: '=/pipelines/{{toPathSegment($parameter.pipelineId)}}/opportunities', }, }, action: 'Create an opportunity', @@ -39,7 +39,7 @@ export const opportunityOperations: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/pipelines/{{$parameter.pipelineId}}/opportunities/{{$parameter.opportunityId}}', + url: '=/pipelines/{{toPathSegment($parameter.pipelineId)}}/opportunities/{{toPathSegment($parameter.opportunityId)}}', }, output: { postReceive: [ @@ -60,7 +60,7 @@ export const opportunityOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/pipelines/{{$parameter.pipelineId}}/opportunities/{{$parameter.opportunityId}}', + url: '=/pipelines/{{toPathSegment($parameter.pipelineId)}}/opportunities/{{toPathSegment($parameter.opportunityId)}}', }, }, action: 'Get an opportunity', @@ -71,7 +71,7 @@ export const opportunityOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/pipelines/{{$parameter.pipelineId}}/opportunities', + url: '=/pipelines/{{toPathSegment($parameter.pipelineId)}}/opportunities', }, send: { paginate: true, @@ -85,7 +85,7 @@ export const opportunityOperations: INodeProperties[] = [ routing: { request: { method: 'PUT', - url: '=/pipelines/{{$parameter.pipelineId}}/opportunities/{{$parameter.opportunityId}}', + url: '=/pipelines/{{toPathSegment($parameter.pipelineId)}}/opportunities/{{toPathSegment($parameter.opportunityId)}}', }, send: { preSend: [opportunityUpdatePreSendAction, splitTagsPreSendAction], diff --git a/packages/nodes-base/nodes/HighLevel/v1/description/TaskDescription.ts b/packages/nodes-base/nodes/HighLevel/v1/description/TaskDescription.ts index 601423f158a..176b7863427 100644 --- a/packages/nodes-base/nodes/HighLevel/v1/description/TaskDescription.ts +++ b/packages/nodes-base/nodes/HighLevel/v1/description/TaskDescription.ts @@ -24,7 +24,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'POST', - url: '=/contacts/{{$parameter.contactId}}/tasks', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks', }, output: { postReceive: [taskPostReceiceAction], @@ -38,7 +38,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/contacts/{{$parameter.contactId}}/tasks/{{$parameter.taskId}}', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks/{{toPathSegment($parameter.taskId)}}', }, output: { postReceive: [ @@ -59,7 +59,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/contacts/{{$parameter.contactId}}/tasks/{{$parameter.taskId}}', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks/{{toPathSegment($parameter.taskId)}}', }, output: { postReceive: [taskPostReceiceAction], @@ -73,7 +73,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/contacts/{{$parameter.contactId}}/tasks', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks', }, output: { postReceive: [ @@ -95,7 +95,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'PUT', - url: '=/contacts/{{$parameter.contactId}}/tasks/{{$parameter.taskId}}', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks/{{toPathSegment($parameter.taskId)}}', }, send: { preSend: [taskUpdatePreSendAction], diff --git a/packages/nodes-base/nodes/HighLevel/v1/test/PathEncoding.test.ts b/packages/nodes-base/nodes/HighLevel/v1/test/PathEncoding.test.ts new file mode 100644 index 00000000000..9dc29847c8a --- /dev/null +++ b/packages/nodes-base/nodes/HighLevel/v1/test/PathEncoding.test.ts @@ -0,0 +1,55 @@ +import type { IExecuteSingleFunctions, IHttpRequestOptions } from 'n8n-workflow'; + +import { opportunityUpdatePreSendAction, taskUpdatePreSendAction } from '../GenericFunctions'; + +describe('HighLevel v1 request paths', () => { + let context: Partial; + + beforeEach(() => { + context = { + getNodeParameter: jest.fn(), + helpers: { + requestWithAuthentication: jest.fn().mockResolvedValue({}), + } as unknown as IExecuteSingleFunctions['helpers'], + }; + }); + + it('encodes opportunity IDs as URL path segments', async () => { + (context.getNodeParameter as jest.Mock) + .mockReturnValueOnce('pipeline/id?x#y') + .mockReturnValueOnce('opportunity/id'); + + await opportunityUpdatePreSendAction.call(context as IExecuteSingleFunctions, { + url: 'https://example.com', + body: {}, + }); + + expect(context.helpers?.requestWithAuthentication).toHaveBeenCalledWith( + 'highLevelApi', + expect.objectContaining({ + uri: 'https://rest.gohighlevel.com/v1/pipelines/pipeline%2Fid%3Fx%23y/opportunities/opportunity%2Fid', + }), + ); + }); + + it('encodes task IDs as URL path segments', async () => { + (context.getNodeParameter as jest.Mock) + .mockReturnValueOnce('contact/id') + .mockReturnValueOnce('task/id?x#y'); + + await taskUpdatePreSendAction.call( + context as IExecuteSingleFunctions, + { + url: 'https://example.com', + body: {}, + } as IHttpRequestOptions, + ); + + expect(context.helpers?.requestWithAuthentication).toHaveBeenCalledWith( + 'highLevelApi', + expect.objectContaining({ + uri: 'https://rest.gohighlevel.com/v1/contacts/contact%2Fid/tasks/task%2Fid%3Fx%23y', + }), + ); + }); +}); diff --git a/packages/nodes-base/nodes/HighLevel/v2/GenericFunctions.ts b/packages/nodes-base/nodes/HighLevel/v2/GenericFunctions.ts index 1147371cfd9..edd2b15ca8f 100644 --- a/packages/nodes-base/nodes/HighLevel/v2/GenericFunctions.ts +++ b/packages/nodes-base/nodes/HighLevel/v2/GenericFunctions.ts @@ -16,7 +16,7 @@ import type { IPollFunctions, IWebhookFunctions, } from 'n8n-workflow'; -import { ApplicationError, NodeApiError } from 'n8n-workflow'; +import { ApplicationError, NodeApiError, toPathSegment } from 'n8n-workflow'; const VALID_EMAIL_REGEX = /^(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/; @@ -218,7 +218,13 @@ export const addNotePostReceiveAction = async function ( body: note, }; - await highLevelApiRequest.call(this, 'POST', `/contacts/${contactId}/notes`, requestBody, {}); + await highLevelApiRequest.call( + this, + 'POST', + `/contacts/${toPathSegment(contactId)}/notes`, + requestBody, + {}, + ); return items; }; @@ -231,7 +237,7 @@ export async function taskUpdatePreSendAction( if (!body.title || !body.dueDate) { const contactId = this.getNodeParameter('contactId'); const taskId = this.getNodeParameter('taskId'); - const resource = `/contacts/${contactId}/tasks/${taskId}`; + const resource = `/contacts/${toPathSegment(contactId)}/tasks/${toPathSegment(taskId)}`; const responseData = await highLevelApiRequest.call(this, 'GET', resource); body.title = body.title || responseData.title; // the api response dueDate has to be formatted or it will error on update diff --git a/packages/nodes-base/nodes/HighLevel/v2/HighLevelV2.node.ts b/packages/nodes-base/nodes/HighLevel/v2/HighLevelV2.node.ts index b9039b78a65..cb5e3e8070a 100644 --- a/packages/nodes-base/nodes/HighLevel/v2/HighLevelV2.node.ts +++ b/packages/nodes-base/nodes/HighLevel/v2/HighLevelV2.node.ts @@ -8,7 +8,7 @@ import type { INodeTypeBaseDescription, INodeTypeDescription, } from 'n8n-workflow'; -import { NodeConnectionTypes } from 'n8n-workflow'; +import { NodeConnectionTypes, toPathSegment } from 'n8n-workflow'; import { calendarFields, calendarOperations } from './description/CalendarDescription'; import { contactFields, contactNotes, contactOperations } from './description/ContactDescription'; @@ -125,8 +125,7 @@ export class HighLevelV2 implements INodeType { this, 'highLevelOAuth2Api', { - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - url: `https://services.leadconnectorhq.com/locations/${locationId}/customFields?model=contact`, + url: `https://services.leadconnectorhq.com/locations/${toPathSegment(locationId)}/customFields?model=contact`, headers: { Accept: 'application/json', 'Content-Type': 'application/json', @@ -162,8 +161,7 @@ export class HighLevelV2 implements INodeType { this, 'highLevelOAuth2Api', { - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - url: `https://services.leadconnectorhq.com/locations/${locationId}/timezones`, + url: `https://services.leadconnectorhq.com/locations/${toPathSegment(locationId)}/timezones`, headers: { Accept: 'application/json', 'Content-Type': 'application/json', diff --git a/packages/nodes-base/nodes/HighLevel/v2/description/CalendarDescription.ts b/packages/nodes-base/nodes/HighLevel/v2/description/CalendarDescription.ts index a30f2d0f030..612cfe41410 100644 --- a/packages/nodes-base/nodes/HighLevel/v2/description/CalendarDescription.ts +++ b/packages/nodes-base/nodes/HighLevel/v2/description/CalendarDescription.ts @@ -29,7 +29,7 @@ export const calendarOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/calendars/{{$parameter.calendarId}}/free-slots', + url: '=/calendars/{{toPathSegment($parameter.calendarId)}}/free-slots', }, }, }, diff --git a/packages/nodes-base/nodes/HighLevel/v2/description/ContactDescription.ts b/packages/nodes-base/nodes/HighLevel/v2/description/ContactDescription.ts index 770d2ed26e2..cb29df9534d 100644 --- a/packages/nodes-base/nodes/HighLevel/v2/description/ContactDescription.ts +++ b/packages/nodes-base/nodes/HighLevel/v2/description/ContactDescription.ts @@ -56,7 +56,7 @@ export const contactOperations: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/contacts/{{$parameter.contactId}}/', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/', }, output: { postReceive: [ @@ -77,7 +77,7 @@ export const contactOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/contacts/{{$parameter.contactId}}/', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/', }, output: { postReceive: [ @@ -113,7 +113,7 @@ export const contactOperations: INodeProperties[] = [ routing: { request: { method: 'PUT', - url: '=/contacts/{{$parameter.contactId}}/', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/', }, send: { preSend: [validEmailAndPhonePreSendAction, splitTagsPreSendAction], diff --git a/packages/nodes-base/nodes/HighLevel/v2/description/OpportunityDescription.ts b/packages/nodes-base/nodes/HighLevel/v2/description/OpportunityDescription.ts index b9d7860b7e6..de008f46cb7 100644 --- a/packages/nodes-base/nodes/HighLevel/v2/description/OpportunityDescription.ts +++ b/packages/nodes-base/nodes/HighLevel/v2/description/OpportunityDescription.ts @@ -38,7 +38,7 @@ export const opportunityOperations: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/opportunities/{{$parameter.opportunityId}}', + url: '=/opportunities/{{toPathSegment($parameter.opportunityId)}}', }, output: { postReceive: [ @@ -59,7 +59,7 @@ export const opportunityOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/opportunities/{{$parameter.opportunityId}}', + url: '=/opportunities/{{toPathSegment($parameter.opportunityId)}}', }, }, action: 'Get an opportunity', @@ -85,7 +85,7 @@ export const opportunityOperations: INodeProperties[] = [ routing: { request: { method: 'PUT', - url: '=/opportunities/{{$parameter.opportunityId}}', + url: '=/opportunities/{{toPathSegment($parameter.opportunityId)}}', }, }, action: 'Update an opportunity', diff --git a/packages/nodes-base/nodes/HighLevel/v2/description/TaskDescription.ts b/packages/nodes-base/nodes/HighLevel/v2/description/TaskDescription.ts index 3869d1e3266..dc19842cf21 100644 --- a/packages/nodes-base/nodes/HighLevel/v2/description/TaskDescription.ts +++ b/packages/nodes-base/nodes/HighLevel/v2/description/TaskDescription.ts @@ -24,7 +24,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'POST', - url: '=/contacts/{{$parameter.contactId}}/tasks/', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks/', }, output: { postReceive: [taskPostReceiceAction], @@ -38,7 +38,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/contacts/{{$parameter.contactId}}/tasks/{{$parameter.taskId}}/', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks/{{toPathSegment($parameter.taskId)}}/', }, output: { postReceive: [ @@ -59,7 +59,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/contacts/{{$parameter.contactId}}/tasks/{{$parameter.taskId}}/', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks/{{toPathSegment($parameter.taskId)}}/', }, output: { postReceive: [taskPostReceiceAction], @@ -73,7 +73,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/contacts/{{$parameter.contactId}}/tasks/', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks/', }, output: { postReceive: [ @@ -95,7 +95,7 @@ export const taskOperations: INodeProperties[] = [ routing: { request: { method: 'PUT', - url: '=/contacts/{{$parameter.contactId}}/tasks/{{$parameter.taskId}}/', + url: '=/contacts/{{toPathSegment($parameter.contactId)}}/tasks/{{toPathSegment($parameter.taskId)}}/', }, send: { preSend: [taskUpdatePreSendAction], diff --git a/packages/nodes-base/nodes/HighLevel/v2/test/AddNotePostReceiveAction.test.ts b/packages/nodes-base/nodes/HighLevel/v2/test/AddNotePostReceiveAction.test.ts index 77c790782eb..366704991f2 100644 --- a/packages/nodes-base/nodes/HighLevel/v2/test/AddNotePostReceiveAction.test.ts +++ b/packages/nodes-base/nodes/HighLevel/v2/test/AddNotePostReceiveAction.test.ts @@ -1,4 +1,6 @@ -import { highLevelApiRequest } from '../GenericFunctions'; +import type { IExecuteSingleFunctions, IN8nHttpFullResponse } from 'n8n-workflow'; + +import { addNotePostReceiveAction, highLevelApiRequest } from '../GenericFunctions'; describe('GenericFunctions - highLevelApiRequest', () => { let mockContext: any; @@ -114,3 +116,44 @@ describe('GenericFunctions - highLevelApiRequest', () => { expect(result).toEqual(mockResponse); }); }); + +describe('addNotePostReceiveAction', () => { + it('encodes the contact ID as one URL path segment', async () => { + const httpRequestWithAuthentication = jest.fn().mockResolvedValue({}); + const context = { + getNodeParameter: jest.fn().mockReturnValue('A note'), + helpers: { httpRequestWithAuthentication }, + }; + + await addNotePostReceiveAction.call( + context as unknown as IExecuteSingleFunctions, + [{ json: {} }], + { + body: { contact: { id: 'contact/id?x#y', locationId: 'location-id' } }, + } as IN8nHttpFullResponse, + ); + + expect(httpRequestWithAuthentication).toHaveBeenCalledWith( + 'highLevelOAuth2Api', + expect.objectContaining({ + url: 'https://services.leadconnectorhq.com/contacts/contact%2Fid%3Fx%23y/notes', + }), + ); + }); + + it('rejects an invalid contact ID before sending a request', async () => { + const httpRequestWithAuthentication = jest.fn(); + const context = { + getNodeParameter: jest.fn().mockReturnValue('A note'), + helpers: { httpRequestWithAuthentication }, + }; + + await expect( + addNotePostReceiveAction.call(context as unknown as IExecuteSingleFunctions, [{ json: {} }], { + body: { contact: { id: '..', locationId: 'location-id' } }, + } as IN8nHttpFullResponse), + ).rejects.toThrow('Invalid identifier'); + + expect(httpRequestWithAuthentication).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/nodes-base/nodes/HighLevel/v2/test/TaskUpdatePreSendAction.test.ts b/packages/nodes-base/nodes/HighLevel/v2/test/TaskUpdatePreSendAction.test.ts index 3598126af3a..4f1dadad52e 100644 --- a/packages/nodes-base/nodes/HighLevel/v2/test/TaskUpdatePreSendAction.test.ts +++ b/packages/nodes-base/nodes/HighLevel/v2/test/TaskUpdatePreSendAction.test.ts @@ -32,7 +32,9 @@ describe('taskUpdatePreSendAction', () => { }); it('should fetch missing title and dueDate from the API', async () => { - (mockThis.getNodeParameter as jest.Mock).mockReturnValueOnce('123').mockReturnValueOnce('456'); + (mockThis.getNodeParameter as jest.Mock) + .mockReturnValueOnce('contact/id?x#y') + .mockReturnValueOnce('task/id'); const mockApiResponse = { title: 'Fetched Task Title', @@ -60,6 +62,12 @@ describe('taskUpdatePreSendAction', () => { title: 'Fetched Task Title', dueDate: '2024-12-25T00:00:00+00:00', }); + expect(mockThis.helpers?.httpRequestWithAuthentication).toHaveBeenCalledWith( + 'highLevelOAuth2Api', + expect.objectContaining({ + url: 'https://services.leadconnectorhq.com/contacts/contact%2Fid%3Fx%23y/tasks/task%2Fid', + }), + ); }); it('should only fetch title if dueDate is provided', async () => { diff --git a/packages/nodes-base/nodes/N8n/CredentialDescription.ts b/packages/nodes-base/nodes/N8n/CredentialDescription.ts index 6ca1ef502be..7b670114c79 100644 --- a/packages/nodes-base/nodes/N8n/CredentialDescription.ts +++ b/packages/nodes-base/nodes/N8n/CredentialDescription.ts @@ -33,7 +33,7 @@ export const credentialOperations: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/credentials/{{ $parameter.credentialId }}', + url: '=/credentials/{{ toPathSegment($parameter.credentialId) }}', }, }, }, @@ -44,7 +44,7 @@ export const credentialOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/credentials/schema/{{ $parameter.credentialTypeName }}', + url: '=/credentials/schema/{{ toPathSegment($parameter.credentialTypeName) }}', }, }, }, diff --git a/packages/nodes-base/nodes/N8n/ExecutionDescription.ts b/packages/nodes-base/nodes/N8n/ExecutionDescription.ts index e4c7eaf854a..20233d9d11f 100644 --- a/packages/nodes-base/nodes/N8n/ExecutionDescription.ts +++ b/packages/nodes-base/nodes/N8n/ExecutionDescription.ts @@ -23,7 +23,7 @@ export const executionOperations: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/executions/{{ $parameter.executionId }}', + url: '=/executions/{{ toPathSegment($parameter.executionId) }}', }, }, }, @@ -51,7 +51,7 @@ export const executionOperations: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/executions/{{ $parameter.executionId }}', + url: '=/executions/{{ toPathSegment($parameter.executionId) }}', }, }, }, diff --git a/packages/nodes-base/nodes/N8n/WorkflowDescription.ts b/packages/nodes-base/nodes/N8n/WorkflowDescription.ts index a6bef085499..c625a74559f 100644 --- a/packages/nodes-base/nodes/N8n/WorkflowDescription.ts +++ b/packages/nodes-base/nodes/N8n/WorkflowDescription.ts @@ -94,7 +94,7 @@ const activateOperation: INodeProperties[] = [ routing: { request: { method: 'POST', - url: '=/workflows/{{ $value }}/activate', + url: '=/workflows/{{ toPathSegment($value) }}/activate', }, }, }, @@ -141,7 +141,7 @@ const deactivateOperation: INodeProperties[] = [ routing: { request: { method: 'POST', - url: '=/workflows/{{ $value }}/deactivate', + url: '=/workflows/{{ toPathSegment($value) }}/deactivate', }, }, }, @@ -160,7 +160,7 @@ const deleteOperation: INodeProperties[] = [ routing: { request: { method: 'DELETE', - url: '=/workflows/{{ $value }}', + url: '=/workflows/{{ toPathSegment($value) }}', }, }, }, @@ -303,7 +303,7 @@ const getOperation: INodeProperties[] = [ routing: { request: { method: 'GET', - url: '=/workflows/{{ $value }}', + url: '=/workflows/{{ toPathSegment($value) }}', }, }, }, @@ -322,7 +322,7 @@ const updateOperation: INodeProperties[] = [ routing: { request: { method: 'PUT', - url: '=/workflows/{{ $value }}', + url: '=/workflows/{{ toPathSegment($value) }}', }, }, }, diff --git a/packages/nodes-base/nodes/N8n/test/N8n.structure.test.ts b/packages/nodes-base/nodes/N8n/test/N8n.structure.test.ts new file mode 100644 index 00000000000..69b35d49e3b --- /dev/null +++ b/packages/nodes-base/nodes/N8n/test/N8n.structure.test.ts @@ -0,0 +1,32 @@ +import { N8n } from '../N8n.node'; + +function collectUrls(value: unknown): string[] { + if (Array.isArray(value)) return value.flatMap(collectUrls); + if (value === null || typeof value !== 'object') return []; + + return Object.entries(value).flatMap(([key, child]) => { + if (key === 'url' && typeof child === 'string') return [child]; + return collectUrls(child); + }); +} + +describe('n8n Node Structure', () => { + it('encodes every dynamic request URL path segment', () => { + const dynamicUrls = collectUrls(new N8n().description.properties).filter( + (url) => url.includes('$value') || url.includes('$parameter'), + ); + + expect(dynamicUrls.length).toBeGreaterThan(0); + for (const url of dynamicUrls) { + const dynamicExpressions = [...url.matchAll(/{{\s*(.*?)\s*}}/g)] + .map((match) => match[1]) + .filter((expression) => expression.includes('$value') || expression.includes('$parameter')); + + expect(url.replace(/{{\s*(.*?)\s*}}/g, '')).not.toMatch(/\$(?:value|parameter)/); + expect(dynamicExpressions.length).toBeGreaterThan(0); + for (const expression of dynamicExpressions) { + expect(expression).toMatch(/^toPathSegment\(.+\)$/); + } + } + }); +}); diff --git a/packages/nodes-base/nodes/Notion/NotionTrigger.node.ts b/packages/nodes-base/nodes/Notion/NotionTrigger.node.ts index 09d0e192fe9..ec35f0974fb 100644 --- a/packages/nodes-base/nodes/Notion/NotionTrigger.node.ts +++ b/packages/nodes-base/nodes/Notion/NotionTrigger.node.ts @@ -5,6 +5,7 @@ import { type INodeExecutionData, type INodeType, type INodeTypeDescription, + toPathSegment, NodeConnectionTypes, } from 'n8n-workflow'; @@ -198,7 +199,7 @@ export class NotionTrigger implements INodeType { let { results: data } = await notionApiRequest.call( this, 'POST', - `/databases/${databaseId}/query`, + `/databases/${toPathSegment(databaseId)}/query`, body, {}, '', @@ -221,7 +222,7 @@ export class NotionTrigger implements INodeType { const { results, has_more, next_cursor } = await notionApiRequest.call( this, 'POST', - `/databases/${databaseId}/query`, + `/databases/${toPathSegment(databaseId)}/query`, body, {}, '', diff --git a/packages/nodes-base/nodes/Notion/shared/GenericFunctions.ts b/packages/nodes-base/nodes/Notion/shared/GenericFunctions.ts index e65c79f93f4..0d3e761688b 100644 --- a/packages/nodes-base/nodes/Notion/shared/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Notion/shared/GenericFunctions.ts @@ -18,7 +18,7 @@ import type { IRequestOptions, JsonObject, } from 'n8n-workflow'; -import { NodeApiError, NodeOperationError, safeRegex } from 'n8n-workflow'; +import { toPathSegment, NodeApiError, NodeOperationError, safeRegex } from 'n8n-workflow'; import { validate as uuidValidate } from 'uuid'; import { blockUrlExtractionRegexp, databasePageUrlValidationRegexp } from './constants'; @@ -129,7 +129,7 @@ export async function notionApiRequestGetBlockChildrens( this, 'results', 'GET', - `/blocks/${block.id}/children`, + `/blocks/${toPathSegment(block.id)}/children`, ); childrens = (childrens || []).map((entry: IDataObject) => ({ diff --git a/packages/nodes-base/nodes/Notion/v1/NotionV1.node.ts b/packages/nodes-base/nodes/Notion/v1/NotionV1.node.ts index 1cc7a817345..a2d3602f751 100644 --- a/packages/nodes-base/nodes/Notion/v1/NotionV1.node.ts +++ b/packages/nodes-base/nodes/Notion/v1/NotionV1.node.ts @@ -9,6 +9,7 @@ import type { INodeTypeBaseDescription, INodeTypeDescription, } from 'n8n-workflow'; +import { toPathSegment } from 'n8n-workflow'; import { versionDescription } from './VersionDescription'; import type { SortData } from '../shared/GenericFunctions'; @@ -46,7 +47,11 @@ export class NotionV1 implements INodeType { const databaseId = this.getCurrentNodeParameter('databaseId', { extractValue: true, }) as string; - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); for (const key of Object.keys(properties as IDataObject)) { //remove parameters that cannot be set from the API. if ( @@ -82,7 +87,11 @@ export class NotionV1 implements INodeType { const databaseId = this.getCurrentNodeParameter('databaseId', { extractValue: true, }) as string; - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); for (const key of Object.keys(properties as IDataObject)) { returnData.push({ name: `${key} - (${properties[key].type})`, @@ -110,7 +119,11 @@ export class NotionV1 implements INodeType { }) as string; const resource = this.getCurrentNodeParameter('resource') as string; const operation = this.getCurrentNodeParameter('operation') as string; - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); if (resource === 'databasePage') { if (['multi_select', 'select'].includes(type) && operation === 'getAll') { return properties[name][type].options.map((option: IDataObject) => ({ @@ -149,8 +162,12 @@ export class NotionV1 implements INodeType { ); const { parent: { database_id: databaseId }, - } = await notionApiRequest.call(this, 'GET', `/pages/${pageId}`); - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + } = await notionApiRequest.call(this, 'GET', `/pages/${toPathSegment(pageId)}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); for (const key of Object.keys(properties as IDataObject)) { //remove parameters that cannot be set from the API. if ( @@ -190,8 +207,12 @@ export class NotionV1 implements INodeType { const [name, type] = (this.getCurrentNodeParameter('&key') as string).split('|'); const { parent: { database_id: databaseId }, - } = await notionApiRequest.call(this, 'GET', `/pages/${pageId}`); - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + } = await notionApiRequest.call(this, 'GET', `/pages/${toPathSegment(pageId)}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); return properties[name][type].options.map((option: IDataObject) => ({ name: option.name, value: option.id, @@ -245,7 +266,7 @@ export class NotionV1 implements INodeType { const block = await notionApiRequest.call( this, 'PATCH', - `/blocks/${blockId}/children`, + `/blocks/${toPathSegment(blockId)}/children`, body, ); @@ -268,7 +289,7 @@ export class NotionV1 implements INodeType { this, 'results', 'GET', - `/blocks/${blockId}/children`, + `/blocks/${toPathSegment(blockId)}/children`, {}, ); } else { @@ -276,7 +297,7 @@ export class NotionV1 implements INodeType { responseData = await notionApiRequest.call( this, 'GET', - `/blocks/${blockId}/children`, + `/blocks/${toPathSegment(blockId)}/children`, {}, qs, ); @@ -298,7 +319,11 @@ export class NotionV1 implements INodeType { const databaseId = extractDatabaseId( this.getNodeParameter('databaseId', i, '', { extractValue: true }) as string, ); - responseData = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + responseData = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); const executionData = this.helpers.constructExecutionMetaData( this.helpers.returnJsonArray(responseData as IDataObject), @@ -421,7 +446,7 @@ export class NotionV1 implements INodeType { this, 'results', 'POST', - `/databases/${databaseId}/query`, + `/databases/${toPathSegment(databaseId)}/query`, body, {}, ); @@ -430,7 +455,7 @@ export class NotionV1 implements INodeType { responseData = await notionApiRequest.call( this, 'POST', - `/databases/${databaseId}/query`, + `/databases/${toPathSegment(databaseId)}/query`, body, qs, ); @@ -466,7 +491,12 @@ export class NotionV1 implements INodeType { if (properties.length !== 0) { body.properties = mapProperties.call(this, properties, timezone) as IDataObject; } - responseData = await notionApiRequest.call(this, 'PATCH', `/pages/${pageId}`, body); + responseData = await notionApiRequest.call( + this, + 'PATCH', + `/pages/${toPathSegment(pageId)}`, + body, + ); if (simple) { responseData = simplifyObjects(responseData, false, 1); } @@ -484,7 +514,11 @@ export class NotionV1 implements INodeType { if (operation === 'get') { for (let i = 0; i < length; i++) { const userId = this.getNodeParameter('userId', i) as string; - responseData = await notionApiRequest.call(this, 'GET', `/users/${userId}`); + responseData = await notionApiRequest.call( + this, + 'GET', + `/users/${toPathSegment(userId)}`, + ); const executionData = this.helpers.constructExecutionMetaData( this.helpers.returnJsonArray(responseData as IDataObject), @@ -555,7 +589,11 @@ export class NotionV1 implements INodeType { for (let i = 0; i < length; i++) { const pageId = extractPageId(this.getNodeParameter('pageId', i) as string); const simple = this.getNodeParameter('simple', i) as boolean; - responseData = await notionApiRequest.call(this, 'GET', `/pages/${pageId}`); + responseData = await notionApiRequest.call( + this, + 'GET', + `/pages/${toPathSegment(pageId)}`, + ); if (simple) { responseData = simplifyObjects(responseData, false, 1); } diff --git a/packages/nodes-base/nodes/Notion/v2/NotionV2.node.ts b/packages/nodes-base/nodes/Notion/v2/NotionV2.node.ts index a5b08b3fe3e..00bf96e4947 100644 --- a/packages/nodes-base/nodes/Notion/v2/NotionV2.node.ts +++ b/packages/nodes-base/nodes/Notion/v2/NotionV2.node.ts @@ -6,7 +6,7 @@ import type { INodeTypeBaseDescription, INodeTypeDescription, } from 'n8n-workflow'; -import { jsonParse, NodeApiError } from 'n8n-workflow'; +import { toPathSegment, jsonParse, NodeApiError } from 'n8n-workflow'; import { loadOptions } from './methods'; import { versionDescription } from './VersionDescription'; @@ -75,7 +75,7 @@ export class NotionV2 implements INodeType { const block = await notionApiRequest.call( this, 'PATCH', - `/blocks/${blockId}/children`, + `/blocks/${toPathSegment(blockId)}/children`, body, ); @@ -109,7 +109,7 @@ export class NotionV2 implements INodeType { this, 'results', 'GET', - `/blocks/${blockId}/children`, + `/blocks/${toPathSegment(blockId)}/children`, {}, ); @@ -122,7 +122,7 @@ export class NotionV2 implements INodeType { responseData = await notionApiRequest.call( this, 'GET', - `/blocks/${blockId}/children`, + `/blocks/${toPathSegment(blockId)}/children`, {}, qs, ); @@ -181,7 +181,11 @@ export class NotionV2 implements INodeType { const databaseId = extractDatabaseId( this.getNodeParameter('databaseId', i, '', { extractValue: true }) as string, ); - responseData = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + responseData = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); if (simple) { responseData = simplifyObjects(responseData, download)[0]; } @@ -316,7 +320,11 @@ export class NotionV2 implements INodeType { const databaseId = this.getNodeParameter('databaseId', 0, '', { extractValue: true, }) as string; - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); let titleKey = ''; for (const key of Object.keys(properties as IDataObject)) { if (properties[key].type === 'title') { @@ -403,7 +411,11 @@ export class NotionV2 implements INodeType { const pageId = getPageId.call(this, i); const simple = this.getNodeParameter('simple', i) as boolean; - responseData = await notionApiRequest.call(this, 'GET', `/pages/${pageId}`); + responseData = await notionApiRequest.call( + this, + 'GET', + `/pages/${toPathSegment(pageId)}`, + ); if (simple) { responseData = simplifyObjects(responseData, download); } @@ -479,7 +491,7 @@ export class NotionV2 implements INodeType { this, 'results', 'POST', - `/databases/${databaseId}/query`, + `/databases/${toPathSegment(databaseId)}/query`, body, {}, ); @@ -488,7 +500,7 @@ export class NotionV2 implements INodeType { responseData = await notionApiRequest.call( this, 'POST', - `/databases/${databaseId}/query`, + `/databases/${toPathSegment(databaseId)}/query`, body, qs, ); @@ -548,7 +560,12 @@ export class NotionV2 implements INodeType { } } - responseData = await notionApiRequest.call(this, 'PATCH', `/pages/${pageId}`, body); + responseData = await notionApiRequest.call( + this, + 'PATCH', + `/pages/${toPathSegment(pageId)}`, + body, + ); if (simple) { responseData = simplifyObjects(responseData, false); } @@ -577,7 +594,11 @@ export class NotionV2 implements INodeType { for (let i = 0; i < itemsLength; i++) { try { const userId = this.getNodeParameter('userId', i) as string; - responseData = await notionApiRequest.call(this, 'GET', `/users/${userId}`); + responseData = await notionApiRequest.call( + this, + 'GET', + `/users/${toPathSegment(userId)}`, + ); const executionData = this.helpers.constructExecutionMetaData( this.helpers.returnJsonArray(responseData as IDataObject), @@ -633,9 +654,14 @@ export class NotionV2 implements INodeType { try { const pageId = getPageId.call(this, i); const simple = this.getNodeParameter('simple', i) as boolean; - responseData = await notionApiRequest.call(this, 'PATCH', `/pages/${pageId}`, { - archived: true, - }); + responseData = await notionApiRequest.call( + this, + 'PATCH', + `/pages/${toPathSegment(pageId)}`, + { + archived: true, + }, + ); if (simple) { responseData = simplifyObjects(responseData, download); } diff --git a/packages/nodes-base/nodes/Notion/v2/methods/loadOptions.ts b/packages/nodes-base/nodes/Notion/v2/methods/loadOptions.ts index 0410032a29c..1d04a76cbb1 100644 --- a/packages/nodes-base/nodes/Notion/v2/methods/loadOptions.ts +++ b/packages/nodes-base/nodes/Notion/v2/methods/loadOptions.ts @@ -1,5 +1,6 @@ import moment from 'moment-timezone'; import type { IDataObject, ILoadOptionsFunctions, INodePropertyOptions } from 'n8n-workflow'; +import { toPathSegment } from 'n8n-workflow'; import { extractPageId, @@ -15,7 +16,11 @@ export async function getDatabaseProperties( const databaseId = this.getCurrentNodeParameter('databaseId', { extractValue: true, }) as string; - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); for (const key of Object.keys(properties as IDataObject)) { //remove parameters that cannot be set from the API. if ( @@ -53,7 +58,11 @@ export async function getFilterProperties( const databaseId = this.getCurrentNodeParameter('databaseId', { extractValue: true, }) as string; - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); for (const key of Object.keys(properties as IDataObject)) { returnData.push({ name: `${key}`, @@ -85,7 +94,11 @@ export async function getPropertySelectValues( }) as string; const resource = this.getCurrentNodeParameter('resource') as string; const operation = this.getCurrentNodeParameter('operation') as string; - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); if (resource === 'databasePage') { if (['multi_select', 'select', 'status'].includes(type) && operation === 'getAll') { return properties[name][type].options.map((option: IDataObject) => ({ @@ -131,8 +144,12 @@ export async function getDatabaseIdFromPage( ); const { parent: { database_id: databaseId }, - } = await notionApiRequest.call(this, 'GET', `/pages/${pageId}`); - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + } = await notionApiRequest.call(this, 'GET', `/pages/${toPathSegment(pageId)}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); for (const key of Object.keys(properties as IDataObject)) { //remove parameters that cannot be set from the API. if ( @@ -172,8 +189,12 @@ export async function getDatabaseOptionsFromPage( const [name, type] = (this.getCurrentNodeParameter('&key') as string).split('|'); const { parent: { database_id: databaseId }, - } = await notionApiRequest.call(this, 'GET', `/pages/${pageId}`); - const { properties } = await notionApiRequest.call(this, 'GET', `/databases/${databaseId}`); + } = await notionApiRequest.call(this, 'GET', `/pages/${toPathSegment(pageId)}`); + const { properties } = await notionApiRequest.call( + this, + 'GET', + `/databases/${toPathSegment(databaseId)}`, + ); return properties[name][type].options.map((option: IDataObject) => ({ name: option.name, value: option.name, diff --git a/packages/nodes-base/nodes/Oracle/Sql/actions/common.descriptions.ts b/packages/nodes-base/nodes/Oracle/Sql/actions/common.descriptions.ts index c8985548af8..4689fce886c 100644 --- a/packages/nodes-base/nodes/Oracle/Sql/actions/common.descriptions.ts +++ b/packages/nodes-base/nodes/Oracle/Sql/actions/common.descriptions.ts @@ -1,4 +1,4 @@ -import type { INodeProperties } from 'n8n-workflow'; +import type { INodeProperties, INodePropertyOptions } from 'n8n-workflow'; const stmtBatchOptions = [ { @@ -381,6 +381,47 @@ export const tableRLC: INodeProperties = { ], }; +// Single source of truth for the WHERE Operator dropdown; the query builder derives +// its allow-list from this so adding an operator here also permits it at runtime. +export const operatorOptions: INodePropertyOptions[] = [ + { + name: 'Equal', + value: 'equal', + }, + { + name: 'Not Equal', + value: '!=', + }, + { + name: 'Like', + value: 'LIKE', + }, + { + name: 'Greater Than', + value: '>', + }, + { + name: 'Less Than', + value: '<', + }, + { + name: 'Greater Than Or Equal', + value: '>=', + }, + { + name: 'Less Than Or Equal', + value: '<=', + }, + { + name: 'Is Null', + value: 'IS NULL', + }, + { + name: 'Is Not Null', + value: 'IS NOT NULL', + }, +]; + export const whereFixedCollection: INodeProperties = { displayName: 'Select Rows', name: 'where', @@ -417,45 +458,7 @@ export const whereFixedCollection: INodeProperties = { type: 'options', description: "The operator to check the column against. When using 'LIKE' operator percent sign ( %) matches zero or more characters, underscore ( _ ) matches any single character.", - // eslint-disable-next-line n8n-nodes-base/node-param-options-type-unsorted-items - options: [ - { - name: 'Equal', - value: 'equal', - }, - { - name: 'Not Equal', - value: '!=', - }, - { - name: 'Like', - value: 'LIKE', - }, - { - name: 'Greater Than', - value: '>', - }, - { - name: 'Less Than', - value: '<', - }, - { - name: 'Greater Than Or Equal', - value: '>=', - }, - { - name: 'Less Than Or Equal', - value: '<=', - }, - { - name: 'Is Null', - value: 'IS NULL', - }, - { - name: 'Is Not Null', - value: 'IS NOT NULL', - }, - ], + options: operatorOptions, default: 'equal', }, { diff --git a/packages/nodes-base/nodes/Oracle/Sql/actions/database/deleteTable.operation.ts b/packages/nodes-base/nodes/Oracle/Sql/actions/database/deleteTable.operation.ts index 20d857aa6fc..c18caf04fe9 100644 --- a/packages/nodes-base/nodes/Oracle/Sql/actions/database/deleteTable.operation.ts +++ b/packages/nodes-base/nodes/Oracle/Sql/actions/database/deleteTable.operation.ts @@ -17,6 +17,7 @@ import type { } from '../../helpers/interfaces'; import { quoteSqlIdentifier, + escapeSqlStringLiteral, addWhereClauses, getColumnMetaData, getColumnMap, @@ -111,11 +112,13 @@ export async function execute( const quotedTableName = quoteSqlIdentifier(schema) + '.' + quoteSqlIdentifier(table); if (deleteCommand === 'drop') { + // Embedded in a single-quoted PL/SQL literal, so ' in the identifier must be doubled. + const dropTableName = escapeSqlStringLiteral(quotedTableName); query = `DECLARE e_table_missing EXCEPTION; PRAGMA EXCEPTION_INIT(e_table_missing, -942); BEGIN - EXECUTE IMMEDIATE ('DROP TABLE ${quotedTableName} PURGE'); + EXECUTE IMMEDIATE ('DROP TABLE ${dropTableName} PURGE'); EXCEPTION WHEN e_table_missing THEN NULL; END;`; @@ -133,6 +136,8 @@ export async function execute( whereClauses, combineConditions, columnMetaDataObject, + this.getNode(), + i, ); } else { throw new NodeOperationError( @@ -178,6 +183,8 @@ export async function execute( whereClauses, combineConditions, columnMetaDataObject, + this.getNode(), + 0, true, ); diff --git a/packages/nodes-base/nodes/Oracle/Sql/actions/database/select.operation.ts b/packages/nodes-base/nodes/Oracle/Sql/actions/database/select.operation.ts index 66832c62e86..cf4c62a348b 100644 --- a/packages/nodes-base/nodes/Oracle/Sql/actions/database/select.operation.ts +++ b/packages/nodes-base/nodes/Oracle/Sql/actions/database/select.operation.ts @@ -112,7 +112,14 @@ export async function execute( const whereClauses = ((this.getNodeParameter('where', i, []) as IDataObject).values as WhereClause[]) || []; const combineConditions = this.getNodeParameter('combineConditions', i, 'AND') as string; - [query, values] = addWhereClauses(query, whereClauses, combineConditions, columnMetaDataObject); + [query, values] = addWhereClauses( + query, + whereClauses, + combineConditions, + columnMetaDataObject, + this.getNode(), + i, + ); const sortRules = ((this.getNodeParameter('sort', i, []) as IDataObject).values as SortRule[]) || []; diff --git a/packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts b/packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts index b3333dd42f0..ff4fed748b0 100644 --- a/packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts +++ b/packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts @@ -25,6 +25,7 @@ import type { ColumnMap, OracleDBNodeOptions, } from './interfaces'; +import { operatorOptions } from '../actions/common.descriptions'; const n8nTypetoDBType: { [key: string]: oracledb.DbType } = { boolean: oracledb.DB_TYPE_BOOLEAN, @@ -197,6 +198,10 @@ export function quoteSqlIdentifier(name: string): string { * Invalid examples: * my"column → contains an illegal ". * my..column → double dot not allowed. + * + * A single quote is legal here: Oracle double-quoted identifiers may contain + * any character except " (e.g. "O'Brien"). Callers that embed the result in a + * string literal (the Drop path) must escape it via escapeSqlStringLiteral. */ const validateRegex = /^(?:"[^"]+"|[^".]+)(?:\.(?:"[^"]+"|[^".]+))*$/; if (!validateRegex.test(name)) { @@ -217,6 +222,25 @@ export function quoteSqlIdentifier(name: string): string { return quotedParts.join('.'); } +// Escapes a value for embedding inside an Oracle single-quoted text literal. +// Doubling ' is Oracle's only literal escape (no backslash processing), so a +// quoted identifier such as "O'Brien" cannot terminate the surrounding literal. +export function escapeSqlStringLiteral(value: string): string { + return value.replace(/'/g, "''"); +} + +// Operators are concatenated into the WHERE clause, so only a fixed set is allowed. +// Derived from the Operator dropdown (so a new dropdown entry is permitted automatically), +// plus the case/synonym variants Oracle accepts that arrive via an expression-driven value. +const VALID_WHERE_OPERATORS = new Set([ + ...operatorOptions.map((option) => + option.value === 'equal' ? '=' : String(option.value).toUpperCase(), + ), + '<>', + '^=', + 'NOT LIKE', +]); + export function addSortRules(query: string, rules: SortRule[]): string { if (rules.length === 0) return query; @@ -654,6 +678,8 @@ export function addWhereClauses( clauses: WhereClause[], combineConditions: string, schema: ColumnMap, + node: INode, + itemIndex: number, isExecuteMany: boolean = false, ): [string, oracledb.BindParameter[] | oracledb.BindDefinition[]] { if (clauses.length === 0) return [query, []]; @@ -672,6 +698,22 @@ export function addWhereClauses( clause.condition = '='; } + // The operator can be expression-driven, so accept the case/whitespace variants + // Oracle itself tolerates, then check against the fixed set it is concatenated from. + // An expression can also yield a non-string; route that to the invalid-operator error. + const normalizedCondition = + typeof clause.condition === 'string' + ? clause.condition.trim().replace(/\s+/g, ' ').toUpperCase() + : ''; + if (!VALID_WHERE_OPERATORS.has(normalizedCondition)) { + throw new NodeOperationError( + node, + `Operator "${clause.condition}" is not valid. Supported operators: ${[...VALID_WHERE_OPERATORS].join(', ')}`, + { itemIndex }, + ); + } + clause.condition = normalizedCondition; + // The condition value is json type, so convert to required type only // if fixed expression is used instead of n8n expressions. if (typeof clause.value === 'string') { diff --git a/packages/nodes-base/nodes/Oracle/Sql/test/operations.test.ts b/packages/nodes-base/nodes/Oracle/Sql/test/operations.test.ts index 53a5543274a..872fdfb3541 100644 --- a/packages/nodes-base/nodes/Oracle/Sql/test/operations.test.ts +++ b/packages/nodes-base/nodes/Oracle/Sql/test/operations.test.ts @@ -681,6 +681,34 @@ VALUES ( expect(runQueries).toHaveBeenCalledWith(queries, emptyInputItems, nodeOptions); }, ); + + it('should neutralise an injected Drop table name by escaping the string literal', async () => { + const nodeParameters: IDataObject = { + operation: 'deleteTable', + schema: { __rl: true, mode: 'list', value: CONFIG.user }, + table: { + __rl: true, + mode: 'list', + value: "X') PURGE; EXECUTE IMMEDIATE 'DROP TABLE SENSITIVE_DATA", + }, + deleteCommand: 'drop', + options: {}, + }; + const mockThis = createMockExecuteFunction(nodeParameters); + const items = [{ json: {}, pairedItem: { item: 0, input: undefined } }]; + const runQueries = getRunQueriesFn(mockThis, pool); + + await deleteTable.execute.call(mockThis, runQueries, items, {}, pool); + + expect(runQueries).toHaveBeenCalledTimes(1); + const { query } = runQueries.mock.calls[0][0][0]; + // The injected quotes are doubled, so they stay inside the identifier and + // cannot terminate the EXECUTE IMMEDIATE literal. + expect(query).toContain( + ".\"X'') PURGE; EXECUTE IMMEDIATE ''DROP TABLE SENSITIVE_DATA\" PURGE')", + ); + expect(query).not.toContain("X') PURGE"); + }); }); describe('Test execute operation', () => { diff --git a/packages/nodes-base/nodes/Oracle/Sql/test/utils.test.ts b/packages/nodes-base/nodes/Oracle/Sql/test/utils.test.ts index 902e7797776..6795ae16a7e 100644 --- a/packages/nodes-base/nodes/Oracle/Sql/test/utils.test.ts +++ b/packages/nodes-base/nodes/Oracle/Sql/test/utils.test.ts @@ -1,14 +1,145 @@ +import { mock } from 'jest-mock-extended'; import { DateTime } from 'luxon'; +import type { INode } from 'n8n-workflow'; import * as oracleDBTypes from 'oracledb'; -import type { ExecuteOpBindParam } from '../helpers/interfaces'; +import type { ColumnMap, ExecuteOpBindParam } from '../helpers/interfaces'; import { addSortRules, + addWhereClauses, + escapeSqlStringLiteral, getBindParameters, getCompatibleValue, getOutBindDefsForExecute, + quoteSqlIdentifier, } from '../helpers/utils'; +describe('Test addWhereClauses', () => { + const schema: ColumnMap = { ID: { type: 'NUMBER', nullable: true, maxSize: 0 } }; + const node = mock(); + + const build = (condition: string, value: unknown = 1) => + addWhereClauses( + 'DELETE FROM "T"', + [{ column: 'ID', condition, value }], + 'AND', + schema, + node, + 0, + ); + + it('should normalise the "equal" operator to =', () => { + const [query] = build('equal'); + expect(query).toEqual('DELETE FROM "T" WHERE "ID" = :0'); + }); + + it('should keep the column and value out of the concatenated SQL', () => { + const [query, binds] = build('>', 5); + expect(query).toEqual('DELETE FROM "T" WHERE "ID" > :0'); + expect(binds).toHaveLength(1); + }); + + it('should build value-less clauses for IS NULL / IS NOT NULL', () => { + const [query, binds] = build('IS NULL', undefined); + expect(query).toEqual('DELETE FROM "T" WHERE "ID" IS NULL'); + expect(binds).toHaveLength(0); + }); + + it.each(['<>', '^=', 'NOT LIKE'])('should accept the %s operator synonym', (condition) => { + const [query] = build(condition); + expect(query).toEqual(`DELETE FROM "T" WHERE "ID" ${condition} :0`); + }); + + it('should accept a lowercase operator, normalising it to uppercase', () => { + const [query] = build('like'); + expect(query).toEqual('DELETE FROM "T" WHERE "ID" LIKE :0'); + }); + + it('should accept a lowercase "is null" and build a value-less clause', () => { + const [query, binds] = build('is null', undefined); + expect(query).toEqual('DELETE FROM "T" WHERE "ID" IS NULL'); + expect(binds).toHaveLength(0); + }); + + it('should tolerate surrounding whitespace in the operator', () => { + const [query] = build(' = '); + expect(query).toEqual('DELETE FROM "T" WHERE "ID" = :0'); + }); + + it('should reject an operator outside the allowed set', () => { + expect(() => build("= 'x' OR 1=1 --")).toThrow('is not valid'); + }); + + it('should reject an empty operator', () => { + expect(() => build('')).toThrow('is not valid'); + }); + + it.each([5, null, { op: '=' }])( + 'should reject the non-string operator %o with the operator error', + (condition) => { + expect(() => build(condition as unknown as string)).toThrow('is not valid'); + }, + ); +}); + +describe('Test quoteSqlIdentifier', () => { + it('should wrap a simple identifier in double quotes', () => { + expect(quoteSqlIdentifier('employees')).toEqual('"employees"'); + }); + + it('should quote each part of a dot-separated identifier', () => { + expect(quoteSqlIdentifier('scott.employees')).toEqual('"scott"."employees"'); + }); + + it('should preserve an already-quoted identifier', () => { + expect(quoteSqlIdentifier('"My Table"')).toEqual('"My Table"'); + }); + + it('should trim surrounding whitespace before quoting', () => { + expect(quoteSqlIdentifier(' employees ')).toEqual('"employees"'); + }); + + it('should reject an identifier containing a double quote', () => { + expect(() => quoteSqlIdentifier('my"table')).toThrow('is not valid'); + }); + + it('should accept a single quote, which is legal in an Oracle identifier', () => { + expect(quoteSqlIdentifier("O'Brien")).toEqual('"O\'Brien"'); + }); + + it('should preserve a single quote inside an already-quoted identifier', () => { + expect(quoteSqlIdentifier('"O\'Brien"')).toEqual('"O\'Brien"'); + }); + + it('should quote (not reject) a value crafted to close a string literal', () => { + // The literal-context safety comes from escapeSqlStringLiteral, not from rejection here. + expect(quoteSqlIdentifier("X') PURGE; EXECUTE IMMEDIATE 'DROP TABLE SENSITIVE_DATA")).toEqual( + '"X\') PURGE; EXECUTE IMMEDIATE \'DROP TABLE SENSITIVE_DATA"', + ); + }); + + it('should reject a double-dot identifier', () => { + expect(() => quoteSqlIdentifier('my..column')).toThrow('is not valid'); + }); +}); + +describe('Test escapeSqlStringLiteral', () => { + it('should double a single quote so it cannot terminate a literal', () => { + expect(escapeSqlStringLiteral('"O\'Brien"')).toEqual('"O\'\'Brien"'); + }); + + it('should leave a value without single quotes unchanged', () => { + expect(escapeSqlStringLiteral('"scott"."employees"')).toEqual('"scott"."employees"'); + }); + + it('should neutralise an identifier crafted to close the DROP literal', () => { + const quoted = quoteSqlIdentifier("X') PURGE; EXECUTE IMMEDIATE 'DROP TABLE SENSITIVE_DATA"); + expect(escapeSqlStringLiteral(quoted)).toEqual( + "\"X'') PURGE; EXECUTE IMMEDIATE ''DROP TABLE SENSITIVE_DATA\"", + ); + }); +}); + describe('Test addSortRules', () => { it('should ORDER BY ASC', () => { const query = 'SELECT * FROM "scott"."employees"'; diff --git a/packages/nodes-base/nodes/Supabase/GenericFunctions.ts b/packages/nodes-base/nodes/Supabase/GenericFunctions.ts index c6c7b457288..04dd9d4234a 100644 --- a/packages/nodes-base/nodes/Supabase/GenericFunctions.ts +++ b/packages/nodes-base/nodes/Supabase/GenericFunctions.ts @@ -302,6 +302,39 @@ export function getFilters( }, default: '', placeholder: 'name=eq.jhon', + hint: 'Use $1, $2, etc. and the parameters below to reference dynamic values, rather than building this string with an expression, to avoid PostgREST filter injection', + }, + { + displayName: 'Filters (String) Parameters', + name: 'filterStringParameters', + type: 'fixedCollection', + typeOptions: { + multipleValues: true, + }, + displayOptions: { + show: { + resource: resources, + operation: operations, + filterType: ['string'], + }, + }, + default: {}, + placeholder: 'Add Parameter', + options: [ + { + displayName: 'Values', + name: 'values', + values: [ + { + displayName: 'Value', + name: 'value', + type: 'string', + default: '', + }, + ], + }, + ], + description: 'Values to substitute for $1, $2, etc. in the filter string above.', }, ]; } @@ -363,6 +396,37 @@ export const buildOrQuery = (value: IDataObject) => export const buildGetQuery = (query: Map, value: IDataObject) => query.set(quotePostgrestComponent(value.keyName), `eq.${String(value.keyValue)}`); +export function applyFilterStringParameters(filterString: string, parameters: IDataObject[]) { + if (parameters.length === 0) return filterString; + + return filterString.replace(/\$(\d+)/g, (_match, index: string) => { + const position = Number(index) - 1; + if (position < 0 || position >= parameters.length) { + throw new UserError( + `Filters (String) references parameter $${index}, but only ${parameters.length} parameter(s) were provided`, + ); + } + + return encodeURIComponent(quotePostgrestComponent(parameters[position].value)); + }); +} + +export function appendFilterStringToEndpoint( + context: IExecuteFunctions, + endpoint: string, + itemIndex: number, +) { + const filterString = context.getNodeParameter('filterString', itemIndex) as string; + const filterStringParameters = context.getNodeParameter( + 'filterStringParameters.values', + itemIndex, + [], + ) as IDataObject[]; + + const encodedTemplate = encodeURI(filterString); + return `${endpoint}?${applyFilterStringParameters(encodedTemplate, filterStringParameters)}`; +} + export async function validateCredentials( this: ICredentialTestFunctions, decryptedCredentials: ICredentialDataDecryptedObject, diff --git a/packages/nodes-base/nodes/Supabase/Supabase.node.ts b/packages/nodes-base/nodes/Supabase/Supabase.node.ts index 1a9290b1464..95f2a524872 100644 --- a/packages/nodes-base/nodes/Supabase/Supabase.node.ts +++ b/packages/nodes-base/nodes/Supabase/Supabase.node.ts @@ -11,9 +11,10 @@ import type { INodeType, INodeTypeDescription, } from 'n8n-workflow'; -import { NodeConnectionTypes, NodeOperationError } from 'n8n-workflow'; +import { NodeConnectionTypes, NodeOperationError, toPathSegment } from 'n8n-workflow'; import { + appendFilterStringToEndpoint, buildGetQuery, buildOrQuery, buildQuery, @@ -175,7 +176,7 @@ export class Supabase implements INodeType { const operation = this.getNodeParameter('operation', 0); if (resource === 'row') { - const tableId = this.getNodeParameter('tableId', 0) as string; + const tableId = toPathSegment(this.getNodeParameter('tableId', 0)); if (operation === 'create') { const records: IDataObject[] = []; @@ -265,8 +266,7 @@ export class Supabase implements INodeType { } if (filterType === 'string') { - const filterString = this.getNodeParameter('filterString', i) as string; - endpoint = `${endpoint}?${encodeURI(filterString)}`; + endpoint = appendFilterStringToEndpoint(this, endpoint, i); } let rows; @@ -367,8 +367,7 @@ export class Supabase implements INodeType { } if (filterType === 'string') { - const filterString = this.getNodeParameter('filterString', i) as string; - endpoint = `${endpoint}?${encodeURI(filterString)}`; + endpoint = appendFilterStringToEndpoint(this, endpoint, i); } if (!returnAll) { @@ -442,8 +441,7 @@ export class Supabase implements INodeType { } if (filterType === 'string') { - const filterString = this.getNodeParameter('filterString', i) as string; - endpoint = `${endpoint}?${encodeURI(filterString)}`; + endpoint = appendFilterStringToEndpoint(this, endpoint, i); } const record: IDataObject = {}; diff --git a/packages/nodes-base/nodes/Supabase/tests/Supabase.node.test.ts b/packages/nodes-base/nodes/Supabase/tests/Supabase.node.test.ts index 32ae18d0147..002886388f1 100644 --- a/packages/nodes-base/nodes/Supabase/tests/Supabase.node.test.ts +++ b/packages/nodes-base/nodes/Supabase/tests/Supabase.node.test.ts @@ -7,6 +7,7 @@ import { type INodeExecutionData, type IPairedItemData, NodeOperationError, + UserError, } from 'n8n-workflow'; import * as utils from '../GenericFunctions'; @@ -135,6 +136,102 @@ describe('Test Supabase Node', () => { }); }); + describe('filter string parameters', () => { + it('should substitute $1 with a quoted value when it contains reserved characters', () => { + const resolved = utils.applyFilterStringParameters('or=(email.eq.$1)', [ + { value: 'nobody@x.com,id.gt.0' }, + ]); + + expect(decodeURIComponent(resolved)).toBe('or=(email.eq."nobody@x.com,id.gt.0")'); + }); + + it('should leave a plain substituted value unquoted', () => { + expect(utils.applyFilterStringParameters('id.eq.$1', [{ value: '42' }])).toBe('id.eq.42'); + }); + + it('should substitute multiple parameters by position', () => { + const resolved = utils.applyFilterStringParameters('or=(a.eq.$1,b.eq.$2)', [ + { value: 'x,y' }, + { value: 'z' }, + ]); + + expect(decodeURIComponent(resolved)).toBe('or=(a.eq."x,y",b.eq.z)'); + }); + + it('should throw when the filter string references a parameter that was not provided', () => { + expect(() => utils.applyFilterStringParameters('email.eq.$2', [{ value: 'x' }])).toThrow( + 'Filters (String) references parameter $2', + ); + }); + + it('should leave the filter string untouched when no parameters are provided, even with a literal $N in it', () => { + expect(utils.applyFilterStringParameters('price.eq.$50', [])).toBe('price.eq.$50'); + }); + + it('should escape & and = in a substituted value so it cannot open a new top-level query parameter', () => { + const resolved = utils.applyFilterStringParameters('or=(email.eq.$1)', [ + { value: 'x@y.com&select=*' }, + ]); + + expect(Array.from(new URLSearchParams(resolved).keys())).toEqual(['or']); + }); + + it.each(['getAll', 'delete', 'update'])( + 'should escape an injected parameter value for %s so it cannot append an extra condition', + async (operation) => { + const supabaseApiRequest = jest.spyOn(utils, 'supabaseApiRequest').mockResolvedValue([]); + + const fakeExecuteFunction = createMockExecuteFunction({ + resource: 'row', + operation, + returnAll: true, + filterType: 'string', + filterString: 'or=(email.eq.$1)', + filterStringParameters: { values: [{ value: 'nobody@x.com,id.gt.0' }] }, + tableId: 'my_table', + dataToSend: 'defineBelow', + fieldsUi: { fieldValues: [] }, + }); + + await node.execute.call(fakeExecuteFunction); + + // the substituted value is percent-encoded (decodeURI alone leaves + // reserved-character escapes like %40/%2C untouched); the receiving + // server decodes it the same way before parsing the filter. + const endpointArg = supabaseApiRequest.mock.calls[0][1] as string; + expect(decodeURIComponent(endpointArg)).toBe( + '/my_table?or=(email.eq."nobody@x.com,id.gt.0")', + ); + + supabaseApiRequest.mockRestore(); + }, + ); + + it.each(['getAll', 'delete', 'update'])( + "should not swallow a $N parameter mismatch under continueOnFail for %s, matching manual mode's unconditional validation errors", + async (operation) => { + const fakeExecuteFunction = createMockExecuteFunction( + { + resource: 'row', + operation, + returnAll: true, + filterType: 'string', + filterString: 'email.eq.$2', + filterStringParameters: { values: [{ value: 'x' }] }, + tableId: 'my_table', + dataToSend: 'defineBelow', + fieldsUi: { fieldValues: [] }, + }, + true, + ); + + await expect(node.execute.call(fakeExecuteFunction)).rejects.toThrow( + 'Filters (String) references parameter $2', + ); + }, + ); + }); + it('should allow filtering on the same field multiple times', async () => { const supabaseApiRequest = jest .spyOn(utils, 'supabaseApiRequest') @@ -285,6 +382,92 @@ describe('Test Supabase Node', () => { ); }); + describe('table name path segment', () => { + const encodedNames = [ + ['a/b', '/a%2Fb'], + ['a\\b', '/a%5Cb'], + ['a?b=c', '/a%3Fb%3Dc'], + ['a#b', '/a%23b'], + ['../a', '/..%2Fa'], + ['my table', '/my%20table'], + ]; + + it.each(encodedNames)( + 'should send the table name %s as a single path segment', + async (tableId, expectedEndpoint) => { + const supabaseApiRequest = jest + .spyOn(utils, 'supabaseApiRequest') + .mockResolvedValueOnce([]); + const fakeExecuteFunction = createMockExecuteFunction({ + resource: 'row', + operation: 'getAll', + returnAll: false, + limit: 50, + tableId, + filterType: 'none', + orderBy: '', + }); + + await node.execute.call(fakeExecuteFunction); + + expect(supabaseApiRequest).toHaveBeenCalledWith( + 'GET', + expectedEndpoint, + expect.anything(), + expect.anything(), + undefined, + expect.anything(), + ); + supabaseApiRequest.mockRestore(); + }, + ); + + const operations: Array<[string, IDataObject]> = [ + ['create', { dataToSend: 'defineBelow', fieldsUi: { fieldValues: [] } }], + ['delete', { filterType: 'none' }], + ['get', { filters: { conditions: [{ keyName: 'id', keyValue: '1' }] } }], + ['update', { filterType: 'none', dataToSend: 'defineBelow', fieldsUi: { fieldValues: [] } }], + ]; + + it.each(operations)( + 'should encode the table name on the %s operation', + async (operation, parameters) => { + const supabaseApiRequest = jest.spyOn(utils, 'supabaseApiRequest').mockResolvedValue([]); + const fakeExecuteFunction = createMockExecuteFunction({ + resource: 'row', + operation, + tableId: 'a/b', + ...parameters, + }); + + await node.execute.call(fakeExecuteFunction); + + expect(supabaseApiRequest.mock.calls[0][1]).toBe('/a%2Fb'); + supabaseApiRequest.mockRestore(); + }, + ); + + it.each(['', '.', '..'])( + 'should reject the table name "%s" before any request is made', + async (tableId) => { + const supabaseApiRequest = jest.spyOn(utils, 'supabaseApiRequest'); + const fakeExecuteFunction = createMockExecuteFunction({ + resource: 'row', + operation: 'getAll', + returnAll: false, + limit: 50, + tableId, + filterType: 'none', + orderBy: '', + }); + + await expect(node.execute.call(fakeExecuteFunction)).rejects.toThrow(UserError); + expect(supabaseApiRequest).not.toHaveBeenCalled(); + supabaseApiRequest.mockRestore(); + }, + ); + }); + describe('getSchemaHeader function', () => { const mockExecuteContext = { getNodeParameter: jest.fn(), diff --git a/packages/nodes-base/nodes/Webflow/V2/WebflowTriggerV2.node.ts b/packages/nodes-base/nodes/Webflow/V2/WebflowTriggerV2.node.ts index 30b7421ee80..2aaf537ab58 100644 --- a/packages/nodes-base/nodes/Webflow/V2/WebflowTriggerV2.node.ts +++ b/packages/nodes-base/nodes/Webflow/V2/WebflowTriggerV2.node.ts @@ -10,6 +10,7 @@ import type { import { NodeConnectionTypes } from 'n8n-workflow'; import { getSites, webflowApiRequest } from '../GenericFunctions'; +import { verifySignature } from '../WebflowTriggerHelpers'; export class WebflowTriggerV2 implements INodeType { description: INodeTypeDescription; @@ -154,6 +155,9 @@ export class WebflowTriggerV2 implements INodeType { let responseData; const webhookData = this.getWorkflowStaticData('node'); const siteId = this.getNodeParameter('site') as string; + if (!webhookData.webhookId) { + return true; + } const endpoint = `/sites/${siteId}/webhooks/${webhookData.webhookId}`; try { responseData = await webflowApiRequest.call(this, 'DELETE', endpoint); @@ -171,6 +175,12 @@ export class WebflowTriggerV2 implements INodeType { }; async webhook(this: IWebhookFunctions): Promise { + if (!(await verifySignature.call(this))) { + const res = this.getResponseObject(); + res.status(401).send('Unauthorized').end(); + return { noWebhookResponse: true }; + } + const req = this.getRequestObject(); return { workflowData: [this.helpers.returnJsonArray(req.body as IDataObject[])], diff --git a/packages/nodes-base/nodes/Webflow/WebflowTriggerHelpers.ts b/packages/nodes-base/nodes/Webflow/WebflowTriggerHelpers.ts new file mode 100644 index 00000000000..c74e5652131 --- /dev/null +++ b/packages/nodes-base/nodes/Webflow/WebflowTriggerHelpers.ts @@ -0,0 +1,56 @@ +import { createHmac, timingSafeEqual } from 'crypto'; +import type { IWebhookFunctions } from 'n8n-workflow'; + +const MAX_TIMESTAMP_AGE_MS = 300_000; + +export async function verifySignature(this: IWebhookFunctions): Promise { + let secret: string; + + try { + const credentials = await this.getCredentials('webflowOAuth2Api'); + if (typeof credentials.clientSecret !== 'string' || credentials.clientSecret === '') { + return false; + } + secret = credentials.clientSecret; + } catch { + return false; + } + + const req = this.getRequestObject(); + const timestamp = req.header('x-webflow-timestamp'); + const signature = req.header('x-webflow-signature'); + const rawBody = req.rawBody; + + if ( + typeof timestamp !== 'string' || + !/^\d+$/.test(timestamp) || + typeof signature !== 'string' || + (!Buffer.isBuffer(rawBody) && typeof rawBody !== 'string') + ) { + return false; + } + + const timestampMs = Number(timestamp); + if ( + !Number.isSafeInteger(timestampMs) || + Math.abs(Date.now() - timestampMs) > MAX_TIMESTAMP_AGE_MS + ) { + return false; + } + + try { + const hmac = createHmac('sha256', secret); + hmac.update(`${timestamp}:`); + hmac.update(rawBody); + + const expectedSignature = Buffer.from(hmac.digest('hex')); + const actualSignature = Buffer.from(signature); + + return ( + expectedSignature.length === actualSignature.length && + timingSafeEqual(expectedSignature, actualSignature) + ); + } catch { + return false; + } +} diff --git a/packages/nodes-base/nodes/Webflow/test/WebflowTrigger.test.ts b/packages/nodes-base/nodes/Webflow/test/WebflowTrigger.test.ts new file mode 100644 index 00000000000..236a02ff74d --- /dev/null +++ b/packages/nodes-base/nodes/Webflow/test/WebflowTrigger.test.ts @@ -0,0 +1,64 @@ +import type { IDataObject, INodeTypeBaseDescription, IWebhookFunctions } from 'n8n-workflow'; + +import { WebflowTriggerV2 } from '../V2/WebflowTriggerV2.node'; +import { verifySignature } from '../WebflowTriggerHelpers'; + +jest.mock('../GenericFunctions', () => ({ + getSites: jest.fn(), +})); + +jest.mock('../WebflowTriggerHelpers', () => ({ + verifySignature: jest.fn(), +})); + +const baseDescription: INodeTypeBaseDescription = { + displayName: 'Webflow Trigger', + name: 'webflowTrigger', + icon: 'file:webflow.svg', + group: ['trigger'], + description: 'Handle Webflow events via webhooks', + defaultVersion: 2, +}; + +describe('WebflowTriggerV2', () => { + const trigger = new WebflowTriggerV2(baseDescription); + let response: { status: jest.Mock; send: jest.Mock; end: jest.Mock }; + let context: IWebhookFunctions; + const body: IDataObject = { triggerType: 'form_submission' }; + + beforeEach(() => { + jest.clearAllMocks(); + response = { + status: jest.fn().mockReturnThis(), + send: jest.fn().mockReturnThis(), + end: jest.fn().mockReturnThis(), + }; + context = { + getRequestObject: jest.fn().mockReturnValue({ body }), + getResponseObject: jest.fn().mockReturnValue(response), + helpers: { + returnJsonArray: jest.fn().mockImplementation((data: IDataObject) => [data]), + }, + } as unknown as IWebhookFunctions; + }); + + it('rejects a delivery when signature verification fails', async () => { + jest.mocked(verifySignature).mockResolvedValue(false); + + const result = await trigger.webhook.call(context); + + expect(response.status).toHaveBeenCalledWith(401); + expect(response.send).toHaveBeenCalledWith('Unauthorized'); + expect(response.end).toHaveBeenCalled(); + expect(result).toEqual({ noWebhookResponse: true }); + }); + + it('starts the workflow when signature verification succeeds', async () => { + jest.mocked(verifySignature).mockResolvedValue(true); + + const result = await trigger.webhook.call(context); + + expect(result).toEqual({ workflowData: [[body]] }); + expect(response.status).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/nodes-base/nodes/Webflow/test/WebflowTriggerHelpers.test.ts b/packages/nodes-base/nodes/Webflow/test/WebflowTriggerHelpers.test.ts new file mode 100644 index 00000000000..51ea09210df --- /dev/null +++ b/packages/nodes-base/nodes/Webflow/test/WebflowTriggerHelpers.test.ts @@ -0,0 +1,110 @@ +import { createHmac } from 'crypto'; +import type { IWebhookFunctions } from 'n8n-workflow'; + +import { verifySignature } from '../WebflowTriggerHelpers'; + +describe('WebflowTriggerHelpers', () => { + const clientSecret = 'oauth-client-secret'; + const rawBody = Buffer.from('{"triggerType":"form_submission"}'); + + function createSignature(secret: string, timestamp: string, body: Buffer | string = rawBody) { + const hmac = createHmac('sha256', secret); + hmac.update(`${timestamp}:`); + hmac.update(body); + return hmac.digest('hex'); + } + + function createContext(options: { + credentialSecret?: string; + timestamp?: string | null; + signature?: string | null; + body?: Buffer | string; + }): IWebhookFunctions { + const headers: Record = { + 'x-webflow-timestamp': options.timestamp ?? null, + 'x-webflow-signature': options.signature ?? null, + }; + + return { + getCredentials: jest.fn().mockResolvedValue({ clientSecret: options.credentialSecret }), + getRequestObject: jest.fn().mockReturnValue({ + header: (name: string) => headers[name] ?? null, + rawBody: options.body ?? rawBody, + }), + } as unknown as IWebhookFunctions; + } + + it('accepts a valid OAuth delivery', async () => { + const timestamp = Date.now().toString(); + const context = createContext({ + credentialSecret: clientSecret, + timestamp, + signature: createSignature(clientSecret, timestamp), + }); + + await expect(verifySignature.call(context)).resolves.toBe(true); + }); + + it('accepts a valid delivery with a string raw body', async () => { + const timestamp = Date.now().toString(); + const body = rawBody.toString(); + const context = createContext({ + credentialSecret: clientSecret, + timestamp, + signature: createSignature(clientSecret, timestamp, body), + body, + }); + + await expect(verifySignature.call(context)).resolves.toBe(true); + }); + + it('rejects a delivery with a missing signature', async () => { + const context = createContext({ + credentialSecret: clientSecret, + timestamp: Date.now().toString(), + }); + + await expect(verifySignature.call(context)).resolves.toBe(false); + }); + + it('rejects a delivery with an invalid signature', async () => { + const context = createContext({ + credentialSecret: clientSecret, + timestamp: Date.now().toString(), + signature: 'invalid', + }); + + await expect(verifySignature.call(context)).resolves.toBe(false); + }); + + it('rejects a delivery with an expired timestamp', async () => { + const timestamp = (Date.now() - 301_000).toString(); + const context = createContext({ + credentialSecret: clientSecret, + timestamp, + signature: createSignature(clientSecret, timestamp), + }); + + await expect(verifySignature.call(context)).resolves.toBe(false); + }); + + it('rejects a delivery with a malformed timestamp', async () => { + const timestamp = `${Date.now()}invalid`; + const context = createContext({ + credentialSecret: clientSecret, + timestamp, + signature: createSignature(clientSecret, timestamp), + }); + + await expect(verifySignature.call(context)).resolves.toBe(false); + }); + + it('rejects an OAuth delivery when the client secret is unavailable', async () => { + const context = createContext({ + timestamp: Date.now().toString(), + signature: 'signature', + }); + + await expect(verifySignature.call(context)).resolves.toBe(false); + }); +}); diff --git a/packages/nodes-base/utils/url.test.ts b/packages/nodes-base/utils/url.test.ts deleted file mode 100644 index 83b8388d86a..00000000000 --- a/packages/nodes-base/utils/url.test.ts +++ /dev/null @@ -1,32 +0,0 @@ -import { UserError } from 'n8n-workflow'; - -import { toPathSegment } from './url'; - -describe('toPathSegment', () => { - it('should leave a plain id unchanged', () => { - expect(toPathSegment('my-index')).toBe('my-index'); - }); - - it('should encode an id so it stays a single path segment', () => { - expect(toPathSegment('a/b/c')).toBe('a%2Fb%2Fc'); - expect(toPathSegment('other-index/_doc/x')).toBe('other-index%2F_doc%2Fx'); - }); - - it.each([ - // A comma is percent-encoded, which Elasticsearch still reads as a multi-index - // separator, so `a,b` keeps resolving to both indices. - ['a,b', 'a%2Cb'], - // A wildcard is left alone, so index patterns keep working. - ['logs-*', 'logs-*'], - ])('should preserve multi-value and wildcard syntax (%s)', (id, expected) => { - expect(toPathSegment(id)).toBe(expected); - }); - - it.each(['', '.', '..'])('should reject the identifier "%s"', (id) => { - expect(() => toPathSegment(id)).toThrow(UserError); - }); - - it.each([null, undefined])('should reject the nullish identifier %s', (id) => { - expect(() => toPathSegment(id)).toThrow(UserError); - }); -}); diff --git a/packages/workflow/src/extensions/extended-functions.ts b/packages/workflow/src/extensions/extended-functions.ts index 37acd72d084..7459a5c6e7f 100644 --- a/packages/workflow/src/extensions/extended-functions.ts +++ b/packages/workflow/src/extensions/extended-functions.ts @@ -1,6 +1,7 @@ import { average as aAverage } from './array-extensions'; import { ExpressionExtensionError } from '../errors/expression-extension.error'; import { ExpressionError } from '../errors/expression.error'; +import { toPathSegment as toPathSegmentValue } from '../url'; // Define an own data field rather than assigning through an inherited setter. function defineField(target: Record, key: PropertyKey, value: unknown): void { @@ -72,6 +73,17 @@ function ifEmpty(value: V, defaultValue: T) { return value; } +function toPathSegment(value: unknown): string { + try { + return toPathSegmentValue(value); + } catch (error) { + if (error instanceof Error) { + throw new ExpressionError(error.message); + } + throw error; + } +} + ifEmpty.doc = { name: 'ifEmpty', description: @@ -91,6 +103,7 @@ export const extendedFunctions = { average, numberList, zip, + toPathSegment, $min: min, $max: max, $average: average, diff --git a/packages/workflow/src/index.ts b/packages/workflow/src/index.ts index 458144197b7..228f1249a6f 100644 --- a/packages/workflow/src/index.ts +++ b/packages/workflow/src/index.ts @@ -22,6 +22,7 @@ export * from './expressions/expression-helpers'; export * from './from-ai-parse-utils'; export * from './node-helpers'; export * from './tool-helpers'; +export * from './url'; export * from './node-reference-parser-utils'; export * from './metadata-utils'; export * from './workflow'; diff --git a/packages/nodes-base/utils/url.ts b/packages/workflow/src/url.ts similarity index 94% rename from packages/nodes-base/utils/url.ts rename to packages/workflow/src/url.ts index fcebcdf2c05..326dc2cf359 100644 --- a/packages/nodes-base/utils/url.ts +++ b/packages/workflow/src/url.ts @@ -1,4 +1,4 @@ -import { UserError } from 'n8n-workflow'; +import { UserError } from './errors'; /** * Encode an id as a single URL path segment. `.`, `..`, and the empty string diff --git a/packages/workflow/test/expression-compatibility.test.ts b/packages/workflow/test/expression-compatibility.test.ts index 9ca9c0274c1..b21479498fc 100644 --- a/packages/workflow/test/expression-compatibility.test.ts +++ b/packages/workflow/test/expression-compatibility.test.ts @@ -170,6 +170,7 @@ describe('Expression — compatibility corpus', () => { selectedCardId: 'c1', dimensions: ['2026-08-24 10:30:00', 'DE'], metrics: [10, 20], + pathId: 'a/b c', jsonString: '{"k":[1,2,3]}', key: 'dimensions', zero: 0, @@ -809,6 +810,7 @@ describe('Expression — compatibility corpus', () => { ['$prevNode.outputIndex', '={{ $prevNode.outputIndex }}', 0], ['$execution.id', '={{ $execution.id }}', 'exec-1'], ['$workflow.active', '={{ $workflow.active }}', false], + ['toPathSegment($json.pathId)', '={{ toPathSegment($json.pathId) }}', 'a%2Fb%20c'], ['`${$runIndex}:${$itemIndex}`', '={{ `${$runIndex}:${$itemIndex}` }}', '0:0'], ['$("Rates").first()?.json?.cardId', '={{ $("Rates").first()?.json?.cardId }}', 'c1'], [ @@ -862,6 +864,7 @@ describe('Expression — compatibility corpus', () => { ['"}}"', '={{ "}}" }}'], ['"{{" + "}}"', '={{ "{{" + "}}" }}'], ['literal {{ }} braces? {{ 1 + 1', '=literal {{ }} braces? {{ 1 + 1 }}'], + ['dot URL path segment', '={{ toPathSegment("..") }}'], ]; it.each(REJECTED)('rejects: %s', (_name, expr) => { diff --git a/packages/workflow/test/url.test.ts b/packages/workflow/test/url.test.ts new file mode 100644 index 00000000000..47b418076da --- /dev/null +++ b/packages/workflow/test/url.test.ts @@ -0,0 +1,18 @@ +import { toPathSegment, UserError } from '../src'; + +describe('toPathSegment', () => { + it.each([ + ['my-id', 'my-id'], + [123, '123'], + ['a/b c', 'a%2Fb%20c'], + ['a?b#c\\d', 'a%3Fb%23c%5Cd'], + ['already%2Fencoded', 'already%252Fencoded'], + ['café', 'caf%C3%A9'], + ])('encodes %s as one path segment', (input, expected) => { + expect(toPathSegment(input)).toBe(expected); + }); + + it.each(['', '.', '..', null, undefined])('rejects %s', (input) => { + expect(() => toPathSegment(input)).toThrow(UserError); + }); +});