html_safe: avoiding escape some strings

this fixes some issues introduced by !859 closes #193

html_safe: avoiding escape some strings
this fixes some issues introduced by !859 closes #193
Joenio Costa
1 parent 1bd0b110
Showing 11 changed files with 105 additions and 20 deletions Show diff stats
app/helpers/application_helper.rb
app/helpers/article_helper.rb
app/helpers/boxes_helper.rb
app/helpers/categories_helper.rb
app/helpers/profile_helper.rb
app/views/blocks/raw_html.html.erb
app/views/friends/_profile_list.html.erb
app/views/shared/_select_categories.html.erb
plugins/people_block/lib/people_block_helper.rb
test/integration/safe_strings_test.rb
test/unit/cms_helper_test.rb
@@ -1055,13 +1055,11 @@ module ApplicationHelper
   end
   def delete_article_message(article)
-    CGI.escapeHTML(
-      if article.folder?
-        _("Are you sure that you want to remove the folder \"%s\"? Note that all the items inside it will also be removed!") % article.name
-      else
-        _("Are you sure that you want to remove the item \"%s\"?") % article.name
-      end
-    )
+    if article.folder?
+      _("Are you sure that you want to remove the folder \"%s\"? Note that all the items inside it will also be removed!") % article.name
+    else
+      _("Are you sure that you want to remove the item \"%s\"?") % article.name
+    end
   end
   def expirable_link_to(expired, content, url, options = {})
@@ -69,14 +69,14 @@ module ArticleHelper
     content_tag('div',
       content_tag('div',
         radio_button(:article, :published, true) +
-          content_tag('span', '&nbsp;', :class => 'access-public-icon') +
+          content_tag('span', '&nbsp;'.html_safe, :class => 'access-public-icon') +
           content_tag('label', _('Public'), :for => 'article_published_true') +
           content_tag('span', _('Visible to other people'), :class => 'access-note'),
           :class => 'access-item'
            ) +
       content_tag('div',
         radio_button(:article, :published, false) +
-          content_tag('span', '&nbsp;', :class => 'access-private-icon') +
+          content_tag('span', '&nbsp;'.html_safe, :class => 'access-private-icon') +
           content_tag('label', _('Private'), :for => 'article_published_false', :id => "label_private") +
           content_tag('span', _('Limit visibility of this article'), :class => 'access-note'),
           :class => 'access-item'
@@ -34,7 +34,7 @@ module BoxesHelper
   def display_boxes_editor(holder)
     with_box_decorator self do
-      content_tag('div', display_boxes(holder, '&lt;' + _('Main content') + '&gt;'), :id => 'box-organizer')
+      content_tag('div', display_boxes(holder, '<' + _('Main content') + '>'), :id => 'box-organizer')
     end
   end
@@ -52,7 +52,7 @@ module BoxesHelper
   def maybe_display_custom_element(holder, element, options = {})
     if holder.respond_to?(element)
-      content_tag('div', holder.send(element), options)
+      content_tag('div', holder.send(element).to_s.html_safe, options)
     else
       ''.html_safe
     end
@@ -64,7 +64,7 @@ module BoxesHelper
   def display_updated_box(box)
     with_box_decorator self do
-      display_box_content(box, '&lt;' + _('Main content') + '&gt;')
+      display_box_content(box, '<' + _('Main content') + '>')
     end
   end
@@ -20,7 +20,7 @@ module CategoriesHelper
   def selected_category_link(cat)
     js_remove = "jQuery('#selected-category-#{cat.id}').remove();"
     content_tag('div', button_to_function_without_text(:remove, _('Remove'), js_remove) +
-      link_to_function(cat.full_name(' &rarr; '), js_remove, :id => "remove-selected-category-#{cat.id}-button", :class => 'select-subcategory-link'),
+      link_to_function(cat.full_name(' &rarr; ').html_safe, js_remove, :id => "remove-selected-category-#{cat.id}-button", :class => 'select-subcategory-link'),
       :class => 'selected-category'
     )
   end
@@ -84,7 +84,7 @@ module ProfileHelper
       entries.map do |entry|
         content = self.send("treat_#{field}", entry)
         unless content.blank?
-          content_tag('tr', content_tag('td', title(field, entry), :class => 'field-name') + content_tag('td', content))
+          content_tag('tr', content_tag('td', title(field, entry), :class => 'field-name') + content_tag('td', content.to_s.html_safe))
         end
       end.join("\n")
     end
 <%= block_title(block.title, block.subtitle) %>
-<%=h block.html %>
+<%= block.html.html_safe %>
 <ul class="profile-list">
   <% profiles.each do |profile| %>
     <li>
-    <%= link_to_profile profile_image(profile) + '<br/>' + profile.short_name,
+    <%= link_to_profile profile_image(profile) + tag('br') + profile.short_name,
                         profile.identifier, :class => 'profile-link' %>
     <div class="controll">
       <%= button_without_text :remove, content_tag('span',_('remove')),
@@ -9,7 +9,7 @@
     categories = [@current_category]
     categories.push(@current_category) while @current_category = @current_category.parent
   %>
-  <%= categories.compact.reverse.map{|i| update_categories_link(nil,i.name, i.id)}.join %>
+  <%= categories.compact.reverse.map{|i| update_categories_link(nil,i.name, i.id)}.join.html_safe %>
   <script>
     function add_category() {
 module PeopleBlockHelper
   def profiles_images_list(profiles)
-    profiles.map { |profile| profile_image_link(profile, :minor) }.join("\n")
+    profiles.map { |profile| profile_image_link(profile, :minor) }.join("\n").html_safe
   end
   def set_address_protocol(address)
@@ -0,0 +1,87 @@
+require_relative "../test_helper"
+
+class SafeStringsTest < ActionDispatch::IntegrationTest
+
+  should 'not escape link to admins on profile page' do
+    person = fast_create Person
+    community = fast_create Community
+    community.add_admin(person)
+    get "/profile/#{community.identifier}"
+    assert_tag :tag => 'td', :content => 'Admins', :sibling => {
+      :tag => 'td', :child => { :tag => 'a', :content => person.name }
+    }
+  end
+
+  should 'not escape people names on members block' do
+    person = fast_create Person
+    community = fast_create Community
+    community.add_member(person)
+    community.boxes << Box.new
+    community.boxes.first.blocks << MembersBlock.new
+    get "/profile/#{community.identifier}"
+    assert_tag :tag => 'div', :attributes => { :id => "block-#{community.blocks.first.id}" }, :descendant => {
+      :tag => 'li', :attributes => { :class => 'vcard' }, :content => person.name
+    }
+  end
+
+  should 'not escape RawHTMLBlock content' do
+    community = fast_create Community
+    community.boxes << Box.new
+    community.boxes.first.blocks << RawHTMLBlock.new(:html => '<b>bold</b>')
+    get "/profile/#{community.identifier}"
+    assert_tag :tag => 'div', :attributes => { :id => "block-#{community.blocks.first.id}" }, :descendant => {
+      :tag => 'b', :content => 'bold'
+    }
+  end
+
+  should 'not escape profile header or footer' do
+    community = fast_create Community
+    community.update_header_and_footer('<b>header</b>', '<b>footer</b>')
+    get "/profile/#{community.identifier}"
+    assert_tag :tag => 'div', :attributes => { :id => 'profile-header' }, :child => { :tag => 'b', :content => 'header' }
+    assert_tag :tag => 'div', :attributes => { :id => 'profile-footer' }, :child => { :tag => 'b', :content => 'footer' }
+  end
+
+  should 'not escape &rarr; symbol from categories' do
+    create_user('marley', :password => 'test', :password_confirmation => 'test').activate
+    category = fast_create Category
+    subcategory = fast_create(Category, :parent_id => category.id)
+    Person['marley'].categories << subcategory
+    login 'marley', 'test'
+    get "/myprofile/marley/profile_editor/edit"
+    assert_tag :tag => 'a', :attributes => { :id => "remove-selected-category-#{subcategory.id}-button" },
+      :content => "#{category.name} &rarr; #{subcategory.name}"
+  end
+
+  should 'not escape MainBlock on profile design' do
+    create_user('jimi', :password => 'test', :password_confirmation => 'test').activate
+    jimi = Person['jimi']
+    jimi.boxes << Box.new
+    jimi.boxes.first.blocks << MainBlock.new
+    login 'jimi', 'test'
+    get "/myprofile/jimi/profile_design"
+    assert_tag :tag => 'div', :attributes => { :class => 'main-content' }, :content => '&lt;Main content&gt;'
+  end
+
+  should 'not escape confirmation message on deleting folders' do
+    create_user('jimi', :password => 'test', :password_confirmation => 'test').activate
+    fast_create(Folder, :name => 'Hey Joe', :profile_id => Person['jimi'].id, :updated_at => DateTime.now)
+    login 'jimi', 'test'
+    get "/myprofile/jimi/cms"
+    assert_tag :tag => 'a', :attributes => {
+      'data-confirm' => /Are you sure that you want to remove the folder &quot;Hey Joe&quot;\?/
+    }
+  end
+
+  should 'not escape people names on manage friends' do
+    create_user('jimi', :password => 'test', :password_confirmation => 'test').activate
+    friend = fast_create Person
+    Person['jimi'].add_friend(friend)
+    login 'jimi', 'test'
+    get '/myprofile/jimi/friends'
+    assert_tag :tag => 'div', :attributes => { :id => 'manage_friends' }, :descendant => {
+      :tag => 'a', :attributes => { :class => 'profile-link' }, :content => friend.name
+    }
+  end
+
+end
@@ -62,7 +62,7 @@ class CmsHelperTest &lt; ActionView::TestCase
     profile = fast_create(Profile)
     name = 'My folder'
     folder = fast_create(Folder, :name => name, :profile_id => profile.id)
-    confirm_message = CGI.escapeHTML("Are you sure that you want to remove the folder \"#{name}\"? Note that all the items inside it will also be removed!")
+    confirm_message = "Are you sure that you want to remove the folder \"#{name}\"? Note that all the items inside it will also be removed!"
     expects(:link_to).with('Delete', {action: 'destroy', id: folder.id}, method: :post, 'data-confirm' => confirm_message, class: 'button with-text icon-delete', title: nil)
     result = display_delete_button(folder)
@@ -73,7 +73,7 @@ class CmsHelperTest &lt; ActionView::TestCase
     profile = fast_create(Profile)
     name = 'My article'
     article = fast_create(TinyMceArticle, :name => name, :profile_id => profile.id)
-    confirm_message = CGI.escapeHTML("Are you sure that you want to remove the item \"#{name}\"?")
+    confirm_message = "Are you sure that you want to remove the item \"#{name}\"?"
     expects(:link_to).with('Delete', {action: 'destroy', id: article.id}, method: :post, 'data-confirm' => confirm_message, class: 'button with-text icon-delete', title: nil)
     result = display_delete_button(article)
	@@ -69,14 +69,14 @@ module ArticleHelper		@@ -69,14 +69,14 @@ module ArticleHelper
69	content_tag('div',	69	content_tag('div',
70	content_tag('div',	70	content_tag('div',
71	radio_button(:article, :published, true) +	71	radio_button(:article, :published, true) +
72	- content_tag('span', ' ', :class => 'access-public-icon') +	72	+ content_tag('span', ' '.html_safe, :class => 'access-public-icon') +
73	content_tag('label', _('Public'), :for => 'article_published_true') +	73	content_tag('label', _('Public'), :for => 'article_published_true') +
74	content_tag('span', _('Visible to other people'), :class => 'access-note'),	74	content_tag('span', _('Visible to other people'), :class => 'access-note'),
75	:class => 'access-item'	75	:class => 'access-item'
76	) +	76	) +
77	content_tag('div',	77	content_tag('div',
78	radio_button(:article, :published, false) +	78	radio_button(:article, :published, false) +
79	- content_tag('span', ' ', :class => 'access-private-icon') +	79	+ content_tag('span', ' '.html_safe, :class => 'access-private-icon') +
80	content_tag('label', _('Private'), :for => 'article_published_false', :id => "label_private") +	80	content_tag('label', _('Private'), :for => 'article_published_false', :id => "label_private") +
81	content_tag('span', _('Limit visibility of this article'), :class => 'access-note'),	81	content_tag('span', _('Limit visibility of this article'), :class => 'access-note'),
82	:class => 'access-item'	82	:class => 'access-item'
	@@ -34,7 +34,7 @@ module BoxesHelper		@@ -34,7 +34,7 @@ module BoxesHelper
34		34
35	def display_boxes_editor(holder)	35	def display_boxes_editor(holder)
36	with_box_decorator self do	36	with_box_decorator self do
37	- content_tag('div', display_boxes(holder, '<' + _('Main content') + '>'), :id => 'box-organizer')	37	+ content_tag('div', display_boxes(holder, '<' + _('Main content') + '>'), :id => 'box-organizer')
38	end	38	end
39	end	39	end
40		40
	@@ -52,7 +52,7 @@ module BoxesHelper		@@ -52,7 +52,7 @@ module BoxesHelper
52		52
53	def maybe_display_custom_element(holder, element, options = {})	53	def maybe_display_custom_element(holder, element, options = {})
54	if holder.respond_to?(element)	54	if holder.respond_to?(element)
55	- content_tag('div', holder.send(element), options)	55	+ content_tag('div', holder.send(element).to_s.html_safe, options)
56	else	56	else
57	''.html_safe	57	''.html_safe
58	end	58	end
	@@ -64,7 +64,7 @@ module BoxesHelper		@@ -64,7 +64,7 @@ module BoxesHelper
64		64
65	def display_updated_box(box)	65	def display_updated_box(box)
66	with_box_decorator self do	66	with_box_decorator self do
67	- display_box_content(box, '<' + _('Main content') + '>')	67	+ display_box_content(box, '<' + _('Main content') + '>')
68	end	68	end
69	end	69	end
70		70
	@@ -20,7 +20,7 @@ module CategoriesHelper		@@ -20,7 +20,7 @@ module CategoriesHelper
20	def selected_category_link(cat)	20	def selected_category_link(cat)
21	js_remove = "jQuery('#selected-category-#{cat.id}').remove();"	21	js_remove = "jQuery('#selected-category-#{cat.id}').remove();"
22	content_tag('div', button_to_function_without_text(:remove, _('Remove'), js_remove) +	22	content_tag('div', button_to_function_without_text(:remove, _('Remove'), js_remove) +
23	- link_to_function(cat.full_name(' → '), js_remove, :id => "remove-selected-category-#{cat.id}-button", :class => 'select-subcategory-link'),	23	+ link_to_function(cat.full_name(' → ').html_safe, js_remove, :id => "remove-selected-category-#{cat.id}-button", :class => 'select-subcategory-link'),
24	:class => 'selected-category'	24	:class => 'selected-category'
25	)	25	)
26	end	26	end
	@@ -84,7 +84,7 @@ module ProfileHelper		@@ -84,7 +84,7 @@ module ProfileHelper
84	entries.map do \|entry\|	84	entries.map do \|entry\|
85	content = self.send("treat_#{field}", entry)	85	content = self.send("treat_#{field}", entry)
86	unless content.blank?	86	unless content.blank?
87	- content_tag('tr', content_tag('td', title(field, entry), :class => 'field-name') + content_tag('td', content))	87	+ content_tag('tr', content_tag('td', title(field, entry), :class => 'field-name') + content_tag('td', content.to_s.html_safe))
88	end	88	end
89	end.join("\n")	89	end.join("\n")
90	end	90	end
1	<%= block_title(block.title, block.subtitle) %>	1	<%= block_title(block.title, block.subtitle) %>
2		2
3	-<%=h block.html %>	3	+<%= block.html.html_safe %>
1	<ul class="profile-list">	1	<ul class="profile-list">
2	<% profiles.each do \|profile\| %>	2	<% profiles.each do \|profile\| %>
3	<li>	3	<li>
4	- <%= link_to_profile profile_image(profile) + '<br/>' + profile.short_name,	4	+ <%= link_to_profile profile_image(profile) + tag('br') + profile.short_name,
5	profile.identifier, :class => 'profile-link' %>	5	profile.identifier, :class => 'profile-link' %>
6	<div class="controll">	6	<div class="controll">
7	<%= button_without_text :remove, content_tag('span',_('remove')),	7	<%= button_without_text :remove, content_tag('span',_('remove')),
	@@ -9,7 +9,7 @@		@@ -9,7 +9,7 @@
9	categories = [@current_category]	9	categories = [@current_category]
10	categories.push(@current_category) while @current_category = @current_category.parent	10	categories.push(@current_category) while @current_category = @current_category.parent
11	%>	11	%>
12	- <%= categories.compact.reverse.map{\|i\| update_categories_link(nil,i.name, i.id)}.join %>	12	+ <%= categories.compact.reverse.map{\|i\| update_categories_link(nil,i.name, i.id)}.join.html_safe %>
13		13
14	<script>	14	<script>
15	function add_category() {	15	function add_category() {
1	module PeopleBlockHelper	1	module PeopleBlockHelper
2	def profiles_images_list(profiles)	2	def profiles_images_list(profiles)
3	- profiles.map { \|profile\| profile_image_link(profile, :minor) }.join("\n")	3	+ profiles.map { \|profile\| profile_image_link(profile, :minor) }.join("\n").html_safe
4	end	4	end
5		5
6	def set_address_protocol(address)	6	def set_address_protocol(address)
@@ -0,0 +1,87 @@		@@ -0,0 +1,87 @@
	1	+require_relative "../test_helper"
	2	+
	3	+class SafeStringsTest < ActionDispatch::IntegrationTest
	4	+
	5	+ should 'not escape link to admins on profile page' do
	6	+ person = fast_create Person
	7	+ community = fast_create Community
	8	+ community.add_admin(person)
	9	+ get "/profile/#{community.identifier}"
	10	+ assert_tag :tag => 'td', :content => 'Admins', :sibling => {
	11	+ :tag => 'td', :child => { :tag => 'a', :content => person.name }
	12	+ }
	13	+ end
	14	+
	15	+ should 'not escape people names on members block' do
	16	+ person = fast_create Person
	17	+ community = fast_create Community
	18	+ community.add_member(person)
	19	+ community.boxes << Box.new
	20	+ community.boxes.first.blocks << MembersBlock.new
	21	+ get "/profile/#{community.identifier}"
	22	+ assert_tag :tag => 'div', :attributes => { :id => "block-#{community.blocks.first.id}" }, :descendant => {
	23	+ :tag => 'li', :attributes => { :class => 'vcard' }, :content => person.name
	24	+ }
	25	+ end
	26	+
	27	+ should 'not escape RawHTMLBlock content' do
	28	+ community = fast_create Community
	29	+ community.boxes << Box.new
	30	+ community.boxes.first.blocks << RawHTMLBlock.new(:html => '<b>bold</b>')
	31	+ get "/profile/#{community.identifier}"
	32	+ assert_tag :tag => 'div', :attributes => { :id => "block-#{community.blocks.first.id}" }, :descendant => {
	33	+ :tag => 'b', :content => 'bold'
	34	+ }
	35	+ end
	36	+
	37	+ should 'not escape profile header or footer' do
	38	+ community = fast_create Community
	39	+ community.update_header_and_footer('<b>header</b>', '<b>footer</b>')
	40	+ get "/profile/#{community.identifier}"
	41	+ assert_tag :tag => 'div', :attributes => { :id => 'profile-header' }, :child => { :tag => 'b', :content => 'header' }
	42	+ assert_tag :tag => 'div', :attributes => { :id => 'profile-footer' }, :child => { :tag => 'b', :content => 'footer' }
	43	+ end
	44	+
	45	+ should 'not escape → symbol from categories' do
	46	+ create_user('marley', :password => 'test', :password_confirmation => 'test').activate
	47	+ category = fast_create Category
	48	+ subcategory = fast_create(Category, :parent_id => category.id)
	49	+ Person['marley'].categories << subcategory
	50	+ login 'marley', 'test'
	51	+ get "/myprofile/marley/profile_editor/edit"
	52	+ assert_tag :tag => 'a', :attributes => { :id => "remove-selected-category-#{subcategory.id}-button" },
	53	+ :content => "#{category.name} → #{subcategory.name}"
	54	+ end
	55	+
	56	+ should 'not escape MainBlock on profile design' do
	57	+ create_user('jimi', :password => 'test', :password_confirmation => 'test').activate
	58	+ jimi = Person['jimi']
	59	+ jimi.boxes << Box.new
	60	+ jimi.boxes.first.blocks << MainBlock.new
	61	+ login 'jimi', 'test'
	62	+ get "/myprofile/jimi/profile_design"
	63	+ assert_tag :tag => 'div', :attributes => { :class => 'main-content' }, :content => '<Main content>'
	64	+ end
	65	+
	66	+ should 'not escape confirmation message on deleting folders' do
	67	+ create_user('jimi', :password => 'test', :password_confirmation => 'test').activate
	68	+ fast_create(Folder, :name => 'Hey Joe', :profile_id => Person['jimi'].id, :updated_at => DateTime.now)
	69	+ login 'jimi', 'test'
	70	+ get "/myprofile/jimi/cms"
	71	+ assert_tag :tag => 'a', :attributes => {
	72	+ 'data-confirm' => /Are you sure that you want to remove the folder "Hey Joe"\?/
	73	+ }
	74	+ end
	75	+
	76	+ should 'not escape people names on manage friends' do
	77	+ create_user('jimi', :password => 'test', :password_confirmation => 'test').activate
	78	+ friend = fast_create Person
	79	+ Person['jimi'].add_friend(friend)
	80	+ login 'jimi', 'test'
	81	+ get '/myprofile/jimi/friends'
	82	+ assert_tag :tag => 'div', :attributes => { :id => 'manage_friends' }, :descendant => {
	83	+ :tag => 'a', :attributes => { :class => 'profile-link' }, :content => friend.name
	84	+ }
	85	+ end
	86	+
	87	+end
	@@ -62,7 +62,7 @@ class CmsHelperTest < ActionView::TestCase		@@ -62,7 +62,7 @@ class CmsHelperTest < ActionView::TestCase
62	profile = fast_create(Profile)	62	profile = fast_create(Profile)
63	name = 'My folder'	63	name = 'My folder'
64	folder = fast_create(Folder, :name => name, :profile_id => profile.id)	64	folder = fast_create(Folder, :name => name, :profile_id => profile.id)
65	- confirm_message = CGI.escapeHTML("Are you sure that you want to remove the folder \"#{name}\"? Note that all the items inside it will also be removed!")	65	+ confirm_message = "Are you sure that you want to remove the folder \"#{name}\"? Note that all the items inside it will also be removed!"
66	expects(:link_to).with('Delete', {action: 'destroy', id: folder.id}, method: :post, 'data-confirm' => confirm_message, class: 'button with-text icon-delete', title: nil)	66	expects(:link_to).with('Delete', {action: 'destroy', id: folder.id}, method: :post, 'data-confirm' => confirm_message, class: 'button with-text icon-delete', title: nil)
67		67
68	result = display_delete_button(folder)	68	result = display_delete_button(folder)
	@@ -73,7 +73,7 @@ class CmsHelperTest < ActionView::TestCase		@@ -73,7 +73,7 @@ class CmsHelperTest < ActionView::TestCase
73	profile = fast_create(Profile)	73	profile = fast_create(Profile)
74	name = 'My article'	74	name = 'My article'
75	article = fast_create(TinyMceArticle, :name => name, :profile_id => profile.id)	75	article = fast_create(TinyMceArticle, :name => name, :profile_id => profile.id)
76	- confirm_message = CGI.escapeHTML("Are you sure that you want to remove the item \"#{name}\"?")	76	+ confirm_message = "Are you sure that you want to remove the item \"#{name}\"?"
77	expects(:link_to).with('Delete', {action: 'destroy', id: article.id}, method: :post, 'data-confirm' => confirm_message, class: 'button with-text icon-delete', title: nil)	77	expects(:link_to).with('Delete', {action: 'destroy', id: article.id}, method: :post, 'data-confirm' => confirm_message, class: 'button with-text icon-delete', title: nil)
78		78
79	result = display_delete_button(article)	79	result = display_delete_button(article)