This project is archived and is in readonly mode.
error_messages_for doesn't html escape it's messages
Reported by Zach Dennis | October 27th, 2008 @ 02:30 PM | in 2.x
error_messages_for doesn't html escape the messages that it displays. It should be doing this. Here's a diff of the fix. I don't have the time right now to post a proper patch, so here's the ticket in case someone has the time.
diff --git a/vendor/rails/actionpack/lib/action_view/helpers/active_record_helper.rb b/vendor/rails/actionpack/lib/action_view/helpers/active_record_helper.rb
index 8b56d24..137fb73 100644
--- a/vendor/rails/actionpack/lib/action_view/helpers/active_record_helper.rb
+++ b/vendor/rails/actionpack/lib/action_view/helpers/active_record_helper.rb
@@ -198,7 +198,7 @@ module ActionView
locale.t :header, :count => count, :model => object_name
end
message = options.include?(:message) ? options[:message] : locale.t(:body)
- error_messages = objects.sum {|object| object.errors.full_messages.map {|msg| content_tag(:li, msg) } }.join
+ error_messages = objects.sum {|object| object.errors.full_messages.map {|msg| content_tag(:li, h(msg)) } }.join
contents = ''
contents << content_tag(options[:header_tag] || :h2, header_message) unless header_message.blank?
Comments and changes to this ticket
-
DHH October 30th, 2008 @ 11:02 AM
Why should the message be escaped? Presumably it's a combination of a fixed error message and an attribute. Neither which have user input?
-
Zach Dennis October 30th, 2008 @ 02:42 PM
If the error message contains <, >, &, or any of the other characters that need to be escaped then you're in for a surprise when it does get rendered. It seems like html escaping error messages at model level isn't the right place to do that.
Granted, this problem doesn't come up all that often, given I'm the first reporting it in the past 3 - 4 years. It seems like having this become a non-issue (rather than just a 1% occurrence) by putting the responsibility of html escaping in error_messages_for is a good thing to do.
If it doesn't make it in there, perhaps update the docs on error_messages_for so its explicitly stated that it will not html escape any characters that require it in an error message.
-
DHH November 4th, 2008 @ 04:58 PM
Ah, I follow. That makes sense. If you wrap this up with as a proper patch and update any tests needed, it's going in.
-
Inge Jørgensen December 15th, 2008 @ 01:44 PM
Ran into this today, here's a patch. The diff posted breaks the test, I had to explicitely state ERB::Util.html_escape(msg).
-
Zach Dennis December 15th, 2008 @ 03:25 PM
Inge, can you re-post patch with corresponding test cases ?
-
-
Yaroslav Markin December 27th, 2008 @ 04:13 PM
If applying this, we should probably also patch
error_message_on. That isactive_record_helper.rb@L123@ -
Dan Barry January 5th, 2009 @ 08:03 PM
I've attached the corresponding patch for error_message_on.
Also tested that Inge's patch still applies cleanly, and it does.
-
Repository March 7th, 2009 @ 06:56 PM
- State changed from “new” to “resolved”
- Tag changed from “actionview, errors” to “actionview, errors, patch”
(from [45494580d9405e80ba124d17c8379436883c8c78]) Ensure Active Record error related view helpers escape the message [#1280 state:resolved] [Inge Jørgensen, Dan Barry]
Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
CancelProfileIsBroken
DHH
Yaroslav Markin
Attachments
Tags
Referenced by
-
1280
error_messages_for doesn't html escape it's messages
(from [45494580d9405e80ba124d17c8379436883c8c78]) Ensure ...
-
2409
activerecord double escapes error_messages_for
Appears to be a deliberate change made in
http://github....