From f7fbf49929e24e2f9bfec0a68fd450c3547f7a78 Mon Sep 17 00:00:00 2001
From: drew cimino <dcimino@gitlab.com>
Date: Wed, 21 Aug 2019 01:42:28 -0400
Subject: [PATCH] Restrict MergeRequests#test_reports to authenticated users
 with read-access on Builds

---
 .../projects/merge_requests_controller.rb     |  6 ++
 .../security-ci-metrics-permissions.yml       |  6 ++
 .../merge_requests_controller_spec.rb         | 60 ++++++++++++++++---
 3 files changed, 64 insertions(+), 8 deletions(-)
 create mode 100644 changelogs/unreleased/security-ci-metrics-permissions.yml

diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index f4d381244d9..c9a1f28f87e 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -12,6 +12,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   skip_before_action :merge_request, only: [:index, :bulk_update]
   before_action :whitelist_query_limiting, only: [:assign_related_issues, :update]
   before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
+  before_action :authorize_test_reports!, only: [:test_reports]
   before_action :set_issuables_index, only: [:index]
   before_action :authenticate_user!, only: [:assign_related_issues]
   before_action :check_user_can_push_to_source_branch!, only: [:rebase]
@@ -336,4 +337,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
       render json: { status_reason: 'Unknown error' }, status: :internal_server_error
     end
   end
+
+  def authorize_test_reports!
+    # MergeRequest#actual_head_pipeline is the pipeline accessed in MergeRequest#compare_reports.
+    return render_404 unless can?(current_user, :read_build, merge_request.actual_head_pipeline)
+  end
 end
diff --git a/changelogs/unreleased/security-ci-metrics-permissions.yml b/changelogs/unreleased/security-ci-metrics-permissions.yml
new file mode 100644
index 00000000000..51c6493442a
--- /dev/null
+++ b/changelogs/unreleased/security-ci-metrics-permissions.yml
@@ -0,0 +1,6 @@
+---
+title: Restrict MergeRequests#test_reports to authenticated users with read-access
+  on Builds
+merge_request:
+author:
+type: security
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 11b1eaf11b7..9cc76eb8c28 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -719,19 +719,63 @@ describe Projects::MergeRequestsController do
   end
 
   describe 'GET test_reports' do
+    let(:merge_request) do
+      create(:merge_request,
+        :with_diffs,
+        :with_merge_request_pipeline,
+        target_project: project,
+        source_project: project
+      )
+    end
+
     subject do
-      get :test_reports,
-          params: {
-            namespace_id: project.namespace.to_param,
-            project_id: project,
-            id: merge_request.iid
-          },
-          format: :json
+      get :test_reports, params: {
+        namespace_id: project.namespace.to_param,
+        project_id: project,
+        id: merge_request.iid
+      },
+      format: :json
     end
 
     before do
       allow_any_instance_of(MergeRequest)
-        .to receive(:compare_test_reports).and_return(comparison_status)
+        .to receive(:compare_test_reports)
+        .and_return(comparison_status)
+
+      allow_any_instance_of(MergeRequest)
+        .to receive(:actual_head_pipeline)
+        .and_return(merge_request.all_pipelines.take)
+    end
+
+    describe 'permissions on a public project with private CI/CD' do
+      let(:project) { create :project, :repository, :public, :builds_private }
+      let(:comparison_status) { { status: :parsed, data: { summary: 1 } } }
+
+      context 'while signed out' do
+        before do
+          sign_out(user)
+        end
+
+        it 'responds with a 404' do
+          subject
+
+          expect(response).to have_gitlab_http_status(404)
+          expect(response.body).to be_blank
+        end
+      end
+
+      context 'while signed in as an unrelated user' do
+        before do
+          sign_in(create(:user))
+        end
+
+        it 'responds with a 404' do
+          subject
+
+          expect(response).to have_gitlab_http_status(404)
+          expect(response.body).to be_blank
+        end
+      end
     end
 
     context 'when comparison is being processed' do