Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "fix(rateLimiter): correct rate limits" #1195

Merged
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/config/config.ts
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@

convict.addFormat({
name: "required-string",
validate: (val: any) => {

Check warning on line 5 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type

Check warning on line 5 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type
if (!val) throw new Error("value cannot be empty, null or undefined")
if (typeof val !== "string") throw new Error("value must be a string")
},
@@ -10,14 +10,14 @@

convict.addFormat({
name: "required-positive-number",
validate: (val: any) => {

Check warning on line 13 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type

Check warning on line 13 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type
if (val === null || val === undefined || val === "")
throw new Error("value cannot be empty, null or undefined")
if (typeof val !== "number") throw new Error("value must be a number")
},
coerce: (val: string) => {
const coercedVal = Number(val)
if (isNaN(coercedVal)) {

Check warning on line 20 in src/config/config.ts

GitHub Actions / lint

Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan

Check warning on line 20 in src/config/config.ts

GitHub Actions / lint

Unexpected use of 'isNaN'. Use Number.isNaN instead https://github.com/airbnb/javascript#standard-library--isnan
throw new Error(
"value provided is not a positive number. please provide a valid positive number"
)
@@ -31,7 +31,7 @@

convict.addFormat({
name: "required-boolean",
validate: (val: any) => {

Check warning on line 34 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type

Check warning on line 34 in src/config/config.ts

GitHub Actions / lint

Unexpected any. Specify a different type
if (val === null || val === undefined)
throw new Error("value cannot be empty, null or undefined")
if (typeof val !== "boolean") throw new Error("value must be a boolean")
@@ -96,7 +96,7 @@
format: ["localhost", "cms.isomer.gov.sg", "isomer.gov.sg"],
default: "localhost",
},
tokenExpiryInMs: {
tokenExpiry: {
doc: "Expiry duration for auth token in milliseconds",
env: "AUTH_TOKEN_EXPIRY_DURATION_IN_MILLISECONDS",
format: "required-positive-number",
9 changes: 7 additions & 2 deletions src/server.js
Original file line number Diff line number Diff line change
@@ -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 RepoCheckerService from "./services/review/RepoCheckerService"
import ReviewCommentService from "./services/review/ReviewCommentService"
import RepoCheckerService from "./services/review/RepoCheckerService"
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.tokenExpiryInMs")
const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiry")

const sequelize = initSequelize([
Site,
@@ -417,6 +417,11 @@ const formsgSiteAuditLogsRouter = new FormsgSiteAuditLogsRouter({

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
18 changes: 2 additions & 16 deletions src/services/utilServices/RateLimiter.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import rateLimit from "express-rate-limit"

import { BaseIsomerError } from "@root/errors/BaseError"
import { isSecure } from "@root/utils/auth-utils"
import { config } from "@config/config"

const DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS = 900000

@@ -14,21 +13,8 @@ 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: DEFAULT_AUTH_TOKEN_EXPIRY_MILLISECONDS,
windowMs: config.get("auth.tokenExpiry") / (1000 * 60),
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
@@ -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.tokenExpiryInMs").toString()
const AUTH_TOKEN_EXPIRY_MS = config.get("auth.tokenExpiry").toString()

const jwtUtil = {
decodeToken: _.wrap(jwt.decode, (decode, token) => decode(token)),

Unchanged files with check annotations Beta

import { config } from "@config/config"
export enum JobStatus {

Check warning on line 5 in src/constants/constants.ts

GitHub Actions / lint

'JobStatus' is already declared in the upper scope on line 5 column 13

Check warning on line 5 in src/constants/constants.ts

GitHub Actions / lint

'JobStatus' is already declared in the upper scope on line 5 column 13
Ready = "READY", // Ready to run jobs
Running = "RUNNING", // A job is running
Failed = "FAILED", // A job has failed and recovery is needed
}
export enum SiteStatus {

Check warning on line 11 in src/constants/constants.ts

GitHub Actions / lint

'SiteStatus' is already declared in the upper scope on line 11 column 13

Check warning on line 11 in src/constants/constants.ts

GitHub Actions / lint

'SiteStatus' is already declared in the upper scope on line 11 column 13
Empty = "EMPTY", // A site record site is being initialized
Initialized = "INITIALIZED",
Launched = "LAUNCHED",
}
export enum RedirectionTypes {

Check warning on line 17 in src/constants/constants.ts

GitHub Actions / lint

'RedirectionTypes' is already declared in the upper scope on line 17 column 13

Check warning on line 17 in src/constants/constants.ts

GitHub Actions / lint

'RedirectionTypes' is already declared in the upper scope on line 17 column 13
CNAME = "CNAME",
A = "A",
}
export enum CollaboratorRoles {

Check warning on line 22 in src/constants/constants.ts

GitHub Actions / lint

'CollaboratorRoles' is already declared in the upper scope on line 22 column 13

Check warning on line 22 in src/constants/constants.ts

GitHub Actions / lint

'CollaboratorRoles' is already declared in the upper scope on line 22 column 13
Admin = "ADMIN",
Contributor = "CONTRIBUTOR",
IsomerAdmin = "ISOMERADMIN",
CollaboratorRoles.IsomerAdmin
>
export enum ReviewRequestStatus {

Check warning on line 33 in src/constants/constants.ts

GitHub Actions / lint

'ReviewRequestStatus' is already declared in the upper scope on line 33 column 13

Check warning on line 33 in src/constants/constants.ts

GitHub Actions / lint

'ReviewRequestStatus' is already declared in the upper scope on line 33 column 13
Approved = "APPROVED",
Open = "OPEN",
Merged = "MERGED",
export const FEATURE_FLAGS = {

Check warning on line 1 in src/constants/featureFlags.ts

GitHub Actions / lint

Prefer default export on a file with single export

Check warning on line 1 in src/constants/featureFlags.ts

GitHub Actions / lint

Prefer default export on a file with single export
IS_BUILD_TIMES_REDUCTION_ENABLED: "is_build_times_reduction_enabled",
IS_GGS_ENABLED: "is_ggs_enabled",
IS_SHOW_STAGING_BUILD_STATUS_ENABLED: "is_show_staging_build_status_enabled",