fix(core): Only resolve the filepath once

packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts

@@ -1,7 +1,7 @@
 import { SecurityConfig } from '@n8n/config';
 import { Container } from '@n8n/di';
 import type { INode } from 'n8n-workflow';
-import { createReadStream } from 'node:fs';
+import { constants, createReadStream } from 'node:fs';
 import { access as fsAccess, realpath as fsRealpath } from 'node:fs/promises';
 import { join } from 'node:path';
 
@@ -15,7 +15,7 @@ import {
 } from '@/constants';
 import { InstanceSettings } from '@/instance-settings';
 
-import { getFileSystemHelperFunctions, isFilePathBlocked } from '../file-system-helper-functions';
+import { getFileSystemHelperFunctions } from '../file-system-helper-functions';
 
 jest.mock('node:fs');
 jest.mock('node:fs/promises');
@@ -39,78 +39,80 @@ beforeEach(() => {
 });
 
 describe('isFilePathBlocked', () => {
+	const node = { type: 'TestNode' } as INode;
+	const { isFilePathBlocked, resolvePath } = getFileSystemHelperFunctions(node);
 	beforeEach(() => {
 		process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true';
 	});
 
 	it('should return true for static cache dir', async () => {
 		const filePath = instanceSettings.staticCacheDir;
-		expect(await isFilePathBlocked(filePath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(filePath))).toBe(true);
 	});
 
 	it('should return true for restricted paths', async () => {
 		const restrictedPath = instanceSettings.n8nFolder;
-		expect(await isFilePathBlocked(restrictedPath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(restrictedPath))).toBe(true);
 	});
 
 	it('should handle empty allowed paths', async () => {
 		securityConfig.restrictFileAccessTo = '';
-		const result = await isFilePathBlocked('/some/random/path');
+		const result = isFilePathBlocked(await resolvePath('/some/random/path'));
 		expect(result).toBe(false);
 	});
 
 	it('should handle multiple allowed paths', async () => {
 		securityConfig.restrictFileAccessTo = '/path1;/path2;/path3';
 		const allowedPath = '/path2/somefile';
-		expect(await isFilePathBlocked(allowedPath)).toBe(false);
+		expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(false);
 	});
 
 	it('should handle empty strings in allowed paths', async () => {
 		securityConfig.restrictFileAccessTo = '/path1;;/path2';
 		const allowedPath = '/path2/somefile';
-		expect(await isFilePathBlocked(allowedPath)).toBe(false);
+		expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(false);
 	});
 
 	it('should trim whitespace in allowed paths', async () => {
 		securityConfig.restrictFileAccessTo = ' /path1 ; /path2 ; /path3 ';
 		const allowedPath = '/path2/somefile';
-		expect(await isFilePathBlocked(allowedPath)).toBe(false);
+		expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(false);
 	});
 
 	it('should return false when BLOCK_FILE_ACCESS_TO_N8N_FILES is false', async () => {
 		process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'false';
 		const restrictedPath = instanceSettings.n8nFolder;
-		expect(await isFilePathBlocked(restrictedPath)).toBe(false);
+		expect(isFilePathBlocked(await resolvePath(restrictedPath))).toBe(false);
 	});
 
 	it('should return true when path is in allowed paths but still restricted', async () => {
 		securityConfig.restrictFileAccessTo = '/some/allowed/path';
 		const restrictedPath = instanceSettings.n8nFolder;
-		expect(await isFilePathBlocked(restrictedPath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(restrictedPath))).toBe(true);
 	});
 
 	it('should return false when path is in allowed paths', async () => {
 		const allowedPath = '/some/allowed/path';
 		securityConfig.restrictFileAccessTo = allowedPath;
-		expect(await isFilePathBlocked(allowedPath)).toBe(false);
+		expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(false);
 	});
 
 	it('should return true when file paths in CONFIG_FILES', async () => {
 		process.env[CONFIG_FILES] = '/path/to/config1,/path/to/config2';
 		const configPath = '/path/to/config1/somefile';
-		expect(await isFilePathBlocked(configPath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(configPath))).toBe(true);
 	});
 
 	it('should return true when file paths in CUSTOM_EXTENSION_ENV', async () => {
 		process.env[CUSTOM_EXTENSION_ENV] = '/path/to/extensions1;/path/to/extensions2';
 		const extensionPath = '/path/to/extensions1/somefile';
-		expect(await isFilePathBlocked(extensionPath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(extensionPath))).toBe(true);
 	});
 
 	it('should return true when file paths in BINARY_DATA_STORAGE_PATH', async () => {
 		process.env[BINARY_DATA_STORAGE_PATH] = '/path/to/binary/storage';
 		const binaryPath = '/path/to/binary/storage/somefile';
-		expect(await isFilePathBlocked(binaryPath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(binaryPath))).toBe(true);
 	});
 
 	it('should block file paths in email template paths', async () => {
@@ -120,8 +122,8 @@ describe('isFilePathBlocked', () => {
 		const invitePath = '/path/to/invite/templates/invite.html';
 		const pwResetPath = '/path/to/pwreset/templates/reset.html';
 
-		expect(await isFilePathBlocked(invitePath)).toBe(true);
-		expect(await isFilePathBlocked(pwResetPath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(invitePath))).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(pwResetPath))).toBe(true);
 	});
 
 	it('should block access to n8n files if restrict and block are set', async () => {
@@ -131,7 +133,7 @@ describe('isFilePathBlocked', () => {
 		securityConfig.restrictFileAccessTo = userHome;
 		process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true';
 		const restrictedPath = instanceSettings.n8nFolder;
-		expect(await isFilePathBlocked(restrictedPath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(restrictedPath))).toBe(true);
 	});
 
 	it('should allow access to parent folder if restrict and block are set', async () => {
@@ -140,8 +142,8 @@ describe('isFilePathBlocked', () => {
 
 		securityConfig.restrictFileAccessTo = userHome;
 		process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true';
-		const restrictedPath = join(userHome, 'somefile.txt');
-		expect(await isFilePathBlocked(restrictedPath)).toBe(false);
+		const restrictedPath = await resolvePath(join(userHome, 'somefile.txt'));
+		expect(isFilePathBlocked(restrictedPath)).toBe(false);
 	});
 
 	it('should not block similar paths', async () => {
@@ -150,8 +152,8 @@ describe('isFilePathBlocked', () => {
 
 		securityConfig.restrictFileAccessTo = userHome;
 		process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] = 'true';
-		const restrictedPath = join(userHome, '.n8n_x');
-		expect(await isFilePathBlocked(restrictedPath)).toBe(false);
+		const restrictedPath = await resolvePath(join(userHome, '.n8n_x'));
+		expect(isFilePathBlocked(restrictedPath)).toBe(false);
 	});
 
 	it('should return true for a symlink in a allowed path to a restricted path', async () => {
@@ -161,7 +163,7 @@ describe('isFilePathBlocked', () => {
 		(fsRealpath as jest.Mock).mockImplementation((path: string) =>
 			path === allowedPath ? actualPath : path,
 		);
-		expect(await isFilePathBlocked(allowedPath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(allowedPath))).toBe(true);
 	});
 
 	it('should handle non-existent file when it is allowed', async () => {
@@ -170,7 +172,7 @@ describe('isFilePathBlocked', () => {
 		// @ts-expect-error undefined property
 		error.code = 'ENOENT';
 		(fsRealpath as jest.Mock).mockRejectedValueOnce(error);
-		expect(await isFilePathBlocked(filePath)).toBe(false);
+		expect(isFilePathBlocked(await resolvePath(filePath))).toBe(false);
 	});
 
 	it('should handle non-existent file when it is not allowed', async () => {
@@ -181,7 +183,7 @@ describe('isFilePathBlocked', () => {
 		// @ts-expect-error undefined property
 		error.code = 'ENOENT';
 		(fsRealpath as jest.Mock).mockRejectedValueOnce(error);
-		expect(await isFilePathBlocked(filePath)).toBe(true);
+		expect(isFilePathBlocked(await resolvePath(filePath))).toBe(true);
 	});
 });
 
@@ -213,17 +215,17 @@ describe('getFileSystemHelperFunctions', () => {
 			error.code = 'ENOENT';
 			(fsAccess as jest.Mock).mockRejectedValueOnce(error);
 
-			await expect(helperFunctions.createReadStream(filePath)).rejects.toThrow(
-				`The file "${filePath}" could not be accessed.`,
-			);
+			await expect(
+				helperFunctions.createReadStream(await helperFunctions.resolvePath(filePath)),
+			).rejects.toThrow(`The file "${filePath}" could not be accessed.`);
 		});
 
 		it('should throw when file access is blocked', async () => {
 			securityConfig.restrictFileAccessTo = '/allowed/path';
 			(fsAccess as jest.Mock).mockResolvedValueOnce({});
-			await expect(helperFunctions.createReadStream('/blocked/path')).rejects.toThrow(
-				'Access to the file is not allowed',
-			);
+			await expect(
+				helperFunctions.createReadStream(await helperFunctions.resolvePath('/blocked/path')),
+			).rejects.toThrow('Access to the file is not allowed');
 		});
 
 		it('should not reveal if file exists if it is within restricted path', async () => {
@@ -234,16 +236,63 @@ describe('getFileSystemHelperFunctions', () => {
 			error.code = 'ENOENT';
 			(fsAccess as jest.Mock).mockRejectedValueOnce(error);
 
-			await expect(helperFunctions.createReadStream('/blocked/path')).rejects.toThrow(
-				'Access to the file is not allowed',
-			);
+			await expect(
+				helperFunctions.createReadStream(await helperFunctions.resolvePath('/blocked/path')),
+			).rejects.toThrow('Access to the file is not allowed');
 		});
 
 		it('should create a read stream if file access is permitted', async () => {
 			const filePath = '/allowed/path';
 			(fsAccess as jest.Mock).mockResolvedValueOnce({});
-			await helperFunctions.createReadStream(filePath);
-			expect(createReadStream).toHaveBeenCalledWith(filePath);
+
+			// Mock createReadStream to return a proper stream-like object
+			const mockStream: { once: jest.Mock } = {
+				once: jest.fn((event: string, callback: (error?: Error) => void): typeof mockStream => {
+					if (event === 'open') {
+						// Immediately call the open callback
+						setImmediate(() => callback());
+					}
+					return mockStream;
+				}),
+			};
+			(createReadStream as jest.Mock).mockReturnValueOnce(mockStream);
+
+			await helperFunctions.createReadStream(await helperFunctions.resolvePath(filePath));
+			expect(createReadStream).toHaveBeenCalledWith(
+				filePath,
+				expect.objectContaining({
+					flags: expect.any(Number),
+				}),
+			);
+		});
+
+		it('should reject symlinks with O_NOFOLLOW to prevent TOCTOU attacks', async () => {
+			const filePath = '/allowed/path/file';
+
+			// Clear previous mocks and set up fresh mocks
+			(fsAccess as jest.Mock).mockReset();
+			(fsAccess as jest.Mock).mockResolvedValue(undefined);
+
+			// Simulate the ELOOP error that occurs when O_NOFOLLOW encounters a symlink
+			const eloopError = new Error('ELOOP: too many symbolic links encountered');
+			// @ts-expect-error undefined property
+			eloopError.code = 'ELOOP';
+
+			// Mock createReadStream to return a stream that emits an error event
+			const mockStream: { once: jest.Mock } = {
+				once: jest.fn((event: string, callback: (error?: Error) => void): typeof mockStream => {
+					if (event === 'error') {
+						// Emit the error asynchronously
+						setImmediate(() => callback(eloopError));
+					}
+					return mockStream;
+				}),
+			};
+			(createReadStream as jest.Mock).mockReturnValueOnce(mockStream);
+
+			await expect(
+				helperFunctions.createReadStream(await helperFunctions.resolvePath(filePath)),
+			).rejects.toThrow('ELOOP: too many symbolic links encountered');
 		});
 	});
 
@@ -253,9 +302,9 @@ describe('getFileSystemHelperFunctions', () => {
 
 			await expect(
 				helperFunctions.writeContentToFile(
-					instanceSettings.n8nFolder + '/test.txt',
+					await helperFunctions.resolvePath(instanceSettings.n8nFolder + '/test.txt'),
 					'content',
-					'w',
+					constants.O_WRONLY | constants.O_CREAT | constants.O_TRUNC,
 				),
 			).rejects.toThrow('not writable');
 		});