diff --git a/src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs b/src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs index b034882364..5f1d538705 100644 --- a/src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.TagHelpers/FormTagHelper.cs @@ -187,7 +187,7 @@ namespace Microsoft.AspNetCore.Mvc.TagHelpers routeValues = new Dictionary(StringComparer.OrdinalIgnoreCase); } - // Unconditionally replace any value from asp-route-area. + // Unconditionally replace any value from asp-route-area. routeValues["area"] = Area; } diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs index ef3247ea04..22f12864d4 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs @@ -156,6 +156,16 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures throw new ArgumentNullException(nameof(viewContext)); } + // If we're inside a BeginForm/BeginRouteForm, the antiforgery token might have already been + // created and appended to the 'end form' content OR the form tag helper might have already generated + // an antiforgery token. + if (viewContext.FormContext.HasAntiforgeryToken) + { + return HtmlString.Empty; + } + + viewContext.FormContext.HasAntiforgeryToken = true; + return _antiforgery.GetHtml(viewContext.HttpContext); } @@ -1468,7 +1478,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures } // Group items in the SelectList if requested. - // The worst case complexity of this algorithm is O(number of groups*n). + // The worst case complexity of this algorithm is O(number of groups*n). // If there aren't any groups, it is O(n) where n is number of items in the list. var optionGenerated = new bool[itemsList.Count]; for (var i = 0; i < itemsList.Count; i++) diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/HtmlHelper.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/HtmlHelper.cs index f19653f629..a82fa8b9cb 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/HtmlHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/HtmlHelper.cs @@ -258,14 +258,6 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures /// public IHtmlContent AntiForgeryToken() { - // If we're inside a BeginForm/BeginRouteForm, the antiforgery token might have already been - // created and appended to the 'end form' content. - if (ViewContext.FormContext.HasAntiforgeryToken) - { - return HtmlString.Empty; - } - - ViewContext.FormContext.HasAntiforgeryToken = true; var html = _htmlGenerator.GenerateAntiforgery(ViewContext); return html ?? HtmlString.Empty; } @@ -900,7 +892,6 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures if (shouldGenerateAntiforgery) { ViewContext.FormContext.EndOfFormContent.Add(_htmlGenerator.GenerateAntiforgery(ViewContext)); - ViewContext.FormContext.HasAntiforgeryToken = true; } return CreateForm(); @@ -957,7 +948,6 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures if (shouldGenerateAntiforgery) { ViewContext.FormContext.EndOfFormContent.Add(_htmlGenerator.GenerateAntiforgery(ViewContext)); - ViewContext.FormContext.HasAntiforgeryToken = true; } return CreateForm(); diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TagHelpersTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TagHelpersTest.cs index 4fab802e9a..4960a8f138 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TagHelpersTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/TagHelpersTest.cs @@ -87,6 +87,42 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests #endif } + [Fact] + public async Task ReregisteringAntiforgeryTokenInsideFormTagHelper_DoesNotAddDuplicateAntiforgeryTokenFields() + { + // Arrange + var expectedMediaType = MediaTypeHeaderValue.Parse("text/html; charset=utf-8"); + var outputFile = "compiler/resources/TagHelpersWebSite.Employee.DuplicateAntiforgeryTokenRegistration.html"; + var expectedContent = + await ResourceFile.ReadResourceAsync(_resourcesAssembly, outputFile, sourceFile: false); + + // Act + var response = await Client.GetAsync("http://localhost/Employee/DuplicateAntiforgeryTokenRegistration"); + 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, "/Employee/DuplicateAntiforgeryTokenRegistration"); + +#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); + // Mono issue - https://github.com/aspnet/External/issues/19 + Assert.Equal( + PlatformNormalizer.NormalizeContent(expectedContent.Trim()), + responseContent, + ignoreLineEndingDifferences: true); +#endif + } + public static TheoryData TagHelpersAreInheritedFromViewImportsPagesData { get diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/TagHelpersWebSite.Employee.DuplicateAntiforgeryTokenRegistration.html b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/TagHelpersWebSite.Employee.DuplicateAntiforgeryTokenRegistration.html new file mode 100644 index 0000000000..d346a8a7ce --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/compiler/resources/TagHelpersWebSite.Employee.DuplicateAntiforgeryTokenRegistration.html @@ -0,0 +1,6 @@ + + +
+ + + diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Rendering/HtmlHelperFormTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Rendering/HtmlHelperFormTest.cs index a4c0ca56ff..073c9ac518 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Rendering/HtmlHelperFormTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/Rendering/HtmlHelperFormTest.cs @@ -396,7 +396,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering // Act & Assert using (var form = htmlHelper.BeginForm()) { - Assert.True(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( @@ -435,50 +434,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering // Act & Assert using (var form = htmlHelper.BeginForm(FormMethod.Post, antiforgery: null, htmlAttributes: null)) { - Assert.True(viewContext.FormContext.HasAntiforgeryToken); - } - - Assert.Equal( - "
", - writer.GetStringBuilder().ToString()); - } - - // This is an integration for the implicit antiforgery token added by BeginForm. - [Fact] - public void BeginForm_EndForm_RendersAntiforgeryToken_WithExplicitCallToAntiforgery() - { - // Arrange - var htmlGenerator = new Mock(MockBehavior.Strict); - htmlGenerator - .Setup(g => g.GenerateForm( - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny())) - .Returns(new TagBuilder("form")); - - htmlGenerator - .Setup(g => g.GenerateAntiforgery(It.IsAny())) - .Returns(new TagBuilder("antiforgery")); - - var htmlHelper = DefaultTemplatesUtilities.GetHtmlHelper(htmlGenerator.Object); - var serviceProvider = new Mock(); - serviceProvider.Setup(s => s.GetService(typeof(HtmlEncoder))).Returns(new HtmlTestEncoder()); - var viewContext = htmlHelper.ViewContext; - viewContext.HttpContext.RequestServices = serviceProvider.Object; - - var writer = viewContext.Writer as StringWriter; - Assert.NotNull(writer); - - // Act & Assert - using (var form = htmlHelper.BeginForm()) - { - Assert.True(viewContext.FormContext.HasAntiforgeryToken); - - // This call will no-op - Assert.Same(HtmlString.Empty, htmlHelper.AntiForgeryToken()); } Assert.Equal( @@ -518,7 +473,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering // Act & Assert using (var form = htmlHelper.BeginForm(FormMethod.Post, antiforgery: false, htmlAttributes: null)) { - Assert.False(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( @@ -557,7 +511,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering // Act & Assert using (var form = htmlHelper.BeginForm(FormMethod.Get, antiforgery: null, htmlAttributes: null)) { - Assert.False(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( @@ -598,7 +551,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering // Act & Assert using (var form = htmlHelper.BeginForm(method, antiforgery: true, htmlAttributes: null)) { - Assert.True(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( @@ -638,8 +590,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering // Act & Assert using (var form = htmlHelper.BeginForm(FormMethod.Post, antiforgery: false, htmlAttributes: null)) { - Assert.False(viewContext.FormContext.HasAntiforgeryToken); - // This call will ouput a token. Assert.Equal("antiforgery", Assert.IsType(htmlHelper.AntiForgeryToken()).TagName); } @@ -680,7 +630,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering // Act & Assert using (var form = htmlHelper.BeginRouteForm(routeValues: null)) { - Assert.True(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( @@ -723,7 +672,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering antiforgery: null, htmlAttributes: null)) { - Assert.True(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( @@ -767,7 +715,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering antiforgery: false, htmlAttributes: null)) { - Assert.False(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( @@ -810,7 +757,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering antiforgery: null, htmlAttributes: null)) { - Assert.False(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( @@ -855,7 +801,6 @@ namespace Microsoft.AspNetCore.Mvc.Rendering antiforgery: true, htmlAttributes: null)) { - Assert.True(viewContext.FormContext.HasAntiforgeryToken); } Assert.Equal( diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/DefaultHtmlGeneratorTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/DefaultHtmlGeneratorTest.cs index 5f29d03f6b..90ab2aaf10 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/DefaultHtmlGeneratorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/DefaultHtmlGeneratorTest.cs @@ -13,6 +13,7 @@ using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.Rendering; using Microsoft.AspNetCore.Mvc.Routing; +using Microsoft.AspNetCore.Mvc.TestCommon; using Microsoft.AspNetCore.Mvc.ViewEngines; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.Options; @@ -636,13 +637,40 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures Assert.Equal(expected, result); } + [Theory] + [InlineData(true, "")] + [InlineData(false, "")] + public void GenerateAntiforgery_GeneratesAntiforgeryFieldsOnlyIfRequired( + bool hasAntiforgeryToken, + string expectedAntiforgeryHtmlField) + { + // Arrange + var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var htmlGenerator = GetGenerator(metadataProvider); + var viewContext = GetViewContext(model: null, metadataProvider: metadataProvider); + viewContext.FormContext.HasAntiforgeryToken = hasAntiforgeryToken; + + // Act + var result = htmlGenerator.GenerateAntiforgery(viewContext); + + // Assert + var antiforgeryField = HtmlContentUtilities.HtmlContentToString(result, HtmlEncoder.Default); + Assert.Equal(expectedAntiforgeryHtmlField, antiforgeryField); + } + // GetCurrentValues uses only the IModelMetadataProvider passed to the DefaultHtmlGenerator constructor. private static IHtmlGenerator GetGenerator(IModelMetadataProvider metadataProvider) { var mvcViewOptionsAccessor = new Mock>(); mvcViewOptionsAccessor.SetupGet(accessor => accessor.Value).Returns(new MvcViewOptions()); var htmlEncoder = Mock.Of(); - var antiforgery = Mock.Of(); + var antiforgery = new Mock(); + antiforgery + .Setup(mock => mock.GetAndStoreTokens(It.IsAny())) + .Returns(() => + { + return new AntiforgeryTokenSet("requestToken", "cookieToken", "formFieldName", "headerName"); + }); var optionsAccessor = new Mock>(); optionsAccessor @@ -650,7 +678,7 @@ namespace Microsoft.AspNetCore.Mvc.ViewFeatures .Returns(new MvcOptions()); return new DefaultHtmlGenerator( - antiforgery, + antiforgery.Object, mvcViewOptionsAccessor.Object, metadataProvider, new UrlHelperFactory(), diff --git a/test/WebSites/BasicWebSite/Views/Antiforgery/Login.cshtml b/test/WebSites/BasicWebSite/Views/Antiforgery/Login.cshtml index 0bda046f1e..ec214f3e01 100644 --- a/test/WebSites/BasicWebSite/Views/Antiforgery/Login.cshtml +++ b/test/WebSites/BasicWebSite/Views/Antiforgery/Login.cshtml @@ -10,6 +10,7 @@
@using (Html.BeginForm("Login", "Antiforgery", FormMethod.Post, new { @class = "form-horizontal", role = "form" })) { + @* BeginForm already registers antiforgery tokens. This Html helper call should be no-op *@ @Html.AntiForgeryToken()

Use a local account to log in.

@@ -39,6 +40,7 @@ @using (Html.BeginForm("UseFacebookLogin", "Antiforgery", FormMethod.Post, new { @class = "form-horizontal", role = "form" })) { + @* BeginForm already registers antiforgery tokens. This Html helper call should be no-op *@ @Html.AntiForgeryToken()

Use Facebook login.

diff --git a/test/WebSites/TagHelpersWebSite/Controllers/EmployeeController.cs b/test/WebSites/TagHelpersWebSite/Controllers/EmployeeController.cs index a801a99aa3..392e3d0940 100644 --- a/test/WebSites/TagHelpersWebSite/Controllers/EmployeeController.cs +++ b/test/WebSites/TagHelpersWebSite/Controllers/EmployeeController.cs @@ -31,5 +31,11 @@ namespace TagHelpersWebSite.Controllers } return View(employee); } + + // GET: Employee/DuplicateAntiforgeryTokenRegistration + public IActionResult DuplicateAntiforgeryTokenRegistration() + { + return View(); + } } } \ No newline at end of file diff --git a/test/WebSites/TagHelpersWebSite/Views/Employee/DuplicateAntiforgeryTokenRegistration.cshtml b/test/WebSites/TagHelpersWebSite/Views/Employee/DuplicateAntiforgeryTokenRegistration.cshtml new file mode 100644 index 0000000000..a5276bec3b --- /dev/null +++ b/test/WebSites/TagHelpersWebSite/Views/Employee/DuplicateAntiforgeryTokenRegistration.cshtml @@ -0,0 +1,11 @@ +@addTagHelper *, Microsoft.AspNetCore.Mvc.TagHelpers +@{ + Layout = null; +} + + + @*Form tag helper already registers antiforgery token. The Html helper call should be no-op*@ +
@Html.AntiForgeryToken()
+ + +