fix(errors): curate AppError.details call sites and gate description leaks

ERRORS.md

@@ -38,3 +38,5 @@ Use this file as a compact memory of recurring AI mistakes.
 - [2026-07-25] billing: `forceRotateForPlanChange` (week-quota snapshot rotation) was wired only into `handleSubscriptionUpdated`'s plan-change block -> `handleCheckoutCompleted` and both branches of `handleSubscriptionCreated` never rotated the current-week quota snapshot after a mid-week plan activation, so an upgraded org kept the previous plan's weekly quota (often 0 on free→paid) until the next weekly reset; fixed by calling the same non-fatal, logged `forceRotateForPlanChange(organizationId, { preserveUsage: true })` from all three activation call sites, and by making `billing.usage.service.js incrementMeter` read the live plan quota (already fetched for the snapshot write) instead of the potentially-stale `updatedDoc.meterQuota` for overflow decisions — mirrors the existing live-quota display fix in `billing.controller.js`; see pierreb-devkit/Node#3988
 - [2026-09-04] deps/engines: `engines.node` declared `">=22.0.0"` while the committed lock was already npm-11-shaped, so `npm ci` under npm 10 (bundled by every Node 22.x) failed mid-install with a confusing `Missing: conventional-commits-filter@6.0.1 from lock file` error instead of a clean engines rejection -> a public package's `engines.node`/`engines.npm` must bound the exact npm major that wrote the lock, not just "a Node version that happens to work today"; pin the toolchain with `.nvmrc` + CI's `setup-node` reading it (`node-version-file`, not a floating `lts/*`) and add `engine-strict=true` to `.npmrc` so an unsupported toolchain fails fast at the engines check instead of at dependency resolution; see pierreb-devkit/Node#4053
 - [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

lib/helpers/AppError.js

@@ -8,11 +8,21 @@ const AppErrorCodes = {
 /**
  * @desc Custom error class with Node + Express
  * @param {String} message error
- * @param {Object} { status, code }
+ * @param {Object} options
+ * @param {*} [options.details] - structured/whitelisted or curated-internal data (see
+ *   `lib/helpers/responses.js#getDescription` — its `details`-derived text is
+ *   production-gated, issue #4059)
+ * @param {number} [options.status] - HTTP status code
+ * @param {string} [options.code] - stable domain error code
+ * @param {string} [options.description] - deliberately-authored, user-facing text a
+ *   throw site chooses explicitly — NOT gated by `getDescription`'s production check
+ *   (same precedence as an explicit `description` argument to `responses.error`; see
+ *   that function's own doc comment). Use this for authored copy a real user should
+ *   see in every environment; use `details` for internal/curated data instead.
  */
 
 class AppError extends Error {
-  constructor(message, { details, status, code } = {}) {
+  constructor(message, { details, status, code, description } = {}) {
     super(message);
     // Set HTTP status code
     this.status = status || 500;
@@ -27,6 +37,12 @@ class AppError extends Error {
     if (details) this.details = details;
     else this.details = [{ message }];
 
+    // Deliberately-authored, user-facing text — see the constructor's own JSDoc
+    // above. Only set when the throw site explicitly passes it; everything else
+    // that reads `err.description` (`getDescription`, `oauthErrorRedirect`)
+    // already treats an absent value as "fall through to the next source".
+    if (description) this.description = description;
+
     // Ensures the AppError subclass is sliced out of the
     // stack trace dump for clarity
     Error.captureStackTrace(this, this.constructor);

lib/helpers/responses.js

@@ -218,6 +218,16 @@ const getHttpStatus = (status, err) => {
 const getDescription = (description, err, details) => {
   if (description) return description;
   if (err?.description) return err.description;
+  // `details`-derived text gets the SAME environment gate `pickWhitelistedDetails`
+  // already applies to the `details` field above (issue #4059): outside
+  // production, resolve the full text below exactly as before — including
+  // whatever a raw caught exception's `.message` carries at a call site that
+  // curates `details: { message: err.message }` (several call sites across the
+  // stack do exactly this). In production none of that text is safe to hand a
+  // client sight-unseen, so this falls through to the same '' every other
+  // unmatched `details` shape already returns below — no explicit `description`
+  // argument or `err.description` is affected, only text sourced from `details`.
+  if (configHelper.isProd()) return '';
   try {
     if (typeof details === 'string') return details;
     if (Array.isArray(details)) {

lib/helpers/tests/responses.detailsWhitelist.unit.tests.js

@@ -463,13 +463,22 @@ describe('responses.error — a throwing `details` getter/Proxy does not crash t
  * resolution logic itself. Expected strings captured from the pre-fix
  * behavior directly (see PR #4056 discussion) — this is a behavior-lock, not
  * a spec derived from reasoning about the new code.
+ *
+ * Run under a DEV-GRADE env (issue #4059): these tests lock the RESOLUTION
+ * LOGIC (which `details` shape wins, how it's turned into a string), which
+ * still applies unchanged outside production. They used to run under
+ * `production` because, before #4059, `getDescription` had no environment
+ * gate at all — production and dev behaved identically. #4059 adds one (see
+ * the "production-gated" describe block below for that new behavior), so
+ * asserting full-text resolution now requires a dev-grade env; asserting it
+ * under `production` would just test the gate, not the resolution logic.
  */
 describe('responses.error — getDescription behavior preservation (finding-4 follow-up must not change these):', () => {
   let originalNodeEnv;
 
   beforeEach(() => {
     originalNodeEnv = process.env.NODE_ENV;
-    process.env.NODE_ENV = 'production';
+    process.env.NODE_ENV = 'development';
   });
 
   afterEach(() => {
@@ -544,6 +553,93 @@ describe('responses.error — getDescription behavior preservation (finding-4 fo
   });
 });
 
+/**
+ * Unit tests — `getDescription`'s `details`-derived text is production-gated
+ * (issue #4059, decision 2). Before this, `details.message` (and the string/
+ * array-of-{message} shapes) reached `description` in EVERY environment,
+ * including production — the second of the two leaks #4059 closes (the first
+ * is the eight raw-caught-error call sites now curated to `details: { message
+ * }`, covered elsewhere in the suite). "Generic in production" here means the
+ * same '' every other unmatched `details` shape already falls through to —
+ * there is no separate placeholder string to invent, mirroring how
+ * `pickWhitelistedDetails` above drops an unlisted key rather than replacing
+ * it with something. An explicit `description` argument or `err.description`
+ * is a call site's own deliberate choice, never sourced from `details` — both
+ * stay ungated in every environment, asserted below alongside the new gate so
+ * a regression that over-applies the gate is caught the same run.
+ */
+describe('responses.error — getDescription details-derived text is production-gated (issue #4059):', () => {
+  let originalNodeEnv;
+
+  beforeEach(() => {
+    originalNodeEnv = process.env.NODE_ENV;
+    process.env.NODE_ENV = 'production';
+  });
+
+  afterEach(() => {
+    process.env.NODE_ENV = originalNodeEnv;
+  });
+
+  test('a string `details` never reaches `description` in production', () => {
+    const res = buildRes();
+    const err = new AppError('Meter exhausted', { status: 402, details: 'internal: mongo replset primary at 10.0.4.12 unreachable' });
+    responses.error(res, 402)(err);
+    expect(res._body.description).toBe('');
+  });
+
+  test('an array-of-{message} `details` never reaches `description` in production', () => {
+    const res = buildRes();
+    const err = new AppError('Meter exhausted', { status: 402, details: [{ message: 'first' }, { message: 'second' }] });
+    responses.error(res, 402)(err);
+    expect(res._body.description).toBe('');
+  });
+
+  test('an object `details.message` never reaches `description` in production — the exact leak reported in #4059', () => {
+    const res = buildRes();
+    const err = new AppError('Meter exhausted', {
+      status: 402,
+      details: { message: 'internal: mongo replset primary at 10.0.4.12 unreachable' },
+    });
+    responses.error(res, 402)(err);
+    expect(res._body.description).toBe('');
+  });
+
+  test('a curated call-site shape (details: { message: err.message }, the post-#4059 pattern at the eight call sites) never reaches `description` in production', () => {
+    const res = buildRes();
+    const caught = new Error('ECONNREFUSED 10.0.4.12:27017 — mongo replset primary unreachable');
+    const err = new AppError('oAuth, find user failed', { code: 'SERVICE_ERROR', details: { message: caught.message } });
+    responses.error(res, 500)(err);
+    expect(res._body.description).toBe('');
+    expect(res._body.details).toBeUndefined();
+  });
+
+  test('an explicit `description` argument still wins over `details` in production — not gated, a call site\'s own deliberate text', () => {
+    const res = buildRes();
+    const err = new AppError('Meter exhausted', { status: 402, details: { message: 'should not be used' } });
+    responses.error(res, 402, 'Meter exhausted', 'explicit description')(err);
+    expect(res._body.description).toBe('explicit description');
+  });
+
+  test('`error.description` still wins over `details` in production when no explicit description argument is given — not gated', () => {
+    const res = buildRes();
+    const err = new AppError('Meter exhausted', { status: 402, details: { message: 'should not be used' } });
+    err.description = 'err-level description';
+    responses.error(res, 402)(err);
+    expect(res._body.description).toBe('err-level description');
+  });
+
+  test('a whitelisted key (upgradeUrl/type) still crosses via `details` in production — the gate is on `description` text only, not the existing whitelist mechanism', () => {
+    const res = buildRes();
+    const err = new AppError('Meter exhausted', {
+      status: 402,
+      details: { type: 'METER_EXHAUSTED', upgradeUrl: '/billing/plans' },
+    });
+    responses.error(res, 402)(err);
+    expect(res._body.description).toBe('');
+    expect(res._body.details).toEqual({ type: 'METER_EXHAUSTED', upgradeUrl: '/billing/plans' });
+  });
+});
+
 /**
  * Unit tests — `buildWhitelist` (issue #3958 review findings 2 & 3): the
  * config-provided extension to the built-in whitelist must reject