fix(express): catch-all 404 covers every HTTP verb, not just GET

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).

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('<center><br /><h1>404</h1><h3>Not Found</h3></center>');
+    return res.status(404).send('<center><br /><h1>404</h1><h3>Not Found</h3></center>');
   });
 };
 

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<import('express').Express>} 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<import('express').Express>} 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' });
+  });
 });