From f7d3ecbdba8384a02c4d66b6c0695edf7442e13e Mon Sep 17 00:00:00 2001 From: kevinkim-ogp Date: Tue, 26 Nov 2024 07:31:40 +0800 Subject: [PATCH] Fix: allow single CC/BCC recipient to be sent via form-data (#2286) * fix cc and bcc to allow stringified array * add tests for cc * abstract email validator, update test cases * revert to 2d array for test inputs --- .../routes/email-transactional.routes.ts | 57 ++++-- .../tests/email-transactional.routes.test.ts | 162 ++++++++++++++++++ 2 files changed, 204 insertions(+), 15 deletions(-) diff --git a/backend/src/email/routes/email-transactional.routes.ts b/backend/src/email/routes/email-transactional.routes.ts index 34e60cf35..b5ba562f5 100644 --- a/backend/src/email/routes/email-transactional.routes.ts +++ b/backend/src/email/routes/email-transactional.routes.ts @@ -21,6 +21,45 @@ export const InitEmailTransactionalRoute = ( const router = Router({ mergeParams: true }) // Validators + const emailValidator = Joi.string() + .trim() + .email() + .options({ convert: true }) + .lowercase() + + const emailArrayValidation = (fieldName: 'cc' | 'bcc') => { + return Joi.alternatives().try( + // array + Joi.array().unique().items(emailValidator), + + // stringified array (form-data) + Joi.string().custom((value: string) => { + let parsed + try { + parsed = JSON.parse(value) + } catch { + throw new Error( + `${fieldName} must be a valid array or stringified array.` + ) + } + + if (!Array.isArray(parsed)) { + throw new Error(`${fieldName} must be a valid stringified array`) + } + const { value: validatedEmails, error } = Joi.array() + .unique() + .items(emailValidator) + .validate(parsed) + + if (error) { + throw new Error(`${fieldName} ${error.message}`) + } + + return validatedEmails + }) + ) + } + const sendValidator = { [Segments.BODY]: Joi.object({ recipient: Joi.string() @@ -43,27 +82,15 @@ export const InitEmailTransactionalRoute = ( ) ), from: fromAddressValidator, - reply_to: Joi.string() - .trim() - .email() - .options({ convert: true }) - .lowercase(), + reply_to: emailValidator, attachments: Joi.array().items(Joi.object().keys().required()), classification: Joi.string() .uppercase() .valid(...Object.values(TransactionalEmailClassification)) .optional(), tag: Joi.string().max(255).optional(), - cc: Joi.array() - .unique() - .items( - Joi.string().trim().email().options({ convert: true }).lowercase() - ), - bcc: Joi.array() - .unique() - .items( - Joi.string().trim().email().options({ convert: true }).lowercase() - ), + cc: emailArrayValidation('cc'), + bcc: emailArrayValidation('bcc'), disable_tracking: Joi.boolean().default(false), }), } diff --git a/backend/src/email/routes/tests/email-transactional.routes.test.ts b/backend/src/email/routes/tests/email-transactional.routes.test.ts index 41c448152..d1b8b3f76 100644 --- a/backend/src/email/routes/tests/email-transactional.routes.test.ts +++ b/backend/src/email/routes/tests/email-transactional.routes.test.ts @@ -1,4 +1,5 @@ import request from 'supertest' +import { Op } from 'sequelize' import { Sequelize } from 'sequelize-typescript' import { User } from '@core/models' @@ -15,6 +16,7 @@ import { import { EmailFromAddress, EmailMessageTransactional, + EmailMessageTransactionalCc, TransactionalEmailMessageStatus, } from '@email/models' import { @@ -863,6 +865,166 @@ describe(`${emailTransactionalRoute}/send`, () => { ]) }) + const ccValidTests = [ + ['cc-recipient@agency.gov.sg'], + ['cc-recipient@agency.gov.sg', 'cc-recipient-2@agency.gov.sg'], + JSON.stringify(['cc-recipient@agency.gov.sg']), + JSON.stringify([ + 'cc-recipient@agency.gov.sg', + 'cc-recipient-2@agency.gov.sg', + ]), + ] + test.each(ccValidTests)( + 'Should send email with cc from valid array or stringified array - JSON payload', + async (cc) => { + mockSendEmail = jest + .spyOn(EmailService, 'sendEmail') + .mockResolvedValue(true) + const res = await request(app) + .post(endpoint) + .set('Authorization', `Bearer ${apiKey}`) + .send({ + ...validApiCall, + cc, + reply_to: user.email, + }) + + const arrayToCheck = Array.isArray(cc) ? cc : JSON.parse(cc) + + expect(res.status).toBe(201) + expect(res.body.cc.sort()).toStrictEqual(arrayToCheck.sort()) + expect(mockSendEmail).toBeCalledTimes(1) + const transactionalEmail = await EmailMessageTransactional.findOne({ + where: { id: res.body.id }, + }) + expect(transactionalEmail).not.toBeNull() + expect(transactionalEmail).toMatchObject({ + recipient: validApiCall.recipient, + from: validApiCall.from, + status: TransactionalEmailMessageStatus.Accepted, + errorCode: null, + }) + } + ) + + test.each(ccValidTests)( + 'Should send email with cc from valid array or stringified array - form-data', + async (cc) => { + mockSendEmail = jest + .spyOn(EmailService, 'sendEmail') + .mockResolvedValue(true) + // in the case where single cc is sent, stringify the cc list + const ccSend = + Array.isArray(cc) && cc.length === 1 ? JSON.stringify(cc) : cc + + const res = await request(app) + .post(endpoint) + .set('Authorization', `Bearer ${apiKey}`) + .field('recipient', validApiCall.recipient) + .field('subject', validApiCall.subject) + .field('body', validApiCall.body) + .field('from', 'Postman ') + .field('reply_to', validApiCall.reply_to) + .field('cc', ccSend) + + const arrayToCheck = Array.isArray(cc) ? cc : JSON.parse(cc) + + expect(res.status).toBe(201) + expect(res.body.cc.sort()).toStrictEqual(arrayToCheck.sort()) + expect(mockSendEmail).toBeCalledTimes(1) + const transactionalEmail = await EmailMessageTransactional.findOne({ + where: { id: res.body.id }, + include: [ + { + model: EmailMessageTransactionalCc, + attributes: ['email', 'ccType'], + where: { errorCode: { [Op.eq]: null } }, + required: false, + }, + ], + }) + + expect(transactionalEmail).not.toBeNull() + expect(transactionalEmail).toMatchObject({ + recipient: validApiCall.recipient, + from: validApiCall.from, + status: TransactionalEmailMessageStatus.Accepted, + errorCode: null, + }) + const transactionalCcEmails = + transactionalEmail?.emailMessageTransactionalCc.map( + (item) => item.email + ) + expect(transactionalCcEmails?.sort()).toStrictEqual(arrayToCheck.sort()) + } + ) + + const ccInvalidTests = [ + { + cc: 'cc-recipient@agency.gov.sg', + errMsg: + '"cc" failed custom validation because cc must be a valid array or stringified array.', + }, + { + cc: JSON.stringify('cc-recipient@agency.gov.sg'), + errMsg: + '"cc" failed custom validation because cc must be a valid stringified array', + }, + { + cc: JSON.stringify({ key: 'cc', email: 'cc-recipient@agency.gov.sg' }), + errMsg: + '"cc" failed custom validation because cc must be a valid stringified array', + }, + ] + test.each(ccInvalidTests)( + 'Should throw api validation error if cc is not valid array or stringified array - JSON payload', + async ({ cc, errMsg }) => { + mockSendEmail = jest.spyOn(EmailService, 'sendEmail') + + const res = await request(app) + .post(endpoint) + .set('Authorization', `Bearer ${apiKey}`) + .send({ + ...validApiCall, + cc, + reply_to: user.email, + }) + + expect(res.status).toBe(400) + expect(mockSendEmail).not.toBeCalled() + + expect(res.body).toStrictEqual({ + code: 'api_validation', + message: errMsg, + }) + } + ) + + test.each(ccInvalidTests)( + 'Should throw api validation error if cc is not valid array or stringified array - form-data', + async ({ cc, errMsg }) => { + mockSendEmail = jest.spyOn(EmailService, 'sendEmail') + + const res = await request(app) + .post(endpoint) + .set('Authorization', `Bearer ${apiKey}`) + .field('recipient', validApiCall.recipient) + .field('subject', validApiCall.subject) + .field('body', validApiCall.body) + .field('from', 'Postman ') + .field('reply_to', validApiCall.reply_to) + .field('cc', cc) + + expect(res.status).toBe(400) + expect(mockSendEmail).not.toBeCalled() + + expect(res.body).toStrictEqual({ + code: 'api_validation', + message: errMsg, + }) + } + ) + test('Requests should be rate limited and metadata and error code is saved correctly in db', async () => { mockSendEmail = jest .spyOn(EmailService, 'sendEmail')