From d5261f37dd83d2347d86a9be188aa0279de12c35 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 31 Aug 2026 10:37:43 -0400 Subject: [PATCH] fix(rate-limit): answer with the JSON error shape rather than plain text Closes #241. Every other 4xx and 5xx on this API is { error }. A 429 was the 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. Grepping for the string found three of the nine sites. The other six set no message at all and inherited the library's own string default, so they were plain text for the same reason without saying so. All nine now send an object. The 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. Two consumers were already coping with the text form: @seamless-auth/core has a makeJsonTolerant shim in authFetch that names this case, and seamless-auth-react was crashed by it once. The admin dashboard maps 429 to fixed wording and never surfaces upstream text. --- .changeset/plain-owls-answer.md | 28 ++++++++++++++ docs/api-contract.md | 6 +++ resources/coverage-badge.svg | 14 +++---- src/middleware/jwksRateLimit.ts | 3 +- src/middleware/rateLimit.ts | 9 ++++- src/middleware/slowDown.ts | 1 - src/middleware/tooManyRequests.ts | 18 +++++++++ .../rateLimit/globalRateLimit.spec.ts | 17 +++------ tests/unit/middleware/rateLimit.spec.ts | 37 +++++++++++++++++++ 9 files changed, 112 insertions(+), 21 deletions(-) create mode 100644 .changeset/plain-owls-answer.md create mode 100644 src/middleware/tooManyRequests.ts 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');