Merge branch 'ag-block-non-get-head-requests' into 'master'

Block non-get and non-head requests

See merge request gitlab-org/gitlab!46045

ee/app/helpers/ee/application_helper.rb

@@ -11,6 +11,8 @@ module EE
 
     override :read_only_message
     def read_only_message
+      return _('You are on a read-only GitLab instance.') if maintenance_mode?
+
       return super unless ::Gitlab::Geo.secondary?
 
       message = @limited_actions_message ? s_('Geo|You may be able to make a limited amount of changes or perform a limited amount of actions on this page.') : s_('Geo|If you want to make changes, you must visit the primary site.')
@@ -133,5 +135,11 @@ module EE
         next_unprocessed_event.created_at < EVENT_LAG_SHOW_THRESHOLD.ago
       end
     end
+
+    def maintenance_mode?
+      return unless ::Feature.enabled?(:maintenance_mode)
+
+      ::Gitlab::CurrentSettings.maintenance_mode
+    end
   end
 end

ee/lib/ee/gitlab/middleware/read_only/controller.rb

@@ -16,8 +16,8 @@ module EE
             'admin/geo/uploads' => %w{destroy}
           }.freeze
 
-          ALLOWLISTED_GIT_WRITE_ROUTES = {
-            'repositories/git_http' => %w{git_receive_pack}
+          ALLOWLISTED_GIT_READ_WRITE_ROUTES = {
+            'repositories/git_http' => %w{git_upload_pack git_receive_pack}
           }.freeze
 
           ALLOWLISTED_GIT_LFS_LOCKS_ROUTES = {
@@ -26,9 +26,24 @@ module EE
 
           private
 
+          # In addition to routes allowed in FOSS, allow geo node update route
+          # and geo api route, on both Geo primary and secondary.
+          # If this is on a Geo secondary, also allow git write routes.
+          # If in maintenance mode, don't allow git write routes on Geo
+          # secondary either
           override :allowlisted_routes
           def allowlisted_routes
-            super || geo_node_update_route? || geo_proxy_git_ssh_route? || geo_api_route? || geo_proxy_git_http_route? || lfs_locks_route?
+            allowed = super || geo_node_update_route? || geo_api_route?
+
+            return true if allowed
+            return false if maintenance_mode?
+            return false unless ::Gitlab::Geo.secondary?
+
+            git_write_routes
+          end
+
+          def git_write_routes
+            geo_proxy_git_ssh_route? || geo_proxy_git_http_route? || lfs_locks_route?
           end
 
           def geo_node_update_route?
@@ -54,7 +69,7 @@ module EE
           def geo_proxy_git_http_route?
             return unless request.path.end_with?('.git/git-receive-pack')
 
-            ALLOWLISTED_GIT_WRITE_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
+            ALLOWLISTED_GIT_READ_WRITE_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
           end
 
           def geo_api_route?
@@ -65,8 +80,6 @@ module EE
 
           def lfs_locks_route?
             # Calling route_hash may be expensive. Only do it if we think there's a possible match
-            return unless ::Gitlab::Geo.secondary?
-
             unless request.path.end_with?('/info/lfs/locks', '/info/lfs/locks/verify') ||
                 %r{/info/lfs/locks/\d+/unlock\z}.match?(request.path)
               return false
@@ -74,6 +87,17 @@ module EE
 
             ALLOWLISTED_GIT_LFS_LOCKS_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
           end
+
+          override :read_only?
+          def read_only?
+            maintenance_mode? || super
+          end
+
+          def maintenance_mode?
+            return unless ::Feature.enabled?(:maintenance_mode)
+
+            ::Gitlab::CurrentSettings.maintenance_mode
+          end
         end
       end
     end

ee/spec/features/read_only_spec.rb

@@ -20,4 +20,12 @@ RSpec.describe 'Geo read-only message', :geo do
 
     it_behaves_like 'Read-only instance', /You are on a secondary, read\-only Geo node\. If you want to make changes, you must visit the primary site.*Go to the primary site/
   end
+
+  context 'when in maintenance mode' do
+    before do
+      stub_application_setting(maintenance_mode: true)
+    end
+
+    it_behaves_like 'Read-only instance', /You are on a read\-only GitLab instance./
+  end
 end

ee/spec/lib/ee/gitlab/middleware/read_only_spec.rb

@@ -3,9 +3,19 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Middleware::ReadOnly do
-  before do
-    allow(Gitlab::Database).to receive(:read_only?) { true }
+  context 'when maintenance mode is on' do
+    before do
+      stub_application_setting(maintenance_mode: true)
+    end
+
+    it_behaves_like 'write access for a read-only GitLab (EE) instance in maintenance mode'
   end
 
-  it_behaves_like 'write access for a read-only GitLab (EE) instance'
+  context 'when maintenance mode is not on' do
+    before do
+      stub_application_setting(maintenance_mode: false)
+    end
+
+    it_behaves_like 'write access for a read-only GitLab (EE) instance'
+  end
 end

ee/spec/support/shared_examples/lib/gitlab/middleware/allowlisted_admin_geo_requests_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'allowlisted /admin/geo requests' do
+  shared_examples 'allowlisted request' do |request_type, request_url|
+    it "expects a #{request_type.upcase} request to #{request_url} to be allowed" do
+      expect(Rails.application.routes).to receive(:recognize_path).and_call_original
+      response = request.send(request_type, request_url)
+
+      expect(response).not_to be_redirect
+      expect(subject).not_to disallow_request
+    end
+  end
+
+  context 'allowlisted requests' do
+    it_behaves_like 'allowlisted request', :patch, '/admin/geo/nodes/1'
+
+    it_behaves_like 'allowlisted request', :delete, '/admin/geo/replication/projects/1'
+
+    it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/1/resync'
+
+    it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/1/reverify'
+
+    it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/reverify_all'
+
+    it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/resync_all'
+
+    it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/1/force_redownload'
+
+    it_behaves_like 'allowlisted request', :delete, '/admin/geo/replication/uploads/1'
+  end
+end

ee/spec/support/shared_examples/lib/gitlab/middleware/maintenance_mode_gitlab_ee_instance_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'write access for a read-only GitLab (EE) instance in maintenance mode' do
+  include Rack::Test::Methods
+  using RSpec::Parameterized::TableSyntax
+
+  include_context 'with a mocked GitLab instance'
+
+  before do
+    stub_application_setting(maintenance_mode: true)
+  end
+
+  context 'normal requests to a read-only GitLab instance' do
+    let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } }
+
+    it_behaves_like 'allowlisted /admin/geo requests'
+
+    context 'on Geo secondary' do
+      before do
+        allow(::Gitlab::Geo).to receive(:secondary?).and_return(true)
+      end
+
+      where(:description, :path) do
+        'LFS request to batch'        | '/root/rouge.git/info/lfs/objects/batch'
+        'to geo replication node api' | "/api/#{API::API.version}/geo_replication/designs/resync"
+      end
+
+      with_them do
+        it "expects a POST #{description} URL to be allowed" do
+          response = request.post(path)
+
+          expect(response).not_to be_redirect
+          expect(subject).not_to disallow_request
+        end
+      end
+
+      where(:description, :path) do
+        'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
+        'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
+        'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
+        'git-receive-pack'            | '/root/rouge.git/git-receive-pack'
+      end
+
+      with_them do
+        it "expects a POST #{description} URL to not be allowed" do
+          response = request.post(path)
+
+          expect(response).to be_redirect
+          expect(subject).to disallow_request
+        end
+      end
+    end
+
+    context 'when not on Geo secondary' do
+      before do
+        allow(::Gitlab::Geo).to receive(:secondary?).and_return(false)
+      end
+
+      where(:description, :path) do
+        'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
+        'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
+        'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
+      end
+
+      with_them do
+        it "expects a POST #{description} URL not to be allowed" do
+          response = request.post(path)
+
+          expect(response).to be_redirect
+          expect(subject).to disallow_request
+        end
+      end
+    end
+  end
+end

ee/spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_ee_instance_shared_examples.rb

@@ -9,83 +9,30 @@ RSpec.shared_examples 'write access for a read-only GitLab (EE) instance' do
   context 'normal requests to a read-only GitLab instance' do
     let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'text/plain' }, ['OK']] } }
 
-    shared_examples 'allowlisted request' do |request_type, request_url|
-      it "expects a #{request_type.upcase} request to #{request_url} to be allowed" do
-        expect(Rails.application.routes).to receive(:recognize_path).and_call_original
-        response = request.send(request_type, request_url)
+    it_behaves_like 'allowlisted /admin/geo requests'
 
-        expect(response).not_to be_redirect
-        expect(subject).not_to disallow_request
+    context 'on Geo secondary' do
+      before do
+        allow(::Gitlab::Geo).to receive(:secondary?).and_return(true)
       end
-    end
-
-    context 'allowlisted requests' do
-      it_behaves_like 'allowlisted request', :patch, '/admin/geo/nodes/1'
-
-      it_behaves_like 'allowlisted request', :delete, '/admin/geo/replication/projects/1'
-
-      it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/1/resync'
-
-      it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/1/reverify'
-
-      it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/reverify_all'
-
-      it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/resync_all'
-
-      it_behaves_like 'allowlisted request', :post, '/admin/geo/replication/projects/1/force_redownload'
-
-      it_behaves_like 'allowlisted request', :delete, '/admin/geo/replication/uploads/1'
-
-      context 'on Geo secondary' do
-        before do
-          allow(::Gitlab::Geo).to receive(:secondary?).and_return(true)
-        end
 
-        where(:description, :path) do
-          'LFS request to batch'        | '/root/rouge.git/info/lfs/objects/batch'
-          'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
-          'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
-          'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
-          'to geo replication node api' | "/api/#{API::API.version}/geo_replication/designs/resync"
-        end
-
-        with_them do
-          it "expects a POST #{description} URL to be allowed" do
-            response = request.post(path)
-
-            expect(response).not_to be_redirect
-            expect(subject).not_to disallow_request
-          end
-        end
+      where(:description, :path) do
+        'LFS request to batch'        | '/root/rouge.git/info/lfs/objects/batch'
+        'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
+        'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
+        'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
+        'to geo replication node api' | "/api/#{API::API.version}/geo_replication/designs/resync"
+        'git-receive-pack'            | '/root/rouge.git/git-receive-pack'
       end
 
-      context 'when not on Geo secondary' do
-        before do
-          allow(::Gitlab::Geo).to receive(:secondary?).and_return(false)
-        end
+      with_them do
+        it "expects a POST #{description} URL to be allowed" do
+          response = request.post(path)
 
-        where(:description, :path) do
-          'LFS request to locks verify' | '/root/rouge.git/info/lfs/locks/verify'
-          'LFS request to locks create' | '/root/rouge.git/info/lfs/locks'
-          'LFS request to locks unlock' | '/root/rouge.git/info/lfs/locks/1/unlock'
-        end
-
-        with_them do
-          it "expects a POST #{description} URL not to be allowed" do
-            response = request.post(path)
-
-            expect(response).to be_redirect
-            expect(subject).to disallow_request
-          end
+          expect(response).not_to be_redirect
+          expect(subject).not_to disallow_request
         end
       end
     end
-
-    it 'expects a POST request to git-receive-pack URL to be allowed' do
-      response = request.post('/root/rouge.git/git-receive-pack')
-
-      expect(response).not_to be_redirect
-      expect(subject).not_to disallow_request
-    end
   end
 end

lib/gitlab/middleware/read_only/controller.rb

@@ -9,7 +9,7 @@ module Gitlab
         APPLICATION_JSON_TYPES = %W{#{APPLICATION_JSON} application/vnd.git-lfs+json}.freeze
         ERROR_MESSAGE = 'You cannot perform write operations on a read-only instance'
 
-        ALLOWLISTED_GIT_ROUTES = {
+        ALLOWLISTED_GIT_READ_ONLY_ROUTES = {
           'repositories/git_http' => %w{git_upload_pack}
         }.freeze
 
@@ -34,7 +34,7 @@ module Gitlab
         end
 
         def call
-          if disallowed_request? && Gitlab::Database.read_only?
+          if disallowed_request? && read_only?
             Gitlab::AppLogger.debug('GitLab ReadOnly: preventing possible non read-only operation')
 
             if json_request?
@@ -57,6 +57,11 @@ module Gitlab
             !allowlisted_routes
         end
 
+        # Overridden in EE module
+        def read_only?
+          Gitlab::Database.read_only?
+        end
+
         def json_request?
           APPLICATION_JSON_TYPES.include?(request.media_type)
         end
@@ -97,7 +102,7 @@ module Gitlab
           return false unless request.post? &&
             request.path.end_with?('.git/git-upload-pack')
 
-          ALLOWLISTED_GIT_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
+          ALLOWLISTED_GIT_READ_ONLY_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
         end
 
         def internal_route?

spec/support/shared_contexts/lib/gitlab/middleware/with_a_mocked_gitlab_instance_shared_context.rb

@@ -25,7 +25,7 @@ RSpec.shared_context 'with a mocked GitLab instance' do
   let(:request) { Rack::MockRequest.new(rack_stack) }
 
   subject do
-    described_class.new(fake_app).tap do |app|
+    Gitlab::Middleware::ReadOnly.new(fake_app).tap do |app|
       app.extend(observe_env)
     end
   end