fix(rateLimiter): correct rate limits (#1183)

## 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](https://expressjs.com/en/guide/behind-proxies.html) 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](https://developers.cloudflare.com/fundamentals/reference/http-request-headers/#x-forwarded-for).  


> 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](https://developers.cloudflare.com/fundamentals/reference/http-request-headers/#cf-connecting-ip) 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
<img width="1310" alt="Screenshot 2024-03-05 at 12 14 04 PM" src="https://github.com/isomerpages/isomercms-backend/assets/42832651/e1fd64ff-f7f7-48c7-bf65-069133786289">

Closes GTA-24-011 WP3.

**Breaking Changes**

<!-- Does this PR contain any backward incompatible changes? If so, what are they and should there be special considerations for release? -->

- [ ] Yes - this PR contains breaking changes
  - Details ...
- [X] No - this PR is backwards compatible with ALL of the following feature flags in this [doc](https://www.notion.so/opengov/Existing-feature-flags-518ad2cdc325420893a105e88c432be5)


## Tests
- [ ] create a file called ddos.js
```
const stg = "https://staging-cms-api.isomer.gov.sg/v2/auth/verify"
async function send() {
  try {
    const resp = await fetch(stg, {
      method: "POST",
      body: JSON.stringify({
        email: "[email protected]",
        otp: "111111",
      }),
      headers: {
        "Content-Type": "application/json",
        "X-Forwarded-For": generateRandomIp(),
      },
    })
    const text = await resp.text()
    console.log(text)
    console.log({
      Limit: resp.headers.get("Ratelimit-Limit"),
      Remaining: resp.headers.get("Ratelimit-Remaining"),
      Reset: resp.headers.get("Ratelimit-Reset"),
    })
  } catch (err) {
    console.log(err.message)
  }
}
for (let i = 1; i <= 25; i++) {
  send()
}

```

- [ ] run `node ddos.js` 

assert that the reset time is around 84400 (units are in seconds)

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)

src/config/config.ts

@@ -96,7 +96,7 @@ const config = convict({
       format: ["localhost", "cms.isomer.gov.sg", "isomer.gov.sg"],
       default: "localhost",
     },
-    tokenExpiry: {
+    tokenExpiryInMs: {
       doc: "Expiry duration for auth token in milliseconds",
       env: "AUTH_TOKEN_EXPIRY_DURATION_IN_MILLISECONDS",
       format: "required-positive-number",

src/server.js

@@ -88,16 +88,16 @@ import CollaboratorsService from "./services/identity/CollaboratorsService"
 import LaunchClient from "./services/identity/LaunchClient"
 import LaunchesService from "./services/identity/LaunchesService"
 import DynamoDBDocClient from "./services/infra/DynamoDBClient"
-import ReviewCommentService from "./services/review/ReviewCommentService"
 import RepoCheckerService from "./services/review/RepoCheckerService"
+import ReviewCommentService from "./services/review/ReviewCommentService"
 import { rateLimiter } from "./services/utilServices/RateLimiter"
 import SgidAuthService from "./services/utilServices/SgidAuthService"
 import { isSecure } from "./utils/auth-utils"
 import { setBrowserPolyfills } from "./utils/growthbook-utils"
 
 const path = require("path")
 
-const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiry")
+const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiryInMs")
 
 const sequelize = initSequelize([
   Site,
@@ -401,11 +401,6 @@ const formsgSiteCheckerRouter = new FormsgSiteCheckerRouter({
 
 const app = express()
 
-if (isSecure) {
-  // Our server only receives requests from the alb reverse proxy, so we need to use the client IP provided in X-Forwarded-For
-  // This is trusted because our security groups block all other access to the server
-  app.set("trust proxy", true)
-}
 app.use(helmet())
 
 // use growthbook across routes

src/services/utilServices/RateLimiter.ts

@@ -1,6 +1,7 @@
 import rateLimit from "express-rate-limit"
 
-import { config } from "@config/config"
+import { BaseIsomerError } from "@root/errors/BaseError"
+import { isSecure } from "@root/utils/auth-utils"
 
 const DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS = 900000
 
@@ -13,8 +14,21 @@ const DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS = 900000
 // but not on the other, leading to inconsistent behaviour.
 // eslint-disable-next-line import/prefer-default-export
 export const rateLimiter = rateLimit({
-  windowMs: config.get("auth.tokenExpiry") / (1000 * 60),
+  windowMs: DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS,
   max: 100, // Limit each IP to 100 requests per `window` (here, per 15 minutes)
   standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers
   legacyHeaders: false, // Disable the `X-RateLimit-*` headers
+  // We know that this key exists in a secure env (Cloudflare)
+  // See https://developers.cloudflare.com/fundamentals/reference/http-request-headers/#cf-connecting-ip
+  keyGenerator: (req) => {
+    const userIp = isSecure ? req.get("cf-connecting-ip") : req.ip
+    if (!userIp) {
+      // This should never happen, but if it does, we should know about it
+      throw new BaseIsomerError({
+        status: 500,
+        message: "No user IP found in the request",
+      })
+    }
+    return userIp
+  },
 })

src/utils/jwt-utils.js

@@ -7,7 +7,7 @@ const { config } = require("@config/config")
 
 const JWT_SECRET = config.get("auth.jwtSecret")
 const ENCRYPTION_SECRET = config.get("auth.encryptionSecret")
-const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiry").toString()
+const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiryInMs").toString()
 
 const jwtUtil = {
   decodeToken: _.wrap(jwt.decode, (decode, token) => decode(token)),