From 53bfa2f89f59bf330c12d87c7b1a055b1d806d9a Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Wed, 18 Sep 2019 20:39:52 +0800 Subject: [PATCH 1/8] Fix the IndexOutOfRangeException warning and add a option for configing if only look at Controller-derived classes. --- docs/Analyzer Configuration.md | 9 + .../UseAutoValidateAntiforgeryToken.cs | 39 +++-- .../UseAutoValidateAntiforgeryTokenTests.cs | 158 ++++++++---------- .../MinimalImplementations/ASPNetCoreApis.cs | 4 +- .../Options/EditorConfigOptionNames.cs | 5 + 5 files changed, 113 insertions(+), 102 deletions(-) diff --git a/docs/Analyzer Configuration.md b/docs/Analyzer Configuration.md index 5890b818bb..af33f7fa2a 100644 --- a/docs/Analyzer Configuration.md +++ b/docs/Analyzer Configuration.md @@ -298,3 +298,12 @@ Option Values: Integer values of System.Runtime.InteropServices.DllImportSearchP Default Value: Specific to each configurable rule ('770', which is AssemblyDirectory | UseDllDirectoryForDependencies | ApplicationDirectory, by default for most rules) Example: `dotnet_code_quality.CA5392.unsafe_DllImportSearchPath_bits = 770` + +#### Configure if only look at Controller-derived classes when considering CSRF +Option Name: `only_look_at_derived_classes_of_Controller` + +Option Values: Boolean values + +Default Value: Specific to each configurable rule ('true' by default for most rules) + +Example: `dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = false` diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs index fa7d51c1f0..7c87477696 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs @@ -25,7 +25,7 @@ public sealed class UseAutoValidateAntiforgeryToken : DiagnosticAnalyzer typeof(MicrosoftNetCoreAnalyzersResources), nameof(MicrosoftNetCoreAnalyzersResources.UseAutoValidateAntiforgeryToken), nameof(MicrosoftNetCoreAnalyzersResources.UseAutoValidateAntiforgeryTokenMessage), - DiagnosticHelpers.EnabledByDefaultIfNotBuildingVSIX, + false, helpLinkUri: null, descriptionResourceStringName: nameof(MicrosoftNetCoreAnalyzersResources.UseAutoValidateAntiforgeryTokenDescription), customTags: WellKnownDiagnosticTags.Telemetry); @@ -49,12 +49,13 @@ public sealed class UseAutoValidateAntiforgeryToken : DiagnosticAnalyzer WellKnownTypeNames.MicrosoftAspNetCoreMvcHttpDeleteAttribute, WellKnownTypeNames.MicrosoftAspNetCoreMvcHttpPatchAttribute); + // It is used to translate ConcurrentDictionary into ConcurrentHashset, which is not provided. + private const bool placeholder = true; + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create( UseAutoValidateAntiforgeryTokenRule, MissHttpVerbAttributeRule); - public delegate bool RequirementsOfValidateMethod(IMethodSymbol methodSymbol); - public override void Initialize(AnalysisContext context) { context.EnableConcurrentExecution(); @@ -90,8 +91,14 @@ public override void Initialize(AnalysisContext context) return; } + var cancellationToken = compilationStartAnalysisContext.CancellationToken; + var onlyLookAtDerivedClassesOfController = compilationStartAnalysisContext.Options.GetBoolOptionValue( + optionName: EditorConfigOptionNames.OnlyLookAtDerivedClassesOfController, + rule: UseAutoValidateAntiforgeryTokenRule, + defaultValue: true, + cancellationToken: cancellationToken); + // A dictionary from method symbol to set of methods calling it directly. - // The bool value in the sub ConcurrentDictionary is not used, use ConcurrentDictionary rather than HashSet just for the concurrency security. var inverseGraph = new ConcurrentDictionary>(); // Ignore cases where a global anti forgery filter is in use. @@ -100,7 +107,7 @@ public override void Initialize(AnalysisContext context) // Verify that validate anti forgery token attributes are used somewhere within this project, // to avoid reporting false positives on projects that use an alternative approach to mitigate CSRF issues. var usingValidateAntiForgeryAttribute = false; - var onAuthorizationAsyncMethodSymbols = new HashSet(); + var onAuthorizationAsyncMethodSymbols = new ConcurrentDictionary(); var actionMethodSymbols = new HashSet<(IMethodSymbol, string)>(); var actionMethodNeedAddingHttpVerbAttributeSymbols = new HashSet(); @@ -152,7 +159,7 @@ public override void Initialize(AnalysisContext context) } callers = inverseGraph.GetOrAdd(calledSymbol, (_) => new ConcurrentDictionary()); - callers.TryAdd(owningSymbol, true); + callers.TryAdd(owningSymbol, placeholder); }, OperationKind.Invocation, OperationKind.FieldReference); }); @@ -190,16 +197,18 @@ public override void Initialize(AnalysisContext context) else if (potentialAntiForgeryFilter.AllInterfaces.Contains(iAsyncAuthorizationFilterTypeSymbol) || potentialAntiForgeryFilter.AllInterfaces.Contains(iAuthorizationFilterTypeSymbol)) { - onAuthorizationAsyncMethodSymbols.Add( + onAuthorizationAsyncMethodSymbols.TryAdd( potentialAntiForgeryFilter .GetMembers() .OfType() .FirstOrDefault( - s => (s.Name == "OnAuthorizationAsync" || - s.Name == "OnAuthorization") && - s.ReturnType.Equals(taskTypeSymbol) && + s => (s.Name == "OnAuthorizationAsync" && + s.ReturnType.Equals(taskTypeSymbol) || + s.Name == "OnAuthorization" && + s.ReturnType.SpecialType == SpecialType.System_Void) && s.Parameters.Length == 1 && - s.Parameters[0].Type.Equals(authorizationFilterContextTypeSymbol))); + s.Parameters[0].Type.Equals(authorizationFilterContextTypeSymbol)), + placeholder); } } } @@ -215,9 +224,10 @@ public override void Initialize(AnalysisContext context) var derivedControllerTypeSymbol = (INamedTypeSymbol)symbolAnalysisContext.Symbol; var baseTypes = derivedControllerTypeSymbol.GetBaseTypes(); - // An subtype of `Microsoft.AspNetCore.Mvc.Controller` or `Microsoft.AspNetCore.Mvc.ControllerBase`). + // An subtype of `Microsoft.AspNetCore.Mvc.Controller`, which indicates that cookie-based authentication is used and thus CSRF is a concern. if (baseTypes.Contains(controllerTypeSymbol) || - baseTypes.Contains(controllerBaseTypeSymbol)) + (!onlyLookAtDerivedClassesOfController && + baseTypes.Contains(controllerBaseTypeSymbol))) { // The controller class is not protected by a validate anti forgery token attribute. if (!IsUsingAntiFogeryAttribute(derivedControllerTypeSymbol)) @@ -340,7 +350,8 @@ void FindAllTheSpecifiedCalleeMethods(ISymbol methodSymbol, HashSet vis foreach (var child in callingMethods.Keys) { - if (onAuthorizationAsyncMethodSymbols.Contains(child)) + if (child is IMethodSymbol childMethodSymbol && + onAuthorizationAsyncMethodSymbols.ContainsKey(childMethodSymbol)) { results[methodSymbol].Add(child); } diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs index eb1a7b4d39..22fb2b0232 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs @@ -28,7 +28,7 @@ public void Test_GlobalAntiForgeryFilter_Add_ChildrenOfIAsyncAuthorizationFilter using Microsoft.AspNetCore.Mvc.Filters; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } @@ -42,7 +42,7 @@ public Task OnAuthorizationAsync (AuthorizationFilterContext context) } } -class TestClass : ControllerBase +class TestClass : Controller { [HttpDelete] public AcceptedAtActionResult CustomizedActionMethod (string actionName) @@ -73,7 +73,7 @@ public void Test_GlobalAntiForgeryFilter_Add_ChildrenOfIAuthorizationFilter_NotC using Microsoft.AspNetCore.Mvc.Filters; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } @@ -81,13 +81,12 @@ class FilterClass : IAuthorizationFilter { public DefaultAntiforgery defaultAntiforgery; - public Task OnAuthorization (AuthorizationFilterContext context) + public void OnAuthorization (AuthorizationFilterContext context) { - return null; } } -class TestClass : ControllerBase +class TestClass : Controller { [HttpDelete] public AcceptedAtActionResult CustomizedActionMethod (string actionName) @@ -104,43 +103,21 @@ public void TestMethod () filterCollection.Add(typeof(FilterClass)); } }", - GetCSharpResultAt(26, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpDelete")); + GetCSharpResultAt(25, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpDelete")); } [Fact] - public void Test_ChildrenOfControllerBase_ActionMethodWithHttpPostAttribute_Diagnostic() + public void Test_ChildrenOfController_ActionMethodWithHttpPostAndHttpGetAttributes_Diagnostic() { VerifyCSharpWithDependencies(@" using Microsoft.AspNetCore.Mvc; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } -class TestClass : ControllerBase -{ - [HttpPost] - public AcceptedAtActionResult CustomizedActionMethod (string actionName) - { - return null; - } -}", - GetCSharpResultAt(12, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpPost")); - } - - [Fact] - public void Test_ChildrenOfControllerBase_ActionMethodWithHttpPostAndHttpGetAttributes_Diagnostic() - { - VerifyCSharpWithDependencies(@" -using Microsoft.AspNetCore.Mvc; - -[MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase -{ -} - -class TestClass : ControllerBase +class TestClass : Controller { [HttpGet] [HttpPost] @@ -153,63 +130,18 @@ public AcceptedAtActionResult CustomizedActionMethod (string actionName) } [Fact] - public void Test_ChildrenOfControllerBase_ActionMethodWithHttpPutAttribute_Diagnostic() - { - VerifyCSharpWithDependencies(@" -using Microsoft.AspNetCore.Mvc; - -[MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase -{ -} - -class TestClass : ControllerBase -{ - [HttpPut] - public AcceptedAtActionResult CustomizedActionMethod (string actionName) - { - return null; - } -}", - GetCSharpResultAt(12, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpPut")); - } - - [Fact] - public void Test_ChildrenOfControllerBase_ActionMethodWithHttpDeleteAttribute_Diagnostic() - { - VerifyCSharpWithDependencies(@" -using System; -using Microsoft.AspNetCore.Mvc; - -[MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase -{ -} - -class TestClass : ControllerBase -{ - [HttpDelete] - public AcceptedAtActionResult CustomizedActionMethod (string actionName) - { - return null; - } -}", - GetCSharpResultAt(13, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpDelete")); - } - - [Fact] - public void Test_ChildrenOfControllerBase_ActionMethodWithHttpPatchAttribute_Diagnostic() + public void Test_ChildrenOfController_ActionMethodWithHttpPatchAttribute_Diagnostic() { VerifyCSharpWithDependencies(@" using System; using Microsoft.AspNetCore.Mvc; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } -class TestClass : ControllerBase +class TestClass : Controller { [HttpPatch] public AcceptedAtActionResult CustomizedActionMethod (string actionName) @@ -227,7 +159,7 @@ public void Test_ChildrenOfController_ActionMethodWithHttpPostAttribute_Diagnost using Microsoft.AspNetCore.Mvc; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } @@ -249,7 +181,7 @@ public void Test_ChildrenOfController_ActionMethodWithHttpPutAttribute_Diagnosti using Microsoft.AspNetCore.Mvc; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } @@ -271,7 +203,7 @@ public void Test_ChildrenOfController_ActionMethodWithHttpDeleteAttribute_Diagno using Microsoft.AspNetCore.Mvc; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } @@ -297,11 +229,11 @@ public void Test_WithoutValidateAntiForgeryAttribute_ActionMethodWithTwoHttpVerv using Microsoft.AspNetCore.Mvc.Filters; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } -class TestClass : ControllerBase +class TestClass : Controller { [HttpDelete] [HttpPost] @@ -323,6 +255,31 @@ public void Test_NoValidateAntiForgeryTokenAttribute_ActionMethodMissingHttpVerb using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; +[MyValidateAntiForgeryAttribute] +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller +{ +} + +class TestClass : Controller +{ + public AcceptedAtActionResult CustomizedActionMethod (string actionName) + { + return null; + } +}", + GetCSharpResultAt(15, 35, UseAutoValidateAntiforgeryToken.MissHttpVerbAttributeRule, "CustomizedActionMethod")); + } + + [Theory] + [InlineData("dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = false")] + public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_DefaultValue_Diagnostic(string editorConfigText) + { + VerifyCSharpAcrossTwoAssemblies( + ASPNetCoreApis.CSharp, + @" +using System; +using Microsoft.AspNetCore.Mvc; + [MyValidateAntiForgeryAttribute] class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase { @@ -330,12 +287,14 @@ class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase class TestClass : ControllerBase { + [HttpDelete] public AcceptedAtActionResult CustomizedActionMethod (string actionName) { return null; } }", - GetCSharpResultAt(15, 35, UseAutoValidateAntiforgeryToken.MissHttpVerbAttributeRule, "CustomizedActionMethod")); + GetEditorConfigAdditionalFile(editorConfigText), + GetCSharpResultAt(13, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpDelete")); } [Fact] @@ -1355,6 +1314,33 @@ public void TestMethod () }"); } + [Theory] + [InlineData("")] + [InlineData("dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = true")] + public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_NonDefaultValue_NoDiagnostic(string editorConfigText) + { + VerifyCSharpAcrossTwoAssemblies( + ASPNetCoreApis.CSharp, + @" +using System; +using Microsoft.AspNetCore.Mvc; + +[MyValidateAntiForgeryAttribute] +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +{ +} + +class TestClass : ControllerBase +{ + [HttpDelete] + public AcceptedAtActionResult CustomizedActionMethod (string actionName) + { + return null; + } +}", + GetEditorConfigAdditionalFile(editorConfigText)); + } + protected override DiagnosticAnalyzer GetBasicDiagnosticAnalyzer() { return new UseAutoValidateAntiforgeryToken(); diff --git a/src/Test.Utilities/MinimalImplementations/ASPNetCoreApis.cs b/src/Test.Utilities/MinimalImplementations/ASPNetCoreApis.cs index 033a174c2d..2f8947ae14 100644 --- a/src/Test.Utilities/MinimalImplementations/ASPNetCoreApis.cs +++ b/src/Test.Utilities/MinimalImplementations/ASPNetCoreApis.cs @@ -8,7 +8,7 @@ public static class ASPNetCoreApis using System; using System.Threading.Tasks; -class MyValidateAntiForgeryAttribute : Attribute +public class MyValidateAntiForgeryAttribute : Attribute { } @@ -115,7 +115,7 @@ public interface IAsyncAuthorizationFilter : IFilterMetadata public interface IAuthorizationFilter : IFilterMetadata { - Task OnAuthorization (AuthorizationFilterContext context); + void OnAuthorization (AuthorizationFilterContext context); } } diff --git a/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs b/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs index 55b62fe664..d94c9b9911 100644 --- a/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs +++ b/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs @@ -73,5 +73,10 @@ internal static partial class EditorConfigOptionNames /// Do not use the OR operator to represent the bitwise combination of its member values, use the integeral value directly. /// public const string UnsafeDllImportSearchPathBits = "unsafe_DllImportSearchPath_bits"; + + /// + /// Boolean option to configure if only look at Controller-derived classes when considering CSRF. + /// + public const string OnlyLookAtDerivedClassesOfController = "only_look_at_derived_classes_of_Controller"; } } From 7de05dd8441539e404f84c0ac90b8d1fd489e610 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Thu, 19 Sep 2019 11:15:50 +0800 Subject: [PATCH 2/8] Address review feedback. --- docs/Analyzer Configuration.md | 42 ++++++++++--------- .../UseAutoValidateAntiforgeryToken.cs | 19 +++++---- .../UseAutoValidateAntiforgeryTokenTests.cs | 4 +- .../Options/EditorConfigOptionNames.cs | 4 +- 4 files changed, 37 insertions(+), 32 deletions(-) diff --git a/docs/Analyzer Configuration.md b/docs/Analyzer Configuration.md index af33f7fa2a..34c694b179 100644 --- a/docs/Analyzer Configuration.md +++ b/docs/Analyzer Configuration.md @@ -155,7 +155,29 @@ Examples: |`dotnet_code_quality.excluded_type_names_with_derived_types = MyType1\|MyType2` | Matches all types named either 'MyType1' or 'MyType2' and all of their derived types in the compilation |`dotnet_code_quality.excluded_type_names_with_derived_types = M:NS.MyType` | Matches specific type 'MyType' with given fully qualified name and all of its derived types |`dotnet_code_quality.excluded_type_names_with_derived_types = M:NS1.MyType1\|M:NS2.MyType2` | Matches specific types 'MyType1' and 'MyType2' with respective fully qualified names and all of their derived types - + +### Configure unsafe DllImportSearchPath bits when using DefaultDllImportSearchPaths attribute +Option Name: `unsafe_DllImportSearchPath_bits` + +Configurable Rules: CA5393 + +Option Values: Integer values of System.Runtime.InteropServices.DllImportSearchPath + +Default Value: '770', which is AssemblyDirectory | UseDllDirectoryForDependencies | ApplicationDirectory + +Example: `dotnet_code_quality.CA5393.unsafe_DllImportSearchPath_bits = 770` + +### Configure if exclude aspnet core mvc ControllerBase when considering CSRF +Option Name: `exclude_aspnet_core_mvc_controller_base` + +Configurable Rules: CA5391 + +Option Values: Boolean values + +Default Value: `true` + +Example: `dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controller_base = false` + ### Dataflow analysis Configurable Rules: [CA1062](https://docs.microsoft.com/visualstudio/code-quality/ca1062-validate-arguments-of-public-methods), [CA1303](https://docs.microsoft.com/visualstudio/code-quality/ca1303-do-not-pass-literals-as-localized-parameters), [CA1508](../src/Microsoft.CodeQuality.Analyzers/Microsoft.CodeQuality.Analyzers.md#ca1508-avoid-dead-conditional-code), [CA2000](https://docs.microsoft.com/visualstudio/code-quality/ca2000-dispose-objects-before-losing-scope), [CA2100](https://docs.microsoft.com/visualstudio/code-quality/ca2100-review-sql-queries-for-security-vulnerabilities), [CA2213](https://docs.microsoft.com/visualstudio/code-quality/ca2213-disposable-fields-should-be-disposed), Taint analysis rules @@ -289,21 +311,3 @@ Option Values: integral values Default Value: Specific to each configurable rule ('100000' by default for most rules) Example: `dotnet_code_quality.CA5387.sufficient_IterationCount_for_weak_KDF_algorithm = 100000` - -#### Configure unsafe DllImportSearchPath bits when using DefaultDllImportSearchPaths attribute -Option Name: `unsafe_DllImportSearchPath_bits` - -Option Values: Integer values of System.Runtime.InteropServices.DllImportSearchPath - -Default Value: Specific to each configurable rule ('770', which is AssemblyDirectory | UseDllDirectoryForDependencies | ApplicationDirectory, by default for most rules) - -Example: `dotnet_code_quality.CA5392.unsafe_DllImportSearchPath_bits = 770` - -#### Configure if only look at Controller-derived classes when considering CSRF -Option Name: `only_look_at_derived_classes_of_Controller` - -Option Values: Boolean values - -Default Value: Specific to each configurable rule ('true' by default for most rules) - -Example: `dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = false` diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs index 7c87477696..57a6f95d32 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs @@ -93,7 +93,7 @@ public override void Initialize(AnalysisContext context) var cancellationToken = compilationStartAnalysisContext.CancellationToken; var onlyLookAtDerivedClassesOfController = compilationStartAnalysisContext.Options.GetBoolOptionValue( - optionName: EditorConfigOptionNames.OnlyLookAtDerivedClassesOfController, + optionName: EditorConfigOptionNames.ExcludeAspnetCoreMvcControllerBase, rule: UseAutoValidateAntiforgeryTokenRule, defaultValue: true, cancellationToken: cancellationToken); @@ -108,8 +108,8 @@ public override void Initialize(AnalysisContext context) // to avoid reporting false positives on projects that use an alternative approach to mitigate CSRF issues. var usingValidateAntiForgeryAttribute = false; var onAuthorizationAsyncMethodSymbols = new ConcurrentDictionary(); - var actionMethodSymbols = new HashSet<(IMethodSymbol, string)>(); - var actionMethodNeedAddingHttpVerbAttributeSymbols = new HashSet(); + var actionMethodSymbols = new ConcurrentDictionary<(IMethodSymbol, string), bool>(); + var actionMethodNeedAddingHttpVerbAttributeSymbols = new ConcurrentDictionary(); // Constructing inverse callGraph. // When it comes to delegate function assignment Del handler = DelegateMethod;, inverse call Graph will add: @@ -205,7 +205,7 @@ public override void Initialize(AnalysisContext context) s => (s.Name == "OnAuthorizationAsync" && s.ReturnType.Equals(taskTypeSymbol) || s.Name == "OnAuthorization" && - s.ReturnType.SpecialType == SpecialType.System_Void) && + s.ReturnsVoid) && s.Parameters.Length == 1 && s.Parameters[0].Type.Equals(authorizationFilterContextTypeSymbol)), placeholder); @@ -269,13 +269,14 @@ public override void Initialize(AnalysisContext context) if (httpVerbAttributeTypeSymbolAbleToModify != null) { var attributeName = httpVerbAttributeTypeSymbolAbleToModify.AttributeClass.Name; - actionMethodSymbols.Add( + actionMethodSymbols.TryAdd( (actionMethodSymbol, - attributeName.EndsWith("Attribute", StringComparison.Ordinal) ? attributeName.Remove(attributeName.Length - "Attribute".Length) : attributeName)); + attributeName.EndsWith("Attribute", StringComparison.Ordinal) ? attributeName.Remove(attributeName.Length - "Attribute".Length) : attributeName), + placeholder); } else if (!actionMethodSymbol.GetAttributes().Any(s => s.AttributeClass.GetBaseTypes().Contains(httpMethodAttributeTypeSymbol))) { - actionMethodNeedAddingHttpVerbAttributeSymbols.Add((actionMethodSymbol)); + actionMethodNeedAddingHttpVerbAttributeSymbols.TryAdd(actionMethodSymbol, placeholder); } } } @@ -310,7 +311,7 @@ public override void Initialize(AnalysisContext context) } } - foreach (var (methodSymbol, attributeName) in actionMethodSymbols) + foreach (var (methodSymbol, attributeName) in actionMethodSymbols.Keys) { compilationAnalysisContext.ReportDiagnostic( methodSymbol.CreateDiagnostic( @@ -319,7 +320,7 @@ public override void Initialize(AnalysisContext context) attributeName)); } - foreach (var methodSymbol in actionMethodNeedAddingHttpVerbAttributeSymbols) + foreach (var methodSymbol in actionMethodNeedAddingHttpVerbAttributeSymbols.Keys) { compilationAnalysisContext.ReportDiagnostic( methodSymbol.CreateDiagnostic( diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs index 22fb2b0232..53878f8f01 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs @@ -271,7 +271,7 @@ public AcceptedAtActionResult CustomizedActionMethod (string actionName) } [Theory] - [InlineData("dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = false")] + [InlineData("dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controller_base = false")] public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_DefaultValue_Diagnostic(string editorConfigText) { VerifyCSharpAcrossTwoAssemblies( @@ -1316,7 +1316,7 @@ public void TestMethod () [Theory] [InlineData("")] - [InlineData("dotnet_code_quality.CA5391.only_look_at_derived_classes_of_Controller = true")] + [InlineData("dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controller_base = true")] public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_NonDefaultValue_NoDiagnostic(string editorConfigText) { VerifyCSharpAcrossTwoAssemblies( diff --git a/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs b/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs index d94c9b9911..88d29fb4b7 100644 --- a/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs +++ b/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs @@ -75,8 +75,8 @@ internal static partial class EditorConfigOptionNames public const string UnsafeDllImportSearchPathBits = "unsafe_DllImportSearchPath_bits"; /// - /// Boolean option to configure if only look at Controller-derived classes when considering CSRF. + /// Boolean option to configure if exclude aspnet core mvc ControllerBase when considering CSRF. /// - public const string OnlyLookAtDerivedClassesOfController = "only_look_at_derived_classes_of_Controller"; + public const string ExcludeAspnetCoreMvcControllerBase = "exclude_aspnet_core_mvc_controller_base"; } } From ddb7bf38ed621de2d52038df2bf02d1b89d4458f Mon Sep 17 00:00:00 2001 From: Lingxia Chen <50311316+LLLXXXCCC@users.noreply.github.com> Date: Fri, 27 Sep 2019 11:43:29 +0800 Subject: [PATCH 3/8] Update docs/Analyzer Configuration.md Co-Authored-By: Genevieve Warren --- docs/Analyzer Configuration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/Analyzer Configuration.md b/docs/Analyzer Configuration.md index 34c694b179..bd6c6075fe 100644 --- a/docs/Analyzer Configuration.md +++ b/docs/Analyzer Configuration.md @@ -167,7 +167,7 @@ Default Value: '770', which is AssemblyDirectory | UseDllDirectoryForDependencie Example: `dotnet_code_quality.CA5393.unsafe_DllImportSearchPath_bits = 770` -### Configure if exclude aspnet core mvc ControllerBase when considering CSRF +### Exclude ASP.NET Core MVC ControllerBase when considering CSRF Option Name: `exclude_aspnet_core_mvc_controller_base` Configurable Rules: CA5391 From 411d52fb4d0d86851ec78b6b04abe23d8ef18ef6 Mon Sep 17 00:00:00 2001 From: Lingxia Chen <50311316+LLLXXXCCC@users.noreply.github.com> Date: Fri, 27 Sep 2019 11:43:40 +0800 Subject: [PATCH 4/8] Update docs/Analyzer Configuration.md Co-Authored-By: Genevieve Warren --- docs/Analyzer Configuration.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/Analyzer Configuration.md b/docs/Analyzer Configuration.md index bd6c6075fe..a23fef4810 100644 --- a/docs/Analyzer Configuration.md +++ b/docs/Analyzer Configuration.md @@ -156,7 +156,7 @@ Examples: |`dotnet_code_quality.excluded_type_names_with_derived_types = M:NS.MyType` | Matches specific type 'MyType' with given fully qualified name and all of its derived types |`dotnet_code_quality.excluded_type_names_with_derived_types = M:NS1.MyType1\|M:NS2.MyType2` | Matches specific types 'MyType1' and 'MyType2' with respective fully qualified names and all of their derived types -### Configure unsafe DllImportSearchPath bits when using DefaultDllImportSearchPaths attribute +### Unsafe DllImportSearchPath bits when using DefaultDllImportSearchPaths attribute Option Name: `unsafe_DllImportSearchPath_bits` Configurable Rules: CA5393 From 1331d0bd6aa0bd2b75a17da5a288bca31a0614aa Mon Sep 17 00:00:00 2001 From: Lingxia Chen <50311316+LLLXXXCCC@users.noreply.github.com> Date: Fri, 27 Sep 2019 11:43:53 +0800 Subject: [PATCH 5/8] Update src/Utilities/Compiler/Options/EditorConfigOptionNames.cs Co-Authored-By: Genevieve Warren --- src/Utilities/Compiler/Options/EditorConfigOptionNames.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs b/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs index 88d29fb4b7..eee0143f2f 100644 --- a/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs +++ b/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs @@ -75,7 +75,7 @@ internal static partial class EditorConfigOptionNames public const string UnsafeDllImportSearchPathBits = "unsafe_DllImportSearchPath_bits"; /// - /// Boolean option to configure if exclude aspnet core mvc ControllerBase when considering CSRF. + /// Boolean option to configure whether to exclude aspnet core mvc ControllerBase when considering CSRF. /// public const string ExcludeAspnetCoreMvcControllerBase = "exclude_aspnet_core_mvc_controller_base"; } From a949bfd1eca576850b2d1c603108ac34d9a806cf Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Fri, 27 Sep 2019 11:46:42 +0800 Subject: [PATCH 6/8] Add test for concurrency issue. --- .../UseAutoValidateAntiforgeryToken.cs | 2 +- .../UseAutoValidateAntiforgeryTokenTests.cs | 31 +++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs b/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs index 57a6f95d32..69a6c65b9a 100644 --- a/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs +++ b/src/Microsoft.NetCore.Analyzers/Core/Security/UseAutoValidateAntiforgeryToken.cs @@ -224,7 +224,7 @@ public override void Initialize(AnalysisContext context) var derivedControllerTypeSymbol = (INamedTypeSymbol)symbolAnalysisContext.Symbol; var baseTypes = derivedControllerTypeSymbol.GetBaseTypes(); - // An subtype of `Microsoft.AspNetCore.Mvc.Controller`, which indicates that cookie-based authentication is used and thus CSRF is a concern. + // An subtype of `Microsoft.AspNetCore.Mvc.Controller`, which probably indicates views are used and maybe cookie-based authentication is used and thus CSRF is a concern. if (baseTypes.Contains(controllerTypeSymbol) || (!onlyLookAtDerivedClassesOfController && baseTypes.Contains(controllerBaseTypeSymbol))) diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs index 53878f8f01..c233404e66 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs @@ -270,6 +270,37 @@ public AcceptedAtActionResult CustomizedActionMethod (string actionName) GetCSharpResultAt(15, 35, UseAutoValidateAntiforgeryToken.MissHttpVerbAttributeRule, "CustomizedActionMethod")); } + [Fact, WorkItem(2844, "https://github.com/dotnet/roslyn-analyzers/issues/2844")] + public void Test_ConcurrencyIssue_Diagnostic() + { + VerifyCSharpWithDependencies(@" +using Microsoft.AspNetCore.Mvc; + +[MyValidateAntiForgeryAttribute] +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +{ +} + +class TestClass : Controller +{ + [HttpPut] + public AcceptedAtActionResult CustomizedActionMethod (string actionName) + { + return null; + } +} + +class TestClass2 : Controller +{ + [HttpPut] + public AcceptedAtActionResult CustomizedActionMethod2 (string actionName) + { + return null; + } +}", + GetCSharpResultAt(12, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpPut")); + } + [Theory] [InlineData("dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controller_base = false")] public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_DefaultValue_Diagnostic(string editorConfigText) From 70f46f5d626f7b6335c32616bedab829d6d64e7a Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Fri, 27 Sep 2019 13:17:13 +0800 Subject: [PATCH 7/8] Update. --- .../Security/UseAutoValidateAntiforgeryTokenTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs index c233404e66..2a71609813 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs @@ -277,7 +277,7 @@ public void Test_ConcurrencyIssue_Diagnostic() using Microsoft.AspNetCore.Mvc; [MyValidateAntiForgeryAttribute] -class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : ControllerBase +class MakeSureValidateAntiForgeryAttributeIsUsedSomeWhereClass : Controller { } @@ -298,7 +298,8 @@ public AcceptedAtActionResult CustomizedActionMethod2 (string actionName) return null; } }", - GetCSharpResultAt(12, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpPut")); + GetCSharpResultAt(12, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpPut"), + GetCSharpResultAt(21, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod2", "HttpPut")); } [Theory] From 860ec1ceb6adb0ce94bded8a3ff06c0df03cfa54 Mon Sep 17 00:00:00 2001 From: LingxiaChen Date: Sun, 29 Sep 2019 10:09:11 +0800 Subject: [PATCH 8/8] Add more controllers for the concurrency test. --- docs/Analyzer Configuration.md | 4 +-- .../UseAutoValidateAntiforgeryTokenTests.cs | 36 +++++++++++++++++-- .../Options/EditorConfigOptionNames.cs | 2 +- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/docs/Analyzer Configuration.md b/docs/Analyzer Configuration.md index 36616ebee2..4338b6c4e2 100644 --- a/docs/Analyzer Configuration.md +++ b/docs/Analyzer Configuration.md @@ -226,7 +226,7 @@ Default Value: '770', which is AssemblyDirectory | UseDllDirectoryForDependencie Example: `dotnet_code_quality.CA5393.unsafe_DllImportSearchPath_bits = 770` ### Exclude ASP.NET Core MVC ControllerBase when considering CSRF -Option Name: `exclude_aspnet_core_mvc_controller_base` +Option Name: `exclude_aspnet_core_mvc_controllerbase` Configurable Rules: CA5391 @@ -234,7 +234,7 @@ Option Values: Boolean values Default Value: `true` -Example: `dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controller_base = false` +Example: `dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controllerbase = false` ### Disallowed symbol names Option Name: `disallowed_symbol_names` diff --git a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs index 2a71609813..33be2768be 100644 --- a/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs +++ b/src/Microsoft.NetCore.Analyzers/UnitTests/Security/UseAutoValidateAntiforgeryTokenTests.cs @@ -297,13 +297,43 @@ public AcceptedAtActionResult CustomizedActionMethod2 (string actionName) { return null; } +} + +class TestClass3 : Controller +{ + [HttpPut] + public AcceptedAtActionResult CustomizedActionMethod3 (string actionName) + { + return null; + } +} + +class TestClass4 : Controller +{ + [HttpPut] + public AcceptedAtActionResult CustomizedActionMethod4 (string actionName) + { + return null; + } +} + +class TestClass5 : Controller +{ + [HttpPut] + public AcceptedAtActionResult CustomizedActionMethod5 (string actionName) + { + return null; + } }", GetCSharpResultAt(12, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod", "HttpPut"), - GetCSharpResultAt(21, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod2", "HttpPut")); + GetCSharpResultAt(21, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod2", "HttpPut"), + GetCSharpResultAt(30, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod3", "HttpPut"), + GetCSharpResultAt(39, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod4", "HttpPut"), + GetCSharpResultAt(48, 35, UseAutoValidateAntiforgeryToken.UseAutoValidateAntiforgeryTokenRule, "CustomizedActionMethod5", "HttpPut")); } [Theory] - [InlineData("dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controller_base = false")] + [InlineData("dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controllerbase = false")] public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_DefaultValue_Diagnostic(string editorConfigText) { VerifyCSharpAcrossTwoAssemblies( @@ -1348,7 +1378,7 @@ public void TestMethod () [Theory] [InlineData("")] - [InlineData("dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controller_base = true")] + [InlineData("dotnet_code_quality.CA5391.exclude_aspnet_core_mvc_controllerbase = true")] public void EditorConfigConfiguration_OnlyLookAtDerivedClassesOfController_NonDefaultValue_NoDiagnostic(string editorConfigText) { VerifyCSharpAcrossTwoAssemblies( diff --git a/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs b/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs index 5ef550ac79..2c8188cab4 100644 --- a/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs +++ b/src/Utilities/Compiler/Options/EditorConfigOptionNames.cs @@ -108,6 +108,6 @@ internal static partial class EditorConfigOptionNames /// /// Boolean option to configure whether to exclude aspnet core mvc ControllerBase when considering CSRF. /// - public const string ExcludeAspnetCoreMvcControllerBase = "exclude_aspnet_core_mvc_controller_base"; + public const string ExcludeAspnetCoreMvcControllerBase = "exclude_aspnet_core_mvc_controllerbase"; } }