diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/AvoidUnderPosting.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/AvoidUnderPosting.cs index dd6e637b2c4..1f0c73e87b8 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/AvoidUnderPosting.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/AvoidUnderPosting.cs @@ -18,26 +18,68 @@ * 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 = "FIXME"; + 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); public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction(c => + context.RegisterCompilationStartAction(compilationStart => + { + if (!compilationStart.Compilation.ReferencesControllers()) + { + return; + } + var actionParameters = new ConcurrentStack(); + + compilationStart.RegisterSymbolStartAction(symbolStart => { - var node = c.Node; - if (true) + var method = (IMethodSymbol)symbolStart.Symbol; + if (method.IsControllerMethod()) { - c.ReportIssue(Diagnostic.Create(Rule, node.GetLocation())); + actionParameters.PushRange([.. method.Parameters]); } - }, - SyntaxKind.InvocationExpression); + }, SymbolKind.Method); + + compilationStart.RegisterCompilationEndAction(compilationEnd => + { + var examinedTypes = new HashSet(); + foreach (var parameter in actionParameters) + { + if (!examinedTypes.Contains(parameter.Type)) + { + examinedTypes.Add(parameter.Type); + CheckInvalidProperties(parameter, compilationEnd); + } + } + }); + }); + + private static void CheckInvalidProperties(IParameterSymbol actionParameter, SonarCompilationReportingContext context) + { + if (actionParameter.Type.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() is { } parameterSyntax) + { + var invalidProperties = actionParameter.Type.GetMembers() + .Where(x => x is IPropertySymbol property + && !property.Type.CanBeNull() + && property.GetEffectiveAccessibility() == Accessibility.Public + && property.SetMethod?.DeclaredAccessibility is Accessibility.Public + && !property.HasAttribute(KnownType.System_ComponentModel_DataAnnotations_RequiredAttribute) + && !property.HasAttribute(KnownType.Microsoft_AspNetCore_Mvc_ModelBinding_Validation_ValidateNeverAttribute)); + foreach (var property in invalidProperties) + { + var propertySyntax = property.DeclaringSyntaxReferences[0].GetSyntax(); + context.ReportIssue(CSharpGeneratedCodeRecognizer.Instance, Diagnostic.Create(Rule, propertySyntax.GetLocation(), parameterSyntax.GetLocation())); + } + } + } } diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs index abe3cd52f53..6cea705d1c4 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs @@ -81,6 +81,7 @@ public sealed partial class KnownType public static readonly KnownType Microsoft_AspNetCore_Mvc_HttpPutAttribute = new("Microsoft.AspNetCore.Mvc.HttpPutAttribute"); public static readonly KnownType Microsoft_AspNetCore_Mvc_IActionResult = new("Microsoft.AspNetCore.Mvc.IActionResult"); public static readonly KnownType Microsoft_AspNetCore_Mvc_IgnoreAntiforgeryTokenAttribute = new("Microsoft.AspNetCore.Mvc.IgnoreAntiforgeryTokenAttribute"); + public static readonly KnownType Microsoft_AspNetCore_Mvc_ModelBinding_Validation_ValidateNeverAttribute = new("Microsoft.AspNetCore.Mvc.ModelBinding.Validation.ValidateNeverAttribute"); public static readonly KnownType Microsoft_AspNetCore_Mvc_NonActionAttribute = new("Microsoft.AspNetCore.Mvc.NonActionAttribute"); public static readonly KnownType Microsoft_AspNetCore_Mvc_NonControllerAttribute = new("Microsoft.AspNetCore.Mvc.NonControllerAttribute"); public static readonly KnownType Microsoft_AspNetCore_Mvc_RazorPages_PageModel = new("Microsoft.AspNetCore.Mvc.RazorPages.PageModel"); @@ -279,6 +280,7 @@ public sealed partial class KnownType public static readonly KnownType System_ComponentModel_Composition_PartCreationPolicyAttribute = new("System.ComponentModel.Composition.PartCreationPolicyAttribute"); public static readonly KnownType System_ComponentModel_DataAnnotations_KeyAttribute = new("System.ComponentModel.DataAnnotations.KeyAttribute"); public static readonly KnownType System_ComponentModel_DataAnnotations_RegularExpressionAttribute = new("System.ComponentModel.DataAnnotations.RegularExpressionAttribute"); + public static readonly KnownType System_ComponentModel_DataAnnotations_RequiredAttribute = new("System.ComponentModel.DataAnnotations.RequiredAttribute"); public static readonly KnownType System_ComponentModel_DefaultValueAttribute = new("System.ComponentModel.DefaultValueAttribute"); public static readonly KnownType System_ComponentModel_EditorBrowsableAttribute = new("System.ComponentModel.EditorBrowsableAttribute"); public static readonly KnownType System_ComponentModel_LocalizableAttribute = new("System.ComponentModel.LocalizableAttribute"); diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/AvoidUnderPostingTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/AvoidUnderPostingTest.cs index 2610cdb6222..3db5b0455e4 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/AvoidUnderPostingTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/AvoidUnderPostingTest.cs @@ -18,6 +18,8 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +#if NET + using SonarAnalyzer.Rules.CSharp; namespace SonarAnalyzer.Test.Rules; @@ -25,9 +27,18 @@ namespace SonarAnalyzer.Test.Rules; [TestClass] public class AvoidUnderPostingTest { - private readonly VerifierBuilder builder = new VerifierBuilder(); + private readonly VerifierBuilder builder = new VerifierBuilder() + .WithOptions(ParseOptionsHelper.FromCSharp12) + .AddReferences([ + AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcAbstractions, + AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcCore, + AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcViewFeatures, + ..NuGetMetadataReference.SystemComponentModelAnnotations(Constants.NuGetLatestVersion) + ]); [TestMethod] - public void AvoidUnderPosting_CS() => - builder.AddPaths("AvoidUnderPosting.cs").Verify(); + public void AvoidUnderPosting_CSharp12() => + builder.AddPaths("AvoidUnderPosting.CSharp12.cs").Verify(); } + +#endif diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/AvoidUnderPosting.CSharp12.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/AvoidUnderPosting.CSharp12.cs new file mode 100644 index 00000000000..0e74d3ff147 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/AvoidUnderPosting.CSharp12.cs @@ -0,0 +1,76 @@ +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; +using System.ComponentModel.DataAnnotations; + +public class ClassNotUsedInRequests +{ + int ValueProperty { get; set; } // Compliant +} + +public struct Struct { public int Foo { get; set; } } +public record struct RecordStruct { public int Foo { get; set; } } + +public class ModelUsedInController +{ + public int ValueProperty { get; set; } // Noncompliant + public int? NullableValueProperty { get; set; } // Compliant + [Required] public int RequiredValueProperty { get; set; } // Compliant + [ValidateNever] public int NotValidatedValueProperty { get; set; } // Compliant + [Range(0, 10)] public int ValuePropertyWithRangeValidation { get; set; } // Noncompliant + [Required] public int? RequiredNullableValueProperty { get; set; } // Compliant + public int PropertyWithPrivateSetter { get; private set; } // Compliant + protected int ProtectedProperty { get; set; } // Compliant + internal int InternalProperty { get; set; } // Compliant + protected internal int ProtectedInternalProperty { get; set; } // Compliant + private int PrivateProperty { get; set; } // Compliant + public int ReadOnlyProperty => 42; // Compliant + public int field = 42; // Compliant + + public Struct StructValueProperty { get; set; } // Noncompliant + public RecordStruct RecordStructValueProperty { get; set; } // Noncompliant + +#nullable enable + public string NonNullableReferenceProperty { get; set; } // Noncompliant + [Required] public string RequiredNonNullableReferenceProperty { get; set; } // Compliant + public string? NullableReferenceProperty { get; set; } // Compliant +#nullable disable + public string ReferenceProperty { get; set; } // Compliant +} + +public class DerivedFromController : Controller +{ + [HttpPost] + public IActionResult Create(ModelUsedInController model) + { + return View(model); + } +} + +[Controller] +public class DecoratedWithControllerAttribute // better suited for parameterized UTs +{ + [HttpGet] public IActionResult Get(ModelUsedInController model) => null; + [HttpPost] public IActionResult Post(ModelUsedInController model) => null; + [HttpPut] public IActionResult Put(ModelUsedInController model) => null; + [HttpDelete] public IActionResult Delete(ModelUsedInController model) => null; +} + +[ApiController] +[Route("api/[controller]")] +public class DecoratedWithApiControlerAttribute : ControllerBase +{ + [HttpGet] + public int Single(ModelUsedInController model) => 42; + + [HttpGet] + [HttpPost] + [HttpPut] + [HttpDelete] + public int Multiple(ModelUsedInController model) => 42; + + [AcceptVerbs("POST")] + public int Verb(ModelUsedInController model) => 42; + + [AcceptVerbs("GET", "POST", "PUT", "DELETE")] + public int MultipleVerbs(ModelUsedInController model) => 42; +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/AvoidUnderPosting.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/AvoidUnderPosting.cs deleted file mode 100644 index a790d6d1f92..00000000000 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/AvoidUnderPosting.cs +++ /dev/null @@ -1,5 +0,0 @@ -using System; - -public class Program -{ -}