From 5c8a161ace331d090326a1013520716db6b9a1cf Mon Sep 17 00:00:00 2001 From: "N. Taylor Mullen" Date: Mon, 17 Apr 2017 17:25:44 -0700 Subject: [PATCH] Change `FormTagHelper` to apply to all form tags. - 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 --- .../FormTagHelper.cs | 66 +++++++--- .../AntiforgeryTestHelper.cs | 20 ++- .../RazorPagesTest.cs | 33 +++++ .../RazorPagesWebSite.SimpleForms.html | 6 + .../FormTagHelperTest.cs | 116 ++++++++++++++++++ .../RazorPagesWebSite/SimpleForms.cshtml | 9 ++ 6 files changed, 218 insertions(+), 32 deletions(-) create mode 100644 test/Microsoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/RazorPagesWebSite.SimpleForms.html create mode 100644 test/WebSites/RazorPagesWebSite/SimpleForms.cshtml diff --git a/src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs b/src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs index 02ae810309..b37369bbd4 100644 --- a/src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; using System.ComponentModel; +using Microsoft.AspNetCore.Html; using Microsoft.AspNetCore.Mvc.Rendering; using Microsoft.AspNetCore.Mvc.ViewFeatures; using Microsoft.AspNetCore.Razor.TagHelpers; @@ -14,15 +15,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers /// /// implementation targeting <form> elements. /// - [HtmlTargetElement("form", Attributes = ActionAttributeName)] - [HtmlTargetElement("form", Attributes = AntiforgeryAttributeName)] - [HtmlTargetElement("form", Attributes = AreaAttributeName)] - [HtmlTargetElement("form", Attributes = PageAttributeName)] - [HtmlTargetElement("form", Attributes = FragmentAttributeName)] - [HtmlTargetElement("form", Attributes = ControllerAttributeName)] - [HtmlTargetElement("form", Attributes = RouteAttributeName)] - [HtmlTargetElement("form", Attributes = RouteValuesDictionaryName)] - [HtmlTargetElement("form", Attributes = RouteValuesPrefix + "*")] + [HtmlTargetElement("form")] public class FormTagHelper : TagHelper { private const string ActionAttributeName = "asp-action"; @@ -152,15 +145,20 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers { throw new ArgumentNullException(nameof(output)); } + if (Method != null) { output.CopyHtmlAttribute(nameof(Method), context); } + else + { + Method = "get"; + } var antiforgeryDefault = true; // If "action" is already set, it means the user is attempting to use a normal
. - if (output.Attributes.ContainsName(HtmlActionAttributeName)) + if (output.Attributes.TryGetAttribute(HtmlActionAttributeName, out var actionAttribute)) { if (Action != null || Controller != null || @@ -184,9 +182,29 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers PageAttributeName)); } - // User is using the FormTagHelper like a normal tag. Antiforgery default should be false to - // not force the antiforgery token on the user. - antiforgeryDefault = false; + string attributeValue = null; + switch (actionAttribute.Value) + { + case HtmlString htmlString: + attributeValue = htmlString.ToString(); + break; + case string stringValue: + attributeValue = stringValue; + break; + } + + if (string.IsNullOrEmpty(attributeValue)) + { + // User is using the FormTagHelper like a normal tag that has an empty or complex IHtmlContent action attribute. + // e.g. or + + // Antiforgery default is already set to true + } + else + { + // User is likely using the element to submit to another site. Do not send an antiforgery token to unknown sites. + antiforgeryDefault = false; + } } else { @@ -223,8 +241,12 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers routeValues["area"] = Area; } - TagBuilder tagBuilder; - if (pageLink) + TagBuilder tagBuilder = null; + if (Action == null && Controller == null && Route == null && _routeValues == null && Fragment == null && Area == null && Page == null) + { + // Empty form tag such as
. Let it flow to the output as-is and only handle anti-forgery. + } + else if (pageLink) { tagBuilder = Generator.GeneratePageForm( ViewContext, @@ -256,13 +278,19 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers htmlAttributes: null); } - output.MergeAttributes(tagBuilder); - if (tagBuilder.HasInnerHtml) + if (tagBuilder != null) { - output.PostContent.AppendHtml(tagBuilder.InnerHtml); + output.MergeAttributes(tagBuilder); + if (tagBuilder.HasInnerHtml) + { + output.PostContent.AppendHtml(tagBuilder.InnerHtml); + } } + } - antiforgeryDefault = !string.Equals(Method, "get", StringComparison.OrdinalIgnoreCase); + if (string.Equals(Method, "get", StringComparison.OrdinalIgnoreCase)) + { + antiforgeryDefault = false; } if (Antiforgery ?? antiforgeryDefault) diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/AntiforgeryTestHelper.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/AntiforgeryTestHelper.cs index 899ec8bf31..d7310e628b 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/AntiforgeryTestHelper.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/AntiforgeryTestHelper.cs @@ -19,21 +19,15 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests foreach (var form in htmlDocument.Descendants("form")) { - foreach (var attribute in form.Attributes()) + foreach (var input in form.Descendants("input")) { - if (string.Equals(attribute.Name.LocalName, "action", StringComparison.OrdinalIgnoreCase)) + if (input.Attribute("name") != null && + input.Attribute("type") != null && + input.Attribute("type").Value == "hidden" && + (input.Attribute("name").Value == "__RequestVerificationToken" || + input.Attribute("name").Value == "HtmlEncode[[__RequestVerificationToken]]")) { - foreach (var input in form.Descendants("input")) - { - if (input.Attribute("name") != null && - input.Attribute("type") != null && - input.Attribute("type").Value == "hidden" && - (input.Attribute("name").Value == "__RequestVerificationToken" || - input.Attribute("name").Value == "HtmlEncode[[__RequestVerificationToken]]")) - { - return input.Attributes("value").First().Value; - } - } + return input.Attributes("value").First().Value; } } } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs index 589101c252..5805b21e1b 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/RazorPagesTest.cs @@ -6,6 +6,8 @@ using System.Collections.Generic; using System.Linq; using System.Net; using System.Net.Http; +using System.Net.Http.Headers; +using System.Reflection; using System.Threading.Tasks; using Xunit; @@ -13,6 +15,8 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests { public class RazorPagesTest : IClassFixture> { + private static readonly Assembly _resourcesAssembly = typeof(RazorPagesTest).GetTypeInfo().Assembly; + public RazorPagesTest(MvcTestFixture fixture) { Client = fixture.Client; @@ -20,6 +24,35 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests public HttpClient Client { get; } + [Fact] + public async Task Page_SimpleForms_RenderAntiforgery() + { + // Arrange + var expectedMediaType = MediaTypeHeaderValue.Parse("text/html; charset=utf-8"); + var outputFile = "compiler/resources/RazorPagesWebSite.SimpleForms.html"; + var expectedContent = await ResourceFile.ReadResourceAsync(_resourcesAssembly, outputFile, sourceFile: false); + + // Act + var response = await Client.GetAsync("http://localhost/SimpleForms"); + var responseContent = await response.Content.ReadAsStringAsync(); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(expectedMediaType, response.Content.Headers.ContentType); + + responseContent = responseContent.Trim(); + + var forgeryToken = AntiforgeryTestHelper.RetrieveAntiforgeryToken(responseContent, "SimpleForms"); +#if GENERATE_BASELINES + // Reverse usual substitution and insert a format item into the new file content. + responseContent = responseContent.Replace(forgeryToken, "{0}"); + ResourceFile.UpdateFile(_resourcesAssembly, outputFile, expectedContent, responseContent); +#else + expectedContent = string.Format(expectedContent, forgeryToken); + Assert.Equal(expectedContent.Trim(), responseContent, ignoreLineEndingDifferences: true); +#endif + } + [Fact] public async Task Page_Handler_HandlerFromQueryString() { diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/RazorPagesWebSite.SimpleForms.html b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/RazorPagesWebSite.SimpleForms.html new file mode 100644 index 0000000000..27ca37082a --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/RazorPagesWebSite.SimpleForms.html @@ -0,0 +1,6 @@ +
+
+
+
+
+
\ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/FormTagHelperTest.cs b/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/FormTagHelperTest.cs index 5c0d899c79..aaea09b846 100644 --- a/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/FormTagHelperTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/FormTagHelperTest.cs @@ -25,6 +25,122 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers { public class FormTagHelperTest { + [Fact] + public async Task ProcessAsync_EmptyHtmlStringActionGeneratesAntiforgery() + { + // Arrange + var expectedTagName = "form"; + var metadataProvider = new TestModelMetadataProvider(); + var tagHelperContext = new TagHelperContext( + tagName: "form", + allAttributes: new TagHelperAttributeList() + { + { "method", new HtmlString("post") } + }, + items: new Dictionary(), + uniqueId: "test"); + var output = new TagHelperOutput( + expectedTagName, + attributes: new TagHelperAttributeList() + { + { "action", HtmlString.Empty }, + }, + getChildContentAsync: (useCachedResult, encoder) => + { + var tagHelperContent = new DefaultTagHelperContent(); + tagHelperContent.SetContent("Something"); + return Task.FromResult(tagHelperContent); + }); + var urlHelper = new Mock(); + urlHelper + .Setup(mock => mock.Action(It.IsAny())).Returns("home/index"); + + var htmlGenerator = new TestableHtmlGenerator(metadataProvider, urlHelper.Object); + var viewContext = TestableHtmlGenerator.GetViewContext( + model: null, + htmlGenerator: htmlGenerator, + metadataProvider: metadataProvider); + var expectedPostContent = HtmlContentUtilities.HtmlContentToString( + htmlGenerator.GenerateAntiforgery(viewContext), + HtmlEncoder.Default); + var formTagHelper = new FormTagHelper(htmlGenerator) + { + ViewContext = viewContext, + Method = "post", + }; + + // Act + await formTagHelper.ProcessAsync(tagHelperContext, output); + + // Assert + var attribute = Assert.Single(output.Attributes, attr => attr.Name.Equals("action")); + Assert.Equal(HtmlString.Empty, attribute.Value); + Assert.Empty(output.PreElement.GetContent()); + Assert.Empty(output.PreContent.GetContent()); + Assert.Empty(output.Content.GetContent()); + Assert.Equal(expectedPostContent, output.PostContent.GetContent()); + Assert.Empty(output.PostElement.GetContent()); + Assert.Equal(expectedTagName, output.TagName); + } + + [Fact] + public async Task ProcessAsync_EmptyStringActionGeneratesAntiforgery() + { + // Arrange + var expectedTagName = "form"; + var metadataProvider = new TestModelMetadataProvider(); + var tagHelperContext = new TagHelperContext( + tagName: "form", + allAttributes: new TagHelperAttributeList() + { + { "method", new HtmlString("post") } + }, + items: new Dictionary(), + uniqueId: "test"); + var output = new TagHelperOutput( + expectedTagName, + attributes: new TagHelperAttributeList() + { + { "action", string.Empty }, + }, + getChildContentAsync: (useCachedResult, encoder) => + { + var tagHelperContent = new DefaultTagHelperContent(); + tagHelperContent.SetContent("Something"); + return Task.FromResult(tagHelperContent); + }); + var urlHelper = new Mock(); + urlHelper + .Setup(mock => mock.Action(It.IsAny())).Returns("home/index"); + + var htmlGenerator = new TestableHtmlGenerator(metadataProvider, urlHelper.Object); + var viewContext = TestableHtmlGenerator.GetViewContext( + model: null, + htmlGenerator: htmlGenerator, + metadataProvider: metadataProvider); + var expectedPostContent = HtmlContentUtilities.HtmlContentToString( + htmlGenerator.GenerateAntiforgery(viewContext), + HtmlEncoder.Default); + var formTagHelper = new FormTagHelper(htmlGenerator) + { + ViewContext = viewContext, + Method = "post", + }; + + // Act + await formTagHelper.ProcessAsync(tagHelperContext, output); + + // Assert + var attribute = Assert.Single(output.Attributes, attr => attr.Name.Equals("action")); + Assert.Equal(string.Empty, attribute.Value); + Assert.Empty(output.PreElement.GetContent()); + Assert.Empty(output.PreContent.GetContent()); + Assert.Empty(output.Content.GetContent()); + Assert.Equal(expectedPostContent, output.PostContent.GetContent()); + Assert.Empty(output.PostElement.GetContent()); + Assert.Equal(expectedTagName, output.TagName); + } + [Fact] public async Task ProcessAsync_GeneratesExpectedOutput() { diff --git a/test/WebSites/RazorPagesWebSite/SimpleForms.cshtml b/test/WebSites/RazorPagesWebSite/SimpleForms.cshtml new file mode 100644 index 0000000000..f4ed9e2b4f --- /dev/null +++ b/test/WebSites/RazorPagesWebSite/SimpleForms.cshtml @@ -0,0 +1,9 @@ +@page +@addTagHelper "*, Microsoft.AspNetCore.Mvc.TagHelpers" + +
+
+
+
+
+
\ No newline at end of file