diff --git a/.env.example b/.env.example index ba0d234..77461e6 100644 --- a/.env.example +++ b/.env.example @@ -52,6 +52,12 @@ ADMIN_BOOTSTRAP_EMAIL= # request shares ONE rate-limit bucket (/sign-in/* allows 3 per 10 seconds, so # one noisy client locks everyone out). Check with: # SELECT `key` FROM rateLimit; -- a "no-trusted-ip" row means it is happening. +# The header the reverse proxy puts the real client IP in. Must be one the proxy +# actually overwrites - trusting a header it does not set lets any client send its +# own value and bypass the sign-in rate limit entirely. +# Set to "none" to trust no header at all: every request then shares one rate-limit +# bucket, which is the safe fallback if the check below fails. Verify after deploy +# with: SELECT ipAddress FROM session ORDER BY createdAt DESC LIMIT 3; CLIENT_IP_HEADERS=x-real-ip TRUSTED_PROXY_IPS= diff --git a/src/app.factory.ts b/src/app.factory.ts index 7acca08..6b7154d 100644 --- a/src/app.factory.ts +++ b/src/app.factory.ts @@ -4,6 +4,7 @@ import swaggerUi from 'swagger-ui-express'; import swaggerJSDoc from 'swagger-jsdoc'; import cors from 'cors'; import {toNodeHandler} from 'better-auth/node'; +import logger from './middleware/logger.js'; // Router imports import {calendarRouter} from './models/calendar/Calendar.router.js'; @@ -91,7 +92,26 @@ export const createApp = (): express.Application => { // better-auth's own handler, mounted before express.json(): it reads the raw // request body stream itself and a parsed body would leave it hanging. - app.all('/admin/auth/*', toNodeHandler(auth)); + // + // Wrapped, because Express 4 does not await an async handler: a rejected + // promise escapes as an unhandled rejection instead of becoming a response. + // Nearly every better-auth route touches the admin database, so a database + // blip would leave the request hanging with no answer at all while the + // process logged an uncaughtException - observed by pointing ADMIN_DB at a + // database the user cannot open. Answer 503 instead: the caller learns, and + // the other domains keep serving. + const authHandler = toNodeHandler(auth); + app.all('/admin/auth/*', (req, res) => { + Promise.resolve(authHandler(req, res)).catch((e: any) => { + logger.error('Admin auth handler failed', {path: req.path, detail: e?.message}); + if (!res.headersSent) { + res.status(503).send({ + status: 'SERVICE_UNAVAILABLE', + message: 'Die Anmeldung ist derzeit nicht verfügbar. Bitte versuche es später erneut.' + }); + } + }); + }); // here we are adding middleware to parse all incoming requests as JSON app.use(express.json()); diff --git a/src/models/admin/admin.config.ts b/src/models/admin/admin.config.ts index 4ea4c80..5317ace 100644 --- a/src/models/admin/admin.config.ts +++ b/src/models/admin/admin.config.ts @@ -101,16 +101,53 @@ export const ADMIN_ALLOWED_ORIGINS = Array.from(new Set([ * brute-force budget - so the default is the single header nginx sets, not a * permissive list. */ -export const CLIENT_IP_HEADERS = parseList(process.env.CLIENT_IP_HEADERS, ['x-real-ip']); + +/** + * `CLIENT_IP_HEADERS=none` trusts no header at all. + * + * This is the escape hatch for the one case where the wrong setting is worse + * than no setting: if the proxy turns out NOT to overwrite the header we are + * trusting, any client can send it and mint itself an unlimited brute-force + * budget against /sign-in. Falling back to the shared bucket is bad (one noisy + * client can lock the organisation out for ten seconds at a time) but it is + * bad in a way that fails closed, and it can be reverted from the environment + * without a deploy. + * + * Reach for it only after a check has actually failed - `SELECT ipAddress FROM + * session ORDER BY createdAt DESC` showing 127.0.0.1 or NULL for a real remote + * sign-in - and take it back out once the header is configured. + * + * An empty or unset value still means "use the default", not "trust nothing": + * a stray blank line in a .env must not silently change how requests are + * bucketed. Only the explicit word does that. + */ +const TRUST_NO_HEADER = 'none'; + +export const TRUST_NO_CLIENT_IP_HEADER = + (process.env.CLIENT_IP_HEADERS || '').trim().toLowerCase() === TRUST_NO_HEADER; + +// An empty array is what better-auth reads as "no headers": it only falls back +// to its own default when the option is absent, and `[]` is truthy. +export const CLIENT_IP_HEADERS = TRUST_NO_CLIENT_IP_HEADER + ? [] + : parseList(process.env.CLIENT_IP_HEADERS, ['x-real-ip']); export const TRUSTED_PROXY_IPS = parseList(process.env.TRUSTED_PROXY_IPS, []); -if (isProd && TRUSTED_PROXY_IPS.length === 0) { +if (isProd && TRUST_NO_CLIENT_IP_HEADER) { + logger.warn( + 'Admin module: CLIENT_IP_HEADERS=none - no client-IP header is trusted, so every ' + + 'request shares one rate-limit bucket and /sign-in allows 3 attempts per 10 seconds ' + + 'for everyone combined. This is the safe fallback, not a destination: configure the ' + + 'header the proxy actually sets and remove it.' + ); +} else if (isProd && TRUSTED_PROXY_IPS.length === 0) { logger.warn( 'Admin module: TRUSTED_PROXY_IPS is not set. If the proxy sends a multi-value ' + `${CLIENT_IP_HEADERS.join('/')}, better-auth cannot resolve a client IP and every ` + 'request shares one rate-limit bucket. Verify with: SELECT `key` FROM rateLimit - ' + - 'a "no-trusted-ip" row means this is happening.' + 'a "no-trusted-ip" row means this is happening. A single-value header needs no ' + + 'trusted proxies, so this warning is expected on a plain single-proxy setup.' ); } diff --git a/test/admin/admin.config.test.ts b/test/admin/admin.config.test.ts new file mode 100644 index 0000000..c30073f --- /dev/null +++ b/test/admin/admin.config.test.ts @@ -0,0 +1,109 @@ +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; + +// admin.config calls dotenv.config(), which would read the repo's own .env and +// quietly reintroduce NODE_ENV=development - the exact value several of these +// cases exist to remove. Stub it so the tests see only what they set. +vi.mock('dotenv', () => ({config: vi.fn()})); + +/** + * admin.config reads the environment once at import, so every case here has to + * reset the module registry and re-import it. The two things worth pinning are + * the ones that are silent when wrong: which client-IP header is trusted, and + * whether an unset NODE_ENV counts as production. + */ + +const ORIGINAL_ENV = {...process.env}; + +const loadConfig = async () => { + vi.resetModules(); + return import('../../src/models/admin/admin.config.js'); +}; + +beforeEach(() => { + process.env = {...ORIGINAL_ENV}; + // dotenv.config() in admin.config does not overwrite what is already set, + // so setting these here is enough to keep the local .env out of the test. + process.env.NODE_ENV = 'test'; + delete process.env.CLIENT_IP_HEADERS; + delete process.env.TRUSTED_PROXY_IPS; +}); + +afterEach(() => { + process.env = {...ORIGINAL_ENV}; +}); + +describe('CLIENT_IP_HEADERS', () => { + it('defaults to the single header Plesk nginx sets', async () => { + const config = await loadConfig(); + expect(config.CLIENT_IP_HEADERS).toEqual(['x-real-ip']); + expect(config.TRUST_NO_CLIENT_IP_HEADER).toBe(false); + }); + + it('reads a comma-separated list', async () => { + process.env.CLIENT_IP_HEADERS = 'x-real-ip, cf-connecting-ip'; + const config = await loadConfig(); + expect(config.CLIENT_IP_HEADERS).toEqual(['x-real-ip', 'cf-connecting-ip']); + }); + + it('trusts nothing when set to "none"', async () => { + // The escape hatch. An empty list is what better-auth reads as "no + // headers" - it only falls back to its own default when the option is + // absent - so this really does stop any header being believed. + process.env.CLIENT_IP_HEADERS = 'none'; + const config = await loadConfig(); + expect(config.CLIENT_IP_HEADERS).toEqual([]); + expect(config.TRUST_NO_CLIENT_IP_HEADER).toBe(true); + }); + + it('accepts the hatch case-insensitively and with stray whitespace', async () => { + process.env.CLIENT_IP_HEADERS = ' NONE '; + const config = await loadConfig(); + expect(config.CLIENT_IP_HEADERS).toEqual([]); + }); + + it('treats an empty value as "use the default", not as the hatch', async () => { + // A blank line in a .env must not silently change how requests are + // bucketed - only the explicit word does that. + process.env.CLIENT_IP_HEADERS = ''; + const config = await loadConfig(); + expect(config.CLIENT_IP_HEADERS).toEqual(['x-real-ip']); + expect(config.TRUST_NO_CLIENT_IP_HEADER).toBe(false); + }); + + it('does not mistake a header actually named none-ish for the hatch', async () => { + process.env.CLIENT_IP_HEADERS = 'x-none'; + const config = await loadConfig(); + expect(config.CLIENT_IP_HEADERS).toEqual(['x-none']); + expect(config.TRUST_NO_CLIENT_IP_HEADER).toBe(false); + }); +}); + +describe('isProd', () => { + it('is false only for the explicit relaxed environments', async () => { + process.env.NODE_ENV = 'development'; + expect((await loadConfig()).isProd).toBe(false); + + process.env.NODE_ENV = 'test'; + expect((await loadConfig()).isProd).toBe(false); + }); + + it('treats an unset NODE_ENV as production, which is what a bare vhost gives', async () => { + delete process.env.NODE_ENV; + // Strict mode refuses to boot without these; supply them so the import + // gets far enough to answer the question being asked. + process.env.BETTER_AUTH_SECRET = 'x'.repeat(48); + process.env.API_BASE_URL = 'https://api.nachklang.art'; + process.env.ADMIN_APP_URL = 'https://admin.nachklang.art'; + + expect((await loadConfig()).isProd).toBe(true); + }); + + it('refuses to start without a signing key outside development', async () => { + delete process.env.NODE_ENV; + delete process.env.BETTER_AUTH_SECRET; + process.env.API_BASE_URL = 'https://api.nachklang.art'; + process.env.ADMIN_APP_URL = 'https://admin.nachklang.art'; + + await expect(loadConfig()).rejects.toThrow(/BETTER_AUTH_SECRET/); + }); +});