Skip to content
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

HttpClient/HttpRequestMessage DELETE in netcoreapp2.1 now includes ContentLength: 0 when it didn't in previous netcoreapp versions #26860

Closed
matthchr opened this issue Jul 19, 2018 · 5 comments · Fixed by dotnet/corefx#32259
Labels
area-System.Net.Http bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@matthchr
Copy link

All of the below applies to Windows only, I haven't tested on Linux, although maybe I need to...

My service performs shared key signing to generate an auth header. We include the content-length in this signature if the request sets one. This means I need to know if the HttpClient will send a request with Content-Length or not.

Unfortunately, .NET Core and .NET Framework seem quite inconsistent about when they include content-length in the request for some request types (DELETE for example), and as far as I know there is no way to tell if it will be there or not other than to hardcode it.

Right now, .NET Framework always includes Content-Length: 0 as a header for DELETE, PATCH, and OPTIONS. In netcoreapp versions <= 2.0 it does not include Content-Length: 0 for those same operations. But starting in netcoreapp 2.1 it seems like it does include Content-Length: 0.

You can observe this behavior yourself by trying the below code-snippet in netcoreapp <= 2.0 and netcoreapp2.1, and inspect the request using Fiddler or similar

using (HttpClient client = new HttpClient())
{
    var request = new HttpRequestMessage();
    request.RequestUri = new Uri("http://this.url.doesnt.exist");
    request.Method = new HttpMethod("DELETE");
    client.SendAsync(request).GetAwaiter().GetResult();
}

I have a few questions:

  1. Is the intended behavior for all operations to include Content-Length always (that seems to be the new behavior in .NET Core and mirrors the old behavior in .NET Framework).
  2. Is there any way for me to hook things at some point that I will know if the content-length will be included or not?
  3. Is there some option on HttpClient/HttpClientHandler I am missing that controls these defaults?

It does look like if I explicitly add an empty body to my DELETE requests like so:
request.Content = new ByteArrayContent(Array.Empty<Byte>()); I can at least force a consistent behavior across netcoreapp versions, so that's probably what I'm going to do, but this change caught my admittedly pretty fragile code by surprise.

@davidsh
Copy link
Contributor

davidsh commented Jul 19, 2018

cc: @geoffkizer @karelz @stephentoub

Change in behavior due to using SocketsHttpHandler in .NET Core 2.1.

@matthchr
Copy link
Author

matthchr commented Jul 19, 2018

For what it's worth, https://tools.ietf.org/html/rfc7230 suggests that the <= netcoreapp2.0 behavior was more compliant

A user agent SHOULD NOT send a Content-Length header field when the request message does not contain a payload body and the method semantics do not anticipate such a body.

@Porges
Copy link

Porges commented Jul 19, 2018

Ideally there would be a way to give HttpClientHandler an inner DelegatingHandler, which would be able to process the really-truly-honestly-final, fully-populated HttpRequestMessage before it hits the wire 🙂

@karelz
Copy link
Member

karelz commented Jul 19, 2018

Sounds like something we could fix, but I don't think it is critical for servicing 2.1.x. The behavior is border line and likely doesn't hurt too many developers (the code has to be fairly sensitive to implementation details).

@Porges what would be benefit of such extensibility beyond compensating for bugs? It seems like a too big hammer to compensate for bugs, I would rather prefer to fix them instead of exposing such API. For logging purposes we should have DiagnosticsSource hooks.

@geoffkizer
Copy link
Contributor

The question is how to interpret the phrase "the method semantics do not anticipate such a body" from the RFC, as quoted above.

Re DELETE, the RFC says:

A payload within a DELETE request message has no defined semantics;
sending a payload body on a DELETE request might cause some existing
implementations to reject the request.

That's not entirely clear, but the most reasonable interpretation seems to be that DELETE does not "anticipate" a body and we should not send Content-Length: 0 in this case.

OPTIONS is even less clear:

A client that generates an OPTIONS request containing a payload body
MUST send a valid Content-Type header field describing the
representation media type. Although this specification does not
define any use for such a payload, future extensions to HTTP might
use the OPTIONS body to make more detailed queries about the target
resource.

I guess for consistency with previous versions, we should not send Content-Length: 0 in this case either.

The PATCH method pretty clearly "anticipates" a body, I think. The behavior of previous versions here is arguably incorrect, and is due to the fact that PATCH was not treated as a known method previously.

So I'd suggest we change the behavior of DELETE and OPTIONS so that we don't send Content-Length: 0. But as Karel mentions, it's probably not a critical issue.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants