fix(rate_limit): cap regex match timeout to prevent fail-open (0c0f54ef) · Commits · GitLab.org / ruby / gems / labkit-ruby

lib/labkit/rate_limit/matcher.rb

+46 −2

Original line number	Diff line number	Diff line
		@@ -24,10 +24,29 @@ module Labkit
		MAX_REGEX_SOURCE_LENGTH = 200
		ERROR_INSPECT_LIMIT = 80

		# Wall-clock budget for a single #match? call.
		#
		# 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.
		#
		# 5ms is ~10,000x the slowest realistic match measured against
		# GitLab-style route patterns (worst observed: 0.0005ms), so a
		# legitimate rule cannot trip it even on a loaded box, while a
		# pathological one is capped well below a request budget.
		MATCH_TIMEOUT_SECONDS = 0.005

		def self.build(input)
		case input
		when Regexp
		new(type: :re, value: input)
		new(type: :re, value: with_match_timeout(input))
		when Hash
		from_hash(input)
		when Array
		@@ -67,7 +86,7 @@ module Labkit
		end

		begin
		new(type: :re, value: Regexp.new(source))
		new(type: :re, value: with_match_timeout(source))
		rescue RegexpError, TypeError => e
		raise ArgumentError,
		"rate-limit match value {re: #{truncate_for_error(source)}} failed to compile: #{e.message}"
		@@ -76,6 +95,31 @@ module Labkit
		end
		private_class_method :compile

		# Compiles +source+ (a String pattern or an existing Regexp) into a
		# Regexp bounded by MATCH_TIMEOUT_SECONDS.
		#
		# The timeout is set per-Regexp rather than via the global
		# +Regexp.timeout=+: labkit is a library, and a global would silently
		# change regex behaviour throughout the host application, including
		# code unrelated to rate limiting.
		#
		# A Regexp that already carries its own timeout is returned untouched -
		# an explicit choice by the rule author wins over our default.
		#
		# Recompiling an existing Regexp preserves its source and options
		# (including the fixed-encoding flags) and therefore +#==+; only object
		# identity changes.
		def self.with_match_timeout(source)
		return source if source.is_a?(Regexp) && source.timeout

		if source.is_a?(Regexp)
		Regexp.new(source.source, source.options, timeout: MATCH_TIMEOUT_SECONDS)
		else
		Regexp.new(source, timeout: MATCH_TIMEOUT_SECONDS)
		end
		end
		private_class_method :with_match_timeout

		def self.truncate_for_error(value)
		s = value.inspect
		s.length > ERROR_INSPECT_LIMIT ? "#{s[0, ERROR_INSPECT_LIMIT]}...(truncated)" : s

spec/labkit/rate_limit/evaluator_spec.rb

+43 −0

Original line number	Diff line number	Diff line
		@@ -544,6 +544,49 @@ RSpec.describe Labkit::RateLimit::Evaluator do
		expect(result.exceeded?).to be(false)
		expect(result.matched?).to be(false)
		end

		# A rule whose match regex times out reaches the same fail-open rescue as
		# a Redis outage: Regexp::TimeoutError < RegexpError < StandardError. The
		# request is allowed and nothing is counted. This is the behaviour the
		# timeout bounds - without it the match runs unbounded and the request
		# blocks on the regex instead.
		it "fails open when a match regex exceeds its timeout" do
		logger = instance_double(Labkit::Logging::JsonLogger)
		expect(logger).to receive(:warn).with(
		hash_including(
		Labkit::Fields::ERROR_TYPE => "rate_limit_error",
		Labkit::Fields::CLASS_NAME => "Regexp::TimeoutError"
		)
		)

		# Backreference opts the pattern out of Ruby 3.2+ regex memoization, so
		# this genuinely backtracks rather than being optimised away.
		rule = make_rule(name: "slow_regex", match: { ip: { re: '(a+)+\1$' } })
		id = Labkit::RateLimit::Identifier.new(user: 42, ip: "#{'a' * 60}!")

		result = described_class.new(
		name: "rack_request", rules: [rule], redis: redis, logger: logger
		).check(id)

		expect(result.error?).to be(true)
		expect(result.matched?).to be(false)
		expect(result.action).to eq(:allow)
		expect(result.to_response_headers).to eq({})
		end

		it "bounds the whole check when a match regex is pathological" do
		rule = make_rule(name: "slow_regex", match: { ip: { re: '(a+)+\1$' } })
		id = Labkit::RateLimit::Identifier.new(user: 42, ip: "#{'a' * 60}!")
		subject = described_class.new(
		name: "rack_request", rules: [rule], redis: redis, logger: null_logger
		)

		elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC)
		subject.check(id)
		elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - elapsed

		expect(elapsed).to be < (Labkit::RateLimit::Matcher::MATCH_TIMEOUT_SECONDS * 20)
		end
		end

		describe "Metrics emission", :with_metrics_config do

spec/labkit/rate_limit/matcher_spec.rb

+69 −2

Original line number	Diff line number	Diff line
		@@ -34,11 +34,35 @@ RSpec.describe Labkit::RateLimit::Matcher do
		end

		context "with a bare Regexp (Ruby convenience)" do
		it "stores the Regexp directly as :re (no recompilation)" do
		it "stores an equivalent Regexp carrying the match timeout" do
		re = %r{^/api/v\d+}
		m = described_class.build(re)
		expect(m.type).to eq(:re)
		expect(m.value).to equal(re) # same object, not a re-compiled copy
		# Recompiled rather than stored as-is, so the timeout applies to this
		# path too. Equivalent pattern, different object.
		expect(m.value).to eq(re)
		expect(m.value.timeout).to eq(described_class::MATCH_TIMEOUT_SECONDS)
		end

		it "does not mutate the caller's Regexp" do
		re = %r{^/api/v\d+}
		described_class.build(re)
		expect(re.timeout).to be_nil
		end

		it "preserves flags and encoding semantics when applying the timeout" do
		re = /\A[[:alpha:]]+\z/iu
		m = described_class.build(re)
		expect(m.value.options).to eq(re.options)
		expect(m.value.fixed_encoding?).to eq(re.fixed_encoding?)
		expect(m.match?("ABC")).to be(true)
		end

		it "leaves a Regexp that already declares its own timeout untouched" do
		re = Regexp.new("^/api/", timeout: 0.5)
		m = described_class.build(re)
		expect(m.value).to equal(re)
		expect(m.value.timeout).to eq(0.5)
		end
		end

		@@ -67,6 +91,11 @@ RSpec.describe Labkit::RateLimit::Matcher do
		expect(m.value.source).to eq("^/api/v\\d+/projects")
		end

		it "compiles with the match timeout applied" do
		m = described_class.build(re: "^/api/v\\d+/projects")
		expect(m.value.timeout).to eq(described_class::MATCH_TIMEOUT_SECONDS)
		end

		it "accepts a Regexp inside { re: ... } and stores it directly" do
		re = %r{^/api/v\d+/projects}
		m = described_class.build(re: re)
		@@ -259,6 +288,44 @@ RSpec.describe Labkit::RateLimit::Matcher do
		expect { matcher.match?("x") }.to raise_error(ArgumentError, /unknown matcher type/)
		end
		end

		context "with a pathological pattern" do
		# Ruby 3.2+ memoization defeats most classic catastrophic patterns, but
		# it does not apply once a backreference is present - so this one really
		# does backtrack, and is the shape the timeout has to catch.
		let(:catastrophic) { '(a+)+\1$' }
		let(:pathological_input) { "#{'a' * 60}!" }

		it "aborts the match instead of backtracking unbounded" do
		matcher = described_class.build(re: catastrophic)

		expect { matcher.match?(pathological_input) }
		.to raise_error(Regexp::TimeoutError)
		end

		it "gives up within a small multiple of the configured timeout" do
		matcher = described_class.build(re: catastrophic)

		elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC)
		begin
		matcher.match?(pathological_input)
		rescue Regexp::TimeoutError
		nil
		end
		elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - elapsed

		# Generous upper bound: asserts the timeout is in force at all,
		# without being flaky on a loaded CI box.
		expect(elapsed).to be < (described_class::MATCH_TIMEOUT_SECONDS * 20)
		end

		it "leaves ordinary patterns unaffected" do
		matcher = described_class.build(re: '\A/api/')

		expect(matcher.match?("/api/v4/projects")).to be(true)
		expect(matcher.match?("/dashboard")).to be(false)
		end
		end
		end

		describe "value equality (Data.define)" do