Add support and tests for HTTP 308 Permanent Redirect by rmkerr · Pull Request #30398 · dotnet/corefx

rmkerr · 2018-06-14T17:50:04Z

The HttpStatusCode enum was only recently updated to include HTTP status code 308, via PR #26727. With the platform handlers we support whatever cURL and WinHttp support, so 308 works even though the status code did not exist in the enum until recently. However in SocketsHttpHandler we only support the redirects that were in the enum when that code was written, which at the time did not include 308.

This PR adds support for 308 redirects to SocketsHttpHandler, and enables 308 redirects in our tests. It is not well documented when WinHttp and cURL added support for HTTP 308, so it is possible these tests will fail on older operating systems. If so I believe we should still take the change, but should add documentation on our support on older systems.

Fixes: #30389

rmkerr · 2018-06-14T17:50:41Z

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

stephentoub · 2018-06-14T17:56:16Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

+
+            new object[] { 308, "GET", "GET" },
+            new object[] { 308, "POST", "POST" },
+            new object[] { 308, "HEAD", "HEAD" },


I'll be interested to see if this passes on NetfxHandler and CurlHandler.

And UapHandler, since:

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

Lines 115 to 122 in 015ee21

if (response.StatusCode != HttpStatusCode.MultipleChoices &&

response.StatusCode != HttpStatusCode.MovedPermanently &&

response.StatusCode != HttpStatusCode.Redirect &&

response.StatusCode != HttpStatusCode.RedirectMethod &&

response.StatusCode != HttpStatusCode.RedirectKeepVerb)

{

break;

}

Agreed. I think it's possible that it will also fail in WinHttpHandler on win7. If so though I think we should keep the change and disable the test on those platforms. I'll extend the fix to UAP though, since we can control the behavior there.

stephentoub · 2018-06-14T17:59:03Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs

                case HttpStatusCode.SeeOther:
                case HttpStatusCode.TemporaryRedirect:
                case HttpStatusCode.MultipleChoices:
+                case HttpStatusCode.PermanentRedirect:


What about

corefx/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/HttpHandlerDiagnosticListener.cs

Lines 652 to 667 in 015ee21

private bool IsLastResponse(HttpWebRequest request, HttpStatusCode statusCode)

{

if (request.AllowAutoRedirect)

{

if (statusCode == HttpStatusCode.Ambiguous || // 300

statusCode == HttpStatusCode.Moved || // 301

statusCode == HttpStatusCode.Redirect || // 302

statusCode == HttpStatusCode.RedirectMethod || // 303

statusCode == HttpStatusCode.RedirectKeepVerb) // 307

{

return s_autoRedirectsAccessor(request) >= request.MaximumAutomaticRedirections;

}

}

return true;

}

?
cc: @vancem

This will probably need to be updated too. It looks like the test for MaxAutomaticRedirections only uses 302, so I'll add an additional test for that case.

corefx/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

Lines 1083 to 1132 in 909dc9e

public async Task GetAsync_MaxAutomaticRedirectionsNServerHops_ThrowsIfTooMany(int maxHops, int hops)

{

if (IsWinHttpHandler && !PlatformDetection.IsWindows10Version1703OrGreater)

{

// Skip this test if using WinHttpHandler but on a release prior to Windows 10 Creators Update.

_output.WriteLine("Skipping test due to Windows 10 version prior to Version 1703.");

return;

}

else if (PlatformDetection.IsFullFramework)

{

// Skip this test if running on .NET Framework. Exceeding max redirections will not throw

// exception. Instead, it simply returns the 3xx response.

_output.WriteLine("Skipping test on .NET Framework due to behavior difference.");

return;

}

HttpClientHandler handler = CreateHttpClientHandler();

handler.MaxAutomaticRedirections = maxHops;

using (var client = new HttpClient(handler))

{

Task<HttpResponseMessage> t = client.GetAsync(Configuration.Http.RedirectUriForDestinationUri(

secure: false,

statusCode: 302,

destinationUri: Configuration.Http.RemoteEchoServer,

hops: hops));

if (hops <= maxHops)

{

using (HttpResponseMessage response = await t)

{

Assert.Equal(HttpStatusCode.OK, response.StatusCode);

Assert.Equal(Configuration.Http.RemoteEchoServer, response.RequestMessage.RequestUri);

}

}

else

{

if (UseSocketsHttpHandler)

{

using (HttpResponseMessage response = await t)

{

Assert.Equal(HttpStatusCode.Redirect, response.StatusCode);

}

}

else

{

await Assert.ThrowsAsync<HttpRequestException>(() => t);

}

}

}

}

@stephentoub I added the new redirect code here, but it is causing build failures on netfx. I can use the integer 308 instead, or do you think I should take a different approach?

Using "308" might still fail on NETFX since it might not be supported. You can try though using the integer. But if the test fails, then you'll need to skip that test case on NETFX.

NETFX does NOT support 308 redirect code:

https://github.com/Microsoft/referencesource/blob/3b1eaf5203992df69de44c783a3eda37d3d4cd10/System/net/System/Net/HttpWebRequest.cs#L5688-L5706

davidsh · 2018-06-14T18:09:17Z

@dotnet-bot Test Outerloop OSX x64 Debug Build

…er and UWP.

rmkerr · 2018-06-14T19:46:39Z

It looks like the redirect tests that are using the remote echo server are failing with a 500 error:

Message :
Assert.Equal() Failure
Expected: OK
Actual:   InternalServerError

Stack Trace :
   at System.Net.Http.Functional.Tests.HttpClientHandlerTest.GetAsync_AllowAutoRedirectTrue_RedirectFromHttpToHttp_StatusCodeOK(Int32 statusCode) in D:\j\workspace\windows-TGrou---db113413\src\System.Net.Http\tests\FunctionalTests\HttpClientHandlerTest.cs:line 979
--- End of stack trace from previous location where exception was thrown ---

I'm going to take a look at the remote echo server code to see if it is causing these failures.

davidsh · 2018-06-14T19:47:00Z

src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs

        }

+        [SkipOnTargetFramework(TargetFrameworkMonikers.Uap, "Not currently supported on UAP")]
+        [OuterLoop] // TODO: Issue #11345


We're trying to remove the obsoleted TODO 11345.

Instead this should be "[Outerloop("Use external server")]"

davidsh · 2018-06-14T19:52:11Z

The remote echo server will need to updated:

                    if (statusCode < 300 || statusCode > 307)
                    {
                        context.Response.StatusCode = 500;
                        context.Response.StatusDescription = "Invalid redirect statuscode: " + statusCodeString;
                        return;
                    }

I will handle that.

And the source code for it in CoreFx is not actually up to date with the real deployed code. That is done from a different repo.

rmkerr · 2018-06-14T20:10:43Z

@davidsh Thanks for taking care of it. For future reference, where is that repo?

davidsh · 2018-06-14T20:15:30Z

@davidsh Thanks for taking care of it. For future reference, where is that repo?

Let's discuss offline in team meeting. To update the service, it's more complex than just knowing where the repo is.

davidsh · 2018-06-14T20:15:57Z

Updated corefx-net.cloudapp.net/redirect.ashx endpoint to support 308 status code:

rmkerr · 2018-06-14T20:20:20Z

Thanks! I'll rerun the tests.

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop OSX x64 Debug Build

rmkerr · 2018-06-15T17:11:07Z

I think the test failures here are a combination of three issues:

It looks like 308 redirects are not supported in WinHttp versions before Windows 10. This causes platform handler failures on win7 and win8. I'll just disable these on the PlatformHandler for anything older than win10.

The remote echo server only uses the specified redirect status code on the first redirect. Subsequent redirects drop the status code, and always use 302:

corefx/src/Common/tests/System/Net/Prerequisites/Servers/CoreFxNetCloudService/WebServer/Redirect.ashx.cs

Lines 69 to 78 in bffef76

    
           else 
        
           { 
        
               context.Response.Headers.Add( 
        
                   "Location", 
        
                   string.Format("/Redirect.ashx?uri={0}&hops={1}", 
        
                   redirectUri, 
        
                   hops - 1)); 
        
           } 
        
           context.Response.StatusCode = statusCode;

I'm not sure what the more realistic behavior is, but it does break the test I added. I can adjust the test if this is the desired behavior. @davidsh do you have any thoughts here?

Failures on netfx due to the usage of the new status code that does not exist yet in .NET Framework. I'll adjust this to use the integer constant 308 instead, as seems to be standard for those tests.

davidsh · 2018-06-15T18:17:33Z

I'm not sure what the more realistic behavior is, but it does break the test I added. I can adjust the test if this is the desired behavior. @davidsh do you have any thoughts here?

Having multiple hops was only about testing the "limits" we set for MaxAutomaticRedirection in the handlers. It wasn't about whether it was "realistic" behavior of clients, i.e. browsers.

I would rather we don't touch the current behavior of the redirect.ashx endpoint for this. And we should fix/remove the tests as needed. I don't think it is necessary to have multi-hop tests for all redirect status codes.

rmkerr · 2018-06-15T18:43:57Z

@davidsh Okay, in that case I'll just remove that test. We have a multi-hop test for 302 and single hop tests for all other status codes, so that should cover it.

davidsh

LGTM. You'll also want to re-run all Outerloop tests again since you've revised the PR.

rmkerr · 2018-06-19T16:11:51Z

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop OSX x64 Debug Build

rmkerr · 2018-07-09T18:14:54Z

Rerunning the tests since this has been inactive for almost a month. If the tests look good I'll go ahead and merge this.

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop OSX x64 Debug Build

rmkerr · 2018-07-10T17:17:49Z

I'm going to go ahead and merge this change. The Linux Outerloop failure was in System.Drawing, and is unrelated. The OSX failure is a known flaky HTTP test that does not use redirection.

This change adds support for 308 redirects to SocketsHttpHandler, and enables 308 redirects in our tests. Fixes: #30389

…30398) This change adds support for 308 redirects to SocketsHttpHandler, and enables 308 redirects in our tests. Fixes: dotnet/corefx#30389 Commit migrated from dotnet/corefx@12e41ed

Add 308 redirect support & tests.

8607df7

rmkerr added bug Product bug (most likely) area-System.Net.Http.SocketsHttpHandler labels Jun 14, 2018

rmkerr self-assigned this Jun 14, 2018

rmkerr requested review from a team and stephentoub June 14, 2018 17:50

stephentoub reviewed Jun 14, 2018

View reviewed changes

Add test for too many redirects, add 308 support to diagnostics handl…

0bcca18

…er and UWP.

davidsh reviewed Jun 14, 2018

View reviewed changes

Fix syntax issue, update outerloop tag.

33fb37d

Fix capitalization of outerloop attribute.

858cbd0

Skip 308 tests on unsupported platforms.

82128e1

davidsh approved these changes Jun 19, 2018

View reviewed changes

rmkerr merged commit 12e41ed into dotnet:master Jul 10, 2018

rmkerr added a commit to rmkerr/corefx that referenced this pull request Jul 10, 2018

Add support and tests for HTTP 308 Permanent Redirect (dotnet#30398)

191c57b

This change adds support for 308 redirects to SocketsHttpHandler, and enables 308 redirects in our tests. Fixes: #30389

rmkerr mentioned this pull request Jul 10, 2018

[release/2.1] Add support and tests for HTTP 308 Permanent Redirect (#30398) #30954

Merged

rmkerr added a commit that referenced this pull request Jul 10, 2018

Add support and tests for HTTP 308 Permanent Redirect (#30398) (#30954)

35a009a

This change adds support for 308 redirects to SocketsHttpHandler, and enables 308 redirects in our tests. Fixes: #30389

karelz added this to the 3.0 milestone Jul 19, 2018

	if (response.StatusCode != HttpStatusCode.MultipleChoices &&
	response.StatusCode != HttpStatusCode.MovedPermanently &&
	response.StatusCode != HttpStatusCode.Redirect &&
	response.StatusCode != HttpStatusCode.RedirectMethod &&
	response.StatusCode != HttpStatusCode.RedirectKeepVerb)
	{
	break;
	}

	private bool IsLastResponse(HttpWebRequest request, HttpStatusCode statusCode)
	{
	if (request.AllowAutoRedirect)
	{
	if (statusCode == HttpStatusCode.Ambiguous \|\| // 300
	statusCode == HttpStatusCode.Moved \|\| // 301
	statusCode == HttpStatusCode.Redirect \|\| // 302
	statusCode == HttpStatusCode.RedirectMethod \|\| // 303
	statusCode == HttpStatusCode.RedirectKeepVerb) // 307
	{
	return s_autoRedirectsAccessor(request) >= request.MaximumAutomaticRedirections;
	}
	}

	return true;
	}

	public async Task GetAsync_MaxAutomaticRedirectionsNServerHops_ThrowsIfTooMany(int maxHops, int hops)
	{
	if (IsWinHttpHandler && !PlatformDetection.IsWindows10Version1703OrGreater)
	{
	// Skip this test if using WinHttpHandler but on a release prior to Windows 10 Creators Update.
	_output.WriteLine("Skipping test due to Windows 10 version prior to Version 1703.");
	return;
	}
	else if (PlatformDetection.IsFullFramework)
	{
	// Skip this test if running on .NET Framework. Exceeding max redirections will not throw
	// exception. Instead, it simply returns the 3xx response.
	_output.WriteLine("Skipping test on .NET Framework due to behavior difference.");
	return;
	}

	HttpClientHandler handler = CreateHttpClientHandler();
	handler.MaxAutomaticRedirections = maxHops;
	using (var client = new HttpClient(handler))
	{
	Task<HttpResponseMessage> t = client.GetAsync(Configuration.Http.RedirectUriForDestinationUri(
	secure: false,
	statusCode: 302,
	destinationUri: Configuration.Http.RemoteEchoServer,
	hops: hops));

	if (hops <= maxHops)
	{
	using (HttpResponseMessage response = await t)
	{
	Assert.Equal(HttpStatusCode.OK, response.StatusCode);
	Assert.Equal(Configuration.Http.RemoteEchoServer, response.RequestMessage.RequestUri);
	}
	}
	else
	{
	if (UseSocketsHttpHandler)
	{
	using (HttpResponseMessage response = await t)
	{
	Assert.Equal(HttpStatusCode.Redirect, response.StatusCode);
	}
	}
	else
	{
	await Assert.ThrowsAsync<HttpRequestException>(() => t);
	}
	}
	}
	}

Conversation

rmkerr commented Jun 14, 2018

Uh oh!

rmkerr commented Jun 14, 2018

Uh oh!

stephentoub Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

stephentoub Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

rmkerr Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

rmkerr Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

rmkerr Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsh Jun 18, 2018

Choose a reason for hiding this comment

Uh oh!

davidsh Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsh commented Jun 14, 2018

Uh oh!

rmkerr commented Jun 14, 2018

Uh oh!

davidsh Jun 14, 2018

Choose a reason for hiding this comment

Uh oh!

davidsh commented Jun 14, 2018

Uh oh!

rmkerr commented Jun 14, 2018

Uh oh!

davidsh commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidsh commented Jun 14, 2018

Uh oh!

rmkerr commented Jun 14, 2018

Uh oh!

rmkerr commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidsh commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rmkerr commented Jun 15, 2018

Uh oh!

davidsh left a comment

Choose a reason for hiding this comment

Uh oh!

rmkerr commented Jun 19, 2018

Uh oh!

rmkerr commented Jul 9, 2018

Uh oh!

rmkerr commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rmkerr Jun 14, 2018 •

edited

Loading

rmkerr Jun 18, 2018 •

edited

Loading

davidsh Jun 18, 2018 •

edited

Loading

davidsh commented Jun 14, 2018 •

edited

Loading

rmkerr commented Jun 15, 2018 •

edited

Loading

davidsh commented Jun 15, 2018 •

edited

Loading