Permalink
Browse files

Use ERB::Utils to percent encode `hfvalue` parts of mailto

`hfvalue` parts should always be percent encoded, so lets do that!

Revert "use path escaping for email addresses"

This reverts commit 21ffef3.
  • Loading branch information...
tenderlove committed Sep 5, 2015
1 parent f2400de commit f443ae670de9ce9bcb8c00adde08405d2cfd70db
Showing with 5 additions and 5 deletions.
  1. +2 −2 actionview/lib/action_view/helpers/url_helper.rb
  2. +3 −3 actionview/test/template/url_helper_test.rb
@@ -464,11 +464,11 @@ def mail_to(email_address, name = nil, html_options = {}, &block)
extras = %w{ cc bcc body subject reply_to }.map! { |item|
option = html_options.delete(item).presence || next
"#{item.dasherize}=#{Rack::Utils.escape_path(option)}"
"#{item.dasherize}=#{ERB::Util.url_encode(option)}"
}.compact
extras = extras.empty? ? '' : '?' + extras.join('&')
encoded_email_address = Rack::Utils.escape_path(email_address)
encoded_email_address = ERB::Util.url_encode(email_address).gsub("%40", "@")

This comment has been minimized.

Show comment
Hide comment
@bradly

bradly Nov 11, 2015

Contributor

@tenderlove I believe an email address can have characters that ERB::Util.url_encode encodes like #, +, and ?. You can see these chars in the previous test cases not being encoded. I think in the 4.0.x/4.1.x days ERB::Util.html_escape was used which preserved those characters but still escaped < and >, but I'm not sure if that solves all the issues. Thoughts? If you think html_escape makes sense I can start a pull request and give it a shot! Thanks!

@bradly

bradly Nov 11, 2015

Contributor

@tenderlove I believe an email address can have characters that ERB::Util.url_encode encodes like #, +, and ?. You can see these chars in the previous test cases not being encoded. I think in the 4.0.x/4.1.x days ERB::Util.html_escape was used which preserved those characters but still escaped < and >, but I'm not sure if that solves all the issues. Thoughts? If you think html_escape makes sense I can start a pull request and give it a shot! Thanks!

This comment has been minimized.

Show comment
Hide comment
@bradly

bradly Nov 11, 2015

Contributor

Also... there is a sold chance I have no idea what I'm talking about and all the chars should be encoded and the mail clients decode them? Not sure 😞

@bradly

bradly Nov 11, 2015

Contributor

Also... there is a sold chance I have no idea what I'm talking about and all the chars should be encoded and the mail clients decode them? Not sure 😞

html_options["href"] = "mailto:#{encoded_email_address}#{extras}"
content_tag("a".freeze, name || email_address, html_options, &block)
@@ -501,14 +501,14 @@ def test_mail_to
def test_mail_to_with_special_characters
assert_dom_equal(
%(<a href="mailto:%23!$%25&amp;&#39;*+-/=?%5E_%60%7B%7D%7C~@example.org">#!$%&amp;&#39;*+-/=?^_`{}|~@example.org</a>),
%{<a href="mailto:%23%21%24%25%26%27%2A%2B-%2F%3D%3F%5E_%60%7B%7D%7C%7E@example.org">#!$%&amp;&#39;*+-/=?^_`{}|~@example.org</a>},
mail_to("#!$%&'*+-/=?^_`{}|~@example.org")
)
end
def test_mail_with_options
assert_dom_equal(
%{<a href="mailto:me@example.com?cc=ccaddress@example.com&amp;bcc=bccaddress@example.com&amp;body=This%20is%20the%20body%20of%20the%20message.&amp;subject=This%20is%20an%20example%20email&amp;reply-to=foo@bar.com">My email</a>},
%{<a href="mailto:me@example.com?cc=ccaddress%40example.com&amp;bcc=bccaddress%40example.com&amp;body=This%20is%20the%20body%20of%20the%20message.&amp;subject=This%20is%20an%20example%20email&amp;reply-to=foo%40bar.com">My email</a>},
mail_to("me@example.com", "My email", cc: "ccaddress@example.com", bcc: "bccaddress@example.com", subject: "This is an example email", body: "This is the body of the message.", reply_to: "foo@bar.com")
)
@@ -533,7 +533,7 @@ def test_mail_to_with_block
end
def test_mail_to_with_block_and_options
assert_dom_equal %{<a class="special" href="mailto:me@example.com?cc=ccaddress@example.com"><span>Email me</span></a>},
assert_dom_equal %{<a class="special" href="mailto:me@example.com?cc=ccaddress%40example.com"><span>Email me</span></a>},
mail_to('me@example.com', cc: "ccaddress@example.com", class: "special") { content_tag(:span, 'Email me') }
end

1 comment on commit f443ae6

@jjb

This comment has been minimized.

Show comment
Hide comment
@jjb

jjb Apr 12, 2016

Contributor

FYI this causes problems with ruby 1.9 and 2.0 https://bugs.ruby-lang.org/issues/8612

(I don't think that's cause for action at this point, but just leaving that here in case it helps someone else)

Contributor

jjb commented on f443ae6 Apr 12, 2016

FYI this causes problems with ruby 1.9 and 2.0 https://bugs.ruby-lang.org/issues/8612

(I don't think that's cause for action at this point, but just leaving that here in case it helps someone else)

Please sign in to comment.