Create rule S6932: Use model binding instead of reading raw request data #8930
Conversation
…mbolStartAnalysisContext
… and registerSymbolEndAction
There was a problem hiding this comment.
This is a left over after the project was moved. It can safely be deleted.
martin-strecker-sonarsource
changed the title
There was a problem hiding this comment.
This file seems to follow Roslyn code conventions, which is where the file is taken from, as opposed to our own.
Given that we don't have an outcome yet, for this Trello card, I will not review this file from a stylistic point of view in this PR, but only functionally.
However, I would ask you to mention this file in the "How we work" ticket, as a possible action to to review/refactor this file, once a decision is taken on how to deal with imported code from Roslyn.
There was a problem hiding this comment.
Please do not review. We will not make any edits to the file; the code is already being tested. We want to treat it as if it was a third party dll.
|
| @@ -179,7 +179,6 @@ node switch | |||
| DestructorDeclarationSyntax { Identifier: var identifier } => identifier, | |||
| EnumMemberDeclarationSyntax { Identifier: var identifier } => identifier, | |||
| EventDeclarationSyntax { Identifier: var identifier } => identifier, | |||
| IdentifierNameSyntax { Identifier: var identifier } => identifier, | |||
There was a problem hiding this comment.
I guess this case wasn't used at all in the code base, and has hence been removed?
I don't see connections between IdentifierNameSyntax and SimpleBaseTypeSyntax: their closest common ancestor is CSharpSyntaxNode.
There was a problem hiding this comment.
IdentifierNameSyntax is already covered by it's base class SimpleNameSyntax in line 202.
This change is "clean as you code" and unrelated to the SimpleBaseTypeSyntax
| @@ -199,6 +198,7 @@ node switch | |||
| PointerTypeSyntax { ElementType: { } elementType } => GetIdentifier(elementType), | |||
| PredefinedTypeSyntax { Keyword: var keyword } => keyword, | |||
| QualifiedNameSyntax { Right.Identifier: var identifier } => identifier, | |||
| SimpleBaseTypeSyntax { Type: { } type } => GetIdentifier(type), | |||
There was a problem hiding this comment.
Given this production rule, I wonder whether Type: { } type is unnecessary, and should be replaced with { Type: var type } instead, as we do in the same method for other cases such as AliasQualifiedNameSyntax, ArgumentSyntax etc.
I am not sure, but I think so, because, when I look at the code auto-generated by Roslyn, I see the following:
public sealed partial class SimpleBaseTypeSyntax : BaseTypeSyntax
{
private TypeSyntax? type; // This is nullable
internal SimpleBaseTypeSyntax(InternalSyntax.CSharpSyntaxNode green, SyntaxNode? parent, int position)
: base(green, parent, position)
{
}
public override TypeSyntax Type => GetRedAtZero(ref this.type)!;
// However, the Type property is not nullable, and non-nullability is asserted with a bangDo you have a case of Type being null?
In any case, I don't think this is a blocking issue, also because the code seems to be covered fully: https://sonarcloud.io/code?id=sonaranalyzer-dotnet&pullRequest=8902&selected=sonaranalyzer-dotnet%3Aanalyzers%2Fsrc%2FSonarAnalyzer.CSharp%2FExtensions%2FSyntaxNodeExtensions.cs&line=201
There was a problem hiding this comment.
As long as we do not use version of Roslyn with nullable annotations, we should always assume null as a possible value. Otherwise, we a re always in the "not sure" camp and have to consult Syntax.xml or any other kind of documentation. Let's just play "Better safe than sorry".
| @@ -503,6 +494,44 @@ public static ConditionalAccessExpressionSyntax GetRootConditionalAccessExpressi | |||
| return current; | |||
| } | |||
|
|
|||
| // Copy of | |||
There was a problem hiding this comment.
Unlike for analyzers/src/SonarAnalyzer.CSharp/Extensions/ExpressionSyntaxExtensions.Roslyn.cs, this file contains also our code, not just code based on, or copied from Roslyn.
So I would need to comment on style since I think we should be consistent on stylistic rules at least within the same file. That makes review quite challenging.
I think it's best to keep Roslyn imported code segregated, in its own file, and with its own namespace (also for copyright concerns, see comment above).
There was a problem hiding this comment.
Agreed. You may want to join https://trello.com/c/nw3yLmEY We have a meeting today.
The way I did it here, is proposed but not decided yet. In this file, there are already a lot of copied parts from Roslyn and I kept that for the time being.
| // based on Type="ArgumentListSyntax" in https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/Syntax/Syntax.xml | ||
| public static ArgumentListSyntax ArgumentList(this SyntaxNode node) => | ||
| public static BaseArgumentListSyntax ArgumentList(this SyntaxNode node) => |
There was a problem hiding this comment.
The signature seems now incompatible with the comment above.
What I understand from based on Type="ArgumentListSyntax" is that this method gets the ArgumentList property/field from all the syntax types for which Syntax.xml defines an AbstractNode that has a Field with Name="ArgumentList" and Type="ArgumentListSyntax".
In that case, the method should return a ArgumentListSyntax, not a BaseArgumentListSyntax.
There was a problem hiding this comment.
The comment was wrong.
| // based on Type="ArgumentListSyntax" in https://github.com/dotnet/roslyn/blob/main/src/Compilers/CSharp/Portable/Syntax/Syntax.xml | ||
| public static ArgumentListSyntax ArgumentList(this SyntaxNode node) => | ||
| public static BaseArgumentListSyntax ArgumentList(this SyntaxNode node) => | ||
| node switch | ||
| { | ||
| ObjectCreationExpressionSyntax creation => creation.ArgumentList, |
There was a problem hiding this comment.
In Syntax.xml this is actually BaseObjectCreationExpressionSyntax (see here).
There was a problem hiding this comment.
There is no BaseObjectCreationExpressionSyntax in our version of Roslyn. The ImplicitObjectCreationExpressionSyntaxWrapper case below covers that case.
| } | ||
| // Within a single code block, access via constant and variable keys could be mixed | ||
| // We only want to raise, if all access were done via constants | ||
| var allConstantAccess = true; |
There was a problem hiding this comment.
Subjective: allConstantAccesses (using access as countable) or onlyConstantAccess (using access as non-countable).
| { | ||
| var argument = (ArgumentSyntax)nodeContext.Node; | ||
| var context = new ArgumentContext(argument, nodeContext.SemanticModel); | ||
| if (allConstantAccess && Array.Exists(argumentDescriptors, x => Language.Tracker.Argument.MatchArgument(x)(context))) |
There was a problem hiding this comment.
Educational: can this new Language.Tracker.Argument be used in cases like this one?
What are the pros and cons w.r.t. use a MethodParameterLookup?
| && typeArguments[0].Is(KnownType.System_String) | ||
| && typeArguments[1].Is(KnownType.Microsoft_Extensions_Primitives_StringValues); | ||
|
|
||
| private readonly record struct ReportCandidate(string Message, Location Location, bool OriginatesFromParameter = false); |
There was a problem hiding this comment.
The default value false of OriginatesFromParameter doesn't seem to ever be used (it is always explicitly set on ReportCandidate construction), so it can be removed.
| { | ||
| var memberAccess = (MemberAccessExpressionSyntax)nodeContext.Node; | ||
| var context = new PropertyAccessContext(memberAccess, nodeContext.SemanticModel, memberAccess.Name.Identifier.ValueText); | ||
| if (Language.Tracker.PropertyAccess.MatchProperty(propertyAccessDescriptors)(context)) |
There was a problem hiding this comment.
Educational: just to test my understanding. In the following scenario:
_ = Request.Form.Files[F()];
string F() => "a string";We would still raise a non-compliance, because, even if the parameter of the indexer is not a constant value, the user can still use model binding:
- using
IFormFileCollectionto bind all the forms to a parameter of type collection - using that collection to access the
F()-th element - and that is considered proper use of model binding
This is different from the ArgumentSyntax cases above, where, if the argument is not a de-facto constant, we just don't raise, as there is no "collection binding" feature available.
Is all the above correct? Can we add a comment explaining all of that? I think this difference between forms and the rest is very important to understand the frame of this implementation, and in particular the need to register to both ArgumentSyntax and SimpleMemberAccessSyntax.
The comment should probably go above the signature of the method.
| // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| _ = Request.Form.Files; // Noncompliant {{Use IFormFile or IFormFileCollection binding instead}} | ||
| // ^^^^^^^^^^^^^^^^^^ | ||
| _ = Request.Form.Files["file"]; // Noncompliant {{Use IFormFile or IFormFileCollection binding instead}} |
There was a problem hiding this comment.
Can we add tests for Form.Files that take the result of a method?
_ = Request.Form.Files[F()];Form.Files is "special", compared to the other Request members, as it doesn't require its parameter to be constant, for the "model binding" suggestion to be valid. Form.Files doesn't seem to be included in the parameterized tests, so I think we should be explicit here.
There was a problem hiding this comment.
I used a parameter instead. It serves the same purpose.
| return current; | ||
| } | ||
|
|
||
| private static ExpressionSyntax GetExpressionOfArgumentParent(ArgumentSyntax argument) => |
There was a problem hiding this comment.
This method is called by GetPrimaryLocation, so it should be placed under it, as for the following codying style rule:
Once grouped as specified above, methods which are called by other methods in the same group should be placed below the callers.
|
|
||
| private static ExpressionSyntax GetExpressionOfArgumentParent(ArgumentSyntax argument) => | ||
| argument switch | ||
| { |
There was a problem hiding this comment.
This method doesn't take into account parenthesized expressions, yet it seems to work correctly.
Can we add some unit tests to make that sure? Diagnostic location should also be asserted.
_ = Request.Form.Files[("file")]; // Noncompliant
// ^^^^^^^^^^^^^^^^^^
_ = Request.Form.Files[(("file"))]; // Noncompliant
// ^^^^^^^^^^^^^^^^^^
_ = (Request.Form.Files)["file"]; // Noncompliant
// ^^^^^^^^^^^^^^^^^^
_ = ((Request.Form.Files))["file"]; // Noncompliant
// ^^^^^^^^^^^^^^^^^^Remark: I am surprised how the round brackets are not included in the primary location in cases like the following: _ = (Request.Form.Files)["file"];
The parent of the ArgumentSyntax is a BracketedArgumentListSyntax, whose parent is a ElementAccessExpressionSyntax, whose Expression is the ParenthesizedExpression, which includes the ( and ) tokens.
What am I missing?
| @@ -1,938 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
Why was this deleted?
There was a problem hiding this comment.
See #8930 (review)
|
Hi @martin-strecker-sonarsource, this is an impressive PR. With 4788 lines changed, it is very hard to review properly. I can see multiple topics handled in this PR:
I suggest splitting this into digestible pieces :) Regarding the implementation. I just started to review it, and I'm still wrapping my head around it, but can't we start with a simple implementation that uses a syntax walker? at least for the code blocks? |
| AddAspNetCoreDescriptors(argumentDescriptors, propertyAccessDescriptors); | ||
| } | ||
| // TODO: Add descriptors for Asp.Net MVC 4.x | ||
| return ([.. argumentDescriptors], [.. propertyAccessDescriptors]); |
There was a problem hiding this comment.
Why not return the lists directly?
|
Fixes #8871
Replaces #8902