Added content-length and guard clauses to HttpContentStream

[zbalkan]

Solves #49

[Copilot]

Pull request overview

Adds HTTP/1.1 message framing validation to HttpContentStream so that when Content-Length is present, already-buffered body bytes cannot exceed the declared length (per RFC 9112 §6.3), addressing TechnitiumLibrary issue #49.

Changes:

• Added constructor guard clauses for baseStream, buffer, and argument ranges.
• Added Content-Length validation and a protocol error when buffered body bytes exceed Content-Length.
• Introduced System.Net.Http dependency to throw HttpRequestException for protocol violations.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The new bounds check if (offset < 0 || offset + length > buffer.Length) is incorrect for this type: in callers length is the number of bytes read into buffer (end index), and offset is the current position within those bytes. For common cases where offset > 0 (headers ended mid-buffer), offset + length can exceed buffer.Length even though both values are valid, causing the constructor to throw on well-formed messages. Validate the invariants as 0 <= offset && offset <= length && length <= buffer.Length (and only then compute bufferedBodyBytes = length - offset).

Suggested change

	if (offset < 0 || offset + length > buffer.Length)
	// Validate that 0 <= offset <= length <= buffer.Length
	if (offset < 0 || offset > length || length > buffer.Length)

[ShreyasZare]

Thanks for the PR. The HttpContentStream class is internal and the values passed to it are from HttpRequest and HttpResponse are passed after validation. Thus these additional validations are not really necessary.

[zbalkan]

I believe, even if it is an internal component, there's value to ensure the component is aligned with RFCs. To me, it's a quality assurance measure. If you're going to drop this class, that's another scenario but I'd this stays, in my humble opinion, the test may help preventing regressions in future changes.

[ShreyasZare]

I believe, even if it is an internal component, there's value to ensure the component is aligned with RFCs. To me, it's a quality assurance measure. If you're going to drop this class, that's another scenario but I'd this stays, in my humble opinion, the test may help preventing regressions in future changes.

The validation added in this PR is actually breaking RFC compliance since HTTP protocol supports pipelining. The buffer data read from the stream may contain more data (another request) than the content-length in the request being read.

Even if the stream has some random data being added, the HttpContentStream will read data only up to the content-length which is handled in the Read() method. The remaining data in the stream has to be read again as a separate HTTP request/response and any issue with this pending data must not affect the current request/response being read.

[ShreyasZare]

Just analyzed the code and there is bug in how the buffer is read later in scenario where buffer read more data than content length. Will see how that can be fixed.