From 10c15874b25aa7a7508c93934a683997418fc9d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Fri, 11 Aug 2023 09:18:33 +0200 Subject: [PATCH] fix(core): Restore body parsing for all content-types for non webhook routes (no-changelog)(#6911) Changes in https://github.com/n8n-io/n8n/pull/6394 removed xml body parsing for all non-webhook routes. This broken SAML endpoints as they need the XML body parser to function correctly. --- packages/cli/src/AbstractServer.ts | 8 ++-- packages/cli/src/WebhookHelpers.ts | 34 +------------- packages/cli/src/middlewares/bodyParser.ts | 47 ++++++++++++++----- .../integration/shared/utils/testServer.ts | 6 +-- 4 files changed, 43 insertions(+), 52 deletions(-) diff --git a/packages/cli/src/AbstractServer.ts b/packages/cli/src/AbstractServer.ts index 01031e4de8..a865a4f80e 100644 --- a/packages/cli/src/AbstractServer.ts +++ b/packages/cli/src/AbstractServer.ts @@ -11,7 +11,7 @@ import { N8nInstanceType } from '@/Interfaces'; import type { IExternalHooksClass } from '@/Interfaces'; import { ExternalHooks } from '@/ExternalHooks'; import { send, sendErrorResponse, ServiceUnavailableError } from '@/ResponseHelper'; -import { rawBody, jsonParser, corsMiddleware } from '@/middlewares'; +import { rawBodyReader, bodyParser, corsMiddleware } from '@/middlewares'; import { TestWebhooks } from '@/TestWebhooks'; import { WaitingWebhooks } from '@/WaitingWebhooks'; import { webhookRequestHandler } from '@/WebhookHelpers'; @@ -98,7 +98,7 @@ export abstract class AbstractServer { this.app.use(compression()); // Read incoming data into `rawBody` - this.app.use(rawBody); + this.app.use(rawBodyReader); } private setupDevMiddlewares() { @@ -274,8 +274,8 @@ export abstract class AbstractServer { this.setupDevMiddlewares(); } - // Setup JSON parsing middleware after the webhook handlers are setup - this.app.use(jsonParser); + // Setup body parsing middleware after the webhook handlers are setup + this.app.use(bodyParser); await this.configure(); diff --git a/packages/cli/src/WebhookHelpers.ts b/packages/cli/src/WebhookHelpers.ts index 6e37b35b13..2eb083e36b 100644 --- a/packages/cli/src/WebhookHelpers.ts +++ b/packages/cli/src/WebhookHelpers.ts @@ -12,8 +12,6 @@ import { Container } from 'typedi'; import get from 'lodash/get'; import stream from 'stream'; import { promisify } from 'util'; -import { parse as parseQueryString } from 'querystring'; -import { Parser as XmlParser } from 'xml2js'; import formidable from 'formidable'; import { BinaryDataManager, NodeExecuteFunctions } from 'n8n-core'; @@ -40,7 +38,6 @@ import { BINARY_ENCODING, createDeferredPromise, ErrorReporterProxy as ErrorReporter, - jsonParse, LoggerProxy as Logger, NodeHelpers, } from 'n8n-workflow'; @@ -64,6 +61,7 @@ import type { User } from '@db/entities/User'; import type { WorkflowEntity } from '@db/entities/WorkflowEntity'; import { EventsService } from '@/services/events.service'; import { OwnershipService } from './services/ownership.service'; +import { parseBody } from './middlewares'; const pipeline = promisify(stream.pipeline); @@ -76,13 +74,6 @@ export const WEBHOOK_METHODS: IHttpRequestMethods[] = [ 'PUT', ]; -const xmlParser = new XmlParser({ - async: true, - normalize: true, // Trim whitespace inside text nodes - normalizeTags: true, // Transform tags to lowercase - explicitArray: false, // Only put properties in array if length > 1 -}); - export const webhookRequestHandler = (webhookManager: IWebhookManager) => async (req: WebhookRequest | WebhookCORSRequest, res: express.Response) => { @@ -316,28 +307,7 @@ export async function executeWebhook( }); }); } else { - await req.readRawBody(); - const { rawBody } = req; - if (rawBody?.length) { - try { - if (contentType === 'application/json') { - req.body = jsonParse(rawBody.toString(encoding)); - } else if (contentType?.endsWith('/xml') || contentType?.endsWith('+xml')) { - req.body = await xmlParser.parseStringPromise(rawBody.toString(encoding)); - } else if (contentType === 'application/x-www-form-urlencoded') { - req.body = parseQueryString(rawBody.toString(encoding), undefined, undefined, { - maxKeys: 1000, - }); - } else if (contentType === 'text/plain') { - req.body = rawBody.toString(encoding); - } - } catch (error) { - throw new ResponseHelper.UnprocessableRequestError( - 'Failed to parse request body', - error.message, - ); - } - } + await parseBody(req); } } diff --git a/packages/cli/src/middlewares/bodyParser.ts b/packages/cli/src/middlewares/bodyParser.ts index a8c5c1b970..676c371258 100644 --- a/packages/cli/src/middlewares/bodyParser.ts +++ b/packages/cli/src/middlewares/bodyParser.ts @@ -1,12 +1,22 @@ import { parse as parseContentDisposition } from 'content-disposition'; import { parse as parseContentType } from 'content-type'; import getRawBody from 'raw-body'; -import { type RequestHandler } from 'express'; +import type { Request, RequestHandler } from 'express'; +import { parse as parseQueryString } from 'querystring'; +import { Parser as XmlParser } from 'xml2js'; import { jsonParse } from 'n8n-workflow'; import config from '@/config'; +import { UnprocessableRequestError } from '@/ResponseHelper'; + +const xmlParser = new XmlParser({ + async: true, + normalize: true, // Trim whitespace inside text nodes + normalizeTags: true, // Transform tags to lowercase + explicitArray: false, // Only put properties in array if length > 1 +}); const payloadSizeMax = config.getEnv('endpoints.payloadSizeMax'); -export const rawBody: RequestHandler = async (req, res, next) => { +export const rawBodyReader: RequestHandler = async (req, res, next) => { if ('content-type' in req.headers) { const { type: contentType, parameters } = (() => { try { @@ -41,21 +51,32 @@ export const rawBody: RequestHandler = async (req, res, next) => { next(); }; -export const jsonParser: RequestHandler = async (req, res, next) => { +export const parseBody = async (req: Request) => { await req.readRawBody(); - - if (Buffer.isBuffer(req.rawBody)) { - if (req.contentType === 'application/json') { - try { - req.body = jsonParse(req.rawBody.toString(req.encoding)); - } catch (error) { - res.status(400).send({ error: 'Failed to parse request body' }); - return; + const { rawBody, contentType, encoding } = req; + if (rawBody?.length) { + try { + if (contentType === 'application/json') { + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + req.body = jsonParse(rawBody.toString(encoding)); + } else if (contentType?.endsWith('/xml') || contentType?.endsWith('+xml')) { + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + req.body = await xmlParser.parseStringPromise(rawBody.toString(encoding)); + } else if (contentType === 'application/x-www-form-urlencoded') { + req.body = parseQueryString(rawBody.toString(encoding), undefined, undefined, { + maxKeys: 1000, + }); + } else if (contentType === 'text/plain') { + req.body = rawBody.toString(encoding); } - } else { - req.body = {}; + } catch (error) { + throw new UnprocessableRequestError('Failed to parse request body', (error as Error).message); } } +}; +export const bodyParser: RequestHandler = async (req, res, next) => { + await parseBody(req); + if (!req.body) req.body = {}; next(); }; diff --git a/packages/cli/test/integration/shared/utils/testServer.ts b/packages/cli/test/integration/shared/utils/testServer.ts index 5ef7f89264..c96422698e 100644 --- a/packages/cli/test/integration/shared/utils/testServer.ts +++ b/packages/cli/test/integration/shared/utils/testServer.ts @@ -30,7 +30,7 @@ import { TagsController, UsersController, } from '@/controllers'; -import { rawBody, jsonParser, setupAuthMiddlewares } from '@/middlewares'; +import { rawBodyReader, bodyParser, setupAuthMiddlewares } from '@/middlewares'; import { InternalHooks } from '@/InternalHooks'; import { LoadNodesAndCredentials } from '@/LoadNodesAndCredentials'; @@ -118,7 +118,7 @@ export const setupTestServer = ({ enabledFeatures, }: SetupProps): TestServer => { const app = express(); - app.use(rawBody); + app.use(rawBodyReader); app.use(cookieParser()); const testServer: TestServer = { @@ -153,7 +153,7 @@ export const setupTestServer = ({ if (!endpointGroups) return; - app.use(jsonParser); + app.use(bodyParser); const [routerEndpoints, functionEndpoints] = classifyEndpointGroups(endpointGroups);