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

Allow smtps:// in MAIL_URL to enable secureConnection #7043

Merged
merged 2 commits into from
Jun 14, 2016

Conversation

MichaelJCole
Copy link
Contributor

@MichaelJCole MichaelJCole commented May 12, 2016

This backwards compatible patch allows smtps protocol, and secureConnection on port 587. Port 465 is non-standard and assigned to something else. See #7042 for details.

@apollo-cla
Copy link

@MichaelJCole: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@MichaelJCole MichaelJCole changed the title Allow smtps protocol Allow smtps protocol. Use standard port 587. Port 465 was never standard for secure email. May 12, 2016
@MichaelJCole
Copy link
Contributor Author

Please see #7042

@MichaelJCole MichaelJCole reopened this May 12, 2016
@MichaelJCole MichaelJCole changed the title Allow smtps protocol. Use standard port 587. Port 465 was never standard for secure email. Allow smtps protocol. Use standard port 587. Port 465 was never standard for secure smtp. May 12, 2016
@MichaelJCole MichaelJCole changed the title Allow smtps protocol. Use standard port 587. Port 465 was never standard for secure smtp. Allow smtps protocol. Allow STARTTLS on port 587. Port 465 was never standard for secure smtp. May 12, 2016
throw new Error("Email protocol in $MAIL_URL (" +
mailUrlString + ") must be 'smtp'");
mailUrlString + ") must be 'smtp' or 'smtps'");
Copy link
Contributor

Choose a reason for hiding this comment

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

This only changes whether or not an error is thrown when smtps is used. It doesn't change secureConnection below.

@abernix
Copy link
Contributor

abernix commented May 13, 2016

This doesn't appear to change the secureConnection value, something must be missing.

@abernix
Copy link
Contributor

abernix commented May 13, 2016

You had this code in the Issue

{ secureConnection: (port === 465) || (mailUrl.protocol === 'smtps:'),

...but it's not here in the PR. This makes the difference between this change working and not working by allowing the additional trigger for secureConnection, which would definitely be necessary if the provider didn't support 465.

Pretty strange for SparkPost to just not support it though. I'm aware it is not RFC and it's silly, but it is widely accepted by many (Gmail, Hotmail, Mandrill, SendGrid, MailGun, AWS SES, etc.). Looks like Postmarkapp doesn't support it either.

Want to update this PR?

@abernix abernix changed the title Allow smtps protocol. Allow STARTTLS on port 587. Port 465 was never standard for secure smtp. Allow smtps:// in MAIL_URL to enable secureConnection May 13, 2016
@MichaelJCole
Copy link
Contributor Author

MichaelJCole commented May 13, 2016

@abernix Hey Jesse, there are steps in git, and I missed one. Pushed the 2nd commit. Good catch.

Yeah, this PR didn't fix the SparkPost issue. I'm in a hurry and switched to Mailgun instead. the 'simpleSMTP' npm package is depricated. Next steps for Sparkpost issue would be to switch to a new mail transport library, but that's a bigger choice than a drive-by PR :-)

@carinakoo
Copy link

@MichaelJCole, thanks for this leg work. I'm experiencing this as well with Sparkpost. Do I understand correctly then that there's no known fix for Sparkpost? Enabling secureConnection did not solve? Thanks.

@MichaelJCole
Copy link
Contributor Author

MichaelJCole commented May 20, 2016

Hey @carinakoo I was in a hurry and switched to mailgun with no problems yet. You might get a better response from Sparkpost because you're not the first. Give their support a try. secureConnection did not solve it for me.

@zol
Copy link
Contributor

zol commented Jun 14, 2016

Looks great, thanks everyone!

@zol zol merged commit e6da008 into meteor:devel Jun 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants