From de4895b8dcb75052a944fb03251d750325d2abcc Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 3 Aug 2026 10:41:51 +0200 Subject: [PATCH 1/3] fix(express): catch-all 404 covers every HTTP verb, not just GET app.get('/{*path}') left unmatched POST/PUT/PATCH/DELETE falling through to Express's default HTML finalhandler, contradicting the content-negotiated JSON 404 introduced for GET in #3975. Register the catch-all with app.all instead so every unmatched verb gets the same negotiated 404. Adds verb coverage (POST/PUT/PATCH/DELETE on API and non-API paths) and two OPTIONS tests proving, by execution rather than assumption, that a real preflight is still answered by cors and never reaches the catch-all. Also adds the MIGRATIONS.md entry the original #3975 change was missing. Closes #3978 --- MIGRATIONS.md | 8 ++ lib/services/express.js | 8 +- .../tests/express.notfound.unit.tests.js | 104 ++++++++++++++++-- 3 files changed, 111 insertions(+), 9 deletions(-) diff --git a/MIGRATIONS.md b/MIGRATIONS.md index 352ceac8d..c57a8a36b 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -4,6 +4,14 @@ Breaking changes and upgrade notes for downstream projects. --- +## Unmatched routes return 404 on every HTTP verb, not just GET (2026-08-03, closes gap from #3975) + +An earlier change replaced the implicit 200 previously returned for any unmatched path with a content-negotiated 404 (JSON `{ error: 'not_found' }` for API paths/JSON-accepting clients, minimal HTML otherwise) — but that catch-all was registered with `app.get`, so an unmatched POST/PUT/PATCH/DELETE still fell through to Express's default HTML finalhandler (`Cannot POST /...`) instead of getting the same negotiated 404. It is now registered with `app.all`, so every unmatched verb gets the same behavior. `OPTIONS` is unaffected: the `cors` middleware answers preflight requests before this route is ever reached. + +**Action required:** any readiness/health check that polls a path that isn't a declared route — regardless of HTTP verb — must instead target a real, declared route, e.g. `GET /api/health`. A check pointed at an undeclared path previously got an implicit 200 and now gets a `404`; that check will start failing instead of silently passing. + +--- + ## Migration runner: claim-with-status — interrupted runs resume instead of being skipped forever (2026-07-28) Fixes a data-integrity gap in `lib/services/migrations.js`: the runner previously claimed a migration as executed (an insert into the `migrations` collection) BEFORE calling its `up()`. A hard process kill mid-`up()` (OOM, SIGKILL, pod eviction) left that claim in place with no completion signal — on the next boot the migration was treated as already done and permanently skipped, even though `up()` never finished (found reviewing #3990's backfill: an interrupted `updateMany` could strand a subset of documents; the runner semantics were the generic root cause). diff --git a/lib/services/express.js b/lib/services/express.js index 9bdb0b9dd..b40fabdd8 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -288,7 +288,13 @@ const initModulesServerRoutes = async (app) => { // Catch-all for every other unmatched path: content-negotiated 404 instead // of the previous implicit 200 HTML (#3975). API consumers and automated // agents get a proper JSON error instead of a false-positive success. - app.get('/{*path}', (req, res) => { + // Registered with app.all (not app.get, #3978) so every unmatched verb + // (POST/PUT/PATCH/DELETE/...) gets the same JSON/HTML negotiation instead + // of falling through to Express's default HTML finalhandler. This also + // matches OPTIONS, but the cors middleware (initMiddleware) is mounted + // before routes and always answers preflight requests itself without + // calling next(), so a real preflight never reaches this handler. + app.all('/{*path}', (req, res) => { const isApiPath = /^\/api(\/|$)/i.test(req.path); if (isApiPath || req.accepts(['html', 'json']) === 'json') { return res.status(404).json({ error: 'not_found' }); diff --git a/lib/services/tests/express.notfound.unit.tests.js b/lib/services/tests/express.notfound.unit.tests.js index 6f4375a20..29653a0b5 100644 --- a/lib/services/tests/express.notfound.unit.tests.js +++ b/lib/services/tests/express.notfound.unit.tests.js @@ -2,26 +2,29 @@ * Module dependencies. * * Unit tests for express.js initModulesServerRoutes — content-negotiated 404 - * for unmatched routes (#3975). Unknown routes must no longer return an + * for unmatched routes (#3975), extended in #3978 to cover every HTTP verb + * (not just GET) via app.all. Unknown routes must no longer return an * implicit 200 HTML page; API paths and explicit JSON accepts must get a - * JSON 404, everything else a minimal HTML 404, and root behavior must be - * unchanged. + * JSON 404, everything else a minimal HTML 404, root behavior must be + * unchanged, and OPTIONS preflight must still be answered by cors rather + * than the catch-all. */ import { jest, describe, test, expect, beforeEach } from '@jest/globals'; import express from 'express'; import request from 'supertest'; -describe('express initModulesServerRoutes — content-negotiated 404 (#3975):', () => { +describe('express initModulesServerRoutes — content-negotiated 404 (#3975, #3978):', () => { beforeEach(() => { jest.resetModules(); }); /** - * Helper: extract initModulesServerRoutes from express.js (with all heavy - * deps mocked) and mount it on a fresh Express app. - * @returns {Promise} A ready-to-request Express app + * Mocks every heavy dependency initModulesServerRoutes (and, transitively, + * express.js's module scope) needs, so the real module can be imported in + * isolation. Shared by getApp and getAppWithCors below. + * @returns {void} */ - const getApp = async () => { + const mockCommonDeps = () => { jest.unstable_mockModule('../../../config/index.js', () => ({ default: { domain: 'http://localhost:3000', @@ -69,9 +72,38 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975):', jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ default: { discoverPolicies: jest.fn().mockResolvedValue(undefined), defineAbilityFor: jest.fn().mockResolvedValue({}) }, })); + }; + + /** + * Helper: extract initModulesServerRoutes from express.js (with all heavy + * deps mocked) and mount it on a fresh Express app. + * @returns {Promise} A ready-to-request Express app + */ + const getApp = async () => { + mockCommonDeps(); + + const mod = await import('../../../lib/services/express.js'); + const app = express(); + await mod.default.initModulesServerRoutes(app); + return app; + }; + + /** + * Helper: like getApp, but also mounts the REAL `cors` middleware (not a + * mock) ahead of the catch-all, mirroring production ordering — in + * lib/services/express.js `init()`, initMiddleware (which mounts cors) runs + * before initModulesServerRoutes. Used to prove, by execution rather than + * assumption, that a preflight OPTIONS request is answered by cors and + * never reaches the app.all catch-all. + * @returns {Promise} A ready-to-request Express app + */ + const getAppWithCors = async () => { + mockCommonDeps(); + const { default: cors } = await import('cors'); const mod = await import('../../../lib/services/express.js'); const app = express(); + app.use(cors({ origin: [], credentials: false, optionsSuccessStatus: 200 })); await mod.default.initModulesServerRoutes(app); return app; }; @@ -152,4 +184,60 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975):', expect(res.text).toContain('Devkit Node Api'); }); + + // #3978: the catch-all was registered with app.get only, so an unmatched + // POST/PUT/PATCH/DELETE fell through to Express's default HTML + // finalhandler instead of getting the same content-negotiated 404. It is + // now registered with app.all — verify every non-GET verb on both an API + // path and a non-API path. + describe.each(['post', 'put', 'patch', 'delete'])('%s (unmatched, all verbs must 404 like GET — #3978)', (method) => { + test(`${method.toUpperCase()} /api/nope → 404 JSON { error: "not_found" }`, async () => { + const app = await getApp(); + + const res = await request(app)[method]('/api/nope').expect(404); + + expect(res.headers['content-type']).toMatch(/json/); + expect(res.body).toEqual({ error: 'not_found' }); + }); + + test(`${method.toUpperCase()} /nope with Accept: text/html → 404 HTML, no JSON`, async () => { + const app = await getApp(); + + const res = await request(app)[method]('/nope').set('Accept', 'text/html').expect(404); + + expect(res.headers['content-type']).toMatch(/html/); + expect(res.text).toContain('404'); + }); + }); + + // #3978: app.all also matches OPTIONS, which is the one real risk the issue + // calls out — a preflight request must still get cors' own answer, not our + // 404. Two tests prove this by execution, not assumption: first that the + // catch-all really does match OPTIONS when nothing else intercepts it + // (i.e. the risk is real), then that production's actual middleware order + // (cors mounted before routes) prevents it. + test('OPTIONS /api/nope with no cors mounted → falls through to the catch-all (proves app.all matches OPTIONS)', async () => { + const app = await getApp(); + + const res = await request(app).options('/api/nope').expect(404); + + expect(res.headers['content-type']).toMatch(/json/); + expect(res.body).toEqual({ error: 'not_found' }); + }); + + test('OPTIONS preflight is answered by the real cors middleware before the catch-all (production ordering, #3978)', async () => { + const app = await getAppWithCors(); + + const res = await request(app) + .options('/api/anything') + .set('Origin', 'https://example.com') + .set('Access-Control-Request-Method', 'GET') + .expect(200); + + // cors' own preflight response: it sets Access-Control-Allow-Methods and + // ends the request itself (optionsSuccessStatus: 200), never calling + // next() — so the catch-all's JSON 404 body must never appear here. + expect(res.headers['access-control-allow-methods']).toBeDefined(); + expect(res.body).not.toEqual({ error: 'not_found' }); + }); }); From 4ff91d91052f62cf1a928335a09bd08430a78d26 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 3 Aug 2026 10:46:56 +0200 Subject: [PATCH 2/3] refactor(simplify): collapse test app builders, dedupe OPTIONS coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge getApp/getAppWithCors into one getApp({ withCors }) builder and fold the OPTIONS-without-cors case into the existing verb matrix instead of a near-duplicate standalone test. No behavior change — same 19 assertions, less repetition. --- .../tests/express.notfound.unit.tests.js | 73 ++++++++----------- 1 file changed, 29 insertions(+), 44 deletions(-) diff --git a/lib/services/tests/express.notfound.unit.tests.js b/lib/services/tests/express.notfound.unit.tests.js index 29653a0b5..1660befc0 100644 --- a/lib/services/tests/express.notfound.unit.tests.js +++ b/lib/services/tests/express.notfound.unit.tests.js @@ -21,7 +21,7 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975, #39 /** * Mocks every heavy dependency initModulesServerRoutes (and, transitively, * express.js's module scope) needs, so the real module can be imported in - * isolation. Shared by getApp and getAppWithCors below. + * isolation. Used by getApp below. * @returns {void} */ const mockCommonDeps = () => { @@ -76,34 +76,27 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975, #39 /** * Helper: extract initModulesServerRoutes from express.js (with all heavy - * deps mocked) and mount it on a fresh Express app. + * deps mocked) and mount it on a fresh Express app. `withCors: true` also + * mounts the REAL `cors` middleware (not a mock) ahead of the catch-all, + * with the same order and config express.js's own `init()` uses (there, + * initMiddleware — which mounts cors — runs before + * initModulesServerRoutes). This only checks that cors' own preflight + * handling wins when wired in that order and config; it does not assert + * that `init()` itself preserves the order (a full-boot test would need + * to mock most of `init()`'s other steps too — out of scope here). + * @param {object} [options] - Options + * @param {boolean} [options.withCors] - Mount the real cors middleware ahead of the catch-all * @returns {Promise} A ready-to-request Express app */ - const getApp = async () => { + const getApp = async ({ withCors = false } = {}) => { mockCommonDeps(); const mod = await import('../../../lib/services/express.js'); const app = express(); - await mod.default.initModulesServerRoutes(app); - return app; - }; - - /** - * Helper: like getApp, but also mounts the REAL `cors` middleware (not a - * mock) ahead of the catch-all, mirroring production ordering — in - * lib/services/express.js `init()`, initMiddleware (which mounts cors) runs - * before initModulesServerRoutes. Used to prove, by execution rather than - * assumption, that a preflight OPTIONS request is answered by cors and - * never reaches the app.all catch-all. - * @returns {Promise} A ready-to-request Express app - */ - const getAppWithCors = async () => { - mockCommonDeps(); - - const { default: cors } = await import('cors'); - const mod = await import('../../../lib/services/express.js'); - const app = express(); - app.use(cors({ origin: [], credentials: false, optionsSuccessStatus: 200 })); + if (withCors) { + const { default: cors } = await import('cors'); + app.use(cors({ origin: [], credentials: false, optionsSuccessStatus: 200 })); + } await mod.default.initModulesServerRoutes(app); return app; }; @@ -186,11 +179,13 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975, #39 }); // #3978: the catch-all was registered with app.get only, so an unmatched - // POST/PUT/PATCH/DELETE fell through to Express's default HTML - // finalhandler instead of getting the same content-negotiated 404. It is - // now registered with app.all — verify every non-GET verb on both an API - // path and a non-API path. - describe.each(['post', 'put', 'patch', 'delete'])('%s (unmatched, all verbs must 404 like GET — #3978)', (method) => { + // POST/PUT/PATCH/DELETE (or OPTIONS, absent cors) fell through to + // Express's default HTML finalhandler instead of getting the same + // content-negotiated 404. It is now registered with app.all — verify + // every non-GET verb on both an API path and a non-API path. `options` is + // included here (with no cors mounted) to prove the catch-all really does + // match it — the risk the dedicated cors-ordering test below mitigates. + describe.each(['post', 'put', 'patch', 'delete', 'options'])('%s (unmatched, all verbs must 404 like GET — #3978)', (method) => { test(`${method.toUpperCase()} /api/nope → 404 JSON { error: "not_found" }`, async () => { const app = await getApp(); @@ -210,23 +205,13 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975, #39 }); }); - // #3978: app.all also matches OPTIONS, which is the one real risk the issue - // calls out — a preflight request must still get cors' own answer, not our - // 404. Two tests prove this by execution, not assumption: first that the - // catch-all really does match OPTIONS when nothing else intercepts it - // (i.e. the risk is real), then that production's actual middleware order - // (cors mounted before routes) prevents it. - test('OPTIONS /api/nope with no cors mounted → falls through to the catch-all (proves app.all matches OPTIONS)', async () => { - const app = await getApp(); - - const res = await request(app).options('/api/nope').expect(404); - - expect(res.headers['content-type']).toMatch(/json/); - expect(res.body).toEqual({ error: 'not_found' }); - }); - + // #3978: app.all also matches OPTIONS preflight — the one real risk the + // issue calls out. The loop above already proves the catch-all matches it + // when nothing else intercepts; this proves production's actual + // middleware order (cors mounted ahead of routes in express.js `init()`) + // means a real preflight never gets there. test('OPTIONS preflight is answered by the real cors middleware before the catch-all (production ordering, #3978)', async () => { - const app = await getAppWithCors(); + const app = await getApp({ withCors: true }); const res = await request(app) .options('/api/anything') From 442f41a2e84caf583ba58d7c69391b4b0a80e9c7 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Mon, 3 Aug 2026 11:08:09 +0200 Subject: [PATCH 3/3] fix(review): address CodeRabbit findings on #4010 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add the required JSDoc header to the catch-all 404 handler (documents req/res/return, and returns the HTML response too so both branches match the documented return type). - Trim mockCommonDeps/getApp JSDoc to one-line descriptions per the repo's JSDoc convention, moving narrative detail to plain comments. - Correct MIGRATIONS.md: the implicit-200-to-404 status flip only ever applied to unmatched GET (#3975); unmatched POST/PUT/PATCH/ DELETE already 404'd via Express's default handler before this PR — what changes here is the response shape (now content-negotiated), not the status code, for those verbs. --- MIGRATIONS.md | 6 ++--- lib/services/express.js | 8 ++++++- .../tests/express.notfound.unit.tests.js | 24 +++++++++---------- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/MIGRATIONS.md b/MIGRATIONS.md index c57a8a36b..124dfcf0d 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -4,11 +4,11 @@ Breaking changes and upgrade notes for downstream projects. --- -## Unmatched routes return 404 on every HTTP verb, not just GET (2026-08-03, closes gap from #3975) +## Unmatched routes return the same content-negotiated 404 on every HTTP verb (2026-08-03, closes gap from #3975) -An earlier change replaced the implicit 200 previously returned for any unmatched path with a content-negotiated 404 (JSON `{ error: 'not_found' }` for API paths/JSON-accepting clients, minimal HTML otherwise) — but that catch-all was registered with `app.get`, so an unmatched POST/PUT/PATCH/DELETE still fell through to Express's default HTML finalhandler (`Cannot POST /...`) instead of getting the same negotiated 404. It is now registered with `app.all`, so every unmatched verb gets the same behavior. `OPTIONS` is unaffected: the `cors` middleware answers preflight requests before this route is ever reached. +An earlier change (#3975) replaced the implicit 200 previously returned for any unmatched **GET** with a content-negotiated 404 (JSON `{ error: 'not_found' }` for API paths/JSON-accepting clients, minimal HTML otherwise) — but the catch-all was registered with `app.get` only. Unmatched POST/PUT/PATCH/DELETE were unaffected by that change and already 404'd via Express's own default finalhandler, just as unstructured HTML (`Cannot POST /...`) rather than the negotiated shape. The catch-all is now registered with `app.all`, so every unmatched verb gets the same negotiated 404 body/content-type as GET. `OPTIONS` is unaffected: the `cors` middleware answers preflight requests before this route is ever reached. -**Action required:** any readiness/health check that polls a path that isn't a declared route — regardless of HTTP verb — must instead target a real, declared route, e.g. `GET /api/health`. A check pointed at an undeclared path previously got an implicit 200 and now gets a `404`; that check will start failing instead of silently passing. +**Action required:** this only changes the response *shape* for non-GET verbs (still a 404, now content-negotiated); the status-code flip from implicit 200 to 404 only ever applied to GET, and only ever needed action there (per #3975: any readiness/health check must target a real, declared route, e.g. `GET /api/health`, not an undeclared path). If any tooling parses the *body* of a 404 on a non-GET verb expecting HTML, it now gets the negotiated JSON/HTML shape instead. --- diff --git a/lib/services/express.js b/lib/services/express.js index b40fabdd8..e866331b8 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -294,12 +294,18 @@ const initModulesServerRoutes = async (app) => { // matches OPTIONS, but the cors middleware (initMiddleware) is mounted // before routes and always answers preflight requests itself without // calling next(), so a real preflight never reaches this handler. + /** + * Content-negotiated 404 for any unmatched path/verb. + * @param {import('express').Request} req - Incoming request + * @param {import('express').Response} res - Outgoing response + * @returns {import('express').Response} The 404 response (JSON for API paths/JSON-accepting clients, HTML otherwise) + */ app.all('/{*path}', (req, res) => { const isApiPath = /^\/api(\/|$)/i.test(req.path); if (isApiPath || req.accepts(['html', 'json']) === 'json') { return res.status(404).json({ error: 'not_found' }); } - res.status(404).send('

404

Not Found

'); + return res.status(404).send('

404

Not Found

'); }); }; diff --git a/lib/services/tests/express.notfound.unit.tests.js b/lib/services/tests/express.notfound.unit.tests.js index 1660befc0..7bcd1a8dd 100644 --- a/lib/services/tests/express.notfound.unit.tests.js +++ b/lib/services/tests/express.notfound.unit.tests.js @@ -19,11 +19,12 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975, #39 }); /** - * Mocks every heavy dependency initModulesServerRoutes (and, transitively, - * express.js's module scope) needs, so the real module can be imported in - * isolation. Used by getApp below. + * Mocks every heavy dependency initModulesServerRoutes needs. Used by getApp below. * @returns {void} */ + // Mocks transitively cover express.js's whole module scope (config, logger, + // guides, requestId, posthog context, error tracker, analytics, policy) so + // the real express.js module can be imported in isolation. const mockCommonDeps = () => { jest.unstable_mockModule('../../../config/index.js', () => ({ default: { @@ -75,19 +76,18 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975, #39 }; /** - * Helper: extract initModulesServerRoutes from express.js (with all heavy - * deps mocked) and mount it on a fresh Express app. `withCors: true` also - * mounts the REAL `cors` middleware (not a mock) ahead of the catch-all, - * with the same order and config express.js's own `init()` uses (there, - * initMiddleware — which mounts cors — runs before - * initModulesServerRoutes). This only checks that cors' own preflight - * handling wins when wired in that order and config; it does not assert - * that `init()` itself preserves the order (a full-boot test would need - * to mock most of `init()`'s other steps too — out of scope here). + * Extracts initModulesServerRoutes from express.js and mounts it on a fresh Express app. * @param {object} [options] - Options * @param {boolean} [options.withCors] - Mount the real cors middleware ahead of the catch-all * @returns {Promise} A ready-to-request Express app */ + // `withCors: true` mounts the REAL `cors` middleware (not a mock) ahead of + // the catch-all, with the same order and config express.js's own `init()` + // uses (there, initMiddleware — which mounts cors — runs before + // initModulesServerRoutes). This only checks that cors' own preflight + // handling wins when wired in that order and config; it does not assert + // that `init()` itself preserves the order (a full-boot test would need + // to mock most of `init()`'s other steps too — out of scope here). const getApp = async ({ withCors = false } = {}) => { mockCommonDeps();