diff --git a/lib/services/express.js b/lib/services/express.js index 9d546d481..9bdb0b9dd 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(\/|$)/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

'); + }); }; /** 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..6f4375a20 --- /dev/null +++ b/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} 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'); + }); +}); 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'); });