New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rendering using Safebuffer #1031

Merged
merged 10 commits into from Jan 31, 2013

Conversation

Projects
None yet
8 participants
@skade
Member

skade commented Jan 28, 2013

This pull request ports the whole rendering stack to using ActiveSupport::SafeBuffer. This should prevent XSS problems described in #936 and #987.

Sadly, it contains 2 extensions to erubis and HAML.

The erubis extension is well within the bounds of the libraries API on the erubis side of things and is similar to the way Rails handles these things. Sadly, it requires a tiny hack to work with tilt 1.3.3 and its twisted template compilation - considering that Tilt hasn't been released since 2011, I see no quick fix there.
It also means that pure erb cannot be used with safe_buffer - this should not break, but the escaping might not work. I wouldn't recommend supporting multiple erb libraries anyways and erubis is the modern option.

The HAML extension is necessary, as the modifications handling SafeBuffer shipped with HAML mainline can only be loaded when ActionView is present. I will lobby with the HAML team to make them available standalone, they are a simple copy.

Slim works out of the box, as "=" is it escapes by default anyways.

Both need a set of default options, which is now supplied.

All helpers are ported and the tests run. The general rule is: structural helpers (content_for, form_for, etc.) and rendering helpers always assume their content to be escaped while code generating helpers (content_tag, tag) assume that all values need to be escaped.

I took the liberty of hiding an easteregg for @postmodern in the core test suite.

See the commit messages for details.

skade added some commits Jan 28, 2013

Make Padrino::Rendering use ActiveSupport::SafeBuffer
This change switches all rendering to use SafeBuffer instead of a raw string.

SafeBuffer is a Subclass of String, so concats and instantiations are cheap.

Concatenating SafeBuffer with a normal String escapes the String. Concatenating
SafeBuffer with another SafeBuffer keeps both intact.

This change requires to extensions: a special template implementation for
Erubis and a port of the XssMods used in HAML. At least the latter can be
dropped once HAML makes them available as a standalone.

All strings returned from #render are considered escaped. Strings can be
marked safe for concatenation using String#html_safe, which turns returns the
String as a SafeBuffer.
Port padrino helpers over to SafeBuffers
This patch ports all helpers over to use SafeBuffers. The changes
follow one general rule: all tag helpers like tag and content_tag
escape everything by default while block helpers like content_for
and form_for assume that the given content is already escaped.

Adds two new methods: safe_content_tag (which assumes that content
is already safe) and mark_safe, which marks a given String or Array
as safe.

Fixes test applications where necessary.
@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 28, 2013

Member

Failures are because of Sinatra 1.3.4 vs. Sinatra 1.3.3 and are unrelated to the patch :/.

Member

skade commented Jan 28, 2013

Failures are because of Sinatra 1.3.4 vs. Sinatra 1.3.3 and are unrelated to the patch :/.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 28, 2013

Member

Finally. Tests pass, even on 1.8.7.

This is blocked by haml/haml#624 , which it currently relies on.

Member

skade commented Jan 28, 2013

Finally. Tests pass, even on 1.8.7.

This is blocked by haml/haml#624 , which it currently relies on.

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Jan 29, 2013

Member

This patch works very good, no excessive memory use or performance drop on MRI19.

Member

ujifgc commented Jan 29, 2013

This patch works very good, no excessive memory use or performance drop on MRI19.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 29, 2013

Member

Nice work @skade this looks like a great feature.

Member

nesquena commented Jan 29, 2013

Nice work @skade this looks like a great feature.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 29, 2013

Member

By the way, for anyone interested in details of how it works and why its fast: http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/

Member

skade commented Jan 29, 2013

By the way, for anyone interested in details of how it works and why its fast: http://yehudakatz.com/2010/02/01/safebuffers-and-rails-3-0/

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Jan 29, 2013

Contributor

@skade +1

Not sure how padrino feels about pulling in activesupport, but at least in Rails3 they made it more modular.

Contributor

postmodern commented Jan 29, 2013

@skade +1

Not sure how padrino feels about pulling in activesupport, but at least in Rails3 they made it more modular.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 29, 2013

Member

We've actually needed parts of activesupport for a while, so the activesupport dependency wasn't an issue with this.

Member

nesquena commented Jan 29, 2013

We've actually needed parts of activesupport for a while, so the activesupport dependency wasn't an issue with this.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 29, 2013

Contributor

Amazing work @skade! :) Thanks!.. So, should we wait for HAML to apply that release haml/haml#624 to merge?

Contributor

dariocravero commented Jan 29, 2013

Amazing work @skade! :) Thanks!.. So, should we wait for HAML to apply that release haml/haml#624 to merge?

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 29, 2013

Member

@dariocravero If you revert #824d508, this can be merged until HAML releases their new version.

Member

skade commented Jan 29, 2013

@dariocravero If you revert #824d508, this can be merged until HAML releases their new version.

@hooopo

This comment has been minimized.

Show comment
Hide comment
@hooopo

hooopo Jan 30, 2013

Contributor

+1
Padrino should be security by default.

Now, I set Sinatra::Base.set :erubis, :escape_html => true in padrino. And in template:

<%= un_escaped_data %>
<%== raw_data %>
Contributor

hooopo commented Jan 30, 2013

+1
Padrino should be security by default.

Now, I set Sinatra::Base.set :erubis, :escape_html => true in padrino. And in template:

<%= un_escaped_data %>
<%== raw_data %>
@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 30, 2013

Member

@hooopo Yes, thats possible. The problem is that this interacts badly with helpers if you don't take care:

<%= link_to("<script>...</script>"...) %>

will fully escape the link, while

<%== link_to("<script>...</script>"...) %>

will let the script tag through in the current state. SafeBuffer cleanly handles all these cases by providing a very nice way to track what strings are safe and which not.

Member

skade commented Jan 30, 2013

@hooopo Yes, thats possible. The problem is that this interacts badly with helpers if you don't take care:

<%= link_to("<script>...</script>"...) %>

will fully escape the link, while

<%== link_to("<script>...</script>"...) %>

will let the script tag through in the current state. SafeBuffer cleanly handles all these cases by providing a very nice way to track what strings are safe and which not.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 30, 2013

Member

Can we decide on milestone for this? API-wise, this is not a big breaking change, but for users that don't sanitize their input into helpers, it is. Then again, users that have problems with this probably should fix their code and quick.

As this is security related, I'd prefer the next release.

Member

skade commented Jan 30, 2013

Can we decide on milestone for this? API-wise, this is not a big breaking change, but for users that don't sanitize their input into helpers, it is. Then again, users that have problems with this probably should fix their code and quick.

As this is security related, I'd prefer the next release.

@postmodern

This comment has been minimized.

Show comment
Hide comment
@postmodern

postmodern Jan 30, 2013

Contributor

The sooner the better. I wager that most padrino apps are probably riddled with XSS. I would be interested in hearing the results of running a XSS web-scan against production apps.

Contributor

postmodern commented Jan 30, 2013

The sooner the better. I wager that most padrino apps are probably riddled with XSS. I would be interested in hearing the results of running a XSS web-scan against production apps.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 30, 2013

Member

I assigned 0.11.0. If the haml project is late with haml 3.2.0, revert

skade@824d508

and include the XssMods in Padrino for the time being.

Member

skade commented Jan 30, 2013

I assigned 0.11.0. If the haml project is late with haml 3.2.0, revert

skade@824d508

and include the XssMods in Padrino for the time being.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Jan 30, 2013

Contributor

@skade, makes sense. Let's schedule it like that. I'm referencing it in 0.11's release issue for the record.

Contributor

dariocravero commented Jan 30, 2013

@skade, makes sense. Let's schedule it like that. I'm referencing it in 0.11's release issue for the record.

@dariocravero dariocravero referenced this pull request Jan 30, 2013

Closed

Release 0.11.0 #755

@dcu

This comment has been minimized.

Show comment
Hide comment
@dcu

dcu Jan 30, 2013

Contributor

What else is needed for 0.11? What can I help with?
On Jan 30, 2013 8:14 AM, "Darío Javier Cravero" notifications@github.com
wrote:

@skade https://github.com/skade, makes sense. Let's schedule it like
that. I'm referencing it in 0.11's release issue.


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/pull/1031#issuecomment-12888828.

Contributor

dcu commented Jan 30, 2013

What else is needed for 0.11? What can I help with?
On Jan 30, 2013 8:14 AM, "Darío Javier Cravero" notifications@github.com
wrote:

@skade https://github.com/skade, makes sense. Let's schedule it like
that. I'm referencing it in 0.11's release issue.


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/pull/1031#issuecomment-12888828.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 30, 2013

Member

@dcu see https://github.com/padrino/padrino-framework/issues?milestone=25&state=open for a list of open tickets for the milestone. All of them are "patches welcome".

Member

skade commented Jan 30, 2013

@dcu see https://github.com/padrino/padrino-framework/issues?milestone=25&state=open for a list of open tickets for the milestone. All of them are "patches welcome".

DAddYE added a commit that referenced this pull request Jan 31, 2013

Merge pull request #1031 from skade/safebuffer
Rendering using Safebuffer

@DAddYE DAddYE merged commit c22cf57 into padrino:master Jan 31, 2013

1 check passed

default The Travis build passed
Details
@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Jan 31, 2013

Member

@dcu yep any help with anything for 0.11 is great appreciated

Member

nesquena commented Jan 31, 2013

@dcu yep any help with anything for 0.11 is great appreciated

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Jan 31, 2013

Member

This was merged a bit fast, it still relies on an unreleased dependency.

Member

skade commented Jan 31, 2013

This was merged a bit fast, it still relies on an unreleased dependency.

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