Return error when user lacks read access for project

There are a number of places where we were not checking for user access
rights (or assuming that authors automatically have them) so we were
potentially in situations where a user could create a merge request,
have their access rights revoked, and they would still be able to access
information or take actions related to their MR. This is potentially a
security issue, so we need to block this potential leak.

changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml

+---
+title: Check user access on API merge request read actions
+merge_request:
+author:
+type: security

lib/api/merge_request_approvals.rb

@@ -26,6 +26,8 @@ module API
         #   GET /projects/:id/merge_requests/:merge_request_iid/approvals
         desc 'List approvals for merge request'
         get 'approvals' do
+          not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
+
           merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
           present_approval(merge_request)

lib/api/merge_request_diffs.rb

@@ -23,6 +23,8 @@ module API
         use :pagination
       end
       get ":id/merge_requests/:merge_request_iid/versions" do
+        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
+
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         present paginate(merge_request.merge_request_diffs.order_id_desc), with: Entities::MergeRequestDiff
@@ -39,6 +41,8 @@ module API
       end
 
       get ":id/merge_requests/:merge_request_iid/versions/:version_id" do
+        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
+
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         present merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull

lib/api/merge_requests.rb

@@ -248,6 +248,8 @@ module API
         success Entities::MergeRequest
       end
       get ':id/merge_requests/:merge_request_iid' do
+        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
+
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         present merge_request,
@@ -264,7 +266,10 @@ module API
         success Entities::UserBasic
       end
       get ':id/merge_requests/:merge_request_iid/participants' do
+        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
+
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
+
         participants = ::Kaminari.paginate_array(merge_request.participants)
 
         present paginate(participants), with: Entities::UserBasic
@@ -274,6 +279,8 @@ module API
         success Entities::Commit
       end
       get ':id/merge_requests/:merge_request_iid/commits' do
+        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
+
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         commits =
@@ -355,6 +362,8 @@ module API
         success Entities::MergeRequestChanges
       end
       get ':id/merge_requests/:merge_request_iid/changes' do
+        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
+
         merge_request = find_merge_request_with_access(params[:merge_request_iid])
 
         present merge_request,
@@ -370,6 +379,8 @@ module API
       get ':id/merge_requests/:merge_request_iid/pipelines' do
         pipelines = merge_request_pipelines_with_access
 
+        not_found!("Merge Request") unless can?(current_user, :read_merge_request, user_project)
+
         present paginate(pipelines), with: Entities::Ci::PipelineBasic
       end
 

lib/api/todos.rb

@@ -28,6 +28,11 @@ module API
         end
         post ":id/#{type}/:#{type_id_str}/todo" do
           issuable = instance_exec(params[type_id_str], &finder)
+
+          unless can?(current_user, :read_merge_request, issuable.project)
+            not_found!(type.split("_").map(&:capitalize).join(" "))
+          end
+
           todo = TodoService.new.mark_todo(issuable, current_user).first
 
           if todo

spec/requests/api/merge_request_approvals_spec.rb

@@ -21,6 +21,12 @@ RSpec.describe API::MergeRequestApprovals do
 
       expect(response).to have_gitlab_http_status(:ok)
     end
+
+    context 'when merge request author has only guest access' do
+      it_behaves_like 'rejects user from accessing merge request info' do
+        let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/approvals" }
+      end
+    end
   end
 
   describe 'POST :id/merge_requests/:merge_request_iid/approve' do

spec/requests/api/merge_request_diffs_spec.rb

@@ -35,6 +35,12 @@ RSpec.describe API::MergeRequestDiffs, 'MergeRequestDiffs' do
       get api("/projects/#{project.id}/merge_requests/0/versions", user)
       expect(response).to have_gitlab_http_status(:not_found)
     end
+
+    context 'when merge request author has only guest access' do
+      it_behaves_like 'rejects user from accessing merge request info' do
+        let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/versions" }
+      end
+    end
   end
 
   describe 'GET /projects/:id/merge_requests/:merge_request_iid/versions/:version_id' do
@@ -63,5 +69,11 @@ RSpec.describe API::MergeRequestDiffs, 'MergeRequestDiffs' do
       get api("/projects/#{project.id}/merge_requests/#{non_existing_record_iid}/versions/#{merge_request_diff.id}", user)
       expect(response).to have_gitlab_http_status(:not_found)
     end
+
+    context 'when merge request author has only guest access' do
+      it_behaves_like 'rejects user from accessing merge request info' do
+        let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/versions/#{merge_request_diff.id}" }
+      end
+    end
   end
 end

spec/requests/api/merge_requests_spec.rb

@@ -1226,6 +1226,12 @@ RSpec.describe API::MergeRequests do
       end
     end
 
+    context 'when merge request author has only guest access' do
+      it_behaves_like 'rejects user from accessing merge request info' do
+        let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}" }
+      end
+    end
+
     context 'merge_request_metrics' do
       let(:pipeline) { create(:ci_empty_pipeline) }
 
@@ -1402,6 +1408,12 @@ RSpec.describe API::MergeRequests do
     it_behaves_like 'issuable participants endpoint' do
       let(:entity) { create(:merge_request, :simple, milestone: milestone1, author: user, assignees: [user], source_project: project, target_project: project, source_branch: 'markdown', title: "Test", created_at: base_time) }
     end
+
+    context 'when merge request author has only guest access' do
+      it_behaves_like 'rejects user from accessing merge request info' do
+        let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/participants" }
+      end
+    end
   end
 
   describe 'GET /projects/:id/merge_requests/:merge_request_iid/commits' do
@@ -1427,6 +1439,12 @@ RSpec.describe API::MergeRequests do
 
       expect(response).to have_gitlab_http_status(:not_found)
     end
+
+    context 'when merge request author has only guest access' do
+      it_behaves_like 'rejects user from accessing merge request info' do
+        let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/commits" }
+      end
+    end
   end
 
   describe 'GET /projects/:id/merge_requests/:merge_request_iid/:context_commits' do
@@ -1502,6 +1520,12 @@ RSpec.describe API::MergeRequests do
       expect(response).to have_gitlab_http_status(:not_found)
     end
 
+    context 'when merge request author has only guest access' do
+      it_behaves_like 'rejects user from accessing merge request info' do
+        let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes" }
+      end
+    end
+
     it_behaves_like 'find an existing merge request'
     it_behaves_like 'accesses diffs via raw_diffs'
 
@@ -1591,6 +1615,12 @@ RSpec.describe API::MergeRequests do
         expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
+
+    context 'when merge request author has only guest access' do
+      it_behaves_like 'rejects user from accessing merge request info' do
+        let(:url) { "/projects/#{project.id}/merge_requests/#{merge_request.iid}/pipelines" }
+      end
+    end
   end
 
   describe 'POST /projects/:id/merge_requests/:merge_request_iid/pipelines' do

spec/requests/api/todos_spec.rb

@@ -331,6 +331,14 @@ RSpec.describe API::Todos do
         expect(response).to have_gitlab_http_status(:not_found)
       end
     end
+
+    it 'returns an error if the issuable author does not have access' do
+      project_1.add_guest(issuable.author)
+
+      post api("/projects/#{project_1.id}/#{issuable_type}/#{issuable.iid}/todo", issuable.author)
+
+      expect(response).to have_gitlab_http_status(:not_found)
+    end
   end
 
   describe 'POST :id/issuable_type/:issueable_id/todo' do

spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'rejects user from accessing merge request info' do
+  let(:project) { create(:project, :private) }
+  let(:merge_request) do
+    create(:merge_request,
+      author: user,
+      source_project: project,
+      target_project: project
+    )
+  end
+
+  before do
+    project.add_guest(user)
+  end
+
+  it 'returns a 404 error' do
+    get api(url, user)
+
+    expect(response).to have_gitlab_http_status(:not_found)
+    expect(json_response['message']).to eq('404 Merge Request Not Found')
+  end
+end