Rule S4834: Controlling permissions is security-sensitive #2096
Conversation
ghost
assigned 
valhristov
force-pushed
the
val/permissions
branch
from
6bee7eb
to
b31502c
Compare
![sonarsource-next[bot]](https://avatars.githubusercontent.com/in/11855?s=60&v=4){kind=small}
There was a problem hiding this comment.
SonarQube analysis found issues:
Including the following issue(s) which could not be reported in line:
Bug: Change this condition so that it does not always evaluate to 'false'; some subsequent code is never executed. (more)
| @@ -51,7 +51,8 @@ public CSharpMethodDeclarationTracker(IAnalyzerConfiguration analysisConfigurati | |||
| } | |||
|
|
|||
| var methodDeclaration = context.MethodSymbol.DeclaringSyntaxReferences | |||
| .Select(r => (MethodDeclarationSyntax)r.GetSyntax()) | |||
| .Select(r => r.GetSyntax()) | |||
| .OfType<BaseMethodDeclarationSyntax>() | |||
There was a problem hiding this comment.
The property getters and setters are also methods, hence we filter only "normal" methods, constructors and destructors here.
| @@ -103,7 +103,7 @@ bool IsTrackedRelationship(SyntaxNode contextNode, SemanticModel semanticModel, | |||
| { | |||
| foreach(var baseTypeNode in context.AllBaseTypeNodes) | |||
| { | |||
| if (context.Model.GetTypeInfo(baseTypeNode).Type?.DerivesFrom(type) ?? false) | |||
| if (context.Model.GetTypeInfo(baseTypeNode).Type?.DerivesOrImplements(type) ?? false) | |||
There was a problem hiding this comment.
I need to support interfaces here too.
| @@ -35,7 +36,7 @@ protected MethodDeclarationTracker(IAnalyzerConfiguration analysisConfiguration, | |||
| { | |||
| } | |||
|
|
|||
| protected abstract SyntaxToken GetMethodIdentifier(SyntaxNode methodDeclaration); | |||
| protected abstract SyntaxToken? GetMethodIdentifier(SyntaxNode methodDeclaration); | |||
There was a problem hiding this comment.
MethodIdentifier is null when the method is not a "normal" method, e.g. a property for example
| @@ -165,6 +165,22 @@ public static SimpleNameSyntax GetIdentifier(this ExpressionSyntax expression) | |||
| } | |||
| } | |||
|
|
|||
| public static SyntaxToken? GetIdentifierOrDefault(this MethodBaseSyntax methodDeclaration) | |||
There was a problem hiding this comment.
From the C# syntax helper
| } | ||
| } | ||
| } | ||
|
|
||
| bool IsTrackedMethod(IMethodSymbol methodSymbol, Compilation compilation) | ||
| { | ||
| if (methodSymbol.MethodKind != MethodKind.Ordinary && | ||
| methodSymbol.MethodKind != MethodKind.ReducedExtension) // Just methods |
There was a problem hiding this comment.
Why not make these conditions? This seems like quite a big constraint to bake into the base tracker class.
Also, the MethodKind enum has values for Constructor, Desctructor, PropertyGet, PropertySet etc. Presumably this check is filtering out all of those types so the changes above to handle those kinds of method symbols aren't required if this check is done?
There was a problem hiding this comment.
I added logic to obtain identifier from all method symbol kinds and extracted the method kind check as a condition.
sonaranalyzer-dotnet/src/SonarAnalyzer.Common/Rules/Hotspots/ControllingPermissionsBase.cs
Show resolved
Hide resolved
valhristov
force-pushed
the
val/permissions
branch
from
b31502c
to
cceceed
Compare
There was a problem hiding this comment.
SonarQube analysis found issues:
Including the following issue(s) which could not be reported in line:
There was a problem hiding this comment.
SonarQube analysis found issues:
Including the following issue(s) which could not be reported in line:
Fix #1992