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

proposal: net/http: accept "x-gzip" content encoding value #40765

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

proposal: net/http: accept "x-gzip" content encoding value #40765

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

Comments

@melardev
Copy link

@melardev melardev commented Aug 13, 2020

Hi, would it be good if I submit the following diff file?
It basically treats x-gzip as gzip, as noted in the HTTP 0.9 RFC: https://www.ietf.org/rfc/rfc1945.txt section 3.5
For the request header, that may not be that important, but for the response it may save more than one response from legacy servers.

@@ -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{}
@andybons andybons changed the title legacy gzip content coding support proposal: net/http: accept "x-gzip" content encoding value Aug 13, 2020
@gopherbot gopherbot added this to the Proposal milestone Aug 13, 2020
@gopherbot gopherbot added the Proposal label Aug 13, 2020
@andybons
Copy link
Member

@andybons andybons commented Aug 13, 2020

@andybons
Copy link
Member

@andybons andybons commented Aug 13, 2020

@melardev are you experiencing this in the wild? Which servers still serve x-gzip as their content-encoding?

@melardev
Copy link
Author

@melardev melardev commented Aug 13, 2020

@andybons actually not, but I was trying to write some HTTP client code for HTTP 0.9 and reviewing the RFC this showed up, I asked myself if the remote server returns such response, would be golang able to decode it or not, looking at the code it seems there may be an issue if that code path is triggered, so I wanted to ask here if it would be interesting to add support for it and see what you think about that.

@melardev
Copy link
Author

@melardev melardev commented Aug 13, 2020

Also, I don't know exactly when that function is being triggered, by default using HttpClient.Do() it is not, it would be interesting to know what would have happened if the client already adds the header "Accept-Encoding" to "gzip", the code as of now, it would not set the requestedGzip boolean flag to true, look at:
https://github.com/golang/go/blob/master/src/net/http/transport.go#L2504
It only sets the requestedGzip boolean to true if the header is absent and not when the header is present with gzip. But again, don't know exactly when that code is triggered.

@melardev
Copy link
Author

@melardev melardev commented Aug 13, 2020

@andybons Digging a little bit further I detected a bug indeed, this same code is at h2_bundle.go which has a copy paste code (as the comment also state):
https://github.com/golang/go/blob/master/src/net/http/h2_bundle.go#L7500
Then the following check is bypassed because requestedGzip is false.
https://github.com/golang/go/blob/master/src/net/http/h2_bundle.go#L8474
So 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 prorject, can I make a pull request to fix this bug?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.