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

lib/gitlab/project_authorizations.rb

@@ -62,13 +62,18 @@ 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
         .select([namespaces[:id], members[:access_level]])
         .except(:order)
-      cte << Group.select([namespaces[:id], 'group_group_links.group_access AS access_level'])
+      # Namespaces shared with any of the group
+      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/frontend/error_tracking/components/error_details_spec.js

 import { createLocalVue, shallowMount } from '@vue/test-utils';
 import Vuex from 'vuex';
-import { GlLoadingIcon, GlLink } from '@gitlab/ui';
+import { GlLoadingIcon, GlLink, GlSprintf } from '@gitlab/ui';
 import LoadingButton from '~/vue_shared/components/loading_button.vue';
 import Stacktrace from '~/error_tracking/components/stacktrace.vue';
 import ErrorDetails from '~/error_tracking/components/error_details.vue';
@@ -16,7 +16,7 @@ describe('ErrorDetails', () => {
   function mountComponent() {
     wrapper = shallowMount(ErrorDetails, {
-      stubs: { LoadingButton },
+      stubs: { LoadingButton, GlSprintf },
       localVue,
       store,
       propsData: {
@@ -188,5 +188,28 @@ describe('ErrorDetails', () => {
         });
       });
     });
+
+    describe('Escape 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;
+        store.state.details.loading = false;
+        store.state.details.error = {
+          id: 1,
+          culprit,
+        };
+        mountComponent();
+      });
+
+      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);
+      });
+    });
   });
 });

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) }
+
+  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/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/models/group_spec.rb

@@ -562,6 +562,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
@@ -583,6 +595,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

@@ -4766,6 +4766,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
@@ -215,10 +216,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

spec/requests/api/triggers_spec.rb

@@ -132,6 +132,18 @@ describe API::Triggers do
         end
       end
     end
+
+    context 'when is triggered by a pipeline hook' do
+      it 'does not create a new pipeline' do
+        expect do
+          post api("/projects/#{project.id}/ref/master/trigger/pipeline?token=#{trigger_token}"),
+            params: { ref: 'refs/heads/other-branch' },
+            headers: { WebHookService::GITLAB_EVENT_HEADER => 'Pipeline Hook' }
+        end.not_to change(Ci::Pipeline, :count)
+
+        expect(response).to have_gitlab_http_status(:forbidden)
+      end
+    end
   end
   describe 'GET /projects/:id/triggers' do

spec/services/projects/lfs_pointers/lfs_download_service_spec.rb

@@ -133,6 +133,21 @@ describe Projects::LfsPointers::LfsDownloadService do
       end
     end
+    context 'when an lfs object with the same oid already exists' do
+      let!(:existing_lfs_object) { create(:lfs_object, oid: oid) }
+
+      before do
+        stub_full_request(download_link).to_return(body: lfs_content)
+      end
+
+      it_behaves_like 'no lfs object is created'
+
+      it 'does not update the file attached to the existing LfsObject' do
+        expect { subject.execute }
+          .not_to change { existing_lfs_object.reload.file.file.file }
+      end
+    end
+
     context 'when credentials present' do
       let(:download_link_with_credentials) { "http://user:password@gitlab.com/#{oid}" }
       let(:lfs_object) { LfsDownloadObject.new(oid: oid, size: size, link: download_link_with_credentials) }
@@ -210,17 +225,5 @@ describe Projects::LfsPointers::LfsDownloadService do
         subject.execute
       end
     end
-
-    context 'when an lfs object with the same oid already exists' do
-      before do
-        create(:lfs_object, oid: oid)
-      end
-
-      it 'does not download the file' do
-        expect(subject).not_to receive(:download_lfs_file!)
-
-        subject.execute
-      end
-    end
   end
 end

spec/services/projects/lfs_pointers/lfs_object_download_list_service_spec.rb

@@ -24,7 +24,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
   describe '#execute' do
     context 'when no lfs pointer is linked' do
       before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return([])
         allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
         expect(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:new).with(project, remote_uri: URI.parse(default_endpoint)).and_call_original
       end
@@ -35,12 +34,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
         subject.execute
       end
-      it 'links existent lfs objects to the project' do
-        expect_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute)
-
-        subject.execute
-      end
-
       it 'retrieves the download links of non existent objects' do
         expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(all_oids)
@@ -48,32 +41,6 @@ describe Projects::LfsPointers::LfsObjectDownloadListService do
       end
     end
-    context 'when some lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(existing_lfs_objects.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).and_return(oid_download_links)
-      end
-
-      it 'retrieves the download links of non existent objects' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with(oids)
-
-        subject.execute
-      end
-    end
-
-    context 'when all lfs objects are linked' do
-      before do
-        allow_any_instance_of(Projects::LfsPointers::LfsLinkService).to receive(:execute).and_return(all_oids.keys)
-        allow_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute)
-      end
-
-      it 'retrieves no download links' do
-        expect_any_instance_of(Projects::LfsPointers::LfsDownloadLinkListService).to receive(:execute).with({}).and_call_original
-
-        expect(subject.execute).to be_empty
-      end
-    end
-
     context 'when lfsconfig file exists' do
       before do
         allow(project.repository).to receive(:lfsconfig_for).and_return("[lfs]\n\turl = #{lfs_endpoint}\n")