php-http
RedirectPlugin sends body in GET requests
Description
If the initial request method is POST and the server responds with a Location header, the RedirectPlugin is configured to follow the redirect with a GET request. However, the RedirectPlugin will resend the body of the initial request. This results in a GET request with a request body.
Most servers will just ignore the request body of GET requests. Nevertheless, this causes trouble:
- AWS CloudFront rejects GET requests with request body
- Unnecessary traffic / performance issues
Possible Solution
The RedirectPlugin clones the original request and changes the method to GET at this point:
https://github.com/php-http/client-common/blob/e37e46c610c87519753135fb893111798c69076a/src/Plugin/RedirectPlugin.php#L187
When switching to GET it should also remove the body.
However, I do not see any way to remove the body easily. One quick & dirty solution that worked for me is to use an empty stream:
$originalRequest = $originalRequest->withBody(GuzzleHttp\Psr7\stream_for(''));
hm, i imagine this could also have security implications, when sending a body to an unexpected target.
on the other hand, it really depends. GET request with body are not prohibited by HTTP. elasticsearch for example uses them to query the index (though they also allow POST, to get around limitations in the network like the one you describe).
i suggest that we introduce a configuration option in the redirect plugin drop_body_when_change_to_get or something like that. if we only drop to an empty body when the redirect changes the method (we enter https://github.com/php-http/client-common/blob/e37e46c610c87519753135fb893111798c69076a/src/Plugin/RedirectPlugin.php#L187 and the method actually is different).
which leaves the question of how to empty the body. short of providing the stream factory as the value of the option, i don't see how it can be done, but maybe i am missing something.
$originalRequest = $originalRequest->withBody(GuzzleHttp\Psr7\stream_for(''));
The problem with doing that is other headers may now be wrong, such as the content type header.
hm, that as well. guess we should also remove the content-type request header when changing to a GET request. content-length should also go. can we mimik the behaviour of guzzle, or is that also not completely correct?
a dependency on guzzle would be a problem - the client-common package afaik does not depend on a concrete implementation. injecting the stream factory sounds like a lot of complication, but would probably be the correct thing.
@josiasmontag still remember this issue? i was cleaning up things around the RedirectPlugin and found this one too. may i ask you to check if #222 will solve the issue correctly or if you have any feedback?
Looking at the code and the tests it should solve this issue, yes. I cannot test it live though because I no longer use CloudFront in this project.