Skip to content

fix: Limit HTTP error response body reads to prevent OOM#4191

Merged
gmlewis merged 4 commits intogoogle:masterfrom
evilgensec:fix/limit-error-response-body-size
May 8, 2026
Merged

fix: Limit HTTP error response body reads to prevent OOM#4191
gmlewis merged 4 commits intogoogle:masterfrom
evilgensec:fix/limit-error-response-body-size

Conversation

@evilgensec
Copy link
Copy Markdown
Contributor

@evilgensec evilgensec commented May 7, 2026

Problem

Two paths in github.go read HTTP error response bodies with no size limit:

// CheckResponse — fires on every non-2xx API response
data, err := io.ReadAll(r.Body)

// AcceptedError handler in bareDo — fires on 202 Accepted
b, readErr := io.ReadAll(resp.Body)

A server returning a very large error body causes the client process to exhaust memory. The webhook payload path already applies a 25 MiB limit via io.LimitReader in messages.go; the error-response paths had no equivalent guard.

Fix

Wrap both reads with io.LimitReader capped at maxErrorBodySize (1 MiB):

data, err := io.ReadAll(io.LimitReader(r.Body, maxErrorBodySize))
b, readErr := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodySize))

All existing tests pass.

Comment thread github/github.go Outdated
Comment thread github/github.go
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.
@evilgensec evilgensec force-pushed the fix/limit-error-response-body-size branch from 192b849 to 499cf82 Compare May 7, 2026 14:42
@gmlewis gmlewis changed the title fix(github): limit HTTP error response body reads to prevent OOM fix: Limit HTTP error response body reads to prevent OOM May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.71%. Comparing base (6b805b3) to head (a24f58a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4191   +/-   ##
=======================================
  Coverage   93.71%   93.71%           
=======================================
  Files         209      209           
  Lines       19772    19772           
=======================================
  Hits        18529    18529           
  Misses       1046     1046           
  Partials      197      197           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

evilgensec added 2 commits May 8, 2026 19:58
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.
@evilgensec evilgensec force-pushed the fix/limit-error-response-body-size branch from 8a06894 to a791678 Compare May 8, 2026 14:30
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @evilgensec and @alexandear!
LGTM.
Merging.

@gmlewis gmlewis merged commit 6c58592 into google:master May 8, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants