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