Cap HTTP response body size across WinInet, WinRt, and Apple transports#1508
Open
bmehta001 wants to merge 6 commits into
Open
Cap HTTP response body size across WinInet, WinRt, and Apple transports#1508bmehta001 wants to merge 6 commits into
bmehta001 wants to merge 6 commits into
Conversation
Extends the memory-amplification DoS hardening (a hostile or MITM'd collector returning an oversized body to exhaust process memory) beyond the libcurl transport to the remaining platform transports. Introduces a single shared constant MAX_HTTP_RESPONSE_SIZE (16 MB) in IHttpClient.hpp so every transport uses the same generous ceiling, well above any legitimate OneCollector or config response. - WinInet: bound m_bodyBuffer in the InternetReadFile loop; over-cap aborts the read and the request is reported as a failure (retried). - WinRt: reject a ReadAsBufferAsync buffer whose length exceeds the cap without copying it; report NetworkFailure. - Apple (NSURLSession): reject a completion-handler NSData larger than the cap without copying it; report NetworkFailure. The libcurl transport is capped separately in its own focused change; a later cleanup can unify its constant onto MAX_HTTP_RESPONSE_SIZE. Files: - lib/include/public/IHttpClient.hpp - lib/http/HttpClient_WinInet.cpp - lib/http/HttpClient_WinRt.cpp - lib/http/HttpClient_Apple.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the existing libcurl response-body size cap hardening to the remaining platform HTTP transports (WinInet, WinRT, Apple) to prevent memory-amplification DoS via oversized collector responses, using a shared MAX_HTTP_RESPONSE_SIZE (16 MB) constant.
Changes:
- Introduces
MAX_HTTP_RESPONSE_SIZEinIHttpClient.hppfor cross-transport reuse. - Enforces the cap in WinRT (
ReadAsBufferAsync) and Apple (NSData) by rejecting oversized bodies without copying. - Enforces the cap in WinInet by aborting the
InternetReadFileloop once the buffered body exceeds the maximum.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/include/public/IHttpClient.hpp | Adds shared 16 MB response-body cap constant and security rationale. |
| lib/http/HttpClient_WinRt.cpp | Rejects oversized WinRT buffered responses before copying into m_body. |
| lib/http/HttpClient_WinInet.cpp | Adds a size check to stop buffering oversized WinInet responses during read loop. |
| lib/http/HttpClient_Apple.mm | Rejects oversized Apple NSData responses before copying into m_body. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…d 1) Address Copilot review: - WinInet: the oversize-response abort set ERROR_NOT_ENOUGH_MEMORY, which fell through to the default case (LocalFailure). Use ERROR_HTTP_INVALID_SERVER_- RESPONSE so it maps to HttpResult_NetworkFailure, consistent with the WinRt and Apple transports (still retried, but correctly classified). - Apple: guard the success-path copy on a non-zero length and cast data.length to size_t, so a nil NSData (bytes == nullptr) never performs pointer arithmetic on nullptr (undefined behavior). Files: - lib/http/HttpClient_WinInet.cpp - lib/http/HttpClient_Apple.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+226
to
+230
| // SECURITY: refuse an over-large response instead of buffering it (see | ||
| // MAX_HTTP_RESPONSE_SIZE), so a hostile/MITM'd collector cannot exhaust | ||
| // process memory. The request is reported as a network failure (retried). | ||
| if (length > MAX_HTTP_RESPONSE_SIZE) | ||
| { |
… (round 2) Address Copilot review round 2: the previous checks ran after the framework had already materialized the whole response body, so an oversized response could still drive a large allocation. Rework each transport to bound memory to the cap: - WinInet: check before every append (pre-loop and in-loop) so m_bodyBuffer never exceeds MAX_HTTP_RESPONSE_SIZE, not "cap + one chunk". (Validated: the Windows `mat` library compiles.) - WinRt: request with HttpCompletionOption::ResponseHeadersRead (so the body is not pre-buffered) and stream it via ReadAsInputStreamAsync in 64 KB chunks, aborting the moment the cap would be exceeded. - Apple: replace the completionHandler NSURLSession API (which materializes the full NSData) with a streaming NSURLSessionDataDelegate that accumulates in didReceiveData: and cancels the task once the cap would be exceeded; an over-cap transfer is surfaced as NetworkFailure (retried). The WinRt and Apple rewrites target UWP/macOS toolchains that aren't available locally, so they are review-verified and must be built/tested on-device before merge. Files: - lib/http/HttpClient_WinInet.cpp - lib/http/HttpClient_WinRt.cpp - lib/http/HttpClient_Apple.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… cast (round 3) Address Copilot review round 3 on the streaming rework: - WinRt: concurrency::task::wait()/get() rethrow if ReadAsInputStreamAsync or a chunk ReadAsync faults (e.g., connection reset) even when the status looks completed. Wrap the whole streamed-read in try/catch so a fault maps to HttpResult_NetworkFailure instead of escaping onRequestComplete and crashing. - Apple: cast the dictionary value (stored as id) back to the concrete block type in didCompleteWithError: to avoid an incompatible-pointer-types warning (which fails builds under -Werror). Files: - lib/http/HttpClient_WinRt.cpp - lib/http/HttpClient_Apple.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nd 4) Address Copilot review round 4: HttpResponseDecoder processes any non-empty response body regardless of HttpResult (processBody runs when GetBody() is non-empty), so a partial body left on a rejected streamed response could be parsed for kill-switch/stats. In the WinRt streaming reader: - Map a caller-initiated cancellation (task_status::canceled, from cancel()) to HttpResult_Aborted instead of NetworkFailure. - Clear response->m_body on every non-success path (cancel, read failure, over-cap, and streaming exceptions) so no partial body is processed. (WinInet and Apple never attach a partial body to the response on rejection, so they need no change.) Files: - lib/http/HttpClient_WinRt.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The streaming session delegate's didCompleteWithError: cleanup called [_overCap removeObjectForKey:key], but _overCap is an NSMutableSet, which has no removeObjectForKey: selector (that belongs to NSMutableDictionary). This was a copy-paste from the _handlers/_buffers dictionary cleanup two lines above and fails to compile, breaking the entire Apple/macOS mat build. Use the correct NSMutableSet selector, removeObject:. Validation (macOS arm64, Apple HTTP transport): - libmat builds clean; full host UnitTests 518/518 pass. - End-to-end test against a local HttpServer through the real HttpClient_Apple: under-cap (64 KB) -> HttpResult_OK with full body; over-cap (16 MB + 1 MB) -> HttpResult_NetworkFailure with an empty body and no crash; exactly MAX_HTTP_RESPONSE_SIZE (16 MB) -> HttpResult_OK with full body. Stable over 8 repeats (no delegate state races). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to the libcurl response-size cap (#1507): extends the same
memory-amplification DoS hardening to the remaining platform transports, so a
hostile or MITM'd collector cannot exhaust the embedding process's memory by
returning an oversized response body on any platform.
Introduces one shared constant,
MAX_HTTP_RESPONSE_SIZE(16 MB), inIHttpClient.hpp, well above any legitimate OneCollector status/kill-switch/config response, and enforces it by streaming each transport's response so
the body is never fully materialized before the cap is checked:
m_bodyBufferin theInternetReadFileloop, checked before every append (pre-loop and in-loop) so the buffer never exceeds the cap. Over-cap ⇒ERROR_HTTP_INVALID_SERVER_RESPONSE⇒NetworkFailure.HttpCompletionOption::ResponseHeadersRead(framework no longer pre-buffers the body) and stream viaReadAsInputStreamAsyncin 64 KB chunks, aborting the moment the cap would be exceeded.NSURLSessionAPI (which materializes the wholeNSData) with a streamingNSURLSessionDataDelegatethat accumulates indidReceiveData:and cancels the task once the cap would be exceeded.A rejected/over-cap response is treated as a transient network failure, so the
upload is retried — no crash, no partial-body corruption.
Scope
trivial cleanup can point curl's private constant at
MAX_HTTP_RESPONSE_SIZE;this PR deliberately does not touch curl to avoid overlapping with Cap curl HTTP response body size to prevent memory-amplification DoS #1507.
Validation
MAX_HTTP_RESPONSE_SIZEinIHttpClient.hpp: compiles on GCC (WSLmatbuild) and MSVC (
/std:c++17header check →cap=16777216).matlibrary builds cleanly with the strictcap (real MSVC toolchain).
locally, so the streaming rewrites are review-verified and must be built and
tested on-device before merge. Points to watch on device: the WinRt
synchronous chunk-read loop on the PPL continuation thread, and the Apple
NSURLSessionDataDelegaterouting/lifetime (ARC is enabled) plus theover-cap cancel →
NetworkFailuremapping.