fix(http): close response body in StrictHTTPClient.Do

docs/pages/release_notes.rst

@@ -18,6 +18,7 @@ Unreleased
 * Network: the default of ``network.maxbackoff`` is lowered from ``24h`` to ``1h``. The backoff is persisted across restarts and only reset when a peer's NutsComm address changes, so a peer that was unreachable for a few days could previously go unattempted for up to a day after it came back. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4467
 * Network: failed connection attempts are now logged at debug level instead of warning level. A warning is logged once, on the attempt that reaches ``network.maxbackoff``, so an unreachable peer no longer repeats the same warning on every retry. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4467
 * Network: connections on which no message was received for ``network.idletimeout`` (default ``2m``) are now closed and re-established. Peers send gossip and diagnostics messages every few seconds, so a silent connection is a dead one: typically a half-open TCP connection or a reverse proxy that kept the stream open after the other side went away. Previously such connections lingered until the proxy or node was restarted, and the peer holding the stale connection rejected new connections with ``already connected``. Set ``network.idletimeout`` to ``0`` to disable. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4562
+* #4620: HTTP client: the response body is now closed when a response exceeds the maximum size, so an oversized response no longer holds a connection until the client timeout. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4621
 
 ## Security
 * #4596: Discovery Service: registered Verifiable Presentations are now limited to 64 KiB, and the Discovery Service client reads responses of up to 10 MiB from the (operator-configured) Discovery Server instead of the 1 MiB applied to other outbound HTTP calls. Previously a client could no longer synchronize a service whose response exceeded 1 MiB, which a few hundred registrations or a couple of deliberately padded ones could cause. By @reinkrul in https://github.com/nuts-foundation/nuts-node/pull/4597

http/client/client.go

@@ -186,7 +186,9 @@ func (s *StrictHTTPClient) Do(req *http.Request) (*http.Response, error) {
 		return nil, err
 	}
 	if result.Body != nil {
-		body, err := limitedReadAll(result.Body, s.maxResponseSize)
+		originalBody := result.Body
+		defer originalBody.Close()
+		body, err := limitedReadAll(originalBody, s.maxResponseSize)
 		if err != nil {
 			return nil, err
 		}

http/client/client_test.go

@@ -24,6 +24,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"strconv"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -283,6 +284,66 @@ func TestWithMaxResponseSize(t *testing.T) {
 	})
 }
 
+func TestStrictHTTPClient_ClosesResponseBody(t *testing.T) {
+	oldStrictMode := StrictMode
+	StrictMode = false
+	t.Cleanup(func() { StrictMode = oldStrictMode })
+	const limit = 10
+	server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) {
+		size, _ := strconv.Atoi(request.URL.Query().Get("size"))
+		_, _ = writer.Write([]byte(strings.Repeat("a", size)))
+	}))
+	t.Cleanup(server.Close)
+
+	t.Run("response exceeds limit", func(t *testing.T) {
+		transport := &closeRecordingTransport{base: SafeHttpTransport}
+		client := newStrictHTTPClient(&http.Client{Transport: transport}, []Option{WithMaxResponseSize(limit)})
+		request, _ := http.NewRequest(http.MethodGet, server.URL+"?size="+strconv.Itoa(limit+1), nil)
+
+		_, err := client.Do(request)
+
+		assert.EqualError(t, err, "data to read exceeds max. safety limit of 10 bytes")
+		assert.Equal(t, int32(1), transport.closed.Load())
+	})
+	t.Run("response within limit", func(t *testing.T) {
+		transport := &closeRecordingTransport{base: SafeHttpTransport}
+		client := newStrictHTTPClient(&http.Client{Transport: transport}, []Option{WithMaxResponseSize(limit)})
+		request, _ := http.NewRequest(http.MethodGet, server.URL+"?size="+strconv.Itoa(limit), nil)
+
+		response, err := client.Do(request)
+
+		require.NoError(t, err)
+		data, _ := io.ReadAll(response.Body)
+		assert.Len(t, data, limit)
+		assert.Equal(t, int32(1), transport.closed.Load())
+	})
+}
+
+// closeRecordingTransport wraps response bodies to count how often they are closed.
+type closeRecordingTransport struct {
+	base   http.RoundTripper
+	closed atomic.Int32
+}
+
+func (c *closeRecordingTransport) RoundTrip(request *http.Request) (*http.Response, error) {
+	response, err := c.base.RoundTrip(request)
+	if err != nil {
+		return nil, err
+	}
+	response.Body = &closeRecordingBody{ReadCloser: response.Body, closed: &c.closed}
+	return response, nil
+}
+
+type closeRecordingBody struct {
+	io.ReadCloser
+	closed *atomic.Int32
+}
+
+func (c *closeRecordingBody) Close() error {
+	c.closed.Add(1)
+	return c.ReadCloser.Close()
+}
+
 func TestMaxConns(t *testing.T) {
 	oldStrictMode := StrictMode
 	StrictMode = false