Release v1.40.2 (#967)

* Add learning resources to sidebar
* Improve missing pipe page
* Prevent duplicate form submissions

Author: kevinkim-ogp

package-lock.json

@@ -106,5 +106,5 @@
     "tsconfig-paths": "^4.2.0",
     "type-fest": "4.10.3"
   },
-  "version": "1.40.1"
+  "version": "1.40.2"
 }

packages/backend/package.json

@@ -12,6 +12,16 @@ import { NricFilter } from '../../triggers/new-submission'
 LuxonSettings.defaultZone = 'Asia/Singapore'
 LuxonSettings.defaultLocale = 'en-SG'
 
+const SUCCESS_DECRYPT_RESPONSE = {
+  verified: true,
+  internalId: 'submissionId',
+}
+
+const FAILED_DECRYPT_RESPONSE = {
+  verified: false,
+  internalId: null as string | null,
+}
+
 // mocks hoisted here so that they can be used in import mocks
 const mocks = vi.hoisted(() => {
   const webhooksAuthenticate = vi.fn()
@@ -128,7 +138,9 @@ describe('decrypt form response', () => {
 
     it('should fail if no request in global variable', async () => {
       delete $.request
-      await expect(decryptFormResponse($)).resolves.toEqual(false)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        FAILED_DECRYPT_RESPONSE,
+      )
       expect(mocks.consoleError).toHaveBeenCalledWith(
         'No trigger item provided',
       )
@@ -138,7 +150,9 @@ describe('decrypt form response', () => {
       mocks.webhooksAuthenticate.mockImplementationOnce(() => {
         throw new Error('error')
       })
-      await expect(decryptFormResponse($)).resolves.toEqual(false)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        FAILED_DECRYPT_RESPONSE,
+      )
       expect(mocks.webhooksAuthenticate).toHaveBeenCalledTimes(1)
       expect(mocks.consoleError).toHaveBeenCalledWith(
         'Unable to verify formsg signature',
@@ -147,7 +161,9 @@ describe('decrypt form response', () => {
 
     it('should fail and give warning if no connection exists', async () => {
       delete $.auth.data
-      await expect(decryptFormResponse($)).resolves.toEqual(false)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        FAILED_DECRYPT_RESPONSE,
+      )
       expect(mocks.consoleWarn).toHaveBeenCalledWith(
         'Form is not connected to any pipe after pipe is transferred',
         {
@@ -161,7 +177,9 @@ describe('decrypt form response', () => {
 
     it('should fail if unable to decrypt form response', async () => {
       mockDecryptedSubmission(null)
-      await expect(decryptFormResponse($)).resolves.toEqual(false)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        FAILED_DECRYPT_RESPONSE,
+      )
       expect(decryptMock).toHaveBeenCalledTimes(1)
       expect(mocks.consoleError).toHaveBeenCalledWith(
         'Unable to decrypt formsg response',
@@ -170,7 +188,9 @@ describe('decrypt form response', () => {
 
     it('should extract submission ID', async () => {
       mockDecryptedSubmission({ responses: [] })
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect($.request.body).toEqual(
         expect.objectContaining({
           submissionId: 'submissionId',
@@ -180,7 +200,9 @@ describe('decrypt form response', () => {
 
     it('should extract submission time as a ISO 8601 SGT formatted string', async () => {
       mockDecryptedSubmission({ responses: [] })
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect($.request.body).toEqual(
         expect.objectContaining({
           submissionTime: '2023-07-06T18:26:27.505+08:00',
@@ -205,7 +227,9 @@ describe('decrypt form response', () => {
           },
         ],
       })
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect($.request.body).toEqual(
         expect.objectContaining({
           fields: {
@@ -260,7 +284,9 @@ describe('decrypt form response', () => {
       })
 
       it('should handle nric filter - do nothing', async () => {
-        await expect(decryptFormResponse($)).resolves.toEqual(true)
+        await expect(decryptFormResponse($)).resolves.toEqual(
+          SUCCESS_DECRYPT_RESPONSE,
+        )
         expect($.request.body).toEqual(
           expect.objectContaining({
             fields: {
@@ -294,7 +320,9 @@ describe('decrypt form response', () => {
 
       it('it should handle nric filter - remove', async () => {
         $.step.parameters.nricFilter = NricFilter.Remove
-        await expect(decryptFormResponse($)).resolves.toEqual(true)
+        await expect(decryptFormResponse($)).resolves.toEqual(
+          SUCCESS_DECRYPT_RESPONSE,
+        )
         expect($.request.body).toEqual(
           expect.objectContaining({
             fields: {
@@ -314,7 +342,9 @@ describe('decrypt form response', () => {
 
       it('it should handle nric filter - hash', async () => {
         $.step.parameters.nricFilter = NricFilter.Hash
-        await expect(decryptFormResponse($)).resolves.toEqual(true)
+        await expect(decryptFormResponse($)).resolves.toEqual(
+          SUCCESS_DECRYPT_RESPONSE,
+        )
         expect($.request.body).toEqual(
           expect.objectContaining({
             fields: {
@@ -348,7 +378,9 @@ describe('decrypt form response', () => {
 
       it('it should handle nric filter - mask', async () => {
         $.step.parameters.nricFilter = NricFilter.Mask
-        await expect(decryptFormResponse($)).resolves.toEqual(true)
+        await expect(decryptFormResponse($)).resolves.toEqual(
+          SUCCESS_DECRYPT_RESPONSE,
+        )
         expect($.request.body).toEqual(
           expect.objectContaining({
             fields: {
@@ -398,7 +430,9 @@ describe('decrypt form response', () => {
           cpUen: '987654321Z',
         },
       })
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect($.request.body).toEqual(
         expect.objectContaining({
           verifiedSubmitterInfo: {
@@ -428,7 +462,9 @@ describe('decrypt form response', () => {
           },
         ],
       })
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect($.request.body).toEqual(
         expect.objectContaining({
           fields: {
@@ -474,7 +510,9 @@ describe('decrypt form response', () => {
           },
         ],
       })
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect($.request.body).toEqual(
         expect.objectContaining({
           fields: {
@@ -508,7 +546,9 @@ describe('decrypt form response', () => {
     it('attachment decryption function not called if pipe does not process files', async () => {
       $.flow.hasFileProcessingActions = false
       mocks.cryptoDecrypt.mockReturnValueOnce({ responses: [] })
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect(mocks.cryptoDecryptWithAttachments).not.toBeCalled()
     })
 
@@ -518,7 +558,9 @@ describe('decrypt form response', () => {
         attachments: {},
         content: { responses: [] },
       })
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect(mocks.cryptoDecrypt).not.toBeCalled()
     })
   })
@@ -529,7 +571,9 @@ describe('decrypt form response', () => {
       mocks.cryptoDecrypt.mockReturnValueOnce({ responses: [] })
       mocks.parseFormEnv.mockReturnValue('staging')
 
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
 
       expect(mocks.parseFormEnv).toBeCalled()
       expect(mocks.getSdk).toBeCalledWith('staging')
@@ -563,7 +607,9 @@ describe('decrypt form response', () => {
         ],
       })
 
-      await expect(decryptFormResponse($)).resolves.toEqual(true)
+      await expect(decryptFormResponse($)).resolves.toEqual(
+        SUCCESS_DECRYPT_RESPONSE,
+      )
       expect($.request.body).toEqual(
         expect.objectContaining({
           fields: {

packages/backend/src/apps/formsg/__tests__/auth/decrypt-form-response.test.ts

@@ -57,10 +57,10 @@ export function filterNric($: IGlobalVariable, value: string): string | null {
 
 export async function decryptFormResponse(
   $: IGlobalVariable,
-): Promise<boolean> {
+): Promise<{ verified: boolean; internalId: string | null }> {
   if (!$.request) {
     logger.error('No trigger item provided')
-    return false
+    return { verified: false, internalId: null }
   }
 
   // Note: this could occur due to pipe transfer since connection becomes null
@@ -71,7 +71,7 @@ export async function decryptFormResponse(
       stepId: $.step.id,
       userId: $.user.id,
     })
-    return false
+    return { verified: false, internalId: null }
   }
 
   const formSgSdk = getSdk(parseFormEnv($))
@@ -88,7 +88,7 @@ export async function decryptFormResponse(
     )
   } catch (e) {
     logger.error('Unable to verify formsg signature')
-    return false
+    return { verified: false, internalId: null }
   }
 
   const formSecretKey = $.auth.data.privateKey as string
@@ -187,10 +187,10 @@ export async function decryptFormResponse(
     delete $.request.headers
     delete $.request.query
 
-    return true
+    return { verified: true, internalId: data.submissionId }
   } else {
     // Could not decrypt the submission
     logger.error('Unable to decrypt formsg response')
-    return false
+    return { verified: false, internalId: null }
   }
 }

packages/backend/src/apps/formsg/auth/decrypt-form-response.ts

@@ -29,7 +29,10 @@ const mocks = vi.hoisted(() => {
         })),
       })),
     },
-    processTrigger: vi.fn(() => ({ executionId: 'execution-id' })),
+    processTrigger: vi.fn(() => ({
+      executionId: 'execution-id',
+      shouldExecute: true,
+    })),
     enqueueActionJob: vi.fn(),
   }
 })
@@ -200,5 +203,19 @@ describe('webhook handler', () => {
         expect(mocks.response.sendStatus).toHaveReturnedWith(200)
       },
     )
+
+    it('should not enqueue job when processTrigger returns shouldExecute: false', async () => {
+      mocks.flow.active = true
+      mocks.processTrigger.mockResolvedValueOnce({
+        executionId: 'execution-id',
+        shouldExecute: false,
+      })
+
+      await webhookHandler(request, mocks.response)
+
+      expect(mocks.processTrigger).toHaveBeenCalledOnce()
+      expect(mocks.enqueueActionJob).not.toHaveBeenCalled()
+      expect(mocks.response.sendStatus).toHaveReturnedWith(200)
+    })
   })
 })

packages/backend/src/controllers/__tests__/webhooks/handler.test.ts

@@ -70,6 +70,7 @@ export default async (request: IRequest, response: Response) => {
     }
   }
 
+  let internalId: string = randomUUID()
   const testRun = !flow.active
   const triggerStep = await flow.getTriggerStep()
 
@@ -80,8 +81,8 @@ export default async (request: IRequest, response: Response) => {
     return response.sendStatus(404)
   }
 
-  const isWebhookApp =
-    triggerApp.key === 'webhook' || triggerApp.key === 'formsg'
+  const isFormsgApp = triggerApp.key === 'formsg'
+  const isWebhookApp = triggerApp.key === 'webhook' || isFormsgApp
 
   if (!isWebhookApp) {
     logger.info(`Invalid trigger app for flow${flowId}`)
@@ -106,11 +107,13 @@ export default async (request: IRequest, response: Response) => {
       request,
     })
 
-    const verified = await triggerApp.auth.verifyWebhook($)
+    const { verified, internalId: newInternalId } =
+      await triggerApp.auth.verifyWebhook($)
 
     if (!verified) {
       return response.sendStatus(401)
     }
+    internalId = newInternalId
   }
 
   // in case trigger type is 'webhook'
@@ -127,11 +130,16 @@ export default async (request: IRequest, response: Response) => {
   const triggerItem: ITriggerItem = {
     raw: payload,
     meta: {
-      internalId: randomUUID(),
+      internalId,
     },
   }
 
-  const { executionId } = await processTrigger({
+  /**
+   * NOTE: special case for when FormSG calls Plumber's webhook twice for payment forms.
+   * Plumber will still return a 200 status code, so that FormSG does not auto-retry,
+   * but Plumber not enqueue the job.
+   */
+  const { executionId, shouldExecute } = await processTrigger({
     flowId,
     stepId: triggerStep.id,
     triggerItem,
@@ -146,7 +154,7 @@ export default async (request: IRequest, response: Response) => {
     testRun,
   })
 
-  if (testRun) {
+  if (testRun || !shouldExecute) {
     return response.sendStatus(200)
   }