Skip to content

Commit 73695f1

Browse files
KrinkleKrinkle
committed
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 jquery/infrastructure#526 Ref jquery/infrastructure#565
1 parent 4f6b66b commit 73695f1

3 files changed

Lines changed: 43 additions & 18 deletions

File tree

‎github-notifier.js‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,20 @@ const crypto = require('crypto');
22
const util = require('util');
33
const EventEmitter2 = require('eventemitter2').EventEmitter2;
44

5-
function Notifier (config = {}) {
5+
function Notifier (config = {}, allowInsecure = false) {
66
EventEmitter2.call(this, {
77
wildcard: true,
88
delimiter: '/'
99
});
1010

1111
this.webhookSecret = config.webhookSecret || '';
1212

13+
// SECURITY: Server requires a secret unless explicitly started with --insecure
14+
// This cannot be set via config.json, only via CLI argument.
15+
if (this.webhookSecret === '' && allowInsecure === true) {
16+
this.allowInsecure = true;
17+
}
18+
1319
// Pre-bind to ease usage as a callback
1420
this.handler = this.handler.bind(this);
1521
}
@@ -65,6 +71,11 @@ Notifier.prototype.process = function (req, payload) {
6571
// Invalid signature, discard unauthorized event
6672
return;
6773
}
74+
} else if (!this.allowInsecure) {
75+
// SECURITY: Ignore events if server started without secret, unless --insecure set.
76+
// Server should have refused to start, double check here just in case.
77+
this.emit('error', 'Missing a secret while --insecure not set.');
78+
return;
6879
}
6980

7081
const eventType = req.headers['x-github-event'];

‎notifier-server.js‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ function makeExec (directory, filename, log) {
6969
* @param {number} opts.port
7070
* @param {string} opts.directory
7171
* @param {boolean} [opts.debug=false]
72+
* @param {boolean} [opts.insecure=false]
7273
* @param {Function} [opts.logFn]
7374
* @return {Promise<http.Server>}
7475
*/
@@ -88,6 +89,16 @@ function start (opts) {
8889
config.webhookSecret = process.env.WEBHOOK_SECRET;
8990
}
9091

92+
// SECURITY: Refuse to start without payload verification, unless --insecure set
93+
// This cannot be set via config.json, only via CLI argument.
94+
if (!config.webhookSecret && !opts.insecure) {
95+
// Fail closed
96+
throw new Error('Missing a secret via WEBHOOK_SECRET or config.json to verify payloads.');
97+
}
98+
if (config.webhookSecret && opts.insecure) {
99+
throw new Error('Refusing to start with both a secret and --insecure set.');
100+
}
101+
91102
// Limits:
92103
// * Timeout: 5s max socket inactivity <https://nodejs.org/api/http.html#servertimeout>.
93104
// * Headers timeout: 60s in total [default] <https://nodejs.org/api/http.html#serverheaderstimeout>.
@@ -96,7 +107,7 @@ function start (opts) {
96107
const server = http.createServer();
97108
server.timeout = 5000;
98109

99-
const notifier = new Notifier(config);
110+
const notifier = new Notifier(config, opts.insecure);
100111

101112
const error = makeLogger(logFn, 'notifier-server:error');
102113
const log = opts.debug ? makeLogger(logFn, 'notifier-server:debug') : function () {};
@@ -159,6 +170,7 @@ function cli () {
159170
path.join(__dirname, 'notifier.d')
160171
)
161172
.option('--debug', 'enable verbose logging')
173+
.option('--insecure', 'disable secure verification of payloads (for testing purposes)')
162174
// --help is included by default
163175
// The parse() method will exit early for help or invalid arg error.
164176
.parse(process.argv);

‎test/server.js‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ QUnit.module('notifier-server', hooks => {
2121

2222
hooks.beforeEach(() => {
2323
tmpDir = util.getTmpDir();
24+
25+
delete process.env.WEBHOOK_SECRET;
2426
});
2527

2628
hooks.afterEach(() => {
@@ -37,14 +39,14 @@ QUnit.module('notifier-server', hooks => {
3739
delete process.env.WEBHOOK_SECRET;
3840
});
3941

40-
async function startServer () {
41-
const server = await notifier.start({ directory: tmpDir, port: 0, debug: true, logFn: logFn });
42+
async function startServer (opt = {}) {
43+
const server = await notifier.start({ directory: tmpDir, port: 0, debug: true, logFn: logFn, ...opt });
4244
servers.push(server);
4345
return server;
4446
}
4547

4648
QUnit.test('start server', async assert => {
47-
const server = await startServer();
49+
const server = await startServer({ insecure: true });
4850
assert.strictEqual(typeof server.address().port, 'number');
4951
});
5052

@@ -56,7 +58,7 @@ QUnit.module('notifier-server', hooks => {
5658
util.writeExportedJs(tmpDir, 'example.js', subscriber);
5759
assert.strictEqual(called, 0);
5860

59-
await startServer();
61+
await startServer({ insecure: true });
6062
assert.strictEqual(called, 1);
6163
});
6264

@@ -76,7 +78,7 @@ QUnit.module('notifier-server', hooks => {
7678
}
7779
util.writeExportedJs(tmpDir, 'example.js', subscriber);
7880

79-
const server = await startServer();
81+
const server = await startServer({ insecure: true });
8082
const address = `http://localhost:${server.address().port}`;
8183
const resp = await util.request(address, util.mocks.examplePushBranch);
8284

@@ -94,7 +96,7 @@ QUnit.module('notifier-server', hooks => {
9496
}
9597
util.writeExportedJs(tmpDir, 'example.js', subscriber);
9698

97-
const server = await startServer();
99+
const server = await startServer({ insecure: true });
98100
const address = `http://localhost:${server.address().port}`;
99101
const resp = await util.request(address, util.mocks.examplePushTag);
100102

@@ -112,7 +114,7 @@ QUnit.module('notifier-server', hooks => {
112114
}
113115
util.writeExportedJs(tmpDir, 'example.js', subscriber);
114116

115-
const server = await startServer();
117+
const server = await startServer({ insecure: true });
116118
const address = `http://localhost:${server.address().port}`;
117119
process.env.WEBHOOK_SECRET = util.mocks.securePushTagSigned.secret;
118120
const resp = await util.request(address, util.mocks.securePushTagSigned);
@@ -129,7 +131,7 @@ QUnit.module('notifier-server', hooks => {
129131
echo "Received arg $1" > "${tmpDir}/example.out";
130132
`);
131133

132-
const server = await startServer();
134+
const server = await startServer({ insecure: true });
133135
const address = `http://localhost:${server.address().port}`;
134136
util.request(address, util.mocks.examplePushBranch);
135137

@@ -164,7 +166,7 @@ sleep 0.1
164166
echo "$SELF: Done!" >> "$OUT"
165167
`);
166168

167-
const server = await startServer();
169+
const server = await startServer({ insecure: true });
168170
const address = `http://localhost:${server.address().port}`;
169171
util.request(address, util.mocks.examplePushA);
170172
util.request(address, util.mocks.examplePushB);
@@ -207,9 +209,9 @@ cccccccc: Done!`
207209
}
208210
util.writeExportedJs(tmpDir, 'example.js', subscriber);
209211

212+
process.env.WEBHOOK_SECRET = util.mocks.securePushTagUnsigned.secret;
210213
const server = await startServer();
211214
const address = `http://localhost:${server.address().port}`;
212-
process.env.WEBHOOK_SECRET = util.mocks.securePushTagUnsigned.secret;
213215
const resp = await util.request(address, util.mocks.securePushTagUnsigned);
214216

215217
const done = assert.async();
@@ -229,9 +231,9 @@ cccccccc: Done!`
229231
}
230232
util.writeExportedJs(tmpDir, 'example.js', subscriber);
231233

234+
process.env.WEBHOOK_SECRET = util.mocks.securePushTagBadlySigned.secret;
232235
const server = await startServer();
233236
const address = `http://localhost:${server.address().port}`;
234-
process.env.WEBHOOK_SECRET = util.mocks.securePushTagBadlySigned.secret;
235237
util.request(address, util.mocks.securePushTagBadlySigned);
236238

237239
const done = assert.async();
@@ -250,7 +252,7 @@ cccccccc: Done!`
250252
}
251253
util.writeExportedJs(tmpDir, 'example.js', subscriber);
252254

253-
const server = await startServer();
255+
const server = await startServer({ insecure: true });
254256
const address = `http://localhost:${server.address().port}`;
255257
util.request(address, util.mocks.examplePushTag);
256258

@@ -270,7 +272,7 @@ cccccccc: Done!`
270272
}
271273
util.writeExportedJs(tmpDir, 'example.js', subscriber);
272274

273-
const server = await startServer();
275+
const server = await startServer({ insecure: true });
274276
const address = `http://localhost:${server.address().port}`;
275277
util.request(address, util.mocks.examplePushBranch);
276278

@@ -290,7 +292,7 @@ cccccccc: Done!`
290292
}
291293
util.writeExportedJs(tmpDir, 'example.js', subscriber);
292294

293-
const server = await startServer();
295+
const server = await startServer({ insecure: true });
294296
const address = `http://localhost:${server.address().port}`;
295297
util.request(address, util.mocks.examplePushBranch);
296298

@@ -310,7 +312,7 @@ cccccccc: Done!`
310312
}
311313
util.writeExportedJs(tmpDir, 'example.js', subscriber);
312314

313-
const server = await startServer();
315+
const server = await startServer({ insecure: true });
314316
const address = `http://localhost:${server.address().port}`;
315317
const resp = await util.request(address, util.mocks.examplePing);
316318

@@ -335,7 +337,7 @@ cccccccc: Done!`
335337
}
336338
util.writeExportedJs(tmpDir, 'example.js', subscriber);
337339

338-
const server = await startServer();
340+
const server = await startServer({ insecure: true });
339341
const address = `http://localhost:${server.address().port}`;
340342
const resp = await util.request(address, util.mocks.badInvalidJson);
341343

0 commit comments

Comments
 (0)