Skip to content
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.

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

Make 303 redirects do GET like Net Framework #49095

Merged
merged 10 commits into from
Mar 9, 2021
Merged

Make 303 redirects do GET like Net Framework #49095

merged 10 commits into from
Mar 9, 2021

Conversation

TimothyByrd
Copy link
Contributor

@TimothyByrd TimothyByrd commented Mar 4, 2021

Make 303 redirects do GET like Net Framework

In Net Framework, PUT redirects on a 303 do a GET.
Net 5.0 breaks compatibility with this.
See #28998
This commit causes redirects of a 303 to do a GET
for POST (existing behavior), PUT, DELETE, PATCH and OPTIONS

Fix #28998

Timothy Byrd added 4 commits March 3, 2021 15:48
…m to net Framework behaviour - issue 28998
In Net Framework, PUT redirects do a GET.
Net 5.0 breaks compatibiltiy with this.
See #28998
This commit causes PUT redirects to act like POST redirects
for 30x responses to conform to Net Framework behaviour.

Fix #28998
@ghost
Copy link

ghost commented Mar 4, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Make PUT redirects do GET like Net Framework

In Net Framework, PUT redirects do a GET.
Net 5.0 breaks compatibility with this.
See #28998
This commit causes PUT redirects to act like POST redirects
for 30x responses to conform to Net Framework behavior.

Fix #28998

Author: TimothyByrd
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@dnfadmin
Copy link

dnfadmin commented Mar 4, 2021

CLA assistant check
All CLA requirements met.

@geoffkizer
Copy link
Contributor

I think we need to have consensus on what the right behavior here is in #28998 before we can consider taking this PR.

I'll add my comments there shortly.

@karelz karelz added the blocked Issue/PR is blocked on something - see comments label Mar 4, 2021
In Net Framework, PUT redirects on a 303 do a GET.
Net 5.0 breaks compatibility with this.
See #28998
This commit causes redirects of a 303 to do a GET
for POST (existing behaviour), PUT, DELETE, PATCH and OPTIONS

Fix #28998
@TimothyByrd TimothyByrd changed the title Make PUT redirects do GET like Net Framework Make 303 redirects do GET like Net Framework Mar 4, 2021
In Net Framework, PUT redirects on a 303 do a GET.
Net 5.0 breaks compatibility with this.
See #28998
This commit causes redirects of a 303 to do a GET for all methods except HEAD.

Fix #28998
@wfurt
Copy link
Member

wfurt commented Mar 7, 2021

You should add tests covering code changes @TimothyByrd. I agree with @geoffkizer that we should reach consensus on #28998 first.

@TimothyByrd
Copy link
Contributor Author

TimothyByrd commented Mar 8, 2021

@wfurt

You should add tests covering code changes @TimothyByrd. I agree with @geoffkizer that we should reach consensus on #28998 first.

I did. They are in src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.AutoRedirect.cs

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

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

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)

@geoffkizer
Copy link
Contributor

Overall this looks good, just one comment to add an addition test case above.

case HttpStatusCode.MultipleChoices:
return requestMethod == HttpMethod.Post;
case HttpStatusCode.SeeOther:
return requestMethod != HttpMethod.Head;
Copy link
Contributor

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

Choose a reason for hiding this comment

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

Okay - done.

@geoffkizer
Copy link
Contributor

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@geoffkizer
Copy link
Contributor

CI failures look unrelated, will merge

@geoffkizer geoffkizer merged commit 0ab99ce into dotnet:main Mar 9, 2021
@geoffkizer
Copy link
Contributor

Thanks for your contribution, @TimothyByrd!

thaystg added a commit to thaystg/runtime that referenced this pull request Mar 10, 2021
* upstream/main: (83 commits)
  Fix a crash in llvm if the sreg of a setret is not set because the methods ends with a throw. (dotnet#49122)
  [macOS-arm64] Disable failing libraries tests (dotnet#49400)
  improve PriorityQueue documentation (dotnet#49392)
  [wasm] Fix debugger tests (dotnet#49206)
  [mono] Fix the emission of EnumEqualityComparer instances into the corlib AOT image. (dotnet#49402)
  jitutils M2M renaming reaction (dotnet#49430)
  WinHttpHandler: Read HTTP/2 trailing headers
  [RyuJIT] Make casthelpers cold for sealed classes (dotnet#49295)
  JIT: Non-void ThrowHelpers (dotnet#48589)
  Update package index for servicing (dotnet#49417)
  Remove unnecessary check on polymorphic serialization (dotnet#48464)
  Remove release build cron triggers from jitstress jobs (dotnet#49333)
  [main] Update dependencies from dotnet/arcade dotnet/llvm-project dotnet/runtime-assets (dotnet#49359)
  Implement AppleCryptoNative_X509GetRawData using SecCertificateCopyData
  [AndroidCrypto] Support a zero-length salt for HMACs. (dotnet#49384)
  Use managed implementation of pbkdf2 for Android's one-shot implementation. (dotnet#49314)
  Make 303 redirects do GET like Net Framework (dotnet#49095)
  Make sure event generation is incremental (dotnet#48903)
  Add amd and Surface arm64 perf runs (dotnet#49389)
  Enregister EH var that are single def (dotnet#47307)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http blocked Issue/PR is blocked on something - see comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Force GET Redirect on PUT and POST on HTTP Status Code 300-303
5 participants