Support SkipStatusCodePages on endpoints and authorized routes

[captainsafia]

Background and Motivation

#10317 identifies an issue where the behavior of the AuthorizationMiddleware circumvents the filter logic in SkipStatusCodeAttribute and results in the StatusCodePageMiddleware being executed when it shouldn't and masking the underlying 401 from the AuthorizationMiddleware.

This PR attempts to resolve this issue by removing the dependency on the IFilter execution order from the SkipStatusCodeAttribute by introducing an ISkipStatusCodesMetadata interface.

Proposed API

namespace Microsoft.AspNetCore.Http.Metadata;

public interface ISkipStatusCodePagesMetadata
{
}

- public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter
+ public class SkipStatusCodePagesAttribute : Attribute, IResourceFilter, ISkipStatusCodePagesMetadata
{
}

Usage Examples

app.UseEndpoints(endpoints =>
{
    endpoints.MapGet("/", [SkipStatusCodePages] (c) =>
    {
        c.Response.StatusCode = 404;
        return Task.CompletedTask;
    });
});

Closes #10317

[Kahbazi]

Having context.GetEndpoint(); before _next(context) implies that UseStatusCodePages should be after UseRouting which might not be the case everywhere, including the sample in the docs.

This could be after _next(context) and also only be called if statusCodeFeature.Enabled is true.

[Tratcher]

That's a fair consideration, and we would want to make sure the doc was accurate.

Do we have examples of anything else that's endpoint aware but placed before UseRouting?

This ordering was recommended so that the attribute behavior could be applied early and then overridden by other middleware/code while processing the request. That's consistent with how the filter works today. If placed after _next, you wouldn't be able to tell if IStatusCodePagesFeature had the default value or if it was set by something in the app.

[Kahbazi]

@Tratcher Sorry, I'm confused a little bit! Are you saying that going forward StatusCodePagesMiddleware should always be after RoutingMiddleware?

[Tratcher]

StatusCodePagesMiddleware can stay where it is and continue to work for existing apps, but if you want it to be route aware and use the SkipStatusCodePages attribute then it will need to be placed after routing.

[Kahbazi]

Oh I see! The implementation has been changed after I add the original comment. It's all clear now.

I assume if StatusCodePagesMiddleware with re-execute would be used after routing it will automatically add the routing, right?

aspnetcore/src/Middleware/Diagnostics/src/StatusCodePage/StatusCodePagesExtensions.cs

Lines 179 to 189 in 4656f15

	if (app.Properties.TryGetValue(globalRouteBuilderKey, out var routeBuilder) && routeBuilder is not null)
	{
	return app.Use(next =>
	{
	RequestDelegate? newNext = null;
	// start a new middleware pipeline
	var builder = app.New();
	// use the old routing pipeline if it exists so we preserve all the routes and matching logic
	// ((IApplicationBuilder)WebApplication).New() does not copy globalRouteBuilderKey automatically like it does for all other properties.
	builder.Properties[globalRouteBuilderKey] = routeBuilder;
	builder.UseRouting();

What if StatusCodePagesFeature set the Enable property lazily? In this case it doesn't matter where StatusCodePagesMiddleware is as long as the feature is accessed after routing, right?

public class StatusCodePagesFeature : IStatusCodePagesFeature
{
    private readonly HttpContext httpContext;
    public StatusCodePagesFeature(HttpContext httpContext)
    {
        _httpContext = httpContext;
    }

    private bool? _enabled;
    public bool Enabled
    {
        get 
        {
            if (_enabled == null)
            {
                var endpoint = _httpContext.GetEndpoint();
                var skipStatusCodePageMetadata = endpoint?.Metadata.GetMetadata<ISkipStatusCodePagesMetadata>();
                if (skipStatusCodePageMetadata is not null)
                {
                    _enabled = false;
                }
                else
                {
                    _enabled = true;
                }
            }
            
            return _enabled;
        }
        set
        {
            _enabled = value;
        }
    }
}

[halter73]

StatusCodePagesFeature is a public class, so if we were to change it, we'd have to make it continue to work as it does today when called with the empty constructor. I think it might be worthwhile to just use a new internal IStatusCodePagesFeature that lazily reads the endpoint.

The only thing I don't like is locking in the wrong value for Enabled if the property is read before UseRouting(). You could definitely argue it's no worse than today where it's locked in immediately when the feature is added, but that feels a little less surprising. I wonder if it'd be better to avoid negative caching unless the property is explicitly set, but maybe that's even more surprising.

[ghost]

Thank you for your API proposal. I'm removing the api-ready-for-review label. API Proposals should be submitted for review through Issues based on this template.

[pranavkm]

Should we drop the in Endpoint.Metadata? I don't know if it helps clarify things?

[halter73]

I like the in Endpoint.Metadata. What's wrong with it? It could point the curious to API docs for endpoint routing.

[pranavkm]

Should we add a test that verifies we let people change the feature in the body of the endpoint?

endpoints.MapGet("/", [SkipStatusCodePages] (c) =>
{
         c.GetFeature<IStatusCodePagesFeature>().Enabled = false;
});

[halter73]

Except verify that you can set it to true! Although it really would be weird to reenable it after adding the attribute.

[captainsafia]

Seems fair! I missed seeing this before the auto-merge kicked it but I'll address in a follow-up.

[Tratcher]

I assume if StatusCodePagesMiddleware with re-execute would be used after routing it will automatically add the routing, right?

@captainsafia, @Kahbazi's comment makes me realize we designed this wrong. UseStatusCodePagesWithReExecute must go before UseRouting to work, but it must go after routing for [SkipStatusCodePages] to work. Both are not possible. We will have to re-think the ordering requirements. Either re-open #10317 or file a new issue please.

[captainsafia]

@Kahbazi's comment makes me realize we designed this wrong. UseStatusCodePagesWithReExecute must go before UseRouting to work, but it must go after routing for [SkipStatusCodePages] to work. Both are not possible. We will have to re-think the ordering requirements. Either re-open #10317 or file a new issue please.

Hm. Will dig into this to understand the flow here and file a new issue.