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

Escape email addresses with name #36454

Merged
merged 1 commit into from Aug 26, 2019

Conversation

@sunny
Copy link
Contributor

commented Jun 10, 2019

Summary

Introduce a safe way to add a name in front of an address when sending emails.

Currently

The guides' advice to do this is to do the formatting yourself:

mail(to: %("#{@user.name}" <#{@user.email}>))

Sadly this breaks if the name is Aaron "Tenderlove" Patterson or -_-" <3. Worse, it can silently let you think the email was sent, when in fact it is later swallowed by an uncomprehensive mail server.

With this change

The formatting can be handled by the email_address_with_name method:

mail(to: email_address_with_name(@user.email, @user.name))

This makes it an opt-in way to make sure that the contents are correctly escaped.

@quadule

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

I've run into this issue before and have been using a similar solution involving Mail::Address.

Even better might be to allow hash arguments mapping addresses to display names for any of the headers that contain addresses. Currently, hash arguments are converted to strings and produce invalid header syntax, so changing this shouldn't break compatibility:

irb(main):001:0> ActionMailer::Base.new.mail(to: { "name@example.com" => "Display Name" }, subject: "", body: "").to_s
=> "Date: Sat, 22 Jun 2019 13:41:12 -0700\r\nTo: {\"name@example.com\"=>\"Display Name\"}\r\nMessage-ID: <2d0c9414c9031_163ccfdcc4c2a1d20d3@example.mail>\r\nMime-Version: 1.0\r\nContent-Type: text/plain;\r\n charset=UTF-8\r\nContent-Transfer-Encoding: 7bit\r\n\r\n"
@sunny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2019

Interesting idea! I'd love thoughts from the core team members on the preferred syntax.

@georgeclaghorn

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

I think I’d prefer a helper method in ActionMailer::Base:

class ActionMailer::Base
  # ...

  private
    def named_address(name, address)
      Mail::Address.new.tap do |builder|
        builder.display_name = name
        builder.address      = address
      end.to_s
    end
end

class PostsMailer < ApplicationMailer
  def text_post(post, subscriber)
    mail to: named_address(subscriber.name, subscriber.email_address) # ...
  end
end
@sunny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

A method sounds good as well! It should also be available as a class method to handle the following:

class PostsMailer < ApplicationMailer
  default from: named_address("Acme & Co.", "acme@example.org")

  #
end

Not sure about the name named_address though.

What about email_with_name(email, name)? Looks more like a railsy helper (number_with_delimiter, …) and informs about the order of arguments.

@simi

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

I like @georgeclaghorn's idea. Is it problem to expose it as classic template view helper as well?

@georgeclaghorn

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

email_address_with_name is fine.

@sunny

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Is it problem to expose it as classic template view helper as well?

I can't think of a common use-case for that for most apps, so I think it might be best to take the conservative approach of keeping it inside ActionMailer::Base.

@sunny sunny force-pushed the sunny:escape_email_addresses_with_name branch 2 times, most recently from 48ac446 to fafbff0 Jul 2, 2019
@sunny

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

@georgeclaghorn Updated using email_address_with_name! 🎉

@louim

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

Hey @georgeclaghorn, could this be merged now that the changes have been applied? 🎉

@georgeclaghorn

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Sorry for neglecting this! We just need a changelog entry and then I can merge.

@sunny sunny force-pushed the sunny:escape_email_addresses_with_name branch from fafbff0 to 0515b0c Aug 26, 2019
@sunny sunny force-pushed the sunny:escape_email_addresses_with_name branch from 0515b0c to 3a8dbe1 Aug 26, 2019
@georgeclaghorn georgeclaghorn merged commit d64010f into rails:master Aug 26, 2019
2 checks passed
2 checks passed
buildkite/rails Build #63302 passed (9 minutes, 8 seconds)
Details
codeclimate All good!
Details
@sunny

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2019

🎉 Thank you for the merge @georgeclaghorn, and to everybody that helped push this forward.

@sunny sunny deleted the sunny:escape_email_addresses_with_name branch Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.