Merge pull request 'feat(backend): check the environment at boot instead of discovering it later (#64)' (#96) from feature/64-env-validation into main

Reviewed-on: #96

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

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

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<void> {
@@ -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}`));

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