HEAD response can include Content-Length header #1163
Conversation
| var responseHeaders = FrameResponseHeaders; | ||
|
|
||
| if (!HttpMethods.IsHead(Method) && | ||
| !responseHeaders.HasTransferEncoding && |
There was a problem hiding this comment.
Do you have to check !responseHeaders.HasTransferEncoding.HasValue since your are checking responseHeaders.HasContentLength?
There was a problem hiding this comment.
And I prefer checking responseHeaders.HeaderContentLengthValue even though you fixed the set raw issue.
There was a problem hiding this comment.
Yes. The spec allows both and Transfer-Encoding should take precedence.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
out of scope of the current issue. Apps almost never set Transfer-Encoding so I'm not too worried about it right now.
There was a problem hiding this comment.
@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)
| { | ||
| var responseHeaders = FrameResponseHeaders; | ||
|
|
||
| if (!HttpMethods.IsHead(Method) && |
There was a problem hiding this comment.
Should you check that if HttpMethods.IsHead(Method) && responseHeaders.ContentLengthValue.HasValue then _responseBytesWritten == 0 || _responseBytesWritten == responseHeaders.ContentLengthValue.Value?
There was a problem hiding this comment.
I forgot that we actually ignore writes now for HEAD responses. I guess what you have is fine.
There was a problem hiding this comment.
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.
|
|
||
| using (var server = new TestServer(httpContext => | ||
| { | ||
| httpContext.Response.ContentLength = 42; |
There was a problem hiding this comment.
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.
|
|
b
cesarblum
force-pushed
the
cesarbs/head-response-content-length
branch
from
7a4fe14 to
ffc3eb3
Compare
StaticFiles tests are failing because of this.
@halter73 @mikeharder @Tratcher