From 999bb73e92a760ba80170954395cde388ee3f2d2 Mon Sep 17 00:00:00 2001 From: Eric Proulx Date: Wed, 9 Sep 2026 09:12:06 +0200 Subject: [PATCH] Resolve a '%'-free path against a union without the encodings Every route pattern is compiled with Mustermann's `uri_decode: true`, which expands each literal character of a path into an alternation with its percent-encodings -- `/resource7` becomes `\/(?:r|%72)(?:e|%65)(?:s|%73)(?:o|%6F|%6f)...` -- so that `/a%2Eb` still reaches a route declared as `/a.b`. The router's per-method union is the disjunction of those patterns, so every request pays for the expansion: it roughly doubles the compiled source, and it hides the literal prefix each branch starts with, which is what would otherwise let Onigmo reject a non-matching path without walking the alternatives. On a 500-route API, failing to match cost 180us. An input holding no '%' cannot take any of those alternatives, so for it the union without them accepts exactly the same strings, by the same path through the regexp, capturing the same substrings. `compile!` now derives that stand-in per union and `transaction`, `#match?` and `#greedy_match?` read it whenever the path has no '%'. The stand-in is cut out of the union's source rather than recompiled from each pattern with `uri_decode: false`. That is 20x cheaper -- one gsub and one `Regexp.new` for the whole union against a fresh Mustermann parse per route -- and it is the more faithful transformation: `uri_decode: false` also drops the `\+` branch that lets a plus match a literal space, which would make `/with+space` (no '%' anywhere, so resolved here) miss a route the decode-aware union matches. Only alternations are rewritten, and only their `%XX` alternatives are dropped, so a `requirements` Regexp inserted into the pattern verbatim survives whatever it holds. Two shapes are stepped over rather than rewritten: a character class, where `(?:` and `|` are literal characters and dropping them would shrink the set a route matches, and an escape, so an escaped bracket cannot open a class. The group itself is only unwrapped around a lone character -- a quantifier binds to the group, so `(?:ab|%41)+` must not become `ab+` -- which is every path literal, and the bare form is what the prefix optimisation needs. GET routes are mirrored for HEAD and the greedy neighbours mirror both, so the same source is usually compiled several times over; the stand-in is built once per distinct source and shared. Without that, and with the rewrite memoised per distinct expansion rather than recomputed per match, boot for 500 routes grew 84% -- with both, 10%. Measured on Ruby 4.0.6 without YJIT, `format :json` and a two-version path prefix, hitting the last route of the union: 43.3k -> 67.9k i/s at 50 routes (+57%), 15.5k -> 31.7k at 200 (+104%), 6.9k -> 15.5k at 500 (+126%). A path that matches nothing: 5.5k -> 433.0k i/s (78x). A path carrying a percent-encoding still goes through the decode-aware union and is unchanged (-0.2%), as is the first route of a union and a single-route API, which pay only the `String#include?`. Boot, full API instance build: 7.3 -> 8.1 ms at 50 routes, 29.5 -> 32.3 at 200, 77.4 -> 85.1 at 500, 160.1 -> 173.5 at 1000. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + lib/grape/router.rb | 40 +++++++- lib/grape/router/plain_union.rb | 130 ++++++++++++++++++++++++++ spec/grape/router/plain_union_spec.rb | 122 ++++++++++++++++++++++++ spec/grape/router_spec.rb | 68 ++++++++++++++ 5 files changed, 357 insertions(+), 4 deletions(-) create mode 100644 lib/grape/router/plain_union.rb create mode 100644 spec/grape/router/plain_union_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index d2c1a6870..e8b30a85a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * [#2915](https://github.com/ruby-grape/grape/pull/2915): Update rubocop to 1.90.0 and rubocop-performance to 1.27.0 - [@ericproulx](https://github.com/ericproulx). * [#2918](https://github.com/ruby-grape/grape/pull/2918): Skip the dry-types round trip when a value already is the declared type - [@ericproulx](https://github.com/ericproulx). * [#2917](https://github.com/ruby-grape/grape/pull/2917): Read path captures out of the router's union match instead of re-running the route's pattern - [@ericproulx](https://github.com/ericproulx). +* [#2919](https://github.com/ruby-grape/grape/pull/2919): Resolve a path carrying no percent-encoding against a union compiled without them - [@ericproulx](https://github.com/ericproulx). * Your contribution here. ### 4.0.0 (2026-09-07) diff --git a/lib/grape/router.rb b/lib/grape/router.rb index a83cd2023..3d98ddd51 100644 --- a/lib/grape/router.rb +++ b/lib/grape/router.rb @@ -10,12 +10,23 @@ def initialize # so request-time reads never mutate shared state (see #match? / #rotation). @map = {} @optimized_map = {} + # The same unions as @optimized_map / @union, with the percent-encoded + # alternatives of their path literals removed (see PlainUnion). Read for + # any input that carries no '%', which is nearly all of them. + @plain_map = {} end def compile! return if @compiled + # A route registered for GET is mirrored for HEAD, and the greedy + # neighbours mirror both, so the same source is usually compiled several + # times over. Its stand-in is built once and shared: a Regexp carries no + # per-match state, and unions with the same source number their groups + # the same way. + plain_unions = {} @union = resolve_capture_groups(Regexp.union(@neutral_regexes), @neutral_map) + @plain_union = plain_unions[@union.source] = PlainUnion.build(@union) @neutral_regexes = nil # Compiled from the routes actually registered rather than from # Grape::HTTP_SUPPORTED_METHODS. A route declared with any other verb @@ -26,10 +37,13 @@ def compile! # answered every request for it with 405. @map.each do |method, routes| optimized_map = routes.map.with_index { |route, index| route.to_regexp(index) } - @optimized_map[method] = resolve_capture_groups(Regexp.union(optimized_map), routes) + union = resolve_capture_groups(Regexp.union(optimized_map), routes) + @optimized_map[method] = union + @plain_map[method] = plain_unions[union.source] ||= PlainUnion.build(union) end @map.freeze @optimized_map.freeze + @plain_map.freeze @compiled = true end @@ -59,6 +73,7 @@ def recognize_path(input) any.endpoint end + PERCENT = '%' DEFAULT_RESPONSE_HEADERS = Grape::Util::Header.new.merge('X-Cascade' => 'pass').freeze DEFAULT_RESPONSE_BODY = ['404 Not Found'].freeze @@ -80,7 +95,7 @@ def transaction(input, method, env) # route's path captures are groups of it (see Route#params_for). exact_route = nil response = nil - @optimized_map[method]&.match(input) do |m| + unions(input)[method]&.match(input) do |m| exact_route = @map[method].detect { |route| m[route.regexp_capture_group] } response = process_route(exact_route, input, env, m) if exact_route end @@ -200,11 +215,28 @@ def default_response # name sends MatchData through the pattern's name table on every lookup, # a number indexes the match region directly. def match?(input, method) - @optimized_map[method]&.match(input) { |m| @map[method].detect { |route| m[route.regexp_capture_group] } } + unions(input)[method]&.match(input) { |m| @map[method].detect { |route| m[route.regexp_capture_group] } } end def greedy_match?(input) - @union.match(input) { |m| @neutral_map.detect { |route| m[route.regexp_capture_group] } } + neutral_union(input).match(input) { |m| @neutral_map.detect { |route| m[route.regexp_capture_group] } } + end + + # The compiled unions to resolve +input+ against. A path with no '%' cannot + # match a percent-encoded literal, so it is resolved against the cheaper + # unions {PlainUnion} derived from these -- which accept exactly the same + # '%'-free strings, through the same groups. + # + # Re-read from +input+ at each of the three call sites rather than resolved + # once per request and threaded down: only the first of them runs on a + # request that matches a route, and String#include? on a path is a byte + # scan, cheaper than the extra argument would be to pass. + def unions(input) + input.include?(PERCENT) ? @optimized_map : @plain_map + end + + def neutral_union(input) + input.include?(PERCENT) ? @union : @plain_union end def cascade?(response) diff --git a/lib/grape/router/plain_union.rb b/lib/grape/router/plain_union.rb new file mode 100644 index 000000000..544c1a787 --- /dev/null +++ b/lib/grape/router/plain_union.rb @@ -0,0 +1,130 @@ +# frozen_string_literal: true + +module Grape + class Router + # Derives, from a compiled route union, an equivalent matcher for the + # inputs that carry no %. + # + # Grape compiles every route pattern with Mustermann's +uri_decode: true+, + # which expands each *literal* character of a path into an alternation with + # its percent-encodings -- +/resource+ becomes + # \/(?:r|%72)(?:e|%65)(?:s|%73)(?:o|%6F|%6f)… -- so that + # +/a%2Eb+ still reaches a route declared as +/a.b+. The router's union is + # the disjunction of those patterns, so the expansion is paid on every + # request: it roughly doubles the compiled source, and on a union of a few + # hundred routes it costs both the successful match and -- by hiding the + # literal prefix every branch starts with -- the far cheaper failure that + # Onigmo could otherwise reach. + # + # An input holding no +%+ cannot take any of those alternatives, so for it + # the union without them accepts exactly the same strings, by the same path + # through the regexp, capturing exactly the same substrings. This module + # removes them. + # + # The surgery is done on the union's source rather than by recompiling each + # pattern with +uri_decode: false+, for two reasons: it is an order of + # magnitude cheaper at boot (one +gsub+ and one +Regexp.new+ for the whole + # union, against a fresh Mustermann parse per route), and it is the more + # faithful transformation. +uri_decode: false+ also drops the +\++ branch + # that lets a plus match a literal space in the path, which would make a + # +%+-free request miss a route the decode-aware union matches; removing + # only the +%XX+ branches keeps it. + module PlainUnion + # One alternative of a compiled alternation: escapes count as one unit, + # and no alternative of Mustermann's character expansion contains a + # bracket or a bar of its own. + ALTERNATIVE = /(?:\\.|[^\\()|])+/ + + # A whole (?:a|b|…) alternation, the shape +Compiler#encoded+ + # emits for a path literal. + ALTERNATION = /\(\?:#{ALTERNATIVE}(?:\|#{ALTERNATIVE})+\)/ + + # Regions to step over rather than rewrite, so that an alternation is only + # ever recognised where one can actually occur. A +requirements+ Regexp is + # inserted into the pattern verbatim, and inside a character class + # (?: and | are literal characters -- rewriting + # [(?:a|%41)] would drop +|+, +%+, +4+ and +1+ from the set it + # matches, which is a change a '%'-free request would see. Consuming + # escapes as units keeps an escaped bracket from opening a class. + SKIPPED = /\\.|\[(?:\\.|[^\]\\])*\]/ + + # One pass over the source: a region to step over, or an alternation to + # rewrite. Ordered so a character class is consumed before anything + # inside it can be recognised as an alternation of its own. + SCANNER = /#{SKIPPED}|#{ALTERNATION}/ + + # An alternative that is only a percent-encoded octet. + PERCENT_ENCODED = /\A%\h\h\z/ + + # An alternative that is a single character, escaped or not -- everything + # +Compiler#encoded+ emits, and the only shape whose group is safe to drop + # (see #strip_percent_alternatives). + SINGLE_CHARACTER = /\A(?:\\.|[^\\])\z/ + + class << self + # @param union [Regexp] the compiled union of every route's pattern + # @return [Regexp] the same union with the percent-encoded alternatives + # of its path literals removed, or +union+ itself when there is + # nothing to remove or the result would not be a faithful stand-in + def build(union) + source = strip_percent_alternatives(union.source) + return union if source == union.source + + plain = Regexp.new(source) + # Route matching reads the union's groups by number + # (see BaseRoute#union_captures), so a stand-in that numbered or + # named them differently would hand the wrong substrings to the + # endpoint. Nothing observed does this -- the alternatives dropped + # here are non-capturing -- but the check is one comparison at boot + # against silently wrong params, and falling back to the decode-aware + # union only costs speed. + plain.named_captures == union.named_captures ? plain : union + rescue RegexpError + union + end + + private + + # Rewrites (?:o|%6F|%6f) to o and + # (?: |%20|\+|%2B) to (?: |\+), and leaves every other + # alternation -- a +requirements+ Regexp, the declared-versions union -- + # untouched. + # + # Replaced through a Hash rather than a block: a union repeats the same + # few dozen expansions thousands of times over, and memoizing their + # rewrites is an order of magnitude faster than recomputing one per + # match. + def strip_percent_alternatives(source) + rewrites = Hash.new { |memo, region| memo[region] = rewrite(region) } + source.gsub(SCANNER, rewrites) + end + + # @param region [String] one {SCANNER} match: an alternation to rewrite, + # or an escape or character class to hand back untouched + def rewrite(region) + # A skipped region starts with a backslash or a bracket, so this tells + # the two apart without a second capture group. + return region unless region.start_with?('(?:') + + # Scanned rather than split on '|': an alternative can be an escaped + # bar of its own ('/a\\|b' compiles to (?:\\||%7C|%7c)), + # which a split would cut in half. + alternatives = region[3..-2].scan(ALTERNATIVE) + kept = alternatives.grep_v(PERCENT_ENCODED) + return region if kept.size == alternatives.size || kept.empty? + + # The group is only dropped around a lone *character*: a quantifier can + # follow the alternation, and it binds to the group, not to what the + # group holds. A path literal is always a single character, so the case + # that matters keeps its bare form -- which is what lets Onigmo read a + # literal prefix off the union again. A longer alternative can only + # come from a +requirements+ Regexp, where (?:ab|%41)+ must + # not become ab+. + return kept.first if kept.size == 1 && kept.first.match?(SINGLE_CHARACTER) + + "(?:#{kept.join('|')})" + end + end + end + end +end diff --git a/spec/grape/router/plain_union_spec.rb b/spec/grape/router/plain_union_spec.rb new file mode 100644 index 000000000..47deca9fa --- /dev/null +++ b/spec/grape/router/plain_union_spec.rb @@ -0,0 +1,122 @@ +# frozen_string_literal: true + +describe Grape::Router::PlainUnion do + def build_pattern(path, requirements: {}, version: nil) + Grape::Router::Pattern.new(origin: path, suffix: '', anchor: true, params: {}, version:, requirements:) + end + + describe '.build' do + subject(:plain) { described_class.build(union) } + + context 'with a path literal' do + let(:union) { build_pattern('/resource/:id').to_regexp } + + it 'drops the percent-encoded alternatives' do + expect(plain.source).not_to include('%') + end + + it 'still matches the path' do + expect(plain).to match('/resource/42') + end + + it 'keeps the named captures the union had' do + expect(plain.named_captures).to eq(union.named_captures) + end + end + + # Mustermann's own `uri_decode: false` compiles a space to a bare `\ `, + # dropping the `+` branch along with the `%20` one. That would make + # `/with+space` -- a request carrying no '%' at all, so one resolved here -- + # miss a route the decode-aware union matches. + context 'with a space in the path' do + let(:union) { build_pattern('/with space/:id').to_regexp } + + it 'keeps the branch that matches a plus' do + expect(plain).to match('/with+space/7') + end + + it 'keeps the branch that matches a literal space' do + expect(plain).to match('/with space/7') + end + end + + # A requirements Regexp can quantify an alternation, and the quantifier + # binds to the group. Unwrapping a multi-character alternative out of it + # would rebind the quantifier to that alternative's last character. + context 'with a quantified multi-character alternation' do + let(:union) { build_pattern('/resource/:id', requirements: { id: /(?:ab|%41)+/ }).to_regexp } + + it 'keeps the group around the surviving alternative' do + expect(plain.source).to include('(?:ab)+') + end + + it 'still matches a repetition of the whole alternative' do + expect(plain).to match('/resource/abab') + end + + it 'does not start matching a repetition of its last character' do + expect(plain).not_to match('/resource/abb') + end + end + + context 'with an escaped bar in the path' do + let(:union) { build_pattern('/a\\|b/:id').to_regexp } + + it 'does not cut the escape in half' do + expect(plain).to match('/a|b/7') + end + end + + context 'with declared versions' do + let(:union) { build_pattern('/:version/resource', version: %w[v1 v2]).to_regexp } + + it 'leaves the versions alternation alone' do + expect(plain.source).to include('v1|v2') + end + end + + context 'when there is nothing to strip' do + let(:union) { build_pattern('/:id').to_regexp } + + it 'returns the union itself' do + expect(plain).to be(union) + end + end + + # A requirements Regexp is inserted into the pattern verbatim, so it can + # carry a character class holding the very characters the rewrite looks for. + # Inside a class they are literals, and dropping them would shrink the set + # the route matches. + context 'with a requirements Regexp whose character class looks like an alternation' do + let(:union) { build_pattern('/resource/:id', requirements: { id: /[(?:a|%41)]+/ }).to_regexp } + + it 'leaves the character class untouched' do + expect(plain.source).to include('[(?:a|%41)]') + end + + it 'still matches every character of the class' do + expect(plain).to match('/resource/|4') + end + end + + context 'when the rewritten source would not compile' do + let(:union) { build_pattern('/resource/:id').to_regexp } + + before { allow(described_class).to receive(:strip_percent_alternatives).and_return('(') } + + it 'falls back to the union' do + expect(plain).to be(union) + end + end + + context 'when the rewritten source would renumber the captures' do + let(:union) { build_pattern('/resource/:id').to_regexp } + + before { allow(described_class).to receive(:strip_percent_alternatives).and_return('(?x)') } + + it 'falls back to the union' do + expect(plain).to be(union) + end + end + end +end diff --git a/spec/grape/router_spec.rb b/spec/grape/router_spec.rb index 8352f841a..3c160b060 100644 --- a/spec/grape/router_spec.rb +++ b/spec/grape/router_spec.rb @@ -18,6 +18,7 @@ it 'freezes the internal maps after compilation' do expect(router.instance_variable_get(:@map)).to be_frozen expect(router.instance_variable_get(:@optimized_map)).to be_frozen + expect(router.instance_variable_get(:@plain_map)).to be_frozen end # Regression: the maps used to be auto-vivifying hashes, so a request whose @@ -179,4 +180,71 @@ def response_body expect(body['params']).to eq('name' => '123') end end + + # Every route pattern is compiled with Mustermann's uri_decode, which expands + # each literal path character into an alternation with its percent-encodings. + # The router resolves a request carrying no '%' against unions built without + # them (see PlainUnion); the two must agree on every such path, and the + # decode-aware ones must still answer the requests that do carry one. + describe 'percent-encoded paths' do + let(:app) do + Class.new(Grape::API) do + format :json + get('/a.b/:id') { { route: 'dotted', id: params[:id] } } + get('/with space/:id') { { route: 'spaced', id: params[:id] } } + end + end + + it 'compiles a separate union without the percent-encoded alternatives' do + router = app.compile!.router + optimized_map = router.instance_variable_get(:@optimized_map) + plain_map = router.instance_variable_get(:@plain_map) + + expect(plain_map['GET']).not_to be(optimized_map['GET']) + expect(plain_map['GET'].source.size).to be < optimized_map['GET'].source.size + end + + # GET routes are mirrored for HEAD, so both unions compile from the same + # source and the stand-in is built once for the two of them. + it 'shares one stand-in between unions compiled from the same source' do + plain_map = app.compile!.router.instance_variable_get(:@plain_map) + + expect(plain_map['HEAD']).to be(plain_map['GET']) + end + + it 'routes a path with no percent-encoding' do + get '/a.b/7' + + expect(last_response.status).to eq(200) + expect(JSON.parse(last_response.body)).to eq('route' => 'dotted', 'id' => '7') + end + + it 'routes a percent-encoded path literal to the same route' do + get '/a%2Eb/7' + + expect(last_response.status).to eq(200) + expect(JSON.parse(last_response.body)).to eq('route' => 'dotted', 'id' => '7') + end + + it 'decodes a percent-encoded path capture' do + get '/a.b/caf%C3%A9' + + expect(JSON.parse(last_response.body)).to eq('route' => 'dotted', 'id' => 'café') + end + + # A plus stands for a space in a path literal, and carries no '%' of its + # own -- so it is resolved against the union the percent-encodings were + # stripped from, which has to have kept that branch. + it 'routes a plus onto a literal space' do + get '/with+space/7' + + expect(JSON.parse(last_response.body)).to eq('route' => 'spaced', 'id' => '7') + end + + it 'still 404s a path that matches no route' do + get '/nothing/here' + + expect(last_response.status).to eq(404) + end + end end