test: exercise the global rate limiters as mounted middleware - #238
Merged
Conversation
Closes #231. Nothing reached these before. Two things kept them out of every run, and both had to be undone here: - src/app.ts mounts dynamicSlowDown and dynamicRateLimit only when NODE_ENV is not "test", and tests/setup/env.ts sets exactly that. - tests/setup/mocks.ts additionally replaces them with pass-throughs for the whole suite, so mounting alone would not have been enough. The existing unit spec covers the modules but mocks express-rate-limit and express-slow-down away, so the real libraries were never exercised as Express middleware by anything. Each case imports the app fresh under a development NODE_ENV. /health/status is the probe: no auth, no database, so the response is the limiter's verdict and nothing else. Covers serving up to the limit, refusing past it, taking the limit from system config rather than the fallback, the RateLimit headers, and the DISABLE_AUTH_RATE_LIMITS skip. One test pins the 429 body as plain text rather than the JSON error shape used everywhere else, because the limiters carry a string message. Asserted rather than corrected: changing it is a contract change for the SDKs, and seamless-auth-react#41 is a client already broken by a non-JSON 429. Verified non-vacuous by unmounting the limiters, which fails four of the six.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #231.
Two blockers, not one
The issue named the
NODE_ENV !== 'test'guard insrc/app.ts. There is a second one underneath it:tests/setup/mocks.tsalso replacesdynamicRateLimitanddynamicSlowDownwith pass-throughs for the entire suite. Mounting them was not enough on its own, and the first run of this spec passed four assertions it should have failed until the mock was lifted. This file opts back into the real middleware the same waytests/unit/middleware/rateLimit.spec.tsdoes.The existing unit spec is thorough but mocks
express-rate-limitandexpress-slow-downaway, so the real libraries had never been exercised as Express middleware anywhere in the suite.What is covered
Each case imports the app fresh under a development
NODE_ENV./health/statusis the probe because it needs no auth and touches no database, so the response is the limiter's verdict and nothing else.system_configrather than the hardcoded fallbackRateLimit-LimitandRateLimit-RemainingDISABLE_AUTH_RATE_LIMITSskipdelay_afteris set well above what these send, becauseexpress-slow-downsits ahead of the limiter and would otherwise sleephits * 1000ms before answering.The 429 shape, pinned rather than fixed
One test asserts that a throttled caller gets plain text, not the JSON error shape every other response on this API uses, because the limiters are configured with a string
message.That is arguably a bug: seamless-auth-react#41 is a client that crashes on exactly this. But changing the body is a contract change for both SDKs, so it should be a deliberate, coordinated change rather than something smuggled into a test PR. The test documents current behaviour and its comment says to change it only with that coordination. Happy to do it as a follow-up if you want it.
Verification
Non-vacuous by construction and by experiment: with the limiters unmounted, four of the six fail. Full suite 1200 passed, 1 skipped.
app.tsstatement coverage rises from 94.23% to 98.07%, which is the limiter mount block now being reached. Full gate green via the pre-commit hook.