From c08f6cf5632d99182fb41a156952c8b6566a9891 Mon Sep 17 00:00:00 2001 From: Kevin Date: Thu, 21 Nov 2024 22:13:21 +0800 Subject: [PATCH 1/4] fix cc and bcc to allow stringified array --- .../routes/email-transactional.routes.ts | 51 +++++++++++++++---- 1 file changed, 41 insertions(+), 10 deletions(-) diff --git a/backend/src/email/routes/email-transactional.routes.ts b/backend/src/email/routes/email-transactional.routes.ts index 34e60cf35..49c2b67d7 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 emailArrayValidation = (fieldName: 'cc' | 'bcc') => { + return Joi.alternatives().try( + // array + Joi.array() + .unique() + .items( + Joi.string().trim().email().options({ convert: true }).lowercase() + ), + + // 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( + Joi.string().email().trim().lowercase().options({ convert: true }) + ) + .validate(parsed) + + if (error) { + throw new Error(`${fieldName} ${error.message}`) + } + + return validatedEmails + }) + ) + } + const sendValidator = { [Segments.BODY]: Joi.object({ recipient: Joi.string() @@ -54,16 +93,8 @@ export const InitEmailTransactionalRoute = ( .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), }), } From 576578f68db51a47bb5b9ec359617744cef01703 Mon Sep 17 00:00:00 2001 From: Kevin Date: Thu, 21 Nov 2024 22:15:45 +0800 Subject: [PATCH 2/4] add tests for cc --- .../tests/email-transactional.routes.test.ts | 157 ++++++++++++++++++ 1 file changed, 157 insertions(+) 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..81dc2709c 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,161 @@ describe(`${emailTransactionalRoute}/send`, () => { ]) }) + const ccValidTests = [ + ['cc-recipient@agency.gov.sg', 'cc-recipient-2@agency.gov.sg'], + JSON.stringify([ + 'cc-recipient@agency.gov.sg', + 'cc-recipient-2@agency.gov.sg', + ]), + ] + test.each(ccValidTests)( + 'Should throw api validation error if cc is not 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 throw api validation error if cc is not valid array or stringified array - form-data', + async (cc) => { + mockSendEmail = jest + .spyOn(EmailService, 'sendEmail') + .mockResolvedValue(true) + 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) + + 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 - JSON payload', + async ({ cc, errMsg }) => { + mockSendEmail = jest.spyOn(EmailService, 'sendEmail') + + const res = await request(app) + .post(endpoint) + .set('Authorization', `Bearer ${apiKey}`) + .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') From 5d1626aeb190d6ec4499842a60f1a7a57c943100 Mon Sep 17 00:00:00 2001 From: Kevin Date: Fri, 22 Nov 2024 12:04:56 +0800 Subject: [PATCH 3/4] abstract email validator, update test cases --- .../routes/email-transactional.routes.ts | 22 ++++++-------- .../tests/email-transactional.routes.test.ts | 29 ++++++++----------- 2 files changed, 21 insertions(+), 30 deletions(-) diff --git a/backend/src/email/routes/email-transactional.routes.ts b/backend/src/email/routes/email-transactional.routes.ts index 49c2b67d7..b5ba562f5 100644 --- a/backend/src/email/routes/email-transactional.routes.ts +++ b/backend/src/email/routes/email-transactional.routes.ts @@ -21,14 +21,16 @@ 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( - Joi.string().trim().email().options({ convert: true }).lowercase() - ), + Joi.array().unique().items(emailValidator), // stringified array (form-data) Joi.string().custom((value: string) => { @@ -46,9 +48,7 @@ export const InitEmailTransactionalRoute = ( } const { value: validatedEmails, error } = Joi.array() .unique() - .items( - Joi.string().email().trim().lowercase().options({ convert: true }) - ) + .items(emailValidator) .validate(parsed) if (error) { @@ -82,11 +82,7 @@ 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() 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 81dc2709c..b02a717d3 100644 --- a/backend/src/email/routes/tests/email-transactional.routes.test.ts +++ b/backend/src/email/routes/tests/email-transactional.routes.test.ts @@ -866,14 +866,11 @@ describe(`${emailTransactionalRoute}/send`, () => { }) const ccValidTests = [ - ['cc-recipient@agency.gov.sg', 'cc-recipient-2@agency.gov.sg'], - JSON.stringify([ - 'cc-recipient@agency.gov.sg', - 'cc-recipient-2@agency.gov.sg', - ]), + [['cc-recipient@agency.gov.sg']], + [['cc-recipient@agency.gov.sg', 'cc-recipient-2@agency.gov.sg']], ] test.each(ccValidTests)( - 'Should throw api validation error if cc is not valid array or stringified array - JSON payload', + 'Should send email with cc from valid array or stringified array - JSON payload', async (cc) => { mockSendEmail = jest .spyOn(EmailService, 'sendEmail') @@ -887,10 +884,8 @@ describe(`${emailTransactionalRoute}/send`, () => { 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(res.body.cc.sort()).toStrictEqual(cc.sort()) expect(mockSendEmail).toBeCalledTimes(1) const transactionalEmail = await EmailMessageTransactional.findOne({ where: { id: res.body.id }, @@ -906,11 +901,14 @@ describe(`${emailTransactionalRoute}/send`, () => { ) test.each(ccValidTests)( - 'Should throw api validation error if cc is not valid array or stringified array - form-data', + '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 = cc.length === 1 ? JSON.stringify(cc) : cc + const res = await request(app) .post(endpoint) .set('Authorization', `Bearer ${apiKey}`) @@ -919,12 +917,10 @@ describe(`${emailTransactionalRoute}/send`, () => { .field('body', validApiCall.body) .field('from', 'Postman ') .field('reply_to', validApiCall.reply_to) - .field('cc', cc) - - const arrayToCheck = Array.isArray(cc) ? cc : JSON.parse(cc) + .field('cc', ccSend) expect(res.status).toBe(201) - expect(res.body.cc.sort()).toStrictEqual(arrayToCheck.sort()) + expect(res.body.cc.sort()).toStrictEqual(cc.sort()) expect(mockSendEmail).toBeCalledTimes(1) const transactionalEmail = await EmailMessageTransactional.findOne({ where: { id: res.body.id }, @@ -949,7 +945,7 @@ describe(`${emailTransactionalRoute}/send`, () => { transactionalEmail?.emailMessageTransactionalCc.map( (item) => item.email ) - expect(transactionalCcEmails?.sort()).toStrictEqual(arrayToCheck.sort()) + expect(transactionalCcEmails?.sort()).toStrictEqual(cc.sort()) } ) @@ -995,14 +991,13 @@ describe(`${emailTransactionalRoute}/send`, () => { ) test.each(ccInvalidTests)( - 'Should throw api validation error if cc is not valid array or stringified array - JSON payload', + '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}`) - .set('Authorization', `Bearer ${apiKey}`) .field('recipient', validApiCall.recipient) .field('subject', validApiCall.subject) .field('body', validApiCall.body) From b109a344c8c2e9c0314c24fbfb18280ce31a42f7 Mon Sep 17 00:00:00 2001 From: Kevin Date: Fri, 22 Nov 2024 13:48:04 +0800 Subject: [PATCH 4/4] revert to 2d array for test inputs --- .../tests/email-transactional.routes.test.ts | 22 ++++++++++++++----- 1 file changed, 16 insertions(+), 6 deletions(-) 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 b02a717d3..d1b8b3f76 100644 --- a/backend/src/email/routes/tests/email-transactional.routes.test.ts +++ b/backend/src/email/routes/tests/email-transactional.routes.test.ts @@ -866,8 +866,13 @@ describe(`${emailTransactionalRoute}/send`, () => { }) const ccValidTests = [ - [['cc-recipient@agency.gov.sg']], - [['cc-recipient@agency.gov.sg', 'cc-recipient-2@agency.gov.sg']], + ['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', @@ -884,8 +889,10 @@ describe(`${emailTransactionalRoute}/send`, () => { reply_to: user.email, }) + const arrayToCheck = Array.isArray(cc) ? cc : JSON.parse(cc) + expect(res.status).toBe(201) - expect(res.body.cc.sort()).toStrictEqual(cc.sort()) + expect(res.body.cc.sort()).toStrictEqual(arrayToCheck.sort()) expect(mockSendEmail).toBeCalledTimes(1) const transactionalEmail = await EmailMessageTransactional.findOne({ where: { id: res.body.id }, @@ -907,7 +914,8 @@ describe(`${emailTransactionalRoute}/send`, () => { .spyOn(EmailService, 'sendEmail') .mockResolvedValue(true) // in the case where single cc is sent, stringify the cc list - const ccSend = cc.length === 1 ? JSON.stringify(cc) : cc + const ccSend = + Array.isArray(cc) && cc.length === 1 ? JSON.stringify(cc) : cc const res = await request(app) .post(endpoint) @@ -919,8 +927,10 @@ describe(`${emailTransactionalRoute}/send`, () => { .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(cc.sort()) + expect(res.body.cc.sort()).toStrictEqual(arrayToCheck.sort()) expect(mockSendEmail).toBeCalledTimes(1) const transactionalEmail = await EmailMessageTransactional.findOne({ where: { id: res.body.id }, @@ -945,7 +955,7 @@ describe(`${emailTransactionalRoute}/send`, () => { transactionalEmail?.emailMessageTransactionalCc.map( (item) => item.email ) - expect(transactionalCcEmails?.sort()).toStrictEqual(cc.sort()) + expect(transactionalCcEmails?.sort()).toStrictEqual(arrayToCheck.sort()) } )