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

net/http: Server removes all explicitly set `Transfer-Encoding` headers #16063

Closed
seoester opened this issue Jun 14, 2016 · 4 comments

Comments

Projects
None yet
5 participants
@seoester
Copy link

commented Jun 14, 2016

The net/http server removes all explicitly set Transfer-Encoding headers (when operating in HTTP/1.1 mode).

This bug was introduced by the fix of #15960 (f9b4556), sorry for any ambiguous wording on my end.

Example:

package main

import (
    "compress/gzip"
    "net/http"
    "strings"
)

const Level int = -1

func myHandler(w http.ResponseWriter, r *http.Request) {
    if strings.Contains(r.Header.Get("TE"), "gzip") {
        writer, err := gzip.NewWriterLevel(w, Level)
        if err != nil {
            panic(err)
        }
        w.Header().Set("Transfer-Encoding", "gzip")
        writer.Write([]byte("Some content..."))
    } else {
        w.Write([]byte("Couldn't gzip (not accepted)"))
    }
}

func main() {
    http.ListenAndServe(":8080", http.HandlerFunc(myHandler))
}

For clients accepting a gzip transfer-coding, net/http removes the explicitly set gzip from the Transfer-Encoding header and only sends chunked.

To fix both issues there must be a check for the header field's value (== "chunked").
E.g. (the line introduced by #15960 also has to be removed):

        ...
        if hasTE && te == "identity" {
            cw.chunking = false
            w.closeAfterReply = true
        } else if hasTE && te == "chunked" {
            cw.chunking = true
            setHeader.transferEncoding = "chunked"
            delHeader("Transfer-Encoding")
        } else {
            // HTTP/1.1 or greater: use chunked transfer encoding
            ...

To reiterate:

  • An explicitly set transfer-coding of chunked should not result in a duplicate Transfer-Encoding: chunked header
  • Whenever another transfer-coding is set, chunked must be applied also, but the explicitly set transfer-encoding should be kept in the header

Details can be found in RFC 2616 Section 3.6.

@ianlancetaylor ianlancetaylor added this to the Go1.7Maybe milestone Jun 14, 2016

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2016

What is our best move for 1.7 here? I want us to be very careful to avoid the kind of problem we had with 1.6, where late changes broke HTTP2 support. Should we roll back the fix for #15960 and leave all of this for 1.8?

@seoester

This comment has been minimized.

Copy link
Author

commented Jun 14, 2016

Pre #15960 is only non-rfc-compliant in one edge case, any client I tested (chrom{e, ium}, firefox, wget) didn't break however.

That being said though I personally think the suggested fix (+ removing the line introduced by #15960) is rather safe from side effects.

@adg

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2016

I sent a change. I think it is a low-risk change, as the net change now only affects users that explicitly set Transfer-Encoding: chunked in their handlers. We also have tests now. I think this is preferable to a rollback.

@gopherbot

This comment has been minimized.

Copy link

commented Jun 15, 2016

CL https://golang.org/cl/24130 mentions this issue.

@gopherbot gopherbot closed this in 0ec6256 Jun 15, 2016

@golang golang locked and limited conversation to collaborators Jun 15, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.