diff --git a/MIGRATIONS.md b/MIGRATIONS.md index 352ceac8d..124dfcf0d 100644 --- a/MIGRATIONS.md +++ b/MIGRATIONS.md @@ -4,6 +4,14 @@ Breaking changes and upgrade notes for downstream projects. --- +## Unmatched routes return the same content-negotiated 404 on every HTTP verb (2026-08-03, closes gap from #3975) + +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:** 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. + +--- + ## 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..e866331b8 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -288,12 +288,24 @@ 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. + /** + * 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 6f4375a20..7bcd1a8dd 100644 --- a/lib/services/tests/express.notfound.unit.tests.js +++ b/lib/services/tests/express.notfound.unit.tests.js @@ -2,26 +2,30 @@ * 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 needs. Used by getApp below. + * @returns {void} */ - const getApp = async () => { + // 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: { domain: 'http://localhost:3000', @@ -69,9 +73,30 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975):', jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ default: { discoverPolicies: jest.fn().mockResolvedValue(undefined), defineAbilityFor: jest.fn().mockResolvedValue({}) }, })); + }; + + /** + * 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(); const mod = await import('../../../lib/services/express.js'); const app = express(); + if (withCors) { + const { default: cors } = await import('cors'); + app.use(cors({ origin: [], credentials: false, optionsSuccessStatus: 200 })); + } await mod.default.initModulesServerRoutes(app); return app; }; @@ -152,4 +177,52 @@ 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 (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(); + + 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 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 getApp({ withCors: true }); + + 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' }); + }); });