]> Cypherpunks repositories - gostls13.git/commitdiff
Revert "net/http: support gzip, x-gzip Transfer-Encodings"
authorFilippo Valsorda <filippo@golang.org>
Wed, 22 Jan 2020 01:06:37 +0000 (01:06 +0000)
committerFilippo Valsorda <filippo@golang.org>
Wed, 22 Jan 2020 22:06:30 +0000 (22:06 +0000)
This reverts commit e6c12c3d0296251f1d5a96ebde811dbfd4a914fe.

Reason for revert: the assumption that a T-E of "gzip" implies
"chunked" seems incorrect. The RFC does state that one "MUST apply
chunked as the final transfer coding" but that should be interpreted to
mean that a "chunked" encoding must be listed as the last one, not that
one should be assumed to be there if not. This is confirmed by the
alternative option to chunking on the server side being to "terminate
the message by closing the connection".

The issue seems confirmed by the fact that the code in the body of
#29162 fails with the following error:

    net/http: HTTP/1.x transport connection broken: http: failed to gunzip body: unexpected EOF

This late in the cycle, revert rather than fix, also because we don't
apparently have tests for the correct behavior.

Change-Id: I920ec928754cd8e96a06fb7ff8a53316c0f959e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/215757
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Katie Hockman <katie@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
src/net/http/transfer.go
src/net/http/transfer_test.go

index 1d6a987545438bbf02fe9e355eef51da4268a63b..2e01a07f84fccdbf8ded4873f31ee151a8d2b414 100644 (file)
@@ -7,7 +7,6 @@ package http
 import (
        "bufio"
        "bytes"
-       "compress/gzip"
        "errors"
        "fmt"
        "io"
@@ -467,34 +466,6 @@ func suppressedHeaders(status int) []string {
        return nil
 }
 
-// proxyingReadCloser is a composite type that accepts and proxies
-// io.Read and io.Close calls to its respective Reader and Closer.
-//
-// It is composed of:
-// a) a top-level reader e.g. the result of decompression
-// b) a symbolic Closer e.g. the result of decompression, the
-//    original body and the connection itself.
-type proxyingReadCloser struct {
-       io.Reader
-       io.Closer
-}
-
-// multiCloser implements io.Closer and allows a bunch of io.Closer values
-// to all be closed once.
-// Example usage is with proxyingReadCloser if we are decompressing a response
-// body on the fly and would like to close both *gzip.Reader and underlying body.
-type multiCloser []io.Closer
-
-func (mc multiCloser) Close() error {
-       var err error
-       for _, c := range mc {
-               if err1 := c.Close(); err1 != nil && err == nil {
-                       err = err1
-               }
-       }
-       return err
-}
-
 // msg is *Request or *Response.
 func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
        t := &transferReader{RequestMethod: "GET"}
@@ -572,7 +543,7 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
        // Prepare body reader. ContentLength < 0 means chunked encoding
        // or close connection when finished, since multipart is not supported yet
        switch {
-       case chunked(t.TransferEncoding) || implicitlyChunked(t.TransferEncoding):
+       case chunked(t.TransferEncoding):
                if noResponseBodyExpected(t.RequestMethod) || !bodyAllowedForStatus(t.StatusCode) {
                        t.Body = NoBody
                } else {
@@ -593,21 +564,6 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
                }
        }
 
-       // Finally if "gzip" was one of the requested transfer-encodings,
-       // we'll unzip the concatenated body/payload of the request.
-       // TODO: As we support more transfer-encodings, extract
-       // this code and apply the un-codings in reverse.
-       if t.Body != NoBody && gzipped(t.TransferEncoding) {
-               zr, err := gzip.NewReader(t.Body)
-               if err != nil {
-                       return fmt.Errorf("http: failed to gunzip body: %v", err)
-               }
-               t.Body = &proxyingReadCloser{
-                       Reader: zr,
-                       Closer: multiCloser{zr, t.Body},
-               }
-       }
-
        // Unify output
        switch rr := msg.(type) {
        case *Request:
@@ -627,41 +583,8 @@ func readTransfer(msg interface{}, r *bufio.Reader) (err error) {
        return nil
 }
 
-// Checks whether chunked is the last part of the encodings stack
-func chunked(te []string) bool { return len(te) > 0 && te[len(te)-1] == "chunked" }
-
-// implicitlyChunked is a helper to check for implicity of chunked, because
-// RFC 7230 Section 3.3.1 says that the sender MUST apply chunked as the final
-// payload body to ensure that the message is framed for both the request
-// and the body. Since "identity" is incompatible with any other transformational
-// encoding cannot co-exist, the presence of "identity" will cause implicitlyChunked
-// to return false.
-func implicitlyChunked(te []string) bool {
-       if len(te) == 0 { // No transfer-encodings passed in, so not implicitly chunked.
-               return false
-       }
-       for _, tei := range te {
-               if tei == "identity" {
-                       return false
-               }
-       }
-       return true
-}
-
-func isGzipTransferEncoding(tei string) bool {
-       // RFC 7230 4.2.3 requests that "x-gzip" SHOULD be considered the same as "gzip".
-       return tei == "gzip" || tei == "x-gzip"
-}
-
-// Checks where either of "gzip" or "x-gzip" are contained in transfer encodings.
-func gzipped(te []string) bool {
-       for _, tei := range te {
-               if isGzipTransferEncoding(tei) {
-                       return true
-               }
-       }
-       return false
-}
+// Checks whether chunked is part of the encodings stack
+func chunked(te []string) bool { return len(te) > 0 && te[0] == "chunked" }
 
 // Checks whether the encoding is explicitly "identity".
 func isIdentity(te []string) bool { return len(te) == 1 && te[0] == "identity" }
@@ -697,47 +620,25 @@ func (t *transferReader) fixTransferEncoding() error {
 
        encodings := strings.Split(raw[0], ",")
        te := make([]string, 0, len(encodings))
-
-       // When adding new encodings, please maintain the invariant:
-       //   if chunked encoding is present, it must always
-       //   come last and it must be applied only once.
-       // See RFC 7230 Section 3.3.1 Transfer-Encoding.
-       for i, encoding := range encodings {
+       // TODO: Even though we only support "identity" and "chunked"
+       // encodings, the loop below is designed with foresight. One
+       // invariant that must be maintained is that, if present,
+       // chunked encoding must always come first.
+       for _, encoding := range encodings {
                encoding = strings.ToLower(strings.TrimSpace(encoding))
-
+               // "identity" encoding is not recorded
                if encoding == "identity" {
-                       // "identity" should not be mixed with other transfer-encodings/compressions
-                       // because it means "no compression, no transformation".
-                       if len(encodings) != 1 {
-                               return &badStringError{`"identity" when present must be the only transfer encoding`, strings.Join(encodings, ",")}
-                       }
-                       // "identity" is not recorded.
                        break
                }
-
-               switch {
-               case encoding == "chunked":
-                       // "chunked" MUST ALWAYS be the last
-                       // encoding as per the  loop invariant.
-                       // That is:
-                       //     Invalid: [chunked, gzip]
-                       //     Valid:   [gzip, chunked]
-                       if i+1 != len(encodings) {
-                               return &badStringError{"chunked must be applied only once, as the last encoding", strings.Join(encodings, ",")}
-                       }
-                       // Supported otherwise.
-
-               case isGzipTransferEncoding(encoding):
-                       // Supported
-
-               default:
+               if encoding != "chunked" {
                        return &unsupportedTEError{fmt.Sprintf("unsupported transfer encoding: %q", encoding)}
                }
-
                te = te[0 : len(te)+1]
                te[len(te)-1] = encoding
        }
-
+       if len(te) > 1 {
+               return &badStringError{"too many transfer encodings", strings.Join(te, ",")}
+       }
        if len(te) > 0 {
                // RFC 7230 3.3.2 says "A sender MUST NOT send a
                // Content-Length header field in any message that
index a8ce2d3709ab3d6ab37635a30b3e4faf156aabc8..65009ee8bf7fd5c9cb97a04e393d194ccc964aa0 100644 (file)
@@ -7,7 +7,6 @@ package http
 import (
        "bufio"
        "bytes"
-       "compress/gzip"
        "crypto/rand"
        "fmt"
        "io"
@@ -62,6 +61,7 @@ func TestFinalChunkedBodyReadEOF(t *testing.T) {
        buf := make([]byte, len(want))
        n, err := res.Body.Read(buf)
        if n != len(want) || err != io.EOF {
+               t.Logf("body = %#v", res.Body)
                t.Errorf("Read = %v, %v; want %d, EOF", n, err, len(want))
        }
        if string(buf) != want {
@@ -290,7 +290,7 @@ func TestFixTransferEncoding(t *testing.T) {
                },
                {
                        hdr:     Header{"Transfer-Encoding": {"chunked, chunked", "identity", "chunked"}},
-                       wantErr: &badStringError{"chunked must be applied only once, as the last encoding", "chunked, chunked"},
+                       wantErr: &badStringError{"too many transfer encodings", "chunked,chunked"},
                },
                {
                        hdr:     Header{"Transfer-Encoding": {"chunked"}},
@@ -310,283 +310,3 @@ func TestFixTransferEncoding(t *testing.T) {
                }
        }
 }
-
-func gzipIt(s string) string {
-       buf := new(bytes.Buffer)
-       gw := gzip.NewWriter(buf)
-       gw.Write([]byte(s))
-       gw.Close()
-       return buf.String()
-}
-
-func TestUnitTestProxyingReadCloserClosesBody(t *testing.T) {
-       var checker closeChecker
-       buf := new(bytes.Buffer)
-       buf.WriteString("Hello, Gophers!")
-       prc := &proxyingReadCloser{
-               Reader: buf,
-               Closer: &checker,
-       }
-       prc.Close()
-
-       read, err := ioutil.ReadAll(prc)
-       if err != nil {
-               t.Fatalf("Read error: %v", err)
-       }
-       if g, w := string(read), "Hello, Gophers!"; g != w {
-               t.Errorf("Read mismatch: got %q want %q", g, w)
-       }
-
-       if checker.closed != true {
-               t.Fatal("closeChecker.Close was never invoked")
-       }
-}
-
-func TestGzipTransferEncoding_request(t *testing.T) {
-       helloWorldGzipped := gzipIt("Hello, World!")
-
-       tests := []struct {
-               payload  string
-               wantErr  string
-               wantBody string
-       }{
-
-               {
-                       // The case of "chunked" properly applied as the last encoding
-                       // and a gzipped request payload that is streamed in 3 parts.
-                       payload: `POST / HTTP/1.1
-Host: golang.org
-Transfer-Encoding: gzip, chunked
-Content-Type: text/html; charset=UTF-8
-
-` + fmt.Sprintf("%02x\r\n%s\r\n%02x\r\n%s\r\n%02x\r\n%s\r\n0\r\n\r\n",
-                               3, helloWorldGzipped[:3],
-                               5, helloWorldGzipped[3:8],
-                               len(helloWorldGzipped)-8, helloWorldGzipped[8:]),
-                       wantBody: `Hello, World!`,
-               },
-
-               {
-                       // The request specifies "Transfer-Encoding: chunked" so its body must be left untouched.
-                       payload: `PUT / HTTP/1.1
-Host: golang.org
-Transfer-Encoding: chunked
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-` + fmt.Sprintf("%0x\r\n%s\r\n0\r\n\r\n", len(helloWorldGzipped), helloWorldGzipped),
-                       // We want that payload as it was sent.
-                       wantBody: helloWorldGzipped,
-               },
-
-               {
-                       // Valid request, the body doesn't have "Transfer-Encoding: chunked" but implicitly encoded
-                       // for chunking as per the advisory from RFC 7230 3.3.1 which advises for cases where.
-                       payload: `POST / HTTP/1.1
-Host: localhost
-Transfer-Encoding: gzip
-Content-Type: text/html; charset=UTF-8
-
-` + fmt.Sprintf("%0x\r\n%s\r\n0\r\n\r\n", len(helloWorldGzipped), helloWorldGzipped),
-                       wantBody: `Hello, World!`,
-               },
-
-               {
-                       // Invalid request, the body isn't chunked nor is the connection terminated immediately
-                       // hence invalid as per the advisory from RFC 7230 3.3.1 which advises for cases where
-                       // a Transfer-Encoding that isn't finally chunked is provided.
-                       payload: `PUT / HTTP/1.1
-Host: golang.org
-Transfer-Encoding: gzip
-Content-Length: 0
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-`,
-                       wantErr: `EOF`,
-               },
-
-               {
-                       // The case of chunked applied before another encoding.
-                       payload: `PUT / HTTP/1.1
-Location: golang.org
-Transfer-Encoding: chunked, gzip
-Content-Length: 0
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-`,
-                       wantErr: `chunked must be applied only once, as the last encoding "chunked, gzip"`,
-               },
-
-               {
-                       // The case of chunked properly applied as the
-                       // last encoding BUT with a bad "Content-Length".
-                       payload: `POST / HTTP/1.1
-Host: golang.org
-Transfer-Encoding: gzip, chunked
-Content-Length: 10
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-` + "0\r\n\r\n",
-                       wantErr: "EOF",
-               },
-       }
-
-       for i, tt := range tests {
-               req, err := ReadRequest(bufio.NewReader(strings.NewReader(tt.payload)))
-               if tt.wantErr != "" {
-                       if err == nil || !strings.Contains(err.Error(), tt.wantErr) {
-                               t.Errorf("test %d. Error mismatch\nGot:  %v\nWant: %s", i, err, tt.wantErr)
-                       }
-                       continue
-               }
-
-               if err != nil {
-                       t.Errorf("test %d. Unexpected ReadRequest error: %v\nPayload:\n%s", i, err, tt.payload)
-                       continue
-               }
-
-               got, err := ioutil.ReadAll(req.Body)
-               req.Body.Close()
-               if err != nil {
-                       t.Errorf("test %d. Failed to read response body: %v", i, err)
-               }
-               if g, w := string(got), tt.wantBody; g != w {
-                       t.Errorf("test %d. Request body mimsatch\nGot:\n%s\n\nWant:\n%s", i, g, w)
-               }
-       }
-}
-
-func TestGzipTransferEncoding_response(t *testing.T) {
-       helloWorldGzipped := gzipIt("Hello, World!")
-
-       tests := []struct {
-               payload  string
-               wantErr  string
-               wantBody string
-       }{
-
-               {
-                       // The case of "chunked" properly applied as the last encoding
-                       // and a gzipped payload that is streamed in 3 parts.
-                       payload: `HTTP/1.1 302 Found
-Location: https://golang.org/
-Transfer-Encoding: gzip, chunked
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-` + fmt.Sprintf("%02x\r\n%s\r\n%02x\r\n%s\r\n%02x\r\n%s\r\n0\r\n\r\n",
-                               3, helloWorldGzipped[:3],
-                               5, helloWorldGzipped[3:8],
-                               len(helloWorldGzipped)-8, helloWorldGzipped[8:]),
-                       wantBody: `Hello, World!`,
-               },
-
-               {
-                       // The response specifies "Transfer-Encoding: chunked" so response body must be left untouched.
-                       payload: `HTTP/1.1 302 Found
-Location: https://golang.org/
-Transfer-Encoding: chunked
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-` + fmt.Sprintf("%0x\r\n%s\r\n0\r\n\r\n", len(helloWorldGzipped), helloWorldGzipped),
-                       // We want that payload as it was sent.
-                       wantBody: helloWorldGzipped,
-               },
-
-               {
-                       // Valid response, the body doesn't have "Transfer-Encoding: chunked" but implicitly encoded
-                       // for chunking as per the advisory from RFC 7230 3.3.1 which advises for cases where.
-                       payload: `HTTP/1.1 302 Found
-Location: https://golang.org/
-Transfer-Encoding: gzip
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-` + fmt.Sprintf("%0x\r\n%s\r\n0\r\n\r\n", len(helloWorldGzipped), helloWorldGzipped),
-                       wantBody: `Hello, World!`,
-               },
-
-               {
-                       // Invalid response, the body isn't chunked nor is the connection terminated immediately
-                       // hence invalid as per the advisory from RFC 7230 3.3.1 which advises for cases where
-                       // a Transfer-Encoding that isn't finally chunked is provided.
-                       payload: `HTTP/1.1 302 Found
-Location: https://golang.org/
-Transfer-Encoding: gzip
-Content-Length: 0
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-`,
-                       wantErr: `EOF`,
-               },
-
-               {
-                       // The case of chunked applied before another encoding.
-                       payload: `HTTP/1.1 302 Found
-Location: https://golang.org/
-Transfer-Encoding: chunked, gzip
-Content-Length: 0
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-`,
-                       wantErr: `chunked must be applied only once, as the last encoding "chunked, gzip"`,
-               },
-
-               {
-                       // The case of chunked properly applied as the
-                       // last encoding BUT with a bad "Content-Length".
-                       payload: `HTTP/1.1 302 Found
-Location: https://golang.org/
-Transfer-Encoding: gzip, chunked
-Content-Length: 10
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-` + "0\r\n\r\n",
-                       wantErr: "EOF",
-               },
-
-               {
-                       // Including "identity" more than once.
-                       payload: `HTTP/1.1 200 OK
-Location: https://golang.org/
-Transfer-Encoding: identity, identity
-Content-Length: 0
-Connection: close
-Content-Type: text/html; charset=UTF-8
-
-` + "0\r\n\r\n",
-                       wantErr: `"identity" when present must be the only transfer encoding "identity, identity"`,
-               },
-       }
-
-       for i, tt := range tests {
-               res, err := ReadResponse(bufio.NewReader(strings.NewReader(tt.payload)), nil)
-               if tt.wantErr != "" {
-                       if err == nil || !strings.Contains(err.Error(), tt.wantErr) {
-                               t.Errorf("test %d. Error mismatch\nGot:  %v\nWant: %s", i, err, tt.wantErr)
-                       }
-                       continue
-               }
-
-               if err != nil {
-                       t.Errorf("test %d. Unexpected ReadResponse error: %v\nPayload:\n%s", i, err, tt.payload)
-                       continue
-               }
-
-               got, err := ioutil.ReadAll(res.Body)
-               res.Body.Close()
-               if err != nil {
-                       t.Errorf("test %d. Failed to read response body: %v", i, err)
-               }
-               if g, w := string(got), tt.wantBody; g != w {
-                       t.Errorf("test %d. Response body mimsatch\nGot:\n%s\n\nWant:\n%s", i, g, w)
-               }
-       }
-}