refactor(core): Create controller for binary data (no-changelog) (#7363)

This PR adds a controller for binary data + integration tests.

packages/cli/src/ResponseHelper.ts

@@ -6,7 +6,7 @@ import type { Request, Response } from 'express';
 import { parse, stringify } from 'flatted';
 import picocolors from 'picocolors';
 import { ErrorReporterProxy as ErrorReporter, NodeApiError } from 'n8n-workflow';
-
+import { Readable } from 'node:stream';
 import type {
 	IExecutionDb,
 	IExecutionFlatted,
@@ -101,6 +101,11 @@ export function sendSuccessResponse(
 		res.header(responseHeader);
 	}
 
+	if (data instanceof Readable) {
+		data.pipe(res);
+		return;
+	}
+
 	if (raw === true) {
 		if (typeof data === 'string') {
 			res.send(data);

packages/cli/src/Server.ts

@@ -26,13 +26,11 @@ import type { RequestOptions } from 'oauth-1.0a';
 import clientOAuth1 from 'oauth-1.0a';
 
 import {
-	BinaryDataService,
 	Credentials,
 	LoadMappingOptions,
 	LoadNodeParameterOptions,
 	LoadNodeListSearch,
 	UserSettings,
-	FileNotFoundError,
 } from 'n8n-core';
 
 import type {
@@ -74,7 +72,6 @@ import {
 import { credentialsController } from '@/credentials/credentials.controller';
 import { oauth2CredentialController } from '@/credentials/oauth2Credential.api';
 import type {
-	BinaryDataRequest,
 	CurlHelper,
 	ExecutionRequest,
 	NodeListSearchRequest,
@@ -99,6 +96,7 @@ import {
 	WorkflowStatisticsController,
 } from '@/controllers';
 
+import { BinaryDataController } from './controllers/binaryData.controller';
 import { ExternalSecretsController } from '@/ExternalSecrets/ExternalSecrets.controller.ee';
 import { executionsController } from '@/executions/executions.controller';
 import { isApiEnabled, loadPublicApiVersions } from '@/PublicApi';
@@ -208,8 +206,6 @@ export class Server extends AbstractServer {
 
 	push: Push;
 
-	binaryDataService: BinaryDataService;
-
 	constructor() {
 		super('main');
 
@@ -374,7 +370,6 @@ export class Server extends AbstractServer {
 		this.endpointPresetCredentials = config.getEnv('credentials.overwrite.endpoint');
 
 		this.push = Container.get(Push);
-		this.binaryDataService = Container.get(BinaryDataService);
 
 		await super.start();
 		LoggerProxy.debug(`Server ID: ${this.uniqueInstanceId}`);
@@ -581,6 +576,7 @@ export class Server extends AbstractServer {
 			Container.get(ExternalSecretsController),
 			Container.get(OrchestrationController),
 			Container.get(WorkflowHistoryController),
+			Container.get(BinaryDataController),
 		];
 
 		if (isLdapEnabled()) {
@@ -1442,50 +1438,6 @@ export class Server extends AbstractServer {
 			}),
 		);
 
-		// ----------------------------------------
-		// Binary data
-		// ----------------------------------------
-
-		// View or download binary file
-		this.app.get(
-			`/${this.restEndpoint}/data`,
-			async (req: BinaryDataRequest, res: express.Response): Promise<void> => {
-				const { id: binaryDataId, action } = req.query;
-				let { fileName, mimeType } = req.query;
-				const [mode] = binaryDataId.split(':') as ['filesystem' | 's3', string];
-
-				try {
-					const binaryPath = this.binaryDataService.getPath(binaryDataId);
-
-					if (!fileName || !mimeType) {
-						try {
-							const metadata = await this.binaryDataService.getMetadata(binaryDataId);
-							fileName = metadata.fileName;
-							mimeType = metadata.mimeType;
-							res.setHeader('Content-Length', metadata.fileSize);
-						} catch {}
-					}
-
-					if (mimeType) res.setHeader('Content-Type', mimeType);
-
-					if (action === 'download') {
-						res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`);
-					}
-
-					if (mode === 's3') {
-						const readStream = await this.binaryDataService.getAsStream(binaryDataId);
-						readStream.pipe(res);
-						return;
-					} else {
-						res.sendFile(binaryPath);
-					}
-				} catch (error) {
-					if (error instanceof FileNotFoundError) res.writeHead(404).end();
-					else throw error;
-				}
-			},
-		);
-
 		// ----------------------------------------
 		// Settings
 		// ----------------------------------------

packages/cli/src/controllers/binaryData.controller.ts

@@ -0,0 +1,54 @@
+import { Service } from 'typedi';
+import express from 'express';
+import { BinaryDataService, FileNotFoundError, isValidNonDefaultMode } from 'n8n-core';
+import { Get, RestController } from '@/decorators';
+import { BinaryDataRequest } from '@/requests';
+
+@RestController('/binary-data')
+@Service()
+export class BinaryDataController {
+	constructor(private readonly binaryDataService: BinaryDataService) {}
+
+	@Get('/')
+	async get(req: BinaryDataRequest, res: express.Response) {
+		const { id: binaryDataId, action } = req.query;
+
+		if (!binaryDataId) {
+			return res.status(400).end('Missing binary data ID');
+		}
+
+		if (!binaryDataId.includes(':')) {
+			return res.status(400).end('Missing binary data mode');
+		}
+
+		const [mode] = binaryDataId.split(':');
+
+		if (!isValidNonDefaultMode(mode)) {
+			return res.status(400).end('Invalid binary data mode');
+		}
+
+		let { fileName, mimeType } = req.query;
+
+		try {
+			if (!fileName || !mimeType) {
+				try {
+					const metadata = await this.binaryDataService.getMetadata(binaryDataId);
+					fileName = metadata.fileName;
+					mimeType = metadata.mimeType;
+					res.setHeader('Content-Length', metadata.fileSize);
+				} catch {}
+			}
+
+			if (mimeType) res.setHeader('Content-Type', mimeType);
+
+			if (action === 'download') {
+				res.setHeader('Content-Disposition', `attachment; filename="${fileName}"`);
+			}
+
+			return await this.binaryDataService.getAsStream(binaryDataId);
+		} catch (error) {
+			if (error instanceof FileNotFoundError) return res.writeHead(404).end();
+			else throw error;
+		}
+	}
+}

packages/cli/test/integration/binaryData.api.test.ts

@@ -0,0 +1,143 @@
+import fsp from 'node:fs/promises';
+import { Readable } from 'node:stream';
+import { BinaryDataService, FileNotFoundError } from 'n8n-core';
+import * as testDb from './shared/testDb';
+import { mockInstance, setupTestServer } from './shared/utils';
+import type { SuperAgentTest } from 'supertest';
+
+jest.mock('fs/promises');
+
+const throwFileNotFound = () => {
+	throw new FileNotFoundError('non/existing/path');
+};
+
+const binaryDataService = mockInstance(BinaryDataService);
+let testServer = setupTestServer({ endpointGroups: ['binaryData'] });
+let authOwnerAgent: SuperAgentTest;
+
+beforeAll(async () => {
+	const owner = await testDb.createOwner();
+	authOwnerAgent = testServer.authAgentFor(owner);
+});
+
+afterEach(() => {
+	jest.restoreAllMocks();
+});
+
+describe('GET /binary-data', () => {
+	const fileId = '599c5f84007-7d14-4b63-8f1e-d726098d0cc0';
+	const fsBinaryDataId = `filesystem:${fileId}`;
+	const s3BinaryDataId = `s3:${fileId}`;
+	const binaryFilePath = `/Users/john/.n8n/binaryData/${fileId}`;
+	const mimeType = 'text/plain';
+	const fileName = 'test.txt';
+	const buffer = Buffer.from('content');
+	const mockStream = new Readable();
+	mockStream.push(buffer);
+	mockStream.push(null);
+
+	describe('should reject on missing or invalid binary data ID', () => {
+		test.each([['view'], ['download']])('on request to %s', async (action) => {
+			binaryDataService.getPath.mockReturnValue(binaryFilePath);
+			fsp.readFile = jest.fn().mockResolvedValue(buffer);
+
+			await authOwnerAgent
+				.get('/binary-data')
+				.query({
+					fileName,
+					mimeType,
+					action,
+				})
+				.expect(400);
+
+			await authOwnerAgent
+				.get('/binary-data')
+				.query({
+					id: 'invalid',
+					fileName,
+					mimeType,
+					action,
+				})
+				.expect(400);
+		});
+	});
+
+	describe('should return binary data [filesystem]', () => {
+		test.each([['view'], ['download']])('on request to %s', async (action) => {
+			binaryDataService.getAsStream.mockResolvedValue(mockStream);
+
+			const res = await authOwnerAgent
+				.get('/binary-data')
+				.query({
+					id: fsBinaryDataId,
+					fileName,
+					mimeType,
+					action,
+				})
+				.expect(200);
+
+			const contentDisposition =
+				action === 'download' ? `attachment; filename="${fileName}"` : undefined;
+
+			expect(binaryDataService.getAsStream).toHaveBeenCalledWith(fsBinaryDataId);
+			expect(res.headers['content-type']).toBe(mimeType);
+			expect(res.headers['content-disposition']).toBe(contentDisposition);
+		});
+	});
+
+	describe('should return 404 on file not found [filesystem]', () => {
+		test.each(['view', 'download'])('on request to %s', async (action) => {
+			binaryDataService.getAsStream.mockImplementation(throwFileNotFound);
+
+			await authOwnerAgent
+				.get('/binary-data')
+				.query({
+					id: fsBinaryDataId,
+					fileName,
+					mimeType,
+					action,
+				})
+				.expect(404);
+		});
+	});
+
+	describe('should return binary data [s3]', () => {
+		test.each([['view'], ['download']])('on request to %s', async (action) => {
+			binaryDataService.getAsStream.mockResolvedValue(mockStream);
+
+			const res = await authOwnerAgent
+				.get('/binary-data')
+				.query({
+					id: s3BinaryDataId,
+					fileName,
+					mimeType,
+					action,
+				})
+				.expect(200);
+
+			expect(binaryDataService.getAsStream).toHaveBeenCalledWith(s3BinaryDataId);
+
+			const contentDisposition =
+				action === 'download' ? `attachment; filename="${fileName}"` : undefined;
+
+			expect(res.headers['content-type']).toBe(mimeType);
+			expect(res.headers['content-disposition']).toBe(contentDisposition);
+		});
+	});
+
+	describe('should return 404 on file not found [s3]', () => {
+		test.each(['view', 'download'])('on request to %s', async (action) => {
+			binaryDataService.getAsStream.mockImplementation(throwFileNotFound);
+
+			await authOwnerAgent
+				.get('/binary-data')
+				.query({
+					id: s3BinaryDataId,
+					fileName,
+					mimeType,
+					action,
+				})
+				.expect(404);
+		});
+	});
+});

packages/cli/test/integration/shared/types.ts

@@ -34,7 +34,8 @@ export type EndpointGroup =
 	| 'mfa'
 	| 'metrics'
 	| 'executions'
-	| 'workflowHistory';
+	| 'workflowHistory'
+	| 'binaryData';
 
 export interface SetupProps {
 	applyAuth?: boolean;

packages/cli/test/integration/shared/utils/testServer.ts

@@ -66,6 +66,7 @@ import { RoleService } from '@/services/role.service';
 import { UserService } from '@/services/user.service';
 import { executionsController } from '@/executions/executions.controller';
 import { WorkflowHistoryController } from '@/workflows/workflowHistory/workflowHistory.controller.ee';
+import { BinaryDataController } from '@/controllers/binaryData.controller';
 
 /**
  * Plugin to prefix a path segment into a request URL pathname.
@@ -316,6 +317,9 @@ export const setupTestServer = ({
 					case 'workflowHistory':
 						registerController(app, config, Container.get(WorkflowHistoryController));
 						break;
+					case 'binaryData':
+						registerController(app, config, Container.get(BinaryDataController));
+						break;
 				}
 			}
 		}

packages/core/src/BinaryData/utils.ts

@@ -15,6 +15,10 @@ export function areValidModes(modes: string[]): modes is BinaryData.Mode[] {
 	return modes.every((m) => BINARY_DATA_MODES.includes(m as BinaryData.Mode));
 }
 
+export function isValidNonDefaultMode(mode: string): mode is BinaryData.NonDefaultMode {
+	return BINARY_DATA_MODES.filter((m) => m !== 'default').includes(mode as BinaryData.Mode);
+}
+
 export async function ensureDirExists(dir: string) {
 	try {
 		await fs.access(dir);

packages/core/src/index.ts

@@ -18,3 +18,4 @@ export { NodeExecuteFunctions, UserSettings };
 export * from './errors';
 export { ObjectStoreService } from './ObjectStore/ObjectStore.service.ee';
 export { BinaryData } from './BinaryData/types';
+export { isValidNonDefaultMode } from './BinaryData/utils';

packages/editor-ui/src/stores/workflows.store.ts

@@ -1393,7 +1393,7 @@ export const useWorkflowsStore = defineStore(STORES.WORKFLOWS, {
 			const rootStore = useRootStore();
 			let restUrl = rootStore.getRestUrl;
 			if (restUrl.startsWith('/')) restUrl = window.location.origin + restUrl;
-			const url = new URL(`${restUrl}/data`);
+			const url = new URL(`${restUrl}/binary-data`);
 			url.searchParams.append('id', binaryDataId);
 			url.searchParams.append('action', action);
 			if (fileName) url.searchParams.append('fileName', fileName);