From f86ddfd36538667cd0c484a62825569a36ef2a2c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 12 Sep 2015 20:54:06 -0700 Subject: Render sanitized SVG images Closes https://github.com/gitlabhq/gitlabhq/issues/9265 --- CHANGELOG | 1 + Gemfile | 3 +++ Gemfile.lock | 1 + app/helpers/blob_helper.rb | 12 ++++++++++++ app/views/projects/blob/_blob.html.haml | 5 ++++- features/project/source/browse_files.feature | 10 ++++++++++ features/steps/project/source/browse_files.rb | 17 +++++++++++++++++ spec/fixtures/logo_sample.svg | 27 +++++++++++++++++++++++++++ 8 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 spec/fixtures/logo_sample.svg 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..13caddc44a4 100644 --- a/features/steps/project/source/browse_files.rb +++ b/features/steps/project/source/browse_files.rb @@ -351,6 +351,19 @@ 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 + expect(find('.file-content')).to have_css('img') + end + private def set_new_content @@ -410,4 +423,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 @@ + + + + Slice 1 + Created with Sketch. + + + + + + -- cgit v1.2.3