Follow redirections to relative URLs - #2390

I'm getting mixed results with the new implementation:

            var uri1 = testuri("http://google.com/test?a=b#foo", "/asd");
            // -> http://google.com/asd#foo OK

            var uri2 = testuri("http://google.com/test?a=b#foo", "//google.de/asd");
            // -> http://google.de/asd#foo OK

            var uri3 = testuri("http://google.com/test?a=b#foo", "/asd?c=d#bar");
            // -> http://google.com/asd%3Fc=d%23bar#foo NOT OK

            var uri4 = testuri("http://google.com/test?a=b#foo", "//google.de/asd?c=d#bar");
            // -> http://google.de/asd%3Fc=d%23bar#foo NOT OK

Am I missing something? Do you get different results with these parameters?

Have you considered using the public Uri (Uri baseUri, string relativeUri); constructor (https://docs.microsoft.com/en-us/dotnet/api/system.uri.-ctor?view=netframework-4.7.2#System_Uri__ctor_System_Uri_System_String_) to do the heavy lifting here? It seems to handle all my examples correctly, apart from the result not having the fragment of the original request URI.

I have found this source code using the same method, for reference:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs#L111

@dominik-weber please review the latest changes, it should work correctly for your cases now. The only thing that will not work (because of a bug in the Uri class, well, I consider it a bug at least - see comments in the diff) is when Location redirects to URL of the a:b form, but this code is not where the issue can/should be solved.

@grendello awesome, thanks for the update! Actually, I think you can even remove the if (location [0] == '/') check alltogether. From the MS Docs:

If relativeUri is an absolute URI (containing a scheme, host name, and optionally a port number), the Uri instance is created using only relativeUri.

https://docs.microsoft.com/en-us/dotnet/api/system.uri.-ctor?view=netframework-4.7.2#System_Uri__ctor_System_Uri_System_String_

So just redirectUrl = new Uri (baseUrl, location); should work in all cases, no matter if location is relative or not. Apart from that, the changes look good to me 👍 I can however confirm your findings with a:b uris where a is just a single letter.

@grendello awesome, thanks for the update! Actually, I think you can even remove the if (location [0] == '/') check alltogether. From the MS Docs:

The check avoids having to deal with this:

csharp> var u = new Uri ("/stuff", UriKind.RelativeOrAbsolute)
csharp> u
file:///stuff

It's simply faster and cleaner to do it this way rather than to check for file scheme in the else case and then create the uri using the base location :)

If relativeUri is an absolute URI (containing a scheme, host name, and optionally a port number), the Uri instance is created using only relativeUri.

https://docs.microsoft.com/en-us/dotnet/api/system.uri.-ctor?view=netframework-4.7.2#System_Uri__ctor_System_Uri_System_String_

So just redirectUrl = new Uri (baseUrl, location); should work in all cases, no matter if location is relative or not. Apart from that, the changes look good to me +1 I can however confirm your findings with a:b uris where a is just a single letter.

Yeah, a:b is a bummer with no solution for now :(

The check avoids having to deal with this:

csharp> var u = new Uri ("/stuff", UriKind.RelativeOrAbsolute)
csharp> u
file:///stuff

It's simply faster and cleaner to do it this way rather than to check for file scheme in the else case and then create the uri using the base location :)

Maybe you misunderstood me there. I think if we used redirectUrl = new Uri (baseUrl, location); we wouldn't need to convert location to Uri at all, thus avoiding the file:// case alltogether. For example:

// baseUrl will always be a valid absolute url since we already sent a request to it
Uri baseUrl = new Uri("http://google.com/foo/bar");
Uri redirectUrl1 = new Uri(baseUrl, "/test"); // http://google.com/test
Uri redirectUrl2 = new Uri(baseUrl, "https://example.com"); // https://example.com

Seems like the Uri(baseUri, relativeUri) constructor was made just for http redirects... ;)