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.

There was a problem

You must be a member of this account.

This project is archived and is in readonly mode.

capture helper assumes that text is html_safe

#5545

That may cause XSS issues if you don't expect it...

Example:

<p><%= @profile.location %></p>
<p><%= capture { @profile.location } %></p>
<p><%= content_tag(:div, @profile.location) %></p>
<p><%= content_tag(:div) { @profile.location } %></p>

# result:

<p>Kraków, Poland &lt;h1&gt;foo&lt;h1&gt;</p> 
<p>Kraków, Poland <h1>foo<h1></p> 
<p><div>Kraków, Poland &lt;h1&gt;foo&lt;h1&gt;</div></p> 
<p><div>Kraków, Poland <h1>foo<h1></div></p>

Reported by Jakub Suder · September 3rd, 2010 @ 12:04 PM

State: resolved
Milestone: 3.0.2
Assigned to: Santiago Pastorino Santiago Pastorino
Importance: High

Activity

  1. Jeff Kreeftmeijer
    Jeff Kreeftmeijer
    • State changed from new to open
    • Milestone set to 3.x
    • Assigned user set to Santiago Pastorino
    • Tag changed from action_view, capturehelper, content_tag, escaping, helpers, xss to action_view, capturehelper, content_tag, escaping, helpers, patch, xss
    • Importance changed from to Low

    I've attached a patch that runs html_escape on capture's output before returning. It also adds .html_safe to the test_link_to_unless assertion and removes another test_link_to_unless I found. Don't ask me why, but there were two. Any thoughts? :)

    October 11th, 2010 @ 07:48 PM

  2. Santiago Pastorino
    Santiago Pastorino
    • Milestone cleared.
    • Importance changed from Low to High

    October 11th, 2010 @ 08:08 PM

  3. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2

    October 15th, 2010 @ 11:02 PM

  4. Ryan Bigg
    Ryan Bigg
    • Tag changed from sheepskin boots, action_view, capturehelper, content_tag, escaping, helpers, patch, xss to action_view capturehelper content_tag escaping helpers patch xss

    Automatic cleanup of spam.

    October 16th, 2010 @ 02:38 AM

  5. Jeff Kreeftmeijer
    Jeff Kreeftmeijer
    • Tag changed from action_view capturehelper content_tag escaping helpers patch xss to action_view, capturehelper, content_tag, helpers, patch, xss

    Ryan's spambot script seems to have reverted my tag changes.

    October 16th, 2010 @ 12:41 PM

  6. Repository
    Repository
    • State changed from open to resolved

    (from [bb9c58eb4aa637fa75c69c705a9918d6322ff834]) Make sure capture's output gets html_escaped [#5545 capture helper assumes that text is html_safe state:resolved]

    Also remove a duplicate test_link_to_unless assertion and add .html_safe to the
    remaining one.

    Signed-off-by: Santiago Pastorino santiago@wyeworks.com
    http://github.com/rails/rails/commit/bb9c58eb4aa637fa75c69c705a9918...

    November 2nd, 2010 @ 10:03 PM

  7. Repository
    Repository

    (from [5cb1dad228fb224a5661e36fe492dae700021b0f]) Make sure capture's output gets html_escaped [#5545 capture helper assumes that text is html_safe state:resolved]

    Also remove a duplicate test_link_to_unless assertion and add .html_safe
    to the remaining one.

    Signed-off-by: Santiago Pastorino santiago@wyeworks.com
    http://github.com/rails/rails/commit/5cb1dad228fb224a5661e36fe492da...

    November 2nd, 2010 @ 10:21 PM

  8. Keith Pitt
    Keith Pitt

    Hrm - this seems like a silly change to me.

    Firstly, the documentation inside the capture helper states:

    "It provides a method to capture blocks into variables through capture and a way to capture a block of markup for use in a layout through content_for."

    Keyword there being "markup". If you automatically escape the content - you cant really use it for markup anymore.

    Secondly, given the following helper:

    def box(&block)
  content_tag :div, capture(&block).html_safe
end

    Wouldnt that automatically escape the html returned by capture, thus making this sort of convention impossible?

    November 24th, 2010 @ 04:48 AM

  9. Wincent Colaiuta
    Wincent Colaiuta

    Also not 100% sure if this is the best change, as it caused some breakage in my app where I was using it to "capture a block of markup", like Keith says.

    On the other hand, at least it is more consistent than before, seeing as now both the block and non-block forms of content_tag will escape their content (if it is not already marked as safe).

    December 3rd, 2010 @ 05:15 PM