-
Notifications
You must be signed in to change notification settings - Fork 226
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
New Rule S6494: The value type properties of a model class should be …
…nullable or marked as "Required" to avoid under-posting. (#9099)
- Loading branch information
1 parent
62d473b
commit 18ecf32
Showing
15 changed files
with
789 additions
and
2 deletions.
There are no files selected for viewing
17 changes: 17 additions & 0 deletions
17
analyzers/its/Projects/ManuallyAddedNoncompliantIssues.CS/IntentionalFindings/S6964.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
10 changes: 10 additions & 0 deletions
10
...ers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6964-IntentionalFindings-net8.0.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} | ||
] | ||
} |
6 changes: 6 additions & 0 deletions
6
...ers/its/expected/ManuallyAddedNoncompliantIssues.CS/S6967-IntentionalFindings-net8.0.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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> | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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" | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -318,6 +318,7 @@ | |
"S6934", | ||
"S6961", | ||
"S6962", | ||
"S6964", | ||
"S6965", | ||
"S6966", | ||
"S6967", | ||
|
144 changes: 144 additions & 0 deletions
144
analyzers/src/SonarAnalyzer.CSharp/Rules/AspNet/AvoidUnderPosting.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} |
Oops, something went wrong.