Fixed html sanitizer loop. Extended tests.

app/models/channel/filter/identify_sender.rb

@@ -126,8 +126,6 @@ module Channel::Filter::IdentifySender
   end
 
   def self.user_create(data)
-
-    # return existing
     user = User.find_by(email: data[:email].downcase)
     if !user
       user = User.find_by(login: data[:email].downcase)
@@ -137,10 +135,7 @@ module Channel::Filter::IdentifySender
     if user
       if user.firstname.blank? && user.lastname.blank?
         if data[:firstname].present?
-          data[:firstname].strip!
+          data[:firstname] = cleanup_name(data[:firstname])
-          data[:firstname].delete!('"')
-          data[:firstname].gsub!(/^'/, '')
-          data[:firstname].gsub!(/'$/, '')
           user.update_attributes(
             firstname: data[:firstname]
           )
@@ -157,10 +152,7 @@ module Channel::Filter::IdentifySender
       if data[item.to_sym].nil?
         data[item.to_sym] = ''
       end
-      data[item.to_sym].strip!
+      data[item.to_sym] = cleanup_name(data[item.to_sym])
-      data[item.to_sym].delete!('"')
-      data[item.to_sym].gsub!(/^'/, '')
-      data[item.to_sym].gsub!(/'$/, '')
     }
     data[:password]      = ''
     data[:active]        = true
@@ -176,4 +168,13 @@ module Channel::Filter::IdentifySender
     user
   end
 
+  def self.cleanup_name(string)
+    string.strip!
+    string.delete!('"')
+    string.gsub!(/^'/, '')
+    string.gsub!(/'$/, '')
+    string.gsub!(/.+?\s\(.+?\)$/, '')
+    string
+  end
+
 end

lib/html_sanitizer.rb

@@ -38,7 +38,7 @@ satinize html string based on whiltelist
 
       # replace tags, keep subtree
       if !tags_whitelist.include?(node.name)
-        node.replace strict(node.children.to_s)
+        node.replace node.children.to_s
         Loofah::Scrubber::STOP
       end
 
@@ -132,7 +132,7 @@ satinize html string based on whiltelist
       # prepare links
       if node['href']
         href = cleanup_target(node['href'])
-        if external && !href.downcase.start_with?('//') && href.downcase !~ %r{^.{1,6}://.+?}
+        if external && href.present? && !href.downcase.start_with?('//') && href.downcase !~ %r{^.{1,6}://.+?}
           node['href'] = "http://#{node['href']}"
           href = node['href']
         end
@@ -145,9 +145,9 @@ satinize html string based on whiltelist
       # check if href is different to text
       if external && node.name == 'a' && !url_same?(node['href'], node.text)
         if node['href'].blank?
-          node.replace strict(node.children.to_s)
+          node.replace node.children.to_s
           Loofah::Scrubber::STOP
-        elsif node.children.empty? || node.children.first.class == Nokogiri::XML::Text
+        elsif (node.children.empty? || node.children.first.class == Nokogiri::XML::Text) && node.text.present?
           text = Nokogiri::XML::Text.new("#{node['href']} (", node.document)
           node.add_previous_sibling(text)
           node['href'] = cleanup_target(node.text)
@@ -178,7 +178,17 @@ satinize html string based on whiltelist
       end
 
     end
-    Loofah.fragment(string).scrub!(scrubber).to_s
+
+    new_string = ''
+    done = true
+    while done
+      new_string = Loofah.fragment(string).scrub!(scrubber).to_s
+      if string == new_string
+        done = false
+      end
+      string = new_string
+    end
+    string
   end
 
 =begin
@@ -253,7 +263,6 @@ cleanup html string:
         Loofah::Scrubber::STOP
       end
     end
-    string = Loofah.fragment(string).scrub!(scrubber_structure).to_s
 
     new_string = ''
     done = true

test/unit/aaa_string_test.rb

@@ -552,7 +552,8 @@ Men-----------------------'
     assert_equal(result, html.html2html_strict)
 
     html   = '<div>https://www.facebook.com/test</div>'
-    result = '<div><a href="https://www.facebook.com/test" rel="nofollow noreferrer noopener" target="_blank">https://www.facebook.com/test</a>
+    result = '<div>
+<a href="https://www.facebook.com/test" rel="nofollow noreferrer noopener" target="_blank">https://www.facebook.com/test</a>
 </div>'
     assert_equal(result, html.html2html_strict)
 
@@ -640,11 +641,11 @@ Men-----------------------'
     assert_equal(result, html.html2html_strict)
 
     html   = "<div>http://example.com</div>"
-    result = "<div><a href=\"http://example.com\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">http://example.com</a>\n</div>"
+    result = "<div>\n<a href=\"http://example.com\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">http://example.com</a>\n</div>"
     assert_equal(result, html.html2html_strict)
 
     html   = "<div>http://example.com.</div>"
-    result = "<div><a href=\"http://example.com\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">http://example.com</a>.</div>"
+    result = "<div>\n<a href=\"http://example.com\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">http://example.com</a>.</div>"
     assert_equal(result, html.html2html_strict)
 
     html   = "<div>lala http://example.com.</div>"
@@ -652,11 +653,11 @@ Men-----------------------'
     assert_equal(result, html.html2html_strict)
 
     html   = "<div>http://example.com, and so on</div>"
-    result = "<div><a href=\"http://example.com\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">http://example.com</a>, and so on</div>"
+    result = "<div>\n<a href=\"http://example.com\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">http://example.com</a>, and so on</div>"
     assert_equal(result, html.html2html_strict)
 
     html   = "<div>http://example.com?lala=me, and so on</div>"
-    result = "<div><a href=\"http://example.com?lala=me\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">http://example.com?lala=me</a>, and so on</div>"
+    result = "<div>\n<a href=\"http://example.com?lala=me\" rel=\"nofollow noreferrer noopener\" target=\"_blank\">http://example.com?lala=me</a>, and so on</div>"
     assert_equal(result, html.html2html_strict)
 
     html   = "<a href=\"http://facebook.de/examplesrbog\"><span lang=\"EN-US\" style='color:blue'>http://facebook.de/examplesrbog</span></a>"

test/unit/email_parser_test.rb

@@ -244,7 +244,7 @@ Managing Director: Martin Edenhofer
       },
       {
         data: IO.binread('test/fixtures/mail8.box'),
-        body_md5: '6b2b3701aaf6b5a1c351664e7d4bab03',
+        body_md5: '166c87ab43c07301686c1c8761e98d48',
         attachments: [
           {
             md5: 'c3ca4aab222eed8a148a716371b70129',
@@ -281,7 +281,7 @@ Düsseldorfer Landstraße 395
 <br>
 D-00000 Hof
 <br>
-<a href="http://www.example.com" rel="nofollow noreferrer noopener" target="_blank"><u><a href="http://www.example.com" rel="nofollow noreferrer noopener" target="_blank">http://www.example.com</a></u></a> <br>
+<a href="http://www.example.com" rel="nofollow noreferrer noopener" target="_blank"><u>www.example.com</u></a> <br>
 <br> <hr>
 <br>
 Geschäftsführung/Management Board: Jan Bauer (Vorsitzender/Chairman), Oliver Bauer, Heiko Bauer, Boudewijn Bauer
@@ -552,7 +552,7 @@ Newsletter abbestellen (<a href="http://newsletters.cylex.de/ref/www.cylex.de/si
       },
       {
         data: IO.binread('test/fixtures/mail19.box'),
-        body_md5: '2e162549ffb5c7832c7be0d6538e8aa1',
+        body_md5: '29a8a50c2931346296f8b8fe782e115c',
         params: {
           from: '"我" <>',
           from_email: '"我" <>',

test/unit/email_process_test.rb

@@ -2291,6 +2291,31 @@ Some Text',
           ],
         },
       },
+      {
+        data: IO.binread('test/fixtures/mail49.box'),
+        success: true,
+        result: {
+          0 => {
+            priority: '2 normal',
+            title: 'Kinderschwimmbrille ABC Little Twist: Schnell angelegt, keine verhedderten Haare (Pressemitteilung)',
+          },
+          1 => {
+            from: '"Marcus Smith (ABC)" <marcus.smith@example.com>',
+            sender: 'Customer',
+            type: 'email',
+          },
+        },
+        verify: {
+          users: [
+            {
+              firstname: 'Marcus',
+              lastname: 'Smith',
+              fullname: 'Marcus Smith',
+              email: 'marcus.smith@example.com',
+            },
+          ],
+        },
+      },
     ]
     assert_process(files)
   end

test/unit/html_sanitizer_test.rb

@@ -48,7 +48,7 @@ class HtmlSanitizerTest < ActiveSupport::TestCase
     assert_equal(HtmlSanitizer.strict('<DIV STYLE="background-image: url(javascript:alert(\'XSS\'), \'\')">'), '<div></div>')
     assert_equal(HtmlSanitizer.strict('<a href="/some/path">test</a>'), '<a href="/some/path">test</a>')
     assert_equal(HtmlSanitizer.strict('<a href="https://some/path">test</a>'), '<a href="https://some/path" rel="nofollow noreferrer noopener" target="_blank">test</a>')
-    assert_equal(HtmlSanitizer.strict('<a href="https://some/path">test</a>', true), 'https://some/path (<a href="test" rel="nofollow noreferrer noopener" target="_blank">test</a>)')
+    assert_equal(HtmlSanitizer.strict('<a href="https://some/path">test</a>', true), '<a href="https://some/path" rel="nofollow noreferrer noopener" target="_blank">https://some/path</a> (<a href="http://test" rel="nofollow noreferrer noopener" target="_blank">test</a>)')
     assert_equal(HtmlSanitizer.strict('<XML ID="xss"><I><B><IMG SRC="javas<!-- -->cript:alert(\'XSS\')"></B></I></XML>'), '<i><b></b></i>')
     assert_equal(HtmlSanitizer.strict('<IMG SRC="javas<!-- -->cript:alert(\'XSS\')">'), '')
     assert_equal(HtmlSanitizer.strict(' <HEAD><META HTTP-EQUIV="CONTENT-TYPE" CONTENT="text/html; charset=UTF-7"> </HEAD>+ADw-SCRIPT+AD4-alert(\'XSS\');+ADw-/SCRIPT+AD4-'), '  +ADw-SCRIPT+AD4-alert(\'XSS\');+ADw-/SCRIPT+AD4-')
@@ -56,9 +56,9 @@ class HtmlSanitizerTest < ActiveSupport::TestCase
     assert_equal(HtmlSanitizer.strict('<A HREF="h
 tt  p://6 6.000146.0x7.147/">XSS</A>'), '<a href="http://66.000146.0x7.147/" rel="nofollow noreferrer noopener" target="_blank">XSS</a>')
     assert_equal(HtmlSanitizer.strict('<A HREF="h
-tt  p://6 6.000146.0x7.147/">XSS</A>', true), 'http://66.000146.0x7.147/ (<a href="XSS" rel="nofollow noreferrer noopener" target="_blank">XSS</a>)')
+tt  p://6 6.000146.0x7.147/">XSS</A>', true), '<a href="http://66.000146.0x7.147/" rel="nofollow noreferrer noopener" target="_blank">http://66.000146.0x7.147/</a> (<a href="http://XSS" rel="nofollow noreferrer noopener" target="_blank">XSS</a>)')
     assert_equal(HtmlSanitizer.strict('<A HREF="//www.google.com/">XSS</A>'), '<a href="//www.google.com/" rel="nofollow noreferrer noopener" target="_blank">XSS</a>')
-    assert_equal(HtmlSanitizer.strict('<A HREF="//www.google.com/">XSS</A>', true), '//www.google.com/ (<a href="XSS" rel="nofollow noreferrer noopener" target="_blank">XSS</a>)')
+    assert_equal(HtmlSanitizer.strict('<A HREF="//www.google.com/">XSS</A>', true), '//www.google.com/ (<a href="http://XSS" rel="nofollow noreferrer noopener" target="_blank">XSS</a>)')
     assert_equal(HtmlSanitizer.strict('<form id="test"></form><button form="test" formaction="javascript:alert(1)">X</button>'), 'X')
     assert_equal(HtmlSanitizer.strict('<maction actiontype="statusline#http://google.com" xlink:href="javascript:alert(2)">CLICKME</maction>'), 'CLICKME')
     assert_equal(HtmlSanitizer.strict('<a xlink:href="javascript:alert(2)">CLICKME</a>'), '<a>CLICKME</a>')
@@ -74,8 +74,7 @@ tt  p://6 6.000146.0x7.147/">XSS</A>', true), 'http://66.000146.0x7.147/ (<a hre
     assert_equal(HtmlSanitizer.strict('<a href="[a]java[b]script[c]:alert(1)">XXX</a>', true), 'XXX')
     assert_equal(HtmlSanitizer.strict('<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>'), 'alert(1)')
     assert_equal(HtmlSanitizer.strict('<a style="position:fixed;top:0;left:0;width: 260px;height:100vh;background-color:red;display: block;" href="http://example.com"></a>'), '<a href="http://example.com" rel="nofollow noreferrer noopener" target="_blank"></a>')
-    assert_equal(HtmlSanitizer.strict('<a style="position:fixed;top:0;left:0;width: 260px;height:100vh;background-color:red;display: block;" href="http://example.com"></a>', true), 'http://example.com (<a href="" rel="nofollow noreferrer noopener" target="_blank"></a>)')
+    assert_equal(HtmlSanitizer.strict('<a style="position:fixed;top:0;left:0;width: 260px;height:100vh;background-color:red;display: block;" href="http://example.com"></a>', true), '<a href="http://example.com" rel="nofollow noreferrer noopener" target="_blank">http://example.com</a>')
-
   end
 
 end

test/unit/user_test.rb

@@ -254,16 +254,21 @@ class UserTest < ActiveSupport::TestCase
 
       test[:create_verify].each { |key, value|
         next if key == :image_md5
-        if user.respond_to?( key )
+        if user.respond_to?(key)
-          assert_equal( value, user.send(key), "create check #{key} in (#{test[:name]})"  )
+          result = user.send(key)
+          if value.nil?
+            assert_nil(result, "create check #{key} in (#{test[:name]})")
+          else
+            assert_equal(value, result, "create check #{key} in (#{test[:name]})")
+          end
         else
-          assert_equal( value, user[key], "create check #{key} in (#{test[:name]})" )
+          assert_equal(value, user[key], "create check #{key} in (#{test[:name]})")
         end
       }
       if test[:create_verify][:image_md5]
         file = Avatar.get_by_hash( user.image )
         file_md5 = Digest::MD5.hexdigest( file.content )
-        assert_equal( test[:create_verify][:image_md5], file_md5, "create avatar md5 check in (#{test[:name]})"  )
+        assert_equal(test[:create_verify][:image_md5], file_md5, "create avatar md5 check in (#{test[:name]})")
       end
       if test[:update]
         user.update_attributes( test[:update] )
@@ -271,16 +276,16 @@ class UserTest < ActiveSupport::TestCase
         test[:update_verify].each { |key, value|
           next if key == :image_md5
           if user.respond_to?( key )
-            assert_equal( value, user.send(key), "update check #{key} in (#{test[:name]})"  )
+            assert_equal(value, user.send(key), "update check #{key} in (#{test[:name]})")
           else
-            assert_equal( value, user[key], "update check #{key} in (#{test[:name]})"  )
+            assert_equal(value, user[key], "update check #{key} in (#{test[:name]})")
           end
         }
 
         if test[:update_verify][:image_md5]
           file = Avatar.get_by_hash( user.image )
           file_md5 = Digest::MD5.hexdigest( file.content )
-          assert_equal( test[:update_verify][:image_md5], file_md5, "update avatar md5 check in (#{test[:name]})"  )
+          assert_equal( test[:update_verify][:image_md5], file_md5, "update avatar md5 check in (#{test[:name]})")
         end
       end