Skip to content

Make WEBHOOK_SECRET mandatory unless --insecure is set #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Krinkle wants to merge 1 commit into
base: main
Choose a base branch
from
+43 −18
Open

Make WEBHOOK_SECRET mandatory unless --insecure is set #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion github-notifier.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,20 @@ 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: '/'
});

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);
}
Expand Down Expand Up @@ -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'];
Expand Down
14 changes: 13 additions & 1 deletion notifier-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<http.Server>}
*/
Expand All @@ -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 <https://nodejs.org/api/http.html#servertimeout>.
// * Headers timeout: 60s in total [default] <https://nodejs.org/api/http.html#serverheaderstimeout>.
Expand All @@ -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 () {};
Expand Down Expand Up @@ -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);
Expand Down
34 changes: 18 additions & 16 deletions test/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ QUnit.module('notifier-server', hooks => {

hooks.beforeEach(() => {
tmpDir = util.getTmpDir();

delete process.env.WEBHOOK_SECRET;
});

hooks.afterEach(() => {
Expand All @@ -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');
});

Expand All @@ -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);
});

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);
Expand All @@ -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);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand Down
Loading