mirror of
https://github.com/discourse/discourse.git
synced 2025-02-25 18:55:32 -06:00
FIX: Incorrect topic per-minute invitation rate limit (#31252)
This fixes an issue where the topic invitation rate limiter for invites for the 1 minute period was incorrectly using 1 day as the length of time the limit should be applied over. The default for `max_topic_invitations_per_minute` is 5, so this would be very easy to exceed, then the user gets a very confusing warning message saying they have to wait 23 hours to send more invites. This commit also makes other `RateLimiter` period parameters more consistent by always using the form `N.PERIOD` instead of things like `86_400` hardcoded seconds per day.
This commit is contained in:
parent
8d3a35e25b
commit
ec7c6b1f96
@ -29,7 +29,7 @@ class UploadsController < ApplicationController
|
|||||||
current_user,
|
current_user,
|
||||||
"uploads-per-minute",
|
"uploads-per-minute",
|
||||||
SiteSetting.max_uploads_per_minute,
|
SiteSetting.max_uploads_per_minute,
|
||||||
1.minute.to_i,
|
1.minute,
|
||||||
).performed!
|
).performed!
|
||||||
|
|
||||||
type =
|
type =
|
||||||
|
@ -871,7 +871,7 @@ class Category < ActiveRecord::Base
|
|||||||
|
|
||||||
def auto_bump_limiter
|
def auto_bump_limiter
|
||||||
return nil if num_auto_bump_daily.to_i == 0
|
return nil if num_auto_bump_daily.to_i == 0
|
||||||
RateLimiter.new(nil, "auto_bump_limit_#{self.id}", 1, 86_400 / num_auto_bump_daily.to_i)
|
RateLimiter.new(nil, "auto_bump_limit_#{self.id}", 1, 1.day.to_i / num_auto_bump_daily.to_i)
|
||||||
end
|
end
|
||||||
|
|
||||||
def clear_auto_bump_cache!
|
def clear_auto_bump_cache!
|
||||||
|
@ -2030,14 +2030,14 @@ class Topic < ActiveRecord::Base
|
|||||||
invited_by,
|
invited_by,
|
||||||
"topic-invitations-per-day",
|
"topic-invitations-per-day",
|
||||||
SiteSetting.max_topic_invitations_per_day,
|
SiteSetting.max_topic_invitations_per_day,
|
||||||
1.day.to_i,
|
1.day,
|
||||||
).performed!
|
).performed!
|
||||||
|
|
||||||
RateLimiter.new(
|
RateLimiter.new(
|
||||||
invited_by,
|
invited_by,
|
||||||
"topic-invitations-per-minute",
|
"topic-invitations-per-minute",
|
||||||
SiteSetting.max_topic_invitations_per_minute,
|
SiteSetting.max_topic_invitations_per_minute,
|
||||||
1.day.to_i,
|
1.minute,
|
||||||
).performed!
|
).performed!
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -123,7 +123,7 @@ class Auth::DefaultCurrentUserProvider
|
|||||||
current_user = nil
|
current_user = nil
|
||||||
|
|
||||||
if auth_token
|
if auth_token
|
||||||
limiter = RateLimiter.new(nil, "cookie_auth_#{request.ip}", COOKIE_ATTEMPTS_PER_MIN, 60)
|
limiter = RateLimiter.new(nil, "cookie_auth_#{request.ip}", COOKIE_ATTEMPTS_PER_MIN, 1.minute)
|
||||||
|
|
||||||
if limiter.can_perform?
|
if limiter.can_perform?
|
||||||
@env[USER_TOKEN_KEY] = @user_token =
|
@env[USER_TOKEN_KEY] = @user_token =
|
||||||
@ -435,7 +435,7 @@ class Auth::DefaultCurrentUserProvider
|
|||||||
limit = [GlobalSetting.max_admin_api_reqs_per_key_per_minute.to_i, limit].max
|
limit = [GlobalSetting.max_admin_api_reqs_per_key_per_minute.to_i, limit].max
|
||||||
end
|
end
|
||||||
@admin_api_key_limiter =
|
@admin_api_key_limiter =
|
||||||
RateLimiter.new(nil, "admin_api_min", limit, 60, error_code: "admin_api_key_rate_limit")
|
RateLimiter.new(nil, "admin_api_min", limit, 1.minute, error_code: "admin_api_key_rate_limit")
|
||||||
end
|
end
|
||||||
|
|
||||||
def user_api_key_limiter_60_secs
|
def user_api_key_limiter_60_secs
|
||||||
@ -444,7 +444,7 @@ class Auth::DefaultCurrentUserProvider
|
|||||||
nil,
|
nil,
|
||||||
"user_api_min_#{@hashed_user_api_key}",
|
"user_api_min_#{@hashed_user_api_key}",
|
||||||
GlobalSetting.max_user_api_reqs_per_minute,
|
GlobalSetting.max_user_api_reqs_per_minute,
|
||||||
60,
|
1.minute,
|
||||||
error_code: "user_api_key_limiter_60_secs",
|
error_code: "user_api_key_limiter_60_secs",
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
@ -455,7 +455,7 @@ class Auth::DefaultCurrentUserProvider
|
|||||||
nil,
|
nil,
|
||||||
"user_api_day_#{@hashed_user_api_key}",
|
"user_api_day_#{@hashed_user_api_key}",
|
||||||
GlobalSetting.max_user_api_reqs_per_day,
|
GlobalSetting.max_user_api_reqs_per_day,
|
||||||
86_400,
|
1.day,
|
||||||
error_code: "user_api_key_limiter_1_day",
|
error_code: "user_api_key_limiter_1_day",
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
@ -243,7 +243,7 @@ module Middleware
|
|||||||
nil,
|
nil,
|
||||||
"logged_in_anon_cache_#{@env["HTTP_HOST"]}/#{@env["REQUEST_URI"]}",
|
"logged_in_anon_cache_#{@env["HTTP_HOST"]}/#{@env["REQUEST_URI"]}",
|
||||||
GlobalSetting.force_anonymous_min_per_10_seconds,
|
GlobalSetting.force_anonymous_min_per_10_seconds,
|
||||||
10,
|
10.seconds,
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -486,7 +486,7 @@ class Middleware::RequestTracker
|
|||||||
nil,
|
nil,
|
||||||
"global_limit_10_#{rate_limit_key}",
|
"global_limit_10_#{rate_limit_key}",
|
||||||
GlobalSetting.max_reqs_per_ip_per_10_seconds,
|
GlobalSetting.max_reqs_per_ip_per_10_seconds,
|
||||||
10,
|
10.seconds,
|
||||||
global:,
|
global:,
|
||||||
aggressive: true,
|
aggressive: true,
|
||||||
error_code: "#{error_code_identifier}_10_secs_limit",
|
error_code: "#{error_code_identifier}_10_secs_limit",
|
||||||
@ -497,7 +497,7 @@ class Middleware::RequestTracker
|
|||||||
nil,
|
nil,
|
||||||
"global_limit_60_#{rate_limit_key}",
|
"global_limit_60_#{rate_limit_key}",
|
||||||
GlobalSetting.max_reqs_per_ip_per_minute,
|
GlobalSetting.max_reqs_per_ip_per_minute,
|
||||||
60,
|
1.minute,
|
||||||
global:,
|
global:,
|
||||||
error_code: "#{error_code_identifier}_60_secs_limit",
|
error_code: "#{error_code_identifier}_60_secs_limit",
|
||||||
aggressive: true,
|
aggressive: true,
|
||||||
@ -508,7 +508,7 @@ class Middleware::RequestTracker
|
|||||||
nil,
|
nil,
|
||||||
"global_limit_10_assets_#{rate_limit_key}",
|
"global_limit_10_assets_#{rate_limit_key}",
|
||||||
GlobalSetting.max_asset_reqs_per_ip_per_10_seconds,
|
GlobalSetting.max_asset_reqs_per_ip_per_10_seconds,
|
||||||
10,
|
10.seconds,
|
||||||
error_code: "#{error_code_identifier}_assets_10_secs_limit",
|
error_code: "#{error_code_identifier}_assets_10_secs_limit",
|
||||||
global:,
|
global:,
|
||||||
)
|
)
|
||||||
|
@ -874,6 +874,21 @@ RSpec.describe Topic do
|
|||||||
RateLimiter::LimitExceeded,
|
RateLimiter::LimitExceeded,
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it "does not rate limit if the invites are spread out" do
|
||||||
|
start = Time.now.tomorrow.beginning_of_minute
|
||||||
|
freeze_time(start)
|
||||||
|
|
||||||
|
topic = Fabricate(:private_message_topic, user: trust_level_2)
|
||||||
|
|
||||||
|
topic.invite(topic.user, user.username)
|
||||||
|
|
||||||
|
freeze_time(start + 5.minutes)
|
||||||
|
|
||||||
|
expect { topic.invite(topic.user, user1.username) }.not_to raise_error(
|
||||||
|
RateLimiter::LimitExceeded,
|
||||||
|
)
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
Loading…
Reference in New Issue
Block a user