Add latest changes from gitlab-org/security/gitlab@12-8-stable-ee

db/post_migrate/20200204113224_schedule_recalculate_project_authorizations_second_run.rb

+# frozen_string_literal: true
+
+class ScheduleRecalculateProjectAuthorizationsSecondRun < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  MIGRATION = 'RecalculateProjectAuthorizationsWithMinMaxUserId'
+  BATCH_SIZE = 2_500
+  DELAY_INTERVAL = 2.minutes.to_i
+
+  disable_ddl_transaction!
+
+  class User < ActiveRecord::Base
+    include ::EachBatch
+
+    self.table_name = 'users'
+  end
+
+  def up
+    say "Scheduling #{MIGRATION} jobs"
+
+    User.each_batch(of: BATCH_SIZE) do |batch, index|
+      delay = index * DELAY_INTERVAL
+      range = batch.pluck('MIN(id)', 'MAX(id)').first
+      BackgroundMigrationWorker.perform_in(delay, MIGRATION, range)
+    end
+  end
+
+  def down
+  end
+end

doc/api/graphql/reference/gitlab_schema.graphql

@@ -1530,7 +1530,7 @@ type DiffRefs {
   """
   Merge base of the branch the comment was made on
   """
-  baseSha: String!
+  baseSha: String
   """
   SHA of the HEAD at the time the comment was made

doc/api/graphql/reference/gitlab_schema.json

@@ -8105,13 +8105,9 @@
               ],
               "type": {
-                "kind": "NON_NULL",
-                "name": null,
-                "ofType": {
-                  "kind": "SCALAR",
-                  "name": "String",
-                  "ofType": null
-                }
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
               },
               "isDeprecated": false,
               "deprecationReason": null

doc/api/graphql/reference/index.md

@@ -253,7 +253,7 @@ Autogenerated return type of DestroySnippet
 | Name  | Type  | Description |
 | ---   |  ---- | ----------  |
-| `baseSha` | String! | Merge base of the branch the comment was made on |
+| `baseSha` | String | Merge base of the branch the comment was made on |
 | `headSha` | String! | SHA of the HEAD at the time the comment was made |
 | `startSha` | String! | SHA of the branch being compared against |

lib/api/triggers.rb

@@ -4,6 +4,8 @@ module API
   class Triggers < Grape::API
     include PaginationParams
+    HTTP_GITLAB_EVENT_HEADER = "HTTP_#{WebHookService::GITLAB_EVENT_HEADER}".underscore.upcase
+
     params do
       requires :id, type: String, desc: 'The ID of a project'
     end
@@ -19,6 +21,8 @@ module API
       post ":id/(ref/:ref/)trigger/pipeline", requirements: { ref: /.+/ } do
         Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/42283')
+        forbidden! if gitlab_pipeline_hook_request?
+
         # validate variables
         params[:variables] = params[:variables].to_h
         unless params[:variables].all? { |key, value| key.is_a?(String) && value.is_a?(String) }
@@ -128,5 +132,11 @@ module API
         destroy_conditionally!(trigger)
       end
     end
+
+    helpers do
+      def gitlab_pipeline_hook_request?
+        request.get_header(HTTP_GITLAB_EVENT_HEADER) == WebHookService.hook_to_event(:pipeline_hooks)
+      end
+    end
   end
 end

lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # rubocop:disable Style/Documentation
+    class RecalculateProjectAuthorizationsWithMinMaxUserId
+      def perform(min_user_id, max_user_id)
+        User.where(id: min_user_id..max_user_id).find_each do |user|
+          service = Users::RefreshAuthorizedProjectsService.new(
+            user,
+            incorrect_auth_found_callback:
+              ->(project_id, access_level) do
+                logger.info(message: 'Removing ProjectAuthorizations',
+                            user_id: user.id,
+                            project_id: project_id,
+                            access_level: access_level)
+              end,
+            missing_auth_found_callback:
+              ->(project_id, access_level) do
+                logger.info(message: 'Creating ProjectAuthorizations',
+                            user_id: user.id,
+                            project_id: project_id,
+                            access_level: access_level)
+              end
+          )
+
+          service.execute
+        end
+      end
+
+      private
+
+      def logger
+        @logger ||= Gitlab::BackgroundMigration::Logger.build
+      end
+    end
+  end
+end

lib/gitlab/dependency_linker/base_linker.rb

@@ -7,6 +7,8 @@ module Gitlab
       GIT_INVALID_URL_REGEX = /^git\+#{URL_REGEX}/.freeze
       REPO_REGEX = %r{[^/'" ]+/[^/'" ]+}.freeze
+      include ActionView::Helpers::SanitizeHelper
+
       class_attribute :file_type
       def self.support?(blob_name)
@@ -62,7 +64,10 @@ module Gitlab
       end
       def link_tag(name, url)
-        %{<a href="#{ERB::Util.html_escape_once(url)}" rel="nofollow noreferrer noopener" target="_blank">#{ERB::Util.html_escape_once(name)}</a>}.html_safe
+        sanitize(
+          %{<a href="#{ERB::Util.html_escape_once(url)}" rel="nofollow noreferrer noopener" target="_blank">#{ERB::Util.html_escape_once(name)}</a>},
+          attributes: %w[href rel target]
+        )
       end
       # Links package names based on regex.

lib/gitlab/project_authorizations.rb

@@ -62,6 +62,7 @@ module Gitlab
       cte = Gitlab::SQL::RecursiveCTE.new(:namespaces_cte)
       members = Member.arel_table
       namespaces = Namespace.arel_table
+      group_group_links = GroupGroupLink.arel_table
       # Namespaces the user is a member of.
       cte << user.groups
@@ -69,7 +70,10 @@ module Gitlab
         .except(:order)
       # Namespaces shared with any of the group
-      cte << Group.select([namespaces[:id], 'group_group_links.group_access AS access_level'])
+      cte << Group.select([namespaces[:id],
+                           least(members[:access_level],
+                                 group_group_links[:group_access],
+                                 'access_level')])
                   .joins(join_group_group_links)
                   .joins(join_members_on_group_group_links)

lib/gitlab/user_access.rb

@@ -67,7 +67,13 @@ module Gitlab
       return false unless can_access_git?
       return false unless project
-      return false if !user.can?(:push_code, project) && !project.branch_allows_collaboration?(user, ref)
+      # Checking for an internal project to prevent an infinite loop:
+      # https://gitlab.com/gitlab-org/gitlab/issues/36805
+      if project.internal?
+        return false unless user.can?(:push_code, project)
+      else
+        return false if !user.can?(:push_code, project) && !project.branch_allows_collaboration?(user, ref)
+      end
       if protected?(ProtectedBranch, project, ref)
         protected_branch_accessible_to?(ref, action: :push)

spec/controllers/groups/group_links_controller_spec.rb

@@ -6,9 +6,13 @@ describe Groups::GroupLinksController do
   let(:shared_with_group) { create(:group, :private) }
   let(:shared_group) { create(:group, :private) }
   let(:user) { create(:user) }
+  let(:group_member) { create(:user) }
+  let!(:project) { create(:project, group: shared_group) }
   before do
     sign_in(user)
+
+    shared_with_group.add_developer(group_member)
   end
   describe '#create' do
@@ -40,13 +44,9 @@ describe Groups::GroupLinksController do
     end
     context 'when user has correct access to both groups' do
-      let(:group_member) { create(:user) }
-
       before do
         shared_with_group.add_developer(user)
         shared_group.add_owner(user)
-
-        shared_with_group.add_developer(group_member)
       end
       context 'when default access level is requested' do
@@ -56,6 +56,10 @@ describe Groups::GroupLinksController do
       context 'when owner access is requested' do
         let(:shared_group_access) { Gitlab::Access::OWNER }
+        before do
+          shared_with_group.add_owner(group_member)
+        end
+
         include_examples 'creates group group link'
         it 'allows admin access for group member' do
@@ -64,6 +68,10 @@ describe Groups::GroupLinksController do
         end
       end
+      it 'updates project permissions' do
+        expect { subject }.to change { group_member.can?(:read_project, project) }.from(false).to(true)
+      end
+
       context 'when shared with group id is not present' do
         let(:shared_with_group_id) { nil }
@@ -149,6 +157,7 @@ describe Groups::GroupLinksController do
     context 'when user has admin access to the shared group' do
       before do
         shared_group.add_owner(user)
+        shared_with_group.refresh_members_authorized_projects
       end
       it 'updates existing link' do
@@ -162,6 +171,10 @@ describe Groups::GroupLinksController do
         expect(link.group_access).to eq(Gitlab::Access::GUEST)
         expect(link.expires_at).to eq(expiry_date)
       end
+
+      it 'updates project permissions' do
+        expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false)
+      end
     end
     context 'when user does not have admin access to the shared group' do
@@ -199,11 +212,16 @@ describe Groups::GroupLinksController do
     context 'when user has admin access to the shared group' do
       before do
         shared_group.add_owner(user)
+        shared_with_group.refresh_members_authorized_projects
       end
       it 'deletes existing link' do
         expect { subject }.to change(GroupGroupLink, :count).by(-1)
       end
+
+      it 'updates project permissions' do
+        expect { subject }.to change { group_member.can?(:create_release, project) }.from(true).to(false)
+      end
     end
     context 'when user does not have admin access to the shared group' do

spec/frontend/error_tracking/components/error_details_spec.js

@@ -130,6 +130,28 @@ describe('ErrorDetails', () => {
       expect(wrapper.findAll('button').length).toBe(3);
     });
+    describe('unsafe chars for culprit field', () => {
+      const findReportedText = () => wrapper.find('[data-qa-selector="reported_text"]');
+      const culprit = '<script>console.log("surprise!")</script>';
+      beforeEach(() => {
+        store.state.details.loadingStacktrace = false;
+        wrapper.setData({
+          error: {
+            culprit,
+          },
+        });
+      });
+
+      it('should not convert interpolated text to html entities', () => {
+        expect(findReportedText().findAll('script').length).toEqual(0);
+        expect(findReportedText().findAll('strong').length).toEqual(1);
+      });
+
+      it('should render text instead of converting to html entities', () => {
+        expect(findReportedText().text()).toContain(culprit);
+      });
+    });
+
     describe('Badges', () => {
       it('should show language and error level badges', () => {
         wrapper.setData({

spec/graphql/types/diff_refs_type_spec.rb

@@ -5,5 +5,9 @@ require 'spec_helper'
 describe GitlabSchema.types['DiffRefs'] do
   it { expect(described_class.graphql_name).to eq('DiffRefs') }
-  it { expect(described_class).to have_graphql_fields(:base_sha, :head_sha, :start_sha) }
+  it { is_expected.to have_graphql_fields(:head_sha, :base_sha, :start_sha).only }
+
+  it { expect(described_class.fields['headSha'].type).to be_non_null }
+  it { expect(described_class.fields['baseSha'].type).not_to be_non_null }
+  it { expect(described_class.fields['startSha'].type).to be_non_null }
 end

spec/lib/gitlab/background_migration/recalculate_project_authorizations_with_min_max_user_id_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::RecalculateProjectAuthorizationsWithMinMaxUserId, :migration, schema: 20200204113224 do
+  let(:users_table) { table(:users) }
+  let(:min) { 1 }
+  let(:max) { 5 }
+
+  before do
+    min.upto(max) do |i|
+      users_table.create!(id: i, email: "user#{i}@example.com", projects_limit: 10)
+    end
+  end
+
+  describe '#perform' do
+    it 'initializes Users::RefreshAuthorizedProjectsService with correct users' do
+      min.upto(max) do |i|
+        user = User.find(i)
+        expect(Users::RefreshAuthorizedProjectsService).to(
+          receive(:new).with(user, any_args).and_call_original)
+      end
+
+      described_class.new.perform(min, max)
+    end
+
+    it 'executes Users::RefreshAuthorizedProjectsService' do
+      expected_call_counts = max - min + 1
+
+      service = instance_double(Users::RefreshAuthorizedProjectsService)
+      expect(Users::RefreshAuthorizedProjectsService).to(
+        receive(:new).exactly(expected_call_counts).times.and_return(service))
+      expect(service).to receive(:execute).exactly(expected_call_counts).times
+
+      described_class.new.perform(min, max)
+    end
+  end
+end

spec/lib/gitlab/dependency_linker/base_linker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::DependencyLinker::BaseLinker do
+  let(:linker_class) do
+    Class.new(described_class) do
+      def link_dependencies
+        link_regex(%r{^(?<name>https?://[^ ]+)}, &:itself)
+      end
+    end
+  end
+
+  let(:plain_content) do
+    <<~CONTENT
+      http://\\njavascript:alert(1)
+      https://gitlab.com/gitlab-org/gitlab
+    CONTENT
+  end
+
+  let(:highlighted_content) do
+    <<~CONTENT
+      <span><span>http://</span><span>\\n</span><span>javascript:alert(1)</span></span>
+      <span><span>https://gitlab.com/gitlab-org/gitlab</span></span>
+    CONTENT
+  end
+
+  let(:linker) { linker_class.new(plain_content, highlighted_content) }
+
+  describe '#link' do
+    subject { linker.link }
+
+    it 'only converts valid links' do
+      expect(subject).to eq(
+        <<~CONTENT
+          <span><span>#{link('http://')}</span><span>#{link('\n', url: '%5Cn')}</span><span>#{link('javascript:alert(1)', url: nil)}</span></span>
+          <span><span>#{link('https://gitlab.com/gitlab-org/gitlab')}</span></span>
+        CONTENT
+      )
+    end
+  end
+
+  def link(text, url: text)
+    attrs = [
+      'rel="nofollow noreferrer noopener"',
+      'target="_blank"'
+    ]
+
+    attrs.unshift(%{href="#{url}"}) if url
+
+    %{<a #{attrs.join(' ')}>#{text}</a>}
+  end
+end

spec/lib/gitlab/project_authorizations_spec.rb

@@ -109,6 +109,20 @@ describe Gitlab::ProjectAuthorizations do
       end
     end
+    context 'with lower group access level than max access level for share' do
+      let(:user) { create(:user) }
+
+      it 'creates proper authorizations' do
+        group.add_reporter(user)
+
+        mapping = map_access_levels(authorizations)
+
+        expect(mapping[project_parent.id]).to be_nil
+        expect(mapping[project.id]).to eq(Gitlab::Access::REPORTER)
+        expect(mapping[project_child.id]).to eq(Gitlab::Access::REPORTER)
+      end
+    end
+
     context 'parent group user' do
       let(:user) { parent_group_user }

spec/lib/gitlab/user_access_spec.rb

@@ -30,6 +30,17 @@ describe Gitlab::UserAccess do
       end
     end
+    describe 'push to branch in an internal project' do
+      it 'will not infinitely loop when a project is internal' do
+        project.visibility_level = Gitlab::VisibilityLevel::INTERNAL
+        project.save!
+
+        expect(project).not_to receive(:branch_allows_collaboration?)
+
+        access.can_push_to_branch?('master')
+      end
+    end
+
     describe 'push to empty project' do
       let(:empty_project) { create(:project_empty_repo) }
       let(:project_access) { described_class.new(user, project: empty_project) }

spec/migrations/schedule_recalculate_project_authorizations_second_run_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200204113224_schedule_recalculate_project_authorizations_second_run.rb')
+
+describe ScheduleRecalculateProjectAuthorizationsSecondRun, :migration do
+  let(:users_table) { table(:users) }
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 2)
+
+    1.upto(4) do |i|
+      users_table.create!(id: i, name: "user#{i}", email: "user#{i}@example.com", projects_limit: 1)
+    end
+  end
+
+  it 'schedules background migration' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(1, 2)
+        expect(described_class::MIGRATION).to be_scheduled_migration(3, 4)
+      end
+    end
+  end
+end

spec/models/group_spec.rb

@@ -563,6 +563,18 @@ describe Group do
             expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
             expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::DEVELOPER)
           end
+
+          context 'with lower group access level than max access level for share' do
+            let(:user) { create(:user) }
+
+            it 'returns correct access level' do
+              group.add_reporter(user)
+
+              expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+              expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
+              expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::REPORTER)
+            end
+          end
         end
         context 'with user in the parent group' do
@@ -584,6 +596,33 @@ describe Group do
             expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
           end
         end
+
+        context 'unrelated project owner' do
+          let(:common_id) { [Project.maximum(:id).to_i, Namespace.maximum(:id).to_i].max + 999 }
+          let!(:group) { create(:group, id: common_id) }
+          let!(:unrelated_project) { create(:project, id: common_id) }
+          let(:user) { unrelated_project.owner }
+
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+          end
+        end
+
+        context 'user without accepted access request' do
+          let!(:user) { create(:user) }
+
+          before do
+            create(:group_member, :developer, :access_request, user: user, group: group)
+          end
+
+          it 'returns correct access level' do
+            expect(shared_group_parent.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+            expect(shared_group_child.max_member_access_for_user(user)).to eq(Gitlab::Access::NO_ACCESS)
+          end
+        end
       end
       context 'when feature flag share_group_with_group is disabled' do

spec/models/project_spec.rb

@@ -4793,6 +4793,38 @@ describe Project do
     end
   end
+  context 'with cross internal project merge requests' do
+    let(:project) { create(:project, :repository, :internal) }
+    let(:forked_project) { fork_project(project, nil, repository: true) }
+    let(:user) { double(:user) }
+
+    it "does not endlessly loop for internal projects with MRs to each other", :sidekiq_inline do
+      allow(user).to receive(:can?).and_return(true, false, true)
+      allow(user).to receive(:id).and_return(1)
+
+      create(
+        :merge_request,
+        target_project: project,
+        target_branch: 'merge-test',
+        source_project: forked_project,
+        source_branch: 'merge-test',
+        allow_collaboration: true
+      )
+
+      create(
+        :merge_request,
+        target_project: forked_project,
+        target_branch: 'merge-test',
+        source_project: project,
+        source_branch: 'merge-test',
+        allow_collaboration: true
+      )
+
+      expect(user).to receive(:can?).at_most(5).times
+      project.branch_allows_collaboration?(user, "merge-test")
+    end
+  end
+
   context 'with cross project merge requests' do
     let(:user) { create(:user) }
     let(:target_project) { create(:project, :repository) }

spec/presenters/ci/pipeline_presenter_spec.rb

@@ -6,6 +6,7 @@ describe Ci::PipelinePresenter do
   include Gitlab::Routing
   let(:user) { create(:user) }
+  let(:current_user) { user }
   let(:project) { create(:project) }
   let(:pipeline) { create(:ci_pipeline, project: project) }
@@ -15,7 +16,7 @@ describe Ci::PipelinePresenter do
   before do
     project.add_developer(user)
-    allow(presenter).to receive(:current_user) { user }
+    allow(presenter).to receive(:current_user) { current_user }
   end
   it 'inherits from Gitlab::View::Presenter::Delegated' do
@@ -224,10 +225,90 @@ describe Ci::PipelinePresenter do
   describe '#all_related_merge_requests' do
     it 'memoizes the returned relation' do
       query_count = ActiveRecord::QueryRecorder.new do
-        2.times { presenter.send(:all_related_merge_requests).count }
+        3.times { presenter.send(:all_related_merge_requests).count }
       end.count
-      expect(query_count).to eq(1)
+      expect(query_count).to eq(2)
+    end
+
+    context 'permissions' do
+      let!(:merge_request) do
+        create(:merge_request, project: project, source_project: project)
+      end
+
+      subject(:all_related_merge_requests) do
+        presenter.send(:all_related_merge_requests)
+      end
+
+      shared_examples 'private merge requests' do
+        context 'when not logged in' do
+          let(:current_user) {}
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'when logged in as a non_member' do
+          let(:current_user) { create(:user) }
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'when logged in as a guest' do
+          let(:current_user) { create(:user) }
+
+          before do
+            project.add_guest(current_user)
+          end
+
+          it { is_expected.to be_empty }
+        end
+
+        context 'when logged in as a developer' do
+          it { is_expected.to contain_exactly(merge_request) }
+        end
+
+        context 'when logged in as a maintainer' do
+          let(:current_user) { create(:user) }
+
+          before do
+            project.add_maintainer(current_user)
+          end
+
+          it { is_expected.to contain_exactly(merge_request) }
+        end
+      end
+
+      context 'with a private project' do
+        it_behaves_like 'private merge requests'
+      end
+
+      context 'with a public project with private merge requests' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+
+          project
+            .project_feature
+            .update!(merge_requests_access_level: ProjectFeature::PRIVATE)
+        end
+
+        it_behaves_like 'private merge requests'
+      end
+
+      context 'with a public project with public merge requests' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+
+          project
+            .project_feature
+            .update!(merge_requests_access_level: ProjectFeature::ENABLED)
+        end
+
+        context 'when not logged in' do
+          let(:current_user) {}
+
+          it { is_expected.to contain_exactly(merge_request) }
+        end
+      end
     end
   end