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

Activity

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? :)
- 5545.diff 2.56 KB
October 11th, 2010 @ 07:48 PM
Santiago Pastorino
- Milestone cleared.
- Importance changed from Low to High
October 11th, 2010 @ 08:08 PM
Jeremy Kemper
- Milestone set to 3.0.2
[bulk edit]

October 15th, 2010 @ 11:02 PM
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
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
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
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
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
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