diff --git a/ERRORS.md b/ERRORS.md index 11ff7b9af..dbc9d3f7a 100644 --- a/ERRORS.md +++ b/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 diff --git a/lib/middlewares/analytics.requireFeatureFlag.js b/lib/middlewares/analytics.requireFeatureFlag.js index 3f689b04c..659e1da3e 100644 --- a/lib/middlewares/analytics.requireFeatureFlag.js +++ b/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} 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`, 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(); diff --git a/lib/middlewares/tests/analytics.requireFeatureFlag.unit.tests.js b/lib/middlewares/tests/analytics.requireFeatureFlag.unit.tests.js index ffd4a5786..87b904864 100644 --- a/lib/middlewares/tests/analytics.requireFeatureFlag.unit.tests.js +++ b/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(); + }); }); diff --git a/modules/home/controllers/home.controller.js b/modules/home/controllers/home.controller.js index fe8e20984..304c80181 100644 --- a/modules/home/controllers/home.controller.js +++ b/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); }; diff --git a/modules/home/tests/home.controller.unit.tests.js b/modules/home/tests/home.controller.unit.tests.js new file mode 100644 index 000000000..f3a786ecb --- /dev/null +++ b/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. 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'); + }); +});