Enforce HTTP request Content-Length correctness#62541
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFixes #62258 Like is already the case for HTTP/3, throw on an attempt to write too much request content. This means the server will never receive more than Also throw if we end up sending too few bytes.
|
src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs
Show resolved
Hide resolved
| { | ||
| string msg = SR.net_http_content_write_larger_than_content_length; | ||
| throw new IOException(msg, new HttpRequestException(msg)); | ||
| throw new HttpRequestException(SR.net_http_content_write_larger_than_content_length); |
There was a problem hiding this comment.
Do we care about this being an IOException?
It's technically a combination of IO + wrong headers.
Currently you would get
new HttpRequestException(
net_http_content_stream_copy_error,
new IOException(
net_http_content_write_larger_than_content_length,
new HttpRequestException(
net_http_content_write_larger_than_content_length)))after this PR
new HttpRequestException(
net_http_content_write_larger_than_content_length)(this extends to the rest of the exceptions we are throwing in this PR)
There was a problem hiding this comment.
I assume the change here is to provide the same exception chain as with H/2 and HTTP/1.1? If so, I don't see a reason against.
There was a problem hiding this comment.
I understand not wanting to change the outer exception type at this point, but why wouldn't this cause an InvalidOperationException? That's what gets thrown by Kestrel if you try to write too much to the response stream after setting a Content-Length header. To me, it indicates a bug in the program.
Edit: On second thought, why not change the outer exception type? I hope nothing is relying on HttpClient throwing an HttpRequestException if they give it a too-large request body.
There was a problem hiding this comment.
why not change the outer exception type? I hope nothing is relying on HttpClient throwing an HttpRequestException
HttpClient will always try to throw either a TaskCanceledException or an HttpRequestException.
Aside from initial argument validation, If something else gets thrown somewhere, we'll wrap it in HttpRequestException.
Sending too much or too little is the same kind of issue - the header you gave us doesn't match the content. Ideally, the user should see a similar kind of error.
why wouldn't this cause an InvalidOperationException? That's what gets thrown by Kestrel if you try to write too much to the response stream
Most HttpClient users won't actually do the WriteAsync that goes over the limit themselves. They will use the built-in HttpContent types.
var content = new StreamContent(myStream);
content.Headers.ContentLength = someContentLength;
await client.PostAsync("foo", content);I wouldn't expect this to throw InvalidOperationException as a user. It's not something I would consider an invalid operation, but more an argument/input exception.
Similarly, I wouldn't expect this to throw something different if my content length calculation was off in the other direction.
Fixes #62258
Like is already the case for HTTP/3, throw on an attempt to write too much request content. This means the server will never receive more than
Content-Lengthbytes.Also throw if we end up sending too few bytes.