diff --git a/.circleci/config.yml b/.circleci/config.yml index f16f0b4..44ac4c0 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,8 +1,8 @@ version: 2.1 orbs: - codacy: codacy/base@11.1.1 - codacy_plugins_test: codacy/plugins-test@2.0.9 + codacy: codacy/base@12.1.3 + codacy_plugins_test: codacy/plugins-test@2.0.11 workflows: build-and-deploy: diff --git a/docs/description/S2053.md b/docs/description/S2053.md index 096766d..09eb0a9 100644 --- a/docs/description/S2053.md +++ b/docs/description/S2053.md @@ -67,7 +67,7 @@ The following code contains examples of hard-coded salts. public static void hash(string password) { - var hashed = new Rfc2898DeriveBytes(password, 16); + var hashed = new Rfc2898DeriveBytes(password, 32, 10000, HashAlgorithmName.SHA256); } ### How does this work? diff --git a/docs/description/S2245.md b/docs/description/S2245.md index 28d4d3b..4285c4e 100644 --- a/docs/description/S2245.md +++ b/docs/description/S2245.md @@ -49,8 +49,7 @@ There is a risk if you answered yes to any of those questions. - OWASP - [Top 10 2021 Category A2 - Cryptographic Failures](https://owasp.org/Top10/A02_2021-Cryptographic_Failures/) - OWASP - [Top 10 2017 Category A3 - Sensitive Data Exposure](https://owasp.org/www-project-top-ten/2017/A3_2017-Sensitive_Data_Exposure) -- [Mobile AppSec - Verification Standard - Cryptography Requirements](https://mobile-security.gitbook.io/masvs/security-requirements/0x08-v3-cryptography_verification_requirements) +- [Mobile AppSec Verification Standard - Cryptography Requirements](https://mas.owasp.org/checklists/MASVS-CRYPTO/) - OWASP - [Mobile Top 10 2016 Category M5 - Insufficient Cryptography](https://owasp.org/www-project-mobile-top-10/2016-risks/m5-insufficient-cryptography) - CWE - [CWE-338 - Use of Cryptographically Weak Pseudo-Random Number Generator (PRNG)](https://cwe.mitre.org/data/definitions/338) diff --git a/docs/description/S2387.md b/docs/description/S2387.md index 0017a0e..60b372f 100644 --- a/docs/description/S2387.md +++ b/docs/description/S2387.md @@ -1,3 +1,5 @@ +This rule is deprecated, and will eventually be removed. + ## Why is this an issue? Having a variable with the same name in two unrelated classes is fine, but do the same thing within a class hierarchy and you’ll get confusion at diff --git a/docs/description/S2674.md b/docs/description/S2674.md index 488ffbe..24776c7 100644 --- a/docs/description/S2674.md +++ b/docs/description/S2674.md @@ -1,39 +1,60 @@ ## Why is this an issue? -You cannot assume that any given stream reading call will fill the `byte[]` passed in to the method with the number of bytes requested. -Instead, you must check the value returned by the read method to see how many bytes were read. Fail to do so, and you introduce a bug that is both -harmful and difficult to reproduce. +Invoking a stream reading method without verifying the number of bytes read can lead to erroneous assumptions. A Stream can represent any I/O +operation, such as reading a file, network communication, or inter-process communication. As such, it is not guaranteed that the `byte[]` +passed into the method will be filled with the requested number of bytes. Therefore, inspecting the value returned by the reading method is important +to ensure the number of bytes read. -This rule raises an issue when a `Stream.Read` or a `Stream.ReadAsync` method is called, but the return value is not -checked. +Neglecting the returned length read can result in a bug that is difficult to reproduce. -### Noncompliant code example +This rule raises an issue when the returned value is ignored for the following methods: - public void DoSomething(string fileName) +- [Stream.Read](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.read) +- [Stream.ReadAsync](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readasync) +- [Stream.ReadAtLeast](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readatleast) +- [Stream.ReadAtLeastAsync](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readatleastasync) + +## How to fix it + +Check the return value of stream reading methods to verify the actual number of bytes read, and use this value when processing the data to avoid +potential bugs. + +### Code examples + +#### Noncompliant code example + + public byte[] ReadFile(string fileName) { - using (var stream = File.Open(fileName, FileMode.Open)) - { + using var stream = File.Open(fileName, FileMode.Open); var result = new byte[stream.Length]; + stream.Read(result, 0, (int)stream.Length); // Noncompliant - // ... do something with result - } + + return result; } -### Compliant solution +#### Compliant solution - public void DoSomething(string fileName) + public byte[] ReadFile(string fileName) { - using (var stream = File.Open(fileName, FileMode.Open)) - { + using var stream = File.Open(fileName, FileMode.Open); + using var ms = new MemoryStream(); var buffer = new byte[1024]; - using (var ms = new MemoryStream()) + int read; + + while ((read = stream.Read(buffer, 0, buffer.Length)) > 0) { - int read; - while ((read = stream.Read(buffer, 0, buffer.Length)) > 0) - { - ms.Write(buffer, 0, read); - } - // ... do something with ms + ms.Write(buffer, 0, read); } - } - } \ No newline at end of file + + return ms.ToArray(); + } + +## Resources + +### Documentation + +- Microsoft Learn - [Stream.Read Method](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.read) +- Microsoft Learn - [Stream.ReadAsync Method](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readasync) +- Microsoft Learn - [Stream.ReadAtLeast Method](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readatleast) +- Microsoft Learn - [Stream.ReadAtLeastAsync Method](https://learn.microsoft.com/en-us/dotnet/api/system.io.stream.readatleastasync) \ No newline at end of file diff --git a/docs/description/S3168.md b/docs/description/S3168.md index e0e38dd..3620cda 100644 --- a/docs/description/S3168.md +++ b/docs/description/S3168.md @@ -59,7 +59,7 @@ Update the return type of the method from `void` to `Task`. throw new InvalidOperationException(); } - public void Method() + public async Task Method() { try { diff --git a/docs/description/S3431.md b/docs/description/S3431.md index 17d3368..66cdf7f 100644 --- a/docs/description/S3431.md +++ b/docs/description/S3431.md @@ -5,40 +5,101 @@ the `ExpectedException` attribute since an exception could be thrown from almost This rule detects MSTest and NUnit `ExpectedException` attribute. -### Noncompliant code example +### Exceptions + +This rule ignores: + +- single-line tests, since it is obvious in such methods where the exception is expected to be thrown +- tests when it tests control flow and assertion are present in either a `catch` or `finally` clause [TestMethod] - [ExpectedException(typeof(ArgumentNullException))] // Noncompliant - public void TestNullArg() + [ExpectedException(typeof(InvalidOperationException))] + public void UsingTest() + { + Console.ForegroundColor = ConsoleColor.Black; + try + { + using var _ = new ConsoleAlert(); + Assert.AreEqual(ConsoleColor.Red, Console.ForegroundColor); + throw new InvalidOperationException(); + } + finally + { + Assert.AreEqual(ConsoleColor.Black, Console.ForegroundColor); // The exception itself is not relevant for the test. + } + } + + public sealed class ConsoleAlert : IDisposable { - //... + private readonly ConsoleColor previous; + + public ConsoleAlert() + { + previous = Console.ForegroundColor; + Console.ForegroundColor = ConsoleColor.Red; + } + + public void Dispose() => + Console.ForegroundColor = previous; } -### Compliant solution +## How to fix it in MSTest + +Remove the `ExpectedException` attribute in favor of using the [Assert.ThrowsException](https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.assert.throwsexception) +assertion. + +### Code examples + +#### Noncompliant code example [TestMethod] - public void TestNullArg() + [ExpectedException(typeof(ArgumentNullException))] // Noncompliant + public void Method_NullParam() { - bool callFailed = false; - try - { - //... - } - catch (ArgumentNullException) - { - callFailed = true; - } - Assert.IsTrue(callFailed, "Expected call to MyMethod to fail with ArgumentNullException"); + var sut = new MyService(); + sut.Method(null); } -or +#### Compliant solution [TestMethod] - public void TestNullArg() + public void Method_NullParam() { - Assert.ThrowsException(() => /*...*/); + var sut = new MyService(); + Assert.ThrowsException(() => sut.Method(null)); } -### Exceptions +## How to fix it in NUnit + +Remove the `ExpectedException` attribute in favor of using the [Assert.Throws](https://docs.nunit.org/articles/nunit/writing-tests/assertions/classic-assertions/Assert.Throws.html) assertion. + +### Code examples + +#### Noncompliant code example + + [Test] + [ExpectedException(typeof(ArgumentNullException))] // Noncompliant + public void Method_NullParam() + { + var sut = new MyService(); + sut.Method(null); + } + +#### Compliant solution + + [Test] + public void Method_NullParam() + { + var sut = new MyService(); + Assert.Throws(() => sut.Method(null)); + } + +## Resources + +### Documentation -This rule ignores one-line test methods, since it is obvious in such methods where the exception is expected to be thrown. \ No newline at end of file +- Microsoft Learn - [Assert.ThrowsException + Method](https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.assert.throwsexception) +- Microsoft Learn - [ExpectedExceptionAttribute Class](https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.testtools.unittesting.expectedexceptionattribute) +- NUnit - [Assert.Throws](https://docs.nunit.org/articles/nunit/writing-tests/assertions/classic-assertions/Assert.Throws.html) +- NUnit - [ExpectedExceptionAttribute](https://docs.nunit.org/2.4/exception.html) \ No newline at end of file diff --git a/docs/description/S3993.md b/docs/description/S3993.md index bab297c..8bf26ba 100644 --- a/docs/description/S3993.md +++ b/docs/description/S3993.md @@ -1,55 +1,49 @@ ## Why is this an issue? -When defining custom attributes, `System.AttributeUsageAttribute` must be used to indicate where the attribute can be applied. This will -determine its valid locations in the code. +When defining custom attributes, [AttributeUsageAttribute](https://learn.microsoft.com/en-us/dotnet/api/system.attributeusageattribute) +must be used to indicate where the attribute can be applied. This will: -### Noncompliant code example +- indicate how the attribute can be used +- prevent it from being used at invalid locations - using System; - - namespace MyLibrary +## How to fix it + +### Code examples + +#### Noncompliant code example + + public sealed class MyAttribute : Attribute // Noncompliant - AttributeUsage is missing { + private string text; - public sealed class MyAttribute :Attribute // Noncompliant - { - string text; + public MyAttribute(string text) + { + this.text = text; + } - public MyAttribute(string myText) - { - text = myText; - } - public string Text - { - get - { - return text; - } - } - } + public string Text => text; } -### Compliant solution +#### Compliant solution - using System; - - namespace MyLibrary + [AttributeUsage(AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Delegate)] + public sealed class MyAttribute : Attribute { + private string text; - [AttributeUsage(AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Interface | AttributeTargets.Delegate)] - public sealed class MyAttribute :Attribute - { - string text; + public MyAttribute(string text) + { + this.text = text; + } - public MyAttribute(string myText) - { - text = myText; - } - public string Text - { - get - { - return text; - } - } - } - } \ No newline at end of file + public string Text => text; + } + +## Resources + +### Documentation + +- Microsoft Learn - [Create custom + attributes](https://learn.microsoft.com/en-us/dotnet/csharp/advanced-topics/reflection-and-attributes/creating-custom-attributes) +- Microsoft Learn - [AttributeUsageAttribute class](https://learn.microsoft.com/en-us/dotnet/api/system.attributeusageattribute) +- Microsoft Learn - [Attribute class](https://learn.microsoft.com/en-us/dotnet/api/system.attribute) \ No newline at end of file diff --git a/docs/description/S4050.md b/docs/description/S4050.md index 2e5ca44..76134d0 100644 --- a/docs/description/S4050.md +++ b/docs/description/S4050.md @@ -1,53 +1,50 @@ ## Why is this an issue? -When implementing operator overloads, it is very important to make sure that all related operators and methods are consistent in their -implementation. +When overloading some arithmetic operator overloads, it is very important to make sure that all related operators and methods are consistent in +their implementation. The following guidelines should be followed: -- When providing `operator ==` you should also provide `operator !=` and vice-versa. -- When providing `operator ==` you should also provide `Equals(Object)` and `GetHashCode()`. -- When providing `operator +, -, *, / or %` you should also provide `operator ==`, respecting previous guidelines. +- When providing `operator ==, !=` you should also provide `Equals(Object)` and `GetHashCode()`. +- When providing `operator +, -, *, / or %` you should also provide `operator ==`, respecting the previous guideline. -This rule raises an issue when any of these guidelines are not followed on publicly-visible type (public, protected or protected internal). +This rule raises an issue when any of these guidelines are not followed on a publicly-visible class or struct (`public`, +`protected` or `protected internal`). -### Noncompliant code example +## How to fix it - using System; - - namespace MyLibrary +Make sure to implement all related operators. + +### Code examples + +#### Noncompliant code example + + public class Foo // Noncompliant { - public class Foo // Noncompliant - { private int left; private int right; public Foo(int l, int r) { - this.left = l; - this.right = r; + this.left = l; + this.right = r; } public static Foo operator +(Foo a, Foo b) { - return new Foo(a.left + b.left, a.right + b.right); + return new Foo(a.left + b.left, a.right + b.right); } public static Foo operator -(Foo a, Foo b) { - return new Foo(a.left - b.left, a.right - b.right); + return new Foo(a.left - b.left, a.right - b.right); } - } } -### Compliant solution +#### Compliant solution - using System; - - namespace MyLibrary + public class Foo { - public class Foo - { private int left; private int right; @@ -57,37 +54,47 @@ This rule raises an issue when any of these guidelines are not followed on publi this.right = r; } - public static Foo operator +(Foo a, Foo b) + public override bool Equals(Object obj) { - return new Foo(a.left + b.left, a.right + b.right); + var a = obj as Foo; + if (a == null) + return false; + return this == a; } - public static Foo operator -(Foo a, Foo b) + public override int GetHashCode() { - return new Foo(a.left - b.left, a.right - b.right); + return HashCode.Combine(right, left); } - public static bool operator ==(Foo a, Foo b) + public static Foo operator +(Foo a, Foo b) { - return (a.left == b.left && a.right == b.right); + return new Foo(a.left + b.left, a.right + b.right); } - public static bool operator !=(Foo a, Foo b) + public static Foo operator -(Foo a, Foo b) { - return !(a == b); + return new Foo(a.left - b.left, a.right - b.right); } - public override bool Equals(Object obj) + public static bool operator ==(Foo a, Foo b) { - Foo a = obj as Foo; - if (a == null) - return false; - return this == a; + return a.left == b.left && a.right == b.right; } - public override int GetHashCode() + public static bool operator !=(Foo a, Foo b) { - return (this.left * 10) + this.right; + return !(a == b); } - } - } \ No newline at end of file + } + +## Resources + +### Documentation + +- Microsoft Learn - [Operator + overloading](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/operator-overloading) +- Microsoft Learn - [Equality + operators](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/equality-operators) +- Microsoft Learn - [Arithmetic + operators (C# reference)](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/arithmetic-operators) \ No newline at end of file diff --git a/docs/description/S4052.md b/docs/description/S4052.md index 487603d..cad1191 100644 --- a/docs/description/S4052.md +++ b/docs/description/S4052.md @@ -1,23 +1,25 @@ ## Why is this an issue? -With the advent of .NET framework version 2, certain practices have become obsolete. +With the advent of .NET Framework 2.0, certain practices and types have become obsolete. In particular, exceptions should now extend `System.Exception` instead of `System.ApplicationException`. Similarly, generic collections should be used instead of the older, non-generic, ones. Finally when creating an XML view, you should not extend -`System.Xml.XmlDocument`. +`System.Xml.XmlDocument`. This rule raises an issue when an externally visible type extends one of these types: -This rule raises an issue when an externally visible type extends one of these types: +- [System.ApplicationException](https://learn.microsoft.com/en-us/dotnet/api/system.applicationexception) +- [System.Xml.XmlDocument](https://learn.microsoft.com/en-us/dotnet/api/system.xml.xmldocument) +- [System.Collections.CollectionBase](https://learn.microsoft.com/en-us/dotnet/api/system.collections.collectionbase) +- [System.Collections.DictionaryBase](https://learn.microsoft.com/en-us/dotnet/api/system.collections.dictionarybase) +- [System.Collections.Queue](https://learn.microsoft.com/en-us/dotnet/api/system.collections.queue) +- [System.Collections.ReadOnlyCollectionBase](https://learn.microsoft.com/en-us/dotnet/api/system.collections.readonlycollectionbase) +- [System.Collections.SortedList](https://learn.microsoft.com/en-us/dotnet/api/system.collections.sortedlist) +- [System.Collections.Stack](https://learn.microsoft.com/en-us/dotnet/api/system.collections.stack) -- `System.ApplicationException` -- `System.Xml.XmlDocument` -- `System.Collections.CollectionBase` -- `System.Collections.DictionaryBase` -- `System.Collections.Queue` -- `System.Collections.ReadOnlyCollectionBase` -- `System.Collections.SortedList` -- `System.Collections.Stack` +## How to fix it -### Noncompliant code example +### Code examples + +#### Noncompliant code example using System; using System.Collections; @@ -29,10 +31,10 @@ This rule raises an issue when an externally visible type extends one of these t } } -### Compliant solution +#### Compliant solution using System; - using System.Collections; + using System.Collections.ObjectModel; namespace MyLibrary { diff --git a/docs/description/S4426.md b/docs/description/S4426.md index 97dd231..8ceafc1 100644 --- a/docs/description/S4426.md +++ b/docs/description/S4426.md @@ -223,8 +223,7 @@ Thus, if data is to remain secure beyond 2030, proactive measures should be take Exposure](https://www.owasp.org/www-project-top-ten/2017/A3_2017-Sensitive_Data_Exposure) - OWASP - [Top 10 2017 Category A6 - Security Misconfiguration](https://owasp.org/www-project-top-ten/2017/A6_2017-Security_Misconfiguration) -- OWASP - [Mobile AppSec - Verification Standard - Cryptography Requirements](https://mobile-security.gitbook.io/masvs/security-requirements/0x08-v3-cryptography_verification_requirements) +- OWASP - [Mobile AppSec Verification Standard - Cryptography Requirements](https://mas.owasp.org/checklists/MASVS-CRYPTO/) - OWASP - [Mobile Top 10 2016 Category M5 - Insufficient Cryptography](https://owasp.org/www-project-mobile-top-10/2016-risks/m5-insufficient-cryptography) - [NIST 800-131A](https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf) - Recommendation for Transitioning the diff --git a/docs/description/S4790.md b/docs/description/S4790.md index 28de0da..c653fdc 100644 --- a/docs/description/S4790.md +++ b/docs/description/S4790.md @@ -39,8 +39,7 @@ because it slows down `brute force attacks`. Exposure](https://owasp.org/www-project-top-ten/2017/A3_2017-Sensitive_Data_Exposure) - OWASP - [Top 10 2017 Category A6 - Security Misconfiguration](https://owasp.org/www-project-top-ten/2017/A6_2017-Security_Misconfiguration) -- OWASP - [Mobile AppSec - Verification Standard - Cryptography Requirements](https://mobile-security.gitbook.io/masvs/security-requirements/0x08-v3-cryptography_verification_requirements) +- OWASP - [Mobile AppSec Verification Standard - Cryptography Requirements](https://mas.owasp.org/checklists/MASVS-CRYPTO/) - OWASP - [Mobile Top 10 2016 Category M5 - Insufficient Cryptography](https://owasp.org/www-project-mobile-top-10/2016-risks/m5-insufficient-cryptography) - CWE - [CWE-1240 - Use of a Risky Cryptographic Primitive](https://cwe.mitre.org/data/definitions/1240) \ No newline at end of file diff --git a/docs/description/S4830.md b/docs/description/S4830.md index 4408fd6..a45798b 100644 --- a/docs/description/S4830.md +++ b/docs/description/S4830.md @@ -81,8 +81,7 @@ roots. Rather than disabling certificate validation in your code, you can add th Misconfiguration](https://owasp.org/www-project-top-ten/2017/A6_2017-Security_Misconfiguration) - OWASP - [Mobile Top 10 2016 Category M3 - Insecure Communication](https://owasp.org/www-project-mobile-top-10/2016-risks/m3-insecure-communication) -- OWASP - [Mobile AppSec - Verification Standard - Network Communication Requirements](https://mobile-security.gitbook.io/masvs/security-requirements/0x10-v5-network_communication_requirements) +- OWASP - [Mobile AppSec Verification Standard - Network Communication Requirements](https://mas.owasp.org/checklists/MASVS-NETWORK/) - CWE - [CWE-295 - Improper Certificate Validation](https://cwe.mitre.org/data/definitions/295) - STIG Viewer - [Application Security and Development: V-222550](https://stigviewer.com/stig/application_security_and_development/2023-06-08/finding/V-222550) - The application must validate certificates by constructing a certification path to an accepted trust anchor. \ No newline at end of file diff --git a/docs/description/S5332.md b/docs/description/S5332.md index 6c8a56e..592bedf 100644 --- a/docs/description/S5332.md +++ b/docs/description/S5332.md @@ -96,8 +96,7 @@ No issue is reported for the following cases because they are not considered sen - OWASP - [Top 10 2017 Category A3 - Sensitive Data Exposure](https://owasp.org/www-project-top-ten/2017/A3_2017-Sensitive_Data_Exposure) - OWASP - [Top 10 2021 Category A2 - Cryptographic Failures](https://owasp.org/Top10/A02_2021-Cryptographic_Failures/) -- OWASP - [Mobile AppSec - Verification Standard - Network Communication Requirements](https://mobile-security.gitbook.io/masvs/security-requirements/0x10-v5-network_communication_requirements) +- OWASP - [Mobile AppSec Verification Standard - Network Communication Requirements](https://mas.owasp.org/checklists/MASVS-NETWORK/) - OWASP - [Mobile Top 10 2016 Category M3 - Insecure Communication](https://owasp.org/www-project-mobile-top-10/2016-risks/m3-insecure-communication) - CWE - [CWE-200 - Exposure of Sensitive Information to an Unauthorized Actor](https://cwe.mitre.org/data/definitions/200) diff --git a/docs/patterns.json b/docs/patterns.json index f709cc7..6a09eee 100644 --- a/docs/patterns.json +++ b/docs/patterns.json @@ -1,6 +1,6 @@ { "name": "sonarcsharp", - "version": "9.30", + "version": "9.32", "patterns": [ { "patternId": "S100", diff --git a/src/Analyzer/Analyzer.csproj b/src/Analyzer/Analyzer.csproj index a037fcb..63b98b2 100644 --- a/src/Analyzer/Analyzer.csproj +++ b/src/Analyzer/Analyzer.csproj @@ -5,9 +5,9 @@ CodacyCSharp.Analyzer - - - + + + diff --git a/src/DocsGenerator/DocsGenerator.csproj b/src/DocsGenerator/DocsGenerator.csproj index 73e038e..6e0bc56 100644 --- a/src/DocsGenerator/DocsGenerator.csproj +++ b/src/DocsGenerator/DocsGenerator.csproj @@ -5,7 +5,7 @@ CodacyCSharp.DocsGenerator - +