diff --git a/.changeset/plain-owls-answer.md b/.changeset/plain-owls-answer.md new file mode 100644 index 0000000..79aacb0 --- /dev/null +++ b/.changeset/plain-owls-answer.md @@ -0,0 +1,28 @@ +--- +'seamless-auth-api': minor +--- + +Answer a rate-limited request with the JSON error shape. + +Every other `4xx` and `5xx` on this API is `{ "error": "..." }`. A `429` was the one +exception: express-rate-limit sends a string `message` through `res.send`, which lands +as `text/html`, so a client parsing error bodies as JSON got a parse failure instead of +an error. + +Three limiters set that string explicitly. The other six set no message at all and +inherited the library's own string default, so they were plain text too. All nine sites, +including the JWKS limiter, now send an object and answer: + +```json +{ "error": "Too many requests, please try again later" } +``` + +**Behaviour change:** the `429` body and its content type change. Two consumers were +already coping with the text form rather than depending on it. `@seamless-auth/core` +carries a `makeJsonTolerant` shim in `authFetch` that names this case in its comment, +and `seamless-auth-react` was crashed by it once already. The admin dashboard maps `429` +to fixed wording and never surfaces upstream text, so it is unaffected. + +The unreachable `message` on the slow-down is removed rather than converted. +express-slow-down replaces the handler with one that only delays and calls `next`, so it +never answers a request and that option could never be read. diff --git a/docs/api-contract.md b/docs/api-contract.md index 5d30c73..14f42c6 100644 --- a/docs/api-contract.md +++ b/docs/api-contract.md @@ -95,6 +95,12 @@ now all set `error`. `tests/unit/routes/errorShapeCoverage.spec.ts` walks every and fails if any failure response declares a schema without a required `error` string, so a new route cannot reintroduce the split. +A `429` from the rate limiters is included. It used to be the one exception, answering plain text +because express-rate-limit sends a string message through `res.send`, and it is now +`{ "error": "Too many requests, please try again later" }` like everything else. The limiters are +middleware rather than route handlers, so a `429` is not declared per route in the OpenAPI +document, but the body is the same shape. + `ErrorSchema` in [`src/schemas/generic.responses.ts`](../src/schemas/generic.responses.ts) is the canonical definition. `InternalErrorSchema` is a deprecated alias of it and is identical on the wire. diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index bfd39a2..8bd6c88 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 98.9% + + coverage: 99% @@ -7,17 +7,17 @@ - + - - + + coverage coverage - 98.9% - 98.9% + 99% + 99% diff --git a/src/middleware/jwksRateLimit.ts b/src/middleware/jwksRateLimit.ts index 62effe7..e9a729d 100644 --- a/src/middleware/jwksRateLimit.ts +++ b/src/middleware/jwksRateLimit.ts @@ -9,6 +9,7 @@ import rateLimit from 'express-rate-limit'; import { getSystemConfig } from '../config/getSystemConfig.js'; import { rateLimitsDisabled } from './rateLimitsDisabled.js'; +import { TOO_MANY_REQUESTS_BODY } from './tooManyRequests.js'; async function getConfiguredRateLimit() { const { rate_limit } = await getSystemConfig(); @@ -21,7 +22,7 @@ const jwksLimiter = rateLimit({ limit: getConfiguredRateLimit, standardHeaders: true, legacyHeaders: false, - message: 'Too many requests, please try again later', + message: TOO_MANY_REQUESTS_BODY, skip: rateLimitsDisabled, }); diff --git a/src/middleware/rateLimit.ts b/src/middleware/rateLimit.ts index c6fc98a..ddce333 100644 --- a/src/middleware/rateLimit.ts +++ b/src/middleware/rateLimit.ts @@ -10,6 +10,7 @@ import rateLimit from 'express-rate-limit'; import { getSystemConfig } from '../config/getSystemConfig.js'; import { AuthenticatedRequest } from '../types/types.js'; import { rateLimitsDisabled } from './rateLimitsDisabled.js'; +import { TOO_MANY_REQUESTS_BODY } from './tooManyRequests.js'; async function getConfiguredRateLimit() { const { rate_limit } = await getSystemConfig(); @@ -63,7 +64,7 @@ const dynamicLimiter = rateLimit({ standardHeaders: true, legacyHeaders: false, skip: rateLimitsDisabled, - message: 'Too many requests, please try again later', + message: TOO_MANY_REQUESTS_BODY, }); const magicLinkIpCachedLimiter = rateLimit({ @@ -72,6 +73,7 @@ const magicLinkIpCachedLimiter = rateLimit({ standardHeaders: true, legacyHeaders: false, skip: rateLimitsDisabled, + message: TOO_MANY_REQUESTS_BODY, }); const magicLinkIdentityCachedLimiter = rateLimit({ @@ -81,6 +83,7 @@ const magicLinkIdentityCachedLimiter = rateLimit({ standardHeaders: true, legacyHeaders: false, skip: rateLimitsDisabled, + message: TOO_MANY_REQUESTS_BODY, }); const otpIpCachedLimiter = rateLimit({ @@ -89,6 +92,7 @@ const otpIpCachedLimiter = rateLimit({ standardHeaders: true, legacyHeaders: false, skip: rateLimitsDisabled, + message: TOO_MANY_REQUESTS_BODY, }); const otpIdentityCachedLimiter = rateLimit({ @@ -98,6 +102,7 @@ const otpIdentityCachedLimiter = rateLimit({ standardHeaders: true, legacyHeaders: false, skip: rateLimitsDisabled, + message: TOO_MANY_REQUESTS_BODY, }); const oauthIpCachedLimiter = rateLimit({ @@ -106,6 +111,7 @@ const oauthIpCachedLimiter = rateLimit({ standardHeaders: true, legacyHeaders: false, skip: rateLimitsDisabled, + message: TOO_MANY_REQUESTS_BODY, }); const oauthProviderCachedLimiter = rateLimit({ @@ -115,6 +121,7 @@ const oauthProviderCachedLimiter = rateLimit({ standardHeaders: true, legacyHeaders: false, skip: rateLimitsDisabled, + message: TOO_MANY_REQUESTS_BODY, }); export function dynamicRateLimit(req: Request, res: Response, next: NextFunction) { diff --git a/src/middleware/slowDown.ts b/src/middleware/slowDown.ts index af3e2a1..bfa0ace 100644 --- a/src/middleware/slowDown.ts +++ b/src/middleware/slowDown.ts @@ -21,7 +21,6 @@ const cachedLimiter: ReturnType = slowDown({ delayAfter: getConfiguredDelayAfter, legacyHeaders: false, delayMs: (hits) => hits * 1000, - message: 'Too many requests, please try again later', skip: rateLimitsDisabled, }); diff --git a/src/middleware/tooManyRequests.ts b/src/middleware/tooManyRequests.ts new file mode 100644 index 0000000..e50df02 --- /dev/null +++ b/src/middleware/tooManyRequests.ts @@ -0,0 +1,18 @@ +/* + * Copyright © 2026 Fells Code, LLC + * Licensed under the GNU Affero General Public License v3.0 + * See LICENSE file in the project root for full license information + */ + +/** + * The body every limiter answers a refused request with. + * + * An object rather than a string because express-rate-limit sends a string through + * `res.send`, which lands as `text/html`. Its own default message is a string too, so + * a limiter that sets nothing is just as inconsistent as one that sets a string. Every + * other 4xx and 5xx on this API is JSON in this shape, and a client that parses error + * bodies should not have to special-case one status. + */ +export const TOO_MANY_REQUESTS_BODY = { + error: 'Too many requests, please try again later', +} as const; diff --git a/tests/integration/rateLimit/globalRateLimit.spec.ts b/tests/integration/rateLimit/globalRateLimit.spec.ts index e5e2d3f..78432b8 100644 --- a/tests/integration/rateLimit/globalRateLimit.spec.ts +++ b/tests/integration/rateLimit/globalRateLimit.spec.ts @@ -97,15 +97,10 @@ describe('global rate limiting', () => { } }); - /** - * Pins what a throttled caller actually receives, which is not the JSON error shape - * every other response on this API uses: the limiters are configured with a plain - * string `message`, so express-rate-limit sends it as text. Asserted rather than - * corrected because changing it is a contract change for the SDKs, and one of them has - * already been bitten by it (seamless-auth-react#41, a non-JSON 429 crashing the - * client). Change this test deliberately, with that coordination, not in passing. - */ - it('answers with a plain-text body rather than the JSON error shape', async () => { + // express-rate-limit sends a string message through res.send, which lands as + // text/html, and its own default message is a string. Only an object reaches the + // caller as the JSON shape every other error on this API uses. + it('answers with the JSON error shape, not plain text', async () => { const app = await loadAppWithLimiters(); for (let i = 0; i < 3; i++) { @@ -115,7 +110,7 @@ describe('global rate limiting', () => { const res = await request(app).get('/health/status'); expect(res.status).toBe(429); - expect(res.headers['content-type']).not.toContain('application/json'); - expect(res.text).toBe('Too many requests, please try again later'); + expect(res.headers['content-type']).toContain('application/json'); + expect(res.body).toEqual({ error: 'Too many requests, please try again later' }); }); }); diff --git a/tests/unit/middleware/rateLimit.spec.ts b/tests/unit/middleware/rateLimit.spec.ts index e348c78..2baa439 100644 --- a/tests/unit/middleware/rateLimit.spec.ts +++ b/tests/unit/middleware/rateLimit.spec.ts @@ -391,6 +391,43 @@ describe('oauthProviderLimiter', () => { }); }); +describe('refusal body', () => { + // Six of these set no message at all and inherited express-rate-limit's own string + // default, so grepping for the string found three of the nine sites. Asserted across + // every constructed limiter rather than per-limiter for that reason. + it('gives every limiter the JSON error shape', async () => { + const { getSystemConfig } = await import('../../../src/config/getSystemConfig'); + const rateLimit = await import('express-rate-limit'); + + (getSystemConfig as any).mockResolvedValue({}); + + await import('../../../src/middleware/rateLimit'); + await import('../../../src/middleware/jwksRateLimit'); + + const messages = (rateLimit.default as any).mock.calls.map( + ([options]: any[]) => options.message, + ); + + expect(messages).toHaveLength(8); + for (const message of messages) { + expect(message).toEqual({ error: 'Too many requests, please try again later' }); + } + }); + + // express-slow-down replaces the handler with one that only delays and calls next, + // so it never answers a request and a message there could not be read. + it('sets no message on the slow-down, which never answers', async () => { + const { getSystemConfig } = await import('../../../src/config/getSystemConfig'); + const slowDown = await import('express-slow-down'); + + (getSystemConfig as any).mockResolvedValue({}); + + await import('../../../src/middleware/slowDown'); + + expect((slowDown.default as any).mock.calls[0][0]).not.toHaveProperty('message'); + }); +}); + describe('dynamicJWKSRateLimit', () => { it('uses config rate_limit and invokes the cached limiter', async () => { const { getSystemConfig } = await import('../../../src/config/getSystemConfig');