Skip to content

Commit

Permalink
Security: Replace white_list with Rails 2.2 sanitizer
Browse files Browse the repository at this point in the history
The Rails 2.2 santizer is an enhanced version of Rick's original
white_list plugin, so let's upgrade and get the latest fixes.

Note that Mephisto had separate rules for sanitizing comments and
non-comments in Atom feeds.  This difference was introduced in commit
88df87e.  Unfortunately, I'm not able
to track down any information on the problem being fixed here.  Since we
already add half of the tags in question to the whitelist, I've decided
to just treat all sanitized Atom feed content the same.  Please let me
know if this breaks anything.
  • Loading branch information
emk committed Dec 12, 2008
1 parent 31430c4 commit d2c8c8e
Show file tree
Hide file tree
Showing 12 changed files with 10 additions and 293 deletions.
5 changes: 2 additions & 3 deletions app/drops/comment_drop.rb
@@ -1,13 +1,12 @@
class CommentDrop < BaseDrop
include Mephisto::Liquid::UrlMethods
include WhiteListHelper

timezone_dates :published_at, :created_at
liquid_attributes.push(*[:author, :author_email, :author_ip, :title])

def initialize(source)
super
@liquid.update 'is_approved' => @source.approved?, 'body' => white_list(@source.body_html)
@liquid.update 'is_approved' => @source.approved?, 'body' => ActionView::Base.white_list_sanitizer.sanitize(@source.body_html)
end

def author_url
Expand Down Expand Up @@ -37,4 +36,4 @@ def presentation_class
"by-user"
end
end
end
end
5 changes: 2 additions & 3 deletions app/helpers/application_helper.rb
Expand Up @@ -75,9 +75,8 @@ def comment_expiration_options
['Expire 3 months after publishing', 90]]
end

def sanitize_feed_content(html, sanitize_tables = false)
options = sanitize_tables ? {} : {:tags => %w(table thead tfoot tbody td tr th)}
returning h(white_list(html.strip, options)) do |html|
def sanitize_feed_content(html)
returning h(sanitize(html.strip)) do |html|
html.gsub! /&amp;(#\d+);/ do |s|
"&#{$1};"
end
Expand Down
2 changes: 1 addition & 1 deletion app/views/feed/_comment.rxml
Expand Up @@ -11,5 +11,5 @@ xm.entry 'xml:base' => home_url do
xm.link "rel" => "alternate", "type" => "text/html",
"href" => "http://#{request.host_with_port}#{relative_url_root}#{section_url_for article}"
xm.title "Comment on '#{strip_tags(article.title)}' by #{comment.author}"
xm << %{<content type="html">#{sanitize_feed_content comment.body_html, true}</content>}
xm << %{<content type="html">#{sanitize_feed_content comment.body_html}</content>}
end
2 changes: 1 addition & 1 deletion app/views/mephisto/_comment.rxml
Expand Up @@ -10,5 +10,5 @@ xm.entry 'xml:base' => home_url do
end
xm.link "rel" => "alternate", "type" => "text/html", "href" => "http://#{request.host_with_port}#{site.permalink_for(article)}"
xm.title "Comment on '#{strip_tags(article.title)}' by #{comment.author}"
xm << %{<content type="html">#{sanitize_feed_content comment.body_html, true}</content>}
xm << %{<content type="html">#{sanitize_feed_content comment.body_html}</content>}
end
3 changes: 3 additions & 0 deletions config/environment.rb
Expand Up @@ -63,6 +63,9 @@ def safe_to_load_application?
config.active_record.observers = [:article_observer, :comment_observer]
end

# Allow table tags in untrusted HTML.
config.action_view.sanitized_allowed_tags = 'table', 'tr', 'td'

# We're slowly moving the contents of vendor and vender/plugins into
# vendor/gems by adding config.gem declarations.
config.gem 'RedCloth', :version => '3.0.4', :lib => 'redcloth'
Expand Down
2 changes: 0 additions & 2 deletions config/initializers/templating.rb
Expand Up @@ -4,8 +4,6 @@ module Liquid

Liquid::For.send :include, Mephisto::Liquid::ForWithSorting

WhiteListHelper.tags.merge %w(table tr td)

class MissingTemplateError < StandardError
attr_reader :template_type, :templates
def initialize(template_type, templates)
Expand Down
2 changes: 1 addition & 1 deletion test/functional/application_helper_test.rb
Expand Up @@ -14,7 +14,7 @@ def host_with_port

class ApplicationHelperTest < Test::Unit::TestCase
fixtures :assets, :users
include ActionView::Helpers::TagHelper, ApplicationHelper, WhiteListHelper
include ActionView::Helpers::TagHelper, ApplicationHelper

def request
@request ||= ApplicationHelperTestController.new
Expand Down
29 changes: 0 additions & 29 deletions vendor/plugins/white_list/README

This file was deleted.

22 changes: 0 additions & 22 deletions vendor/plugins/white_list/Rakefile

This file was deleted.

2 changes: 0 additions & 2 deletions vendor/plugins/white_list/init.rb

This file was deleted.

97 changes: 0 additions & 97 deletions vendor/plugins/white_list/lib/white_list_helper.rb

This file was deleted.

132 changes: 0 additions & 132 deletions vendor/plugins/white_list/test/white_list_test.rb

This file was deleted.

0 comments on commit d2c8c8e

Please sign in to comment.