Aspnetcore: Content-Type Enforcement Logic

[robkeimig]

It appears that when upgrading from ASP.Net Core 2.1.x libraries to 2.2.0, our existing MVC controllers exhibit issues with GET requests for JSON content returning 415 errors. Example controller definition:

[Route("Test")]
[Produces("application/json")]
[Consumes("application/json")]
public class TestController : Controller
{
    [HttpGet]
    [ProducesResponseType(typeof(IEnumerable<string>), 200)]
    public async Task<IActionResult> GetTestAsync()
    {
        var results = new List<string>() { "1", "2", "3" };
        return Json(results);
    }

    //Also some potential [HttpPost] items down here as well that accept JSON as [FromBody] args.
    //This is an important consideration as some of our controllers have 50+ methods covering GET/POST/PUT/etc.
}

Steps to reproduce the behavior:

1. Create .NET Framework 4.6.2 project.
2. Install ASP.NET Core 2.1.x.
3. Create a controller similar to the above example.
4. Attempt to request the resource GET /Test w/out explicitly specifying Content-Type application/json header.
5. JSON response body is returned as expected.
6. Update all ASP.NET Core libraries to the 2.2.0 version.
7. Retry 4. above and you should see a HTTP 415 response.

Additionally, we find that removal of the [Consumes("application/json")] attribute at the class-level will restore the former behavior under the 2.2.0 version, but we are skeptical that this would continue to function as expected for all of our other POST methods which _do_ consume JSON content. Alternatively, we could specify these [Consumes("application/json")] attributes at a per-method-call level if that is the actual intent here.

I am simply looking to understand if this is an intentional change in behavior or a bug. I feel as if GET requests should be exempt from this apparent new Content-Type constraint (especially if declared in the class scope), but there are likely other considerations at play.

[jc-m7t]

Also am simply looking to understand if this is an intentional change in behavior or a bug. I feel as if GET requests should be exempt from this apparent new Content-Type constraint (especially if declared in the class scope), but there are likely other considerations at play. Please advise.

[jbuckmccready]

I am in the same situation with the consumes attribute being applied at the class level now affecting GET methods that were not affected with ASP.Net Core 2.1.x. This is essentially a "breaking" change for a non-major version bump.

Having to add the attribute at a method level involves a lot of boiler plate and more work in maintaining code. Restricting the Content-Type header in a GET request seems like a niche use case when compared to applying the ConsumesAttribute at a class level.

[JamesNK]

Hi

We'll investigate this, and see whether this is an intended behavior change or not. In the meantime, if you want to upgrade to 2.2 now then you can disable endpoint routing with this code in your Startup.cs file:

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/routing?view=aspnetcore-2.2

EDIT: This still happens even with endpoint routing disabled. Remain on 2.1, or move the ConsumesAttribute to the action if you want to upgrade immediately.

[vanbukin]

+1 same issue

[JamesNK]

Tracked this down to a bug fix for 2.2 - https://github.com/aspnet/Mvc/issues/8174

GET requests don't include a content-type header and were bypassing the consumes constraint. After the 2.2 change they are rejected. Both the ConsumesActionConstraint in MVC and the ConsumesMatcherPolicy in routing are effected.

@rynowak and I discussed this and we think the original behavior should be restored. Placing a ConsumesAttribute on a controller that has GET actions looks like a common pattern that has been broken in 2.2.

// @Eilon @DamianEdwards

[Eilon]

Is there a workaround for 2.2 users?

[JamesNK]

The 2.2 workaround is to remove the ConsumesAttribute from the controller and place it on non-GET actions.

[Route("Test")]
[Produces("application/json")]
// [Consumes("application/json")]
public class TestController : Controller
{
    [HttpGet]
    [ProducesResponseType(typeof(IEnumerable<string>), 200)]
    public async Task<IActionResult> GetTestAsync()
    {
        var results = new List<string>() { "1", "2", "3" };
        return Json(results);
    }

    [HttpPost]
    [Consumes("application/json")]
    public async Task<IActionResult> Post([FromOb)
    {
        var results = new List<string>() { "1", "2", "3" };
        return Json(results);
    }
}

[Eilon]

OK certainly not ideal of course, but let's fix in 3.0 for now.

[vanbukin]

@Eilon That bug is really annoying. Specially when you read the announce ( https://blogs.msdn.microsoft.com/webdev/2018/12/04/asp-net-core-2-2-available-today ) about «Better integration with popular Open API (Swagger) libraries including design-time checks with code analyzers». But really what I get as end user is breaking change, that force me to remove attribute from controller-level and put attributes per action. Because otherwise GET actions did not working. Maybe it can be fixed earlier than 3.0? It looks like 2.2 is broke some code that works before and looks better without any workarounds.

[JamesNK]

@Eilon Fixed in 3.0 here https://github.com/aspnet/AspNetCore/pull/4459

I think it is a candidate for a 2.2 service release.

[Eilon]

Can we come up with an easy-to-use workaround for people affected by this? Like, "here, add these 5 lines of magic code to your startup to get the old behavior"? I'm not entirely opposed to an actual patch, but patches have their own risks.

[JamesNK]

The only workaround is going through your app, finding affected controllers, and moving the ConsumesAttribute like here: https://github.com/aspnet/AspNetCore/issues/4396#issuecomment-444359491

[JamesNK]

Another person with this issue: https://github.com/dotnet/core/issues/2098#issuecomment-444985993

[AndreaCuneo]

This issue is blocking any upgrate to aspnetcore 2.2 and forcing us to wait for 3.0, skipping 2.2 version and its improvements.

In our APIs we enforced correct Produces and Consumes attributes as is a strict REST API. We setup them on AddMvcOptions(). Since controllers usually includes both GETs and POSTs, this is breaking all controllers. We're forced to edit every controller and put the attribute on any PUT/POST/PATCH. Then when 3.0 is out with the fix, we've to revert them back to a more correct global configuration. This is budget waste hard to justify in any project and forcing us to skip 2.2 and delay HealthChecks and OpenApi 3.0 adoption.

Also consider that per HTTP spec, GET and HEAD cannot have content so makes no sense no sense at all to verify the Content-Type.

Please release a 2.2.1 fix.

[AndreaCuneo]

I've got a possible solution, which I'm not proud of.
Do you any pitfalls? I'm not expert on ApplicationModels and fiddling with them.
Do you have any better way of filtering Action with those Verbs?

    public class FixBrokenAspNetCoreConsume : IActionModelConvention
    {
        private static HashSet<string> _methods = new HashSet<string> { "POST", "PUT", "PATCH" };

        public void Apply(ActionModel action)
        {
            var model = action.Selectors.OfType<SelectorModel>().SingleOrDefault();
            var mm = model?.EndpointMetadata.OfType<HttpMethodMetadata>().SingleOrDefault();
            if (mm != null && _methods.Intersect(mm.HttpMethods).Any() && action.Parameters.Any(x => x.Attributes.OfType<FromBodyAttribute>().Any()))
            {
                action.Filters.Add(new ConsumesAttribute("application/json"));
            }

        }
    }

services.AddMvcCore()
    .AddMvcOptions(opt =>
    {
        opt.Filters.Add(new ProducesAttribute("application/json"));
        //opt.Filters.Add(new ConsumesAttribute("application/json")); // broken in 2.2
        opt.Conventions.Add(new FixBrokenAspNetCoreConsume());
    })

[Eilon]

@JamesNK yeah I was thinking using some app model convention to apply logic globally, like that ^^^

[JamesNK]

I looked into this and there are two situations.

1. ConsumesAttribute is registered as a global filter in Startup.cs. In this case then you can use the filter from @AndreaCuneo here - https://github.com/aspnet/AspNetCore/issues/4396#issuecomment-446651530. It will add the consumes constraint to non-GET actions.

2. ConsumesAttribute is registered as an attribute on controllers. In this case you would use the convention below. It removes the consumes constraint from the controller and places it on non-GET actions in the controller.

public class ConsumesControllerConvention : IControllerModelConvention
{
    public void Apply(ControllerModel controller)
    {
        var consumes = controller.Filters.OfType<ConsumesAttribute>().FirstOrDefault();
        if (consumes != null)
        {
            // Remove consumes from the controller
            controller.Filters.Remove(consumes);
            foreach (var selector in controller.Selectors)
            {
                selector.ActionConstraints.Remove(consumes);
            }

            // Search through controller actions, and place the attribute on non-GET actions
            foreach (var action in controller.Actions)
            {
                if (action.Filters.OfType<ConsumesAttribute>().Any())
                {
                    // Action already has a [Consumes]
                    break;
                }

                var httpMethods = action.Attributes.OfType<HttpMethodAttribute>().ToList();
                if (!httpMethods.Any(httpMethod => httpMethod.HttpMethods.Contains("GET", StringComparer.OrdinalIgnoreCase)))
                {
                    // Action does not have [HttpGet] on it so re-add constraint at the action level
                    action.Filters.Add(consumes);
                }
            }
        }
    }
}

[krispenner]

The work-around above has two issues still. If the action has both POST and GET, you will not have the Consumes attribute applied. Secondly, it does not consider other non-content methods such as DELETE which still fail. Below is a modified version of the convention above to handle any non-content based methods. However, due to the first issue regarding the work-around I raised and the fact this will affect a lot of users, I hope you will provide a patch to 2.2 with a proper fix to this.

This also does not work for POST methods that do not have a FromBody parameter as Swagger and other clients will not be inclined to send a Content-Type header when no content is required, even for POST. Does the fix in 3.0 address this?

public class ConsumesControllerConvention : IControllerModelConvention
{
    private static readonly HashSet<string> _httpMethodsWithoutContent = new HashSet<string>(StringComparer.OrdinalIgnoreCase)
    {
        HttpMethod.Delete.Method,
        HttpMethod.Get.Method,
        HttpMethod.Head.Method,
        HttpMethod.Options.Method,
        HttpMethod.Trace.Method,
    };

    public void Apply(ControllerModel controller)
    {
        var consumes = controller.Filters.OfType<ConsumesAttribute>().FirstOrDefault();
        if (consumes != null)
        {
            // Remove [Consumes] from the controller
            controller.Filters.Remove(consumes);
            foreach (var selector in controller.Selectors)
            {
                selector.ActionConstraints.Remove(consumes);
            }

            // Search through controller actions, and place [Consumes] on any action without content
            foreach (var action in controller.Actions)
            {
                if (action.Filters.OfType<ConsumesAttribute>().Any())
                {
                    // Action already has a [Consumes]
                    break;
                }

                var httpMethodAttributes = action.Attributes.OfType<HttpMethodAttribute>().ToList();
                if (!httpMethodAttributes.Any(httpMethodAttribute => httpMethodAttribute.HttpMethods.Any(method => _httpMethodsWithoutContent.Contains(method))))
                {
                    // Action does not support content, so re-add [Consumes] constraint at the action level
                    action.Filters.Add(consumes);
                }
            }
        }
    }
}

[krispenner]

I am now thinking that maybe the HTTP method check is irrelevant, we want this to work for POST actions which have no body parameter. So the only time we need Consumes on the action is when we have a FromBody or FromForm parameter, would that not be a correct assumption? Here is the code:

public class ConsumesControllerConvention : IControllerModelConvention
{
    public void Apply(ControllerModel controller)
    {
        var consumes = controller.Filters.OfType<ConsumesAttribute>().FirstOrDefault();
        if (consumes != null)
        {
            // Remove [Consumes] from the controller
            controller.Filters.Remove(consumes);
            foreach (var selector in controller.Selectors)
            {
                selector.ActionConstraints.Remove(consumes);
            }

            // Search through controller actions, and place [Consumes] on any action without content
            foreach (var action in controller.Actions)
            {
                if (action.Filters.OfType<ConsumesAttribute>().Any())
                {
                    // Action already has a [Consumes]
                    break;
                }

                if (action.Parameters.Any(parameter =>
                    parameter.Attributes.OfType<FromBodyAttribute>().Any() ||
                    parameter.Attributes.OfType<FromFormAttribute>().Any()))
                {
                    // Action has a content based parameter,
                    // so re-add [Consumes] constraint at the action level
                    action.Filters.Add(consumes);
                }
            }
        }
    }
}

I still think this is very ugly and needs a proper service patch in 2.2 due to issues I noted in previous comment.

[JamesNK]

Good news: a fix will be included 2.2.2 to revert to the old behavior. It should be publicly available in February.

Thanks for reporting this issue and providing a simple repo.

[JechoJekov]

Until the bug fix is released it is possible to just override the affected methods of ConsumesAttribute with the fixed code and use the new attribute as a quick-and-easy replacement of the built-in attribute (or even create an entirely new attribute from ConsumesAttribute's code):

https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/ConsumesAttribute.cs

```C#
public class BugFixConsumesAttribute : ConsumesAttribute, IResourceFilter, IConsumesActionConstraint
{
///

///
///

///
///
public BugFixConsumesAttribute(string contentType, params string[] otherContentTypes) : base(contentType, otherContentTypes)
{
}

    /// <inheritdoc />
    void IResourceFilter.OnResourceExecuting(ResourceExecutingContext context)
    {
        if (context == null)
        {
            throw new ArgumentNullException(nameof(context));
        }

        // Only execute if the current filter is the one which is closest to the action.
        // Ignore all other filters. This is to ensure we have a overriding behavior.
        if (IsApplicable(context.ActionDescriptor))
        {
            var requestContentType = context.HttpContext.Request.ContentType;

            // Confirm the request's content type is more specific than a media type this action supports e.g. OK
            // if client sent "text/plain" data and this action supports "text/*".
            //
            // Requests without a content type do not return a 415. It is a common pattern to place [Consumes] on
            // a controller and have GET actions
            if (requestContentType != null && !IsSubsetOfAnyContentType(requestContentType))
            {
                context.Result = new UnsupportedMediaTypeResult();
            }
        }
    }

    private bool IsSubsetOfAnyContentType(string requestMediaType)
    {
        var parsedRequestMediaType = new MediaType(requestMediaType);
        for (var i = 0; i < ContentTypes.Count; i++)
        {
            var contentTypeMediaType = new MediaType(ContentTypes[i]);
            if (parsedRequestMediaType.IsSubsetOf(contentTypeMediaType))
            {
                return true;
            }
        }
        return false;
    }

    /// <inheritdoc />
    bool IActionConstraint.Accept(ActionConstraintContext context)
    {
        // If this constraint is not closest to the action, it will be skipped.
        if (!IsApplicable(context.CurrentCandidate.Action))
        {
            // Since the constraint is to be skipped, returning true here
            // will let the current candidate ignore this constraint and will
            // be selected based on other constraints for this action.
            return true;
        }

        var requestContentType = context.RouteContext.HttpContext.Request.ContentType;

        // If the request content type is null we need to act like pass through.
        // In case there is a single candidate with a constraint it should be selected.
        // If there are multiple actions with consumes action constraints this should result in ambiguous exception
        // unless there is another action without a consumes constraint.
        if (requestContentType == null)
        {
            var isActionWithoutConsumeConstraintPresent = context.Candidates.Any(
                candidate => candidate.Constraints == null ||
                !candidate.Constraints.Any(constraint => constraint is IConsumesActionConstraint));

            return !isActionWithoutConsumeConstraintPresent;
        }

        // Confirm the request's content type is more specific than (a media type this action supports e.g. OK
        // if client sent "text/plain" data and this action supports "text/*".
        if (IsSubsetOfAnyContentType(requestContentType))
        {
            return true;
        }

        var firstCandidate = context.Candidates[0];
        if (firstCandidate.Action != context.CurrentCandidate.Action)
        {
            // If the current candidate is not same as the first candidate,
            // we need not probe other candidates to see if they apply.
            // Only the first candidate is allowed to probe other candidates and based on the result select itself.
            return false;
        }

        // Run the matching logic for all IConsumesActionConstraints we can find, and see what matches.
        // 1). If we have a unique best match, then only that constraint should return true.
        // 2). If we have multiple matches, then all constraints that match will return true
        // , resulting in ambiguity(maybe).
        // 3). If we have no matches, then we choose the first constraint to return true.It will later return a 415
        foreach (var candidate in context.Candidates)
        {
            if (candidate.Action == firstCandidate.Action)
            {
                continue;
            }

            var tempContext = new ActionConstraintContext()
            {
                Candidates = context.Candidates,
                RouteContext = context.RouteContext,
                CurrentCandidate = candidate
            };

            if (candidate.Constraints == null || candidate.Constraints.Count == 0 ||
                candidate.Constraints.Any(constraint => constraint is IConsumesActionConstraint &&
                                                        constraint.Accept(tempContext)))
            {
                // There is someone later in the chain which can handle the request.
                // end the process here.
                return false;
            }
        }

        // There is no one later in the chain that can handle this content type return a false positive so that
        // later we can detect and return a 415.
        return true;
    }

    private bool IsApplicable(ActionDescriptor actionDescriptor)
    {
        // If there are multiple IConsumeActionConstraints which are defined at the class and
        // at the action level, the one closest to the action overrides the others. To ensure this
        // we take advantage of the fact that ConsumesAttribute is both an IActionFilter and an
        // IConsumeActionConstraint. Since FilterDescriptor collection is ordered (the last filter is the one
        // closest to the action), we apply this constraint only if there is no IConsumeActionConstraint after this.
        return actionDescriptor.FilterDescriptors.Last(
            filter => filter.Filter is IConsumesActionConstraint).Filter == this;
    }
}

```

[JamesNK]

Yes that will work, but you will need to disable endpoint routing (and/or have compat mode be 2.1 or less)

[JechoJekov]

@JamesNK Thanks for the comment. Does this mean that endpoint routing ignores the logic of ConsumesAttribute and just uses the values from the ContentTypes property ? Is there any alternative to disabling endpoint routing (e.g. by configuring it in some way)?

[JamesNK]

Does this mean that endpoint routing ignores the logic of ConsumesAttribute and just uses the values from the ContentTypes property ?

Correct. Endpoint routing uses https://github.com/aspnet/AspNetCore/blob/371595962a3c10d870d503269ce1c38f7083e618/src/Mvc/src/Microsoft.AspNetCore.Mvc.Core/Routing/ConsumesMatcherPolicy.cs

Is there any alternative to disabling endpoint routing (e.g. by configuring it in some way)?

No, but only until 2.2.2 is publicly available.