From e5c99903ea177955ae648d1292199398cf4d34b6 Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 10 Aug 2026 11:35:19 +0200 Subject: [PATCH] Pin the request body ceiling instead of inheriting it The Express shell set a 100kb limit and stopped applying it the moment the Nest instance took over parsing: bootstrap passed bodyParser:false and nothing replaced it. The number happened to stay the same, because it is also the framework default - but it lived nowhere anybody would look for it, and one NestFactory default away from silently becoming something else. It goes through Nest's own parser rather than a second express.json(), because that parser carries the verify hook that keeps req.rawBody, and the plugin webhook routes need the exact payload bytes to check a provider's HMAC. Adding a parser in front of it would have taken rawBody away without failing anything. MISC-009 and MISC-010 pin it from both sides. The bodyParser flag on applyGlobalMiddleware goes with it: one caller, which passed false. Not moved: cookieParser stays on the raw Express layer rather than becoming a configure(consumer) middleware. The platform routes - the OAuth consent flow among them - are registered on that instance before Nest routing exists, so a Nest-slot cookie parser would simply not run for them. helmet, CORS, compression, the forced-HTTPS 301 and the res.on('finish') logging stay for the same reason: they have to see requests Nest never routes. --- server/src/bootstrap.ts | 14 +++++++++++-- server/src/middleware/globalMiddleware.ts | 13 +++++------- server/tests/integration/misc.test.ts | 21 +++++++++++++++++++ .../middleware/globalMiddleware.csp.test.ts | 2 +- 4 files changed, 39 insertions(+), 11 deletions(-) diff --git a/server/src/bootstrap.ts b/server/src/bootstrap.ts index de81836c1..ebe7dd49e 100644 --- a/server/src/bootstrap.ts +++ b/server/src/bootstrap.ts @@ -1,5 +1,5 @@ import { NestFactory } from '@nestjs/core'; -import { ExpressAdapter } from '@nestjs/platform-express'; +import { ExpressAdapter, type NestExpressApplication } from '@nestjs/platform-express'; import type { INestApplication } from '@nestjs/common'; import type { ConfigType } from '@nestjs/config'; import { AppModule } from './nest/app.module'; @@ -54,10 +54,20 @@ export async function buildApp(): Promise { // the one bridge that lets the pre-init Express layer consume the validated // config instead of reading process.env itself. const http = app.get>(httpConfig.KEY); - applyGlobalMiddleware(instance, { bodyParser: false, http }); + applyGlobalMiddleware(instance, { http }); applyPlatformUploads(instance); applyPlatformTransport(instance); applyPlatformStatic(instance); + // Pin the request-body ceiling explicitly. The Express shell used to set + // '100kb' and stopped doing it when the Nest instance took over parsing, which + // left the limit implicit — the same number, but nowhere anybody would find + // it, and one NestFactory default away from silently becoming something else. + // Configured through Nest rather than a second express.json(): its parser + // carries the verify hook that keeps req.rawBody, which the plugin webhook + // routes need to check a provider's HMAC over the exact payload. + const express = app as unknown as NestExpressApplication; + express.useBodyParser('json', { limit: '100kb' }); + express.useBodyParser('urlencoded', { limit: '100kb', extended: true }); if (apiDocsEnabled()) setupApiDocs(app); await app.init(); // The /mcp handler is mounted pre-init (step 3) and has no DI access — hand diff --git a/server/src/middleware/globalMiddleware.ts b/server/src/middleware/globalMiddleware.ts index 0555dec3e..e5ee7611e 100644 --- a/server/src/middleware/globalMiddleware.ts +++ b/server/src/middleware/globalMiddleware.ts @@ -15,18 +15,19 @@ import { logDebug, logWarn, logError } from '../nest/audit/audit-log.logger'; * behaviourally transparent — and is the prerequisite for retiring Express, * since the Nest instance must carry the whole shell on its own. * - * `bodyParser` is opt-out: the Nest instance does its own body parsing, so it - * passes `false` to avoid parsing the request twice. + * Body parsing is NOT here: the Nest instance does it, with the limit pinned in + * bootstrap.ts. The flag that used to switch a second parser on had exactly one + * caller, which passed false. */ export function applyGlobalMiddleware( app: express.Application, - opts: { bodyParser?: boolean; http?: AppEnv['http'] } = {}, + opts: { http?: AppEnv['http'] } = {}, ): void { // The whole pipeline is configured at APPLY time (the per-request closures // capture these values), so a snapshot is the correct semantic. bootstrap // threads in the DI-loaded httpConfig; direct callers fall back to an // apply-time readEnv() — same values, same freeze point. - const { bodyParser = true, http = readEnv().http } = opts; + const { http = readEnv().http } = opts; const { nodeEnv, isProduction } = readEnv().app; // Trust first proxy (nginx/Docker) for correct req.ip @@ -169,10 +170,6 @@ export function applyGlobalMiddleware( }); } - if (bodyParser) { - app.use(express.json({ limit: '100kb' })); - app.use(express.urlencoded({ extended: true })); - } app.use(cookieParser()); // Request logging with sensitive field redaction diff --git a/server/tests/integration/misc.test.ts b/server/tests/integration/misc.test.ts index 7966c2d8c..65898bc1f 100644 --- a/server/tests/integration/misc.test.ts +++ b/server/tests/integration/misc.test.ts @@ -145,3 +145,24 @@ describe('Force HTTPS redirect', () => { }); }); + +describe('Request body ceiling', () => { + // The Express shell set '100kb' and stopped when the Nest instance took over + // parsing, which left the limit implicit. This pins it, so it cannot drift + // back to a framework default without somebody noticing. + it('MISC-009 — a body over 100kb is refused with 413', async () => { + const res = await request(app) + .post('/api/auth/login') + .set('Content-Type', 'application/json') + .send({ email: 'a@b.c', password: 'x'.repeat(200 * 1024) }); + expect(res.status).toBe(413); + }); + + it('MISC-010 — a body under it still reaches the handler', async () => { + const res = await request(app) + .post('/api/auth/login') + .set('Content-Type', 'application/json') + .send({ email: 'a@b.c', password: 'x'.repeat(1024) }); + expect(res.status).not.toBe(413); + }); +}); diff --git a/server/tests/unit/middleware/globalMiddleware.csp.test.ts b/server/tests/unit/middleware/globalMiddleware.csp.test.ts index effaa754c..07b179b59 100644 --- a/server/tests/unit/middleware/globalMiddleware.csp.test.ts +++ b/server/tests/unit/middleware/globalMiddleware.csp.test.ts @@ -5,7 +5,7 @@ import { applyGlobalMiddleware } from '../../../src/middleware/globalMiddleware' async function connectSrcSources(): Promise { const app = express(); - applyGlobalMiddleware(app, { bodyParser: false }); + applyGlobalMiddleware(app); app.get('/probe', (_req, res) => res.json({ ok: true })); const res = await request(app).get('/probe');