Skip to content

Enforce HTTP request Content-Length correctness #62541

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

MihaZupan
Copy link
Member

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-Length bytes.

Also throw if we end up sending too few bytes.

@MihaZupan MihaZupan added this to the 7.0.0 milestone Dec 8, 2021
@ghost
Copy link

ghost commented Dec 8, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

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-Length bytes.

Also throw if we end up sending too few bytes.

Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http

Milestone: 7.0.0

@MihaZupan MihaZupan requested a review from a team December 8, 2021 17:39
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@@ -414,8 +422,7 @@ private async ValueTask WriteRequestContentAsync(ReadOnlyMemory<byte> buffer, Ca

if (buffer.Length > _requestContentLengthRemaining)
{
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);
Copy link
Member Author

@MihaZupan MihaZupan Dec 9, 2021

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@halter73 halter73 Dec 10, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@MihaZupan MihaZupan closed this Dec 10, 2021
@MihaZupan MihaZupan reopened this Dec 10, 2021
@MihaZupan MihaZupan merged commit 7b2925b into dotnet:main Dec 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SocketsHttpHandler does not enforce request Content-Length correctness
3 participants