From c3f6350602a8f7124f575add37a3f3ac6ad36aab Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Tue, 2 Dec 2025 12:32:06 +0100 Subject: [PATCH 01/10] only resolve the filepath once --- .../file-system-helper-functions.test.ts | 99 ++++++++++++------- .../utils/file-system-helper-functions.ts | 67 +++++++++---- .../ReadWriteFile/actions/read.operation.ts | 4 +- .../ReadWriteFile/actions/write.operation.ts | 6 +- packages/nodes-base/nodes/Git/Git.node.ts | 9 +- .../nodes/Git/__test__/Git.node.test.ts | 33 ++++++- .../nodes/Git/test/Git.node.test.ts | 2 + .../ReadBinaryFile/ReadBinaryFile.node.ts | 4 +- .../ReadBinaryFiles/ReadBinaryFiles.node.ts | 2 +- .../WriteBinaryFile/WriteBinaryFile.node.ts | 6 +- packages/workflow/src/interfaces.ts | 13 ++- 11 files changed, 177 insertions(+), 68 deletions(-) diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts index 6710ff04fdfb0..c26098055f713 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts @@ -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,43 @@ 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); + 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 throw ELOOP synchronously (which gets caught by try-catch) + (createReadStream as jest.Mock).mockImplementationOnce(() => { + throw eloopError; + }); + + await expect( + helperFunctions.createReadStream(await helperFunctions.resolvePath(filePath)), + ).rejects.toThrow('could not be opened'); }); }); @@ -253,7 +282,7 @@ describe('getFileSystemHelperFunctions', () => { await expect( helperFunctions.writeContentToFile( - instanceSettings.n8nFolder + '/test.txt', + await helperFunctions.resolvePath(instanceSettings.n8nFolder + '/test.txt'), 'content', 'w', ), 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 24c0c27360d59..843638df6f409 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 @@ -3,7 +3,9 @@ import { SecurityConfig } from '@n8n/config'; import { Container } from '@n8n/di'; import type { FileSystemHelperFunctions, INode } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; -import { createReadStream } from 'node:fs'; +import type { PathLike } from 'node:fs'; +import { constants, createReadStream } from 'node:fs'; +import type { ResolvedFilePath } from 'n8n-workflow'; import { access as fsAccess, writeFile as fsWriteFile, @@ -35,18 +37,19 @@ const getAllowedPaths = () => { return allowedPaths; }; -export async function isFilePathBlocked(filePath: string): Promise { - const allowedPaths = getAllowedPaths(); - let resolvedFilePath = ''; +const resolvePath = async (path: PathLike): Promise => { try { - resolvedFilePath = await fsRealpath(filePath); + return (await fsRealpath(path)) as ResolvedFilePath; // apply brand, since we know it's resolved now } catch (error: unknown) { if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { - resolvedFilePath = resolve(filePath); - } else { - throw error; + return resolve(path.toString()) as ResolvedFilePath; // apply brand, since we know it's resolved now } + throw error; } +}; + +function isFilePathBlocked(resolvedFilePath: ResolvedFilePath): boolean { + const allowedPaths = getAllowedPaths(); const blockFileAccessToN8nFiles = process.env[BLOCK_FILE_ACCESS_TO_N8N_FILES] !== 'false'; const restrictedPaths = blockFileAccessToN8nFiles ? getN8nRestrictedPaths() : []; @@ -64,8 +67,8 @@ export async function isFilePathBlocked(filePath: string): Promise { } export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunctions => ({ - async createReadStream(filePath) { - if (await isFilePathBlocked(filePath.toString())) { + async createReadStream(resolvedFilePath) { + if (isFilePathBlocked(resolvedFilePath)) { const allowedPaths = getAllowedPaths(); const message = allowedPaths.length ? ` Allowed paths: ${allowedPaths.join(', ')}` : ''; throw new NodeOperationError(node, `Access to the file is not allowed.${message}`, { @@ -74,34 +77,58 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct } try { - await fsAccess(filePath); + await fsAccess(resolvedFilePath); } catch (error) { // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access throw error.code === 'ENOENT' ? // eslint-disable-next-line @typescript-eslint/no-unsafe-argument new NodeOperationError(node, error, { - message: `The file "${String(filePath)}" could not be accessed.`, + message: `The file "${String(resolvedFilePath)}" could not be accessed.`, level: 'warning', }) : error; } - return createReadStream(filePath); + // Use O_NOFOLLOW to prevent TOCTOU race conditions where the file could be + // swapped to a symlink after validation but before opening + try { + return createReadStream(resolvedFilePath, { + // TypeScript types expect flags to be a string, but Node.js accepts numbers + // We use numeric flags here to combine O_RDONLY and O_NOFOLLOW bitwise + flags: (constants.O_RDONLY | constants.O_NOFOLLOW) as unknown as string, + }); + } catch (error) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (error.code === 'ELOOP') { + throw new NodeOperationError( + node, + `The file "${String(resolvedFilePath)}" could not be opened.`, + { + level: 'warning', + }, + ); + } + throw error; + } }, getStoragePath() { return safeJoinPath(Container.get(InstanceSettings).n8nFolder, `storage/${node.type}`); }, - async writeContentToFile(filePath, content, flag) { - if (await isFilePathBlocked(filePath as string)) { - throw new NodeOperationError(node, `The file "${String(filePath)}" is not writable.`, { - level: 'warning', - }); + async writeContentToFile(resolvedFilePath, content, flag) { + if (isFilePathBlocked(resolvedFilePath)) { + throw new NodeOperationError( + node, + `The file "${String(resolvedFilePath)}" is not writable.`, + { + level: 'warning', + }, + ); } - return await fsWriteFile(filePath, content, { encoding: 'binary', flag }); + return await fsWriteFile(resolvedFilePath, content, { encoding: 'binary', flag }); }, - + resolvePath, isFilePathBlocked, }); diff --git a/packages/nodes-base/nodes/Files/ReadWriteFile/actions/read.operation.ts b/packages/nodes-base/nodes/Files/ReadWriteFile/actions/read.operation.ts index 89396660e4f1e..0a30940fef8c1 100644 --- a/packages/nodes-base/nodes/Files/ReadWriteFile/actions/read.operation.ts +++ b/packages/nodes-base/nodes/Files/ReadWriteFile/actions/read.operation.ts @@ -101,7 +101,9 @@ export async function execute(this: IExecuteFunctions, items: INodeExecutionData const newItems: INodeExecutionData[] = []; for (const filePath of files) { - const stream = await this.helpers.createReadStream(filePath); + const stream = await this.helpers.createReadStream( + await this.helpers.resolvePath(filePath), + ); const binaryData = await this.helpers.prepareBinaryData(stream, filePath); if (options.fileName !== undefined) { diff --git a/packages/nodes-base/nodes/Files/ReadWriteFile/actions/write.operation.ts b/packages/nodes-base/nodes/Files/ReadWriteFile/actions/write.operation.ts index 5f4d28775a6f3..eb3d58dfde7cc 100644 --- a/packages/nodes-base/nodes/Files/ReadWriteFile/actions/write.operation.ts +++ b/packages/nodes-base/nodes/Files/ReadWriteFile/actions/write.operation.ts @@ -90,7 +90,11 @@ export async function execute(this: IExecuteFunctions, items: INodeExecutionData } // Write the file to disk - await this.helpers.writeContentToFile(fileName, fileContent, flag); + await this.helpers.writeContentToFile( + await this.helpers.resolvePath(fileName), + fileContent, + flag, + ); if (item.binary !== undefined) { // Create a shallow copy of the binary data so that the old diff --git a/packages/nodes-base/nodes/Git/Git.node.ts b/packages/nodes-base/nodes/Git/Git.node.ts index ac722bc0b8dda..03d4f177e1039 100644 --- a/packages/nodes-base/nodes/Git/Git.node.ts +++ b/packages/nodes-base/nodes/Git/Git.node.ts @@ -287,7 +287,8 @@ export class Git implements INodeType { for (let itemIndex = 0; itemIndex < items.length; itemIndex++) { try { const repositoryPath = this.getNodeParameter('repositoryPath', itemIndex, '') as string; - const isFilePathBlocked = await this.helpers.isFilePathBlocked(repositoryPath); + const resolvedRepositoryPath = await this.helpers.resolvePath(repositoryPath); + const isFilePathBlocked = this.helpers.isFilePathBlocked(resolvedRepositoryPath); if (isFilePathBlocked) { throw new NodeOperationError( this.getNode(), @@ -300,9 +301,9 @@ export class Git implements INodeType { if (operation === 'clone') { // Create repository folder if it does not exist try { - await access(repositoryPath); + await access(resolvedRepositoryPath); } catch (error) { - await mkdir(repositoryPath); + await mkdir(resolvedRepositoryPath); } } @@ -321,7 +322,7 @@ export class Git implements INodeType { } const gitOptions: Partial = { - baseDir: repositoryPath, + baseDir: resolvedRepositoryPath, config: gitConfig, }; 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 5256b92e8729a..4b30fdb0922b9 100644 --- a/packages/nodes-base/nodes/Git/__test__/Git.node.test.ts +++ b/packages/nodes-base/nodes/Git/__test__/Git.node.test.ts @@ -145,11 +145,42 @@ describe('Git Node', () => { describe('Restricted file paths', () => { it('should throw an error if the repository path is blocked', async () => { - (executeFunctions.helpers.isFilePathBlocked as jest.Mock).mockResolvedValue(true); + (executeFunctions.helpers.isFilePathBlocked as jest.Mock).mockReturnValue(true); + (executeFunctions.helpers.resolvePath as jest.Mock).mockResolvedValue('/tmp/test-repo'); await expect(gitNode.execute.call(executeFunctions)).rejects.toThrow( 'Access to the repository path is not allowed', ); }); + + it('should use the resolved repository path for git operations', async () => { + const originalPath = '/tmp/link-to-repo'; + const resolvedPath = '/tmp/actual-repo'; + + executeFunctions.getNodeParameter.mockImplementation((name: string) => { + switch (name) { + case 'operation': + return 'log'; + case 'repositoryPath': + return originalPath; + case 'options': + return {}; + default: + return ''; + } + }); + + (executeFunctions.helpers.resolvePath as jest.Mock).mockResolvedValue(resolvedPath); + (executeFunctions.helpers.isFilePathBlocked as jest.Mock).mockReturnValue(false); + + await gitNode.execute.call(executeFunctions); + + // Verify git is initialized with the resolved path, not the original + expect(mockSimpleGit).toHaveBeenCalledWith( + expect.objectContaining({ + baseDir: resolvedPath, + }), + ); + }); }); }); 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 568511249eb54..7d6006c5ee481 100644 --- a/packages/nodes-base/nodes/Git/test/Git.node.test.ts +++ b/packages/nodes-base/nodes/Git/test/Git.node.test.ts @@ -50,6 +50,8 @@ describe('Git Node', () => { continueOnFail: jest.fn(() => false), helpers: { returnJsonArray: jest.fn((data: any[]) => data.map((item: any) => ({ json: item }))), + resolvePath: jest.fn(async (path: string) => path as any), + isFilePathBlocked: jest.fn(() => false), }, }); jest.clearAllMocks(); diff --git a/packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts b/packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts index 7944e5194b076..483a653659878 100644 --- a/packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts +++ b/packages/nodes-base/nodes/ReadBinaryFile/ReadBinaryFile.node.ts @@ -69,7 +69,9 @@ export class ReadBinaryFile implements INodeType { const filePath = this.getNodeParameter('filePath', itemIndex); - const stream = await this.helpers.createReadStream(filePath); + const stream = await this.helpers.createReadStream( + await this.helpers.resolvePath(filePath), + ); const dataPropertyName = this.getNodeParameter('dataPropertyName', itemIndex); newItem.binary![dataPropertyName] = await this.helpers.prepareBinaryData(stream, filePath); diff --git a/packages/nodes-base/nodes/ReadBinaryFiles/ReadBinaryFiles.node.ts b/packages/nodes-base/nodes/ReadBinaryFiles/ReadBinaryFiles.node.ts index af73db19a1ceb..1569e0b89e767 100644 --- a/packages/nodes-base/nodes/ReadBinaryFiles/ReadBinaryFiles.node.ts +++ b/packages/nodes-base/nodes/ReadBinaryFiles/ReadBinaryFiles.node.ts @@ -54,7 +54,7 @@ export class ReadBinaryFiles implements INodeType { const items: INodeExecutionData[] = []; for (const filePath of files) { - const stream = await this.helpers.createReadStream(filePath); + const stream = await this.helpers.createReadStream(await this.helpers.resolvePath(filePath)); items.push({ binary: { [dataPropertyName]: await this.helpers.prepareBinaryData(stream, filePath), diff --git a/packages/nodes-base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts b/packages/nodes-base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts index 2cfcf21e19483..5a84a23f0362f 100644 --- a/packages/nodes-base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts +++ b/packages/nodes-base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts @@ -98,7 +98,11 @@ export class WriteBinaryFile implements INodeType { // Write the file to disk - await this.helpers.writeContentToFile(fileName, fileContent, flag); + await this.helpers.writeContentToFile( + await this.helpers.resolvePath(fileName), + fileContent, + flag, + ); if (item.binary !== undefined) { // Create a shallow copy of the binary data so that the old diff --git a/packages/workflow/src/interfaces.ts b/packages/workflow/src/interfaces.ts index 5415f80d4620d..32b34d8ae8114 100644 --- a/packages/workflow/src/interfaces.ts +++ b/packages/workflow/src/interfaces.ts @@ -694,12 +694,19 @@ export interface BaseHelperFunctions { returnJsonArray(jsonData: IDataObject | IDataObject[]): INodeExecutionData[]; } +const __brand = Symbol('brand'); + +export type ResolvedFilePath = string & { + [__brand]: 'ResolvedFilePath'; +}; + export interface FileSystemHelperFunctions { - isFilePathBlocked(filePath: string): Promise; - createReadStream(path: PathLike): Promise; + resolvePath(path: PathLike): Promise; + isFilePathBlocked(filePath: ResolvedFilePath): boolean; + createReadStream(filePath: ResolvedFilePath): Promise; getStoragePath(): string; writeContentToFile( - path: PathLike, + path: ResolvedFilePath, content: string | Buffer | Readable, flag?: string, ): Promise; From 5a7bf76430d39e994062f1d9ba124dd2e147f5bd Mon Sep 17 00:00:00 2001 From: mfsiega <93014743+mfsiega@users.noreply.github.com> Date: Thu, 4 Dec 2025 21:16:52 +0100 Subject: [PATCH 02/10] Update packages/core/src/execution-engine/node-execution-context/utils/file-system-helper-functions.ts Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> --- .../utils/file-system-helper-functions.ts | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) 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 843638df6f409..614326e3641b9 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 @@ -91,22 +91,29 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct // Use O_NOFOLLOW to prevent TOCTOU race conditions where the file could be // swapped to a symlink after validation but before opening - try { - return createReadStream(resolvedFilePath, { - // TypeScript types expect flags to be a string, but Node.js accepts numbers - // We use numeric flags here to combine O_RDONLY and O_NOFOLLOW bitwise - flags: (constants.O_RDONLY | constants.O_NOFOLLOW) as unknown as string, + const stream = createReadStream(resolvedFilePath, { + // TypeScript types expect flags to be a string, but Node.js accepts numbers + // We use numeric flags here to combine O_RDONLY and O_NOFOLLOW bitwise + flags: (constants.O_RDONLY | constants.O_NOFOLLOW) as unknown as string, + }); + + return new Promise>((resolve, reject) => { + stream.once('error', (error) => { + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if ((error as NodeJS.ErrnoException).code === 'ELOOP') { + reject( + new NodeOperationError( + node, + `The file "${String(resolvedFilePath)}" could not be opened.`, + { level: 'warning' }, + ), + ); + } else { + reject(error); + } }); - } catch (error) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - if (error.code === 'ELOOP') { - throw new NodeOperationError( - node, - `The file "${String(resolvedFilePath)}" could not be opened.`, - { - level: 'warning', - }, - ); + stream.once('open', () => resolve(stream)); + }); } throw error; } From 77d62448e0a401a09df491862d811812ff726103 Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Thu, 4 Dec 2025 21:24:28 +0100 Subject: [PATCH 03/10] fix lint --- .../utils/file-system-helper-functions.ts | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) 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 614326e3641b9..8ca5481185f37 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,11 +1,10 @@ import { isContainedWithin, safeJoinPath } from '@n8n/backend-common'; import { SecurityConfig } from '@n8n/config'; import { Container } from '@n8n/di'; -import type { FileSystemHelperFunctions, INode } from 'n8n-workflow'; import { NodeOperationError } from 'n8n-workflow'; +import type { FileSystemHelperFunctions, INode, ResolvedFilePath } from 'n8n-workflow'; import type { PathLike } from 'node:fs'; import { constants, createReadStream } from 'node:fs'; -import type { ResolvedFilePath } from 'n8n-workflow'; import { access as fsAccess, writeFile as fsWriteFile, @@ -89,17 +88,14 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct : error; } - // Use O_NOFOLLOW to prevent TOCTOU race conditions where the file could be - // swapped to a symlink after validation but before opening + // Use O_NOFOLLOW to prevent createReadStream from following symlinks. We require that the path + // already be resolved beforehand. const stream = createReadStream(resolvedFilePath, { - // TypeScript types expect flags to be a string, but Node.js accepts numbers - // We use numeric flags here to combine O_RDONLY and O_NOFOLLOW bitwise flags: (constants.O_RDONLY | constants.O_NOFOLLOW) as unknown as string, }); - return new Promise>((resolve, reject) => { + return await new Promise>((resolve, reject) => { stream.once('error', (error) => { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access if ((error as NodeJS.ErrnoException).code === 'ELOOP') { reject( new NodeOperationError( @@ -114,9 +110,6 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct }); stream.once('open', () => resolve(stream)); }); - } - throw error; - } }, getStoragePath() { From ea4923a5ecacc3a1a627a1482a241787b52e9951 Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Thu, 4 Dec 2025 21:46:32 +0100 Subject: [PATCH 04/10] fix unit tests --- .../file-system-helper-functions.test.ts | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts index c26098055f713..fe2c86f978365 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts @@ -244,6 +244,19 @@ describe('getFileSystemHelperFunctions', () => { it('should create a read stream if file access is permitted', async () => { const filePath = '/allowed/path'; (fsAccess as jest.Mock).mockResolvedValueOnce({}); + + // Mock createReadStream to return a proper stream-like object + const mockStream = { + once: jest.fn((event: string, callback: Function) => { + 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, @@ -265,10 +278,17 @@ describe('getFileSystemHelperFunctions', () => { // @ts-expect-error undefined property eloopError.code = 'ELOOP'; - // Mock createReadStream to throw ELOOP synchronously (which gets caught by try-catch) - (createReadStream as jest.Mock).mockImplementationOnce(() => { - throw eloopError; - }); + // Mock createReadStream to return a stream that emits an error event + const mockStream = { + once: jest.fn((event: string, callback: Function) => { + 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)), From 671881a5273b26a6cc868b2d7189b1412e234479 Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Thu, 4 Dec 2025 22:01:26 +0100 Subject: [PATCH 05/10] fix test typecheck --- .../utils/__tests__/file-system-helper-functions.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts index fe2c86f978365..cb29838c43bfb 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts @@ -246,8 +246,8 @@ describe('getFileSystemHelperFunctions', () => { (fsAccess as jest.Mock).mockResolvedValueOnce({}); // Mock createReadStream to return a proper stream-like object - const mockStream = { - once: jest.fn((event: string, callback: Function) => { + const mockStream: { once: jest.Mock } = { + once: jest.fn((event: string, callback: Function): typeof mockStream => { if (event === 'open') { // Immediately call the open callback setImmediate(() => callback()); @@ -279,8 +279,8 @@ describe('getFileSystemHelperFunctions', () => { eloopError.code = 'ELOOP'; // Mock createReadStream to return a stream that emits an error event - const mockStream = { - once: jest.fn((event: string, callback: Function) => { + const mockStream: { once: jest.Mock } = { + once: jest.fn((event: string, callback: Function): typeof mockStream => { if (event === 'error') { // Emit the error asynchronously setImmediate(() => callback(eloopError)); From ef4770795d4d814ab864c33e13f53b46830779da Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Thu, 4 Dec 2025 22:06:29 +0100 Subject: [PATCH 06/10] fix lint --- .../utils/__tests__/file-system-helper-functions.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts index cb29838c43bfb..1e04520123370 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts @@ -247,7 +247,7 @@ describe('getFileSystemHelperFunctions', () => { // Mock createReadStream to return a proper stream-like object const mockStream: { once: jest.Mock } = { - once: jest.fn((event: string, callback: Function): typeof mockStream => { + once: jest.fn((event: string, callback: (error?: Error) => void): typeof mockStream => { if (event === 'open') { // Immediately call the open callback setImmediate(() => callback()); @@ -280,7 +280,7 @@ describe('getFileSystemHelperFunctions', () => { // Mock createReadStream to return a stream that emits an error event const mockStream: { once: jest.Mock } = { - once: jest.fn((event: string, callback: Function): typeof mockStream => { + once: jest.fn((event: string, callback: (error?: Error) => void): typeof mockStream => { if (event === 'error') { // Emit the error asynchronously setImmediate(() => callback(eloopError)); From 5dc914417677e0c38e1cfa43b386f8bdb7ce0247 Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Fri, 5 Dec 2025 11:06:51 +0100 Subject: [PATCH 07/10] update test mocks --- packages/nodes-base/nodes/Crypto/test/Crypto.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts b/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts index b6a53e8d61d6c..117554a76871a 100644 --- a/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts +++ b/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts @@ -10,7 +10,12 @@ describe('Test Crypto Node', () => { const realpathSpy = jest.spyOn(fsPromises, 'realpath'); realpathSpy.mockImplementation(async (path) => path as string); jest.mock('fs'); - fs.createReadStream = () => Readable.from(Buffer.from('test')) as fs.ReadStream; + fs.createReadStream = () => { + const stream = Readable.from(Buffer.from('test')) as fs.ReadStream; + // Emit 'open' event asynchronously to match real fs.ReadStream behavior + setImmediate(() => stream.emit('open')); + return stream; + }; new NodeTestHarness().setupTests(); }); From 2e752b41ea2badc5798721b51eed9cb65f86d64f Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Fri, 5 Dec 2025 16:59:51 +0100 Subject: [PATCH 08/10] prevent fs write helper from re-resolving --- .../file-system-helper-functions.test.ts | 4 ++-- .../utils/file-system-helper-functions.ts | 18 ++++++++++-------- .../ReadWriteFile/actions/write.operation.ts | 5 ++++- .../WriteBinaryFile/WriteBinaryFile.node.ts | 5 ++++- packages/workflow/src/interfaces.ts | 4 ++-- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts index 1e04520123370..c15c4f9d08e4a 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts +++ b/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'; @@ -304,7 +304,7 @@ describe('getFileSystemHelperFunctions', () => { helperFunctions.writeContentToFile( await helperFunctions.resolvePath(instanceSettings.n8nFolder + '/test.txt'), 'content', - 'w', + constants.O_WRONLY | constants.O_CREAT | constants.O_TRUNC, ), ).rejects.toThrow('not writable'); }); 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 8ca5481185f37..b4ad30bcdd752 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 @@ -36,7 +36,7 @@ const getAllowedPaths = () => { return allowedPaths; }; -const resolvePath = async (path: PathLike): Promise => { +async function resolvePath(path: PathLike): Promise { try { return (await fsRealpath(path)) as ResolvedFilePath; // apply brand, since we know it's resolved now } catch (error: unknown) { @@ -45,7 +45,7 @@ const resolvePath = async (path: PathLike): Promise => { } throw error; } -}; +} function isFilePathBlocked(resolvedFilePath: ResolvedFilePath): boolean { const allowedPaths = getAllowedPaths(); @@ -98,11 +98,10 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct stream.once('error', (error) => { if ((error as NodeJS.ErrnoException).code === 'ELOOP') { reject( - new NodeOperationError( - node, - `The file "${String(resolvedFilePath)}" could not be opened.`, - { level: 'warning' }, - ), + new NodeOperationError(node, error, { + level: 'warning', + description: 'Symlinks are not allowed.', + }), ); } else { reject(error); @@ -126,7 +125,10 @@ export const getFileSystemHelperFunctions = (node: INode): FileSystemHelperFunct }, ); } - return await fsWriteFile(resolvedFilePath, content, { encoding: 'binary', flag }); + return await fsWriteFile(resolvedFilePath, content, { + encoding: 'binary', + flag: (flag ?? 0) | constants.O_NOFOLLOW, + }); }, resolvePath, isFilePathBlocked, diff --git a/packages/nodes-base/nodes/Files/ReadWriteFile/actions/write.operation.ts b/packages/nodes-base/nodes/Files/ReadWriteFile/actions/write.operation.ts index eb3d58dfde7cc..ba84476797102 100644 --- a/packages/nodes-base/nodes/Files/ReadWriteFile/actions/write.operation.ts +++ b/packages/nodes-base/nodes/Files/ReadWriteFile/actions/write.operation.ts @@ -10,6 +10,7 @@ import type { Readable } from 'stream'; import { updateDisplayOptions } from '@utils/utilities'; import { errorMapper } from '../helpers/utils'; +import { constants } from 'node:fs'; export const properties: INodeProperties[] = [ { @@ -68,7 +69,9 @@ export async function execute(this: IExecuteFunctions, items: INodeExecutionData const dataPropertyName = this.getNodeParameter('dataPropertyName', itemIndex); fileName = this.getNodeParameter('fileName', itemIndex) as string; const options = this.getNodeParameter('options', itemIndex, {}); - const flag: string = options.append ? 'a' : 'w'; + const flag: number = options.append + ? constants.O_APPEND + : constants.O_WRONLY | constants.O_CREAT | constants.O_TRUNC; item = items[itemIndex]; diff --git a/packages/nodes-base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts b/packages/nodes-base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts index 5a84a23f0362f..ab978bb017f52 100644 --- a/packages/nodes-base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts +++ b/packages/nodes-base/nodes/WriteBinaryFile/WriteBinaryFile.node.ts @@ -5,6 +5,7 @@ import type { INodeType, INodeTypeDescription, } from 'n8n-workflow'; +import { constants } from 'node:fs'; import type { Readable } from 'stream'; export class WriteBinaryFile implements INodeType { @@ -75,7 +76,9 @@ export class WriteBinaryFile implements INodeType { const options = this.getNodeParameter('options', 0, {}); - const flag: string = options.append ? 'a' : 'w'; + const flag: number = options.append + ? constants.O_APPEND + : constants.O_WRONLY | constants.O_CREAT | constants.O_TRUNC; item = items[itemIndex]; diff --git a/packages/workflow/src/interfaces.ts b/packages/workflow/src/interfaces.ts index 32b34d8ae8114..c39cc5201767e 100644 --- a/packages/workflow/src/interfaces.ts +++ b/packages/workflow/src/interfaces.ts @@ -694,7 +694,7 @@ export interface BaseHelperFunctions { returnJsonArray(jsonData: IDataObject | IDataObject[]): INodeExecutionData[]; } -const __brand = Symbol('brand'); +const __brand = Symbol('resolvedFilePath'); export type ResolvedFilePath = string & { [__brand]: 'ResolvedFilePath'; @@ -708,7 +708,7 @@ export interface FileSystemHelperFunctions { writeContentToFile( path: ResolvedFilePath, content: string | Buffer | Readable, - flag?: string, + flag?: number, ): Promise; } From 7140e5755eb857a0d104af78b447b3757b839ab6 Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Fri, 5 Dec 2025 20:45:39 +0100 Subject: [PATCH 09/10] fix test expectation --- .../utils/__tests__/file-system-helper-functions.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts index c15c4f9d08e4a..6e1385e19874b 100644 --- a/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts +++ b/packages/core/src/execution-engine/node-execution-context/utils/__tests__/file-system-helper-functions.test.ts @@ -292,7 +292,7 @@ describe('getFileSystemHelperFunctions', () => { await expect( helperFunctions.createReadStream(await helperFunctions.resolvePath(filePath)), - ).rejects.toThrow('could not be opened'); + ).rejects.toThrow('ELOOP: too many symbolic links encountered'); }); }); From b8df2f685a9d105ecf5c4d5eace03c86a69e1e72 Mon Sep 17 00:00:00 2001 From: Michael Siega Date: Mon, 8 Dec 2025 14:45:10 +0100 Subject: [PATCH 10/10] add docs to resolvePath --- packages/workflow/src/interfaces.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/workflow/src/interfaces.ts b/packages/workflow/src/interfaces.ts index c39cc5201767e..2292c2934b0b4 100644 --- a/packages/workflow/src/interfaces.ts +++ b/packages/workflow/src/interfaces.ts @@ -702,9 +702,18 @@ export type ResolvedFilePath = string & { export interface FileSystemHelperFunctions { resolvePath(path: PathLike): Promise; + /** + * Use {@link resolvePath} to resolve the path first. + */ isFilePathBlocked(filePath: ResolvedFilePath): boolean; + /** + * Use {@link resolvePath} to resolve the path first. + */ createReadStream(filePath: ResolvedFilePath): Promise; getStoragePath(): string; + /** + * Use {@link resolvePath} to resolve the path first. + */ writeContentToFile( path: ResolvedFilePath, content: string | Buffer | Readable,