Fixes encoded values on Blazor Router #53470
Conversation
dotnet-issue-labeler
bot
added
the
area-blazor
javiercn
force-pushed
the
javiercn/issue53138
branch
3 times, most recently
from
a67e10b to
ace334c
Compare
javiercn
force-pushed
the
javiercn/issue53138
branch
from
ace334c to
a12be23
Compare
| @@ -112,9 +113,11 @@ private static string[] GetTemplates(Type componentType) | |||
| [UnconditionalSuppressMessage("Trimming", "IL2067", Justification = "Application code does not get trimmed, and the framework does not define routable components.")] | |||
| internal static RouteTable Create(Dictionary<Type, string[]> templatesByHandler, IServiceProvider serviceProvider) | |||
| { | |||
| var routeOptions = Options.Create(new RouteOptions()); | |||
| routeOptions.Value.SetParameterPolicy("regex", typeof(RegexInlineRouteConstraint)); | |||
There was a problem hiding this comment.
I'm unsure if this is part of dealing with encoding or if it's addressing a separate issue. Could you clarify?
There was a problem hiding this comment.
This is addressing a separate issue, we weren't wiring up the regex, which was needed to test this fully, as it doesn't affect other constraints that the Blazor router supports and we don't have support for authoring custom constraints either.
There was a problem hiding this comment.
Have we effectively added a whole new feature with this line of code then? I'm totally fine with this being added, assuming it doesn't appreciably impact the default bundle size for a Blazor WebAssembly app.
Could you confirm if that's the case?
There was a problem hiding this comment.
I'm in the process of confirming the bundle size, it's not a feature, it's a bug fix IMO. It works on SSR routing but won't work once it becomes interactive.
There was a problem hiding this comment.
Thanks, please do confirm! I think we still have a choice of whether to include that feature for interactive mode, given that it's never worked on interactive mode in the past.
There was a problem hiding this comment.
It turns out that this regressed the size, so I'm going to take it out into its own separate PR that enables this conditionally on WebAssembly behind a feature flag. The feature flag will also need a change in the Blazor SDK to control it, but for the purposes of this repo, we'll keep it enabled.
There was a problem hiding this comment.
I've taken out the regex bit and will put it on a separate commit.
...testassets/Components.TestServer/RazorComponents/Pages/Routing/Encoded/EncodedCatchAll.razor
Show resolved
Hide resolved
| @@ -0,0 +1,24 @@ | |||
| @page "/🙂/routing/constrained-catch-all/{*parameter:regex(http:%2F%2Fwww\\.example\\.com%2Flogin%2Fcallback\\/another)}" | |||
There was a problem hiding this comment.
Yeah, this is a bit extreme, but I wanted to make sure we cover all the edge cases correctly, as this is very tricky.
As part of the routing unification process we switched the way we were decoding the URL prior to feeding it to routing and that introduced a small regression in interactive routing compared to .NET 7.0. This commit fixes that regression by using the same logic for decoding the URL in the client that is used on the server. In addition to that, the Blazor router now post processes the URL to replace instances of `%2F` with `/` when providing values to maintain the behavior in 7.0 where it used UnescapeDataString. This also makes the routing on the server and on the client consistent in their handling of encoded `/` characters.
javiercn
force-pushed
the
javiercn/issue53138
branch
from
a12be23 to
11b7c57
Compare
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/7616006018 |
|
@javiercn backporting to release/8.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: [Blazor] Fixed encoded values on Blazor Router
Using index info to reconstruct a base tree...
M src/Components/Components/src/Microsoft.AspNetCore.Components.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/Components/Components/src/Microsoft.AspNetCore.Components.csproj
CONFLICT (content): Merge conflict in src/Components/Components/src/Microsoft.AspNetCore.Components.csproj
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 [Blazor] Fixed encoded values on Blazor Router
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@javiercn an error occurred while backporting to release/8.0, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
As part of the routing unification process we switched the way we were decoding the URL prior to feeding it to routing and that introduced a small regression in interactive routing compared to .NET 7.0. This commit fixes that regression by using the same logic for decoding the URL in the client that is used on the server. In addition to that, the Blazor router now post processes the URL to replace instances of `%2F` with `/` when providing values to maintain the behavior in 7.0 where it used UnescapeDataString. This also makes the routing on the server and on the client consistent in their handling of encoded `/` characters.
As part of the routing unification process we switched the way we were decoding the URL prior to feeding it to routing and that introduced a small regression in interactive routing compared to .NET 7.0. This commit fixes that regression by using the same logic for decoding the URL in the client that is used on the server. In addition to that, the Blazor router now post processes the URL to replace instances of `%2F` with `/` when providing values to maintain the behavior in 7.0 where it used UnescapeDataString. This also makes the routing on the server and on the client consistent in their handling of encoded `/` characters.
Fix handling of encoded
/in URLs within the Blazor router.Fixes #52808, #53138, #53262
In .NET 8.0 we unified the routing system for ASP.NET Core and Blazor in order to bridge the functionality gaps and provide a consistent experience for SSR and interactive routing.
As part of such, we had to make adjustments to account for some of the quirks of the Blazor router (where it would behave slightly different than the ASP.NET Core router).
As part of this, we were decoding the entire URL before passing it to the Blazor router instead of (as it did before), splitting each segment and decoding each segment individually.
This introduced a regression when using URLs that contained encoded segments with
/in them, as the router would then treat the/as a path separator instead of part of the segment and we would not be able to route correctly.To fix this, we now decode each segment individually inside the router when the code runs as part of Blazor. In addition to that, when the routing happens server side, we now post-process the resulting values to URL decode them so that they match the behavior of the Blazor router.
To summarize, here is the before and after behavior:
Before:
/some/{parameter}/some/encoded%2Fvalueparameter = "encoded/value"/some/{parameter}/some/encoded%2Fvalueparameter = "encoded%2Fvalue"/some/{parameter}/some/encoded%2FvalueAfter:
/some/{parameter}/some/encoded%2Fvalueparameter = "encoded/value"/some/{parameter}/some/encoded%2Fvalueparameter = "encoded/value"/some/{parameter}/some/encoded%2Fvalueparameter = "encoded/value"