diff --git a/packages/core/src/BinaryData/BinaryData.service.ts b/packages/core/src/BinaryData/BinaryData.service.ts index 4aec461ff3f2d..b3377f8181327 100644 --- a/packages/core/src/BinaryData/BinaryData.service.ts +++ b/packages/core/src/BinaryData/BinaryData.service.ts @@ -1,14 +1,13 @@ import { readFile, stat } from 'node:fs/promises'; import prettyBytes from 'pretty-bytes'; import Container, { Service } from 'typedi'; -import { BINARY_ENCODING, LoggerProxy as Logger, IBinaryData } from 'n8n-workflow'; +import { BINARY_ENCODING } from 'n8n-workflow'; import { UnknownManagerError, InvalidModeError } from './errors'; import { areConfigModes, toBuffer } from './utils'; -import { LogCatch } from '../decorators/LogCatch.decorator'; import type { Readable } from 'stream'; import type { BinaryData } from './types'; -import type { INodeExecutionData } from 'n8n-workflow'; +import type { INodeExecutionData, IBinaryData } from 'n8n-workflow'; @Service() export class BinaryDataService { @@ -40,7 +39,6 @@ export class BinaryDataService { } } - @LogCatch((error) => Logger.error('Failed to copy binary data file', { error })) async copyBinaryFile( workflowId: string, executionId: string, @@ -76,7 +74,6 @@ export class BinaryDataService { return binaryData; } - @LogCatch((error) => Logger.error('Failed to write binary data file', { error })) async store( workflowId: string, executionId: string, @@ -152,9 +149,6 @@ export class BinaryDataService { if (manager.deleteMany) await manager.deleteMany(ids); } - @LogCatch((error) => - Logger.error('Failed to copy all binary data files for execution', { error }), - ) async duplicateBinaryData( workflowId: string, executionId: string, diff --git a/packages/core/src/BinaryData/FileSystem.manager.ts b/packages/core/src/BinaryData/FileSystem.manager.ts index f2105972666dd..23b26dee09986 100644 --- a/packages/core/src/BinaryData/FileSystem.manager.ts +++ b/packages/core/src/BinaryData/FileSystem.manager.ts @@ -3,8 +3,8 @@ import fs from 'node:fs/promises'; import path from 'node:path'; import { v4 as uuid } from 'uuid'; import { jsonParse } from 'n8n-workflow'; -import { assertDir } from './utils'; -import { FileNotFoundError } from '../errors'; +import { assertDir, doesNotExist } from './utils'; +import { BinaryFileNotFoundError, InvalidPathError } from '../errors'; import type { Readable } from 'stream'; import type { BinaryData } from './types'; @@ -46,17 +46,21 @@ export class FileSystemManager implements BinaryData.Manager { async getAsStream(fileId: string, chunkSize?: number) { const filePath = this.resolvePath(fileId); + if (await doesNotExist(filePath)) { + throw new BinaryFileNotFoundError(filePath); + } + return createReadStream(filePath, { highWaterMark: chunkSize }); } async getAsBuffer(fileId: string) { const filePath = this.resolvePath(fileId); - try { - return await fs.readFile(filePath); - } catch { - throw new Error(`Error finding file: ${filePath}`); + if (await doesNotExist(filePath)) { + throw new BinaryFileNotFoundError(filePath); } + + return fs.readFile(filePath); } async getMetadata(fileId: string): Promise { @@ -167,7 +171,7 @@ export class FileSystemManager implements BinaryData.Manager { const returnPath = path.join(this.storagePath, ...args); if (path.relative(this.storagePath, returnPath).startsWith('..')) { - throw new FileNotFoundError('Invalid path detected'); + throw new InvalidPathError(returnPath); } return returnPath; @@ -186,7 +190,7 @@ export class FileSystemManager implements BinaryData.Manager { const stats = await fs.stat(filePath); return stats.size; } catch (error) { - throw new Error('Failed to find binary data file in filesystem', { cause: error }); + throw new BinaryFileNotFoundError(filePath); } } } diff --git a/packages/core/src/BinaryData/utils.ts b/packages/core/src/BinaryData/utils.ts index 14a9b457886c0..cea0c40af53df 100644 --- a/packages/core/src/BinaryData/utils.ts +++ b/packages/core/src/BinaryData/utils.ts @@ -23,6 +23,15 @@ export async function assertDir(dir: string) { } } +export async function doesNotExist(dir: string) { + try { + await fs.access(dir); + return false; + } catch { + return true; + } +} + export async function toBuffer(body: Buffer | Readable) { return new Promise((resolve) => { if (Buffer.isBuffer(body)) resolve(body); diff --git a/packages/core/src/decorators/LogCatch.decorator.ts b/packages/core/src/decorators/LogCatch.decorator.ts deleted file mode 100644 index a5999c50ad7b2..0000000000000 --- a/packages/core/src/decorators/LogCatch.decorator.ts +++ /dev/null @@ -1,29 +0,0 @@ -/* eslint-disable @typescript-eslint/no-unsafe-call */ -/* eslint-disable @typescript-eslint/no-unsafe-member-access */ -/* eslint-disable @typescript-eslint/no-unsafe-assignment */ - -export const LogCatch = (logFn: (error: unknown) => void) => { - return (target: unknown, propertyKey: string, descriptor: PropertyDescriptor) => { - const originalMethod = descriptor.value; - - descriptor.value = function (...args: unknown[]) { - try { - const result: unknown = originalMethod.apply(this, args); - - if (result && result instanceof Promise) { - return result.catch((error: unknown) => { - logFn(error); - throw error; - }); - } - - return result; - } catch (error) { - logFn(error); - throw error; - } - }; - - return descriptor; - }; -}; diff --git a/packages/core/src/errors.ts b/packages/core/src/errors.ts index c425675c89371..a8fea813804c6 100644 --- a/packages/core/src/errors.ts +++ b/packages/core/src/errors.ts @@ -3,3 +3,11 @@ export class FileNotFoundError extends Error { super(`File not found: ${filePath}`); } } + +export class BinaryFileNotFoundError extends FileNotFoundError {} + +export class InvalidPathError extends Error { + constructor(readonly filePath: string) { + super(`Invalid path detected: ${filePath}`); + } +} diff --git a/packages/core/test/FileSystem.manager.test.ts b/packages/core/test/FileSystem.manager.test.ts index 039307a57dc37..7087242726d61 100644 --- a/packages/core/test/FileSystem.manager.test.ts +++ b/packages/core/test/FileSystem.manager.test.ts @@ -53,6 +53,7 @@ describe('getPath()', () => { describe('getAsBuffer()', () => { it('should return a buffer', async () => { fsp.readFile = jest.fn().mockResolvedValue(mockBuffer); + fsp.access = jest.fn().mockImplementation(async () => {}); const result = await fsManager.getAsBuffer(fileId); @@ -64,6 +65,7 @@ describe('getAsBuffer()', () => { describe('getAsStream()', () => { it('should return a stream', async () => { fs.createReadStream = jest.fn().mockReturnValue(mockStream); + fsp.access = jest.fn().mockImplementation(async () => {}); const stream = await fsManager.getAsStream(fileId); @@ -123,6 +125,7 @@ describe('copyByFilePath()', () => { const targetPath = toFullFilePath(otherFileId); fsp.cp = jest.fn().mockResolvedValue(undefined); + fsp.writeFile = jest.fn().mockResolvedValue(undefined); const result = await fsManager.copyByFilePath( workflowId, @@ -132,6 +135,11 @@ describe('copyByFilePath()', () => { ); expect(fsp.cp).toHaveBeenCalledWith(sourceFilePath, targetPath); + expect(fsp.writeFile).toHaveBeenCalledWith( + `${toFullFilePath(otherFileId)}.metadata`, + JSON.stringify({ ...metadata, fileSize: mockBuffer.length }), + { encoding: 'utf-8' }, + ); expect(result.fileSize).toBe(mockBuffer.length); }); });