Merge pull request 'Isolate internal bot-config route from the public listener (#33)' (#36) from bugfix/internal-token-endpoint-33 into main

Reviewed-on: UOM/website#36
Reviewed-by: Colby Whitlock <whitlocktech@gmail.com>

.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
+# It also guards the server's /internal/bot-config route, which returns the
-# panel (Discord Bot page) and stored encrypted in the DB (see bot_config table).
+# 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

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
+# (GET .../internal/bot-config), so a restart self-reconnects without needing
-# needing the admin panel to push config again.
+# the admin panel to push config again. This targets the site's UNPUBLISHED
-SITE_INTERNAL_URL=http://localhost:3000/api/v1/internal/bot-config
+# 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

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:

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 <server>/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
+# every server<->bot call with 401s. It also guards the server's
-# var — it's entered in the admin panel (Discord Bot page) and stored
+# /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

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

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(

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
+// NOTE: /internal is intentionally NOT mounted here. Those routes return the
-// through the public reverse proxy. See server/src/middleware/requireInternalKey.js.
+// decrypted Discord bot token and must never share the public listener that
-v1Router.use('/internal', internalRouter)
+// Pangolin proxies. They live on a separate, unpublished port via
+// server/src/internalApp.js (started in server.js). See issue #33.
 
 module.exports = v1Router

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')

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 }

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)
+})

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
+  }
+})