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 c6a6b6d09ee..2a7f3496be9 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 original 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/github.go b/github/github.go index 9e2b13bfe3c..010ce8192bf 100644 --- a/github/github.go +++ b/github/github.go @@ -1281,9 +1281,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. @@ -1788,6 +1792,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/github_test.go b/github/github_test.go index 9ab842661c6..45526d0ba6b 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. 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 9363aaa081f..1c94d043f30 100644 --- a/github/repos_releases_test.go +++ b/github/repos_releases_test.go @@ -537,6 +537,56 @@ func TestRepositoriesService_DownloadReleaseAsset_FollowRedirectToError(t *testi } } +// CheckResponse substitutes resp.Body with a re-readable copy on error +// 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) + + 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)