Redirect should encode URLs if it needs to

[miloush]

All these issues are result of people trying to redirect to a URL that is not all ASCII. Resolution seems to be "you should have encoded your URL".

#2678
aspnet/Mvc#7701
aspnet/KestrelHttpServer#2508
aspnet/KestrelHttpServer#2296
aspnet/Security#1646
aspnet/HttpAbstractions#971
aspnet/Routing#513
aspnet/Mvc#7529
aspnet/HttpAbstractions#907
aspnet/Mvc#6609
#2106
madskristensen/Miniblog.Core#87
#37337 (unicode host)

However, the fact that the RedirectResult puts the URL into a HTTP header (which is the ultimate source of the issue) is an implementation detail that the caller cannot magically assume. It is therefore responsibility of e.g. RedirectResultExecutor to meet the requirements and ensure the URL is properly formatted in case it decides to use HTTP headers for redirection.

[mkArtakMSFT]

Thanks for contacting us, @miloush.
@Tratcher, what are your thoughts regarding this?

[Tratcher]

We have helpers for generating properly encoded urls like UriHelper.BuildAbsolute. By the time you have combine everything into one string it's too late automatically encode, some parts are ambiguous like ? and # if present in the path.

Perhaps overhauling RedirectResult to take in PathString, QueryString, etc. rather than a full string?

[miloush]

Please note there are two ideas of "encoding" and I agree standard URL encode is too late for Redirect to do.

Perhaps overhauling RedirectResult to take in PathString, QueryString, etc. rather than a full string?

That wouldn't help much as you will face the same problem just with individual parts. The issue is you don't want a typical UrlEncode, you just want to fast-check and encode non-ASCII chars.

We have helpers for generating properly encoded urls

I am not disputing that, that is basically "you should have encoded your URL". But you can have Unicode URLs on the page itself (which is often preferred), it's only limitation of the Location HTTP header value used for redirecting that ASP.NET Core is enforcing, and the code that decides to use this mechanism should take care of that requirement.

Unless Unicode is allowed in other HTTP headers, this is a HTTP problem, not URL one. Perhaps FrameResponseHeaders.SetValueFast is the place where this should be happening rather than throwing.

[Tratcher]

Right, we need to be specific about the kind of encodings. To create an unambiguous url there are several syntax characters you may need to escape like /?#:%.& etc. since they affected the meaning of the individual parts. Then you may need to encode it for transport / embedding in your specific, especially unicode, but # and other symbols can also be problematic in certain context like HTML.

PathString and company are specifically designed to encode URLs for use in HTTP headers. They take care of both the syntax escaping when you combine components, as well as fully encoding any unicode. It's important to do both of these at the same time so you don't double encode anything.

Unicode has a long messy relationship with HTTP headers and it's best to be explicit about the encoding.

[Tratcher]

I've filed #3574 for a Response.Redirect overload that accepts the components.

[miloush]

Okay so nowadays when one wants to fix this issue, they need to create a special ASCII encoded URL to pass to the Redirect method, and use the Unicode URL in links and on pages, e.g. madskristensen/Miniblog.Core@14ec33b

Imagine you add the suggested overload to Response.Redirect that takes the components, how would that eliminate the need for such fix?

[Tratcher]

UriHelper.GetDisplayUrl is what I refer people to when they want the friendly display for a request url. The signature doesn't work well for self-generated urls (it takes an HttpRequest), but that's another case where adding an overload that takes in the same components and renders them in a user displayable format would make sense. Note that one doesn't do the syntax escaping, you'd only use it for the display text of a link and use the encoded version for the actual link.

[miloush]

Okay so for self-generated urls, you would recommend a sort of GetLink() method that returns (scheme, HostString, PathString, QueryString, FragmentString) and then feeding it to either Redirect or GetDisplayUrl? I am worried that just sticking to a string instead is much more convenient...

[Tratcher]

Understood on the convenience of a string. The trouble is that it doesn't provide any context, the user has to know if it's encoded or decoded and handle it accordingly. I'm starting to think we need a url structure that aggregates all of the components for easy flow. (Not System.Uri because that causes quite a few transformations and doesn't encode appropriately for HTTP headers or HTML pages).

[Tratcher]

@mkArtakMSFT Looking at RedirectResult/Executor it's a 1:1 mapping to Response.Redirect. @miloush is certainly right on one main point, these API do not tell you what to do nor help you do it successfully. The doc comments make no reference to any encoding requirements. I will dispute the bit about putting the value into a HTTP header being an implementation detail, that's the core mechanic for HTTP redirects, this API is only sugar around that. Unfortunately it's not enough sugar. Changing the API to take in PathString, QueryString, etc. or a higher level construct would give users much more guidance here.

[mkArtakMSFT]

Thanks @Tratcher.
@javiercn, can you please handle the proposed change? Thanks!

[javiercn]

I understand the concerns here. As far as I understand this is just a limitation for the location header, on other places (other than HTML encoding) is totally valid to have non ASCII characters on the Uri.

That said, I'm not sure adding new APIs will do anything to solve this problem. Most people will simply keep calling the string based API (as is the most convenient one) to later realize that the Url is not properly encoded.

When that happens, I don't really see an advantage in adding one overload for each of the redirect methods that takes all the individual components as HostString, PathString, etc compared to using UriHelper before calling on the redirect methods as suggested above.

I think the main thing we can change here is to make generating correctly encoded urls in MVC easier. This is something that we can add to UrlHelper. That way, we don't needs these overloads and this just becomes or something similar.

return Redirect(UrlHelper.CreateUrl(host,port,base,path,query,fragment));