DefaultProblemDetailsWriter Is Not RFC 7231 Compliant

[commonsensesoftware]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

builder.Services.AddProblemDetails() adds the internal type DefaultProblemDetailsWriter. As seen here:

aspnetcore/src/Http/Http.Extensions/src/DefaultProblemDetailsWriter.cs

Line 28 in 5918bc9

if (acceptHeader is { Count: > 0 })

The CanWrite implement returns false when the Accept header is missing or empty. This is violates conformance with RFC 7231 §5.3.2, which states:

A request without any Accept header field implies that the user agent will accept any media type in response.

This results in a behavioral breaking change for a server expected to return ProblemDetails to a client.

Expected Behavior

DefaultProblemDetailsWriter.CanWrite should return true and ultimately write ProblemDetails when Accept is missing or empty.

Steps To Reproduce

var builder = WebApplication.CreateBuilder( args );

builder.Services
       .AddProblemDetails()
       .AddApiVersioning( options => options.ApiVersionReader = new MediaTypeApiVersionReader() );

var forecast = app.MapGroup( "/weatherforecast" ).WithApiVersionSet();

forecast.MapPost( "/", ( WeatherForecast forecast ) => Results.Ok() ).HasApiVersion( 1.0 );

app.Run();

POST /weatherforecast HTTP/2
Host: localhost:5000
Content-Type: application/json; v=2.0
Content-Length: 61

{"date": "2022-11-12", "temperatureC": 42, "summary": "Cold"}

The response is 415 (because version v=2.0 is not supported), but no ProblemDetails are included.

Exceptions (if any)

No response

.NET Version

7.0.100

Anything else?

DefaultProblemDetailsWriter is internal and sealed so it cannot be easily extended or changed. It also uses other internal types or properties, which makes it difficult or impossible to fork.

A possible workaround is to decorate the implementation, but this is a very yucky DI setup:

private static void TryAddProblemDetailsRfc7231Compliance( IServiceCollection services )
{
    var descriptor = services.FirstOrDefault( IsDefaultProblemDetailsWriter );

    if ( descriptor == null )
    {
        return;
    }

    var decoratedType = descriptor.ImplementationType!;
    var lifetime = descriptor.Lifetime;

    services.Add( Describe( decoratedType, decoratedType, lifetime ) );
    services.Replace( Describe( typeof( IProblemDetailsWriter ), sp => NewProblemDetailsWriter( sp, decoratedType ), lifetime ) );

    static bool IsDefaultProblemDetailsWriter( ServiceDescriptor serviceDescriptor ) =>
        serviceDescriptor.ServiceType == typeof( IProblemDetailsWriter ) &&
        serviceDescriptor.ImplementationType?.FullName == "Microsoft.AspNetCore.Http.DefaultProblemDetailsWriter";

    static Rfc7231ProblemDetailsWriter NewProblemDetailsWriter( IServiceProvider serviceProvider, Type decoratedType ) =>
        new( (IProblemDetailsWriter) serviceProvider.GetRequiredService( decoratedType ) );
}

Additional Consideration

RFC 7231 §5.3.2 semantics for the Accept header also states:

...disregard the header field by treating the response as if it is not subject to content negotiation.

I propose that ProblemDetailsOptions add a new property to allow that configuration. The default value would be true and have parity with the current behavior. Developers can set it to false to retain the ProblemDetailsFactory behavior versus the new IProblemDetailsService behavior.

public class ProblemDetailsOptions
{
    /// <summary>
    /// The operation that customizes the current <see cref="Mvc.ProblemDetails"/> instance.
    /// </summary>
    public Action<ProblemDetailsContext>? CustomizeProblemDetails { get; set; }

+   /// <summary>
+   /// Gets or sets a value indicating whether to use HTTP content negotiation.
+   /// </summary>
+   /// <value>True if the content negotiation is used; otherwise, false. The default value is <c>true</c>.</value>
+   public bool UseContentNegotiation { get; set; } = true;
}

I realize this is a feature request. I'm happy to create a separate issue for it, but since it's so closely related to this bug, and arguably part of it (because it can't be achieved), I'm starting with the proposal here.

[commonsensesoftware]

cc @brunolins16

[brunolins16]

DefaultProblemDetailsWriter is internal and sealed so it cannot be easily extended or changed. It also uses other internal types or properties, which makes it difficult or impossible to fork.

@commonsensesoftware thanks for the feedback. We have decided to understand how users would use the new feature before making the DefaultProblemDetailsWriter public, however, I agree that having this type public and not sealed will help custom implementation. Do you have any scenario already?

[brunolins16]

I realize this is a feature request. I'm happy to create a separate issue for it, but since it's so closely related to this bug, and arguably part of it (because it can't be achieved), I'm starting with the proposal here.

@commonsensesoftware if you don't mind, let's create a new API Proposal issue to have the discussion about the feature.

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. 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.

[commonsensesoftware]

@brunolins16, my scenario is very simple... API Versioning supports versioning by media type. If the version specified via the media type isn't supported, then the appropriate 406 or 415 response is returned. Since the cause is also related to versioning, ProblemDetails are created that indicate the requested API version is not supported.

Using the new IProblemDetailsService, no body (e.g. ProblemDetails) is returned if the client did not provide the Accept header. This is a derivation of the HTTP spec (e.g. RFC 7231), which clearly states when no Accept is specified, then it's equivalent to */*. This isn't a custom behavior. This is how it is supposed to work. The most common place for this to happen is a POST where only Content-Type is specified. I discovered this because adopting the IProblemDetailsService broke my acceptance tests. This is a landmine for any API author that returns ProblemDetails and expects Accept to be handled the way the RFC 7231 says.

I'm fine with the fact that nothing is returned if the client specified Accept: application/json instead of Accept: application/json, application/problem+json. I'd even argue that it's a welcomed addition because this is requiring the client to understand an error response.

It's fairly common for a server to ignore the media type requested by a client in the case of an error, but I think this should be in the hands of the API author. The current default is very sensible, but a server may want to opt out. Per your request, I've added an API proposal in #45159