Fix verified findings in middlewares and helpers

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
 

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)
 		}

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
 					}
 				}
 			}

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")
+}

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
 		}
 	}
 

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")
+}

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
 )

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=