Don't send body in HEAD response when using PipeWriter.Advance before headers flushed #59725
+186
−28
Conversation
This file contains 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
BrennanConroy
added
feature-kestrel
area-networking
BrennanConroy
requested review from
halter73,
JamesNK and
mgravell
as code owners
There was a problem hiding this comment.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/Servers/Kestrel/Core/src/Internal/Http/IHttpOutputProducer.cs: Evaluated as low risk
BrennanConroy
commented
Jan 6, 2025
| @@ -1471,7 +1534,7 @@ public async Task HeadResponseBodyNotWrittenWithSyncWrite() | |||
| await using (var server = new TestServer(async httpContext => | |||
| { | |||
| httpContext.Response.ContentLength = 12; | |||
| await httpContext.Response.BodyWriter.WriteAsync(new Memory<byte>(Encoding.ASCII.GetBytes("hello, world"), 0, 12)); | |||
| httpContext.Response.Body.Write(Encoding.ASCII.GetBytes("hello, world"), 0, 12); | |||
There was a problem hiding this comment.
#7110 removed the sync write which this test and HeadResponseBodyNotWrittenWithSyncWrite above were explicitly testing.
BrennanConroy
commented
Jan 6, 2025
| } | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task HeadResponseBodyNotWrittenWithAsyncWrite() | ||
| public async Task HeadResponseHeadersWrittenWithAsyncWriteBeforeAppCompletes() |
There was a problem hiding this comment.
aspnet/KestrelHttpServer#1204 changed some of the HEAD response tests to only check the response headers are flushed and doesn't fully check that the body doesn't exist. So split the test into two for the flushed headers check, and the lack of body check.
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.
Bug exposed/introduced by #8199
Fixes #59691
Reason this hasn't been found in the >5 years since the bug was introduced is that it requires using
PipeWriter.GetMemory(...)+PipeWriter.Advance(...)before flushing the headers and using a HEAD request at the same time. This is very uncommon except for in .NET 9 where we made writing JSON responses use thePipeWriterwhich uses theGetMemory+Advancepath.The fix is to pass the
canWriteBodyparameter toWriteDataWrittenBeforeHeadersand only copy the bytes from previousGetMemory+Advancecalls to the body ifcanWriteBodyis true.One additional change we might want to consider here, is removing the
Transfer-Encoding: chunkedheader from the HEAD response. We have TransferEncodingNotSetOnHeadResponse and ManuallySettingTransferEncodingThrowsForHeadResponse which show us explicitly not letting the transfer encoding exist on a HEAD response, however according to the RFC 7231 section 4.3.2And https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding
Both of which indicate it is perfectly fine to include the Transfer-Encoding header.
Looking at the history of why we added this restriction to Kestrel, we find aspnet/KestrelHttpServer#952 which was about a browser request hanging when accessing an endpoint that responded with another non-body response in the form of a 304 and we just added the restriction to HEAD response as well since it is a non-body response.