Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create rule S6932: Use model binding instead of reading raw request data #8930

Closed
wants to merge 62 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
79b5811
WIP: Add the RegisterSymbolStart wrapper
martin-strecker-sonarsource Feb 22, 2024
d2c4e6c
Clean up
martin-strecker-sonarsource Feb 27, 2024
f09a581
Rename
martin-strecker-sonarsource Feb 27, 2024
15659e6
Add RegisterCodeBlockStartActionCS and RegisterOperationAction
martin-strecker-sonarsource Feb 28, 2024
3a72741
Add remaining register methods
martin-strecker-sonarsource Feb 28, 2024
f56b0d2
Clean up
martin-strecker-sonarsource Feb 29, 2024
203a1d0
Add tests for missing VB support
martin-strecker-sonarsource Feb 29, 2024
aa567ef
Add comments
martin-strecker-sonarsource Feb 29, 2024
02d7aa7
Move TestDiagnosticAnalyzer
martin-strecker-sonarsource Feb 29, 2024
7deaaec
Cleanup
martin-strecker-sonarsource Feb 29, 2024
383b87f
Use property accessor instead of passing per ctor
martin-strecker-sonarsource Mar 4, 2024
2a2ffaa
Move registerCodeBlockAction and registerCodeBlockStartActionCS to Sy…
martin-strecker-sonarsource Mar 4, 2024
2c60f8c
Move registerOperationAction
martin-strecker-sonarsource Mar 4, 2024
927639a
Move registerOperationBlockAction, registerOperationBlockStartAction,…
martin-strecker-sonarsource Mar 4, 2024
d31f523
Move registerSyntaxNodeActionCS
martin-strecker-sonarsource Mar 4, 2024
43ec236
Move comments
martin-strecker-sonarsource Mar 4, 2024
7e8d31d
Make SymbolStartAnalysisContext a readonly struct
martin-strecker-sonarsource Mar 4, 2024
8142b5c
Code review.
martin-strecker-sonarsource Mar 4, 2024
99a523b
Refactorings
martin-strecker-sonarsource Mar 5, 2024
2743a4e
Improve RegisterSymbolStartAction_SymbolStartProperties
martin-strecker-sonarsource Mar 5, 2024
8e64f8e
Add support for VB
martin-strecker-sonarsource Mar 5, 2024
6cd98dc
Add packages.lock.json
martin-strecker-sonarsource Mar 6, 2024
f5922df
Remove packages.locks.json (VS created it after re-base)
martin-strecker-sonarsource Mar 6, 2024
f83cb2c
Rename SymbolStartAnalysisContext to SymbolStartAnalysisContextWrapper
martin-strecker-sonarsource Mar 11, 2024
dd1a605
Make fields readonly and rename
martin-strecker-sonarsource Mar 11, 2024
9c9f0c5
Move property
martin-strecker-sonarsource Mar 11, 2024
0c1ff8a
Remove alias
martin-strecker-sonarsource Mar 11, 2024
f259d3e
Inline "code" variable
martin-strecker-sonarsource Mar 11, 2024
a07b70b
Add RegisterSymbolStartAction_RegisterCodeBlockAction_ConditionalRegi…
martin-strecker-sonarsource Mar 11, 2024
d5f16fb
Improve test coverage
martin-strecker-sonarsource Mar 11, 2024
93c9cb5
Scaffold rule with ./scripts/rspec/rspec -language cs -ruleKey S6932 …
martin-strecker-sonarsource Mar 11, 2024
19d418f
Add registrations for asp.net core controller
martin-strecker-sonarsource Mar 11, 2024
6204f53
Pick changes from Martin/S2077_PetaPoco2 branch
martin-strecker-sonarsource Mar 11, 2024
b067076
Use tracker
martin-strecker-sonarsource Mar 11, 2024
e3de32e
Add support for Form access
martin-strecker-sonarsource Mar 11, 2024
af82bbc
Comments
martin-strecker-sonarsource Mar 11, 2024
942252f
Support all NonCompliant cases
martin-strecker-sonarsource Mar 12, 2024
f35eb46
Add support for overrides
martin-strecker-sonarsource Mar 12, 2024
c1726a3
Add IsOverridingFilterMethods and use ConcurrentStack
martin-strecker-sonarsource Mar 13, 2024
a084719
Extract descriptors
martin-strecker-sonarsource Mar 13, 2024
1dfab22
Re-factor
martin-strecker-sonarsource Mar 13, 2024
af98074
Styling
martin-strecker-sonarsource Mar 13, 2024
3ca6b46
Add invokedMemberNodeConstraint to ArgumentDescriptor
martin-strecker-sonarsource Mar 13, 2024
892d211
Refactor and comments
martin-strecker-sonarsource Mar 14, 2024
460d2ca
Fix UT
martin-strecker-sonarsource Mar 14, 2024
1b61e74
Use LanguageFacade
martin-strecker-sonarsource Mar 14, 2024
259c7cf
Collection expression
martin-strecker-sonarsource Mar 14, 2024
6131b53
Add Request.RouteValues write test case and simplify descriptors. Pas…
martin-strecker-sonarsource Mar 14, 2024
ae75770
Move Compliant() and NoncompliantKeyVariations to parameterized tests.
martin-strecker-sonarsource Mar 14, 2024
173fb53
Add CombinatorialDataAttribute
martin-strecker-sonarsource Mar 14, 2024
7efce24
Move remaining parameterized tests
martin-strecker-sonarsource Mar 14, 2024
c1d3ad0
Fix after re-base
martin-strecker-sonarsource Mar 14, 2024
383e5a5
EOF
martin-strecker-sonarsource Mar 14, 2024
016edfa
Code smells
martin-strecker-sonarsource Mar 14, 2024
bbe60e8
Add DottedExpressions tests and ElementAccess/Binding
martin-strecker-sonarsource Mar 14, 2024
cd149e1
Fix CAE handling
martin-strecker-sonarsource Mar 14, 2024
e8ec464
Review
martin-strecker-sonarsource Mar 18, 2024
a157346
Rename rule.
martin-strecker-sonarsource Mar 18, 2024
053e04f
Rename test file
martin-strecker-sonarsource Mar 18, 2024
26b0d76
Move IsOverridingFilterMethods logic
martin-strecker-sonarsource Mar 18, 2024
d1b9fea
Review
martin-strecker-sonarsource Mar 18, 2024
1d95916
Review
martin-strecker-sonarsource Mar 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
336 changes: 336 additions & 0 deletions analyzers/rspec/cs/S6932.html

Large diffs are not rendered by default.

25 changes: 25 additions & 0 deletions analyzers/rspec/cs/S6932.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"title": "Use model binding instead of reading raw request data",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [
"asp.net"
],
"defaultSeverity": "Major",
"ruleSpecification": "RSPEC-6932",
"sqKey": "S6932",
"scope": "Main",
"quickfix": "infeasible",
"code": {
"impacts": {
"MAINTAINABILITY": "HIGH",
"RELIABILITY": "MEDIUM",
"SECURITY": "MEDIUM"
},
"attribute": "FOCUSED"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,288 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about Sonar copyright claims in a file that is completely copied from Roslyn.
Would it be possible to copy the original file in its entirety, including any copyright disclaimer?
That would also avoid a lot of those Copied from comments that we have below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be discussed in https://trello.com/c/nw3yLmEY

* 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.CodeDom.Compiler;

namespace SonarAnalyzer.Extensions;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To have a separation between our extensions and Roslyn extensions, and avoid potential clash with new methods, when we will need to update this code, I would keep the original namespace from Roslyn, and have two separate extension classes, instead of partial classes.
This would not create much more ambiguity, since we already have multiple ExpressionSyntaxExtensions, for C# and VB.NET.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be discussed in https://trello.com/c/nw3yLmEY


[GeneratedCode("Copied From Roslyn", "575bc42589145ba18b4f1cc2267d02695f861d8f")]
public partial class ExpressionSyntaxExtensions
{
// Copied from
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method copied are out-of-order w.r.t. the original file in Roslyn.
Could we sort them by the same order? That would make future maintenance of this file, and alignment with changes in Roslyn, easier.
Even better would be to copy the entire file from Roslyn, as is or with minor modifications, so that we would not need to go method by method, when aligning to Roslyn.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is discussed in https://trello.com/c/nw3yLmEY

// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L319
public static bool IsWrittenTo(
this ExpressionSyntax expression,
SemanticModel semanticModel,
CancellationToken cancellationToken)
{
if (expression == null)
return false;

expression = GetExpressionToAnalyzeForWrites(expression);

if (expression.IsOnlyWrittenTo())
return true;

if (expression.IsInRefContext(out var refParent))
{
// most cases of `ref x` will count as a potential write of `x`. An important exception is:
// `ref readonly y = ref x`. In that case, because 'y' can't be written to, this would not
// be a write of 'x'.
if (refParent.Parent is EqualsValueClauseSyntax { Parent: VariableDeclaratorSyntax { Parent: VariableDeclarationSyntax { Type: { } variableDeclarationType } } })
{
if (ScopedTypeSyntaxWrapper.IsInstance(variableDeclarationType) && (ScopedTypeSyntaxWrapper)variableDeclarationType is { } scopedType)
{
variableDeclarationType = scopedType.Type;
}

if (RefTypeSyntaxWrapper.IsInstance(variableDeclarationType) && ((RefTypeSyntaxWrapper)variableDeclarationType).ReadOnlyKeyword != default)
{
return false;
}
}

return true;
}

// Similar to `ref x`, `&x` allows reads and write of the value, meaning `x` may be (but is not definitely)
// written to.
if (expression.Parent.IsKind(SyntaxKind.AddressOfExpression))
return true;

// We're written if we're used in a ++, or -- expression.
if (expression.IsOperandOfIncrementOrDecrementExpression())
return true;

if (expression.IsLeftSideOfAnyAssignExpression())
return true;

// An extension method invocation with a ref-this parameter can write to an expression.
if (expression.Parent is MemberAccessExpressionSyntax memberAccess &&
expression == memberAccess.Expression)
{
var symbol = semanticModel.GetSymbolInfo(memberAccess, cancellationToken).Symbol;
if (symbol is IMethodSymbol
{
MethodKind: MethodKind.ReducedExtension,
ReducedFrom.Parameters: { Length: > 0 } parameters,
} && parameters[0].RefKind == RefKind.Ref)
{
return true;
}
}

return false;
}

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L221
private static ExpressionSyntax GetExpressionToAnalyzeForWrites(ExpressionSyntax? expression)
{
if (expression.IsRightSideOfDotOrArrow())
{
expression = (ExpressionSyntax)expression.Parent;
}

expression = (ExpressionSyntax)expression.WalkUpParentheses();

return expression;
}

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L63
public static bool IsRightSideOfDotOrArrow(this ExpressionSyntax name)
=> IsAnyMemberAccessExpressionName(name) || IsRightSideOfQualifiedName(name);

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L41
public static bool IsAnyMemberAccessExpressionName(this ExpressionSyntax expression)
{
if (expression == null)
return false;

return expression == (expression.Parent as MemberAccessExpressionSyntax)?.Name ||
expression.IsMemberBindingExpressionName();
}

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L50
public static bool IsMemberBindingExpressionName(this ExpressionSyntax expression)
=> expression?.Parent is MemberBindingExpressionSyntax memberBinding &&
memberBinding.Name == expression;

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L54
public static bool IsRightSideOfQualifiedName(this ExpressionSyntax expression)
=> expression?.Parent is QualifiedNameSyntax qualifiedName && qualifiedName.Right == expression;

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L233
public static bool IsOnlyWrittenTo(this ExpressionSyntax expression)
{
expression = GetExpressionToAnalyzeForWrites(expression);

if (expression != null)
{
if (expression.IsInOutContext())
{
return true;
}

if (expression.Parent != null)
{
if (expression.IsLeftSideOfAssignExpression())
{
return true;
}

if (expression.IsAttributeNamedArgumentIdentifier())
{
return true;
}
}

if (IsExpressionOfArgumentInDeconstruction(expression))
{
return true;
}
}

return false;
}

/// <summary>
/// If this declaration or identifier is part of a deconstruction, find the deconstruction.
/// If found, returns either an assignment expression or a foreach variable statement.
/// Returns null otherwise.
///
/// copied from SyntaxExtensions.GetContainingDeconstruction.
/// </summary>
// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L273
private static bool IsExpressionOfArgumentInDeconstruction(ExpressionSyntax expr)
{
if (!expr.IsParentKind(SyntaxKind.Argument))
{
return false;
}

while (true)
{
var parent = expr.Parent;
if (parent == null)
{
return false;
}

switch (parent.Kind())
{
case SyntaxKind.Argument:
if (parent.Parent?.Kind() == SyntaxKindEx.TupleExpression)
{
expr = (ExpressionSyntax)parent.Parent;
continue;
}

return false;
case SyntaxKind.SimpleAssignmentExpression:
if (((AssignmentExpressionSyntax)parent).Left == expr)
{
return true;
}

return false;
case SyntaxKindEx.ForEachVariableStatement:
if (((ForEachVariableStatementSyntaxWrapper)parent).Variable == expr)
{
return true;
}

return false;

default:
return false;
}
}
}

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L190
public static bool IsInOutContext(this ExpressionSyntax expression)
=> expression?.Parent is ArgumentSyntax { RefOrOutKeyword: SyntaxToken { RawKind: (int)SyntaxKind.OutKeyword } } argument &&
argument.Expression == expression;

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L383
public static bool IsAttributeNamedArgumentIdentifier(this ExpressionSyntax expression)
{
var nameEquals = expression?.Parent as NameEqualsSyntax;
return nameEquals.IsParentKind(SyntaxKind.AttributeArgument);
}

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L194
public static bool IsInRefContext(this ExpressionSyntax expression)
=> IsInRefContext(expression, out _);

/// <summary>
/// Returns true if this expression is in some <c>ref</c> keyword context. If <see langword="true"/> then
/// <paramref name="refParent"/> will be the node containing the <see langword="ref"/> keyword.
/// </summary>
// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L201
public static bool IsInRefContext(this ExpressionSyntax expression, out SyntaxNode refParent)
{
while (expression?.Parent is ParenthesizedExpressionSyntax or PostfixUnaryExpressionSyntax { RawKind: (int)SyntaxKindEx.SuppressNullableWarningExpression })
expression = (ExpressionSyntax)expression.Parent;

if (expression?.Parent switch
{
ArgumentSyntax { RefOrOutKeyword.RawKind: (int)SyntaxKind.RefKeyword } => true,
var x when RefExpressionSyntaxWrapper.IsInstance(x) => true,
_ => false,
})
{
refParent = expression.Parent;
return true;
}

refParent = null;
return false;
}

// Copy of
// https://github.com/dotnet/roslyn/blob/575bc42589145ba18b4f1cc2267d02695f861d8f/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/ExpressionSyntaxExtensions.cs#L389
public static bool IsOperandOfIncrementOrDecrementExpression(this ExpressionSyntax expression)
{
if (expression?.Parent is SyntaxNode parent)
{
switch (parent.Kind())
{
case SyntaxKind.PostIncrementExpression:
case SyntaxKind.PreIncrementExpression:
case SyntaxKind.PostDecrementExpression:
case SyntaxKind.PreDecrementExpression:
return true;
}
}

return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

namespace SonarAnalyzer.Extensions
{
public static class ExpressionSyntaxExtensions
public static partial class ExpressionSyntaxExtensions
{
private static readonly ISet<SyntaxKind> EqualsOrNotEquals = new HashSet<SyntaxKind>
{
Expand Down
Loading
Loading