Render sanitized SVG imagesstanhu/gitlab-ce-add-svg-support

Closes https://github.com/gitlabhq/gitlabhq/issues/9265

8 files changed, 76 insertions, 1 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 58efbe2db7b..d7d07cd1c46 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.5.0 (unreleased)
   - Ensure rake tasks that don't need a DB connection can be run without one
   - Add "visibility" flag to GET /projects api endpoint
   - Ignore binary files in code search to prevent Error 500 (Stan Hu)
+  - Render sanitized SVG images (Stan Hu)
   - Upgrade gitlab_git to 7.2.23 to fix commit message mentions in first branch push
   - New UI for pagination
   - Don't prevent sign out when 2FA enforcement is enabled and user hasn't yet
diff --git a/Gemfile b/Gemfile
index a09d44f8bfd..c9d428a1798 100644
--- a/Gemfile
+++ b/Gemfile
@@ -179,6 +179,9 @@ gem "underscore-rails", "~> 1.8.0"
 gem "sanitize", '~> 2.0'
 gem 'babosa', '~> 1.0.2'
 
+# Sanitizes SVG input
+gem "loofah", "~> 2.0.3"
+
 # Protect against bruteforcing
 gem "rack-attack", '~> 4.3.1'
 
diff --git a/Gemfile.lock b/Gemfile.lock
index ec92964df25..b4f7587c419 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -953,6 +953,7 @@ DEPENDENCIES
   jquery-ui-rails (~> 5.0.0)
   kaminari (~> 0.16.3)
   letter_opener (~> 1.1.2)
+  loofah (~> 2.0.3)
   mail_room (~> 0.6.1)
   method_source (~> 0.8)
   minitest (~> 5.7.0)
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index 694c03206bd..16967927922 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -126,4 +126,16 @@ module BlobHelper
       blob.size
     end
   end
+
+  def blob_svg?(blob)
+    blob.language && blob.language.name == 'SVG'
+  end
+
+  # SVGs can contain malicious JavaScript; only include whitelisted
+  # elements and attributes. Note that this whitelist is by no means complete
+  # and may omit some elements.
+  def sanitize_svg(blob)
+    blob.data = Loofah.scrub_fragment(blob.data, :strip).to_xml
+    blob
+  end
 end
diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml
index 3d8d88834e2..2c5b8dc4356 100644
--- a/app/views/projects/blob/_blob.html.haml
+++ b/app/views/projects/blob/_blob.html.haml
@@ -35,7 +35,10 @@
     - if blob.lfs_pointer?
       = render "download", blob: blob
     - elsif blob.text?
-      = render "text", blob: blob
+      - if blob_svg?(blob)
+        = render "image", blob: sanitize_svg(blob)
+      - else
+        = render "text", blob: blob
     - elsif blob.image?
       = render "image", blob: blob
     - else
diff --git a/features/project/source/browse_files.feature b/features/project/source/browse_files.feature
index a8c276b949e..1e09dbc4c8f 100644
--- a/features/project/source/browse_files.feature
+++ b/features/project/source/browse_files.feature
@@ -320,3 +320,13 @@ Feature: Project Source Browse Files
     Then I should see download link and object size
     And I should not see lfs pointer details
     And I should see buttons for allowed commands
+
+  @javascript
+  Scenario: I preview an SVG file
+    Given I click on "Upload file" link in repo
+    And I upload a new SVG file
+    And I fill the upload file commit message
+    And I fill the new branch name
+    And I click on "Upload file"
+    Given I visit the SVG file
+    Then I can see the new rendered SVG image
diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb
index d08935aa101..54878499b28 100644
--- a/features/steps/project/source/browse_files.rb
+++ b/features/steps/project/source/browse_files.rb
@@ -351,6 +351,20 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
     expect(page).to have_content "You're not allowed to make changes to this project directly. A fork of this project has been created that you can make changes in, so you can submit a merge request."
   end
 
+  # SVG files
+  step 'I upload a new SVG file' do
+    drop_in_dropzone test_svg_file
+  end
+
+  step 'I visit the SVG file' do
+    visit namespace_project_blob_path(@project.namespace, @project, 'new_branch_name/logo_sample.svg')
+  end
+
+  step 'I can see the new rendered SVG image' do
+    # visit namespace_project_tree_path(@project.namespace, @project, "master")
+    expect(find('.file-content')).to have_css('img')
+  end
+
   private
 
   def set_new_content
@@ -410,4 +424,8 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
   def test_image_file
     File.join(Rails.root, 'spec', 'fixtures', 'banana_sample.gif')
   end
+
+  def test_svg_file
+    File.join(Rails.root, 'spec', 'fixtures', 'logo_sample.svg')
+  end
 end
diff --git a/spec/fixtures/logo_sample.svg b/spec/fixtures/logo_sample.svg
new file mode 100644
index 00000000000..883e7e6cf92
--- /dev/null
+++ b/spec/fixtures/logo_sample.svg
@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<svg width="210px" height="210px" viewBox="0 0 210 210" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xmlns:sketch="http://www.bohemiancoding.com/sketch/ns">
+    <!-- Generator: Sketch 3.3.2 (12043) - http://www.bohemiancoding.com/sketch -->
+    <title>Slice 1</title>
+    <desc>Created with Sketch.</desc>
+    <script>alert('FAIL')</script>
+    <defs></defs>
+    <g id="Page-1" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd" sketch:type="MSPage">
+        <g id="logo" sketch:type="MSLayerGroup" transform="translate(0.000000, 10.000000)">
+            <g id="Page-1" sketch:type="MSShapeGroup">
+                <g id="Fill-1-+-Group-24">
+                    <g id="Group-24">
+                        <g id="Group">
+                            <path d="M105.0614,193.655 L105.0614,193.655 L143.7014,74.734 L66.4214,74.734 L105.0614,193.655 L105.0614,193.655 Z" id="Fill-4" fill="#E24329" class="tanuki-shape"></path>
+                            <path d="M105.0614,193.6548 L66.4214,74.7338 L12.2684,74.7338 L105.0614,193.6548 L105.0614,193.6548 Z" id="Fill-8" fill="#FC6D26" class="tanuki-shape"></path>
+                            <path d="M12.2685,74.7341 L12.2685,74.7341 L0.5265,110.8731 C-0.5445,114.1691 0.6285,117.7801 3.4325,119.8171 L105.0615,193.6551 L12.2685,74.7341 L12.2685,74.7341 Z" id="Fill-12" fill="#FCA326" class="tanuki-shape"></path>
+                            <path d="M12.2685,74.7342 L66.4215,74.7342 L43.1485,3.1092 C41.9515,-0.5768 36.7375,-0.5758 35.5405,3.1092 L12.2685,74.7342 L12.2685,74.7342 Z" id="Fill-16" fill="#E24329" class="tanuki-shape"></path>
+                            <path d="M105.0614,193.6548 L143.7014,74.7338 L197.8544,74.7338 L105.0614,193.6548 L105.0614,193.6548 Z" id="Fill-18" fill="#FC6D26" class="tanuki-shape"></path>
+                            <path d="M197.8544,74.7341 L197.8544,74.7341 L209.5964,110.8731 C210.6674,114.1691 209.4944,117.7801 206.6904,119.8171 L105.0614,193.6551 L197.8544,74.7341 L197.8544,74.7341 Z" id="Fill-20" fill="#FCA326" class="tanuki-shape"></path>
+                            <path d="M197.8544,74.7342 L143.7014,74.7342 L166.9744,3.1092 C168.1714,-0.5768 173.3854,-0.5758 174.5824,3.1092 L197.8544,74.7342 L197.8544,74.7342 Z" id="Fill-22" fill="#E24329" class="tanuki-shape"></path>
+                        </g>
+                    </g>
+                </g>
+            </g>
+        </g>
+    </g>
+</svg>