Merge branch '330787-omits-trailing-slash-when-checking-for-allowed-requests' into 'master'

Omit trailing slash when checking allowed requests in the read-only middleware [RUN ALL RSPEC]

See merge request gitlab-org/gitlab!61641

changelogs/unreleased/330787-omits-trailing-slash-when-checking-for-allowed-requests.yml

+---
+title: Omit trailing slash when checking allowed requests in the read-only middleware
+merge_request: 61641
+author:
+type: fixed

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

@@ -79,7 +79,7 @@ module EE
           end
 
           def geo_proxy_git_http_route?
-            return unless request.path.end_with?('.git/git-receive-pack')
+            return unless request_path.end_with?('.git/git-receive-pack')
 
             ALLOWLISTED_GIT_READ_WRITE_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
           end
@@ -98,8 +98,8 @@ module EE
 
           def lfs_locks_route?
             # Calling route_hash may be expensive. Only do it if we think there's a possible match
-            unless request.path.end_with?('/info/lfs/locks', '/info/lfs/locks/verify') ||
-                %r{/info/lfs/locks/\d+/unlock\z}.match?(request.path)
+            unless request_path.end_with?('/info/lfs/locks', '/info/lfs/locks/verify') ||
+                %r{/info/lfs/locks/\d+/unlock\z}.match?(request_path)
               return false
             end
 

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

@@ -46,6 +46,13 @@ RSpec.shared_examples 'write access for a read-only GitLab (EE) instance in main
           expect(response).not_to be_redirect
           expect(subject).not_to disallow_request
         end
+
+        it "expects a POST #{description} URL with trailing slash 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
@@ -63,6 +70,13 @@ RSpec.shared_examples 'write access for a read-only GitLab (EE) instance in main
           expect(response).to be_redirect
           expect(subject).to disallow_request
         end
+
+        it "expects a POST #{description} URL with traling slash to not be allowed" do
+          response = request.post("#{path}/")
+
+          expect(response).to be_redirect
+          expect(subject).to disallow_request
+        end
       end
 
       it "expects a PUT request to /api/v4/application/settings to not be allowed" do
@@ -91,6 +105,13 @@ RSpec.shared_examples 'write access for a read-only GitLab (EE) instance in main
           expect(response).to be_redirect
           expect(subject).to disallow_request
         end
+
+        it "expects a POST #{description} URL with trailing backslash not to be allowed" do
+          response = request.post("#{path}/")
+
+          expect(response).to be_redirect
+          expect(subject).to disallow_request
+        end
       end
 
       where(:description, :path) do
@@ -105,6 +126,13 @@ RSpec.shared_examples 'write access for a read-only GitLab (EE) instance in main
           expect(response).not_to be_redirect
           expect(subject).not_to disallow_request
         end
+
+        it "expects a POST to #{description} URL with trailing slash to be allowed" do
+          response = request.post("#{path}/")
+
+          expect(response).not_to be_redirect
+          expect(subject).not_to disallow_request
+        end
       end
     end
   end

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

@@ -32,6 +32,13 @@ RSpec.shared_examples 'write access for a read-only GitLab (EE) instance' do
           expect(response).not_to be_redirect
           expect(subject).not_to disallow_request
         end
+
+        it "expects a POST #{description} URL with a trailing slash to be allowed" do
+          response = request.post("#{path}/")
+
+          expect(response).not_to be_redirect
+          expect(subject).not_to disallow_request
+        end
       end
     end
   end

lib/gitlab/middleware/read_only/controller.rb

@@ -83,7 +83,15 @@ module Gitlab
         end
 
         def route_hash
-          @route_hash ||= Rails.application.routes.recognize_path(request.url, { method: request.request_method }) rescue {}
+          @route_hash ||= Rails.application.routes.recognize_path(request_url, { method: request.request_method }) rescue {}
+        end
+
+        def request_url
+          request.url.chomp('/')
+        end
+
+        def request_path
+          @request_path ||= request.path.chomp('/')
         end
 
         def relative_url
@@ -100,7 +108,7 @@ module Gitlab
         def workhorse_passthrough_route?
           # Calling route_hash may be expensive. Only do it if we think there's a possible match
           return false unless request.post? &&
-            request.path.end_with?('.git/git-upload-pack')
+            request_path.end_with?('.git/git-upload-pack')
 
           ALLOWLISTED_GIT_READ_ONLY_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
         end
@@ -120,14 +128,14 @@ module Gitlab
         # https://gitlab.com/gitlab-org/gitlab/blob/master/app/controllers/repositories/lfs_api_controller.rb#L106
         def lfs_batch_route?
           # Calling route_hash may be expensive. Only do it if we think there's a possible match
-          return unless request.path.end_with?('/info/lfs/objects/batch')
+          return unless request_path.end_with?('/info/lfs/objects/batch')
 
           ALLOWLISTED_GIT_LFS_BATCH_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])
         end
 
         def session_route?
           # Calling route_hash may be expensive. Only do it if we think there's a possible match
-          return false unless request.post? && request.path.end_with?('/users/sign_out',
+          return false unless request.post? && request_path.end_with?('/users/sign_out',
             '/admin/session', '/admin/session/destroy')
 
           ALLOWLISTED_SESSION_ROUTES[route_hash[:controller]]&.include?(route_hash[:action])

spec/support/shared_examples/lib/gitlab/middleware/read_only_gitlab_instance_shared_examples.rb

@@ -70,6 +70,14 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do
         expect(subject).not_to disallow_request
       end
 
+      it 'expects a POST internal request with trailing slash to be allowed' do
+        expect(Rails.application.routes).not_to receive(:recognize_path)
+        response = request.post("/api/#{API::API.version}/internal/")
+
+        expect(response).not_to be_redirect
+        expect(subject).not_to disallow_request
+      end
+
       it 'expects a graphql request to be allowed' do
         response = request.post("/api/graphql")
 
@@ -77,6 +85,13 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do
         expect(subject).not_to disallow_request
       end
 
+      it 'expects a graphql request with trailing slash to be allowed' do
+        response = request.post("/api/graphql/")
+
+        expect(response).not_to be_redirect
+        expect(subject).not_to disallow_request
+      end
+
       context 'relative URL is configured' do
         before do
           stub_config_setting(relative_url_root: '/gitlab')
@@ -88,6 +103,13 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do
           expect(response).not_to be_redirect
           expect(subject).not_to disallow_request
         end
+
+        it 'expects a graphql request with trailing slash to be allowed' do
+          response = request.post("/gitlab/api/graphql/")
+
+          expect(response).not_to be_redirect
+          expect(subject).not_to disallow_request
+        end
       end
 
       context 'sidekiq admin requests' do
@@ -119,6 +141,19 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do
             expect(response).not_to be_redirect
             expect(subject).not_to disallow_request
           end
+
+          it 'allows requests with trailing slash' do
+            path = File.join(mounted_at, 'admin/sidekiq')
+            response = request.post("#{path}/")
+
+            expect(response).not_to be_redirect
+            expect(subject).not_to disallow_request
+
+            response = request.get("#{path}/")
+
+            expect(response).not_to be_redirect
+            expect(subject).not_to disallow_request
+          end
         end
       end
 
@@ -138,6 +173,14 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do
           expect(response).not_to be_redirect
           expect(subject).not_to disallow_request
         end
+
+        it "expects a POST #{description} URL with trailing slash to be allowed" do
+          expect(Rails.application.routes).to receive(:recognize_path).and_call_original
+          response = request.post("#{path}/")
+
+          expect(response).not_to be_redirect
+          expect(subject).not_to disallow_request
+        end
       end
 
       where(:description, :path) do
@@ -153,11 +196,18 @@ RSpec.shared_examples 'write access for a read-only GitLab instance' do
           expect(response).to be_redirect
           expect(subject).to disallow_request
         end
+
+        it "expects a POST #{description} URL with trailing slash not to be allowed" do
+          response = request.post("#{path}/")
+
+          expect(response).to be_redirect
+          expect(subject).to disallow_request
+        end
       end
     end
   end
 
-  context 'json requests to a read-only GitLab instance' do
+  context 'JSON requests to a read-only GitLab instance' do
     let(:fake_app) { lambda { |env| [200, { 'Content-Type' => 'application/json' }, ['OK']] } }
     let(:content_json) { { 'CONTENT_TYPE' => 'application/json' } }