From f1309787b2582cc27347070626567d27668b58c0 Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Fri, 20 Sep 2024 16:46:20 +0300 Subject: [PATCH] refactor(core): Extract form data parsing from webhook-helpers (#10904) --- .../__tests__/webhook-form-data.test.ts | 185 ++++++++++++++++++ .../cli/src/webhooks/webhook-form-data.ts | 38 ++++ packages/cli/src/webhooks/webhook-helpers.ts | 29 +-- 3 files changed, 228 insertions(+), 24 deletions(-) create mode 100644 packages/cli/src/webhooks/__tests__/webhook-form-data.test.ts create mode 100644 packages/cli/src/webhooks/webhook-form-data.ts diff --git a/packages/cli/src/webhooks/__tests__/webhook-form-data.test.ts b/packages/cli/src/webhooks/__tests__/webhook-form-data.test.ts new file mode 100644 index 0000000000..1b43143d38 --- /dev/null +++ b/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 = 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) { + 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(); + }); + }); +}); diff --git a/packages/cli/src/webhooks/webhook-form-data.ts b/packages/cli/src/webhooks/webhook-form-data.ts new file mode 100644 index 0000000000..75e1134020 --- /dev/null +++ b/packages/cli/src/webhooks/webhook-form-data.ts @@ -0,0 +1,38 @@ +import formidable from 'formidable'; +import type { IncomingMessage } from 'http'; + +const normalizeFormData = (values: Record) => { + 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 }); + }); + }); + }; +}; diff --git a/packages/cli/src/webhooks/webhook-helpers.ts b/packages/cli/src/webhooks/webhook-helpers.ts index 1827fe7ed3..c2c3b59bff 100644 --- a/packages/cli/src/webhooks/webhook-helpers.ts +++ b/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 = (values: Record) => { - for (const key in values) { - const value = values[key]; - if (Array.isArray(value) && value.length === 1) { - values[key] = value[0]; - } - } -}; +const { formDataFileSizeMax } = Container.get(GlobalConfig).endpoints; +const parseFormData = createMultiFormDataParser(formDataFileSizeMax); /** * 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; - 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 }); - }); - }); + req.body = await parseFormData(req); } else { if (nodeVersion > 1) { if (