From 0710cccac1819a5647559acebd18f26712dd5cde Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 21 Aug 2026 17:24:55 -0500 Subject: [PATCH 1/2] fix(qa): give the container the environment variables it now requires (#107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QA refuses to start: "UPLOADS_DIR is required and is not set." #64 made UPLOADS_DIR always required, on the reasoning that its fallback of /app/uploads is correct inside the container and wrong everywhere else, so nothing should inherit it silently. That reasoning stands — but docker-compose.qa.yml had never set it either. QA was relying on exactly the fallback that change set out to stop people relying on, so the first deploy after it merged is the first one to fail. That is an incomplete check, and one that looked confident. #64 verified both CI workflows set every always-required variable and said so. CI is not what deploys. The compose file, which is, was never opened. ADMIN_GATE_SECRET was missing for a different reason, and the container reported it unset even after it was added to the Portainer stack. That is not a mistake, it is how compose works: stack variables are substituted into this file as ${VAR}, not handed to the container. A service receives exactly what its environment block lists. #87 already wrote that down; this is the first time it has bitten. The two get different treatment for a reason. UPLOADS_DIR is hardcoded because it is not a secret and because it has to match the right-hand side of the volume mapping — splitting one value across two places in the same file is how they drift apart. ADMIN_GATE_SECRET is interpolated from the stack so the secret itself never enters the repository, and the header comment now lists it among the required stack variables. Verified by feeding the environment docker compose config actually renders into validateEnv, the same function that was rejecting it: zero errors and zero warnings, the absence of warnings confirming the admin gate is now configured rather than merely quiet. Production runs from a stack outside this repository with the same history and will refuse to boot on its next rebuild unless UPLOADS_DIR is set there first. This commit does not fix that. Closes #107 Co-Authored-By: Claude Opus 5 --- docker-compose.qa.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docker-compose.qa.yml b/docker-compose.qa.yml index 65a1d78..3fb87fd 100644 --- a/docker-compose.qa.yml +++ b/docker-compose.qa.yml @@ -37,6 +37,9 @@ # QA_SMTP_USER — Brevo SMTP login. Named QA_ for the same reason as the # QA_SMTP_PASSWORD database password: pasting production's variables in here # QA_SMTP_FROM must not silently work. +# ADMIN_GATE_SECRET — the shared secret Nginx Proxy Manager injects as the +# X-Admin-Gate header on the gated location. Both sides must +# hold the same value or the admin API returns 403. See #63. services: redefined-designs-qa: @@ -98,6 +101,19 @@ services: - SITE_CURRENCY=USD - RESERVATION_MINUTES=15 - PUBLIC_URL=${PUBLIC_URL} + + # Required since #64, and previously inherited from the code's fallback — + # which is exactly what that change set out to stop. Hardcoded rather than + # taken from a stack variable because it is not a secret and because it + # has to match the right-hand side of the volume mapping below; splitting + # it across two files is how they drift apart. + - UPLOADS_DIR=/app/uploads + + # Interpolated from the stack environment so the secret itself never + # enters the repository. Note that setting it in Portainer alone is not + # enough: stack variables are substituted into this file, not handed to + # the container, so a variable with no line here never reaches the app. + - ADMIN_GATE_SECRET=${ADMIN_GATE_SECRET} volumes: # Separate uploads directory. Sharing production's would let a QA run # write into, and a QA teardown delete, real product images. From a2500a9901553d5fd3ce7f1be01dc6a60d2f1954 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 21 Aug 2026 17:35:17 -0500 Subject: [PATCH 2/2] test(backend): fail the build when the compose file lacks a required variable (#107) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The one-line compose fix in the previous commit unblocks QA. This is the part that stops it happening again, and it is the more useful half. The failure was not really a missing variable. It was that nothing connected two files: envValidation.ts gained a required variable, docker-compose.qa.yml did not set it, and nothing noticed until a container refused to boot on deploy. CI passed the whole time, because CI supplies its own environment and never reads the compose file — which is exactly why "CI is green" was the wrong evidence to have offered. So a unit test now reads the compose file and asserts it sets everything the validator demands. It imports ALWAYS_REQUIRED rather than restating it, which is the only version of this test worth having: a copied list would pass forever while the next variable added to the validator went unguarded in precisely the same way. Two further assertions earn their place. UPLOADS_DIR is hardcoded rather than taken from a stack variable on the grounds that it must agree with the volume mapping, so the test checks it against the mount rather than leaving that a claim in a comment. And ADMIN_GATE_SECRET must be present as an interpolation rather than a literal, since a secret in the repository would defeat the point of having one. There is also a test guarding the test: a regex that matched nothing would make every other assertion in the file vacuously true, so one case asserts that parsing found entries at all. Fired deliberately rather than assumed. Removing the UPLOADS_DIR line reproduces the original failure as two failing tests; restoring it returns to ten passing. A guard that has only ever been observed passing is not known to guard anything. What this cannot do is check production, which runs from a Portainer stack outside this repository. That gap is now written into the README beside the validation rules, along with the reason a variable set only in Portainer's stack UI never reaches the container: stack variables are interpolated into the compose file, not handed to the service. 172 unit tests pass, lint unchanged at 4 warnings. Refs #107 Co-Authored-By: Claude Opus 5 --- README.md | 4 + backend/src/envValidation.ts | 8 +- backend/tests/unit/composeEnvironment.test.ts | 76 +++++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 backend/tests/unit/composeEnvironment.test.ts diff --git a/README.md b/README.md index 0f8eccd..aab7e55 100755 --- a/README.md +++ b/README.md @@ -97,6 +97,10 @@ mkdir -p /tmp/redefined-uploads `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. +**Adding to the required list has reach beyond this repository.** A variable added to `ALWAYS_REQUIRED` must also be set in every environment that deploys, and there are two of those. `docker-compose.qa.yml` is checked automatically — `backend/tests/unit/composeEnvironment.test.ts` reads the validator's own list and fails if the compose file does not set something on it, which is what #107 existed to prevent from recurring. Production runs from a Portainer stack **outside this repository**, so nothing can check it: that one has to be updated by hand, before the deploy rather than during it. + +Note also that setting a variable in Portainer's stack environment is not the same as giving it to the container. Stack variables are interpolated into the compose file as `${VAR}`; a service receives exactly what its own `environment:` block lists. A variable with no line there never arrives, however carefully it was set in the UI. + **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 index 55ce96c..2d6155d 100644 --- a/backend/src/envValidation.ts +++ b/backend/src/envValidation.ts @@ -27,7 +27,13 @@ export interface EnvValidation { } // Without these the process cannot do its job at all. -const ALWAYS_REQUIRED = [ +// +// Exported so tests/unit/composeEnvironment.test.ts can assert the deploying +// environment actually sets them. #107 happened because this list grew and +// docker-compose.qa.yml did not: the check has to read this list rather than a +// copy of it, or the next variable added here goes unguarded in exactly the +// same way. +export const ALWAYS_REQUIRED = [ 'PGHOST', 'PGPORT', 'PGUSER', diff --git a/backend/tests/unit/composeEnvironment.test.ts b/backend/tests/unit/composeEnvironment.test.ts new file mode 100644 index 0000000..add172a --- /dev/null +++ b/backend/tests/unit/composeEnvironment.test.ts @@ -0,0 +1,76 @@ +import { readFileSync } from 'fs'; +import path from 'path'; +import { ALWAYS_REQUIRED } from '../../src/envValidation'; + +/** + * The guard for #107. + * + * That failure was not the missing variable — it was that nothing connected two + * files. envValidation.ts gained a required variable and docker-compose.qa.yml + * did not set it, and nothing noticed until the container refused to boot on a + * deploy. CI passed throughout, because CI sets its own environment and never + * reads the compose file. + * + * So this reads the real list from the validator rather than a copy. A copy + * would pass forever while the next added variable went unguarded in precisely + * the same way. + * + * What this cannot cover: production runs from a Portainer stack outside this + * repository, so nothing here can check it. Adding a required variable still + * means updating that stack by hand, and this test is not evidence that it was + * done. + */ + +const COMPOSE_PATH = path.resolve(__dirname, '..', '..', '..', 'docker-compose.qa.yml'); +const compose = readFileSync(COMPOSE_PATH, 'utf8'); + +// Only real environment entries — `- NAME=value` at an indented list position. +// A mention inside a comment cannot match, because a comment line starts with #. +function environmentEntries(source: string): Map { + const entries = new Map(); + for (const line of source.split('\n')) { + const match = /^\s+- ([A-Z_0-9]+)=(.*)$/.exec(line); + if (match) { + entries.set(match[1], match[2].trim()); + } + } + return entries; +} + +const entries = environmentEntries(compose); + +describe('the QA compose file provides everything the app requires to boot', () => { + it('parsed some environment entries at all', () => { + // Guards the guard: a regex that matched nothing would make every + // assertion below vacuously true. + expect(entries.size).toBeGreaterThan(5); + }); + + it.each([...ALWAYS_REQUIRED])('sets %s', (name) => { + expect(entries.has(name)).toBe(true); + }); + + // DEMO_MODE is required too, but validated separately from ALWAYS_REQUIRED + // because its rule is stricter than presence — it must be exactly 'true' or + // 'false'. Named explicitly here so it is not missed by reading only the list. + it('sets DEMO_MODE, to one of the two values that are allowed', () => { + expect(entries.has('DEMO_MODE')).toBe(true); + expect(['true', 'false']).toContain(entries.get('DEMO_MODE')); + }); + + // The reason UPLOADS_DIR is hardcoded rather than taken from a stack + // variable is that it has to agree with the volume mapping. That claim is + // only worth making if something checks it. + it('points UPLOADS_DIR at the directory the uploads volume is mounted on', () => { + const uploadsDir = entries.get('UPLOADS_DIR'); + expect(uploadsDir).toBeTruthy(); + expect(compose).toContain(`:${uploadsDir}`); + }); + + // Interpolated rather than hardcoded, so the secret itself never enters the + // repository. Present as a reference is what matters; an unset stack variable + // resolves to empty, which the validator and the gate both read as "off". + it('references ADMIN_GATE_SECRET from the stack rather than holding a value', () => { + expect(entries.get('ADMIN_GATE_SECRET')).toBe('${ADMIN_GATE_SECRET}'); + }); +});