From 73695f1e46169ce4ec3f6ca8226e53f1092f7235 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Mon, 1 Jun 2026 01:03:46 +0100 Subject: [PATCH] Make WEBHOOK_SECRET mandatory unless `--insecure` is set Reduce chances of misconfiguration by explicilty refusing to start without a secret for payload verification, unless it is explicitly disabled. Credit to Quarkslab for the discovery and recommended mitigation. Ref https://github.com/jquery/infrastructure/issues/526 Ref https://github.com/jquery/infrastructure/issues/565 --- github-notifier.js | 13 ++++++++++++- notifier-server.js | 14 +++++++++++++- test/server.js | 34 ++++++++++++++++++---------------- 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/github-notifier.js b/github-notifier.js index 78c436f..b7f0b69 100644 --- a/github-notifier.js +++ b/github-notifier.js @@ -2,7 +2,7 @@ const crypto = require('crypto'); const util = require('util'); const EventEmitter2 = require('eventemitter2').EventEmitter2; -function Notifier (config = {}) { +function Notifier (config = {}, allowInsecure = false) { EventEmitter2.call(this, { wildcard: true, delimiter: '/' @@ -10,6 +10,12 @@ function Notifier (config = {}) { this.webhookSecret = config.webhookSecret || ''; + // SECURITY: Server requires a secret unless explicitly started with --insecure + // This cannot be set via config.json, only via CLI argument. + if (this.webhookSecret === '' && allowInsecure === true) { + this.allowInsecure = true; + } + // Pre-bind to ease usage as a callback this.handler = this.handler.bind(this); } @@ -65,6 +71,11 @@ Notifier.prototype.process = function (req, payload) { // Invalid signature, discard unauthorized event return; } + } else if (!this.allowInsecure) { + // SECURITY: Ignore events if server started without secret, unless --insecure set. + // Server should have refused to start, double check here just in case. + this.emit('error', 'Missing a secret while --insecure not set.'); + return; } const eventType = req.headers['x-github-event']; diff --git a/notifier-server.js b/notifier-server.js index 4cdb2c0..f87cee3 100644 --- a/notifier-server.js +++ b/notifier-server.js @@ -69,6 +69,7 @@ function makeExec (directory, filename, log) { * @param {number} opts.port * @param {string} opts.directory * @param {boolean} [opts.debug=false] + * @param {boolean} [opts.insecure=false] * @param {Function} [opts.logFn] * @return {Promise} */ @@ -88,6 +89,16 @@ function start (opts) { config.webhookSecret = process.env.WEBHOOK_SECRET; } + // SECURITY: Refuse to start without payload verification, unless --insecure set + // This cannot be set via config.json, only via CLI argument. + if (!config.webhookSecret && !opts.insecure) { + // Fail closed + throw new Error('Missing a secret via WEBHOOK_SECRET or config.json to verify payloads.'); + } + if (config.webhookSecret && opts.insecure) { + throw new Error('Refusing to start with both a secret and --insecure set.'); + } + // Limits: // * Timeout: 5s max socket inactivity . // * Headers timeout: 60s in total [default] . @@ -96,7 +107,7 @@ function start (opts) { const server = http.createServer(); server.timeout = 5000; - const notifier = new Notifier(config); + const notifier = new Notifier(config, opts.insecure); const error = makeLogger(logFn, 'notifier-server:error'); const log = opts.debug ? makeLogger(logFn, 'notifier-server:debug') : function () {}; @@ -159,6 +170,7 @@ function cli () { path.join(__dirname, 'notifier.d') ) .option('--debug', 'enable verbose logging') + .option('--insecure', 'disable secure verification of payloads (for testing purposes)') // --help is included by default // The parse() method will exit early for help or invalid arg error. .parse(process.argv); diff --git a/test/server.js b/test/server.js index 9653928..3b5771e 100644 --- a/test/server.js +++ b/test/server.js @@ -21,6 +21,8 @@ QUnit.module('notifier-server', hooks => { hooks.beforeEach(() => { tmpDir = util.getTmpDir(); + + delete process.env.WEBHOOK_SECRET; }); hooks.afterEach(() => { @@ -37,14 +39,14 @@ QUnit.module('notifier-server', hooks => { delete process.env.WEBHOOK_SECRET; }); - async function startServer () { - const server = await notifier.start({ directory: tmpDir, port: 0, debug: true, logFn: logFn }); + async function startServer (opt = {}) { + const server = await notifier.start({ directory: tmpDir, port: 0, debug: true, logFn: logFn, ...opt }); servers.push(server); return server; } QUnit.test('start server', async assert => { - const server = await startServer(); + const server = await startServer({ insecure: true }); assert.strictEqual(typeof server.address().port, 'number'); }); @@ -56,7 +58,7 @@ QUnit.module('notifier-server', hooks => { util.writeExportedJs(tmpDir, 'example.js', subscriber); assert.strictEqual(called, 0); - await startServer(); + await startServer({ insecure: true }); assert.strictEqual(called, 1); }); @@ -76,7 +78,7 @@ QUnit.module('notifier-server', hooks => { } util.writeExportedJs(tmpDir, 'example.js', subscriber); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; const resp = await util.request(address, util.mocks.examplePushBranch); @@ -94,7 +96,7 @@ QUnit.module('notifier-server', hooks => { } util.writeExportedJs(tmpDir, 'example.js', subscriber); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; const resp = await util.request(address, util.mocks.examplePushTag); @@ -112,7 +114,7 @@ QUnit.module('notifier-server', hooks => { } util.writeExportedJs(tmpDir, 'example.js', subscriber); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; process.env.WEBHOOK_SECRET = util.mocks.securePushTagSigned.secret; const resp = await util.request(address, util.mocks.securePushTagSigned); @@ -129,7 +131,7 @@ QUnit.module('notifier-server', hooks => { echo "Received arg $1" > "${tmpDir}/example.out"; `); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; util.request(address, util.mocks.examplePushBranch); @@ -164,7 +166,7 @@ sleep 0.1 echo "$SELF: Done!" >> "$OUT" `); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; util.request(address, util.mocks.examplePushA); util.request(address, util.mocks.examplePushB); @@ -207,9 +209,9 @@ cccccccc: Done!` } util.writeExportedJs(tmpDir, 'example.js', subscriber); + process.env.WEBHOOK_SECRET = util.mocks.securePushTagUnsigned.secret; const server = await startServer(); const address = `http://localhost:${server.address().port}`; - process.env.WEBHOOK_SECRET = util.mocks.securePushTagUnsigned.secret; const resp = await util.request(address, util.mocks.securePushTagUnsigned); const done = assert.async(); @@ -229,9 +231,9 @@ cccccccc: Done!` } util.writeExportedJs(tmpDir, 'example.js', subscriber); + process.env.WEBHOOK_SECRET = util.mocks.securePushTagBadlySigned.secret; const server = await startServer(); const address = `http://localhost:${server.address().port}`; - process.env.WEBHOOK_SECRET = util.mocks.securePushTagBadlySigned.secret; util.request(address, util.mocks.securePushTagBadlySigned); const done = assert.async(); @@ -250,7 +252,7 @@ cccccccc: Done!` } util.writeExportedJs(tmpDir, 'example.js', subscriber); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; util.request(address, util.mocks.examplePushTag); @@ -270,7 +272,7 @@ cccccccc: Done!` } util.writeExportedJs(tmpDir, 'example.js', subscriber); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; util.request(address, util.mocks.examplePushBranch); @@ -290,7 +292,7 @@ cccccccc: Done!` } util.writeExportedJs(tmpDir, 'example.js', subscriber); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; util.request(address, util.mocks.examplePushBranch); @@ -310,7 +312,7 @@ cccccccc: Done!` } util.writeExportedJs(tmpDir, 'example.js', subscriber); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; const resp = await util.request(address, util.mocks.examplePing); @@ -335,7 +337,7 @@ cccccccc: Done!` } util.writeExportedJs(tmpDir, 'example.js', subscriber); - const server = await startServer(); + const server = await startServer({ insecure: true }); const address = `http://localhost:${server.address().port}`; const resp = await util.request(address, util.mocks.badInvalidJson);