Rule S4790: Hashing data is security-sensitive #2084
Conversation
ghost
assigned
|
|
||
| namespace SonarAnalyzer.Helpers | ||
| { | ||
| public class CSharpBaseTypeTracker : BaseTypeTracker<SyntaxKind> |
There was a problem hiding this comment.
Syntax-level tracker that starts from the base list of inherited/implemented methods types.
| CSharp.GeneratedCodeRecognizer.Instance; | ||
|
|
||
| protected override SyntaxKind[] TrackedSyntaxKinds { get; } = | ||
| new[] { SyntaxKind.ClassDeclaration }; |
There was a problem hiding this comment.
Symbol-level tracker.
This isn't used by the rule I just added. We can delete it later if we don't use it for any of the other rules.
![sonarsource-next[bot]](https://avatars.githubusercontent.com/in/11855?s=60&v=4){kind=small}
|
|
||
| bool IsTrackedRelationship(SyntaxNode objectCreationExpression, SemanticModel semanticModel, out Location issueLocation) | ||
| { | ||
| var baseClassContext = CreateContext(objectCreationExpression, semanticModel); |
There was a problem hiding this comment.
This tracker is slightly different from the others - the syntax node being analyzed is the base list that contains the list of types being derived from or inherited. CreateContext is an abstract method as it's the responsibility of the language-specific child class to extract the type nodes from the list.
There was a problem hiding this comment.
This does not feel nice, but I understand why you did it like this... Perhaps in the future we could split the Tracker into Tracker and Reporter...
There was a problem hiding this comment.
Splitting out the reporting would certainly give us more flexibility. Something to keep in mind.
| /// Checker method called by <see cref="BaseClassTracker"/> to check whether | ||
| /// an issue should be reported because of a type the class is inheriting from. | ||
| /// </summary> | ||
| public delegate bool BaseClassCondition(BaseTypeContext context, out Location issueLocation); |
There was a problem hiding this comment.
This delegate has an out parameter so the condition can specify where the squiggly should appear.
| public override InvocationCondition WhenMethodNameIs(string methodName) => | ||
| (context) => context.Identifier is SimpleNameSyntax nameSyntax | ||
| && nameSyntax.Identifier.ValueText == methodName; | ||
|
|
There was a problem hiding this comment.
Just checking the method name and not the type here (the hash rule is looking for any method called Create that returns a specific type).
| .WithNotConfigurable(); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = | ||
| ImmutableArray.Create(rule); |
There was a problem hiding this comment.
Unrelated: we could probably push the rule and SupportedDiagnostics up to the base class, so this child class only needs to specify the language-specific resources to use.
There was a problem hiding this comment.
Whisper that to @Evangelink and he will do it in 5min for all rules :)
There was a problem hiding this comment.
Oh good, more merge conflicts to deal with ;)
| InvocationTracker.WhenReturnTypeIs(KnownType.System_Security_Cryptography_HashAlgorithm) | ||
| ); | ||
|
|
||
| BaseClassTracker.Track(context, |
There was a problem hiding this comment.
I'll rename this BaseTypeTracker to match the tracker class type name (I renamed the tracker and forgot to rename these fields).
duncanp-sonar
force-pushed
the
duncanp/hash4790
branch
from
23a3ffe
to
54c64af
Compare
|
Fixes #1984 |
| .WithNotConfigurable(); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = | ||
| ImmutableArray.Create(rule); |
There was a problem hiding this comment.
Whisper that to @Evangelink and he will do it in 5min for all rules :)
|
|
||
| bool IsTrackedRelationship(SyntaxNode objectCreationExpression, SemanticModel semanticModel, out Location issueLocation) | ||
| { | ||
| var baseClassContext = CreateContext(objectCreationExpression, semanticModel); |
There was a problem hiding this comment.
This does not feel nice, but I understand why you did it like this... Perhaps in the future we could split the Tracker into Tracker and Reporter...
| // If a class both inherits and implements then this tracker will check | ||
| // the conditions against Inherits and Implements *separately* | ||
| // i.e. the conditions will be called twice | ||
| switch (contextNode) |
There was a problem hiding this comment.
This could become switch (contextNode.Kind()) and then cast (for perf reasons)
| protected override BaseTypeContext CreateContext(SyntaxNode baseTypeList, SemanticModel semanticModel) => | ||
| new BaseTypeContext(baseTypeList, GetBaseTypeNodes(baseTypeList), semanticModel); | ||
|
|
||
| private static IEnumerable<SyntaxNode> GetBaseTypeNodes(SyntaxNode contextNode) |
There was a problem hiding this comment.
Why not abstract GetBaseTypeNodes and call it in the BaseTypeTracker, instead of abstract CreateContext? It kind of shows the intent better...
| (context) => | ||
| context.InvokedConstructorSymbol.Value != null && | ||
| context.InvokedConstructorSymbol.Value.IsConstructor() && | ||
| context.InvokedConstructorSymbol.Value.ContainingType.ConstructedFrom.DerivesFrom(baseType); |
There was a problem hiding this comment.
I am not sure if ConstructedFrom is needed here, but I might be wrong.
duncanp-sonar
force-pushed
the
duncanp/hash4790
branch
from
54c64af
to
e3660f1
Compare
Fix #1984