From fc932720213fb3e8d74864f9f00e900d6d927255 Mon Sep 17 00:00:00 2001 From: mfsiega <93014743+mfsiega@users.noreply.github.com> Date: Mon, 8 Dec 2025 16:04:21 +0100 Subject: [PATCH] fix(core): Only resolve the filepath once (#22767) Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> --- .../file-system-helper-functions.test.ts | 123 ++++++++++++------ .../utils/file-system-helper-functions.ts | 71 +++++++--- .../nodes/Crypto/test/Crypto.test.ts | 7 +- .../ReadWriteFile/actions/read.operation.ts | 4 +- .../ReadWriteFile/actions/write.operation.ts | 11 +- 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 | 11 +- packages/workflow/src/interfaces.ts | 24 +++- 12 files changed, 226 insertions(+), 75 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 6710ff04fdf..6e1385e1987 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'; @@ -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'); }); 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 24c0c27360d..b4ad30bcdd7 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 { 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 { createReadStream } from 'node:fs'; +import type { FileSystemHelperFunctions, INode, ResolvedFilePath } from 'n8n-workflow'; +import type { PathLike } from 'node:fs'; +import { constants, createReadStream } from 'node:fs'; import { access as fsAccess, writeFile as fsWriteFile, @@ -35,18 +36,19 @@ const getAllowedPaths = () => { return allowedPaths; }; -export async function isFilePathBlocked(filePath: string): Promise { - const allowedPaths = getAllowedPaths(); - let resolvedFilePath = ''; +async function resolvePath(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 +66,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 +76,61 @@ 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 createReadStream from following symlinks. We require that the path + // already be resolved beforehand. + const stream = createReadStream(resolvedFilePath, { + flags: (constants.O_RDONLY | constants.O_NOFOLLOW) as unknown as string, + }); + + return await new Promise>((resolve, reject) => { + stream.once('error', (error) => { + if ((error as NodeJS.ErrnoException).code === 'ELOOP') { + reject( + new NodeOperationError(node, error, { + level: 'warning', + description: 'Symlinks are not allowed.', + }), + ); + } else { + reject(error); + } + }); + stream.once('open', () => resolve(stream)); + }); }, 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: (flag ?? 0) | constants.O_NOFOLLOW, + }); }, - + resolvePath, isFilePathBlocked, }); diff --git a/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts b/packages/nodes-base/nodes/Crypto/test/Crypto.test.ts index b6a53e8d61d..117554a7687 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(); }); 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 783981399ae..258df540523 100644 --- a/packages/nodes-base/nodes/Files/ReadWriteFile/actions/read.operation.ts +++ b/packages/nodes-base/nodes/Files/ReadWriteFile/actions/read.operation.ts @@ -105,7 +105,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 5f4d28775a6..ba844767971 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]; @@ -90,7 +93,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 ac722bc0b8d..03d4f177e10 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 5256b92e872..4b30fdb0922 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 568511249eb..7d6006c5ee4 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 7944e5194b0..483a6536598 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 af73db19a1c..1569e0b89e7 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 2cfcf21e194..ab978bb017f 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]; @@ -98,7 +101,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 5415f80d462..2292c2934b0 100644 --- a/packages/workflow/src/interfaces.ts +++ b/packages/workflow/src/interfaces.ts @@ -694,14 +694,30 @@ export interface BaseHelperFunctions { returnJsonArray(jsonData: IDataObject | IDataObject[]): INodeExecutionData[]; } +const __brand = Symbol('resolvedFilePath'); + +export type ResolvedFilePath = string & { + [__brand]: 'ResolvedFilePath'; +}; + export interface FileSystemHelperFunctions { - isFilePathBlocked(filePath: string): Promise; - createReadStream(path: PathLike): Promise; + 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: PathLike, + path: ResolvedFilePath, content: string | Buffer | Readable, - flag?: string, + flag?: number, ): Promise; }