From a562214f56c550ece064a5ada157343a2fd43c72 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 15 Jan 2024 19:54:50 +0000 Subject: [PATCH] FIX: Update global rate limiter keys/messages to clarify user vs ip (#25264) --- lib/middleware/request_tracker.rb | 26 ++++++++++++++------- spec/lib/middleware/request_tracker_spec.rb | 26 +++++++++++---------- 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/lib/middleware/request_tracker.rb b/lib/middleware/request_tracker.rb index d2229acbe70..fbb88bca593 100644 --- a/lib/middleware/request_tracker.rb +++ b/lib/middleware/request_tracker.rb @@ -211,9 +211,9 @@ class Middleware::RequestTracker cookie = find_auth_cookie(env) if error_details = rate_limit(request, cookie) - available_in, error_code = error_details + available_in, error_code, limit_on_id = error_details message = <<~TEXT - Slow down, too many requests from this IP address. + Slow down, too many requests from this #{limit_on_id ? "user" : "IP address"}. Please retry again in #{available_in} seconds. Error code: #{error_code}. TEXT @@ -329,7 +329,7 @@ class Middleware::RequestTracker limiter10 = RateLimiter.new( nil, - "global_ip_limit_10_#{ip_or_id}", + "global_limit_10_#{ip_or_id}", GlobalSetting.max_reqs_per_ip_per_10_seconds, 10, global: !limit_on_id, @@ -340,7 +340,7 @@ class Middleware::RequestTracker limiter60 = RateLimiter.new( nil, - "global_ip_limit_60_#{ip_or_id}", + "global_limit_60_#{ip_or_id}", GlobalSetting.max_reqs_per_ip_per_minute, 60, global: !limit_on_id, @@ -351,7 +351,7 @@ class Middleware::RequestTracker limiter_assets10 = RateLimiter.new( nil, - "global_ip_limit_10_assets_#{ip_or_id}", + "global_limit_10_assets_#{ip_or_id}", GlobalSetting.max_asset_reqs_per_ip_per_10_seconds, 10, error_code: limit_on_id ? "id_assets_10_secs_limit" : "ip_assets_10_secs_limit", @@ -363,13 +363,20 @@ class Middleware::RequestTracker if !limiter_assets10.can_perform? if warn + limited_on = limit_on_id ? "user_id" : "ip" Discourse.warn( - "Global asset IP rate limit exceeded for #{ip}: 10 second rate limit", + "Global asset rate limit exceeded for #{limited_on}: #{ip}: 10 second rate limit", uri: request.env["REQUEST_URI"], ) end - return limiter_assets10.seconds_to_wait(Time.now.to_i), limiter_assets10.error_code if block + if block + return [ + limiter_assets10.seconds_to_wait(Time.now.to_i), + limiter_assets10.error_code, + limit_on_id + ] + end end begin @@ -382,13 +389,14 @@ class Middleware::RequestTracker nil rescue RateLimiter::LimitExceeded => e if warn + limited_on = limit_on_id ? "user_id" : "ip" Discourse.warn( - "Global IP rate limit exceeded for #{ip}: #{type} second rate limit", + "Global rate limit exceeded for #{limited_on}: #{ip}: #{type} second rate limit", uri: request.env["REQUEST_URI"], ) end if block - [e.available_in, e.error_code] + [e.available_in, e.error_code, limit_on_id] else nil end diff --git a/spec/lib/middleware/request_tracker_spec.rb b/spec/lib/middleware/request_tracker_spec.rb index 8edb6319a71..3aa8c433f2c 100644 --- a/spec/lib/middleware/request_tracker_spec.rb +++ b/spec/lib/middleware/request_tracker_spec.rb @@ -307,9 +307,9 @@ RSpec.describe Middleware::RequestTracker do status, _ = middleware.call(env1) status, _ = middleware.call(env1) - expect( - @fake_logger.warnings.count { |w| w.include?("Global IP rate limit exceeded") }, - ).to eq(warn_count) + expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq( + warn_count, + ) expect(status).to eq(429) warn_count += 1 end @@ -398,9 +398,9 @@ RSpec.describe Middleware::RequestTracker do status, _ = middleware.call(env1) status, _ = middleware.call(env1) - expect( - @fake_logger.warnings.count { |w| w.include?("Global IP rate limit exceeded") }, - ).to eq(0) + expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq( + 0, + ) expect(status).to eq(200) end end @@ -412,9 +412,7 @@ RSpec.describe Middleware::RequestTracker do status, _ = middleware.call(env) status, headers = middleware.call(env) - expect(@fake_logger.warnings.count { |w| w.include?("Global IP rate limit exceeded") }).to eq( - 1, - ) + expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(1) expect(status).to eq(429) expect(headers["Retry-After"]).to eq("10") end @@ -426,9 +424,7 @@ RSpec.describe Middleware::RequestTracker do status, _ = middleware.call(env) status, _ = middleware.call(env) - expect(@fake_logger.warnings.count { |w| w.include?("Global IP rate limit exceeded") }).to eq( - 1, - ) + expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(1) expect(status).to eq(200) end @@ -495,6 +491,7 @@ RSpec.describe Middleware::RequestTracker do expect(status).to eq(429) expect(called).to eq(1) expect(headers["Discourse-Rate-Limit-Error-Code"]).to eq("ip_10_secs_limit") + expect(response.first).to include("too many requests from this IP address") expect(response.first).to include("Error code: ip_10_secs_limit.") end @@ -518,6 +515,7 @@ RSpec.describe Middleware::RequestTracker do expect(status).to eq(429) expect(called).to eq(1) expect(headers["Discourse-Rate-Limit-Error-Code"]).to eq("ip_60_secs_limit") + expect(response.first).to include("too many requests from this IP address") expect(response.first).to include("Error code: ip_60_secs_limit.") end @@ -542,6 +540,7 @@ RSpec.describe Middleware::RequestTracker do expect(status).to eq(429) expect(called).to eq(1) expect(headers["Discourse-Rate-Limit-Error-Code"]).to eq("ip_assets_10_secs_limit") + expect(response.first).to include("too many requests from this IP address") expect(response.first).to include("Error code: ip_assets_10_secs_limit.") end end @@ -582,6 +581,7 @@ RSpec.describe Middleware::RequestTracker do status, headers, response = middleware.call(env) expect(status).to eq(429) expect(headers["Discourse-Rate-Limit-Error-Code"]).to eq("id_60_secs_limit") + expect(response.first).to include("too many requests from this user") expect(response.first).to include("Error code: id_60_secs_limit.") end expect(called).to eq(3) @@ -617,6 +617,7 @@ RSpec.describe Middleware::RequestTracker do status, headers, response = middleware.call(env) expect(status).to eq(429) expect(headers["Discourse-Rate-Limit-Error-Code"]).to eq("ip_60_secs_limit") + expect(response.first).to include("too many requests from this IP address") expect(response.first).to include("Error code: ip_60_secs_limit.") end end @@ -652,6 +653,7 @@ RSpec.describe Middleware::RequestTracker do status, headers, response = middleware.call(env) expect(status).to eq(429) expect(headers["Discourse-Rate-Limit-Error-Code"]).to eq("ip_60_secs_limit") + expect(response.first).to include("too many requests from this IP address") expect(response.first).to include("Error code: ip_60_secs_limit.") end end