Merge pull request #1202 from isomerpages/release/0.70.0

* fix(otp): increment instead of update for concurrency (#1186) * fix(otp): increment instead of update for concurrency * ref(otp): migrate to use neverthrow instead * chore: remove unneeded check for empty string * Fix/add validators (#1197) * chore: add validators * fix: tests * fix: add test * chore: enforce 6 length otp * fix(autoLogoutIssue): failing whoami (#1196) ## 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**  - [ ] 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) * 0.70.0 --------- Co-authored-by: Alexander Lee <[email protected]> Co-authored-by: Kishore <[email protected]>
isomerpages · Mar 11, 2024 · b649898 · b649898
2 parents d2684d2 + 483b225
commit b649898
Show file tree

Hide file tree

Showing 25 changed files with 599 additions and 171 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,11 +4,22 @@ All notable changes to this project will be documented in this file. Dates are d
 
 Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 
-#### [v0.69.0](https://github.com/isomerpages/isomercms-backend/compare/v0.68.2...v0.69.0)
-
+#### [v0.70.0](https://github.com/isomerpages/isomercms-backend/compare/v0.69.0...v0.70.0)
+
+- fix: schema [`#1203`](https://github.com/isomerpages/isomercms-backend/pull/1203)
+- fix(otp): unwrap ResultAsync value [`#1204`](https://github.com/isomerpages/isomercms-backend/pull/1204)
+- fix(autoLogoutIssue): failing whoami [`#1196`](https://github.com/isomerpages/isomercms-backend/pull/1196)
+- Fix/add validators [`#1197`](https://github.com/isomerpages/isomercms-backend/pull/1197)
+- fix(otp): increment instead of update for concurrency [`#1186`](https://github.com/isomerpages/isomercms-backend/pull/1186)
+- 0.69.0 [`#1190`](https://github.com/isomerpages/isomercms-backend/pull/1190)
 - Revert "fix(rateLimiter): correct rate limits (#1183)" [`#1195`](https://github.com/isomerpages/isomercms-backend/pull/1195)
 - fix(site checker): dont trigger alarms [`#1194`](https://github.com/isomerpages/isomercms-backend/pull/1194)
 - chore(audit): update form question phrasing [`#1192`](https://github.com/isomerpages/isomercms-backend/pull/1192)
+
+#### [v0.69.0](https://github.com/isomerpages/isomercms-backend/compare/v0.68.2...v0.69.0)
+
+> 7 March 2024
+
 - fix(api): fix media route timeout [`#1170`](https://github.com/isomerpages/isomercms-backend/pull/1170)
 - fix(linkChecker): bug fixes [`#1187`](https://github.com/isomerpages/isomercms-backend/pull/1187)
 - Feat/add back repair form lock [`#1179`](https://github.com/isomerpages/isomercms-backend/pull/1179)
@@ -68,14 +79,13 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 - Release/0.66.2 [`#1144`](https://github.com/isomerpages/isomercms-backend/pull/1144)
 - release(0.66.1): merge to develop [`#1142`](https://github.com/isomerpages/isomercms-backend/pull/1142)
 - 0.66.0 [`#1140`](https://github.com/isomerpages/isomercms-backend/pull/1140)
-- fix(tsak-def): add env vars [`fdf1230`](https://github.com/isomerpages/isomercms-backend/commit/fdf123001fc5fa6d8feaed0545eacb2670ff4ffb)
 
 #### [v0.66.4](https://github.com/isomerpages/isomercms-backend/compare/v0.66.3...v0.66.4)
 
 > 20 February 2024
 
 - fix(ci): use workflwo [`a5a225d`](https://github.com/isomerpages/isomercms-backend/commit/a5a225dd203826726ad9f8e39c420bdb4a8e0d2c)
-- fix(tsak-def): add env vars [`ad02a2f`](https://github.com/isomerpages/isomercms-backend/commit/ad02a2f630d657b468bb48fd48f0572051ae85e3)
+- fix(tsak-def): add env vars [`fdf1230`](https://github.com/isomerpages/isomercms-backend/commit/fdf123001fc5fa6d8feaed0545eacb2670ff4ffb)
 
 #### [v0.66.3](https://github.com/isomerpages/isomercms-backend/compare/v0.66.2...v0.66.3)
 
@@ -138,12 +148,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 - fix: remove unnecessary push logs [`#1109`](https://github.com/isomerpages/isomercms-backend/pull/1109)
 - fix(rr): skip checking the existence of review request [`#1102`](https://github.com/isomerpages/isomercms-backend/pull/1102)
 - release/0.61.0 [`#1104`](https://github.com/isomerpages/isomercms-backend/pull/1104)
-- fix(sl): include issuewild if CAA records are needed [`#1106`](https://github.com/isomerpages/isomercms-backend/pull/1106)
 
 #### [v0.61.0](https://github.com/isomerpages/isomercms-backend/compare/v0.60.0...v0.61.0)
 
-> 10 January 2024
+> 11 January 2024
 
+- fix(sl): include issuewild if CAA records are needed [`#1106`](https://github.com/isomerpages/isomercms-backend/pull/1106)
 - chore: upgrade axios [`#1100`](https://github.com/isomerpages/isomercms-backend/pull/1100)
 - build(deps): bump follow-redirects from 1.15.2 to 1.15.4 [`#1101`](https://github.com/isomerpages/isomercms-backend/pull/1101)
 - fix(ci): reverts ci changes to allow staging updates [`#1084`](https://github.com/isomerpages/isomercms-backend/pull/1084)
@@ -225,12 +235,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 - fix(siteCreate): add redirect rules [`#1036`](https://github.com/isomerpages/isomercms-backend/pull/1036)
 - chore: remove extra and unused submodules [`#1031`](https://github.com/isomerpages/isomercms-backend/pull/1031)
 - release/0.54.0 [`#1033`](https://github.com/isomerpages/isomercms-backend/pull/1033)
-- fix: use cTimeMs instead of birthtime due to EFS [`#1035`](https://github.com/isomerpages/isomercms-backend/pull/1035)
 
 #### [v0.54.0](https://github.com/isomerpages/isomercms-backend/compare/v0.53.0...v0.54.0)
 
 > 14 November 2023
 
+- fix: use cTimeMs instead of birthtime due to EFS [`#1035`](https://github.com/isomerpages/isomercms-backend/pull/1035)
 - fix(pagination): total length [`#1032`](https://github.com/isomerpages/isomercms-backend/pull/1032)
 - fix(staging-lite): apps were created for wrong br [`#1014`](https://github.com/isomerpages/isomercms-backend/pull/1014)
 - fix(cm): extra timeout [`#1027`](https://github.com/isomerpages/isomercms-backend/pull/1027)
@@ -357,12 +367,12 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 - build(deps-dev): bump @babel/traverse from 7.22.8 to 7.23.2 [`#984`](https://github.com/isomerpages/isomercms-backend/pull/984)
 - release/v0.48.0 [`#979`](https://github.com/isomerpages/isomercms-backend/pull/979)
 - feat(staging-id): add column to store the id [`#983`](https://github.com/isomerpages/isomercms-backend/pull/983)
-- Fix: collaborators service tests [`#978`](https://github.com/isomerpages/isomercms-backend/pull/978)
 
 #### [v0.48.0](https://github.com/isomerpages/isomercms-backend/compare/v0.47.0...v0.48.0)
 
 > 18 October 2023
 
+- Fix: collaborators service tests [`#978`](https://github.com/isomerpages/isomercms-backend/pull/978)
 - chore(commitService): proper naming [`#975`](https://github.com/isomerpages/isomercms-backend/pull/975)
 - Feat/is 585 govt sgid login rollout [`#976`](https://github.com/isomerpages/isomercms-backend/pull/976)
 - test(quickie): unit tests  [`#973`](https://github.com/isomerpages/isomercms-backend/pull/973)

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "isomercms",
-  "version": "0.69.0",
+  "version": "0.70.0",
   "private": true,
   "scripts": {
     "build": "tsc -p tsconfig.build.json",

diff --git a/src/config/config.ts b/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",

diff --git a/src/fixtures/sessionData.ts b/src/fixtures/sessionData.ts
@@ -21,7 +21,7 @@ import {
 export const mockAccessToken = "mockAccessToken"
 export const mockGithubId = "mockGithubId"
 export const mockIsomerUserId = "1"
-export const mockEmail = "mockEmail"
+export const mockEmail = "mockEmail@email.com"
 export const mockTreeSha = "mockTreeSha"
 export const mockCurrentCommitSha = "mockCurrentCommitSha"
 export const mockSiteName = "mockSiteName"

diff --git a/src/integration/Reviews.spec.ts b/src/integration/Reviews.spec.ts
@@ -975,9 +975,11 @@ describe("Review Requests Integration Tests", () => {
       )
 
       // Act
-      const actual = await request(app).post(
-        `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`
-      )
+      const actual = await request(app)
+        .post(`/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`)
+        .send({
+          reviewers: [MOCK_USER_EMAIL_THREE],
+        })
 
       // Assert
       expect(actual.statusCode).toEqual(404)
@@ -992,7 +994,11 @@ describe("Review Requests Integration Tests", () => {
       )
 
       // Act
-      const actual = await request(app).post(`/${MOCK_REPO_NAME_ONE}/123456`)
+      const actual = await request(app)
+        .post(`/${MOCK_REPO_NAME_ONE}/123456`)
+        .send({
+          reviewers: [MOCK_USER_EMAIL_THREE],
+        })
 
       // Assert
       expect(actual.statusCode).toEqual(404)
@@ -1007,9 +1013,11 @@ describe("Review Requests Integration Tests", () => {
       )
 
       // Act
-      const actual = await request(app).post(
-        `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`
-      )
+      const actual = await request(app)
+        .post(`/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`)
+        .send({
+          reviewers: [MOCK_USER_EMAIL_THREE],
+        })
 
       // Assert
       expect(actual.statusCode).toEqual(403)
@@ -1078,88 +1086,88 @@ describe("Review Requests Integration Tests", () => {
       })
     })
 
-    it("should close the review request successfully", async () => {
+    it("should return 404 if site is not found", async () => {
       // Arrange
       const app = generateRouterForUserWithSite(
         subrouter,
         MOCK_USER_SESSION_DATA_ONE,
-        MOCK_REPO_NAME_ONE
+        MOCK_REPO_NAME_TWO
       )
-      mockGenericAxios.patch.mockResolvedValueOnce(null)
 
       // Act
       const actual = await request(app).delete(
-        `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`
+        `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`
       )
 
       // Assert
-      expect(actual.statusCode).toEqual(200)
+      expect(actual.statusCode).toEqual(404)
     })
 
-    it("should return 404 if site is not found", async () => {
+    it("should return 404 if the review request is not found", async () => {
       // Arrange
       const app = generateRouterForUserWithSite(
         subrouter,
         MOCK_USER_SESSION_DATA_ONE,
-        MOCK_REPO_NAME_TWO
+        MOCK_REPO_NAME_ONE
       )
 
       // Act
-      const actual = await request(app).post(
-        `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`
-      )
+      const actual = await request(app).delete(`/${MOCK_REPO_NAME_ONE}/123456`)
 
       // Assert
       expect(actual.statusCode).toEqual(404)
     })
 
-    it("should return 404 if the review request is not found", async () => {
+    it("should return 403 if user is not the original requestor", async () => {
       // Arrange
       const app = generateRouterForUserWithSite(
         subrouter,
-        MOCK_USER_SESSION_DATA_ONE,
+        MOCK_USER_SESSION_DATA_TWO,
         MOCK_REPO_NAME_ONE
       )
 
       // Act
-      const actual = await request(app).post(`/${MOCK_REPO_NAME_ONE}/123456`)
+      const actual = await request(app).delete(
+        `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`
+      )
 
       // Assert
-      expect(actual.statusCode).toEqual(404)
+      expect(actual.statusCode).toEqual(403)
     })
 
-    it("should return 403 if user is not the original requestor", async () => {
+    it("should return 403 if the user is not a valid site member", async () => {
       // Arrange
       const app = generateRouterForUserWithSite(
         subrouter,
-        MOCK_USER_SESSION_DATA_TWO,
+        MOCK_USER_SESSION_DATA_THREE,
         MOCK_REPO_NAME_ONE
       )
 
       // Act
-      const actual = await request(app).post(
+      const actual = await request(app).delete(
         `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`
       )
 
       // Assert
       expect(actual.statusCode).toEqual(403)
     })
 
-    it("should return 403 if the user is not a valid site member", async () => {
+    it("should close the review request successfully", async () => {
       // Arrange
       const app = generateRouterForUserWithSite(
         subrouter,
-        MOCK_USER_SESSION_DATA_THREE,
+        MOCK_USER_SESSION_DATA_ONE,
         MOCK_REPO_NAME_ONE
       )
+      mockGenericAxios.patch.mockResolvedValueOnce(null)
 
       // Act
-      const actual = await request(app).post(
+      const actual = await request(app).delete(
         `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}`
       )
 
       // Assert
-      expect(actual.statusCode).toEqual(403)
+      expect(actual.statusCode).toEqual(200)
     })
   })
 
@@ -1871,9 +1879,11 @@ describe("Review Requests Integration Tests", () => {
       mockGenericAxios.post.mockResolvedValueOnce(null)
 
       // Act
-      const actual = await request(app).post(
-        `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments`
-      )
+      const actual = await request(app)
+        .post(
+          `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments`
+        )
+        .send({ message: "blahblah" })
 
       // Assert
       expect(actual.statusCode).toEqual(200)
@@ -1888,9 +1898,11 @@ describe("Review Requests Integration Tests", () => {
       )
 
       // Act
-      const actual = await request(app).post(
-        `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments`
-      )
+      const actual = await request(app)
+        .post(
+          `/${MOCK_REPO_NAME_TWO}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments`
+        )
+        .send({ message: "blahblah" })
 
       // Assert
       expect(actual.statusCode).toEqual(404)
@@ -1905,9 +1917,11 @@ describe("Review Requests Integration Tests", () => {
       )
 
       // Act
-      const actual = await request(app).post(
-        `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments`
-      )
+      const actual = await request(app)
+        .post(
+          `/${MOCK_REPO_NAME_ONE}/${MOCK_GITHUB_PULL_REQUEST_NUMBER}/comments`
+        )
+        .send({ message: "blahblah" })
 
       // Assert
       expect(actual.statusCode).toEqual(404)
@@ -1922,9 +1936,9 @@ describe("Review Requests Integration Tests", () => {
       )
 
       // Act
-      const actual = await request(app).post(
-        `/${MOCK_REPO_NAME_ONE}/123456/comments`
-      )
+      const actual = await request(app)
+        .post(`/${MOCK_REPO_NAME_ONE}/123456/comments`)
+        .send({ message: "blahblah" })
 
       // Assert
       expect(actual.statusCode).toEqual(404)