Skip to content

Commit

Permalink
Fields verification filters malformed html correctly
Browse files Browse the repository at this point in the history
* The field verification now occurs before de validation.
	* Fields that uses the full filter escape residual '<' and '>' after
	  filtering wellformed tags.
	* Fields that uses white_list filter only escape the '<' and '>'
	  symbols if the tag is not wellformed.
	* This patch only fixes the fields that were before being
	  filtered. A full field verification is need to check the way each
	  field must be filtered.

(ActionItem1491)
  • Loading branch information
diguliu authored and terceiro committed May 12, 2010
1 parent 52b5818 commit 688faae
Show file tree
Hide file tree
Showing 26 changed files with 273 additions and 33 deletions.
2 changes: 1 addition & 1 deletion app/models/article.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class Article < ActiveRecord::Base
article.published_at = article.created_at if article.published_at.nil?
end

xss_terminate :only => [ :name ]
xss_terminate :only => [ :name ], :on => 'validation'

named_scope :in_category, lambda { |category|
{:include => 'categories', :conditions => { 'categories.id' => category.id }}
Expand Down
2 changes: 1 addition & 1 deletion app/models/comment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Comment < ActiveRecord::Base
end
end

xss_terminate :only => [ :body, :title, :name ]
xss_terminate :only => [ :body, :title, :name ], :on => 'validation'

def author_name
if author
Expand Down
4 changes: 2 additions & 2 deletions app/models/community.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ class Community < Organization
settings_items :language
settings_items :zip_code, :city, :state, :country

xss_terminate :only => [ :name, :address, :contact_phone, :description ]

before_create do |community|
community.moderated_articles = true if community.environment.enabled?('organizations_are_moderated_by_default')
end
Expand All @@ -22,6 +20,8 @@ def self.create_after_moderation(requestor, attributes = {})
community
end

xss_terminate :only => [ :name, :address, :contact_phone, :description ], :on => 'validation'

FIELDS = %w[
city
state
Expand Down
2 changes: 1 addition & 1 deletion app/models/consumption.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ class Consumption < ActiveRecord::Base

validates_uniqueness_of :product_category_id, :scope => :profile_id

xss_terminate :only => [ :aditional_specifications ]
xss_terminate :only => [ :aditional_specifications ], :on => 'validation'

end
2 changes: 1 addition & 1 deletion app/models/environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ def force_www=(value)

validates_format_of :contact_email, :with => Noosfero::Constants::EMAIL_FORMAT, :if => (lambda { |record| ! record.contact_email.blank? })

xss_terminate :only => [ :message_for_disabled_enterprise ], :with => 'white_list'
xss_terminate :only => [ :message_for_disabled_enterprise ], :with => 'white_list', :on => 'validation'


# #################################################
Expand Down
3 changes: 2 additions & 1 deletion app/models/event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ class Event < Article
settings_items :link, :type => :string
settings_items :address, :type => :string

xss_terminate :only => [ :description, :link, :address ], :with => 'white_list'
xss_terminate :only => [ :link ], :on => 'validation'
xss_terminate :only => [ :description, :link, :address ], :with => 'white_list', :on => 'validation'

validates_presence_of :title, :start_date

Expand Down
2 changes: 1 addition & 1 deletion app/models/folder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class Folder < Article

settings_items :view_as, :type => :string, :default => 'folder'

xss_terminate :only => [ :body ], :with => 'white_list'
xss_terminate :only => [ :body ], :with => 'white_list', :on => 'validation'

def self.select_views
[[_('Folder'), 'folder'], [_('Image gallery'), 'image_gallery']]
Expand Down
2 changes: 1 addition & 1 deletion app/models/organization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def active_fields

validates_format_of :contact_email, :with => Noosfero::Constants::EMAIL_FORMAT, :if => (lambda { |org| !org.contact_email.blank? })

xss_terminate :only => [ :acronym, :contact_person, :contact_email, :legal_form, :economic_activity, :management_information ]
xss_terminate :only => [ :acronym, :contact_person, :contact_email, :legal_form, :economic_activity, :management_information ], :on => 'validation'

# Yes, organizations have members.
#
Expand Down
2 changes: 1 addition & 1 deletion app/models/product.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class Product < ActiveRecord::Base

acts_as_searchable :fields => [ :name, :description, :category_full_name ]

xss_terminate :only => [ :name, :description ]
xss_terminate :only => [ :name, :description ], :on => 'validation'

acts_as_mappable

Expand Down
4 changes: 2 additions & 2 deletions app/models/profile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ def apply_template(template, options = {:copy_articles => true})
self.save_without_validation!
end

xss_terminate :only => [ :name, :nickname, :address, :contact_phone, :description ]
xss_terminate :only => [ :custom_footer, :custom_header ], :with => 'white_list'
xss_terminate :only => [ :name, :nickname, :address, :contact_phone, :description ], :on => 'validation'
xss_terminate :only => [ :custom_footer, :custom_header ], :with => 'white_list', :on => 'validation'

# returns the contact email for this profile.
#
Expand Down
2 changes: 1 addition & 1 deletion app/models/text_article.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# a base class for all text article types.
class TextArticle < Article

xss_terminate :only => [ :name, :abstract, :body ]
xss_terminate :only => [ :name, :abstract, :body ], :on => 'validation'
end
2 changes: 1 addition & 1 deletion app/models/tiny_mce_article.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ def self.description
end

xss_terminate :except => [ :abstract, :body ]
xss_terminate :only => [ :abstract, :body ], :with => 'white_list'
xss_terminate :only => [ :abstract, :body ], :with => 'white_list', :on => 'validation'

end
2 changes: 1 addition & 1 deletion app/models/validation_info.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ class ValidationInfo < ActiveRecord::Base

belongs_to :organization

xss_terminate :only => [ :validation_methodology, :restrictions ]
xss_terminate :only => [ :validation_methodology, :restrictions ], :on => 'validation'
end
16 changes: 16 additions & 0 deletions test/unit/article_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -859,4 +859,20 @@ def setup
assert_no_match /</, article.tags.last.name
end

should 'sanitize name before validation' do
article = Article.new
article.name = "<h1 Bla </h1>"
article.valid?

assert article.errors.invalid?(:name)
end

should 'escape malformed html tags' do
article = Article.new
article.name = "<h1 Malformed >> html >< tag"
article.valid?

assert_no_match /[<>]/, article.name
end

end
22 changes: 22 additions & 0 deletions test/unit/comment_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -187,4 +187,26 @@ class CommentTest < Test::Unit::TestCase
assert_no_match(/<script>/, comment.name)
end

should 'sanitize required fields before validation' do
owner = create_user('testuser').person
article = owner.articles.create(:name => 'test', :body => '...')
comment = article.comments.new(:title => '<h1 title </h1>', :body => '<h1 body </h1>', :name => '<h1 name </h1>', :email => 'cracker@test.org')
comment.valid?

assert comment.errors.invalid?(:name)
assert comment.errors.invalid?(:title)
assert comment.errors.invalid?(:body)
end

should 'escape malformed html tags' do
owner = create_user('testuser').person
article = owner.articles.create(:name => 'test', :body => '...')
comment = article.comments.new(:title => '<h1 title </h1>>> sd f <<', :body => '<h1>> sdf><asd>< body </h1>', :name => '<h1 name </h1>>><<dfsf<sd', :email => 'cracker@test.org')
comment.valid?

assert_no_match /[<>]/, comment.title
assert_no_match /[<>]/, comment.body
assert_no_match /[<>]/, comment.name
end

end
17 changes: 16 additions & 1 deletion test/unit/community_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def setup
should 'require fields if community needs' do
e = Environment.default
e.expects(:required_community_fields).returns(['contact_phone']).at_least_once
community = Community.new(:environment => e)
community = Community.new(:name => 'My community', :environment => e)
assert ! community.valid?
assert community.errors.invalid?(:contact_phone)

Expand Down Expand Up @@ -200,4 +200,19 @@ def setup
community.add_member(person)
end
end

should 'escape malformed html tags' do
community = Community.new
community.name = "<h1 Malformed >> html >< tag"
community.address = "<h1 Malformed >,<<<asfdf> html >< tag"
community.contact_phone = "<h1 Malformed<<> >> html >><>< tag"
community.description = "<h1 Malformed /h1>>><<> html ><>h1< tag"
community.valid?

assert_no_match /[<>]/, community.name
assert_no_match /[<>]/, community.address
assert_no_match /[<>]/, community.contact_phone
assert_no_match /[<>]/, community.description
end

end
10 changes: 7 additions & 3 deletions test/unit/consumption_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@
class ConsumptionTest < Test::Unit::TestCase
fixtures :consumptions

# Replace this with your real tests.
def test_truth
assert true
should 'escape malformed html tags' do
consumption = Consumption.new
consumption.aditional_specifications = "<h1 Malformed >> html >< tag"
consumption.valid?

assert_no_match /[<>]/, consumption.aditional_specifications
end

end
16 changes: 16 additions & 0 deletions test/unit/environment_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -863,4 +863,20 @@ def test_should_list_all_categories
assert_equal env.message_for_member_invitation, env.invitation_mail_template(community)
end

should 'filter fields with white_list filter' do
environment = Environment.new
environment.message_for_disabled_enterprise = "<h1> Disabled Enterprise </h1>"
environment.valid?

assert_equal "<h1> Disabled Enterprise </h1>", environment.message_for_disabled_enterprise
end

should 'escape malformed html tags' do
environment = Environment.new
environment.message_for_disabled_enterprise = "<h1> Disabled Enterprise /h1>"
environment.valid?

assert_no_match /[<>]/, environment.message_for_disabled_enterprise
end

end
28 changes: 28 additions & 0 deletions test/unit/event_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -222,4 +222,32 @@ class EventTest < ActiveSupport::TestCase
assert_not_includes profile.events.by_day(today), event_out_of_range
end

should 'filter fields with full filter' do
event = Event.new
event.link = "<h1 Malformed >> html >< tag"
event.valid?

assert_no_match /[<>]/, event.link
end

should 'filter fields with white_list filter' do
event = Event.new
event.description = "<h1> Description </h1>"
event.address = "<strong> Address <strong>"
event.valid?

assert_equal "<h1> Description </h1>", event.description
assert_equal "<strong> Address <strong>", event.address
end

should 'escape malformed html tags' do
event = Event.new
event.description = "<h1<< Description >>/h1>"
event.address = "<strong>><< Address <strong>"
event.valid?

assert_no_match /[<>]/, event.description
assert_no_match /[<>]/, event.address
end

end
28 changes: 19 additions & 9 deletions test/unit/folder_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,27 @@ class FolderTest < ActiveSupport::TestCase
end

should 'not let pass javascript in the body' do
owner = create_user('testuser').person
folder = fast_create(Folder, {:profile_id => owner.id, :body => '<script>alert("Xss Attack!")</script>'})
folder.save!
assert_no_match(/<script>/, folder.body)
folder = Folder.new
folder.body = "<script> alert(Xss!); </script>"
folder.valid?

assert_no_match /(<script>)/, folder.body
end

should 'let pass html in the body' do
owner = create_user('testuser').person
folder = fast_create(Folder, {:profile_id => owner.id, :body => '<strong>I am not a Xss Attack!")</strong>'})
folder.save!
assert_match(/<strong>/, folder.body)
should 'filter fields with white_list filter' do
folder = Folder.new
folder.body = "<h1> Body </h1>"
folder.valid?

assert_equal "<h1> Body </h1>", folder.body
end

should 'escape malformed html tags' do
folder = Folder.new
folder.body = "<h1<< Description >>/h1>"
folder.valid?

assert_no_match /[<>]/, folder.body
end

end
20 changes: 20 additions & 0 deletions test/unit/organization_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -248,4 +248,24 @@ def create_create_enterprise(org)

assert organization.closed
end

should 'escape malformed html tags' do
organization = Organization.new
organization.acronym = "<h1 Malformed >> html >< tag"
organization.contact_person = "<h1 Malformed >,<<<asfdf> html >< tag"
organization.contact_email = "<h1<malformed@html.com>>"
organization.description = "<h1 Malformed /h1>>><<> html ><>h1< tag"
organization.legal_form = "<h1 Malformed /h1>>><<> html ><>h1< tag"
organization.economic_activity = "<h1 Malformed /h1>>><<> html ><>h1< tag"
organization.management_information = "<h1 Malformed /h1>>><<> html ><>h1< tag"
organization.valid?

assert_no_match /[<>]/, organization.acronym
assert_no_match /[<>]/, organization.contact_person
assert_no_match /[<>]/, organization.contact_email
assert_no_match /[<>]/, organization.legal_form
assert_no_match /[<>]/, organization.economic_activity
assert_no_match /[<>]/, organization.management_information
end

end
18 changes: 18 additions & 0 deletions test/unit/product_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -193,4 +193,22 @@ class ProductTest < Test::Unit::TestCase
end
end

should 'sanitize name before validation' do
product = Product.new
product.name = "<h1 Bla </h1>"
product.valid?

assert product.errors.invalid?(:name)
end

should 'escape malformed html tags' do
product = Product.new
product.name = "<h1 Malformed >> html >< tag"
product.description = "<h1 Malformed</h1>><<<a>> >> html >< tag"
product.valid?

assert_no_match /[<>]/, product.name
assert_no_match /[<>]/, product.description
end

end
38 changes: 38 additions & 0 deletions test/unit/profile_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1515,6 +1515,44 @@ def p.validate
assert profile.errors.invalid?(:description)
end

should 'sanitize name before validation' do
profile = Profile.new
profile.name = "<h1 Bla </h1>"
profile.valid?

assert profile.errors.invalid?(:name)
end

should 'filter fields with white_list filter' do
profile = Profile.new
profile.custom_header = "<h1> Custom Header </h1>"
profile.custom_footer = "<strong> Custom Footer <strong>"
profile.valid?

assert_equal "<h1> Custom Header </h1>", profile.custom_header
assert_equal "<strong> Custom Footer <strong>", profile.custom_footer
end

should 'escape malformed html tags' do
profile = Profile.new
profile.name = "<h1 Malformed >> html >>></a>< tag"
profile.nickname = "<h1 Malformed <<h1>>< html >< tag"
profile.address = "<h1><</h2< Malformed >> html >< tag"
profile.contact_phone = "<h1<< Malformed ><>>> html >< tag"
profile.description = "<h1<a> Malformed >> html ></a>< tag"
profile.custom_header = "<h1<a>><<> Malformed >> html ></a>< tag"
profile.custom_footer = "<h1> Malformed <><< html ></a>< tag"
profile.valid?

assert_no_match /[<>]/, profile.name
assert_no_match /[<>]/, profile.nickname
assert_no_match /[<>]/, profile.address
assert_no_match /[<>]/, profile.contact_phone
assert_no_match /[<>]/, profile.description
assert_no_match /[<>]/, profile.custom_header
assert_no_match /[<>]/, profile.custom_footer
end

private

def assert_invalid_identifier(id)
Expand Down

0 comments on commit 688faae

Please sign in to comment.