From df92af56dc7b86abfaab9cd9fcb474872854d651 Mon Sep 17 00:00:00 2001 From: Ralf Anton Beier Date: Fri, 1 May 2026 08:06:58 +0200 Subject: [PATCH] fix: fail fast when WEBHOOK_SECRET is unset or equals "development" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Why Probot defaults its webhook secret to the literal string `"development"` when `WEBHOOK_SECRET` is unset. Without it, the bot **silently accepts forged webhooks signed with that trivially-known string** — a complete trust-boundary collapse on a misconfigured deploy. Wave-1 Security auditor flagged this as Bug #2 in `docs/agent-fleet/bugs.md`, the single highest-impact trust issue in the codebase. ## What New exported `assertWebhookSecret(env = process.env)` in `src/app.js`. Throws `Error` when `WEBHOOK_SECRET`: - is missing or empty - is whitespace-only - equals the literal string `"development"` (exact match — substrings like `"development-secret-9f2c"` are accepted, since they could be legitimate in a dev environment) `index.js` calls it after `mapLegacyEnvVars()` (so legacy `GITHUB_WEBHOOK_SECRET → WEBHOOK_SECRET` mapping happens first) and before `run(registerApp)`. The existing `try/catch` produces the standard `Error starting Probot:` console output and exits non-zero. `.env.example` updated with a comment explaining the constraint. ## Source Wave-1 Security auditor (Bug #2, `docs/agent-fleet/bugs.md`). ## Test plan - [x] 813 tests pass (was 806; +7 covering missing / empty / whitespace / exact-`development` / substring-`development*` / valid secret / reads-from-process.env-by-default) - [x] eslint clean - [ ] After deploy: stop pm2, unset `WEBHOOK_SECRET` in `/opt/temper/.env`, `pm2 start temper`. Bot should fail to start with a clear message instead of silently running with the fail-open default. ## Risk & rollout - Risk: low for any deployment that already has a real secret configured. Will fail-fast for misconfigured deploys, which is the point. - Rollout: self-update on merge. Operator must verify the env var is set before the bot tries to restart — the current netcup deploy already has it set. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) --- .env.example | 5 +++ __tests__/unit/webhook-secret.test.js | 53 +++++++++++++++++++++++++++ index.js | 6 ++- src/app.js | 37 +++++++++++++++++++ 4 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 __tests__/unit/webhook-secret.test.js diff --git a/.env.example b/.env.example index a237851..861faf5 100644 --- a/.env.example +++ b/.env.example @@ -1,6 +1,11 @@ # Required: GitHub App credentials APP_ID= PRIVATE_KEY= + +# Required. Temper refuses to start if this is unset or equals the literal +# string "development" (Probot's fail-open default — without a real secret, +# forged webhooks signed with "development" would be silently accepted). +# Generate with: openssl rand -hex 32 WEBHOOK_SECRET= # Optional: Target organization (also configurable in config.yml) diff --git a/__tests__/unit/webhook-secret.test.js b/__tests__/unit/webhook-secret.test.js new file mode 100644 index 0000000..eb1d552 --- /dev/null +++ b/__tests__/unit/webhook-secret.test.js @@ -0,0 +1,53 @@ +jest.mock('../../src/logger.js', () => { + const log = { info: jest.fn(), warn: jest.fn(), error: jest.fn() }; + return { getLogger: () => log, setLogger: jest.fn() }; +}); + +import { assertWebhookSecret } from '../../src/app.js'; + +describe('assertWebhookSecret (Bug #2 — trust-boundary fail-fast)', () => { + it('throws when WEBHOOK_SECRET is missing', () => { + expect(() => assertWebhookSecret({})).toThrow(/required/i); + }); + + it('throws when WEBHOOK_SECRET is the empty string', () => { + expect(() => assertWebhookSecret({ WEBHOOK_SECRET: '' })).toThrow(/required/i); + }); + + it('throws when WEBHOOK_SECRET is whitespace-only', () => { + expect(() => assertWebhookSecret({ WEBHOOK_SECRET: ' ' })).toThrow(/required/i); + }); + + it('throws when WEBHOOK_SECRET equals the literal "development"', () => { + expect(() => assertWebhookSecret({ WEBHOOK_SECRET: 'development' })).toThrow( + /"development"/ + ); + }); + + it('does NOT throw when WEBHOOK_SECRET merely contains "development" as substring', () => { + // Exact-match only — a real secret may legitimately have the word. + expect(() => + assertWebhookSecret({ WEBHOOK_SECRET: 'development-secret-9f2c' }) + ).not.toThrow(); + expect(() => + assertWebhookSecret({ WEBHOOK_SECRET: 'predev-elopment' }) + ).not.toThrow(); + }); + + it('passes for a non-trivial secret', () => { + expect(() => + assertWebhookSecret({ WEBHOOK_SECRET: 'a1b2c3d4e5f6g7h8i9j0' }) + ).not.toThrow(); + }); + + it('reads from process.env by default', () => { + const original = process.env.WEBHOOK_SECRET; + process.env.WEBHOOK_SECRET = 'real-secret-here'; + try { + expect(() => assertWebhookSecret()).not.toThrow(); + } finally { + if (original === undefined) delete process.env.WEBHOOK_SECRET; + else process.env.WEBHOOK_SECRET = original; + } + }); +}); diff --git a/index.js b/index.js index 0c32faf..4835a9e 100644 --- a/index.js +++ b/index.js @@ -1,6 +1,6 @@ import { fileURLToPath } from 'url'; import { run } from 'probot'; -import { registerApp, mapLegacyEnvVars } from './src/app.js'; +import { registerApp, mapLegacyEnvVars, assertWebhookSecret } from './src/app.js'; import { configureRepository } from './src/repository.js'; import { applyBranchProtection } from './src/branch-protection.js'; import { applyTemplates, applyCodeowners } from './src/templates.js'; @@ -29,6 +29,10 @@ if (process.argv[1] === fileURLToPath(import.meta.url)) { (async () => { try { mapLegacyEnvVars(); + // Trust-boundary check: refuse to boot with a missing or + // "development" webhook secret. See Bug #2 in + // docs/agent-fleet/bugs.md (wave-1 Security auditor). + assertWebhookSecret(); console.log('Starting Temper...'); console.log(`Environment: ${process.env.NODE_ENV || 'development'}`); console.log(`Port: ${process.env.PORT || 3000}`); diff --git a/src/app.js b/src/app.js index dbe9bdf..db1446d 100644 --- a/src/app.js +++ b/src/app.js @@ -1133,6 +1133,42 @@ function initScheduler(app) { return { store: _taskStore, scheduler: _scheduler }; } +/** + * Fail fast at startup when WEBHOOK_SECRET is unset, empty, or equals the + * literal string "development". + * + * Without this, Probot defaults the webhook secret to "development" when the + * env var is missing — silently accepting forged webhooks signed with that + * trivially-known string. The wave-1 security auditor flagged this as the + * single highest-impact trust-boundary fail-open in the bot. (Bug #2 in + * docs/agent-fleet/bugs.md.) + * + * Exported so it can be unit-tested without booting Probot. Throws Error so + * the caller's existing try/catch in index.js produces the standard + * "Error starting Probot:" message. + * + * Call this AFTER mapLegacyEnvVars so legacy GITHUB_WEBHOOK_SECRET → WEBHOOK_SECRET + * mapping is honoured before validation. + */ +function assertWebhookSecret(env = process.env) { + const secret = env.WEBHOOK_SECRET; + if (!secret || secret.trim() === '') { + throw new Error( + 'WEBHOOK_SECRET environment variable is required. ' + + 'Without it Probot defaults to the literal string "development", ' + + 'silently accepting forged webhooks. Set it in .env or via your ' + + 'process supervisor.' + ); + } + if (secret === 'development') { + throw new Error( + 'WEBHOOK_SECRET cannot equal the literal string "development" — ' + + 'that is Probot\'s fail-open default. Set a real secret (e.g. ' + + '`openssl rand -hex 32`).' + ); + } +} + function mapLegacyEnvVars() { if (process.env.GITHUB_APP_ID && !process.env.APP_ID) { process.env.APP_ID = process.env.GITHUB_APP_ID; @@ -1151,6 +1187,7 @@ function mapLegacyEnvVars() { export { registerApp, mapLegacyEnvVars, + assertWebhookSecret, createCustomRoutesHandler, applySecurityHeaders, initScheduler,