diff --git a/app.js b/app.js index 00077a42..cace28fb 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, config.github.hook_secret); 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..52c14d65 --- /dev/null +++ b/lib/body-parsers.js @@ -0,0 +1,27 @@ +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. + * 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, 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 }) + ); + 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..a05b8b2b --- /dev/null +++ b/test/body-parsers.test.js @@ -0,0 +1,52 @@ +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, '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 => { + 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?secret=s3cret`, 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?secret=s3cret`, + 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); +}); + +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); +});