From 21c58dd012219847d91b7f14ca7f41e3677f6d14 Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Tue, 20 Oct 2026 14:20:22 -0700 Subject: [PATCH 1/2] Accept webhooks up to GitHub's 25 MB Every POST shared one 1 MB body limit, so a webhook for a big PR (a long body, many labels, a large push) got a 413 and its update was dropped until the hourly reconcile. Prod logged 47 of these on Oct 12-13 and 50 between Oct 16 and 20, about a dozen a day. The webhook route now parses up to GitHub's 25 MB limit; the board and the API keep 1 MB. The parsers move to lib/body-parsers.js so a test can mount them without starting the app. Co-Authored-By: Claude Opus 5.5 --- app.js | 6 ++---- lib/body-parsers.js | 23 +++++++++++++++++++++ test/body-parsers.test.js | 42 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 4 deletions(-) create mode 100644 lib/body-parsers.js create mode 100644 test/body-parsers.test.js diff --git a/app.js b/app.js index 00077a42..926c7769 100644 --- a/app.js +++ b/app.js @@ -1,6 +1,5 @@ import config from './lib/config-loader.js'; import express from 'express'; -import bodyParser from 'body-parser'; import expressSession from 'express-session'; import authManager from './lib/authentication.js'; import socketAuthenticator from './lib/socket-auth.js'; @@ -13,6 +12,7 @@ import { createRefreshAll } from './lib/refresh-all.js'; import { createPacer } from './lib/pacer.js'; import mainController from './controllers/main.js'; import hooksController from './controllers/githubHooks.js'; +import { useBodyParsers } from './lib/body-parsers.js'; import statsController from './controllers/stats.js'; import userNamesController from './controllers/user-names.js'; import projectsController from './controllers/projects.js'; @@ -52,7 +52,6 @@ const refreshAll = createRefreshAll({ const app = express(); const httpServer = createServer(app); -const maxPostSize = 1024 * 1024; const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); @@ -63,8 +62,7 @@ app.set('view engine', 'html'); * Middleware */ app.use('/public', express.static(__dirname + '/public')); -app.use(bodyParser.urlencoded({ limit: maxPostSize, extended: false })); -app.use(bodyParser.json({ limit: maxPostSize })); +useBodyParsers(app); app.use( expressSession({ secret: config.session.secret, diff --git a/lib/body-parsers.js b/lib/body-parsers.js new file mode 100644 index 00000000..1dbe4a52 --- /dev/null +++ b/lib/body-parsers.js @@ -0,0 +1,23 @@ +import bodyParser from 'body-parser'; + +// Everything the board and the API post is small. +const MAX_POST_SIZE = 1024 * 1024; +// GitHub sends webhook payloads up to 25 MB, and a big PR (a long body, many +// labels or a large push) passes 1 MB. Rejected with a 413, that update was +// lost until the hourly reconcile, about a dozen a day on prod. +const MAX_HOOK_SIZE = 25 * 1024 * 1024; + +/** + * Parse request bodies: the webhook route with GitHub's limit, everything + * else with the small one. The webhook parsers run first; body-parser skips a + * body that's already parsed, so the global ones leave hook requests alone. + */ +export function useBodyParsers(app) { + app.use( + '/hooks', + bodyParser.urlencoded({ limit: MAX_HOOK_SIZE, extended: false }), + bodyParser.json({ limit: MAX_HOOK_SIZE }) + ); + app.use(bodyParser.urlencoded({ limit: MAX_POST_SIZE, extended: false })); + app.use(bodyParser.json({ limit: MAX_POST_SIZE })); +} diff --git a/test/body-parsers.test.js b/test/body-parsers.test.js new file mode 100644 index 00000000..2442391b --- /dev/null +++ b/test/body-parsers.test.js @@ -0,0 +1,42 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import express from 'express'; +import { useBodyParsers } from '../lib/body-parsers.js'; + +// a JSON body just past the general 1 MB limit, the size of a big webhook +const big = JSON.stringify({ action: 'edited', body: 'x'.repeat(2 * 1024 * 1024) }); + +async function withServer(t) { + const app = express(); + useBodyParsers(app); + app.post('/hooks/main', (req, res) => res.json({ got: req.body.body.length })); + app.post('/roadmap', (req, res) => res.json({ got: req.body.body.length })); + const server = await new Promise(resolve => { + const s = app.listen(0, () => resolve(s)); + }); + t.after(() => server.close()); + return `http://127.0.0.1:${server.address().port}`; +} + +const post = (url, body, type) => + fetch(url, { method: 'POST', headers: { 'content-type': type }, body }); + +test('a webhook past 1 MB is parsed', async t => { + const base = await withServer(t); + const res = await post(`${base}/hooks/main`, big, 'application/json'); + assert.equal(res.status, 200); + assert.equal((await res.json()).got, 2 * 1024 * 1024); +}); + +test('a form-encoded webhook past 1 MB is parsed', async t => { + const base = await withServer(t); + const form = new URLSearchParams({ body: 'x'.repeat(2 * 1024 * 1024) }).toString(); + const res = await post(`${base}/hooks/main`, form, 'application/x-www-form-urlencoded'); + assert.equal(res.status, 200); +}); + +test('other routes keep the 1 MB limit', async t => { + const base = await withServer(t); + const res = await post(`${base}/roadmap`, big, 'application/json'); + assert.equal(res.status, 413); +}); From 7c46fa796a90ae091b801d5f8033415f5ffed9db Mon Sep 17 00:00:00 2001 From: Jarred Stelfox Date: Tue, 20 Oct 2026 14:30:49 -0700 Subject: [PATCH 2/2] Check the webhook secret before reading the body Review: the route is public, and the secret was checked only after parsing, so anyone could make the server buffer 25 MB per request. The secret is in the query string, so it can be checked first; a request without it gets the same 401 before any of its body is read. Co-Authored-By: Claude Opus 5.5 --- app.js | 2 +- lib/body-parsers.js | 6 +++++- test/body-parsers.test.js | 16 +++++++++++++--- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/app.js b/app.js index 926c7769..cace28fb 100644 --- a/app.js +++ b/app.js @@ -62,7 +62,7 @@ app.set('view engine', 'html'); * Middleware */ app.use('/public', express.static(__dirname + '/public')); -useBodyParsers(app); +useBodyParsers(app, config.github.hook_secret); app.use( expressSession({ secret: config.session.secret, diff --git a/lib/body-parsers.js b/lib/body-parsers.js index 1dbe4a52..52c14d65 100644 --- a/lib/body-parsers.js +++ b/lib/body-parsers.js @@ -11,10 +11,14 @@ const MAX_HOOK_SIZE = 25 * 1024 * 1024; * Parse request bodies: the webhook route with GitHub's limit, everything * else with the small one. The webhook parsers run first; body-parser skips a * body that's already parsed, so the global ones leave hook requests alone. + * The route is public (GitHub has to reach it), so the secret in its query is + * checked before reading up to 25 MB, not after. */ -export function useBodyParsers(app) { +export function useBodyParsers(app, hookSecret) { app.use( '/hooks', + (req, res, next) => + req.query.secret === hookSecret ? next() : res.status(401).send('Invalid POST'), bodyParser.urlencoded({ limit: MAX_HOOK_SIZE, extended: false }), bodyParser.json({ limit: MAX_HOOK_SIZE }) ); diff --git a/test/body-parsers.test.js b/test/body-parsers.test.js index 2442391b..a05b8b2b 100644 --- a/test/body-parsers.test.js +++ b/test/body-parsers.test.js @@ -8,7 +8,7 @@ const big = JSON.stringify({ action: 'edited', body: 'x'.repeat(2 * 1024 * 1024) async function withServer(t) { const app = express(); - useBodyParsers(app); + useBodyParsers(app, 's3cret'); app.post('/hooks/main', (req, res) => res.json({ got: req.body.body.length })); app.post('/roadmap', (req, res) => res.json({ got: req.body.body.length })); const server = await new Promise(resolve => { @@ -23,7 +23,7 @@ const post = (url, body, type) => test('a webhook past 1 MB is parsed', async t => { const base = await withServer(t); - const res = await post(`${base}/hooks/main`, big, 'application/json'); + const res = await post(`${base}/hooks/main?secret=s3cret`, big, 'application/json'); assert.equal(res.status, 200); assert.equal((await res.json()).got, 2 * 1024 * 1024); }); @@ -31,7 +31,11 @@ test('a webhook past 1 MB is parsed', async t => { test('a form-encoded webhook past 1 MB is parsed', async t => { const base = await withServer(t); const form = new URLSearchParams({ body: 'x'.repeat(2 * 1024 * 1024) }).toString(); - const res = await post(`${base}/hooks/main`, form, 'application/x-www-form-urlencoded'); + const res = await post( + `${base}/hooks/main?secret=s3cret`, + form, + 'application/x-www-form-urlencoded' + ); assert.equal(res.status, 200); }); @@ -40,3 +44,9 @@ test('other routes keep the 1 MB limit', async t => { const res = await post(`${base}/roadmap`, big, 'application/json'); assert.equal(res.status, 413); }); + +test('a webhook without the secret is refused before its body is read', async t => { + const base = await withServer(t); + const res = await post(`${base}/hooks/main?secret=wrong`, big, 'application/json'); + assert.equal(res.status, 401); +});