Skip to content

System.Net.Http.Json: Consider relaxing content type verification in ReadFromJsonAsync #38713

Closed
#40594
Closed
System.Net.Http.Json: Consider relaxing content type verification in ReadFromJsonAsync#38713
#40594
Assignees
jozkee
Labels
area-System.Net.Httphelp wanted[up-for-grabs] Good issue for external contributors
Milestone
5.0.0

Description

@huoyaoyuan
huoyaoyuan
opened on Jul 2, 2020

I'm doing interop with aria2. It's using application/json-rpc for its returning content type, which is refused by ReadFromJsonAsync.

Proposed solutions:
  1. Add a boolean parameter to totally bypass content type checking.
  2. Pass a string as the expected content type.
  3. Widen the accepted content types to include it. I'm not very familiar to the specification of content type, and not sure whether StartsWith is abusing it.
Current workaround:

Copy the implementation and delete code doing verification.
This is opposed to the purpose of System.Net.Http.Json. They are mostly simple helpers. If there's some restriction makes it unusable, the whole type becomes no-sense.

Activity

  1. Dotnet-GitSync-Bot
    added
    area-System.Net.Http
    untriagedNew issue has not been triaged by the area owner
    on Jul 2, 2020

  3. scalablecory commented on Jul 2, 2020

    @scalablecory
    scalablecory
    on Jul 2, 2020
    Contributor

    @jozkee @layomia

  4. samsosa commented on Jul 3, 2020

    @samsosa
    samsosa
    on Jul 3, 2020

    Yeah. There are so many weird servers that send weird content media types, in which case System.Net.Http.Json would be worthless.

    The first option would solve all problems:

    1. Add a boolean parameter to totally bypass content type checking.

  5. scalablecory commented on Jul 6, 2020

    @scalablecory
    scalablecory
    on Jul 6, 2020
    Contributor

    This seems worth doing to me. At least to make it configurable.

  6. scalablecory
    assigned
    jozkee
    on Jul 6, 2020

  7. halter73 commented on Jul 6, 2020

    @halter73
    halter73
    on Jul 6, 2020
    Member

    Why would this need to be configurable? Couldn't you just check the Content-Type header yourself before calling ReadFromJsonAsync if you want content type checking? If the body's not actually Json, parsing would fail anyway.

  8. geoffkizer commented on Jul 6, 2020

    @geoffkizer
    geoffkizer
    on Jul 6, 2020
    Contributor

    Do we enforce content type for other methods like ReadAsStringAsync?

    Seems like we just shouldn't check Content-Type here at all.

  9. scalablecory commented on Jul 6, 2020

    @scalablecory
    scalablecory
    on Jul 6, 2020
    Contributor

    Why would this need to be configurable? Couldn't you just check the Content-Type header yourself before calling ReadFromJsonAsync if you want content type checking? If the body's not actually Json, parsing would fail anyway.

    Yep, I should have been more clear -- I think we should get rid of the check. And if there is a reason to care (@rynowak? @jozkee?) then it should be configurable.

  10. jozkee commented on Jul 7, 2020

    @jozkee
    jozkee
    on Jul 7, 2020
    Member

    The main reason to have it was to have parity with the old APIs from System.Net.Http.Formatting
    https://dotnetfiddle.net/ijQAdJ

    Linking the original discussion for such check #33459 (comment).

    If other ReadAs* methods does not have such validation I would agree on adding a parameter to bypass the validation or even remove it so people that is not very experienced in using the HTTP APIs don't get frustrated by this error (dotnet/aspnetcore#21288).

    cc @pranavkm @terrajobst

    Keep in mind that these APIs are already available as a NuGet package that targets netcoreapp3.1 and netstandard2.0; if we decide to change behavior here would that be considered a breaking change?

  11. pranavkm commented on Jul 7, 2020

    @pranavkm
    pranavkm
    on Jul 7, 2020
    Contributor

    System.Net.Http.Formatting performed content-negotation, hence it made sense for it to complain about an unknown media type.

    With S.Net.Http.Json since you're explicitly asking to read JSON, there isn't much value in trying to validate this. What would help, is to include the media type as part of the error if deserializing fails and the content-type is unknown. This may help users self-diagnose a little better:

    try
{
   return DeserializeAsync();
} 
catch (JsonException ex) when (IsNotJsonContentType())
{
  throw new DivideByZeroException("It does not look like you're reading json. Here's your content-type {content-type}. ", ex);
}

    Keep in mind that these APIs are already available as a NuGet package that targets netcoreapp3.1 and netstandard2.0; if we decide to change behavior here would that be considered a breaking change?

    The code would have previously thrown when it encountered an unsupported media type. It wouldn't be a breaking change if it was more accomodating.

  12. rynowak commented on Jul 7, 2020

    @rynowak
    rynowak
    on Jul 7, 2020
    Contributor

    The original intent for the check (in this iteration of the library) is to make sure you get a reasonable error message when parsing fails. The really common case is when you're interacting with a website that returns an HTML page on a failure.

  13. terrajobst commented on Jul 7, 2020

    @terrajobst
    terrajobst
    on Jul 7, 2020
    Contributor

    @pranavkm @rynowak

    So what do you think the ideal change is? We could check the content type and throw a better exception for known content types that are common and unlikely to produce JSON, such as text/html. This would catch common issues resulting in parser errors while avoiding an overly aggressive content type enforcement.

  14. scalablecory commented on Jul 7, 2020

    @scalablecory
    scalablecory
    on Jul 7, 2020
    Contributor

    I think we should not check content type, but if parser throws an exception, throw with a message like "Content of type '{0}' was not valid json...".

  15. pranavkm commented on Jul 7, 2020

    @pranavkm
    pranavkm
    on Jul 7, 2020
    Contributor

    👍 with @scalablecory's suggestion. We want to give users good diagnostics when parsing fails rather than police the web.

  16. jozkee
    added this to the Future milestone on Jul 7, 2020
  17. jozkee
    removed
    untriagedNew issue has not been triaged by the area owner
    on Jul 7, 2020

  18. jozkee commented on Jul 7, 2020

    @jozkee
    jozkee
    on Jul 7, 2020
    Member

    I think we should not check content type, but if parser throws an exception, throw with a message like "Content of type '{0}' was not valid json...".

    In that case this is my understanding of the change:

    1. Remove MediaType-related validations.
    2. Wrap JsonSerializer.DeserializeAsync on a try-catch.
    3. Append the MediaType before rethrowing.
  19. jozkee
    added
    help wanted[up-for-grabs] Good issue for external contributors
    on Jul 7, 2020
  20. jozkee
    modified the milestones: Future, 5.0.0 on Jul 7, 2020
  21. jozkee
    closed this as completedin #40594on Aug 14, 2020
  22. jozkee
    added a commit that references this issue on Aug 14, 2020

    Move content type verification to a later stage (#40594)

    a62b38d
  23. ghost locked as resolved and limited conversation to collaborators on Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area-System.Net.Httphelp wanted[up-for-grabs] Good issue for external contributors

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions