From cd24f7aa8c989965edfc4d8825e34662bee0e2b2 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 22:22:09 +0100 Subject: [PATCH 01/10] fix: serve metrics unrestricted when no ips defined Previously, Metrics() without arguments passed an empty list to matchSourceIP, which returns false for every request, so the middleware answered 403 to all callers instead of serving expvar as documented. --- metrics.go | 8 +++++--- metrics_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/metrics.go b/metrics.go index 6edc2da..c9c0821 100644 --- a/metrics.go +++ b/metrics.go @@ -12,9 +12,11 @@ func Metrics(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 len(onlyIps) > 0 { // no restrictions if no ips defined + 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..6a94420 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -30,6 +30,31 @@ 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) + }) + + // empty list means no ip restriction, expvar has to be served + 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") // any source is fine when unrestricted + + 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 TestMetricsRejected(t *testing.T) { handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, err := w.Write([]byte("blah blah")) From 21250a0fed8a1a3a68ad61cf43604c21823af590 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 22:22:09 +0100 Subject: [PATCH 02/10] fix: re-panic http.ErrAbortHandler in Recoverer Previously, the sentinel was caught like any other panic and turned into a 500, so net/http never learned it had to abort the response and close the connection. Only the stack trace was suppressed for it. --- middleware.go | 9 ++++++--- middleware_test.go | 28 +++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 4 deletions(-) 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...)) } From 9e263a44f75f98897e0481be6c763190f08e7b04 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 22:22:09 +0100 Subject: [PATCH 03/10] fix: encode json before committing the status in EncodeJSON Previously, the status was written before encoding started, so a value that fails to marshal left the caller with an error after a 2xx was already on the wire and no way to replace it with a 500. Encoding into a buffer first makes a returned error mean nothing was written. --- rest.go | 12 ++++++++---- rest_test.go | 6 ++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/rest.go b/rest.go index 94b3e9d..3fe31b1 100644 --- a/rest.go +++ b/rest.go @@ -105,12 +105,16 @@ 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. +// Encoding happens before anything is written, so a returned error guarantees the response is still +// uncommitted and the caller is free to replace it with an error status. func EncodeJSON[T any](w http.ResponseWriter, status int, v T) error { - w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.WriteHeader(status) - if err := json.NewEncoder(w).Encode(v); err != nil { + 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) + _, _ = w.Write(buf.Bytes()) return nil } diff --git a/rest_test.go b/rest_test.go index 2a2956e..301c047 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 { From 3a48be33214854163662ef65fcf88f5ba5a977c8 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 22:22:09 +0100 Subject: [PATCH 04/10] fix: close file handles opened while probing directories Previously, customFS.Open discarded the handle used to probe for index.html and returned without closing the directory on the Stat and missing-index error paths. http.FileServer closes only the file it receives, so each directory request leaked a descriptor until the finalizer ran. --- file_server.go | 14 ++++-- file_server_test.go | 114 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 121 insertions(+), 7 deletions(-) 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") +} From f20dcf6372682ee5e278ce7ec318b07b87a71081 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 22:22:09 +0100 Subject: [PATCH 05/10] fix: add Vary Accept-Encoding to gzip responses The middleware changes the representation based on Accept-Encoding but never told caches about it, so a shared cache could hand gzip bytes to a client that asked for none. --- gzip.go | 3 +++ gzip_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) 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) From 3398bcabb16691a7798d28b41f5a1acc94a56a13 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 22:36:42 +0100 Subject: [PATCH 06/10] fix: refuse requests whose body can't be inspected in BlackWords Previously, a failing io.ReadAll skipped the word check entirely and passed a partially consumed body to the handler, so a truncated or aborted request went through unchecked. Such a request now gets a 400 and never reaches the handler. --- blackwords.go | 22 +++++++++++++--------- blackwords_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 9 deletions(-) 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..d755763 100644 --- a/blackwords_test.go +++ b/blackwords_test.go @@ -2,6 +2,7 @@ package rest import ( "bytes" + "errors" "fmt" "net/http" "net/http/httptest" @@ -98,3 +99,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") +} From d3a9e3fe307389220f0ad56a5e7603af9cc04c93 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 22:35:42 +0100 Subject: [PATCH 07/10] chore: update modules, apply go fix x/crypto stays at v0.48.0, the last release that still builds with the go 1.24.0 declared in go.mod. --- benchmarks_test.go | 10 +++++----- blackwords_test.go | 2 -- go.mod | 8 +++----- go.sum | 16 ++++++---------- logger/logger_test.go | 4 +--- sizelimit_test.go | 3 --- throttle_test.go | 4 ++-- 7 files changed, 17 insertions(+), 30 deletions(-) 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_test.go b/blackwords_test.go index d755763..ab6ee21 100644 --- a/blackwords_test.go +++ b/blackwords_test.go @@ -35,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) @@ -72,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) 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/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/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) From 92f71765e4aff6bac10d9d904f29d78607955bcb Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 22:51:39 +0100 Subject: [PATCH 08/10] docs: describe the Metrics, Recoverer, Gzip, BlackWords and EncodeJSON changes --- README.md | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 099e541..1171131 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. @@ -116,11 +119,15 @@ _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. +Optionally allows a restricted 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 serves every source. ### 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 +199,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 +465,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. Encoding runs before anything is written, so a returned error leaves the response uncommitted and the caller can still replace it with an error status ## Profiler From 9ee60a76ec039320c4665e5b364b4f7601c3e751 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Tue, 18 Aug 2026 23:25:56 +0100 Subject: [PATCH 09/10] fix: report write failures from EncodeJSON Buffering the encode moved the write behind a discarded error, so a client disconnect or short write left the caller with a nil error where json.Encoder.Encode used to surface it. The write error is returned again, while an encoding failure still comes back before anything is committed. --- README.md | 2 +- rest.go | 9 ++++++--- rest_test.go | 17 +++++++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 1171131..0bc8d9d 100644 --- a/README.md +++ b/README.md @@ -465,7 +465,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. Encoding runs before anything is written, so a returned error leaves the response uncommitted and the caller can still replace it with an error status +- `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/rest.go b/rest.go index 3fe31b1..b4ed629 100644 --- a/rest.go +++ b/rest.go @@ -106,8 +106,9 @@ func DecodeJSON[T any](r *http.Request, res *T) error { } // EncodeJSON encodes given type to http.ResponseWriter and sets status code and content type header. -// Encoding happens before anything is written, so a returned error guarantees the response is still -// uncommitted and the caller is free to replace it with an error status. +// 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 { @@ -115,6 +116,8 @@ func EncodeJSON[T any](w http.ResponseWriter, status int, v T) error { } w.Header().Set("Content-Type", "application/json; charset=utf-8") w.WriteHeader(status) - _, _ = w.Write(buf.Bytes()) + 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 301c047..2c45dd0 100644 --- a/rest_test.go +++ b/rest_test.go @@ -223,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) +} From 8d8dfc9a83706685e7bcf6ffb802f118dad5ccfc Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Wed, 19 Aug 2026 00:21:34 +0100 Subject: [PATCH 10/10] fix: keep Metrics deny-all by default, add MetricsAllowAll for the open case Serving expvar to everyone when no ips are given turns a dependency bump into an exposed endpoint for anyone who mounted Metrics() bare, and expvar publishes cmdline with the process flag values. Empty arguments deny every request again, as before, and callers who want the endpoint public now ask for it by name. --- README.md | 10 +++++++--- metrics.go | 17 +++++++++++++++-- metrics_test.go | 41 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 0bc8d9d..63466fa 100644 --- a/README.md +++ b/README.md @@ -118,9 +118,13 @@ _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, i.e. `rest.Metrics("127.0.0.1", "192.168.0.0/16")`. -Called without any ip, as `rest.Metrics()`, it serves every source. +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 diff --git a/metrics.go b/metrics.go index c9c0821..6874ed6 100644 --- a/metrics.go +++ b/metrics.go @@ -7,12 +7,25 @@ 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 len(onlyIps) > 0 { // no restrictions if no ips defined + 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 diff --git a/metrics_test.go b/metrics_test.go index 6a94420..3da1c3f 100644 --- a/metrics_test.go +++ b/metrics_test.go @@ -36,13 +36,32 @@ func TestMetrics_EmptyList(t *testing.T) { require.NoError(t, err) }) - // empty list means no ip restriction, expvar has to be served + // 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") // any source is fine when unrestricted + 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) @@ -55,6 +74,24 @@ func TestMetrics_EmptyList(t *testing.T) { 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"))