fix(api): treat blank env vars as unset; verify the whole flow in Docker Compose
Found by running the delivery flow against the actual built container rather
than a local dev process.
DEPLOY-02 (Critical) — ADMIN LOGIN RETURNED 500 FOR EVERY USER IN DOCKER.
docker-compose.yml passes each optional variable as ${X:-}, so an unconfigured
one arrives as "" rather than undefined. The code read them with
`process.env.X ?? 'default'`, which only falls back on undefined, so the empty
string reached jsonwebtoken and it threw
"expiresIn" should be a number of seconds or string representing a timespan
on every sign. Invisible to every existing suite, all of which run against a
local process where the variable is genuinely absent.
Added src/common/env.ts (envOr / envOrNull), which treat blank as unset, and
applied it to every auth, port, upload and SMS read. Three were not cosmetic:
a blank UPLOAD_DIR would have served static files from the FILESYSTEM ROOT, a
blank PORT would have bound a random ephemeral port (Number('') === 0), and a
blank JWT_SECRET would have become the verifying key. Recorded as D25, with a
standing rule to use these readers instead of `??`.
Pinned by src/common/env.spec.ts (9 tests), which reproduces the original throw
against the real jsonwebtoken and asserts the reader can no longer produce it.
DEPLOY-03 (process, documented) — the API image is `COPY release/ ./`, so it
ships a bundle pre-built on the developer machine. `docker compose build` will
happily produce an image from a STALE release/app.js and report success; at the
start of this run that bundle was four days old and the rebuilt container was
missing every route added since. DELIVERY.md's deploy section now leads with
`pnpm build:release`.
New: scripts/test-compose-e2e.cjs — the whole delivery flow against the built
container, over HTTP only, as a black box: a sales expert and an ordinary
customer both sign in with the master code, both place an order, both orders
appear in the admin grid and open there, and the three token boundaries still
hold in the deployed image. 32/32.
Verified: compose e2e 32/32 · jest 54/54 · build 5/5 ·
against the CONTAINER: complete-profile 38/38, report 40/40, expert login 36/36 ·
locally: apps smoke 43/43, admin smoke 43/43, B2B scan 34 ok/0 broken.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
3f549f1749
commit
24b7a4c395
@@ -1338,3 +1338,26 @@ than rendering a fabricated value. Recorded as REP-01.
|
||||
The reference gated the page on `myStore.hasMCode()`; with no mcode in the
|
||||
target the gate is simply a session, and the server scopes every row to the
|
||||
token.
|
||||
|
||||
## D25 — blank env vars are treated as unset, 2026-09-23
|
||||
|
||||
Found by the pre-delivery compose run (DEPLOY-02): **admin login returned 500
|
||||
for every user in Docker.** `docker-compose.yml` passes each optional variable
|
||||
as `${X:-}`, so an unconfigured one arrives as `""`, not `undefined`. The code
|
||||
read them with `process.env.X ?? 'default'`, which only falls back on
|
||||
`undefined` — so the empty string won and `jsonwebtoken` threw on every sign.
|
||||
|
||||
Invisible to every existing suite, all of which ran against a local dev process
|
||||
where the variable is genuinely absent.
|
||||
|
||||
`src/common/env.ts` now provides `envOr` / `envOrNull`, which treat blank as
|
||||
unset, and every auth, port, upload and SMS read uses them. Three of those were
|
||||
not merely cosmetic: a blank `UPLOAD_DIR` would have served static files from
|
||||
the **filesystem root**, a blank `PORT` would have bound a random ephemeral port
|
||||
(`Number('') === 0`), and a blank `JWT_SECRET` would have become the verifying
|
||||
key. Pinned by `src/common/env.spec.ts`, which reproduces the original throw
|
||||
against the real `jsonwebtoken` and asserts the reader can no longer produce it.
|
||||
|
||||
**Standing rule from here on:** read an environment variable with `envOr` /
|
||||
`envOrNull`, never with `??`, unless an empty string is a meaningful value for
|
||||
that specific variable.
|
||||
|
||||
@@ -165,3 +165,10 @@ Severity: `Critical` (money loss / security / data loss) · `High` · `Medium`
|
||||
| **REP-01** | Medium (documented gap) | REF external Tahesab system vs TGT `LedgerEntry` | The reference's statement carried columns the target has no source for: foreign currency (`rial_V2`/`rial_Kh2`/`tahesabP2`), `vaznPlastik`, `nO_kar` (نام کارساخته), `jens_felez` (جنس فلز) and the external system's internal ids (`gid`/`rec`/`reg`). | `henza_pwa/src/type/accounting.ts`; `AccountingDetail.vue:34-46` | The API returns `null`/zero for these and the UI **omits** the row rather than showing a fabricated value. If the business needs them, they need columns on `LedgerEntry` first. |
|
||||
| **REP-02** | Low (performance) | TGT `customer-report.service.ts` `summary()` | The running balance makes a row depend on every entry before it, so the whole confirmed ledger for the customer is folded in memory and the page sliced afterwards. Cheap at one customer's own document counts; linear in their history. | `summary()` `findMany` without `take` | If a customer ever accumulates enough entries for it to matter, move the fold to a stored running total written at entry time. |
|
||||
| **SEC-17** | Medium (reference, not applicable to target) | REF `Customer/TahesabController.php` | The reference calls the external accounting system with `Http::withoutVerifying()` (TLS certificate verification **disabled**) against a bare IP, with a bearer token and an `sfx` secret hardcoded in the source and embedded in the URL. | `->withToken("S3F1P8…")`, `sfx=wcG97tkF…` in four URLs | Not applicable — the target makes no external call. Listed so it is not reintroduced if a Tahesab integration is ever added; the credentials in that reference file should be rotated regardless. |
|
||||
|
||||
## Found by the pre-delivery Docker Compose run (2026-09-23)
|
||||
|
||||
| ID | Severity | Where | Description | Evidence | Suggested fix |
|
||||
|---|---|---|---|---|---|
|
||||
| **DEPLOY-02** | ~~**Critical**~~ **FIXED** | TGT `auth.module.ts`, `customer-auth.service.ts`, `jwt.strategy.ts`, `main.ts`, `upload-path.ts`, `sms.service.ts` | **Admin login returned 500 for every user in Docker.** `docker-compose.yml` passes each optional variable as `${X:-}`, so an unconfigured one arrives as `""`, not `undefined`. The code read them with `process.env.X ?? 'default'`, which only falls back on `undefined` — so the empty string won and `jsonwebtoken` threw `"expiresIn" should be a number of seconds or string representing a timespan` on **every** sign. Invisible to every suite that ran against a local dev process, where the variable is genuinely absent. The same pattern also meant a blank `UPLOAD_DIR` would have served static files from the **filesystem root**, and a blank `PORT` would have bound a random ephemeral port (`Number('') === 0`), making the container unreachable. | Container log: `ERROR [ExceptionsHandler] "expiresIn" should be a number of seconds or string representing a timespan` | ✅ Added `src/common/env.ts` (`envOr` / `envOrNull`) which treats blank as unset, and applied it to every auth, port, upload and SMS read. Pinned by `src/common/env.spec.ts` (9 tests), which reproduces the original throw against the real `jsonwebtoken` and asserts the reader can no longer produce it. Re-verified end to end in compose: **32/32**. |
|
||||
| **DEPLOY-03** | Medium (process) | TGT `apps/api/Dockerfile` | The image is `COPY release/ ./` — it ships a **pre-built bundle** produced on the developer machine by `pnpm build:release`, not a build performed inside the image. So `docker compose build` happily produces an image from a **stale** `release/app.js` and reports success. At the start of this run that bundle was four days old, and the rebuilt container was missing every route added since. | `apps/api/release/app.js` dated Sep 19 while the source was Sep 23; the container mapped none of the new routes | **`pnpm build:release` MUST be run before `docker compose build`.** Noted in DELIVERY.md's deploy section. A longer-term fix is to build inside the Dockerfile, or to have the build fail when `release/` is older than `src/`. |
|
||||
|
||||
Reference in New Issue
Block a user