Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.NET8 routing with UrlEncode(url) in parameter does not work #53138

Closed
1 task done
TDroogers opened this issue Jan 4, 2024 · 16 comments
Closed
1 task done

.NET8 routing with UrlEncode(url) in parameter does not work #53138

TDroogers opened this issue Jan 4, 2024 · 16 comments
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug.
Milestone

Comments

@TDroogers
Copy link

TDroogers commented Jan 4, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

After updating from .NET7 (blazor server) to .NET8 the routing has broken.
On project A (still on .NET7) I forward to project B (.NET8)

var returnUrl = HttpUtility.UrlEncode("https://localhost:5001/authentication/login-callback");
Navigation.NavigateTo($"{url}/login/{returnUrl}");

returnUrl == https%3a%2f%2flocalhost%3a5001%2fauthentication%2flogin-callback

project B

@page "/login/{LoginUrl}"

url is not changed in the process https://localhost:44322/login/https%3a%2f%2flocalhost%3a5001%2fauthentication%2flogin-callback

However on project B I'm now forwarded to the 404 page.
If I change {returnUrl} to a non url like "text" I get on the page and the parameter gets the "text"

Expected Behavior

Should work like in .NET7
Encoded url should be read as a parameter and not forwarded to 404

Steps To Reproduce

See Describe the bug

Exceptions (if any)

No

.NET Version

8.0.100

Anything else?

Same issue in azure after deployment in release mode.

I have also tried adding some more optional paramaters, @page "/login/{LoginUrl}/{Test1?}/{Test2?}/{Test3?}" but I still got forwarded tot the 404

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Jan 4, 2024
@javiercn javiercn added the Needs: Repro Indicates that the team needs a repro project to continue the investigation on this issue label Jan 4, 2024
@ghost
Copy link

ghost commented Jan 4, 2024

Thank you for filing this issue. In order for us to investigate this issue, please provide a minimal repro project that illustrates the problem without unnecessary code. Please share with us in a public GitHub repo because we cannot open ZIP attachments, and don't include any confidential content.

@ghost ghost added the Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. label Jan 4, 2024
@TDroogers
Copy link
Author

@javiercn I created TDroogers/BugUrlRouting to reproduce this issue.

@ghost ghost added Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. and removed Needs: Author Feedback The author of this issue needs to respond in order for us to continue investigating this issue. labels Jan 4, 2024
@TDroogers
Copy link
Author

I have added a second project in .NET7 in the same solution to prove the issue is new in .NET 8

@TDroogers
Copy link
Author

I did some more investigation and the problem is that the encoded slash %2F is the problem. I have updated the reproduction project.

TDroogers/BugUrlRouting@e9ee2fb

My guess is that something goes wrong in the order of decoding the url and the decision what path to use.

@mtavares628
Copy link

I can also confirm that I have run into this same issue after upgrading my .NET 7 Blazor Server project to .NET 8.

@TDroogers

This comment was marked as outdated.

@javiercn
Copy link
Member

javiercn commented Jan 17, 2024

@TDroogers thanks for your patience.

I think I've tracked this down to this change https://github.com/dotnet/aspnetcore/blob/main/src/Components/Components/src/Routing/RouteContext.cs#L14 where we were previously splitting every segment and the URL decoding that and now we are URL decoding the entire path

The previous version of the code is https://github.com/dotnet/aspnetcore/blob/771532a3a31e8df2811a6201a2d3850a4171de15/src/Components/Components/src/Routing/RouteContext.cs

The key thing here is that the behavior needs to be the same between SSR (ASP.NET Routing) and whatever the Blazor router does. They share the same codebase, but it seems that some of the setup is slightly different.

Hopefully the behavior matches between the old implementation and the asp.net core implementation and we can "just" fix a bug. If the behavior is different, we'll likely need to go with the new behavior, as we aren't changing ASP.NET Core routing and both routing systems (interactive and SSR) need to offer the same results.

@javiercn
Copy link
Member

javiercn commented Jan 17, 2024

Removing the decoding step makes the scenario "work consistently" across all the implementations, but I suspect there's more work to do or else we will break other scenarios.

SSR: Click on link -> Url: https:%2f%2flocalhost:5001%2fauthentication%2flogin-callback
Interactive Server click on link -> Url: https%3a%2f%2flocalhost%3a5001%2fauthentication%2flogin-callback
Interactive Server click on button -> Url: https%3a%2f%2flocalhost%3a5001%2fauthentication%2flogin-callback

The last part/diff then seems to be that in 7.0, the parameter is decoded https://localhost:5001/authentication/login-callback. So likely in addition to not decoding the path on the route context, we need to add an extra step to decode the values after we've captured the parameters.

@TDroogers
Copy link
Author

Thanks for your response!

javiercn added a commit that referenced this issue Jan 22, 2024
This was discovered when working on the issue for #53138. The regex
constraint is not enabled by default on the blazor router.

As a result if a route uses a regex constraint, it will work on SSR but
will fail the moment the Blazor router tries to construct the route.

The fix enables the regex constraint on the blazor router,
unconditionally for server, and behind a feature flag for webassembly.

This is because enabling the regex constraint adds +80kb to the payload
due to the inclusion of the System.Text.RegularExpressions assembly.
@javiercn
Copy link
Member

#53470 This is now fixed in main and we'll probably backport it to 8.0 in a future patch

@mtavares628
Copy link

@javiercn Thanks for fixing this. Do you have an idea of when that 8.0 patch will be deployed? This bug is preventing me from upgrading my project to 8.0 because a lot of my navigation encodes a return url in the querystring.

javiercn added a commit that referenced this issue Jan 25, 2024
This was discovered when working on the issue for #53138. The regex
constraint is not enabled by default on the blazor router.

As a result if a route uses a regex constraint, it will work on SSR but
will fail the moment the Blazor router tries to construct the route.

The fix enables the regex constraint on the blazor router,
unconditionally for server, and behind a feature flag for webassembly.

This is because enabling the regex constraint adds +80kb to the payload
due to the inclusion of the System.Text.RegularExpressions assembly.
javiercn added a commit that referenced this issue Jan 25, 2024
This was discovered when working on the issue for #53138. The regex
constraint is not enabled by default on the blazor router.

As a result if a route uses a regex constraint, it will work on SSR but
will fail the moment the Blazor router tries to construct the route.

The fix enables the regex constraint on the blazor router,
unconditionally for server, and behind a feature flag for webassembly.

This is because enabling the regex constraint adds +80kb to the payload
due to the inclusion of the System.Text.RegularExpressions assembly.
javiercn added a commit that referenced this issue Jan 25, 2024
This was discovered when working on the issue for #53138. The regex
constraint is not enabled by default on the blazor router.

As a result if a route uses a regex constraint, it will work on SSR but
will fail the moment the Blazor router tries to construct the route.

The fix enables the regex constraint on the blazor router,
unconditionally for server, and behind a feature flag for webassembly.

This is because enabling the regex constraint adds +80kb to the payload
due to the inclusion of the System.Text.RegularExpressions assembly.
javiercn added a commit that referenced this issue Feb 1, 2024
This was discovered when working on the issue for #53138. The regex
constraint is not enabled by default on the blazor router.

As a result if a route uses a regex constraint, it will work on SSR but
will fail the moment the Blazor router tries to construct the route.

The fix enables the regex constraint on the blazor router,
unconditionally for server, and behind a feature flag for webassembly.

This is because enabling the regex constraint adds +80kb to the payload
due to the inclusion of the System.Text.RegularExpressions assembly.
@mkArtakMSFT
Copy link
Member

@javiercn Thanks for fixing this. Do you have an idea of when that 8.0 patch will be deployed? This bug is preventing me from upgrading my project to 8.0 because a lot of my navigation encodes a return url in the querystring.

@mtavares628 we're probably looking to including this as part of the 8.0.3 release.
In the meantime, it would help if you could try out our nightly builds to confirm if the fix (in main) has addressed the issue you were facing.

@mtavares628
Copy link

@mkArtakMSFT What are the steps for targeting the nightly build in my project? I downloaded and installed the latest nightly build of the SDK for .NET 9, but when I try to change the target on my project, .NET 9 isn't listed as an option. Is there something that I'm missing to get this running?

@mkArtakMSFT
Copy link
Member

Thanks for giving this a try, @mtavares628.
The guidance here includes information about the feeds to configure. Make sure you configure .NET 9 feeds. Basically, you create a nuget.config file with the content shown there and place it in the root of your project /solution.
Also make sure you update your project's .csproj file to have <TargetFramework>net9.0</TargetFramework>.

The option doesn't show up in the VS because most probably you are not using the latest VS preview which is required for the latest preview .NET SDKs.

@mtavares628
Copy link

Thanks @mkArtakMSFT, I was able to test it out and confirm that fix solves my problem. Is there a general time frame planned for the 8.0.3 release?

@mkArtakMSFT
Copy link
Member

Thanks for confirming this, @mtavares628. Most probably yes - 8.0.3. In the worst case - 8.0.4, but hopefully it won't get to that.

@mkArtakMSFT mkArtakMSFT added this to the 8.0.3 milestone Feb 9, 2024
@mkArtakMSFT mkArtakMSFT added bug This issue describes a behavior which is not expected - a bug. and removed Needs: Attention 👋 This issue needs the attention of a contributor, typically because the OP has provided an update. Needs: Repro Indicates that the team needs a repro project to continue the investigation on this issue labels Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug.
Projects
None yet
Development

No branches or pull requests

4 participants