Skip to content

Commit 5e2edae

Browse files
committed
Resolve dot segments in linear time when canonicalizing a URL
## Motivation and Context `Discovery.canonicalize_url` resolved dot segments with RFC 3986 Section 5.2.4 applied literally: every `.` or `..` rewrote the remaining input buffer, which copies it each time, so the cost grew with the square of the path's length. The path is the server's to choose, through a Protected Resource Metadata `resource` or an endpoint URL that reaches the canonicalization: resolving a 2 MB path made of `a/../` took over 30 seconds, on every flow that received it. The segments are now visited once with a stack, which is the same algorithm expressed over segments instead of over the buffer. The output is unchanged, including the RFC's handling of a trailing `/.` or `/..` (the slash stays) and of a relative path (which `URI#path` never yields), as checked against the previous implementation on 100,000 random paths. ## How Has This Been Tested? New tests in `test/mcp/client/oauth/discovery_test.rb` canonicalize a URL with 300,000 dot segments within a bound the previous implementation exceeded many times over, and pin the trailing-slash and empty-segment cases. ## Breaking Changes None.
1 parent c9641fb commit 5e2edae

2 files changed

Lines changed: 56 additions & 34 deletions

File tree

‎lib/mcp/client/oauth/discovery.rb‎

Lines changed: 33 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -579,47 +579,46 @@ def normalize_query(query)
579579
end.join("&")
580580
end
581581

582-
# Implements RFC 3986 Section 5.2.4 `remove_dot_segments`. Walks the input
583-
# buffer one segment at a time, popping the previous output segment
584-
# whenever a `..` is encountered, so that `/api/../mcp` collapses to
585-
# `/mcp` and `/foo/./bar` collapses to `/foo/bar`.
582+
# Implements RFC 3986 Section 5.2.4 `remove_dot_segments` over the path's segments, so that `/api/../mcp` collapses to
583+
# `/mcp` and `/foo/./bar` collapses to `/foo/bar`. Each segment is visited once: the RFC's buffer rewriting,
584+
# applied literally, copies the remaining input for every dot segment, and the path is the server's to choose.
585+
#
586+
# The output matches the RFC's algorithm for a relative path as well, including its quirk that a `..` popping
587+
# the first segment leaves the result absolute (`a/../b` becomes `/b`), although `URI#path` never hands over a relative path.
588+
# The rule letters below are the RFC's own: steps A through E of its loop.
586589
# https://www.rfc-editor.org/rfc/rfc3986#section-5.2.4
587590
def remove_dot_segments(path)
588591
return path if path.nil? || path.empty?
589592

590-
input = path.dup
591-
output = +""
592-
until input.empty?
593-
if input.start_with?("../")
594-
input = input[3..]
595-
elsif input.start_with?("./")
596-
input = input[2..]
597-
elsif input.start_with?("/./")
598-
input = "/#{input[3..]}"
599-
elsif input == "/."
600-
input = "/"
601-
elsif input.start_with?("/../")
602-
input = "/#{input[4..]}"
603-
output = remove_last_segment(output)
604-
elsif input == "/.."
605-
input = "/"
606-
output = remove_last_segment(output)
607-
elsif input == "." || input == ".."
608-
input = ""
593+
absolute = path.start_with?("/")
594+
segments = path.split("/", -1)
595+
segments.shift if absolute
596+
597+
output = []
598+
599+
# True until a segment other than `.` or `..` is kept: a relative path's leading `./` and `../` are dropped together with
600+
# the slash after them (Rule A), so the segment that follows is still the slash-less first one.
601+
leading = !absolute
602+
segments.each_with_index do |segment, index|
603+
last = index == segments.length - 1
604+
605+
# Rule A, and Rule D for a path that is nothing but `.` or `..`.
606+
next if leading && (segment == "." || segment == "..")
607+
608+
if segment == "."
609+
# Rule B: `/./` disappears; a final `/.` leaves its slash behind.
610+
output << "/" if last
611+
elsif segment == ".."
612+
# Rule C: `/../` removes the previous segment; a final `/..` leaves its slash behind.
613+
output.pop
614+
output << "/" if last
609615
else
610-
segment = input.match(%r{\A/?[^/]*})[0]
611-
output << segment
612-
input = input[segment.length..]
616+
# Rule E: the first segment of a relative path carries no slash.
617+
output << (leading ? segment : "/#{segment}")
613618
end
619+
leading = false
614620
end
615-
output
616-
end
617-
618-
def remove_last_segment(output)
619-
idx = output.rindex("/")
620-
return +"" if idx.nil?
621-
622-
output[0...idx]
621+
output.join
623622
end
624623
end
625624
end

‎test/mcp/client/oauth/discovery_test.rb‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -291,6 +291,29 @@ def test_canonicalize_url_resolves_percent_encoded_dot_segments
291291
)
292292
end
293293

294+
def test_canonicalize_url_keeps_the_slash_a_final_dot_segment_leaves_behind
295+
# RFC 3986 Section 5.2.4 turns a final `/.` or `/..` into `/`, so the path ends in a slash rather than
296+
# losing it with the segment; an empty segment is a segment too.
297+
assert_equal("https://srv.example.com/a/", Discovery.canonicalize_url("https://srv.example.com/a/."))
298+
assert_equal("https://srv.example.com/a/", Discovery.canonicalize_url("https://srv.example.com/a/b/.."))
299+
assert_equal("https://srv.example.com/a//b", Discovery.canonicalize_url("https://srv.example.com/a//b"))
300+
assert_equal("https://srv.example.com", Discovery.canonicalize_url("https://srv.example.com/.."))
301+
end
302+
303+
def test_canonicalize_url_resolves_a_path_with_many_dot_segments_in_linear_time
304+
# The path is the server's to choose (a PRM `resource`, an endpoint URL). Rewriting the input buffer
305+
# for every dot segment copied the remainder each time, so 300,000 of them took tens of seconds;
306+
# the bound below is loose enough for a slow CI machine and far below that.
307+
url = "https://srv.example.com/#{"a/../" * 300_000}mcp"
308+
309+
started = Process.clock_gettime(Process::CLOCK_MONOTONIC)
310+
canonical = Discovery.canonicalize_url(url)
311+
elapsed = Process.clock_gettime(Process::CLOCK_MONOTONIC) - started
312+
313+
assert_equal("https://srv.example.com/mcp", canonical)
314+
assert_operator(elapsed, :<, 5)
315+
end
316+
294317
def test_canonicalize_url_drops_userinfo
295318
# The canonicalized URL is sent on the wire as the RFC 8707 `resource`
296319
# claim and is surfaced in error messages, so credentials in

0 commit comments

Comments
 (0)