From a274726f468b482b1553dd8fbbc3e3bc8f6a7a5f Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Fri, 25 Aug 2023 19:21:53 +0100 Subject: [PATCH] Add rate limiting for changeset comments Fixes #4196 --- app/assets/javascripts/index/changeset.js | 6 ++++++ .../api/changeset_comments_controller.rb | 1 + app/models/user.rb | 13 ++++++++++++ app/views/browse/changeset.html.erb | 2 ++ config/settings.yml | 3 +++ ...tore_author_index_to_changeset_comments.rb | 7 +++++++ db/structure.sql | 8 +++++++ lib/osm.rb | 7 +++++++ .../api/changeset_comments_controller_test.rb | 21 +++++++++++++++++++ 9 files changed, 68 insertions(+) create mode 100644 db/migrate/20230825162137_restore_author_index_to_changeset_comments.rb diff --git a/app/assets/javascripts/index/changeset.js b/app/assets/javascripts/index/changeset.js index 9e38917afe..a6213b9c43 100644 --- a/app/assets/javascripts/index/changeset.js +++ b/app/assets/javascripts/index/changeset.js @@ -29,6 +29,7 @@ OSM.Changeset = function (map) { function updateChangeset(form, method, url, include_data) { var data; + $(form).find("#comment-error").prop("hidden", true); $(form).find("input[type=submit]").prop("disabled", true); if (include_data) { @@ -44,6 +45,11 @@ OSM.Changeset = function (map) { data: data, success: function () { OSM.loadSidebarContent(window.location.pathname, page.load); + }, + error: function (xhr, xhr_status, http_status) { + $(form).find("#comment-error").text(http_status); + $(form).find("#comment-error").prop("hidden", false); + $(form).find("input[type=submit]").prop("disabled", false); } }); } diff --git a/app/controllers/api/changeset_comments_controller.rb b/app/controllers/api/changeset_comments_controller.rb index 8b971834d7..a9e80630e2 100644 --- a/app/controllers/api/changeset_comments_controller.rb +++ b/app/controllers/api/changeset_comments_controller.rb @@ -17,6 +17,7 @@ def create # Check the arguments are sane raise OSM::APIBadUserInput, "No id was given" unless params[:id] raise OSM::APIBadUserInput, "No text was given" if params[:text].blank? + raise OSM::APIRateLimitExceeded if current_user.changeset_comments.where("created_at >= ?", Time.now.utc - 1.hour).count >= current_user.max_changeset_comments_per_hour # Extract the arguments id = params[:id].to_i diff --git a/app/models/user.rb b/app/models/user.rb index 5c21736b0f..fe2a98d61e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -395,6 +395,19 @@ def max_friends_per_hour max_friends.clamp(0, Settings.max_friends_per_hour) end + def max_changeset_comments_per_hour + if moderator? + 36000 + else + previous_comments = changeset_comments.limit(200).count + active_reports = issues.with_status(:open).sum(:reports_count) + max_comments = previous_comments / 200.0 * Settings.max_changeset_comments_per_hour + max_comments = max_comments.floor.clamp(Settings.min_changeset_comments_per_hour, Settings.max_changeset_comments_per_hour) + max_comments /= 2**active_reports + max_comments.floor.clamp(1, Settings.max_changeset_comments_per_hour) + end + end + private def encrypt_password diff --git a/app/views/browse/changeset.html.erb b/app/views/browse/changeset.html.erb index 3c5ba7de0d..b5c360a03d 100644 --- a/app/views/browse/changeset.html.erb +++ b/app/views/browse/changeset.html.erb @@ -78,6 +78,8 @@
+
" data-changeset-id="<%= @changeset.id %>" data-method="POST" data-url="<%= changeset_comment_url(@changeset) %>" disabled="1" class="btn btn-sm btn-primary" />
diff --git a/config/settings.yml b/config/settings.yml index d9910ce28a..1a1d137cda 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -55,6 +55,9 @@ user_block_periods: [0, 1, 3, 6, 12, 24, 48, 96, 168, 336, 731, 4383, 8766, 8766 max_messages_per_hour: 60 # Rate limit for friending max_friends_per_hour: 60 +# Rate limit for changeset comments +min_changeset_comments_per_hour: 6 +max_changeset_comments_per_hour: 60 # Domain for handling message replies #messages_domain: "messages.openstreetmap.org" # MaxMind GeoIPv2 database diff --git a/db/migrate/20230825162137_restore_author_index_to_changeset_comments.rb b/db/migrate/20230825162137_restore_author_index_to_changeset_comments.rb new file mode 100644 index 0000000000..81741322ea --- /dev/null +++ b/db/migrate/20230825162137_restore_author_index_to_changeset_comments.rb @@ -0,0 +1,7 @@ +class RestoreAuthorIndexToChangesetComments < ActiveRecord::Migration[7.0] + disable_ddl_transaction! + + def change + add_index :changeset_comments, [:author_id, :created_at], :algorithm => :concurrently + end +end diff --git a/db/structure.sql b/db/structure.sql index 86537003fc..1874e64616 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -2370,6 +2370,13 @@ CREATE UNIQUE INDEX index_active_storage_blobs_on_key ON public.active_storage_b CREATE UNIQUE INDEX index_active_storage_variant_records_uniqueness ON public.active_storage_variant_records USING btree (blob_id, variation_digest); +-- +-- Name: index_changeset_comments_on_author_id_and_created_at; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_changeset_comments_on_author_id_and_created_at ON public.changeset_comments USING btree (author_id, created_at); + + -- -- Name: index_changeset_comments_on_changeset_id_and_created_at; Type: INDEX; Schema: public; Owner: - -- @@ -3396,6 +3403,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20220201183346'), ('20220223140543'), ('20230816135800'), +('20230825162137'), ('21'), ('22'), ('23'), diff --git a/lib/osm.rb b/lib/osm.rb index 4241ad700a..6d945c4fe8 100644 --- a/lib/osm.rb +++ b/lib/osm.rb @@ -353,6 +353,13 @@ def status end end + # Raised when a rate limit is exceeded + class APIRateLimitExceeded < APIError + def status + :too_many_requests + end + end + # Helper methods for going to/from mercator and lat/lng. class Mercator include Math diff --git a/test/controllers/api/changeset_comments_controller_test.rb b/test/controllers/api/changeset_comments_controller_test.rb index 26500babdc..624b8a3580 100644 --- a/test/controllers/api/changeset_comments_controller_test.rb +++ b/test/controllers/api/changeset_comments_controller_test.rb @@ -132,6 +132,27 @@ def test_create_comment_fail assert_response :bad_request end + ## + # create comment rate limit + def test_create_comment_rate_limit + changeset = create(:changeset, :closed) + user = create(:user) + + auth_header = basic_authorization_header user.email, "test" + + assert_difference "ChangesetComment.count", Settings.min_changeset_comments_per_hour do + 1.upto(Settings.min_changeset_comments_per_hour) do |count| + post changeset_comment_path(:id => changeset, :text => "Comment #{count}"), :headers => auth_header + assert_response :success + end + end + + assert_no_difference "ChangesetComment.count" do + post changeset_comment_path(:id => changeset, :text => "One comment too many"), :headers => auth_header + assert_response :too_many_requests + end + end + ## # test hide comment fail def test_destroy_comment_fail