Don't send body in HEAD response when using PipeWriter.Advance before headers flushed

[BrennanConroy]

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 the PipeWriter which uses the GetMemory + Advance path.

The fix is to pass the canWriteBody parameter to WriteDataWrittenBeforeHeaders and only copy the bytes from previous GetMemory + Advance calls to the body if canWriteBody is true.

One additional change we might want to consider here, is removing the Transfer-Encoding: chunked header 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.2

The server SHOULD send the same
header fields in response to a HEAD request as it would have sent if
the request had been a GET, except that the payload header fields
(Section 3.3) MAY be omitted.

And https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding

When present on a response to a HEAD request that has no body, it indicates the value that would have applied to the corresponding GET message.

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.

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]

#7110 removed the sync write which this test and HeadResponseBodyNotWrittenWithSyncWrite above were explicitly testing.

[BrennanConroy]

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.

[mgravell]

LGTM, interesting find!

[dotnet-policy-service]

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

[BrennanConroy]

There was a bug in the PR where calling PipeWriter.WriteAsync went through a code path that didn't set _canWriteBody, and since we weren't resetting the value it could result in a normal request after a non-body request not writing the entire body. Fixed by both resetting the _canWriteBody value, and also changing how we set the _canWriteBody to work for all code paths.

[halter73]

How is _autoChunk ever true when _canWriteBody is false? _autoChunk is set inside of HttProtocol.CreateResponseHeaders(bool appCompleted) and this is always called afterwards, so we should always be able to identify that we're not auto chunking HEAD requests.

Can we just write an assert that this invariant holds? Or if it doesn't hold, can we make it hold? It doesn't make sense to me to set _autoChunk unless/until we actually know we're auto chunking.

[halter73]

I don't love splitting this state between Http1OutputProducer and HttpProtocol. Some of the usage in HttpProtocol now appears to be vestigial to me.

I know we're already sharing the _autoChunk state which is part of the problem. The other problem seems to be that neither true nor false is a good default state for either _autoChunk or _canWriteBody since we don't know what either will end up being until response headers are sent, but it's still possible to call into the Http1OutputProducer before then for buffering purposes. I think the better solution would be to combine _autoChunk and _canWriteBody into a 4-state enum. This probably would still have to be shared between HttpProtocol and Http1OutputProducer but at least it's only one field then.

Then we can continue to pass it via WriteResponseHeaders instead of adding a new SetCanWriteBody method.

enum ResponseBodyMode
{
    Uninitialized,
    Disabled,
    Chunked,
    ContentLength
}

[halter73]

Both of which indicate it is perfectly fine to include the Transfer-Encoding header.

I'm fine with changing the behavior here if we think it's better. Personally, unlike Content-Length, I don't see the value in sending the Transfer-Encoding header in response to a HEAD request, but it's more important to me that writing to the response Stream or BodyWriter behaves the same way.