From 232787084f03c51e587b02e0f753f3c60fe1c08b Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 17 Jul 2026 16:21:49 +0200 Subject: [PATCH 1/2] fix(express): content-negotiated 404 for unmatched routes (#3975) The catch-all route previously sent the friendly "Devkit Node Api" HTML snippet with an implicit 200 for every unmatched path, including /api/* typos. API consumers and automated agents relying on status codes for error handling saw false-positive success responses. Register the root route explicitly (keeps the friendly HTML at 200, only reached when static hosting served no index.html) before the catch-all, since Express 5's `/{*path}` also matches `/`. The catch-all now returns 404 with JSON `{ error: 'not_found' }` for /api/* paths or when the request explicitly accepts application/json, and a minimal HTML 404 otherwise. Non-GET methods are unaffected (already 404 by default). Adds unit tests covering: unknown /api path -> JSON 404, /api without trailing slash -> JSON 404, unknown page with Accept: text/html -> HTML 404, unknown page with Accept: application/json -> JSON 404, and root route unchanged at 200. --- lib/services/express.js | 15 ++- .../tests/express.notfound.unit.tests.js | 122 ++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) create mode 100644 lib/services/tests/express.notfound.unit.tests.js diff --git a/lib/services/express.js b/lib/services/express.js index 9d546d481..a2b7f9743 100644 --- a/lib/services/express.js +++ b/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('

Devkit Node Api

Available on /api. #LetsGetTogether

'); }); + // 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(\/|$)/.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

'); + }); }; /** diff --git a/lib/services/tests/express.notfound.unit.tests.js b/lib/services/tests/express.notfound.unit.tests.js new file mode 100644 index 000000000..c925925aa --- /dev/null +++ b/lib/services/tests/express.notfound.unit.tests.js @@ -0,0 +1,122 @@ +/** + * 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} 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' }); + }); + + 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'); + }); +}); From 446ae2c87a3a0eb3b4e8ade9bd728715a2712a82 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Fri, 17 Jul 2026 16:45:01 +0200 Subject: [PATCH 2/2] test(express): update tests that relied on the 200 catch-all + case-insensitive api match (#3975) --- lib/services/express.js | 2 +- .../tests/express.notfound.unit.tests.js | 33 +++++++++++++++++++ .../auth.authorization.integration.tests.js | 14 +++++--- modules/core/tests/core.integration.tests.js | 13 ++++---- 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/lib/services/express.js b/lib/services/express.js index a2b7f9743..9bdb0b9dd 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -289,7 +289,7 @@ const initModulesServerRoutes = async (app) => { // 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(\/|$)/.test(req.path); + 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 c925925aa..6f4375a20 100644 --- a/lib/services/tests/express.notfound.unit.tests.js +++ b/lib/services/tests/express.notfound.unit.tests.js @@ -112,6 +112,39 @@ describe('express initModulesServerRoutes — content-negotiated 404 (#3975):', 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(); diff --git a/modules/auth/tests/auth.authorization.integration.tests.js b/modules/auth/tests/auth.authorization.integration.tests.js index 307d36f3e..6cf346596 100644 --- a/modules/auth/tests/auth.authorization.integration.tests.js +++ b/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 () => { diff --git a/modules/core/tests/core.integration.tests.js b/modules/core/tests/core.integration.tests.js index cf6fe0c77..e5461ac99 100644 --- a/modules/core/tests/core.integration.tests.js +++ b/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'); });