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

13 files changed, 227 insertions, 56 deletions

diff --git a/changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml b/changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml
new file mode 100644
index 00000000000..c1174904018
--- /dev/null
+++ b/changelogs/unreleased/security-check-user-access-on-api-mr-read-actions-master.yml
@@ -0,0 +1,5 @@
+---
+title: Check user access on API merge request read actions
+merge_request:
+author:
+type: security
diff --git a/changelogs/unreleased/security-fix-unauthenticated-lint.yml b/changelogs/unreleased/security-fix-unauthenticated-lint.yml
new file mode 100644
index 00000000000..94521ba7ec9
--- /dev/null
+++ b/changelogs/unreleased/security-fix-unauthenticated-lint.yml
@@ -0,0 +1,5 @@
+---
+title: Updates authorization for linting API
+merge_request:
+author:
+type: security
diff --git a/lib/api/lint.rb b/lib/api/lint.rb
index f1f34622187..2d30754a36d 100644
--- a/lib/api/lint.rb
+++ b/lib/api/lint.rb
@@ -11,6 +11,8 @@ module API
         optional :include_merged_yaml, type: Boolean, desc: 'Whether or not to include merged CI config yaml in the response'
       end
       post '/lint' do
+        unauthorized! unless Gitlab::CurrentSettings.signup_enabled? && current_user
+
         result = Gitlab::Ci::YamlProcessor.new(params[:content], user: current_user).execute
 
         status 200
@@ -55,7 +57,7 @@ module API
         optional :dry_run, type: Boolean, default: false, desc: 'Run pipeline creation simulation, or only do static check.'
       end
       post ':id/ci/lint' do
-        authorize! :download_code, user_project
+        authorize! :create_pipeline, user_project
 
         result = Gitlab::Ci::Lint
           .new(project: user_project, current_user: current_user)
diff --git a/lib/api/merge_request_approvals.rb b/lib/api/merge_request_approvals.rb
index 00f42703731..0cdfd8f94b4 100644
--- a/lib/api/merge_request_approvals.rb
+++ b/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)
diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb
index 0ffb38438eb..97a6c7075b3 100644
--- a/lib/api/merge_request_diffs.rb
+++ b/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
diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb
index ab0e9b95e4a..142ecd0dc1e 100644
--- a/lib/api/merge_requests.rb
+++ b/lib/api/merge_requests.rb
@@ -246,6 +246,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,
@@ -262,7 +264,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
@@ -272,6 +277,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 =
@@ -353,6 +360,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,
@@ -368,6 +377,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
 
diff --git a/lib/api/todos.rb b/lib/api/todos.rb
index 03850ba1c4e..afc1525cbe2 100644
--- a/lib/api/todos.rb
+++ b/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
diff --git a/spec/requests/api/lint_spec.rb b/spec/requests/api/lint_spec.rb
index 2653653c896..2316e702c3e 100644
--- a/spec/requests/api/lint_spec.rb
+++ b/spec/requests/api/lint_spec.rb
@@ -4,91 +4,136 @@ require 'spec_helper'
 
 RSpec.describe API::Lint do
   describe 'POST /ci/lint' do
-    context 'with valid .gitlab-ci.yaml content' do
-      let(:yaml_content) do
-        File.read(Rails.root.join('spec/support/gitlab_stubs/gitlab_ci.yml'))
-      end
+    context 'when signup settings are disabled' do
+      Gitlab::CurrentSettings.signup_enabled = false
 
-      it 'passes validation without warnings or errors' do
-        post api('/ci/lint'), params: { content: yaml_content }
+      context 'when unauthenticated' do
+        it 'returns authentication error' do
+          post api('/ci/lint'), params: { content: 'content' }
 
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response).to be_an Hash
-        expect(json_response['status']).to eq('valid')
-        expect(json_response['warnings']).to eq([])
-        expect(json_response['errors']).to eq([])
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
       end
 
-      it 'outputs expanded yaml content' do
-        post api('/ci/lint'), params: { content: yaml_content, include_merged_yaml: true }
+      context 'when authenticated' do
+        it 'returns unauthorized error' do
+          post api('/ci/lint'), params: { content: 'content' }
 
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response).to have_key('merged_yaml')
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
       end
     end
 
-    context 'with valid .gitlab-ci.yaml with warnings' do
-      let(:yaml_content) { { job: { script: 'ls', rules: [{ when: 'always' }] } }.to_yaml }
+    context 'when signup settings are enabled' do
+      Gitlab::CurrentSettings.signup_enabled = true
 
-      it 'passes validation but returns warnings' do
-        post api('/ci/lint'), params: { content: yaml_content }
+      context 'when unauthenticated' do
+        it 'returns authentication error' do
+          post api('/ci/lint'), params: { content: 'content' }
 
-        expect(response).to have_gitlab_http_status(:ok)
-        expect(json_response['status']).to eq('valid')
-        expect(json_response['warnings']).not_to be_empty
-        expect(json_response['status']).to eq('valid')
-        expect(json_response['errors']).to eq([])
+          expect(response).to have_gitlab_http_status(:unauthorized)
+        end
+      end
+
+      context 'when authenticated' do
+        let_it_be(:api_user) { create(:user) }
+        it 'returns authentication success' do
+          post api('/ci/lint', api_user), params: { content: 'content' }
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
       end
     end
 
-    context 'with an invalid .gitlab_ci.yml' do
-      context 'with invalid syntax' do
-        let(:yaml_content) { 'invalid content' }
+    context 'when authenticated' do
+      let_it_be(:api_user) { create(:user) }
 
-        it 'responds with errors about invalid syntax' do
-          post api('/ci/lint'), params: { content: yaml_content }
+      context 'with valid .gitlab-ci.yaml content' do
+        let(:yaml_content) do
+          File.read(Rails.root.join('spec/support/gitlab_stubs/gitlab_ci.yml'))
+        end
+
+        it 'passes validation without warnings or errors' do
+          post api('/ci/lint', api_user), params: { content: yaml_content }
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['status']).to eq('invalid')
+          expect(json_response).to be_an Hash
+          expect(json_response['status']).to eq('valid')
           expect(json_response['warnings']).to eq([])
-          expect(json_response['errors']).to eq(['Invalid configuration format'])
+          expect(json_response['errors']).to eq([])
         end
 
         it 'outputs expanded yaml content' do
-          post api('/ci/lint'), params: { content: yaml_content, include_merged_yaml: true }
+          post api('/ci/lint', api_user), params: { content: yaml_content, include_merged_yaml: true }
 
           expect(response).to have_gitlab_http_status(:ok)
           expect(json_response).to have_key('merged_yaml')
         end
       end
 
-      context 'with invalid configuration' do
-        let(:yaml_content) { '{ image: "ruby:2.7",  services: ["postgres"], invalid }' }
+      context 'with valid .gitlab-ci.yaml with warnings' do
+        let(:yaml_content) { { job: { script: 'ls', rules: [{ when: 'always' }] } }.to_yaml }
 
-        it 'responds with errors about invalid configuration' do
-          post api('/ci/lint'), params: { content: yaml_content }
+        it 'passes validation but returns warnings' do
+          post api('/ci/lint', api_user), params: { content: yaml_content }
 
           expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['status']).to eq('invalid')
-          expect(json_response['warnings']).to eq([])
-          expect(json_response['errors']).to eq(['jobs invalid config should implement a script: or a trigger: keyword', 'jobs config should contain at least one visible job'])
+          expect(json_response['status']).to eq('valid')
+          expect(json_response['warnings']).not_to be_empty
+          expect(json_response['status']).to eq('valid')
+          expect(json_response['errors']).to eq([])
         end
+      end
 
-        it 'outputs expanded yaml content' do
-          post api('/ci/lint'), params: { content: yaml_content, include_merged_yaml: true }
+      context 'with an invalid .gitlab_ci.yml' do
+        context 'with invalid syntax' do
+          let(:yaml_content) { 'invalid content' }
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response).to have_key('merged_yaml')
+          it 'responds with errors about invalid syntax' do
+            post api('/ci/lint', api_user), params: { content: yaml_content }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response['status']).to eq('invalid')
+            expect(json_response['warnings']).to eq([])
+            expect(json_response['errors']).to eq(['Invalid configuration format'])
+          end
+
+          it 'outputs expanded yaml content' do
+            post api('/ci/lint', api_user), params: { content: yaml_content, include_merged_yaml: true }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response).to have_key('merged_yaml')
+          end
+        end
+
+        context 'with invalid configuration' do
+          let(:yaml_content) { '{ image: "ruby:2.7",  services: ["postgres"] }' }
+
+          it 'responds with errors about invalid configuration' do
+            post api('/ci/lint', api_user), params: { content: yaml_content }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response['status']).to eq('invalid')
+            expect(json_response['warnings']).to eq([])
+            expect(json_response['errors']).to eq(['jobs config should contain at least one visible job'])
+          end
+
+          it 'outputs expanded yaml content' do
+            post api('/ci/lint', api_user), params: { content: yaml_content, include_merged_yaml: true }
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response).to have_key('merged_yaml')
+          end
         end
       end
-    end
 
-    context 'without the content parameter' do
-      it 'responds with validation error about missing content' do
-        post api('/ci/lint')
+      context 'without the content parameter' do
+        it 'responds with validation error about missing content' do
+          post api('/ci/lint', api_user)
 
-        expect(response).to have_gitlab_http_status(:bad_request)
-        expect(json_response['error']).to eq('content is missing')
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['error']).to eq('content is missing')
+        end
       end
     end
   end
@@ -364,6 +409,18 @@ RSpec.describe API::Lint do
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
+
+      context 'when project is public' do
+        before do
+          project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+        end
+
+        it 'returns authentication error' do
+          ci_lint
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+      end
     end
 
     context 'when authenticated as non-member' do
@@ -387,13 +444,10 @@ RSpec.describe API::Lint do
         context 'when running as dry run' do
           let(:dry_run) { true }
 
-          it 'returns pipeline creation error' do
+          it 'returns authentication error' do
             ci_lint
 
-            expect(response).to have_gitlab_http_status(:ok)
-            expect(json_response['merged_yaml']).to eq(nil)
-            expect(json_response['valid']).to eq(false)
-            expect(json_response['errors']).to eq(['Insufficient permissions to create a new pipeline'])
+            expect(response).to have_gitlab_http_status(:forbidden)
           end
         end
 
@@ -410,7 +464,11 @@ RSpec.describe API::Lint do
             )
           end
 
-          it_behaves_like 'valid project config'
+          it 'returns authentication error' do
+            ci_lint
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+          end
         end
       end
     end
diff --git a/spec/requests/api/merge_request_approvals_spec.rb b/spec/requests/api/merge_request_approvals_spec.rb
index fad5c3fb60e..b18f3017e03 100644
--- a/spec/requests/api/merge_request_approvals_spec.rb
+++ b/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
diff --git a/spec/requests/api/merge_request_diffs_spec.rb b/spec/requests/api/merge_request_diffs_spec.rb
index 2e6cbe7bee7..971fb5e991c 100644
--- a/spec/requests/api/merge_request_diffs_spec.rb
+++ b/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
diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb
index 3a3eae73932..a04867658e8 100644
--- a/spec/requests/api/merge_requests_spec.rb
+++ b/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
diff --git a/spec/requests/api/todos_spec.rb b/spec/requests/api/todos_spec.rb
index eaffa49fc9d..00de1ef5964 100644
--- a/spec/requests/api/todos_spec.rb
+++ b/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
diff --git a/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb b/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb
new file mode 100644
index 00000000000..e6f9e5a434c
--- /dev/null
+++ b/spec/support/shared_examples/requests/api/merge_requests_shared_examples.rb
@@ -0,0 +1,23 @@
+# 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