Skip to content

fix: cap HTTP response body reads to prevent heap exhaustion - #382

Merged
Matt Sutkus (msuitcase) merged 5 commits into
masterfrom
sec-787-bounded-response-body
Oct 21, 2026
Merged

fix: cap HTTP response body reads to prevent heap exhaustion#382
Matt Sutkus (msuitcase) merged 5 commits into
masterfrom
sec-787-bounded-response-body

Conversation

@msuitcase

@msuitcase Matt Sutkus (msuitcase) commented Oct 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Fixes SEC-787 (pentest finding, Medium). Requestor read whole HTTP response bodies into memory with no size limit (Scanner(...).useDelimiter("\\A")). A compromised upstream, a misbehaving proxy, or a MITM sending a very large body could exhaust the JVM heap.

Changes:

  • One bounded read helper. getResponseBody(InputStream, long contentLength) reads in 8 KB chunks and stops at Constants.Http.MAX_RESPONSE_BODY_BYTES (10 MB).
    • Content-Length is checked up front. If the declared length is over the cap, it throws before reading anything.
    • The cap is also enforced while streaming. This covers responses where Content-Length is missing (chunked) or understated.
    • Oversized bodies throw HttpError with a descriptive message (Constants.ErrorMessages.RESPONSE_BODY_TOO_LARGE), not an OOM. HttpError is not an IOException, so the generic "could not connect" wrapper doesn't swallow it.
    • The stream is always closed, including on error.
    • A null error stream (a 4xx/5xx with no body) now returns "" instead of throwing an NPE.
    • Removed the available() == 0 short-circuit, which could wrongly return "" for a real stream that just had no bytes buffered yet.
  • Same cap on the App Engine fallback path (makeAppEngineRequest).
  • Stripe path: the ticket also mentions the Stripe tokenization response read in ReferralCustomerService.createStripeToken. That code was already removed on master in chore: remove deprecated, unused addCreditCardToUser function #381 (addCreditCardToUser removal), so nothing is left to patch here.

No unbounded reads remain in src/main.

Release plan: no backport to 8.8.x. Both halves of SEC-787 ship together in the next release from master: the Stripe-path removal from #381 and this cap. Users get both by upgrading to that version. Note that the release includes the breaking addCreditCardToUser removal, so its version number should be chosen accordingly.

Testing

New RequestorTest (it extends Requestor to reach the protected helper, the same pattern ErrorTest uses):

  • A normal UTF-8 body, including a multi-byte character split across the 8 KB read boundary
  • Empty stream and null stream
  • A body exactly at the limit is accepted
  • A Content-Length over the cap is rejected with zero bytes read
  • A missing Content-Length with an endless stream is rejected once the cap is crossed
  • An understated Content-Length with an oversized stream is rejected
  • End-to-end through client.address.retrieve(...) with a mocked HttpsURLConnection (via EasyPost._vcrUrlFunction): a normal response deserializes, and both oversized cases throw HttpError

⚠️ I couldn't build this locally (there was no JDK/Maven in my environment), so CI is the first compile, checkstyle and test run.

Pull Request Type

Please select the option(s) that are relevant to this PR.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement (fixing a typo, updating readme, renaming a variable name, etc)

🤖 Generated with Claude Code

Matt Sutkus (msuitcase) and others added 2 commits October 2, 2026 20:33
@msuitcase @claude
fix: cap HTTP response body reads to prevent heap exhaustion
b10b212 
Requestor read entire response bodies into memory with no size limit, so
a compromised upstream, misbehaving proxy, or MITM returning a very large
body could exhaust the JVM heap.

Response bodies are now capped at 10 MB. A Content-Length above the cap is
rejected before anything is read, and the cap is also enforced while
streaming in case the header is missing or wrong. Oversized responses
throw an HttpError with a descriptive message. The App Engine fallback
path applies the same cap.

SEC-787

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@msuitcase @claude
docs: restore unintentionally edited historical CHANGELOG entries
6ea8f75 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@msuitcase Matt Sutkus (msuitcase) mentioned this pull request Oct 3, 2026
Merged
1 of 4 tasks
Matt Sutkus (msuitcase) and others added 2 commits October 20, 2026 15:18
@msuitcase
Merge branch 'master' into sec-787-bounded-response-body
e707566
@msuitcase @claude
fix(test): allow Mockito to mock JDK classes on newest JDKs
3919113 
Byte Buddy bundled with Mockito 4.11 rejects Java 25 class files, breaking
RequestorTest (mocks HttpsURLConnection). Set net.bytebuddy.experimental for surefire.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Matt Sutkus (msuitcase) added a commit that referenced this pull request Oct 21, 2026
@msuitcase @claude
fix: redact API key in headers passed to request/response hooks (#383)
0941902 
# Description

Fixes SEC-786 (pentest finding "API Key Forwarded to Request-Hook
Handlers", Medium). `Requestor.httpRequest` built the hook headers with
`generateHeaders(client.getApiKey())` and passed them to both
`RequestHookResponses` and `ResponseHookResponses`, so every hook
received `Authorization: Bearer <full api key>`. A hook that logs or
exports its event (a common use for debugging, tracing, or APM) would
write the full key to logs or a third-party platform.

Changes:
- **Hooks get a redacted copy of the headers.** The new
`generateHookHeaders` builds the hook headers map and replaces the
`Authorization` value with `Bearer ****WXYZ` (last four characters
only). This keeps enough auth context to tell keys apart in logs without
exposing the credential.
- The mask has a fixed length, so it doesn't reveal the key length
either.
- Keys of 8 characters or fewer are fully masked (`Bearer ****`), since
showing four characters would reveal half or more of the key. Real
EasyPost keys are much longer, so this only affects unusual or test
keys.
- **The outbound request is unchanged.** `createEasyPostConnection` and
the App Engine path still build their own headers with the full key. The
hook map was already a separate instance from the one used on the
connection; it's now also redacted.
- Updated the `headers` Javadoc on both hook classes. The
`ResponseHookResponses` doc said "headers of the response", but these
are the request headers.
- CHANGELOG entry under `Next Release`.

`Authorization` is the only sensitive header the SDK sets, so no other
headers needed redacting.

# Testing

New tests in `HookTest` send a request through a mocked
`HttpsURLConnection` (via `EasyPost._vcrUrlFunction`, the same approach
as #382) and capture what both hooks receive:
- `testHooksReceiveRedactedApiKey`: both hooks see `Bearer ****WXYZ`, no
header value contains the full key, and `Mockito.verify` confirms the
real connection was sent `Bearer <full key>`.
- `testHooksFullyRedactShortApiKey`: an 8-character key is fully masked
in both hooks and still sent in full on the wire.

The existing VCR hook tests are unchanged. An `@AfterEach` resets
`_vcrUrlFunction` so the mock doesn't leak into other tests.

⚠️ I couldn't build this locally (my sandbox couldn't clone the repo or
reach Maven), so CI is the first compile, checkstyle and test run.

**Notes for merging:**
- #382 also adds a line under `Next Release` in `CHANGELOG.md`, so
whichever lands second will have a one-line conflict to resolve. The
code changes touch different parts of `Requestor` and don't overlap.
- Same release caveat as #382: the ticket asks for a patch release, but
`Next Release` already contains a breaking change (`addCreditCardToUser`
removal). A true 8.8.1 would need a backport branch from `v8.8.0`; this
change applies there cleanly in concept (only `httpRequest` and two new
private helpers).
- The ticket also asks to check the other SDKs (node, python, ruby, php,
go, csharp) for the same hook behavior. That's not covered here.

# Pull Request Type

Please select the option(s) that are relevant to this PR.

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] Improvement (fixing a typo, updating readme, renaming a variable
name, etc)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@msuitcase
Matt Sutkus (msuitcase) merged commit 0c7678f into master Oct 21, 2026
24 checks passed
@msuitcase
Matt Sutkus (msuitcase) deleted the sec-787-bounded-response-body branch October 21, 2026 02:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Justintime50 Justin Hammond (Justintime50) Justintime50 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@msuitcase @Justintime50