fix(errors): pass an AppError to responses.error at the last two non-error sites

ERRORS.md

@@ -41,3 +41,4 @@ Use this file as a compact memory of recurring AI mistakes.
 - [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
+- [2026-09-05] error handling: #4062's sweep of the `responses.error(res, ...)(x)` misuse (`x` not error-shaped, so `x.details` is `undefined` and `payload.details` never emits) covered `billing.requireQuota.js` only -> two more sites outside billing: `analytics.requireFeatureFlag.js`'s 403 branch passed `{ type, flag }` flat (no `.details` wrapper) — real signal loss, a client blocked by a feature flag got no machine-readable `type` in production; `home.controller.js`'s degraded-health 503 branch passed the raw health payload directly — same wrong shape, harmless today only because nothing on that payload matches the whitelist. Fixed both by wrapping the payload in a real `AppError`'s `.details`. Deliberately did NOT whitelist `flag` (an internal PostHog feature-toggle key) alongside `type` — publishing it would let a client enumerate which flags gate which routes; a grep of every remaining `responses.error(res, ...)(x)` call site in the repo found no further non-error-shaped `x`; see pierreb-devkit/Node#4064

lib/middlewares/analytics.requireFeatureFlag.js

@@ -3,6 +3,7 @@
  */
 import FeatureFlagsService from '../services/analytics.featureFlags.js';
 
+import AppError from '../helpers/AppError.js';
 import responses from '../helpers/responses.js';
 
 /**
@@ -20,6 +21,18 @@ import responses from '../helpers/responses.js';
  * @returns {Function} Express middleware function
  */
 function requireFeatureFlag(flagName) {
+  /**
+   * Evaluate the flag for the caller and block the request when it is off.
+   * @param {import('express').Request} req - Express request object.
+   * @param {import('express').Response} res - Express response object.
+   * @param {import('express').NextFunction} next - Express next callback.
+   * @returns {Promise<object|void>} Resolves when middleware handling
+   *   completes: the `responses.error` envelope object on the 401 and 403
+   *   branches, otherwise whatever `next()` returns. Note the sibling
+   *   `requireQuotaMiddleware` in `modules/billing/middlewares/billing.requireQuota.js`
+   *   documents the same shape as `Promise<void>`, which is inaccurate for
+   *   its error branches too — not corrected here, out of this issue's scope.
+   */
   return async function requireFeatureFlagMiddleware(req, res, next) {
     const distinctId = req.user?._id ? String(req.user._id) : undefined;
     if (!distinctId) {
@@ -42,10 +55,25 @@ function requireFeatureFlag(flagName) {
         // undefined -> analytics not configured -> fail-open
         if (variant === undefined) return next();
 
-        return responses.error(res, 403, 'Forbidden', 'Feature not available on your current plan')({
-          type: 'FEATURE_FLAG_DISABLED',
-          flag: flagName,
-        });
+        // responses.error(...)(x) reads `x.details`, not `x` itself (issue
+        // #4064 — same call-convention bug fixed for billing.requireQuota.js
+        // in #4062). Pass an AppError whose `.details` carries this data,
+        // not a flat object, or the whitelist below never sees it.
+        //
+        // `type` is on the built-in whitelist (lib/helpers/responses.js
+        // DEFAULT_DETAILS_WHITELIST) and safe: it only tells a client "you
+        // were gated by a feature flag", the same class of signal as an
+        // HTTP status code. `flag` is deliberately left OFF the whitelist —
+        // it names an internal PostHog feature-toggle key, and publishing
+        // it would let any authenticated caller enumerate which flags gate
+        // which routes. It stays in `details` for dev-only debugging
+        // (the serialized-error blob), never in the production body.
+        return responses.error(res, 403, 'Forbidden', 'Feature not available on your current plan')(
+          new AppError('Feature not available on your current plan', {
+            status: 403,
+            details: { type: 'FEATURE_FLAG_DISABLED', flag: flagName },
+          }),
+        );
       }
 
       return next();

lib/middlewares/tests/analytics.requireFeatureFlag.unit.tests.js

@@ -120,4 +120,40 @@ describe('requireFeatureFlag middleware unit tests:', () => {
 
     expect(next).toHaveBeenCalledWith(error);
   });
+
+  // Issue #4064: responses.error(res, ...)(x) reads `x.details` off whatever
+  // it is handed. Before this fix, the 403 branch passed `{ type, flag }`
+  // flat, with no `.details` key at all — pickWhitelistedDetails
+  // (lib/helpers/responses.js) received `undefined` and produced no
+  // `payload.details` in ANY environment, not just production. This test
+  // forces NODE_ENV=production so the dev-only serialized-error blob
+  // (payload.error) cannot exist either — the same production-mode-toggle
+  // convention used in billing.quota.unit.tests.js (#4062) — so the only way
+  // this test can pass is via the real `payload.details` field a production
+  // client reads. Proven red against the pre-fix code (see commit history).
+  test('production mode: 403 response carries type in payload.details, not just the dev-only error blob', async () => {
+    mockFeatureFlagsService.isEnabled.mockResolvedValue(false);
+    mockFeatureFlagsService.getVariant.mockResolvedValue(false);
+
+    const previousNodeEnv = process.env.NODE_ENV;
+    process.env.NODE_ENV = 'production';
+    try {
+      const middleware = requireFeatureFlag('beta-feature');
+      await middleware(req, res, next);
+    } finally {
+      process.env.NODE_ENV = previousNodeEnv;
+    }
+
+    expect(res.status).toHaveBeenCalledWith(403);
+    const payload = res.json.mock.calls[0][0];
+    // `type` is on the built-in whitelist (lib/helpers/responses.js
+    // DEFAULT_DETAILS_WHITELIST) and is safe/useful for a client to branch
+    // on. `flag` names an internal PostHog feature-toggle key and is
+    // deliberately NOT whitelisted (see analytics.requireFeatureFlag.js) —
+    // it must never appear in the production body.
+    expect(payload.details).toEqual({ type: 'FEATURE_FLAG_DISABLED' });
+    expect(payload.details.flag).toBeUndefined();
+    // Production never leaks the raw serialized-error blob.
+    expect(payload.error).toBeUndefined();
+  });
 });

modules/home/controllers/home.controller.js

@@ -3,6 +3,7 @@
  */
 import fs from 'fs';
 
+import AppError from '../../../lib/helpers/AppError.js';
 import errors from '../../../lib/helpers/errors.js';
 import responses from '../../../lib/helpers/responses.js';
 import HomeService from '../services/home.service.js';
@@ -63,7 +64,19 @@ const health = (req, res) => {
   const isAdmin = req.user?.roles?.includes('admin');
   const payload = isAdmin ? data : { status: data.status };
   if (data.status !== 'ok') {
-    return responses.error(res, 503, 'Service Unavailable', 'degraded')(payload);
+    // responses.error(...)(x) reads `x.details`, not `x` itself (issue
+    // #4064 — same call-convention bug fixed for
+    // analytics.requireFeatureFlag.js in this issue and
+    // billing.requireQuota.js in #4062). The raw payload was passed
+    // directly, so any whitelisted key it might one day carry (a health
+    // check is exactly the kind of object that accumulates internal detail
+    // over time) would have been silently swallowed instead of reaching the
+    // whitelist. No leak today — nothing on `payload` matches
+    // DEFAULT_DETAILS_WHITELIST — but wrap it under `.details` on a real
+    // AppError so the shape is correct regardless.
+    return responses.error(res, 503, 'Service Unavailable', 'degraded')(
+      new AppError('degraded', { status: 503, details: payload }),
+    );
   }
   responses.success(res, 'health check')(payload);
 };

modules/home/tests/home.controller.unit.tests.js

@@ -0,0 +1,128 @@
+/**
+ * Module dependencies.
+ */
+import { jest, beforeEach, afterEach, describe, test, expect } from '@jest/globals';
+
+/**
+ * Unit tests for home.controller.js's `health` endpoint.
+ *
+ * Issue #4064: responses.error(res, status, title, description)(x) reads
+ * `x.details` off whatever it is handed. The degraded branch used to pass
+ * the raw health-check payload directly — not error-shaped, no `.details`
+ * key — the same wrong call convention fixed for
+ * analytics.requireFeatureFlag.js (this issue) and billing.requireQuota.js
+ * (#4062). No leak resulted (the payload carries nothing whitelisted), but
+ * the shape was wrong and a health payload is exactly the kind of object
+ * that accumulates internal detail over time.
+ */
+describe('home.controller health unit tests:', () => {
+  let health;
+  let mockHomeService;
+  let req;
+  let res;
+
+  beforeEach(async () => {
+    jest.resetModules();
+
+    mockHomeService = {
+      getHealthStatus: jest.fn(),
+    };
+
+    jest.unstable_mockModule('../services/home.service.js', () => ({
+      default: mockHomeService,
+    }));
+
+    const mod = await import('../controllers/home.controller.js');
+    health = mod.default.health;
+
+    req = { user: undefined };
+    res = {
+      status: jest.fn().mockReturnThis(),
+      json: jest.fn().mockReturnThis(),
+    };
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
+  // Proven red against the pre-fix code: before this fix, the raw payload
+  // (no `.details` key) was handed straight to responses.error(...), so
+  // pickWhitelistedDetails always received `undefined` — this call shape
+  // could never have produced a non-error object's data as `payload.details`.
+  test('production mode: degraded response carries no payload.details today (no whitelisted key on the health payload), and never leaks the raw payload flat', async () => {
+    mockHomeService.getHealthStatus.mockReturnValue({
+      status: 'degraded',
+      db: 'disconnected',
+      uptime: 12,
+      version: '1.0.0',
+      memory: { heapUsed: 1 },
+    });
+
+    const previousNodeEnv = process.env.NODE_ENV;
+    process.env.NODE_ENV = 'production';
+    try {
+      health(req, res);
+    } finally {
+      process.env.NODE_ENV = previousNodeEnv;
+    }
+
+    expect(res.status).toHaveBeenCalledWith(503);
+    const payload = res.json.mock.calls[0][0];
+    expect(payload.type).toBe('error');
+    expect(payload.message).toBe('Service Unavailable');
+    // No whitelisted key (type/upgradeUrl/retryAfter) exists on a health
+    // payload today, so payload.details stays absent — same observable
+    // behavior as before this fix. This proves the fix did not newly LEAK
+    // db/uptime/version/memory into payload.details.
+    expect(payload.details).toBeUndefined();
+    // Production never leaks the raw serialized-error blob either way.
+    expect(payload.error).toBeUndefined();
+  });
+
+  test('dev mode: the health payload is now nested under payload.error.details, not spread flat at payload.error\'s top level', async () => {
+    req.user = { roles: ['admin'] };
+    mockHomeService.getHealthStatus.mockReturnValue({
+      status: 'degraded',
+      db: 'disconnected',
+      uptime: 12,
+      version: '1.0.0',
+      memory: { heapUsed: 1 },
+    });
+
+    const previousNodeEnv = process.env.NODE_ENV;
+    process.env.NODE_ENV = 'test';
+    try {
+      health(req, res);
+    } finally {
+      process.env.NODE_ENV = previousNodeEnv;
+    }
+
+    const payload = res.json.mock.calls[0][0];
+    expect(typeof payload.error).toBe('string');
+    const errorBlob = JSON.parse(payload.error);
+    // Shape change (dev-only, never seen in production): the health data now
+    // lives under `.details` (an AppError's shape) instead of being spread
+    // at the blob's top level. Any dev-only consumer parsing payload.error
+    // for `db`/`uptime`/`version`/`memory` directly must now read
+    // payload.error.details.<field> instead.
+    expect(errorBlob.db).toBeUndefined();
+    expect(errorBlob.details).toEqual({
+      status: 'degraded',
+      db: 'disconnected',
+      uptime: 12,
+      version: '1.0.0',
+      memory: { heapUsed: 1 },
+    });
+  });
+
+  test('healthy status still returns 200 via responses.success, unaffected by this fix', async () => {
+    mockHomeService.getHealthStatus.mockReturnValue({ status: 'ok', db: 'connected' });
+
+    health(req, res);
+
+    expect(res.status).toHaveBeenCalledWith(200);
+    const payload = res.json.mock.calls[0][0];
+    expect(payload.type).toBe('success');
+  });
+});