FEATURE: rate limit anon searches per second (#19708)

app/assets/javascripts/discourse/app/controllers/full-page-search.js

@@ -63,6 +63,7 @@ export default Controller.extend({
   resultCount: null,
   searchTypes: null,
   selected: [],
+  error: null,
 
   init() {
     this._super(...arguments);
@@ -382,6 +383,10 @@ export default Controller.extend({
               model.grouped_search_result = results.grouped_search_result;
               this.set("model", model);
             }
+            this.set("error", null);
+          })
+          .catch((e) => {
+            this.set("error", e.jqXHR.responseJSON?.message);
           })
           .finally(() => {
             this.setProperties({

app/assets/javascripts/discourse/app/templates/full-page-search.hbs

@@ -163,9 +163,9 @@
                 {{#if this.searchActive}}
                   <h3>{{i18n "search.no_results"}}</h3>
 
-                  {{#if this.model.grouped_search_result.error}}
+                  {{#if this.error}}
                     <div class="warning">
-                      {{this.model.grouped_search_result.error}}
+                      {{this.error}}
                     </div>
                   {{/if}}
 

app/controllers/search_controller.rb

@@ -29,8 +29,6 @@ class SearchController < ApplicationController
     # check for a malformed page parameter
     raise Discourse::InvalidParameters if page && (!page.is_a?(String) || page.to_i.to_s != page)
 
-    rate_limit_errors = rate_limit_search
-
     discourse_expires_in 1.minute
 
     search_args = {
@@ -50,15 +48,11 @@ class SearchController < ApplicationController
     search_args[:ip_address] = request.remote_ip
     search_args[:user_id] = current_user.id if current_user.present?
 
-    if rate_limit_errors
+    if rate_limit_search
-      result =
+      return(
-        Search::GroupedSearchResults.new(
+        render json: failed_json.merge(message: I18n.t("rate_limiter.slow_down")),
-          type_filter: search_args[:type_filter],
+               status: :too_many_requests
-          term: @search_term,
+      )
-          search_context: context,
-        )
-
-      result.error = I18n.t("rate_limiter.slow_down")
     elsif site_overloaded?
       result =
         Search::GroupedSearchResults.new(
@@ -89,8 +83,6 @@ class SearchController < ApplicationController
       raise Discourse::InvalidParameters.new("string contains null byte")
     end
 
-    rate_limit_errors = rate_limit_search
-
     discourse_expires_in 1.minute
 
     search_args = { guardian: guardian }
@@ -112,15 +104,11 @@ class SearchController < ApplicationController
       :restrict_to_archetype
     ].present?
 
-    if rate_limit_errors
+    if rate_limit_search
-      result =
+      return(
-        Search::GroupedSearchResults.new(
+        render json: failed_json.merge(message: I18n.t("rate_limiter.slow_down")),
-          type_filter: search_args[:type_filter],
+               status: :too_many_requests
-          term: params[:term],
+      )
-          search_context: context,
-        )
-
-      result.error = I18n.t("rate_limiter.slow_down")
     elsif site_overloaded?
       result =
         GroupedSearchResults.new(
@@ -188,14 +176,26 @@ class SearchController < ApplicationController
       else
         RateLimiter.new(
           nil,
-          "search-min-#{request.remote_ip}",
+          "search-min-#{request.remote_ip}-per-sec",
-          SiteSetting.rate_limit_search_anon_user,
+          SiteSetting.rate_limit_search_anon_user_per_second,
+          1.second,
+        ).performed!
+        RateLimiter.new(
+          nil,
+          "search-min-#{request.remote_ip}-per-min",
+          SiteSetting.rate_limit_search_anon_user_per_minute,
           1.minute,
         ).performed!
         RateLimiter.new(
           nil,
-          "search-min-anon-global",
+          "search-min-anon-global-per-sec",
-          SiteSetting.rate_limit_search_anon_global,
+          SiteSetting.rate_limit_search_anon_global_per_second,
+          1.second,
+        ).performed!
+        RateLimiter.new(
+          nil,
+          "search-min-anon-global-per-min",
+          SiteSetting.rate_limit_search_anon_global_per_minute,
           1.minute,
         ).performed!
       end

config/site_settings.yml

@@ -1921,12 +1921,18 @@ rate_limits:
   rate_limit_create_post: 5
   rate_limit_new_user_create_topic: 120
   rate_limit_new_user_create_post: 30
-  rate_limit_search_anon_global:
+  rate_limit_search_anon_global_per_minute:
     hidden: true
     default: 150
-  rate_limit_search_anon_user:
+  rate_limit_search_anon_user_per_minute:
     hidden: true
     default: 15
+  rate_limit_search_anon_global_per_second:
+    hidden: true
+    default: 8
+  rate_limit_search_anon_user_per_second:
+    hidden: true
+    default: 2
   rate_limit_search_user:
     hidden: true
     default: 30

db/migrate/20230104054425_rename_rate_limit_search_anon.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+class RenameRateLimitSearchAnon < ActiveRecord::Migration[7.0]
+  RENAME_SETTINGS = [
+    %w[rate_limit_search_anon_user rate_limit_search_anon_user_per_minute],
+    %w[rate_limit_search_anon_global rate_limit_search_anon_global_per_minute],
+  ]
+
+  def up
+    # Copying the rows so that things keep working during deploy
+    # They will be dropped in post_migrate/..delete_old_rate_limit_search_anon
+    #
+    RENAME_SETTINGS.each { |old_name, new_name| execute <<~SQL }
+        INSERT INTO site_settings (name, data_type, value, created_at, updated_at)
+        SELECT '#{new_name}', data_type, value, created_at, updated_at
+        FROM site_settings
+        WHERE name = '#{old_name}'
+      SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/post_migrate/20230104054426_delete_old_rate_limit_search_anon.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class DeleteOldRateLimitSearchAnon < ActiveRecord::Migration[7.0]
+  def change
+    execute "DELETE FROM site_settings WHERE name in ('rate_limit_search_anon_user', 'rate_limit_search_anon_global')"
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/requests/search_controller_spec.rb

@@ -229,42 +229,60 @@ RSpec.describe SearchController do
     end
 
     context "when rate limited" do
+      def unlimited_request(ip_address = "1.2.3.4")
+        get "/search/query.json", params: { term: "wookie" }, env: { REMOTE_ADDR: ip_address }
+
+        expect(response.status).to eq(200)
+        json = response.parsed_body
+        expect(json["grouped_search_result"]["error"]).to eq(nil)
+      end
+
+      def limited_request(ip_address = "1.2.3.4")
+        get "/search/query.json", params: { term: "wookie" }, env: { REMOTE_ADDR: ip_address }
+        expect(response.status).to eq(429)
+        json = response.parsed_body
+        expect(json["message"]).to eq(I18n.t("rate_limiter.slow_down"))
+      end
+
       it "rate limits anon searches per user" do
-        SiteSetting.rate_limit_search_anon_user = 2
+        SiteSetting.rate_limit_search_anon_user_per_second = 2
+        SiteSetting.rate_limit_search_anon_user_per_minute = 3
         RateLimiter.enable
         RateLimiter.clear_all!
 
-        2.times do
+        start = Time.now
-          get "/search/query.json", params: { term: "wookie" }
+        freeze_time start
 
-          expect(response.status).to eq(200)
+        unlimited_request
-          json = response.parsed_body
+        unlimited_request
-          expect(json["grouped_search_result"]["error"]).to eq(nil)
+        limited_request
-        end
 
-        get "/search/query.json", params: { term: "wookie" }
+        freeze_time start + 2
-        expect(response.status).to eq(200)
+
-        json = response.parsed_body
+        unlimited_request
-        expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+        limited_request
+
+        # cause it is a diff IP
+        unlimited_request("100.0.0.0")
       end
 
       it "rate limits anon searches globally" do
-        SiteSetting.rate_limit_search_anon_global = 2
+        SiteSetting.rate_limit_search_anon_global_per_second = 2
+        SiteSetting.rate_limit_search_anon_global_per_minute = 3
         RateLimiter.enable
         RateLimiter.clear_all!
 
-        2.times do
+        t = Time.now
-          get "/search/query.json", params: { term: "wookie" }
+        freeze_time t
 
-          expect(response.status).to eq(200)
+        unlimited_request("1.2.3.4")
-          json = response.parsed_body
+        unlimited_request("1.2.3.5")
-          expect(json["grouped_search_result"]["error"]).to eq(nil)
+        limited_request("1.2.3.6")
-        end
 
-        get "/search/query.json", params: { term: "wookie" }
+        freeze_time t + 2
-        expect(response.status).to eq(200)
+
-        json = response.parsed_body
+        unlimited_request("1.2.3.7")
-        expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+        limited_request("1.2.3.8")
       end
 
       context "with a logged in user" do
@@ -284,9 +302,9 @@ RSpec.describe SearchController do
           end
 
           get "/search/query.json", params: { term: "wookie" }
-          expect(response.status).to eq(200)
+          expect(response.status).to eq(429)
           json = response.parsed_body
-          expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+          expect(json["message"]).to eq(I18n.t("rate_limiter.slow_down"))
         end
       end
     end
@@ -350,42 +368,57 @@ RSpec.describe SearchController do
     end
 
     context "when rate limited" do
+      def unlimited_request(ip_address = "1.2.3.4")
+        get "/search.json", params: { q: "wookie" }, env: { REMOTE_ADDR: ip_address }
+
+        expect(response.status).to eq(200)
+        json = response.parsed_body
+        expect(json["grouped_search_result"]["error"]).to eq(nil)
+      end
+
+      def limited_request(ip_address = "1.2.3.4")
+        get "/search.json", params: { q: "wookie" }, env: { REMOTE_ADDR: ip_address }
+        expect(response.status).to eq(429)
+        json = response.parsed_body
+        expect(json["message"]).to eq(I18n.t("rate_limiter.slow_down"))
+      end
+
       it "rate limits anon searches per user" do
-        SiteSetting.rate_limit_search_anon_user = 2
+        SiteSetting.rate_limit_search_anon_user_per_second = 2
+        SiteSetting.rate_limit_search_anon_user_per_minute = 3
         RateLimiter.enable
         RateLimiter.clear_all!
 
-        2.times do
+        t = Time.now
-          get "/search.json", params: { q: "bantha" }
+        freeze_time t
 
-          expect(response.status).to eq(200)
+        unlimited_request
-          json = response.parsed_body
+        unlimited_request
-          expect(json["grouped_search_result"]["error"]).to eq(nil)
+        limited_request
-        end
 
-        get "/search.json", params: { q: "bantha" }
+        freeze_time(t + 2)
-        expect(response.status).to eq(200)
+
-        json = response.parsed_body
+        unlimited_request
-        expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+        limited_request
+        unlimited_request("1.2.3.100")
       end
 
       it "rate limits anon searches globally" do
-        SiteSetting.rate_limit_search_anon_global = 2
+        SiteSetting.rate_limit_search_anon_global_per_second = 2
+        SiteSetting.rate_limit_search_anon_global_per_minute = 3
         RateLimiter.enable
         RateLimiter.clear_all!
 
-        2.times do
+        unlimited_request("1.1.1.1")
-          get "/search.json", params: { q: "bantha" }
+        unlimited_request("2.2.2.2")
+        limited_request("3.3.3.3")
 
-          expect(response.status).to eq(200)
+        t = Time.now
-          json = response.parsed_body
+        freeze_time t
-          expect(json["grouped_search_result"]["error"]).to eq(nil)
+        freeze_time(t + 2)
-        end
 
-        get "/search.json", params: { q: "bantha" }
+        unlimited_request("4.4.4.4")
-        expect(response.status).to eq(200)
+        limited_request("5.5.5.5")
-        json = response.parsed_body
-        expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
       end
 
       context "with a logged in user" do
@@ -405,9 +438,9 @@ RSpec.describe SearchController do
           end
 
           get "/search.json", params: { q: "bantha" }
-          expect(response.status).to eq(200)
+          expect(response.status).to eq(429)
           json = response.parsed_body
-          expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+          expect(json["message"]).to eq(I18n.t("rate_limiter.slow_down"))
         end
       end
     end

spec/system/page_objects/pages/search.rb

@@ -8,6 +8,11 @@ module PageObjects
         self
       end
 
+      def clear_search_input
+        find("input.full-page-search").set("")
+        self
+      end
+
       def heading_text
         find("h1.search-page-heading").text
       end
@@ -28,6 +33,10 @@ module PageObjects
         within(".search-results") { page.has_selector?(".fps-result", visible: true) }
       end
 
+      def has_warning_message?
+        within(".search-results") { page.has_selector?(".warning", visible: true) }
+      end
+
       def is_search_page
         has_css?("body.search-page")
       end

spec/system/search_spec.rb

@@ -36,4 +36,46 @@ describe "Search", type: :system, js: true do
       expect(search_page.heading_text).to eq("Search")
     end
   end
+
+  describe "when using full page search on desktop" do
+    before do
+      SearchIndexer.enable
+      SearchIndexer.index(topic, force: true)
+      SiteSetting.rate_limit_search_anon_user_per_minute = 15
+      RateLimiter.enable
+    end
+
+    after { SearchIndexer.disable }
+
+    it "rate limits searches for anonymous users" do
+      queries = %w[
+        one
+        two
+        three
+        four
+        five
+        six
+        seven
+        eight
+        nine
+        ten
+        eleven
+        twelve
+        thirteen
+        fourteen
+        fifteen
+      ]
+
+      visit("/search?expanded=true")
+
+      queries.each do |query|
+        search_page.clear_search_input
+        search_page.type_in_search(query)
+        search_page.click_search_button
+      end
+
+      # Rate limit error should kick in after 15 queries
+      expect(search_page).to have_warning_message
+    end
+  end
 end