nytimes / gziphandler

Go middleware to gzip HTTP responses
https://godoc.org/github.com/NYTimes/gziphandler
Apache License 2.0
868 stars 129 forks source link

Vary: Accept-encoding header is duplicated if inner handler sets it #90

Open austin-searchpilot opened 5 years ago

austin-searchpilot commented 5 years ago

If the inner HTTP handler sets a Vary: Accept-Encoding header, then as the gzip middleware will always add in the same header, the output will have two identical headers.

Here is a failing test case:

func TestEnsureVaryHeaderNoDuplicate(t *testing.T) {
    handler := GzipHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
        w.Header().Add(vary, acceptEncoding)
        w.Write([]byte("test"))
        w.(io.Closer).Close()
    }))

    req := httptest.NewRequest("GET", "/", nil)
    req.Header.Set(acceptEncoding, "gzip")
    w := httptest.NewRecorder()
    handler.ServeHTTP(w, req)
    assert.Equal(t, w.Header()[vary], []string{acceptEncoding})
}

I don't think the HTTP spec explicitly disallows you from having the same key/value appearing twice in the headers but it feels tidier to only have one instance of it.

jameshartig commented 4 years ago

I'm not sure there's a great way to fix this because technically the gziphandler package is adding the header BEFORE your inner handler, so technically your inner handler should be checking if the header already exists like so:


// if something else already set Vary: Accept-Encoding, don't set it twice
if !strings.Contains(w.Header().Get(vary), acceptEncoding) {
    w.Header().Add(vary, acceptEncoding)
}
renthraysk commented 4 years ago

Get() only returns the first element of a slice. Have to iterate over the slice. Also use canonical values.

func AddVary(w http.ResponseWriter, value string) {
    value = textproto.CanonicalMIMEHeaderKey(value)
    for _, v := range w.Header()["Vary"] {
        if textproto.CanonicalMIMEHeaderKey(v) == value {
            return
        }
    }
    w.Header().Add("Vary", value)
}