From 69f3bcf7537892850d30b84b1c88fc45d44ebcfb Mon Sep 17 00:00:00 2001 From: Gregory Paidis Date: Mon, 26 Feb 2024 17:52:04 +0100 Subject: [PATCH] New rule S6674: Log message template should be syntactically correct --- analyzers/rspec/cs/S6674.html | 46 +++++ analyzers/rspec/cs/S6674.json | 15 ++ analyzers/rspec/cs/Sonar_way_profile.json | 1 + .../MessageTemplateAnalyzer.cs | 2 +- .../Rules/MessageTemplatesShouldBeCorrect.cs | 164 ++++++++++++++++++ .../Helpers/MessageTemplates.cs | 7 +- .../PackagingTests/RuleTypeMappingCS.cs | 2 +- .../MessageTemplatesShouldBeCorrectTest.cs | 114 ++++++++++++ .../MessageTemplatesShouldBeCorrect.cs | 92 ++++++++++ 9 files changed, 439 insertions(+), 4 deletions(-) create mode 100644 analyzers/rspec/cs/S6674.html create mode 100644 analyzers/rspec/cs/S6674.json create mode 100644 analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs create mode 100644 analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs create mode 100644 analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs diff --git a/analyzers/rspec/cs/S6674.html b/analyzers/rspec/cs/S6674.html new file mode 100644 index 00000000000..7cdc825118a --- /dev/null +++ b/analyzers/rspec/cs/S6674.html @@ -0,0 +1,46 @@ +

A message template must conform to the specification. The rule raises an issue if the template string +violates the template string grammar.

+

Why is this an issue?

+

A message template needs to comply with a set of rules. Logging provider parse the template and enrich log entries with +the information found in the template. An unparsable message template leads to corrupted log entries and might result in a loss of information in the +logs.

+

The rule covers the following logging frameworks:

+ +

Code examples

+

Noncompliant code example

+
+logger.LogError("Login failed for {User", user);       // Noncompliant: Missing closing bracket
+logger.LogError("Login failed for {{User}", user);     // Noncompliant: Opening bracket is escaped
+logger.LogError("Login failed for {&User}", user);     // Noncompliant: Only '@' and '$' are allowed as prefix
+logger.LogError("Login failed for {User-Name}", user); // Noncompliant: Only chars, numbers, and underscore are allowed for placeholders
+logger.LogDebug("Retry attempt {Cnt,r}", cnt);         // Noncompliant: The alignment specifier must be numeric
+logger.LogDebug("Retry attempt {Cnt:}", cnt);          // Noncompliant: Empty format specifier is not allowed
+
+

Compliant solution

+
+logger.LogError("Login failed for {User}", user);
+logger.LogError("Login failed for {{{User}}}", user);
+logger.LogError("Login failed for {User}", user);
+logger.LogError("Login failed for {User_Name}", user);
+logger.LogDebug("Retry attempt {Cnt,-5}", cnt);
+logger.LogDebug("Retry attempt {Cnt:000}", cnt);
+
+

Resources

+

Documentation

+ + diff --git a/analyzers/rspec/cs/S6674.json b/analyzers/rspec/cs/S6674.json new file mode 100644 index 00000000000..b242fd3dc58 --- /dev/null +++ b/analyzers/rspec/cs/S6674.json @@ -0,0 +1,15 @@ +{ + "title": "Log message template should be syntactically correct", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-6674", + "sqKey": "S6674", + "scope": "Main", + "quickfix": "infeasible" +} diff --git a/analyzers/rspec/cs/Sonar_way_profile.json b/analyzers/rspec/cs/Sonar_way_profile.json index 9efd2194c57..c33b21f6a15 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -292,6 +292,7 @@ "S6618", "S6640", "S6667", + "S6674", "S6677", "S6678", "S6797", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs index cc5ed279022..917bf8a5505 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplates/MessageTemplateAnalyzer.cs @@ -51,7 +51,7 @@ protected override void Initialize(SonarAnalysisContext context) => }, SyntaxKind.InvocationExpression); - private static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) => + public static ArgumentSyntax TemplateArgument(InvocationExpressionSyntax invocation, SemanticModel model) => TemplateArgument(invocation, model, KnownType.Microsoft_Extensions_Logging_LoggerExtensions, MicrosoftExtensionsLogging, "message") ?? TemplateArgument(invocation, model, KnownType.Serilog_Log, Serilog, "messageTemplate") ?? TemplateArgument(invocation, model, KnownType.Serilog_ILogger, Serilog, "messageTemplate") diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs new file mode 100644 index 00000000000..aa46fe71563 --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/MessageTemplatesShouldBeCorrect.cs @@ -0,0 +1,164 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 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.Text.RegularExpressions; + +namespace SonarAnalyzer.Rules.CSharp; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class MessageTemplatesShouldBeCorrect : SonarDiagnosticAnalyzer +{ + private const string DiagnosticId = "S6674"; + private const string MessageFormat = "Log message template {0}."; + + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(c => + { + var invocation = (InvocationExpressionSyntax)c.Node; + if (MessageTemplateAnalyzer.TemplateArgument(invocation, c.SemanticModel) is { } argument + && argument.Expression.IsKind(SyntaxKind.StringLiteralExpression) + && TemplateValidator.ContainsErrors(argument.Expression.ToString(), out var errors)) + { + var templateStart = argument.Expression.GetLocation().SourceSpan.Start; + foreach (var error in errors) + { + var location = Location.Create(c.Tree, new(templateStart + error.Start, error.Length)); + c.ReportIssue(Diagnostic.Create(Rule, location, error.Message)); + } + } + }, + SyntaxKind.InvocationExpression); + + private static class TemplateValidator + { + private const string TextPattern = @"([^\{]|\{\{|\}\})+"; + private const string HolePattern = @"{(?[^\}]*)}"; + private const string TemplatePattern = $"^({TextPattern}|{HolePattern})*$"; + // This is similar to the regex used for MessageTemplatesAnalyzer, but it is far more permissive. + // The goal is to manually parse the placeholders, so that we can report more specific issues than just "malformed template". + private static readonly Regex TemplateRegex = new(TemplatePattern, RegexOptions.Compiled, TimeSpan.FromMilliseconds(300)); + + private static readonly Regex PlaceholderNameRegex = new("^[0-9a-zA-Z_]+$", RegexOptions.Compiled, RegexConstants.DefaultTimeout); + private static readonly Regex PlaceholderAlignmentRegex = new("^-?[0-9]+$", RegexOptions.Compiled, RegexConstants.DefaultTimeout); + + public static bool ContainsErrors(string template, out List errors) + { + var result = Helpers.MessageTemplates.Parse(template, TemplateRegex); + errors = result.Success + ? result.Placeholders.Select(ParsePlaceholder).Where(x => x is not null).ToList() + : [new("should be syntactically correct", 0, template.Length)]; + return errors.Count > 0; + } + + private static ParsingError ParsePlaceholder(Helpers.MessageTemplates.Placeholder placeholder) + { + if (placeholder.Length == 0) + { + return new($"should not contain empty placeholder", placeholder.Start - 1, 2); + } + + var parts = Split(placeholder.Name); + if (!PlaceholderNameRegex.SafeIsMatch(parts.Name)) + { + return new($"placeholder '{parts.Name}' should only contain chars, numbers, and underscore", placeholder); + } + else if (parts.Alignment is not null && !PlaceholderAlignmentRegex.SafeIsMatch(parts.Alignment)) + { + return new($"placeholder '{parts.Name}' should have numeric alignment instead of '{parts.Alignment}'", placeholder); + } + else if (parts.Format == string.Empty) + { + return new($"placeholder '{parts.Name}' should not have empty format", placeholder); + } + else + { + return null; + } + } + + // pattern is: name[,alignment][:format] + private static Parts Split(string placeholder) + { + var start = placeholder[0] is '@' or '$' ? 1 : 0; // skip prefix + + string name; + string alignment = null; + string format = null; + + var formatIndex = placeholder.IndexOf(':'); + var alignmentIndex = placeholder.IndexOf(','); + if (formatIndex >= 0 && alignmentIndex > formatIndex) + { + // case like {name:format,alignment} + // where ^^^^^^^^^^^^^^^^ all of this is format, need to reset alignment + alignmentIndex = -1; + } + + if (NotFound(alignmentIndex)) + { + if (NotFound(formatIndex)) + { + name = placeholder.Substring(start); + } + else + { + name = placeholder.Substring(start, formatIndex - start); + format = IsEmpty(formatIndex) + ? string.Empty + : placeholder.Substring(formatIndex + 1); + } + } + else + { + if (NotFound(formatIndex)) + { + name = placeholder.Substring(start, alignmentIndex - start); + alignment = IsEmpty(alignmentIndex) + ? string.Empty + : placeholder.Substring(alignmentIndex + 1); + } + else + { + name = placeholder.Substring(start, alignmentIndex - start); + alignment = placeholder.Substring(alignmentIndex + 1, formatIndex - alignmentIndex - 1); // TODO: check boundaries here+next line, for {User,:} + format = placeholder.Substring(formatIndex + 1); + } + } + + return new(name, alignment, format); + + bool NotFound(int index) => index == -1; + bool IsEmpty(int index) => index == placeholder.Length - 1; + } + + private sealed record Parts(string Name, string Alignment, string Format); + + public sealed record ParsingError(string Message, int Start, int Length) + { + public ParsingError(string message, Helpers.MessageTemplates.Placeholder placeholder) + : this(message, placeholder.Start, placeholder.Length) + { } + } + } +} diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs index 2c6b2f41e51..d6bc8420ee1 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/MessageTemplates.cs @@ -42,9 +42,12 @@ public static class MessageTemplates /// Matches and extracts placeholders from a template string. /// For more info, see Message Templates. /// - public static ParseResult Parse(string template) + public static ParseResult Parse(string template) => + Parse(template, TemplateRegex); + + public static ParseResult Parse(string template, Regex regex) { - if (TemplateRegex.SafeMatch(template) is { Success: true } match) + if (regex.SafeMatch(template) is { Success: true } match) { var placeholders = match.Groups["Placeholder"].Captures .OfType() diff --git a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs index 6ffc1e2bda8..51d7d398dc5 100644 --- a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs @@ -6598,7 +6598,7 @@ internal static class RuleTypeMappingCS // ["S6671"], // ["S6672"], // ["S6673"], - // ["S6674"], + ["S6674"] = "BUG", // ["S6675"], // ["S6676"], ["S6677"] = "BUG", diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs new file mode 100644 index 00000000000..4642bee8451 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/MessageTemplatesShouldBeCorrectTest.cs @@ -0,0 +1,114 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 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 SonarAnalyzer.Rules.CSharp; + +namespace SonarAnalyzer.Test.Rules; + +[TestClass] +public class MessageTemplatesShouldBeCorrectTest +{ + private static readonly IEnumerable LoggingReferences = + NuGetMetadataReference.MicrosoftExtensionsLoggingAbstractions() + .Concat(NuGetMetadataReference.NLog(Constants.NuGetLatestVersion)) + .Concat(NuGetMetadataReference.Serilog(Constants.NuGetLatestVersion)); + + private static readonly VerifierBuilder Builder = new VerifierBuilder() + .AddReferences(LoggingReferences); + + [TestMethod] + public void MessageTemplatesShouldBeCorrect_CS() => + Builder.AddPaths("MessageTemplatesShouldBeCorrect.cs").Verify(); + + [DataTestMethod] + [DataRow("LogCritical")] + [DataRow("LogDebug")] + [DataRow("LogError")] + [DataRow("LogInformation")] + [DataRow("LogTrace")] + [DataRow("LogWarning")] + public void MessageTemplatesShouldBeCorrect_MicrosoftExtensionsLogging_CS(string methodName) => + Builder.AddSnippet($$$""" + using System; + using Microsoft.Extensions.Logging; + + public class Program + { + public void Method(ILogger logger, string user, int cnt) + { + Console.WriteLine("Login failed for {User", user); // Compliant + logger.{{{methodName}}}("Login failed for {User}", user); // Compliant + + logger.{{{methodName}}}("Login failed for {User", user); // Noncompliant: Missing closing bracket + logger.{{{methodName}}}("Login failed for {{User}", user); // Noncompliant: Opening bracket is escaped + logger.{{{methodName}}}("Login failed for {&User}", user); // Noncompliant: Only '@' and '$' are allowed as prefix + logger.{{{methodName}}}("Login failed for {User-Name}", user); // Noncompliant: Only chars, numbers, and underscore are allowed for placeholders + logger.{{{methodName}}}("Retry attempt {Cnt,r}", cnt); // Noncompliant: The alignment specifier must be numeric + logger.{{{methodName}}}("Retry attempt {Cnt:}", cnt); // Noncompliant: Empty format specifier is not allowed + } + } + """).Verify(); + + [DataTestMethod] + [DataRow("Debug")] + [DataRow("Error")] + [DataRow("Information")] + [DataRow("Fatal")] + [DataRow("Warning")] + [DataRow("Verbose")] + public void MessageTemplatesShouldBeCorrect_Serilog_CS(string methodName) => + Builder.AddSnippet($$""" + using Serilog; + using Serilog.Events; + + public class Program + { + public void Method(ILogger logger, int arg) + { + logger.{{methodName}}("Arg: {Arg}", arg); // Compliant + logger.{{methodName}}("Arg: {arg}", arg); // Noncompliant + } + } + """).Verify(); + + [DataTestMethod] + [DataRow("Debug")] + [DataRow("ConditionalDebug")] + [DataRow("Error")] + [DataRow("Fatal")] + [DataRow("Info")] + [DataRow("Trace")] + [DataRow("ConditionalTrace")] + [DataRow("Warn")] + public void MessageTemplatesShouldBeCorrect_NLog_CS(string methodName) => + Builder.AddSnippet($$""" + using NLog; + + public class Program + { + public void Method(ILogger iLogger, Logger logger, MyLogger myLogger, int arg) + { + logger.{{methodName}}("Arg: {Arg}", arg); // Compliant + logger.{{methodName}}("Arg: {arg}", arg); // Noncompliant + } + } + public class MyLogger : Logger { } + """).Verify(); +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs new file mode 100644 index 00000000000..8bb69915ba3 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/MessageTemplatesShouldBeCorrect.cs @@ -0,0 +1,92 @@ +using Microsoft.Extensions.Logging; +using System; + +public class Program +{ + public void Compliant(ILogger logger, string user) + { + Console.WriteLine("Login failed for {User", user); + + logger.LogError(""); + logger.LogError("Login failed for User"); + logger.LogError("{User}", user); + logger.LogError("{User}{User}{User}", user); + logger.LogError("User{User}User", user); + + logger.LogError($"Login failed for {{User", user); + logger.LogError("Login failed for {User}", user); + logger.LogError("Login failed for {{User}}", user); + logger.LogError("Login failed for {{{User}}}", user); + + logger.LogError("{{ }} {{ {{{{ {{🔥}}"); // Caveman logging + + logger.LogError("Login failed for {$User,42}", user); + logger.LogError("Login failed for {@User:format}", user); + logger.LogError("Login failed for {User42,42:format}", user); + logger.LogError("Login failed for {User_Name:format,42}", user); + + logger.LogError("Login failed for {User: }", user); + logger.LogError("Login failed for {User: -f0rmat c@n contain _any!@#$thing_ {you{want{ }", user); + logger.LogError("Login failed for {User,-42}", user); + + logger.LogError("Login failed for {{User}", user); // Compliant FN. Syntactically valid, in reality it's probably a typo. + } + + public void InvalidSyntax(ILogger logger, string user) + { + logger.LogError("Login failed for {User", user); // Noncompliant {{Log message template should be syntactically correct.}} + // ^^^^^^^^^^^^^^^^^^^^^^^^ + } + + public void EmptyPlaceholder(ILogger logger, string user) + { + logger.LogError("Login failed for {}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + // ^^ + logger.LogError("{}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + // ^^ + logger.LogError("{{{}}}", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + // ^^ + logger.LogError("{{Login for {} user }}failed", user); // Noncompliant {{Log message template should not contain empty placeholder.}} + // ^^ + } + + public void InvalidName(ILogger logger, string user) + { + logger.LogError("Login failed for {@}", user); // Noncompliant {{Log message template placeholder '' should only contain chars, numbers, and underscore.}} + // ^ + logger.LogError("Login failed for {%User}", user); // Noncompliant {{Log message template placeholder '%User' should only contain chars, numbers, and underscore.}} + // ^^^^^ + logger.LogError("Login failed for {User-Name}", user); // Noncompliant {{Log message template placeholder 'User-Name' should only contain chars, numbers, and underscore.}} + // ^^^^^^^^^ + logger.LogError("Login failed for {User Name}", user); // Noncompliant {{Log message template placeholder 'User Name' should only contain chars, numbers, and underscore.}} + // ^^^^^^^^^ + } + + public void InvalidAlignment(ILogger logger, int cnt) + { + logger.LogError("Retry attempt {Cnt,r}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of 'r'.}} + // ^^^^^ + + logger.LogError("Retry attempt {Cnt,}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of ''.}} + // ^^^^ + + logger.LogError("Retry attempt {Cnt,-foo}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should have numeric alignment instead of '-foo'.}} + // ^^^^^^^^ + } + + public void InvalidFormat(ILogger logger, int cnt) + { + logger.LogError("Retry attempt {Cnt:}", cnt); // Noncompliant {{Log message template placeholder 'Cnt' should not have empty format.}} + // ^^^^ + } + + public void InvalidAlignmentAndFormat(ILogger logger, int cnt) + { + // TODO + } + + public void MultipleErrors(ILogger logger, string user, int cnt) + { + // TODO + } +}