feat(backend): check the environment at boot instead of discovering it later (#64)
The backend reads environment variables in a couple of dozen places and validated none of them. A missing or misspelled one was undefined until the first line of code that happened to need it, which could be a long time after the container reported healthy — and several of those failures are silent and customer-visible. DEMO_MODE is the one that mattered most. It was read as "demo unless the value is exactly the string false", so DEMO_MODE=False, DEMO_MODE=0, or any typo meant demo mode stayed on and the shop quietly stopped charging anyone. It is now required and strict: exactly 'true' or 'false', and anything else refuses to start while quoting the value it was given, so the typo is visible in the message rather than inferred. Two requirements are conditional, and that is what makes them expressible at all. PayPal credentials are demanded only when DEMO_MODE=false, because QA runs with none of them on purpose and an unconditional rule would be simply wrong there. PUBLIC_URL is demanded only when SMTP is configured, because its only job is building links in email — an environment that cannot send mail does not need it, and requiring it everywhere would break every existing local setup to prevent nothing. UPLOADS_DIR gets no such reprieve: its fallback is correct inside the container and wrong everywhere else, so inheriting it writes uploads somewhere nobody is looking. Every problem is reported at once rather than one per restart, and the process then exits — the same shape as the container refusing to start on a failed migration rather than serving against a schema it does not match. Warnings are printed but do not stop anything: SMTP absent, the admin gate inactive, or an allowlist missing while mail can be sent. That last one is new and earns its place, since SMTP with no allowlist means the environment can reach real customers, which is what #87 exists to prevent. The admin-gate warning moved here from server.ts, so one place says what this container is and is not configured to do. validateEnv is a pure function of the environment handed to it rather than a reader of process.env, so it is tested exhaustively without booting anything or mutating global state. It is called from server.ts and deliberately not from app.ts: the integration suite imports app directly and would otherwise become a configuration exercise. Its rules are one small function each at module level, because cognitive complexity counts everything declared inside a function and the first version scored 24 against a limit of 15. Verified as a real process, not only in tests. A missing DEMO_MODE, a DEMO_MODE of 'False', real payments with no PayPal credentials, and half-configured SMTP each exit 1 with the problems listed; a valid environment starts and serves. Note the exit codes were checked without a pipe, because $? after `| head` reports head rather than node and had first suggested a clean exit. 141 unit, 169 integration and 94 end-to-end passing, lint unchanged at 0 errors and 8 warnings. Both CI workflows already set all six always-required variables plus DEMO_MODE, so the pipeline is unaffected. Refs #64 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,189 @@
|
||||
import { validateEnv } from '../../src/envValidation';
|
||||
|
||||
// The smallest environment that should boot: demo mode on, a database, and
|
||||
// somewhere to put uploads. Everything else is optional or conditional.
|
||||
const MINIMAL: NodeJS.ProcessEnv = {
|
||||
DEMO_MODE: 'true',
|
||||
PGHOST: 'localhost',
|
||||
PGPORT: '5432',
|
||||
PGUSER: 'someone',
|
||||
PGPASSWORD: 'secret',
|
||||
PGDATABASE: 'redefined',
|
||||
UPLOADS_DIR: '/tmp/uploads'
|
||||
};
|
||||
|
||||
const withEnv = (extra: NodeJS.ProcessEnv): NodeJS.ProcessEnv => ({ ...MINIMAL, ...extra });
|
||||
|
||||
// `undefined` removes a key rather than setting it to the string "undefined".
|
||||
const without = (...names: string[]): NodeJS.ProcessEnv => {
|
||||
const env = { ...MINIMAL };
|
||||
for (const name of names) delete env[name];
|
||||
return env;
|
||||
};
|
||||
|
||||
describe('validateEnv', () => {
|
||||
it('accepts the minimal environment local development already uses', () => {
|
||||
expect(validateEnv(MINIMAL).errors).toEqual([]);
|
||||
});
|
||||
|
||||
describe('variables that are always required', () => {
|
||||
it.each(['PGHOST', 'PGPORT', 'PGUSER', 'PGPASSWORD', 'PGDATABASE', 'UPLOADS_DIR'])(
|
||||
'refuses to boot without %s',
|
||||
(name) => {
|
||||
const { errors } = validateEnv(without(name));
|
||||
expect(errors.some((e) => e.includes(name))).toBe(true);
|
||||
}
|
||||
);
|
||||
|
||||
// The fallback of '/app/uploads' is right inside the container and wrong
|
||||
// everywhere else, which is why this one gets no reprieve.
|
||||
it('names UPLOADS_DIR rather than silently accepting its fallback', () => {
|
||||
const { errors } = validateEnv(without('UPLOADS_DIR'));
|
||||
expect(errors.some((e) => e.includes('UPLOADS_DIR'))).toBe(true);
|
||||
});
|
||||
|
||||
// Reporting one problem per boot makes fixing a fresh environment a
|
||||
// sequence of restarts.
|
||||
it('reports every problem at once rather than stopping at the first', () => {
|
||||
const { errors } = validateEnv(without('PGHOST', 'PGUSER', 'UPLOADS_DIR'));
|
||||
expect(errors).toHaveLength(3);
|
||||
});
|
||||
});
|
||||
|
||||
describe('DEMO_MODE', () => {
|
||||
it('is required', () => {
|
||||
const { errors } = validateEnv(without('DEMO_MODE'));
|
||||
expect(errors.some((e) => e.includes('DEMO_MODE'))).toBe(true);
|
||||
});
|
||||
|
||||
it.each(['true', 'false'])('accepts the exact value %s', (value) => {
|
||||
const env = withEnv({
|
||||
DEMO_MODE: value,
|
||||
// Real payments need credentials; supplied so this case tests DEMO_MODE
|
||||
// alone rather than tripping the PayPal rule.
|
||||
PAYPAL_CLIENT_ID: 'id',
|
||||
PAYPAL_CLIENT_SECRET: 'secret',
|
||||
PAYPAL_WEBHOOK_ID: 'hook',
|
||||
PAYPAL_ENV: 'sandbox'
|
||||
});
|
||||
expect(validateEnv(env).errors).toEqual([]);
|
||||
});
|
||||
|
||||
// The whole point of this issue. Before, any value that was not exactly
|
||||
// 'false' meant demo mode was on — so a typo silently stopped the shop
|
||||
// charging anyone.
|
||||
it.each(['False', 'FALSE', '0', 'no', 'flase', ''])(
|
||||
'refuses %p rather than reading it as demo mode',
|
||||
(value) => {
|
||||
const { errors } = validateEnv(withEnv({ DEMO_MODE: value }));
|
||||
expect(errors.some((e) => e.includes('DEMO_MODE'))).toBe(true);
|
||||
}
|
||||
);
|
||||
|
||||
it('quotes the value it was given, so the typo is visible in the message', () => {
|
||||
const { errors } = validateEnv(withEnv({ DEMO_MODE: 'False' }));
|
||||
expect(errors.find((e) => e.includes('DEMO_MODE'))).toContain("'False'");
|
||||
});
|
||||
});
|
||||
|
||||
describe('PayPal credentials', () => {
|
||||
// QA runs with no PayPal on purpose, so this cannot be an unconditional
|
||||
// requirement — it is tied to real payments being switched on.
|
||||
it('are not required while demo mode is on', () => {
|
||||
expect(validateEnv(withEnv({ DEMO_MODE: 'true' })).errors).toEqual([]);
|
||||
});
|
||||
|
||||
it.each(['PAYPAL_CLIENT_ID', 'PAYPAL_CLIENT_SECRET', 'PAYPAL_WEBHOOK_ID', 'PAYPAL_ENV'])(
|
||||
'are required when demo mode is off — missing %s',
|
||||
(name) => {
|
||||
const full = withEnv({
|
||||
DEMO_MODE: 'false',
|
||||
PAYPAL_CLIENT_ID: 'id',
|
||||
PAYPAL_CLIENT_SECRET: 'secret',
|
||||
PAYPAL_WEBHOOK_ID: 'hook',
|
||||
PAYPAL_ENV: 'live'
|
||||
});
|
||||
delete full[name];
|
||||
|
||||
const { errors } = validateEnv(full);
|
||||
expect(errors.some((e) => e.includes(name))).toBe(true);
|
||||
}
|
||||
);
|
||||
});
|
||||
|
||||
describe('SMTP', () => {
|
||||
it('is optional, and its absence is a warning rather than an error', () => {
|
||||
const { errors, warnings } = validateEnv(MINIMAL);
|
||||
expect(errors).toEqual([]);
|
||||
expect(warnings.some((w) => w.includes('SMTP'))).toBe(true);
|
||||
});
|
||||
|
||||
// Half-configured is worse than not configured: it looks set up and fails
|
||||
// at send time.
|
||||
it('refuses SMTP_USER without SMTP_PASSWORD', () => {
|
||||
const { errors } = validateEnv(withEnv({ SMTP_USER: 'someone', PUBLIC_URL: 'https://x.test' }));
|
||||
expect(errors.some((e) => e.includes('SMTP_PASSWORD'))).toBe(true);
|
||||
});
|
||||
|
||||
it('refuses SMTP_PASSWORD without SMTP_USER', () => {
|
||||
const { errors } = validateEnv(withEnv({ SMTP_PASSWORD: 'secret', PUBLIC_URL: 'https://x.test' }));
|
||||
expect(errors.some((e) => e.includes('SMTP_USER'))).toBe(true);
|
||||
});
|
||||
|
||||
it('accepts both together', () => {
|
||||
const env = withEnv({
|
||||
SMTP_USER: 'someone',
|
||||
SMTP_PASSWORD: 'secret',
|
||||
PUBLIC_URL: 'https://x.test',
|
||||
MAIL_ALLOWLIST: 'someone@example.com'
|
||||
});
|
||||
expect(validateEnv(env).errors).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('PUBLIC_URL', () => {
|
||||
// It exists only to build links in email. A local environment that cannot
|
||||
// send mail does not need it, and demanding it would break every existing
|
||||
// local setup to prevent nothing.
|
||||
it('is not required when no mail can be sent', () => {
|
||||
expect(validateEnv(MINIMAL).errors).toEqual([]);
|
||||
});
|
||||
|
||||
it('is required once SMTP is configured, because the links would read undefined', () => {
|
||||
const env = withEnv({ SMTP_USER: 'someone', SMTP_PASSWORD: 'secret' });
|
||||
const { errors } = validateEnv(env);
|
||||
expect(errors.some((e) => e.includes('PUBLIC_URL'))).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('warnings that are not failures', () => {
|
||||
// An environment that can send mail with no allowlist can reach real
|
||||
// customers, which is what #87 exists to prevent.
|
||||
it('warns when mail can be sent with no allowlist', () => {
|
||||
const env = withEnv({ SMTP_USER: 'someone', SMTP_PASSWORD: 'secret', PUBLIC_URL: 'https://x.test' });
|
||||
const { warnings } = validateEnv(env);
|
||||
expect(warnings.some((w) => w.includes('MAIL_ALLOWLIST'))).toBe(true);
|
||||
});
|
||||
|
||||
it('does not warn about the allowlist when there is no way to send mail', () => {
|
||||
const { warnings } = validateEnv(MINIMAL);
|
||||
expect(warnings.some((w) => w.includes('MAIL_ALLOWLIST'))).toBe(false);
|
||||
});
|
||||
|
||||
it('warns when the admin gate is inactive', () => {
|
||||
const { warnings } = validateEnv(MINIMAL);
|
||||
expect(warnings.some((w) => w.includes('ADMIN_GATE_SECRET'))).toBe(true);
|
||||
});
|
||||
|
||||
it('stays quiet about the admin gate once it is configured', () => {
|
||||
const { warnings } = validateEnv(withEnv({ ADMIN_GATE_SECRET: 'a-secret' }));
|
||||
expect(warnings.some((w) => w.includes('ADMIN_GATE_SECRET'))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// A variable set to spaces is a configuration mistake, not a value.
|
||||
it('treats a whitespace-only value as absent', () => {
|
||||
const { errors } = validateEnv(withEnv({ UPLOADS_DIR: ' ' }));
|
||||
expect(errors.some((e) => e.includes('UPLOADS_DIR'))).toBe(true);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user