diff --git a/packages/@n8n/backend-common/src/index.ts b/packages/@n8n/backend-common/src/index.ts index 6f3f9092af..8f26f40690 100644 --- a/packages/@n8n/backend-common/src/index.ts +++ b/packages/@n8n/backend-common/src/index.ts @@ -6,3 +6,4 @@ export { isObjectLiteral } from './utils/is-object-literal'; export { Logger } from './logging/logger'; export { ModuleRegistry } from './modules/module-registry'; export { ModulesConfig, ModuleName } from './modules/modules.config'; +export { isContainedWithin, safeJoinPath } from './utils/path-util'; diff --git a/packages/@n8n/backend-common/src/utils/__tests__/path-util.test.ts b/packages/@n8n/backend-common/src/utils/__tests__/path-util.test.ts new file mode 100644 index 0000000000..9eb11347a9 --- /dev/null +++ b/packages/@n8n/backend-common/src/utils/__tests__/path-util.test.ts @@ -0,0 +1,53 @@ +import { isContainedWithin, safeJoinPath } from '../path-util'; + +describe('isContainedWithin', () => { + it('should return true when parent and child paths are the same', () => { + expect(isContainedWithin('/some/parent/folder', '/some/parent/folder')).toBe(true); + }); + + test.each([ + ['/some/parent/folder', '/some/parent/folder/subfolder/file.txt'], + ['/some/parent/folder', '/some/parent/folder/../folder/subfolder/file.txt'], + ['/some/parent/folder/', '/some/parent/folder/subfolder/file.txt'], + ['/some/parent/folder', '/some/parent/folder/subfolder/'], + ])('should return true for parent %s and child %s', (parent, child) => { + expect(isContainedWithin(parent, child)).toBe(true); + }); + + test.each([ + ['/some/parent/folder', '/some/other/folder/file.txt'], + ['/some/parent/folder', '/some/parent/folder_but_not_really'], + ['/one/path', '/another/path'], + ])('should return false for parent %s and child %s', (parent, child) => { + expect(isContainedWithin(parent, child)).toBe(false); + }); +}); + +describe('safeJoinPath', () => { + it('should join valid paths successfully', () => { + expect(safeJoinPath('path', '')).toBe('path'); + expect(safeJoinPath('path', '.')).toBe('path'); + expect(safeJoinPath('path', '../path')).toBe('path'); + expect(safeJoinPath('path', 'foo')).toBe('path/foo'); + expect(safeJoinPath('path', 'foo/file.json')).toBe('path/foo/file.json'); + expect(safeJoinPath('path', './foo/file.json')).toBe('path/foo/file.json'); + expect(safeJoinPath('path', './foo/../file.json')).toBe('path/file.json'); + expect(safeJoinPath('/foo/bar', 'baz')).toBe('/foo/bar/baz'); + expect(safeJoinPath('/foo/bar/', 'baz')).toBe('/foo/bar/baz'); + expect(safeJoinPath('/foo', '')).toBe('/foo'); + expect(safeJoinPath('/foo', '.')).toBe('/foo'); + expect(safeJoinPath('/foo', 'bar//baz')).toBe('/foo/bar/baz'); + expect(safeJoinPath('/foo', 'bar/../baz')).toBe('/foo/baz'); + expect(safeJoinPath('/foo', '/bar/baz')).toBe('/foo/bar/baz'); + expect(safeJoinPath('/foo', '.././foo/bar')).toBe('/foo/bar'); + }); + + it('should throw an error for invalid paths', () => { + expect(() => safeJoinPath('path', '../outside/file.json')).toThrow('Path traversal detected'); + expect(() => safeJoinPath('path', './foo/../../file.json')).toThrow('Path traversal detected'); + expect(() => safeJoinPath('/foo/bar', '../../baz')).toThrow('Path traversal detected'); + expect(() => safeJoinPath('/foo/bar', '../baz')).toThrow('Path traversal detected'); + expect(() => safeJoinPath('path', '..')).toThrow('Path traversal detected'); + expect(() => safeJoinPath('/foo/bar', '..')).toThrow('Path traversal detected'); + }); +}); diff --git a/packages/@n8n/backend-common/src/utils/path-util.ts b/packages/@n8n/backend-common/src/utils/path-util.ts new file mode 100644 index 0000000000..f55a4ea170 --- /dev/null +++ b/packages/@n8n/backend-common/src/utils/path-util.ts @@ -0,0 +1,36 @@ +import { UnexpectedError } from 'n8n-workflow'; +import * as path from 'node:path'; + +/** + * Checks if the given childPath is contained within the parentPath. Resolves + * the paths before comparing them, so that relative paths are also supported. + */ +export function isContainedWithin(parentPath: string, childPath: string): boolean { + parentPath = path.resolve(parentPath); + childPath = path.resolve(childPath); + + if (parentPath === childPath) { + return true; + } + + return childPath.startsWith(parentPath + path.sep); +} + +/** + * Joins the given paths to the parentPath, ensuring that the resulting path + * is still contained within the parentPath. If not, it throws an error to + * prevent path traversal vulnerabilities. + * + * @throws {UnexpectedError} If the resulting path is not contained within the parentPath. + */ +export function safeJoinPath(parentPath: string, ...paths: string[]): string { + const candidate = path.join(parentPath, ...paths); + + if (!isContainedWithin(parentPath, candidate)) { + throw new UnexpectedError( + `Path traversal detected, refusing to join paths: ${parentPath} and ${JSON.stringify(paths)}`, + ); + } + + return candidate; +} diff --git a/packages/cli/src/environments.ee/source-control/source-control-helper.ee.ts b/packages/cli/src/environments.ee/source-control/source-control-helper.ee.ts index 8d95bb0512..4a35ba216a 100644 --- a/packages/cli/src/environments.ee/source-control/source-control-helper.ee.ts +++ b/packages/cli/src/environments.ee/source-control/source-control-helper.ee.ts @@ -1,5 +1,5 @@ import type { SourceControlledFile } from '@n8n/api-types'; -import { Logger } from '@n8n/backend-common'; +import { Logger, isContainedWithin, safeJoinPath } from '@n8n/backend-common'; import type { TagEntity, WorkflowTagMapping } from '@n8n/db'; import { Container } from '@n8n/di'; import { generateKeyPairSync } from 'crypto'; @@ -10,7 +10,6 @@ import { readFile as fsReadFile } from 'node:fs/promises'; import path from 'path'; import { License } from '@/license'; -import { isContainedWithin } from '@/utils/path-util'; import { SOURCE_CONTROL_FOLDERS_EXPORT_FILE, @@ -28,26 +27,26 @@ export function stringContainsExpression(testString: string): boolean { } export function getWorkflowExportPath(workflowId: string, workflowExportFolder: string): string { - return path.join(workflowExportFolder, `${workflowId}.json`); + return safeJoinPath(workflowExportFolder, `${workflowId}.json`); } export function getCredentialExportPath( credentialId: string, credentialExportFolder: string, ): string { - return path.join(credentialExportFolder, `${credentialId}.json`); + return safeJoinPath(credentialExportFolder, `${credentialId}.json`); } export function getVariablesPath(gitFolder: string): string { - return path.join(gitFolder, SOURCE_CONTROL_VARIABLES_EXPORT_FILE); + return safeJoinPath(gitFolder, SOURCE_CONTROL_VARIABLES_EXPORT_FILE); } export function getTagsPath(gitFolder: string): string { - return path.join(gitFolder, SOURCE_CONTROL_TAGS_EXPORT_FILE); + return safeJoinPath(gitFolder, SOURCE_CONTROL_TAGS_EXPORT_FILE); } export function getFoldersPath(gitFolder: string): string { - return path.join(gitFolder, SOURCE_CONTROL_FOLDERS_EXPORT_FILE); + return safeJoinPath(gitFolder, SOURCE_CONTROL_FOLDERS_EXPORT_FILE); } export async function readTagAndMappingsFromSourceControlFile(file: string): Promise<{ @@ -232,7 +231,9 @@ export function normalizeAndValidateSourceControlledFilePath( ) { ok(path.isAbsolute(gitFolderPath), 'gitFolder must be an absolute path'); - const normalizedPath = path.isAbsolute(filePath) ? filePath : path.join(gitFolderPath, filePath); + const normalizedPath = path.isAbsolute(filePath) + ? filePath + : safeJoinPath(gitFolderPath, filePath); if (!isContainedWithin(gitFolderPath, filePath)) { throw new UserError(`File path ${filePath} is invalid`); diff --git a/packages/cli/src/load-nodes-and-credentials.ts b/packages/cli/src/load-nodes-and-credentials.ts index a3f5cc8454..2985df30b6 100644 --- a/packages/cli/src/load-nodes-and-credentials.ts +++ b/packages/cli/src/load-nodes-and-credentials.ts @@ -1,4 +1,4 @@ -import { inTest, Logger } from '@n8n/backend-common'; +import { inTest, isContainedWithin, Logger } from '@n8n/backend-common'; import { GlobalConfig } from '@n8n/config'; import { Container, Service } from '@n8n/di'; import glob from 'fast-glob'; @@ -31,7 +31,6 @@ import path from 'path'; import picocolors from 'picocolors'; import { CUSTOM_API_CALL_KEY, CUSTOM_API_CALL_NAME, CLI_DIR, inE2ETests } from '@/constants'; -import { isContainedWithin } from '@/utils/path-util'; @Service() export class LoadNodesAndCredentials { diff --git a/packages/cli/src/utils/__tests__/path-util.test.ts b/packages/cli/src/utils/__tests__/path-util.test.ts deleted file mode 100644 index d67e97f9e0..0000000000 --- a/packages/cli/src/utils/__tests__/path-util.test.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { isContainedWithin } from '../path-util'; - -describe('isContainedWithin', () => { - it('should return true when parent and child paths are the same', () => { - expect(isContainedWithin('/some/parent/folder', '/some/parent/folder')).toBe(true); - }); - - test.each([ - ['/some/parent/folder', '/some/parent/folder/subfolder/file.txt'], - ['/some/parent/folder', '/some/parent/folder/../folder/subfolder/file.txt'], - ['/some/parent/folder/', '/some/parent/folder/subfolder/file.txt'], - ['/some/parent/folder', '/some/parent/folder/subfolder/'], - ])('should return true for parent %s and child %s', (parent, child) => { - expect(isContainedWithin(parent, child)).toBe(true); - }); - - test.each([ - ['/some/parent/folder', '/some/other/folder/file.txt'], - ['/some/parent/folder', '/some/parent/folder_but_not_really'], - ['/one/path', '/another/path'], - ])('should return false for parent %s and child %s', (parent, child) => { - expect(isContainedWithin(parent, child)).toBe(false); - }); -}); diff --git a/packages/cli/src/utils/path-util.ts b/packages/cli/src/utils/path-util.ts deleted file mode 100644 index f42dc01890..0000000000 --- a/packages/cli/src/utils/path-util.ts +++ /dev/null @@ -1,16 +0,0 @@ -import * as path from 'node:path'; - -/** - * Checks if the given childPath is contained within the parentPath. Resolves - * the paths before comparing them, so that relative paths are also supported. - */ -export function isContainedWithin(parentPath: string, childPath: string): boolean { - parentPath = path.resolve(parentPath); - childPath = path.resolve(childPath); - - if (parentPath === childPath) { - return true; - } - - return childPath.startsWith(parentPath + path.sep); -} diff --git a/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts b/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts index 23153bb94d..175f780a41 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts @@ -1,9 +1,10 @@ +import { safeJoinPath } from '@n8n/backend-common'; import { Container } from '@n8n/di'; import type { FileSystemHelperFunctions, INode } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; import { createReadStream } from 'node:fs'; import { access as fsAccess, writeFile as fsWriteFile } from 'node:fs/promises'; -import { join, resolve } from 'node:path'; +import { resolve } from 'node:path'; import { BINARY_DATA_STORAGE_PATH, @@ -107,7 +108,7 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct }, getStoragePath() { - return join(Container.get(InstanceSettings).n8nFolder, `storage/${node.type}`); + return safeJoinPath(Container.get(InstanceSettings).n8nFolder, `storage/${node.type}`); }, async writeContentToFile(filePath, content, flag) { diff --git a/packages/core/src/nodes-loader/__tests__/directory-loader.test.ts b/packages/core/src/nodes-loader/__tests__/directory-loader.test.ts index c558952394..796e451118 100644 --- a/packages/core/src/nodes-loader/__tests__/directory-loader.test.ts +++ b/packages/core/src/nodes-loader/__tests__/directory-loader.test.ts @@ -1,5 +1,6 @@ import { mock } from 'jest-mock-extended'; import type { + IconFile, ICredentialType, INodeType, INodeTypeDescription, @@ -133,6 +134,20 @@ describe('DirectoryLoader', () => { expect(mockNode2.description.iconUrl).toBe('icons/n8n-nodes-testing/dist/Node2/node2.svg'); }); + it('should throw error if node has icon not contained within the package directory', async () => { + mockFs.readFileSync.calledWith(`${directory}/package.json`).mockReturnValue(packageJson); + mockNode2.description.icon = { + light: 'file:../../../../../../evil' as IconFile, + dark: 'file:dark.svg', + }; + + const loader = new PackageDirectoryLoader(directory); + + await expect(loader.loadAll()).rejects.toThrow( + 'Icon path "../../../../evil" is not contained within', + ); + }); + it('should throw error when package.json is missing', async () => { mockFs.readFileSync.mockImplementationOnce(() => { throw new Error('ENOENT'); @@ -673,6 +688,23 @@ describe('DirectoryLoader', () => { expect(nodeWithIcon.description.icon).toBeUndefined(); }); + it('should error if icon path is not contained within the package directory', () => { + const loader = new CustomDirectoryLoader(directory); + const filePath = 'dist/Node1/Node1.node.js'; + + const nodeWithIcon = createNode('nodeWithIcon'); + nodeWithIcon.description.icon = { + light: 'file:../../../evil' as IconFile, + dark: 'file:dark.svg', + }; + + jest.spyOn(classLoader, 'loadClassInIsolation').mockReturnValueOnce(nodeWithIcon); + + expect(() => loader.loadNodeFromFile(filePath)).toThrow( + 'Icon path "../evil" is not contained within', + ); + }); + it('should skip node if not in includeNodes', () => { const loader = new CustomDirectoryLoader(directory, [], ['CUSTOM.other']); const filePath = 'dist/Node1/Node1.node.js'; diff --git a/packages/core/src/nodes-loader/directory-loader.ts b/packages/core/src/nodes-loader/directory-loader.ts index 7088933a10..45ec4d8a7e 100644 --- a/packages/core/src/nodes-loader/directory-loader.ts +++ b/packages/core/src/nodes-loader/directory-loader.ts @@ -1,4 +1,4 @@ -import { Logger } from '@n8n/backend-common'; +import { isContainedWithin, Logger } from '@n8n/backend-common'; import { Container } from '@n8n/di'; import uniqBy from 'lodash/uniqBy'; import type { @@ -16,7 +16,7 @@ import type { IVersionedNodeType, KnownNodesAndCredentials, } from 'n8n-workflow'; -import { ApplicationError, isSubNodeType } from 'n8n-workflow'; +import { ApplicationError, isSubNodeType, UnexpectedError } from 'n8n-workflow'; import { realpathSync } from 'node:fs'; import * as path from 'path'; @@ -383,6 +383,13 @@ export abstract class DirectoryLoader { private getIconPath(icon: string, filePath: string) { const iconPath = path.join(path.dirname(filePath), icon.replace('file:', '')); + + if (!isContainedWithin(this.directory, path.join(this.directory, iconPath))) { + throw new UnexpectedError( + `Icon path "${iconPath}" is not contained within the package directory "${this.directory}"`, + ); + } + return `icons/${this.packageName}/${iconPath}`; }