Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

AntiForgeryValidation attribute seems to conflict with CookieAuthenticationEvents OnRedirectToLogin event handler #1009

Closed
imranbaloch opened this issue Oct 20, 2016 · 11 comments
Assignees
Milestone

Comments

@imranbaloch
Copy link

imranbaloch commented Oct 20, 2016

Reposting from https://forums.asp.net/t/2101587.aspx,

From Vincent H,

Hi,

My app is based on the SDK: 1.0.0-preview2-003121 and uses a combination of MVC, Web API and AngularJs (v1)

When making API calls, I want to return a 401 unauthorized when the cookie based session has expired. I have adjusted the "Startup.cs" to include an "OnRedirectToLogin" event handler so that API calls can be intercepted to return 401.

However when the API controller is decorated with the "[ValidateAntiForgeryToken]", the cookie authentication event is never fired and a 400 bad request is returned instead.

Can someone assist me?

Here is my "Startup.cs" > "ConfigureServices" code:

services.AddIdentity<ApplicationUser, ApplicationRole>(config =>
{
    config.User.RequireUniqueEmail = true;
    config.Password.RequiredLength = 8;
    config.Cookies.ApplicationCookie.CookieSecure = CookieSecurePolicy.SameAsRequest;
    config.Cookies.ApplicationCookie.ExpireTimeSpan = TimeSpan.FromMinutes(5);
    config.Cookies.ApplicationCookie.LoginPath = "/account/login";
    config.Cookies.ApplicationCookie.LogoutPath = "/account/logout";
    config.Cookies.ApplicationCookie.Events = new CookieAuthenticationEvents
    {
        OnRedirectToLogin = ctx =>
        {
            if (ctx.Request.Path.StartsWithSegments("/api") &&
                ctx.Response.StatusCode == (int)HttpStatusCode.OK)
            {
                ctx.Response.StatusCode = (int)HttpStatusCode.Unauthorized;
                return Task.FromResult(ctx.RedirectUri);
            }
            else
            {
                ctx.Response.Redirect(ctx.RedirectUri);
            }
            return Task.FromResult(0);
        }
    };
});

And this is the "Startup.cs" > "Configure" code for the AntiForgeryValidation support:

app.UseIdentity();
app.Use(next => context =>
{
    if (context.Request.Path.Value.ToLower().Equals("/") ||
        context.Request.Path.Value.ToLower().StartsWith("/home"))
    {
        var tokens = antiforgery.GetAndStoreTokens(context);
        context.Response.Cookies.Append("XSRF-TOKEN", tokens.RequestToken, new CookieOptions() { HttpOnly = false });
    }
    return next(context);
});
@imranbaloch
Copy link
Author

@imranbaloch
Copy link
Author

Reposting from https://forums.asp.net/t/2101587.aspx,

From Alex,

Hi Vincent,

Just curious if you ever found a solution for that problem.

I'm currently facing a very similar one, where antiforgery token validation fails before authorization fails, and "400 - Bad request" is being returned to the client.
In my case, this occurs when a user who had logged in and left the SPA page idling for a long time, comes back and attempts to refresh. At that point, the auth coookie has already expired and therefore the identity of the user associated with the previously issued XSRF token is no longer available on the request.
One would expect that, given the expired auth cookie, the cookie authorization filter would initiate a login attempt or at least return "401 - Unauthorized" immediately, as the controller being called has been decorated with [Authorize]. However, it seems like antiforgery validation takes place before the autorization filter is called.
In the debug log, I can see the following entries back-to-back:

Microsoft.AspNetCore.Hosting.Internal.WebHost: Information: Request starting HTTP/1.1 GET https://localhost:5443/api/processes/all 
Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationMiddleware: Information: Cookies was not authenticated. Failure message: Ticket expired
Microsoft.AspNetCore.Mvc.ViewFeatures.Internal.ValidateAntiforgeryTokenAuthorizationFilter: Information: Antiforgery token validation failed. The required antiforgery cookie ".AspNetCore.Antiforgery.WJaHc1Q-Os0" is not present.

Microsoft.AspNetCore.Antiforgery.AntiforgeryValidationException: The required antiforgery cookie ".AspNetCore.Antiforgery.WJaHc1Q-Os0" is not present.

So, cookie authentication fails, and that's fine as some of the controllers don't require an authenticated user. But then, the antiforgery filter kicks in and fails the request (the particular controller being called does require an authenticated user, and has both the authorize and antiforgery attributes set).
As Imran mentioned above, there's a particular order in which these filters get called, but I cannot figure out a way to change that order.
The ValidateAntiForgeryToken attribute does have an Order property alright, but what use does it make when the Authorize attribute itself does not have one?
I tried setting that Order to a ridiculously high value, - no effect.
I've also been looking for a way to re-order these filters programmatically using ConfigureServices() or Configure() in the startup class, but am unable to find any information on that.
What's really odd is that the exception handler middleware I added using IApplicationBuilder.UseExceptionHandler() works fine in all other cases but never gets hit by the AntiforgeryValidationException reported in the debug log. So, it looks like it gets caught and suppressed inside the antiforgery validation filter, which then "peacefully" returns a 400 response instead.
Totally baffled here, as this appears to be a pretty legit scenario which needs to be handled gracefully. IMO, it must be left to the developer to decide what to do when antiforgery validation fails, instead of the blanket 400 response. But in any case, if a controller has [Authorize] declared and the authentication cookie has expired, authorization must fail first (to give a chance to redirect to the login page), i.e. before even checking for antiforgery token.

@Tratcher
Copy link
Member

@kichalla @rynowak is this by design or a filter ordering bug?

@rynowak
Copy link
Member

rynowak commented Oct 20, 2016

This is the first I've heard of this. It sounds like there are two bugs here. We should force AF to run after [Authorize], but hearing that doesn't solve the problem we need to dig deeper.

In this case, does the authorize filter let the request 'pass through' in this scenario or does it short-circuit?

@alobakov
Copy link

alobakov commented Oct 20, 2016

I'm not very privy to the internals of these filters, but IMHO the Authorize filter should be allowed to inspect the request and potentially redirect to login if the auth cookie ticket has expired, and do so before antiforgery validation takes place. I'm guessing the only legitimate scenario where a request would need to fail with 400 is when the auth cookie was indeed valid and unexpired, but the antiforgery token was either not present or turned out wrong for the identity specified in the (valid) auth cookie.

@rynowak
Copy link
Member

rynowak commented Oct 20, 2016

@alobakov that's my understanding of the desired behavior

@Eilon Eilon added this to the 1.1.0 milestone Oct 20, 2016
@Tratcher
Copy link
Member

I'm starting to wonder how much of this has to do with using cookie auth for WebApi scenarios. This is not recommended, JwtBearer is more appropriate.

@alobakov
Copy link

Based on the logic that is apparently causing this error, I'd think that it could easily manifest itself in a regular web controller as well, and the user would see the "400 - Bad request" message in their browser after trying to refresh a protected page left idle for a long time. And I'm guessing any further attempts to refresh the window would keep failing until the auth cookie (expires:session) got purged. So, they'd effectively have to restart the browser in order to purge the cookie and finally get redirected to a login page.

@Eilon
Copy link
Member

Eilon commented Oct 27, 2016

@ryanbrandenburg can you try to get a repro for this and we can discuss how to proceed after that?

@ryanbrandenburg
Copy link
Contributor

ryanbrandenburg commented Oct 28, 2016

@imranbaloch I was able to get this working as expected by setting the order to a number greater than 0 on ValidateAntiForgeryToken. I have my example here, if you make a request against Home/Antiforgery you'll get redirected rather than getting a 400. Please review that sample and let us know either how your code differed to make ordering not work, or the way in which my sample doesn't reflect your (general) use case.

With that said, @rynowak and I talked this over and we think ValidateAntiForgeryToken should default to a high order so that it goes after the Authorization filter where we expect it by default.

@Eilon
Copy link
Member

Eilon commented Nov 1, 2016

FYI I moved this bug to MVC here: aspnet/Mvc#5483

And we will include it in the next 1.0.x patch release.

@Eilon Eilon closed this as completed Nov 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants