From 67ba600bf0e90023d3492ae48d4ce0f534fd74df Mon Sep 17 00:00:00 2001 From: Steven van der Vegt Date: Fri, 9 Oct 2026 10:27:36 +0200 Subject: [PATCH 1/2] fix(http): close response body in StrictHTTPClient.Do Do replaced the response body with an in-memory copy but never closed the original. When a response exceeded the maximum size, the error was returned with the body still open, so the connection stayed held until the client timeout. With MaxConnsPerHost = 5, a server returning oversized bodies could occupy every connection slot for that host. The original body is now closed on both the error and success paths. On overflow it is closed without draining, so the connection is dropped rather than reused. Assisted-by: AI --- docs/pages/release_notes.rst | 1 + http/client/client.go | 4 ++- http/client/client_test.go | 61 ++++++++++++++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/docs/pages/release_notes.rst b/docs/pages/release_notes.rst index 865bcccc0..870508ab6 100644 --- a/docs/pages/release_notes.rst +++ b/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/TBD ## 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 diff --git a/http/client/client.go b/http/client/client.go index acfa254b7..62f82e060 100644 --- a/http/client/client.go +++ b/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 } diff --git a/http/client/client_test.go b/http/client/client_test.go index 337e63252..997d47459 100644 --- a/http/client/client_test.go +++ b/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 From 3170d92705f069494f497d0b1110c6ec75d02db0 Mon Sep 17 00:00:00 2001 From: Steven van der Vegt Date: Fri, 9 Oct 2026 10:29:30 +0200 Subject: [PATCH 2/2] docs: add PR number to release note Assisted-by: AI --- docs/pages/release_notes.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/pages/release_notes.rst b/docs/pages/release_notes.rst index 870508ab6..2b1e033a6 100644 --- a/docs/pages/release_notes.rst +++ b/docs/pages/release_notes.rst @@ -18,7 +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/TBD +* #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