SMTP Connection String Requires URI Encoding

[jamesrenaud]

If you're using a complex password for the SMTP user, it's possible to end up using characters that need to be URI encoded (like /).

Without this, Nodemailer when it attempts to do the DNS hostname resolution of the SMTP server ends up throwing an ENOTFOUND error:

[2021-07-22T19:34:02.320] [WARN] services/email-service.ts - Failed to send getting started email Error: getaddrinfo ENOTFOUND achunkofmypassword
at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:69:26) {
errno: -3008,
code: 'EDNS',
syscall: 'getaddrinfo',
hostname: 'achunkofmypassword',
command: 'CONN'
}

URI Encoding the connection string here https://github.com/Unleash/unleash/blob/master/src/lib/services/email-service.ts#L60 should resolve this issue.

[jamesrenaud]

Please feel free to assign to me, I'll submit a PR fix for this shortly.

[ivarconr]

Thanks 👍🏼 , makes sense as long as Nodemailer accepts a URI encoded string.

[jamesrenaud]

Thanks 👍🏼 , makes sense as long as Nodemailer accepts a URI encoded string.

A good point to check. I verified this with a quick check as follows, just so anyone looking back on history can see what I did:

const connString = escape( 'someuser:my/password/with/url/[email protected]:567',);
const mailer: Transporter = createTransport(`smtp://${connString}`);

This does result in a valid Nodemailer Transporter, which means it properly passes the internal dns.resolve() check that Nodemailer does when you create the transport

From https://nodemailer.com/smtp/

Hostnames for the host field are resolved using dns.resolve().

[ivarconr]

We tested this change in out demo-environment and it has broken the email handling. It turns out escaping the full url will not create a valid url for the mail transporter.

const { escape } = require('querystring');
escape( 'someuser:[email protected]:567');

yields:

'someuser%3password/@email-smtp.us-east-1.amazonaws.com%3A567'

In our logs we now get:

[2021-08-16T08:09:26.718] [WARN] services/email-service.ts - Failed to send reset-password email Error: getaddrinfo ENOTFOUND password
  at GetAddrInfoReqWrap.onlookup [as oncomplete] (node:dns:71:26) {
  errno: -3008,
  code: 'EDNS',
  syscall: 'getaddrinfo',
  hostname: 'password',
  command: 'CONN'
}

We will revert the applied change on master and investigate better approaches.

Possible solutions:

1. Use hostname, user, password, port directly on the getTransporter instead of first converting it to a connection string, like this:

nodemailer.createTransport({
  host: "smtp.example.com",
  port: 587,
  secure: false, // upgrade later with STARTTLS
  auth: {
    user: "username",
    pass: "password",
  },
});

2. Escape individual parts of the connection string.

[jamesrenaud]

Odd this seemed to work alright when I tested this with nodemailer though thinking back I didn't attempt to match versions of the dependency from this project, so it's possible it worked with a newer version - or only worked in my specific connection scenario.

Both resolutions seem valid to me, Option 1 solves the original issue without the added dependency on querystring.

[ivarconr]

closing due to inactivity.