The compose drift guard covers only ALWAYS_REQUIRED, and only QA — production is unguarded entirely #118

New Issue

2026-08-22T09:50:03-05:00

bermudalamb commented

2026-08-22 09:50:03 -05:00

backend/tests/unit/composeEnvironment.test.ts asserts that docker-compose.qa.yml sets every name in ALWAYS_REQUIRED. That list holds six entries:

PGHOST, PGPORT, PGUSER, PGPASSWORD, PGDATABASE, UPLOADS_DIR

The backend reads twenty-six distinct variables:

ADMIN_GATE_SECRET DEMO_MODE MAIL_ALLOWLIST NODE_ENV PAYPAL_CLIENT_ID
PAYPAL_CLIENT_SECRET PAYPAL_ENV PAYPAL_WEBHOOK_ID PGDATABASE PGHOST
PGPASSWORD PGPORT PGUSER PORT PUBLIC_URL SITE_CURRENCY SMTP_FROM SMTP_HOST
SMTP_PASSWORD SMTP_PORT SMTP_SECURE SMTP_USER UPLOADS_DIR USPS_CLIENT_ID
USPS_CLIENT_SECRET USPS_ENV

So a new process.env.X added to the code, with no entry in the compose file and none in ALWAYS_REQUIRED, passes CI and then fails at QA boot. That is exactly what #107 was.

The fix is not to require all twenty-six. PayPal and USPS are conditional on purpose, QA runs with no PayPal credentials at all by design, and NODE_ENV and PORT have legitimate defaults. The gap is not that the list is too short. It is that nothing notices when the list and the code disagree.

The second, larger gap: production is not checked at all

The guard reads docker-compose.qa.yml. Production is a separate Portainer stack whose compose file lives only in Portainer's UI and is not in this repository, so no test can read it. Every variable in production is unguarded, including the six in ALWAYS_REQUIRED that QA is protected against.

That is worse than an even gap, because the guard's existence implies a coverage it does not have. UPLOADS_DIR is in ALWAYS_REQUIRED, the test is green, and the natural reading is that the deploying environments set it. QA does. Production did not.

This is not hypothetical — see the comment below for the boot failure that demonstrated it.

It also interacts badly with how Portainer works, which the QA compose already documents at the ADMIN_GATE_SECRET line: stack variables are substituted into the compose file, not handed to the container. A variable set in Portainer's UI with no corresponding line in the compose file never reaches the app at all. The failure therefore looks like "I set it and it says it is not set", which is the most confusing shape a configuration error can take — and a compose-versus-code drift test is exactly what catches it, because the missing thing is the line in the file, not the value.

Committing production's compose to the repository, with secrets left as ${...} placeholders exactly as QA does, is what makes the guard able to cover both. The placeholders keep secrets out of the repository; the variable names are the thing being checked, and those are not sensitive.

Suggested shape

A unit test that scans backend/src/ for process.env.NAME and asserts every name it finds is accounted for in exactly one of:

ALWAYS_REQUIRED
a conditional rule in envValidation.ts
an explicit INTENTIONALLY_UNVALIDATED list, each entry carrying a one-line reason

The third list is the point of the whole thing. It turns "this one is fine to leave out" into a decision someone wrote down, rather than an absence nobody noticed. And it moves the moment of choice to when the variable is added, instead of the next QA boot.

The same test should then run over every compose file in the repository rather than QA's alone, so adding production's file is all it takes to bring it under the guard — and so a third environment added later is covered by being committed rather than by someone remembering to extend a test.

Environments differ legitimately, so the per-file expectations have to differ too: QA deliberately sets DEMO_MODE=true and no PayPal credentials, production is the reverse. The check should be that each file satisfies the rules envValidation.ts would apply to it, not that the files match each other.

Verification

Add a throwaway process.env.NOT_A_REAL_VAR read to a source file and confirm the test fails naming it. A guard of this kind that has never been seen to fire is indistinguishable from one that cannot.

For the production half, the equivalent check: delete the UPLOADS_DIR line from the committed production compose and confirm the test fails. That is the exact defect that took production down, so it is the one the guard must be shown to catch.

Acceptance criteria

Every process.env.NAME read in backend/src/ is accounted for in ALWAYS_REQUIRED, a conditional rule, or INTENTIONALLY_UNVALIDATED with a stated reason
Production's compose file is committed, with secrets as ${...} placeholders
The drift test runs over every compose file in the repository, not only QA's
Per-file expectations reflect what each environment legitimately needs, rather than requiring the files to be identical
Both halves are demonstrated to fail when the thing they guard is removed

`backend/tests/unit/composeEnvironment.test.ts` asserts that `docker-compose.qa.yml` sets every name in `ALWAYS_REQUIRED`. That list holds six entries: ``` PGHOST, PGPORT, PGUSER, PGPASSWORD, PGDATABASE, UPLOADS_DIR ``` The backend reads twenty-six distinct variables: ``` ADMIN_GATE_SECRET DEMO_MODE MAIL_ALLOWLIST NODE_ENV PAYPAL_CLIENT_ID PAYPAL_CLIENT_SECRET PAYPAL_ENV PAYPAL_WEBHOOK_ID PGDATABASE PGHOST PGPASSWORD PGPORT PGUSER PORT PUBLIC_URL SITE_CURRENCY SMTP_FROM SMTP_HOST SMTP_PASSWORD SMTP_PORT SMTP_SECURE SMTP_USER UPLOADS_DIR USPS_CLIENT_ID USPS_CLIENT_SECRET USPS_ENV ``` So a new `process.env.X` added to the code, with no entry in the compose file and none in `ALWAYS_REQUIRED`, passes CI and then fails at QA boot. That is exactly what #107 was. The fix is not to require all twenty-six. PayPal and USPS are conditional on purpose, QA runs with no PayPal credentials at all by design, and `NODE_ENV` and `PORT` have legitimate defaults. The gap is not that the list is too short. It is that nothing notices when the list and the code disagree. ## The second, larger gap: production is not checked at all The guard reads `docker-compose.qa.yml`. Production is a separate Portainer stack whose compose file lives only in Portainer's UI and is not in this repository, so **no test can read it**. Every variable in production is unguarded, including the six in `ALWAYS_REQUIRED` that QA is protected against. That is worse than an even gap, because the guard's existence implies a coverage it does not have. `UPLOADS_DIR` is in `ALWAYS_REQUIRED`, the test is green, and the natural reading is that the deploying environments set it. QA does. Production did not. This is not hypothetical — see the comment below for the boot failure that demonstrated it. It also interacts badly with how Portainer works, which the QA compose already documents at the `ADMIN_GATE_SECRET` line: stack variables are **substituted into the compose file, not handed to the container**. A variable set in Portainer's UI with no corresponding line in the compose file never reaches the app at all. The failure therefore looks like "I set it and it says it is not set", which is the most confusing shape a configuration error can take — and a compose-versus-code drift test is exactly what catches it, because the missing thing is the *line in the file*, not the value. Committing production's compose to the repository, with secrets left as `${...}` placeholders exactly as QA does, is what makes the guard able to cover both. The placeholders keep secrets out of the repository; the variable *names* are the thing being checked, and those are not sensitive. ## Suggested shape A unit test that scans `backend/src/` for `process.env.NAME` and asserts every name it finds is accounted for in exactly one of: - `ALWAYS_REQUIRED` - a conditional rule in `envValidation.ts` - an explicit `INTENTIONALLY_UNVALIDATED` list, each entry carrying a one-line reason The third list is the point of the whole thing. It turns "this one is fine to leave out" into a decision someone wrote down, rather than an absence nobody noticed. And it moves the moment of choice to when the variable is added, instead of the next QA boot. The same test should then run over **every** compose file in the repository rather than QA's alone, so adding production's file is all it takes to bring it under the guard — and so a third environment added later is covered by being committed rather than by someone remembering to extend a test. Environments differ legitimately, so the per-file expectations have to differ too: QA deliberately sets `DEMO_MODE=true` and no PayPal credentials, production is the reverse. The check should be that each file satisfies the rules `envValidation.ts` would apply to it, not that the files match each other. ## Verification Add a throwaway `process.env.NOT_A_REAL_VAR` read to a source file and confirm the test fails naming it. A guard of this kind that has never been seen to fire is indistinguishable from one that cannot. For the production half, the equivalent check: delete the `UPLOADS_DIR` line from the committed production compose and confirm the test fails. That is the exact defect that took production down, so it is the one the guard must be shown to catch. ## Acceptance criteria - Every `process.env.NAME` read in `backend/src/` is accounted for in `ALWAYS_REQUIRED`, a conditional rule, or `INTENTIONALLY_UNVALIDATED` with a stated reason - Production's compose file is committed, with secrets as `${...}` placeholders - The drift test runs over every compose file in the repository, not only QA's - Per-file expectations reflect what each environment legitimately needs, rather than requiring the files to be identical - Both halves are demonstrated to fail when the thing they guard is removed

bermudalamb added this to the Code Quality and Hardening 2 project 2026-08-22 09:59:59 -05:00

bermudalamb self-assigned this 2026-08-22 10:00:23 -05:00

bermudalamb changed title from ~~The compose drift guard only covers ALWAYS_REQUIRED, so most environment variables are unguarded~~ to The compose drift guard covers only ALWAYS_REQUIRED, and only QA — production is unguarded entirely

2026-08-23 09:47:38 -05:00

bermudalamb commented

2026-08-23 09:48:22 -05:00

The boot failure that prompted extending this issue, recorded because it is the exact defect the production half of the guard has to catch.

Production refused to start:

[config] refusing to start — 4 problem(s) with the environment:
[config]   - UPLOADS_DIR is required and is not set.
[config]   - PAYPAL_CLIENT_ID is required when DEMO_MODE=false, because real payments are enabled.
[config]   - PAYPAL_CLIENT_SECRET is required when DEMO_MODE=false, because real payments are enabled.
[config]   - PAYPAL_WEBHOOK_ID is required when DEMO_MODE=false, because real payments are enabled.

with ADMIN_GATE_SECRET also warned about, and UPLOADS_DIR set to /app/uploads in Portainer's stack variables at the time.

UPLOADS_DIR is in ALWAYS_REQUIRED. The drift guard is green. It reads docker-compose.qa.yml, which sets it correctly — and production, which did not, is not a file any test can see.

What the error output proves, read against envValidation.ts, is worth writing down because it shows the failure is selective rather than total:

checkPayPal returns early unless env.DEMO_MODE === 'false' exactly, so the PayPal errors firing means the container received the literal string false — not true, and not unset, since checkDemoMode would then have produced its own error and did not.
PAYPAL_ENV is in PAYPAL_REQUIRED and did not error, so it arrived.
PUBLIC_URL did not error while SMTP was configured, so it arrived.
The MAIL_ALLOWLIST warning rather than the "SMTP is not configured" one means SMTP_USER and SMTP_PASSWORD both arrived.
Migrations ran, so every PG* variable arrived.

So PG*, SMTP_*, PUBLIC_URL, PAYPAL_ENV and DEMO_MODE reached the container, while UPLOADS_DIR and ADMIN_GATE_SECRET did not. The dividing line is not which variables were set in Portainer — it is which ones have a line in the compose file. That is the substitution behaviour the QA compose already warns about at its ADMIN_GATE_SECRET entry, hit in production where nothing was watching for it.

The shape of the failure is the part that makes this worth guarding rather than remembering: the operator had set the variable, the platform reported it as set, and the application correctly reported it as missing. Every one of those statements was true at once. Nothing short of a test that reads the deploying file and the code together can reconcile them.

The boot failure that prompted extending this issue, recorded because it is the exact defect the production half of the guard has to catch. Production refused to start: ``` [config] refusing to start — 4 problem(s) with the environment: [config] - UPLOADS_DIR is required and is not set. [config] - PAYPAL_CLIENT_ID is required when DEMO_MODE=false, because real payments are enabled. [config] - PAYPAL_CLIENT_SECRET is required when DEMO_MODE=false, because real payments are enabled. [config] - PAYPAL_WEBHOOK_ID is required when DEMO_MODE=false, because real payments are enabled. ``` with `ADMIN_GATE_SECRET` also warned about, and `UPLOADS_DIR` set to `/app/uploads` in Portainer's stack variables at the time. `UPLOADS_DIR` is in `ALWAYS_REQUIRED`. The drift guard is green. It reads `docker-compose.qa.yml`, which sets it correctly — and production, which did not, is not a file any test can see. What the error output proves, read against `envValidation.ts`, is worth writing down because it shows the failure is selective rather than total: - `checkPayPal` returns early unless `env.DEMO_MODE === 'false'` exactly, so the PayPal errors firing means the container received the literal string `false` — not `true`, and not unset, since `checkDemoMode` would then have produced its own error and did not. - `PAYPAL_ENV` is in `PAYPAL_REQUIRED` and did not error, so it arrived. - `PUBLIC_URL` did not error while SMTP was configured, so it arrived. - The `MAIL_ALLOWLIST` warning rather than the "SMTP is not configured" one means `SMTP_USER` and `SMTP_PASSWORD` both arrived. - Migrations ran, so every `PG*` variable arrived. So `PG*`, `SMTP_*`, `PUBLIC_URL`, `PAYPAL_ENV` and `DEMO_MODE` reached the container, while `UPLOADS_DIR` and `ADMIN_GATE_SECRET` did not. The dividing line is not which variables were set in Portainer — it is which ones have a line in the compose file. That is the substitution behaviour the QA compose already warns about at its `ADMIN_GATE_SECRET` entry, hit in production where nothing was watching for it. The shape of the failure is the part that makes this worth guarding rather than remembering: the operator had set the variable, the platform reported it as set, and the application correctly reported it as missing. Every one of those statements was true at once. Nothing short of a test that reads the deploying file and the code together can reconcile them.

bermudalamb referenced this issue from a commit

2026-08-23 16:12:48 -05:00

fix(deploy): commit production's compose and bring it under the drift guard (#118)

bermudalamb referenced this issue

2026-08-23 16:14:37 -05:00

fix(deploy): commit production's compose and bring it under the drift guard (#118) #145

bermudalamb added reference feature/118-production-compose

2026-08-23 16:14:59 -05:00

bermudalamb referenced this issue from a commit

2026-08-23 16:15:17 -05:00

Merge pull request 'fix(deploy): commit production's compose and bring it under the drift guard (#118)' (#145) from feature/118-production-compose into main

bermudalamb moved this to Review in Code Quality and Hardening 2 on 2026-08-23 16:16:18 -05:00