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

Jump to bottom

x/net/http2: gzip response not decoded #40774

Closed
melardev opened this issue Aug 13, 2020 · 5 comments
Closed

x/net/http2: gzip response not decoded #40774

melardev opened this issue Aug 13, 2020 · 5 comments
Labels
FrozenDueToAge

Comments

@melardev
Copy link

Go Version

$ go version
go version go1.13.8 linux/amd64

Summary

Hi, Gzip response is not decoded if the user specifies Accept-Encoding in the request headers manually.

Details

In my previous issue #40765 I was asked to support the x-gzip value, looking at the code for how gzip responses are handled I found a bug, first, it was in
https://github.com/golang/go/blob/master/src/net/http/transport.go#L2503
Then I noticed the same bug is here too:
https://github.com/golang/go/blob/master/src/net/http/h2_bundle.go#L7500

Basically, if the user specifies the Accept-Encoding to gzip, the requestedGzip variable won't be set and hence the response won't be decoded.
Then the following check is bypassed because requestedGzip is false.
https://github.com/golang/go/blob/master/src/net/http/h2_bundle.go#L8474

Steps to reproduce

To reproduce the bug run the following:

client := http.Client{}
	request, err := http.NewRequest("GET", "https://httpbin.org/gzip", nil)
	request.Header.Set("Accept-Encoding", "gzip")

	if err != nil {
		log.Fatalln(err)
	}

	resp, err := client.Do(request)
	if err != nil {
		log.Fatalln(err)
	}

	content, _ := ioutil.ReadAll(resp.Body)
        // Garbage will be printed, gzip is not decoded!
	log.Println(string(content))

If we remove the line

request.Header.Set("Accept-Encoding", "gzip")

The code would work fine.
This would be my first contribution to such a big project, can I make a pull request to fix this bug?

The diff file would look like this: (the only difference is that the changes have to be made to the func (cc *http2ClientConn) roundTrip(*Request) method

@@ -2012,7 +2012,8 @@ func (pc *persistConn) readLoop() {
 		}
 
 		resp.Body = body
-		if rc.addedGzip && strings.EqualFold(resp.Header.Get("Content-Encoding"), "gzip") {
+		if rc.addedGzip && (strings.EqualFold(resp.Header.Get("Content-Encoding"), "gzip") ||
+			strings.EqualFold(resp.Header.Get("Content-Encoding"), "x-gzip")) {
 			resp.Body = &gzipReader{body: body}
 			resp.Header.Del("Content-Encoding")
 			resp.Header.Del("Content-Length")
@@ -2368,9 +2369,9 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
 	// requested it.
 	requestedGzip := false
 	if !pc.t.DisableCompression &&
-		req.Header.Get("Accept-Encoding") == "" &&
 		req.Header.Get("Range") == "" &&
 		req.Method != "HEAD" {
+
 		// Request gzip only, not deflate. Deflate is ambiguous and
 		// not as universally supported anyway.
 		// See: https://zlib.net/zlib_faq.html#faq39
@@ -2383,8 +2384,14 @@ func (pc *persistConn) roundTrip(req *transportRequest) (resp *Response, err err
 		// We don't request gzip if the request is for a range, since
 		// auto-decoding a portion of a gzipped document will just fail
 		// anyway. See https://golang.org/issue/8923
-		requestedGzip = true
-		req.extraHeaders().Set("Accept-Encoding", "gzip")
+		if req.Header.Get("Accept-Encoding") == "" || req.Header.Get("Accept-Encoding") == "gzip" {
+			requestedGzip = true
+			req.extraHeaders().Set("Accept-Encoding", "gzip")
+		} else if req.Header.Get("Accept-Encoding") == "x-gzip" {
+			requestedGzip = true
+			req.extraHeaders().Set("Accept-Encoding", "x-gzip")
+		}
+
 	}
 
 	var continueCh chan struct{}
@cespare
Copy link
Contributor

cespare commented Aug 13, 2020

Isn't this intended behavior? Automatic decompression only happens if a Transport adds Accept-Encoding: gzip on its own.

From the Transport.DisableCompression docs:

    // DisableCompression, if true, prevents the Transport from
    // requesting compression with an "Accept-Encoding: gzip"
    // request header when the Request contains no existing
    // Accept-Encoding value. If the Transport requests gzip on
    // its own and gets a gzipped response, it's transparently
    // decoded in the Response.Body. However, if the user
    // explicitly requested gzip it is not automatically
    // uncompressed.
    DisableCompression bool

@ianlancetaylor
Copy link
Contributor

Thanks, but please don't send us patches in the issue tracker. Please send them via Gerrit or GitHub, as discussed at https://golang.org/doc/contribute.html. That ensures that the copyright agreements are correct. We can't use or theoretically even look at patches submitted in a GitHub issue. Thanks.

@ianlancetaylor ianlancetaylor changed the title Gzip decoding bug x/net/http2: gzip response not decoded Aug 13, 2020
@melardev
Copy link
Author

@cespare in my steps to reproduce I showed how the snippet adds the Accept-Encoding gzip, despite this, the HTTP client does not decode the gzip response, the reason is because http2ClientConn (h2_bundle.go) is only decoding gzip responses if the user has left the Accept-Encoding header empty, which is not appropriate.

@cespare
Copy link
Contributor

cespare commented Aug 13, 2020

@melardev yes, and I'm saying that behavior is intended.

If you don't set the Accept-Encoding header, a Transport transparently tries to use gzip: it addes Accept-encoding: gzip itself and then decompresses the response. As the user, you don't even see that this is happening. DisableCompression turns this automatic gzip support off.

If you do set Accept-Encoding, as in your repro, then the Transport does not do anything special. It does not mess with the header and it does not do anything to the response. It is up to the user to decompress the response that they have asked to be gzipped (or perhaps not decompress!).

@melardev
Copy link
Author

@cespare hm, I see, it looks opaque and strange to me, but I understand your point, I close then.

@golang golang locked and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cespare @ianlancetaylor @gopherbot @melardev