diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 8c8237bdd..af9d36964 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -8,7 +8,7 @@ class ApplicationController < ActionController::Base before_action :reset_current_user before_action :set_current_user before_action :normalize_search - before_action :api_check + before_action :check_rate_limit before_action :ip_ban_check before_action :set_variant before_action :add_headers @@ -69,18 +69,12 @@ class ApplicationController < ActionController::Base response.headers["X-Git-Hash"] = Rails.application.config.x.git_hash end - def api_check - return if CurrentUser.is_anonymous? || request.get? || request.head? - - rate_limiter = RateLimiter.new( - "write", - [CurrentUser.user.cache_key], - cost: 1, - rate: CurrentUser.user.api_regen_multiplier, - burst: CurrentUser.user.api_burst_limit - ) + def check_rate_limit + return if request.get? || request.head? + rate_limiter = RateLimiter.for_action(controller_name, action_name, CurrentUser.user, CurrentUser.ip_addr) headers["X-Rate-Limit"] = rate_limiter.to_json + rate_limiter.limit! end diff --git a/app/controllers/comment_votes_controller.rb b/app/controllers/comment_votes_controller.rb index c85faf4d8..8e280385a 100644 --- a/app/controllers/comment_votes_controller.rb +++ b/app/controllers/comment_votes_controller.rb @@ -1,5 +1,4 @@ class CommentVotesController < ApplicationController - skip_before_action :api_check respond_to :js, :json, :xml, :html def index diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index f655385ce..54783e0c1 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -1,7 +1,6 @@ class CommentsController < ApplicationController respond_to :html, :xml, :json, :atom respond_to :js, only: [:new, :destroy, :undelete] - skip_before_action :api_check def index params[:group_by] ||= "comment" if params[:search].present? diff --git a/app/controllers/favorites_controller.rb b/app/controllers/favorites_controller.rb index 8449dac63..2f086b6b8 100644 --- a/app/controllers/favorites_controller.rb +++ b/app/controllers/favorites_controller.rb @@ -1,6 +1,5 @@ class FavoritesController < ApplicationController respond_to :html, :xml, :json, :js - skip_before_action :api_check rescue_with Favorite::Error, status: 422 def index diff --git a/app/controllers/forum_posts_controller.rb b/app/controllers/forum_posts_controller.rb index 53af4054b..0a239591c 100644 --- a/app/controllers/forum_posts_controller.rb +++ b/app/controllers/forum_posts_controller.rb @@ -1,6 +1,5 @@ class ForumPostsController < ApplicationController respond_to :html, :xml, :json, :js - skip_before_action :api_check def new @forum_post = authorize ForumPost.new_reply(params) diff --git a/app/controllers/forum_topics_controller.rb b/app/controllers/forum_topics_controller.rb index f9a0ef00c..03865d1ae 100644 --- a/app/controllers/forum_topics_controller.rb +++ b/app/controllers/forum_topics_controller.rb @@ -2,7 +2,6 @@ class ForumTopicsController < ApplicationController respond_to :html, :xml, :json respond_to :atom, only: [:index, :show] before_action :normalize_search, :only => :index - skip_before_action :api_check def new @forum_topic = authorize ForumTopic.new diff --git a/app/controllers/moderator/post/posts_controller.rb b/app/controllers/moderator/post/posts_controller.rb index bc0603c16..ab28bd2f5 100644 --- a/app/controllers/moderator/post/posts_controller.rb +++ b/app/controllers/moderator/post/posts_controller.rb @@ -1,7 +1,6 @@ module Moderator module Post class PostsController < ApplicationController - skip_before_action :api_check respond_to :html, :json, :xml, :js def confirm_move_favorites diff --git a/app/controllers/post_disapprovals_controller.rb b/app/controllers/post_disapprovals_controller.rb index 6dbd93f46..e27853ee9 100644 --- a/app/controllers/post_disapprovals_controller.rb +++ b/app/controllers/post_disapprovals_controller.rb @@ -1,5 +1,4 @@ class PostDisapprovalsController < ApplicationController - skip_before_action :api_check respond_to :js, :html, :json, :xml def create diff --git a/app/controllers/post_votes_controller.rb b/app/controllers/post_votes_controller.rb index 1c8f8ff2f..1f66f4cf3 100644 --- a/app/controllers/post_votes_controller.rb +++ b/app/controllers/post_votes_controller.rb @@ -1,5 +1,4 @@ class PostVotesController < ApplicationController - skip_before_action :api_check respond_to :js, :json, :xml, :html def index diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 387037ea1..acd68815d 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,6 +1,5 @@ class UsersController < ApplicationController respond_to :html, :xml, :json - skip_before_action :api_check def new @user = authorize User.new diff --git a/app/logical/rate_limiter.rb b/app/logical/rate_limiter.rb index e8fc52430..56851f282 100644 --- a/app/logical/rate_limiter.rb +++ b/app/logical/rate_limiter.rb @@ -11,6 +11,28 @@ class RateLimiter @burst = burst end + def self.for_action(controller_name, action_name, user, ip_addr) + action = "#{controller_name}:#{action_name}" + keys = [(user.cache_key unless user.is_anonymous?), "ip/#{ip_addr.to_s}"].compact + + case action + when "users:create" + rate, burst = 1.0/5.minutes, 10 + when "emails:update", "sessions:create", "moderation_reports:create" + rate, burst = 1.0/1.minute, 10 + when "dmails:create", "comments:create", "forum_posts:create", "forum_topics:create" + rate, burst = 1.0/1.minute, 50 + when "comment_votes:create", "comment_votes:destroy", "post_votes:create", + "post_votes:destroy", "favorites:create", "favorites:destroy", "post_disapprovals:create" + rate, burst = 1.0/1.second, 200 + else + rate = user.api_regen_multiplier + burst = 200 + end + + RateLimiter.new(action, keys, rate: rate, burst: burst) + end + def limit! raise RateLimitError if limited? end diff --git a/app/models/user.rb b/app/models/user.rb index 22dfa82b1..03226fc26 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -464,26 +464,12 @@ class User < ApplicationRecord # regen this amount per second def api_regen_multiplier(level) - if level >= User::Levels::PLATINUM + if level >= User::Levels::GOLD 4 - elsif level == User::Levels::GOLD - 2 else 1 end end - - # can make this many api calls at once before being bound by - # api_regen_multiplier refilling your pool - def api_burst_limit(level) - if level >= User::Levels::PLATINUM - 60 - elsif level == User::Levels::GOLD - 30 - else - 10 - end - end end def max_saved_searches @@ -534,10 +520,6 @@ class User < ApplicationRecord User.api_regen_multiplier(level) end - def api_burst_limit - User.api_burst_limit(level) - end - def statement_timeout User.statement_timeout(level) end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index b5bef3ba9..fd6f93d06 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -61,10 +61,8 @@ class UserPolicy < ApplicationPolicy updated_at last_logged_in_at last_forum_read_at comment_threshold default_image_size favorite_tags blacklisted_tags time_zone per_page - custom_style favorite_count api_regen_multiplier - api_burst_limit statement_timeout - favorite_group_limit favorite_limit tag_query_limit - max_saved_searches theme + custom_style favorite_count statement_timeout favorite_group_limit + favorite_limit tag_query_limit max_saved_searches theme ] end diff --git a/test/functional/sessions_controller_test.rb b/test/functional/sessions_controller_test.rb index 29b3dbff6..eb9ab6fb6 100644 --- a/test/functional/sessions_controller_test.rb +++ b/test/functional/sessions_controller_test.rb @@ -72,6 +72,31 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert_equal(0, @ip_ban.reload.hit_count) assert_nil(@ip_ban.last_hit_at) end + + should "rate limit logins to 10 per minute per IP" do + freeze_time + + 11.times do + post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" } + assert_redirected_to posts_path + assert_equal(@user.id, session[:user_id]) + delete_auth session_path, @user + end + + post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" } + assert_response 429 + assert_not_equal(@user.id, session[:user_id]) + + travel 59.seconds + post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" } + assert_response 429 + assert_not_equal(@user.id, session[:user_id]) + + travel 10.seconds + post session_path, params: { name: @user.name, password: "password" }, headers: { REMOTE_ADDR: "1.2.3.4" } + assert_redirected_to posts_path + assert_equal(@user.id, session[:user_id]) + end end context "destroy action" do diff --git a/test/unit/api_key_test.rb b/test/unit/api_key_test.rb index 26b68a67f..760035ae2 100644 --- a/test/unit/api_key_test.rb +++ b/test/unit/api_key_test.rb @@ -48,11 +48,5 @@ class ApiKeyTest < ActiveSupport::TestCase should "not authenticate with the wrong name" do assert_equal(false, create(:user).authenticate_api_key(@api_key.key)) end - - should "have the same limits whether or not they have an api key" do - assert_no_difference(["@user.reload.api_regen_multiplier", "@user.reload.api_burst_limit"]) do - @api_key.destroy - end - end end end