feat: harden rate limits (#909)

* feat: return 429 when rate limit is exceeded

* feat: format auth OTP err message correctly

* feat: return consistent error shape for send OTP

* feat: improve error message

* test: update tests

* feat: update env var default rate limit

src/app/modules/auth/__tests__/auth.controller.spec.ts

@@ -104,7 +104,7 @@ describe('auth.controller', () => {
 
       // Assert
       expect(mockRes.status).toBeCalledWith(expectedError.status)
-      expect(mockRes.json).toBeCalledWith(expectedError.message)
+      expect(mockRes.json).toBeCalledWith({ message: expectedError.message })
     })
 
     it('should return 500 when there is an error generating login OTP', async () => {
@@ -123,9 +123,10 @@ describe('auth.controller', () => {
 
       // Assert
       expect(mockRes.status).toBeCalledWith(500)
-      expect(mockRes.json).toBeCalledWith(
-        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
-      )
+      expect(mockRes.json).toBeCalledWith({
+        message:
+          'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
+      })
       // Sending login OTP should not have been called.
       expect(MockAuthService.createLoginOtp).toHaveBeenCalledTimes(1)
       expect(MockMailService.sendLoginOtp).not.toHaveBeenCalled()
@@ -148,9 +149,10 @@ describe('auth.controller', () => {
 
       // Assert
       expect(mockRes.status).toBeCalledWith(500)
-      expect(mockRes.json).toBeCalledWith(
-        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
-      )
+      expect(mockRes.json).toBeCalledWith({
+        message:
+          'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
+      })
       // Services should have been invoked.
       expect(MockAuthService.createLoginOtp).toHaveBeenCalledTimes(1)
       expect(MockMailService.sendLoginOtp).toHaveBeenCalledTimes(1)

src/app/modules/auth/__tests__/auth.routes.spec.ts

@@ -167,9 +167,10 @@ describe('auth.routes', () => {
 
       // Assert
       expect(response.status).toEqual(401)
-      expect(response.body).toEqual(
-        'This is not a whitelisted public service email domain. Please log in with your official government or government-linked email address.',
-      )
+      expect(response.body).toEqual({
+        message:
+          'This is not a whitelisted public service email domain. Please log in with your official government or government-linked email address.',
+      })
     })
 
     it('should return 500 when error occurs whilst creating OTP', async () => {
@@ -186,9 +187,10 @@ describe('auth.routes', () => {
       // Assert
       expect(createLoginOtpSpy).toHaveBeenCalled()
       expect(response.status).toEqual(500)
-      expect(response.body).toEqual(
-        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
-      )
+      expect(response.body).toEqual({
+        message:
+          'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
+      })
     })
 
     it('should return 500 when error occurs whilst sending login OTP', async () => {
@@ -205,9 +207,10 @@ describe('auth.routes', () => {
       // Assert
       expect(sendLoginOtpSpy).toHaveBeenCalled()
       expect(response.status).toEqual(500)
-      expect(response.body).toEqual(
-        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
-      )
+      expect(response.body).toEqual({
+        message:
+          'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
+      })
     })
 
     it('should return 500 when validating domain returns a database error', async () => {
@@ -224,9 +227,10 @@ describe('auth.routes', () => {
       // Assert
       expect(getAgencySpy).toBeCalled()
       expect(response.status).toEqual(500)
-      expect(response.body).toEqual(
-        'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
-      )
+      expect(response.body).toEqual({
+        message:
+          'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
+      })
     })
 
     it('should return 200 when otp is sent successfully', async () => {

src/app/modules/auth/auth.controller.ts

@@ -54,7 +54,7 @@ export const handleCheckUser: RequestHandler<
  */
 export const handleLoginSendOtp: RequestHandler<
   ParamsDictionary,
-  string,
+  { message: string } | string,
   { email: string }
 > = async (req, res) => {
   // Joi validation ensures existence.
@@ -99,7 +99,7 @@ export const handleLoginSendOtp: RequestHandler<
           error,
           /* coreErrorMessage=*/ 'Failed to send login OTP. Please try again later and if the problem persists, contact us.',
         )
-        return res.status(statusCode).json(errorMessage)
+        return res.status(statusCode).json({ message: errorMessage })
       })
   )
 }

src/app/utils/__tests__/limit-rate.spec.ts

@@ -30,15 +30,17 @@ describe('limitRate', () => {
     )
   })
 
-  it('should call next() in the handler', () => {
-    limitRate()
+  it('should return 429 when the rate limit is exceeded', () => {
+    limitRate({ max: 0 })
     const handler = MockRateLimit.mock.calls[0][0]!.handler!
     const mockNext = jest.fn()
-    handler(
-      expressHandler.mockRequest(),
-      expressHandler.mockResponse(),
-      mockNext,
-    )
-    expect(mockNext).toHaveBeenCalled()
+    const mockRes = expressHandler.mockResponse()
+    handler(expressHandler.mockRequest(), mockRes, mockNext)
+    expect(mockNext).not.toHaveBeenCalled()
+    expect(mockRes.status).toHaveBeenCalledWith(429)
+    expect(mockRes.json).toHaveBeenCalledWith({
+      message:
+        'We are experiencing a temporary issue. Please try again in one minute.',
+    })
   })
 })

src/app/utils/limit-rate.ts

@@ -2,6 +2,7 @@ import RateLimit, {
   Options as RateLimitOptions,
   RateLimit as RateLimitFn,
 } from 'express-rate-limit'
+import { StatusCodes } from 'http-status-codes'
 import { merge } from 'lodash'
 
 import { createLoggerWithLabel } from '../../config/logger'
@@ -21,7 +22,7 @@ export const limitRate = (options: RateLimitOptions = {}): RateLimitFn => {
   const defaultOptions: RateLimitOptions = {
     windowMs: 60 * 1000, // Apply rate per-minute
     max: 1200,
-    handler: (req, _res, next) => {
+    handler: (req, res) => {
       logger.warn({
         message: 'Rate limit exceeded',
         meta: {
@@ -31,8 +32,10 @@ export const limitRate = (options: RateLimitOptions = {}): RateLimitFn => {
           rateLimitInfo: req.rateLimit,
         },
       })
-      // TODO (private #49): terminate the request with HTTP 429
-      return next()
+      return res.status(StatusCodes.TOO_MANY_REQUESTS).json({
+        message:
+          'We are experiencing a temporary issue. Please try again in one minute.',
+      })
     },
   }
   return RateLimit(merge(defaultOptions, options))

src/config/schema.ts

@@ -268,9 +268,10 @@ export const optionalVarsSchema: Schema<IOptionalVarsSchema> = {
   },
   rateLimit: {
     submissions: {
-      doc: 'Per-minute, per-IP request limit for submissions endpoints',
+      doc:
+        'Per-minute, per-IP, per-instance request limit for submissions endpoints',
       format: 'int',
-      default: 200,
+      default: 80,
       env: 'SUBMISSIONS_RATE_LIMIT',
     },
     sendAuthOtp: {

src/public/modules/users/controllers/authentication.client.controller.js

@@ -180,11 +180,14 @@ function AuthenticationController($state, $timeout, $window, Auth, GTag) {
       function (error) {
         vm.isOtpSending = false
         vm.buttonClicked = false
-        // Configure message to be show
+        // Configure message to be shown
+        const msg =
+          (error && error.data && error.data.message) ||
+          'Failed to send login OTP. Please try again later and if the problem persists, contact us.'
         vm.signInMsg = {
           isMsg: true,
           isError: true,
-          msg: error.data,
+          msg,
         }
         $timeout(function () {
           angular.element('#otp-input').focus()