Merge branch 'security-mr-head-pipeline-leak' into 'master'

Permission fix for MergeRequestsController#pipeline_status

See merge request gitlab/gitlab-ee!1128

app/controllers/projects/merge_requests_controller.rb

@@ -189,7 +189,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   def pipeline_status
     render json: PipelineSerializer
       .new(project: @project, current_user: @current_user)
-      .represent_status(@merge_request.head_pipeline)
+      .represent_status(head_pipeline)
   end
 
   def ci_environments_status
@@ -239,6 +239,13 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
 
   private
 
+  def head_pipeline
+    strong_memoize(:head_pipeline) do
+      pipeline = @merge_request.head_pipeline
+      pipeline if can?(current_user, :read_pipeline, pipeline)
+    end
+  end
+
   def ci_environments_status_on_merge_result?
     params[:environment_target] == 'merge_commit'
   end

changelogs/unreleased/security-mr-head-pipeline-leak.yml

+---
+title: Check permissions before responding in MergeController#pipeline_status
+merge_request:
+author:
+type: security

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -1096,17 +1096,39 @@ describe Projects::MergeRequestsController do
 
       let(:status) { pipeline.detailed_status(double('user')) }
 
-      before do
+      it 'returns a detailed head_pipeline status in json' do
         get_pipeline_status
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['text']).to eq status.text
+        expect(json_response['label']).to eq status.label
+        expect(json_response['icon']).to eq status.icon
+        expect(json_response['favicon']).to match_asset_path "/assets/ci_favicons/#{status.favicon}.png"
       end
 
-      it 'return a detailed head_pipeline status in json' do
+      context 'with project member visibility on a public project' do
+        let(:user)    { create(:user) }
+        let(:project) { create(:project, :repository, :public, :builds_private) }
+
+        it 'returns pipeline data to project members' do
+          project.add_developer(user)
+
+          get_pipeline_status
+
           expect(response).to have_gitlab_http_status(:ok)
           expect(json_response['text']).to eq status.text
           expect(json_response['label']).to eq status.label
           expect(json_response['icon']).to eq status.icon
           expect(json_response['favicon']).to match_asset_path "/assets/ci_favicons/#{status.favicon}.png"
         end
+
+        it 'returns blank OK response to non-project-members' do
+          get_pipeline_status
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response).to be_empty
+        end
+      end
     end
 
     context 'when head_pipeline does not exist' do
@@ -1114,7 +1136,7 @@ describe Projects::MergeRequestsController do
         get_pipeline_status
       end
 
-      it 'return empty' do
+      it 'returns blank OK response' do
         expect(response).to have_gitlab_http_status(:ok)
         expect(json_response).to be_empty
       end