feat: log ip address on callback from twilio (#3937)

* feat: log mobile number when otp generated * Revert "feat: log mobile number when otp generated" This reverts commit 58b4bc5. * feat: pass senderIp as query param to twilio status callback * chore: update tests * chore: use node url api * chore: pass in full url to URL api * chore: add try catch block, fix tests
opengovsg · Jun 7, 2022 · 0493774 · 0493774
1 parent e5e173d
commit 0493774
Show file tree

Hide file tree

Showing 14 changed files with 141 additions and 10 deletions.
diff --git a/src/app/modules/twilio/__tests__/twilio.controller.spec.ts b/src/app/modules/twilio/__tests__/twilio.controller.spec.ts
@@ -48,6 +48,13 @@ describe('twilio.controller', () => {
     it('should return 200 when successfully delivered message is sent', async () => {
       const mockReq = expressHandler.mockRequest({
         body: MOCK_SUCCESSFUL_MESSAGE,
+        others: {
+          protocol: 'https',
+          host: 'webhook-endpoint.gov.sg',
+          url: `/endpoint?${encodeURI('senderIp=200.0.0.0')}`,
+          originalUrl: `/endpoint?${encodeURI('senderIp=200.0.0.0')}`,
+          get: () => 'webhook-endpoint.gov.sg',
+        },
       })
       const mockRes = expressHandler.mockResponse()
       await twilioSmsUpdates(mockReq, mockRes, jest.fn())
@@ -57,6 +64,7 @@ describe('twilio.controller', () => {
         meta: {
           action: 'twilioSmsUpdates',
           body: MOCK_SUCCESSFUL_MESSAGE,
+          senderIp: '200.0.0.0',
         },
       })
       expect(mockLogger.error).not.toBeCalled()
@@ -66,6 +74,13 @@ describe('twilio.controller', () => {
     it('should return 200 when failed delivered message is sent', async () => {
       const mockReq = expressHandler.mockRequest({
         body: MOCK_FAILED_MESSAGE,
+        others: {
+          protocol: 'https',
+          host: 'webhook-endpoint.gov.sg',
+          url: `/endpoint?${encodeURI('senderIp=200.0.0.0')}`,
+          originalUrl: `/endpoint?${encodeURI('senderIp=200.0.0.0')}`,
+          get: () => 'webhook-endpoint.gov.sg',
+        },
       })
       const mockRes = expressHandler.mockResponse()
       await twilioSmsUpdates(mockReq, mockRes, jest.fn())
@@ -76,6 +91,7 @@ describe('twilio.controller', () => {
         meta: {
           action: 'twilioSmsUpdates',
           body: MOCK_FAILED_MESSAGE,
+          senderIp: '200.0.0.0',
         },
       })
       expect(mockRes.sendStatus).toBeCalledWith(200)

diff --git a/src/app/modules/twilio/twilio.controller.ts b/src/app/modules/twilio/twilio.controller.ts
@@ -54,6 +54,24 @@ export const twilioSmsUpdates: ControllerHandler<
    * Example: https://www.twilio.com/docs/usage/webhooks/sms-webhooks.
    */
 
+  // Extract public sender's ip address which was passed to twilio as a query param in the status callback
+  let senderIp = null
+  try {
+    const url = new URL(
+      req.protocol + '://' + req.get('host') + req.originalUrl,
+    )
+    senderIp = url.searchParams.get('senderIp')
+  } catch {
+    logger.error({
+      message: 'Error occurred when extracting senderIp',
+      meta: {
+        action: 'twilioSmsUpdates',
+        body: req.body,
+        originalUrl: req.originalUrl,
+      },
+    })
+  }
+
   const ddTags: TwilioSmsStatsdTags = {
     // msgSrvcSid not included to limit tag cardinality (for now?)
     smsstatus: req.body.SmsStatus,
@@ -70,6 +88,7 @@ export const twilioSmsUpdates: ControllerHandler<
       meta: {
         action: 'twilioSmsUpdates',
         body: req.body,
+        senderIp,
       },
     })
   } else {
@@ -78,6 +97,7 @@ export const twilioSmsUpdates: ControllerHandler<
       meta: {
         action: 'twilioSmsUpdates',
         body: req.body,
+        senderIp,
       },
     })
   }

diff --git a/src/app/modules/user/__tests__/user.controller.spec.ts b/src/app/modules/user/__tests__/user.controller.spec.ts
@@ -63,6 +63,7 @@ describe('user.controller', () => {
         MOCK_REQ.body.contact,
         expectedOtp,
         MOCK_REQ.body.userId,
+        'MOCK_IP',
       )
       expect(mockRes.sendStatus).toBeCalledWith(200)
     })

diff --git a/src/app/modules/user/user.controller.ts b/src/app/modules/user/user.controller.ts
@@ -40,10 +40,12 @@ export const _handleContactSendOtp: ControllerHandler<
     return res.status(StatusCodes.UNAUTHORIZED).json('User is unauthorized.')
   }
 
+  const senderIp = getRequestIp(req)
+
   const logMeta = {
     action: 'handleContactSendOtp',
     userId,
-    ip: getRequestIp(req),
+    ip: senderIp,
   }
 
   // Step 1: Create OTP for contact verification.
@@ -67,6 +69,7 @@ export const _handleContactSendOtp: ControllerHandler<
     contact,
     otp,
     userId,
+    senderIp,
   )
 
   // Error sending OTP.

diff --git a/src/app/modules/verification/__tests__/verification.controller.spec.ts b/src/app/modules/verification/__tests__/verification.controller.spec.ts
@@ -391,6 +391,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.sendStatus).toHaveBeenCalledWith(StatusCodes.CREATED)
     })
@@ -409,6 +410,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND)
       expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) })
@@ -428,6 +430,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) })
@@ -447,6 +450,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND)
       expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) })
@@ -481,6 +485,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(
         StatusCodes.UNPROCESSABLE_ENTITY,
@@ -502,6 +507,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) })
@@ -521,6 +527,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) })
@@ -540,6 +547,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) })
@@ -559,6 +567,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) })
@@ -578,6 +587,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith({ message: expect.any(String) })
@@ -597,6 +607,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(
         StatusCodes.INTERNAL_SERVER_ERROR,
@@ -664,6 +675,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(
         MockVerificationService.disableVerifiedFieldsIfRequired,
@@ -702,6 +714,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
@@ -734,6 +747,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
@@ -766,6 +780,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
@@ -799,6 +814,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
@@ -832,6 +848,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
@@ -886,6 +903,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.BAD_REQUEST)
       expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
@@ -944,6 +962,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND)
       expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
@@ -976,6 +995,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(StatusCodes.NOT_FOUND)
       expect(mockRes.json).toHaveBeenCalledWith(expectedResponse)
@@ -1008,6 +1028,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(
         StatusCodes.UNPROCESSABLE_ENTITY,
@@ -1067,6 +1088,7 @@ describe('Verification controller', () => {
         otp: MOCK_OTP,
         hashedOtp: MOCK_HASHED_OTP,
         recipient: MOCK_ANSWER,
+        senderIp: 'MOCK_IP',
       })
       expect(mockRes.status).toHaveBeenCalledWith(
         StatusCodes.INTERNAL_SERVER_ERROR,

diff --git a/src/app/modules/verification/__tests__/verification.service.spec.ts b/src/app/modules/verification/__tests__/verification.service.spec.ts
@@ -66,6 +66,7 @@ import {
   MOCK_HASHED_OTP,
   MOCK_OTP,
   MOCK_RECIPIENT,
+  MOCK_SENDER_IP,
   MOCK_SIGNED_DATA,
 } from './verification.test.helpers'
 
@@ -327,13 +328,15 @@ describe('Verification service', () => {
         hashedOtp: MOCK_HASHED_OTP,
         otp: MOCK_OTP,
         recipient: MOCK_RECIPIENT,
+        senderIp: MOCK_SENDER_IP,
       })
 
       // Default mock params has fieldType: 'mobile'
       expect(MockSmsFactory.sendVerificationOtp).toHaveBeenCalledWith(
         MOCK_RECIPIENT,
         MOCK_OTP,
         mockTransaction.formId,
+        MOCK_SENDER_IP,
       )
       expect(MockFormsgSdk.verification.generateSignature).toHaveBeenCalledWith(
         {
@@ -360,6 +363,7 @@ describe('Verification service', () => {
         hashedOtp: MOCK_HASHED_OTP,
         otp: MOCK_OTP,
         recipient: MOCK_RECIPIENT,
+        senderIp: MOCK_SENDER_IP,
       })
 
       expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled()
@@ -384,6 +388,7 @@ describe('Verification service', () => {
         hashedOtp: MOCK_HASHED_OTP,
         otp: MOCK_OTP,
         recipient: MOCK_RECIPIENT,
+        senderIp: MOCK_SENDER_IP,
       })
 
       expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled()
@@ -403,6 +408,7 @@ describe('Verification service', () => {
         hashedOtp: MOCK_HASHED_OTP,
         otp: MOCK_OTP,
         recipient: MOCK_RECIPIENT,
+        senderIp: MOCK_SENDER_IP,
       })
 
       expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled()
@@ -434,6 +440,7 @@ describe('Verification service', () => {
         hashedOtp: MOCK_HASHED_OTP,
         otp: MOCK_OTP,
         recipient: MOCK_RECIPIENT,
+        senderIp: MOCK_SENDER_IP,
       })
 
       expect(MockMailService.sendVerificationOtp).not.toHaveBeenCalled()
@@ -492,6 +499,7 @@ describe('Verification service', () => {
         hashedOtp: MOCK_HASHED_OTP,
         otp: MOCK_OTP,
         recipient: MOCK_RECIPIENT,
+        senderIp: MOCK_SENDER_IP,
       })
 
       expect(MockMailService.sendVerificationOtp).toHaveBeenCalledWith(
@@ -526,12 +534,14 @@ describe('Verification service', () => {
         hashedOtp: MOCK_HASHED_OTP,
         otp: MOCK_OTP,
         recipient: MOCK_RECIPIENT,
+        senderIp: MOCK_SENDER_IP,
       })
 
       expect(MockSmsFactory.sendVerificationOtp).toHaveBeenCalledWith(
         MOCK_RECIPIENT,
         MOCK_OTP,
         new ObjectId(mockFormId),
+        MOCK_SENDER_IP,
       )
       expect(
         MockFormsgSdk.verification.generateSignature,
@@ -551,13 +561,15 @@ describe('Verification service', () => {
         hashedOtp: MOCK_HASHED_OTP,
         otp: MOCK_OTP,
         recipient: MOCK_RECIPIENT,
+        senderIp: MOCK_SENDER_IP,
       })
 
       // Mock params default to mobile
       expect(MockSmsFactory.sendVerificationOtp).toHaveBeenCalledWith(
         MOCK_RECIPIENT,
         MOCK_OTP,
         new ObjectId(mockFormId),
+        MOCK_SENDER_IP,
       )
       expect(MockFormsgSdk.verification.generateSignature).toHaveBeenCalledWith(
         {

diff --git a/src/app/modules/verification/__tests__/verification.test.helpers.ts b/src/app/modules/verification/__tests__/verification.test.helpers.ts
@@ -7,6 +7,7 @@ export const MOCK_SIGNED_DATA = 'mockSignedData'
 export const MOCK_HASHED_OTP = 'mockHashedOtp'
 export const MOCK_OTP = '123456'
 export const MOCK_RECIPIENT = '81234567'
+export const MOCK_SENDER_IP = '200.0.0.0'
 
 export const VFN_FIELD_DEFAULTS = {
   signedData: null,