Commit 9538c34f authored by Sam Wiskow's avatar Sam Wiskow
Browse files

refactor(rate_limit): tighten Matcher per round-2 review 

Address @reprazent's second-round review on !283:

  - KNOWN_HASH_KEYS narrows to %i[eq re]. The :equality alias is
    dropped before release (cheaper to keep the surface tight than
    to support an alias forever for marginal convenience).

  - The explicit "{regex: ...} -> use :re" rejection branch is gone.
    Nothing has shipped under :regex, so callers who pass it now hit
    the same generic "unknown type key" path as any other typo.

  - Length cap applies to both String and Regexp sources via
    source.to_s.length, since both are engineer-provided and the
    error fires at boot time anyway. Regexp#to_s wraps the source
    with a few non-printable chars, so the effective allowed source
    length is slightly under MAX_REGEX_SOURCE_LENGTH for Regexp
    inputs -- acceptable.

  - Matcher#match? for :re now coerces the identifier value via
    #to_s instead of guarding on is_a?(String). Lets callers match
    Integer status codes (e.g. {status: { re: '^5\\d\\d$' }} against
    status: 503), which was the concrete use case Bob raised. The
    earlier "quietly skip non-String values" semantics inverts:
    nil.to_s == "" can match an empty-string regex, 42 against /42/
    matches, etc. Existing tests reframed accordingly; new positive
    cast scenarios added in matcher_spec and evaluator_spec.

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

lib/labkit/rate_limit/matcher.rb

+9 −10
Original line number Diff line number Diff line
@@ -9,14 +9,18 @@ module Labkit 
    # Accepted input shapes (everything else raises ArgumentError):

    #   - any plain value (String, Symbol, Integer, ...) -> :eq matcher

    #   - a Regexp instance                              -> :re matcher (Ruby convenience)

    #   - { eq: <value> } / { equality: <value> }        -> :eq matcher (canonical, YAML-compatible)

    #   - { eq: <value> }                                -> :eq matcher (canonical, YAML-compatible)

    #   - { re: <String|Regexp> }                        -> :re matcher (canonical, YAML-compatible)

    #

    # Hash-key naming follows the metrics-catalog selector pattern. Glob,

    # prefix, and other matcher kinds are intentionally out of scope here; see

    # gitlab-com/gl-infra/production-engineering#28853 for that follow-up.

    #

    # An :re matcher coerces the identifier value via #to_s before applying

    # the regex, so callers can match non-String identifier values such as

    # Integer status codes (e.g. {status: { re: "^5" }} against status: 503).

    class Matcher < Data.define(:type, :value)

      KNOWN_HASH_KEYS = %i[eq equality re].freeze

      KNOWN_HASH_KEYS = %i[eq re].freeze

      MAX_REGEX_SOURCE_LENGTH = 200

      ERROR_INSPECT_LIMIT = 80
@@ -43,11 +47,6 @@ module Labkit 
        type, source = input.first

        type_sym = type.to_sym



        if type_sym == :regex

          raise ArgumentError,

            "rate-limit match value uses removed key :regex; use :re instead, e.g. {re: \"^/api\"}"

        end



        unless KNOWN_HASH_KEYS.include?(type_sym)

          raise ArgumentError,

            "rate-limit match value has unknown type key #{truncate_for_error(type)}; accepted: #{KNOWN_HASH_KEYS.inspect}"
@@ -59,10 +58,10 @@ module Labkit 


      def self.compile(type_sym, source)

        case type_sym

        when :eq, :equality

        when :eq

          new(type: :eq, value: source)

        when :re

          if source.is_a?(String) && source.length > MAX_REGEX_SOURCE_LENGTH

          if source.to_s.length > MAX_REGEX_SOURCE_LENGTH

            raise ArgumentError,

              "rate-limit match value {re: ...} source exceeds #{MAX_REGEX_SOURCE_LENGTH} characters"

          end
@@ -88,7 +87,7 @@ module Labkit 
        when :eq

          value == identifier_value

        when :re

          identifier_value.is_a?(String) && value.match?(identifier_value)

          value.match?(identifier_value.to_s)

        else

          raise ArgumentError, "unknown matcher type: #{type.inspect}"

        end

spec/labkit/rate_limit/evaluator_spec.rb

+15 −4
Original line number Diff line number Diff line
@@ -497,8 +497,19 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
    end



    context "when the identifier value is not a String" do

      it "does not match and does not raise" do

        rule = make_rule(match: { user: { re: "42" } })

      it "casts the identifier value via to_s and matches when the cast value matches the regex" do

        rule = make_rule(name: "status_5xx",

          match: { status: { re: "^5\\d\\d$" } },

          characteristics: [:user])

        id_503 = Labkit::RateLimit::Identifier.new(user: 42, status: 503)

        expect(pipe).to receive(:incr).and_return(nil)

        expect(raw_redis).to receive(:expire)



        expect(evaluator(rules: [rule]).check(id_503).matched?).to be(true)

      end



      it "does not match (and does not raise) when the cast value fails the regex" do

        rule = make_rule(match: { user: { re: "^9\\d+$" } })

        int_id = Labkit::RateLimit::Identifier.new(user: 42)

        expect(raw_redis).not_to receive(:pipelined)
@@ -506,8 +517,8 @@ RSpec.describe Labkit::RateLimit::Evaluator do 
        expect(evaluator(rules: [rule]).check(int_id).matched?).to be(false)

      end



      it "falls through to a subsequent equality rule that does match" do

        regex_rule = make_rule(name: "regex_first", match: { user: { re: "42" } }, action: :block)

      it "falls through to a subsequent rule when the cast value does not match the first" do

        regex_rule = make_rule(name: "regex_first", match: { user: { re: "^9\\d+$" } }, action: :block)

        eq_rule    = make_rule(name: "eq_second",   match: { user: 42 }, action: :log)

        int_id = Labkit::RateLimit::Identifier.new(user: 42)

spec/labkit/rate_limit/matcher_spec.rb

+28 −25
Original line number Diff line number Diff line
@@ -42,7 +42,7 @@ RSpec.describe Labkit::RateLimit::Matcher do 
      end

    end



    context "with the canonical { eq: ... } / { equality: ... } shapes" do

    context "with the canonical { eq: ... } shape" do

      it "treats { eq: x } identically to a bare x for any plain value" do

        ["api", :api, 42, true, nil].each do |v|

          eq_form    = described_class.build(eq: v)
@@ -53,13 +53,6 @@ RSpec.describe Labkit::RateLimit::Matcher do 
        end

      end



      it "accepts { equality: x } as an alias for { eq: x }" do

        long  = described_class.build(equality: "api")

        short = described_class.build(eq: "api")

        expect(long).to eq(short)

        expect(long.type).to eq(:eq)

      end



      it "accepts a string-keyed hash too (mirrors YAML.safe_load output)" do

        m = described_class.build("eq" => "api")

        expect(m).to eq(described_class.build(eq: "api"))
@@ -102,23 +95,15 @@ RSpec.describe Labkit::RateLimit::Matcher do 
          .to raise_error(ArgumentError, /failed to compile/)

      end



      it "caps a String source at 200 characters" do

      it "applies the 200-char cap to a String source" do

        expect { described_class.build(re: "a" * 201) }

          .to raise_error(ArgumentError, /exceeds 200/)

      end



      it "does not apply the 200-char cap to a Regexp source" do

        # The Regexp's source string can be longer than 200 chars without

        # tripping the cap -- it's already a compiled Regexp, not user-typed input.

      it "applies the 200-char cap to a Regexp source via #to_s" do

        long_re = Regexp.new("a" * 201)

        expect { described_class.build(re: long_re) }.not_to raise_error

      end

    end



    context "with the removed { regex: ... } key" do

      it "raises ArgumentError naming :re as the replacement" do

        expect { described_class.build(regex: "^/api") }

          .to raise_error(ArgumentError, /removed key :regex.*use :re/m)

        expect { described_class.build(re: long_re) }.to raise_error(ArgumentError, /exceeds 200/)

      end

    end
@@ -147,6 +132,16 @@ RSpec.describe Labkit::RateLimit::Matcher do 
        expect { described_class.build(prefix: "/a") }

          .to raise_error(ArgumentError, /unknown type key/)

      end



      it "rejects { equality: \"...\" } as an unknown type key (no aliases)" do

        expect { described_class.build(equality: "api") }

          .to raise_error(ArgumentError, /unknown type key/)

      end



      it "rejects { regex: \"...\" } as an unknown type key (use :re)" do

        expect { described_class.build(regex: "^/api") }

          .to raise_error(ArgumentError, /unknown type key/)

      end

    end



    context "with unbounded user-controlled rejection input" do
@@ -214,16 +209,24 @@ RSpec.describe Labkit::RateLimit::Matcher do 
      end



      context "when the identifier value is not a String" do

        it "returns false for an Integer (no exception, no log)" do

          expect(matcher.match?(42)).to be(false)

        # Pattern matchers coerce via #to_s before matching, so callers can use

        # patterns against Integer/Symbol identifier values (e.g. status codes).

        it "casts an Integer identifier value via to_s and matches when the cast value matches" do

          status_matcher = described_class.build(re: "^5\\d\\d$")

          expect(status_matcher.match?(503)).to be(true)

          expect(status_matcher.match?(200)).to be(false)

        end



        it "returns false for nil" do

          expect(matcher.match?(nil)).to be(false)

        it "casts a Symbol identifier value via to_s and matches against its name" do

          m = described_class.build(re: "^api$")

          expect(m.match?(:api)).to be(true)

          expect(m.match?(:web)).to be(false)

        end



        it "returns false for a Symbol" do

          expect(matcher.match?(:api)).to be(false)

        it "treats nil as the empty String (#to_s)" do

          empty_matcher = described_class.build(re: "^$")

          expect(empty_matcher.match?(nil)).to be(true)

          expect(matcher.match?(nil)).to be(false) # the let(:matcher) regex needs /api/...

        end

      end

    end

spec/labkit/rate_limit/rule_spec.rb

+0 −10
Original line number Diff line number Diff line
@@ -141,11 +141,6 @@ RSpec.describe Labkit::RateLimit::Rule do 
      expect(from_hash.match[:endpoint].type).to eq(:eq)

    end



    it "normalizes { equality: x } as an alias of { eq: x }" do

      rule = valid_rule(match: { endpoint: { equality: "/api/v4" } })

      expect(rule.match[:endpoint]).to have_attributes(type: :eq, value: "/api/v4")

    end



    it "normalizes { re: \"...\" } to a :re matcher" do

      rule = valid_rule(match: { path: { re: "^/api/v\\d+/projects" } })

      matcher = rule.match[:path]
@@ -176,11 +171,6 @@ RSpec.describe Labkit::RateLimit::Rule do 
        .to raise_error(ArgumentError, /failed to compile/)

    end



    it "raises ArgumentError on the removed :regex hash key, naming :re as the replacement" do

      expect { valid_rule(match: { path: { regex: "^/api" } }) }

        .to raise_error(ArgumentError, /removed key :regex.*use :re/m)

    end



    it "raises ArgumentError on { glob: \"...\" } as an unknown type" do

      expect { valid_rule(match: { path: { glob: "/api/v*/projects" } }) }

        .to raise_error(ArgumentError, /unknown type key/)