From 5df943095db12ec2083337cd2c0d1db1096a5a61 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 4 Jul 2026 17:35:07 -0500 Subject: [PATCH] Isolate internal bot-config route from the public listener (#33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GET /internal/bot-config route returns the DECRYPTED Discord bot token and was mounted on the same Express app / port 3000 that Pangolin proxies publicly. Its only guard was the BOT_INTERNAL_KEY shared secret, and .env.example shipped a placeholder default — so a forwarded path or a weak/unrotated key would expose the plaintext token to the internet. Move server<->bot internal traffic onto its own listener and fail fast on a weak key: - Add server/src/internalApp.js: a standalone Express app mounting requireInternalKey + /internal (and a no-secret /health), mirroring the bot's unpublished port-4100 pattern. - server.js starts a second listener on INTERNAL_PORT (default 3001), closed on graceful shutdown. - Remove the /internal mount from the public v1.router; the public app now 404s /api/v1/internal/bot-config even with a valid key. - Fail fast: new utils/botInternalKey.js rejects an empty, placeholder, or <16-char BOT_INTERNAL_KEY — fatal in production (exit 1), warning in dev. - docker-compose: bot SITE_INTERNAL_URL -> app:3001/internal/bot-config; document that INTERNAL_PORT stays unpublished. - .env.example (root/server/bot): document INTERNAL_PORT, the fail-fast behavior, and a defense-in-depth Pangolin deny rule for /api/v1/internal. Tests: add requireInternalKey.test.js and botInternalKey.test.js (node --test: 93 pass). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_019rao86n5cXpwAyjdBFEshV --- .env.example | 15 ++++- bot/.env.example | 7 +- docker-compose.yml | 8 ++- server/.env.example | 11 +++- server/src/internalApp.js | 26 ++++++++ .../src/router/v1/internal/internal.routes.js | 3 +- server/src/router/v1/v1.router.js | 8 +-- server/src/server.js | 34 +++++++++- server/src/utils/botInternalKey.js | 43 ++++++++++++ server/test/botInternalKey.test.js | 55 ++++++++++++++++ server/test/requireInternalKey.test.js | 66 +++++++++++++++++++ 11 files changed, 261 insertions(+), 15 deletions(-) create mode 100644 server/src/internalApp.js create mode 100644 server/src/utils/botInternalKey.js create mode 100644 server/test/botInternalKey.test.js create mode 100644 server/test/requireInternalKey.test.js diff --git a/.env.example b/.env.example index 33c3b84..416a131 100644 --- a/.env.example +++ b/.env.example @@ -4,6 +4,10 @@ # App NODE_ENV=production PORT=3000 +# Separate, UNPUBLISHED port for server<->bot internal traffic (the decrypted +# bot-token route). Must match the port in the bot's SITE_INTERNAL_URL +# (docker-compose.yml) and must NEVER be published/proxied. See issue #33. +INTERNAL_PORT=3001 UPLOAD_DIR=/app/uploads # Logging — written to BOTH the console and a log file. LOG_LEVEL=info # console verbosity: error | warn | info | debug @@ -64,7 +68,14 @@ CLIENT_ORIGIN=http://localhost:5173 # service). BOT_INTERNAL_KEY MUST be byte-for-byte identical to the same # variable in bot/.env.example — it is the only auth on both sides' /internal/* # routes, so a mismatch silently breaks every server<->bot call with 401s. -# The Discord bot TOKEN itself is not an env var — it's entered in the admin -# panel (Discord Bot page) and stored encrypted in the DB (see bot_config table). +# It also guards the server's /internal/bot-config route, which returns the +# DECRYPTED Discord token; with NODE_ENV=production the app REFUSES TO START if +# this is left blank, at this placeholder, or shorter than 16 chars. Generate a +# long random string. The Discord bot TOKEN itself is not an env var — it's +# entered in the admin panel (Discord Bot page) and stored encrypted in the DB. +# +# Defense in depth: even with a strong key, configure Pangolin/your reverse +# proxy to DENY /api/v1/internal (and never forward INTERNAL_PORT). The route no +# longer rides the public listener, but an explicit deny rule is belt-and-braces. BOT_INTERNAL_URL=http://bot:4100 BOT_INTERNAL_KEY=change-me-to-a-long-random-string diff --git a/bot/.env.example b/bot/.env.example index ac7d5e9..8cd4a01 100644 --- a/bot/.env.example +++ b/bot/.env.example @@ -24,9 +24,10 @@ LOG_TO_FILE=true # set false for console-only BOT_INTERNAL_KEY=dev-only-change-me-bot-key # Where this bot calls back to the main site to fetch its config on boot -# (GET .../api/v1/internal/bot-config), so a restart self-reconnects without -# needing the admin panel to push config again. -SITE_INTERNAL_URL=http://localhost:3000/api/v1/internal/bot-config +# (GET .../internal/bot-config), so a restart self-reconnects without needing +# the admin panel to push config again. This targets the site's UNPUBLISHED +# internal port (INTERNAL_PORT, default 3001) — NOT the public 3000. See #33. +SITE_INTERNAL_URL=http://localhost:3001/internal/bot-config # Read-only PUBLIC API base (Phase 7) — no shared secret, same data any # visitor's browser can fetch. Used by /wiki (search) and /announce diff --git a/docker-compose.yml b/docker-compose.yml index f16de16..dd9ba50 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -35,6 +35,10 @@ services: - uploads:/app/uploads # Bind-mount logs to the host so app.log is directly readable at ./logs/ - ./logs:/app/logs + # Only the PUBLIC API port (3000) is published. The internal server<->bot + # port (INTERNAL_PORT, default 3001) is deliberately NOT listed here, so it + # stays reachable only over the private compose network — Pangolin/the public + # reverse proxy can never forward to it. See issue #33. # Binds 0.0.0.0 (no 127.0.0.1 prefix) so Pangolin can reach the container. ports: - "3000:3000" @@ -47,7 +51,9 @@ services: env_file: .env environment: DB_HOST: db - SITE_INTERNAL_URL: http://app:3000/api/v1/internal/bot-config + # Internal config fetch goes to the app's UNPUBLISHED internal port (3001), + # not the public 3000. Keep the port in sync with the app's INTERNAL_PORT. + SITE_INTERNAL_URL: http://app:3001/internal/bot-config SITE_PUBLIC_URL: http://app:3000/api/v1/public LOG_DIR: /app/bot/logs depends_on: diff --git a/server/.env.example b/server/.env.example index c5930d9..44c4ffb 100644 --- a/server/.env.example +++ b/server/.env.example @@ -4,6 +4,10 @@ NODE_ENV=development PORT=3000 +# Separate, unpublished port for server<->bot internal traffic (the decrypted +# bot-token route). Must match the port in bot/.env's SITE_INTERNAL_URL and must +# never be exposed through a public reverse proxy. See issue #33. +INTERNAL_PORT=3001 # Logging — written to BOTH the console and a log file (default /logs/app.log). LOG_LEVEL=debug # console verbosity: error | warn | info | debug FILE_LOG_LEVEL=debug # file verbosity @@ -77,8 +81,11 @@ CLIENT_ORIGIN=http://localhost:5173 # Discord bot — internal API (server <-> bot/). BOT_INTERNAL_KEY MUST be # byte-for-byte identical to the same variable in bot/.env.example — it is the # only auth on both sides' /internal/* routes, so a mismatch silently breaks -# every server<->bot call with 401s. The Discord bot TOKEN itself is not an env -# var — it's entered in the admin panel (Discord Bot page) and stored +# every server<->bot call with 401s. It also guards the server's +# /internal/bot-config route, which returns the DECRYPTED Discord token: with +# NODE_ENV=production the app REFUSES TO START if this is blank, a documented +# placeholder, or shorter than 16 chars (a warning only in dev). The Discord bot +# TOKEN itself is not an env var — it's entered in the admin panel and stored # encrypted in the DB (see the bot_config table / SECRET_ENC_KEY above). BOT_INTERNAL_URL=http://localhost:4100 BOT_INTERNAL_KEY=dev-only-change-me-bot-key diff --git a/server/src/internalApp.js b/server/src/internalApp.js new file mode 100644 index 0000000..7e9cd39 --- /dev/null +++ b/server/src/internalApp.js @@ -0,0 +1,26 @@ +// Standalone Express app for server<->bot internal traffic. It is mounted on its +// OWN http listener (INTERNAL_PORT, default 3001) in server.js — an unpublished, +// compose-network-only port, mirroring how the bot exposes its internal API on +// 4100. Crucially it is NOT part of the public API app (app.js), so /internal/* +// (which returns the DECRYPTED Discord bot token) can never ride the same +// listener Pangolin proxies to the world. Shared-secret gated by +// requireInternalKey inside internal.routes. See issue #33. +const express = require('express') + +const internalRouter = require('./router/v1/internal/internal.routes') + +const internalApp = express() + +internalApp.use(express.json()) + +// Liveness probe for this listener (no secret required); mirrors the bot's +// /health on 4100. Useful for compose healthchecks without exposing anything. +internalApp.get('/health', (req, res) => res.json({ status: 'ok' })) + +// requireInternalKey is applied inside internal.routes. +internalApp.use('/internal', internalRouter) + +// Anything else on this listener is not a real internal route. +internalApp.use((req, res) => res.status(404).json({ message: 'Not found' })) + +module.exports = internalApp diff --git a/server/src/router/v1/internal/internal.routes.js b/server/src/router/v1/internal/internal.routes.js index 6a0d1cb..9fa4cfc 100644 --- a/server/src/router/v1/internal/internal.routes.js +++ b/server/src/router/v1/internal/internal.routes.js @@ -6,7 +6,8 @@ const ctrl = require('./internal.controller') const router = express.Router() // Shared-secret gated, not session-gated — the caller is the bot process, not -// a logged-in browser. Mounted before any auth/session middleware in v1.router. +// a logged-in browser. This router is mounted on the standalone internalApp +// (its own unpublished port), never on the public /api app. See internalApp.js. router.use(requireInternalKey) router.get( diff --git a/server/src/router/v1/v1.router.js b/server/src/router/v1/v1.router.js index ac6dac6..73af4ef 100644 --- a/server/src/router/v1/v1.router.js +++ b/server/src/router/v1/v1.router.js @@ -5,13 +5,13 @@ const v1Router = express.Router() const authRouter = require('./auth/auth.routes') const publicRouter = require('./public/public.routes') const adminRouter = require('./admin/admin.routes') -const internalRouter = require('./internal/internal.routes') v1Router.use('/auth', authRouter) v1Router.use('/public', publicRouter) v1Router.use('/admin', adminRouter) -// Shared-secret gated (not session-gated) — server<->bot only, never exposed -// through the public reverse proxy. See server/src/middleware/requireInternalKey.js. -v1Router.use('/internal', internalRouter) +// NOTE: /internal is intentionally NOT mounted here. Those routes return the +// decrypted Discord bot token and must never share the public listener that +// Pangolin proxies. They live on a separate, unpublished port via +// server/src/internalApp.js (started in server.js). See issue #33. module.exports = v1Router diff --git a/server/src/server.js b/server/src/server.js index 5b7b898..c61821b 100644 --- a/server/src/server.js +++ b/server/src/server.js @@ -2,16 +2,23 @@ require('dotenv').config() const http = require('http') const app = require('./app') +const internalApp = require('./internalApp') const botScore = require('./middleware/botScore') const { ensureSchema, close } = require('./utils/db') const { seedDefaults, createInitialAdminFromEnv } = require('../db/seed') const settings = require('./model/settings/settings.model') const mailer = require('./utils/mailer') const createLogger = require('./utils/logger') +const { evaluateBotInternalKey } = require('./utils/botInternalKey') const pkg = require('../package.json') const log = createLogger('server') const PORT = Number(process.env.PORT) || 3000 +// Separate, UNPUBLISHED listener for server<->bot /internal/* traffic. Kept off +// the public PORT so the decrypted-token route can't ride the listener Pangolin +// proxies to the world (issue #33). Must match the port in the bot's +// SITE_INTERNAL_URL (docker-compose.yml). +const INTERNAL_PORT = Number(process.env.INTERNAL_PORT) || 3001 const HOST = '0.0.0.0' // bind all interfaces so Pangolin / the LAN can reach it async function start() { @@ -25,6 +32,20 @@ async function start() { smtp: mailer.isConfigured() ? 'configured' : 'not configured (mailto fallback)', }) + // Fail fast if the server<->bot shared secret is weak/placeholder. Fatal in + // production (the /internal/bot-config route hands back the decrypted Discord + // token and this key is its only guard); a warning otherwise. + const keyCheck = evaluateBotInternalKey({ + key: process.env.BOT_INTERNAL_KEY, + nodeEnv: process.env.NODE_ENV, + }) + if (keyCheck.fatal) { + log.error(keyCheck.message) + process.exit(1) + } else if (!keyCheck.ok) { + log.warn(keyCheck.message) + } + log.info('ensuring database schema...') await ensureSchema() log.info('seeding defaults...') @@ -39,10 +60,18 @@ async function start() { log.info(`listening on http://${HOST}:${PORT} (API at /api/v1, health at /api/health)`) }) - setupShutdown(server) + // Internal server<->bot API on a separate, unpublished port. NEVER expose this + // through Pangolin/the public reverse proxy — it serves the decrypted Discord + // bot token to the bot process over the private compose network only (#33). + const internalServer = http.createServer(internalApp) + internalServer.listen(INTERNAL_PORT, HOST, () => { + log.info(`internal API listening on http://${HOST}:${INTERNAL_PORT} (server<->bot only — do NOT proxy)`) + }) + + setupShutdown(server, internalServer) } -function setupShutdown(server) { +function setupShutdown(server, internalServer) { let closing = false const shutdown = async (signal) => { if (closing) return @@ -50,6 +79,7 @@ function setupShutdown(server) { log.warn(`${signal} received — shutting down gracefully`) botScore.stopSweeper() // stop the bot-store cleanup interval server.close(() => log.info('http server closed')) + if (internalServer) internalServer.close(() => log.info('internal http server closed')) try { await close() log.info('database pool closed') diff --git a/server/src/utils/botInternalKey.js b/server/src/utils/botInternalKey.js new file mode 100644 index 0000000..9d3ecfa --- /dev/null +++ b/server/src/utils/botInternalKey.js @@ -0,0 +1,43 @@ +// Boot-time validation of BOT_INTERNAL_KEY — the shared secret that is the ONLY +// auth on the server<->bot /internal/* routes (which return the decrypted Discord +// bot token). A missing, placeholder, or trivially short key would leave that +// endpoint effectively unguarded, so in production we refuse to start; in dev we +// warn but continue so local work isn't blocked. See issue #33. + +// The placeholders shipped in the repo's .env.example files. If any of these +// reaches production it means the operator never generated a real key. +const PLACEHOLDERS = new Set([ + 'change-me-to-a-long-random-string', // root .env.example + 'dev-only-change-me-bot-key', // server/.env.example, bot/.env.example +]) + +const MIN_LENGTH = 16 + +// Returns { ok, fatal, message }. `fatal` is only ever true in production — +// callers should exit non-zero on fatal, and log a warning (but continue) when +// !ok && !fatal. +function evaluateBotInternalKey({ key, nodeEnv } = {}) { + const value = key || '' + + let reason = null + if (value.length === 0) reason = 'BOT_INTERNAL_KEY is not set' + else if (PLACEHOLDERS.has(value)) reason = 'BOT_INTERNAL_KEY is still the documented placeholder value' + else if (value.length < MIN_LENGTH) reason = `BOT_INTERNAL_KEY is too short (< ${MIN_LENGTH} chars)` + + if (!reason) return { ok: true, fatal: false, message: null } + + const production = nodeEnv === 'production' + const detail = + `${reason}. It is the only guard on /internal/bot-config, which returns the ` + + 'decrypted Discord bot token.' + + return { + ok: false, + fatal: production, + message: production + ? `${detail} Refusing to start in production — set a long random BOT_INTERNAL_KEY (matching bot/.env).` + : `${detail} Continuing because NODE_ENV is not "production" — set a strong value before deploying.`, + } +} + +module.exports = { evaluateBotInternalKey, PLACEHOLDERS, MIN_LENGTH } diff --git a/server/test/botInternalKey.test.js b/server/test/botInternalKey.test.js new file mode 100644 index 0000000..02c47d0 --- /dev/null +++ b/server/test/botInternalKey.test.js @@ -0,0 +1,55 @@ +const { test } = require('node:test') +const assert = require('node:assert/strict') + +const { evaluateBotInternalKey, MIN_LENGTH } = require('../src/utils/botInternalKey') + +const STRONG = 'x'.repeat(MIN_LENGTH + 8) + +test('a strong key is ok in every environment', () => { + for (const nodeEnv of ['production', 'development', undefined]) { + const r = evaluateBotInternalKey({ key: STRONG, nodeEnv }) + assert.equal(r.ok, true) + assert.equal(r.fatal, false) + assert.equal(r.message, null) + } +}) + +test('empty key is fatal in production, warn otherwise', () => { + const prod = evaluateBotInternalKey({ key: '', nodeEnv: 'production' }) + assert.equal(prod.ok, false) + assert.equal(prod.fatal, true) + + const dev = evaluateBotInternalKey({ key: '', nodeEnv: 'development' }) + assert.equal(dev.ok, false) + assert.equal(dev.fatal, false) +}) + +test('undefined key behaves like empty', () => { + const r = evaluateBotInternalKey({ key: undefined, nodeEnv: 'production' }) + assert.equal(r.ok, false) + assert.equal(r.fatal, true) +}) + +test('documented placeholders are rejected (fatal in production)', () => { + for (const key of ['change-me-to-a-long-random-string', 'dev-only-change-me-bot-key']) { + const r = evaluateBotInternalKey({ key, nodeEnv: 'production' }) + assert.equal(r.ok, false, `placeholder should be rejected: ${key}`) + assert.equal(r.fatal, true) + } +}) + +test('a too-short key is rejected', () => { + const short = 'a'.repeat(MIN_LENGTH - 1) + const r = evaluateBotInternalKey({ key: short, nodeEnv: 'production' }) + assert.equal(r.ok, false) + assert.equal(r.fatal, true) + + const dev = evaluateBotInternalKey({ key: short, nodeEnv: 'development' }) + assert.equal(dev.ok, false) + assert.equal(dev.fatal, false) +}) + +test('a key exactly MIN_LENGTH long is accepted', () => { + const r = evaluateBotInternalKey({ key: 'a'.repeat(MIN_LENGTH), nodeEnv: 'production' }) + assert.equal(r.ok, true) +}) diff --git a/server/test/requireInternalKey.test.js b/server/test/requireInternalKey.test.js new file mode 100644 index 0000000..cc63247 --- /dev/null +++ b/server/test/requireInternalKey.test.js @@ -0,0 +1,66 @@ +const { test } = require('node:test') +const assert = require('node:assert/strict') + +const requireInternalKey = require('../src/middleware/requireInternalKey') +const { startApp } = require('./_helper') + +const KEY = 'test-internal-key-1234567890' + +async function withApp(run) { + const prev = process.env.BOT_INTERNAL_KEY + process.env.BOT_INTERNAL_KEY = KEY + const app = await startApp((a) => { + a.get('/internal/thing', requireInternalKey, (req, res) => res.json({ ok: true })) + }) + try { + await run(app) + } finally { + await app.close() + if (prev === undefined) delete process.env.BOT_INTERNAL_KEY + else process.env.BOT_INTERNAL_KEY = prev + } +} + +test('requireInternalKey: 401 when no key header is sent', async () => { + await withApp(async (app) => { + const res = await fetch(`${app.url}/internal/thing`) + assert.equal(res.status, 401) + }) +}) + +test('requireInternalKey: 401 on a wrong key', async () => { + await withApp(async (app) => { + const res = await fetch(`${app.url}/internal/thing`, { + headers: { 'X-Internal-Key': 'nope' }, + }) + assert.equal(res.status, 401) + }) +}) + +test('requireInternalKey: 200 with the correct key', async () => { + await withApp(async (app) => { + const res = await fetch(`${app.url}/internal/thing`, { + headers: { 'X-Internal-Key': KEY }, + }) + assert.equal(res.status, 200) + const body = await res.json() + assert.deepEqual(body, { ok: true }) + }) +}) + +test('requireInternalKey: 401 when the expected key is empty (never a wildcard)', async () => { + const prev = process.env.BOT_INTERNAL_KEY + process.env.BOT_INTERNAL_KEY = '' + const app = await startApp((a) => { + a.get('/internal/thing', requireInternalKey, (req, res) => res.json({ ok: true })) + }) + try { + // Even sending an empty key must not match an empty expected key. + const res = await fetch(`${app.url}/internal/thing`, { headers: { 'X-Internal-Key': '' } }) + assert.equal(res.status, 401) + } finally { + await app.close() + if (prev === undefined) delete process.env.BOT_INTERNAL_KEY + else process.env.BOT_INTERNAL_KEY = prev + } +})