From b6edfa5225c2e95bc83fdbb58b88c1e66bae4566 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 2 Jul 2026 16:22:42 +0100 Subject: [PATCH] github-notifier: Restore MAX_BODY_LENGTH and raise from 200 kB to 1 MiB Follows-up 0e9aa90774 which introduced MAX_BODY_LENGTH, and fec6bd9898 which removed it again due to being too low for static site deployments where small changes often change all files and thus produce fairly large GitHub Push eventsm because they are essentially a JSON wrapper around `git show` with a full file listing. GitHub Docs promise to limit the 'added', 'removed', and 'changed' arrays to 5000 file paths each (if more, the array is capped, in favor of recommending you call their API for the full details instead), or 25 MB overall (event is dropped by them if still larger than that). Note that we don't need any of this information, but there's no way to opt-out of this afaik. Note that we did not actually remove the limit in that patch in practice, because we run it with Nginx in front, and that limits the request body to 1MiB. That has been large enough so reflect that in the Node.js service directly as well, to benefit other potential users of the package. Credit to Quarkslab for the discovery and recommended mitigation. Ref https://github.com/jquery/infrastructure/issues/565. --- github-notifier.js | 17 +++++++++++++++++ test/server.js | 21 +++++++++++++++++++++ test/util.js | 13 +++++++++++++ 3 files changed, 51 insertions(+) diff --git a/github-notifier.js b/github-notifier.js index 738e42c..a8e3c32 100644 --- a/github-notifier.js +++ b/github-notifier.js @@ -2,6 +2,8 @@ const crypto = require('crypto'); const util = require('util'); const EventEmitter2 = require('eventemitter2').EventEmitter2; +const MAX_BODY_LENGTH = 1024 * 1024; // 1 MiB + function Notifier (config = {}, allowInsecure = false) { EventEmitter2.call(this, { wildcard: true, @@ -41,10 +43,25 @@ Notifier.prototype.handler = function (request, response) { let body = ''; request.on('data', function onData (chunk) { body += chunk; + + if (body.length > MAX_BODY_LENGTH) { + body = ''; + // SECURITY: Prevent crash/DOS through OOM by limiting the body buffer we receive. + // + // If you need to raise this, remember to also raise Nginx client_max_body_size. + // + // HTTP 413 Payload Too Large + response.writeHead(413); + response.end(); + + request.off('data', onData); + request.destroy(); + } }); request.on('end', function onEnd () { // Accept the request and close the connection + // // SECURITY: We decide on and close the response regardless of, // and prior to, any secret-based signature validation, so as to not // expose details about the outcome or timing of it to external clients. diff --git a/test/server.js b/test/server.js index 3b5771e..ec553bb 100644 --- a/test/server.js +++ b/test/server.js @@ -349,4 +349,25 @@ cccccccc: Done!` assert.strictEqual(resp.statusCode, 202, 'status code'); }, 500); }); + + QUnit.test('notifier ignores too large json', async assert => { + let called = 0; + function subscriber (notifier) { + notifier.on('example/test/push/heads/main', function () { + called++; + }); + } + util.writeExportedJs(tmpDir, 'example.js', subscriber); + + const server = await startServer({ insecure: true }); + const address = `http://localhost:${server.address().port}`; + const resp = await util.request(address, util.mocks.badLargeJson); + + const done = assert.async(); + setTimeout(() => { + done(); + assert.strictEqual(called, 0); + assert.strictEqual(resp.statusCode, 413, 'status code'); + }, 500); + }); }); diff --git a/test/util.js b/test/util.js index 6ffbc35..286bdc3 100644 --- a/test/util.js +++ b/test/util.js @@ -163,6 +163,19 @@ module.exports = { deleted: false } }, + badLargeJson: { + method: 'POST', + headers: { + 'content-type': 'application/json', + 'X-GitHub-Event': 'push' + }, + body: { + a: 'x'.repeat(512 * 1024), + b: 'x'.repeat(512 * 1024), + c: 'x'.repeat(512 * 1024), + d: 'x'.repeat(512 * 1024) + } + }, badInvalidJson: { method: 'POST', headers: {