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

DecoderPlugin - cURL 8.1.0 issue with HTTP/2 #230

Open cleptric opened 1 year ago

cleptric commented 1 year ago

We got an issue reported over at https://github.com/getsentry/sentry-php, that cURL 8.1.0 and up no longer work as expected when using the DecoderPlugin.

See https://github.com/getsentry/sentry-php/issues/1537 and https://github.com/curl/curl/issues/11175.

From @krowinski

The problem is in \Http\Client\Common\Plugin\DecoderPlugin::handleRequest as is adding header 'TE' $request = $request->withHeader('TE', $encodings);

with values "TE" => array:3 [ 0 => "gzip" 1 => "deflate" 2 => "chunked" ]

And RFC for HTTP/2 specifies The only exception to this is the TE header field, which MAY be present in an HTTP/2 request; when it is, it MUST NOT contain any value other than "trailers"."

https://httpwg.org/specs/rfc9113.html#rfc.section.8.2.2

Its connected to curl lib as on 8.1.0 we got error 

curl -X POST https://google.com  -d ''  -H "TE: gzip"
curl: (56) HTTP/2 stream 1 was reset

and before is was ok 

curl -X POST https://google.com  -d ''  -H "TE: gzip"                                                                                                                                                                                                                                                                               
<!DOCTYPE html>

But still php http client should not add invalid headers as curl seems to be more strict(?)

GrahamCampbell commented 1 year ago

Is this issue present in curl 8.1.1?

cleptric commented 1 year ago

According to https://github.com/curl/curl/issues/11175#issuecomment-1559739707 it is

GrahamCampbell commented 1 year ago

Hmmm. I am not so sure this is a bug here. We have an decoder plugin, but it is not used by default. Sentry are the ones who are adding it.

GrahamCampbell commented 1 year ago

Looking at other repos that use this repo, they typically do not use this plugin.

cleptric commented 1 year ago

We indeed use this plugin directly, https://github.com/getsentry/sentry-php/blob/master/src/HttpClient/HttpClientFactory.php#L102.

dbu commented 1 year ago

i am not too familiar with HTTP/2 nor the decoder plugin. the code happens here: https://github.com/php-http/client-common/blob/676f98fb3785d2c9e1b16aa24d9fcc39066f3f06/src/Plugin/DecoderPlugin.php#L54

if the HTTP/2 forbids this header, is the plugin still useful and needed? if not, maybe change the sentry setup to not include the plugin when HTTP/2 is used. if the plugin does useful things for HTTP/2, could we make the plugin detect if the request is a HTTP/2 request and in that case not add the header or with the trailers value if that is needed? at which point does the client know whether it is initiating a HTTP/2 request and not a 1.x request?

cleptric commented 1 year ago

We worked around this issue by enforcing HTTP/1.1 for now. I still suggest finding a proper solution, as we will be one of many running into this issue.

dbu commented 1 year ago

sure, happy to merge a proper solution if somebody can find out how that solution should look like.

frankmckechnie commented 1 year ago

We are now seeing this issue within our Symfony app, how would one go about configuring sentry to not use the decoder.

frankmckechnie commented 1 year ago

Seems within symfony you can disable compression which will stop sentry from using this plugin.