fix(copilot): bound streaming SSE body to prevent unbounded read OOM#1674
Merged
Conversation
The Copilot Anthropic-Messages SSE parser read the success-response body line-by-line with no accumulated-byte tracking and no cap. A hostile or broken endpoint that streams an unbounded SSE body -- or a single never-terminating data: line with no newline -- would buffer without bound and could exhaust gateway memory (OOM-DoS). This is the streaming complement to #1653, which bounded only the non-streaming Copilot path via BoundedHttpContent. - Add ByteCountingStream: a reusable read-only Stream wrapper in the Copilot provider that tracks total bytes against a 16 MiB cap (matching BoundedHttpContent.DefaultMaxResponseBytes) and bytes-since-newline against a 64 KiB per-frame cap, throwing the canonical ResponseContentTooLargeException on overflow and aborting the read mid-flight. - Bound the read loop in CopilotMessagesStreamParser.ProcessStreamAsync by wrapping the response stream before the StreamReader, so an unbounded body or a single endless data: line trips the cap regardless of how the reader buffers. - Well-formed, under-cap streams are byte-identical: the guard only trips on overflow; normal parsing is unchanged. The Anthropic streaming parser shares the same loop shape but lives in a separate project and reusing the guard would require relocating it to Core; deferred to a follow-up to keep this a tight, single-surface change. Closes #1668
Owner
CI Health Check -- PR #1674
Branch: Actions taken:
Blockers for Jon:
Farnsworth (automated CI monitor) -- BotNexus -- Last updated: 2026-06-27 23:03 UTC |
This was referenced Jun 27, 2026
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.
Closes #1668
Summary
The GitHub Copilot Anthropic-Messages SSE streaming parser
(
CopilotMessagesStreamParser.ProcessStreamAsync) read the success-responsebody line-by-line with no accumulated-byte tracking and no cap. A hostile or
malfunctioning Copilot endpoint that streams an unbounded SSE body -- or a
single never-terminating
data:line with no newline -- would buffer withoutbound and could exhaust gateway memory (OOM-DoS of the gateway).
This is the streaming complement to #1653, which added
BoundedHttpContent.ReadFromJsonWithLimitAsyncand bounded only thenon-streaming Copilot path (discovery JSON + the error-response body). The
streaming success path was still unbounded; this PR closes that gap.
Changes
New
ByteCountingStream(src/agent/BotNexus.Agent.Providers.Copilot/Streaming/ByteCountingStream.cs):a small, reusable, read-only
Streamwrapper -- the C# equivalent ofOpenClaw's
createSseByteGuard. It tracks, as bytes are read:\n).On either overflow it throws the canonical
ResponseContentTooLargeException(reused fromBoundedHttpContent, [Security] Bound Copilot discovery/auth JSON reads (unbounded ReadFromJsonAsync -> OOM) #1653)and aborts the read mid-flight, so the oversized body is never fully
materialized. The frame counter resets at every
\n, so normal multi-framestreams never trip it.
Bounded the read loop in
CopilotMessagesStreamParser.ProcessStreamAsync:the parser now receives the raw response
Stream, wraps it in aByteCountingStream, and constructs itsStreamReaderover the boundedstream. Because every byte the reader consumes flows through the guard, an
unbounded body or a single endless
data:line trips the cap regardless ofhow
StreamReader.ReadLineAsyncbuffers internally (this is what catches the"single never-terminating line with no
\n" case).Updated the single call site in
CopilotMessagesProvider.StreamCoreAsyncto hand the raw response stream tothe parser (the parser now owns the bounded reader). The
using StreamReaderthat previously lived in the provider moved into the parser.
Cap values
private const long MaxResponseBytes = BoundedHttpContent.DefaultMaxResponseBytes;(16 MiB).
DefaultMaxResponseBytesispublic constinBotNexus.Agent.Providers.Core.Utilitiesand the Copilot project alreadyreferences that namespace, so no duplicated literal was introduced -- the
streaming and non-streaming paths agree on a legitimate body size by
construction.
const long MaxFrameBytes = 64L * 1024;(64 KiB),documented inline.
The overflow error shape is aligned with the non-streaming path: the same
ResponseContentTooLargeException(maxBytes, observedBytes)type is thrown, sothe provider's existing
catch (Exception ex)surfaces it as aStopReason.Errorstream result.Behaviour for normal streams is unchanged
The chunk-by-chunk parsing is byte-identical for well-formed, under-cap
streams; the guard only trips on overflow. Verified by a dedicated regression
test plus the full pre-existing Copilot wire-replay suite.
Tests (TDD)
Added
tests/agent/BotNexus.Agent.Providers.Copilot.Tests/Messages/CopilotMessagesStreamGuardTests.cs(7 tests):
response whose total body exceeds 16 MiB surfaces the overflow error and is
not buffered unbounded.
data:line (e2e) -- one ~512 KiBdata:linewith no
\ntrips the per-frame cap (stays under the total cap, so itexercises the frame guard specifically).
is unaffected (non-error, populated usage, non-empty content).
4-7. Direct unit tests for
ByteCountingStream: pass-through under cap,total-cap trip, per-frame-cap trip, and frame-counter-resets-on-newline.
All feed bytes via
MemoryStream/ a custom hostileStream.RED -> GREEN proof (honest)
long.MaxValue), forceddotnet build <Copilot.Tests>.csproj --no-incremental, then ran the filteredsuite: the two overflow e2e tests FAILED genuinely
(
result.StopReason should be StopReason.Error but was StopReason.Stop-- the17 MiB body and the 512 KiB line were read unbounded). The 4 direct
ByteCountingStreamtests stayed green because they pass explicit small capsvia the constructor, proving the e2e tests truly depend on the parser's cap
enforcement.
--no-incrementalagain (to defeatthe stale-DLL false-green trap), and re-ran: 7/7 passed.
Validation
scripts/repo/test-impacted.ps1from the worktree: printedAll impacted tests passed.(9 projects, incl.Architecture.TestsandScenarios.Testssafety nets; Copilot tests 120/120).Merge Notes
fix(cron): purge retention on real terminal statuses (ok/error/timed_out) #1669, refactor(blazor-client): extract ToChatMessage factory, split history loader, unify user echo #1670, refactor(subagent): decompose SpawnAsync into named helpers #1671, fix(blazor-client): observe reconnect fire-and-forget and synchronize shared HashSet state #1672, fix(cron): scope cron create/update target agent to the calling agent #1673). Those touch
BotNexus.Gateway,BotNexus.Cron,BotNexus.Agent.Core, and the Blazor client; this PR isconfined entirely to
src/agent/BotNexus.Agent.Providers.Copilot/**and itstest project. No expected merge conflicts.
reuses its
BoundedHttpContent.DefaultMaxResponseBytesconstant andResponseContentTooLargeExceptiontype rather than redefining them.AnthropicStreamParser.ProcessStreamAsyncshares the identical read-loopshape and has the same unbounded-read exposure, but it lives in a separate
project (
BotNexus.Agent.Providers.Anthropic). Applying the guard there wouldrequire either duplicating
ByteCountingStreamor relocating it toBotNexus.Agent.Providers.Core, which broadens scope and risk. Per theissue's guidance to "prefer a tight, single-surface PR," it is left as a
follow-up (move the guard to Core, then apply it to the Anthropic parser
with its own tests).