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

transfer: Switch PUT to GET/HEAD on 303 redirect #5248

Closed
wants to merge 2 commits into from

Conversation

jay
Copy link
Member

@jay jay commented Apr 16, 2020

Prior to this change if there was a 303 reply to a PUT request then
the subsequent request to respond to that redirect would also be a PUT.
It was determined that was most likely incorrect based on the language
of the RFCs. Basically 303 means "see other" resource, which implies it
is most likely not the same resource, therefore we should not try to PUT
to that different resource.

Refer to the discussion in #5237 for more information.

Fixes #5237
Closes #xxxx


In Curl_http it will override the request string to PUT when data->set.upload is true, which is why I decided to fix this issue by setting data->set.upload as false on 303.

curl/lib/http.c

Lines 2070 to 2073 in b81e0b0

if((conn->handler->protocol&(PROTO_FAMILY_HTTP|CURLPROTO_FTP)) &&
data->set.upload) {
httpreq = HTTPREQ_PUT;
}

/cc @PofMagicfingers @jzakrzewski

Prior to this change if there was a 303 reply to a PUT request then
the subsequent request to respond to that redirect would also be a PUT.
It was determined that was most likely incorrect based on the language
of the RFCs. Basically 303 means "see other" resource, which implies it
is most likely not the same resource, therefore we should not try to PUT
to that different resource.

Refer to the discussion in curl#5237 for more information.

Fixes curl#5237
Closes #xxxx
@michael-o
Copy link
Contributor

I'd like to hear the clarification of the HTTP WG before this gets merged.

@bagder
Copy link
Member

bagder commented Apr 16, 2020

I believe this change matches what the spirit of RFC 7231 section 6.4.4 seems to be. I do however think that this change also requires a test to verify it. I could only spot test 1332 when looking for existing tests using 303.

@jay jay added the needs-info label Apr 22, 2020
@jay
Copy link
Member Author

jay commented Apr 22, 2020

@michael-o do you have an update? I would like to land this before the release next week.

@michael-o
Copy link
Contributor

michael-o commented Apr 22, 2020

@jay I did not inquire anything with the HTTP WG. I expect this either you doing or @PofMagicfingers. I have just expressed my concerns.

@PofMagicfingers
Copy link

PofMagicfingers commented Apr 22, 2020

I did send an email to the http-wg mailing list a week ago, but as I'm not a member of their mailing list my message is pending manual processing. I've had no news about it and the question is not yet published :

https://lists.w3.org/Archives/Public/ietf-http-wg/2020AprJun/thread.html

@michael-o
Copy link
Contributor

@PofMagicfingers You can also try the GitHub project of them. @reschke, can you help?

@reschke
Copy link

reschke commented Apr 22, 2020

I would recommend to subscribe to the mailing list and re-post.

@PofMagicfingers
Copy link

@reschke Indeed. Reposted and it worked :
https://lists.w3.org/Archives/Public/ietf-http-wg/2020AprJun/0044.html

Thanks!

@PofMagicfingers
Copy link

We got an answer : https://lists.w3.org/Archives/Public/ietf-http-wg/2020AprJun/0046.html

The Location URL in a 303 is defined for retrieval. Only retrieval
methods can be expected to work on it.

HTTP defined GET and HEAD for regular use, but other protocols may
define more methods for retrieval. So there is no requirement to use
GET/HEAD specifically. Those other protocol alternatives may be used if
the context needs one.

Amos

@jay jay closed this in c0e139a Apr 22, 2020
@jay
Copy link
Member Author

jay commented Apr 22, 2020

Thanks

@jay jay deleted the fix_303_PUT branch April 22, 2020 21:57
@jay jay removed the needs-info label Apr 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Following a 303 after a PUT requests uses PUT instead of GET
5 participants