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

IgnoreAntiforgeryTokenAttribute possibly broken on 1.1.0 #5552

Closed
luisgoncalves opened this issue Nov 20, 2016 · 13 comments
Closed

IgnoreAntiforgeryTokenAttribute possibly broken on 1.1.0 #5552

luisgoncalves opened this issue Nov 20, 2016 · 13 comments

Comments

@luisgoncalves
Copy link

I have the following test application with a global filter for anti-forgery token validation and then a validation bypass on a specific action method:

Startup

    public class Startup
    {
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddMvc(options =>
            {
                options.Filters.Add(new AutoValidateAntiforgeryTokenAttribute());
            });
        }

        public void Configure(IApplicationBuilder app)
        {
            app.UseMvcWithDefaultRoute();
        }
    }

Controller

    public class TestController : Controller
    {
        [HttpGet]
        public IActionResult Index()
        {
            return View();
        }

        [HttpPost, IgnoreAntiforgeryToken]
        public IActionResult Index(string test)
        {
            return Content("OK");
        }
    }

Index view

<form asp-antiforgery="false">
    <button>Submit</button>
</form>

Using the 1.0.1 MVC package I can access /Test, submit and get the successful response. However, if I use the 1.1.0 package I get a 400 Bad Request when submitting the form

Am i missing something or did this behavior actually change?

@rynowak
Copy link
Member

rynowak commented Nov 21, 2016

There's no intended change here. We need to take a look at this /cc @Eilon

@Eilon Eilon added this to the 1.2.0 milestone Nov 22, 2016
@Eilon
Copy link
Member

Eilon commented Nov 22, 2016

@ryanbrandenburg can you investigate, and also see if perhaps it's patch-worthy? (Is there a workaround?)

@kevinchalet
Copy link
Contributor

kevinchalet commented Nov 23, 2016

FWIW, this bug also impacts Orchard 2's OpenID module, which worked fine with the 1.0.0 bits.

/cc @jersiovic @sebastienros

@rynowak
Copy link
Member

rynowak commented Nov 23, 2016

The bug here is that this PR 01b237d should have also update the order value of [IgnoreAntiforgery]

If you need a workaround for this, I'd recommend setting [IgnoreAntiforgeryToken(Order = 1000)] - OR using application model to set the order (sample to follow).

@rynowak
Copy link
Member

rynowak commented Nov 23, 2016

Here's a convention that will fix it.

            services.AddMvc(options =>
            {
                options.Filters.Add(new AutoValidateAntiforgeryTokenAttribute());

                options.Conventions.Add(new FixIgnoreAntiforgeryConvention());
            });
        private class FixIgnoreAntiforgeryConvention : IApplicationModelConvention
        {
            public void Apply(ApplicationModel application)
            {
                foreach (var filter in application.Filters)
                {
                    ProcessFilter(filter);
                }

                foreach (var controller in application.Controllers)
                {
                    foreach (var filter in controller.Filters)
                    {
                        ProcessFilter(filter);
                    }

                    foreach (var action in controller.Actions)
                    {
                        foreach (var filter in action.Filters)
                        {
                            ProcessFilter(filter);
                        }
                    }
                }
            }

            private void ProcessFilter(IFilterMetadata filter)
            {
                var ignoreFilter = filter as IgnoreAntiforgeryTokenAttribute;
                if (ignoreFilter != null)
                {
                    ignoreFilter.Order = 1000;
                }
            }
        }

@Eilon
Copy link
Member

Eilon commented Dec 13, 2016

Done?

@Eilon
Copy link
Member

Eilon commented Jan 14, 2017

Re-opening so we can take this for the 1.1.1 branch.

@ryanbrandenburg
Copy link
Contributor

@Eilon I assume this is an oversight but this is patch-approved not just patch-proposed right? I can go ahead with merging this commit into rel/1.1.2?

@Eilon
Copy link
Member

Eilon commented Feb 4, 2017

Not yet approved, but soon!

@Eilon
Copy link
Member

Eilon commented Feb 7, 2017

This patch bug is approved. Please use the normal code review process w/ a PR and make sure the fix is in the correct branch, then close the bug and mark it as done.

@Eilon
Copy link
Member

Eilon commented Feb 7, 2017

Also please remember that this is 1.1.2, so use the right branch! 😄

@viewtance
Copy link

A little confuses, did this bug fixed? In asp.net core 1.1.1 or asp.net core mvc 1.1.2?

@Eilon
Copy link
Member

Eilon commented Mar 15, 2017

@viewtance it's in ASP.NET Core MVC 1.1.2, which was released about a week ago: https://www.nuget.org/packages/Microsoft.AspNetCore.Mvc/1.1.2

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

7 participants