fix(billing): pass the AppError to responses.error so the 402 whitelist emits in production

ERRORS.md

@@ -40,3 +40,4 @@ Use this file as a compact memory of recurring AI mistakes.
 - [2026-09-04] docker: #4053 pinned the toolchain for CI and local installs, but the Dockerfile still copied `package*.json`, ran `npm ci`, and only then `COPY . .` — so `.npmrc` (`engine-strict=true`) landed AFTER the install, and `FROM node:lts-slim` floated independently of `.nvmrc`, so an unsupported base image inside Docker still reproduced the confusing lockfile error instead of a clean `EBADENGINE`, and the image could silently drift onto a newer Node major once it enters LTS -> a toolchain fail-fast fix must be verified in every install path, not just the one exercised by CI; copy `.npmrc` alongside `package*.json` before `npm ci` in every installing stage, and pin `FROM` to the `.nvmrc` major with a unit test checking the two against each other instead of a floating tag; see pierreb-devkit/Node#4060
 - [2026-09-04] error handling: eight call sites (`auth.controller.js` OAuth catch blocks, `uploads.repository.js` GridFS catch blocks) built `AppError` with `details: err` / `details: err.details || err` — a raw caught exception handed wholesale to the response layer — and `getDescription` (`lib/helpers/responses.js`) read `details.message` into the client-facing `description` field in EVERY environment, including production, so whatever a dependency's error object happened to carry (stack fragments, internal hostnames, driver taxonomy) decided what leaked -> curate `details` at each throw site to an explicit, deliberately-chosen field set (here: `{ message: err.message }` — a short reason, nothing else) instead of forwarding the exception, AND separately production-gate every `details`-derived read (`getDescription`'s string/array/`.message` resolution, plus `auth.controller.js`'s `oauthErrorRedirect` — a SECOND, independent consumer of `details.message` that bypasses `getDescription` entirely by building its own redirect envelope by hand) — NOTE the "same way" wording below turned out to be only partially true, see the 2026-09-05 follow-up entry; curating the call site alone is not sufficient — the curated `message` still flows through the same `details.message` slot every consumer reads, so both the source (call sites) and every sink (each place that reads `details.message`) must be fixed together; see pierreb-devkit/Node#4059
 - [2026-09-05] error handling / review follow-up: #4059's review surfaced two gaps in the 2026-09-04 fix -> (1) "eight call sites" undercounted: `users.images.controller.js updateAvatar` forwarded a raw Multer error wholesale via `details: req.multerErr` (`code`/`field`/etc. included), the identical anti-pattern, curated the same way (`details: { message: req.multerErr.message }`); (2) `oauthErrorRedirect` was NOT gated "the same way" as `getDescription` as claimed — only its `details.message` read was production-gated, its `title` (`err?.message || fallbackTitle`) was not, so a future non-AppError with a dynamic `.message` could still leak in production (today's real producers are safe, but nothing enforced it) -> when a fix claims parity with an existing safeguard ("gated the same way"), verify EVERY read the safeguard covers has a matching gate, not just the one that prompted the fix; and re-grep the whole pattern (not just the files named in the originating issue) before trusting a count like "eight call sites" — see the two new tests added for `oauthErrorRedirect`'s title gate and `users.images.controller.js`'s curation; see pierreb-devkit/Node#4059
+- [2026-09-05] billing/error handling: `billing.requireQuota.js`'s catch block extracted `const details = err.details` (for branching on the AppError sub-type) and then passed that extracted sub-object — not `err` itself — into `responses.error(res, status, ...)(details)` at all five call sites (402/429/503) -> `responses.error` reads `error.details` off whatever it is handed, so it read `details.details`, always `undefined`, silently dropping the whitelisted `type`/`upgradeUrl` payload from every response this middleware ever sent, in every environment; fix = pass `err` (keep the extracted `details` var only for the `?.type ===` branching). This also reshaped the dev-only `payload.error` blob: it now serializes the real AppError, so its curated fields moved from the blob's top level to nested under `.details` — eight existing tests across two files (`billing.quota.unit.tests.js`, `billing.webhook.hardening.unit.tests.js`) were asserting the old (buggy) flat shape and needed the same nested-path update; see pierreb-devkit/Node#4062

modules/billing/middlewares/billing.requireQuota.js

@@ -56,26 +56,42 @@ function requireQuota(resource, action) {
       return next();
     } catch (err) {
       // Map AppError status codes to HTTP responses matching previous behavior.
-      // Extract denial details from the AppError (may be array or object).
+      // `details` here is used ONLY to branch on the AppError sub-type — it
+      // must never be what gets handed to `responses.error(...)` below. That
+      // function reads `error.details` itself off whatever object it is
+      // given, so it needs the AppError `err`, not this already-extracted
+      // sub-object (issue #4062: passing `details` made it read
+      // `details.details`, always `undefined`, which silently dropped the
+      // whitelisted `type`/`upgradeUrl` payload from every response this
+      // middleware sends).
+      // `err.details` may in principle be array-shaped — `AppError` defaults
+      // `details` to `[{ message }]` when a throw site omits it — hence the
+      // unwrap below. Unreachable today (every throw in
+      // billing.quota.service.js passes a plain object), and it wouldn't
+      // help production either way: `responses.error` reads `err.details`
+      // directly, not this local `details`, and `pickWhitelistedDetails`
+      // (lib/helpers/responses.js) returns `undefined` for any array-shaped
+      // `details` — so an array-shaped `err.details` ships with no
+      // `payload.details` at all, regardless of which branch below fires.
       const details = Array.isArray(err.details) ? err.details[0] : err.details;
 
       if (err.status === 402) {
         if (details?.type === 'PAYMENT_PAST_DUE') {
-          return responses.error(res, 402, 'Payment Required', 'Subscription past due, please update payment')(details);
+          return responses.error(res, 402, 'Payment Required', 'Subscription past due, please update payment')(err);
         }
         if (details?.type === 'METER_EXHAUSTED') {
-          return responses.error(res, 402, 'Payment Required', 'Meter exhausted')(details);
+          return responses.error(res, 402, 'Payment Required', 'Meter exhausted')(err);
         }
         // Defensive: an unknown 402 sub-type would leak err.message verbatim.
         // The service only throws known types today, so send the generic phrase
         // instead — any future 402 type must be mapped explicitly above.
-        return responses.error(res, 402, 'Payment Required', 'Payment required')(details);
+        return responses.error(res, 402, 'Payment Required', 'Payment required')(err);
       }
       if (err.status === 429) {
-        return responses.error(res, 429, 'Quota exceeded', 'You have reached the usage limit for this resource')(details);
+        return responses.error(res, 429, 'Quota exceeded', 'You have reached the usage limit for this resource')(err);
       }
       if (err.status === 503) {
-        return responses.error(res, 503, 'Service Unavailable', 'Billing plan configuration is temporarily unavailable')(details);
+        return responses.error(res, 503, 'Service Unavailable', 'Billing plan configuration is temporarily unavailable')(err);
       }
       return next(err);
     }