Skip to content

Make 303 redirects do GET like Net Framework #49095

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Mar 9, 2021
Merged

Make 303 redirects do GET like Net Framework #49095

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
73c3f7f
make PUT redirects act like POST redirects for 30x reponses to confor…
Mar 3, 2021
8dafbc8
Make PUT redirects do GET like Net Framework
Mar 3, 2021
d5c8555
Merge branch 'issue/28998' of https://github.com/TimothyByrd/dotnet_r…
Mar 4, 2021
7adca75
Merge branch 'issue/28998' of https://github.com/TimothyByrd/dotnet_r…
Mar 4, 2021
2717413
Merge branch 'issue/28998' of https://github.com/TimothyByrd/dotnet_r…
Mar 4, 2021
66f59c4
Make 303 redirects do GET like Net Framework
Mar 4, 2021
98b4652
Make 303 redirects do GET like Net Framework
Mar 7, 2021
ca834c0
Add test for MYCUSTOMMETHOD to check 303 redirect
Mar 9, 2021
3ec922c
Group test code by behavior
Mar 9, 2021
728a81f
Do not change GET to GET on 303
Mar 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.AutoRedirect.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ public static IEnumerable<object[]> RedirectStatusCodesOldMethodsNewMethods()
{
yield return new object[] { statusCode, "GET", "GET" };
yield return new object[] { statusCode, "POST", statusCode <= 303 ? "GET" : "POST" };
yield return new object[] { statusCode, "PUT", statusCode == 303 ? "GET" : "PUT" };
yield return new object[] { statusCode, "DELETE", statusCode == 303 ? "GET" : "DELETE" };
yield return new object[] { statusCode, "PATCH", statusCode == 303 ? "GET" : "PATCH" };
yield return new object[] { statusCode, "OPTIONS", statusCode == 303 ? "GET" : "OPTIONS" };
Copy link
Contributor

@geoffkizer geoffkizer Mar 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one thing I'd add here is a completely custom method, e.g. "MYCUSTOMMETHOD" or whatever, and validate it gets changed to GET on 303.

Copy link
Contributor Author

@TimothyByrd TimothyByrd Mar 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done - first i verified that an incorrect test of
yield return new object[] { statusCode, "MYCUSTOMMETHOD", "MYCUSTOMMETHOD" };
fails and then that the correct
yield return new object[] { statusCode, "MYCUSTOMMETHOD", statusCode == 303 ? "GET" : "MYCUSTOMMETHOD" };
passes
(I like breaking tests so I can tell I did something when I make them work)

yield return new object[] { statusCode, "HEAD", "HEAD" };
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,10 @@ private static bool RequestRequiresForceGet(HttpStatusCode statusCode, HttpMetho
{
case HttpStatusCode.Moved:
case HttpStatusCode.Found:
case HttpStatusCode.SeeOther:
case HttpStatusCode.MultipleChoices:
return requestMethod == HttpMethod.Post;
case HttpStatusCode.SeeOther:
return requestMethod != HttpMethod.Head;
Copy link
Contributor

@geoffkizer geoffkizer Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nit: I think this should return false when requestMethod == HttpMethod.Get.

It probably doesn't make much difference, but it seems better to treat this as not changing the method, as we do with GET in other cases like Moved. If nothing else, it will avoid an unnecessary and confusing trace call.

Copy link
Contributor Author

@TimothyByrd TimothyByrd Mar 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay - done.

default:
return false;
}
Expand Down