Change `FormTagHelper` to apply to all form tags. #6156

NTaylorMullen · 2017-04-18T23:04:38Z

Added functional test to validate that non-attributed form tags have an antiforgery input generated. Re-generated baseline to reflect changes.
Added a unit test to validate that parameterless FormTagHelpers behave as expected.

ajaybhargavb · 2017-04-18T23:16:51Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs

-                TagBuilder tagBuilder;
-                if (Route == null)
+                TagBuilder tagBuilder = null;
+                if (Action == null && Controller == null && Route == null && _routeValues == null && Fragment == null && Area == null)


Will there ever be a case where _routeValues is non-null but empty? What gets generated in that case?

Yes, that could happen if a user did <form asp-all-route-data="new Dictionary<string, string>()">. In that case the user is being explicit and providing attributes; we'd want to follow the normal FormTagHelper rendering path.

dougbu

Will look at a few more things later...

dougbu · 2017-04-18T23:49:57Z

...e.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Form.html

@@ -1,4 +1,4 @@
-<html>
+<html>


No clue. :thuglife:

dougbu · 2017-04-18T23:51:50Z

test/WebSites/HtmlGenerationWebSite/Startup.cs

-            // null which is interpreted as true unless element includes an action attribute.
-            services.AddMvc().InitializeTagHelper<FormTagHelper>((helper, _) => helper.Antiforgery = false);
+            // Add MVC services to the services container.
+            services.AddMvc();


Undo this. The site is intentionally testing e2e behaviour with the Antiforgery property set to false.

dougbu · 2017-04-19T01:30:14Z

...e.Mvc.FunctionalTests/compiler/resources/HtmlGenerationWebSite.HtmlGeneration_Home.Form.html

-    <form></form>
-    <form action="/HtmlGeneration_Home/Form" method="post"></form>
+    <form><input name="__RequestVerificationToken" type="hidden" value="{0}" /></form>
+    <form action="/HtmlGeneration_Home/Form" method="post"><input name="__RequestVerificationToken" type="hidden" value="{0}" /></form>


Amazed we only have two <form> elements without attributes we recognized. That's sad.

dougbu · 2017-04-19T01:30:22Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/FormTagHelperTest.cs

+                metadataProvider: metadataProvider);
+            var expectedPostContent = HtmlContentUtilities.HtmlContentToString(
+                    htmlGenerator.GenerateAntiforgery(viewContext),
+                    HtmlEncoder.Default);


dougbu · 2017-04-19T01:30:26Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/FormTagHelperTest.cs

+            // Assert
+            Assert.Empty(output.Attributes);
+            Assert.Empty(output.PreContent.GetContent());
+            Assert.True(output.Content.GetContent().Length == 0);


Assert.Empty(output.Content.GetContent())

dougbu · 2017-04-19T01:30:32Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/FormTagHelperTest.cs

+            Assert.Empty(output.Attributes);
+            Assert.Empty(output.PreContent.GetContent());
+            Assert.True(output.Content.GetContent().Length == 0);
+            Assert.Equal(expectedPostContent, output.PostContent.GetContent());


Why no confirmation that PreElement and PostElement remain empty?

Just staying consistent with the other tests. I'll add though.

NTaylorMullen · 2017-04-19T23:42:11Z

🆙 📅

dougbu · 2017-04-20T02:42:54Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/AntiforgeryTestHelper.cs

                {
-                    if (string.Equals(attribute.Name.LocalName, "action", StringComparison.OrdinalIgnoreCase))


Agree confirming the action attribute exists was useless. Nice your new tests found that 👍

dougbu · 2017-04-20T02:43:20Z

test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs

+#endif
+        }
+
+


Blankety Blank Blank?

dougbu · 2017-04-20T02:44:59Z

...crosoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/RazorPagesWebSite.SimpleForms.html

@@ -0,0 +1,5 @@
+<form><input name="__RequestVerificationToken" type="hidden" value="{0}" /></form>


There's a bug here. The default method is get and so no antiforgery token should be added by default. Line 248 in the tag helper needs to handle null or empty too.

dougbu · 2017-04-20T02:45:37Z

...crosoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/RazorPagesWebSite.SimpleForms.html

+<form method="get"></form>
+<form method="post"><input name="__RequestVerificationToken" type="hidden" value="{0}" /></form>
+<form action="/Foo/Bar/Baz.html" method="get"></form>
+<form action="/Foo/Bar/Baz.html" method="post"></form>


Tag helper should have added an antiforgery token, changing the behaviour when user includes action attribute. Need to make a change around line 168 in the tag helper to match the updated line 248.

Talked with Damian about this. Since the user specified action, they're opting out of our TagHelper ways. Therefore, don't generate a token.

dougbu · 2017-04-20T02:48:29Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs

-    [HtmlTargetElement("form", Attributes = ControllerAttributeName)]
-    [HtmlTargetElement("form", Attributes = RouteAttributeName)]
-    [HtmlTargetElement("form", Attributes = RouteValuesDictionaryName)]
-    [HtmlTargetElement("form", Attributes = RouteValuesPrefix + "*")]


Let's be explicit rather than use the naming convention to target <form> elements: Stick with [HtmlTargetElement("form")].

NTaylorMullen · 2017-04-21T00:32:29Z

🆙 📅

dougbu · 2017-04-21T02:42:21Z

...crosoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/RazorPagesWebSite.SimpleForms.html

+<form method="get"></form>
+<form method="post"><input name="__RequestVerificationToken" type="hidden" value="{0}" /></form>
+<form action="/Foo/Bar/Baz.html" method="get"></form>
+<form action="/Foo/Bar/Baz.html" method="post"></form>


@dougbu

Tag helper should have added an antiforgery token, changing the behaviour when user includes action attribute. Need to make a change around line 168 in the tag helper to match the updated line 248.

@NTaylorMullen

Talked with Damian about this. Since the user specified action, they're opting out of our TagHelper ways. Therefore, don't generate a token.

Let's chat quickly tomorrow. I agree the user has opted out of the tag helper generating an action in this case. But, I'm not sure why <form action="" method="post"> is any more an opt out of antiforgery than <form method="post"> is -- both submit the same data to the same URL.

NTaylorMullen · 2017-04-21T23:55:52Z

🆙 📅

dougbu

A few small comments. But, let's wait for Monday to get this in.

dougbu · 2017-04-22T02:33:52Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs

+                {
+                    // User is using the FormTagHelper like a normal <form> tag that has an empty action attribute.
+                    // i.e. <form action="" method="post">
+                    antiforgeryDefault = true;


Don't set antiforgeryDefault to its default again.

Mention the action="@Url.Action(action: "Index", controller: "Home") possibility in the comments.

nit: "e.g." not "i.e." -- don't know anything more than action isn't "/A/Non-empty/String".

What's special about the @Url.Action case? It's also an HtmlString

Shoot. It actually returns a plain string. I thought (hoped) it avoided concatenation and returned a more-segmented IHtmlContent.

Just mention the non-string / non-HtmlString possibility. It's not necessarily an empty string or HtmlString.Empty.

dougbu · 2017-04-22T02:37:25Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs

+                {
+                    // User is using the FormTagHelper like a normal <form> tag. Antiforgery default should be false to
+                    // not force the antiforgery token on the user.
+                    antiforgeryDefault = false;


If we leave this alone (pending Monday's discussion), update the comments to better distinguish things from the if case e.g. User is likely using the <form> element to submit to another site. Do not send an antiforgery token to unknown sites.

Getting this in. If the decision is changed we can account for that afterwards.

dougbu · 2017-04-22T02:38:19Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs

@@ -4,6 +4,8 @@
 using System;
 using System.Collections.Generic;
 using System.ComponentModel;
+using System.IO;


What needs System.IO?

- Added functional test to validate that non-attributed form tags have an antiforgery input generated. Re-generated baseline to reflect changes. - Added a unit test to validate that parameterless `FormTagHelper`s behave as expected. #6006

NTaylorMullen assigned ajaybhargavb and dougbu Apr 18, 2017

NTaylorMullen requested review from ajaybhargavb and dougbu April 18, 2017 23:04

dnfclas added the cla-already-signed label Apr 18, 2017

ajaybhargavb approved these changes Apr 18, 2017

View reviewed changes

dougbu suggested changes Apr 18, 2017

View reviewed changes

dougbu reviewed Apr 19, 2017

View reviewed changes

NTaylorMullen force-pushed the nimullen/6006 branch from 6c3ef52 to b4b3151 Compare April 19, 2017 23:41

dougbu suggested changes Apr 20, 2017

View reviewed changes

NTaylorMullen force-pushed the nimullen/6006 branch from b4b3151 to c10ce6c Compare April 21, 2017 00:32

dougbu reviewed Apr 21, 2017

View reviewed changes

NTaylorMullen force-pushed the nimullen/6006 branch from c10ce6c to 8ea8521 Compare April 21, 2017 23:55

dougbu reviewed Apr 22, 2017

View reviewed changes

Change FormTagHelper to apply to all form tags.

3d895a9

- Added functional test to validate that non-attributed form tags have an antiforgery input generated. Re-generated baseline to reflect changes. - Added a unit test to validate that parameterless `FormTagHelper`s behave as expected. #6006

NTaylorMullen force-pushed the nimullen/6006 branch from 8ea8521 to 3d895a9 Compare April 25, 2017 01:34

NTaylorMullen merged commit 5c8a161 into dev Apr 25, 2017

NTaylorMullen deleted the nimullen/6006 branch April 25, 2017 01:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `FormTagHelper` to apply to all form tags. #6156

Change `FormTagHelper` to apply to all form tags. #6156

NTaylorMullen commented Apr 18, 2017

ajaybhargavb Apr 18, 2017

NTaylorMullen Apr 18, 2017

dougbu left a comment

dougbu Apr 18, 2017

NTaylorMullen Apr 19, 2017

dougbu Apr 18, 2017

dougbu Apr 19, 2017

dougbu Apr 19, 2017

dougbu Apr 19, 2017

dougbu Apr 19, 2017

NTaylorMullen Apr 19, 2017

NTaylorMullen commented Apr 19, 2017

dougbu Apr 20, 2017

dougbu Apr 20, 2017

dougbu Apr 20, 2017

dougbu Apr 20, 2017

NTaylorMullen Apr 21, 2017

dougbu Apr 20, 2017

dougbu Apr 21, 2017

NTaylorMullen commented Apr 21, 2017

dougbu Apr 21, 2017

NTaylorMullen commented Apr 21, 2017

dougbu left a comment

dougbu Apr 22, 2017

NTaylorMullen Apr 24, 2017

dougbu Apr 24, 2017

dougbu Apr 22, 2017

NTaylorMullen Apr 25, 2017

dougbu Apr 22, 2017

		{
		if (string.Equals(attribute.Name.LocalName, "action", StringComparison.OrdinalIgnoreCase))

		@@ -0,0 +1,5 @@
		<form><input name="__RequestVerificationToken" type="hidden" value="{0}" /></form>

		@@ -1,4 +1,4 @@
		<html>
		<html>

		#endif
		}

Change FormTagHelper to apply to all form tags. #6156

Change FormTagHelper to apply to all form tags. #6156

Conversation

NTaylorMullen commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen commented Apr 21, 2017

Choose a reason for hiding this comment

NTaylorMullen commented Apr 21, 2017

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Change `FormTagHelper` to apply to all form tags. #6156

Change `FormTagHelper` to apply to all form tags. #6156