feat(rate-limit): adds rate limiting to auth endpoints

isomerpages / isomercms-backend

A static website builder and host for the Singapore Government

5 stars 1 forks source link

feat(rate-limit): adds rate limiting to auth endpoints #610

Closed seaerchin closed 1 year ago

seaerchin commented 1 year ago

Problem

Closes VAPT issue for brute forcing.

Solution

This was done through installing an external library for rate limiting.

the library was chosen cos it was in ts + seems easy to use
limit of 100/window (otp space = 10 ** 6 so the chance is 100/10^6 << 1%)
limit of 100 is applied to only auth endpoint (no strong opinion here, can apply to all if needed) as auth endpoint was the one flagged. didn't want it to be the most specific one as auth as a whole is p sensitive so would rather err on the safe side.

seaerchin commented 1 year ago

Hey, according to the docs, since we are behind cloudflare, the IP requests would be the IP of the load balancer. I believe we would need to add a app.set('trust proxy', numberOfProxies) in order to make this a non-global rate limit

yea you're entirely right and i missed this! just to confirm there are actually 2 proxies right? (aws load balancer as well as cloudflare reverse proxy)

seaerchin commented 1 year ago

Hey, mostly lgtm with nits. non nit: may I please trouble you to test this on staging before pushing this to develop? I think a trivial manual testing is also fine!

this isn't gonna be merged into develop but vapt! also, writing load testing here would involve either writing a script or changing the given max parameter. would the provided test cases suffice?

seaerchin commented 1 year ago

I think the unit tests are sufficient. What I thought would be safer was to test if the number of proxies is indeed 2. Concretely, if we can just verify that our otp rate limits work in staging, ie, the limit is a per IP rather than a global one, I think this is fine.

My concern is that this is something that we cannot have unit tests for, but it will cause issues in production!

sure, i was gonna let horangi check this AHHA but i'll test it out on staging later today or tomorrow!

kishore03109 commented 1 year ago

I think the unit tests are sufficient. What I thought would be safer was to test if the number of proxies is indeed 2. Concretely, if we can just verify that our otp rate limits work in staging, ie, the limit is a per IP rather than a global one, I think this is fine. My concern is that this is something that we cannot have unit tests for, but it will cause issues in production!

sure, i was gonna let horangi check this AHHA but i'll test it out on staging later today or tomorrow!

HAHAHAH sounds good, we paid for this anyways LOL

mergify[bot] commented 1 year ago

:warning: The sha of the head commit of this PR conflicts with #590. Mergify cannot evaluate rules on this PR. :warning: