fix: Close the original response body on non-2xx responses (#4486)

Author: yavorl

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
 	}
 

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)

github/github.go

@@ -1305,9 +1305,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.
@@ -1821,6 +1825,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,

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.

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

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)