Fix XSS protection in article titles

(ActionItem1667)

Fix XSS protection in article titles
(ActionItem1667)
Antonio Terceiro
1 parent f4480800
Showing 4 changed files with 26 additions and 5 deletions Show diff stats
app/models/article.rb
app/models/tiny_mce_article.rb
test/unit/article_test.rb
test/unit/tiny_mce_article_test.rb
@@ -26,7 +26,7 @@ class Article &lt; ActiveRecord::Base
     article.published_at = article.created_at if article.published_at.nil?
   end
-  xss_terminate :only => [ :name ], :on => 'validation'
+  xss_terminate :only => [ :name ], :on => 'validation', :with => 'white_list'
   named_scope :in_category, lambda { |category|
     {:include => 'categories', :conditions => { 'categories.id' => category.id }}
@@ -8,9 +8,9 @@ class TinyMceArticle &lt; TextArticle
     _('Not accessible for visually impaired users.')
   end
-  xss_terminate :except => [ :abstract, :body ]
+  xss_terminate :only => [  ]
-  xss_terminate :only => [ :abstract, :body ], :with => 'white_list', :on => 'validation'
+  xss_terminate :only => [ :name, :abstract, :body ], :with => 'white_list', :on => 'validation'
   include WhiteListFilter
   filter_iframes :abstract, :body, :whitelist => lambda { profile && profile.environment && profile.environment.trusted_sites_for_iframe }
@@ -862,7 +862,24 @@ class ArticleTest &lt; Test::Unit::TestCase
     article.name = "<h1 Bla </h1>"
     article.valid?
-    assert article.errors.invalid?(:name)
+    assert_no_match /<[^>]*</, article.name
+  end
+
+  should 'not doubly escape quotes in the name' do
+    profile = fast_create(Profile)
+    a = fast_create(Article, :profile_id => profile.id)
+    p = PublishedArticle.create!(:reference_article => a, :profile => fast_create(Community))
+
+    p.name = 'title with "quotes"'
+    p.save
+    assert_equal 'title with "quotes"', p.name
+  end
+
+  should 'remove script tags from name' do
+    a = Article.new(:name => 'hello <script>alert(1)</script>')
+    a.valid?
+
+    assert_no_match(/<script>/, a.name)
   end
   should 'escape malformed html tags' do
@@ -878,5 +895,4 @@ class ArticleTest &lt; Test::Unit::TestCase
     article.name = 'a123456789abcdefghij'
     assert_equal 'a123456789ab...', article.short_title
   end
-
 end
@@ -113,4 +113,9 @@ class TinyMceArticleTest &lt; Test::Unit::TestCase
     assert_match  /<!-- .* --> <h1> Wellformed html code <\/h1>/, article.body
   end
+  should 'not allow XSS on name' do
+    article = TinyMceArticle.create!(:name => 'title with <script>alert("xss")</script>', :profile => profile)
+    assert_no_match /script/, article.name
+  end
+
 end
	@@ -26,7 +26,7 @@ class Article < ActiveRecord::Base		@@ -26,7 +26,7 @@ class Article < ActiveRecord::Base
26	article.published_at = article.created_at if article.published_at.nil?	26	article.published_at = article.created_at if article.published_at.nil?
27	end	27	end
28		28
29	- xss_terminate :only => [ :name ], :on => 'validation'	29	+ xss_terminate :only => [ :name ], :on => 'validation', :with => 'white_list'
30		30
31	named_scope :in_category, lambda { \|category\|	31	named_scope :in_category, lambda { \|category\|
32	{:include => 'categories', :conditions => { 'categories.id' => category.id }}	32	{:include => 'categories', :conditions => { 'categories.id' => category.id }}
	@@ -8,9 +8,9 @@ class TinyMceArticle < TextArticle		@@ -8,9 +8,9 @@ class TinyMceArticle < TextArticle
8	_('Not accessible for visually impaired users.')	8	_('Not accessible for visually impaired users.')
9	end	9	end
10		10
11	- xss_terminate :except => [ :abstract, :body ]	11	+ xss_terminate :only => [ ]
12		12
13	- xss_terminate :only => [ :abstract, :body ], :with => 'white_list', :on => 'validation'	13	+ xss_terminate :only => [ :name, :abstract, :body ], :with => 'white_list', :on => 'validation'
14		14
15	include WhiteListFilter	15	include WhiteListFilter
16	filter_iframes :abstract, :body, :whitelist => lambda { profile && profile.environment && profile.environment.trusted_sites_for_iframe }	16	filter_iframes :abstract, :body, :whitelist => lambda { profile && profile.environment && profile.environment.trusted_sites_for_iframe }
	@@ -862,7 +862,24 @@ class ArticleTest < Test::Unit::TestCase		@@ -862,7 +862,24 @@ class ArticleTest < Test::Unit::TestCase
862	article.name = "<h1 Bla </h1>"	862	article.name = "<h1 Bla </h1>"
863	article.valid?	863	article.valid?
864		864
865	- assert article.errors.invalid?(:name)	865	+ assert_no_match /<[^>]*</, article.name
		866	+ end
		867	+
		868	+ should 'not doubly escape quotes in the name' do
		869	+ profile = fast_create(Profile)
		870	+ a = fast_create(Article, :profile_id => profile.id)
		871	+ p = PublishedArticle.create!(:reference_article => a, :profile => fast_create(Community))
		872	+
		873	+ p.name = 'title with "quotes"'
		874	+ p.save
		875	+ assert_equal 'title with "quotes"', p.name
		876	+ end
		877	+
		878	+ should 'remove script tags from name' do
		879	+ a = Article.new(:name => 'hello <script>alert(1)</script>')
		880	+ a.valid?
		881	+
		882	+ assert_no_match(/<script>/, a.name)
866	end	883	end
867		884
868	should 'escape malformed html tags' do	885	should 'escape malformed html tags' do
	@@ -878,5 +895,4 @@ class ArticleTest < Test::Unit::TestCase		@@ -878,5 +895,4 @@ class ArticleTest < Test::Unit::TestCase
878	article.name = 'a123456789abcdefghij'	895	article.name = 'a123456789abcdefghij'
879	assert_equal 'a123456789ab...', article.short_title	896	assert_equal 'a123456789ab...', article.short_title
880	end	897	end
881	-
882	end	898	end
	@@ -113,4 +113,9 @@ class TinyMceArticleTest < Test::Unit::TestCase		@@ -113,4 +113,9 @@ class TinyMceArticleTest < Test::Unit::TestCase
113	assert_match /<!-- .* --> <h1> Wellformed html code <\/h1>/, article.body	113	assert_match /<!-- .* --> <h1> Wellformed html code <\/h1>/, article.body
114	end	114	end
115		115
		116	+ should 'not allow XSS on name' do
		117	+ article = TinyMceArticle.create!(:name => 'title with <script>alert("xss")</script>', :profile => profile)
		118	+ assert_no_match /script/, article.name
		119	+ end
		120	+
116	end	121	end