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

Force GET Redirect on PUT and POST on HTTP Status Code 300-303 #28998

Closed
mnaumanali94 opened this issue Mar 18, 2019 · 18 comments · Fixed by #49095
Closed

Force GET Redirect on PUT and POST on HTTP Status Code 300-303 #28998

mnaumanali94 opened this issue Mar 18, 2019 · 18 comments · Fixed by #49095
Labels
area-System.Net.Http bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@mnaumanali94
Copy link

Currently, GET request is just forced for POST requests with a 300-303 response. This behavior should also be in place for PUT and DELETE considering they are not safe functions either. RestSharp does it like that.

If not, I couldn't even figure out a way to keep the current behavior and just handling it differently for these methods. From the looks of it, I'll have to implement the complete redirection logic to achieve the desired behavior.

@stephentoub
Copy link
Member

This behavior should also be in place for PUT and DELETE considering they are not safe functions either

Is there an HTTP-related specification somewhere that dictates that?

@davidsh
Copy link
Contributor

davidsh commented Mar 18, 2019

We generally follow RFC 7231. Section 6.4 covers behaviors for redirection: https://tools.ietf.org/html/rfc7231.html#section-6.4

@davidsh
Copy link
Contributor

davidsh commented Mar 18, 2019

Currently, GET request is just forced for POST requests with a 300-303 response.

RFC 7231 describes the behavior for POST redirects and indicates it should transform to GET.

This behavior should also be in place for PUT and DELETE considering they are not safe functions either. RestSharp does it like that.

However, there are no described behaviors in the RFC for transforming PUT or DELETE. If RestSharp is doing that, then it is inventing a new behavior.

What do browsers do in these cases?

@mnaumanali94
Copy link
Author

Alright. You are right the RFC's are not clear. I tried tools like postman that did the mapping to GET even for PUT and DELETE. My understanding is that due to the confusion 307 and 308 were made to ensure request method are not rewritten. For 301 and 302 the behavior varies across tools. 303 maps to GET in most cases even for PUT and DELETE.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Redirections#Temporary_redirections.

For 303 the rewriting should happen in my opinion. 301 and 302 are debatable.

@JoeEdwardsCode
Copy link

It should be noted that forcing a GET on 303 for a PUT is the behavior of System.Net.Http in .NET Framework from what I can tell.

@karelz
Copy link
Member

karelz commented Oct 8, 2019

Triage: We should match .NET Framework in cases where RFC is not clear.

@TimothyByrd
Copy link
Contributor

TimothyByrd commented Mar 2, 2021

Is this stackoverflow answer wrong?

303: Redirect for undefined reason. Typically, 'Operation has completed, continue elsewhere.'
Clients making subsequent requests for this resource should not use the new URI. Clients
should follow the redirect for POST/PUT/DELETE requests, but use GET for the follow-up request.

I just converted a utility from Net Framework to Net 5.0, and the results were not pretty.

I'm doing a PUT to https://www.example.com/thing/12345 and the server returns a 303 with the same URI in the Location header, as a way to say "if you want the new value of this expensive thing, come and GET it".

In Net Framework, the retry does a GET on https://www.example.com/thing/12345 and the server returns a 200 result and all is well.

In Net 5.0, the retry does another PUT to https://www.example.com/thing/12345 and the server returns 303 again.
This repeats for the default 50 retries, and then I finally get the 303 back to do something with.

I'm not clear on what use case this is desirable behaviour.

Since I didn't want to completely disable retries for my GETs, too, the smallest way to compensate seemed to be to set MaxAutomaticRedirections = 1 then manually handle the 303 for the PUT.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 4, 2021
@geoffkizer
Copy link
Contributor

Here's my take on the RFC language, taken from https://tools.ietf.org/html/rfc7231#section-6.4

300 Multiple Choices is a weird one and not really used in practice. The RFC is unclear here. I would suggest we not make any change here.

301 Moved Permanently and 302 Found both explicitly state:

For historical reasons, a user agent MAY change the request method from POST to GET for the subsequent request.

So I think our current behavior here (only change POST) is correct.

303 See Other explicitly states:

This status code is applicable to any HTTP method.

So, I think we should change the behavior for 303 to change the method to GET for all methods (not just PUT). I believe this matches both .NET Framework behavior and common practice.

@scalablecory @stephentoub Any concerns here?

@TimothyByrd
Copy link
Contributor

TimothyByrd commented Mar 4, 2021

Is there a place I can check the Net Framework code to make sure the behaviour is matching?
If there are places where the RFC is unclear, I suggest compatibility with the existing Net Framework.
Otherwise, I can't trust porting code that has worked for years.

@stephentoub
Copy link
Member

@stephentoub Any concerns here?

If it brings us better in line with the spec and netfx, sounds good.

@karelz
Copy link
Member

karelz commented Mar 4, 2021

@TimothyByrd best option is to write a test for each case and try it out. Code is here: https://referencesource.microsoft.com/

If there are places where the RFC is unclear, I suggest compatibility with the existing Net Framework.
Otherwise, I can't trust porting code that has worked for years.

That is our high-level desire as well -- see #28998 (comment).
However note, that we do not have plans to be bug-by-bug compatible with .NET Framework (as it is technically impossible and extremely limits innovation and improvements). If we decide that a behavior on .NET Framework was buggy / undesirable / not conformant with RFC, it might be acceptable to make a breaking change in .NET Core in certain circumstances.
Of course, we do not want to break majority of customers intentionally, so for "bugs" which many customers depend on, we will likely keep them also in .NET Core to make everyone's life easier.

@TimothyByrd
Copy link
Contributor

TimothyByrd commented Mar 4, 2021

Fair enough.
In cases where the RFC is not clear, the code still needs to do something, so figuring out the most sensible something is important.

I have updated the PR such that:

  • The behaviour will only change for 303/SeeOther, not for any of the other 30x responses.
  • A redirect on a 303 will change to a GET for POST (existing behaviour), PUT, DELETE, PATCH and OPTIONS
    • GET is already GET and HEAD should stay HEAD
    • You said "all" methods - but I kind of think OPTIONS should stay OPTIONS, too.

@karelz @stephentoub Is that what you meant?

@geoffkizer
Copy link
Contributor

A redirect on a 303 will change to a GET for POST (existing behaviour), PUT, DELETE, PATCH and OPTIONS

Why not all methods?

I think special casing HEAD is probably reasonable; HEAD is sort of a special case of GET, and if you did a HEAD in the first place it's pretty weird for that to result in a response body.

But for other methods, including OPTIONS and any unrecognized method, the RFC seems pretty clear that 303 applies to this. The server chose to send a 303. That means it wants the client to follow the redirect.

@TimothyByrd
Copy link
Contributor

TimothyByrd commented Mar 7, 2021

Why not all methods?

Well, we know the RFC is unclear, and that since HEAD stays HEAD, "all" doesn't really mean "all".

(This paragraph is how I was imagining things.) POST/PUT/PATCH all create/update a resource, so having a redirect to a GET makes sense for the case where assembling the resource is expensive. OPTIONS sounds like it retrieves metadata about a resource, which makes it seem more like HEAD in a way.

To figure things out, I took a brute force approach and checked what Firefox, Edge/Chrome and Net Framework do.

It looks like the only major discrepancy is that Net Framework changes "all" methods (except HEAD) to GET when redirecting on a 300, and doesn't follow the redirect on a 308. I don't suggest doing that, but I do think it should be documented as change between Net Framework and Net.

And I've updated the pull request to change all methods (except HEAD) to GET.

@wfurt
Copy link
Member

wfurt commented Mar 7, 2021

It seems like compatibility with Framework could be useful if RFC is ambiguous. Would it make sense to add AppContext switch @geoffkizer for legacy behavior?

@geoffkizer
Copy link
Contributor

Would it make sense to add AppContext switch @geoffkizer for legacy behavior?

You mean, to match current behavior? I think our current behavior here is pretty broken, so I don't think there's a need for a compat switch.

@geoffkizer
Copy link
Contributor

So to conclude the discussion, here's where we have landed:

(1) 303 should change all methods to GET, except HEAD. This matches .NET Framework and common browser behavior.
(2) 300 behavior is inconsistent across browsers/.NET Framework; we will not make any changes here.

If anything looks wrong, please yell now.

@geoffkizer
Copy link
Contributor

BTW @TimothyByrd thanks for doing the research re browser behavior here, that's very helpful.

geoffkizer pushed a commit that referenced this issue Mar 9, 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 all methods except HEAD.

Co-authored-by: Timothy Byrd <timothy.byrd@laserfiche.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 9, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2021
@karelz karelz modified the milestones: Future, 6.0.0 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 bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants