Skip to content
This repository

ActionMailer is html escaping ampersand (&) in Urls in plain text messages #687

Closed
lighthouse-import opened this Issue May 15, 2011 · 47 comments
Lighthouse Import

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/4858
Created by Rodrigo Rosenfeld Rosas - 2010-11-08 23:12:12 UTC

A view views/mailer/message.text.erb with this content:

http://ab.cd/e?f=1&g=2 (read an ampersand only, cause LightHouse will mess with it)

is sent as a plain text message with this link:

http://ab.cd/e?f=1&g=2

Lighthouse Import

Imported from Lighthouse.
Comment by Michael Koziarski - 2010-06-14 20:17:12 UTC

This is probably as a result of unifying the AM and AC use of AV, it's a regression and should be fixed before 3.0

Lighthouse Import

Imported from Lighthouse.
Comment by Amos King - 2010-06-15 02:41:25 UTC

I can't reproduce this issue with the latest. Can I get an example so I can tackle this.

Lighthouse Import

Imported from Lighthouse.
Comment by Rodrigo Rosenfeld Rosas - 2010-06-15 02:57:26 UTC

I've just found the root cause. url_for is not marking the url as html_safe and the ERB template used by the plain text templating is sanitizing the text by default as the other ERB templates.

This mean that now the plain text ERB template should be something like:

Please, visit <%= raw @confirmation_url %>

I think plain text templates should not be sanitized in favor of the least surprise principle...

What do you think?

Lighthouse Import

Imported from Lighthouse.
Comment by Akira Matsuda - 2010-08-23 07:00:17 UTC

I also got stuck on this problem.

I think plain text templates should not be sanitized in favor of the least surprise principle...
What do you think?

+1. Plain text template should better not be html_escaped.

Lighthouse Import

Imported from Lighthouse.
Comment by Akira Matsuda - 2010-08-23 07:01:13 UTC

Sorry, I meant, +1

Lighthouse Import

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-08-30 03:10:33 UTC

[bulk edit]

Lighthouse Import

Imported from Lighthouse.
Comment by José Valim - 2010-09-01 22:11:52 UTC

This is not related to the mail gem, but the ActionView bit. Assigning it back to Mr. Katz.

Lighthouse Import

Imported from Lighthouse.
Comment by Wincent Colaiuta - 2010-09-04 13:07:37 UTC

I've had to use raw in a few places in my non-HTML templates for this reason.

It's not limited to templates with the .text.erb extension. I've also had to do it in .js.erb templates. I imagine that if I had other non-HTML ERB templates it would behave the same way too.

No idea if this is limited to ERB, seeing as the only other template format I use is Haml, which is obviously always producing HTML output only.

Lighthouse Import

Imported from Lighthouse.
Comment by Jeremy Kemper - 2010-10-15 22:01:56 UTC

[bulk edit]

Lighthouse Import

Imported from Lighthouse.
Comment by Ryan Bigg - 2010-10-19 07:34:58 UTC

Automatic cleanup of spam.

Lighthouse Import

Imported from Lighthouse.
Comment by Fjan - 2010-11-03 19:08:39 UTC

I just wanted to point out that the it's not just URLs and not just ampersands: any text gets HTML escaped. This is going to trip people up who accidentally use < and > characters in an e-mail address for example. My plain text e-mail templates are now littered with .html_safe everywhere to prevent this.

As far as I can tell from the code this is going to be pretty hard to fix without ruining the nice unification of the mailer views with the regular views we just had. My suggestion is to define a new erb character sequence such as <%== to mean a "raw concat". That will beatify the code a little and make sure that we can upgrade old templates with a simple search/replace, mailer or regular view.

Lighthouse Import

Imported from Lighthouse.
Comment by Fjan - 2010-11-03 20:51:04 UTC

Just a quick follow up to myself, I looked at the code and all that is needed to make "<%== ..." behave as "<%= raw ..." is this snippet in an initializer:

module ActionView
  class Template
    module Handlers
      class Erubis < ::Erubis::Eruby
        def add_expr_escaped(src, code)
          src << "@output_buffer.safe_concat(" << escape_text(code.html_safe) << ");"
        end
      end
    end
  end
end

It can be made more efficient by adding to the string directly.

Lighthouse Import

Imported from Lighthouse.
Comment by Wincent Colaiuta - 2010-11-03 20:57:14 UTC

I think adding new syntax is a non-solution to this problem which just wall-papers over the underlying issue.

The fact is that it doesn't make sense to HTML-escape content which is not HTML.

Merely adding a short-cut syntax for avoiding the undesired escaping is just giving us a more concise means of doing something that we shouldn't have to do in the first place, and IMO it would be worse than just ignoring the problem.

Lighthouse Import

Imported from Lighthouse.
Comment by Fjan - 2010-11-03 21:07:29 UTC

I agree a proper fix where plain text templates do net get escaped would be better.

However, I still like that new syntax for the regular views. I just did a quick count for "html_safe" and I came up with 390 in my views and helpers. So it would sure save a lot of typing and it can also make the templates more efficient by not converting to an output_buffer and back to a string (as I did above).

I simply like to use some like without all the html_safe stuff:
<%= "Alert".html_safe if level

Lighthouse Import

Imported from Lighthouse.
Comment by Fjan - 2010-11-03 23:16:42 UTC

Oops, that method in my first posts should have read:

def add_expr_escaped(src, code)
  src << "@output_buffer.safe_concat((" << code << ").to_s);"
end

Also apologies for the lack of proofreading in the second post. I'm actually tempted to override the XSS escaping when upgrading from Rails 2 projects by doing a global search/replace of <%= with <%== now. The on-by-default escaping idea is sound in theory but it's just introducing more problems than it solves when upgrading an old and stable code base.

Lighthouse Import

Imported from Lighthouse.
Comment by Wincent Colaiuta - 2010-11-03 23:21:41 UTC

I agree a proper fix where plain text templates do net get escaped would be better.

However, I still like that new syntax for the regular views.

Fjan, your proposed shortcut syntax probably belongs in a separate ticket.

Lighthouse Import

Imported from Lighthouse.
Comment by Fjan - 2010-11-03 23:24:47 UTC

You are right. Sorry for the spam here, not thinking to clearly after typing 390 html_safes. I'll put in a new ticket tomorrow.

Lighthouse Import

Imported from Lighthouse.
Comment by Bertg - 2010-11-05 19:53:12 UTC

We have been investigating this issue, and it goes beyond ActionMailer.

We have detected this happening when rendering csv files, ical compatible files etc.. Basically html escaping is happening all the time in ActionView regardless of format.

Templates and Handlers always choose a ActionView::SafeBuffer or a subclass of it. This is desired behaviour in case of html (or alike, e.g.: xml) but not in any other context like plain text emails or csv.

The template should be smart enough to choose the correct buffer based on the format passed to it. E.g.: when the extension of a template is .text.haml it should not be doing any escaping, i.e. not be using the safe buffer.

We do think the priority of this bug should be high, as it's influencing all formats besides html, and thus breaking these.

The impact is probably going to be beyond rails, but in any rails3 compatible handler, like haml.

Lighthouse Import

Imported from Lighthouse.
Comment by Fjan - 2010-11-11 15:25:22 UTC

Now that my patch to add a non-escaping option to Erubis has been committed to 3.0 stable this problem can be fixed by simply letting the handler check if the template identifier contains "html" and disabling escape otherwise. Attached is a monkey patch that adds one line to the template setup:

   :escape => template.identifier !~ /\.html/   # Note: :escape => true disables escaping

Note that this patch will only work if the patch mentioned above is also applied.

Lighthouse Import

Imported from Lighthouse.
Comment by Rodrigo Rosenfeld Rosas - 2010-11-11 15:52:31 UTC

Worth noting that this monkey patch will only work on ERB templates but won't work for HAML for instance...

Lighthouse Import

Imported from Lighthouse.
Comment by Fjan - 2010-11-11 16:40:19 UTC

Here is the one-line patch that can be applied to edge. Sorry, I have no idea how to add a test for this, I'm kind of new to all this.

Lighthouse Import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2010-11-15 21:04:51 UTC

[bulk edit]

Lighthouse Import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2011-02-12 21:58:21 UTC

[bulk edit]

Lighthouse Import

Imported from Lighthouse.
Comment by Santiago Pastorino - 2011-02-27 03:15:38 UTC

[bulk edit]

Lighthouse Import

Imported from Lighthouse.
Comment by Matthew Stopa - 2011-04-28 19:38:49 UTC

Does anyone know if this issue has been fixed yet? It looks like things have changed a lot in Rails3 since the patch was submitted. We're experiencing the problem currently where our users are getting their emails with escaped characters. Anyone know a workaround?

Lighthouse Import

Imported from Lighthouse.
Comment by Bertg - 2011-04-28 20:19:25 UTC

I have no idea what the status on this ticket is, but the work around we used is to indicate EACH string as html safe manually. That works great :)

Lighthouse Import

Imported from Lighthouse.
Comment by Matthew Stopa - 2011-04-28 21:08:42 UTC

That hasn't worked for me at all. Even with a basic ActionMailer where I specify the body as body => "blah blah".html_safe I am still seeing escaped characters in the emails it sends.

Lighthouse Import

Imported from Lighthouse.
Comment by Fjan - 2011-04-29 07:15:57 UTC

@Matthew: I've been using the monkey patch I provided in the November 11th post in production for quite some time now and it works great. Just drop it in the config/initializers directory.

Lighthouse Import

Attachments saved to Gist: http://gist.github.com/971675

José Valim josevalim closed this May 15, 2011
Joshua Isaac

Not sure why this is closed... this still doesn't look like it's made it into 3.1. Did this get rejected?

Jean-Daniel Guyot
jdguyot commented May 27, 2011

Same question here. Should we continue to monkey patch Rails for this? What is the recommended way?

Mike Cavaliere

+1, any progress on this?

Alexander Lang

+1, it doesn't look like the problem has been solved

Rasmus Rønn Nielsen

This problem is not fixed. I'm still seeing &amp; in my text emails.

+1

Andy Allan

Please, either re-open this ticked or explain why it has been rejected!

Alexander Lang

This is still not fixed in 3.2.1. I've written an updated initializer: https://gist.github.com/1923080 that patches ActionView

Ryan McGeary
rmm5t commented March 09, 2012

I still see HTML entities in plain text emails in v3.2.2. Is there interest in accepting a pull request to resolve this? I'd be willing to look into the core fix.

Kurt Preston

Seeing this in Rails v3.0.12.

Tim Jones

Still broken in 3.2.3

H. Arnold Jones

Using <%== foo %> seems to work for me. It feels a little hacky, but if you're going to use a separate .html.erb for HTML mails anyway, remembering to use the double-"=" syntax for plaintext isn't as horrible as it otherwise could be.

+1 for a "proper" fix, but this workaround does it for me with no patching required (YMMV).

Currently using rails 3.2.2.

Aaron Patterson tenderlove reopened this June 27, 2012
Aaron Patterson
Owner

Would it be possible for someone to provide a failing test case?

Steve Klabnik
Collaborator

Dipping my toes into this, let's see how it goes. I did a little work with template rendering a few weeks ago.

is @langalex's patch a good idea? Are there any cases in which a non-HTML template should get escaped?

Steve Klabnik
Collaborator

Based on the patch, I tried to write some tests:

steveklabnik@ffd4892

However, the test_basic_html_template_does_html_escape doesn't escape the & either way; I can pass true or false in and I still get a &.

Hmmmmm.

Sick issue now present in Redmine.

Doug Smith

+1. It would be great to have this fixed in Rails soon.

Yves Senn
Collaborator

There is a pending PR to fix this issue #7976

Rafael Mendonça França rafaelfranca closed this November 16, 2012
Rafael Mendonça França

Closed by #8235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.