Commit 74312ce7 authored by Nidhey Indurkar's avatar Nidhey Indurkar 💻
Browse files

refactor(rate_limit): address review feedback on match timeout

parent 4f6d0fee
Loading
Loading
Loading
Loading
+8 −22
Original line number Diff line number Diff line
@@ -24,29 +24,15 @@ module Labkit
      MAX_REGEX_SOURCE_LENGTH = 200
      ERROR_INSPECT_LIMIT = 80

      # Wall-clock budget for a single #match? call.
      # Wall-clock budget for a single #match? call. Without it a match is
      # bounded only by whatever global the host sets (40s in GitLab Rails,
      # unbounded elsewhere), and a timeout reaches Evaluator's fail-open
      # rescue, so the request goes unlimited.
      #
      # Ruby applies no regex timeout by default (Regexp.timeout is nil), so
      # without this a match is bounded only by whatever global the host
      # application happens to set - 40s inside GitLab Rails, unbounded
      # everywhere else. Neither is a budget a rate limiter should accept:
      # matching runs once per rule per request on the hot path, and when a
      # match finally times out the error reaches Evaluator's fail-open
      # rescue, so the request is not rate limited at all.
      #
      # MAX_REGEX_SOURCE_LENGTH bounds the pattern, not the match: short
      # patterns can still backtrack badly. This bounds the match.
      #
      # Measured against GitLab's real RackAttack path patterns
      # (API_PATH_REGEX, WEB_PATH_REGEX, Packages::API_PATH_REGEX and the
      # health/internal/registry/collector patterns) on inputs up to 16KB,
      # including many-segment and newline-heavy paths: worst observed match
      # 0.0419ms, none timed out. 5ms is therefore ~119x the slowest real
      # match - enough that a legitimate rule will not trip it, while a
      # pathological one is capped well below a request budget.
      #
      # None of those patterns use nested quantifiers or backreferences, so
      # none can backtrack exponentially; re-measure if that changes.
      # 5ms is ~119x the slowest real match measured against GitLab's
      # RackAttack path patterns on inputs up to 16KB (worst: 0.0419ms, none
      # timed out). Re-measure if a rule ever needs nested quantifiers or
      # backreferences, which are what make backtracking exponential.
      MATCH_TIMEOUT_SECONDS = 0.005

      def self.build(input)
+2 −1
Original line number Diff line number Diff line
@@ -96,11 +96,12 @@ RSpec.describe Labkit::RateLimit::Matcher do
        expect(m.value.timeout).to eq(described_class::MATCH_TIMEOUT_SECONDS)
      end

      it "accepts a Regexp inside { re: ... } and stores it directly" do
      it "accepts a Regexp inside { re: ... } and recompiles it with the timeout" do
        re = %r{^/api/v\d+/projects}
        m = described_class.build(re: re)
        expect(m.type).to eq(:re)
        expect(m.value).to eq(re)
        expect(m.value.timeout).to eq(described_class::MATCH_TIMEOUT_SECONDS)
      end

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