No boot-time validation of required environment variables #64

New Issue

2026-08-19T11:06:29-05:00

bermudalamb commented

2026-08-19 11:06:29 -05:00

Found during a Microsoft/React/SonarQube best-practices review.

Problem

The backend reads process.env in 34 places and validates none of them at startup. A missing or misspelled variable is undefined until the first line of code that happens to need it, which may be a long time after the container reports healthy.

The consequences differ by variable, and none of them announce themselves:

PUBLIC_URL — used to build links in the favorite-sold, item-withdrawn, password-reset and verification emails. If unset, customers receive mail containing undefined in the URL. The send succeeds, so nothing looks wrong from the server's side.
SMTP settings — a misconfiguration means mail throws at send time. Favorite alerts catch per-recipient and log, so sales continue working while nobody is notified.
UPLOADS_DIR — falls back to /app/uploads, which is correct in the container and wrong everywhere else.
DEMO_MODE — read as process.env.DEMO_MODE !== 'false', so anything other than the exact string false means demo mode is on. A typo such as DEMO_MODE=False or DEMO_MODE=0 silently disables real payments in production.

That last one is worth stating plainly: the current default means a configuration mistake fails toward not charging customers, and does so silently.

Why it fits this codebase's existing concerns

The container already refuses to start on a failed migration rather than serving against a schema it does not match — the reasoning being that a loud failure at boot beats a quiet wrong answer later. Environment configuration is the same argument and does not currently get the same treatment.

Suggested approach

A single config module, read once at startup, that fails fast with a clear message naming the missing variable — and is the only place in the backend that touches process.env, so the requirements are enumerable by reading one file.

Worth deciding:

Which variables are genuinely required versus optional-with-a-default. PUBLIC_URL should probably be required in production and defaulted in development. SMTP is required only if any mail is expected to send, which is false in QA by design.
Whether DEMO_MODE should keep failing toward demo. The safe default for an unset value is arguable both ways, but the current typo behaviour is not: DEMO_MODE=False should be an error, not a silent true.
How much QA's deliberately incomplete configuration complicates this. QA runs with no SMTP and no PayPal credentials on purpose, so validation has to accommodate a legitimately partial environment rather than treating it as broken.

Severity

Medium. Nothing is broken today, but several of the failure modes are silent and customer-visible, and one of them affects whether money is taken.

Found during a Microsoft/React/SonarQube best-practices review. ## Problem The backend reads `process.env` in **34 places** and validates none of them at startup. A missing or misspelled variable is `undefined` until the first line of code that happens to need it, which may be a long time after the container reports healthy. The consequences differ by variable, and none of them announce themselves: - **`PUBLIC_URL`** — used to build links in the favorite-sold, item-withdrawn, password-reset and verification emails. If unset, customers receive mail containing `undefined` in the URL. The send succeeds, so nothing looks wrong from the server's side. - **SMTP settings** — a misconfiguration means mail throws at send time. Favorite alerts catch per-recipient and log, so sales continue working while nobody is notified. - **`UPLOADS_DIR`** — falls back to `/app/uploads`, which is correct in the container and wrong everywhere else. - **`DEMO_MODE`** — read as `process.env.DEMO_MODE !== 'false'`, so anything other than the exact string `false` means demo mode is **on**. A typo such as `DEMO_MODE=False` or `DEMO_MODE=0` silently disables real payments in production. That last one is worth stating plainly: the current default means a configuration mistake fails toward *not charging customers*, and does so silently. ## Why it fits this codebase's existing concerns The container already refuses to start on a failed migration rather than serving against a schema it does not match — the reasoning being that a loud failure at boot beats a quiet wrong answer later. Environment configuration is the same argument and does not currently get the same treatment. ## Suggested approach A single config module, read once at startup, that fails fast with a clear message naming the missing variable — and is the only place in the backend that touches `process.env`, so the requirements are enumerable by reading one file. Worth deciding: - **Which variables are genuinely required versus optional-with-a-default.** `PUBLIC_URL` should probably be required in production and defaulted in development. SMTP is required only if any mail is expected to send, which is false in QA by design. - **Whether `DEMO_MODE` should keep failing toward demo.** The safe default for an unset value is arguable both ways, but the current *typo* behaviour is not: `DEMO_MODE=False` should be an error, not a silent `true`. - **How much QA's deliberately incomplete configuration complicates this.** QA runs with no SMTP and no PayPal credentials on purpose, so validation has to accommodate a legitimately partial environment rather than treating it as broken. ## Severity Medium. Nothing is broken today, but several of the failure modes are silent and customer-visible, and one of them affects whether money is taken.

bermudalamb added this to the Code Quality and Hardening project 2026-08-19 11:38:25 -05:00

bermudalamb referenced this issue

2026-08-21 09:08:07 -05:00

QA cannot send email at all, so the four mail flows have never been regression tested #87

bermudalamb referenced this issue from a commit

2026-08-21 09:38:29 -05:00

feat: let QA send real email, guarded by a recipient allowlist (#87)

bermudalamb referenced this issue

2026-08-21 09:38:51 -05:00

feat: let QA send real email, guarded by a recipient allowlist (#87) #88

bermudalamb moved this to In Progress in Code Quality and Hardening on 2026-08-21 13:57:07 -05:00

bermudalamb commented

2026-08-21 15:13:10 -05:00

Design settled

The backend now reads 26 distinct variables across 9 files — the count has grown since this issue was written, with MAIL_ALLOWLIST (#87) and ADMIN_GATE_SECRET (#63) added since.

Decisions

Validate at boot; leave the existing reads where they are. Not the full centralisation this issue suggested. Moving all 26 reads into one module touches the database pool and the PayPal client and would need every one of those files re-tested, for a benefit — enumerability — that a single validator already mostly provides. The accepted cost is drift: a process.env.NEW_THING added later is simply unvalidated and nothing notices. A test that greps src/ for unknown variables would close that, in the same shape as routesAreWrapped.test.ts, and is a reasonable follow-up rather than part of this.

DEMO_MODE becomes required and strict. Present, and exactly true or false. Anything else — False, 0, empty — refuses to boot naming the value it got. Local development and QA already set DEMO_MODE=true, so this costs nothing in either.

Where it runs, and where it deliberately does not

server.ts, immediately before app.listen. Not app.ts: the 160 integration tests import app directly, and validating on import would turn every one of them into a configuration exercise. It also keeps the validator a pure function that can be unit-tested without booting anything.

All problems are collected and reported together, then the process exits non-zero. Fixing configuration one boot at a time is miserable, and the container already refuses to start on a failed migration rather than serving against a schema it does not match — same reasoning, same failure shape.

The rules

	Variables
Always required	`DEMO_MODE` (strict), `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`; `SMTP_USER` and `SMTP_PASSWORD` all-or-nothing
Warned, not fatal	SMTP absent, `ADMIN_GATE_SECRET` absent, `MAIL_ALLOWLIST` absent while SMTP is configured

Two of these are conditional rather than absolute, and that is what makes them expressible at all:

PayPal is tied to DEMO_MODE=false. This issue asked how QA's deliberately incomplete configuration complicates validation — this is the answer. QA runs with no PayPal credentials on purpose, so an unconditional requirement would be wrong. Tying it to real payments being switched on means the requirement is exactly as strict as it needs to be.

PUBLIC_URL is tied to SMTP. It exists only to build links in emails. A local environment that cannot send mail does not need it, and requiring it unconditionally would break every existing local setup to prevent nothing. UPLOADS_DIR gets no such reprieve — its fallback of /app/uploads is right in the container and wrong everywhere else, exactly as this issue says, so it is required outright.

The MAIL_ALLOWLIST warning is not from the original issue and earns its place: SMTP configured with no allowlist means that environment can email real customers, which is the thing #87 exists to prevent.

Testing

Unit tests against the pure function — each required variable missing, DEMO_MODE as False/0/empty, PayPal required only when demo is off, SMTP half-configured, PUBLIC_URL demanded only alongside SMTP, and a clean environment producing nothing. No integration test: the function is pure, and booting a server to check it would test less rather than more.

Before this merges — worth checking today, separately from this issue

Local development and QA both set DEMO_MODE=true. Production's Portainer stack is outside this repository and I cannot see it.

If production does not currently set DEMO_MODE=false, it is running in demo mode right now and customers are not being charged. That is worth verifying immediately and is independent of this work. After this change, a production stack missing the variable refuses to boot rather than quietly running in demo mode — so it needs setting before the next deploy in either case.

## Design settled The backend now reads **26 distinct variables across 9 files** — the count has grown since this issue was written, with `MAIL_ALLOWLIST` (#87) and `ADMIN_GATE_SECRET` (#63) added since. ## Decisions **Validate at boot; leave the existing reads where they are.** Not the full centralisation this issue suggested. Moving all 26 reads into one module touches the database pool and the PayPal client and would need every one of those files re-tested, for a benefit — enumerability — that a single validator already mostly provides. The accepted cost is drift: a `process.env.NEW_THING` added later is simply unvalidated and nothing notices. A test that greps `src/` for unknown variables would close that, in the same shape as `routesAreWrapped.test.ts`, and is a reasonable follow-up rather than part of this. **`DEMO_MODE` becomes required and strict.** Present, and exactly `true` or `false`. Anything else — `False`, `0`, empty — refuses to boot naming the value it got. Local development and QA already set `DEMO_MODE=true`, so this costs nothing in either. ## Where it runs, and where it deliberately does not `server.ts`, immediately before `app.listen`. **Not** `app.ts`: the 160 integration tests import `app` directly, and validating on import would turn every one of them into a configuration exercise. It also keeps the validator a pure function that can be unit-tested without booting anything. All problems are collected and reported together, then the process exits non-zero. Fixing configuration one boot at a time is miserable, and the container already refuses to start on a failed migration rather than serving against a schema it does not match — same reasoning, same failure shape. ## The rules | | Variables | | --- | --- | | Always required | `DEMO_MODE` (strict), `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`; `SMTP_USER` and `SMTP_PASSWORD` all-or-nothing | | Warned, not fatal | SMTP absent, `ADMIN_GATE_SECRET` absent, `MAIL_ALLOWLIST` absent while SMTP is configured | Two of these are conditional rather than absolute, and that is what makes them expressible at all: **PayPal is tied to `DEMO_MODE=false`.** This issue asked how QA's deliberately incomplete configuration complicates validation — this is the answer. QA runs with no PayPal credentials on purpose, so an unconditional requirement would be wrong. Tying it to real payments being switched on means the requirement is exactly as strict as it needs to be. **`PUBLIC_URL` is tied to SMTP.** It exists only to build links in emails. A local environment that cannot send mail does not need it, and requiring it unconditionally would break every existing local setup to prevent nothing. `UPLOADS_DIR` gets no such reprieve — its fallback of `/app/uploads` is right in the container and wrong everywhere else, exactly as this issue says, so it is required outright. The `MAIL_ALLOWLIST` warning is not from the original issue and earns its place: SMTP configured with no allowlist means that environment can email real customers, which is the thing #87 exists to prevent. ## Testing Unit tests against the pure function — each required variable missing, `DEMO_MODE` as `False`/`0`/empty, PayPal required only when demo is off, SMTP half-configured, `PUBLIC_URL` demanded only alongside SMTP, and a clean environment producing nothing. No integration test: the function is pure, and booting a server to check it would test less rather than more. ## Before this merges — worth checking today, separately from this issue Local development and QA both set `DEMO_MODE=true`. Production's Portainer stack is outside this repository and I cannot see it. **If production does not currently set `DEMO_MODE=false`, it is running in demo mode right now and customers are not being charged.** That is worth verifying immediately and is independent of this work. After this change, a production stack missing the variable refuses to boot rather than quietly running in demo mode — so it needs setting before the next deploy in either case.

bermudalamb commented

2026-08-21 15:24:10 -05:00

Implemented on `feature/64-env-validation` — one commit, not pushed

validateEnv is a pure function of the environment handed to it, called from server.ts before app.listen. Every problem is reported at once and then the process exits.

Verified as a real process, not only in tests

The unit tests prove the rules; these prove the process actually refuses:

Environment	Result
`DEMO_MODE` missing	exit 1, names it
`DEMO_MODE=False`	exit 1, quotes `'False'` back
`DEMO_MODE=false` with no PayPal	exit 1, all four credentials listed together
`SMTP_USER` without `SMTP_PASSWORD`	exit 1
Valid minimal environment	starts, serves `/api/items` 200, prints its two warnings

Worth recording how nearly that went wrong: my first exit-code check was node dist/server.js 2>&1 | head -4; echo $?, which reports head's status rather than node's and cheerfully printed exit=0. Re-checked without the pipe, it is 1. A validator that prints the right words while exiting 0 would leave a container restarting into a broken configuration forever, and the first check would have said it was fine.

The rules as built

	Variables
Always required	`DEMO_MODE` (strict), `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`; `SMTP_USER`/`SMTP_PASSWORD` together
Warned, not fatal	SMTP absent, `ADMIN_GATE_SECRET` absent, `MAIL_ALLOWLIST` absent while SMTP is configured

The DEMO_MODE message quotes the value it received, so the typo is visible rather than inferred — that is the difference between "DEMO_MODE is invalid" and seeing 'False' in the log.

The admin-gate warning moved here from server.ts, so one place now says what this container is and is not configured to do rather than two.

A finding from my own lint gate

The first validateEnv scored 24 cognitive complexity against a limit of 15, and would have added a warning. Each rule is now its own small function at module level — which reads better anyway, and applies what #81 established: nesting is what counts toward the parent, so extraction has to go to module level to help.

Verification

141 unit (35 new), 169 integration, 94 end-to-end on a fresh container, lint unchanged at 0 errors and 8 warnings, build clean.

Both CI workflows already set all six always-required variables plus DEMO_MODE=true, so the pipeline is unaffected — checked rather than assumed, since tests.yml and sonarqube.yml both boot the real server.

The README gains a section stating the rules and why two of them are conditional.

Still needs your hand, and it may be live

Local development and QA both set DEMO_MODE=true. I cannot see production's Portainer stack. If it does not currently set DEMO_MODE=false, production is in demo mode right now and customers are not being charged — worth checking today, independently of this branch. After this merges, a stack missing the variable refuses to boot rather than starting in demo mode.

Deliberately not done

Full centralisation of process.env access, which this issue originally suggested. The accepted cost is drift — a new process.env.NEW_THING is simply unvalidated and nothing notices. A unit test grepping src/ for unknown variables would close that in the same shape as routesAreWrapped.test.ts, and is a reasonable follow-up.

Not pushed, per the usual arrangement.

## Implemented on `feature/64-env-validation` — one commit, not pushed `validateEnv` is a pure function of the environment handed to it, called from `server.ts` before `app.listen`. Every problem is reported at once and then the process exits. ## Verified as a real process, not only in tests The unit tests prove the rules; these prove the process actually refuses: | Environment | Result | | --- | --- | | `DEMO_MODE` missing | **exit 1**, names it | | `DEMO_MODE=False` | **exit 1**, quotes `'False'` back | | `DEMO_MODE=false` with no PayPal | **exit 1**, all four credentials listed together | | `SMTP_USER` without `SMTP_PASSWORD` | **exit 1** | | Valid minimal environment | starts, serves `/api/items` 200, prints its two warnings | Worth recording how nearly that went wrong: my first exit-code check was `node dist/server.js 2>&1 | head -4; echo $?`, which reports **head's** status rather than node's and cheerfully printed `exit=0`. Re-checked without the pipe, it is 1. A validator that prints the right words while exiting 0 would leave a container restarting into a broken configuration forever, and the first check would have said it was fine. ## The rules as built | | Variables | | --- | --- | | Always required | `DEMO_MODE` (strict), `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`; `SMTP_USER`/`SMTP_PASSWORD` together | | Warned, not fatal | SMTP absent, `ADMIN_GATE_SECRET` absent, `MAIL_ALLOWLIST` absent while SMTP is configured | The `DEMO_MODE` message quotes the value it received, so the typo is visible rather than inferred — that is the difference between "DEMO_MODE is invalid" and seeing `'False'` in the log. The admin-gate warning **moved here** from `server.ts`, so one place now says what this container is and is not configured to do rather than two. ## A finding from my own lint gate The first `validateEnv` scored **24** cognitive complexity against a limit of 15, and would have added a warning. Each rule is now its own small function at module level — which reads better anyway, and applies what #81 established: nesting is what counts toward the parent, so extraction has to go to module level to help. ## Verification **141 unit** (35 new), **169 integration**, **94 end-to-end** on a fresh container, lint unchanged at 0 errors and 8 warnings, build clean. Both CI workflows already set all six always-required variables plus `DEMO_MODE=true`, so the pipeline is unaffected — checked rather than assumed, since `tests.yml` and `sonarqube.yml` both boot the real server. The README gains a section stating the rules and why two of them are conditional. ## Still needs your hand, and it may be live Local development and QA both set `DEMO_MODE=true`. **I cannot see production's Portainer stack.** If it does not currently set `DEMO_MODE=false`, production is in demo mode right now and customers are not being charged — worth checking today, independently of this branch. After this merges, a stack missing the variable refuses to boot rather than starting in demo mode. ## Deliberately not done Full centralisation of `process.env` access, which this issue originally suggested. The accepted cost is drift — a new `process.env.NEW_THING` is simply unvalidated and nothing notices. A unit test grepping `src/` for unknown variables would close that in the same shape as `routesAreWrapped.test.ts`, and is a reasonable follow-up. Not pushed, per the usual arrangement.

bermudalamb referenced this issue from a commit

2026-08-21 15:28:11 -05:00

feat(backend): check the environment at boot instead of discovering it later (#64)

bermudalamb referenced this issue

2026-08-21 15:28:58 -05:00

feat(backend): check the environment at boot instead of discovering it later (#64) #96

bermudalamb added reference feature/64-env-validation

2026-08-21 15:29:59 -05:00

bermudalamb referenced this issue from a commit

2026-08-21 15:30:18 -05:00

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

bermudalamb moved this to Ready for Release in Code Quality and Hardening on 2026-08-21 15:31:36 -05:00