Skip to content

DefaultProblemDetailsWriter Doesn't Handle Media Type Subsets #52577

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

Open
1 task done
commonsensesoftware opened this issue Dec 4, 2023 · 4 comments
Open
1 task done
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone

Comments

@commonsensesoftware
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

The DefaultProblemDetailsWriter is meant to support clients that specify a media type in the Accept header field that is compatible application/json or application/problem+json:

private static readonly MediaTypeHeaderValue _jsonMediaType = new("application/json");

The matching logic, however, is inverted:

if (_jsonMediaType.IsSubsetOf(acceptHeaderValue) ||

The correct logic should be:

if (acceptHeaderValue.IsSubsetOf(_jsonMediaType) ||
    acceptHeaderValue.IsSubsetOf(_problemDetailsJsonMediaType))
{
    return true;
}

Expected Behavior

A client that specifies a more specific media type should be matched by DefaultProblemDetailsWriter.CanWrite. This happens because MediaTypeHeaderValue.IsSubsetOf considers media type parameters:

public bool IsSubsetOf(MediaTypeHeaderValue otherMediaType)

The source code cites RFC 2068 §14.1 which has been superseded by RFC 2616, RFC 7230, and is currently defined by RFC 9110 §12.5.1. A media type is identified by <type>/<subtype>[+<suffix>]*. The documentation for MediaTypeHeaderValue.IsSubsetOf also states:

For example "multipart/mixed; boundary=1234" is a subset of "multipart/mixed; boundary=1234", "multipart/mixed", "multipart/", and "/*" but not "multipart/mixed; boundary=2345" or "multipart/message; boundary=1234".

This means that:

  • application/json; charset=utf-8
  • application/json; v=1.0

should be considered subsets of application/json. Currently, they are not because the logic is inverted. These scenarios were missed because the test cases do not include a variant which includes a media type parameter:

public void CanWrite_ReturnsTrue_WhenJsonAccepted(string contentType)

Steps To Reproduce

The following scenarios assume that a problem has occurred on the server.

Example 1

Although not officially supported as part of application/json, it is commonplace for message exchanges to use the media type parameter charset=utf-8 as opposed to Accept-Charset, which is arguably the correct approach. If a problem occurs, no problem details are reported as a result.

POST /example HTTP/2
Host: localhost
Accept: application/json; charset=utf-8
Content-Type: application/json; charset=uft-8
Content-Length: 42

{"test": true, "message": "Why, hello there!"}
HTTP/2 400 Bad Request

Example 2

A truly RESTful API would version itself using media type negotiation, which might be achieved using a media type parameter. If a problem occurs or the version is not supported, no problem details are reported.

POST /example HTTP/2
Host: localhost
Accept: application/json; v=42.0
Content-Type: application/json; v=42.0
Content-Length: 42

{"test": true, "message": "Why, hello there!"}
HTTP/2 415 Unsupported Media Type

Exceptions (if any)

InvalidationOperationException is IProblemDetailsService.WriteAsync is used versus IProblemDetailsService.TryWriteAsync.

.NET Version

8.0.100

Anything else?

@ghost ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Dec 4, 2023
@captainsafia
Copy link
Member

captainsafia commented Dec 15, 2023

@commonsensesoftware Thanks for opening this issue! This does indeed seem like a bug. Happy to review a PR.

EDIT: Putting this in the backlog for now but that doesn't mean we won't take a PR for it.

@captainsafia captainsafia added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Dec 19, 2023
@ghost
Copy link

ghost commented Dec 19, 2023

Looks like this issue has been identified as a candidate for community contribution. If you're considering sending a PR for this issue, look for the Summary Comment link in the issue description. That comment has been left by an engineer on our team to help you get started with handling this issue. You can learn more about our Help Wanted process here

@captainsafia captainsafia added this to the Backlog milestone Dec 19, 2023
@ghost
Copy link

ghost commented Dec 19, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@MattyLeslie
Copy link
Contributor

This should be closed as per merged PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
Development

No branches or pull requests

5 participants
@captainsafia @commonsensesoftware @wtgodbe @MattyLeslie and others