Decode route values from RawTarget (#11544)#66251
Decode route values from RawTarget (#11544)#66251mikekistler wants to merge 1 commit intodotnet:mainfrom
Conversation
Use RawTarget when capturing route values in endpoint and legacy routing while preserving Request.Path-based matching and constraint behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates ASP.NET Core routing so that captured route values are decoded from IHttpRequestFeature.RawTarget (when applicable) after a match is established, fixing inconsistent/partial decoding scenarios like %2F vs %2B described in #11544.
Changes:
- Add RawTarget-based decoding for captured route values in endpoint matching (
DfaMatcher) and legacy routing (RoutePatternMatcherviaTemplateMatcher). - Introduce
RawTargetRouteValueDecoderhelper to tokenizeRawTarget(accounting forPathBase) and decode captured segments once. - Add regression tests covering
%2F,%252F, catch-alls, absolute-like segments, andPathBase.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Http/Routing/test/UnitTests/RouteTest.cs | Adds legacy routing test validating RawTarget-based decoding of captured values. |
| src/Http/Routing/test/UnitTests/Matching/DfaMatcherTest.cs | Adds endpoint routing tests validating RawTarget-based decoding (including PathBase). |
| src/Http/Routing/src/Tree/TreeRouter.cs | Switches legacy route matching to pass HttpContext into template matching so capture can use RawTarget. |
| src/Http/Routing/src/Template/TemplateMatcher.cs | Adds TryMatch(HttpContext, ...) to enable RawTarget-aware capture. |
| src/Http/Routing/src/RouteBase.cs | Switches RouteBase matching to pass HttpContext into template matching. |
| src/Http/Routing/src/Patterns/RoutePatternMatcher.cs | Implements RawTarget-aware capture logic during TryMatch. |
| src/Http/Routing/src/Matching/RawTargetRouteValueDecoder.cs | New helper to extract/tokenize raw path from RawTarget and decode segments. |
| src/Http/Routing/src/Matching/DfaMatcher.cs | Updates capture/catch-all processing to decode captured values from RawTarget when available. |
| public bool TryMatch(PathString path, RouteValueDictionary values) | ||
| { | ||
| return TryMatch(path, values, httpContext: null); | ||
| } | ||
|
|
||
| private bool TryMatch(PathString path, RouteValueDictionary values, HttpContext httpContext) |
There was a problem hiding this comment.
RoutePatternMatcher is compiled into the Blazor Components build via Microsoft.AspNetCore.Components.Routing.targets, where COMPONENTS is defined. In that configuration there is no Microsoft.AspNetCore.Http.HttpContext, and Microsoft.AspNetCore.Routing.Matching.RawTargetRouteValueDecoder is not included, so introducing a HttpContext parameter and calling Matching.RawTargetRouteValueDecoder in the main TryMatch implementation will break the Components build. Consider keeping the original TryMatch(PathString, ...) implementation for COMPONENTS and wrapping the new RawTarget-based overload/helper + decoder usage in #if !COMPONENTS (or otherwise avoiding any HttpContext/Matching references when COMPONENTS is defined).
| public bool TryMatch(PathString path, RouteValueDictionary values) | |
| { | |
| return TryMatch(path, values, httpContext: null); | |
| } | |
| private bool TryMatch(PathString path, RouteValueDictionary values, HttpContext httpContext) | |
| public bool TryMatch(PathString path, RouteValueDictionary values) | |
| #if !COMPONENTS | |
| { | |
| return TryMatch(path, values, httpContext: null); | |
| } | |
| private bool TryMatch(PathString path, RouteValueDictionary values, HttpContext httpContext) | |
| #endif |
javiercn
left a comment
There was a problem hiding this comment.
I noticed this and wanted to add my take here.
I don't believe we should do this change without at the very least as an opt-in. I understand it's desirable to do things in one place, but this adds an extra path to routing (an already critical path) to decode the parameters and changes what appears inside the route values, which not only affects MVC, Minimal, Razor or Blazor, but also every middleware on the pipeline that is currently reading URL values.
I think the alternative proposal to handle this at the binding level is more scoped, provides a natural opt-in, avoids paying the cost on all requests, and impacting existing code.
One alternative to explore here is to extend routing with a new symbol (similar to how we do {*slug} and {**slug} to explicitly signal and handle this on the template as an opt-in, but I believe this is too aggressive to do in routing (it's the same reason why ** was created instead of changing *.
Decode route values from RawTarget (#11544)
Use RawTarget for route value capture and decoding
Description
This change fixes the partially decoded route value behavior described in #11544 by moving the decode logic into routing itself.
Routing still matches endpoints and evaluates constraints against
Request.Path, preserving existing precedence and selection behavior. Once a route is known to match, endpoint and legacy routing now useIHttpRequestFeature.RawTargetto recover the raw segment and decode the captured route value exactly once.This fixes MVC and minimal API route parameters without adding a new opt-in API, and includes regression tests covering
%2F,%252F, catch-all parameters, andPathBase.Fixes #11544