[deploy] Rate Limit Feedback Message Creation (#7832)

app/controllers/application_controller.rb

@@ -92,5 +92,11 @@ class ApplicationController < ActionController::Base
     rate_limiter.check_limit!(action)
   end
 
-  delegate :rate_limiter, to: :current_user
+  def rate_limiter
+    (current_user || anonymous_user).rate_limiter
+  end
+
+  def anonymous_user
+    User.new(ip_address: request.env["HTTP_FASTLY_CLIENT_IP"])
+  end
 end

app/controllers/feedback_messages_controller.rb

@@ -4,6 +4,7 @@ class FeedbackMessagesController < ApplicationController
 
   def create
     flash.clear
+    rate_limit!(:feedback_message_creation)
 
     params = feedback_message_params.merge(reporter_id: current_user&.id)
     @feedback_message = FeedbackMessage.new(params)
@@ -16,6 +17,7 @@ class FeedbackMessagesController < ApplicationController
         reported_url: feedback_message_params[:reported_url],
         message: feedback_message_params[:message],
       )
+      rate_limiter.track_limit_by_action(:feedback_message_creation)
 
       redirect_to feedback_messages_path
     else

app/helpers/rate_limit_checker_helper.rb

@@ -5,6 +5,11 @@ module RateLimitCheckerHelper
       placeholder: 150,
       description: "The number of article updates a user can make in 30 seconds"
     },
+    rate_limit_feedback_message_creation: {
+      min: 1,
+      placeholder: 5,
+      description: "The number of times a user can submit feedback in a 5 minute period"
+    },
     rate_limit_follow_count_daily: {
       min: 0,
       placeholder: 500,

app/models/site_config.rb

@@ -83,6 +83,7 @@ class SiteConfig < RailsSettings::Base
   field :rate_limit_email_recipient, type: :integer, default: 5
   field :rate_limit_article_update, type: :integer, default: 150
   field :rate_limit_send_email_confirmation, type: :integer, default: 2
+  field :rate_limit_feedback_message_creation, type: :integer, default: 5
 
   # Google Analytics Reporting API v4
   # <https://developers.google.com/analytics/devguides/reporting/core/v4>

app/models/user.rb

@@ -30,10 +30,9 @@ class User < ApplicationRecord
     reserved_username: "username is reserved"
   }.freeze
 
-  attr_accessor(
-    :scholar_email, :new_note, :note_for_current_role, :user_status, :pro, :merge_user_id,
-    :add_credits, :remove_credits, :add_org_credits, :remove_org_credits, :ghostify
-  )
+  attr_accessor :scholar_email, :new_note, :note_for_current_role, :user_status, :pro, :merge_user_id,
+                :add_credits, :remove_credits, :add_org_credits, :remove_org_credits, :ghostify,
+                :ip_address
 
   rolify after_add: :index_roles, after_remove: :index_roles
 

app/services/rate_limit_checker.rb

@@ -5,6 +5,7 @@ class RateLimitChecker
   ACTION_LIMITERS = {
     article_update: { retry_after: 30 },
     send_email_confirmation: { retry_after: 120 },
+    feedback_message_creation: { retry_after: 300 },
     image_upload: { retry_after: 30 },
     listing_creation: { retry_after: 60 },
     published_article_creation: { retry_after: 30 },
@@ -66,9 +67,10 @@ class RateLimitChecker
   end
 
   def limit_cache_key(action)
-    raise "Invalid Cache Key: user ID can't be blank" unless @user.id
+    unique_key_component = @user&.id || @user&.ip_address
+    raise "Invalid Cache Key: no unique component present" if unique_key_component.blank?
 
-    "#{@user.id}_#{action}"
+    "#{unique_key_component}_#{action}"
   end
 
   def action_rate_limit(action)

spec/requests/feedback_messages_spec.rb

@@ -19,6 +19,8 @@ RSpec.describe "feedback_messages", type: :request do
       }
     }
 
+    headers = { "HTTP_FASTLY_CLIENT_IP" => "5.6.7.8" }
+
     context "with valid params" do
       before do
         mock_recaptcha_verification
@@ -26,7 +28,7 @@ RSpec.describe "feedback_messages", type: :request do
 
       it "creates a feedback message" do
         expect do
-          post feedback_messages_path, params: valid_abuse_report_params
+          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
         end.to change(FeedbackMessage, :count).by(1)
 
         feedback_message = FeedbackMessage.last
@@ -37,24 +39,36 @@ RSpec.describe "feedback_messages", type: :request do
 
       it "queues a slack message to be sent" do
         sidekiq_assert_enqueued_with(job: Slack::Messengers::Worker) do
-          post feedback_messages_path, params: valid_abuse_report_params
+          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
         end
       end
     end
 
     context "with invalid recaptcha" do
       it "rerenders page" do
-        post "/feedback_messages", params: valid_abuse_report_params
+        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
         expect(response.body).to include("Make sure the forms are filled")
       end
 
       it "queues a slack message to be sent" do
         sidekiq_assert_no_enqueued_jobs(only: Slack::Messengers::Worker) do
-          post feedback_messages_path, params: valid_abuse_report_params
+          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
         end
       end
     end
 
+    context "when rate limit is reached" do
+      it "returns a 429" do
+        user = create(:user)
+        limiter = user.rate_limiter
+        allow(RateLimitChecker).to receive(:new) { limiter }
+        allow(limiter).to receive(:limit_by_action).and_return(true)
+
+        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
+        expect(response.status).to eq(429)
+      end
+    end
+
     context "when a user submits a report" do
       let(:user) { create(:user) }
 
@@ -65,14 +79,14 @@ RSpec.describe "feedback_messages", type: :request do
       end
 
       it "creates a feedback message reported by the user" do
-        post feedback_messages_path, params: valid_abuse_report_params
+        post feedback_messages_path, params: valid_abuse_report_params, headers: headers
 
         expect(FeedbackMessage.exists?(reporter_id: user.id)).to be(true)
       end
 
       it "queues a slack message to be sent" do
         sidekiq_assert_enqueued_jobs(1, only: Slack::Messengers::Worker) do
-          post feedback_messages_path, params: valid_abuse_report_params
+          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
         end
       end
     end
@@ -83,25 +97,25 @@ RSpec.describe "feedback_messages", type: :request do
       end
 
       it "does not add any user as the reporter" do
-        post "/feedback_messages", params: valid_abuse_report_params
+        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
 
         expect(FeedbackMessage.last.reporter).to be(nil)
       end
 
       it "queues a slack message to be sent" do
         sidekiq_assert_enqueued_jobs(1, only: Slack::Messengers::Worker) do
-          post feedback_messages_path, params: valid_abuse_report_params
+          post feedback_messages_path, params: valid_abuse_report_params, headers: headers
         end
       end
 
       it "redirects to the index page" do
-        post "/feedback_messages", params: valid_abuse_report_params
+        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
 
         expect(response).to redirect_to(feedback_messages_path)
       end
 
       it "redirects and continues to the index page with the correct message" do
-        post "/feedback_messages", params: valid_abuse_report_params
+        post "/feedback_messages", params: valid_abuse_report_params, headers: headers
 
         follow_redirect!
 

spec/services/rate_limit_checker_spec.rb

@@ -14,10 +14,16 @@ RSpec.describe RateLimitChecker, type: :service do
       expect(rate_limit_checker.limit_by_action("random-nothing")).to be(false)
     end
 
-    it "raises an error if user does not have an ID" do
+    it "will limit action by ip_address if present" do
+      action = described_class::ACTION_LIMITERS.keys.first
+      limiter = described_class.new(build(:user, ip_address: "1.1.1.1"))
+      expect { limiter.limit_by_action(action) }.not_to raise_error
+    end
+
+    it "raises an error if no unique component is present for a cache key" do
       action = described_class::ACTION_LIMITERS.keys.first
       limiter = described_class.new(build(:user))
-      expect { limiter.limit_by_action(action) }.to raise_error("Invalid Cache Key: user ID can't be blank")
+      expect { limiter.limit_by_action(action) }.to raise_error("Invalid Cache Key: no unique component present")
     end
 
     # published_article_creation limit we check against the database rather than our cache