Rule S6423: Always log failures in Azure Functions

analyzers/rspec/cs/S6423_c#.html

@@ -0,0 +1,47 @@
+<p>Capturing and logging errors is critical to monitoring the health of your Azure Functions application.</p>
+<p>Each <code>catch</code> block inside an Azure Function should log helpful details about the failure. Moreover, the logging should not be done at
+<code>Debug</code> or <code>Trace</code> level.</p>
+<p>Consider using the built-in integration with Application Insights for better monitoring of your Application.</p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+[FunctionName("Foo")]
+public static async Task&lt;IActionResult&gt; Run(
+	[HttpTrigger(AuthorizationLevel.Anonymous, "get", "post", Route = null)] HttpRequest req,
+	ILogger log)
+{
+	try
+	{
+		// do stuff that can fail
+	}
+	catch (Exception ex)
+	{
+		// the failure is not logged at all OR is logged at DEBUG/TRACE level
+	}
+}
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+[FunctionName("Foo")]
+public static async Task&lt;IActionResult&gt; Run(
+	[HttpTrigger(AuthorizationLevel.Anonymous, "get", "post", Route = null)] HttpRequest req,
+	ILogger log)
+{
+	try
+	{
+		// do stuff that can fail
+	}
+	catch (Exception ex)
+	{
+		log.LogError("Give details that will help investigations", ex);
+	}
+}
+</pre>
+<h2>See</h2>
+<ul>
+  <li> <a href="https://docs.microsoft.com/en-us/azure/azure-functions/functions-bindings-error-pages?tabs=csharp">Azure Functions error handling and
+  retries</a> </li>
+  <li> <a href="https://docs.microsoft.com/en-us/azure/azure-functions/functions-monitoring">Monitor Azure Functions</a> </li>
+  <li> <a href="https://docs.microsoft.com/en-us/azure/azure-monitor/app/azure-functions-supported-features">Application Insights for Azure Functions
+  supported features</a> </li>
+</ul>
+

analyzers/rspec/cs/S6423_c#.json

@@ -0,0 +1,17 @@
+{
+  "title": "Azure Functions should log all failures.",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "5min"
+  },
+  "tags": [
+    "error-handling"
+  ],
+  "defaultSeverity": "Major",
+  "ruleSpecification": "RSPEC-6423",
+  "sqKey": "S6423",
+  "scope": "Main",
+  "quickfix": "unknown"
+}

analyzers/src/SonarAnalyzer.CSharp/RspecStrings.resx

@@ -12844,6 +12844,39 @@
   <data name="S6354_Type" xml:space="preserve">
     <value>CODE_SMELL</value>
   </data>
+  <data name="S6423_Category" xml:space="preserve">
+    <value>Major Code Smell</value>
+  </data>
+  <data name="S6423_Description" xml:space="preserve">
+    <value>Capturing and logging errors is critical to monitoring the health of your Azure Functions application.</value>
+  </data>
+  <data name="S6423_IsActivatedByDefault" xml:space="preserve">
+    <value>False</value>
+  </data>
+  <data name="S6423_Remediation" xml:space="preserve">
+    <value>Constant/Issue</value>
+  </data>
+  <data name="S6423_RemediationCost" xml:space="preserve">
+    <value>5min</value>
+  </data>
+  <data name="S6423_Scope" xml:space="preserve">
+    <value>Main</value>
+  </data>
+  <data name="S6423_Severity" xml:space="preserve">
+    <value>Major</value>
+  </data>
+  <data name="S6423_Status" xml:space="preserve">
+    <value>ready</value>
+  </data>
+  <data name="S6423_Tags" xml:space="preserve">
+    <value>error-handling</value>
+  </data>
+  <data name="S6423_Title" xml:space="preserve">
+    <value>Azure Functions should log all failures.</value>
+  </data>
+  <data name="S6423_Type" xml:space="preserve">
+    <value>CODE_SMELL</value>
+  </data>
   <data name="S818_Category" xml:space="preserve">
     <value>Minor Code Smell</value>
   </data>

analyzers/src/SonarAnalyzer.CSharp/Rules/CloudNative/AzureFunctionsLogFailures.cs

@@ -0,0 +1,172 @@
+/*
+ * SonarAnalyzer for .NET
+ * Copyright (C) 2015-2022 SonarSource SA
+ * 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.Collections.Generic;
+using System.Collections.Immutable;
+using System.Linq;
+using System.Threading;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+using SonarAnalyzer.Extensions;
+using SonarAnalyzer.Helpers;
+
+namespace SonarAnalyzer.Rules.CSharp
+{
+    [DiagnosticAnalyzer(LanguageNames.CSharp)]
+    public sealed class AzureFunctionsLogFailures : SonarDiagnosticAnalyzer
+    {
+        private const string DiagnosticId = "S6423";
+        private const string MessageFormat = "Log exception via ILogger with LogLevel Information, Warning, Error, or Critical.";
+
+        private static readonly int[] InvalidLogLevel =
+        {
+            0, // Trace
+            1, // Debug
+            6, // None
+        };
+
+        private static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager);
+
+        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);
+
+        protected override void Initialize(SonarAnalysisContext context) =>
+            context.RegisterSyntaxNodeActionInNonGenerated(c =>
+            {
+                var catchClause = (CatchClauseSyntax)c.Node;
+                if (c.AzureFunctionMethod() is { } entryPoint
+                    && HasLoggerInScope(entryPoint))
+                {
+                    var walker = new LoggerCallWalker(c.SemanticModel, c.CancellationToken);
+                    walker.SafeVisit(catchClause.Block);
+                    // Exception handling in the filter clause preserves log scopes and is therefore recommended
+                    // See https://blog.stephencleary.com/2020/06/a-new-pattern-for-exception-logging.html
+                    walker.SafeVisit(catchClause.Filter?.FilterExpression);
+                    if (!walker.HasValidLoggerCall)
+                    {
+                        c.ReportIssue(Diagnostic.Create(Rule, catchClause.CatchKeyword.GetLocation(), walker.InvalidLoggerInvocationLocations));
+                    }
+                }
+            },
+            SyntaxKind.CatchClause);
+
+        private static bool HasLoggerInScope(IMethodSymbol entryPoint) =>
+            entryPoint.Parameters.Any(x => x.Type.DerivesOrImplements(KnownType.Microsoft_Extensions_Logging_ILogger))
+                // Instance method entry points might have access to an ILogger via injected fields/properties
+                // https://docs.microsoft.com/en-us/azure/azure-functions/functions-dotnet-dependency-injection
+                || (entryPoint is { IsStatic: false, ContainingType: { } container }
+                    && HasLoggerMember(container));
+
+        internal static bool HasLoggerMember(ITypeSymbol typeSymbol)
+        {
+            var isOriginalType = true;
+            foreach (var type in typeSymbol.GetSelfAndBaseTypes())
+            {
+                if (type.GetMembers().Any(x => x.Kind is SymbolKind.Field or SymbolKind.Property
+                                               && (isOriginalType || x.GetEffectiveAccessibility() != Accessibility.Private)
+                                               && x.GetSymbolType()?.DerivesOrImplements(KnownType.Microsoft_Extensions_Logging_ILogger) is true))
+                {
+                    return true;
+                }
+                isOriginalType = false;
+            }
+            return false;
+        }
+
+        private sealed class LoggerCallWalker : SafeCSharpSyntaxWalker
+        {
+            private readonly SemanticModel model;
+            private readonly CancellationToken cancellationToken;
+            private List<Location> invalidInvocations;
+
+            public bool HasValidLoggerCall { get; private set; }
+            public IEnumerable<Location> InvalidLoggerInvocationLocations => invalidInvocations;
+
+            public LoggerCallWalker(SemanticModel model, CancellationToken cancellationToken)
+            {
+                this.model = model;
+                this.cancellationToken = cancellationToken;
+            }
+
+            public override void Visit(SyntaxNode node)
+            {
+                if (!HasValidLoggerCall)
+                {
+                    cancellationToken.ThrowIfCancellationRequested();
+                    base.Visit(node);
+                }
+            }
+
+            public override void VisitInvocationExpression(InvocationExpressionSyntax node)
+            {
+                if (model.GetSymbolInfo(node, cancellationToken).Symbol is IMethodSymbol { ReceiverType: { } receiver } methodSymbol
+                    && receiver.DerivesOrImplements(KnownType.Microsoft_Extensions_Logging_ILogger))
+                {
+                    if (IsValidLogCall(node, methodSymbol))
+                    {
+                        HasValidLoggerCall = true;
+                    }
+                    else
+                    {
+                        invalidInvocations ??= new();
+                        invalidInvocations.Add(node.GetLocation());
+                    }
+                }
+                base.VisitInvocationExpression(node);
+            }
+
+            public override void VisitArgument(ArgumentSyntax node)
+            {
+                HasValidLoggerCall = HasValidLoggerCall || model.GetTypeInfo(node.Expression, cancellationToken).Type?.DerivesOrImplements(KnownType.Microsoft_Extensions_Logging_ILogger) is true;
+                base.VisitArgument(node);
+            }
+
+            private bool IsValidLogCall(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol) =>
+                methodSymbol switch
+                {
+                    // Wellknown LoggerExtensions methods invocations
+                    { IsExtensionMethod: true } when methodSymbol.ContainingType.Is(KnownType.Microsoft_Extensions_Logging_LoggerExtensions) =>
+                        methodSymbol.Name switch
+                        {
+                            "LogInformation" or "LogWarning" or "LogError" or "LogCritical" => true,
+                            "Log" => IsPassingValidLogLevel(invocation, methodSymbol),
+                            "LogTrace" or "LogDebug" or "BeginScope" => false,
+                            _ => true, // Some unknown extension method on LoggerExtensions was called. Avoid FPs and assume it logs something.
+                        },
+                    { IsExtensionMethod: true } => true, // Any other extension method is assumed to log something to avoid FP.
+                    _ => // Instance invocations on an ILogger instance.
+                        methodSymbol.Name switch
+                        {
+                            "Log" => IsPassingValidLogLevel(invocation, methodSymbol),
+                            "IsEnabled" or "BeginScope" => false,
+                            _ => true, // Some unknown method on an ILogger was called. Avoid FPs and assume it logs something.
+                        },
+                };
+
+            private bool IsPassingValidLogLevel(InvocationExpressionSyntax invocation, IMethodSymbol symbol) =>
+                symbol.Parameters.FirstOrDefault(x => x.Name == "logLevel") is { } logLevelParameter
+                && new CSharpMethodParameterLookup(invocation, symbol).TryGetNonParamsSyntax(logLevelParameter, out var argumentSyntax)
+                && argumentSyntax.FindConstantValue(model) is int logLevel
+                    ? !InvalidLogLevel.Contains(logLevel)
+                    : true; // Compliant: Some non-constant value is passed as loglevel or there is no logLevel parameter
+        }
+    }
+}

analyzers/src/SonarAnalyzer.Common/Extensions/SyntaxNodeAnalysisContextExtensions.cs

@@ -20,6 +20,7 @@
 
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.Diagnostics;
+using SonarAnalyzer.Helpers;
 
 namespace SonarAnalyzer.Extensions
 {
@@ -37,5 +38,10 @@ internal static class SyntaxNodeAnalysisContextExtensions
         /// <seealso href="https://github.com/dotnet/roslyn/issues/50989"/>
         internal static bool IsRedundantPositionalRecordContext(this SyntaxNodeAnalysisContext context) =>
             context.ContainingSymbol.Kind == SymbolKind.Method;
+
+        public static IMethodSymbol AzureFunctionMethod(this SyntaxNodeAnalysisContext context) =>
+            context.ContainingSymbol is IMethodSymbol method && method.HasAttribute(KnownType.Microsoft_Azure_WebJobs_FunctionNameAttribute)
+                ? method
+                : null;
     }
 }

analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs

@@ -66,6 +66,7 @@ internal sealed class KnownType
         internal static readonly KnownType Microsoft_AspNetCore_Mvc_RequestFormLimitsAttribute = new("Microsoft.AspNetCore.Mvc.RequestFormLimitsAttribute");
         internal static readonly KnownType Microsoft_AspNetCore_Mvc_RequestSizeLimitAttribute = new("Microsoft.AspNetCore.Mvc.RequestSizeLimitAttribute");
         internal static readonly KnownType Microsoft_AspNetCore_Razor_Hosting_RazorCompiledItemAttribute = new("Microsoft.AspNetCore.Razor.Hosting.RazorCompiledItemAttribute");
+        internal static readonly KnownType Microsoft_Azure_WebJobs_FunctionNameAttribute = new("Microsoft.Azure.WebJobs.FunctionNameAttribute");
         internal static readonly KnownType Microsoft_Data_Sqlite_SqliteCommand = new("Microsoft.Data.Sqlite.SqliteCommand");
         internal static readonly KnownType Microsoft_EntityFrameworkCore_DbContextOptionsBuilder = new("Microsoft.EntityFrameworkCore.DbContextOptionsBuilder");
         internal static readonly KnownType Microsoft_EntityFrameworkCore_Migrations_Migration = new("Microsoft.EntityFrameworkCore.Migrations.Migration");
@@ -85,7 +86,9 @@ internal sealed class KnownType
         internal static readonly KnownType Microsoft_Extensions_Logging_DebugLoggerFactoryExtensions = new("Microsoft.Extensions.Logging.DebugLoggerFactoryExtensions");
         internal static readonly KnownType Microsoft_Extensions_Logging_EventLoggerFactoryExtensions = new("Microsoft.Extensions.Logging.EventLoggerFactoryExtensions");
         internal static readonly KnownType Microsoft_Extensions_Logging_EventSourceLoggerFactoryExtensions = new("Microsoft.Extensions.Logging.EventSourceLoggerFactoryExtensions");
+        internal static readonly KnownType Microsoft_Extensions_Logging_ILogger = new("Microsoft.Extensions.Logging.ILogger");
         internal static readonly KnownType Microsoft_Extensions_Logging_ILoggerFactory = new("Microsoft.Extensions.Logging.ILoggerFactory");
+        internal static readonly KnownType Microsoft_Extensions_Logging_LoggerExtensions = new("Microsoft.Extensions.Logging.LoggerExtensions");
         internal static readonly KnownType Microsoft_Extensions_Primitives_StringValues = new("Microsoft.Extensions.Primitives.StringValues");
         internal static readonly KnownType Microsoft_Net_Http_Headers_HeaderNames = new("Microsoft.Net.Http.Headers.HeaderNames");
         internal static readonly KnownType Microsoft_VisualBasic_Interaction = new("Microsoft.VisualBasic.Interaction");

analyzers/src/SonarAnalyzer.Common/Helpers/TypeHelper.cs

@@ -188,41 +188,18 @@ public static bool DerivesOrImplements(this ITypeSymbol type, ITypeSymbol baseTy
         public static bool DerivesOrImplementsAny(this ITypeSymbol type, ImmutableArray<KnownType> baseTypes) =>
             type.ImplementsAny(baseTypes) || type.DerivesFromAny(baseTypes);
 
-        public static ITypeSymbol GetSymbolType(this ISymbol symbol)
-        {
-            if (symbol is ILocalSymbol localSymbol)
-            {
-                return localSymbol.Type;
-            }
-
-            if (symbol is IFieldSymbol fieldSymbol)
-            {
-                return fieldSymbol.Type;
-            }
-
-            if (symbol is IPropertySymbol propertySymbol)
-            {
-                return propertySymbol.Type;
-            }
-
-            if (symbol is IParameterSymbol parameterSymbol)
+        public static ITypeSymbol GetSymbolType(this ISymbol symbol) =>
+            symbol switch
             {
-                return parameterSymbol.Type;
-            }
-
-            if (symbol is IAliasSymbol aliasSymbol)
-            {
-                return aliasSymbol.Target as ITypeSymbol;
-            }
-
-            if (symbol is IMethodSymbol methodSymbol)
-            {
-                return methodSymbol.MethodKind == MethodKind.Constructor
-                    ? methodSymbol.ContainingType
-                    : methodSymbol.ReturnType as INamedTypeSymbol;
-            }
-
-            return symbol as ITypeSymbol;
-        }
+                ILocalSymbol x => x.Type,
+                IFieldSymbol x => x.Type,
+                IPropertySymbol x => x.Type,
+                IParameterSymbol x => x.Type,
+                IAliasSymbol x => x.Target as ITypeSymbol,
+                IMethodSymbol { MethodKind: MethodKind.Constructor } x => x.ContainingType,
+                IMethodSymbol x => x.ReturnType,
+                ITypeSymbol x => x,
+                _ => null,
+            };
     }
 }

analyzers/tests/SonarAnalyzer.UnitTest/Helpers/SymbolHelperTest.cs

@@ -32,6 +32,7 @@ public class SymbolHelperTest
 namespace NS
 {
   using System.Collections.Generic;
+  using PropertyBag = System.Collections.Generic.Dictionary<string, object>;
 
   public class Base
   {