fix(core): Make our file reads more robust (no-changelog) (#17162)

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';

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');
+	});
+});

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;
+}

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`);

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 {

packages/cli/src/utils/__tests__/path-util.test.ts

@@ -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);
-	});
-});

packages/cli/src/utils/path-util.ts

@@ -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);
-}

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) {

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';

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}`;
 	}