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

DecoderPlugin - cURL 8.1.0 issue with HTTP/2 #230

Open
cleptric opened this issue May 23, 2023 · 10 comments
Open

DecoderPlugin - cURL 8.1.0 issue with HTTP/2 #230

cleptric opened this issue May 23, 2023 · 10 comments

Comments

@cleptric
Copy link

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 getsentry/sentry-php#1537 and curl/curl#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
Copy link
Contributor

GrahamCampbell commented May 23, 2023

Is this issue present in curl 8.1.1?

@cleptric
Copy link
Author

According to curl/curl#11175 (comment) it is

@GrahamCampbell
Copy link
Contributor

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
Copy link
Contributor

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

@cleptric
Copy link
Author

@dbu
Copy link
Contributor

dbu commented May 24, 2023

i am not too familiar with HTTP/2 nor the decoder plugin. the code happens here:

public function handleRequest(RequestInterface $request, callable $next, callable $first): Promise

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
Copy link
Author

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
Copy link
Contributor

dbu commented May 25, 2023

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

@frankmckechnie
Copy link

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

@frankmckechnie
Copy link

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

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

No branches or pull requests

4 participants