Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

HEAD response can include Content-Length header #1163

Merged
merged 1 commit into from
Oct 12, 2016
Merged

HEAD response can include Content-Length header #1163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,21 @@ private void VerifyAndUpdateWrite(int count)
_responseBytesWritten += count;
}

protected void VerifyResponseContentLength()
{
var responseHeaders = FrameResponseHeaders;

if (!HttpMethods.IsHead(Method) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you check that if HttpMethods.IsHead(Method) && responseHeaders.ContentLengthValue.HasValue then _responseBytesWritten == 0 || _responseBytesWritten == responseHeaders.ContentLengthValue.Value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot that we actually ignore writes now for HEAD responses. I guess what you have is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though we count those bytes, we ignore writes to HEAD responses. It would alert the app that there's an issue, but the error would surface anyways on a response to the same resource using a different method.

!responseHeaders.HasTransferEncoding &&
Copy link
Member

@halter73 halter73 Oct 12, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have to check !responseHeaders.HasTransferEncoding.HasValue since your are checking responseHeaders.HasContentLength?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I prefer checking responseHeaders.HeaderContentLengthValue even though you fixed the set raw issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The spec allows both and Transfer-Encoding should take precedence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes. I think the spec suggests removing the content-length header in this case which makes sense to me.

   If a message is received with both a Transfer-Encoding and a
   Content-Length header field, the Transfer-Encoding overrides the
   Content-Length.  Such a message might indicate an attempt to
   perform request smuggling (Section 9.5) or response splitting
   (Section 9.4) and ought to be handled as an error.  A sender MUST
   remove the received Content-Length field prior to forwarding such
   a message downstream.

https://tools.ietf.org/html/rfc7230#section-3.3.3

@Tratcher thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of scope of the current issue. Apps almost never set Transfer-Encoding so I'm not too worried about it right now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halter73 might want to add that as a separate issue to request parsing? (and proxy https://github.com/aspnet/Proxy though I assume the httpclient its using would be handling it)

responseHeaders.HeaderContentLengthValue.HasValue &&
_responseBytesWritten < responseHeaders.HeaderContentLengthValue.Value)
{
_keepAlive = false;
ReportApplicationError(new InvalidOperationException(
$"Response Content-Length mismatch: too few bytes written ({_responseBytesWritten} of {responseHeaders.HeaderContentLengthValue.Value})."));
}
}

private void WriteChunked(ArraySegment<byte> data)
{
SocketOutput.Write(data, chunk: true);
Expand Down
12 changes: 2 additions & 10 deletions src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Hosting.Server;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Logging;

namespace Microsoft.AspNetCore.Server.Kestrel.Internal.Http
Expand Down Expand Up @@ -92,16 +93,7 @@ public override async Task RequestProcessingAsync()
try
{
await _application.ProcessRequestAsync(context).ConfigureAwait(false);

var responseHeaders = FrameResponseHeaders;
if (!responseHeaders.HasTransferEncoding &&
responseHeaders.HasContentLength &&
_responseBytesWritten < responseHeaders.HeaderContentLengthValue.Value)
{
_keepAlive = false;
ReportApplicationError(new InvalidOperationException(
$"Response Content-Length mismatch: too few bytes written ({_responseBytesWritten} of {responseHeaders.HeaderContentLengthValue.Value})."));
}
VerifyResponseContentLength();
}
catch (Exception ex)
{
Expand Down
56 changes: 56 additions & 0 deletions test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,62 @@ await connection.Receive(
Assert.Equal(0, testLogger.ApplicationErrorsLogged);
}

[Fact]
public async Task HeadResponseCanContainContentLengthHeader()
{
var testLogger = new TestApplicationErrorLogger();
var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) };

using (var server = new TestServer(httpContext =>
{
httpContext.Response.ContentLength = 42;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a HEAD test where we set the content-length and attempt to write a response body in the app. We have ResponseTests.ResponseBodyNotWrittenOnHeadResponseAndLoggedOnlyOnce, but it doesn't attempt to set a content-length.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

return TaskCache.CompletedTask;
}, serviceContext))
{
using (var connection = server.CreateConnection())
{
await connection.SendEnd(
"HEAD / HTTP/1.1",
"",
"");
await connection.ReceiveEnd(
$"HTTP/1.1 200 OK",
$"Date: {server.Context.DateHeaderValue}",
"Content-Length: 42",
"",
"");
}
}
}

[Fact]
public async Task HeadResponseCanContainContentLengthHeaderButBodyNotWritten()
{
var testLogger = new TestApplicationErrorLogger();
var serviceContext = new TestServiceContext { Log = new TestKestrelTrace(testLogger) };

using (var server = new TestServer(async httpContext =>
{
httpContext.Response.ContentLength = 12;
await httpContext.Response.WriteAsync("hello, world");
}, serviceContext))
{
using (var connection = server.CreateConnection())
{
await connection.SendEnd(
"HEAD / HTTP/1.1",
"",
"");
await connection.ReceiveEnd(
$"HTTP/1.1 200 OK",
$"Date: {server.Context.DateHeaderValue}",
"Content-Length: 12",
"",
"");
}
}
}

public static TheoryData<string, StringValues, string> NullHeaderData
{
get
Expand Down