Add a CLIENT_IP_HEADERS escape hatch and stop the auth handler hanging
Two changes, both from working out how to verify the client-IP configuration on the Plesk vhost before deploying. CLIENT_IP_HEADERS=none now trusts no header at all. This covers the one case where the wrong setting is worse than no setting: if the proxy does not overwrite the header we trust, any client can send it and mint itself an unlimited brute-force budget against /sign-in. Falling back to the shared rate-limit bucket is a nuisance - one noisy client can lock everyone out for ten seconds at a time - but it fails closed, and it can be reverted from the environment without a deploy. An empty or unset value still means "use the default": a stray blank line in a .env must not silently change how requests are bucketed, so only the explicit word does that. An empty array is what better-auth reads as "no headers"; it falls back to its own default only when the option is absent, and [] is truthy. The TRUSTED_PROXY_IPS boot warning was overstating the risk. It now says that a single-value header needs no trusted proxies, so seeing it on a plain single-proxy setup is expected rather than a problem to chase. Separately: a rejected promise from better-auth's handler used to escape as an unhandled rejection, leaving the request hanging forever with no response while the process logged an uncaughtException. Express 4 does not await an async handler, and nearly every better-auth route touches the admin database, so any database blip would have done this. Found by pointing ADMIN_DB at a database the user cannot open while testing the hatch. The handler now answers 503, so the caller learns and the other domains keep serving; verified as a 24ms response instead of a hang, with / and /feedback/admin/me unaffected. test/admin/admin.config.test.ts is new: it pins both branches of the hatch and the rule that an unset NODE_ENV counts as production. It stubs dotenv, because admin.config would otherwise read the repo's own .env and quietly reintroduce NODE_ENV=development - the exact value several of those cases exist to remove. 157 unit tests and 41 integration tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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/);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user