Commit d23c7495 authored by Sam Wiskow's avatar Sam Wiskow
Browse files

fix: address adversarial MR review findings (RC-1, RC-2) 

RC-1: Replace Evaluator::KNOWN_CHARACTERISTICS reference in rate_limit.rb
with an inline constant definition to avoid forcing eager autoload of
Evaluator at module load time.

RC-2: Add missing multi-characteristic aggregation tests to Scenario 4:
- Exceeded :log-action rule alone does not produce :block
- Multi-characteristic :block rule uses OR semantics (any characteristic
  exceeded triggers the action); documents and tests this explicitly
parent 251bb2a0

lib/labkit/rate_limit.rb

+3 −1
Original line number Diff line number Diff line
@@ -7,7 +7,9 @@ module Labkit 
    autoload :Rule, "labkit/rate_limit/rule"

    autoload :Evaluator, "labkit/rate_limit/evaluator"



    KNOWN_CHARACTERISTICS = Evaluator::KNOWN_CHARACTERISTICS

    # Defined independently to avoid forcing eager load of Evaluator at module load time.

    # Must stay in sync with Evaluator::KNOWN_CHARACTERISTICS.

    KNOWN_CHARACTERISTICS = [:user, :ip, :namespace, :plan, :endpoint].freeze



    # Check whether the given call_site + identifier combination is within the

    # configured rules.

spec/labkit/rate_limit_spec.rb

+16 −0
Original line number Diff line number Diff line
@@ -100,6 +100,22 @@ RSpec.describe Labkit::RateLimit do 
      expect(redis.get("labkit:rl:rack_request:0:user:42")).to eq(1)

      expect(redis.get("labkit:rl:rack_request:1:ip:1.2.3.4")).to eq(1)

    end



    it "exceeded :log-action rule alone does not produce :block" do

      # Pre-fill :log rule's counter above its limit

      51.times { redis.incr("labkit:rl:rack_request:0:ip:1.2.3.4") }

      rules = [rule(action: :log, limit: 50, characteristics: [:ip])]

      result = check(identifier: { user: 42, ip: "1.2.3.4" }, rules: rules)

      expect(result).to eq(:allow)

    end



    it "multi-characteristic :block rule blocks when any characteristic is exceeded (OR semantics)" do

      # Only :ip counter is pre-filled above limit; :user is within limit

      51.times { redis.incr("labkit:rl:rack_request:0:ip:1.2.3.4") }

      rules = [rule(action: :block, limit: 50, characteristics: [:user, :ip])]

      result = check(identifier: { user: 42, ip: "1.2.3.4" }, rules: rules)

      expect(result).to eq(:block)

    end

  end



  # Scenario 5: All rules within limit → :allow; all counters incremented