FIX: Update global rate limiter keys/messages to clarify user vs ip (#25264)

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

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(
+        expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(
-          @fake_logger.warnings.count { |w| w.include?("Global IP rate limit exceeded") },
+          warn_count,
-        ).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(
+        expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(
-          @fake_logger.warnings.count { |w| w.include?("Global IP rate limit exceeded") },
+          0,
-        ).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(
+      expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(1)
-        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(
+      expect(@fake_logger.warnings.count { |w| w.include?("Global rate limit exceeded") }).to eq(1)
-        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