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

Recommend mandatory STARTTLS for Google #43594

Merged
merged 3 commits into from Sep 9, 2022

Conversation

c960657
Copy link
Contributor

@c960657 c960657 commented Nov 4, 2021

Summary

The Action Mailer guide recommends using opportunistic TLS (enable_starttls_auto: true) for connecting to smtp.google.com.

This setting is vulnerable to man-in-the-middle attacks. Google definitely supports STARTTLS, so this should be required using enable_starttls: true.

@rails-bot rails-bot bot added the docs label Nov 4, 2021
user_name: '<username>',
password: '<password>',
authentication: 'plain',
enable_starttls: true,
Copy link
Member

Choose a reason for hiding this comment

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

This option isn't documented here

# * <tt>:enable_starttls_auto</tt> - Detects if STARTTLS is enabled in your SMTP server and starts

Should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realised that enable_starttls was added in mail version 2.7.0. Surprisingly, the auto-feature was added first.

Action Mailer currently requires version 2.5.4.

I don't think there is any reason to bump the requirement just because of the guide, but the new option definitely deserves to be mentioned. I'll fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arg, I made this fix a long time ago but I somehow failed to push my branch. In the meantime this option has been documented in #44096.

Copy link
Member

Choose a reason for hiding this comment

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

So if you are using an older version of mail and set this option, will that be fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a note about this.

@rails-bot
Copy link

rails-bot bot commented Feb 2, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@ghiculescu ghiculescu added the ready PRs ready to merge label Feb 2, 2022
@rails-bot
Copy link

rails-bot bot commented May 3, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label May 3, 2022
@rails-bot rails-bot bot removed the stale label May 5, 2022
@gregmolnar
Copy link
Member

@ghiculescu Is this one still good to be merged?

@ghiculescu
Copy link
Member

I think so, but it needs to be reviewed by someone from the core or committer teams. They will get to it, it can jus take a little while. Thank you for keeping it open.

@rafaelfranca rafaelfranca merged commit 89fdd5e into rails:main Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants