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

Fix/transfer encoding fix for get method #1795

Closed
wants to merge 2 commits into from
Closed

Fix/transfer encoding fix for get method #1795

wants to merge 2 commits into from

Conversation

smecnarowski
Copy link

When CURLOPT_UPLOAD is sent to curl_setopt method it's resulting with 'Transfer-uncoding: chunked' which rather should not be present in GET or HEAD requests as in section 4.4 of https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html

Szymon Mecnarowski added 2 commits March 31, 2017 01:41
… methods

When CURLOPT_UPLOAD is sent to curl_setopt method it's resulting with 'Transfer-uncoding: chunked' which rather should not be present in requeast as in section 4.4 of https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html
@sagikazarmark
Copy link
Member

sagikazarmark commented Apr 7, 2017

While I generally agree with this, GET and HEAD requests CAN have a body. So the question is whether this causes any real trouble or not.

@smecnarowski
Copy link
Author

Sorry i put wrong paragraph. What I was thinking about was written in section 4.3:

The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers. A message-body MUST NOT be included in a request if the specification of the request method (section 5.1.1) does not allow sending an entity-body in requests. A server SHOULD read and forward a message-body on any request; if the request method does not include defined semantics for an entity-body, then the message-body SHOULD be ignored when handling the request.

I needed to connect IIS server behind ARR proxy and there i found this problem. The second one is very restrictive about above MUST-NOT be included and throws 502 error.

@sagikazarmark
Copy link
Member

Okay. Since we have another PR which tries to loosen these rules within Guzzle, I would like to cross-check with that one to see how it would interfere with this one before merging. But if there is a conflict between sending and not sending body in a GET/HEAD request, I would like Guzzle to stick to the standard and what some servers do in ninja mode.

@sagikazarmark sagikazarmark self-requested a review April 7, 2017 20:04
@smecnarowski
Copy link
Author

OK - Thank you

@smecnarowski
Copy link
Author

@sagikazarmark And how it looks like ?

@sagikazarmark sagikazarmark added this to the 6.3.0 milestone May 1, 2017
@Nyholm
Copy link
Member

Nyholm commented Jun 9, 2017

Thank you for this PR. I do understand your use case but Im not sure the solution is the best one. As Mark said, GET request may have a body.

In side applyBody we cannot really tell if there is a body or not. However, if it is a HEAD request, no body is allowed and we should unset($conf[CURLOPT_UPLOAD]) in `applyMethod``. (Like we do with CURLOPT_READFUNCTION).

To solve your issue for GET request you may set a Content-Length header to 0.

@sagikazarmark
Copy link
Member

I think the aformentioned PR is already merged and will be released as 6.3

I guess merging this PR would go quite the opposite direction, so I am not sure at this point we can merge it as well.

@sagikazarmark sagikazarmark removed this from the 6.3.0 milestone Jun 22, 2017
@smecnarowski
Copy link
Author

Sorry for late answer. For my point of view and real live problem I would like to share one on Stackoverflow user analyze. https://stackoverflow.com/a/15656884

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants