Skip to content

Commit

Permalink
fix(autoLogoutIssue): failing whoami (#1196)
Browse files Browse the repository at this point in the history
## Problem

There was an issue with  #1183 due to the removal of the 

`trust proxy` setting in express. There are two packages that rely on express apis,  `express-rate-limit` and `express-session`. due to changes in the way we used `express-rate-limit`, I thought that this setting can be removed, and this was only done with the intention of a cleanup and does not affect the functionality of how we are currently using `express-rate-limit`.


The [documentation](https://github.com/expressjs/session?tab=readme-ov-file#cookiesecure) for `express-session` also states that:
>  If secure is set, and you access your site over HTTP, the cookie will not be set. If you have your node.js behind a proxy and are using secure: true, you need to set "trust proxy" in express

Checking in with the vapt folks to sense check this fix as well


## Solution

add back trust proxy 

**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)


#  Note: This test will take a while, and requires at worse 15 mins to conduct 
## 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()
}

```
- [ ] connect to ogp vpn 
- [ ] run `node ddos.js` 
- [ ] assert that the remaining counter fell from 100 
![Screenshot 2024-03-08 at 9 09 39 AM](https://github.com/isomerpages/isomercms-backend/assets/42832651/9cfb6417-c96c-45fd-8b58-820dd14c90bc)
- [ ] note the reset time (this is the window time, and by extension the amount of time to wait for this test)
- [ ] unconnect from vpn
- [ ] run `node ddos.js` 
- [ ] assert that the remaining counter fell from 100 
![Screenshot 2024-03-08 at 9 09 39 AM](https://github.com/isomerpages/isomercms-backend/assets/42832651/9cfb6417-c96c-45fd-8b58-820dd14c90bc)
- [ ] After the reset time is achieved, do above steps again and verify that after the reset time, the counters for both the simulated user resets. 
![Screenshot 2024-03-08 at 9 14 36 AM](https://github.com/isomerpages/isomercms-backend/assets/42832651/54e99386-6339-43ee-8bf8-1d182e299d33)
  • Loading branch information
kishore03109 authored Mar 11, 2024
1 parent a7284e5 commit b09fd23
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
4 changes: 2 additions & 2 deletions src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,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,
Expand Down
18 changes: 16 additions & 2 deletions src/services/utilServices/RateLimiter.ts
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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
},
})
2 changes: 1 addition & 1 deletion src/utils/jwt-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
Expand Down

0 comments on commit b09fd23

Please sign in to comment.