Merge pull request #4202 from tomhughes/changeset-comment-limit

Add rate limiting for changeset comments

diff --git a/app/assets/javascripts/index/changeset.js b/app/assets/javascripts/index/changeset.js
index 9e38917afeda29892b0ebcfa4df68c172234ceeb..a6213b9c434367a9141062be608e1a2b53d49bf0 100644 (file)
--- 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 8b971834d76432d2728a2d7c0bd059691a9328e4..a9e80630e22226447bc9797f363ac2b854801987 100644 (file)
--- a/app/controllers/api/changeset_comments_controller.rb
+++ b/app/controllers/api/changeset_comments_controller.rb
@@ -17,6 +17,7 @@ module Api
       # 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 5c21736b0f6a9cbe031056cf4ef3ebaeae297cc1..fe2a98d61e31f6af42e6a085bdcf19e93a21d546 100644 (file)
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -395,6 +395,19 @@ class User < ApplicationRecord
     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 3c5ba7de0dd33868d28e5e3ba616f4d595dd5a68..b5c360a03db4c15fa80cc1db03fd176a40d856c7 100644 (file)
--- a/app/views/browse/changeset.html.erb
+++ b/app/views/browse/changeset.html.erb
@@ -78,6 +78,8 @@
         <div class="mb-3">
           <textarea class="form-control" name="text" cols="40" rows="5"></textarea>
         </div>
+        <div id="comment-error" class="alert-danger p-2 mb-3" hidden>
+        </div>
         <div>
           <input type="submit" name="comment" value="<%= t("javascripts.changesets.show.comment") %>" data-changeset-id="<%= @changeset.id %>" data-method="POST" data-url="<%= changeset_comment_url(@changeset) %>" disabled="1" class="btn btn-sm btn-primary" />
         </div>

diff --git a/config/settings.yml b/config/settings.yml
index 8ac27df4085d881493962a11a5d505ccd4eec94c..5330ed678c47333acd0e5ca388709da14100227b 100644 (file)
--- 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 (file)
index 0000000..8174132
--- /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 86537003fc4627ef768adb6b84e8bf88f4936438..1874e64616154aa4e24c41beb9d6403dcb709a70 100644 (file)
--- 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 4241ad700a575c0bc9b6e088b97c1cf5137467fe..6d945c4fe871250f404af3926a1813cfa7858881 100644 (file)
--- a/lib/osm.rb
+++ b/lib/osm.rb
@@ -353,6 +353,13 @@ module OSM
     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 26500babdcb24f47b727c3be384ecc084a668db8..624b8a35808645c1964c3b7fd40aa5fa5f46b262 100644 (file)
--- a/test/controllers/api/changeset_comments_controller_test.rb
+++ b/test/controllers/api/changeset_comments_controller_test.rb
@@ -132,6 +132,27 @@ module Api
       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