Skip to content

fix: IA-1609 implement request rate-limiting per CodeQL issue #24#296

Merged
aaronfowles merged 1 commit into
mainfrom
rate-limit-auth-calls
Sep 8, 2025
Merged

fix: IA-1609 implement request rate-limiting per CodeQL issue #24#296
aaronfowles merged 1 commit into
mainfrom
rate-limit-auth-calls

Conversation

@aaronfowles
Copy link
Copy Markdown
Contributor

@aaronfowles aaronfowles commented Sep 3, 2025

This fixes https://github.com/alphagov/govuk-knowledge-graph-search/security/code-scanning/24.

Approach

I initially thought about just covering the /login route with rate-limiting and maybe that is the best approach but just slightly more complicated - happy to discuss. The reasons I went for a global rate limiter on all routes:

  • simplicity
  • while testing it became apparent that there were other issues with repeatedly hitting other r endpoints that fetch data and perhaps those routes should also be rate-limited. (Arguably two separate rate-limiters (one for auth, one for data fetching) would probably be more appropriate)

Testing

image

@aaronfowles aaronfowles marked this pull request as ready for review September 3, 2025 10:24
Comment thread package.json
@aaronfowles aaronfowles force-pushed the rate-limit-auth-calls branch 3 times, most recently from 562781f to a817143 Compare September 4, 2025 13:51
@aaronfowles aaronfowles changed the title fix: implement request rate-limiting per CodeQL issue #24 fix: IA-1609 implement request rate-limiting per CodeQL issue #24 Sep 4, 2025
…service attacks

This addresses a CodeQL issue which flagged that the auth routes were not covered by rate-limiting.
A global limit on all routes was chosen as reasonable use of the app functionality shouldn't require many server calls
(once logged in data is fetched only at the user's request). A limit of 20 requests per IP seemed suitable in order
to not interfere with genuine use of the tool while providing some protection against denial-of-service.
@aaronfowles aaronfowles force-pushed the rate-limit-auth-calls branch from a817143 to 5d60cfc Compare September 8, 2025 08:10
Copy link
Copy Markdown
Contributor

@JonathanHallam JonathanHallam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTGM! Thanks :)

@aaronfowles aaronfowles merged commit aaa0bac into main Sep 8, 2025
6 checks passed
@aaronfowles aaronfowles deleted the rate-limit-auth-calls branch September 8, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants