search

php-http / client-common

Common HTTP Client implementations and tools for HTTPlug
http://httplug.io
MIT License
1.01k stars 53 forks source link

RedirectPlugin sends body in GET requests #205

Closed josiasmontag closed 1 year ago

josiasmontag commented 3 years ago

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:

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(''));
dbu commented 3 years ago

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.

dbu commented 3 years ago

@Nyholm do you have an idea how we can empty the body of the request?

GrahamCampbell commented 3 years ago

$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.

dbu commented 3 years ago

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.

dbu commented 1 year ago

@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?

josiasmontag commented 1 year ago

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.