]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: keep Content-Encoding in Error, add GODEBUG for ServeContent
authorDamien Neil <dneil@google.com>
Mon, 17 Jun 2024 19:30:19 +0000 (12:30 -0700)
committerDamien Neil <dneil@google.com>
Tue, 18 Jun 2024 19:33:10 +0000 (19:33 +0000)
This reverts the changes to Error from CL 571995, and adds a
GODEBUG controlling the changes to ServeContent/ServeFile/ServeFS.

The change to remove the Content-Encoding header when serving an error
breaks middleware which sets Content-Encoding: gzip and wraps a
ResponseWriter in one which compresses the response body.

This middleware already breaks when ServeContent handles a Range request.
Correct uses of ServeContent which serve pre-compressed content with
a Content-Encoding: gzip header break if we don't remove that header
when serving errors. Therefore, we keep the change to ServeContent/
ServeFile/ServeFS, but we add the ability to disable the new behavior
by setting GODEBUG=httpservecontentkeepheaders=1.

We revert the change to Error, because users who don't want to include
a Content-Encoding header in errors can simply remove the header
themselves, or not add it in the first place.

Fixes #66343

Change-Id: Ic19a24b73624a5ac1a258ed7a8fe7d9bf86c6a38
Reviewed-on: https://go-review.googlesource.com/c/go/+/593157
Reviewed-by: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

doc/godebug.md
doc/next/6-stdlib/99-minor/net/http/66343.md
src/internal/godebugs/table.go
src/net/http/fs.go
src/net/http/fs_test.go
src/net/http/serve_test.go
src/net/http/server.go
src/runtime/metrics/doc.go

index 86e02e820cd85518edb74d85ed4366921101f0cd..b3a43664c42cd4ab6fcd4cc59abd15e25da469e2 100644 (file)
@@ -200,6 +200,15 @@ This behavior is controlled by the `x509keypairleaf` setting. For Go 1.23, it
 defaults to `x509keypairleaf=1`. Previous versions default to
 `x509keypairleaf=0`.
 
+Go 1.23 changed
+[`net/http.ServeContent`](/pkg/net/http#ServeContent),
+[`net/http.ServeFile`](/pkg/net/http#ServeFile), and
+[`net/http.ServeFS`](/pkg/net/http#ServeFS) to
+remove Cache-Control, Content-Encoding, Etag, and Last-Modified headers
+when serving an error. This behavior is controlled by
+the [`httpservecontentkeepheaders` setting](/pkg/net/http#ServeContent).
+Using `httpservecontentkeepheaders=1` restores the pre-Go 1.23 behavior.
+
 ### Go 1.22
 
 Go 1.22 adds a configurable limit to control the maximum acceptable RSA key size
index 128ce68d45ebeb1e466e219ae5f5e84e04ec1d72..b39e8624e7def10396ab034cbff3c620eca5b1ef 100644 (file)
@@ -1 +1,16 @@
-[Error] now removes misleading response headers.
+[ServeContent], [ServeFile], and [ServeFileFS] now remove
+the `Cache-Control`, `Content-Encoding`, `Etag`, and `Last-Modified`
+headers when serving an error. These headers usually apply to the
+non-error content, but not to the text of errors.
+
+Middleware which wraps a [ResponseWriter] and applies on-the-fly
+encoding, such as `Content-Encoding: gzip`, will not function after
+this change.  The previous behavior of [ServeContent], [ServeFile],
+and [ServeFileFS] may be restored by setting
+`GODEBUG=httpservecontentkeepheaders=1`.
+
+Note that middleware which changes the size of the served content
+(such as by compressing it) already does not function properly when
+[ServeContent] handles a Range request. On-the-fly compression
+should use the `Transfer-Encoding` header instead of `Content-Encoding`.
+
index eb512559168c32e76eeb2f3954782cc3a548e0ec..f4262b6695d03237278d1ab260753a4678c4f007 100644 (file)
@@ -36,6 +36,7 @@ var All = []Info{
        {Name: "http2server", Package: "net/http"},
        {Name: "httplaxcontentlength", Package: "net/http", Changed: 22, Old: "1"},
        {Name: "httpmuxgo121", Package: "net/http", Changed: 22, Old: "1"},
+       {Name: "httpservecontentkeepheaders", Package: "net/http", Changed: 23, Old: "0"},
        {Name: "installgoroot", Package: "go/build"},
        {Name: "jstmpllitinterp", Package: "html/template", Opaque: true}, // bug #66217: remove Opaque
        //{Name: "multipartfiles", Package: "mime/multipart"},
index c213d8a328564c3dc13284b73c9367c306af0809..70653550f027f61bcbdfe9738b05dcc80ea0b485 100644 (file)
@@ -9,6 +9,7 @@ package http
 import (
        "errors"
        "fmt"
+       "internal/godebug"
        "io"
        "io/fs"
        "mime"
@@ -171,15 +172,37 @@ func dirList(w ResponseWriter, r *Request, f File) {
        fmt.Fprintf(w, "</pre>\n")
 }
 
+// GODEBUG=httpservecontentkeepheaders=1 restores the pre-1.23 behavior of not deleting
+// Cache-Control, Content-Encoding, Etag, or Last-Modified headers on ServeContent errors.
+var httpservecontentkeepheaders = godebug.New("httpservecontentkeepheaders")
+
 // serveError serves an error from ServeFile, ServeFileFS, and ServeContent.
 // Because those can all be configured by the caller by setting headers like
 // Etag, Last-Modified, and Cache-Control to send on a successful response,
 // the error path needs to clear them, since they may not be meant for errors.
 func serveError(w ResponseWriter, text string, code int) {
        h := w.Header()
-       h.Del("Etag")
-       h.Del("Last-Modified")
-       h.Del("Cache-Control")
+
+       nonDefault := false
+       for _, k := range []string{
+               "Cache-Control",
+               "Content-Encoding",
+               "Etag",
+               "Last-Modified",
+       } {
+               if !h.has(k) {
+                       continue
+               }
+               if httpservecontentkeepheaders.Value() == "1" {
+                       nonDefault = true
+               } else {
+                       h.Del(k)
+               }
+       }
+       if nonDefault {
+               httpservecontentkeepheaders.IncNonDefault()
+       }
+
        Error(w, text, code)
 }
 
@@ -203,11 +226,17 @@ func serveError(w ResponseWriter, text string, code int) {
 //
 // The content's Seek method must work: ServeContent uses
 // a seek to the end of the content to determine its size.
+// Note that [*os.File] implements the [io.ReadSeeker] interface.
 //
 // If the caller has set w's ETag header formatted per RFC 7232, section 2.3,
 // ServeContent uses it to handle requests using If-Match, If-None-Match, or If-Range.
 //
-// Note that [*os.File] implements the [io.ReadSeeker] interface.
+// If an error occurs when serving the request (for example, when
+// handling an invalid range request), ServeContent responds with an
+// error message. By default, ServeContent strips the Cache-Control,
+// Content-Encoding, ETag, and Last-Modified headers from error responses.
+// The GODEBUG setting httpservecontentkeepheaders=1 causes ServeContent
+// to preserve these headers.
 func ServeContent(w ResponseWriter, req *Request, name string, modtime time.Time, content io.ReadSeeker) {
        sizeFunc := func() (int64, error) {
                size, err := content.Seek(0, io.SeekEnd)
index 2c3426f735271f63e65b22b75b1ed2346d4f026e..2ffffbf0b33c9705fdd22f2199f7ee71f6b4c9d3 100644 (file)
@@ -1223,8 +1223,20 @@ type issue12991File struct{ File }
 func (issue12991File) Stat() (fs.FileInfo, error) { return nil, fs.ErrPermission }
 func (issue12991File) Close() error               { return nil }
 
-func TestFileServerErrorMessages(t *testing.T) { run(t, testFileServerErrorMessages) }
-func testFileServerErrorMessages(t *testing.T, mode testMode) {
+func TestFileServerErrorMessages(t *testing.T) {
+       run(t, func(t *testing.T, mode testMode) {
+               t.Run("keepheaders=0", func(t *testing.T) {
+                       testFileServerErrorMessages(t, mode, false)
+               })
+               t.Run("keepheaders=1", func(t *testing.T) {
+                       testFileServerErrorMessages(t, mode, true)
+               })
+       }, testNotParallel)
+}
+func testFileServerErrorMessages(t *testing.T, mode testMode, keepHeaders bool) {
+       if keepHeaders {
+               t.Setenv("GODEBUG", "httpservecontentkeepheaders=1")
+       }
        fs := fakeFS{
                "/500": &fakeFileInfo{
                        err: errors.New("random error"),
@@ -1254,8 +1266,12 @@ func testFileServerErrorMessages(t *testing.T, mode testMode) {
                        t.Errorf("GET /%d: StatusCode = %d; want %d", code, res.StatusCode, code)
                }
                for _, hdr := range []string{"Etag", "Last-Modified", "Cache-Control"} {
-                       if v, ok := res.Header[hdr]; ok {
-                               t.Errorf("GET /%d: Header[%q] = %q, want not present", code, hdr, v)
+                       if v, got := res.Header[hdr]; got != keepHeaders {
+                               want := "not present"
+                               if keepHeaders {
+                                       want = "present"
+                               }
+                               t.Errorf("GET /%d: Header[%q] = %q, want %v", code, hdr, v, want)
                        }
                }
        }
@@ -1710,6 +1726,17 @@ func testFileServerDirWithRootFile(t *testing.T, mode testMode) {
 }
 
 func TestServeContentHeadersWithError(t *testing.T) {
+       t.Run("keepheaders=0", func(t *testing.T) {
+               testServeContentHeadersWithError(t, false)
+       })
+       t.Run("keepheaders=1", func(t *testing.T) {
+               testServeContentHeadersWithError(t, true)
+       })
+}
+func testServeContentHeadersWithError(t *testing.T, keepHeaders bool) {
+       if keepHeaders {
+               t.Setenv("GODEBUG", "httpservecontentkeepheaders=1")
+       }
        contents := []byte("content")
        ts := newClientServerTest(t, http1Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
                w.Header().Set("Content-Type", "application/octet-stream")
@@ -1738,6 +1765,12 @@ func TestServeContentHeadersWithError(t *testing.T) {
        out, _ := io.ReadAll(res.Body)
        res.Body.Close()
 
+       ifKept := func(s string) string {
+               if keepHeaders {
+                       return s
+               }
+               return ""
+       }
        if g, e := res.StatusCode, 416; g != e {
                t.Errorf("got status = %d; want %d", g, e)
        }
@@ -1750,16 +1783,16 @@ func TestServeContentHeadersWithError(t *testing.T) {
        if g, e := res.Header.Get("Content-Length"), strconv.Itoa(len(out)); g != e {
                t.Errorf("got content-length = %q, want %q", g, e)
        }
-       if g, e := res.Header.Get("Content-Encoding"), ""; g != e {
+       if g, e := res.Header.Get("Content-Encoding"), ifKept("gzip"); g != e {
                t.Errorf("got content-encoding = %q, want %q", g, e)
        }
-       if g, e := res.Header.Get("Etag"), ""; g != e {
+       if g, e := res.Header.Get("Etag"), ifKept(`"abcdefgh"`); g != e {
                t.Errorf("got etag = %q, want %q", g, e)
        }
-       if g, e := res.Header.Get("Last-Modified"), ""; g != e {
+       if g, e := res.Header.Get("Last-Modified"), ifKept("Wed, 21 Oct 2015 07:28:00 GMT"); g != e {
                t.Errorf("got last-modified = %q, want %q", g, e)
        }
-       if g, e := res.Header.Get("Cache-Control"), ""; g != e {
+       if g, e := res.Header.Get("Cache-Control"), ifKept("immutable"); g != e {
                t.Errorf("got cache-control = %q, want %q", g, e)
        }
        if g, e := res.Header.Get("Content-Range"), "bytes */7"; g != e {
index 06bf5089d8fe63ae48627f429d8a827cf72a6789..3ec10c2f14da56d0ac2c3cfb96db944919658373 100644 (file)
@@ -7166,13 +7166,12 @@ func testErrorContentLength(t *testing.T, mode testMode) {
 func TestError(t *testing.T) {
        w := httptest.NewRecorder()
        w.Header().Set("Content-Length", "1")
-       w.Header().Set("Content-Encoding", "ascii")
        w.Header().Set("X-Content-Type-Options", "scratch and sniff")
        w.Header().Set("Other", "foo")
        Error(w, "oops", 432)
 
        h := w.Header()
-       for _, hdr := range []string{"Content-Length", "Content-Encoding"} {
+       for _, hdr := range []string{"Content-Length"} {
                if v, ok := h[hdr]; ok {
                        t.Errorf("%s: %q, want not present", hdr, v)
                }
index 190f56501379b0eb43af91a2ff7ba8a5b6d3ad8c..a5e98f1d957dcba585d501cf3dcafc833d2e5ee3 100644 (file)
@@ -2226,17 +2226,22 @@ func (f HandlerFunc) ServeHTTP(w ResponseWriter, r *Request) {
 // writes are done to w.
 // The error message should be plain text.
 //
-// Error deletes the Content-Length and Content-Encoding headers,
+// Error deletes the Content-Length header,
 // sets Content-Type to “text/plain; charset=utf-8”,
 // and sets X-Content-Type-Options to “nosniff”.
 // This configures the header properly for the error message,
 // in case the caller had set it up expecting a successful output.
 func Error(w ResponseWriter, error string, code int) {
        h := w.Header()
-       // We delete headers which might be valid for some other content,
-       // but not anymore for the error content.
+
+       // Delete the Content-Length header, which might be for some other content.
+       // Assuming the error string fits in the writer's buffer, we'll figure
+       // out the correct Content-Length for it later.
+       //
+       // We don't delete Content-Encoding, because some middleware sets
+       // Content-Encoding: gzip and wraps the ResponseWriter to compress on-the-fly.
+       // See https://go.dev/issue/66343.
        h.Del("Content-Length")
-       h.Del("Content-Encoding")
 
        // There might be content type already set, but we reset it to
        // text/plain for the error message.
index c1d0ca90727e84d7014c46d7d572bb4364a7a05d..b8be9f8272ba76e9e2bbbc0b37d3e83c40b51312 100644 (file)
@@ -267,6 +267,11 @@ Below is the full list of supported metrics, ordered lexicographically.
                The number of non-default behaviors executed by the net/http
                package due to a non-default GODEBUG=httpmuxgo121=... setting.
 
+       /godebug/non-default-behavior/httpservecontentkeepheaders:events
+               The number of non-default behaviors executed
+               by the net/http package due to a non-default
+               GODEBUG=httpservecontentkeepheaders=... setting.
+
        /godebug/non-default-behavior/installgoroot:events
                The number of non-default behaviors executed by the go/build
                package due to a non-default GODEBUG=installgoroot=... setting.