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
Closed

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

seoester opened this issue Jun 14, 2016 · 4 comments

Comments

@seoester
Copy link

seoester 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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

adg 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
Copy link

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

@golang golang locked and limited conversation to collaborators Jun 15, 2017
@rsc rsc unassigned bradfitz and adg Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants