feat: insert cid field in attachments conditionally (#2073)

* feat: parse cid conditionally * fix: content-id backend tests
opengovsg · Jul 4, 2023 · 8d12a55 · 8d12a55
1 parent 230aea4
commit 8d12a55
Show file tree

Hide file tree

Showing 4 changed files with 96 additions and 10 deletions.
diff --git a/backend/src/core/services/file-attachment.service.ts b/backend/src/core/services/file-attachment.service.ts
@@ -13,12 +13,15 @@ const checkExtensions = async (
   return _.every(isAllowed)
 }
 
-const parseFiles = async (
-  files: { data: Buffer; name: string }[]
+const parseFilesAsMailAttachments = async (
+  files: { data: Buffer; name: string }[],
+  bodyContainsCidTags: boolean
 ): Promise<MailAttachment[]> => {
   return files.map(({ data, name }, index) => {
     // include cid field to support content-id images; see PR #1905
-    return { filename: name, content: data, cid: index.toString() }
+    return bodyContainsCidTags
+      ? { filename: name, content: data, cid: index.toString() }
+      : { filename: name, content: data }
   })
 }
 
@@ -27,7 +30,8 @@ export const UNSUPPORTED_FILE_TYPE_ERROR_CODE =
 
 const sanitizeFiles = async (
   files: { data: Buffer; name: string }[],
-  emailMessageTransactionalId: string
+  emailMessageTransactionalId: string,
+  bodyContainsCidTags: boolean
 ): Promise<MailAttachment[]> => {
   const isAcceptedType = await checkExtensions(files)
   if (!isAcceptedType) {
@@ -41,7 +45,7 @@ const sanitizeFiles = async (
     )
     throw new UnsupportedFileTypeError()
   }
-  return parseFiles(files)
+  return parseFilesAsMailAttachments(files, bodyContainsCidTags)
 }
 
 export const FileAttachmentService = {

diff --git a/backend/src/email/routes/tests/email-transactional.routes.test.ts b/backend/src/email/routes/tests/email-transactional.routes.test.ts
@@ -610,7 +610,6 @@ describe(`${emailTransactionalRoute}/send`, () => {
         messageId: expect.any(String),
         attachments: [
           {
-            cid: '0',
             content: expect.any(Buffer),
             filename: validAttachmentName,
           },
@@ -647,6 +646,82 @@ describe(`${emailTransactionalRoute}/send`, () => {
     ])
   })
 
+  test('Should send email with a valid attachment and attachment metadata is saved correctly in db (with content id tag)', async () => {
+    mockSendEmail = jest
+      .spyOn(EmailService, 'sendEmail')
+      .mockResolvedValue(true)
+
+    await EmailFromAddress.create({
+      email: user.email,
+      name: 'Agency ABC',
+    } as EmailFromAddress)
+
+    // request.send() cannot be used with file attachments
+    // substitute form values with request.field(). refer to
+    // https://visionmedia.github.io/superagent/#multipart-requests
+    const bodyWithContentIdTag =
+      validApiCallAttachment.body + '<img src="cid:0">'
+    const res = await request(app)
+      .post(endpoint)
+      .set('Authorization', `Bearer ${apiKey}`)
+      .field('recipient', validApiCallAttachment.recipient)
+      .field('subject', validApiCallAttachment.subject)
+      .field('body', bodyWithContentIdTag)
+      .field('from', validApiCallAttachment.from)
+      .field('reply_to', validApiCallAttachment.reply_to)
+      .attach('attachments', validAttachment, validAttachmentName)
+
+    expect(res.status).toBe(201)
+    expect(res.body).toBeDefined()
+    expect(res.body.attachments_metadata).toBeDefined()
+    expect(mockSendEmail).toBeCalledTimes(1)
+    expect(mockSendEmail).toBeCalledWith(
+      {
+        body: bodyWithContentIdTag,
+        from: validApiCallAttachment.from,
+        replyTo: validApiCallAttachment.reply_to,
+        subject: validApiCallAttachment.subject,
+        recipients: [validApiCallAttachment.recipient],
+        messageId: expect.any(String),
+        attachments: [
+          {
+            cid: '0',
+            content: expect.any(Buffer),
+            filename: validAttachmentName,
+          },
+        ],
+      },
+      {
+        extraSmtpHeaders: { isTransactional: true },
+      }
+    )
+    const transactionalEmail = await EmailMessageTransactional.findOne({
+      where: { userId: user.id.toString() },
+    })
+    expect(transactionalEmail).not.toBeNull()
+    expect(transactionalEmail).toMatchObject({
+      recipient: validApiCallAttachment.recipient,
+      from: validApiCallAttachment.from,
+      status: TransactionalEmailMessageStatus.Accepted,
+      errorCode: null,
+    })
+    expect(transactionalEmail?.params).toMatchObject({
+      subject: validApiCallAttachment.subject,
+      body: bodyWithContentIdTag,
+      from: validApiCallAttachment.from,
+      reply_to: validApiCallAttachment.reply_to,
+    })
+    expect(transactionalEmail?.attachmentsMetadata).not.toBeNull()
+    expect(transactionalEmail?.attachmentsMetadata).toHaveLength(1)
+    expect(transactionalEmail?.attachmentsMetadata).toMatchObject([
+      {
+        fileName: validAttachmentName,
+        fileSize: validAttachmentSize,
+        hash: expect.stringMatching(validAttachmentHashRegex),
+      },
+    ])
+  })
+
   test('Email with attachment that exceeds size limit should fail', async () => {
     mockSendEmail = jest.spyOn(EmailService, 'sendEmail')
     const invalidAttachmentTooBig = generateRandomFileSizeInMb(10)
@@ -740,12 +815,10 @@ describe(`${emailTransactionalRoute}/send`, () => {
         messageId: expect.any(String),
         attachments: [
           {
-            cid: '0',
             content: expect.any(Buffer),
             filename: validAttachmentName,
           },
           {
-            cid: '1',
             content: expect.any(Buffer),
             filename: validAttachment2Name,
           },

diff --git a/backend/src/email/services/email-transactional.service.ts b/backend/src/email/services/email-transactional.service.ts
@@ -71,11 +71,12 @@ async function sendMessage({
     )
     throw new MessageError()
   }
-
+  const bodyContainsCidTags = EmailTemplateService.client.containsCidTags(body)
   const sanitizedAttachments = attachments
     ? await FileAttachmentService.sanitizeFiles(
         attachments,
-        emailMessageTransactionalId
+        emailMessageTransactionalId,
+        bodyContainsCidTags
       )
     : undefined
 

diff --git a/shared/src/templating/template-client.ts b/shared/src/templating/template-client.ts
@@ -38,6 +38,14 @@ export class TemplateClient {
     return filterXSS(value, this.xssOptions)
   }
 
+  /**
+   * Check whether input contains cid tags
+   * @param value input to be checked
+   */
+  containsCidTags(value: string): boolean {
+    return value.includes('src="cid:')
+  }
+
   /**
    * Removes non-whitelisted html tags
    * Replaces new lines with html <br> so that the new lines can be displayed on the front-end