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

Antiforgery goes at the end of filters#5470

Merged
ryanbrandenburg merged 1 commit intodevfrom
rybrande/AntiforgeryEndFilter
Nov 2, 2016
Merged

Antiforgery goes at the end of filters#5470
ryanbrandenburg merged 1 commit intodevfrom
rybrande/AntiforgeryEndFilter

Conversation

@ryanbrandenburg
Copy link
Copy Markdown
Contributor

Fixes aspnet/Security#1009. Since Antiforgery should be at the end of filters by default set its order to be a large number by default.

Copy link
Copy Markdown
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs functional tests, also need to verify this works with [AutoValidate....] as well

@ryanbrandenburg
Copy link
Copy Markdown
Contributor Author

🆙📅

/// validation of antiforgery tokens by default for an application. Use
/// <see cref="IgnoreAntiforgeryTokenAttribute"/> to suppress validation of the antiforgery token for
/// a controller or action.
/// The default Order for this attribute is 1000 because it must run after any filter which does authentication
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest putting this document on the order property. Also when you're working on doc comments, newlines in the source code are not significant. Use the <para></para> tag if you want to make paragraphs

@@ -0,0 +1,42 @@
using System;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Header

"warningsAsErrors": true
},
"dependencies": {
"AjaxAntiForgeryValidation": "1.0.0",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest calling this SecurityWebSite

{
Console.WriteLine();
});
services.AddIdentity<ApplicationUser, ApplicationRole>(config =>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need Identity here? This is pretty heavyweight. Identity is primarily concerned which information about the user, not just whether or not the user is authentic.

Suggest talking to @Tratcher about how we we could just get away with cookie middleware (which Identity uses).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you can simplify down to cookie/forms auth

services.AddMvc();
services.AddAntiforgery(options =>
{
Console.WriteLine();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derp


public IConfigurationRoot Configuration { get; }

// This method gets called by the runtime. Use this method to add services to the container.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need all this silliness

context.Request.Path.Value.ToLower().StartsWith("/home"))
{
var tokens = antiforgery.GetAndStoreTokens(context);
context.Response.Cookies.Append("XSRF-TOKEN", tokens.RequestToken, new CookieOptions() { HttpOnly = false });
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here?

@ryanbrandenburg
Copy link
Copy Markdown
Contributor Author

🆙📅

@@ -22,7 +22,11 @@ namespace Microsoft.AspNetCore.Mvc
public class AutoValidateAntiforgeryTokenAttribute : Attribute, IFilterFactory, IOrderedFilter
{
/// <inheritdoc />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inheritdoc doesn't really work with these kinds of thing. you'll want to also provide a summary here

@ryanbrandenburg ryanbrandenburg force-pushed the rybrande/AntiforgeryEndFilter branch from 9d1f647 to a0847a9 Compare November 2, 2016 21:01
@ryanbrandenburg
Copy link
Copy Markdown
Contributor Author

🆙📅

@ryanbrandenburg ryanbrandenburg force-pushed the rybrande/AntiforgeryEndFilter branch from a0847a9 to 01b237d Compare November 2, 2016 23:51
@ryanbrandenburg ryanbrandenburg merged commit 01b237d into dev Nov 2, 2016
@ryanbrandenburg ryanbrandenburg deleted the rybrande/AntiforgeryEndFilter branch November 2, 2016 23:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants