isomerpages / isomercms-backend

A static website builder and host for the Singapore Government
5 stars 1 forks source link

fix(rateLimiter): correct rate limits #1183

Closed kishore03109 closed 7 months ago

kishore03109 commented 7 months ago

Problem

Our rate limiter is implemented wrongly. Couple of problems exist:

  1. careless conversion of units. the unit of env var was already ms, but the code assumes it is in s instead. for clarity, renaming the env var from tokenExpiry to tokenExpiryInMs
  2. We are currently trusting proxy, where we trust the proxy to expose the client ip. The documentation states that

When setting to true, it is important to ensure that the last reverse proxy trusted is removing/overwriting all of the following HTTP headers: X-Forwarded-For, X-Forwarded-Host, and X-Forwarded-Proto otherwise it may be possible for the client to provide any value.

This is not entirely true from reading the Cloudflare's documentation.

If, on the other hand, an X-Forwarded-For header was already present in the request to Cloudflare, Cloudflare will append the IP address of the HTTP proxy connecting to Cloudflare to the header.

This PR uses the recommended approach of using the CF-Connecting-IP that cloudflare provides to assert the ip of the client instead.

Moving forward, we never trust the proxy. When using Cloudflare in production env, we should use the CF-Connecting-IP instead to verify the cilent ip instead. We continue to use req.ip for dev environments

I have verified that the cf-incoming-ip exists in staging env by logging it

Screenshot 2024-03-05 at 12 14 04 PM

Closes GTA-24-011 WP3.

Breaking Changes

Tests



- [ ] run `node ddos.js` 

assert that the reset time is around 900 (units are in seconds, resets once every 15 mins)

Reset value before:
![Screenshot 2024-03-05 at 12 44 20 PM](https://github.com/isomerpages/isomercms-backend/assets/42832651/9cf2360e-0447-471d-8810-5ba7e8465beb)

Reset value after:

![Screenshot 2024-03-05 at 12 45 08 PM](https://github.com/isomerpages/isomercms-backend/assets/42832651/e7c11093-e1cf-4fa6-8541-4317859d232c)
kishore03109 commented 7 months ago

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kishore03109 and the rest of your teammates on Graphite Graphite

mergify[bot] commented 6 months ago

⚠️ The sha of the head commit of this PR conflicts with #1252. Mergify cannot evaluate rules on this PR. ⚠️