Improve sensitive data detection when logging records

docs/list-of-diagnostics.md

@@ -129,6 +129,7 @@ if desired.
 | `LOGGEN032` | Can only have one of [LogProperties], [TagProvider], and [LogPropertyIgnore] |
 | `LOGGEN033` | Method parameter can't be used with a tag provider |
 | `LOGGEN034` | Attribute can't be used in this context |
+| `LOGGEN035` | The logging method parameter leaks sensitive data |
 
 
 # Metrics

src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs

@@ -231,4 +231,10 @@ internal sealed class DiagDescriptors : DiagDescriptorsBase
         title: Resources.InvalidAttributeUsageTitle,
         messageFormat: Resources.InvalidAttributeUsageMessage,
         category: Category);
+
+    public static DiagnosticDescriptor RecordTypeSensitiveArgumentIsInTemplate { get; } = Make(
+        id: DiagnosticIds.LoggerMessage.LOGGEN035,
+        title: Resources.RecordTypeSensitiveArgumentIsInTemplateTitle,
+        messageFormat: Resources.RecordTypeSensitiveArgumentIsInTemplateMessage,
+        category: Category);
 }

src/Generators/Microsoft.Gen.Logging/Parsing/Parser.Records.cs

@@ -0,0 +1,166 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Collections.Generic;
+using System.Threading;
+using Microsoft.CodeAnalysis;
+using Microsoft.Gen.Shared;
+
+namespace Microsoft.Gen.Logging.Parsing
+{
+    internal partial class Parser
+    {
+        internal bool RecordHasSensitivePublicMembers(ITypeSymbol type, SymbolHolder symbols)
+        {
+            if (symbols.DataClassificationAttribute is null)
+            {
+                return false;
+            }
+
+            // Check full type hierarchy for sensitive members, including base types and transitive fields/properties
+            var typesChain = new HashSet<ITypeSymbol>(SymbolEqualityComparer.Default);
+            _ = typesChain.Add(type); // Add itself
+            return RecordHasSensitivePublicMembers(type, typesChain, symbols, _cancellationToken);
+        }
+
+        private readonly record struct MemberInfo(
+            string Name,
+            ITypeSymbol Type,
+            IPropertySymbol? Property);
+
+        private static bool RecordHasSensitivePublicMembers(ITypeSymbol type, HashSet<ITypeSymbol> typesChain, SymbolHolder symbols, CancellationToken token)
+        {
+            var overriddenMembers = new HashSet<string>();
+            var namedType = type as INamedTypeSymbol;
+            while (namedType != null && namedType.SpecialType != SpecialType.System_Object)
+            {
+                token.ThrowIfCancellationRequested();
+
+                // Check if the type itself has any data classification attributes applied:
+                var recordDataClassAttributes = GetDataClassificationAttributes(namedType, symbols);
+                if (recordDataClassAttributes.Count > 0)
+                {
+                    return true;
+                }
+
+                var members = namedType.GetMembers();
+                if (members.IsDefaultOrEmpty)
+                {
+                    namedType = namedType.BaseType;
+                    continue;
+                }
+
+                foreach (var m in members)
+                {
+                    // Skip static, non-public members:
+                    if (m.DeclaredAccessibility != Accessibility.Public ||
+                        m.IsStatic)
+                    {
+                        continue;
+                    }
+
+                    MemberInfo? maybeMemberInfo =
+                        m switch
+                        {
+                            IPropertySymbol property => new(property.Name, property.Type, property),
+                            IFieldSymbol field => new(field.Name, field.Type, Property: null),
+                            _ => null
+                        };
+
+                    if (maybeMemberInfo is null)
+                    {
+                        if (m is not IMethodSymbol { MethodKind: MethodKind.Constructor } ctorMethod)
+                        {
+                            continue;
+                        }
+
+                        // Check if constructor has any sensitive parameters:
+                        foreach (var param in ctorMethod.Parameters)
+                        {
+                            // We don't check for "DataClassifierWrappers" here as they will be covered in field/property checks:
+                            var parameterDataClassAttributes = GetDataClassificationAttributes(param, symbols);
+                            if (parameterDataClassAttributes.Count > 0)
+                            {
+                                return true;
+                            }
+                        }
+
+                        // No sensitive parameters in the ctor, continue with next member:
+                        continue;
+                    }
+
+                    var memberInfo = maybeMemberInfo.Value;
+                    if (memberInfo.Property is not null)
+                    {
+                        var property = memberInfo.Property;
+                        var getMethod = property.GetMethod;
+
+                        // We don't check if getter is "public", it doesn't matter: https://github.com/dotnet/runtime/issues/86700
+                        if (getMethod is null)
+                        {
+                            continue;
+                        }
+
+                        // Property is already overridden in derived class, skip it:
+                        if ((property.IsVirtual || property.IsOverride) &&
+                            overriddenMembers.Contains(property.Name))
+                        {
+                            continue;
+                        }
+
+                        // Adding property that overrides some base class virtual property:
+                        if (property.IsOverride)
+                        {
+                            _ = overriddenMembers.Add(property.Name);
+                        }
+                    }
+
+                    // Skip members with delegate types:
+                    var memberType = memberInfo.Type;
+                    if (memberType.TypeKind == TypeKind.Delegate)
+                    {
+                        continue;
+                    }
+
+                    var extractedType = memberType;
+                    if (memberType.IsNullableOfT())
+                    {
+                        // extract the T from a Nullable<T>
+                        extractedType = ((INamedTypeSymbol)memberType).TypeArguments[0];
+                    }
+
+                    // Checking "extractedType" here:
+                    if (typesChain.Contains(extractedType))
+                    {
+                        continue; // Move to the next member
+                    }
+
+                    var propertyDataClassAttributes = GetDataClassificationAttributes(m, symbols);
+                    if (propertyDataClassAttributes.Count > 0)
+                    {
+                        return true;
+                    }
+
+                    if (extractedType.IsRecord && // Going inside record types only
+                        extractedType.Kind == SymbolKind.NamedType &&
+                        !extractedType.IsSpecialType(symbols))
+                    {
+                        _ = typesChain.Add(namedType);
+                        if (RecordHasSensitivePublicMembers(extractedType, typesChain, symbols, token))
+                        {
+                            return true;
+                        }
+
+                        _ = typesChain.Remove(namedType);
+                    }
+                }
+
+                // This is to support inheritance, i.e. to fetch public members of base class(-es) as well:
+                namedType = namedType.BaseType;
+            }
+
+            return false;
+        }
+    }
+}

src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs

@@ -93,6 +93,7 @@ public IReadOnlyList<LoggingType> GetLogTypes(IEnumerable<TypeDeclarationSyntax>
 
                         parameterSymbols[lp] = paramSymbol;
 
+                        var foundDataClassificationAttributesInProps = false;
                         var logPropertiesAttribute = ParserUtilities.GetSymbolAttributeAnnotationOrDefault(symbols.LogPropertiesAttribute, paramSymbol);
                         if (logPropertiesAttribute is not null)
                         {
@@ -101,7 +102,8 @@ public IReadOnlyList<LoggingType> GetLogTypes(IEnumerable<TypeDeclarationSyntax>
                                 lm,
                                 lp,
                                 paramSymbol,
-                                symbols))
+                                symbols,
+                                ref foundDataClassificationAttributesInProps))
                             {
                                 lp.Properties.Clear();
                             }
@@ -134,36 +136,49 @@ public IReadOnlyList<LoggingType> GetLogTypes(IEnumerable<TypeDeclarationSyntax>
                         }
 
                         bool forceAsTemplateParam = false;
-                        if (lp.IsLogger && lm.TemplateToParameterName.ContainsKey(lp.Name))
+                        bool parameterInTemplate = lm.TemplateToParameterName.ContainsKey(lp.Name);
+                        var loggingProperties = logPropertiesAttribute != null || tagProviderAttribute != null;
+                        if (lp.IsLogger && parameterInTemplate)
                         {
                             Diag(DiagDescriptors.ShouldntMentionLoggerInMessage, attrLoc, lp.Name);
                             forceAsTemplateParam = true;
                         }
-                        else if (lp.IsException && lm.TemplateToParameterName.ContainsKey(lp.Name))
+                        else if (lp.IsException && parameterInTemplate)
                         {
                             Diag(DiagDescriptors.ShouldntMentionExceptionInMessage, attrLoc, lp.Name);
                             forceAsTemplateParam = true;
                         }
-                        else if (lp.IsLogLevel && lm.TemplateToParameterName.ContainsKey(lp.Name))
+                        else if (lp.IsLogLevel && parameterInTemplate)
                         {
                             Diag(DiagDescriptors.ShouldntMentionLogLevelInMessage, attrLoc, lp.Name);
                             forceAsTemplateParam = true;
                         }
-#pragma warning disable S1067 // Expressions should not be too complex
                         else if (lp.IsNormalParameter
-                            && !lm.TemplateToParameterName.ContainsKey(lp.Name)
-                            && logPropertiesAttribute == null
-                            && tagProviderAttribute == null
+                            && !parameterInTemplate
+                            && !loggingProperties
                             && !string.IsNullOrEmpty(lm.Message))
                         {
                             Diag(DiagDescriptors.ParameterHasNoCorrespondingTemplate, paramSymbol.GetLocation(), lp.Name);
                         }
-#pragma warning restore S1067 // Expressions should not be too complex
+
+                        var purelyStructuredLoggingParameter = loggingProperties && !parameterInTemplate;
+                        if (lp.IsNormalParameter &&
+                            !lp.HasDataClassification &&
+                            !purelyStructuredLoggingParameter &&
+                            paramSymbol.Type.IsRecord)
+                        {
+                            if (foundDataClassificationAttributesInProps ||
+                                RecordHasSensitivePublicMembers(paramSymbol.Type, symbols))
+                            {
+                                Diag(DiagDescriptors.RecordTypeSensitiveArgumentIsInTemplate, paramSymbol.GetLocation(), lp.Name, lm.Name);
+                                keepMethod = false;
+                            }
+                        }
 
                         lm.Parameters.Add(lp);
                         if (lp.IsNormalParameter || forceAsTemplateParam)
                         {
-                            if (lm.TemplateToParameterName.ContainsKey(lp.Name))
+                            if (parameterInTemplate)
                             {
                                 lp.UsedAsTemplate = true;
                             }

src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx

@@ -327,4 +327,10 @@
   <data name="InvalidAttributeUsageTitle" xml:space="preserve">
     <value>Attribute can't be used in this context</value>
   </data>
-</root>
+  <data name="RecordTypeSensitiveArgumentIsInTemplateMessage" xml:space="preserve">
+    <value>Parameter "{0}" of logging method "{1}" has a sensitive field/property in its type</value>
+  </data>
+  <data name="RecordTypeSensitiveArgumentIsInTemplateTitle" xml:space="preserve">
+    <value>The logging method parameter leaks sensitive data</value>
+  </data>
+</root>

src/Generators/Microsoft.Gen.Logging/Parsing/SymbolLoader.cs

@@ -52,7 +52,7 @@ internal static class SymbolLoader
     {
         var loggerSymbol = compilation.GetTypeByMetadataName(ILoggerType);
         var logLevelSymbol = compilation.GetTypeByMetadataName(LogLevelType);
-        var logMethodAttributeSymbol = compilation.GetTypeByMetadataName(LoggerMessageAttribute);
+        var loggerMessageAttributeSymbol = compilation.GetTypeByMetadataName(LoggerMessageAttribute);
         var logPropertiesAttributeSymbol = compilation.GetTypeByMetadataName(LogPropertiesAttribute);
         var tagProviderAttributeSymbol = compilation.GetTypeByMetadataName(TagProviderAttribute);
         var tagCollectorSymbol = compilation.GetTypeByMetadataName(ITagCollectorType);
@@ -62,7 +62,7 @@ internal static class SymbolLoader
 #pragma warning disable S1067 // Expressions should not be too complex
         if (loggerSymbol == null
             || logLevelSymbol == null
-            || logMethodAttributeSymbol == null
+            || loggerMessageAttributeSymbol == null
             || logPropertiesAttributeSymbol == null
             || tagProviderAttributeSymbol == null
             || tagCollectorSymbol == null
@@ -97,7 +97,7 @@ internal static class SymbolLoader
 
         return new(
             compilation,
-            logMethodAttributeSymbol,
+            loggerMessageAttributeSymbol,
             logPropertiesAttributeSymbol,
             tagProviderAttributeSymbol,
             logPropertyIgnoreAttributeSymbol,

src/Shared/DiagnosticIds/DiagnosticIds.cs

@@ -111,6 +111,7 @@ internal static class LoggerMessage
         internal const string LOGGEN032 = nameof(LOGGEN032);
         internal const string LOGGEN033 = nameof(LOGGEN033);
         internal const string LOGGEN034 = nameof(LOGGEN034);
+        internal const string LOGGEN035 = nameof(LOGGEN035);
     }
 
     internal static class Metrics