Don't HTML escape ActionMailer plaintext templates #6943

Closed
wants to merge 1 commit into
from

Projects

None yet

10 participants

@mguterl

Here's a test case and fix for issue #687.

The problem only exists when the evaluating block expressions.

@josevalim josevalim commented on the diff Jul 3, 2012
actionpack/lib/action_view/template/handlers/erb.rb
@@ -79,6 +79,7 @@ def call(template)
self.class.erb_implementation.new(
erb,
+ :escape => template.identifier !~ /\.html/, # only escape HTML templates
@josevalim
josevalim Jul 3, 2012

I think we should explicitly check for text instead of everything that is not an html. We should probably do it via a configuration option too. So people have a better control.

@mguterl
mguterl Jul 3, 2012

Thanks for the feedback @josevalim. Are you thinking we should provide an option of which formats that should be escaped or a list of those which should not be escaped?

Do you have any preference for what the configuration option should be called?

@steveklabnik
steveklabnik Jul 3, 2012

@josevalim when I was working on this, I had the same feeling, then I remembered that... you'd only want to HTML escape an HTML template. So I think this is actually better than only not escaping text.

@iblue

So I tried to submit a pull request for a bug where a fix exists for more than one month. Why has this not been merged yet?

@steveklabnik
Ruby on Rails member

Rails is all-volunteer, and someone with merge privileges (not me) hasn't decided to merge it yet. That's the best answer I can give you. :/

@josevalim
Ruby on Rails member

@lblue, this pull request is not complete:

1) We are talking about security, so I would prefer to explicitly whitelist a few formats than saying everything that is not html. For this reason, we should give people a configuration option, so they can opt-in for other formats;

2) template.identifier is not the best way to check the template format, I believe we have template.formats or template.format which would be more appropriate.

@steveklabnik
Ruby on Rails member

What do you think about my comment that HTML escaping only applies to content that's encoded as HTML?

@schneems
Ruby on Rails member

ping, @mguterl will you have a chance to implement @josevalim's proposed changes? Do you have questions about his feedback?

@mguterl

I haven't had a chance to implement @josevalim's proposed changes yet. I'll see what I can do in the next couple of days.

@kenn

If whitelisting is the way to go, why don't we modify the line

:escape => template.identifier !~ /\.html/

to this?

:escape => template.identifier =~ /\.text/

I believe making it configurable is overkill and many people are talking about plain text ActionMailer templates.

There is a regression in 3.2.8 due to 28f2c6f, as single quotes are now HTML-escaped.

Hi xxx, here's the link to activate your account

is now rendered

Hi xxx, here's the link to activate your account
@cluesque

Better regex:

:escape => template.identifier =~ /\.te?xt/

So welcome.txt.erb templates are also not escaped.

@tilsammans

The whitelist approach seems valid to me. There's a tradeoff between automatic security everywhere and leaving some power in the hands of the developer. When she creates a template called confirmation.text.erb we should acknowledge her decision and stop escaping HTML, since it quite obviously means the contents should be rendered as plain text.

This regression in Rails makes for some pretty stealthy problems where e.g. an ampersand in a person's name is suddenly escaped in emails. It looks horrible and unprofessional for the recipient of the email. So I would rather the pull request merged sooner than later. Thanks!

@jarl-dk

Affects me too...

@jarl-dk

Please will someone take a look at this? Please also consider @cluesque s proposal for a better regex.

@schneems
Ruby on Rails member

@mguterl are you still working on this, or should we ask one of these other interested gentleman to take a stab at the proposed changes?

@mguterl

@schneems - I'm pretty swamped right now and I don't think I'll have time for a couple weeks.

@tilsammans

Took over this pull at #7976

@rafaelfranca
Ruby on Rails member

Closing in favor of #7976

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment