diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 3018dc9fc..15e22afb4 100644 --- a/app/controllers/application_controller.rb +++ b/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 diff --git a/app/controllers/feedback_messages_controller.rb b/app/controllers/feedback_messages_controller.rb index 0bbefea84..f397003bd 100644 --- a/app/controllers/feedback_messages_controller.rb +++ b/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 diff --git a/app/helpers/rate_limit_checker_helper.rb b/app/helpers/rate_limit_checker_helper.rb index 641a9c4a2..446cc38b2 100644 --- a/app/helpers/rate_limit_checker_helper.rb +++ b/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, diff --git a/app/models/site_config.rb b/app/models/site_config.rb index 0122dbe89..5ca1ad4a5 100644 --- a/app/models/site_config.rb +++ b/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 # diff --git a/app/models/user.rb b/app/models/user.rb index d49cee617..857629782 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/services/rate_limit_checker.rb b/app/services/rate_limit_checker.rb index 0a9b658bf..f544ed519 100644 --- a/app/services/rate_limit_checker.rb +++ b/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) diff --git a/spec/requests/feedback_messages_spec.rb b/spec/requests/feedback_messages_spec.rb index f5cbbce09..4367b0fb9 100644 --- a/spec/requests/feedback_messages_spec.rb +++ b/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! diff --git a/spec/services/rate_limit_checker_spec.rb b/spec/services/rate_limit_checker_spec.rb index 62943027e..9ecc5987c 100644 --- a/spec/services/rate_limit_checker_spec.rb +++ b/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