Use Loofah to scrub HTML attachments

`:prune` removes unknown/unsafe tags and their contents (including their subtrees): unsafe_html = "ohai! <div>div is safe</div> <foo>but foo is <b>not</b></foo>" Loofah.fragment(unsafe_html).scrub!(:prune) # => "ohai! <div>div is safe</div> " * Adds a `DOCTYPE` to the fixture file so that Nokogiri doesn't insert a HTML 4 `DOCTYPE` automatically, making comparison in the spec uglier * Nokogiri also adds a `meta` tag to the output. Not much we can do about this: sparklemotion/nokogiri#1008
mysociety · Aug 3, 2017 · 3ddaf9a · 3ddaf9a
1 parent ed3edad
commit 3ddaf9a
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 4 deletions.
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
@@ -654,7 +654,10 @@ def get_attachment
             apply_masks(@attachment.default_body, @attachment.content_type)
 
     if content_type == 'text/html'
-      body = ActionController::Base.helpers.sanitize(body)
+      body =
+        Loofah.scrub_document(body, :prune).
+        to_html(encoding: 'UTF-8').
+        try(:html_safe)
     end
 
     render :body => body, :content_type => content_type

diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb
@@ -604,9 +604,21 @@ def get_attachment(params = {})
                            :part => 2,
                            :file_name => 'interesting.html',
                            :skip_cache => 1
-      expected =
-        "<html>\n  <head>\n  </head>\n  <body>dull\n    \n  </body>\n</html>\n"
-      expect(response.body).to eq(expected)
+
+      # Nokogiri adds the meta tag; see
+      # https://github.com/sparklemotion/nokogiri/issues/1008
+      expected = <<-EOF.squish
+      <!DOCTYPE html>
+      <html>
+        <head>
+          <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
+        </head>
+        <body>dull
+        </body>
+      </html>
+      EOF
+
+      expect(response.body.squish).to eq(expected)
     end
 
     it "censors attachments downloaded directly" do

diff --git a/spec/fixtures/files/interesting.html b/spec/fixtures/files/interesting.html
@@ -1,3 +1,4 @@
+<!DOCTYPE html>
 <html>
   <head>
   </head>
-Original file line number
+Diff line change
@@ -1,3 +1,4 @@
+    <!DOCTYPE html>
     <html>
       <head>
       </head>
@@ Expand Down @@