Skip to content

Commit

Permalink
fix: optimize MX server by switching to striptags instead of `html-…
Browse files Browse the repository at this point in the history
…to-text` and improved performance of parseAddresses helper function, discovered bug where ARC broken due to DKIM on MX (will fix)
  • Loading branch information
titanism committed Dec 18, 2024
1 parent 02f3170 commit 31d4c77
Show file tree
Hide file tree
Showing 12 changed files with 58 additions and 37 deletions.
10 changes: 5 additions & 5 deletions app/models/emails.js
Original file line number Diff line number Diff line change
Expand Up @@ -927,8 +927,8 @@ Emails.statics.queue = async function (
// rewrite from header to be without "+" symbol
// so that users can send with "+" address filtering
//
const name = parseUsername(addresses[0].address); // converts to ASCII
const domain = parseHostFromDomainOrAddress(addresses[0].address); // converts to ASCII
const name = parseUsername(addresses[0]); // converts to ASCII
const domain = parseHostFromDomainOrAddress(addresses[0]); // converts to ASCII
from = `${name}@${domain}`; // ASCII formatted From address header
}

Expand All @@ -944,12 +944,12 @@ Emails.statics.queue = async function (
(isEnvelopeFromEmpty && lowercaseKey === 'from')) &&
SENDER_KEYS.has(lowercaseKey)
)
info.envelope.from = addresses[0].address;
info.envelope.from = addresses[0];
// envelope to
else if (isEnvelopeToEmpty && RCPT_TO_KEYS.has(lowercaseKey)) {
for (const address of addresses) {
if (info.envelope.to.includes(address.address)) continue;
info.envelope.to.push(address.address);
if (info.envelope.to.includes(address)) continue;
info.envelope.to.push(address);
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions app/models/logs.js
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,7 @@ Logs.pre('save', function (next) {
// keywords.add(obj.name);
// }

if (isSANB(obj.address) && isEmail(checkSRS(obj.address)))
keywords.add(checkSRS(obj.address));
keywords.add(checkSRS(obj));
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions helpers/get-attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ async function getAttributes(headers, session, resolver, isAligned = false) {

const replyTo = [
// check the Reply-To header
...replyToAddresses.map((addr) => checkSRS(addr.address).toLowerCase()),
...replyToAddresses.map((addr) => checkSRS(addr).toLowerCase()),
// check the Reply-To header domains
...replyToAddresses.map((addr) =>
parseHostFromDomainOrAddress(checkSRS(addr.address))
parseHostFromDomainOrAddress(checkSRS(addr))
),
// check the Reply-To header root domains
...replyToAddresses.map((addr) =>
parseRootDomain(parseHostFromDomainOrAddress(checkSRS(addr.address)))
parseRootDomain(parseHostFromDomainOrAddress(checkSRS(addr)))
)
];

Expand Down
2 changes: 1 addition & 1 deletion helpers/get-forwarding-addresses.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const { decrypt } = require('#helpers/encrypt-decrypt');
const USER_AGENT = `${config.pkg.name}/${config.pkg.version}`;

function parseFilter(address) {
({ address } = parseAddresses(address)[0]);
address = parseAddresses(address)[0];
return address.includes('+') ? address.split('+')[1].split('@')[0] : '';
}

Expand Down
4 changes: 1 addition & 3 deletions helpers/get-from-address.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,7 @@ function getFromAddress(originalFrom) {
);

// set original from address that was parsed
return checkSRS(
punycode.toASCII(originalFromAddresses[0].address)
).toLowerCase();
return checkSRS(punycode.toASCII(originalFromAddresses[0])).toLowerCase();
}

module.exports = getFromAddress;
3 changes: 3 additions & 0 deletions helpers/on-data-mx.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ async function sendSysAdminEmail(template, err, session, headers) {
const cache = await this.client.get(key);
if (boolean(cache)) return;
await this.client.set(key, true, 'PX', ms('30d'));
// TODO: use Emails.queue or something
await emailHelper({
template,
message: {
Expand Down Expand Up @@ -1190,6 +1191,7 @@ async function forward(recipient, headers, session, raw, body) {
const locals = { locale };
if (user) locals.user = user;

// TODO: use Emails.queue or something
await emailHelper({
template: 'self-test',
message: { to: normal },
Expand Down Expand Up @@ -1603,6 +1605,7 @@ async function onDataMX(raw, session, headers, body) {
const cache = await this.client.get(key);
if (cache) return;
await this.client.set(key, true, 'PX', ms('30d'));
// TODO: use Emails.queue or something
await emailHelper({
template: 'phishing',
message: { to, bcc: config.email.message.from },
Expand Down
9 changes: 3 additions & 6 deletions helpers/on-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,15 +156,12 @@ async function onData(stream, _session, fn) {
const originalToAddresses = parseAddresses(getHeaders(headers, 'to'));
for (const obj of originalToAddresses) {
const shouldThrow =
parseRootDomain(parseHostFromDomainOrAddress(obj.address)) ===
env.WEB_HOST;
parseRootDomain(parseHostFromDomainOrAddress(obj)) === env.WEB_HOST;
// rewrite the to line
if (checkSRS(obj.address, shouldThrow) !== obj.address)
if (checkSRS(obj, shouldThrow) !== obj)
headers.update(
'to',
headers
.getFirst('to')
.replaceAll(obj.address, checkSRS(obj.address, shouldThrow))
headers.getFirst('to').replaceAll(obj, checkSRS(obj, shouldThrow))
);
}

Expand Down
36 changes: 30 additions & 6 deletions helpers/parse-addresses.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,30 +3,54 @@
* SPDX-License-Identifier: BUSL-1.1
*/

const _ = require('lodash');
const addressParser = require('nodemailer/lib/addressparser');
const addrs = require('email-addresses');
const isSANB = require('is-string-and-not-blank');

const isEmail = require('#helpers/is-email');

//
// NOTE: if we ever use `email-regex-safe` in future
// we need to ensure `tlds` are considered against punycode toASCII cases
//

// <https://github.com/validatorjs/validator.js/issues/2508>
function parseAddresses(input) {
if (!isSANB(input)) return [];

// <[email protected]>
if (
input.startsWith('<') &&
input.endsWith('>') &&
isEmail(input.slice(1, -1))
)
return [input.slice(1, -1)];

// [email protected]
if (isEmail(input)) return [input];

// more complex stuff here
// `"Adobe Acrobat" <[email protected]>`
// `"[email protected]" <[email protected]>, "[email protected]" <[email protected]>`
let addresses = addrs.parseAddressList({ input, partial: true }) || [];

//
// NOTE: we can't use `email-regex-safe` for values with quotes and such because it returns the wrong values (and dups)
//
// > const emailRegexSafe = require('email-regex-safe')
// > `"[email protected]" <[email protected]>, "[email protected]" <[email protected]>`.match(emailRegexSafe())
// [ '[email protected]', '[email protected]', '[email protected]', '[email protected]' ]
//

if (addresses.length === 0)
addresses = addrs.parseAddressList({ input }) || [];

// safeguard
if (addresses.length === 0) addresses = addressParser(input);

addresses = addresses.filter(
(addr) => _.isObject(addr) && isSANB(addr.address) && isEmail(addr.address)
);

return addresses;
return addresses
.filter((addr) => isEmail(addr?.address))
.map((addr) => addr.address);
}

module.exports = parseAddresses;
9 changes: 1 addition & 8 deletions helpers/parse-host-from-domain-or-address.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const punycode = require('node:punycode');
const { isIP } = require('node:net');

const URLParse = require('url-parse');
const _ = require('lodash');
const isFQDN = require('is-fqdn');
const isSANB = require('is-string-and-not-blank');
const { isURL } = require('@forwardemail/validator');
Expand All @@ -25,13 +24,7 @@ function parseHostFromDomainOrAddress(address) {
domain = url.hostname;
} else {
const parsedAddresses = parseAddresses(address);
if (
_.isArray(parsedAddresses) &&
_.isObject(parsedAddresses[0]) &&
isSANB(parsedAddresses[0].address)
) {
domain = parsedAddresses[0].address;
}
if (parsedAddresses[0]) domain = parsedAddresses[0];
}

const atPos = domain.indexOf('@');
Expand Down
2 changes: 1 addition & 1 deletion helpers/parse-username.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const punycode = require('node:punycode');
const parseAddresses = require('#helpers/parse-addresses');

function parseUsername(address, ignorePlus = false) {
({ address } = parseAddresses(address)[0]);
address = parseAddresses(address)[0];
let username =
!ignorePlus && address.includes('+')
? address.split('+')[0]
Expand Down
8 changes: 6 additions & 2 deletions helpers/refine-and-log-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@

const _ = require('lodash');
const splitLines = require('split-lines');
const { convert } = require('html-to-text');
const striptags = require('striptags');
// const { convert } = require('html-to-text');

const getErrorCode = require('./get-error-code');
const isRetryableError = require('./is-retryable-error');
const isLockingError = require('./is-locking-error');
const isCodeBug = require('./is-code-bug');
const logger = require('./logger');

const env = require('#config/env');
// const env = require('#config/env');

// this is sourced from FE original codebase
function refineAndLogError(err, session, isIMAP = false, instance) {
Expand Down Expand Up @@ -72,6 +73,8 @@ function refineAndLogError(err, session, isIMAP = false, instance) {
// NOTE: this was inspired from `koa-better-error-handler` response for API endpoints
// (and it is used because some errors are translated with HTML tags, e.g. notranslate)
//
err.message = striptags(err.message);
/*
err.message = convert(err.message, {
wordwrap: false,
selectors: [
Expand All @@ -86,6 +89,7 @@ function refineAndLogError(err, session, isIMAP = false, instance) {
],
linkBrackets: false
});
*/

//
// replace linebreaks
Expand Down
3 changes: 3 additions & 0 deletions helpers/send-email.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,8 @@ async function sendEmail(
// signed the message with our DKIM-Signature yet, which we only
// want to do if this was an MX server (since `#helpers/process-email` already applies it for SMTP)
//
// TODO: this breaks ARC
/*
if (!pgp && !email && !domain) {
const signResult = await dkimSign(raw, {
canonicalization: 'relaxed/relaxed',
Expand All @@ -208,6 +210,7 @@ async function sendEmail(
const signatures = Buffer.from(signResult.signatures, 'utf8');
raw = Buffer.concat([signatures, raw], signatures.length + raw.length);
}
*/

//
// if we're in development mode then use preview-email to render queue processing
Expand Down

0 comments on commit 31d4c77

Please sign in to comment.