From 499cf827c1ec78543ed522ae431bac5b0a289b73 Mon Sep 17 00:00:00 2001 From: evilgensec Date: Thu, 7 May 2026 20:26:18 +0545 Subject: [PATCH 1/3] fix(github): limit HTTP error response body reads to prevent OOM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CheckResponse and the AcceptedError handler in bareDo both read HTTP error response bodies with no size limit: data, err := io.ReadAll(r.Body) // CheckResponse b, readErr := io.ReadAll(resp.Body) // AcceptedError A server returning a very large error body causes the client to exhaust memory. Wrap both reads with io.LimitReader capped at 1 MiB — sufficient for any real GitHub API error JSON. The webhook payload path already applies a 25 MiB limit in messages.go; this brings the error-response paths into alignment. --- github/github.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/github/github.go b/github/github.go index 91af5aa665f..8777e07f116 100644 --- a/github/github.go +++ b/github/github.go @@ -920,6 +920,11 @@ const ( SleepUntilPrimaryRateLimitResetWhenRateLimited ) +// maxErrorBodySize is the maximum number of bytes read from an HTTP error +// response body. Limits memory allocation when a server returns an +// unexpectedly large error body. +const maxErrorBodySize = 1 * 1024 * 1024 // 1 MiB + // bareDo sends an API request using `caller` http.Client passed in the parameters // and lets you handle the api response. If an error or API Error occurs, the error // will contain more information. Otherwise, you are supposed to read and close the @@ -997,7 +1002,7 @@ func (c *Client) bareDo(caller *http.Client, req *http.Request) (*Response, erro // Issue #1022 var aerr *AcceptedError if errors.As(err, &aerr) { - b, readErr := io.ReadAll(resp.Body) + b, readErr := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize)) if readErr != nil { return response, readErr } @@ -1502,7 +1507,7 @@ func CheckResponse(r *http.Response) error { } errorResponse := &ErrorResponse{Response: r} - data, err := io.ReadAll(r.Body) + data, err := io.ReadAll(io.LimitReader(r.Body, maxErrorBodySize)) if err == nil && data != nil { err = json.Unmarshal(data, errorResponse) if err != nil { From 196dbd7fc4abec72b7c52ed30c9c9f50ad9d4ea2 Mon Sep 17 00:00:00 2001 From: evilgensec Date: Fri, 8 May 2026 19:58:54 +0545 Subject: [PATCH 2/3] test(github): add tests for error response body size limit Add two tests verifying that HTTP error response bodies are capped at maxErrorBodySize bytes: - TestCheckResponse_LargeBodyTruncated: sends a body of maxErrorBodySize+1 bytes to a non-2xx response; asserts the body restored on the response is exactly maxErrorBodySize bytes after CheckResponse returns. - TestDo_AcceptedError_LargeBodyTruncated: serves a 202 Accepted with a body of maxErrorBodySize+1 bytes; asserts AcceptedError.Raw is exactly maxErrorBodySize bytes. --- github/github_test.go | 62 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/github/github_test.go b/github/github_test.go index b41ebd8f849..1fcae60c364 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1400,6 +1400,36 @@ func TestDo_preservesResponseInHTTPError(t *testing.T) { } } +// TestDo_AcceptedError_LargeBodyTruncated verifies that when the API returns a +// 202 Accepted with a body larger than maxErrorBodySize, the client reads at +// most maxErrorBodySize bytes into AcceptedError.Raw and does not allocate +// unbounded memory. +func TestDo_AcceptedError_LargeBodyTruncated(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + // Serve a 202 response whose body exceeds the cap by one byte. + mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusAccepted) + fmt.Fprint(w, strings.Repeat("x", maxErrorBodySize+1)) + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + _, err := client.Do(req, nil) + if err == nil { + t.Fatal("Expected AcceptedError, got nil") + } + + var aerr *AcceptedError + if !errors.As(err, &aerr) { + t.Fatalf("Expected *AcceptedError, got %T: %v", err, err) + } + + if got, want := len(aerr.Raw), maxErrorBodySize; got != want { + t.Errorf("AcceptedError.Raw length = %d, want %d (maxErrorBodySize)", got, want) + } +} + // Test that an error caused by the internal http client's Do() function // does not leak the client secret. func TestDo_sanitizeURL(t *testing.T) { @@ -2982,6 +3012,38 @@ func TestCheckResponse_unexpectedErrorStructure(t *testing.T) { } } +// TestCheckResponse_LargeBodyTruncated verifies that CheckResponse reads at +// most maxErrorBodySize bytes from an error response body, so that a +// malicious or buggy server cannot cause the client to allocate unbounded +// memory. +func TestCheckResponse_LargeBodyTruncated(t *testing.T) { + t.Parallel() + // Build a body that is one byte larger than the cap. + body := strings.Repeat("x", maxErrorBodySize+1) + res := &http.Response{ + Request: &http.Request{}, + StatusCode: http.StatusBadRequest, + Body: io.NopCloser(strings.NewReader(body)), + } + + // CheckResponse should not return an error from the read itself; the HTTP + // error status is the expected error. + if err := CheckResponse(res); err == nil { + t.Fatal("Expected error from CheckResponse, got nil") + } + + // After CheckResponse, the body is restored with the (truncated) bytes that + // were actually read. Verify the restored body is exactly maxErrorBodySize + // bytes — not the full maxErrorBodySize+1 that the server sent. + restored, err := io.ReadAll(res.Body) + if err != nil { + t.Fatalf("io.ReadAll on restored body: %v", err) + } + if got, want := len(restored), maxErrorBodySize; got != want { + t.Errorf("restored body length = %d, want %d (maxErrorBodySize)", got, want) + } +} + func TestParseBooleanResponse_true(t *testing.T) { t.Parallel() result, err := parseBoolResponse(nil) From a7916783d544d94750f056e61e1f7598060b69fb Mon Sep 17 00:00:00 2001 From: evilgensec Date: Fri, 8 May 2026 20:15:00 +0545 Subject: [PATCH 3/3] fix(test): use %v instead of %d in error format strings (fmtpercentv) --- github/github_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/github/github_test.go b/github/github_test.go index 1fcae60c364..87703736508 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -1426,7 +1426,7 @@ func TestDo_AcceptedError_LargeBodyTruncated(t *testing.T) { } if got, want := len(aerr.Raw), maxErrorBodySize; got != want { - t.Errorf("AcceptedError.Raw length = %d, want %d (maxErrorBodySize)", got, want) + t.Errorf("AcceptedError.Raw length = %v, want %v (maxErrorBodySize)", got, want) } } @@ -3040,7 +3040,7 @@ func TestCheckResponse_LargeBodyTruncated(t *testing.T) { t.Fatalf("io.ReadAll on restored body: %v", err) } if got, want := len(restored), maxErrorBodySize; got != want { - t.Errorf("restored body length = %d, want %d (maxErrorBodySize)", got, want) + t.Errorf("restored body length = %v, want %v (maxErrorBodySize)", got, want) } }