Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(email): Fix SendGrid fallback integration #1026

Merged
merged 17 commits into from
Apr 19, 2021
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions backend/src/core/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,14 @@ const config = convict({
env: 'SMS_FALLBACK_SENDER_ID',
},
},
emailFallback: {
activate: {
doc:
'Switch to true to use the SendGrid fallback for emails. Ensure that the SMTP settings are properly configured as well.',
default: false,
env: 'EMAIL_FALLBACK_ACTIVATE',
},
},
})

// If mailFrom was not set in an env var, set it using the app_name
Expand Down
7 changes: 7 additions & 0 deletions backend/src/core/loaders/express.loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,13 @@ const expressApp = ({ app }: { app: express.Application }): void => {
app.use(requestTracerMiddleware)

app.use(overrideContentTypeHeaderMiddleware)

// We need to parse the body as text specifically for SendGrid callbacks.
// This is to avoid stripping whitespace characters and messing with unicode encoding.
// This route affects SES callbacks as well, so we'll need to parse the text as JSON
// in the parseEvent() handle before parsing the SES event.
app.use('/v1/callback/email', bodyParser.text({ type: 'application/json' }))
Copy link
Contributor Author

@zwliew zwliew Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SendGrid events contain carriage return and newline characters (\r\n) and some even have unicode-encoded characters. The JSON body-parser that we currently use strips the newline characters and decodes the unicode characters, which causes the ECDSA verification to fail.

I had a less intrusive fix here: 201d0f4
But, I think it's not great for reasons stated in the revert: 5473c72

I ended up with this solution since it isn't super intrusive, but it might affect SES callbacks. I doubt it would in practice though.

One other solution which doesn't affect SES callbacks is to split the SES and SendGrid callback endpoints, and only apply the text body-parser for SendGrid, but that would be sort of a breaking change. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. I've checked and only one body-parser middleware will run each time.


app.use(bodyParser.json())
app.use(bodyParser.urlencoded({ extended: false }))
// ref: https://expressjs.com/en/resources/middleware/cors.html#configuration-options
Expand Down
6 changes: 5 additions & 1 deletion backend/src/core/services/mail-client.class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import nodemailer from 'nodemailer'
import directTransport from 'nodemailer-direct-transport'
import { loggerWithLabel } from '@core/logger'
import { MailToSend, MailCredentials } from '@core/interfaces'
import config from '@core/config'

const REFERENCE_ID_HEADER = 'X-SMTPAPI' // Case sensitive
const logger = loggerWithLabel(module)

Expand Down Expand Up @@ -38,7 +40,9 @@ export default class MailClient {
public sendMail(input: MailToSend): Promise<string | void> {
return new Promise<string | void>((resolve, reject) => {
const options = {
from: input.from,
from: config.get('emailFallback.activate')
? config.get('mailFrom')
: input.from,
to: input.recipients,
subject: input.subject,
replyTo: input.replyTo,
Expand Down
52 changes: 42 additions & 10 deletions backend/src/email/middlewares/email.middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,35 @@ const previewFirstMessage = async (
}
}

/**
* Checks if the from address is custom and rejects it if necessary.
*/
const isCustomFromAddressAllowed = (
req: Request,
res: Response,
next: NextFunction
): void => {
const { from } = req.body
const defaultEmail = config.get('mailFrom')

if (from === defaultEmail) {
next()
return
}

// We don't allow custom from address for the SendGrid fallback
// since they aren't DKIM-authenticated currently
if (config.get('emailFallback.activate')) {
res.status(503).json({
message:
'Unable to use a custom from address due to downtime. Please use the default from address instead.',
})
return
}

next()
}

/**
* Checks that the from address is either the user's email or the default app email
*/
Expand Down Expand Up @@ -241,18 +270,20 @@ const getCustomFromAddress = async (
res: Response,
next: NextFunction
): Promise<Response | void> => {
const email =
req.session?.user?.email ||
(await AuthService.findUser(req.session?.user?.id))?.email
const defaultEmail = config.get('mailFrom')
const result = []
const result = [defaultEmail]

try {
const fromAddress = await CustomDomainService.getCustomFromAddress(email)
if (fromAddress) result.push(fromAddress)
result.push(defaultEmail)
} catch (err) {
return next(err)
if (!config.get('emailFallback.activate')) {
lamkeewei marked this conversation as resolved.
Show resolved Hide resolved
const email =
req.session?.user?.email ||
(await AuthService.findUser(req.session?.user?.id))?.email
try {
const fromAddress = await CustomDomainService.getCustomFromAddress(email)
// Display the custom address first
if (fromAddress) result.unshift(fromAddress)
} catch (err) {
return next(err)
}
}

return res.status(200).json({ from: result })
Expand Down Expand Up @@ -333,6 +364,7 @@ export const EmailMiddleware = {
storeFromAddress,
getCustomFromAddress,
existsFromAddress,
isCustomFromAddressAllowed,
isFromAddressAccepted,
sendValidationMessage,
duplicateCampaign,
Expand Down
3 changes: 3 additions & 0 deletions backend/src/email/routes/email-campaign.routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,14 @@ router.get('/', EmailMiddleware.getCampaignDetails)
* description: Forbidden as there is a job in progress
* "500":
* description: Internal Server Error
* "503":
* description: Service Unavailable. Try using the default from address instead.
*/
router.put(
'/template',
celebrate(storeTemplateValidator),
CampaignMiddleware.canEditCampaign,
EmailMiddleware.isCustomFromAddressAllowed,
EmailMiddleware.isFromAddressAccepted,
EmailMiddleware.existsFromAddress,
EmailMiddleware.verifyFromAddress,
Expand Down
3 changes: 3 additions & 0 deletions backend/src/email/routes/email-settings.routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,14 @@ const verifyValidator = {
* description: OK
* 400:
* description: Bad Request (verification fails)
* 503:
* description: Service Unavailable. Try using the default from address instead.
*
*/
router.post(
'/from/verify',
celebrate(verifyValidator),
EmailMiddleware.isCustomFromAddressAllowed,
EmailMiddleware.isFromAddressAccepted,
EmailMiddleware.verifyFromAddress,
EmailMiddleware.sendValidationMessage,
Expand Down
5 changes: 3 additions & 2 deletions backend/src/email/services/email-callback.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,16 @@ const isAuthenticated = (authHeader?: string): boolean => {
}

const parseEvent = async (req: Request): Promise<void> => {
const parsed = JSON.parse(req.body)
let records: Promise<void>[] = []
if (ses.isEvent(req)) {
// body could be one record or an array of records, hence we concat
const body: ses.SesRecord[] = []
const sesHttpEvent = body.concat(req.body)
const sesHttpEvent = body.concat(parsed)
records = sesHttpEvent.map(ses.parseRecord)
} else if (sendgrid.isEvent(req)) {
// body is always an array
const sgEvent = req.body
const sgEvent = parsed
records = sgEvent.map(sendgrid.parseRecord)
} else {
throw new Error('Unable to handle this event')
Expand Down
15 changes: 8 additions & 7 deletions docs/configure/backend.md
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,14 @@ Further reference: [Express-session documentation](https://www.npmjs.com/package

If not set, `nodemailer-direct-transport` will be used (for testing locally)

| Name | Description |
| ---------- | ------------------------------------------------------------ |
| `SES_HOST` | Amazon SES SMTP endpoint. |
| `SES_PORT` | Amazon SES SMTP port, defaults to 465 |
| `SES_USER` | SMTP username |
| `SES_PASS` | SMTP password |
| `SES_FROM` | The email address that appears in the From field of an email |
| Name | Description |
| ------------------------- | -------------------------------------------------------------------------------------------------------------------------- |
| `BACKEND_SES_HOST` | Amazon SES SMTP endpoint. |
| `BACKEND_SES_PORT` | Amazon SES SMTP port, defaults to 465 |
| `BACKEND_SES_USER` | SMTP username |
| `BACKEND_SES_PASS` | SMTP password |
| `BACKEND_SES_FROM` | The email address that appears in the From field of an email |
| `EMAIL_FALLBACK_ACTIVATE` | Switch to true to use the SendGrid fallback for all emails. Ensure that the SMTP settings are properly configured as well. |

Further reference: [AWS SES documentation](https://docs.aws.amazon.com/ses/latest/DeveloperGuide/smtp-credentials.html)

Expand Down
15 changes: 8 additions & 7 deletions docs/configure/worker.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,14 @@ For testing locally, you may need to configure your `AWS_ACCESS_KEY_ID` and `AWS

If not set, `nodemailer-direct-transport` will be used (for testing locally)

| Name | Description |
| ---------- | ------------------------------------------------------------ |
| `SES_HOST` | Amazon SES SMTP endpoint. |
| `SES_PORT` | Amazon SES SMTP port, defaults to 465 |
| `SES_USER` | SMTP username |
| `SES_PASS` | SMTP password |
| `SES_FROM` | The email address that appears in the From field of an email |
| Name | Description |
| ------------------------- | -------------------------------------------------------------------------------------------------------------------------- |
| `WORKER_SES_HOST` | Amazon SES SMTP endpoint. |
| `WORKER_SES_PORT` | Amazon SES SMTP port, defaults to 465 |
| `WORKER_SES_USER` | SMTP username |
| `WORKER_SES_PASS` | SMTP password |
| `WORKER_SES_FROM` | The email address that appears in the From field of an email |
| `EMAIL_FALLBACK_ACTIVATE` | Switch to true to use the SendGrid fallback for all emails. Ensure that the SMTP settings are properly configured as well. |

#### Sending smses

Expand Down
8 changes: 8 additions & 0 deletions worker/src/core/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,14 @@ const config = convict({
env: 'SMS_FALLBACK_SENDER_ID',
},
},
emailFallback: {
activate: {
doc:
'Switch to true to use the SendGrid fallback for emails. Ensure that the SMTP settings are properly configured as well.',
default: false,
env: 'EMAIL_FALLBACK_ACTIVATE',
},
},
})

// If mailFrom was not set in an env var, set it using the app_name
Expand Down
5 changes: 4 additions & 1 deletion worker/src/email/services/mail-client.class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import nodemailer from 'nodemailer'
import directTransport from 'nodemailer-direct-transport'
import { loggerWithLabel } from '@core/logger'
import { MailToSend, MailCredentials } from '@email/interfaces'
import config from '@core/config'

const logger = loggerWithLabel(module)
const REFERENCE_ID_HEADER = 'X-SMTPAPI' // Case sensitive
Expand Down Expand Up @@ -35,7 +36,9 @@ export default class MailClient {
public sendMail(input: MailToSend): Promise<string | void> {
return new Promise<string | void>((resolve, reject) => {
const options = {
from: input.from,
from: config.get('emailFallback.activate')
? config.get('mailFrom')
: input.from,
to: input.recipients,
subject: input.subject,
replyTo: input.replyTo,
Expand Down