From 93d9c11778662eed82d1022b1e8b3f5a2324d69c Mon Sep 17 00:00:00 2001 From: "ASP.NET CI" Date: Sun, 12 Aug 2018 19:22:44 +0000 Subject: [PATCH 1/7] Update dependencies.props [auto-updated: dependencies] --- build/dependencies.props | 146 +++++++++++++++++++-------------------- korebuild-lock.txt | 4 +- 2 files changed, 75 insertions(+), 75 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index c32829e4ce..7fbb2a958b 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -16,87 +16,87 @@ 0.43.0 2.1.1.1 2.1.1 - 2.2.0-preview1-34913 - 2.2.0-preview1-20180731.1 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 + 2.2.0-preview1-34967 + 2.2.0-preview1-20180807.2 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 5.2.6 2.8.0 2.8.0 - 2.2.0-preview1-34913 + 2.2.0-preview1-34967 1.7.0 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 2.1.0 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 2.0.9 2.1.2 2.2.0-preview1-26618-02 - 2.2.0-preview1-34913 - 2.2.0-preview1-34913 + 2.2.0-preview1-34967 + 2.2.0-preview1-34967 15.6.1 4.7.49 2.0.3 diff --git a/korebuild-lock.txt b/korebuild-lock.txt index c7af2292c7..3fbcc80189 100644 --- a/korebuild-lock.txt +++ b/korebuild-lock.txt @@ -1,2 +1,2 @@ -version:2.2.0-preview1-20180731.1 -commithash:29fde58465439f4bb9df40830635ed758e063daf +version:2.2.0-preview1-20180807.2 +commithash:11495dbd236104434e08cb1152fcb58cf2a20923 From 2421ae8c833b7cbb1991e43c8dda9632f9e2d61a Mon Sep 17 00:00:00 2001 From: Kristian Hellang Date: Tue, 14 Aug 2018 00:39:20 +0200 Subject: [PATCH 2/7] Add IStatusCodeActionResult (#8265) * Add IStatusCodeActionResult * Add unit test for explicitly implemented property on StatusCodeResult --- .../ContentResult.cs | 2 +- .../Infrastructure/IStatusCodeActionResult.cs | 17 +++++++++++++++++ .../ObjectResult.cs | 2 +- .../StatusCodeResult.cs | 5 ++++- .../JsonResult.cs | 3 ++- .../PartialViewResult.cs | 2 +- .../ViewComponentResult.cs | 2 +- .../ViewResult.cs | 2 +- .../HttpStatusCodeResultTests.cs | 14 ++++++++++++++ 9 files changed, 42 insertions(+), 7 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IStatusCodeActionResult.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ContentResult.cs b/src/Microsoft.AspNetCore.Mvc.Core/ContentResult.cs index 65ad59bf47..29c9ddab2d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ContentResult.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ContentResult.cs @@ -9,7 +9,7 @@ using Microsoft.Extensions.DependencyInjection; namespace Microsoft.AspNetCore.Mvc { - public class ContentResult : ActionResult + public class ContentResult : ActionResult, IStatusCodeActionResult { /// /// Gets or set the content representing the body of the response. diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IStatusCodeActionResult.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IStatusCodeActionResult.cs new file mode 100644 index 0000000000..a8eabf652d --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/IStatusCodeActionResult.cs @@ -0,0 +1,17 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + /// + /// Represents an that when executed will + /// produce an HTTP response with the specified . + /// + public interface IStatusCodeActionResult : IActionResult + { + /// + /// Gets or sets the HTTP status code. + /// + int? StatusCode { get; } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ObjectResult.cs b/src/Microsoft.AspNetCore.Mvc.Core/ObjectResult.cs index 8e850a363c..e81dbccfa1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ObjectResult.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ObjectResult.cs @@ -9,7 +9,7 @@ using Microsoft.Extensions.DependencyInjection; namespace Microsoft.AspNetCore.Mvc { - public class ObjectResult : ActionResult + public class ObjectResult : ActionResult, IStatusCodeActionResult { public ObjectResult(object value) { diff --git a/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs b/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs index 8db690c94e..305d279372 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/StatusCodeResult.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; @@ -12,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc /// Represents an that when executed will /// produce an HTTP response with the given response status code. /// - public class StatusCodeResult : ActionResult + public class StatusCodeResult : ActionResult, IStatusCodeActionResult { /// /// Initializes a new instance of the class @@ -29,6 +30,8 @@ namespace Microsoft.AspNetCore.Mvc /// public int StatusCode { get; } + int? IStatusCodeActionResult.StatusCode => StatusCode; + /// public override void ExecuteResult(ActionContext context) { diff --git a/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonResult.cs b/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonResult.cs index 4380224057..a4a576a83f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonResult.cs +++ b/src/Microsoft.AspNetCore.Mvc.Formatters.Json/JsonResult.cs @@ -4,6 +4,7 @@ using System; using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc.Formatters.Json.Internal; +using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.Extensions.DependencyInjection; using Newtonsoft.Json; @@ -12,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc /// /// An action result which formats the given object as JSON. /// - public class JsonResult : ActionResult + public class JsonResult : ActionResult, IStatusCodeActionResult { /// /// Creates a new with the given . diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/PartialViewResult.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/PartialViewResult.cs index e9f2763057..56b0f80139 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/PartialViewResult.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/PartialViewResult.cs @@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Mvc /// /// Represents an that renders a partial view to the response. /// - public class PartialViewResult : ActionResult + public class PartialViewResult : ActionResult, IStatusCodeActionResult { /// /// Gets or sets the HTTP status code. diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewComponentResult.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewComponentResult.cs index 898f676067..cbdb85574c 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewComponentResult.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewComponentResult.cs @@ -13,7 +13,7 @@ namespace Microsoft.AspNetCore.Mvc /// /// An which renders a view component to the response. /// - public class ViewComponentResult : ActionResult + public class ViewComponentResult : ActionResult, IStatusCodeActionResult { /// /// Gets or sets the arguments provided to the view component. diff --git a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewResult.cs b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewResult.cs index c98562fc9e..5534ec8666 100644 --- a/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewResult.cs +++ b/src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewResult.cs @@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Mvc /// /// Represents an that renders a view to the response. /// - public class ViewResult : ActionResult + public class ViewResult : ActionResult, IStatusCodeActionResult { /// /// Gets or sets the HTTP status code. diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/HttpStatusCodeResultTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/HttpStatusCodeResultTests.cs index e69fabaf5e..48634917fb 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/HttpStatusCodeResultTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/HttpStatusCodeResultTests.cs @@ -3,6 +3,7 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Infrastructure; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; @@ -32,6 +33,19 @@ namespace Microsoft.AspNetCore.Mvc Assert.Equal(StatusCodes.Status404NotFound, httpContext.Response.StatusCode); } + [Fact] + public void HttpStatusCodeResult_ReturnsCorrectStatusCodeAsIStatusCodeActionResult() + { + // Arrange + var result = new StatusCodeResult(StatusCodes.Status404NotFound); + + // Act + var statusResult = result as IStatusCodeActionResult; + + // Assert + Assert.Equal(StatusCodes.Status404NotFound, statusResult?.StatusCode); + } + private static IServiceCollection CreateServices() { var services = new ServiceCollection(); From 92f1dbe16cfd945d9759f290ae7e00fd1efa6e5a Mon Sep 17 00:00:00 2001 From: Mikael Mengistu Date: Tue, 14 Aug 2018 10:36:57 -0700 Subject: [PATCH 3/7] Remove AppVeyor badge from README (#8271) --- README.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/README.md b/README.md index e5aa14d771..a121f8b427 100644 --- a/README.md +++ b/README.md @@ -3,8 +3,6 @@ ASP.NET Core MVC **Note: For ASP.NET MVC 5.x, Web API 2.x, and Web Pages 3.x (not ASP.NET Core), see https://github.com/aspnet/AspNetWebStack** -AppVeyor: [![AppVeyor](https://ci.appveyor.com/api/projects/status/969jbosi0qwc1awg/branch/dev?svg=true)](https://ci.appveyor.com/project/aspnetci/mvc/branch/dev) - Travis: [![Travis](https://travis-ci.org/aspnet/Mvc.svg?branch=dev)](https://travis-ci.org/aspnet/Mvc) ASP.NET Core MVC gives you a powerful, patterns-based way to build dynamic websites that enables a clean separation of concerns and gives you full control over markup for enjoyable, agile development. ASP.NET Core MVC includes many features that enable fast, TDD-friendly development for creating sophisticated applications that use the latest web standards. From 7e25d7908a599c7c88718ad558894c57718cac5f Mon Sep 17 00:00:00 2001 From: Pranav K Date: Tue, 7 Aug 2018 17:49:50 -0700 Subject: [PATCH 4/7] Warn when the parameter name for a model bound complex parameter has the same name as a top level property Fixes #7753 --- .../DiagnosticDescriptors.cs | 10 + .../MvcFacts.cs | 2 +- .../SymbolNames.cs | 6 + .../TopLevelParameterNameAnalyzer.cs | 174 +++++++++++++ .../ApiControllerFacts.cs | 1 + ...rosoft.AspNetCore.Mvc.Api.Analyzers.csproj | 1 + ...ctionsWithParametersThatMatchProperties.cs | 13 + ...ticsAreReturned_ForModelBoundParameters.cs | 17 ++ ...NameProviderIsUsedToModifyParameterName.cs | 15 ++ .../GetNameTests.cs | 13 + .../IsProblematicParameter_IgnoresFields.cs | 9 + .../IsProblematicParameter_IgnoresMethods.cs | 9 + ...ticParameter_IgnoresNonPublicProperties.cs | 9 + ...ematicParameter_IgnoresStaticProperties.cs | 9 + ...meter_ReturnsFalse_ForFromBodyParameter.cs | 9 + ...lBinderAttributeIsUsedToRenameParameter.cs | 9 + ...elBinderAttributeIsUsedToRenameProperty.cs | 10 + ...IfParameterNameIsTheSameAsModelProperty.cs | 9 + ...erAttributeIsTheSameNameAsModelProperty.cs | 9 + ...lBindingAttributeHasSameNameAsParameter.cs | 10 + ...DiagnosticsAreReturnedForApiControllers.cs | 14 ++ .../NoDiagnosticsAreReturnedForNonActions.cs | 13 + ...ParameterIsRenamedUsingBindingAttribute.cs | 13 + .../TopLevelParameterNameAnalyzerTest.cs | 237 ++++++++++++++++++ test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs | 1 + 25 files changed, 621 insertions(+), 1 deletion(-) rename src/{Microsoft.AspNetCore.Mvc.Api.Analyzers => Microsoft.AspNetCore.Mvc.Analyzers}/MvcFacts.cs (98%) create mode 100644 src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForModelBoundParameters.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/GetNameTests.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresFields.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresMethods.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresNonPublicProperties.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresStaticProperties.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForFromBodyParameter.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForApiControllers.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForNonActions.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute.cs create mode 100644 test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs index b2684297bd..8b0aa761d1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/DiagnosticDescriptors.cs @@ -42,5 +42,15 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers "Usage", DiagnosticSeverity.Warning, isEnabledByDefault: true); + + public static readonly DiagnosticDescriptor MVC1004_ParameterNameCollidesWithTopLevelProperty = + new DiagnosticDescriptor( + "MVC1004", + "Rename model bound parameter.", + "Property on type '{0}' has the same name as parameter '{1}'. This may result in incorrect model binding. Consider renaming the parameter or using a model binding attribute to override the name.", + "Naming", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + helpLinkUri: "https://aka.ms/AA20pbc"); } } diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/MvcFacts.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs similarity index 98% rename from src/Microsoft.AspNetCore.Mvc.Api.Analyzers/MvcFacts.cs rename to src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs index a5dfe71dcc..90651247c4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/MvcFacts.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/MvcFacts.cs @@ -5,7 +5,7 @@ using System; using System.Diagnostics; using Microsoft.CodeAnalysis; -namespace Microsoft.AspNetCore.Mvc.Api.Analyzers +namespace Microsoft.AspNetCore.Mvc.Analyzers { internal static class MvcFacts { diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs index ca00478262..8448c01337 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs @@ -19,10 +19,14 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers public const string AuthorizeAttribute = "Microsoft.AspNetCore.Authorization.AuthorizeAttribute"; + public const string BindAttribute = "Microsoft.AspNetCore.Mvc.BindAttribute"; + public const string ControllerAttribute = "Microsoft.AspNetCore.Mvc.ControllerAttribute"; public const string DefaultStatusCodeAttribute = "Microsoft.AspNetCore.Mvc.Infrastructure.DefaultStatusCodeAttribute"; + public const string FromBodyAttribute = "Microsoft.AspNetCore.Mvc.FromBodyAttribute"; + public const string HtmlHelperPartialExtensionsType = "Microsoft.AspNetCore.Mvc.Rendering.HtmlHelperPartialExtensions"; public const string IApiBehaviorMetadata = "Microsoft.AspNetCore.Mvc.Internal.IApiBehaviorMetadata"; @@ -35,6 +39,8 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers public const string IHtmlHelperType = "Microsoft.AspNetCore.Mvc.Rendering.IHtmlHelper"; + public const string IModelNameProvider = "Microsoft.AspNetCore.Mvc.ModelBinding.IModelNameProvider"; + public const string IRouteTemplateProvider = "Microsoft.AspNetCore.Mvc.Routing.IRouteTemplateProvider"; public const string ModelStateDictionary = "Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary"; diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs new file mode 100644 index 0000000000..22f0ba92ba --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs @@ -0,0 +1,174 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class TopLevelParameterNameAnalyzer : DiagnosticAnalyzer + { + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create( + DiagnosticDescriptors.MVC1004_ParameterNameCollidesWithTopLevelProperty); + + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + + context.RegisterCompilationStartAction(compilationStartAnalysisContext => + { + var typeCache = new SymbolCache(compilationStartAnalysisContext.Compilation); + if (typeCache.ControllerAttribute == null || typeCache.ControllerAttribute.TypeKind == TypeKind.Error) + { + // No-op if we can't find types we care about. + return; + } + + InitializeWorker(compilationStartAnalysisContext, typeCache); + }); + } + + private void InitializeWorker(CompilationStartAnalysisContext compilationStartAnalysisContext, SymbolCache symbolCache) + { + compilationStartAnalysisContext.RegisterSymbolAction(symbolAnalysisContext => + { + var method = (IMethodSymbol)symbolAnalysisContext.Symbol; + if (method.MethodKind != MethodKind.Ordinary) + { + return; + } + + if (method.Parameters.Length == 0) + { + return; + } + + if (!MvcFacts.IsController(method.ContainingType, symbolCache.ControllerAttribute, symbolCache.NonControllerAttribute) || + !MvcFacts.IsControllerAction(method, symbolCache.NonActionAttribute, symbolCache.IDisposableDispose)) + { + return; + } + + if (method.ContainingType.HasAttribute(symbolCache.IApiBehaviorMetadata, inherit: true)) + { + // The issue of parameter name collision with properties affects complex model-bound types + // and not input formatting. Ignore ApiController instances since they default to formatting. + return; + } + + for (var i = 0; i < method.Parameters.Length; i++) + { + var parameter = method.Parameters[i]; + if (IsProblematicParameter(symbolCache, parameter)) + { + var location = parameter.Locations.Length != 0 ? + parameter.Locations[0] : + Location.None; + + symbolAnalysisContext.ReportDiagnostic( + Diagnostic.Create( + DiagnosticDescriptors.MVC1004_ParameterNameCollidesWithTopLevelProperty, + location, + parameter.Type.Name, + parameter.Name)); + } + } + }, SymbolKind.Method); + } + + internal static bool IsProblematicParameter(in SymbolCache symbolCache, IParameterSymbol parameter) + { + if (parameter.GetAttributes(symbolCache.FromBodyAttribute).Any()) + { + // Ignore input formatted parameters. + return false; + } + + var parameterName = GetName(symbolCache, parameter); + + var type = parameter.Type; + while (type != null) + { + foreach (var member in type.GetMembers()) + { + if (member.DeclaredAccessibility != Accessibility.Public || + member.IsStatic || + member.Kind != SymbolKind.Property) + { + continue; + } + + var propertyName = GetName(symbolCache, member); + if (string.Equals(parameterName, propertyName, StringComparison.OrdinalIgnoreCase)) + { + return true; + } + } + + type = type.BaseType; + } + + return false; + } + + internal static string GetName(in SymbolCache symbolCache, ISymbol symbol) + { + foreach (var attribute in symbol.GetAttributes(symbolCache.IModelNameProvider)) + { + // BindAttribute uses the Prefix property as an alias for IModelNameProvider.Name + var nameProperty = attribute.AttributeClass == symbolCache.BindAttribute ? "Prefix" : "Name"; + + // All of the built-in attributes (FromQueryAttribute, ModelBinderAttribute etc) only support setting the name via + // a property. We'll ignore constructor values. + for (var i = 0; i < attribute.NamedArguments.Length; i++) + { + var namedArgument = attribute.NamedArguments[i]; + var namedArgumentValue = namedArgument.Value; + if (string.Equals(namedArgument.Key, nameProperty, StringComparison.Ordinal) && + namedArgumentValue.Kind == TypedConstantKind.Primitive && + namedArgumentValue.Type.SpecialType == SpecialType.System_String && + namedArgumentValue.Value is string name) + { + return name; + } + } + } + + return symbol.Name; + } + + internal readonly struct SymbolCache + { + public SymbolCache(Compilation compilation) + { + BindAttribute = compilation.GetTypeByMetadataName(SymbolNames.BindAttribute); + ControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.ControllerAttribute); + FromBodyAttribute = compilation.GetTypeByMetadataName(SymbolNames.FromBodyAttribute); + IApiBehaviorMetadata = compilation.GetTypeByMetadataName(SymbolNames.IApiBehaviorMetadata); + IModelNameProvider = compilation.GetTypeByMetadataName(SymbolNames.IModelNameProvider); + NonControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonControllerAttribute); + NonActionAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonActionAttribute); + + var disposable = compilation.GetSpecialType(SpecialType.System_IDisposable); + var members = disposable.GetMembers(nameof(IDisposable.Dispose)); + IDisposableDispose = members.Length == 1 ? (IMethodSymbol)members[0] : null; + } + + public INamedTypeSymbol BindAttribute { get; } + public INamedTypeSymbol ControllerAttribute { get; } + public INamedTypeSymbol FromBodyAttribute { get; } + public INamedTypeSymbol IApiBehaviorMetadata { get; } + public INamedTypeSymbol IModelNameProvider { get; } + public INamedTypeSymbol NonControllerAttribute { get; } + public INamedTypeSymbol NonActionAttribute { get; } + public IMethodSymbol IDisposableDispose { get; } + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerFacts.cs b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerFacts.cs index b947b8dc5d..84534a5d9f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerFacts.cs +++ b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/ApiControllerFacts.cs @@ -1,6 +1,7 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using Microsoft.AspNetCore.Mvc.Analyzers; using Microsoft.CodeAnalysis; namespace Microsoft.AspNetCore.Mvc.Api.Analyzers diff --git a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/Microsoft.AspNetCore.Mvc.Api.Analyzers.csproj b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/Microsoft.AspNetCore.Mvc.Api.Analyzers.csproj index 5081b0d502..a4c48adc4b 100644 --- a/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/Microsoft.AspNetCore.Mvc.Api.Analyzers.csproj +++ b/src/Microsoft.AspNetCore.Mvc.Api.Analyzers/Microsoft.AspNetCore.Mvc.Api.Analyzers.csproj @@ -12,6 +12,7 @@ + diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties.cs new file mode 100644 index 0000000000..85642db52a --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties.cs @@ -0,0 +1,13 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties : Controller + { + [HttpPost] + public IActionResult EditPerson(DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchPropertiesModel /*MM*/model) => null; + } + + public class DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchPropertiesModel + { + public string Model { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForModelBoundParameters.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForModelBoundParameters.cs new file mode 100644 index 0000000000..be2bc2d7f0 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_ForModelBoundParameters.cs @@ -0,0 +1,17 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class DiagnosticsAreReturned_ForModelBoundParameters : Controller + { + [HttpPost] + public IActionResult EditPerson( + [FromBody] DiagnosticsAreReturned_ForModelBoundParametersModel model, + [FromQuery] DiagnosticsAreReturned_ForModelBoundParametersModel /*MM*/value) => null; + } + + public class DiagnosticsAreReturned_ForModelBoundParametersModel + { + public string Model { get; } + + public string Value { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName.cs new file mode 100644 index 0000000000..ed15be578a --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName.cs @@ -0,0 +1,15 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName : Controller + { + [HttpPost] + public IActionResult Edit([ModelBinder(Name = "model")] DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterNameModel /*MM*/parameter) => null; + } + + public class DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterNameModel + { + public string Model { get; } + + public string Value { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/GetNameTests.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/GetNameTests.cs new file mode 100644 index 0000000000..2810ab1f49 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/GetNameTests.cs @@ -0,0 +1,13 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class GetNameTests + { + public void NoAttribute(int param) { } + + public void SingleAttribute([ModelBinder(Name = "testModelName")] int param) { } + + public void SingleAttributeWithoutName([ModelBinder] int param) { } + + public void MultipleAttributes([ModelBinder(Name = "name1")][Bind(Prefix = "name2")] int param) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresFields.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresFields.cs new file mode 100644 index 0000000000..e0136bc5f3 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresFields.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_IgnoresFields + { + public string model; + + public void ActionMethod(IsProblematicParameter_IgnoresFields model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresMethods.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresMethods.cs new file mode 100644 index 0000000000..b5f79812d6 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresMethods.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_IgnoresMethods + { + public string Item() => null; + + public void ActionMethod(IsProblematicParameter_IgnoresMethods item) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresNonPublicProperties.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresNonPublicProperties.cs new file mode 100644 index 0000000000..fbfb437f54 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresNonPublicProperties.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_IgnoresNonPublicProperties + { + protected string Model { get; set; } + + public void ActionMethod(IsProblematicParameter_IgnoresNonPublicProperties model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresStaticProperties.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresStaticProperties.cs new file mode 100644 index 0000000000..065fca633f --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_IgnoresStaticProperties.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_IgnoresStaticProperties + { + public static string Model { get; set; } + + public void ActionMethod(IsProblematicParameter_IgnoresStaticProperties model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForFromBodyParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForFromBodyParameter.cs new file mode 100644 index 0000000000..f1b0011291 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForFromBodyParameter.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_ForFromBodyParameter + { + public string Model { get; set; } + + public void ActionMethod([FromBody] IsProblematicParameter_ReturnsFalse_ForFromBodyParameter model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs new file mode 100644 index 0000000000..4c83838f02 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter + { + public string Model { get; set; } + + public void ActionMethod([FromRoute(Name = "id")] IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs new file mode 100644 index 0000000000..e4a565bd62 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs @@ -0,0 +1,10 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty + { + [FromQuery(Name = "different")] + public string Model { get; set; } + + public void ActionMethod([Bind(Prefix = nameof(Model))] IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty different) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty.cs new file mode 100644 index 0000000000..adfcf507d0 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty + { + public string Model { get; set; } + + public void ActionMethod(IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty.cs new file mode 100644 index 0000000000..2b96ecc579 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty + { + public string Model { get; set; } + + public void ActionMethod([Bind(Prefix = "model")] IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty different) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter.cs new file mode 100644 index 0000000000..7f3d9c499d --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter.cs @@ -0,0 +1,10 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter + { + [ModelBinder(typeof(object), Name = "model")] + public string Different { get; set; } + + public void ActionMethod(IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForApiControllers.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForApiControllers.cs new file mode 100644 index 0000000000..3be2d2a07a --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForApiControllers.cs @@ -0,0 +1,14 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + [ApiController] + public class NoDiagnosticsAreReturnedForApiControllers : Controller + { + [HttpPost] + public IActionResult EditPerson(NoDiagnosticsAreReturnedForApiControllersModel model) => null; + } + + public class NoDiagnosticsAreReturnedForApiControllersModel + { + public string Model { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForNonActions.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForNonActions.cs new file mode 100644 index 0000000000..d6e7edce31 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedForNonActions.cs @@ -0,0 +1,13 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class NoDiagnosticsAreReturnedForNonActions : Controller + { + [NonAction] + public IActionResult EditPerson(NoDiagnosticsAreReturnedForNonActionsModel model) => null; + } + + public class NoDiagnosticsAreReturnedForNonActionsModel + { + public string Model { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute.cs new file mode 100644 index 0000000000..e870af3704 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute.cs @@ -0,0 +1,13 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute : Controller + { + [HttpPost] + public IActionResult EditPerson([FromForm(Name = "")] NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttributeModel model) => null; + } + + public class NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttributeModel + { + public string Model { get; } + } +} diff --git a/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs b/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs new file mode 100644 index 0000000000..e5add91ee2 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs @@ -0,0 +1,237 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Linq; +using System.Runtime.CompilerServices; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Analyzer.Testing; +using Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles; +using Microsoft.CodeAnalysis; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Analyzers +{ + public class TopLevelParameterNameAnalyzerTest + { + private MvcDiagnosticAnalyzerRunner Runner { get; } = new MvcDiagnosticAnalyzerRunner(new TopLevelParameterNameAnalyzer()); + + [Fact] + public Task DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchProperties() + => RunTest(nameof(DiagnosticsAreReturned_ForControllerActionsWithParametersThatMatchPropertiesModel), "model"); + + [Fact] + public Task DiagnosticsAreReturned_ForModelBoundParameters() + => RunTest(nameof(DiagnosticsAreReturned_ForModelBoundParametersModel), "value"); + + [Fact] + public Task DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterName() + => RunTest(nameof(DiagnosticsAreReturned_IfModelNameProviderIsUsedToModifyParameterNameModel), "parameter"); + + [Fact] + public Task NoDiagnosticsAreReturnedForApiControllers() + => RunNoDiagnosticsAreReturned(); + + [Fact] + public Task NoDiagnosticsAreReturnedIfParameterIsRenamedUsingBindingAttribute() + => RunNoDiagnosticsAreReturned(); + + [Fact] + public Task NoDiagnosticsAreReturnedForNonActions() + => RunNoDiagnosticsAreReturned(); + + [Fact] + public async Task IsProblematicParameter_ReturnsTrue_IfParameterNameIsTheSameAsModelProperty() + { + var result = await IsProblematicParameterTest(); + Assert.True(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsTrue_IfParameterNameWithBinderAttributeIsTheSameNameAsModelProperty() + { + var result = await IsProblematicParameterTest(); + Assert.True(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter() + { + var result = await IsProblematicParameterTest(); + Assert.True(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_ForFromBodyParameter() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_IgnoresStaticProperties() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_IgnoresFields() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_IgnoresMethods() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + [Fact] + public async Task IsProblematicParameter_IgnoresNonPublicProperties() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + + private async Task IsProblematicParameterTest([CallerMemberName] string testMethod = "") + { + var testSource = MvcTestSource.Read(GetType().Name, testMethod); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + + var modelType = compilation.GetTypeByMetadataName($"Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles.{testMethod}"); + var method = (IMethodSymbol)modelType.GetMembers("ActionMethod").First(); + var parameter = method.Parameters[0]; + + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var result = TopLevelParameterNameAnalyzer.IsProblematicParameter(symbolCache, parameter); + return result; + } + + [Fact] + public async Task GetName_ReturnsValueFromFirstAttributeWithValue() + { + var methodName = nameof(GetNameTests.SingleAttribute); + var compilation = await GetCompilationForGetName(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(GetNameTests).FullName); + var method = (IMethodSymbol)type.GetMembers(methodName).First(); + + var parameter = method.Parameters[0]; + var name = TopLevelParameterNameAnalyzer.GetName(symbolCache, parameter); + + Assert.Equal("testModelName", name); + } + + [Fact] + public async Task GetName_ReturnsName_IfNoAttributesAreSpecified() + { + var methodName = nameof(GetNameTests.NoAttribute); + var compilation = await GetCompilationForGetName(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(GetNameTests).FullName); + var method = (IMethodSymbol)type.GetMembers(methodName).First(); + + var parameter = method.Parameters[0]; + var name = TopLevelParameterNameAnalyzer.GetName(symbolCache, parameter); + + Assert.Equal("param", name); + } + + [Fact] + public async Task GetName_ReturnsName_IfAttributeDoesNotSpecifyName() + { + var methodName = nameof(GetNameTests.SingleAttributeWithoutName); + var compilation = await GetCompilationForGetName(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(GetNameTests).FullName); + var method = (IMethodSymbol)type.GetMembers(methodName).First(); + + var parameter = method.Parameters[0]; + var name = TopLevelParameterNameAnalyzer.GetName(symbolCache, parameter); + + Assert.Equal("param", name); + } + + [Fact] + public async Task GetName_ReturnsFirstName_IfMultipleAttributesAreSpecified() + { + var methodName = nameof(GetNameTests.MultipleAttributes); + var compilation = await GetCompilationForGetName(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(GetNameTests).FullName); + var method = (IMethodSymbol)type.GetMembers(methodName).First(); + + var parameter = method.Parameters[0]; + var name = TopLevelParameterNameAnalyzer.GetName(symbolCache, parameter); + + Assert.Equal("name1", name); + } + + private async Task GetCompilationForGetName() + { + var testSource = MvcTestSource.Read(GetType().Name, "GetNameTests"); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + return compilation; + } + + private async Task RunNoDiagnosticsAreReturned([CallerMemberName] string testMethod = "") + { + // Arrange + var testSource = MvcTestSource.Read(GetType().Name, testMethod); + var expectedLocation = testSource.DefaultMarkerLocation; + + // Act + var result = await Runner.GetDiagnosticsAsync(testSource.Source); + + // Assert + Assert.Empty(result); + } + + private async Task RunTest(string typeName, string parameterName, [CallerMemberName] string testMethod = "") + { + // Arrange + var descriptor = DiagnosticDescriptors.MVC1004_ParameterNameCollidesWithTopLevelProperty; + var testSource = MvcTestSource.Read(GetType().Name, testMethod); + var expectedLocation = testSource.DefaultMarkerLocation; + + // Act + var result = await Runner.GetDiagnosticsAsync(testSource.Source); + + // Assert + Assert.Collection( + result, + diagnostic => + { + Assert.Equal(descriptor.Id, diagnostic.Id); + Assert.Same(descriptor, diagnostic.Descriptor); + AnalyzerAssert.DiagnosticLocation(expectedLocation, diagnostic.Location); + Assert.Equal(string.Format(descriptor.MessageFormat.ToString(), typeName, parameterName), diagnostic.GetMessage()); + }); + } + } +} diff --git a/test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs b/test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs index e6009e7125..0544262951 100644 --- a/test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs +++ b/test/Mvc.Api.Analyzers.Test/MvcFactsTest.cs @@ -5,6 +5,7 @@ using System; using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Analyzer.Testing; +using Microsoft.AspNetCore.Mvc.Analyzers; using Microsoft.CodeAnalysis; using Xunit; From 200a70bb86ec36bfd4d3ec08c14bcd7883060dce Mon Sep 17 00:00:00 2001 From: Ryan Brandenburg Date: Wed, 8 Aug 2018 12:42:48 -0700 Subject: [PATCH 5/7] Update doc comments --- src/Microsoft.AspNetCore.Mvc.Abstractions/IUrlHelper.cs | 2 +- src/Microsoft.AspNetCore.Mvc.Core/UrlHelperExtensions.cs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/IUrlHelper.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/IUrlHelper.cs index fc329aadc4..33a23fef25 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/IUrlHelper.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/IUrlHelper.cs @@ -26,7 +26,7 @@ namespace Microsoft.AspNetCore.Mvc string Action(UrlActionContext actionContext); /// - /// Converts a virtual (relative) path to an application absolute path. + /// Converts a virtual (relative, starting with ~/) path to an application absolute path. /// /// /// If the specified content path does not start with the tilde (~) character, diff --git a/src/Microsoft.AspNetCore.Mvc.Core/UrlHelperExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/UrlHelperExtensions.cs index 93720a5826..af43e847bb 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/UrlHelperExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/UrlHelperExtensions.cs @@ -338,7 +338,7 @@ namespace Microsoft.AspNetCore.Mvc } /// - /// Generates a URL with an absolute path for the specified . + /// Generates a URL with a relative path for the specified . /// /// The . /// The page name to generate the url for. @@ -347,7 +347,7 @@ namespace Microsoft.AspNetCore.Mvc => Page(urlHelper, pageName, values: null); /// - /// Generates a URL with an absolute path for the specified . + /// Generates a URL with a relative path for the specified . /// /// The . /// The page name to generate the url for. @@ -357,7 +357,7 @@ namespace Microsoft.AspNetCore.Mvc => Page(urlHelper, pageName, pageHandler, values: null); /// - /// Generates a URL with an absolute path for the specified . + /// Generates a URL with a relative path for the specified . /// /// The . /// The page name to generate the url for. @@ -367,7 +367,7 @@ namespace Microsoft.AspNetCore.Mvc => Page(urlHelper, pageName, pageHandler: null, values: values); /// - /// Generates a URL with an absolute path for the specified . + /// Generates a URL with a relative path for the specified . /// /// The . /// The page name to generate the url for. From af770ede876670abd37aa5a034c3d82730a12746 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Wed, 15 Aug 2018 11:13:05 -0700 Subject: [PATCH 6/7] Ignore parameters that specify a model binder type --- .../SymbolNames.cs | 2 + .../TopLevelParameterNameAnalyzer.cs | 41 +++++++++++ ...alse_ForParametersWithCustomModelBinder.cs | 9 +++ ...SourceAttributeIsUsedToRenameParameter.cs} | 4 +- ...ngSourceAttributeIsUsedToRenameProperty.cs | 10 +++ ...elBinderAttributeIsUsedToRenameProperty.cs | 10 --- ...lBinderAttributeIsUsedToRenameParameter.cs | 9 +++ .../SpecifiesModelTypeTests.cs | 11 +++ .../TopLevelParameterNameAnalyzerTest.cs | 72 ++++++++++++++++++- 9 files changed, 154 insertions(+), 14 deletions(-) create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder.cs rename test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/{IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs => IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter.cs} (52%) create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty.cs delete mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter.cs create mode 100644 test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/SpecifiesModelTypeTests.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs index 8448c01337..ad152657df 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/SymbolNames.cs @@ -31,6 +31,8 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers public const string IApiBehaviorMetadata = "Microsoft.AspNetCore.Mvc.Internal.IApiBehaviorMetadata"; + public const string IBinderTypeProviderMetadata = "Microsoft.AspNetCore.Mvc.ModelBinding.IBinderTypeProviderMetadata"; + public const string IActionResult = "Microsoft.AspNetCore.Mvc.IActionResult"; public const string IConvertToActionResult = "Microsoft.AspNetCore.Mvc.IConvertToActionResult"; diff --git a/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs b/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs index 22f0ba92ba..ad5aeeaeab 100644 --- a/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Analyzers/TopLevelParameterNameAnalyzer.cs @@ -91,6 +91,12 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return false; } + if (SpecifiesModelType(symbolCache, parameter)) + { + // Ignore parameters that specify a model type. + return false; + } + var parameterName = GetName(symbolCache, parameter); var type = parameter.Type; @@ -144,6 +150,39 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return symbol.Name; } + internal static bool SpecifiesModelType(in SymbolCache symbolCache, IParameterSymbol parameterSymbol) + { + foreach (var attribute in parameterSymbol.GetAttributes(symbolCache.IBinderTypeProviderMetadata)) + { + // Look for a attribute property named BinderType being assigned. This would match + // [ModelBinder(BinderType = typeof(SomeBinder))] + for (var i = 0; i < attribute.NamedArguments.Length; i++) + { + var namedArgument = attribute.NamedArguments[i]; + var namedArgumentValue = namedArgument.Value; + if (string.Equals(namedArgument.Key, "BinderType", StringComparison.Ordinal) && + namedArgumentValue.Kind == TypedConstantKind.Type) + { + return true; + } + } + + // Look for the binder type being specified in the constructor. This would match + // [ModelBinder(typeof(SomeBinder))] + var constructorParameters = attribute.AttributeConstructor?.Parameters ?? ImmutableArray.Empty; + for (var i = 0; i < constructorParameters.Length; i++) + { + if (string.Equals(constructorParameters[i].Name, "binderType", StringComparison.Ordinal)) + { + // A constructor that requires binderType was used. + return true; + } + } + } + + return false; + } + internal readonly struct SymbolCache { public SymbolCache(Compilation compilation) @@ -152,6 +191,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers ControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.ControllerAttribute); FromBodyAttribute = compilation.GetTypeByMetadataName(SymbolNames.FromBodyAttribute); IApiBehaviorMetadata = compilation.GetTypeByMetadataName(SymbolNames.IApiBehaviorMetadata); + IBinderTypeProviderMetadata = compilation.GetTypeByMetadataName(SymbolNames.IBinderTypeProviderMetadata); IModelNameProvider = compilation.GetTypeByMetadataName(SymbolNames.IModelNameProvider); NonControllerAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonControllerAttribute); NonActionAttribute = compilation.GetTypeByMetadataName(SymbolNames.NonActionAttribute); @@ -165,6 +205,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers public INamedTypeSymbol ControllerAttribute { get; } public INamedTypeSymbol FromBodyAttribute { get; } public INamedTypeSymbol IApiBehaviorMetadata { get; } + public INamedTypeSymbol IBinderTypeProviderMetadata { get; } public INamedTypeSymbol IModelNameProvider { get; } public INamedTypeSymbol NonControllerAttribute { get; } public INamedTypeSymbol NonActionAttribute { get; } diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder.cs new file mode 100644 index 0000000000..4e86cb1df5 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder + { + public string Model { get; set; } + + public void ActionMethod([ModelBinder(typeof(object))] IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter.cs similarity index 52% rename from test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs rename to test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter.cs index 4c83838f02..20c2a39c3d 100644 --- a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter.cs +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter.cs @@ -1,9 +1,9 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles { - public class IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter + public class IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter { public string Model { get; set; } - public void ActionMethod([FromRoute(Name = "id")] IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter model) { } + public void ActionMethod([FromRoute(Name = "id")] IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter model) { } } } diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty.cs new file mode 100644 index 0000000000..902465213f --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty.cs @@ -0,0 +1,10 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty + { + [FromQuery(Name = "different")] + public string Model { get; set; } + + public void ActionMethod(IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs deleted file mode 100644 index e4a565bd62..0000000000 --- a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty.cs +++ /dev/null @@ -1,10 +0,0 @@ -namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles -{ - public class IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty - { - [FromQuery(Name = "different")] - public string Model { get; set; } - - public void ActionMethod([Bind(Prefix = nameof(Model))] IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty different) { } - } -} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter.cs new file mode 100644 index 0000000000..0c28fb1132 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter.cs @@ -0,0 +1,9 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter + { + public string Model { get; set; } + + public void ActionMethod([ModelBinder(Name = "model")] IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter different) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/SpecifiesModelTypeTests.cs b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/SpecifiesModelTypeTests.cs new file mode 100644 index 0000000000..e707001d87 --- /dev/null +++ b/test/Mvc.Analyzers.Test/TestFiles/TopLevelParameterNameAnalyzerTest/SpecifiesModelTypeTests.cs @@ -0,0 +1,11 @@ +namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFiles +{ + public class SpecifiesModelTypeTests + { + public void SpecifiesModelType_ReturnsFalse_IfModelBinderDoesNotSpecifyType([ModelBinder(Name = "Name")] object model) { } + + public void SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromConstructor([ModelBinder(typeof(object))] object model) { } + + public void SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromProperty([ModelBinder(BinderType = typeof(object))] object model) { } + } +} diff --git a/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs b/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs index e5add91ee2..7329ae9d13 100644 --- a/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs +++ b/test/Mvc.Analyzers.Test/TopLevelParameterNameAnalyzerTest.cs @@ -61,14 +61,21 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers } [Fact] - public async Task IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameParameter() + public async Task IsProblematicParameter_ReturnsTrue_IfModelBinderAttributeIsUsedToRenameParameter() + { + var result = await IsProblematicParameterTest(); + Assert.True(result); + } + + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameProperty() { var result = await IsProblematicParameterTest(); Assert.False(result); } [Fact] - public async Task IsProblematicParameter_ReturnsFalse_IfModelBinderAttributeIsUsedToRenameProperty() + public async Task IsProblematicParameter_ReturnsFalse_IfBindingSourceAttributeIsUsedToRenameParameter() { var result = await IsProblematicParameterTest(); Assert.False(result); @@ -81,6 +88,13 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers Assert.False(result); } + [Fact] + public async Task IsProblematicParameter_ReturnsFalse_ForParametersWithCustomModelBinder() + { + var result = await IsProblematicParameterTest(); + Assert.False(result); + } + [Fact] public async Task IsProblematicParameter_IgnoresStaticProperties() { @@ -199,6 +213,60 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers return compilation; } + [Fact] + public async Task SpecifiesModelType_ReturnsFalse_IfModelBinderDoesNotSpecifyType() + { + var testMethod = nameof(SpecifiesModelType_ReturnsFalse_IfModelBinderDoesNotSpecifyType); + var testSource = MvcTestSource.Read(GetType().Name, "SpecifiesModelTypeTests"); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(SpecifiesModelTypeTests).FullName); + var method = (IMethodSymbol)type.GetMembers(testMethod).First(); + + var parameter = method.Parameters[0]; + var result = TopLevelParameterNameAnalyzer.SpecifiesModelType(symbolCache, parameter); + Assert.False(result); + } + + [Fact] + public async Task SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromConstructor() + { + var testMethod = nameof(SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromConstructor); + var testSource = MvcTestSource.Read(GetType().Name, "SpecifiesModelTypeTests"); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(SpecifiesModelTypeTests).FullName); + var method = (IMethodSymbol)type.GetMembers(testMethod).First(); + + var parameter = method.Parameters[0]; + var result = TopLevelParameterNameAnalyzer.SpecifiesModelType(symbolCache, parameter); + Assert.True(result); + } + + [Fact] + public async Task SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromProperty() + { + var testMethod = nameof(SpecifiesModelType_ReturnsTrue_IfModelBinderSpecifiesTypeFromProperty); + var testSource = MvcTestSource.Read(GetType().Name, "SpecifiesModelTypeTests"); + var project = DiagnosticProject.Create(GetType().Assembly, new[] { testSource.Source }); + + var compilation = await project.GetCompilationAsync(); + var symbolCache = new TopLevelParameterNameAnalyzer.SymbolCache(compilation); + + var type = compilation.GetTypeByMetadataName(typeof(SpecifiesModelTypeTests).FullName); + var method = (IMethodSymbol)type.GetMembers(testMethod).First(); + + var parameter = method.Parameters[0]; + var result = TopLevelParameterNameAnalyzer.SpecifiesModelType(symbolCache, parameter); + Assert.True(result); + } + private async Task RunNoDiagnosticsAreReturned([CallerMemberName] string testMethod = "") { // Arrange From 522006d2c866add522a546aa3b4720577f1cee57 Mon Sep 17 00:00:00 2001 From: Pranav K Date: Fri, 10 Aug 2018 11:24:12 -0700 Subject: [PATCH 7/7] [Design] Add a MaxValidationDepth option to ValidationVisitor Fixes #7357 --- .../{Internal => }/DefaultObjectValidator.cs | 19 +- .../MvcCoreServiceCollectionExtensions.cs | 2 +- ...MvcOptionsConfigureCompatibilityOptions.cs | 3 + .../NullableCompatibilitySwitch.cs | 35 ++++ .../Validation/ValidationVisitor.cs | 56 +++++ .../MvcOptions.cs | 46 +++++ .../Properties/Resources.Designer.cs | 42 ++++ .../Resources.resx | 9 + .../ControllerBaseTest.cs | 11 +- .../DefaultControllerActivatorTest.cs | 2 +- .../DefaultControllerFactoryTest.cs | 3 +- ...ptionsConfigureCompatibilityOptionsTest.cs | 83 ++++++++ .../NullableCompatibilitySwitchTest.cs | 77 +++++++ .../ControllerBinderDelegateProviderTest.cs | 195 +++++------------- .../Internal/DefaultObjectValidatorTests.cs | 102 ++++++++- .../ModelBinding/ModelBindingHelperTest.cs | 14 +- .../ModelBinding/ParameterBinderTest.cs | 24 ++- .../InputObjectValidationTests.cs | 23 ++- .../ModelBindingTestHelper.cs | 10 +- .../Internal/PageActionInvokerTest.cs | 2 +- .../Internal/PageBinderFactoryTest.cs | 9 +- .../CompatibilitySwitchIntegrationTest.cs | 4 + .../ControllerTest.cs | 2 +- .../Controllers/ValidationController.cs | 6 + .../Models/InfinitelyRecursiveModel.cs | 29 +++ .../Models/RecursiveIdentifier.cs | 18 ++ test/WebSites/FormatterWebSite/Startup.cs | 4 +- 27 files changed, 643 insertions(+), 187 deletions(-) rename src/Microsoft.AspNetCore.Mvc.Core/{Internal => }/DefaultObjectValidator.cs (70%) create mode 100644 src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/NullableCompatibilitySwitch.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/MvcOptionsConfigureCompatibilityOptionsTest.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/NullableCompatibilitySwitchTest.cs create mode 100644 test/WebSites/FormatterWebSite/Models/InfinitelyRecursiveModel.cs create mode 100644 test/WebSites/FormatterWebSite/Models/RecursiveIdentifier.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultObjectValidator.cs b/src/Microsoft.AspNetCore.Mvc.Core/DefaultObjectValidator.cs similarity index 70% rename from src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultObjectValidator.cs rename to src/Microsoft.AspNetCore.Mvc.Core/DefaultObjectValidator.cs index 0afcdb0f5e..8fedb7702b 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultObjectValidator.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/DefaultObjectValidator.cs @@ -2,26 +2,33 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System.Collections.Generic; +using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; +using Microsoft.Extensions.Options; -namespace Microsoft.AspNetCore.Mvc.Internal +namespace Microsoft.AspNetCore.Mvc { /// /// The default implementation of . /// - public class DefaultObjectValidator : ObjectModelValidator + internal class DefaultObjectValidator : ObjectModelValidator { + private readonly MvcOptions _mvcOptions; + /// /// Initializes a new instance of . /// /// The . /// The list of . + /// Accessor to . public DefaultObjectValidator( IModelMetadataProvider modelMetadataProvider, - IList validatorProviders) + IList validatorProviders, + MvcOptions mvcOptions) : base(modelMetadataProvider, validatorProviders) { + _mvcOptions = mvcOptions; } public override ValidationVisitor GetValidationVisitor( @@ -31,12 +38,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal IModelMetadataProvider metadataProvider, ValidationStateDictionary validationState) { - return new ValidationVisitor( + var visitor = new ValidationVisitor( actionContext, validatorProvider, validatorCache, metadataProvider, validationState); + + visitor.MaxValidationDepth = _mvcOptions.MaxValidationDepth; + + return visitor; } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs index 3ebdc371fa..e7e7d5e0dd 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/DependencyInjection/MvcCoreServiceCollectionExtensions.cs @@ -228,7 +228,7 @@ namespace Microsoft.Extensions.DependencyInjection { var options = s.GetRequiredService>().Value; var metadataProvider = s.GetRequiredService(); - return new DefaultObjectValidator(metadataProvider, options.ModelValidatorProviders); + return new DefaultObjectValidator(metadataProvider, options.ModelValidatorProviders, options); }); services.TryAddSingleton(); services.TryAddSingleton(); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/MvcOptionsConfigureCompatibilityOptions.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/MvcOptionsConfigureCompatibilityOptions.cs index 76e3a16916..dcc5243fbb 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/MvcOptionsConfigureCompatibilityOptions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/MvcOptionsConfigureCompatibilityOptions.cs @@ -35,6 +35,9 @@ namespace Microsoft.AspNetCore.Mvc.Infrastructure if (Version >= CompatibilityVersion.Version_2_2) { values[nameof(MvcOptions.EnableEndpointRouting)] = true; + + // Matches JsonSerializerSettingsProvider.DefaultMaxDepth + values[nameof(MvcOptions.MaxValidationDepth)] = 32; } return values; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/NullableCompatibilitySwitch.cs b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/NullableCompatibilitySwitch.cs new file mode 100644 index 0000000000..53563ba157 --- /dev/null +++ b/src/Microsoft.AspNetCore.Mvc.Core/Infrastructure/NullableCompatibilitySwitch.cs @@ -0,0 +1,35 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + internal class NullableCompatibilitySwitch : ICompatibilitySwitch where TValue : struct + { + private TValue? _value; + + public NullableCompatibilitySwitch(string name) + { + Name = name; + } + + public bool IsValueSet { get; private set; } + + public string Name { get; } + + public TValue? Value + { + get => _value; + set + { + IsValueSet = true; + _value = value; + } + } + + object ICompatibilitySwitch.Value + { + get => Value; + set => Value = (TValue?)value; + } + } +} diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index fa467588a8..164928c54a 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -4,8 +4,10 @@ using System; using System.Collections.Generic; using System.Runtime.CompilerServices; +using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Mvc.ModelBinding.Internal; +using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation { @@ -15,6 +17,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation /// public class ValidationVisitor { + private int? _maxValidationDepth; + /// /// Creates a new . /// @@ -76,6 +80,31 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation protected ModelMetadata Metadata { get; set; } protected IValidationStrategy Strategy { get; set; } + /// + /// Gets or sets the maximum depth to constrain the validation visitor when validating. + /// + /// traverses the object graph of the model being validated. For models + /// that are very deep or are infinitely recursive, validation may result in stack overflow. + /// + /// + /// When not , will throw if + /// current traversal depth exceeds the specified value. + /// + /// + public int? MaxValidationDepth + { + get => _maxValidationDepth; + set + { + if (value != null && value <= 0) + { + throw new ArgumentOutOfRangeException(nameof(value)); + } + + _maxValidationDepth = value; + } + } + /// /// Indicates whether validation of a complex type should be performed if validation fails for any of its children. The default behavior is false. /// @@ -192,6 +221,33 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation return true; } + if (MaxValidationDepth != null && CurrentPath.Count > MaxValidationDepth) + { + // Non cyclic but too deep an object graph. + + // Pop the current model to make ValidationStack.Dispose happy + CurrentPath.Pop(model); + + string message; + switch (metadata.MetadataKind) + { + case ModelMetadataKind.Property: + message = Resources.FormatValidationVisitor_ExceededMaxPropertyDepth(nameof(ValidationVisitor), MaxValidationDepth, metadata.Name, metadata.ContainerType); + break; + + default: + // Since the minimum depth is never 0, MetadataKind can never be Parameter. Consequently we only special case MetadataKind.Property. + message = Resources.FormatValidationVisitor_ExceededMaxDepth(nameof(ValidationVisitor), MaxValidationDepth, metadata.ModelType); + break; + } + + message += " " + Resources.FormatValidationVisitor_ExceededMaxDepthFix(nameof(MvcOptions), nameof(MvcOptions.MaxValidationDepth)); + throw new InvalidOperationException(message) + { + HelpLink = "https://aka.ms/AA21ue1", + }; + } + var entry = GetValidationEntry(model); key = entry?.Key ?? key ?? string.Empty; metadata = entry?.Metadata ?? metadata; diff --git a/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs b/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs index cafe4e746e..34a55b4ce4 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/MvcOptions.cs @@ -31,6 +31,7 @@ namespace Microsoft.AspNetCore.Mvc private readonly CompatibilitySwitch _inputFormatterExceptionPolicy; private readonly CompatibilitySwitch _suppressBindingUndefinedValueToEnumType; private readonly CompatibilitySwitch _enableEndpointRouting; + private readonly NullableCompatibilitySwitch _maxValidationDepth; private readonly ICompatibilitySwitch[] _switches; /// @@ -56,6 +57,7 @@ namespace Microsoft.AspNetCore.Mvc _inputFormatterExceptionPolicy = new CompatibilitySwitch(nameof(InputFormatterExceptionPolicy), InputFormatterExceptionPolicy.AllExceptions); _suppressBindingUndefinedValueToEnumType = new CompatibilitySwitch(nameof(SuppressBindingUndefinedValueToEnumType)); _enableEndpointRouting = new CompatibilitySwitch(nameof(EnableEndpointRouting)); + _maxValidationDepth = new NullableCompatibilitySwitch(nameof(MaxValidationDepth)); _switches = new ICompatibilitySwitch[] { @@ -65,6 +67,7 @@ namespace Microsoft.AspNetCore.Mvc _inputFormatterExceptionPolicy, _suppressBindingUndefinedValueToEnumType, _enableEndpointRouting, + _maxValidationDepth, }; } @@ -396,6 +399,49 @@ namespace Microsoft.AspNetCore.Mvc /// public bool RequireHttpsPermanent { get; set; } + /// + /// Gets or sets the maximum depth to constrain the validation visitor when validating. Set to + /// to disable this feature. + /// + /// traverses the object graph of the model being validated. For models + /// that are very deep or are infinitely recursive, validation may result in stack overflow. + /// + /// + /// When not , will throw if + /// traversing an object exceeds the maximum allowed validation depth. + /// + /// + /// This property is associated with a compatibility switch and can provide a different behavior depending on + /// the configured compatibility version for the application. See for + /// guidance and examples of setting the application's compatibility version. + /// + /// + /// Configuring the desired value of the compatibility switch by calling this property's setter will take precedence + /// over the value implied by the application's . + /// + /// + /// If the application's compatibility version is set to then + /// this setting will have the value 200 unless explicitly configured. + /// + /// + /// If the application's compatibility version is set to or + /// earlier then this setting will have the value unless explicitly configured. + /// + /// + public int? MaxValidationDepth + { + get => _maxValidationDepth.Value; + set + { + if (value != null && value <= 0) + { + throw new ArgumentOutOfRangeException(nameof(value)); + } + + _maxValidationDepth.Value = value; + } + } + IEnumerator IEnumerable.GetEnumerator() { return ((IEnumerable)_switches).GetEnumerator(); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs index 65b63b8d27..354ce64258 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs @@ -1536,6 +1536,48 @@ namespace Microsoft.AspNetCore.Mvc.Core internal static string FormatApiConventionMethod_NoMethodFound(object p0, object p1) => string.Format(CultureInfo.CurrentCulture, GetString("ApiConventionMethod_NoMethodFound"), p0, p1); + /// + /// {0} exceeded the maximum configured validation depth '{1}' when validating type '{2}'. + /// + internal static string ValidationVisitor_ExceededMaxDepth + { + get => GetString("ValidationVisitor_ExceededMaxDepth"); + } + + /// + /// {0} exceeded the maximum configured validation depth '{1}' when validating type '{2}'. + /// + internal static string FormatValidationVisitor_ExceededMaxDepth(object p0, object p1, object p2) + => string.Format(CultureInfo.CurrentCulture, GetString("ValidationVisitor_ExceededMaxDepth"), p0, p1, p2); + + /// + /// This may indicate a very deep or infinitely recursive object graph. Consider modifying '{0}.{1}' or suppressing validation on the model type. + /// + internal static string ValidationVisitor_ExceededMaxDepthFix + { + get => GetString("ValidationVisitor_ExceededMaxDepthFix"); + } + + /// + /// This may indicate a very deep or infinitely recursive object graph. Consider modifying '{0}.{1}' or suppressing validation on the model type. + /// + internal static string FormatValidationVisitor_ExceededMaxDepthFix(object p0, object p1) + => string.Format(CultureInfo.CurrentCulture, GetString("ValidationVisitor_ExceededMaxDepthFix"), p0, p1); + + /// + /// {0} exceeded the maximum configured validation depth '{1}' when validating property '{2}' on type '{3}'. + /// + internal static string ValidationVisitor_ExceededMaxPropertyDepth + { + get => GetString("ValidationVisitor_ExceededMaxPropertyDepth"); + } + + /// + /// {0} exceeded the maximum configured validation depth '{1}' when validating property '{2}' on type '{3}'. + /// + internal static string FormatValidationVisitor_ExceededMaxPropertyDepth(object p0, object p1, object p2, object p3) + => string.Format(CultureInfo.CurrentCulture, GetString("ValidationVisitor_ExceededMaxPropertyDepth"), p0, p1, p2, p3); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx index 4b31a83761..4916783a8c 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx @@ -457,4 +457,13 @@ A method named '{0}' was not found on convention type '{1}'. + + {0} exceeded the maximum configured validation depth '{1}' when validating type '{2}'. + + + This may indicate a very deep or infinitely recursive object graph. Consider modifying '{0}.{1}' or suppressing validation on the model type. + + + {0} exceeded the maximum configured validation depth '{1}' when validating property '{2}' on type '{3}'. + \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ControllerBaseTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ControllerBaseTest.cs index d46f73d642..d26aa506c7 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ControllerBaseTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ControllerBaseTest.cs @@ -2711,7 +2711,8 @@ namespace Microsoft.AspNetCore.Mvc.Core.Test var controller = GetController(binder, valueProvider: null); controller.ObjectValidator = new DefaultObjectValidator( controller.MetadataProvider, - new[] { Mock.Of() }); + new[] { Mock.Of() }, + new MvcOptions()); var model = new TryValidateModelModel(); @@ -2747,7 +2748,8 @@ namespace Microsoft.AspNetCore.Mvc.Core.Test var controller = GetController(binder, valueProvider: null); controller.ObjectValidator = new DefaultObjectValidator( controller.MetadataProvider, - new[] { provider.Object }); + new[] { provider.Object }, + new MvcOptions()); // Act var result = controller.TryValidateModel(model, "Prefix"); @@ -2783,7 +2785,8 @@ namespace Microsoft.AspNetCore.Mvc.Core.Test var controller = GetController(binder, valueProvider: null); controller.ObjectValidator = new DefaultObjectValidator( controller.MetadataProvider, - new[] { provider.Object }); + new[] { provider.Object }, + new MvcOptions()); // Act var result = controller.TryValidateModel(model); @@ -2867,7 +2870,7 @@ namespace Microsoft.AspNetCore.Mvc.Core.Test ControllerContext = controllerContext, MetadataProvider = metadataProvider, ModelBinderFactory = binderFactory.Object, - ObjectValidator = new DefaultObjectValidator(metadataProvider, validatorProviders), + ObjectValidator = new DefaultObjectValidator(metadataProvider, validatorProviders, new MvcOptions()), }; return controller; diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs index 90305c7a08..2e72325feb 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerActivatorTest.cs @@ -135,7 +135,7 @@ namespace Microsoft.AspNetCore.Mvc.Controllers .Returns(metadataProvider); services .Setup(s => s.GetService(typeof(IObjectModelValidator))) - .Returns(new DefaultObjectValidator(metadataProvider, new List())); + .Returns(new DefaultObjectValidator(metadataProvider, new List(), new MvcOptions())); return services.Object; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs index 436693bbb9..3a705e334e 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Controllers/DefaultControllerFactoryTest.cs @@ -215,7 +215,8 @@ namespace Microsoft.AspNetCore.Mvc.Controllers .Setup(s => s.GetService(typeof(IObjectModelValidator))) .Returns(new DefaultObjectValidator( metadataProvider, - TestModelValidatorProvider.CreateDefaultProvider().ValidatorProviders)); + TestModelValidatorProvider.CreateDefaultProvider().ValidatorProviders, + new MvcOptions())); return services.Object; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/MvcOptionsConfigureCompatibilityOptionsTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/MvcOptionsConfigureCompatibilityOptionsTest.cs new file mode 100644 index 0000000000..d661fe0bd5 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/MvcOptionsConfigureCompatibilityOptionsTest.cs @@ -0,0 +1,83 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNetCore.Mvc.Infrastructure; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Core.Infrastructure +{ + public class MvcOptionsConfigureCompatibilityOptionsTest + { + [Fact] + public void PostConfigure_ConfiguresMaxValidationDepth() + { + // Arrange + var mvcOptions = new MvcOptions(); + var mvcCompatibilityOptions = new MvcCompatibilityOptions + { + CompatibilityVersion = CompatibilityVersion.Version_2_2, + }; + + var configureOptions = new MvcOptionsConfigureCompatibilityOptions( + NullLoggerFactory.Instance, + Options.Create(mvcCompatibilityOptions)); + + // Act + configureOptions.PostConfigure(string.Empty, mvcOptions); + + // Assert + Assert.Equal(32, mvcOptions.MaxValidationDepth); + } + + [Fact] + public void PostConfigure_DoesNotConfiguresMaxValidationDepth_WhenSetToNull() + { + // Arrange + var mvcOptions = new MvcOptions + { + MaxValidationDepth = null, + }; + var mvcCompatibilityOptions = new MvcCompatibilityOptions + { + CompatibilityVersion = CompatibilityVersion.Version_2_2, + }; + + var configureOptions = new MvcOptionsConfigureCompatibilityOptions( + NullLoggerFactory.Instance, + Options.Create(mvcCompatibilityOptions)); + + // Act + configureOptions.PostConfigure(string.Empty, mvcOptions); + + // Assert + Assert.Null(mvcOptions.MaxValidationDepth); + } + + [Fact] + public void PostConfigure_DoesNotConfiguresMaxValidationDepth_WhenSetToValue() + { + // Arrange + var expected = 13; + var mvcOptions = new MvcOptions + { + MaxValidationDepth = expected, + }; + var mvcCompatibilityOptions = new MvcCompatibilityOptions + { + CompatibilityVersion = CompatibilityVersion.Version_2_2, + }; + + var configureOptions = new MvcOptionsConfigureCompatibilityOptions( + NullLoggerFactory.Instance, + Options.Create(mvcCompatibilityOptions)); + + // Act + configureOptions.PostConfigure(string.Empty, mvcOptions); + + // Assert + Assert.Equal(expected, mvcOptions.MaxValidationDepth); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/NullableCompatibilitySwitchTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/NullableCompatibilitySwitchTest.cs new file mode 100644 index 0000000000..bb5f1de6ae --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Infrastructure/NullableCompatibilitySwitchTest.cs @@ -0,0 +1,77 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.Infrastructure +{ + public class NullableCompatibilitySwitchTest + { + [Fact] + public void Constructor_WithName_IsValueSetIsFalse() + { + // Arrange & Act + var @switch = new NullableCompatibilitySwitch("TestProperty"); + + // Assert + Assert.Null(@switch.Value); + Assert.False(@switch.IsValueSet); + } + + [Fact] + public void ValueNonInterface_SettingValueToNull_SetsIsValueSetToTrue() + { + // Arrange + var @switch = new NullableCompatibilitySwitch("TestProperty"); + + // Act + @switch.Value = null; + + // Assert + Assert.Null(@switch.Value); + Assert.True(@switch.IsValueSet); + } + + [Fact] + public void ValueNonInterface_SettingValue_SetsIsValueSetToTrue() + { + // Arrange + var @switch = new NullableCompatibilitySwitch("TestProperty"); + + // Act + @switch.Value = false; + + // Assert + Assert.False(@switch.Value); + Assert.True(@switch.IsValueSet); + } + + [Fact] + public void ValueInterface_SettingValueToNull_SetsIsValueSetToTrue() + { + // Arrange + var @switch = new NullableCompatibilitySwitch("TestProperty"); + + // Act + ((ICompatibilitySwitch)@switch).Value = null; + + // Assert + Assert.Null(@switch.Value); + Assert.True(@switch.IsValueSet); + } + + [Fact] + public void ValueInterface_SettingValue_SetsIsValueSetToTrue() + { + // Arrange + var @switch = new NullableCompatibilitySwitch("TestProperty"); + + // Act + ((ICompatibilitySwitch)@switch).Value = true; + + // Assert + Assert.True(@switch.Value); + Assert.True(@switch.IsValueSet); + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerBinderDelegateProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerBinderDelegateProviderTest.cs index 6b9b9041db..a666abd913 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerBinderDelegateProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerBinderDelegateProviderTest.cs @@ -62,14 +62,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal var factory = GetModelBinderFactory(binder.Object); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); var controller = new TestController(); - var parameterBinder = new ParameterBinder( + + var parameterBinder = GetParameterBinder( modelMetadataProvider, factory, - new DefaultObjectValidator( - modelMetadataProvider, - new[] { GetModelValidatorProvider(mockValidator.Object) }), - _optionsAccessor, - NullLoggerFactory.Instance); + GetModelValidatorProvider(mockValidator.Object)); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -115,18 +112,16 @@ namespace Microsoft.AspNetCore.Mvc.Internal var mockValidator = new Mock(MockBehavior.Strict); mockValidator.Setup(o => o.Validate(It.IsAny())); + + var validatorProvider = GetModelValidatorProvider(mockValidator.Object); var factory = GetModelBinderFactory(binder.Object); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); var controller = new TestController(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, factory, - new DefaultObjectValidator( - modelMetadataProvider, - new[] { GetModelValidatorProvider(mockValidator.Object) }), - _optionsAccessor, - NullLoggerFactory.Instance); + GetModelValidatorProvider(mockValidator.Object)); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -170,14 +165,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var factory = GetModelBinderFactory(binder.Object); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); var controllerContext = GetControllerContext(actionDescriptor); var controller = new TestController(); @@ -217,14 +207,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var factory = GetModelBinderFactory(binder.Object); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); var controllerContext = GetControllerContext(actionDescriptor); var controller = new TestController(); @@ -272,14 +257,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var factory = GetModelBinderFactory(binder.Object); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); var controllerContext = GetControllerContext(actionDescriptor); var controller = new TestController(); @@ -329,14 +309,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal .Setup(p => p.GetMetadataForParameter(ParameterInfos.NoAttributesParameterInfo)) .Returns(modelMetadata.Object); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( mockMetadataProvider.Object, - factory, - new DefaultObjectValidator( - mockMetadataProvider.Object, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -382,14 +357,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal .Setup(p => p.GetMetadataForType(typeof(Person))) .Returns(modelMetadata.Object); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( mockMetadataProvider.Object, - factory, - new DefaultObjectValidator( - mockMetadataProvider.Object, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -431,14 +401,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal .Returns(new[] { new ModelValidationResult("memberName", "some message") }); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, factory, - new DefaultObjectValidator( - modelMetadataProvider, - new[] { GetModelValidatorProvider(mockValidator.Object) }), - _optionsAccessor, - NullLoggerFactory.Instance); + GetModelValidatorProvider(mockValidator.Object)); + var controller = new TestController(); var arguments = new Dictionary(StringComparer.Ordinal); @@ -489,9 +456,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var parameterBinder = new ParameterBinder( modelMetadataProvider, factory, - new DefaultObjectValidator( - modelMetadataProvider, - new[] { GetModelValidatorProvider(mockValidator.Object) }), + GetObjectValidator(modelMetadataProvider, GetModelValidatorProvider(mockValidator.Object)), Options.Create(mvcOptions), NullLoggerFactory.Instance); var controller = new TestController(); @@ -538,14 +503,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal var factory = GetModelBinderFactory(binder.Object); var controller = new TestController(); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + GetModelValidatorProvider(mockValidator.Object)); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -590,9 +551,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var parameterBinder = new ParameterBinder( modelMetadataProvider, factory, - new DefaultObjectValidator( - modelMetadataProvider, - new[] { GetModelValidatorProvider(mockValidator.Object) }), + GetObjectValidator(modelMetadataProvider, GetModelValidatorProvider(mockValidator.Object)), _optionsAccessor, NullLoggerFactory.Instance); @@ -689,9 +648,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var parameterBinder = new ParameterBinder( modelMetadataProvider, factory, - new DefaultObjectValidator( - modelMetadataProvider, - new[] { GetModelValidatorProvider(mockValidator.Object) }), + GetObjectValidator(modelMetadataProvider, GetModelValidatorProvider(mockValidator.Object)), _optionsAccessor, NullLoggerFactory.Instance); @@ -731,14 +688,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var factory = GetModelBinderFactory("Hello"); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -776,14 +728,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var expected = new List { "Hello", "World", "!!" }; var factory = GetModelBinderFactory(expected); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -821,14 +768,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var binder = new StubModelBinder(ModelBindingResult.Success(model: null)); var factory = GetModelBinderFactory(binder); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Some non default value. controller.NonNullableProperty = -1; @@ -867,14 +809,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var binder = new StubModelBinder(ModelBindingResult.Success(model: null)); var factory = GetModelBinderFactory(binder); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Some non default value. controller.NullableProperty = -1; @@ -934,14 +871,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var binder = new StubModelBinder(ModelBindingResult.Success(model: null)); var factory = GetModelBinderFactory(binder); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Some non default value. controller.NullableProperty = -1; @@ -1002,14 +934,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var binder = new StubModelBinder(ModelBindingResult.Success(model: null)); var factory = GetModelBinderFactory(binder); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Some non default value. controller.NullableProperty = -1; @@ -1094,14 +1021,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal var factory = GetModelBinderFactory(inputValue); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -1174,14 +1096,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal controllerContext.ValueProviderFactories.Add(new SimpleValueProviderFactory()); var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); - var parameterBinder = new ParameterBinder( + var parameterBinder = GetParameterBinder( modelMetadataProvider, - factory, - new DefaultObjectValidator( - modelMetadataProvider, - new IModelValidatorProvider[] { }), - _optionsAccessor, - NullLoggerFactory.Instance); + factory); // Act var binderDelegate = ControllerBinderDelegateProvider.CreateBinderDelegate( @@ -1278,7 +1195,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var parameterBinder = new Mock( new EmptyModelMetadataProvider(), factory, - new DefaultObjectValidator(modelMetadataProvider, new[] { modelValidatorProvider }), + GetObjectValidator(modelMetadataProvider, modelValidatorProvider), _optionsAccessor, NullLoggerFactory.Instance); parameterBinder.Setup(p => p.BindModelAsync( @@ -1415,16 +1332,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal } private static ParameterBinder GetParameterBinder( - IModelBinderFactory factory = null, - IObjectModelValidator validator = null, IModelMetadataProvider modelMetadataProvider = null, + IModelBinderFactory factory = null, IModelValidatorProvider modelValidatorProvider = null) { - if (validator == null) - { - validator = CreateObjectValidator(); - } - if (factory == null) { factory = TestModelBinderFactory.CreateDefault(); @@ -1436,9 +1347,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } var metadataProvider = modelMetadataProvider ?? TestModelMetadataProvider.CreateDefaultProvider(); - var objectModelValidator = new DefaultObjectValidator( - metadataProvider, - new[] { modelValidatorProvider }); + var objectModelValidator = GetObjectValidator(modelMetadataProvider, modelValidatorProvider); return new ParameterBinder( metadataProvider, @@ -1448,16 +1357,14 @@ namespace Microsoft.AspNetCore.Mvc.Internal NullLoggerFactory.Instance); } - private static IObjectModelValidator CreateObjectValidator() + private static DefaultObjectValidator GetObjectValidator( + IModelMetadataProvider modelMetadataProvider, + IModelValidatorProvider validatorProvider) { - var mockValidator = new Mock(MockBehavior.Strict); - mockValidator - .Setup(o => o.Validate( - It.IsAny(), - It.IsAny(), - It.IsAny(), - It.IsAny())); - return mockValidator.Object; + return new DefaultObjectValidator( + modelMetadataProvider, + new[] { validatorProvider }, + _options); } // No need for bind-related attributes on properties in this controller class. Properties are added directly diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs index a9ee38cfc6..fad6540560 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/DefaultObjectValidatorTests.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.ComponentModel.DataAnnotations; using System.IO; using System.Linq; +using System.Reflection; using System.Text; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Infrastructure; @@ -22,7 +23,9 @@ namespace Microsoft.AspNetCore.Mvc.Internal { public class DefaultObjectValidatorTests { - private IModelMetadataProvider MetadataProvider { get; } = TestModelMetadataProvider.CreateDefaultProvider(); + private readonly MvcOptions _options = new MvcOptions(); + + private ModelMetadataProvider MetadataProvider { get; } = TestModelMetadataProvider.CreateDefaultProvider(); [Fact] public void Validate_SimpleValueType_Valid_WithPrefix() @@ -1258,6 +1261,70 @@ namespace Microsoft.AspNetCore.Mvc.Internal }); } + [Fact] + public void Validate_Throws_IfValidationDepthExceedsMaxDepth() + { + // Arrange + var maxDepth = 5; + var expected = $"ValidationVisitor exceeded the maximum configured validation depth '{maxDepth}' when validating property '{nameof(DepthObject.Depth)}' on type '{typeof(DepthObject)}'. " + + "This may indicate a very deep or infinitely recursive object graph. Consider modifying 'MvcOptions.MaxValidationDepth' or suppressing validation on the model type."; + _options.MaxValidationDepth = maxDepth; + var actionContext = new ActionContext(); + var validator = CreateValidator(); + var model = new DepthObject(maxDepth); + var validationState = new ValidationStateDictionary + { + { model, new ValidationStateEntry() } + }; + + // Act & Assert + var ex = Assert.Throws(() => validator.Validate(actionContext, validationState, prefix: string.Empty, model)); + Assert.Equal(expected, ex.Message); + } + + [Fact] + public void Validate_WorksIfObjectGraphIsSmallerThanMaxDepth() + { + // Arrange + var maxDepth = 5; + _options.MaxValidationDepth = maxDepth; + var actionContext = new ActionContext(); + var validator = CreateValidator(); + var model = new DepthObject(maxDepth - 1); + var validationState = new ValidationStateDictionary + { + { model, new ValidationStateEntry() } + }; + + // Act & Assert + validator.Validate(actionContext, validationState, prefix: string.Empty, model); + Assert.True(actionContext.ModelState.IsValid); + } + + [Fact] + public void Validate_Throws_WithMaxDepth_1() + { + // Arrange + var maxDepth = 1; + var expected = $"ValidationVisitor exceeded the maximum configured validation depth '{maxDepth}' when validating property '{nameof(DepthObject.Depth)}' on type '{typeof(DepthObject)}'. " + + "This may indicate a very deep or infinitely recursive object graph. Consider modifying 'MvcOptions.MaxValidationDepth' or suppressing validation on the model type."; + _options.MaxValidationDepth = maxDepth; + var actionContext = new ActionContext(); + var validator = CreateValidator(); + var model = new DepthObject(maxDepth + 1); + var validationState = new ValidationStateDictionary + { + { model, new ValidationStateEntry() } + }; + var method = GetType().GetMethod(nameof(Validate_Throws_ForTopLeveleMetadataData), BindingFlags.NonPublic | BindingFlags.Instance); + var metadata = MetadataProvider.GetMetadataForParameter(method.GetParameters()[0]); + + // Act & Assert + var ex = Assert.Throws(() => validator.Validate(actionContext, validationState, prefix: string.Empty, model)); + Assert.Equal(expected, ex.Message); + Assert.NotNull(ex.HelpLink); + } + private static DefaultObjectValidator CreateValidator(Type excludedType) { var excludeFilters = new List(); @@ -1268,14 +1335,14 @@ namespace Microsoft.AspNetCore.Mvc.Internal var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(excludeFilters.ToArray()); var validatorProviders = TestModelValidatorProvider.CreateDefaultProvider().ValidatorProviders; - return new DefaultObjectValidator(metadataProvider, validatorProviders); + return new DefaultObjectValidator(metadataProvider, validatorProviders, new MvcOptions()); } - private static DefaultObjectValidator CreateValidator(params IMetadataDetailsProvider[] providers) + private DefaultObjectValidator CreateValidator(params IMetadataDetailsProvider[] providers) { var metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(providers); var validatorProviders = TestModelValidatorProvider.CreateDefaultProvider().ValidatorProviders; - return new DefaultObjectValidator(metadataProvider, validatorProviders); + return new DefaultObjectValidator(metadataProvider, validatorProviders, _options); } private static void AssertKeysEqual(ModelStateDictionary modelState, params string[] keys) @@ -1398,6 +1465,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal void DoSomething(); } + private void Validate_Throws_ForTopLeveleMetadataData(DepthObject depthObject) { } + // Custom validation attribute that returns multiple entries in ValidationResult.MemberNames and those member // names are indexers. An example scenario is an attribute that confirms all entries in a list are unique. private class InvalidItemsAttribute : ValidationAttribute @@ -1442,5 +1511,30 @@ namespace Microsoft.AspNetCore.Mvc.Internal { public string City { get; set; } } + + private class DepthObject + { + public DepthObject(int maxAllowedDepth, int depth = 0) + { + MaxAllowedDepth = maxAllowedDepth; + Depth = depth; + } + + public int Depth { get; } + public int MaxAllowedDepth { get; } + + public DepthObject Instance + { + get + { + if (Depth == MaxAllowedDepth - 1) + { + return this; + } + + return new DepthObject(MaxAllowedDepth, Depth + 1); + } + } + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBindingHelperTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBindingHelperTest.cs index 062b5fddce..116224e517 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBindingHelperTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ModelBindingHelperTest.cs @@ -84,7 +84,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding metadataProvider, GetModelBinderFactory(binderProviders), valueProvider, - new DefaultObjectValidator(metadataProvider, new[] { validator })); + new DefaultObjectValidator(metadataProvider, new[] { validator }, new MvcOptions())); // Assert Assert.False(result); @@ -124,7 +124,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding metadataProvider, GetModelBinderFactory(binderProviders), valueProvider, - new DefaultObjectValidator(metadataProvider, new[] { validator })); + new DefaultObjectValidator(metadataProvider, new[] { validator }, new MvcOptions())); // Assert Assert.True(result); @@ -202,7 +202,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding metadataProvider, GetModelBinderFactory(binderProviders), valueProvider, - new DefaultObjectValidator(metadataProvider, new[] { validator }), + new DefaultObjectValidator(metadataProvider, new[] { validator }, new MvcOptions()), propertyFilter); // Assert @@ -278,7 +278,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding TestModelMetadataProvider.CreateDefaultProvider(), GetModelBinderFactory(binderProviders), valueProvider, - new DefaultObjectValidator(metadataProvider, new[] { validator }), + new DefaultObjectValidator(metadataProvider, new[] { validator }, new MvcOptions()), m => m.IncludedProperty, m => m.MyProperty); @@ -329,7 +329,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding metadataProvider, GetModelBinderFactory(binderProviders), valueProvider, - new DefaultObjectValidator(metadataProvider, new[] { validator })); + new DefaultObjectValidator(metadataProvider, new[] { validator }, new MvcOptions())); // Assert // Includes everything. @@ -531,7 +531,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding metadataProvider, GetModelBinderFactory(binderProviders), valueProvider, - new DefaultObjectValidator(metadataProvider, new[] { validator }), + new DefaultObjectValidator(metadataProvider, new[] { validator }, new MvcOptions()), propertyFilter); // Assert @@ -599,7 +599,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding metadataProvider, GetModelBinderFactory(binderProviders), valueProvider, - new DefaultObjectValidator(metadataProvider, new[] { validator })); + new DefaultObjectValidator(metadataProvider, new[] { validator }, new MvcOptions())); // Assert Assert.True(result); diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs index d82f37de92..9464e6def7 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs @@ -522,7 +522,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Mock.Of(), new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -576,7 +577,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Mock.Of(), new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -630,7 +632,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Mock.Of(), new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -683,7 +686,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Mock.Of(), new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -741,7 +745,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Mock.Of(), new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -802,7 +807,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Mock.Of(), new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -868,7 +874,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Mock.Of(), new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -947,7 +954,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding mockModelBinderFactory.Object, new DefaultObjectValidator( mockModelMetadataProvider.Object, - new[] { GetModelValidatorProvider(validator) }), + new[] { GetModelValidatorProvider(validator) }, + new MvcOptions()), optionsAccessor, loggerFactory ?? NullLoggerFactory.Instance); } diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputObjectValidationTests.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputObjectValidationTests.cs index 05e12e9f64..138e70f6c8 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputObjectValidationTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/InputObjectValidationTests.cs @@ -1,11 +1,13 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Generic; using System.Net; using System.Net.Http; using System.Text; using System.Threading.Tasks; +using FormatterWebSite; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Testing.xunit; using Newtonsoft.Json; @@ -209,7 +211,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests // Arrange var invalidRequestData = "{\"FirstName\":\"Test\", \"LastName\": \"Testsson\"}"; var content = new StringContent(invalidRequestData, Encoding.UTF8, "application/json"); - var expectedErrorMessage = + var expectedErrorMessage = "{\"LastName\":[\"The field LastName must be a string with a maximum length of 5.\"]}"; // Act @@ -229,7 +231,7 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests // Arrange var invalidRequestData = "{\"FirstName\":\"Testname\", \"LastName\": \"\"}"; var content = new StringContent(invalidRequestData, Encoding.UTF8, "application/json"); - var expectedError = + var expectedError = "{\"LastName\":[\"The LastName field is required.\"]," + "\"FirstName\":[\"The field FirstName must be a string with a maximum length of 5.\"]}"; @@ -243,5 +245,22 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests var responseContent = await response.Content.ReadAsStringAsync(); Assert.Equal(expectedError, actual: responseContent); } + + // Test for https://github.com/aspnet/Mvc/issues/7357 + [Fact] + public async Task ValidationThrowsError_WhenValidationExceedsMaxValidationDepth() + { + // Arrange + var expected = $"ValidationVisitor exceeded the maximum configured validation depth '32' when validating property 'Value' on type '{typeof(RecursiveIdentifier)}'. " + + "This may indicate a very deep or infinitely recursive object graph. Consider modifying 'MvcOptions.MaxValidationDepth' or suppressing validation on the model type."; + var requestMessage = new HttpRequestMessage(HttpMethod.Post, "Validation/ValidationThrowsError_WhenValidationExceedsMaxValidationDepth") + { + Content = new StringContent(@"{ ""Id"": ""S-1-5-21-1004336348-1177238915-682003330-512"" }", Encoding.UTF8, "application/json"), + }; + + // Act & Assert + var ex = await Assert.ThrowsAsync(() => Client.SendAsync(requestMessage)); + Assert.Equal(expected, ex.Message); + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ModelBindingTestHelper.cs b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ModelBindingTestHelper.cs index b3686104a8..91a290406a 100644 --- a/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ModelBindingTestHelper.cs +++ b/test/Microsoft.AspNetCore.Mvc.IntegrationTests/ModelBindingTestHelper.cs @@ -9,7 +9,6 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Mvc.ApplicationParts; using Microsoft.AspNetCore.Mvc.Controllers; -using Microsoft.AspNetCore.Mvc.Internal; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.AspNetCore.Routing; @@ -79,7 +78,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests new ModelBinderFactory(metadataProvider, options, serviceProvider), new DefaultObjectValidator( metadataProvider, - new[] { new CompositeModelValidatorProvider(GetModelValidatorProviders(options)) }), + new[] { new CompositeModelValidatorProvider(GetModelValidatorProviders(options)) }, + options.Value), options, NullLoggerFactory.Instance); } @@ -102,7 +102,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests new ModelBinderFactory(metadataProvider, options, services), new DefaultObjectValidator( metadataProvider, - new[] { new CompositeModelValidatorProvider(GetModelValidatorProviders(options)) }), + new[] { new CompositeModelValidatorProvider(GetModelValidatorProviders(options)) }, + options.Value), options, NullLoggerFactory.Instance); } @@ -127,7 +128,8 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests { return new DefaultObjectValidator( metadataProvider, - GetModelValidatorProviders(options)); + GetModelValidatorProviders(options), + options?.Value ?? new MvcOptions()); } private static IList GetModelValidatorProviders(IOptions options) diff --git a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerTest.cs b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerTest.cs index 5b7899ac3e..45078ef126 100644 --- a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageActionInvokerTest.cs @@ -1555,7 +1555,7 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal return new ParameterBinder( metadataProvider, factory, - new DefaultObjectValidator(metadataProvider, new[] { validator }), + new DefaultObjectValidator(metadataProvider, new[] { validator }, new MvcOptions()), Options.Create(mvcOptions), NullLoggerFactory.Instance); } diff --git a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageBinderFactoryTest.cs b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageBinderFactoryTest.cs index eed043db10..6c983e1138 100644 --- a/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageBinderFactoryTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.RazorPages.Test/Internal/PageBinderFactoryTest.cs @@ -553,7 +553,8 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal modelBinderFactory, new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -678,7 +679,8 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal modelBinderFactory, new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), _optionsAccessor, NullLoggerFactory.Instance); @@ -732,7 +734,8 @@ namespace Microsoft.AspNetCore.Mvc.RazorPages.Internal modelBinderFactory, new DefaultObjectValidator( modelMetadataProvider, - new[] { TestModelValidatorProvider.CreateDefaultProvider() }), + new[] { TestModelValidatorProvider.CreateDefaultProvider() }, + new MvcOptions()), Options.Create(mvcOptions), NullLoggerFactory.Instance); diff --git a/test/Microsoft.AspNetCore.Mvc.Test/IntegrationTest/CompatibilitySwitchIntegrationTest.cs b/test/Microsoft.AspNetCore.Mvc.Test/IntegrationTest/CompatibilitySwitchIntegrationTest.cs index 858dcdf26d..f5a19379d0 100644 --- a/test/Microsoft.AspNetCore.Mvc.Test/IntegrationTest/CompatibilitySwitchIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Test/IntegrationTest/CompatibilitySwitchIntegrationTest.cs @@ -40,6 +40,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTest Assert.False(jsonOptions.AllowInputFormatterExceptionMessages); Assert.False(razorPagesOptions.AllowAreas); Assert.False(mvcOptions.EnableEndpointRouting); + Assert.Null(mvcOptions.MaxValidationDepth); } [Fact] @@ -65,6 +66,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTest Assert.True(jsonOptions.AllowInputFormatterExceptionMessages); Assert.True(razorPagesOptions.AllowAreas); Assert.False(mvcOptions.EnableEndpointRouting); + Assert.Null(mvcOptions.MaxValidationDepth); } [Fact] @@ -90,6 +92,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTest Assert.True(jsonOptions.AllowInputFormatterExceptionMessages); Assert.True(razorPagesOptions.AllowAreas); Assert.True(mvcOptions.EnableEndpointRouting); + Assert.Equal(32, mvcOptions.MaxValidationDepth); } [Fact] @@ -115,6 +118,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTest Assert.True(jsonOptions.AllowInputFormatterExceptionMessages); Assert.True(razorPagesOptions.AllowAreas); Assert.True(mvcOptions.EnableEndpointRouting); + Assert.Equal(32, mvcOptions.MaxValidationDepth); } // This just does the minimum needed to be able to resolve these options. diff --git a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ControllerTest.cs b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ControllerTest.cs index d88b338f7c..6abab5d7a5 100644 --- a/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ControllerTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ControllerTest.cs @@ -458,7 +458,7 @@ namespace Microsoft.AspNetCore.Mvc.Test { ControllerContext = controllerContext, MetadataProvider = metadataProvider, - ObjectValidator = new DefaultObjectValidator(metadataProvider, valiatorProviders), + ObjectValidator = new DefaultObjectValidator(metadataProvider, valiatorProviders, new MvcOptions()), TempData = tempData, ViewData = viewData, }; diff --git a/test/WebSites/FormatterWebSite/Controllers/ValidationController.cs b/test/WebSites/FormatterWebSite/Controllers/ValidationController.cs index 9f2bd1b536..5aee43c803 100644 --- a/test/WebSites/FormatterWebSite/Controllers/ValidationController.cs +++ b/test/WebSites/FormatterWebSite/Controllers/ValidationController.cs @@ -78,5 +78,11 @@ namespace FormatterWebSite return Ok(); } + + [HttpPost] + public IActionResult ValidationThrowsError_WhenValidationExceedsMaxValidationDepth([FromBody] InfinitelyRecursiveModel model) + { + return Ok(); + } } } \ No newline at end of file diff --git a/test/WebSites/FormatterWebSite/Models/InfinitelyRecursiveModel.cs b/test/WebSites/FormatterWebSite/Models/InfinitelyRecursiveModel.cs new file mode 100644 index 0000000000..370151143e --- /dev/null +++ b/test/WebSites/FormatterWebSite/Models/InfinitelyRecursiveModel.cs @@ -0,0 +1,29 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using Newtonsoft.Json; + +namespace FormatterWebSite +{ + public class InfinitelyRecursiveModel + { + [JsonConverter(typeof(StringIdentifierConverter))] + public RecursiveIdentifier Id { get; set; } + + private class StringIdentifierConverter : JsonConverter + { + public override bool CanConvert(Type objectType) => objectType == typeof(RecursiveIdentifier); + + public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer) + { + return new RecursiveIdentifier(reader.Value.ToString()); + } + + public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer) + { + throw new NotImplementedException(); + } + } + } +} diff --git a/test/WebSites/FormatterWebSite/Models/RecursiveIdentifier.cs b/test/WebSites/FormatterWebSite/Models/RecursiveIdentifier.cs new file mode 100644 index 0000000000..847a01b428 --- /dev/null +++ b/test/WebSites/FormatterWebSite/Models/RecursiveIdentifier.cs @@ -0,0 +1,18 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace FormatterWebSite +{ + // A System.Security.Principal.SecurityIdentifier like type that works on xplat + public class RecursiveIdentifier + { + public RecursiveIdentifier(string identifier) + { + Value = identifier; + } + + public string Value { get; } + + public RecursiveIdentifier AccountIdentifier => new RecursiveIdentifier(Value); + } +} \ No newline at end of file diff --git a/test/WebSites/FormatterWebSite/Startup.cs b/test/WebSites/FormatterWebSite/Startup.cs index ee2744a028..c1b5895cea 100644 --- a/test/WebSites/FormatterWebSite/Startup.cs +++ b/test/WebSites/FormatterWebSite/Startup.cs @@ -19,12 +19,12 @@ namespace FormatterWebSite options.InputFormatters.Add(new StringInputFormatter()); }) - .AddXmlDataContractSerializerFormatters(); + .AddXmlDataContractSerializerFormatters() + .SetCompatibilityVersion(CompatibilityVersion.Latest); services.Configure(options => { options.SerializerSettings.Converters.Insert(0, new IModelConverter()); }); } - public void Configure(IApplicationBuilder app) { app.UseMvc(routes =>