New Rule S6494: The value type properties of a model class should be nullable or marked as "Required" to avoid under-posting.

analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6964.cs

@@ -0,0 +1,17 @@
+using Microsoft.AspNetCore.Mvc;
+
+namespace IntentionalFindings
+{
+    public class S6964
+    {
+        public class Model
+        {
+            public int ValueProperty { get; set; }  // Noncompliant
+        }
+    }
+
+    public class ControllerClass : Controller
+    {
+        [HttpPost] public IActionResult Create(S6964.Model model) => View(model);
+    }
+}

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S1451-IntentionalFindings-net8.0.json

@@ -66,6 +66,12 @@
       "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6675.cs#L1",
       "Location": "Line 1 Position 1-1"
     },
+    {
+      "Id": "S1451",
+      "Message": "Add or update the header of this file.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6964.cs#L1",
+      "Location": "Line 1 Position 1-1"
+    },
     {
       "Id": "S1451",
       "Message": "Add or update the header of this file.",

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6964-IntentionalFindings-net8.0.json

@@ -0,0 +1,10 @@
+{
+  "Issues": [
+    {
+      "Id": "S6964",
+      "Message": "Property used as input in a controller action should be nullable or annotated with the Required attribute to avoid under-posting.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6964.cs#L9",
+      "Location": "Line 9 Position 13-51"
+    }
+  ]
+}

analyzers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6967-IntentionalFindings-net8.0.json

@@ -1,5 +1,11 @@
 {
   "Issues": [
+    {
+      "Id": "S6967",
+      "Message": "ModelState.IsValid should be checked in controller actions.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6964.cs#L15",
+      "Location": "Line 15 Position 41-47"
+    },
     {
       "Id": "S6967",
       "Message": "ModelState.IsValid should be checked in controller actions.",

analyzers/rspec/cs/S6964.html

@@ -0,0 +1,112 @@
+<p><a
+href="https://learn.microsoft.com/en-us/aspnet/web-api/overview/formats-and-model-binding/model-validation-in-aspnet-web-api#data-annotations">"Under-posting"</a>
+refers to a situation where a client sends less data than expected to the server during an HTTP request, for example when the client omits some
+properties from the request body that the server expects to receive.</p>
+<h2>Why is this an issue?</h2>
+<p>One of the main issues that under-posting can cause is data inconsistency. If the client sends less data than expected, the application might fill
+any value type properties with their default values, leading to inaccurate or inconsistent data in your database. Additionally, there might be
+unexpected behavior if there are certain data expected that are not provided and even security issues; for example, if a user omits a role or
+permission field from a POST request, and the server fills in a default value, it could inadvertently grant more access than intended.</p>
+<p>A <a href="https://learn.microsoft.com/en-us/aspnet/core/tutorials/first-mvc-app/adding-model">model class</a> (in this case the
+<code>Product</code> class) can be an input of an HTTP handler method:</p>
+<pre>
+public class ProductsController : Controller
+{
+    [HttpPost]
+    public IActionResult Create([FromBody]Product product)
+    {
+        // Process product data...
+    }
+}
+</pre>
+<h3>Exceptions</h3>
+<ul>
+  <li> This rule does not raise an issue for properties decorated with the <a
+  href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.modelbinding.validation.validateneverattribute">ValidateNever</a>
+  attribute. </li>
+</ul>
+<h2>How to fix it</h2>
+<p>You should mark any model value-type property as <a
+href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/nullable-value-types">nullable</a> or annotate it with the <a
+href="https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.requiredattribute">Required attribute</a>. Thus, when a
+client underposts, you ensure that the missing properties can be detected on the server side rather than being auto-filled, and therefore, incoming
+data meets the application’s expectations.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+public class Product
+{
+    public int Id { get; set; }
+    public string Name { get; set; }
+    public int NumberOfItems { get; set; }  // Noncompliant
+    public decimal Price { get; set; }      // Noncompliant
+}
+</pre>
+<p>If the client sends a request without setting the <code>NumberOfItems</code> or <code>Price</code> properties, they will default to <code>0</code>.
+In the request handler method there’s no way to determine whether they were intentionally set to <code>0</code> or omitted by mistake.</p>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+public class Product
+{
+    public int Id { get; set; }
+    public string Name { get; set; }
+    public int? NumberOfItems { get; set; }         // Compliant - property is optional
+    [Required] public decimal Price { get; set; }  // Compliant - property must have a value
+}
+</pre>
+<p>In this example the request handler method can</p>
+<ul>
+  <li> manually check whether <code>NumberOfItems</code> was filled out through the <code>HasValue</code> property </li>
+  <li> rely on Model Validation to make sure <code>Price</code> is not missing </li>
+</ul>
+<pre>
+public class ProductsController : Controller
+{
+    [HttpPost]
+    public IActionResult Create(Product product)
+    {
+        if (!ModelState.IsValid)    // if product.Price is missing then the model state will not be valid
+        {
+            return View(product);
+        }
+
+        if (product.NumberOfItems.HasValue)
+        {
+            // ...
+        }
+        // Process input...
+    }
+}
+</pre>
+<h2>Recommended Secure Coding Practices</h2>
+<ul>
+  <li> Client-Side Validation: While server-side validation is crucial, implementing client-side validation can provide immediate feedback to the user
+  when certain fields are not filled out, which helps to avoid under-posting. </li>
+  <li> Comprehensive Testing: Include testing scenarios that specifically check for under-posting vulnerabilities to ensure that all required data is
+  correctly validated and processed. </li>
+</ul>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/mvc/overview">Overview of ASP.NET Core MVC</a> </li>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/mvc/overview/getting-started/introduction/getting-started">Overview of
+  ASP.NET MVC 5</a> </li>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/razor-pages">Overview of ASP.NET Razor Pages</a> </li>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/tutorials/first-mvc-app/adding-model">Model Classes in ASP.NET MVC</a>
+  </li>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/aspnet/core/mvc/models/model-binding">Model Binding in ASP.NET Core</a> </li>
+  <li> Microsoft Learn - <a
+  href="https://learn.microsoft.com/en-us/aspnet/web-api/overview/formats-and-model-binding/model-validation-in-aspnet-web-api">Model Validation in
+  ASP.NET Web API</a> </li>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/nullable-value-types">Nullable
+  Value Types in .NET</a> </li>
+  <li> Microsoft Learn - <a
+  href="https://learn.microsoft.com/en-us/aspnet/web-api/overview/formats-and-model-binding/model-validation-in-aspnet-web-api#data-annotations">Data
+  Annotations</a> </li>
+  <li> Microsoft Learn - <a href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.routing.httpmethodattribute">RequiredAttribute
+  Class</a> </li>
+  <li> Microsoft Learn - <a
+  href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.modelbinding.validation.validateneverattribute">ValidateNeverAttribute
+  Class</a> </li>
+</ul>
+

analyzers/rspec/cs/S6964.json

@@ -0,0 +1,23 @@
+{
+  "title": "Property used as input in a controller action should be nullable or annotated with the Required attribute to avoid under-posting.",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "asp.net"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6964",
+  "sqKey": "S6964",
+  "scope": "Main",
+  "quickfix": "targeted",
+  "code": {
+    "impacts": {
+      "RELIABILITY": "HIGH"
+    },
+    "attribute": "TRUSTWORTHY"
+  }
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -318,6 +318,7 @@
     "S6934",
     "S6961",
     "S6962",
+    "S6964",
     "S6965",
     "S6966",
     "S6967",

analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/AvoidUnderPosting.cs

@@ -0,0 +1,144 @@
+/*
+ * 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 AvoidUnderPosting : SonarDiagnosticAnalyzer
+{
+    private const string DiagnosticId = "S6964";
+    private const string MessageFormat = "Property used as input in a controller action should be nullable or annotated with the Required attribute to avoid under-posting.";
+
+    private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);
+    private static readonly ImmutableArray<KnownType> IgnoredTypes = ImmutableArray.Create(
+        KnownType.Microsoft_AspNetCore_Http_IFormCollection,
+        KnownType.Microsoft_AspNetCore_Http_IFormFile,
+        KnownType.Microsoft_AspNetCore_Http_IFormFileCollection);
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+
+    protected override void Initialize(SonarAnalysisContext context) =>
+        context.RegisterCompilationStartAction(compilationStart =>
+        {
+            if (!compilationStart.Compilation.ReferencesControllers())
+            {
+                return;
+            }
+            var examinedTypes = new ConcurrentDictionary<ITypeSymbol, bool>();
+
+            compilationStart.RegisterSymbolStartAction(symbolStart =>
+            {
+                var type = (INamedTypeSymbol)symbolStart.Symbol;
+                if (type.IsControllerType())
+                {
+                    symbolStart.RegisterSyntaxNodeAction(nodeContext => ProcessControllerMethods(nodeContext, examinedTypes), SyntaxKind.MethodDeclaration);
+                }
+            }, SymbolKind.NamedType);
+        });
+
+    private static void ProcessControllerMethods(SonarSyntaxNodeReportingContext context, ConcurrentDictionary<ITypeSymbol, bool> examinedTypes)
+    {
+        if (context.SemanticModel.GetDeclaredSymbol(context.Node) is IMethodSymbol method
+            && method.IsControllerActionMethod())
+        {
+            var modelParameterTypes = method.Parameters
+                .Where(x => !HasValidateNeverAttribute(x))
+                .Select(x => x.Type)
+                .OfType<INamedTypeSymbol>()
+                .SelectMany(RelatedTypesToExamine)
+                .Distinct();
+            foreach (var modelParameterType in modelParameterTypes)
+            {
+                CheckInvalidProperties(modelParameterType, context, examinedTypes);
+            }
+        }
+    }
+
+    private static void CheckInvalidProperties(INamedTypeSymbol parameterType, SonarSyntaxNodeReportingContext context, ConcurrentDictionary<ITypeSymbol, bool> examinedTypes)
+    {
+        var declaredProperties = new List<IPropertySymbol>();
+        GetAllDeclaredProperties(parameterType, examinedTypes, declaredProperties);
+        var invalidProperties = declaredProperties.Where(x => !CanBeNull(x.Type)
+            && !x.HasAttribute(KnownType.System_ComponentModel_DataAnnotations_RequiredAttribute));
+        foreach (var property in invalidProperties)
+        {
+            var propertySyntax = property.DeclaringSyntaxReferences[0].GetSyntax();
+            context.ReportIssue(Rule, propertySyntax.GetLocation());
+        }
+    }
+
+    private static bool IgnoreType(ITypeSymbol type) =>
+        type is not INamedTypeSymbol namedType      // e.g. dynamic type
+        || namedType.IsAny(IgnoredTypes)
+        || !CanBeUsedInModelBinding(namedType);
+
+    private static bool CanBeUsedInModelBinding(INamedTypeSymbol type) =>
+        !type.IsTupleType()                                             // Tuples are not supported (unless a custom Model Binder is used)
+        && (type.Constructors.Any(x => x.Parameters.Length == 0)        // The type must have a parameterless constructor, unless
+            || type.IsValueType                                         // - it's a value type
+            || type.IsRecord()                                          // - it's a record type
+            || type.IsInterface()                                       // - it's an interface (although the type that implements will be actually used)
+            || type.Is(KnownType.System_String));                       // - it has a custom Model Binder (e.g. System.String has one)
+
+    private static bool CanBeNull(ITypeSymbol type) =>
+        type is ITypeParameterSymbol { HasValueTypeConstraint: false }
+        || (type.IsReferenceType && type.NullableAnnotation() != NullableAnnotation.NotAnnotated)
+        || type.IsNullableValueType();
+
+    private static void GetAllDeclaredProperties(ITypeSymbol type, ConcurrentDictionary<ITypeSymbol, bool> examinedTypes, List<IPropertySymbol> declaredProperties)
+    {
+        if (type is INamedTypeSymbol namedType
+            && examinedTypes.TryAdd(namedType, true)
+            && !IgnoreType(namedType)
+            && !HasValidateNeverAttribute(type))
+        {
+            var properties = namedType.GetMembers()
+                .OfType<IPropertySymbol>()
+                .Where(x => x.GetEffectiveAccessibility() == Accessibility.Public
+                    && x.SetMethod?.DeclaredAccessibility is Accessibility.Public
+                    && !HasValidateNeverAttribute(x)
+                    && x.DeclaringSyntaxReferences.Length > 0
+                    && !IgnoreType(x.Type));
+            foreach (var property in properties)
+            {
+                declaredProperties.Add(property);
+                if (property.Type.DeclaringSyntaxReferences.Length > 0)
+                {
+                    GetAllDeclaredProperties(property.Type, examinedTypes, declaredProperties);
+                }
+            }
+            ITypeSymbol[] relatedTypes = [namedType.BaseType, .. namedType.TypeArguments];
+            foreach (var relatedType in relatedTypes)
+            {
+                GetAllDeclaredProperties(relatedType, examinedTypes, declaredProperties);
+            }
+        }
+    }
+
+    private static IEnumerable<INamedTypeSymbol> RelatedTypesToExamine(INamedTypeSymbol type) =>
+        type.DerivesOrImplements(KnownType.System_Collections_Generic_IEnumerable_T)
+            ? type.TypeArguments.OfType<INamedTypeSymbol>()
+            : [type];
+
+    private static bool HasValidateNeverAttribute(ISymbol symbol) =>
+        symbol.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_ModelBinding_Validation_ValidateNeverAttribute);
+}