Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

error_messages_for doesn't html escape it's messages

#1280

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?

Reported by Zach Dennis · October 27th, 2008 @ 02:30 PM

State: resolved
Milestone: 2.x
Assigned to: nobody
Importance: none

Activity

  1. DHH
    DHH

    Why should the message be escaped? Presumably it's a combination of a fixed error message and an attribute. Neither which have user input?

    October 30th, 2008 @ 11:02 AM

  2. Zach Dennis
    Zach Dennis

    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.

    October 30th, 2008 @ 02:42 PM

  3. DHH
    DHH

    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.

    November 4th, 2008 @ 04:58 PM

  4. Inge Jørgensen
    Inge Jørgensen

    Ran into this today, here's a patch. The diff posted breaks the test, I had to explicitely state ERB::Util.html_escape(msg).

    December 15th, 2008 @ 01:44 PM

  5. Zach Dennis
    Zach Dennis

    Inge, can you re-post patch with corresponding test cases ?

    December 15th, 2008 @ 03:25 PM

  6. Inge Jørgensen
    Inge Jørgensen

    Certainly. Something like this?

    December 15th, 2008 @ 04:34 PM

  7. Yaroslav Markin
    Yaroslav Markin

    If applying this, we should probably also patch error_message_on. That is active_record_helper.rb @L123@

    December 27th, 2008 @ 04:13 PM

  8. Dan Barry
    Dan Barry

    I've attached the corresponding patch for error_message_on.

    Also tested that Inge's patch still applies cleanly, and it does.

    January 5th, 2009 @ 08:03 PM

  9. Repository
    Repository
    • 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 error_messages_for doesn't html escape it's messages state:resolved] [Inge Jørgensen, Dan Barry]

    Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...

    March 7th, 2009 @ 06:56 PM