Fix HttpClient redirection logic on UAP #22702

davidsh · 2017-07-27T21:39:00Z

This PR changes the implementation of sending requests in HttpClient for UAP when
dealing with redirection. In the past, we would let the WinRT layer handle auto
redirection logic. However, due to #9003, the cookies were getting lost on 3xx responses
since the .NET layer didn't see them. So, this PR implements the redirection ourselves.

One important part of redirection is that we need to drop credentials. The WinRT layer
did this for us. However, we are unable to use a single WinRT HttpBaseProtocolFilter object
since we need to remove the credential after the first redirect. So, we need to maintain
a second filter that uses no credentials and keep it in sync regarding all the other properties
of the primary filter.

With this PR, the behavior of other aspects (such as controlling max number of redirects, etc.)
now matches .NET Framework. So, some tests were adjusted to remove the UAP specific behavior
checks.

Fixes #9003
Fixes #22191

This PR changes the implementation of sending requests in HttpClient for UAP when dealing with redirection. In the past, we would let the WinRT layer handle auto redirection logic. However, due to #9003, the cookies were getting lost on 3xx responses since the .NET layer didn't see them. So, this PR implements the redirection ourselves. One important part of redirection is that we need to drop credentials. The WinRT layer did this for us. However, we are unable to use a single WinRT HttpBaseProtocolFilter object since we need to remove the credential after the first redirect. So, we need to maintain a second filter that uses no credentials and keep it in sync regarding all the other properties of the primary filter. With this PR, the behavior of other aspects (such as controlling max number of redirects, etc.) now matches .NET Framework. So, some tests were adjusted to remove the UAP specific behavior checks. Fixes #9003 Fixes #22191

davidsh · 2017-07-27T21:42:08Z

@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot Test Outerloop Windows x64 Debug Build
@dotnet-bot Test Outerloop Linux x64 Release Build
@dotnet-bot Test Outerloop NETFX x86 Release Build

davidsh · 2017-07-27T22:55:18Z

@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot Test Outerloop Windows x64 Debug Build
@dotnet-bot Test Outerloop Linux x64 Release Build
@dotnet-bot Test Outerloop NETFX x86 Release Build

rmkerr · 2017-07-27T23:22:40Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+        internal HttpHandlerToFilter(
+            RTHttpBaseProtocolFilter filter,
+            RTHttpBaseProtocolFilter filterWithNoCredentials,
+            HttpClientHandler handler)
        {
            if (filter == null)


Since we're testing that filter is non-null here, should we also check filterWithNoCredentials? Or are they related enough that one being alright implies that the other is too?

Good point. But now that I am using Lazy pattern, there no longer is the filterWithNoCredentials in the handler object.

davidsh · 2017-07-28T00:24:31Z

@dotnet-bot Test Outerloop Windows x64 Debug Build
@dotnet-bot Test Outerloop Linux x64 Release Build

davidsh · 2017-07-28T01:38:24Z

Unrelated failures in Outerloop Linux:
https://ci3.dot.net/job/dotnet_corefx/job/master/job/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_true_prtest/27/

DavidGoll · 2017-07-28T16:22:59Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+
+            _allowAutoRedirect = true;
+            _maxAutomaticRedirections = 50;
+


Originally hard-coded to 10, with a comment about the WinRT implementation via wininet. Why not 10 here? #Closed

It is not 10 because it was only 10 due to using auto redirection with WinRT. And that 10 was not settable...it was hard coded within WinRT.

But now that we do it ourselves,, we can have the default be the same as the rest of .NET Framework, .NET Core. #Closed

DavidGoll · 2017-07-28T16:46:00Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+                {
+                    redirectUri = new Uri(string.Format("{0}://{1}:{2}{3}",
+                        request.RequestUri.Scheme,
+                        request.RequestUri.Host,


Host [](start = 43, length = 4)

If an IPv6 address, will this already have the square brackets?

We have no tests for this currently. But, if I change this logic as per @CIPop suggestion below:

In that case would the ctor Uri(Uri, string) work better?

then it becomes less of a concern.

I changed the code to remove the manually formatting of the uri. So, any IPv6 behavior would be done internally by the Uri ctor itself.

if (!redirectUri.IsAbsoluteUri) { redirectUri = new Uri(request.RequestUri, redirectUri.OriginalString); }

DavidGoll · 2017-07-28T17:02:26Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+                }
+
+                if (response.StatusCode != HttpStatusCode.RedirectKeepVerb &&
+                    requestHttpMethod == HttpMethod.Post)


Could you mention the RFC for why 307 is different? RFC 7231, section 6.4.7.

Added comment.

DavidGoll · 2017-07-28T17:45:10Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+                    requestHttpMethod == HttpMethod.Post)
+                {
+                    requestHttpMethod = HttpMethod.Get;
+                    skipRequestContentIfPresent = true;


Also, it would be nice to have a comment referencing the Post/Redirect/Get pattern since it's not obvious why a Post becomes a Get.

Added comment.

CIPop · 2017-07-28T19:02:18Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+                {
+                    if (redirects > 0)
+                    {
+                        rtResponse = await _filterWithNoCredentials.SendRequestAsync(rtRequest).AsTask(cancel).ConfigureAwait(false);


Could we create the filter right here as a clone of the original filter but leaving out the credentials (instead of keeping 2 filter objects just in case there is a redirection - which in most cases I believe is not true).

I suppose I could use a Lazy pattern here and only create the rtFilterWithNoCredentials as a "clone, minus credentials" when we first need it. At that point, since the first operation on the rtFilter has happened, the original rtFilter properties are considered locked. Therefore, we don't have to constantly re-create the rtFilterWithNoCredentials.

I thought it was simpler to just create it and update properties but perhaps I can revise that.

I pushed a new commit to use Lazy pattern and create a copy (minus creds) of the filter.

CIPop · 2017-07-28T19:03:15Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+
+                if (!redirectUri.IsAbsoluteUri)
+                {
+                    redirectUri = new Uri(string.Format("{0}://{1}:{2}{3}",


Use UriBuilder instead?

I tried using UriBuilder. It doesn't work well. It doesn't have the same properties. For example, there is a Uri.PathAndQuery but UriBuilder only has .Path and .Query. There were other problems as well with getting things to work properly. So, this logic ended up working and all tests pass.

In that case would the ctor Uri(Uri, string) work better?

I think I tried that and it didn't work. I can investigate again.

The problem is that the original URI is a full URI with path and possible querystring, fragment, etc. pieces. So, will this ctor actually "replace" the path and subsequent parts of the original URI with this relative URI piece?

Update: I checked again and I can see that the ctor overload works. I changed that code and tests still pass.

if (!redirectUri.IsAbsoluteUri) { redirectUri = new Uri(request.RequestUri, redirectUri.OriginalString); }

CIPop · 2017-07-28T19:12:26Z

src/System.Net.Http/src/uap/System/Net/HttpClientHandler.cs

+                    _rtFilterWithNoCredentials.IgnorableServerCertificateErrors.Add(RTChainValidationResult.RevocationInformationMissing);
+                    _rtFilterWithNoCredentials.IgnorableServerCertificateErrors.Add(RTChainValidationResult.Untrusted);
+                    _rtFilterWithNoCredentials.IgnorableServerCertificateErrors.Add(RTChainValidationResult.WrongUsage);
+                    _rtFilterWithNoCredentials.ServerCustomValidationRequested += RTServerCertificateCallback;


If for some reason both filters are active, this would cause the callback to be called several times causing race conditions that could corrupt the application state.

In general, that callback can be called several times even without redirection involved. For example, start multiple parallel requests to the same or different servers. There is only one callback defined per filter. So, it is already something that a developer needs to handle (i.e. handle multiple calls on the callback.

And this pattern is also the same for just the .NET layer itself. We have a TLS/SSL certificate validator callback defined per handler. So, multiple requests (at the same time perhaps) can happen against the single callback method.

CIPop · 2017-07-28T19:16:57Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+                if (redirectUri.Scheme != Uri.UriSchemeHttp &&
+                    redirectUri.Scheme != Uri.UriSchemeHttps)
+                {
+                    break;


Is http->https not allowed in .Net Framework?

This particular check makes sure that we don't try to redirect to a scheme like FTP or FILE or WS. So, the only possible destination schemes need to be HTTP or HTTPS.

CIPop · 2017-07-28T19:17:39Z

src/System.Net.Http/src/uap/System/Net/HttpHandlerToFilter.cs

+                if (request.RequestUri.Scheme == Uri.UriSchemeHttps &&
+                    redirectUri.Scheme == Uri.UriSchemeHttp)
+                {
+                    break;


Shouldn't this be a security exception so that the app understands that even if a redirect was requested, it was not honored because it would cause a https->http transition.

The current .NET Core behavior is not to throw an exception. But simply to not follow the redirect.

See this test that already is there:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs#L544

FWIW, the .NET Framework behavior allows HTTPS -> HTTP

davidsh · 2017-07-28T20:54:52Z

@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot Test Outerloop Windows x64 Debug Build
@dotnet-bot Test Outerloop Linux x64 Release Build
@dotnet-bot Test Outerloop NETFX x86 Release Build

davidsh · 2017-07-28T20:55:16Z

I pushed a new commit to address all outstanding feedback.

DavidGoll

LGTM

davidsh · 2017-07-28T21:41:30Z

Outerloop tests for .NET Core and UWP are failing on this test:
https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/16/

SendAsync_RequestVersion20_ResponseVersion20(server: https://www.microsoft.com/)

It looks like the www.microsoft.com server is not using HTTP/2.0 protocol today. Our tests assume that this server always uses HTTP/2 protocol. Don't know if this is temporary problem. We might need to switch to a different HTTP/2 test server. I also confirmed using the Chrome browser that www.microsoft.com is not using HTTP/2.

cc: @stephentoub @CIPop

davidsh · 2017-07-28T21:51:31Z

And FWIW, the HTTP/2 test failing on Windows doesn't run on Linux. There is another HTTP/2 test that does run on Linux but it allows HTTP/2 or HTTP/1.1 for a response.

davidsh · 2017-07-28T21:55:12Z

I added a new issue #22735 and will disable that test for now.

davidsh · 2017-07-28T22:00:39Z

@dotnet-bot Test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot Test Outerloop Windows x64 Debug Build
@dotnet-bot Test Outerloop Linux x64 Release Build
@dotnet-bot Test Outerloop NETFX x86 Release Build

davidsh · 2017-07-28T23:04:13Z

@dotnet-bot Test Outerloop NETFX x86 Release Build

davidsh · 2017-07-28T23:43:07Z

Unrelated CI failures in Linux and UWP
https://ci3.dot.net/job/dotnet_corefx/job/master/job/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_true_prtest/29/

https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_uap+CGroup_Debug+AGroup_x64+TestOuter_true_prtest/34/

System.IO.Tests.FileStream_LockUnlock

[release/uwp6.0] Fix HttpClient redirection logic on UAP (#22702)

* Address delayed feedback Address delayed feedback from PR #22702 and release/uwp6.0 port PR #22759 * Remove active issue on Uap TRACE test Uap platform doesn't support the 'TRACE' verb. After discussion with Wininet team, the method is explicitly not allowed. So, removing the activeissue and just skipping that part of the test. Fixes #22161

* Fix HttpClient redirection logic on UAP This PR changes the implementation of sending requests in HttpClient for UAP when dealing with redirection. In the past, we would let the WinRT layer handle auto redirection logic. However, due to dotnet/corefx#9003, the cookies were getting lost on 3xx responses since the .NET layer didn't see them. So, this PR implements the redirection ourselves. One important part of redirection is that we need to drop credentials. The WinRT layer did this for us. However, we are unable to use a single WinRT HttpBaseProtocolFilter object since we need to remove the credential after the first redirect. So, we need to maintain a second filter that uses no credentials and keep it in sync regarding all the other properties of the primary filter. With this PR, the behavior of other aspects (such as controlling max number of redirects, etc.) now matches .NET Framework. So, some tests were adjusted to remove the UAP specific behavior checks. Fixes dotnet/corefx#9003 Fixes dotnet/corefx#22191 * Disable test on Linux * Address PR feedback * Disable failing HTTP/2 test Commit migrated from dotnet/corefx@f2308f8

* Address delayed feedback Address delayed feedback from PR dotnet/corefx#22702 and release/uwp6.0 port PR dotnet/corefx#22759 * Remove active issue on Uap TRACE test Uap platform doesn't support the 'TRACE' verb. After discussion with Wininet team, the method is explicitly not allowed. So, removing the activeissue and just skipping that part of the test. Fixes dotnet/corefx#22161 Commit migrated from dotnet/corefx@f7bc8b4

davidsh added area-System.Net.Http os-windows-uwp labels Jul 27, 2017

davidsh self-assigned this Jul 27, 2017

davidsh requested review from stephentoub, rmkerr, caesar-chen, CIPop and DavidGoll July 27, 2017 21:39

dnfclas added the cla-already-signed label Jul 27, 2017

davidsh added this to the UWP6.0 milestone Jul 27, 2017

Disable test on Linux

ff3e895

rmkerr reviewed Jul 27, 2017

View reviewed changes

DavidGoll reviewed Jul 28, 2017

View reviewed changes

CIPop reviewed Jul 28, 2017

View reviewed changes

Address PR feedback

a1722c1

DavidGoll approved these changes Jul 28, 2017

View reviewed changes

CIPop approved these changes Jul 28, 2017

View reviewed changes

Disable failing HTTP/2 test

ae1353f

davidsh merged commit f2308f8 into dotnet:master Jul 28, 2017

davidsh deleted the redirect_uwp branch July 28, 2017 23:43

davidsh mentioned this pull request Jul 30, 2017

[release/uwp6.0] Fix HttpClient redirection logic on UAP (#22702) #22759

Merged

davidsh added a commit that referenced this pull request Jul 31, 2017

Merge pull request #22759 from davidsh/port_22702

344e822

[release/uwp6.0] Fix HttpClient redirection logic on UAP (#22702)

karelz modified the milestones: UWP6.0, 2.1.0 Aug 14, 2017

JunielKatarn mentioned this pull request Sep 10, 2022

Implement HTTP redirection microsoft/react-native-windows#10534

Merged

Fix HttpClient redirection logic on UAP #22702

Fix HttpClient redirection logic on UAP #22702

Conversation

davidsh commented Jul 27, 2017

davidsh commented Jul 27, 2017

davidsh commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh commented Jul 28, 2017

davidsh commented Jul 28, 2017

DavidGoll Jul 28, 2017 • edited

Choose a reason for hiding this comment

davidsh Jul 28, 2017 • edited by DavidGoll

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 28, 2017 • edited

Choose a reason for hiding this comment

davidsh Jul 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh Jul 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidsh commented Jul 28, 2017

davidsh commented Jul 28, 2017

DavidGoll left a comment

Choose a reason for hiding this comment

davidsh commented Jul 28, 2017 • edited

davidsh commented Jul 28, 2017

davidsh commented Jul 28, 2017

davidsh commented Jul 28, 2017

davidsh commented Jul 28, 2017

davidsh commented Jul 28, 2017

DavidGoll Jul 28, 2017 •

edited

davidsh Jul 28, 2017 •

edited by DavidGoll

davidsh Jul 28, 2017 •

edited

davidsh Jul 28, 2017 •

edited

davidsh Jul 28, 2017 •

edited

davidsh commented Jul 28, 2017 •

edited