Fix S6964 FN: Rule should raise in case of value type property annotated with RequiredAttribute

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

@@ -2,9 +2,15 @@
   "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.",
+      "Message": "Value type property used as input in a controller action should be nullable, required or annotated with the JsonRequiredAttribute 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 24-37"
+    },
+    {
+      "Id": "S6964",
+      "Message": "Value type property used as input in a controller action should be nullable, required or annotated with the JsonRequiredAttribute to avoid under-posting.",
+      "Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6967.cs#L12",
+      "Location": "Line 12 Position 35-37"
     }
   ]
 }

analyzers/rspec/cs/S6964.html

@@ -20,41 +20,45 @@ <h2>Why is this an issue?</h2>
 }
 </pre>
 <h3>Exceptions</h3>
+<p>This rule does not raise an issue when:</p>
 <ul>
-  <li> This rule does not raise an issue for properties decorated with the <a
+  <li> properties are decorated with the <a
   href="https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.modelbinding.validation.validateneverattribute">ValidateNever</a>
   attribute. </li>
+  <li> properties are in model classes which are not in the same project as the Controller class that references them. This is due to a limitation of
+  Roslyn (see <a href="https://github.com/SonarSource/sonar-dotnet/issues/9243">here</a>). </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>
+href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/nullable-value-types">nullable</a>, <a
+href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/required">required</a> or <a
+href="https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonrequiredattribute">JsonRequired</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 int Id { get; set; }             // Noncompliant
     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>
+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 required 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
+    public int? NumberOfItems { get; set; }            // Compliant - property is optional
+    [JsonRequired] public decimal Price { get; set; }  // Compliant - property must have a value
 }
 </pre>
-<p>In this example the request handler method can</p>
+<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>

analyzers/rspec/cs/S6964.json

@@ -1,5 +1,5 @@
 {
-  "title": "Property used as input in a controller action should be nullable or annotated with the Required attribute to avoid under-posting.",
+  "title": "Value type property used as input in a controller action should be nullable, required or annotated with the JsonRequiredAttribute to avoid under-posting.",
   "type": "CODE_SMELL",
   "status": "ready",
   "remediation": {

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

@@ -26,16 +26,13 @@ namespace SonarAnalyzer.Rules.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 const string MessageFormat = "Value type property used as input in a controller action should be nullable, required or annotated with the JsonRequiredAttribute 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);
-    private static readonly ImmutableArray<KnownType> ValidationAttributes = ImmutableArray.Create(
-        KnownType.System_ComponentModel_DataAnnotations_RequiredAttribute,
-        KnownType.System_Text_Json_Serialization_JsonRequiredAttribute);
 
     public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
 
@@ -79,7 +76,7 @@ private static void CheckInvalidProperties(INamedTypeSymbol parameterType, Sonar
         var declaredProperties = new List<IPropertySymbol>();
         GetAllDeclaredProperties(parameterType, examinedTypes, declaredProperties);
         var invalidProperties = declaredProperties
-            .Where(x => !CanBeNull(x.Type) && !x.HasAnyAttribute(ValidationAttributes) && !x.IsRequired());
+            .Where(x => !CanBeNull(x.Type) && !x.HasAttribute(KnownType.System_Text_Json_Serialization_JsonRequiredAttribute) && !x.IsRequired());
         foreach (var property in invalidProperties)
         {
             context.ReportIssue(Rule, property.GetFirstSyntaxRef().GetIdentifier()?.GetLocation());

analyzers/src/SonarAnalyzer.Common/Extensions/ISymbolExtensions.cs

@@ -27,9 +27,6 @@ public static class ISymbolExtensions
     public static bool HasAttribute(this ISymbol symbol, KnownType type) =>
         symbol.GetAttributes(type).Any();
 
-    public static bool HasAnyAttribute(this ISymbol symbol, ImmutableArray<KnownType> types) =>
-        symbol.GetAttributes(types).Any();
-
     public static SyntaxNode GetFirstSyntaxRef(this ISymbol symbol) =>
         symbol?.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax();
 

analyzers/tests/SonarAnalyzer.Test/Rules/AspNet/AvoidUnderPostingTest.cs

@@ -35,7 +35,7 @@ public class AvoidUnderPostingTest
 
     private readonly VerifierBuilder builder = new VerifierBuilder<AvoidUnderPosting>()
             .WithBasePath("AspNet")
-            .AddReferences(AspNetReferences);
+            .AddReferences([..AspNetReferences, .. NuGetMetadataReference.SystemTextJson("7.0.4")]);
 
     [TestMethod]
     public void AvoidUnderPosting_CSharp() =>
@@ -56,7 +56,6 @@ public void AvoidUnderPosting_CSharp9() =>
     [TestMethod]
     public void AvoidUnderPosting_CSharp12() =>
         builder.AddPaths("AvoidUnderPosting.CSharp12.cs")
-            .AddReferences(NuGetMetadataReference.SystemTextJson("7.0.4"))
             .WithOptions(ParseOptionsHelper.FromCSharp12)
             .Verify();
 
@@ -74,15 +73,15 @@ public void AvoidUnderPosting_EnclosingTypes_CSharp(string enclosingType) =>
             {
                 public int ValueProperty { get; set; }                      // Noncompliant
                 public int? NullableValueProperty { get; set; }             // Compliant
-                [Required] public int RequiredValueProperty { get; set; }   // Compliant
+                public required int RequiredValueProperty { get; set; }     // Compliant
             }
 
             public class ControllerClass : Controller
             {
                 [HttpPost] public IActionResult Create(Model model) => View(model);
             }
             """)
-        .WithOptions(ParseOptionsHelper.FromCSharp10)
+        .WithOptions(ParseOptionsHelper.FromCSharp11)
         .Verify();
 
     [DataTestMethod]

analyzers/tests/SonarAnalyzer.Test/TestCases/AspNet/AvoidUnderPosting.CSharp8.cs

@@ -1,13 +1,14 @@
 using Microsoft.AspNetCore.Mvc;
 using System;
 using System.ComponentModel.DataAnnotations;
+using System.Text.Json.Serialization;
 
 namespace NullableReferences
 {
     public class ModelUsedInController
     {
 #nullable enable
-        public string NonNullableReferenceProperty { get; set; }  // Compliant - ASP.NET Core treats non-nullable reference types as if they were decorated with the [Required] attribute
+        public string NonNullableReferenceProperty { get; set; }  // Compliant - the JSON serializer will throw an exception if the value is missing from the request
         public object AnotherNonNullableReferenceProperty { get; set; }
         [Required] public string RequiredNonNullableReferenceProperty { get; set; }
         public string? NullableReferenceProperty { get; set; }
@@ -39,7 +40,7 @@ public class GenericType<TNoContstraint, TClass, TStruct, TNotNull>
         public TNoContstraint NoConstraintProperty { get; set; }
         public TClass ClassProperty { get; set; }
         public TStruct StructProperty { get; set; }                             // Noncompliant
-        [Required] public TStruct RequiredStructProperty { get; set; }
+        [JsonRequired] public TStruct RequiredStructProperty { get; set; }
         public TStruct? NullableStructProperty { get; set; }
         public TNotNull NotNullProperty { get; set; }                           // Noncompliant
     }

analyzers/tests/SonarAnalyzer.Test/TestCases/AspNet/AvoidUnderPosting.CSharp9.cs

@@ -1,12 +1,13 @@
 using Microsoft.AspNetCore.Mvc;
 using System.ComponentModel.DataAnnotations;
+using System.Text.Json.Serialization;
 
 namespace CSharp9
 {
     // https://learn.microsoft.com/en-us/aspnet/core/mvc/models/model-binding#constructor-binding-and-record-types
     public record RecordModel(
         int ValueProperty,                                                          // Noncompliant
-        [property: Required] int RequiredValueProperty,                             // without the property prefix the attribute would have been applied to the constructor parameter instead
+        [property: JsonRequired] int RequiredValueProperty,                             // without the property prefix the attribute would have been applied to the constructor parameter instead
         int? NullableValueProperty);
 
     public class ModelUsedInController

analyzers/tests/SonarAnalyzer.Test/TestCases/AspNet/AvoidUnderPosting.cs

@@ -13,10 +13,10 @@ public class ClassNotUsedInRequests
 
     public class ModelUsedInController
     {
-        public int ValueProperty { get; set; }                                      // Noncompliant {{Property used as input in a controller action should be nullable or annotated with the Required attribute to avoid under-posting.}}
+        public int ValueProperty { get; set; }                                      // Noncompliant {{Value type property used as input in a controller action should be nullable, required or annotated with the JsonRequiredAttribute to avoid under-posting.}}
 //                 ^^^^^^^^^^^^^
         public int? NullableValueProperty { get; set; }
-        [Required] public int RequiredValueProperty { get; set; }
+        [Required] public int RequiredValueProperty { get; set; }                   // Noncompliant, RequiredAttribute has no effect on value types
         [Range(0, 10)] public int ValuePropertyWithRangeValidation { get; set; }    // Noncompliant
         [Required] public int? RequiredNullableValueProperty { get; set; }
         public int PropertyWithPrivateSetter { get; private set; }