From 9c9e9c3ded0475ce764f5f2d7a91ff81d835b63a Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 21 Aug 2026 15:22:42 -0500 Subject: [PATCH] feat(backend): check the environment at boot instead of discovering it later (#64) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- README.md | 15 ++ backend/src/envValidation.ts | 171 ++++++++++++++++++++ backend/src/server.ts | 32 ++-- backend/tests/unit/envValidation.test.ts | 189 +++++++++++++++++++++++ 4 files changed, 396 insertions(+), 11 deletions(-) create mode 100644 backend/src/envValidation.ts create mode 100644 backend/tests/unit/envValidation.test.ts diff --git a/README.md b/README.md index f2926f4..0f8eccd 100755 --- a/README.md +++ b/README.md @@ -82,6 +82,21 @@ mkdir -p /tmp/redefined-uploads `DEMO_MODE=true` enables a "Buy Now (Demo)" button on the storefront that completes a purchase without needing real PayPal credentials — useful for local development and for the Playwright tests below. To exercise real PayPal checkout locally, also set `PAYPAL_CLIENT_ID`, `PAYPAL_CLIENT_SECRET`, and `PAYPAL_ENV=sandbox`. +#### The server checks its configuration before it starts + +`server.ts` validates the environment at boot, reports every problem at once, and exits rather than starting — the same reasoning as the container refusing to start on a failed migration. A missing variable used to be `undefined` until the first line of code that happened to need it, which could be long after the container reported healthy, and several of those failures were silent and customer-visible. + +| | Variables | +| --- | --- | +| Always required | `DEMO_MODE`, `PGHOST`, `PGPORT`, `PGUSER`, `PGPASSWORD`, `PGDATABASE`, `UPLOADS_DIR` | +| Required when `DEMO_MODE=false` | `PAYPAL_CLIENT_ID`, `PAYPAL_CLIENT_SECRET`, `PAYPAL_WEBHOOK_ID`, `PAYPAL_ENV` | +| Required when SMTP is configured | `PUBLIC_URL`, and `SMTP_USER`/`SMTP_PASSWORD` together | +| Warned about, but not fatal | SMTP absent, `ADMIN_GATE_SECRET` absent, `MAIL_ALLOWLIST` absent while SMTP is configured | + +**`DEMO_MODE` must be exactly `true` or `false`.** It used to mean "demo unless the value is exactly `false`", so `DEMO_MODE=False`, `0`, or any typo left demo mode on — which meant the shop quietly stopped charging anyone. It is now required and strict, so a slip is a startup failure instead. + +`PUBLIC_URL` is required only alongside SMTP because its only job is building links in email; an environment that cannot send mail does not need it. `UPLOADS_DIR` has no such reprieve — its fallback of `/app/uploads` is correct inside the container and wrong everywhere else. + **Note (PowerShell):** environment variables set with `$env:` only last for the current terminal session/tab. If you close and reopen VS Code's terminal, you'll need to re-run step 3 before starting the backend again. ### 4. Run the backend diff --git a/backend/src/envValidation.ts b/backend/src/envValidation.ts new file mode 100644 index 0000000..55ce96c --- /dev/null +++ b/backend/src/envValidation.ts @@ -0,0 +1,171 @@ +/** + * Boot-time configuration checks. + * + * The backend reads environment variables in a couple of dozen places, and a + * missing or misspelled one used to be `undefined` until the first line of code + * that happened to need it — which could be a very long time after the + * container reported healthy. Several of those failures are silent and + * customer-visible: mail containing `undefined` in a link, or a shop that + * quietly stops charging anyone. + * + * The container already refuses to start on a failed migration rather than + * serving against a schema it does not match. This is the same argument applied + * to configuration. + * + * Kept a pure function of the environment it is handed, rather than reading + * `process.env` itself, so it can be tested exhaustively without booting a + * server or mutating global state. `server.ts` calls it; `app.ts` deliberately + * does not, because the integration suite imports `app` directly and would + * otherwise become a configuration exercise. + */ + +export interface EnvValidation { + /** Configuration that must be fixed. The process should not start. */ + errors: string[]; + /** Working, but worth saying out loud — usually a capability that is off. */ + warnings: string[]; +} + +// Without these the process cannot do its job at all. +const ALWAYS_REQUIRED = [ + 'PGHOST', + 'PGPORT', + 'PGUSER', + 'PGPASSWORD', + 'PGDATABASE', + // No reprieve for this one despite having a fallback: '/app/uploads' is + // correct inside the container and wrong everywhere else, so inheriting it + // silently writes uploads somewhere nobody is looking. + 'UPLOADS_DIR' +] as const; + +// Only meaningful once real payments are switched on. QA runs with none of +// these on purpose, which is why the requirement is conditional rather than +// absolute. +const PAYPAL_REQUIRED = [ + 'PAYPAL_CLIENT_ID', + 'PAYPAL_CLIENT_SECRET', + 'PAYPAL_WEBHOOK_ID', + 'PAYPAL_ENV' +] as const; + +// A variable set to spaces is a configuration mistake, not a value. +function isPresent(env: NodeJS.ProcessEnv, name: string): boolean { + const value = env[name]; + return typeof value === 'string' && value.trim() !== ''; +} + +// One function per rule, at module level rather than nested. Each is small +// enough to read on its own, and cognitive complexity counts everything +// declared inside a function — so keeping these out of validateEnv is what +// keeps the composition below flat. + +function checkAlwaysRequired(env: NodeJS.ProcessEnv): string[] { + return ALWAYS_REQUIRED.filter((name) => !isPresent(env, name)).map( + (name) => `${name} is required and is not set.` + ); +} + +// Strict rather than truthy. This used to be read as "demo unless the value is +// exactly 'false'", so DEMO_MODE=False, 0, or any typo meant demo mode was on — +// a configuration slip that stopped the shop taking money and said nothing. +function checkDemoMode(env: NodeJS.ProcessEnv): string[] { + const demoMode = env.DEMO_MODE; + + if (demoMode === undefined || demoMode.trim() === '') { + return [ + "DEMO_MODE is required and must be exactly 'true' or 'false'. It decides whether real " + + 'payments are taken, so it has to be stated rather than inherited.' + ]; + } + + if (demoMode !== 'true' && demoMode !== 'false') { + return [ + `DEMO_MODE must be exactly 'true' or 'false', but is '${demoMode}'. Anything else used to ` + + 'be read as demo mode, which meant a typo here quietly stopped the shop charging anyone.' + ]; + } + + return []; +} + +// Conditional rather than absolute: QA runs with no PayPal credentials on +// purpose, so requiring them unconditionally would be wrong. +function checkPayPal(env: NodeJS.ProcessEnv): string[] { + if (env.DEMO_MODE !== 'false') { + return []; + } + return PAYPAL_REQUIRED.filter((name) => !isPresent(env, name)).map( + (name) => `${name} is required when DEMO_MODE=false, because real payments are enabled.` + ); +} + +// SMTP is all or nothing, and two other variables hang off whether it is set. +function checkMail(env: NodeJS.ProcessEnv): EnvValidation { + const errors: string[] = []; + const warnings: string[] = []; + + const hasUser = isPresent(env, 'SMTP_USER'); + const hasPassword = isPresent(env, 'SMTP_PASSWORD'); + + // Half-configured is worse than absent: the mailer only skips when both are + // missing, so setting one produces a connection that fails at send time + // instead of a clean "mail is off". + if (hasUser && !hasPassword) { + errors.push('SMTP_PASSWORD is required when SMTP_USER is set — set both or neither.'); + } + if (hasPassword && !hasUser) { + errors.push('SMTP_USER is required when SMTP_PASSWORD is set — set both or neither.'); + } + + if (!hasUser || !hasPassword) { + warnings.push( + 'SMTP is not configured — no email will be sent. Verification, password reset, favorite ' + + 'alerts and cart reminders will all be skipped with a warning.' + ); + return { errors, warnings }; + } + + // Demanded only alongside SMTP. Its sole job is building links in email, so a + // local environment that cannot send mail does not need it, and requiring it + // there would break every existing local setup to prevent nothing. + if (!isPresent(env, 'PUBLIC_URL')) { + errors.push( + 'PUBLIC_URL is required when SMTP is configured, or every link in a verification, ' + + 'password-reset, favorite-alert or cart-reminder email reads "undefined".' + ); + } + + if (!isPresent(env, 'MAIL_ALLOWLIST')) { + warnings.push( + 'MAIL_ALLOWLIST is not set while SMTP is configured — this environment can email real ' + + 'customers. That is correct for production and a hazard anywhere else.' + ); + } + + return { errors, warnings }; +} + +function checkAdminGate(env: NodeJS.ProcessEnv): string[] { + if (isPresent(env, 'ADMIN_GATE_SECRET')) { + return []; + } + return [ + 'ADMIN_GATE_SECRET is not set — /api/admin is protected only by the reverse proxy. ' + + 'Anything able to reach this container directly can administer the store.' + ]; +} + +export function validateEnv(env: NodeJS.ProcessEnv): EnvValidation { + const mail = checkMail(env); + + return { + errors: [ + ...checkAlwaysRequired(env), + ...checkDemoMode(env), + ...checkPayPal(env), + ...mail.errors + ], + warnings: [...mail.warnings, ...checkAdminGate(env)] + }; +} diff --git a/backend/src/server.ts b/backend/src/server.ts index 8103405..e8f8e9c 100755 --- a/backend/src/server.ts +++ b/backend/src/server.ts @@ -2,6 +2,7 @@ import cron from 'node-cron'; import app from './app'; import { pool } from './db'; import { sendMail } from './mailer'; +import { validateEnv } from './envValidation'; // Release cart holds whose expiry has passed. async function sweepExpiredCarts(): Promise { @@ -65,17 +66,26 @@ setInterval(() => void sweepExpiredCarts(), 5 * 60 * 1000); cron.schedule('0 9 * * *', () => void sendCartReminders()); const PORT = parseInt(process.env.PORT || '3000', 10); -// Said at boot rather than left to be discovered. Without the secret the admin -// API is protected only by the reverse proxy's auth_request regex, which lives -// outside this repository and is bypassed entirely by anything reaching this -// container's published port directly. That is a defensible way to run — it is -// how this app has always run — but it should be a visible choice rather than a -// silent one. See middleware/adminGate.ts and #63. -if (!process.env.ADMIN_GATE_SECRET) { - console.warn( - '[admin-gate] ADMIN_GATE_SECRET is not set — /api/admin is protected only by the reverse ' + - 'proxy. Anything able to reach this container directly can administer the store.' - ); + +// Checked at boot rather than left to be discovered by the first request that +// happens to need a missing value. Every problem is reported at once — fixing a +// fresh environment one restart at a time is miserable — and anything fatal +// stops the process, the same way a failed migration does rather than serving +// against a schema it does not match. The admin-gate warning lives here too now +// (#63), so there is one place that says what this container is and is not +// configured to do. See envValidation.ts and #64. +const { errors, warnings } = validateEnv(process.env); + +for (const warning of warnings) { + console.warn(`[config] ${warning}`); +} + +if (errors.length) { + console.error(`[config] refusing to start — ${errors.length} problem(s) with the environment:`); + for (const error of errors) { + console.error(`[config] - ${error}`); + } + process.exit(1); } app.listen(PORT, () => console.log(`redefined-designs listening on ${PORT}`)); diff --git a/backend/tests/unit/envValidation.test.ts b/backend/tests/unit/envValidation.test.ts new file mode 100644 index 0000000..1c41ae7 --- /dev/null +++ b/backend/tests/unit/envValidation.test.ts @@ -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); + }); +}); -- 2.54.0