Skip to content

Commit 2327870

Browse files
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.
1 parent d89f05c commit 2327870

2 files changed

Lines changed: 136 additions & 1 deletion

File tree

lib/services/express.js

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,9 +279,22 @@ const initModulesServerRoutes = async (app) => {
279279
const route = await import(path.resolve(routePath));
280280
route.default(app);
281281
}
282-
app.get('/{*path}', (req, res) => {
282+
// Explicit root route: keeps the friendly landing page at 200, only reached
283+
// when express.static served no public/index.html. Must be registered
284+
// before the catch-all below, since Express 5's `/{*path}` also matches `/`.
285+
app.get('/', (req, res) => {
283286
res.send('<center><br /><h1>Devkit Node Api</h1><h3>Available on <a href="/api/tasks">/api</a>. #LetsGetTogether</h3></center>');
284287
});
288+
// Catch-all for every other unmatched path: content-negotiated 404 instead
289+
// of the previous implicit 200 HTML (#3975). API consumers and automated
290+
// agents get a proper JSON error instead of a false-positive success.
291+
app.get('/{*path}', (req, res) => {
292+
const isApiPath = /^\/api(\/|$)/.test(req.path);
293+
if (isApiPath || req.accepts(['html', 'json']) === 'json') {
294+
return res.status(404).json({ error: 'not_found' });
295+
}
296+
res.status(404).send('<center><br /><h1>404</h1><h3>Not Found</h3></center>');
297+
});
285298
};
286299

287300
/**
Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
/**
2+
* Module dependencies.
3+
*
4+
* Unit tests for express.js initModulesServerRoutes — content-negotiated 404
5+
* for unmatched routes (#3975). Unknown routes must no longer return an
6+
* implicit 200 HTML page; API paths and explicit JSON accepts must get a
7+
* JSON 404, everything else a minimal HTML 404, and root behavior must be
8+
* unchanged.
9+
*/
10+
import { jest, describe, test, expect, beforeEach } from '@jest/globals';
11+
import express from 'express';
12+
import request from 'supertest';
13+
14+
describe('express initModulesServerRoutes — content-negotiated 404 (#3975):', () => {
15+
beforeEach(() => {
16+
jest.resetModules();
17+
});
18+
19+
/**
20+
* Helper: extract initModulesServerRoutes from express.js (with all heavy
21+
* deps mocked) and mount it on a fresh Express app.
22+
* @returns {Promise<import('express').Express>} A ready-to-request Express app
23+
*/
24+
const getApp = async () => {
25+
jest.unstable_mockModule('../../../config/index.js', () => ({
26+
default: {
27+
domain: 'http://localhost:3000',
28+
app: { title: 'Test', description: '', keywords: '', url: '', logo: '' },
29+
secure: { ssl: false },
30+
log: {},
31+
bodyParser: {},
32+
csrf: {},
33+
cors: { origin: [], credentials: false, optionsSuccessStatus: 200 },
34+
trust: { proxy: false },
35+
openapi: { enable: false },
36+
files: { routes: [], configs: [], policies: [], preRoutes: [], openapi: [], guides: [] },
37+
analytics: { posthog: { autoCapture: false } },
38+
docs: {},
39+
},
40+
}));
41+
jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
42+
default: {
43+
warn: jest.fn(),
44+
error: jest.fn(),
45+
info: jest.fn(),
46+
debug: jest.fn(),
47+
getLogFormat: jest.fn().mockReturnValue('combined'),
48+
getMorganOptions: jest.fn().mockReturnValue({}),
49+
},
50+
}));
51+
jest.unstable_mockModule('../../../lib/helpers/guides.js', () => ({
52+
default: { loadGuides: jest.fn().mockReturnValue([]), mergeGuidesIntoSpec: jest.fn() },
53+
}));
54+
jest.unstable_mockModule('../../../lib/middlewares/requestId.js', () => ({
55+
default: jest.fn((req, res, next) => next()),
56+
}));
57+
jest.unstable_mockModule('../../../lib/middlewares/posthog-context.middleware.js', () => ({
58+
posthogContextMiddleware: jest.fn((req, res, next) => next()),
59+
}));
60+
jest.unstable_mockModule('../../../lib/services/errorTracker.js', () => ({
61+
default: { setupExpressErrorHandler: jest.fn() },
62+
}));
63+
jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({
64+
default: { init: jest.fn().mockResolvedValue(undefined), identify: jest.fn(), groupIdentify: jest.fn() },
65+
}));
66+
jest.unstable_mockModule('../../../lib/middlewares/analytics.js', () => ({
67+
default: jest.fn((req, res, next) => next()),
68+
}));
69+
jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({
70+
default: { discoverPolicies: jest.fn().mockResolvedValue(undefined), defineAbilityFor: jest.fn().mockResolvedValue({}) },
71+
}));
72+
73+
const mod = await import('../../../lib/services/express.js');
74+
const app = express();
75+
await mod.default.initModulesServerRoutes(app);
76+
return app;
77+
};
78+
79+
test('GET /api/nope → 404 JSON { error: "not_found" }', async () => {
80+
const app = await getApp();
81+
82+
const res = await request(app).get('/api/nope').expect(404);
83+
84+
expect(res.headers['content-type']).toMatch(/json/);
85+
expect(res.body).toEqual({ error: 'not_found' });
86+
});
87+
88+
test('GET /api (no trailing slash) → 404 JSON', async () => {
89+
const app = await getApp();
90+
91+
const res = await request(app).get('/api').expect(404);
92+
93+
expect(res.headers['content-type']).toMatch(/json/);
94+
expect(res.body).toEqual({ error: 'not_found' });
95+
});
96+
97+
test('GET /nope with Accept: text/html → 404 HTML, no JSON', async () => {
98+
const app = await getApp();
99+
100+
const res = await request(app).get('/nope').set('Accept', 'text/html').expect(404);
101+
102+
expect(res.headers['content-type']).toMatch(/html/);
103+
expect(res.text).toContain('404');
104+
});
105+
106+
test('GET /nope with Accept: application/json → 404 JSON { error: "not_found" }', async () => {
107+
const app = await getApp();
108+
109+
const res = await request(app).get('/nope').set('Accept', 'application/json').expect(404);
110+
111+
expect(res.headers['content-type']).toMatch(/json/);
112+
expect(res.body).toEqual({ error: 'not_found' });
113+
});
114+
115+
test('GET / → 200, friendly root page unchanged', async () => {
116+
const app = await getApp();
117+
118+
const res = await request(app).get('/').expect(200);
119+
120+
expect(res.text).toContain('Devkit Node Api');
121+
});
122+
});

0 commit comments

Comments
 (0)