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