[Git][noosfero/noosfero][master] 2 commits: metadata: marks urls as html safe

Bráulio Bhavamitra gitlab at gitlab.com
Fri Aug 21 09:43:39 BRT 2015


Bráulio Bhavamitra pushed to branch master at Noosfero / noosfero


Commits:
8235fc9d by Braulio Bhavamitra at 2015-08-13T14:05:48Z
metadata: marks urls as html safe

- - - - -
e1d56f4b by Bráulio Bhavamitra at 2015-08-21T12:43:24Z
Merge branch 'metadata-plugin' into 'master'

metadata: marks urls as html safe

This also fix a core bug of double escaping urls.

See merge request !653

- - - - -


9 changed files:

- app/models/article.rb
- plugins/metadata/lib/ext/article.rb
- plugins/metadata/lib/ext/product.rb
- plugins/metadata/lib/ext/profile.rb
- plugins/metadata/lib/ext/uploaded_file.rb
- plugins/metadata/lib/metadata_plugin/base.rb
- plugins/metadata/lib/metadata_plugin/url_helper.rb
- plugins/metadata/test/functional/content_viewer_controller_test.rb
- test/unit/article_test.rb


Changes:

=====================================
app/models/article.rb
=====================================
--- a/app/models/article.rb
+++ b/app/models/article.rb
@@ -743,9 +743,10 @@ class Article < ActiveRecord::Base
   end
 
   def body_images_paths
-    require 'uri'
     Nokogiri::HTML.fragment(self.body.to_s).css('img[src]').collect do |i|
-      (self.profile && self.profile.environment) ? URI.join(self.profile.environment.top_url, URI.escape(i['src'])).to_s : i['src']
+      src = i['src']
+      src = URI.escape src if self.new_record? # xss_terminate runs on save
+      (self.profile && self.profile.environment) ? URI.join(self.profile.environment.top_url, src).to_s : src
     end
   end
 


=====================================
plugins/metadata/lib/ext/article.rb
=====================================
--- a/plugins/metadata/lib/ext/article.rb
+++ b/plugins/metadata/lib/ext/article.rb
@@ -12,9 +12,9 @@ class Article
     end,
     title: proc{ |a, plugin| "#{a.title} - #{a.profile.name}" },
     image: proc do |a, plugin|
-      img = a.body_images_paths
-      img = "#{a.profile.environment.top_url}#{a.profile.image.public_filename}" if a.profile.image if img.blank?
-      img ||= MetadataPlugin.config[:open_graph][:environment_logo] rescue nil if img.blank?
+      img = a.body_images_paths.map! &:html_safe
+      img = "#{a.profile.environment.top_url}#{a.profile.image.public_filename}".html_safe if a.profile.image if img.blank?
+      img ||= MetadataPlugin.config[:open_graph][:environment_logo].html_safe rescue nil if img.blank?
       img
     end,
     see_also: [],
@@ -31,10 +31,10 @@ class Article
     card: 'summary',
     description: proc do |a, plugin|
       description = a.body.to_s || a.environment.name
-      CGI.escapeHTML(plugin.helpers.truncate(plugin.helpers.strip_tags(description), length: 200))
+      plugin.helpers.truncate plugin.helpers.strip_tags(description), length: 200
     end,
     title: proc{ |a, plugin| "#{a.title} - #{a.profile.name}" },
-    image: proc{ |a, plugin| a.body_images_paths },
+    image: proc{ |a, plugin| a.body_images_paths.map! &:html_safe },
   }
 
   metadata_spec namespace: :article, key_attr: :property, tags: {


=====================================
plugins/metadata/lib/ext/product.rb
=====================================
--- a/plugins/metadata/lib/ext/product.rb
+++ b/plugins/metadata/lib/ext/product.rb
@@ -14,8 +14,8 @@ class Product
     description: proc{ |p, plugin| ActionView::Base.full_sanitizer.sanitize p.description },
 
     image: proc do |p, plugin|
-      img = "#{p.environment.top_url}#{p.image.public_filename}" if p.image
-      img = "#{p.environment.top_url}#{p.profile.image.public_filename}" if img.blank? and p.profile.image
+      img = "#{p.environment.top_url}#{p.image.public_filename}".html_safe if p.image
+      img = "#{p.environment.top_url}#{p.profile.image.public_filename}".html_safe if img.blank? and p.profile.image
       img ||= MetadataPlugin.config[:open_graph][:environment_logo] rescue nil if img.blank?
       img
     end,


=====================================
plugins/metadata/lib/ext/profile.rb
=====================================
--- a/plugins/metadata/lib/ext/profile.rb
+++ b/plugins/metadata/lib/ext/profile.rb
@@ -5,8 +5,8 @@ class Profile
   metadata_spec namespace: :og, tags: {
     type: proc{ |p, plugin| plugin.context.params[:og_type] || MetadataPlugin.og_types[:profile] || :profile },
     image: proc do |p, plugin|
-      img = "#{p.environment.top_url}#{p.image.public_filename}" if p.image
-      img ||= MetadataPlugin.config[:open_graph][:environment_logo] rescue nil if img.blank?
+      img = "#{p.environment.top_url}#{p.image.public_filename}".html_safe if p.image
+      img ||= MetadataPlugin.config[:open_graph][:environment_logo].html_safe rescue nil if img.blank?
       img
     end,
     title: proc{ |p, plugin| if p.nickname.present? then p.nickname else p.name end },


=====================================
plugins/metadata/lib/ext/uploaded_file.rb
=====================================
--- a/plugins/metadata/lib/ext/uploaded_file.rb
+++ b/plugins/metadata/lib/ext/uploaded_file.rb
@@ -13,7 +13,7 @@ class UploadedFile
       plugin.og_url_for url
     end,
     title: proc{ |u, plugin| u.title },
-    image: proc{ |u, plugin| "#{u.environment.top_url}#{u.public_filename}" if u.image? },
+    image: proc{ |u, plugin| "#{u.environment.top_url}#{u.public_filename}".html_safe if u.image? },
     description: proc{ |u, plugin| u.abstract || u.title },
   }
 


=====================================
plugins/metadata/lib/metadata_plugin/base.rb
=====================================
--- a/plugins/metadata/lib/metadata_plugin/base.rb
+++ b/plugins/metadata/lib/metadata_plugin/base.rb
@@ -50,7 +50,8 @@ class MetadataPlugin::Base < Noosfero::Plugin
           Array(values).each do |value|
             value = value.call(object, plugin) if value.is_a? Proc rescue nil
             next if value.blank?
-            r << tag(:meta, key_attr => key, value_attr => CGI.escape_html(value.to_s))
+            value = h value unless value.html_safe?
+            r << tag(:meta, {key_attr => key, value_attr => value.to_s}, false, false)
           end
         end
       end


=====================================
plugins/metadata/lib/metadata_plugin/url_helper.rb
=====================================
--- a/plugins/metadata/lib/metadata_plugin/url_helper.rb
+++ b/plugins/metadata/lib/metadata_plugin/url_helper.rb
@@ -7,7 +7,8 @@ module MetadataPlugin::UrlHelper
   def og_url_for options
     options.delete :port
     options[:host] = self.og_domain
-    Noosfero::Application.routes.url_helpers.url_for options
+    url = Noosfero::Application.routes.url_helpers.url_for options
+    url.html_safe
   end
 
   def og_profile_url profile


=====================================
plugins/metadata/test/functional/content_viewer_controller_test.rb
=====================================
--- a/plugins/metadata/test/functional/content_viewer_controller_test.rb
+++ b/plugins/metadata/test/functional/content_viewer_controller_test.rb
@@ -48,6 +48,14 @@ class ContentViewerControllerTest < ActionController::TestCase
     assert_tag tag: 'meta', attributes: { property: 'og:image', content: /\/images\/x.png/  }
   end
 
+  should 'escape utf8 characters correctly' do
+    a = TinyMceArticle.create(name: 'Article to be shared with images', body: 'This article should be shared with all social networks <img src="/images/ç.png" />', profile: profile)
+
+    get :view_page, profile: profile.identifier, page: [ a.name.to_slug ]
+    assert_tag tag: 'meta', attributes: { property: 'og:image', content: /\/images\/%C3%A7.png/  }
+  end
+
+
   should 'render not_found page properly' do
     assert_equal false, Article.exists?(:slug => 'non-existing-page')
     assert_nothing_raised do


=====================================
test/unit/article_test.rb
=====================================
--- a/test/unit/article_test.rb
+++ b/test/unit/article_test.rb
@@ -1497,6 +1497,17 @@ class ArticleTest < ActiveSupport::TestCase
     assert_includes a.body_images_paths, 'http://test.com/noosfero.png'
   end
 
+  should 'escape utf8 characters correctly' do
+    Environment.any_instance.stubs(:default_hostname).returns('noosfero.org')
+    a = build TinyMceArticle, profile: @profile
+    a.body = 'Noosfero <img src="http://noosfero.com/cabeça.png" /> '
+    assert_includes a.body_images_paths, 'http://noosfero.com/cabe%C3%A7a.png'
+
+    # check if after save (that is, after xss_terminate run)
+    a.save!
+    assert_includes a.body_images_paths, 'http://noosfero.com/cabe%C3%A7a.png'
+  end
+
   should 'get absolute images paths in article body' do
     Environment.any_instance.stubs(:default_hostname).returns('noosfero.org')
     a = build TinyMceArticle, :profile => @profile



View it on GitLab: https://gitlab.com/noosfero/noosfero/compare/1cc6c42276a2c2bcc1faad02c7693fd1f77f647a...e1d56f4be78aa13f5dc7a200e72095d56322d81c
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listas.softwarelivre.org/pipermail/noosfero-dev/attachments/20150821/3515948e/attachment-0001.html>


More information about the Noosfero-dev mailing list