Merge branch '344807-refactor-rackattack-predicates' into 'master'

Always return booleans from RackAttack request predicate methods

See merge request gitlab-org/gitlab!79528

ee/lib/ee/gitlab/rack_attack/request.rb

@@ -12,7 +12,11 @@ module EE
         end
 
         def geo?
-          ::Gitlab::Geo::JwtRequestDecoder.geo_auth_attempt?(env['HTTP_AUTHORIZATION']) if env['HTTP_AUTHORIZATION']
+          if env['HTTP_AUTHORIZATION']
+            ::Gitlab::Geo::JwtRequestDecoder.geo_auth_attempt?(env['HTTP_AUTHORIZATION'])
+          else
+            false
+          end
         end
       end
     end

ee/spec/lib/ee/gitlab/rack_attack/request_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::RackAttack::Request do
+  using RSpec::Parameterized::TableSyntax
+
+  let(:path) { '/' }
+  let(:env) { {} }
+  let(:request) do
+    ::Rack::Attack::Request.new(
+      env.reverse_merge(
+        'REQUEST_METHOD' => 'GET',
+        'PATH_INFO' => path,
+        'rack.input' => StringIO.new
+      )
+    )
+  end
+
+  describe '#should_be_skipped?' do
+    where(
+      super_value: [true, false],
+      geo: [true, false]
+    )
+
+    with_them do
+      it 'returns true if any condition is true' do
+        allow(request).to receive(:api_internal_request?).and_return(super_value)
+        allow(request).to receive(:health_check_request?).and_return(super_value)
+        allow(request).to receive(:container_registry_event?).and_return(super_value)
+        allow(request).to receive(:geo?).and_return(geo)
+
+        expect(request.should_be_skipped?).to be(super_value || geo)
+      end
+    end
+  end
+
+  describe '#geo?' do
+    subject { request.geo? }
+
+    where(:env, :geo_auth_attempt, :expected) do
+      {}                                   | false | false
+      {}                                   | true  | false
+      { 'HTTP_AUTHORIZATION' => 'secret' } | false | false
+      { 'HTTP_AUTHORIZATION' => 'secret' } | true  | true
+    end
+
+    with_them do
+      before do
+        allow(Gitlab::Geo::JwtRequestDecoder).to receive(:geo_auth_attempt?).and_return(geo_auth_attempt)
+      end
+
+      it { is_expected.to be(expected) }
+    end
+  end
+end

lib/gitlab/rack_attack/request.rb

@@ -30,15 +30,15 @@ module Gitlab
       end
 
       def api_internal_request?
-        path =~ %r{^/api/v\d+/internal/}
+        path.match?(%r{^/api/v\d+/internal/})
       end
 
       def health_check_request?
-        path =~ %r{^/-/(health|liveness|readiness|metrics)}
+        path.match?(%r{^/-/(health|liveness|readiness|metrics)})
       end
 
       def container_registry_event?
-        path =~ %r{^/api/v\d+/container_registry_event/}
+        path.match?(%r{^/api/v\d+/container_registry_event/})
       end
 
       def product_analytics_collector_request?
@@ -54,11 +54,7 @@ module Gitlab
       end
 
       def protected_path?
-        !protected_path_regex.nil?
-      end
-
-      def protected_path_regex
-        path =~ protected_paths_regex
+        path.match?(protected_paths_regex)
       end
 
       def throttle?(throttle, authenticated:)
@@ -178,15 +174,15 @@ module Gitlab
       end
 
       def packages_api_path?
-        path =~ ::Gitlab::Regex::Packages::API_PATH_REGEX
+        path.match?(::Gitlab::Regex::Packages::API_PATH_REGEX)
       end
 
       def git_lfs_path?
-        path =~ Gitlab::PathRegex.repository_git_lfs_route_regex
+        path.match?(::Gitlab::PathRegex.repository_git_lfs_route_regex)
       end
 
       def files_api_path?
-        path =~ FILES_PATH_REGEX
+        path.match?(FILES_PATH_REGEX)
       end
 
       def deprecated_api_request?
@@ -195,7 +191,7 @@ module Gitlab
         with_projects = params['with_projects']
         with_projects = true if with_projects.blank?
 
-        path =~ GROUP_PATH_REGEX && Gitlab::Utils.to_boolean(with_projects)
+        path.match?(GROUP_PATH_REGEX) && Gitlab::Utils.to_boolean(with_projects)
       end
     end
   end

spec/lib/gitlab/rack_attack/request_spec.rb

@@ -5,6 +5,20 @@ require 'spec_helper'
 RSpec.describe Gitlab::RackAttack::Request do
   using RSpec::Parameterized::TableSyntax
 
+  let(:path) { '/' }
+  let(:env) { {} }
+  let(:session) { {} }
+  let(:request) do
+    ::Rack::Attack::Request.new(
+      env.reverse_merge(
+        'REQUEST_METHOD' => 'GET',
+        'PATH_INFO' => path,
+        'rack.input' => StringIO.new,
+        'rack.session' => session
+      )
+    )
+  end
+
   describe 'FILES_PATH_REGEX' do
     subject { described_class::FILES_PATH_REGEX }
 
@@ -16,11 +30,172 @@ RSpec.describe Gitlab::RackAttack::Request do
     it { is_expected.not_to match('/api/v4/projects/some/nested/repo/repository/files/README') }
   end
 
+  describe '#api_request?' do
+    subject { request.api_request? }
+
+    where(:path, :expected) do
+      '/'        | false
+      '/groups'  | false
+      '/foo/api' | false
+
+      '/api'             | true
+      '/api/v4/groups/1' | true
+    end
+
+    with_them do
+      it { is_expected.to eq(expected) }
+    end
+  end
+
+  describe '#api_internal_request?' do
+    subject { request.api_internal_request? }
+
+    where(:path, :expected) do
+      '/'                    | false
+      '/groups'              | false
+      '/api'                 | false
+      '/api/v4/groups/1'     | false
+      '/api/v4/internal'     | false
+      '/foo/api/v4/internal' | false
+
+      '/api/v4/internal/'    | true
+      '/api/v4/internal/foo' | true
+      '/api/v1/internal/foo' | true
+    end
+
+    with_them do
+      it { is_expected.to eq(expected) }
+    end
+  end
+
+  describe '#health_check_request?' do
+    subject { request.health_check_request? }
+
+    where(:path, :expected) do
+      '/'             | false
+      '/groups'       | false
+      '/foo/-/health' | false
+
+      '/-/health'        | true
+      '/-/liveness'      | true
+      '/-/readiness'     | true
+      '/-/metrics'       | true
+      '/-/health/foo'    | true
+      '/-/liveness/foo'  | true
+      '/-/readiness/foo' | true
+      '/-/metrics/foo'   | true
+    end
+
+    with_them do
+      it { is_expected.to eq(expected) }
+    end
+  end
+
+  describe '#container_registry_event?' do
+    subject { request.container_registry_event? }
+
+    where(:path, :expected) do
+      '/'                                     | false
+      '/groups'                               | false
+      '/api/v4/container_registry_event'      | false
+      '/foo/api/v4/container_registry_event/' | false
+
+      '/api/v4/container_registry_event/'    | true
+      '/api/v4/container_registry_event/foo' | true
+      '/api/v1/container_registry_event/foo' | true
+    end
+
+    with_them do
+      it { is_expected.to eq(expected) }
+    end
+  end
+
+  describe '#product_analytics_collector_request?' do
+    subject { request.product_analytics_collector_request? }
+
+    where(:path, :expected) do
+      '/'                  | false
+      '/groups'            | false
+      '/-/collector'       | false
+      '/-/collector/foo'   | false
+      '/foo/-/collector/i' | false
+
+      '/-/collector/i'     | true
+      '/-/collector/ifoo'  | true
+      '/-/collector/i/foo' | true
+    end
+
+    with_them do
+      it { is_expected.to eq(expected) }
+    end
+  end
+
+  describe '#should_be_skipped?' do
+    where(
+      api_internal_request: [true, false],
+      health_check_request: [true, false],
+      container_registry_event: [true, false]
+    )
+
+    with_them do
+      it 'returns true if any condition is true' do
+        allow(request).to receive(:api_internal_request?).and_return(api_internal_request)
+        allow(request).to receive(:health_check_request?).and_return(health_check_request)
+        allow(request).to receive(:container_registry_event?).and_return(container_registry_event)
+
+        expect(request.should_be_skipped?).to be(api_internal_request || health_check_request || container_registry_event)
+      end
+    end
+  end
+
+  describe '#web_request?' do
+    subject { request.web_request? }
+
+    where(:path, :expected) do
+      '/'        | true
+      '/groups'  | true
+      '/foo/api' | true
+
+      '/api'             | false
+      '/api/v4/groups/1' | false
+    end
+
+    with_them do
+      it { is_expected.to eq(expected) }
+    end
+  end
+
+  describe '#protected_path?' do
+    subject { request.protected_path? }
+
+    before do
+      stub_application_setting(protected_paths: [
+        '/protected',
+        '/secure'
+      ])
+    end
+
+    where(:path, :expected) do
+      '/'              | false
+      '/groups'        | false
+      '/foo/protected' | false
+      '/foo/secure'    | false
+
+      '/protected'  | true
+      '/secure'     | true
+      '/secure/'    | true
+      '/secure/foo' | true
+    end
+
+    with_them do
+      it { is_expected.to eq(expected) }
+    end
+  end
+
   describe '#deprecated_api_request?' do
-    let(:env) { { 'REQUEST_METHOD' => 'GET', 'rack.input' => StringIO.new, 'PATH_INFO' => path, 'QUERY_STRING' => query } }
-    let(:request) { ::Rack::Attack::Request.new(env) }
+    subject { request.send(:deprecated_api_request?) }
 
-    subject { !!request.__send__(:deprecated_api_request?) }
+    let(:env) { { 'QUERY_STRING' => query } }
 
     where(:path, :query, :expected) do
       '/' | '' | false

spec/requests/rack_attack_global_spec.rb

@@ -184,6 +184,7 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
 
     context 'unauthenticated requests' do
       let(:protected_path_that_does_not_require_authentication) do
+        # This is one of the default values for `application_settings.protected_paths`
         '/users/sign_in'
       end
 
@@ -227,6 +228,20 @@ RSpec.describe 'Rack Attack global throttles', :use_clean_rails_memory_store_cac
           expect_rejection { post protected_path_that_does_not_require_authentication, params: post_params }
         end
 
+        it 'allows non-POST requests to protected paths over the rate limit' do
+          (1 + requests_per_period).times do
+            get protected_path_that_does_not_require_authentication
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+        end
+
+        it 'allows POST requests to unprotected paths over the rate limit' do
+          (1 + requests_per_period).times do
+            post '/api/graphql'
+            expect(response).to have_gitlab_http_status(:ok)
+          end
+        end
+
         it_behaves_like 'tracking when dry-run mode is set' do
           let(:throttle_name) { 'throttle_unauthenticated_protected_paths' }
         end