From 210afcd5a15745040769bc60000ca41069abcda9 Mon Sep 17 00:00:00 2001 From: Yavor Lulchev Date: Tue, 25 Aug 2026 22:47:25 +0300 Subject: [PATCH 1/4] test: assert the original response body is closed on non-2xx responses CheckResponse substitutes r.Body with a re-readable NopCloser copy on error responses, so asserting on resp.Body.Close alone cannot catch a leak of the network body. Wrap the test client's transport and record whether the body it returned is closed. This test fails at this commit; the fix follows in the next one. --- github/github_test.go | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/github/github_test.go b/github/github_test.go index ae585cede4d..776abaf475d 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -2296,6 +2296,50 @@ func TestDo_httpError(t *testing.T) { } } +// closeRecorder flags when the response body handed back by the transport +// is closed. +type closeRecorder struct { + io.ReadCloser + closed *bool +} + +func (r *closeRecorder) Close() error { + *r.closed = true + return r.ReadCloser.Close() +} + +// CheckResponse substitutes resp.Body with a re-readable copy on error +// responses; the network body it replaces must still be closed. +func TestDo_closesOriginalBodyOnErrorResponse(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, `{"message":"Bad Request"}`, 400) + }) + + var closed bool + base := client.client.Transport + if base == nil { + base = http.DefaultTransport + } + client.client.Transport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { + resp, err := base.RoundTrip(req) + if resp != nil { + resp.Body = &closeRecorder{ReadCloser: resp.Body, closed: &closed} + } + return resp, err + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + if _, err := client.Do(req, nil); err == nil { + t.Fatal("Expected HTTP 400 error, got no error.") + } + if !closed { + t.Error("original response body was not closed on an error response") + } +} + // Test handling of an error caused by the internal http client's Do() // function. A redirect loop is pretty unlikely to occur within the GitHub // API, but does allow us to exercise the right code path. From 25b74349f03c5aa66ae7b23914f59627a1c0cbd9 Mon Sep 17 00:00:00 2001 From: Yavor Lulchev Date: Tue, 25 Aug 2026 23:07:17 +0300 Subject: [PATCH 2/4] Fixes #4484 `CheckResponse` substitutes `r.Body` with a re-readable `NopCloser` copy on every non-2xx response (#1363). Since #1772, the error-path `defer resp.Body.Close()` in `bareDo` is registered after that substitution, so the deferred close releases the copy and the network body is never closed. `CopilotService.fetchMetricsReport` and `RepositoriesService.downloadReleaseAssetFromURL` have the same pattern. Consequences of the unclosed network body: - With an `http.Client` that has `Timeout` set and a wrapped transport (which includes clients built via `WithAuthToken`), every non-2xx response parks one `net/http.setRequestCancel.func4` goroutine for the remainder of the timeout. Bounded, but it intermittently fails `goleak`-checked test suites downstream. - Error bodies larger than `maxErrorBodySize` are only partially drained, so the connection is additionally lost. The fix captures the network body before calling CheckResponse and closes that instead. The #1363 behavior (re-readable error bodies) is unchanged: the substitute is left open for callers. --- github/github.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/github/github.go b/github/github.go index 07b08ea27bf..a997659bad3 100644 --- a/github/github.go +++ b/github/github.go @@ -1282,9 +1282,13 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro c.rateMu.Unlock() } + // CheckResponse substitutes r.Body with a re-readable copy on error + // responses, so capture the network body first: it is the one that must + // be closed. + origBody := resp.Body err = CheckResponse(resp) if err != nil { - defer resp.Body.Close() + defer origBody.Close() // Special case for AcceptedErrors. If an AcceptedError // has been encountered, the response's payload will be // added to the AcceptedError and returned. From afa7265994d176edcdb342480b3c171802fe6f6d Mon Sep 17 00:00:00 2001 From: Yavor Lulchev Date: Wed, 26 Aug 2026 03:26:36 +0300 Subject: [PATCH 3/4] Add regression tests for the other 2 callers of CheckResponse() --- github/copilot_test.go | 32 +++++++++++++++++++++++ github/repos_releases_test.go | 48 +++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/github/copilot_test.go b/github/copilot_test.go index bf3453cf442..0ea637fc71b 100644 --- a/github/copilot_test.go +++ b/github/copilot_test.go @@ -3212,6 +3212,38 @@ func TestCopilotService_DownloadDailyMetrics(t *testing.T) { } } +// CheckResponse substitutes resp.Body with a re-readable copy on error +// responses; fetchMetricsReport must still close the network body it replaces. +func TestCopilotService_fetchMetricsReport_closesOriginalBodyOnErrorResponse(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/path/to/daily", func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, `{"message":"Bad Request"}`, 400) + }) + + var closed bool + base := client.client.Transport + if base == nil { + base = http.DefaultTransport + } + client.client.Transport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { + resp, err := base.RoundTrip(req) + if resp != nil { + resp.Body = &closeRecorder{ReadCloser: resp.Body, closed: &closed} + } + return resp, err + }) + + ctx := t.Context() + if _, _, err := client.Copilot.DownloadDailyMetrics(ctx, client.baseURL.String()+"path/to/daily"); err == nil { + t.Fatal("Copilot.DownloadDailyMetrics expected error but got none") + } + if !closed { + t.Error("original response body was not closed on an error response") + } +} + func TestCopilotService_DownloadPeriodicMetrics(t *testing.T) { t.Parallel() client, mux, _ := setup(t) diff --git a/github/repos_releases_test.go b/github/repos_releases_test.go index 1a0b9d3aabd..3f6ae60b949 100644 --- a/github/repos_releases_test.go +++ b/github/repos_releases_test.go @@ -537,6 +537,54 @@ func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToError(t *testi } } +// CheckResponse substitutes resp.Body with a re-readable copy on error +// responses; downloadReleaseAssetFromURL must still close the network body it +// replaces. The recorder wraps the follow-redirects client's transport because +// that client, not the library client, performs the redirected request. +func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToErrorClosesOriginalBody(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/repos/o/r/releases/assets/1", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + testHeader(t, r, "Accept", defaultMediaType) + // /yo, below will be served as baseURLPath/yo + http.Redirect(w, r, baseURLPath+"/yo", http.StatusFound) + }) + mux.HandleFunc("/yo", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + testHeader(t, r, "Accept", defaultMediaType) + http.Error(w, `{"message":"Not Found"}`, 404) + }) + + var closed bool + followRedirectsClient := &http.Client{ + Transport: roundTripperFunc(func(req *http.Request) (*http.Response, error) { + resp, err := http.DefaultTransport.RoundTrip(req) + if resp != nil { + resp.Body = &closeRecorder{ReadCloser: resp.Body, closed: &closed} + } + return resp, err + }), + } + + ctx := t.Context() + rc, loc, err := client.Repositories.DownloadReleaseAsset(ctx, "o", "r", 1, followRedirectsClient) + if err == nil { + t.Error("Repositories.DownloadReleaseAsset did not return an error") + } + if rc != nil { + rc.Close() + t.Error("Repositories.DownloadReleaseAsset returned stream, want nil") + } + if loc != "" { + t.Errorf(`Repositories.DownloadReleaseAsset returned "%v", want empty ""`, loc) + } + if !closed { + t.Error("original response body was not closed on an error response") + } +} + func TestRepositoriesService_DownloadReleaseAsset_APIError(t *testing.T) { t.Parallel() client, mux, _ := setup(t) From 8c8468bb21b07f971b916f0b491cd3c4f56c3404 Mon Sep 17 00:00:00 2001 From: Yavor Lulchev Date: Wed, 26 Aug 2026 03:45:11 +0300 Subject: [PATCH 4/4] Fix the remaining call sites & update the method godoc --- github/copilot.go | 6 +++++- github/copilot_test.go | 2 +- github/github.go | 6 ++++++ github/repos_releases.go | 6 +++++- github/repos_releases_test.go | 8 +++++--- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/github/copilot.go b/github/copilot.go index 9dd24eb8ea3..adbfae32210 100644 --- a/github/copilot.go +++ b/github/copilot.go @@ -1212,8 +1212,12 @@ func (s *CopilotService) fetchMetricsReport(ctx context.Context, url string) (*h return nil, nil, err } + // CheckResponse substitutes resp.Body with a re-readable copy on error + // responses, so capture the original body first: it is the one that must + // be closed. + origBody := resp.Body if err := CheckResponse(resp); err != nil { - resp.Body.Close() + _ = origBody.Close() return nil, newResponse(resp), err } diff --git a/github/copilot_test.go b/github/copilot_test.go index 0ea637fc71b..ee1f804a7f8 100644 --- a/github/copilot_test.go +++ b/github/copilot_test.go @@ -3213,7 +3213,7 @@ func TestCopilotService_DownloadDailyMetrics(t *testing.T) { } // CheckResponse substitutes resp.Body with a re-readable copy on error -// responses; fetchMetricsReport must still close the network body it replaces. +// responses; fetchMetricsReport must still close the original body it replaces. func TestCopilotService_fetchMetricsReport_closesOriginalBodyOnErrorResponse(t *testing.T) { t.Parallel() client, mux, _ := setup(t) diff --git a/github/github.go b/github/github.go index a997659bad3..efb4c677569 100644 --- a/github/github.go +++ b/github/github.go @@ -1795,6 +1795,12 @@ func (e *Error) UnmarshalJSON(data []byte) error { // API error responses are expected to have response // body, and a JSON response body that maps to [ErrorResponse]. // +// On error responses other than 202 Accepted, CheckResponse consumes r.Body +// and replaces it with an in-memory copy so that the error body can be +// re-read. Closing r.Body after CheckResponse returns therefore closes only +// the copy: to release the original body and its underlying connection, +// capture r.Body before the call and close the captured body instead. +// // The error type will be *[RateLimitError] for rate limit exceeded errors, // *[AcceptedError] for 202 Accepted status codes, // *[TwoFactorAuthError] for two-factor authentication errors, diff --git a/github/repos_releases.go b/github/repos_releases.go index 889afb6da86..3361a5d79d1 100644 --- a/github/repos_releases.go +++ b/github/repos_releases.go @@ -375,8 +375,12 @@ func (s *RepositoriesService) downloadReleaseAssetFromURL(ctx context.Context, f if err != nil { return nil, err } + // CheckResponse substitutes resp.Body with a re-readable copy on error + // responses, so capture the original body first: it is the one that must + // be closed. + origBody := resp.Body if err := CheckResponse(resp); err != nil { - _ = resp.Body.Close() + _ = origBody.Close() return nil, err } return resp.Body, nil diff --git a/github/repos_releases_test.go b/github/repos_releases_test.go index 3f6ae60b949..4fb85c75356 100644 --- a/github/repos_releases_test.go +++ b/github/repos_releases_test.go @@ -538,9 +538,11 @@ func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToError(t *testi } // CheckResponse substitutes resp.Body with a re-readable copy on error -// responses; downloadReleaseAssetFromURL must still close the network body it -// replaces. The recorder wraps the follow-redirects client's transport because -// that client, not the library client, performs the redirected request. +// responses; downloadReleaseAssetFromURL must still close the original body it +// replaces. Unlike its sibling tests, the recorder wraps the follow-redirects +// client's transport: that client, not the library client, performs the +// redirected request, so wrapping the library client would only ever observe +// the first hop's correctly-closed redirect response and never the leak. func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToErrorClosesOriginalBody(t *testing.T) { t.Parallel() client, mux, _ := setup(t)