Merge remote-tracking branch 'dev/13-10-stable' into 13-10-stable

CHANGELOG.md

@@ -2,6 +2,28 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 13.10.1 (2021-03-31)
+
+### Security (6 changes)
+
+- Leave pool repository on fork unlinking.
+- Fixed XSS in merge requests sidebar.
+- Fix arbitrary read/write in AsciiDoctor and Kroki gems.
+- Prevent infinite loop when checking if collaboration is allowed.
+- Disable arbitrary URI and file reads in JSON validator.
+- Require POST request to trigger system hooks.
+
+### Removed (1 change)
+
+- Make HipChat project service do nothing. !57434
+
+### Other (3 changes)
+
+- Remove direct mimemagic dependency. !57387
+- Refactor MimeMagic calls to new MimeType class. !57421
+- Switch to using a fake mimemagic gem. !57443
+
+
 ## 13.10.0 (2021-03-22)
 
 ### Security (3 changes)

GITALY_SERVER_VERSION

-13.10.0
\ No newline at end of file
+13.10.1
\ No newline at end of file

VERSION

-13.10.0
\ No newline at end of file
+13.10.1
\ No newline at end of file

app/models/merge_request.rb

@@ -1337,8 +1337,8 @@ class MergeRequest < ApplicationRecord
     has_no_commits? || branch_missing? || cannot_be_merged?
   end
 
-  def can_be_merged_by?(user)
-    access = ::Gitlab::UserAccess.new(user, container: project)
+  def can_be_merged_by?(user, skip_collaboration_check: false)
+    access = ::Gitlab::UserAccess.new(user, container: project, skip_collaboration_check: skip_collaboration_check)
     access.can_update_branch?(target_branch)
   end
 

app/models/project.rb

@@ -2704,7 +2704,7 @@ class Project < ApplicationRecord
       # Issue for N+1: https://gitlab.com/gitlab-org/gitlab-foss/issues/49322
       Gitlab::GitalyClient.allow_n_plus_1_calls do
         merge_requests_allowing_collaboration(branch_name).any? do |merge_request|
-          merge_request.can_be_merged_by?(user)
+          merge_request.can_be_merged_by?(user, skip_collaboration_check: true)
         end
       end
     end

app/services/projects/unlink_fork_service.rb

@@ -32,6 +32,8 @@ module Projects
         if fork_network = @project.root_of_fork_network
           fork_network.update(root_project: nil, deleted_root_project_name: @project.full_name)
         end
+
+        @project.leave_pool_repository
       end
 
       # rubocop: disable Cop/InBatches

app/views/shared/issuable/_sidebar.html.haml

@@ -138,7 +138,7 @@
               = clipboard_button(text: source_branch, title: _('Copy branch name'), placement: "left", boundary: 'viewport')
             .sidebar-mr-source-branch.hide-collapsed
               %span
-                = _('Source branch: %{source_branch_open}%{source_branch}%{source_branch_close}').html_safe % { source_branch_open: "<cite class='ref-name' title='#{source_branch}'>".html_safe, source_branch_close: "</cite>".html_safe, source_branch: source_branch }
+                = _('Source branch: %{source_branch_open}%{source_branch}%{source_branch_close}').html_safe % { source_branch_open: "<cite class='ref-name' title='#{html_escape(source_branch)}'>".html_safe, source_branch_close: "</cite>".html_safe, source_branch: html_escape(source_branch) }
               = clipboard_button(text: source_branch, title: _('Copy branch name'), placement: "left", boundary: 'viewport')
 
       - if show_forwarding_email

changelogs/unreleased/mimemagic_shim.yml

----
-title: Switch to using a fake mimemagic gem
-merge_request: 57443
-author:
-type: other

changelogs/unreleased/remove-direct-mimemagic-dependency-minimal.yml

----
-title: Refactor MimeMagic calls to new MimeType class
-merge_request: 57421
-author:
-type: other

changelogs/unreleased/remove-direct-mimemagic-dependency.yml

----
-title: Remove direct mimemagic dependency
-merge_request: 57387
-author:
-type: other

changelogs/unreleased/remove_hipchat_gem.yml

----
-title: Make HipChat project service do nothing
-merge_request: 57434
-author:
-type: removed

config/initializers/asciidoctor_patch.rb

+# frozen_string_literal: true
+
+# Ensure that locked attributes can not be changed using a counter.
+# TODO: this can be removed once `asciidoctor` gem is > 2.0.12
+#       and https://github.com/asciidoctor/asciidoctor/issues/3939 is merged
+module Asciidoctor
+  module DocumentPatch
+    def counter(name, seed = nil)
+      return @parent_document.counter(name, seed) if @parent_document   # rubocop: disable Gitlab/ModuleWithInstanceVariables
+
+      unless attribute_locked? name
+        super
+      end
+    end
+  end
+end
+
+class Asciidoctor::Document
+  prepend Asciidoctor::DocumentPatch
+end

config/initializers/json_validator_patch.rb

+# frozen_string_literal: true
+
+# This patches https://github.com/ruby-json-schema/json-schema/blob/765e6d8fdbfdaca1a42fa743f4621e757f9f6a03/lib/json-schema/validator.rb
+# to address https://github.com/ruby-json-schema/json-schema/issues/148.
+require 'json-schema'
+
+module JSON
+  class Validator
+    def initialize_data(data)
+      if @options[:parse_data]
+        if @options[:json]
+          data = self.class.parse(data)
+        elsif @options[:uri]
+          json_uri = Util::URI.normalized_uri(data)
+          data = self.class.parse(custom_open(json_uri))
+        elsif data.is_a?(String)
+          begin
+            data = self.class.parse(data)
+          rescue JSON::Schema::JsonParseError
+            # Silently discard the error - use the data as-is
+          end
+        end
+      end
+
+      JSON::Schema.stringify(data)
+    end
+  end
+end

doc/api/system_hooks.md

@@ -88,7 +88,7 @@ Example response:
 ## Test system hook
 
 ```plaintext
-GET /hooks/:id
+POST /hooks/:id
 ```
 
 | Attribute | Type | Required | Description |
@@ -98,7 +98,7 @@ GET /hooks/:id
 Example request:
 
 ```shell
-curl --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/hooks/2"
+curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" "https://gitlab.example.com/api/v4/hooks/1"
 ```
 
 Example response:

lib/api/system_hooks.rb

@@ -47,7 +47,7 @@ module API
       params do
         requires :id, type: Integer, desc: 'The ID of the system hook'
       end
-      get ":id" do
+      post ":id" do
         hook = SystemHook.find(params[:id])
         data = {
           event_name: "project_create",

lib/gitlab/markdown_cache.rb

@@ -3,7 +3,7 @@
 module Gitlab
   module MarkdownCache
     # Increment this number every time the renderer changes its output
-    CACHE_COMMONMARK_VERSION        = 26
+    CACHE_COMMONMARK_VERSION        = 27
     CACHE_COMMONMARK_VERSION_START  = 10
 
     BaseError = Class.new(StandardError)

lib/gitlab/user_access.rb

@@ -11,10 +11,11 @@ module Gitlab
     attr_reader :user, :push_ability
     attr_accessor :container
 
-    def initialize(user, container: nil, push_ability: :push_code)
+    def initialize(user, container: nil, push_ability: :push_code, skip_collaboration_check: false)
       @user = user
       @container = container
       @push_ability = push_ability
+      @skip_collaboration_check = skip_collaboration_check
     end
 
     def can_do_action?(action)
@@ -87,6 +88,8 @@ module Gitlab
 
     private
 
+    attr_reader :skip_collaboration_check
+
     def can_push?
       user.can?(push_ability, container)
     end
@@ -98,6 +101,8 @@ module Gitlab
     end
 
     def branch_allows_collaboration_for?(ref)
+      return false if skip_collaboration_check
+
       # Checking for an internal project or group to prevent an infinite loop:
       # https://gitlab.com/gitlab-org/gitlab/issues/36805
       (!project.internal? && project.branch_allows_collaboration?(user, ref))

spec/factories/pool_repositories.rb

@@ -6,7 +6,7 @@ FactoryBot.define do
     state { :none }
 
     before(:create) do |pool|
-      pool.source_project = create(:project, :repository)
+      pool.source_project ||= create(:project, :repository)
       pool.source_project.update!(pool_repository: pool)
     end
 

spec/features/merge_request/user_views_open_merge_request_spec.rb

@@ -111,4 +111,21 @@ RSpec.describe 'User views an open merge request' do
       end
     end
   end
+
+  context 'XSS source branch' do
+    let(:project) { create(:project, :public, :repository) }
+    let(:source_branch) { "'><iframe/srcdoc=''></iframe>" }
+
+    before do
+      project.repository.create_branch(source_branch, "master")
+
+      mr = create(:merge_request, source_project: project, target_project: project, source_branch: source_branch)
+
+      visit(merge_request_path(mr))
+    end
+
+    it 'encodes branch name' do
+      expect(find('cite.ref-name')[:title]).to eq(source_branch)
+    end
+  end
 end

spec/initializers/json_validator_patch_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'rspec-parameterized'
+
+RSpec.describe 'JSON validator patch' do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:schema) { '{"format": "string"}' }
+
+  subject { JSON::Validator.validate(schema, data) }
+
+  context 'with invalid JSON' do
+    where(:data) do
+      [
+        'https://example.com',
+        '/tmp/test.txt'
+      ]
+    end
+
+    with_them do
+      it 'does not attempt to open a file or URI' do
+        allow(File).to receive(:read).and_call_original
+        allow(URI).to receive(:open).and_call_original
+        expect(File).not_to receive(:read).with(data)
+        expect(URI).not_to receive(:open).with(data)
+        expect(subject).to be true
+      end
+    end
+  end
+
+  context 'with valid JSON' do
+    let(:data) { %({ 'somekey': 'value' }) }
+
+    it 'validates successfully' do
+      expect(subject).to be true
+    end
+  end
+end

spec/lib/gitlab/asciidoc_spec.rb

@@ -92,6 +92,15 @@ module Gitlab
             expect(render(data[:input], context)).to include(data[:output])
           end
         end
+
+        it 'does not allow locked attributes to be overridden' do
+          input = <<~ADOC
+            {counter:max-include-depth:1234}
+            <|-- {max-include-depth}
+          ADOC
+
+          expect(render(input, {})).not_to include('1234')
+        end
       end
 
       context "images" do
@@ -543,6 +552,40 @@ module Gitlab
 
           expect(render(input, context)).to include(output.strip)
         end
+
+        it 'does not allow kroki-plantuml-include to be overridden' do
+          input = <<~ADOC
+            [plantuml, test="{counter:kroki-plantuml-include:/etc/passwd}", format="png"]
+            ....
+            class BlockProcessor
+
+            BlockProcessor <|-- {counter:kroki-plantuml-include}
+            ....
+          ADOC
+
+          output = <<~HTML
+            <div>
+            <div>
+            <a class=\"no-attachment-icon\" href=\"https://kroki.io/plantuml/png/eNpLzkksLlZwyslPzg4oyk9OLS7OL-LiQuUr2NTo6ipUJ-eX5pWkFlllF-VnZ-oW5CTmlZTm5uhm5iXnlKak1gIABQEb8A==\" target=\"_blank\" rel=\"noopener noreferrer\"><img src=\"data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==\" alt=\"Diagram\" class=\"lazy\" data-src=\"https://kroki.io/plantuml/png/eNpLzkksLlZwyslPzg4oyk9OLS7OL-LiQuUr2NTo6ipUJ-eX5pWkFlllF-VnZ-oW5CTmlZTm5uhm5iXnlKak1gIABQEb8A==\"></a>
+            </div>
+            </div>
+          HTML
+
+          expect(render(input, {})).to include(output.strip)
+        end
+
+        it 'does not allow kroki-server-url to be overridden' do
+          input = <<~ADOC
+            [plantuml, test="{counter:kroki-server-url:evilsite}", format="png"]
+            ....
+            class BlockProcessor
+
+            BlockProcessor
+            ....
+          ADOC
+
+          expect(render(input, {})).not_to include('evilsite')
+        end
       end
 
       context 'with Kroki and BlockDiag (additional format) enabled' do

spec/lib/gitlab/user_access_spec.rb

@@ -216,6 +216,15 @@ RSpec.describe Gitlab::UserAccess do
         expect(access.can_merge_to_branch?(@branch.name)).to be_falsey
       end
     end
+
+    context 'when skip_collaboration_check is true' do
+      let(:access) { described_class.new(user, container: project, skip_collaboration_check: true) }
+
+      it 'does not call Project#branch_allows_collaboration?' do
+        expect(project).not_to receive(:branch_allows_collaboration?)
+        expect(access.can_push_to_branch?('master')).to be_falsey
+      end
+    end
   end
 
   describe '#can_create_tag?' do

spec/models/project_spec.rb

@@ -5291,6 +5291,64 @@ RSpec.describe Project, factory_default: :keep do
     end
   end
 
+  describe '#branch_allows_collaboration?' do
+    context 'when there are open merge requests that have their source/target branches point to each other' do
+      let_it_be(:project) { create(:project, :repository) }
+      let_it_be(:developer) { create(:user) }
+      let_it_be(:reporter) { create(:user) }
+      let_it_be(:guest) { create(:user) }
+
+      before_all do
+        create(
+          :merge_request,
+          target_project: project,
+          target_branch: 'master',
+          source_project: project,
+          source_branch: 'merge-test',
+          allow_collaboration: true
+        )
+
+        create(
+          :merge_request,
+          target_project: project,
+          target_branch: 'merge-test',
+          source_project: project,
+          source_branch: 'master',
+          allow_collaboration: true
+        )
+
+        project.add_developer(developer)
+        project.add_reporter(reporter)
+        project.add_guest(guest)
+      end
+
+      shared_examples_for 'successful check' do
+        it 'does not go into an infinite loop' do
+          expect { project.branch_allows_collaboration?(user, 'master') }
+            .not_to raise_error
+        end
+      end
+
+      context 'when user is a developer' do
+        let(:user) { developer }
+
+        it_behaves_like 'successful check'
+      end
+
+      context 'when user is a reporter' do
+        let(:user) { reporter }
+
+        it_behaves_like 'successful check'
+      end
+
+      context 'when user is a guest' do
+        let(:user) { guest }
+
+        it_behaves_like 'successful check'
+      end
+    end
+  end
+
   context 'with cross project merge requests' do
     let(:user) { create(:user) }
     let(:target_project) { create(:project, :repository) }

spec/requests/api/system_hooks_spec.rb

@@ -103,15 +103,15 @@ RSpec.describe API::SystemHooks do
     end
   end
 
-  describe "GET /hooks/:id" do
-    it "returns hook by id" do
-      get api("/hooks/#{hook.id}", admin)
-      expect(response).to have_gitlab_http_status(:ok)
+  describe 'POST /hooks/:id' do
+    it "returns and trigger hook by id" do
+      post api("/hooks/#{hook.id}", admin)
+      expect(response).to have_gitlab_http_status(:created)
       expect(json_response['event_name']).to eq('project_create')
     end
 
     it "returns 404 on failure" do
-      get api("/hooks/404", admin)
+      post api("/hooks/404", admin)
       expect(response).to have_gitlab_http_status(:not_found)
     end
   end

spec/services/projects/fork_service_spec.rb

@@ -403,7 +403,7 @@ RSpec.describe Projects::ForkService do
   end
 
   context 'when forking with object pools' do
-    let(:fork_from_project) { create(:project, :public) }
+    let(:fork_from_project) { create(:project, :repository, :public) }
     let(:forker) { create(:user) }
 
     context 'when no pool exists' do

spec/services/projects/unlink_fork_service_spec.rb

@@ -207,6 +207,17 @@ RSpec.describe Projects::UnlinkForkService, :use_clean_rails_memory_store_cachin
     end
   end
 
+  context 'a project with pool repository' do
+    let(:project) { create(:project, :public, :repository) }
+    let!(:pool_repository) { create(:pool_repository, :ready, source_project: project) }
+
+    subject { described_class.new(project, user) }
+
+    it 'when unlinked leaves pool repository' do
+      expect { subject.execute }.to change { project.reload.has_pool_repository? }.from(true).to(false)
+    end
+  end
+
   context 'when given project is not part of a fork network' do
     let!(:project_without_forks) { create(:project, :public) }