Merge 'dev/8-17-stable' into 8-17-stable

VERSION

-8.17.0-rc1
+8.17.0-rc2

app/uploaders/file_uploader.rb

@@ -36,7 +36,7 @@ class FileUploader < GitlabUploader
     escaped_filename = filename.gsub("]", "\\]")
 
     markdown = "[#{escaped_filename}](#{self.secure_url})"
-    markdown.prepend("!") if image_or_video?
+    markdown.prepend("!") if image_or_video? || dangerous?
 
     {
       alt:      filename,

app/uploaders/uploader_helper.rb

 # Extra methods for uploader
 module UploaderHelper
-  IMAGE_EXT = %w[png jpg jpeg gif bmp tiff svg]
+  IMAGE_EXT = %w[png jpg jpeg gif bmp tiff]
   # We recommend using the .mp4 format over .mov. Videos in .mov format can
   # still be used but you really need to make sure they are served with the
   # proper MIME type video/mp4 and not video/quicktime or your videos won't play
   # on IE >= 9.
   # http://archive.sublimevideo.info/20150912/docs.sublimevideo.net/troubleshooting.html
   VIDEO_EXT = %w[mp4 m4v mov webm ogv]
+  # These extension types can contain dangerous code and should only be embedded inline with
+  # proper filtering. They should always be tagged as "Content-Disposition: attachment", not "inline".
+  DANGEROUS_EXT = %w[svg]
 
   def image?
     extension_match?(IMAGE_EXT)
@@ -20,6 +23,10 @@ module UploaderHelper
     image? || video?
   end
 
+  def dangerous?
+    extension_match?(DANGEROUS_EXT)
+  end
+
   def extension_match?(extensions)
     return false unless file
 

changelogs/unreleased/asciidocs-xss-patch.yml

+---
+title: Patch Asciidocs rendering to block XSS
+merge_request:
+author:

changelogs/unreleased/fix-xss-svg.yml

+---
+title: Fix XSS vulnerability in SVG attachments
+merge_request:
+author:

changelogs/unreleased/labels-assigned-to-wrong-project.yml

+---
+title: Prevent the GitHub importer from assigning labels and comments to merge requests or issues belonging to other projects.
+merge_request:
+author:

changelogs/unreleased/patch-rdoc-xss.yml

+---
+title: Patch XSS vulnerability in RDOC support
+merge_request:
+author:

db/migrate/20160519203051_add_developers_can_merge_to_protected_branches.rb

 class AddDevelopersCanMergeToProtectedBranches < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
+  DOWNTIME = false
+
   disable_ddl_transaction!
 
   def change

lib/gitlab/asciidoc.rb

@@ -36,6 +36,9 @@ module Gitlab
 
       html = Banzai.post_process(html, context)
 
+      filter = Banzai::Filter::SanitizationFilter.new(html)
+      html = filter.call.to_s
+
       html.html_safe
     end
 

lib/gitlab/github_import/importer.rb

@@ -115,7 +115,7 @@ module Gitlab
             begin
               issuable =
                 if gh_issue.pull_request?
-                  MergeRequest.find_by_iid(gh_issue.number)
+                  MergeRequest.find_by(target_project_id: project.id, iid: gh_issue.number)
                 else
                   gh_issue.create!
                 end
@@ -212,8 +212,12 @@ module Gitlab
               comment         = CommentFormatter.new(project, raw)
               # GH does not return info about comment's parent, so we guess it by checking its URL!
               *_, parent, iid = URI(raw.html_url).path.split('/')
-              issuable_class = parent == 'issues' ? Issue : MergeRequest
-              issuable       = issuable_class.find_by_iid(iid)
+              if parent == 'issues'
+                issuable = Issue.find_by(project_id: project.id, iid: iid)
+              else
+                issuable = MergeRequest.find_by(target_project_id: project.id, iid: iid)
+              end
+
               next unless issuable
 
               issuable.notes.create!(comment.attributes)

lib/gitlab/other_markup.rb

@@ -17,6 +17,9 @@ module Gitlab
 
       html = Banzai.post_process(html, context)
 
+      filter = Banzai::Filter::SanitizationFilter.new(html)
+      html = filter.call.to_s
+
       html.html_safe
     end
   end

spec/controllers/uploads_controller_spec.rb

@@ -4,6 +4,28 @@ describe UploadsController do
   let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
 
   describe "GET show" do
+    context 'Content-Disposition security measures' do
+      let(:project) { create(:empty_project, :public) }
+
+      context 'for PNG files' do
+        it 'returns Content-Disposition: inline' do
+          note = create(:note, :with_attachment, project: project)
+          get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.png'
+
+          expect(response['Content-Disposition']).to start_with('inline;')
+        end
+      end
+
+      context 'for SVG files' do
+        it 'returns Content-Disposition: attachment' do
+          note = create(:note, :with_svg_attachment, project: project)
+          get :show, model: 'note', mounted_as: 'attachment', id: note.id, filename: 'image.svg'
+
+          expect(response['Content-Disposition']).to start_with('attachment;')
+        end
+      end
+    end
+
     context "when viewing a user avatar" do
       context "when signed in" do
         before do

spec/factories/notes.rb

@@ -97,7 +97,11 @@ FactoryGirl.define do
     end
 
     trait :with_attachment do
-      attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "`/png") }
+      attachment { fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png") }
+    end
+
+    trait :with_svg_attachment do
+      attachment { fixture_file_upload(Rails.root + "spec/fixtures/unsanitized.svg", "image/svg+xml") }
     end
   end
 end

spec/lib/gitlab/asciidoc_spec.rb

@@ -41,6 +41,29 @@ module Gitlab
           render(input, context, asciidoc_opts)
         end
       end
+      
+      context "XSS" do
+        links = {
+          'links' => {
+            input: 'link:mylink"onmouseover="alert(1)[Click Here]',
+            output: "<div>\n<p><a href=\"mylink\">Click Here</a></p>\n</div>"
+          },
+          'images' => {
+            input: 'image:https://localhost.com/image.png[Alt text" onerror="alert(7)]',
+            output: "<div>\n<p><span><img src=\"https://localhost.com/image.png\" alt=\"Alt text\"></span></p>\n</div>"
+          },
+          'pre' => {
+            input: '```mypre"><script>alert(3)</script>',
+            output: "<div>\n<div>\n<pre lang=\"mypre\">\"&gt;<code></code></pre>\n</div>\n</div>"
+          }
+        }
+
+        links.each do |name, data|
+          it "does not convert dangerous #{name} into HTML" do
+            expect(render(data[:input], context)).to eql data[:output]
+          end
+        end
+      end
     end
 
     def render(*args)

spec/lib/gitlab/other_markup.rb

+require 'spec_helper'
+
+describe Gitlab::OtherMarkup, lib: true do
+  context "XSS Checks" do
+    links = {
+      'links' => {
+        file: 'file.rdoc',
+        input: 'XSS[JaVaScriPt:alert(1)]',
+        output: '<p><a>XSS</a></p>'
+      }
+    }
+    links.each do |name, data|
+      it "does not convert dangerous #{name} into HTML" do
+        expect(render(data[:file], data[:input], context)).to eql data[:output]
+      end
+    end
+  end
+
+  def render(*args)
+    described_class.render(*args)
+  end
+end