Commit 9e5b0779 authored by Elliot Forbes's avatar Elliot Forbes 2️⃣
Browse files

fix(rate_limit): skip characteristic when value is missing 

Previously, a nil or empty characteristic value produced an empty-string
suffix in the Redis key (e.g. labkit:rl:rack_request:0:user:), causing
all anonymous callers to share a single bucket. Now resolve_characteristic
returns nil for missing values and evaluate_rule skips that characteristic,
emitting a structured log entry with skipped: true so operators can
surface rules that silently do nothing for unauthenticated traffic.

Adds unit + integration coverage for the skip path, plus pinning tests
for the empty-rules and Identifier-passthrough branches that were
previously untested.

Co-Authored-By: default avatarClaude Opus 4.7 (1M context) <noreply@anthropic.com>
parent bfe84cb4

lib/labkit/rate_limit/evaluator.rb

+24 −0
Original line number Diff line number Diff line
@@ -74,6 +74,12 @@ module Labkit 


        rule.characteristics.each do |char|

          char_value = resolve_characteristic(char, @identifier)



          if char_value.nil?

            log_skipped_characteristic(rule, index, char)

            next

          end



          redis_key = build_redis_key(@call_site, index, char, char_value)



          count = incr_with_ttl(redis_key, rule.period)
@@ -103,6 +109,9 @@ module Labkit 
        # Normalize endpoint: strip query string

        value = Identifier.normalize_endpoint(value) if char == :endpoint



        # Treat nil and empty-string the same: anonymous traffic must not collide on a shared bucket.

        return nil if value.nil? || value.to_s.empty?



        value.to_s

      end
@@ -142,6 +151,21 @@ module Labkit 
        )

      end



      def log_skipped_characteristic(rule, index, char)

        @logger.info(

          message: "rate_limit_check",

          call_site: @call_site,

          rule_index: index,

          action: rule.action.to_s,

          limit: rule.limit,

          period: rule.period,

          characteristic: char,

          matched: true,

          skipped: true,

          identifier: @identifier.to_h

        )

      end



      def log_evaluate_error(error)

        @logger.warn(

          message: "rate_limit_redis_error",

spec/labkit/rate_limit/evaluator_spec.rb

+37 −0
Original line number Diff line number Diff line
@@ -124,6 +124,43 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end

  end



  describe "missing characteristic value" do

    it "skips the characteristic and does not write to Redis when value is nil" do

      id = Labkit::RateLimit::Identifier.new(ip: "1.2.3.4")

      rule = make_rule(characteristics: [:user])

      expect(redis).not_to receive(:incr)

      expect(logger).to receive(:info).with(hash_including(skipped: true, characteristic: :user))



      result = evaluator(rules: [rule], id: id).evaluate

      expect(result).to eq(:allow)

    end



    it "skips the characteristic when value is an empty string" do

      id = Labkit::RateLimit::Identifier.new(user: "", ip: "1.2.3.4")

      rule = make_rule(characteristics: [:user])

      expect(redis).not_to receive(:incr)

      expect(logger).to receive(:info).with(hash_including(skipped: true, characteristic: :user))



      evaluator(rules: [rule], id: id).evaluate

    end



    it "still increments other characteristics when one is missing" do

      id = Labkit::RateLimit::Identifier.new(ip: "1.2.3.4")

      rule = make_rule(characteristics: [:user, :ip])



      keys_seen = []

      allow(redis).to receive(:incr) do |key|

        keys_seen << key

        1

      end

      allow(redis).to receive(:expire)



      evaluator(rules: [rule], id: id).evaluate



      expect(keys_seen).to eq(["labkit:rl:rack_request:0:ip:1.2.3.4"])

    end

  end



  describe "non-matching rules" do

    it "does not write to Redis for non-matching rules" do

      rule = make_rule(match: { user: 999 })

spec/labkit/rate_limit_spec.rb

+47 −2
Original line number Diff line number Diff line
@@ -260,8 +260,20 @@ RSpec.describe Labkit::RateLimit do 
    end

  end



  # Scenario 14: char_value > 200 chars -> SHA-256 used; distinct long values -> different keys

  describe "scenario 14: long char values use SHA-256" do

  # Scenario 14: empty rules -> :allow; no Redis writes; no logs

  describe "scenario 14: empty rules" do

    it "returns :allow without writing to Redis or logging" do

      result = check(rules: [])



      expect(result).to eq(:allow)

      expect(redis.get("labkit:rl:rack_request:0:user:42")).to eq(0)

      expect(logger).not_to have_received(:info)

      expect(logger).not_to have_received(:warn)

    end

  end



  # Scenario 15: char_value > 200 chars -> SHA-256 used; distinct long values -> different keys

  describe "scenario 15: long char values use SHA-256" do

    it "uses SHA-256 digest for values longer than 200 chars" do

      long_val = "v" * 201

      id = { user: long_val }
@@ -283,4 +295,37 @@ RSpec.describe Labkit::RateLimit do 
      expect(hash_a).not_to eq(hash_b)

    end

  end



  # Scenario 16: missing characteristic value -> skip; sibling characteristics still count

  describe "scenario 16: missing characteristic value" do

    it "skips a nil characteristic and does not write a shared empty-string-suffix key" do

      rules = [rule(match: {}, limit: 100, characteristics: [:user, :ip])]

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



      expect(result).to eq(:allow)

      expect(redis.get("labkit:rl:rack_request:0:user:")).to eq(0)

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

    end

  end



  # Scenario 17: .check accepts an Identifier instance directly (passthrough branch)

  describe "scenario 17: .check with Identifier instance" do

    it "uses the Identifier without re-wrapping it" do

      id = Labkit::RateLimit::Identifier.new(user: 42, ip: "1.2.3.4")

      rules = [rule(match: { user: 42 }, characteristics: [:user])]



      expect(Labkit::RateLimit::Identifier).not_to receive(:new)



      result = described_class.check(

        call_site: "rack_request",

        identifier: id,

        rules: rules,

        redis: redis,

        logger: logger

      )



      expect(result).to eq(:allow)

      expect(redis.get("labkit:rl:rack_request:0:user:42")).to eq(1)

    end

  end

end