Copy rule implementation

analyzers/rspec/cs/S6932.json

@@ -0,0 +1,25 @@
+{
+  "title": "Use model binding instead of reading raw request data",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "asp.net"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6932",
+  "sqKey": "S6932",
+  "scope": "Main",
+  "quickfix": "infeasible",
+  "code": {
+    "impacts": {
+      "MAINTAINABILITY": "HIGH",
+      "RELIABILITY": "MEDIUM",
+      "SECURITY": "MEDIUM"
+    },
+    "attribute": "FOCUSED"
+  }
+}

analyzers/src/SonarAnalyzer.CSharp/Rules/UseAspNetModelBinding.cs

@@ -0,0 +1,261 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2024 SonarSource SA
+ * mailto: contact AT sonarsource DOT com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+using System.Collections.Concurrent;
+
+namespace SonarAnalyzer.Rules.CSharp;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class UseAspNetModelBinding : SonarDiagnosticAnalyzer<SyntaxKind>
+{
+    private const string DiagnosticId = "S6932";
+    private const string UseAspNetModelBindingMessage = "Use model binding instead of accessing the raw request data";
+    private const string UseIFormFileBindingMessage = "Use IFormFile or IFormFileCollection binding instead";
+
+    protected override string MessageFormat => "{0}";
+
+    protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;
+
+    public UseAspNetModelBinding() : base(DiagnosticId) { }
+
+    protected override void Initialize(SonarAnalysisContext context)
+    {
+        context.RegisterCompilationStartAction(compilationStartContext =>
+        {
+            var (argumentDescriptors, propertyAccessDescriptors) = GetDescriptors(compilationStartContext.Compilation);
+            if (argumentDescriptors.Any() || propertyAccessDescriptors.Any())
+            {
+                compilationStartContext.RegisterSymbolStartAction(symbolStart =>
+                {
+                    // If the user overrides any action filters, model binding may not be working as expected.
+                    // Then we do not want to raise on expressions that originate from parameters.
+                    // See the OverridesController.Undecidable test cases for details.
+                    var hasActionFiltersOverrides = false;
+                    var candidates = new ConcurrentStack<ReportCandidate>(); // In SymbolEnd, we filter the candidates based on the overriding we learn on the go.
+                    if (symbolStart.Symbol is INamedTypeSymbol namedType && namedType.IsControllerType())
+                    {
+                        symbolStart.RegisterCodeBlockStartAction<SyntaxKind>(codeBlockStart =>
+                            {
+                                if (IsOverridingFilterMethods(codeBlockStart.OwningSymbol))
+                                {
+                                    // We do not want to raise in ActionFilter overrides and so we do not register.
+                                    // The SymbolEndAction needs to be made aware, that there are
+                                    // ActionFilter overrides, so it can filter out some candidates.
+                                    hasActionFiltersOverrides = true;
+                                }
+                                else
+                                {
+                                    RegisterCodeBlockActions(codeBlockStart, argumentDescriptors, propertyAccessDescriptors, candidates);
+                                }
+                            });
+                    }
+                    symbolStart.RegisterSymbolEndAction(symbolEnd =>
+                    {
+                        foreach (var candidate in candidates.Where(x => !(hasActionFiltersOverrides && x.OriginatesFromParameter)))
+                        {
+                            symbolEnd.ReportIssue(Diagnostic.Create(Rule, candidate.Location, candidate.Message));
+                        }
+                    });
+                }, SymbolKind.NamedType);
+            }
+        });
+    }
+
+    private void RegisterCodeBlockActions(SonarCodeBlockStartAnalysisContext<SyntaxKind> codeBlockStart,
+        ArgumentDescriptor[] argumentDescriptors, MemberDescriptor[] propertyAccessDescriptors,
+        ConcurrentStack<ReportCandidate> controllerCandidates)
+    {
+        // Within a single code block, access via constant and variable keys could be mixed.
+        // We only want to raise, if all access were done via constants.
+        var allConstantAccesses = true;
+        var codeBlockCandidates = new ConcurrentStack<ReportCandidate>();
+        if (argumentDescriptors.Any())
+        {
+            codeBlockStart.RegisterNodeAction(nodeContext =>
+            {
+                var argument = (ArgumentSyntax)nodeContext.Node;
+                var context = new ArgumentContext(argument, nodeContext.SemanticModel);
+                if (allConstantAccesses && Array.Exists(argumentDescriptors, x => Language.Tracker.Argument.MatchArgument(x)(context)))
+                {
+                    allConstantAccesses &= nodeContext.SemanticModel.GetConstantValue(argument.Expression) is { HasValue: true, Value: string };
+                    codeBlockCandidates.Push(new(UseAspNetModelBindingMessage, GetPrimaryLocation(argument), IsOriginatingFromParameter(nodeContext.SemanticModel, argument)));
+                }
+            }, SyntaxKind.Argument);
+        }
+        if (propertyAccessDescriptors.Any())
+        {
+            codeBlockStart.RegisterNodeAction(nodeContext =>
+            {
+                // The property access of Request.Form.Files can be replaced by an IFormFile binding.
+                // Any access to a "Files" property is therefore noncompliant. This is different from the Argument handling above.
+                var memberAccess = (MemberAccessExpressionSyntax)nodeContext.Node;
+                var context = new PropertyAccessContext(memberAccess, nodeContext.SemanticModel, memberAccess.Name.Identifier.ValueText);
+                if (Language.Tracker.PropertyAccess.MatchProperty(propertyAccessDescriptors)(context))
+                {
+                    codeBlockCandidates.Push(new(UseIFormFileBindingMessage, memberAccess.GetLocation(), IsOriginatingFromParameter(nodeContext.SemanticModel, memberAccess)));
+                }
+            }, SyntaxKind.SimpleMemberAccessExpression);
+        }
+        codeBlockStart.RegisterCodeBlockEndAction(codeBlockEnd =>
+        {
+            if (allConstantAccesses)
+            {
+                controllerCandidates.PushRange([.. codeBlockCandidates]);
+            }
+        });
+    }
+
+    private static (ArgumentDescriptor[] ArgumentDescriptors, MemberDescriptor[] PropertyAccessDescriptors) GetDescriptors(Compilation compilation)
+    {
+        var argumentDescriptors = new List<ArgumentDescriptor>();
+        var propertyAccessDescriptors = new List<MemberDescriptor>();
+        if (compilation.GetTypeByMetadataName(KnownType.Microsoft_AspNetCore_Mvc_ControllerAttribute) is { })
+        {
+            AddAspNetCoreDescriptors(argumentDescriptors, propertyAccessDescriptors);
+        }
+        // TODO: Add descriptors for Asp.Net MVC 4.x
+        return ([.. argumentDescriptors], [.. propertyAccessDescriptors]);
+    }
+
+    private static void AddAspNetCoreDescriptors(List<ArgumentDescriptor> argumentDescriptors, List<MemberDescriptor> propertyAccessDescriptors)
+    {
+        argumentDescriptors.AddRange([
+            ArgumentDescriptor.ElementAccess(// Request.Form["id"]
+                invokedIndexerContainer: KnownType.Microsoft_AspNetCore_Http_IFormCollection,
+                invokedIndexerExpression: "Form",
+                parameterConstraint: _ => true, // There is only a single overload and it is getter only
+                argumentPosition: 0),
+            ArgumentDescriptor.MethodInvocation(// Request.Form.TryGetValue("id", out _)
+                invokedType: KnownType.Microsoft_AspNetCore_Http_IFormCollection,
+                methodName: "TryGetValue",
+                parameterName: "key",
+                argumentPosition: 0),
+            ArgumentDescriptor.MethodInvocation(// Request.Form.ContainsKey("id")
+                invokedType: KnownType.Microsoft_AspNetCore_Http_IFormCollection,
+                methodName: "ContainsKey",
+                parameterName: "key",
+                argumentPosition: 0),
+            ArgumentDescriptor.ElementAccess(// Request.Headers["id"]
+                invokedIndexerContainer: KnownType.Microsoft_AspNetCore_Http_IHeaderDictionary,
+                invokedIndexerExpression: "Headers",
+                parameterConstraint: IsGetterParameter, // Headers are read/write
+                argumentPosition: 0),
+            ArgumentDescriptor.MethodInvocation(// Request.Headers.TryGetValue("id", out _)
+                invokedMethodSymbol: x => IsIDictionaryStringStringValuesInvocation(x, "TryGetValue"), // TryGetValue is from IDictionary<TKey, TValue> here. We check the type arguments.
+                invokedMemberNameConstraint: (name, comparison) => string.Equals(name, "TryGetValue", comparison),
+                invokedMemberNodeConstraint: IsAccessedViaHeaderDictionary,
+                parameterConstraint: x => string.Equals(x.Name, "key", StringComparison.Ordinal),
+                argumentListConstraint: (list, position) => list.Count == 2 && position is 0 or null,
+                refKind: RefKind.None),
+            ArgumentDescriptor.MethodInvocation(// Request.Headers.ContainsKey("id")
+                invokedMethodSymbol: x => IsIDictionaryStringStringValuesInvocation(x, "ContainsKey"),
+                invokedMemberNameConstraint: (name, comparison) => string.Equals(name, "ContainsKey", comparison),
+                invokedMemberNodeConstraint: IsAccessedViaHeaderDictionary,
+                parameterConstraint: x => string.Equals(x.Name, "key", StringComparison.Ordinal),
+                argumentListConstraint: (list, _) => list.Count == 1,
+                refKind: RefKind.None),
+            ArgumentDescriptor.ElementAccess(// Request.Query["id"]
+                invokedIndexerContainer: KnownType.Microsoft_AspNetCore_Http_IQueryCollection,
+                invokedIndexerExpression: "Query",
+                parameterConstraint: _ => true, // There is only a single overload and it is getter only
+                argumentPosition: 0),
+            ArgumentDescriptor.MethodInvocation(// Request.Query.TryGetValue("id", out _)
+                invokedType: KnownType.Microsoft_AspNetCore_Http_IQueryCollection,
+                methodName: "TryGetValue",
+                parameterName: "key",
+                argumentPosition: 0),
+            ArgumentDescriptor.ElementAccess(// Request.RouteValues["id"]
+                invokedIndexerContainer: KnownType.Microsoft_AspNetCore_Routing_RouteValueDictionary,
+                invokedIndexerExpression: "RouteValues",
+                parameterConstraint: IsGetterParameter, // RouteValues are read/write
+                argumentPosition: 0),
+            ArgumentDescriptor.MethodInvocation(// Request.RouteValues.TryGetValue("id", out _)
+                invokedType: KnownType.Microsoft_AspNetCore_Routing_RouteValueDictionary,
+                methodName: "TryGetValue",
+                parameterName: "key",
+                argumentPosition: 0)]);
+
+        propertyAccessDescriptors.Add(new(KnownType.Microsoft_AspNetCore_Http_IFormCollection, "Files")); // Request.Form.Files...
+    }
+
+    // Check that the "Headers" expression in the Headers.TryGetValue("id", out _) invocation is of type IHeaderDictionary
+    private static bool IsAccessedViaHeaderDictionary(SemanticModel model, ILanguageFacade language, SyntaxNode invocation) =>
+        invocation is InvocationExpressionSyntax { Expression: { } expression }
+            && GetLeftOfDot(expression) is { } left
+            && model.GetTypeInfo(left) is { Type: { } typeSymbol } && typeSymbol.Is(KnownType.Microsoft_AspNetCore_Http_IHeaderDictionary);
+
+    private static bool IsOverridingFilterMethods(ISymbol owningSymbol) =>
+        (owningSymbol.GetOverriddenMember() ?? owningSymbol).ExplicitOrImplicitInterfaceImplementations().Any(x => x is IMethodSymbol { ContainingType: { } container }
+            && container.IsAny(
+                KnownType.Microsoft_AspNetCore_Mvc_Filters_IActionFilter,
+                KnownType.Microsoft_AspNetCore_Mvc_Filters_IAsyncActionFilter));
+
+    private static bool IsOriginatingFromParameter(SemanticModel semanticModel, ArgumentSyntax argument) =>
+        GetExpressionOfArgumentParent(argument) is { } parentExpression && IsOriginatingFromParameter(semanticModel, parentExpression);
+
+    private static bool IsOriginatingFromParameter(SemanticModel semanticModel, ExpressionSyntax expression) =>
+        MostLeftOfDottedChain(expression) is { } mostLeft && semanticModel.GetSymbolInfo(mostLeft).Symbol is IParameterSymbol;
+
+    private static ExpressionSyntax GetLeftOfDot(ExpressionSyntax expression) =>
+        expression switch
+        {
+            MemberAccessExpressionSyntax memberAccessExpression => memberAccessExpression.Expression,
+            MemberBindingExpressionSyntax memberBindingExpression => memberBindingExpression.GetParentConditionalAccessExpression()?.Expression,
+            _ => null,
+        };
+
+    private static ExpressionSyntax MostLeftOfDottedChain(ExpressionSyntax root)
+    {
+        var current = root.GetRootConditionalAccessExpression()?.Expression ?? root;
+        while (current.Kind() is SyntaxKind.SimpleMemberAccessExpression or SyntaxKind.ElementAccessExpression)
+        {
+            current = current switch
+            {
+                MemberAccessExpressionSyntax { Expression: { } left } => left,
+                ElementAccessExpressionSyntax { Expression: { } left } => left,
+                _ => throw new InvalidOperationException("Unreachable"),
+            };
+        }
+        return current;
+    }
+
+    private static ExpressionSyntax GetExpressionOfArgumentParent(ArgumentSyntax argument) =>
+        argument switch
+        {
+            { Parent: BracketedArgumentListSyntax { Parent: ElementBindingExpressionSyntax expression } } => expression.GetParentConditionalAccessExpression(),
+            { Parent: BracketedArgumentListSyntax { Parent: ElementAccessExpressionSyntax { Expression: { } expression } } } => expression,
+            { Parent: ArgumentListSyntax { Parent: InvocationExpressionSyntax { Expression: { } expression } } } => expression,
+            _ => null,
+        };
+
+    private static Location GetPrimaryLocation(ArgumentSyntax argument) =>
+        ((SyntaxNode)GetExpressionOfArgumentParent(argument) ?? argument).GetLocation();
+
+    private static bool IsGetterParameter(IParameterSymbol parameter) =>
+        parameter.ContainingSymbol is IMethodSymbol { MethodKind: MethodKind.PropertyGet };
+
+    private static bool IsIDictionaryStringStringValuesInvocation(IMethodSymbol method, string name) =>
+        method.Is(KnownType.System_Collections_Generic_IDictionary_TKey_TValue, name)
+            && method.ContainingType.TypeArguments is { Length: 2 } typeArguments
+            && typeArguments[0].Is(KnownType.System_String)
+            && typeArguments[1].Is(KnownType.Microsoft_Extensions_Primitives_StringValues);
+
+    private readonly record struct ReportCandidate(string Message, Location Location, bool OriginatesFromParameter);
+}

analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs

@@ -69,6 +69,7 @@ public sealed partial class KnownType
         public static readonly KnownType Microsoft_AspNetCore_Http_IFormFile = new("Microsoft.AspNetCore.Http.IFormFile");
         public static readonly KnownType Microsoft_AspNetCore_Http_IFormFileCollection = new("Microsoft.AspNetCore.Http.IFormFileCollection");
         public static readonly KnownType Microsoft_AspNetCore_Http_IHeaderDictionary = new("Microsoft.AspNetCore.Http.IHeaderDictionary");
+        public static readonly KnownType Microsoft_AspNetCore_Http_IQueryCollection = new("Microsoft.AspNetCore.Http.IQueryCollection");
         public static readonly KnownType Microsoft_AspNetCore_Http_IRequestCookieCollection = new("Microsoft.AspNetCore.Http.IRequestCookieCollection");
         public static readonly KnownType Microsoft_AspNetCore_Http_IResponseCookies = new("Microsoft.AspNetCore.Http.IResponseCookies");
         public static readonly KnownType Microsoft_AspNetCore_Http_IResult = new("Microsoft.AspNetCore.Http.IResult");
@@ -83,6 +84,8 @@ public sealed partial class KnownType
         public static readonly KnownType Microsoft_AspNetCore_Mvc_ControllerAttribute = new("Microsoft.AspNetCore.Mvc.ControllerAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_DisableRequestSizeLimitAttribute = new("Microsoft.AspNetCore.Mvc.DisableRequestSizeLimitAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_Filters_ActionFilterAttribute = new("Microsoft.AspNetCore.Mvc.Filters.ActionFilterAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_Filters_IActionFilter = new("Microsoft.AspNetCore.Mvc.Filters.IActionFilter");
+        public static readonly KnownType Microsoft_AspNetCore_Mvc_Filters_IAsyncActionFilter = new("Microsoft.AspNetCore.Mvc.Filters.IAsyncActionFilter");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_FromServicesAttribute = new("Microsoft.AspNetCore.Mvc.FromServicesAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpDeleteAttribute = new("Microsoft.AspNetCore.Mvc.HttpDeleteAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpGetAttribute = new("Microsoft.AspNetCore.Mvc.HttpGetAttribute");
@@ -109,6 +112,7 @@ public sealed partial class KnownType
         public static readonly KnownType Microsoft_AspNetCore_Mvc_RouteAttribute = new("Microsoft.AspNetCore.Mvc.RouteAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Mvc_Routing_HttpMethodAttribute = new("Microsoft.AspNetCore.Mvc.Routing.HttpMethodAttribute");
         public static readonly KnownType Microsoft_AspNetCore_Razor_Hosting_RazorCompiledItemAttribute = new("Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemAttribute");
+        public static readonly KnownType Microsoft_AspNetCore_Routing_RouteValueDictionary = new("Microsoft.AspNetCore.Routing.RouteValueDictionary");
         public static readonly KnownType Microsoft_Azure_Cosmos_CosmosClient = new("Microsoft.Azure.Cosmos.CosmosClient");
         public static readonly KnownType Microsoft_Azure_Documents_Client_DocumentClient = new("Microsoft.Azure.Documents.Client.DocumentClient");
         public static readonly KnownType Microsoft_Azure_ServiceBus_Management_ManagementClient = new("Microsoft.Azure.ServiceBus.Management.ManagementClient");

analyzers/src/SonarAnalyzer.Common/Trackers/PropertyAccessContext.cs

@@ -30,5 +30,11 @@ public PropertyAccessContext(SonarSyntaxNodeReportingContext context, string pro
             PropertyName = propertyName;
             PropertySymbol = new Lazy<IPropertySymbol>(() => context.SemanticModel.GetSymbolInfo(context.Node).Symbol as IPropertySymbol);
         }
+
+        public PropertyAccessContext(SyntaxNode node, SemanticModel semanticModel, string propertyName) : base(node, semanticModel)
+        {
+            PropertyName = propertyName;
+            PropertySymbol = new Lazy<IPropertySymbol>(() => semanticModel.GetSymbolInfo(node).Symbol as IPropertySymbol);
+        }
     }
 }