aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.62k stars 2.14k forks source link

Global or controller level AutoValidateAntiforgeryTokenAttribute prevents app.UseStatusCodePagesWithReExecute() from firing #8670

Closed ghost closed 5 years ago

ghost commented 5 years ago

Is this a Bug or Feature request?:

Bug

Steps to reproduce (preferably a link to a GitHub repo with a repro project):

  1. Enable status code page with re-execute in Startup.cs e.g. app.UseStatusCodePagesWithReExecute("/Home/Error/{0}");

  2. Configure an action and view to display the custom error page.

  3. Add an AutoValidateAntiforgeryTokenAttribute either as controller attribute [AutoValidateAntiforgeryToken] or registered as a global filter:

    services.AddMvc(options =>
    {
        options.Filters.Add(new AutoValidateAntiforgeryTokenAttribute());
    }).SetCompatibilityVersion(CompatibilityVersion.Version_2_1);
  4. When running a view containing a <form method="post">, use browser webdev tools to delete the anti-forgery token input and submit the form.

Description of the problem:

Expected behavior: the configured status code page is displayed with a 400 Bad request response status code. Actual behavior: 400 Bad Request is returned with an empty response. A break point in the action configured in app.UseStatusCodePagesWithReExecute("/Home/Error/{0}"); is never hit.

The same outcome is observed if ValidateAntiforgeryTokenAttribute is used in place of AutoValidateAntiforgeryTokenAttribute as well.

Note: if AutoValidateAntiforgeryTokenAttribute or ValidateAntiforgeryTokenAttribute are added at the action level instead of at the controller level or as a global filter, then the expected behavior occurs: the configured status code page is displayed (still with a 400 Bad Request response status code).

This leads to inconsistent results depending on where the attribute is used:

(Auto)ValidateAntiforgeryTokenAttribute Location Result
Global filter 400 Bad Request with empty response body
Controller level 400 Bad Request with empty response body
Action level 400 Bad Request with executed StatusCodePage response body

I believe the action level result is the desired outcome.

Version of Microsoft.AspNetCore.Mvc or Microsoft.AspNetCore.App or Microsoft.AspNetCore.All:

Microsoft.AspNetCore.App 2.1.5. Also observed in Microsoft.AspNetCore.App 2.1.1.

mkArtakMSFT commented 5 years ago

Thanks for contacting us, @webwizgordon. @javiercn, can you please look into this? Thanks!

ghost commented 5 years ago

I have some more information regarding this issue. The issue is not specific to the AutoValidateAntiforgeryTokenAttribute or ValidateAntiforgeryTokenAttribute.

It appears to apply to any AuthorizationFilter. What I believe is happening, is that the filter runs a second time for the re-executed request and so the re-executed context also gets a status code change and returns a blank page so that it doesn't end up in an endless loop of re-executing contexts. This would explain why it only happens when the filter is registered globally and, in my case, at the controller level (because my Error action was on the same controller as the initial action).

I am not sure if it's the filter's responsibility to handle this. Maybe the status code middleware can/should just ignore any status code changes and just run to completion. There are probably other cases where a re-executed context could change a status code and create a blank page as well.

For my own filters, I can add logic to detect if I am in a status code page re-execution scenario, but I cannot force the built in or third party filters to handle those cases.

This filter will show a blank 400 response for any POST requests.

public class BadRequestPostFilterAttribute : Attribute, IAsyncAuthorizationFilter
{
    public async Task OnAuthorizationAsync(AuthorizationFilterContext context)
    {
        if (context.HttpContext.Request.Method == "POST")
        {
            context.Result = new BadRequestResult();
        }
    }
}

This filter will show the status code page 400 response for any POST requests (the error scenario detection is rudimentary for demo purposes, I would probably look for IStatusCodeReExecuteFeature instead):

public class BadRequestPostFilterAttribute : Attribute, IAsyncAuthorizationFilter
{
    public async Task OnAuthorizationAsync(AuthorizationFilterContext context)
    {
        if (context.HttpContext.Request.Method == "POST" && !context.HttpContext.Request.Path.StartsWithSegments(new PathString("/Home/Error")))
        {
            context.Result = new BadRequestResult();
        }
    }
}
javiercn commented 5 years ago

@webwizgordon Can you put [IgnoreAntiforgeryToken] in your /Home/Error action? I'm guessing that the issue is that there is an antiforgery error and that when the request is re-executed, the antiforgery error is produced again due to nothing changing on the request and that's most likely why the page is not rendering.

None of the individual parts you are using is wrong, but the combination of both is. Your /Home/Error/ action should explicitly opt-out of antiforgery validation and similar functionality in order to provide a straightforward path to render the error.

While plugging filters globally is great, you need to be aware that there are some actions where you might need to opt-out.

ghost commented 5 years ago

That is such an obvious solution to this issue that I am embarrassed to say that I did not think to try it. The error page is indeed displayed as expected when [IgnoreAntiforgeryToken] is used on the Error action.

There may be scenarios where other IAuthorizationFilter implementations do not provide a similar mechanism to opt-out. But I suppose in such a case, it would be up to the implementation to address the issue and provide an opt-out option and not the framework's responsibility.

javiercn commented 5 years ago

@webwizgordon Yep. The framework can't do anything on the general case, it needs to be done by each filter. I'm glad that we were able to solve the issue.

javiercn commented 5 years ago

Closing as there's no more action to be taken here, the behavior is by design.

ghost commented 5 years ago

Would it make sense for the default scaffolded Error action to have the [IgnoreAntiforgeryToken] attribute out of the box?