Reject POST and PUT requests with no Content-Length or Transfer-Encoding (#1130) #1193
Conversation
| { | ||
| long contentLength; | ||
| if (!long.TryParse(unparsedContentLength, out contentLength) || contentLength < 0) | ||
| if (!long.TryParse(unparsedContentLength.ToString(), out contentLength) || contentLength < 0) |
There was a problem hiding this comment.
I thought we were going to change this to use your new FrameHeaders.UseContentLength method.
| @@ -280,6 +281,13 @@ protected virtual void OnConsumedBytes(int count) | |||
| } | |||
| } | |||
|
|
|||
| // If we got here, request contains no Content-Length or Transfer-Encoding header. | |||
| // Reject with 411 Length Required. | |||
| if (HttpMethods.IsPost(context.Method) || HttpMethods.IsPut(context.Method)) | |||
There was a problem hiding this comment.
No user code has run at this point. Do a strict reference equals.
There was a problem hiding this comment.
Were there any other methods you considered requiring a content length for?
There was a problem hiding this comment.
Nope. IIS and nginx seem to require a length only for POST and PUT, so I'm doing the same here.
| @@ -196,13 +196,29 @@ public void ForThrowsWhenFinalTransferCodingIsNotChunked() | |||
| using (var input = new TestInput()) | |||
| { | |||
| var ex = Assert.Throws<BadHttpRequestException>(() => | |||
| MessageBody.For(HttpVersion.Http10, new FrameRequestHeaders { HeaderTransferEncoding = "chunked, not-chunked" }, input.FrameContext)); | |||
| MessageBody.For(HttpVersion.Http11, new FrameRequestHeaders { HeaderTransferEncoding = "chunked, not-chunked" }, input.FrameContext)); | |||
There was a problem hiding this comment.
Technically an HTTP/1.0 request shouldn't be chunked. Not sure we want to change this to reject such requests - we can an open an issue for it though.
| { | ||
| input.FrameContext.Method = method; | ||
| var ex = Assert.Throws<BadHttpRequestException>(() => | ||
| MessageBody.For(HttpVersion.Http11, new FrameRequestHeaders(), input.FrameContext)); |
There was a problem hiding this comment.
Should we test this for HTTP/1.0 too?
There was a problem hiding this comment.
Good point. I've checked the HTTP/1.0 spec and we have to respond with a 400 in case it's HTTP/1.0 - there's no 411 status code in HTTP/1.0.
There was a problem hiding this comment.
If a request
contains an entity body and Content-Length is not specified, and the
server does not recognize or cannot calculate the length from other
fields, then the server should send a 400 (bad request) response.
|
@halter73 Updated with early check for != GET. |
|
Both travis builds are failing with "Block being garbage collected instead of returned to pool: IncomingStart, /Users/travis/build/aspnet/KestrelHttpServer/src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/SocketInput.cs, 66" https://travis-ci.org/aspnet/KestrelHttpServer/builds/173249429 |
| [Theory] | ||
| [InlineData("POST")] | ||
| [InlineData("PUT")] | ||
| public void ForThrowsWhenMethodRequiresLengthButNoContentLengthSetHttp10(string method) |
There was a problem hiding this comment.
My guess is that this test is throwing causing the block to be leaked.
There was a problem hiding this comment.
Never mind this issue repros in the dev branch. I found your issue #1195.
|
Opened https://github.com/dotnet/corefx/issues/13369 to track test failures caused by hang on 0-byte async write to |
cesarblum
force-pushed
the
cesarbs/reject-requests-no-length
branch
from
6a40315 to
7f1ffa0
Compare
|
Rebased with b3aca04. |
cesarblum
force-pushed
the
cesarbs/reject-requests-no-length
branch
from
7f1ffa0 to
a1c5987
Compare
#1130
@halter73 @davidfowl @Tratcher