fix(express): content-negotiated 404 for unmatched routes (#3975)

lib/services/express.js

@@ -279,9 +279,22 @@ const initModulesServerRoutes = async (app) => {
     const route = await import(path.resolve(routePath));
     route.default(app);
   }
-  app.get('/{*path}', (req, res) => {
+  // Explicit root route: keeps the friendly landing page at 200, only reached
+  // when express.static served no public/index.html. Must be registered
+  // before the catch-all below, since Express 5's `/{*path}` also matches `/`.
+  app.get('/', (req, res) => {
     res.send('<center><br /><h1>Devkit Node Api</h1><h3>Available on <a href="/api/tasks">/api</a>. #LetsGetTogether</h3></center>');
   });
+  // 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) => {
+    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>');
+  });
 };
 
 /**

lib/services/tests/express.notfound.unit.tests.js

@@ -0,0 +1,155 @@
+/**
+ * Module dependencies.
+ *
+ * Unit tests for express.js initModulesServerRoutes — content-negotiated 404
+ * for unmatched routes (#3975). 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.
+ */
+import { jest, describe, test, expect, beforeEach } from '@jest/globals';
+import express from 'express';
+import request from 'supertest';
+
+describe('express initModulesServerRoutes — content-negotiated 404 (#3975):', () => {
+  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
+   */
+  const getApp = async () => {
+    jest.unstable_mockModule('../../../config/index.js', () => ({
+      default: {
+        domain: 'http://localhost:3000',
+        app: { title: 'Test', description: '', keywords: '', url: '', logo: '' },
+        secure: { ssl: false },
+        log: {},
+        bodyParser: {},
+        csrf: {},
+        cors: { origin: [], credentials: false, optionsSuccessStatus: 200 },
+        trust: { proxy: false },
+        openapi: { enable: false },
+        files: { routes: [], configs: [], policies: [], preRoutes: [], openapi: [], guides: [] },
+        analytics: { posthog: { autoCapture: false } },
+        docs: {},
+      },
+    }));
+    jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
+      default: {
+        warn: jest.fn(),
+        error: jest.fn(),
+        info: jest.fn(),
+        debug: jest.fn(),
+        getLogFormat: jest.fn().mockReturnValue('combined'),
+        getMorganOptions: jest.fn().mockReturnValue({}),
+      },
+    }));
+    jest.unstable_mockModule('../../../lib/helpers/guides.js', () => ({
+      default: { loadGuides: jest.fn().mockReturnValue([]), mergeGuidesIntoSpec: jest.fn() },
+    }));
+    jest.unstable_mockModule('../../../lib/middlewares/requestId.js', () => ({
+      default: jest.fn((req, res, next) => next()),
+    }));
+    jest.unstable_mockModule('../../../lib/middlewares/posthog-context.middleware.js', () => ({
+      posthogContextMiddleware: jest.fn((req, res, next) => next()),
+    }));
+    jest.unstable_mockModule('../../../lib/services/errorTracker.js', () => ({
+      default: { setupExpressErrorHandler: jest.fn() },
+    }));
+    jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({
+      default: { init: jest.fn().mockResolvedValue(undefined), identify: jest.fn(), groupIdentify: jest.fn() },
+    }));
+    jest.unstable_mockModule('../../../lib/middlewares/analytics.js', () => ({
+      default: jest.fn((req, res, next) => next()),
+    }));
+    jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({
+      default: { discoverPolicies: jest.fn().mockResolvedValue(undefined), defineAbilityFor: jest.fn().mockResolvedValue({}) },
+    }));
+
+    const mod = await import('../../../lib/services/express.js');
+    const app = express();
+    await mod.default.initModulesServerRoutes(app);
+    return app;
+  };
+
+  test('GET /api/nope → 404 JSON { error: "not_found" }', async () => {
+    const app = await getApp();
+
+    const res = await request(app).get('/api/nope').expect(404);
+
+    expect(res.headers['content-type']).toMatch(/json/);
+    expect(res.body).toEqual({ error: 'not_found' });
+  });
+
+  test('GET /api (no trailing slash) → 404 JSON', async () => {
+    const app = await getApp();
+
+    const res = await request(app).get('/api').expect(404);
+
+    expect(res.headers['content-type']).toMatch(/json/);
+    expect(res.body).toEqual({ error: 'not_found' });
+  });
+
+  test('GET /nope with Accept: text/html → 404 HTML, no JSON', async () => {
+    const app = await getApp();
+
+    const res = await request(app).get('/nope').set('Accept', 'text/html').expect(404);
+
+    expect(res.headers['content-type']).toMatch(/html/);
+    expect(res.text).toContain('404');
+  });
+
+  test('GET /nope with Accept: application/json → 404 JSON { error: "not_found" }', async () => {
+    const app = await getApp();
+
+    const res = await request(app).get('/nope').set('Accept', 'application/json').expect(404);
+
+    expect(res.headers['content-type']).toMatch(/json/);
+    expect(res.body).toEqual({ error: 'not_found' });
+  });
+
+  // No explicit Accept header — supertest (and curl, and most agents) sends
+  // none by default, which Express treats as an implicit `*/*`. Pins the
+  // most common agent/curl default for both branches of the negotiation.
+  test('GET /api/nope with no explicit Accept header → 404 JSON (isApiPath wins regardless of Accept)', async () => {
+    const app = await getApp();
+
+    const res = await request(app).get('/api/nope').expect(404);
+
+    expect(res.headers['content-type']).toMatch(/json/);
+    expect(res.body).toEqual({ error: 'not_found' });
+  });
+
+  test('GET /nope with no explicit Accept header → 404 HTML (implicit */* resolves to html)', async () => {
+    const app = await getApp();
+
+    const res = await request(app).get('/nope').expect(404);
+
+    expect(res.headers['content-type']).toMatch(/html/);
+    expect(res.text).toContain('404');
+  });
+
+  // Express itself matches routes case-insensitively, so the isApiPath check
+  // must too — otherwise /API/nope falls through to the HTML branch instead
+  // of the JSON 404 an API consumer expects.
+  test('GET /API/nope (mixed-case) → 404 JSON, matching /api case-insensitively', async () => {
+    const app = await getApp();
+
+    const res = await request(app).get('/API/nope').expect(404);
+
+    expect(res.headers['content-type']).toMatch(/json/);
+    expect(res.body).toEqual({ error: 'not_found' });
+  });
+
+  test('GET / → 200, friendly root page unchanged', async () => {
+    const app = await getApp();
+
+    const res = await request(app).get('/').expect(200);
+
+    expect(res.text).toContain('Devkit Node Api');
+  });
+});

modules/auth/tests/auth.authorization.integration.tests.js

@@ -161,12 +161,18 @@ describe('Authorization integration tests:', () => {
       await publicAgent.get('/api/users/stats').expect(200);
     });
 
-    test('GET /api/home/releases should return 200 for guests', async () => {
-      await publicAgent.get('/api/home/releases').expect(200);
+    // NOTE: /api/home/releases and /api/home/changelogs never existed as real
+    // routes (see modules/home/routes/home.route.js) — they only "passed" via
+    // the old implicit 200 catch-all (#3975), which is the exact false-positive
+    // the content-negotiated 404 fix removes. Repointed to real public routes
+    // so the guest-authorization intent ("guest can reach a public endpoint")
+    // still means something.
+    test('GET /api/health should return 200 for guests', async () => {
+      await publicAgent.get('/api/health').expect(200);
     });
 
-    test('GET /api/home/changelogs should return 200 for guests', async () => {
-      await publicAgent.get('/api/home/changelogs').expect(200);
+    test('GET /api/home/pages/terms should return 200 for guests', async () => {
+      await publicAgent.get('/api/home/pages/terms').expect(200);
     });
 
     test('GET /api/home/team should return 200 for guests', async () => {

modules/core/tests/core.integration.tests.js

@@ -315,12 +315,13 @@ describe('Core integration tests:', () => {
 
     it('should NOT serve a dedicated Redoc UI on /api/docs (decommissioned)', async () => {
       // The Redoc UI handler was removed. /api/docs is no longer a dedicated
-      // route, so it falls through to the generic catch-all landing (200) —
-      // NOT a Redoc reference page that loads /api/spec.json. We assert it IS
-      // the catch-all (positive) so the 200 is unambiguously the fall-through,
-      // not some other docs surface.
-      const res = await request(app).get('/api/docs').expect(200);
-      expect(res.text).toContain('Devkit Node Api'); // the generic catch-all landing
+      // route, so it falls through to the content-negotiated 404 catch-all
+      // (#3975) — NOT a Redoc reference page that loads /api/spec.json, and
+      // NOT the old implicit 200 HTML landing either. We assert the JSON 404
+      // (positive) so "decommissioned" is unambiguous: the path simply does
+      // not exist, not some other docs surface.
+      const res = await request(app).get('/api/docs').expect(404);
+      expect(res.body).toEqual({ error: 'not_found' });
       expect(res.text).not.toMatch(/redoc/i);
       expect(res.text).not.toContain('/api/spec.json');
     });