-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TransferEncoding chunked testing only if it is last header value #105647
TransferEncoding chunked testing only if it is last header value #105647
Conversation
src/libraries/System.Net.Http/tests/UnitTests/Headers/HttpHeadersTest.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also changing how we interpret response bodies that contain "chunked" as the non-last value.
By the spec, we should likely be falling back to reading until the connection is closed instead of using the content length value.
We rely on headers.TransferEncodingChunked
to decide whether to apply chunking when writing request bodies (e.g. in WinHttpHandler or SocketsHttpHandler's HttpConnection.cs
).
Instead of potentially generating garbage, should we tighten request validation to throw if a Transfer-Encoding
header was set but chunked
wasn't the last value?
Is this part in H3 correct?
https://datatracker.ietf.org/doc/html/rfc9114#section-4.1-11
runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs
Lines 653 to 657 in e60db07
// H3 does not support Transfer-Encoding: chunked. | |
if (request.HasHeaders && request.Headers.TransferEncodingChunked == true) | |
{ | |
request.Headers.TransferEncodingChunked = false; | |
} |
Should we unconditionally remove the header instead?
Our docs currently state:
Gets or sets a value that indicates if the Transfer-Encoding header for an HTTP request contains chunked.
This feels like it has a non-zero risk of breaking someone. Should we push it to .NET 10 just in case?
{ | ||
if (_parent.ContainsParsedValue(KnownHeaders.TransferEncoding.Descriptor, HeaderUtilities.TransferEncodingChunked, lastValueOnly: false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will now do an extra pass over the values in the common case where the value wasn't set yet.
Can we instead do something like
_parent.RemoveParsedValue(KnownHeaders.TransferEncoding.Descriptor, HeaderUtilities.TransferEncodingChunked);
_parent.AddParsedValue(KnownHeaders.TransferEncoding.Descriptor, HeaderUtilities.TransferEncodingChunked);
@@ -89,7 +89,7 @@ internal sealed class HttpGeneralHeaders | |||
return true; | |||
} | |||
|
|||
if (parent.ContainsParsedValue(KnownHeaders.TransferEncoding.Descriptor, HeaderUtilities.TransferEncodingChunked)) | |||
if (parent.ContainsParsedValue(KnownHeaders.TransferEncoding.Descriptor, HeaderUtilities.TransferEncodingChunked, lastValueOnly:true)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (parent.ContainsParsedValue(KnownHeaders.TransferEncoding.Descriptor, HeaderUtilities.TransferEncodingChunked, lastValueOnly:true)) | |
if (parent.ContainsParsedValue(KnownHeaders.TransferEncoding.Descriptor, HeaderUtilities.TransferEncodingChunked, lastValueOnly: true)) |
I believe this PR shall be abandoned. |
Fixes #63053
Context
Transfer-Encodings are ordered and so the check should only succeed if "chunked" is the final Transfer-Encoding value
See #63053
Changes made
Test only final value.
During
TransferEncodingChunked
is set to true, it is ensuredchunked
will be last value, even when it was already present.Testing
Few new unit tests.
Note
This could be a breaking change if server behave out of spec. Currently if server send
Transfer-Encoding: chunked, gzip
, but actually do gzip and then chunk, client would have handled it without error, with changes in this PR it will fail to receive content.