From 6a88d8e084b777b6926db79c3f5dc409c784a07e Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 18 Sep 2026 03:00:15 +0900 Subject: [PATCH] Parse the WWW-Authenticate challenge in linear time ## Motivation and Context `Discovery.parse_www_authenticate` walked the Bearer challenge's parameters by slicing the header at the cursor and trimming the slice for every `key=value` pair, which copies the remainder of the header each time. The cost therefore grew with the square of the header's length, and the header is the server's to choose: parsing one with 200,000 parameters took over 20 seconds, on every request that drew such a `401` or `403`, before the client could act on it. The walk now uses a `StringScanner`, which matches each pair at the current position without copying, so the cost grows with the length of the header. The scanner locates the Bearer challenge as well, so a header whose earlier challenge holds multibyte text yields the same parameters as before. The header is also the server's to fill. A value whose bytes are not valid in the string's encoding, such as a header tagged UTF-8 that carries a stray `0xFF`, made the patterns raise `ArgumentError` from inside the transport's `401` handling, ahead of any `AuthorizationError` a caller is prepared for; the default adapter hands header values over as ASCII-8BIT, where every byte is valid, so it took a caller-supplied adapter or a header re-tagged along the way. The header is now scrubbed before it is parsed, so such bytes become the replacement character and the other parameters still come through, while a binary header is unchanged by the scrub. ## How Has This Been Tested? New tests in `test/mcp/client/oauth/discovery_test.rb` parse a header with 200,000 parameters within a bound the previous implementation exceeded many times over, and a header whose earlier challenge holds multibyte text. Two more parse a UTF-8 header with an invalid byte inside a quoted value, which raised against the previous library, and a binary header with a high byte. ## Breaking Changes None. --- lib/mcp/client/oauth/discovery.rb | 35 +++++++++++--------- test/mcp/client/oauth/discovery_test.rb | 43 +++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 16 deletions(-) diff --git a/lib/mcp/client/oauth/discovery.rb b/lib/mcp/client/oauth/discovery.rb index a8ad3be7..15cd73ee 100644 --- a/lib/mcp/client/oauth/discovery.rb +++ b/lib/mcp/client/oauth/discovery.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "ipaddr" +require "strscan" require "uri" module MCP @@ -43,6 +44,12 @@ module Discovery # or a bare token, per RFC 7235. WWW_AUTH_PARAM_PATTERN = /\A([A-Za-z0-9_-]+)\s*=\s*(?:"((?:[^"\\]|\\.)*)"|([^\s,]+))/.freeze + # The whitespace and optional comma between two `key=value` pairs, or before the first one. + WWW_AUTH_PARAM_SEPARATOR_PATTERN = /\s*,?\s*/.freeze + + # The `Bearer` challenge: at the start of the header or after a comma. + WWW_AUTH_BEARER_PATTERN = /(?:\A|,)\s*Bearer(?:\s+|\z)/i.freeze + class << self # Parses a `WWW-Authenticate` header and returns the parameters of # the `Bearer` challenge as a hash with lower-cased keys (e.g. `resource_metadata`, @@ -55,25 +62,21 @@ class << self def parse_www_authenticate(header) return {} unless header - # Locate the Bearer challenge: at the start of the header or after a comma. - bearer = header.match(/(?:\A|,)\s*Bearer(?:\s+|\z)/i) - return {} unless bearer - # Walk key=value pairs starting where Bearer's parameters begin. - # The loop stops at the first token that is not a key=value pair, - # which marks the next challenge (e.g. `, DPoP algs="..."`). - cursor = bearer.end(0) - params = {} - while cursor < header.length - prefix = header[cursor..] - prefix = prefix.sub(/\A\s*,?\s*/, "") - break if prefix.empty? + # The loop stops at the first token that is not a key=value pair, which marks the next challenge (e.g. `, DPoP algs="..."`). + # The scanner keeps the walk linear in the header's length: slicing off the consumed prefix instead copies the remainder + # for every pair, and the server chooses how many pairs it sends. The header is also the server's to fill: a byte sequence + # that is not valid in the string's encoding would make the patterns raise `ArgumentError`, so such bytes are replaced first. + scanner = StringScanner.new(header.scrub) + return {} unless scanner.skip_until(WWW_AUTH_BEARER_PATTERN) - match = prefix.match(WWW_AUTH_PARAM_PATTERN) - break unless match + params = {} + until scanner.eos? + scanner.skip(WWW_AUTH_PARAM_SEPARATOR_PATTERN) + break if scanner.eos? + break unless scanner.scan(WWW_AUTH_PARAM_PATTERN) - params[match[1].downcase] = match[2] ? unescape_quoted_pair(match[2]) : match[3] - cursor = header.length - prefix.length + match.end(0) + params[scanner[1].downcase] = scanner[2] ? unescape_quoted_pair(scanner[2]) : scanner[3] end params end diff --git a/test/mcp/client/oauth/discovery_test.rb b/test/mcp/client/oauth/discovery_test.rb index d58c7f2f..02a19cd9 100644 --- a/test/mcp/client/oauth/discovery_test.rb +++ b/test/mcp/client/oauth/discovery_test.rb @@ -79,6 +79,49 @@ def test_parse_www_authenticate_unescapes_quoted_pair assert_equal('value with "quoted" word and a back\\slash', params["error_description"]) end + def test_parse_www_authenticate_finds_bearer_after_a_challenge_with_multibyte_text + # The Bearer challenge is located by byte position, so text before it that is wider than + # one byte per character must not shift where its parameters are read from. + params = Discovery.parse_www_authenticate(%(Basic realm="café", Bearer scope="s")) + + assert_equal({ "scope" => "s" }, params) + end + + def test_parse_www_authenticate_tolerates_bytes_that_are_invalid_in_the_header_encoding + # A value the server fills with bytes that are not valid UTF-8 must not turn the `401` into + # an `ArgumentError`; the bytes are replaced and the other parameters still come through. + header = %(Bearer error="invalid_token", scope="s\xff", realm="r").dup.force_encoding(Encoding::UTF_8) + + params = Discovery.parse_www_authenticate(header) + + assert_equal("invalid_token", params["error"]) + assert_equal("s�", params["scope"]) + assert_equal("r", params["realm"]) + end + + def test_parse_www_authenticate_reads_a_binary_header + # Net::HTTP hands header values over as ASCII-8BIT; high bytes are kept as they are. + params = Discovery.parse_www_authenticate(%(Bearer scope="s\xff", realm="r").b) + + assert_equal("s\xff".b, params["scope"]) + assert_equal("r", params["realm"]) + end + + def test_parse_www_authenticate_walks_a_header_with_many_parameters_in_linear_time + # The header is the server's to choose. Slicing off the consumed prefix for every pair copied + # the remainder each time, so 200,000 pairs took tens of seconds; the bound below is loose enough + # for a slow CI machine and far below that. + header = "Bearer " + (1..200_000).map { |i| %(k#{i}="v#{i}") }.join(", ") + + started = Process.clock_gettime(Process::CLOCK_MONOTONIC) + params = Discovery.parse_www_authenticate(header) + elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started + + assert_equal(200_000, params.size) + assert_equal("v200000", params["k200000"]) + assert_operator(elapsed, :<, 5) + end + def test_protected_resource_metadata_urls_uses_explicit_url_first urls = Discovery.protected_resource_metadata_urls( server_url: "https://api.example.com/mcp",