Runtime: Bug in HttpContent.ReadAsStringAsync() if the content-type charset is surrounded with quotes (which is allowed by http spec)

[tjamin]

I have just found a small bug in System.Net.HttpContent.ReadAsStringAsync(). This method tries to get the encoding from the content-type HTTP header. Unfortunately the HTTP spec allows to surround the charset value with quotes as in the following example:

application/json; charset="utf-8"

You can find the spec here:

https://tools.ietf.org/html/rfc2616#section-3.6

Of course Encoding.GetEncoding("\"utf-8\"") throws an ArgumentException because of the surrounding quotes which leads to an InvalidOperationException in System.Net.HttpContent.ReadAsStringAsync().

This is the corresponding line:

https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/HttpContent.cs#L204

[davidsh]

RFC2616 is an older spec which is now deprecated. RFC7230 - RFC7235 are the current specs:
https://tools.ietf.org/html/rfc7230

We'll take a look to see what can be done to fix this. We probably have the same parsing logic in Desktop as well.

[tjamin]

Hey David. You absolutely have the same parsing logic in the desktop framework! I recognized the problem with the desktop framework 4.5 but I didn't find the source code on github. Isn't the full framework open source as well????

The current spec RFC7231 also defines charset as "token | quoted-string" and even the examples contains an example with quotes around the charset:

 text/html;charset=utf-8
 text/html;charset=UTF-8
 Text/HTML;Charset="utf-8"
 text/html; charset="utf-8"

[davidsh]

The full .NET Framework (Desktop) has not been open sourced on GitHub. However, the source code is available for viewing/debugging thru referencesource.microsoft.com

In terms of GitHub CoreFx repo, the HttpClient source is here:
https://github.com/dotnet/corefx/tree/master/src/System.Net.Http/src/System/Net/Http

[CIPop]

There are two ways to look at this issue:

1. Decoding data from the wire to the application:

The RFC clearly accepts all 4 examples above.
@tjamin, do we have a bug if the quoted string is received on the wire?

1. Encoding data from the application to the wire:

From https://tools.ietf.org/html/rfc7231#section-3.1.1.1:

For example, the following examples are all equivalent, but the first is preferred for consistency:

     text/html;charset=utf-8
     text/html;charset=UTF-8
     Text/HTML;Charset="utf-8"
     text/html; charset="utf-8"

I'm not completely convinced that we should allow the quoted string. Even if we would accept it, we should still transform it to the RFC preferred form on the wire.

[tjamin]

@CIPop I am not sure what exactly you mean with your question "do we have a bug...". Sure, there is a bug as I mentioned: Encoding.GetEncoding(""utf-8"") throws an ArgumentException because of the surrounding quotes which leads to an InvalidOperationException in System.Net.HttpContent.ReadAsStringAsync().

You are not completely convinced that the code should allow the quoted string??? Really? If you want to be RFC compliant you have to. Am I right?

The line https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/HttpContent.cs#L203 throws the exception. The value of the property headers.ContentType.CharSet has to be utf-8 and not "utf-8". The quick fix is to trim the surrounding quotes before calling Encoding.GetEncoding...

[jnm2]

Can this be tagged to port to desktop please?

[CIPop]

Can this be tagged to port to desktop please?

Done.

@CIPop I am not sure what exactly you mean with your question "do we have a bug...". Sure, there is a bug as I mentioned: Encoding.GetEncoding(""utf-8"") throws an ArgumentException

I mean if the quoted string is received _from the wire_, will it be properly decoded by our API? Your example setting the encoding programmatically isn't from the wire, but from the application. (I.e. we'd need a test to check that if a server replies with the quoted formatted string, HttpClient would not crash.)

You are not completely convinced that the code should allow the quoted string??? Really? If you want to be RFC compliant you have to. Am I right?

We want to be RFC compliant. What I'm saying is that I'm not sure the RFC mandates accepting quotes. It actually recommends against using them, instead normalizing to the first example.
In detail, here's my interpretation of the RFC:

1. On the wire, the preferred bytes that need to be sent are in the form of text/html;charset=utf-8. (per above quote from the RFC)
2. The RFC doesn't mention anything about APIs accepting certain strings or if these should be accepted as quoted or not.
3. Even if we decide to accept the quoted string in the API (and not throw), given point 1, the RFC above is recommending normalization before sending the data to the other endpoint.

Could you point out the scenario that is impacted by not allowing the quotes? Is there a client/server application that would not function well if quotes are not present?
Would point 3 be acceptable or are you asking to make possible sending all formats on the wire?

[tmenier]

@CIPop I can point out a scenario that is affected:

https://github.com/tmenier/Flurl/pull/76

In short, clients working with an API that they have no control over that return the charset in quotes. Even if the RFC _recommends_ against it, strictly speaking the quotes are legal as you pointed out.

[CIPop]

tmenier/Flurl#76

/cc @davidsh, @karelz

Thanks @tmenier. From my understanding of the issue, your example shows that HttpContent will fail trying to decode a quoted response from the server (scenario 1 above).

To fix this I recommend starting with a simple client/server test where the server is designed to reply with all allowed RFC forms (see above).
The fix is already pointed out by @tjamin in the description: we should add code to parse the string per RFC rules then extract the Encoding.GetEncoding compatible string (or fail if the format is incorrect).

[CIPop]

BTW: Should be very simple to fix and test (some test ideas at tmenier/Flurl#76) . Anyone wants to give it a try?

[lafe]

I just encountered the issue myself while connecting to the UPNP endpoint of my router (AVM Fritz!Box).
From my point of view, the framework should be able to parse such data, because "in the real world" there are imperfect implementations that we have to be able to connect to (and in this case it is not even incorrect but only frowned upon). If we can't rely on the framework to handle these imperfect implementations, we are forced to workaround these issues and write our own parsing method.

[karelz]

@lafe just to clarify if I fully understood your reply:
I think we are all on the same page - as @CIPop says above, we agree and support code changes that make CoreFX code conform with RFC spec. That's why the issue is marked as "up for grabs" (anyone can grab it).

[kellypleahy]

@karelz I was going to go after this one, but it seems like the way we should handle this is that the ContentType.CharSet should be stripped of the quotes before it gets to the line of code indicated above in the OP comment: https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/HttpContent.cs#L205

I would propose that the header parsing for the ContentType header should be changed to strip out the quotes and return the string in the canonical form recommended by the RFC.

Comments?

[kellypleahy]

I think the fix belongs here: https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/Headers/MediaTypeHeaderValue.cs#L25

And I also think that assignments to this value (the setter for CharSet) should not modify the value provided (i.e. leave it as supplied). I'm not as set on this opinion as I am on the first ;)

[karelz]

I will let area experts to answer - @davidsh @CIPop

[davidsh]

I would propose that the header parsing for the ContentType header should be changed to strip out the quotes and return the string in the canonical form recommended by the RFC.

We should not change the behavior of these public APIs to returns something different. So, "header should be changed to strip out the quotes and return the string in the canonical form recommended by the RFC" is something that can break app-compat.

However, we can, possibly, be more permissible in what we parse from the wire. So, instead of throwing an error because of a quoted-string, we could allow it. In general, our principle in networking is to "be lenient in what we read, but strict in what we write". There are some exceptions to this because having a public API means that developers will retry on the "valid" or "invalid" decisions we making when they call our APIs to 'validate' something. But in cases of parsing data on the wire, it is a case-by-case decision about whether we should be "flexible" in parsing it instead of throwing an error.

[kellypleahy]

OK. I guess that means the fix must be made in the usage of this header, not the header parser itself. That's a shame because it means everyone else in the world that encounters this on some weird server that is sending it that way will also have to do the same double-check. I was hoping to avoid that 'virus' of 💩

That said, if we're really sure we can't change the behavior of this, I guess that's the only way to go. Personally I'd vote for the breaking change if it were my decision, as "utf-8" and utf-8 and UTF-8, ... are all equivalent in the eyes of the RFC. Also, given that the current functionality doesn't work if presented with a quoted string, doesn't that mean that there's no real "breaking change"? I guess that depends on whether the read or write site has the issue (or both).

[Eirenarch]

I just encountered what I assume is the same bug trying to use ReadStringAsync() trying to generate link preview for our system for a facebook URL. I'd say facebook is fairly popular site so this fix is needed.

[osamaalnuimi]

I just encountered what I assume is the same bug trying to use ReadStringAsync() trying to generate link preview for our system for a facebook URL. I'd say facebook is fairly popular site so this fix is needed.

+1

[pranavmehta]

+1 as I have the same issue with Facebook (example URL: https://www.facebook.com/groups/sem.marketing/) as well.

[rmkerr]

Thanks for the additional info! It's good to know this issue is really something users are running in to.

The change resolving this issue was merged in dotnet/corefx#33280, and will ship with the next release of .NET Core. Currently I don't think that we plan to port it back to older versions, but if it's something that the community really needs that could change.

[Eirenarch]

@rmkerr as someone who does run into this issue today I'd rather get a backported fix rather than deal with workarounds that I would later remove. I am not sure that justifies backporting.

[karelz]

@Eirenarch backporting brings risk, it has associated cost.
How bad is the workaround? How bad is it to live with it?
How many devs are impacted to the point they would like to see a backport?

Answers to these questions might change our mind and backport it.

[Eirenarch]

As I said I am not sure my use case justifies it. If I was making a decision about the .NET Framework I probably wouldn't backport. For me the issue is annoying but I can live with it (probably will let the bug be). I also have the freedom to upgrade quite fast after the new framework arrive (hopefully I will still have this freedom when it arrives) but it might be more annoying for people who can't.

[russpenska]

I believe this was fixed in the .NET Core, but looks like it's still an issue in . NET Framework (at least in 4.6.2). Does this need to be raised as a bug somewhere else to have it fixed in Framework?

[MaceWindu]

@russpenska it was never fixed in netfx (still an issue in 4.8), but could be easily workarounded using custom EncodingProvider which unquote charset name.

https://docs.microsoft.com/en-us/dotnet/api/system.text.encoding.registerprovider