Skip to content

UseStatusCodePagesWithReExecute should respect [SkipStatusCodePages] #39472

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

Closed
halter73 opened this issue Jan 13, 2022 · 4 comments · Fixed by #46109
Closed

UseStatusCodePagesWithReExecute should respect [SkipStatusCodePages] #39472

halter73 opened this issue Jan 13, 2022 · 4 comments · Fixed by #46109
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. help wanted Up for grabs. We would accept a PR to help resolve this issue
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Jan 13, 2022

Describe the bug

app.UseStatusCodePagesWithReExecute(string pathFormat) must come before any explicit call to app.UseRouting() in order to redo route matching with pathFormat. Reversing this order causes requests that should result in a status code page to result in a 404 instead because the route matching is never rerun.

The problem is that correctly putting UseStatusCodePagesWithReExecute before UseRouting causes [SkipStatusCodePages] to be ignored. StatusCodePagesMiddlewarechecks for ISkipStatusCodePagesMetadata before UseRouting sets the metadata. (One possible fix might be to check for the metadata again after calling _next if GetEndpoint() returned null before calling _next.)

public void Configure(IApplicationBuilder app)
{
    app.UseStatusCodePagesWithReExecute("/status");

    app.UseRouting();

    app.UseEndpoints(endpoints =>
    {
        endpoints.MapGet("/", () =>
        {
            return Results.BadRequest(); // "Status: 400" correctly written to body
        });

        endpoints.MapGet("/skip", [SkipStatusCodePages] () =>
        {
            return Results.BadRequest(); // "Status: 400" *incorrectly* written to body
        });

        endpoints.MapGet("/status", (HttpResponse response) =>
        {
            return $"Status: {response.StatusCode}"; // "Status: 200" correctly written to body when hit directly
        });
    });
}

Reversing the order leads to 404s. This is unfortunately by design, but it would be great if we could make this work similar to the way we do for the implicit UseRouting in WebApplication.

public void Configure(IApplicationBuilder app)
{
    app.UseRouting();

    app.UseStatusCodePagesWithReExecute("/status");

    app.UseEndpoints(endpoints =>
    {
        endpoints.MapGet("/", () =>
        {
            return Results.BadRequest(); // Empty 404 response
        });

        endpoints.MapGet("/skip", [SkipStatusCodePages] () =>
        {
            return Results.BadRequest(); // Empty 404 response
        });

        endpoints.MapGet("/status", (HttpResponse response) =>
        {
            return $"Status: {response.StatusCode}"; // "Status: 200" correctly written to body when hit directly
        });
    });
}

If WebApplication is used instead with its implicit call to UseRouting, everything works correctly. Rerouting does not 404 and [SkipStatusCodePages] is respected. This is thanks to @BrennanConroy's work in #35426. Maybe we could do something similar for non-WebApplication scenarios.

using Microsoft.AspNetCore.Mvc;

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.UseStatusCodePagesWithReExecute("/status");

// Adding app.UseRouting() here would reintroduce the broken behavior of ignoring [SkipStatusCodePages]

app.MapGet("/", () =>
{
    return Results.BadRequest(); // "Status: 400" correctly written to body
});

app.MapGet("/skip", [SkipStatusCodePages] () =>
{
    return Results.BadRequest(); // Correct empty 400 response because /status is skipped
});

app.MapGet("/status", (HttpResponse response) =>
{
    return $"Status: {response.StatusCode}"; // "Status: 200" correctly written to body when hit directly
});

app.Run();

More context from previous work in this area: #38509 (comment)

Expected Behavior

[SkipStatusCodePages] should be respected and rerouting should not 404.

public void Configure(IApplicationBuilder app)
{
    app.UseStatusCodePagesWithReExecute("/status");

    app.UseRouting();

    app.UseEndpoints(endpoints =>
    {
        endpoints.MapGet("/", () =>
        {
            return Results.BadRequest(); // "Status: 400" correctly written to body
        });

        endpoints.MapGet("/skip", [SkipStatusCodePages] () =>
        {
            return Results.BadRequest(); // Correct empty 400 response because /status is skipped
        });

        endpoints.MapGet("/status", (HttpResponse response) =>
        {
            return $"Status: {response.StatusCode}"; // "Status: 200" correctly written to body when hit directly
        });
    });
}

.NET Version

7.0.100-alpha.1.22060.1

@halter73 halter73 added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jan 13, 2022
@rafikiassumani-msft rafikiassumani-msft added the bug This issue describes a behavior which is not expected - a bug. label Jan 13, 2022
@Tratcher
Copy link
Member

(One possible fix might be to check for the metadata again after calling _next if GetEndpoint() returned null before calling _next.)

This seems like a good, simple fix.

@rafikiassumani-msft rafikiassumani-msft added area-runtime and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jan 13, 2022
@adityamandaleeka adityamandaleeka added this to the .NET 7 Planning milestone Jan 14, 2022
@ghost
Copy link

ghost commented Jan 14, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 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.

@adityamandaleeka
Copy link
Member

Sounds like the fix should be simple here, so let's try to get it in for 7.

@ghost
Copy link

ghost commented Sep 9, 2022

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.

@Tratcher Tratcher added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Sep 9, 2022
@Tratcher Tratcher self-assigned this Jan 18, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 18, 2023
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions bug This issue describes a behavior which is not expected - a bug. help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
None yet
6 participants
@halter73 @adityamandaleeka @Tratcher @amcasey @rafikiassumani-msft and others