Skip to content
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

Accepts Extension method for null Content-Type in Minimal API #43794

Closed
1 task done
rachna-lad opened this issue Sep 7, 2022 · 3 comments
Closed
1 task done

Accepts Extension method for null Content-Type in Minimal API #43794

rachna-lad opened this issue Sep 7, 2022 · 3 comments
Assignees
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels bug This issue describes a behavior which is not expected - a bug. ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. Status: Resolved
Milestone

Comments

@rachna-lad
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

I have used Accepts extension method to allow only specific Content-Type. Refer below code snippet for reference.
webApplication.MapPost("v1/todo", async (long id, [FromBody] TODO request) => { // Perform operation }) .Accepts<TODO>("application/json") .Produces(StatusCodes.Status415UnsupportedMediaType);
But this Accepts doesn't handle null Content-Type in header.

Expected Behavior

Should throw 415 if define Content-type is not passed in header.

Steps To Reproduce

  1. Use extension method Accepts which supports only specific Content-Type otherwise throw 415. Here i have taken application/json.
  2. Now make API request without request body(body is null) and Content-Type is also null
  3. Verify response

Exceptions (if any)

No response

.NET Version

6.0.101

Anything else?

No response

@javiercn javiercn added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Sep 7, 2022
@rafikiassumani-msft rafikiassumani-msft added the bug This issue describes a behavior which is not expected - a bug. label Sep 8, 2022
@captainsafia
Copy link
Member

captainsafia commented Sep 13, 2022

This appears to be a bug in the way the AcceptsMatcherPolicy is applied on endpoints. In particular, it doesn't look like it is registered as a policy on the endpoint when the DfaMatcher is constructed. AFAIK, the logic below looks incomplete because it doesn't actually go in and evaluate if the endpoint has the correct metadata if it is not dynamic. It's assumed that if there are no dynamic endpoints then the AcceptsMatcher policy is not registered at all.

bool IEndpointSelectorPolicy.AppliesToEndpoints(IReadOnlyList<Endpoint> endpoints)
{
if (endpoints == null)
{
throw new ArgumentNullException(nameof(endpoints));
}
// When the node contains dynamic endpoints we can't make any assumptions.
return ContainsDynamicEndpoints(endpoints);
}

To validate this assumption, I updated the implementation of AppliesToEndpoints to as follows:

  bool IEndpointSelectorPolicy.AppliesToEndpoints(IReadOnlyList<Endpoint> endpoints)
  {
      if (endpoints == null)
      {
          throw new ArgumentNullException(nameof(endpoints));
      }

      // When the node contains dynamic endpoints we can't make any assumptions.
      if (ContainsDynamicEndpoints(endpoints))
      {
          return false;
      }

      return AppliesToEndpointsCore(endpoints);
  }

This seems to populate some nodes in the matcher with the correct policies, however the matcher doesn't pick up those candidate nodes during routing here.

var (candidates, policies) = FindCandidateSet(httpContext, path, segments);

A sidenote to all this analysis is that we do have content-type checks for minimal APIs generated in the RequestDelegateFactory. However, the rules are not nearly as robust as what you would get in the AcceptsMatcherPolicy and the rules behave rather esoterically depending on whether or not you have (a) provided a payload in your request and (b) provided a malformed payload.

More notes incoming...

@captainsafia
Copy link
Member

OK, we spent some time discussing this in the team chat and @brunolins16 was able to provide some illuminating insights in this regard.

TL;DR: this is expected behavior given the way the AcceptsMatcherPolicy is expected to behave. For real-world scenarios, the user will still get a 415 in dev.

Request content types in minimal API applications are validated at two layers:

  • During routing by the AcceptsMatcherPolicy
  • At runtime when validated by the JSON deserialization logic codegened in the RequestDelegateFactory

What logic comes into play depends on some rules that are enforced in the AcceptsMatcherPolicy. In particular, it creates the following edges in the routing jump table:

  1. Edges for each of the content types defined in Accepts metadata (just application/json for most minimal apps)
  2. Edges for */*: the wildcard content-type
  3. Edges for string.Empty

#1 and #3 will evaluate to the underlying endpoint. The AcceptsMatcherPolicy makes an explicit call-out that it will no-op in the event that the request contains an empty content type. That means that content-type validation falls back to the logic in the RDF. #2 will invoke the validation logic in the AcceptsMatcherPolicy which will verify whether arbitrary content types are allowed.

@captainsafia captainsafia added ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. and removed investigate labels Sep 14, 2022
@ghost ghost added the Status: Resolved label Sep 14, 2022
@ghost
Copy link

ghost commented Sep 15, 2022

This issue has been resolved and has not had any activity for 1 day. It will be closed for housekeeping purposes.

See our Issue Management Policies for more information.

@ghost ghost closed this as completed Sep 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 15, 2022
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels bug This issue describes a behavior which is not expected - a bug. ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. Status: Resolved
Projects
None yet
Development

No branches or pull requests

4 participants