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

Slashes incorrectly encoded when building URL with catchall param? #2669

Closed
aspnet-hello opened this issue Jan 2, 2018 · 51 comments
Closed
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed Needs: Design This issue requires design work before implementating.
Milestone

Comments

@aspnet-hello
Copy link

From @Daniel15 on Saturday, September 10, 2016 10:27:27 PM

Consider this controller:

public class TestController : Controller
{
  [Route("foo/{*path}")]
  public IActionResult Index(string path)
  {
    var url = Url.Action("Index", new { path = "hello/world" });
    return Content("Path = [" + path + "], built URL = " + url);
  }
}

When you hit http://example.com/foo/some/url, ASP.NET MVC Core will display:

Path = [some/url], built URL = /foo/hello%2Fworld

Whereas ASP.NET MVC 5 will display:

Path = [some/url], built URL = /foo/hello/world

Notice that the URL returned by Url.Action encodes / as %2F, whereas the previous MVC version did not do this.

Is this an expected change? If so, how do I render the URL such that the slash is not encoded?

Copied from original issue: aspnet/Routing#363

@aspnet-hello
Copy link
Author

From @SonicGD on Tuesday, November 29, 2016 10:55:46 AM

Have same problem right now. Any news on this? @Daniel15, did you bypass it somehow?

@aspnet-hello
Copy link
Author

From @Daniel15 on Tuesday, November 29, 2016 11:36:18 AM

I never got time to look into it, these routes on my site are still broken 😞

@aspnet-hello
Copy link
Author

From @SonicGD on Wednesday, November 30, 2016 9:21:40 AM

So i did some investigation... Character encoding happens in https://github.com/aspnet/Routing/blob/dev/src/Microsoft.AspNetCore.Routing/Internal/UriBuildingContext.cs#L100

and UrlEncoder always filters '/' char - https://github.com/dotnet/corefx/blob/master/src/System.Text.Encodings.Web/src/System/Text/Encodings/Web/UrlEncoder.cs#L125

I don't see any way to configure or disable this behavior.

Maybe @NTaylorMullen or @rynowak will help us?

@aspnet-hello
Copy link
Author

From @SonicGD on Wednesday, November 30, 2016 9:58:29 AM

Also, even if i generate correct url - i doesn't match route. ex:

[HttpGet("/{parentUrl}/files/{catPath}/{fileName}.html")

where catPath is path like 'video/others'.

Only workaround i found is to use catchall param and then parse it with regex. And it looks ugly 😢

@aspnet-hello
Copy link
Author

From @DeadSith on Sunday, December 11, 2016 9:02:58 AM

The only way to bypass it now is to set urls in a tags using href, not built-in tag helpers. It makes it harder to refactor code, but at least you get good-looking urls.

@aspnet-hello
Copy link
Author

From @Daniel15 on Saturday, April 29, 2017 11:01:09 PM

This is still broken :(

I worked around it by using Url.Action then calling .Replace("%2F", "/") on the resulting string. Ugly but it works: Daniel15/Website@26b1d2a

@aspnet-hello
Copy link
Author

From @DamianEdwards on Tuesday, May 2, 2017 10:52:01 AM

@rynowak @Eilon @danroth27 comments?

@aspnet-hello
Copy link
Author

From @Eilon on Tuesday, May 2, 2017 4:47:21 PM

We'll see if we can take another look at this. The problem, if I recall, isn't so much with how to encode slashes, but rather what to do when they come back in. I believe that servers/reverse-proxies such as IIS (and no doubt others) will decode the slashes on the way in, so you don't get round-tripping of data. We spent a lot of time back in MVC 1.x-5.x trying to make this work and never could. It's possible that today things are different.

@aspnet-hello
Copy link
Author

From @Daniel15 on Tuesday, May 2, 2017 11:47:40 PM

I believe that servers/reverse-proxies such as IIS (and no doubt others) will decode the slashes on the way in

Why would the slashes be encoded in the first place? If I have a route like foo/{*path}, hitting /foo/bar/baz is a valid path. I never had issues with that in older ASP.NET MVC versions, other than IIS6 which is not really a problem these days.

@aspnet-hello
Copy link
Author

From @Eilon on Wednesday, May 3, 2017 9:24:07 AM

I believe that an incoming URL in the form http://localhost/foo/bar is different from http://localhost/foo%2fbar. The first URL has a path with two segments: foo and bar. The second URL has a path with one segment: foo/bar (once it's decoded).

@aspnet-hello
Copy link
Author

From @kaa on Wednesday, May 3, 2017 9:45:29 AM

Arguably, if it was a "normal" path segment, but this is specifically a "catch all" path segment which by its nature is going to contain slashes in unencoded form. For a normal path segment the current implementation is the expected behavior of course.

Currently there is no way to create an url using the UrlHelper that fits your first form, as far as I understand. This is what we're asking for.

@aspnet-hello
Copy link
Author

From @Daniel15 on Wednesday, May 3, 2017 10:15:13 AM

Yeah, @Eilon I should have clarified that this is a catchall segment, so it should capture everything including slashes.

The actual routing is working fine, it's just the URL builder that's incorrectly HTML encoding the slashes.

@doggy8088
Copy link
Contributor

@DamianEdwards @Daniel15 I think I met the same issue in the Azure Function Proxy too.. Check this out: Azure/azure-functions-host#2249

@mkArtakMSFT
Copy link
Member

@danroth27, any thoughts regarding this?

@mkArtakMSFT
Copy link
Member

@javiercn, can you please investigate what options we can have here? Just come up with some proposal about this and let's discuss it with the team.
//cc: @Tratcher

@javiercn
Copy link
Member

javiercn commented Feb 2, 2018

I spoke about this with @rynowak a while ago and I spoke to @Tratcher too.

There are some security implications with not completely escaping the path as we need to deal with things like "/Path/../../../" but Kestrel and IIS take care of those things on the inbound path.

We can special case the way we encode a catchAll parameter when we build the url by just creating a new instance of PathString (calling the constructor) and then calling ToString() on the PathString to produce the properly encoded value.

The fix will probably happen somewhere around here
https://github.com/aspnet/Routing/blob/dev/src/Microsoft.AspNetCore.Routing/Template/TemplateBinder.cs#L228

@javiercn
Copy link
Member

javiercn commented Feb 3, 2018

This would be a breaking change, so we need to think about if we are willing to take it.
@mkArtakMSFT I'm sending this back to triage.

@juho-hanhimaki
Copy link
Contributor

I run into this issue in API projects quite often and the workaround is pain in the ass. So my vote to fix this in future.

@doggy8088
Copy link
Contributor

@javiercn May I ask why this issue would be a breaking change?

@javiercn
Copy link
Member

javiercn commented Feb 4, 2018

@doggy8088 Because the '/' are getting encoded right now into '%2F' and they won't be if we do this change, and that has the potential to break people's apps depending on how their webservers are configured.

@doggy8088
Copy link
Contributor

@javiercn It’s obviously a bug on encoding when using catchall routing. I think there is nobody will relying on this feature and the workaround right now is not affecting if you patched this bug. If user’s backend server is using IIS, no matter what they apply this patch no not, they won’t be affected either. If user use Apache Web server, there is same things. No one will be affected. So I don’t think it’s a breaking change.

@rynowak
Copy link
Member

rynowak commented Mar 30, 2018

The plan we discussed was to move to 2.2.0

@Eilon
Copy link
Member

Eilon commented Mar 30, 2018

Ah right. @mkArtakMSFT let's move to 2.2.0 and un-assign.

@mkArtakMSFT mkArtakMSFT modified the milestones: 2.1.0-preview2, 2.2.0 Mar 30, 2018
@mkArtakMSFT
Copy link
Member

mkArtakMSFT commented Mar 30, 2018

We've decided to add a new feature to support this scenario in 2.2.0
We will add a new syntax {**catchAll} that will parse catchAll segments as PathString instances.
Users will be able to generate urls without encoded slashshes in catch all values by passing a PathString as the value (or if its on the ambient values)

@develax
Copy link

develax commented May 9, 2018

The same trouble with RedirectToRoute & RedirectToAction methods.
This
return RedirectToRoute(RouteNames.HomeRoute, new { path = "xxx/yyy" });
generates url with "xxx%2fyyy".

Is there a workaround for it?

@develax
Copy link

develax commented May 10, 2018

I couldn't find any easy workaround so I wrote a custom MyRedirectResult class for this purpose:
https://gist.github.com/DevelAx/f873f1beaf86b1f31ee8bb679762f088

@doggy8088
Copy link
Contributor

doggy8088 commented May 11, 2018

@develax In your sample, I think you don't have to implement a MyRedirectResult. It can simply use return Redirect(URL); to do the same thing.

@develax
Copy link

develax commented May 11, 2018

@doggy8088 return Redirect(URL) converts "xxx/yyy" to "xxx%2fyyy" therefore it doesn't work for catchall paths. Also, it requires for non-ASCII characters to be encoded, in MyRedirectResult it's done automatically.

@doggy8088
Copy link
Contributor

@develax The RedirectResult has many method signatures, for passing "URL as a string" in the first parameter shouldn't URLEncode the url at all.

@develax
Copy link

develax commented May 11, 2018

@doggy8088 If you try to pass a non-ASCII string to default RedirectResult, for example, "我的网站", you'll likely get the exception:
System.InvalidOperationException: Invalid non-ASCII or control character in header. But this is not the major problem: you can't redirect to a URL like "xxx/yyy" because default RedirectResult converts all slashes to %2F, so the URL will be broken as "xxx%2fyyy", that's why I created MyRedirectResult.

@doggy8088
Copy link
Contributor

@develax I mean this one below. Passing "xxx/yyy" in the default RedirectResult won't converts all slashes to %2F, the URL won't broken.

image

@develax
Copy link

develax commented May 11, 2018

@doggy8088 guess what... you're right, it's my mistake. So, it can be useful only for the first case in order not to do encoding for each redirect, example:

string url = UrlEncoder.Default.Encode("/我的网站/About").Replace("%2F", "/", StringComparison.OrdinalIgnoreCase);
return Redirect(url);

But this case can be considered a bit offtopic for this thread.

@mkArtakMSFT
Copy link
Member

@JamesNK, @rynowak, @kichalla please consider this as part of the Dispatcher work:

We've decided to add a new feature to support this scenario in 2.2.0
We will add a new syntax {**catchAll} that will parse catchAll segments as PathString instances.
Users will be able to generate urls without encoded slashshes in catch all values by passing a PathString as the value (or if its on the ambient values)

@mkArtakMSFT
Copy link
Member

Closing this as done!

@mkArtakMSFT mkArtakMSFT added the Done This issue has been fixed label Oct 22, 2018
@Eilon Eilon added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed repo:Routing labels Nov 7, 2018
@russgove
Copy link

Hey, so how do i call an azure function that has a url segment with slashes from typescript?

I have my function signature set like this:
public static async Task GetRoleToTransactionsForRoleName(
[HttpTrigger(AuthorizationLevel.Function, "get", Route = "RoleToTransactions/{**RoleName}")]HttpRequestMessage req,
TraceWriter log,
string RoleName)
{

still getting errors

@doominator42
Copy link

I can't get this to work. My route is /somepath/{**name} and here is how i'm trying to generate the url:

// name = "x/y"
Url.Action("ActionName", "ControllerName", new { name }) 
// results with "/somepath/x%2Fy"

also tried

// name = "/x/y"
Url.Action("ActionName", "ControllerName", new { name = new PathString(name) }) 
// results with "/somepath/%2Fx%2Fy"

Anyone has managed to make it work and can help me?

@Eilon
Copy link
Member

Eilon commented Dec 21, 2018

Hi, it looks like you are posting on a closed issue/PR/commit!

We're very likely to lose track of your bug/feedback/question unless you:

  1. Open a new issue
  2. Explain very clearly what you need help with
  3. If you think you have found a bug, include detailed repro steps so that we can investigate the problem.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed Needs: Design This issue requires design work before implementating.
Projects
None yet
Development

No branches or pull requests