refactor(core): Extract form data parsing from webhook-helpers (#10904)

packages/cli/src/webhooks/__tests__/webhook-form-data.test.ts

@@ -0,0 +1,185 @@
+import express from 'express';
+import nock from 'nock';
+import type { Server, IncomingMessage } from 'node:http';
+import { createServer } from 'node:http';
+import request from 'supertest';
+import type TestAgent from 'supertest/lib/agent';
+
+import { rawBodyReader } from '@/middlewares';
+
+import { createMultiFormDataParser } from '../webhook-form-data';
+
+// Formidable requires FS to store the uploaded files
+jest.unmock('node:fs');
+
+/** Test server for testing the form data parsing */
+class TestServer {
+	public agent: TestAgent;
+
+	private app: express.Application;
+
+	private server: Server;
+
+	private testFn: (req: IncomingMessage) => Promise<void> = async () => {};
+
+	private hasBeenCalled = false;
+
+	constructor() {
+		this.app = express();
+		// rawBodyReader is required to parse the encoding of the incoming request
+		this.app.use(rawBodyReader, async (req, res) => {
+			try {
+				this.hasBeenCalled = true;
+
+				await this.testFn(req);
+			} finally {
+				res.end('done');
+			}
+		});
+
+		this.server = createServer(this.app);
+		this.agent = request.agent(this.app);
+	}
+
+	assertHasBeenCalled() {
+		expect(this.hasBeenCalled).toBeTruthy();
+	}
+
+	reset() {
+		this.testFn = async () => {};
+		this.hasBeenCalled = false;
+	}
+
+	sendRequestToHandler(handlerFn: (req: IncomingMessage) => Promise<void>) {
+		this.testFn = handlerFn;
+
+		return this.agent.post('/');
+	}
+
+	start() {
+		this.server.listen(0);
+	}
+
+	async stop() {
+		await new Promise((resolve) => this.server.close(resolve));
+	}
+}
+
+describe('webhook-form-data', () => {
+	describe('createMultiFormDataParser', () => {
+		const oneKbData = Buffer.from('1'.repeat(1024));
+		const testServer = new TestServer();
+
+		beforeAll(() => {
+			nock.enableNetConnect('127.0.0.1');
+
+			testServer.start();
+		});
+
+		afterEach(() => {
+			testServer.reset();
+		});
+
+		afterAll(async () => {
+			await testServer.stop();
+		});
+
+		it('should parse fields from the multipart form data', async () => {
+			const parseFn = createMultiFormDataParser(1);
+
+			await testServer
+				.sendRequestToHandler(async (req) => {
+					const parsedData = await parseFn(req);
+
+					expect(parsedData).toStrictEqual({
+						data: {
+							foo: 'bar',
+						},
+						files: {},
+					});
+				})
+				.field('foo', 'bar')
+				.expect(200);
+
+			testServer.assertHasBeenCalled();
+		});
+
+		it('should parse text/plain file from the multipart form data', async () => {
+			const parseFn = createMultiFormDataParser(1);
+
+			await testServer
+				.sendRequestToHandler(async (req) => {
+					const parsedData = await parseFn(req);
+
+					expect(parsedData).toStrictEqual({
+						data: {
+							filename: 'file.txt',
+						},
+						files: {
+							file: expect.objectContaining({
+								originalFilename: 'file.txt',
+								size: oneKbData.length,
+								mimetype: 'text/plain',
+							}),
+						},
+					});
+				})
+				.attach('file', oneKbData, 'file.txt')
+				.field('filename', 'file.txt');
+
+			testServer.assertHasBeenCalled();
+		});
+
+		it('should parse multiple files and fields from the multipart form data', async () => {
+			const parseFn = createMultiFormDataParser(1);
+
+			await testServer
+				.sendRequestToHandler(async (req) => {
+					const parsedData = await parseFn(req);
+
+					expect(parsedData).toStrictEqual({
+						data: {
+							file1: 'file.txt',
+							file2: 'file.bin',
+						},
+						files: {
+							txt_file: expect.objectContaining({
+								originalFilename: 'file.txt',
+								size: oneKbData.length,
+								mimetype: 'text/plain',
+							}),
+							bin_file: expect.objectContaining({
+								originalFilename: 'file.bin',
+								size: oneKbData.length,
+								mimetype: 'application/octet-stream',
+							}),
+						},
+					});
+				})
+				.attach('txt_file', oneKbData, 'file.txt')
+				.attach('bin_file', oneKbData, 'file.bin')
+				.field('file1', 'file.txt')
+				.field('file2', 'file.bin');
+
+			testServer.assertHasBeenCalled();
+		});
+
+		it('should ignore file that is too large', async () => {
+			const oneByteInMb = 1 / 1024 / 1024;
+			const parseFn = createMultiFormDataParser(oneByteInMb);
+
+			await testServer
+				.sendRequestToHandler(async (req) => {
+					const parsedData = await parseFn(req);
+
+					expect(parsedData).toStrictEqual({
+						data: {},
+						files: {},
+					});
+				})
+				.attach('file', oneKbData, 'file.txt');
+
+			testServer.assertHasBeenCalled();
+		});
+	});
+});

packages/cli/src/webhooks/webhook-form-data.ts

@@ -0,0 +1,38 @@
+import formidable from 'formidable';
+import type { IncomingMessage } from 'http';
+
+const normalizeFormData = <T>(values: Record<string, T | T[]>) => {
+	for (const key in values) {
+		const value = values[key];
+		if (Array.isArray(value) && value.length === 1) {
+			values[key] = value[0];
+		}
+	}
+};
+
+/**
+ * Creates a function that parses the multipart form data into the request's `body` property
+ */
+export const createMultiFormDataParser = (maxFormDataSizeInMb: number) => {
+	return async function parseMultipartFormData(req: IncomingMessage): Promise<{
+		data: formidable.Fields;
+		files: formidable.Files;
+	}> {
+		const { encoding } = req;
+
+		const form = formidable({
+			multiples: true,
+			encoding: encoding as formidable.BufferEncoding,
+			maxFileSize: maxFormDataSizeInMb * 1024 * 1024,
+			// TODO: pass a custom `fileWriteStreamHandler` to create binary data files directly
+		});
+
+		return await new Promise((resolve) => {
+			form.parse(req, async (_err, data, files) => {
+				normalizeFormData(data);
+				normalizeFormData(files);
+				resolve({ data, files });
+			});
+		});
+	};
+};

packages/cli/src/webhooks/webhook-helpers.ts

@@ -8,7 +8,6 @@
 /* eslint-disable @typescript-eslint/restrict-template-expressions */
 import { GlobalConfig } from '@n8n/config';
 import type express from 'express';
-import formidable from 'formidable';
 import get from 'lodash/get';
 import { BinaryDataService, NodeExecuteFunctions } from 'n8n-core';
 import type {
@@ -50,6 +49,7 @@ import { Logger } from '@/logger';
 import { parseBody } from '@/middlewares';
 import { OwnershipService } from '@/services/ownership.service';
 import { WorkflowStatisticsService } from '@/services/workflow-statistics.service';
+import { createMultiFormDataParser } from '@/webhooks/webhook-form-data';
 import * as WorkflowExecuteAdditionalData from '@/workflow-execute-additional-data';
 import * as WorkflowHelpers from '@/workflow-helpers';
 import { WorkflowRunner } from '@/workflow-runner';
@@ -92,14 +92,8 @@ export function getWorkflowWebhooks(
 	return returnData;
 }
 
-const normalizeFormData = <T>(values: Record<string, T | T[]>) => {
+const { formDataFileSizeMax } = Container.get(GlobalConfig).endpoints;
-	for (const key in values) {
+const parseFormData = createMultiFormDataParser(formDataFileSizeMax);
-		const value = values[key];
-		if (Array.isArray(value) && value.length === 1) {
-			values[key] = value[0];
-		}
-	}
-};
 
 /**
  * Executes a webhook
@@ -213,22 +207,9 @@ export async function executeWebhook(
 		// if `Webhook` or `Wait` node, and binaryData is enabled, skip pre-parse the request-body
 		// always falsy for versions higher than 1
 		if (!binaryData) {
-			const { contentType, encoding } = req;
+			const { contentType } = req;
 			if (contentType === 'multipart/form-data') {
-				const { formDataFileSizeMax } = Container.get(GlobalConfig).endpoints;
+				req.body = await parseFormData(req);
-				const form = formidable({
-					multiples: true,
-					encoding: encoding as formidable.BufferEncoding,
-					maxFileSize: formDataFileSizeMax * 1024 * 1024,
-					// TODO: pass a custom `fileWriteStreamHandler` to create binary data files directly
-				});
-				req.body = await new Promise((resolve) => {
-					form.parse(req, async (_err, data, files) => {
-						normalizeFormData(data);
-						normalizeFormData(files);
-						resolve({ data, files });
-					});
-				});
 			} else {
 				if (nodeVersion > 1) {
 					if (