Merge branch 'security-fix-unauthenticated-lint' into 'master'

Change authorization policy for /lint

See merge request gitlab-org/security/gitlab!1190

changelogs/unreleased/security-fix-unauthenticated-lint.yml

+---
+title: Updates authorization for linting API
+merge_request:
+author:
+type: security

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)

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