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

ASP.NET Core MVC 2.0 non asp-* attribute form elements now auto-insert anti-forgery tokens. #6204

Closed
NTaylorMullen opened this issue Apr 25, 2017 · 19 comments

Comments

@NTaylorMullen
Copy link
Member

NTaylorMullen commented Apr 25, 2017

The change

In ASP.NET Core MVC 2.0 the FormTagHelper from the Microsoft.AspNetCore.Mvc.TagHelpers assembly now injects anti-forgery tokens for plain form elements (elements that don't have asp-*). This happens under the following conditions:

  1. There is a method="post" attribute
  2. The action attribute is empty. e.g. action=""
  3. The action attribute is not supplied

Workarounds

Explicitly turn off anti-forgery

<form method="post" asp-antiforgery="false">
</form>

Opt your form element out of TagHelperification by using the ! symbol.

<!form method="post">
</!form>

Remove the FormTagHelper entirely from the view.
@removeTagHelper Microsoft.AspNetCore.Mvc.TagHelpers.FormTagHelper, Microsoft.AspNetCore.Mvc.TagHelpers


See announcement: aspnet/Announcements#233

@peterblazejewicz
Copy link

Is there a better approach to this? Technically (meaning: tools) speaking this is no longer form element I think:

<!form method="post">
</!form>

This one causes invalid markup as the '!form' is not a valid tag or custom element tag name:
https://www.w3.org/TR/html51/syntax.html#tag-name
https://www.w3.org/TR/custom-elements/#valid-custom-element-name

@Antaris
Copy link

Antaris commented Apr 25, 2017

@peterblazejewicz It's the first I have seen of it, but I imagine that the ! character would be stripped out of the final HTML output, it's there to support Razor Tag Helpers exclusions.

@peterblazejewicz
Copy link

Thanks! I got the idea. My concerns are with dev burden. This will outline as another invalid markup when editing in e.g. VSCode:
image
not mentioning problems wiht 3rd party syntax validation

@poke
Copy link
Contributor

poke commented Apr 25, 2017

Can you explain why this change was made? Why is there an antiforgery token added when the target clearly isn’t handled by MVC (otherwise a asp-route attribute or something should be present)?

It’s really not uncommon for HTML forms to exist without having an actual target and action, as a way to group form elements that are handled differently. For what it’s worth, form submission is not mandatory for a form element, so it does not make sense to default to that behavior with this tag helper. Especially not when neither action nor method are set. Also, it’s not unheard of to have an action to a different application making the token pretty redundant.

I also don’t like this ! syntax. Tag helpers are designed to be valid HTML markup themselves, so having this invalid syntax to turn them off is counter-productive.

@Bartmax
Copy link

Bartmax commented Apr 25, 2017

I agree 100% with @poke concerns. Also want to know the reasons behind this.

@MaximRouiller
Copy link

Hey @NTaylorMullen,

There was a prime directive at some point that tag helpers should only apply when you want them to be applied so as to not introduce magic. This sounds like magic to me. Opt-out is generally bad because people are not aware of it or will have them digging for documentation.

What prompted this addition? I get it if it's security. I, and the rest of the community, just want to know the history of this change so we can rationalize it better.

Thanks,

@DamianEdwards
Copy link
Member

This is a reaction to patterns that fall out of using Razor Pages. It is idiomatic in those cases to use a <form method="post"> and rely on the behavior of posting back to the current URL. With the current FormTagHelper behavior this results in no anti-XSRF token being generated as the Tag Helper doesn't apply. The form post will then fail as Razor Pages default to expecting an anti-XSRF token for posts. The assumption that was made was that the vast majority of the time a form element appears without an explicit action specified, it is intended to be submitted back to the current URL, which will be a Controller or Page given a Tag Helper ran in the first place. i.e. if action is manually specified in the source, then the anti-XSRF token shouldn't be emitted (unless explicitly enabled via asp-antiforgery="true".

This is something we can certainly tweak in subsequent pre-releases before we finalize 2.0, so I encourage people to try it out in their apps with their use-cases and report back thoughts and findings to help us land on the best (and hopefully least compromised) design.

@MaximRouiller
Copy link

Thanks for the quick reply @DamianEdwards

@Bartmax
Copy link

Bartmax commented Apr 25, 2017

Good to know @DamianEdwards
One question, the <!tag> syntax is supposed to mean "do not use any tag helper on this tag" and will work for any other taghelper as well or is form specific?

@DamianEdwards
Copy link
Member

@Bartmax it works for any tag, it basically forcefully opts an element out of tag helper processing.

@poke
Copy link
Contributor

poke commented Apr 26, 2017

@DamianEdwards In my opinion, the razor pages feature shouldn’t be a reason to change how tag helpers are designed (to only run when explicitly requested by the author). I would like you to consider adding something for the razor pages use case that makes it clear to those using the razor pages that this also involves a tag helper. If posting to self is such a common use case, maybe we can just add an attribute for that then to make this behavior still explicit (e.g. <form asp-post-self>). That way we could also move this over to MVC based views, where this is just as common.

The assumption that was made was that the vast majority of the time a form element appears without an explicit action specified, it is intended to be submitted back to the current URL, which will be a Controller or Page given a Tag Helper ran in the first place. i.e. if action is manually specified in the source, then the anti-XSRF token shouldn't be emitted (unless explicitly enabled via asp-antiforgery="true").

In my opinion, the logic should be something like this:

  1. Add token if asp-antiforgery="true"
  2. Otherwise if method is set to anything
    1. action is set to a static value (not calculated by other tag helper attributes): No token
    2. action is some route inside the application: Add token
    3. action is missing: Add token
  3. Otherwise if no method is set and no action is set: No token

This would enable the token for the following examples:

<form asp-antiforgery="true">
<form asp-controller="Account" asp-action="Login" method="post">
<form method="post">

But it would not be enabled for the following examples:

<form asp-antiforgery="false" (ignoring all other attributes)>
<form action="/some/other/route" method="post">
<form>

Since the tag helper now runs for every form tag (unless we change that again), without that being super clear (since we don’t use a tag helper tag or attribute), we should also make sure that there are no side effects happening because of that to further press on what @MaximRouiller said about tag helpers not applying magic on their own behalf.

@dpaquette
Copy link
Contributor

@poke I don't think this is significantly changing the design of Tag Helpers. Tag helpers already run on tags without explicitly asking for it. That's how the things like <img url="~/images/logo.png"> get resolved to a relative path.

See https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Razor/TagHelpers/UrlResolutionTagHelper.cs

@Bartmax
Copy link

Bartmax commented Apr 26, 2017

Target html tags without explicit telling to be processed by a tag helpers existed since v1. Anyone can author a tag helper to target html tags without any explicit parameters. This is not a change on how Tag Helpers are designed.

@poke I think what's confusing is that there are 2 things going on.

  1. ! Opt-out tag helpers. (specially useful for tag helpers that targets html tags)
  2. Making the form tag helper be processed on the method attribute (which is a html attribute).

For 1, as of right now, opting out of taghelpers without an explicit option provided by the author (like asp-antiforgery) wasn't straight forward because you can only exclude the tag helper from the whole page. Adding ! feature to prevent a single tag to be processed by any tag helpers is a good and very useful thing.

For 2, I think it's a valid reason to make the form tag helper targets the method attribute. Not sure I agree with the decision but you want this for most cases. (mvc or not).

I don't see many scenarios when you will need to actually use ! (or any other alternative) to the form tag helper, considering the default method is GET.

If there's any scenario I am missing when you don't want an antiforgerytoken on a form post please share.

@ericwj
Copy link

ericwj commented May 1, 2017

Why not fix this particular thing with some sort of analyzer-like behavior? Is it really important that the asp-antiforgery attribute isn't in the code, but the anti forgery token to be generated anyhow (i.e. 'magic')?

The mentioned conditions imho should simply issue a warning that can be removed by adding either asp-antiforgery="true" or asp-antiforgery="false", either by typing every letter or by hitting a light bulb or Ctrl+. and Enter.

The downside is when you're not using VS or VS Code, you'll get - what is it? 403? - which you know how to fix - people that use notepad do, right!?

@SolderedMushroom
Copy link

Wouldn't this be about a two-minute fix for anyone running into the problem you described as a result of the "idiom?" They write <form method="post">, their form post fails in the debug build and gives them an error message telling them to include the XSRF token, and they go and include it.

@DamianEdwards
Copy link
Member

@SolderedMushroom it doesn't give them a specific error actually, it simply replies with a 400 Bad Request, which makes it kinda hard to diagnose.

@SolderedMushroom
Copy link

Doesn't the developer exception page report the specifics of any 400 or 500 responses?

@DamianEdwards
Copy link
Member

Only 500

@Eilon
Copy link
Member

Eilon commented Jun 9, 2017

We are closing this issue because no further action is planned for this issue. If you still have any issues or questions, please log a new issue with any additional details that you have.

@Eilon Eilon closed this as completed Jun 9, 2017
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