Endpoint Routing Incorrectly Filters Candidates By Media Type

[commonsensesoftware]

Describe the bug

Endpoint Routing uses a series of policies to filter candidate actions. When duplicate routes are defined, an appropriate endpoint can still be selected by media type when it shouldn't be. All such cases should result in HTTP 500 with:

AmbiguousMatchException: The request matched multiple endpoints.

This was first discovered in dotnet/aspnet-api-versioning#744. Upon further investigation, it was found that the API Versioning MatcherPolicy only received a single candidate when it should have received two.

In most API applications, it's unlikely a duplicate route with different media types would be specified, which is probably why it hasn't been previously detected. This is of serious concern to API Versioning because it is very likely that multiple endpoints for different API versions will have the same route template and different media types.

cc: @jacalvar

To Reproduce

The following will reproduce the issue in a vanilla ASP.NET Core application without API Versioning.

namespace AspNet.Repro
{
    using Microsoft.AspNetCore.Mvc;
    using System;

    public class TestModel
    {
        public int Id { get; set; }
    }

    namespace V1
    {
        [ApiController]
        [Route( "api/[controller]" )]
        public class TestController : ControllerBase
        {
            [HttpPatch( "{id}" )]
            [Consumes( "application/merge-patch+json" )]
            public IActionResult Patch( int id, [FromBody] TestModel model )
            {
                model.Id = id;
                return Ok( model );
            }
        }
    }

    namespace V2
    {
        [ApiController]
        [Route( "api/[controller]" )]
        public class TestController : ControllerBase
        {
            [HttpPatch( "{id}" )]
            // [Consumes( "application/json" )]
            public IActionResult Patch( int id, [FromBody] TestModel model )
            {
                model.Id = id;
                return Ok( model );
            }
        }
    }
}

Behaviors

Sample request:

PATCH api/test/1 HTTP/1.1
Host: localhost:5000
Content-Length: 9

{"id":42}"

Content-Type varies according to the following table:

Consumes (on V2)	Content Type	Response	Controller
<unspecified>	<unspecified>	200	V2.TestController
<unspecified>	application/json	200	V2.TestController
<unspecified>	application/merge-patch+json	200	V1.TestController
application/json	<unspecified>	200	V2.TestController
application/json	application/json	200	V2.TestController
application/json	application/merge-patch+json	500	<exception>

Further technical details

• ASP.NET Core version
• Include the output of dotnet --info
• The IDE (VS / VS Code/ VS4Mac) you're running on, and its version

.NET SDK (reflecting any global.json):
 Version:   5.0.301
 Commit:    ef17233f86

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19042
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\5.0.301\

Host (useful for support):
  Version: 5.0.7
  Commit:  556582d964

.NET SDKs installed:
  5.0.301 [C:\Program Files\dotnet\sdk]

[javiercn]

@commonsensesoftware thanks for bringing this up Chris.

If I understand correctly, you are suggesting that the consumers matcher policy is incorrectly filtering out one of the endpoints which results in the route being matched instead of a 500 (ambiguous route) being produced?

I think I need to think a bit more about this, since I'm not super familiar with the behavior here. We definitely support the media type and the subtype, but I don't remember how we deal with suffixes. There are three possibilities here:

• We treat everything after the "/" as a single unit (meaning application/json is different from application/merge-patch+json).
• We treat application/merge-patch+json as compatible with application/json.
• We treat application/json as compatible with application/merge-patch+json.
• We treat application/merge-patch+json as more specific than application/json

I'll have to read the details on how this worked and remember what was going on here. This is potentially a breaking change, so we'll have to evaluate our options here.

It'll help if you can point out which endpoint matches when the response is 200, since that will help wrap my head around this.

[commonsensesoftware]

Thanks for the quick reply Javier. Per your request, I've updated the table with the selected controller.

For clarity, I think the selection criteria you've outlined is correct and matches the behavior. Matching unspecified media types seems not entirely obvious, but I can understand that decision. The primary issue is the only single candidate is being discovered. Both candidates should be discovered in all scenarios, regardless of the selected matches. Candidates should be marked as invalid based on media type. This should be the same behavior of HTTP methods or other criteria.

One of the limitations addressed in Endpoint Routing was to separate candidate discovery from matching/selection. This gives other policies an opportunity to make a decision.

[ghost]

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[javiercn]

@commonsensesoftware I agree in general.

I suspect that the ConsumesMatcherPolicy is involved here through the INodeBuilderPolicy, and I'm wondering if the behavior is the same or is different when you have a dynamic endpoint in the list of candidates (Having a dynamic endpoint prevents the node policy builder from taking effect).

If you want to dig into this, you can try it out by creating an additional endpoint that matches the route and that has an attribute that implements IDynamicEndpointMetadata and returns true. You can make that endpoint only consume xml or something like that, and that should be enough for it to get filtered out but prevent the INodeBuilderPolicy from applying.

If we see different results, then we can reason about it. In general, filtering via an INodeBuilderPolicy should offer the same end to end result as filtering through the IEndpointSelectorPolicy.

[commonsensesoftware]

Thanks @javiercn. I spent quite a bit of time spelunking back through the Endpoint Routing infrastructure. In short, this is not a bug and is the expected behavior from a vanilla routing perspective. This results weren't what I would have been expecting, but upon further analysis, it does seem to be correct.

The fundamental issue is that candidates are eliminated not merely invalidated by earlier MatcherPolicy instances such as HttpMethodMatcherPolicy and ConsumesMatcherPolicy. This works fine and well for out-of-the-box scenarios, but is definitely a problem for API Versioning and is unexpected in its current design. This appears to be my misunderstanding. The basic design has always been to merely disambiguate otherwise ambiguous routes by way of an API version. Things fall down in the case of 405 and 415 responses. I think I largely had 405 solved, which is why this hasn't come up before. In the case of 415, things simply don't work right.

After printing out an existing DFA, I created this simple variant, which I believe illustrates what needs to happen.

(Show source)

digraph DFA {
0 [label="/"]
0 -> 1
1 [label="/api/"]
1 -> 2
2 [label="/test/{...}/"]
2 -> 3
2 -> 4
3 [label="1.0"]
4 [label="2.0"]
3 -> 5 [label="HTTP: PATCH"]
5 [label="/api/test/ \nHTTP: PATCH"]
5 -> 6 [label="application/merge-patch+json"]
6 [label="/api/test/ \nHTTP: PATCH \napplication/merge-patch+json"]
5 -> 7 [label="*/*"]
7 [label="/api/test/ \nHTTP: PATCH \n*/*"]
5 -> 8
8 [label="/api/test/ \nHTTP: PATCH"]
4 -> 9 [label="HTTP: PATCH"]
9 [label="/api/test/ \nHTTP: PATCH"]
9 -> 10 [label="HTTP: PATCH"]
10 [label="/api/test/ \nHTTP: PATCH"]
9 -> 11 [label="application/json"]
11 [label="/api/test/ \nHTTP: PATCH \napplication/json"]
9 -> 12 [label="*/*"]
12 [label="/api/test/ \nHTTP: PATCH \n*/*"]
}

This is not a trivial change and would require implementing INodeBuilderPolicy. Back when I was working with the team to vet the design, we agreed that IEndpointSelectorPolicy would have been sufficient. In most cases it is, but not for 405 or 415 because they eliminate candidates before API Versioning sees them, which results in the incorrect response.

Assuming that I can make INodeBuilderPolicy work this way, I think it's the way to go. Eliminating candidates by API version ahead of HttpMethodMatcherPolicy and ConsumesMatcherPolicy should yield the correct behavior. I also think there is a chance that versioned endpoints will be resolved faster (we'll see). This will unfortunately mean that the order that the ApiVersionMatcherPolicy runs in will be important.

It's probably going to take a bit to flush out whether this is the right approach and will work. If it does, then I'll close this issue out. If it doesn't, then I'll elaborate further so we can look at queueing up whatever future feature may be necessary to enable the right behavior. If you have any other suggestions or ideas, I'm open to them. 😃

[ghost]

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.

[commonsensesoftware]

After a lot of investigation and diving into the weeds of the routing system, I can confirm that my last assessment was correct. Endpoint Routing is working as expected. This is not a bug. I did, however, discover that in order to make API Versioning work properly with these behaviors, it will need to implement INodeBuilderPolicy and return a specialized PolicyJumpTable which runs in front of the default policies. Understanding how node policies work isn't terribly difficult. Implementing said policy is not for the faint of heart. It's arguably TMI for this issue, but it will now effectively work as above. The next major release for API Versioning will have the necessary changes in place.

Thanks for the assist and taking the time to consider looking into this more deeply. I feel we can close this out since there's nothing more to do on your side and I've ironed out what must be done in API Versioning.

cc: @javiercn