]> Cypherpunks repositories - gostls13.git/commitdiff
net/http: remove misleading response headers on error
authorRuss Cox <rsc@golang.org>
Fri, 15 Mar 2024 16:56:23 +0000 (12:56 -0400)
committerRuss Cox <rsc@golang.org>
Thu, 9 May 2024 17:06:47 +0000 (17:06 +0000)
This is a reapply of CL 544019 and CL 569815, but with
less aggressive semantics as discussed in proposal #66343.

Error deletes Content-Encoding, since it is writing the response
and any preset encoding may not be correct.

On the error-serving path in ServeContent/ServeFile/ServeFS,
these functions delete additional headers: Etag, Last-Modified,
and Cache-Control. The caller may have set these intending
them for the success response, and they may well not be correct
for error responses.

Fixes #50905.
Fixes #66343.

Change-Id: I873d33edde1805990ca16d85ea8d7735b7448626
Reviewed-on: https://go-review.googlesource.com/c/go/+/571995
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/net/http/fs.go
src/net/http/fs_test.go
src/net/http/serve_test.go
src/net/http/server.go

index 25e9406a581fc587382c342913c09d58957dcf18..c213d8a328564c3dc13284b73c9367c306af0809 100644 (file)
@@ -171,6 +171,18 @@ func dirList(w ResponseWriter, r *Request, f File) {
        fmt.Fprintf(w, "</pre>\n")
 }
 
+// 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")
+       Error(w, text, code)
+}
+
 // ServeContent replies to the request using the content in the
 // provided ReadSeeker. The main benefit of ServeContent over [io.Copy]
 // is that it handles Range requests properly, sets the MIME type, and
@@ -247,7 +259,7 @@ func serveContent(w ResponseWriter, r *Request, name string, modtime time.Time,
                        ctype = DetectContentType(buf[:n])
                        _, err := content.Seek(0, io.SeekStart) // rewind to output whole file
                        if err != nil {
-                               Error(w, "seeker can't seek", StatusInternalServerError)
+                               serveError(w, "seeker can't seek", StatusInternalServerError)
                                return
                        }
                }
@@ -258,12 +270,12 @@ func serveContent(w ResponseWriter, r *Request, name string, modtime time.Time,
 
        size, err := sizeFunc()
        if err != nil {
-               Error(w, err.Error(), StatusInternalServerError)
+               serveError(w, err.Error(), StatusInternalServerError)
                return
        }
        if size < 0 {
                // Should never happen but just to be sure
-               Error(w, "negative content size computed", StatusInternalServerError)
+               serveError(w, "negative content size computed", StatusInternalServerError)
                return
        }
 
@@ -285,7 +297,7 @@ func serveContent(w ResponseWriter, r *Request, name string, modtime time.Time,
                w.Header().Set("Content-Range", fmt.Sprintf("bytes */%d", size))
                fallthrough
        default:
-               Error(w, err.Error(), StatusRequestedRangeNotSatisfiable)
+               serveError(w, err.Error(), StatusRequestedRangeNotSatisfiable)
                return
        }
 
@@ -311,7 +323,7 @@ func serveContent(w ResponseWriter, r *Request, name string, modtime time.Time,
                // multipart responses."
                ra := ranges[0]
                if _, err := content.Seek(ra.start, io.SeekStart); err != nil {
-                       Error(w, err.Error(), StatusRequestedRangeNotSatisfiable)
+                       serveError(w, err.Error(), StatusRequestedRangeNotSatisfiable)
                        return
                }
                sendSize = ra.length
@@ -644,7 +656,7 @@ func serveFile(w ResponseWriter, r *Request, fs FileSystem, name string, redirec
        f, err := fs.Open(name)
        if err != nil {
                msg, code := toHTTPError(err)
-               Error(w, msg, code)
+               serveError(w, msg, code)
                return
        }
        defer f.Close()
@@ -652,7 +664,7 @@ func serveFile(w ResponseWriter, r *Request, fs FileSystem, name string, redirec
        d, err := f.Stat()
        if err != nil {
                msg, code := toHTTPError(err)
-               Error(w, msg, code)
+               serveError(w, msg, code)
                return
        }
 
@@ -670,7 +682,7 @@ func serveFile(w ResponseWriter, r *Request, fs FileSystem, name string, redirec
                        if base == "/" || base == "." {
                                // The FileSystem maps a path like "/" or "/./" to a file instead of a directory.
                                msg := "http: attempting to traverse a non-directory"
-                               Error(w, msg, StatusInternalServerError)
+                               serveError(w, msg, StatusInternalServerError)
                                return
                        }
                        localRedirect(w, r, "../"+base)
@@ -769,7 +781,7 @@ func ServeFile(w ResponseWriter, r *Request, name string) {
                // here and ".." may not be wanted.
                // Note that name might not contain "..", for example if code (still
                // incorrectly) used filepath.Join(myDir, r.URL.Path).
-               Error(w, "invalid URL path", StatusBadRequest)
+               serveError(w, "invalid URL path", StatusBadRequest)
                return
        }
        dir, file := filepath.Split(name)
@@ -802,7 +814,7 @@ func ServeFileFS(w ResponseWriter, r *Request, fsys fs.FS, name string) {
                // here and ".." may not be wanted.
                // Note that name might not contain "..", for example if code (still
                // incorrectly) used filepath.Join(myDir, r.URL.Path).
-               Error(w, "invalid URL path", StatusBadRequest)
+               serveError(w, "invalid URL path", StatusBadRequest)
                return
        }
        serveFile(w, r, FS(fsys), name, false)
index 70a4b8982f4fe454461521c30e3ef3ddcbfbaa36..63278d890f78daef2b513c11d184cb5b65bd4757 100644 (file)
@@ -27,6 +27,7 @@ import (
        "reflect"
        "regexp"
        "runtime"
+       "strconv"
        "strings"
        "testing"
        "testing/fstest"
@@ -1222,8 +1223,8 @@ type issue12991File struct{ File }
 func (issue12991File) Stat() (fs.FileInfo, error) { return nil, fs.ErrPermission }
 func (issue12991File) Close() error               { return nil }
 
-func TestServeContentErrorMessages(t *testing.T) { run(t, testServeContentErrorMessages) }
-func testServeContentErrorMessages(t *testing.T, mode testMode) {
+func TestFileServerErrorMessages(t *testing.T) { run(t, testFileServerErrorMessages) }
+func testFileServerErrorMessages(t *testing.T, mode testMode) {
        fs := fakeFS{
                "/500": &fakeFileInfo{
                        err: errors.New("random error"),
@@ -1232,7 +1233,15 @@ func testServeContentErrorMessages(t *testing.T, mode testMode) {
                        err: &fs.PathError{Err: fs.ErrPermission},
                },
        }
-       ts := newClientServerTest(t, mode, FileServer(fs)).ts
+       server := FileServer(fs)
+       h := func(w http.ResponseWriter, r *http.Request) {
+               w.Header().Set("Etag", "étude")
+               w.Header().Set("Cache-Control", "yes")
+               w.Header().Set("Content-Type", "awesome")
+               w.Header().Set("Last-Modified", "yesterday")
+               server.ServeHTTP(w, r)
+       }
+       ts := newClientServerTest(t, mode, http.HandlerFunc(h)).ts
        c := ts.Client()
        for _, code := range []int{403, 404, 500} {
                res, err := c.Get(fmt.Sprintf("%s/%d", ts.URL, code))
@@ -1240,10 +1249,15 @@ func testServeContentErrorMessages(t *testing.T, mode testMode) {
                        t.Errorf("Error fetching /%d: %v", code, err)
                        continue
                }
+               res.Body.Close()
                if res.StatusCode != code {
-                       t.Errorf("For /%d, status code = %d; want %d", code, res.StatusCode, code)
+                       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)
+                       }
                }
-               res.Body.Close()
        }
 }
 
@@ -1694,3 +1708,64 @@ func testFileServerDirWithRootFile(t *testing.T, mode testMode) {
                testDirFile(t, FileServerFS(os.DirFS("testdata/index.html")))
        })
 }
+
+func TestServeContentHeadersWithError(t *testing.T) {
+       contents := []byte("content")
+       ts := newClientServerTest(t, http1Mode, HandlerFunc(func(w ResponseWriter, r *Request) {
+               w.Header().Set("Content-Type", "application/octet-stream")
+               w.Header().Set("Content-Length", strconv.Itoa(len(contents)))
+               w.Header().Set("Content-Encoding", "gzip")
+               w.Header().Set("Etag", `"abcdefgh"`)
+               w.Header().Set("Last-Modified", "Wed, 21 Oct 2015 07:28:00 GMT")
+               w.Header().Set("Cache-Control", "immutable")
+               w.Header().Set("Other-Header", "test")
+               ServeContent(w, r, "", time.Time{}, bytes.NewReader(contents))
+       })).ts
+       defer ts.Close()
+
+       req, err := NewRequest("GET", ts.URL, nil)
+       if err != nil {
+               t.Fatal(err)
+       }
+       req.Header.Set("Range", "bytes=100-10000")
+
+       c := ts.Client()
+       res, err := c.Do(req)
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       out, _ := io.ReadAll(res.Body)
+       res.Body.Close()
+
+       if g, e := res.StatusCode, 416; g != e {
+               t.Errorf("got status = %d; want %d", g, e)
+       }
+       if g, e := string(out), "invalid range: failed to overlap\n"; g != e {
+               t.Errorf("got body = %q; want %q", g, e)
+       }
+       if g, e := res.Header.Get("Content-Type"), "text/plain; charset=utf-8"; g != e {
+               t.Errorf("got content-type = %q, want %q", g, e)
+       }
+       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 {
+               t.Errorf("got content-encoding = %q, want %q", g, e)
+       }
+       if g, e := res.Header.Get("Etag"), ""; g != e {
+               t.Errorf("got etag = %q, want %q", g, e)
+       }
+       if g, e := res.Header.Get("Last-Modified"), ""; g != e {
+               t.Errorf("got last-modified = %q, want %q", g, e)
+       }
+       if g, e := res.Header.Get("Cache-Control"), ""; g != e {
+               t.Errorf("got cache-control = %q, want %q", g, e)
+       }
+       if g, e := res.Header.Get("Content-Range"), "bytes */7"; g != e {
+               t.Errorf("got content-range = %q, want %q", g, e)
+       }
+       if g, e := res.Header.Get("Other-Header"), "test"; g != e {
+               t.Errorf("got other-header = %q, want %q", g, e)
+       }
+}
index c03157e814ec11a24ddb38d9ea22b26335dd96fb..e21af8b1595612b2f42164eb38db7669e70e0e3e 100644 (file)
@@ -7144,3 +7144,25 @@ func testErrorContentLength(t *testing.T, mode testMode) {
                t.Fatalf("read body: %q, want %q", string(body), errorBody)
        }
 }
+
+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"} {
+               if v, ok := h[hdr]; ok {
+                       t.Errorf("%s: %q, want not present", hdr, v)
+               }
+       }
+       if v := h.Get("Content-Type"); v != "text/plain; charset=utf-8" {
+               t.Errorf("Content-Type: %q, want %q", v, "text/plain; charset=utf-8")
+       }
+       if v := h.Get("X-Content-Type-Options"); v != "nosniff" {
+               t.Errorf("X-Content-Type-Options: %q, want %q", v, "nosniff")
+       }
+}
index cd0303b5b90368712afb0dc44c40cc00e5182d77..a50b20b7da905b2ef9dc497000de382ee4be3492 100644 (file)
@@ -2175,10 +2175,23 @@ func (f HandlerFunc) ServeHTTP(w ResponseWriter, r *Request) {
 // It does not otherwise end the request; the caller should ensure no further
 // writes are done to w.
 // The error message should be plain text.
+//
+// Error deletes the Content-Length and Content-Encoding headers,
+// 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) {
-       w.Header().Del("Content-Length")
-       w.Header().Set("Content-Type", "text/plain; charset=utf-8")
-       w.Header().Set("X-Content-Type-Options", "nosniff")
+       h := w.Header()
+       // We delete headers which might be valid for some other content,
+       // but not anymore for the error content.
+       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.
+       h.Set("Content-Type", "text/plain; charset=utf-8")
+       h.Set("X-Content-Type-Options", "nosniff")
        w.WriteHeader(code)
        fmt.Fprintln(w, error)
 }