Create rule S6967: ModelState.IsValid should be called in controller actions (#3856)

rules/S6967/S6967_csharp.cs

@@ -0,0 +1,211 @@
+using System.ComponentModel.DataAnnotations;
+using System.Net;
+using Microsoft.AspNetCore.Mvc;
+using Microsoft.AspNetCore.Mvc.Filters;
+
+[assembly: ApiController] // This testcase should be on a dedicated test.
+public class ControllerWithApiAttributeAtTheAssemblyLevel : ControllerBase
+{
+    [HttpPost("/[controller]")]
+    public string Add(Movie movie)                                              // Compliant, ApiController attribute is applied at the assembly level.
+    {
+        return "Hello!";
+    }
+}
+
+[ApiController]
+public class ControllerWithApiAttributeAtTheClassLevel : ControllerBase
+{
+    [HttpPost("/[controller]")]
+    public string Add(Movie movie)                                              // Compliant, ApiController attribute is applied at the class level.
+    {
+        return "Hello!";
+    }
+}
+
+[Controller]
+public class ControllerThatDoesNotInheritFromControllerBase
+{
+    [HttpPost("/[controller]")]
+    public string Add(Movie movie)                                              // Compliant, ModelState is not available in this context.
+    {
+        return "Hello!";
+    }
+}
+
+public class SimpleController
+{
+    [HttpPost("/[controller]")]
+    public string Add(Movie movie)                                              // Compliant, ModelState is not available in this context.
+    {
+        return "Hello!";
+    }
+}
+
+public class NonCompliantController : ControllerBase
+{
+    [HttpGet("/[controller]")]
+    public string Get([Required, FromQuery] string id)                           // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [HttpPost("/[controller]")]
+    public string Post(Movie movie)                                             // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [HttpPut("/[controller]")]
+    public string Put(Movie movie)                                              // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [HttpDelete("/[controller]")]
+    public string Delete(Movie movie)                                           // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [HttpPatch("/[controller]")]
+    public string Patch(Movie movie)                                            // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [HttpGet]
+    [HttpPost]
+    [HttpPut]
+    [HttpDelete]
+    [HttpPatch]
+    [Route("/[controller]/mix")]
+    public string Mix([Required, FromQuery, EmailAddress] string email)         // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [AcceptVerbs("GET")]
+    [Route("/[controller]/accept-verbs")]
+    public string AGet([Required] string id)                                    // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [AcceptVerbs("POST")]
+    [Route("/[controller]/accept-verbs")]
+    public string APost(Movie movie)                                            // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [AcceptVerbs("PUT")]
+    [Route("/[controller]/accept-verbs")]
+    public string APut(Movie movie)                                             // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [AcceptVerbs("DELETE")]
+    [Route("/[controller]/accept-verbs")]
+    public string ADelete(Movie movie)                                          // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [AcceptVerbs("PATCH")]
+    [Route("/[controller]/accept-verbs")]
+    public string APatch(Movie movie)                                           // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [AcceptVerbs("GET", "POST", "PUT", "DELETE", "PATCH")]
+    [Route("/[controller]/many")]
+    public string Many([Required, FromQuery, EmailAddress] string email)        // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [HttpPost("/[controller]/without-validation")]
+    public string DtoWithoutValidation(DtoWithoutValidation dto)                // Compliant, DtoWithoutValidation does not have any validation attributes.
+    {
+        return "Hello!";
+    }
+
+    [HttpPost("/[controller]/custom-attribute-validation")]
+    public string DtoWithCustomAttribute([ClassicMovie] string title)           // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [HttpPost("/[controller]/custom-validation")]
+    public string DtoImplementingIValidatableObject(ValidatableMovie movie)     // Noncompliant
+    {
+        return "Hello!";
+    }
+
+    [ValidateModel]
+    [HttpPost("/[controller]/validation-filter")]
+    public string WithValidationFilter(ValidatableMovie movie)                  // Compliant, the model is validated by the ValidateModelAttribute filter.
+    {
+        return "Hello!";
+    }
+
+    [HttpPost("/[controller]/try-validate-model")]
+    public string CallsTryValidateModel([Required] string email)                // Compliant, calls TryValidateModel
+    {
+        return TryValidateModel(email) ? "Hi!" : "Hello!";
+    }
+
+    [HttpGet("/[controller]/list")]
+    public string[] List() => null;                                             // Compliant
+}
+
+public class Movie
+{
+    [Required]
+    public string Title { get; set; }
+
+    [Range(1900, 2200)]
+    public int Year { get; set; }
+}
+
+public class DtoWithoutValidation
+{
+    public int? Id { get; set; }
+    public string? Name { get; set; }
+}
+
+public class ClassicMovieAttribute : ValidationAttribute
+{
+    protected override ValidationResult? IsValid(object? value, ValidationContext validationContext)
+    {
+        return ValidationResult.Success;
+    }
+}
+
+public class ValidatableMovie : IValidatableObject
+{
+    public int Id { get; set; }
+
+    [Required]
+    [StringLength(100)]
+    public string Title { get; set; } = null!;
+
+    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
+    {
+        yield return new ValidationResult("Title is required", new[] { nameof(Title) });
+    }
+}
+
+public class ValidateModelAttribute : ActionFilterAttribute
+{
+    public override void OnActionExecuting(ActionExecutingContext context)
+    {
+        if (!context.ModelState.IsValid)
+        {
+            context.Result = new BadRequestObjectResult(context.ModelState);
+        }
+    }
+}

rules/S6967/csharp/metadata.json

@@ -0,0 +1,2 @@
+{
+}

rules/S6967/csharp/rule.adoc

@@ -0,0 +1,101 @@
+In the context of ASP.NET Core MVC web applications, both model binding and model validation are processes that take place prior to the execution of a controller action. It is imperative for the application to examine the `ModelState.IsValid` and respond accordingly.
+
+This rule enforces the developer to validate the model within a controller action, ensuring the application's robustness and reliability.
+
+== Why is this an issue?
+
+Querying the `ModelState.IsValid` property is necessary because it checks if the submitted data in the HTTP request is valid or not. This property evaluates all the validation attributes applied on your model properties and determines whether the data provided satisfies those validation rules.
+
+=== What is the potential impact?
+
+Skipping model validation can lead to:
+
+* Data Integrity Issues: Without validation, incorrect or inconsistent data can be saved to your database, leading to potential data corruption or loss.
+
+* Security Vulnerabilities: Skipping validation can expose your application to security risks.
+
+* Application Errors: Invalid data can lead to unexpected application errors or crashes, which can disrupt the user experience and potentially lead to data loss.
+
+* Poor User Experience: Without validation, users may not receive appropriate feedback about any mistakes in the data they have entered, leading to confusion and frustration.
+
+* Increased Debugging Time: If invalid data causes issues in your application and was not validatated at the entry point, it can take significantly more time to debug and fix these issues.
+
+Therefore, it's highly recommended to always validate models in your application to ensure data integrity, application stability, and a good user experience.
+
+While client-side validation enhances user experience by providing immediate feedback, it's not sufficient due to potential manipulation of client-side code, browser compatibility issues, and dependence on JavaScript. Users can bypass or disable it, leading to invalid or malicious data being submitted. Therefore, server-side validation is essential to ensure data integrity and security, making it a best practice to use both client-side and server-side validation in your application.
+
+=== Exceptions
+
+* Web API controllers don't have to check `ModelState.IsValid` if they have the `[ApiController]` attribute. In that case, an automatic HTTP `400` response containing error details is returned when model state is invalid.
+
+* When action filters are used for controller actions, the analyzer will skip the model validation detection to avoid false positives since the model state could be verified by the action filer.
+
+* `TryValidateModel` can also be used for model validation.
+
+* The project references a 3rd-party library for validation, e.g. FluentValidation.
+
+* The rule will not raise issues if the model, or the model members, are not decorated with validation attributes, or if it does not implement custom validation.
+
+== How to fix it
+
+If `ModelState.IsValid` returns true, it means that the data is valid and the process can continue. If it returns false, it means that the validation failed, indicating that the data is not in the expected format or is missing required information.
+
+In such cases, the controller action should handle this by returning an appropriate response, such as re-displaying the form with error messages. This helps maintain the integrity of the data and provides feedback to the user, enhancing the overall user experience and security of your application.
+
+=== Code examples
+
+==== Noncompliant code example
+
+[source,csharp,diff-id=1,diff-type=noncompliant]
+----
+public async Task<IActionResult> Create(Movie movie) // Noncompliant: model validity check is missing
+{
+    _context.Movies.Add(movie);
+    await _context.SaveChangesAsync();
+
+    return RedirectToAction(nameof(Index));
+}
+----
+
+==== Compliant solution
+
+[source,csharp,diff-id=1,diff-type=compliant]
+----
+public async Task<IActionResult> Create(Movie movie)
+{
+    if (!ModelState.IsValid)
+    {
+        return View(movie);
+    }
+
+    _context.Movies.Add(movie);
+    await _context.SaveChangesAsync();
+
+    return RedirectToAction(nameof(Index));
+}
+----
+
+== Resources
+
+=== Documentation
+
+* Microsoft Learn - https://learn.microsoft.com/en-us/aspnet/core/mvc/models/validation[Model Validation]
+* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.modelbinding.modelstatedictionary.isvalid[IsValid property]
+* Microsoft Learn - https://learn.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.validationattribute[ValidationAttribute]
+* Fluent Validation - https://docs.fluentvalidation.net/en/latest/aspnet.html[ASP.NET Core integration]
+
+ifdef::env-github,rspecator-view[]
+
+'''
+== Implementation Specification
+(visible only on this page)
+
+=== Message
+
+ModelState.IsValid should be checked in controller actions.
+
+=== Highlighting
+
+Controller action identifier.
+
+endif::env-github,rspecator-view[]

rules/S6967/metadata.json

@@ -0,0 +1,26 @@
+{
+    "title": "ModelState.IsValid should be called in controller actions",
+    "type": "CODE_SMELL",
+    "status": "ready",
+    "remediation": {
+      "func": "Constant\/Issue",
+      "constantCost": "5min"
+    },
+    "tags": [
+        "asp.net"
+    ],
+    "defaultSeverity": "Major",
+    "ruleSpecification": "RSPEC-6967",
+    "sqKey": "S6967",
+    "scope": "All",
+    "defaultQualityProfiles": ["Sonar way"],
+    "quickfix": "infeasible",
+    "code": {
+      "impacts": {
+        "MAINTAINABILITY": "MEDIUM",
+        "RELIABILITY": "HIGH",
+        "SECURITY": "HIGH"
+      },
+      "attribute": "TRUSTWORTHY"
+    }
+}