Merge remote-tracking branch 'origin/master' into git-raw-workhorse

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
@@ -14,7 +15,9 @@ v 8.5.0 (unreleased)
   - Display 404 error on group not found
   - Track project import failure
   - Fix visibility level text in admin area (Zeger-Jan van de Weg)
+  - Warn admin during OAuth of granting admin rights (Zeger-Jan van de Weg)
   - Update the ExternalIssue regex pattern (Blake Hitchcock)
+  - Optimized performance of finding issues to be closed by a merge request
   - Revert "Add IP check against DNSBLs at account sign-up"
   - Deprecate API "merge_request/:merge_request_id/comments". Use "merge_requests/:merge_request_id/notes" instead
   - Deprecate API "merge_request/:merge_request_id/...". Use "merge_requests/:merge_request_id/..." instead
@@ -46,7 +49,6 @@ v 8.4.1
     and Nokogiri (1.6.7.2)
   - Fix redirect loop during import
   - Fix diff highlighting for all syntax themes
-  - Warn admin during OAuth of granting admin rights (Zeger-Jan van de Weg)
   - Delete project and associations in a background worker
 v 8.4.0

CONTRIBUTING.md

@@ -177,6 +177,26 @@ is probably 1, adding a new Git Hook maybe 4 or 5, big features 7-9.
 issues or chunks. You can simply not set the weight of a parent issue and set
 weights to children issues.
+### Regression issues
+
+Every monthly release has a corresponding issue on the CE issue tracker to keep
+track of functionality broken by that release and any fixes that need to be
+included in a patch release (see [8.3 Regressions] as an example).
+
+As outlined in the issue description, the intended workflow is to post one note
+with a reference to an issue describing the regression, and then to update that
+note with a reference to the merge request that fixes it as it becomes available.
+
+If you're a contributor who doesn't have the required permissions to update
+other users' notes, please post a new note with a reference to both the issue
+and the merge request.
+
+The release manager will [update the notes] in the regression issue as fixes are
+addressed.
+
+[8.3 Regressions]: https://gitlab.com/gitlab-org/gitlab-ce/issues/4127
+[update the notes]: https://gitlab.com/gitlab-org/release-tools/blob/master/doc/pro-tips.md#update-the-regression-issue
+
 ## Merge requests
 We welcome merge requests with fixes and improvements to GitLab code, tests,

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'

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)

app/assets/javascripts/behaviors/autosize.js.coffee

+#= require jquery.ba-resize
 #= require autosize
 $ ->
-  autosize($('.js-autosize'))
+  $fields = $('.js-autosize')
+
+  $fields.on 'autosize:resized', ->
+    $field = $(@)
+    $field.data('height', $field.outerHeight())
+
+  $fields.on 'resize.autosize', ->
+    $field = $(@)
+
+    if $field.data('height') != $field.outerHeight()
+      $field.data('height', $field.outerHeight())
+      autosize.destroy($field)
+      $field.css('max-height', window.outerHeight)
+
+  autosize($fields)
+  autosize.update($fields)
+
+  $fields.css('resize', 'vertical')

app/assets/stylesheets/framework/markdown_area.scss

@@ -83,7 +83,7 @@
   background: #FFF;
   border: 1px solid #ddd;
   min-height: 140px;
-  max-height: 430px;
+  max-height: 500px;
   padding: 5px;
   box-shadow: none;
   width: 100%;

app/assets/stylesheets/pages/note_form.scss

@@ -147,7 +147,7 @@
 .edit_note {
   .markdown-area {
     min-height: 140px;
-    max-height: 430px;
+    max-height: 500px;
   }
   .note-form-actions {
     background: transparent;

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

app/mailers/email_rejection_mailer.rb

@@ -10,7 +10,7 @@ class EmailRejectionMailer < BaseMailer
       subject: "[Rejected] #{@original_message.subject}"
     }
-    headers['Message-ID'] = SecureRandom.hex
+    headers['Message-ID'] = "<#{SecureRandom.hex}@#{Gitlab.config.gitlab.host}>"
     headers['In-Reply-To'] = @original_message.message_id
     headers['References'] = @original_message.message_id

app/models/merge_request.rb

@@ -346,10 +346,10 @@ class MergeRequest < ActiveRecord::Base
   # Return the set of issues that will be closed if this merge request is accepted.
   def closes_issues(current_user = self.author)
     if target_branch == project.default_branch
-      issues = commits.flat_map { |c| c.closes_issues(current_user) }
-      issues.push(*Gitlab::ClosingIssueExtractor.new(project, current_user).
-                  closed_by_message(description))
-      issues.uniq(&:id)
+      messages = commits.map(&:safe_message) << description
+
+      Gitlab::ClosingIssueExtractor.new(project, current_user).
+        closed_by_message(messages.join("\n"))
     else
       []
     end

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

config/initializers/metrics.rb

@@ -49,12 +49,14 @@ if Gitlab::Metrics.enabled?
     config.instrument_instance_methods(Gitlab::Shell)
     config.instrument_methods(Gitlab::Git)
-    config.instrument_instance_methods(Gitlab::Git::Repository)
     Gitlab::Git.constants.each do |name|
       const = Gitlab::Git.const_get(name)
-      config.instrument_methods(const) if const.is_a?(Module)
+      next unless const.is_a?(Module)
+
+      config.instrument_methods(const)
+      config.instrument_instance_methods(const)
     end
     Dir[Rails.root.join('app', 'finders', '*.rb')].each do |path|
@@ -62,6 +64,16 @@ if Gitlab::Metrics.enabled?
       config.instrument_instance_methods(const)
     end
+
+    [
+      :Blame, :Branch, :BranchCollection, :Blob, :Commit, :Diff, :Repository,
+      :Tag, :TagCollection, :Tree
+    ].each do |name|
+      const = Rugged.const_get(name)
+
+      config.instrument_methods(const)
+      config.instrument_instance_methods(const)
+    end
   end
   GC::Profiler.enable

doc/ci/api/projects.md

@@ -18,7 +18,7 @@ GET /ci/projects
 Returns:
 ```json
-    [
+[
   {
     "id" : 271,
     "name" : "gitlabhq",

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

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

lib/gitlab/regex.rb

@@ -44,19 +44,19 @@ module Gitlab
     def file_name_regex
-      @file_name_regex ||= /\A[a-zA-Z0-9_\-\.]*\z/.freeze
+      @file_name_regex ||= /\A[a-zA-Z0-9_\-\.\@]*\z/.freeze
     end
     def file_name_regex_message
-      "can contain only letters, digits, '_', '-' and '.'. "
+      "can contain only letters, digits, '_', '-', '@' and '.'. "
     end
     def file_path_regex
-      @file_path_regex ||= /\A[a-zA-Z0-9_\-\.\/]*\z/.freeze
+      @file_path_regex ||= /\A[a-zA-Z0-9_\-\.\/\@]*\z/.freeze
     end
     def file_path_regex_message
-      "can contain only letters, digits, '_', '-' and '.'. Separate directories with a '/'. "
+      "can contain only letters, digits, '_', '-', '@' and '.'. Separate directories with a '/'. "
     end

spec/fixtures/logo_sample.svg

+<?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>

spec/javascripts/behaviors/autosize_spec.js.coffee

+#= require behaviors/autosize
+
+describe 'Autosize behavior', ->
+  beforeEach ->
+    fixture.set('<textarea class="js-autosize" style="resize: vertical"></textarea>')
+
+  it 'does not overwrite the resize property', ->
+    load()
+    expect($('textarea')).toHaveCss(resize: 'vertical')
+
+  load = -> $(document).trigger('page:load')

spec/lib/gitlab/regex_spec.rb

@@ -21,4 +21,12 @@ describe Gitlab::Regex, lib: true do
     it { expect('Dash – is this').to match(Gitlab::Regex.project_name_regex) }
     it { expect('?gitlab').not_to match(Gitlab::Regex.project_name_regex) }
   end
+
+  describe 'file name regex' do
+    it { expect('foo@bar').to match(Gitlab::Regex.file_name_regex) }
+  end
+
+  describe 'file path regex' do
+    it { expect('foo@/bar').to match(Gitlab::Regex.file_path_regex) }
+  end
 end

spec/models/merge_request_spec.rb

@@ -137,9 +137,10 @@ describe MergeRequest, models: true do
   describe 'detection of issues to be closed' do
     let(:issue0) { create :issue, project: subject.project }
     let(:issue1) { create :issue, project: subject.project }
-    let(:commit0) { double('commit0', closes_issues: [issue0]) }
-    let(:commit1) { double('commit1', closes_issues: [issue0]) }
-    let(:commit2) { double('commit2', closes_issues: [issue1]) }
+
+    let(:commit0) { double('commit0', safe_message: "Fixes #{issue0.to_reference}") }
+    let(:commit1) { double('commit1', safe_message: "Fixes #{issue0.to_reference}") }
+    let(:commit2) { double('commit2', safe_message: "Fixes #{issue1.to_reference}") }
     before do
       allow(subject).to receive(:commits).and_return([commit0, commit1, commit2])
@@ -149,7 +150,9 @@ describe MergeRequest, models: true do
       allow(subject.project).to receive(:default_branch).
         and_return(subject.target_branch)
-      expect(subject.closes_issues).to eq([issue0, issue1].sort_by(&:id))
+      closed = subject.closes_issues
+
+      expect(closed).to include(issue0, issue1)
     end
     it 'only lists issues as to be closed if it targets the default branch' do
@@ -167,17 +170,6 @@ describe MergeRequest, models: true do
       expect(subject.closes_issues).to include(issue2)
     end
-
-    context 'for a project with JIRA integration' do
-      let(:issue0) { JiraIssue.new('JIRA-123', subject.project) }
-      let(:issue1) { JiraIssue.new('FOOBAR-4567', subject.project) }
-
-      it 'returns sorted JiraIssues' do
-        allow(subject.project).to receive_messages(default_branch: subject.target_branch)
-
-        expect(subject.closes_issues).to eq([issue0, issue1])
-      end
-    end
   end
   describe "#work_in_progress?" do