x/net/http2: gzip response not decoded

[melardev]

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]

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]

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.

[melardev]

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

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

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