diff --git a/app/assets/javascripts/discourse/app/controllers/full-page-search.js b/app/assets/javascripts/discourse/app/controllers/full-page-search.js
index 3b778a2a07b..59157ac2646 100644
--- a/app/assets/javascripts/discourse/app/controllers/full-page-search.js
+++ b/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({
diff --git a/app/assets/javascripts/discourse/app/templates/full-page-search.hbs b/app/assets/javascripts/discourse/app/templates/full-page-search.hbs
index e735832cdf3..f860d768771 100644
--- a/app/assets/javascripts/discourse/app/templates/full-page-search.hbs
+++ b/app/assets/javascripts/discourse/app/templates/full-page-search.hbs
@@ -163,9 +163,9 @@
{{#if this.searchActive}}
{{i18n "search.no_results"}}
- {{#if this.model.grouped_search_result.error}}
+ {{#if this.error}}
- {{this.model.grouped_search_result.error}}
+ {{this.error}}
{{/if}}
diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb
index f626e620f28..b2574d91660 100644
--- a/app/controllers/search_controller.rb
+++ b/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
- result =
- Search::GroupedSearchResults.new(
- type_filter: search_args[:type_filter],
- term: @search_term,
- search_context: context,
- )
-
- result.error = I18n.t("rate_limiter.slow_down")
+ if rate_limit_search
+ return(
+ render json: failed_json.merge(message: I18n.t("rate_limiter.slow_down")),
+ status: :too_many_requests
+ )
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
- result =
- Search::GroupedSearchResults.new(
- type_filter: search_args[:type_filter],
- term: params[:term],
- search_context: context,
- )
-
- result.error = I18n.t("rate_limiter.slow_down")
+ if rate_limit_search
+ return(
+ render json: failed_json.merge(message: I18n.t("rate_limiter.slow_down")),
+ status: :too_many_requests
+ )
elsif site_overloaded?
result =
GroupedSearchResults.new(
@@ -188,14 +176,26 @@ class SearchController < ApplicationController
else
RateLimiter.new(
nil,
- "search-min-#{request.remote_ip}",
- SiteSetting.rate_limit_search_anon_user,
+ "search-min-#{request.remote_ip}-per-sec",
+ 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",
- SiteSetting.rate_limit_search_anon_global,
+ "search-min-anon-global-per-sec",
+ 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
diff --git a/config/site_settings.yml b/config/site_settings.yml
index e2764ab5796..8268a3537bc 100644
--- a/config/site_settings.yml
+++ b/config/site_settings.yml
@@ -1905,12 +1905,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
diff --git a/db/migrate/20230104054425_rename_rate_limit_search_anon.rb b/db/migrate/20230104054425_rename_rate_limit_search_anon.rb
new file mode 100644
index 00000000000..80ba703a8ff
--- /dev/null
+++ b/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
diff --git a/db/post_migrate/20230104054426_delete_old_rate_limit_search_anon.rb b/db/post_migrate/20230104054426_delete_old_rate_limit_search_anon.rb
new file mode 100644
index 00000000000..977d202d9db
--- /dev/null
+++ b/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
diff --git a/spec/requests/search_controller_spec.rb b/spec/requests/search_controller_spec.rb
index f3300e34724..52e22e26727 100644
--- a/spec/requests/search_controller_spec.rb
+++ b/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
- get "/search/query.json", params: { term: "wookie" }
+ start = Time.now
+ freeze_time start
- expect(response.status).to eq(200)
- json = response.parsed_body
- expect(json["grouped_search_result"]["error"]).to eq(nil)
- end
+ unlimited_request
+ unlimited_request
+ limited_request
- get "/search/query.json", params: { term: "wookie" }
- expect(response.status).to eq(200)
- json = response.parsed_body
- expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+ freeze_time start + 2
+
+ unlimited_request
+ 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
- get "/search/query.json", params: { term: "wookie" }
+ t = Time.now
+ freeze_time t
- expect(response.status).to eq(200)
- json = response.parsed_body
- expect(json["grouped_search_result"]["error"]).to eq(nil)
- end
+ unlimited_request("1.2.3.4")
+ unlimited_request("1.2.3.5")
+ limited_request("1.2.3.6")
- get "/search/query.json", params: { term: "wookie" }
- expect(response.status).to eq(200)
- json = response.parsed_body
- expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+ freeze_time t + 2
+
+ unlimited_request("1.2.3.7")
+ 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
- get "/search.json", params: { q: "bantha" }
+ t = Time.now
+ freeze_time t
- expect(response.status).to eq(200)
- json = response.parsed_body
- expect(json["grouped_search_result"]["error"]).to eq(nil)
- end
+ unlimited_request
+ unlimited_request
+ limited_request
- get "/search.json", params: { q: "bantha" }
- expect(response.status).to eq(200)
- json = response.parsed_body
- expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+ freeze_time(t + 2)
+
+ unlimited_request
+ 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
- get "/search.json", params: { q: "bantha" }
+ unlimited_request("1.1.1.1")
+ unlimited_request("2.2.2.2")
+ limited_request("3.3.3.3")
- expect(response.status).to eq(200)
- json = response.parsed_body
- expect(json["grouped_search_result"]["error"]).to eq(nil)
- end
+ t = Time.now
+ freeze_time t
+ freeze_time(t + 2)
- get "/search.json", params: { q: "bantha" }
- expect(response.status).to eq(200)
- json = response.parsed_body
- expect(json["grouped_search_result"]["error"]).to eq(I18n.t("rate_limiter.slow_down"))
+ unlimited_request("4.4.4.4")
+ limited_request("5.5.5.5")
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
diff --git a/spec/system/page_objects/pages/search.rb b/spec/system/page_objects/pages/search.rb
index 417f34ccc13..5e60f76cf67 100644
--- a/spec/system/page_objects/pages/search.rb
+++ b/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
diff --git a/spec/system/search_spec.rb b/spec/system/search_spec.rb
index f10b05a038d..1ec870c6ec9 100644
--- a/spec/system/search_spec.rb
+++ b/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