diff --git a/.changeset/quiet-hounds-report.md b/.changeset/quiet-hounds-report.md new file mode 100644 index 0000000..239a083 --- /dev/null +++ b/.changeset/quiet-hounds-report.md @@ -0,0 +1,28 @@ +--- +'seamless-auth-api': minor +--- + +Report an unhandled server error as 500 rather than 404. + +The first error handler answered the CORS rejection and passed everything else on +with a bare `next()`. Calling `next()` with no argument from an error handler clears +the error and resumes at the next **regular** middleware, so the 500 handler directly +below it was skipped and control landed on the 404. Any unhandled exception was +therefore reported to the caller as `404 {"error":"Not Found"}`. + +Two consequences beyond the wrong status: + +- **Server faults were recorded against the caller.** The 404 handler logs an + `AuthEventService.requestSuspicious` event with reason "Request to an unknown + route." Every internal error was written into the anomaly signal the dashboard and + the security views read, as suspicious behaviour by whoever happened to send the + request. That stream is now free of them. +- **Real errors were easy to miss.** A 500 is alertable. A 404 on an auth API is + background noise, and this masked a genuine regression through a full test run. + +**Behaviour change:** a request that triggers an unhandled exception now answers +`500 {"error":"Internal server error"}` instead of `404 {"error":"Not Found"}`. A +genuinely unmatched route still answers 404, unchanged. Callers that retry on 5xx but +not 4xx will now retry these. No dependent needed changing: the React SDK does not +branch on 404, and the admin dashboard already maps `>= 500` to a clearer message than +the 404 text it was getting. diff --git a/src/app.ts b/src/app.ts index 34aa7ae..04b6af3 100644 --- a/src/app.ts +++ b/src/app.ts @@ -147,7 +147,11 @@ export async function createApp() { // is not one tells it part of the allowlist and helps the browser not at all. return res.status(403).json({ message: 'CORS policy does not allow this origin.' }); } - return next(); + // next(err), not next(): a bare next() from an error handler clears the error and + // resumes at the next regular middleware, which skips the 500 handler below and lands + // on the 404. That reported server faults as unknown routes and recorded the caller + // for them. + return next(err); }); app.use((err: unknown, req: Request, res: Response, next: NextFunction) => { diff --git a/tests/unit/app.spec.ts b/tests/unit/app.spec.ts index 797a573..408be29 100644 --- a/tests/unit/app.spec.ts +++ b/tests/unit/app.spec.ts @@ -129,11 +129,20 @@ describe('query parsing', () => { }); describe('createApp error handling', () => { - it('passes non-CORS errors through to the not-found handler', async () => { + it('reports a non-CORS error as a server error', async () => { const res = await request(built).get('/__test_plain_error'); - expect(res.status).toBe(404); - expect(res.body).toEqual({ error: 'Not Found' }); + expect(res.status).toBe(500); + expect(res.body).toEqual({ error: 'Internal server error' }); + }); + + // The 404 handler records the caller as suspicious. A fault on this side is not the + // caller misbehaving, and letting it land there put server bugs into the signal the + // anomaly views read. + it('does not record a server error as suspicious activity', async () => { + await request(built).get('/__test_plain_error'); + + expect(AuthEventService.requestSuspicious).not.toHaveBeenCalled(); }); it('falls back to a 500 when the error pipeline itself throws', async () => {