From b54a287f9d77aa2c8d6353f8fb820b8252dda5d1 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Wed, 17 Jan 2024 14:53:41 +0100 Subject: [PATCH 1/2] Ensure all Cache store have consistent TTL behavior on increment Make sure they all increment the counter but don't update the TTL. --- .../lib/active_support/cache/file_store.rb | 19 ++++++++++++------- .../cache_increment_decrement_behavior.rb | 14 ++++++++++++++ 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/activesupport/lib/active_support/cache/file_store.rb b/activesupport/lib/active_support/cache/file_store.rb index 95bd2e7eac0..821747f9650 100644 --- a/activesupport/lib/active_support/cache/file_store.rb +++ b/activesupport/lib/active_support/cache/file_store.rb @@ -211,17 +211,22 @@ module ActiveSupport # If the key is not found it is created and set to +amount+. def modify_value(name, amount, options) file_name = normalize_key(name, options) + options = merged_options(options) + key = normalize_key(name, options) + version = normalize_version(name, options) + amount = Integer(amount) lock_file(file_name) do - options = merged_options(options) + entry = read_entry(key, **options) - if num = read(name, options) - num = num.to_i + amount - write(name, num, options) - num - else - write(name, Integer(amount), options) + if !entry || entry.expired? || entry.mismatched?(version) + write(name, amount, options) amount + else + num = entry.value.to_i + amount + entry = Entry.new(num, expires_at: entry.expires_at, version: entry.version) + write_entry(key, entry) + num end end end diff --git a/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb b/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb index c139eea8512..7685fa98870 100644 --- a/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb +++ b/activesupport/test/cache/behaviors/cache_increment_decrement_behavior.rb @@ -30,4 +30,18 @@ module CacheIncrementDecrementBehavior missing = @cache.decrement(SecureRandom.alphanumeric, 100) assert_equal @cache.is_a?(ActiveSupport::Cache::MemCacheStore) ? 0 : -100, missing end + + def test_ttl_isnt_updated + key = SecureRandom.uuid + + assert_equal 1, @cache.increment(key, 1, expires_in: 1) + assert_equal 2, @cache.increment(key, 1, expires_in: 5000) + + # having to sleep two seconds in a test is bad, but we're testing + # a wide range of backends with different TTL mecanisms, most without + # subsecond granularity, so this is the only reliable way. + sleep 2 + + assert_nil @cache.read(key, raw: true) + end end From d839ddb71abea276cc1a79b5eaddaa9620353f31 Mon Sep 17 00:00:00 2001 From: Jean Boussier Date: Wed, 17 Jan 2024 15:24:22 +0100 Subject: [PATCH 2/2] Refactor `ActionController::RateLimiting` to use `AS::Cache` Given that the limiter implementation provided by Kredis is a simple increment with a limit, all `ActiveSupport::Cache` already provide that same capability, with a wide range of backing stores, and not just Redis. This even allow to use SolidCache has a backend if you so desire. If we feel particularly fancy, we could also accept a more generic limiter interface to better allow users to swap the implementation for better algorithms such as leaky-bucket etc. --- Gemfile | 1 - Gemfile.lock | 5 --- actionpack/CHANGELOG.md | 4 +- .../action_controller/metal/rate_limiting.rb | 40 +++++++------------ .../test/controller/rate_limiting_test.rb | 19 +++------ 5 files changed, 23 insertions(+), 46 deletions(-) diff --git a/Gemfile b/Gemfile index f87dfadcd7c..5446542668f 100644 --- a/Gemfile +++ b/Gemfile @@ -87,7 +87,6 @@ else gem "rack", git: "https://github.com/rack/rack.git", branch: "main" end -gem "kredis", ">= 1.7.0", require: false gem "useragent", require: false # Active Job diff --git a/Gemfile.lock b/Gemfile.lock index 39415f4d0f8..a967cc7fd97 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -299,10 +299,6 @@ GEM rexml kramdown-parser-gfm (1.1.0) kramdown (~> 2.0) - kredis (1.7.0) - activemodel (>= 6.0.0) - activesupport (>= 6.0.0) - redis (>= 4.2, < 6) language_server-protocol (3.17.0.3) libxml-ruby (5.0.0) listen (3.8.0) @@ -611,7 +607,6 @@ DEPENDENCIES jbuilder jsbundling-rails json (>= 2.0.0, != 2.7.0) - kredis (>= 1.7.0) libxml-ruby listen (~> 3.3) mdl (!= 0.13.0) diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index 98abc0287ca..11b54256f82 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -21,7 +21,7 @@ *DHH* -* Add rate limiting API using Redis and the [Kredis limiter type](https://github.com/rails/kredis/blob/main/lib/kredis/types/limiter.rb). +* Add rate limiting API. ```ruby class SessionsController < ApplicationController @@ -34,7 +34,7 @@ end ``` - *DHH* + *DHH*, *Jean Boussier* * Add `image/svg+xml` to the compressible content types of ActionDispatch::Static diff --git a/actionpack/lib/action_controller/metal/rate_limiting.rb b/actionpack/lib/action_controller/metal/rate_limiting.rb index 2427bdf2593..10e27e9ae32 100644 --- a/actionpack/lib/action_controller/metal/rate_limiting.rb +++ b/actionpack/lib/action_controller/metal/rate_limiting.rb @@ -15,6 +15,10 @@ module ActionController # :nodoc: # Requests that exceed the rate limit are refused with a 429 Too Many Requests response. You can specialize this by passing a callable # in the with: parameter. It's evaluated within the context of the controller processing the request. # + # Rate limiting relies on a backing ActiveSupport::Cache store and defaults to config.action_controller.cache_store, which + # itself default to the global `config.cache_store`. If you don't want to store rate limits in the same datastore than your general caches + # you can pass a custom store in the store parameter. + # # Examples: # # class SessionsController < ApplicationController @@ -26,38 +30,24 @@ module ActionController # :nodoc: # by: -> { request.domain }, with: -> { redirect_to busy_controller_url, alert: "Too many signups on domain!" }, only: :new # end # - # Note: Rate limiting relies on the application having an accessible Redis server and on Kredis 1.7.0+ being available in the bundle. - # This uses the Kredis limiter type underneath, which is failsafe, so in case Redis is inaccessible, the rate limit will not refuse action execution. - def rate_limit(to:, within:, by: -> { request.remote_ip }, with: -> { head :too_many_requests }, **options) - require_compatible_kredis - before_action -> { rate_limiting(to: to, within: within, by: by, with: with) }, **options + # class APIController < ApplicationController + # RATE_LIMIT_STORE = ActiveSupport::Cache::RedisCacheStore.new(url: ENV["REDIS_URL"]) + # rate_limit to: 10, within: 3.minutes, store: RATE_LIMIT_STORE + # end + # + # TODO Note + def rate_limit(to:, within:, by: -> { request.remote_ip }, with: -> { head :too_many_requests }, store: cache_store, **options) + before_action -> { rate_limiting(to: to, within: within, by: by, with: with, store: store) }, **options end - - private - def require_compatible_kredis - require "kredis" - - if Kredis::VERSION < "1.7.0" - raise StandardError, \ - "Rate limiting requires Kredis 1.7.0+. Please update by calling `bundle update kredis`." - end - rescue LoadError - raise LoadError, \ - "Rate limiting requires Redis and Kredis. " + - "Please ensure you have Redis installed on your system and the Kredis gem in your Gemfile." - end end private - def rate_limiting(to:, within:, by:, with:) - limiter = Kredis.limiter "rate-limit:#{controller_path}:#{instance_exec(&by)}", limit: to, expires_in: within - - if limiter.exceeded? + def rate_limiting(to:, within:, by:, with:, store:) + count = store.increment("rate-limit:#{controller_path}:#{instance_exec(&by)}", 1, expires_in: within) + if count && count > to ActiveSupport::Notifications.instrument("rate_limit.action_controller", request: request) do instance_exec(&with) end - else - limiter.poke end end end diff --git a/actionpack/test/controller/rate_limiting_test.rb b/actionpack/test/controller/rate_limiting_test.rb index 84b4dfbd1f3..a6d29efdfbd 100644 --- a/actionpack/test/controller/rate_limiting_test.rb +++ b/actionpack/test/controller/rate_limiting_test.rb @@ -1,17 +1,9 @@ # frozen_string_literal: true require "abstract_unit" -require "kredis" - -Kredis.configurator = Class.new do - def config_for(name) { db: "2" } end - def root() Pathname.new(Dir.pwd) end -end.new - -# Enable Kredis logging -# ActiveSupport::LogSubscriber.logger = ActiveSupport::Logger.new(STDOUT) class RateLimitedController < ActionController::Base + self.cache_store = ActiveSupport::Cache::MemoryStore.new rate_limit to: 2, within: 2.seconds, by: -> { Thread.current[:redis_test_seggregation] }, only: :limited_to_two def limited_to_two @@ -29,7 +21,7 @@ class RateLimitingTest < ActionController::TestCase setup do Thread.current[:redis_test_seggregation] = Random.hex(10) - Kredis.counter("rate-limit:rate_limited:#{Thread.current[:redis_test_seggregation]}").del + RateLimitedController.cache_store.clear end test "exceeding basic limit" do @@ -46,9 +38,10 @@ class RateLimitingTest < ActionController::TestCase get :limited_to_two assert_response :ok - sleep 3 - get :limited_to_two - assert_response :ok + travel_to Time.now + 3.seconds do + get :limited_to_two + assert_response :ok + end end test "limited with" do