From 988fd7da7d125a3dedf48964c7b01e8c1f17fc01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 18:33:51 +0200 Subject: [PATCH 1/2] A database outage is not a 401, and not healthy either MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authentication reads the database, so anything that can go wrong with the database surfaces as an exception in the auth middleware - and the catch turned every one of them into 401 with the driver's message as the body: 401 {"error":"connect ECONNREFUSED 127.0.0.1:27017"} 401 {"error":"Operation `users.findOne()` buffering timed out after 10000ms"} The status says the caller sent a bad key. It sends whoever is debugging to look at credentials that were never judged, because the request never got far enough to judge them. The healthcheck agreed: it returned 200 unconditionally, proving only that the HTTP listener was up. A SPACE whose MongoDB container had stopped reported healthy for fifteen hours while refusing every authenticated request as unauthorised. Two signals agreeing on the wrong answer is what made this expensive. authenticateUserApiKey and authenticateOrgApiKey now throw a typed InvalidApiKeyError for the two cases that really are bad credentials. The catch answers 401 only for those; anything else is 503 with Retry-After, since the credential was never judged and the caller should try again. The healthcheck reports 503 when Mongoose is not connected, and pings the database when it is - readyState is what the driver believes, a ping is what the database says, and they disagree when a connection has gone stale. Verified by stopping the database under a running server: main: healthcheck 200, authed call 401 {"error":"connect ECONNREFUSED…"} this PR: healthcheck 503 {"database":"disconnected"} authed call 503 {"error":"Space cannot verify credentials right now."} --- api/src/main/middlewares/AuthMiddleware.ts | 32 ++++++++++++-- api/src/main/routes/HealthcheckRoutes.ts | 49 ++++++++++++++++++---- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/api/src/main/middlewares/AuthMiddleware.ts b/api/src/main/middlewares/AuthMiddleware.ts index 687c108..97a6209 100644 --- a/api/src/main/middlewares/AuthMiddleware.ts +++ b/api/src/main/middlewares/AuthMiddleware.ts @@ -19,6 +19,18 @@ import { HttpMethod, OrganizationApiKeyRole } from '../types/permissions'; * Sets req.user for User API Keys * Sets req.org for Organization API Keys */ +/** + * A credential that was read and found wanting. + * + * Distinguished from every other failure on purpose. Authentication reads the + * database, so anything that can go wrong with the database - a dropped + * connection, a replica-set election, a Mongo that is simply not running - + * surfaces as an exception here too. Answering 401 to those says the caller + * sent a bad key, which is untrue and sends whoever is debugging to look at + * their credentials. + */ +class InvalidApiKeyError extends Error {} + const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: NextFunction) => { const apiKey = req.headers['x-api-key'] as string; @@ -40,11 +52,25 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N return checkPermissions(req, res, next); } catch (err: any) { - if (!res.headersSent) { + if (res.headersSent) { + return; + } + + if (err instanceof InvalidApiKeyError) { return res.status(401).json({ error: err.message || 'Invalid API Key', }); } + + // Anything else got as far as trying and could not finish - almost always + // the database. 503 rather than 401, because the credential was never + // judged, and Retry-After because it is worth trying again. + console.error('Authentication could not be completed:', err); + res.setHeader('Retry-After', '5'); + return res.status(503).json({ + error: 'Space cannot verify credentials right now.', + details: err?.message ?? String(err), + }); } }; @@ -57,7 +83,7 @@ async function authenticateUserApiKey(req: Request, apiKey: string): Promise { - res.status(200).json({ - message: 'Service is up and running!', - }); - } - ); + app.route(`${baseUrl}/healthcheck`).get(async (req: any, res: any) => { + // The database is not a detail of this service, it is the service: every + // authenticated route reads it before it can answer anything. A check that + // proves only the HTTP listener is up reports a Space that cannot serve a + // single request as healthy, and keeps reporting it indefinitely while + // every call fails. + if (mongoose.connection.readyState !== CONNECTED) { + return res.status(503).json({ + message: 'Service is up but cannot reach its database.', + database: 'disconnected', + }); + } + + try { + // readyState is what the driver believes. A ping is what the database + // says, and the two disagree when a connection has gone stale. + await mongoose.connection.db!.admin().ping(); + } catch (error: any) { + return res.status(503).json({ + message: 'Service is up but its database is not answering.', + database: 'unreachable', + details: error?.message ?? String(error), + }); + } + + res.status(200).json({ + message: 'Service is up and running!', + database: 'connected', + }); + }); }; export default loadFileRoutes; From 2179283d09cc4d5f4bb40fa712ca3a8932bc46d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 19:26:40 +0200 Subject: [PATCH 2/2] Keep a rejected key a 401 while the lookup itself fails with 503 Narrowing the catch to a dedicated error type was not enough on its own: UserService.findByApiKey reports an unknown key by throwing rather than by returning nothing, so the commonest 401 in the suite was being answered 503. The lookup is now wrapped in one place that tells the codebase's own INVALID DATA: signal - a caller's mistake - apart from anything else, which is the database being unable to answer. Both directions are pinned by tests. --- api/src/main/middlewares/AuthMiddleware.ts | 47 ++++++++--- api/src/test/middlewares/authOutage.test.ts | 93 +++++++++++++++++++++ 2 files changed, 130 insertions(+), 10 deletions(-) create mode 100644 api/src/test/middlewares/authOutage.test.ts diff --git a/api/src/main/middlewares/AuthMiddleware.ts b/api/src/main/middlewares/AuthMiddleware.ts index 97a6209..bb68ec2 100644 --- a/api/src/main/middlewares/AuthMiddleware.ts +++ b/api/src/main/middlewares/AuthMiddleware.ts @@ -8,6 +8,7 @@ import { import { matchPath, extractApiPath } from '../utils/routeMatcher'; import { LeanOrganization, OrganizationMember, OrganizationUserRole } from '../types/models/Organization'; import { HttpMethod, OrganizationApiKeyRole } from '../types/permissions'; +import { LeanUser } from '../types/models/User'; /** * Middleware to authenticate API Keys (both User and Organization types) @@ -80,16 +81,43 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N async function authenticateUserApiKey(req: Request, apiKey: string): Promise { const userService = container.resolve('userService'); - const user = await userService.findByApiKey(apiKey); - - if (!user) { - throw new InvalidApiKeyError('Invalid User API Key'); - } + const user = await rejectionOrOutage( + () => userService.findByApiKey(apiKey), + 'Invalid User API Key' + ); req.user = user; req.authType = 'user'; } +/** + * Run a credential lookup, telling a refusal apart from a failure. + * + * `UserService.findByApiKey` reports an unknown key by throwing rather than by + * returning nothing, using the `INVALID DATA:` prefix this codebase gives to a + * caller's own mistake. That has to keep answering 401. Anything else thrown by + * a lookup is the database being unable to answer, which is the case this + * middleware exists to stop reporting as a bad credential. + */ +async function rejectionOrOutage(lookup: () => Promise, absent: string): Promise { + let found: T; + + try { + found = await lookup(); + } catch (err: any) { + if (typeof err?.message === 'string' && err.message.startsWith('INVALID DATA:')) { + throw new InvalidApiKeyError(err.message); + } + throw err; + } + + if (!found) { + throw new InvalidApiKeyError(absent); + } + + return found; +} + /** * Authenticates an Organization API Key and populates req.org */ @@ -97,11 +125,10 @@ async function authenticateOrgApiKey(req: Request, apiKey: string): Promise organizationRepository.findByApiKey(apiKey), + 'Invalid Organization API Key' + ); req.org = { id: result.id!, diff --git a/api/src/test/middlewares/authOutage.test.ts b/api/src/test/middlewares/authOutage.test.ts new file mode 100644 index 0000000..890ec45 --- /dev/null +++ b/api/src/test/middlewares/authOutage.test.ts @@ -0,0 +1,93 @@ +import request from 'supertest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { Server } from 'http'; +import { getApp, shutdownApp, baseUrl } from '../utils/testApp'; +import { createTestUser, deleteTestUser } from '../utils/users/userTestUtils'; +import container from '../../main/config/container'; +import { LeanUser } from '../../main/types/models/User'; + +/** + * Telling "your key is wrong" apart from "we could not check your key". + * + * Authentication reads the database, so a dropped connection, a replica-set + * election or a Mongo that is simply not running all surface as an exception in + * the same place an unknown key does. Answering 401 to those states something + * untrue about the caller's credential and sends whoever is debugging to look + * at their API key, which is the one place the problem is not. + * + * Both directions are pinned here, because the interesting part of the change + * is the boundary: a refusal must stay a refusal. + */ +describe('Authentication when the database cannot answer', function () { + let app: Server; + let user: LeanUser; + + beforeAll(async function () { + app = await getApp(); + user = await createTestUser('ADMIN'); + }); + + afterAll(async function () { + await deleteTestUser(user.username); + vi.restoreAllMocks(); + await shutdownApp(); + }); + + it('answers 503, not 401, when the lookup fails', async function () { + const userService: any = container.resolve('userService'); + const outage = vi + .spyOn(userService, 'findByApiKey') + .mockRejectedValue(new Error('MongooseServerSelectionError: connection timed out')); + + try { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', user.apiKey); + + expect(response.status).toBe(503); + expect(response.body.error).toContain('cannot verify credentials'); + } finally { + outage.mockRestore(); + } + }); + + it('asks the caller to try again', async function () { + const userService: any = container.resolve('userService'); + const outage = vi + .spyOn(userService, 'findByApiKey') + .mockRejectedValue(new Error('MongooseServerSelectionError: connection timed out')); + + try { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', user.apiKey); + + // A 503 without Retry-After tells a client nothing about whether waiting + // is worth it. + expect(response.headers['retry-after']).toBeDefined(); + } finally { + outage.mockRestore(); + } + }); + + it('still answers 401 to a key that was read and rejected', async function () { + // The regression this pairs with. `UserService.findByApiKey` reports an + // unknown key by throwing rather than by returning nothing, so narrowing + // the catch to a dedicated error type is not by itself enough to keep this + // a 401. + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', 'usr_nosuchkeyatall'); + + expect(response.status).toBe(401); + expect(response.body.error).toContain('INVALID DATA: Invalid API Key'); + }); + + it('still answers 401 to a key of no recognisable kind', async function () { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', 'not-a-prefixed-key'); + + expect(response.status).toBe(401); + }); +});