diff --git a/README.md b/README.md index 099e541..63466fa 100644 --- a/README.md +++ b/README.md @@ -102,6 +102,9 @@ Recoverer is a middleware that recovers from panics, logs the panic (and a backt and returns an HTTP 500 (Internal Server Error) status if possible. It prevents server crashes in case of panic in one of the controllers. +`http.ErrAbortHandler` is re-panicked untouched and neither logged nor turned into a 500, as `net/http` +relies on the sentinel reaching the server to abort the response and close the connection. + ### OnlyFrom middleware OnlyFrom middleware allows access from a limited list of source IPs. @@ -115,12 +118,20 @@ _Note: headers should be trusted and set by a proxy, otherwise it is possible to ### Metrics middleware -Metrics middleware responds to GET /metrics with list of [expvar](https://golang.org/pkg/expvar/). -Optionally allows a restricted list of source ips. +Metrics middleware responds to GET /metrics with list of [expvar](https://golang.org/pkg/expvar/), +limited to a list of source ips, i.e. `rest.Metrics("127.0.0.1", "192.168.0.0/16")`. +Called without any ip, as `rest.Metrics()`, it rejects every request. + +To serve the endpoint to everyone, ask for it explicitly with `rest.MetricsAllowAll()`. Note that expvar +publishes `cmdline`, which usually carries the flag values the process was started with, so only do this +where something else already keeps the endpoint private. ### BlackWords middleware BlackWords middleware doesn't allow user-defined words in the request body. +It reads the whole body to inspect it and responds with `StatusBadRequest` (400) if the body can't be read. +The body is not capped on its own, so put `SizeLimit` in front of it to bound what a request can allocate: +`rest.Wrap(handler, rest.SizeLimit(1024*1024), rest.BlackWords("word1", "word2"))`. ### SizeLimit middleware @@ -192,7 +203,8 @@ Sets headers (passed as key:value) to requests. I.e. `rest.Headers("Server:MySer ### Gzip middleware -Compresses response with gzip. +Compresses response with gzip. Adds `Vary: Accept-Encoding` to every response it handles, compressed or not, +so shared caches key on the encoding rather than serving gzip bytes to a client that never asked for them. ### RealIP middleware @@ -457,7 +469,7 @@ example with chi router: - `realip.Get` - returns client's IP address - `rest.ParseFromTo` - parses "from" and "to" request's query params with various formats - `rest.DecodeJSON` - decodes request body to the provided struct -- `rest.EncodeJSON` - encodes response body from the provided struct, sets `Content-Type` to `application/json` and sends the status code +- `rest.EncodeJSON` - encodes response body from the provided struct, sets `Content-Type` to `application/json` and sends the status code. The value is encoded before anything is written, so an encoding failure leaves the response uncommitted and the caller can still replace it with an error status. Write failures are reported too, by which point the response has already been committed ## Profiler diff --git a/benchmarks_test.go b/benchmarks_test.go index 0c1b90c..ca2e5d3 100644 --- a/benchmarks_test.go +++ b/benchmarks_test.go @@ -108,7 +108,7 @@ func TestBenchmark_WithTimeRange(t *testing.T) { func TestBenchmark_Cleanup(t *testing.T) { bench := NewBenchmarks() - for i := 0; i < 1000; i++ { + for i := range 1000 { bench.nowFn = func() time.Time { return time.Date(2022, 5, 15, 0, 0, 0, 0, time.UTC).Add(time.Duration(i) * time.Second) // every 2s fake time } @@ -142,7 +142,7 @@ func TestBenchmarks_Handler(t *testing.T) { ts := httptest.NewServer(bench.Handler(handler)) defer ts.Close() - for i := 0; i < 100; i++ { + for range 100 { resp, err := ts.Client().Get(ts.URL) require.NoError(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode) @@ -176,7 +176,7 @@ func TestBenchmark_ConcurrentAccess(t *testing.T) { var wg sync.WaitGroup // simulate concurrent updates - for i := 0; i < 100; i++ { + for i := range 100 { wg.Add(1) go func(i int) { defer wg.Done() @@ -185,7 +185,7 @@ func TestBenchmark_ConcurrentAccess(t *testing.T) { } // simulate concurrent stats reads while updating - for i := 0; i < 10; i++ { + for range 10 { wg.Add(1) go func() { defer wg.Done() @@ -229,7 +229,7 @@ func TestBenchmark_TimeWindowBoundaries(t *testing.T) { bench.nowFn = func() time.Time { return now } // add data points exactly at minute boundaries - for i := 0; i < 120; i++ { + for i := range 120 { bench.nowFn = func() time.Time { return now.Add(time.Duration(i) * time.Second) } diff --git a/blackwords.go b/blackwords.go index 8954db3..86ad9d7 100644 --- a/blackwords.go +++ b/blackwords.go @@ -13,16 +13,20 @@ func BlackWords(words ...string) func(http.Handler) http.Handler { return func(h http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { - if content, err := io.ReadAll(r.Body); err == nil { - body := strings.ToLower(string(content)) - r.Body = io.NopCloser(bytes.NewReader(content)) + content, err := io.ReadAll(r.Body) + if err != nil { + // the body can't be inspected, refuse rather than pass a partially consumed one through + _ = EncodeJSON(w, http.StatusBadRequest, JSON{"error": "can't read request body"}) + return + } + r.Body = io.NopCloser(bytes.NewReader(content)) - if body != "" { - for _, word := range words { - if strings.Contains(body, strings.ToLower(word)) { - _ = EncodeJSON(w, http.StatusForbidden, JSON{"error": "one of blacklisted words detected"}) - return - } + body := strings.ToLower(string(content)) + if body != "" { + for _, word := range words { + if strings.Contains(body, strings.ToLower(word)) { + _ = EncodeJSON(w, http.StatusForbidden, JSON{"error": "one of blacklisted words detected"}) + return } } } diff --git a/blackwords_test.go b/blackwords_test.go index 02c6216..ab6ee21 100644 --- a/blackwords_test.go +++ b/blackwords_test.go @@ -2,6 +2,7 @@ package rest import ( "bytes" + "errors" "fmt" "net/http" "net/http/httptest" @@ -34,7 +35,6 @@ func TestBlackwords(t *testing.T) { client := http.Client{Timeout: 5 * time.Second} for n, tt := range tbl { - tt := tt t.Run(fmt.Sprintf("test-%d", n), func(t *testing.T) { req, err := http.NewRequest("GET", u, bytes.NewBuffer([]byte(tt.inp))) assert.Nil(t, err) @@ -71,7 +71,6 @@ func TestBlackwordsFn(t *testing.T) { client := http.Client{Timeout: 5 * time.Second} for n, tt := range tbl { - tt := tt t.Run(fmt.Sprintf("test-%d", n), func(t *testing.T) { req, err := http.NewRequest("GET", u, bytes.NewBuffer([]byte(tt.inp))) assert.Nil(t, err) @@ -98,3 +97,30 @@ func TestBlackwordsContentType(t *testing.T) { assert.Equal(t, http.StatusForbidden, resp.StatusCode) assert.Equal(t, "application/json; charset=utf-8", resp.Header.Get("Content-Type")) } + +// errReader fails partway through, mimicking a truncated or aborted request body +type errReader struct { + data []byte + pos int +} + +func (e *errReader) Read(p []byte) (int, error) { + if e.pos >= len(e.data) { + return 0, errors.New("read failed") + } + n := copy(p, e.data[e.pos:]) + e.pos += n + return n, nil +} + +func TestBlackwordsUnreadableBody(t *testing.T) { + var called bool + handler := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { called = true }) + + req := httptest.NewRequest("POST", "/", &errReader{data: []byte("badword and more")}) + w := httptest.NewRecorder() + BlackWords("badword")(handler).ServeHTTP(w, req) + + assert.Equal(t, http.StatusBadRequest, w.Code) + assert.False(t, called, "handler must not run on a body that can't be inspected") +} diff --git a/file_server.go b/file_server.go index 088ac9d..d437ffb 100644 --- a/file_server.go +++ b/file_server.go @@ -116,7 +116,9 @@ type customFS struct { listing bool } -// Open file on FS, for directory enforce index.html and fail on a missing index +// Open file on FS, for directory enforce index.html and fail on a missing index. +// Every handle opened here is either returned to the caller or closed, as http.FileServer +// closes only the file it gets back. func (cfs customFS) Open(name string) (http.File, error) { f, err := cfs.fs.Open(name) @@ -129,19 +131,21 @@ func (cfs customFS) Open(name string) (http.File, error) { finfo, err := f.Stat() if err != nil { + _ = f.Close() return nil, err } if finfo.IsDir() { index := strings.TrimSuffix(name, "/") + "/index.html" - if _, err := cfs.fs.Open(index); err == nil { // index.html will be served if found + indexFile, ierr := cfs.fs.Open(index) + if ierr == nil { // index.html will be served if found + _ = indexFile.Close() // opened to probe for existence only, http.FileServer opens it again on its own return f, nil } // no index.html in directory if !cfs.listing { // listing disabled - if _, err := cfs.fs.Open(index); err != nil { - return nil, err - } + _ = f.Close() + return nil, ierr } } diff --git a/file_server_test.go b/file_server_test.go index 7a10a22..b898a0e 100644 --- a/file_server_test.go +++ b/file_server_test.go @@ -2,11 +2,14 @@ package rest import ( "bytes" + "errors" "io" "net/http" "net/http/httptest" + "os" "strconv" "strings" + "sync" "testing" "time" @@ -54,7 +57,6 @@ func TestFileServerDefault(t *testing.T) { } for i, tt := range tbl { - tt := tt t.Run(strconv.Itoa(i), func(t *testing.T) { for _, ts := range []*httptest.Server{ts1, ts2} { req, err := http.NewRequest("GET", ts.URL+tt.req, http.NoBody) @@ -216,7 +218,6 @@ func TestFileServerSPA(t *testing.T) { } for i, tt := range tbl { - tt := tt t.Run(strconv.Itoa(i), func(t *testing.T) { for _, ts := range []*httptest.Server{ts1, ts2} { req, err := http.NewRequest("GET", ts.URL+tt.req, http.NoBody) @@ -238,3 +239,112 @@ func TestFileServerSPA(t *testing.T) { }) } } + +// trackingFS counts opened and closed handles to catch descriptors customFS forgets to release +type trackingFS struct { + fs http.FileSystem + statFail bool + + mu sync.Mutex + opened int + closed int +} + +func (t *trackingFS) Open(name string) (http.File, error) { + f, err := t.fs.Open(name) + if err != nil { + return nil, err + } + t.mu.Lock() + t.opened++ + t.mu.Unlock() + return &trackingFile{File: f, fs: t, statFail: t.statFail}, nil +} + +func (t *trackingFS) counts() (opened, closed int) { + t.mu.Lock() + defer t.mu.Unlock() + return t.opened, t.closed +} + +type trackingFile struct { + http.File + fs *trackingFS + statFail bool + once sync.Once +} + +func (f *trackingFile) Stat() (os.FileInfo, error) { + if f.statFail { + return nil, errors.New("stat failed") + } + return f.File.Stat() +} + +func (f *trackingFile) Close() error { + f.once.Do(func() { + f.fs.mu.Lock() + f.fs.closed++ + f.fs.mu.Unlock() + }) + return f.File.Close() +} + +func TestCustomFSHandles(t *testing.T) { + tbl := []struct { + name string + path string + listing bool + spa bool + statFail bool + wantErr bool + }{ + {name: "dir with index", path: "/", wantErr: false}, + {name: "dir with index, nested", path: "/2", wantErr: false}, + {name: "dir without index, listing disabled", path: "/1", wantErr: true}, + {name: "dir without index, listing enabled", path: "/1", listing: true, wantErr: false}, + {name: "regular file", path: "/xyz.js", wantErr: false}, + {name: "missing file", path: "/nope.js", wantErr: true}, + {name: "missing file, spa", path: "/nope.js", spa: true, wantErr: false}, + {name: "stat failure", path: "/", statFail: true, wantErr: true}, + } + + for _, tt := range tbl { + t.Run(tt.name, func(t *testing.T) { + tfs := &trackingFS{fs: http.Dir("./testdata/root"), statFail: tt.statFail} + cfs := customFS{fs: tfs, spa: tt.spa, listing: tt.listing} + + f, err := cfs.Open(tt.path) + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, f) + opened, closed := tfs.counts() + assert.Equal(t, opened, closed, "every opened handle must be closed on the error path") + return + } + + require.NoError(t, err) + require.NotNil(t, f) + require.NoError(t, f.Close()) + + opened, closed := tfs.counts() + assert.Equal(t, opened, closed, "probe handles must be closed once the caller closes the result") + }) + } +} + +func TestFileServerNoDescriptorLeak(t *testing.T) { + tfs := &trackingFS{fs: http.Dir("./testdata/root")} + cfs := customFS{fs: tfs} + + // repeated directory requests are what accumulated descriptors before + for range 50 { + f, err := cfs.Open("/") + require.NoError(t, err) + require.NoError(t, f.Close()) + } + + opened, closed := tfs.counts() + assert.Equal(t, opened, closed) + assert.Equal(t, 100, opened, "each directory request opens the dir plus one index probe") +} diff --git a/go.mod b/go.mod index 412bc09..065c5f6 100644 --- a/go.mod +++ b/go.mod @@ -3,13 +3,11 @@ module github.com/go-pkgz/rest go 1.24.0 require ( - github.com/stretchr/testify v1.10.0 - golang.org/x/crypto v0.46.0 + github.com/stretchr/testify v1.12.0 + golang.org/x/crypto v0.48.0 ) require ( - github.com/davecgh/go-spew v1.1.1 // indirect - github.com/pmezard/go-difflib v1.0.0 // indirect - golang.org/x/sys v0.39.0 // indirect + golang.org/x/sys v0.41.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index 9f894fb..1a18184 100644 --- a/go.sum +++ b/go.sum @@ -1,13 +1,9 @@ -github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= -github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= -github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= -github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA= -github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= -golang.org/x/crypto v0.46.0 h1:cKRW/pmt1pKAfetfu+RCEvjvZkA9RimPbh7bhFjGVBU= -golang.org/x/crypto v0.46.0/go.mod h1:Evb/oLKmMraqjZ2iQTwDwvCtJkczlDuTmdJXoZVzqU0= -golang.org/x/sys v0.39.0 h1:CvCKL8MeisomCi6qNZ+wbb0DN9E5AATixKsvNtMoMFk= -golang.org/x/sys v0.39.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= +github.com/stretchr/testify v1.12.0 h1:K6Mr6jO9JICuend/5xzTM03ydSV3vdNRYAdPSukj8uI= +github.com/stretchr/testify v1.12.0/go.mod h1:bOYBZb5qJ00vPzWfIqBUZPaxK8jWiXc6d3ErP4Ca9Gw= +golang.org/x/crypto v0.48.0 h1:/VRzVqiRSggnhY7gNRxPauEQ5Drw9haKdM0jqfcCFts= +golang.org/x/crypto v0.48.0/go.mod h1:r0kV5h3qnFPlQnBSrULhlsRfryS2pmewsg+XfMgkVos= +golang.org/x/sys v0.41.0 h1:Ivj+2Cp/ylzLiEU89QhWblYnOE9zerudt9Ftecq2C6k= +golang.org/x/sys v0.41.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/gzip.go b/gzip.go index 67cac26..0a84cf7 100644 --- a/gzip.go +++ b/gzip.go @@ -55,6 +55,9 @@ func Gzip(contentTypes ...string) func(http.Handler) http.Handler { f := func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // the representation depends on Accept-Encoding, caches must key on it even when not compressing + w.Header().Add("Vary", "Accept-Encoding") + if !strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { next.ServeHTTP(w, r) return diff --git a/gzip_test.go b/gzip_test.go index 40a07bb..7226edd 100644 --- a/gzip_test.go +++ b/gzip_test.go @@ -64,6 +64,50 @@ func TestGzipCustom(t *testing.T) { } +func TestGzipVary(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, err := w.Write([]byte(strings.Repeat("compress me. ", 50))) + require.NoError(t, err) + }) + ts := httptest.NewServer(Gzip()(handler)) + defer ts.Close() + + tbl := []struct { + name string + acceptEncoding string + encoded bool + }{ + {"gzip accepted", "gzip", true}, + {"gzip not accepted", "", false}, + {"other encoding", "br", false}, + } + + for _, tt := range tbl { + t.Run(tt.name, func(t *testing.T) { + req, err := http.NewRequest("GET", ts.URL+"/something", http.NoBody) + require.NoError(t, err) + req.Header.Set("Content-Type", "text/plain") + if tt.acceptEncoding != "" { + req.Header.Set("Accept-Encoding", tt.acceptEncoding) + } else { + req.Header.Set("Accept-Encoding", "identity") + } + + resp, err := http.DefaultTransport.RoundTrip(req) + require.NoError(t, err) + defer resp.Body.Close() + + // caches must be told the body varies by Accept-Encoding whether or not it got compressed + assert.Contains(t, resp.Header.Values("Vary"), "Accept-Encoding") + if tt.encoded { + assert.Equal(t, "gzip", resp.Header.Get("Content-Encoding")) + return + } + assert.Empty(t, resp.Header.Get("Content-Encoding")) + }) + } +} + func TestGzipWriteHeader(t *testing.T) { // test that explicit WriteHeader call works with gzip middleware longText := strings.Repeat("This is a test message for gzip compression. ", 20) diff --git a/logger/logger_test.go b/logger/logger_test.go index 8ee6e65..2442368 100644 --- a/logger/logger_test.go +++ b/logger/logger_test.go @@ -283,7 +283,7 @@ type mockLgr struct { buf bytes.Buffer } -func (m *mockLgr) Logf(format string, args ...interface{}) { +func (m *mockLgr) Logf(format string, args ...any) { _, _ = m.buf.WriteString(fmt.Sprintf(format, args...)) } @@ -528,8 +528,6 @@ func TestSanitizeReqURL(t *testing.T) { } var l *Middleware for i, tt := range tbl { - i := i - tt := tt t.Run(tt.in, func(t *testing.T) { assert.Equal(t, tt.out, unesc(l.sanitizeQuery(tt.in)), "check #%d, %s", i, tt.in) }) diff --git a/metrics.go b/metrics.go index 6edc2da..6874ed6 100644 --- a/metrics.go +++ b/metrics.go @@ -7,14 +7,29 @@ import ( "strings" ) -// Metrics responds to GET /metrics with list of expvar +// Metrics responds to GET /metrics with list of expvar, limited to the given source ips. +// Called without any ip it rejects every request, as an endpoint nobody can reach is the safe +// default for one that publishes expvar; use MetricsAllowAll to serve it to everyone on purpose. func Metrics(onlyIps ...string) func(http.Handler) http.Handler { + return metricsHandler(false, onlyIps) +} + +// MetricsAllowAll responds to GET /metrics with list of expvar for any source, without any ip check. +// expvar exposes cmdline, which usually carries the flag values the process was started with, so +// only use this where something else already keeps the endpoint private. +func MetricsAllowAll() func(http.Handler) http.Handler { + return metricsHandler(true, nil) +} + +func metricsHandler(allowAll bool, onlyIps []string) func(http.Handler) http.Handler { return func(h http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { if r.Method == "GET" && strings.HasSuffix(strings.ToLower(r.URL.Path), "/metrics") { - if matched, ip, err := matchSourceIP(r, onlyIps); !matched || err != nil { - _ = EncodeJSON(w, http.StatusForbidden, JSON{"error": fmt.Sprintf("ip %s rejected", ip)}) - return + if !allowAll { + if matched, ip, err := matchSourceIP(r, onlyIps); !matched || err != nil { + _ = EncodeJSON(w, http.StatusForbidden, JSON{"error": fmt.Sprintf("ip %s rejected", ip)}) + return + } } expvar.Handler().ServeHTTP(w, r) return diff --git a/metrics_test.go b/metrics_test.go index 09a44da..3da1c3f 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -30,6 +30,68 @@ func TestMetrics(t *testing.T) { assert.True(t, strings.Contains(string(b), "memstats")) } +func TestMetrics_EmptyList(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, err := w.Write([]byte("blah blah")) + require.NoError(t, err) + }) + + // no ips means nobody is allowed, expvar must stay unreachable + ts := httptest.NewServer(Metrics()(handler)) + defer ts.Close() + + req, err := http.NewRequest("GET", ts.URL+"/metrics", http.NoBody) + require.NoError(t, err) + req.Header.Set("X-Real-IP", "1.2.3.4") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + assert.Equal(t, http.StatusForbidden, resp.StatusCode) +} + +func TestMetricsAllowAll(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, err := w.Write([]byte("blah blah")) + require.NoError(t, err) + }) + + ts := httptest.NewServer(MetricsAllowAll()(handler)) + defer ts.Close() + + req, err := http.NewRequest("GET", ts.URL+"/metrics", http.NoBody) + require.NoError(t, err) + req.Header.Set("X-Real-IP", "1.2.3.4") // any source is served once opted in + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Contains(t, string(b), "cmdline") + assert.Contains(t, string(b), "memstats") +} + +func TestMetricsAllowAll_NonMetricsPath(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, err := w.Write([]byte("other path")) + require.NoError(t, err) + }) + ts := httptest.NewServer(MetricsAllowAll()(handler)) + defer ts.Close() + + resp, err := http.Get(ts.URL + "/other") + require.NoError(t, err) + defer resp.Body.Close() + assert.Equal(t, http.StatusOK, resp.StatusCode) + + b, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Equal(t, "other path", string(b)) +} + func TestMetricsRejected(t *testing.T) { handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, err := w.Write([]byte("blah blah")) diff --git a/middleware.go b/middleware.go index 3050507..6a5e699 100644 --- a/middleware.go +++ b/middleware.go @@ -96,15 +96,18 @@ func Health(path string, checkers ...func(ctx context.Context) (name string, err } // Recoverer is a middleware that recovers from panics, logs the panic and returns a HTTP 500 status if possible. +// http.ErrAbortHandler is passed through untouched, as net/http relies on it reaching the server to abort +// the response and close the connection. func Recoverer(l logger.Backend) func(http.Handler) http.Handler { return func(h http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { defer func() { if rvr := recover(); rvr != nil { - l.Logf("request panic for %s from %s, %v", r.URL.String(), r.RemoteAddr, rvr) - if rvr != http.ErrAbortHandler { - l.Logf(string(debug.Stack())) + if rvr == http.ErrAbortHandler { + panic(rvr) } + l.Logf("request panic for %s from %s, %v", r.URL.String(), r.RemoteAddr, rvr) + l.Logf(string(debug.Stack())) http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) } }() diff --git a/middleware_test.go b/middleware_test.go index 18f9ef7..99edce9 100644 --- a/middleware_test.go +++ b/middleware_test.go @@ -133,6 +133,32 @@ func TestMiddleware_Recoverer(t *testing.T) { assert.Equal(t, "blah blah", string(b)) } +func TestMiddleware_RecovererAbortHandler(t *testing.T) { + handler := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + panic(http.ErrAbortHandler) + }) + l := &mockLgr{} + + t.Run("propagated to the caller", func(t *testing.T) { + req := httptest.NewRequest("GET", "/failed", http.NoBody) + w := httptest.NewRecorder() + assert.PanicsWithValue(t, http.ErrAbortHandler, func() { + Recoverer(l)(handler).ServeHTTP(w, req) + }) + assert.Empty(t, l.buf.String(), "abort sentinel should not be logged") + assert.Empty(t, w.Body.String(), "no error body should be written") + }) + + t.Run("connection aborted without response", func(t *testing.T) { + // net/http recovers the sentinel itself and closes the connection without a reply + ts := httptest.NewServer(Recoverer(l)(handler)) + defer ts.Close() + + _, err := http.Get(ts.URL + "/failed") + require.Error(t, err, "server must drop the connection instead of answering 500") + }) +} + func TestWrap(t *testing.T) { handler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { t.Logf("%s", r.URL.String()) @@ -378,6 +404,6 @@ type mockLgr struct { buf bytes.Buffer } -func (m *mockLgr) Logf(format string, args ...interface{}) { +func (m *mockLgr) Logf(format string, args ...any) { _, _ = m.buf.WriteString(fmt.Sprintf(format+"\n", args...)) } diff --git a/rest.go b/rest.go index 94b3e9d..b4ed629 100644 --- a/rest.go +++ b/rest.go @@ -105,12 +105,19 @@ func DecodeJSON[T any](r *http.Request, res *T) error { return nil } -// EncodeJSON encodes given type to http.ResponseWriter and sets status code and content type header +// EncodeJSON encodes given type to http.ResponseWriter and sets status code and content type header. +// The value is encoded before anything is written, so an encoding failure leaves the response +// uncommitted and the caller is free to replace it with an error status. Write failures are reported +// as well, by which point the response has already been committed. func EncodeJSON[T any](w http.ResponseWriter, status int, v T) error { + buf := &bytes.Buffer{} + if err := json.NewEncoder(buf).Encode(v); err != nil { + return fmt.Errorf("encode json: %w", err) + } w.Header().Set("Content-Type", "application/json; charset=utf-8") w.WriteHeader(status) - if err := json.NewEncoder(w).Encode(v); err != nil { - return fmt.Errorf("encode json: %w", err) + if _, err := w.Write(buf.Bytes()); err != nil { + return fmt.Errorf("write json: %w", err) } return nil } diff --git a/rest_test.go b/rest_test.go index 2a2956e..2c45dd0 100644 --- a/rest_test.go +++ b/rest_test.go @@ -209,6 +209,12 @@ func TestEncodeJSON_EncodingError(t *testing.T) { // channels cannot be encoded to JSON err := EncodeJSON(w, http.StatusOK, make(chan int)) assert.Error(t, err) + + // nothing was committed, so the caller can still answer with an error + assert.Empty(t, w.Body.String()) + assert.Empty(t, w.Header().Get("Content-Type")) + http.Error(w, "oops", http.StatusInternalServerError) + assert.Equal(t, http.StatusInternalServerError, w.Code) } func getTestHandlerBlah() http.HandlerFunc { @@ -217,3 +223,20 @@ func getTestHandlerBlah() http.HandlerFunc { } return fn } + +// failingWriter fails on Write, mimicking a client that disconnected mid-response +type failingWriter struct { + http.ResponseWriter + err error +} + +func (f *failingWriter) Write([]byte) (int, error) { return 0, f.err } + +func TestEncodeJSON_WriteError(t *testing.T) { + wantErr := errors.New("connection reset") + w := &failingWriter{ResponseWriter: httptest.NewRecorder(), err: wantErr} + + err := EncodeJSON(w, http.StatusOK, JSON{"key": "value"}) + require.Error(t, err, "a failed write has to reach the caller") + assert.ErrorIs(t, err, wantErr) +} diff --git a/sizelimit_test.go b/sizelimit_test.go index 252e622..02ceea1 100644 --- a/sizelimit_test.go +++ b/sizelimit_test.go @@ -41,10 +41,7 @@ func TestSizeLimit(t *testing.T) { defer ts.Close() for i, tt := range tbl { - i := i - tt := tt for _, wrap := range []bool{false, true} { - wrap := wrap t.Run(fmt.Sprintf("test-%d/%v", i, wrap), func(t *testing.T) { client := http.Client{Timeout: 1 * time.Second} var reader io.Reader = strings.NewReader(tt.body) diff --git a/throttle_test.go b/throttle_test.go index 1f41828..1f73c4c 100644 --- a/throttle_test.go +++ b/throttle_test.go @@ -26,7 +26,7 @@ func TestThrottle(t *testing.T) { var wg sync.WaitGroup wg.Add(100) - for i := 0; i < 100; i++ { + for range 100 { go func() { defer wg.Done() resp, err := http.Get(ts.URL) @@ -71,7 +71,7 @@ func TestThrottleDisabled(t *testing.T) { var wg sync.WaitGroup wg.Add(100) - for i := 0; i < 100; i++ { + for range 100 { go func() { defer wg.Done() resp, err := http.Get(ts.URL)