diff --git a/docs/list-of-diagnostics.md b/docs/list-of-diagnostics.md index 5df6588bba8..933d64c493a 100644 --- a/docs/list-of-diagnostics.md +++ b/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 diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs b/src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs index 43e1abef1d6..275fc2dc82c 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/DiagDescriptors.cs +++ b/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); } diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.LogProperties.cs b/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.LogProperties.cs index 88869bdf551..4fabb10e603 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.LogProperties.cs +++ b/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.LogProperties.cs @@ -20,12 +20,13 @@ internal partial class Parser { private static readonly HashSet _allowedTypeKinds = [TypeKind.Class, TypeKind.Struct, TypeKind.Interface]; - private bool ProcessLogPropertiesForParameter( + internal bool ProcessLogPropertiesForParameter( AttributeData logPropertiesAttribute, LoggingMethod lm, LoggingMethodParameter lp, IParameterSymbol paramSymbol, - SymbolHolder symbols) + SymbolHolder symbols, + ref bool foundDataClassificationAttributes) { var paramName = paramSymbol.Name; if (!lp.IsNormalParameter) @@ -52,7 +53,7 @@ private bool ProcessLogPropertiesForParameter( _ = typesChain.Add(paramTypeSymbol); // Add itself - var props = GetTypePropertiesToLog(paramTypeSymbol, typesChain, symbols); + var props = GetTypePropertiesToLog(paramTypeSymbol, typesChain, symbols, ref foundDataClassificationAttributes); if (props == null) { return false; @@ -84,7 +85,8 @@ static string GetPropertyIdentifier(IPropertySymbol property, CancellationToken List? GetTypePropertiesToLog( ITypeSymbol type, ISet typesChain, - SymbolHolder symbols) + SymbolHolder symbols, + ref bool foundDataClassificationAttributes) { var result = new List(); var seenProps = new HashSet(); @@ -94,180 +96,212 @@ static string GetPropertyIdentifier(IPropertySymbol property, CancellationToken _cancellationToken.ThrowIfCancellationRequested(); var members = namedType.GetMembers(); - if (members != null) + if (members.IsDefaultOrEmpty) { - foreach (var property in members.Where(m => m.Kind == SymbolKind.Property).Cast()) - { - var logPropertiesAttribute = ParserUtilities.GetSymbolAttributeAnnotationOrDefault(symbols.LogPropertiesAttribute, property); - var logPropertyIgnoreAttribute = ParserUtilities.GetSymbolAttributeAnnotationOrDefault(symbols.LogPropertyIgnoreAttribute, property); - var tagProviderAttribute = ParserUtilities.GetSymbolAttributeAnnotationOrDefault(symbols.TagProviderAttribute, property); + namedType = namedType.BaseType; + continue; + } - var count = 0; - if (logPropertiesAttribute != null) + var sensitivePropsFromCtor = new Dictionary>(); + if (namedType.IsRecord && !namedType.InstanceConstructors.IsDefaultOrEmpty) + { + // We can also decide to skip here "protected", "internal" and "private" constructors in future: + foreach (var ctor in namedType.InstanceConstructors) + { + // Check if constructor has any sensitive parameters: + foreach (var parameter in ctor.Parameters) { - count++; + var maybeDataClasses = GetDataClassificationAttributes(parameter, symbols); + if (maybeDataClasses.Count > 0) + { + sensitivePropsFromCtor[parameter.Name] = maybeDataClasses; + } } + } + } - if (logPropertyIgnoreAttribute != null) - { - count++; - } + foreach (var property in members.Where(m => m.Kind == SymbolKind.Property).Cast()) + { + var logPropertiesAttribute = ParserUtilities.GetSymbolAttributeAnnotationOrDefault(symbols.LogPropertiesAttribute, property); + var logPropertyIgnoreAttribute = ParserUtilities.GetSymbolAttributeAnnotationOrDefault(symbols.LogPropertyIgnoreAttribute, property); + var tagProviderAttribute = ParserUtilities.GetSymbolAttributeAnnotationOrDefault(symbols.TagProviderAttribute, property); - if (tagProviderAttribute != null) - { - count++; - } + var count = 0; + if (logPropertiesAttribute != null) + { + count++; + } - if (count > 1) - { - Diag(DiagDescriptors.CantMixAttributes, property.GetLocation()); - continue; - } + if (logPropertyIgnoreAttribute != null) + { + count++; + } - if (!seenProps.Add(property.Name)) - { - // already saw this property in a derived type, skip it - continue; - } + if (tagProviderAttribute != null) + { + count++; + } - var classification = new HashSet(); - var current = type; - while (current != null) - { - classification.UnionWith(GetDataClassificationAttributes(current, symbols).Select(x => x.ToDisplayString())); - current = current.BaseType; - } + if (count > 1) + { + Diag(DiagDescriptors.CantMixAttributes, property.GetLocation()); + continue; + } - classification.UnionWith(GetDataClassificationAttributes(property, symbols).Select(x => x.ToDisplayString())); + if (!seenProps.Add(property.Name)) + { + // already saw this property in a derived type, skip it + continue; + } - var extractedType = property.Type; - if (extractedType.IsNullableOfT()) - { - // extract the T from a Nullable - extractedType = ((INamedTypeSymbol)extractedType).TypeArguments[0]; - } + var classification = new HashSet(); + var current = type; + while (current != null) + { + classification.UnionWith(GetDataClassificationAttributes(current, symbols).Select(x => x.ToDisplayString())); + current = current.BaseType; + } - var lp = new LoggingProperty - { - Name = property.Name, - Type = property.Type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), - ClassificationAttributeTypes = classification, - IsReference = property.Type.IsReferenceType, - IsEnumerable = property.Type.IsEnumerable(symbols), - IsNullable = property.Type.NullableAnnotation == NullableAnnotation.Annotated, - ImplementsIConvertible = property.Type.ImplementsIConvertible(symbols), - ImplementsIFormattable = property.Type.ImplementsIFormattable(symbols), - ImplementsISpanFormattable = property.Type.ImplementsISpanFormattable(symbols), - }; - - if (!property.DeclaringSyntaxReferences.IsDefaultOrEmpty) - { - var propertyIdentifier = GetPropertyIdentifier(property, _cancellationToken); + classification.UnionWith(GetDataClassificationAttributes(property, symbols).Select(x => x.ToDisplayString())); - if (!string.IsNullOrEmpty(propertyIdentifier)) - { - lp.NeedsAtSign = propertyIdentifier[0] == '@'; - } - } + // A property might be a sensitive parameter of a constructor: + if (sensitivePropsFromCtor.TryGetValue(property.Name, out var dataClassesFromCtor)) + { + classification.UnionWith(dataClassesFromCtor.Select(x => x.ToDisplayString())); + } + + if (classification.Count > 0) + { + foundDataClassificationAttributes = true; + } + + var extractedType = property.Type; + if (extractedType.IsNullableOfT()) + { + // extract the T from a Nullable + extractedType = ((INamedTypeSymbol)extractedType).TypeArguments[0]; + } + + var lp = new LoggingProperty + { + Name = property.Name, + Type = property.Type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat), + ClassificationAttributeTypes = classification, + IsReference = property.Type.IsReferenceType, + IsEnumerable = property.Type.IsEnumerable(symbols), + IsNullable = property.Type.NullableAnnotation == NullableAnnotation.Annotated, + ImplementsIConvertible = property.Type.ImplementsIConvertible(symbols), + ImplementsIFormattable = property.Type.ImplementsIFormattable(symbols), + ImplementsISpanFormattable = property.Type.ImplementsISpanFormattable(symbols), + }; + + if (!property.DeclaringSyntaxReferences.IsDefaultOrEmpty) + { + var propertyIdentifier = GetPropertyIdentifier(property, _cancellationToken); - if (property.Type.IsValueType && !property.Type.IsNullableOfT()) + if (!string.IsNullOrEmpty(propertyIdentifier)) { + lp.NeedsAtSign = propertyIdentifier[0] == '@'; + } + } + + if (property.Type.IsValueType && !property.Type.IsNullableOfT()) + { #pragma warning disable S125 - // deal with an oddity in Roslyn. If you have this case: - // - // class Gen - // { - // public T? Value { get; set; } - // } - // - // class Foo - // { - // public Gen MyInt { get; set; } - // } - // - // then Roslyn claims that MyInt has nullable annotations. Yet, doing x.MyInt?.ToString isn't valid. - // So we explicitly check whether the value is indeed a Nullable. - lp.IsNullable = false; + // deal with an oddity in Roslyn. If you have this case: + // + // class Gen + // { + // public T? Value { get; set; } + // } + // + // class Foo + // { + // public Gen MyInt { get; set; } + // } + // + // then Roslyn claims that MyInt has nullable annotations. Yet, doing x.MyInt?.ToString isn't valid. + // So we explicitly check whether the value is indeed a Nullable. + lp.IsNullable = false; #pragma warning restore S125 - } + } + + // Check if this property hides a base property + if (ParserUtilities.PropertyHasModifier(property, SyntaxKind.NewKeyword, _cancellationToken)) + { + Diag(DiagDescriptors.LogPropertiesHiddenPropertyDetected, paramSymbol.GetLocation(), paramName, lm.Name, property.Name); + return null; + } - // Check if this property hides a base property - if (ParserUtilities.PropertyHasModifier(property, SyntaxKind.NewKeyword, _cancellationToken)) + if (logPropertiesAttribute != null) + { + _ = CanLogProperties(property, property.Type, symbols); + + if ((property.DeclaredAccessibility != Accessibility.Public || property.IsStatic) + || (property.GetMethod == null || property.GetMethod.DeclaredAccessibility != Accessibility.Public)) { - Diag(DiagDescriptors.LogPropertiesHiddenPropertyDetected, paramSymbol.GetLocation(), paramName, lm.Name, property.Name); - return null; + Diag(DiagDescriptors.InvalidAttributeUsage, logPropertiesAttribute.ApplicationSyntaxReference?.GetSyntax(_cancellationToken).GetLocation(), "LogProperties"); + continue; } - if (logPropertiesAttribute != null) + // Checking "extractedType" here + if (typesChain.Contains(extractedType)) { - _ = CanLogProperties(property, property.Type, symbols); - - if ((property.DeclaredAccessibility != Accessibility.Public || property.IsStatic) - || (property.GetMethod == null || property.GetMethod.DeclaredAccessibility != Accessibility.Public)) - { - Diag(DiagDescriptors.InvalidAttributeUsage, logPropertiesAttribute.ApplicationSyntaxReference?.GetSyntax(_cancellationToken).GetLocation(), "LogProperties"); - continue; - } - - // Checking "extractedType" here - if (typesChain.Contains(extractedType)) - { - Diag(DiagDescriptors.LogPropertiesCycleDetected, paramSymbol.GetLocation(), paramName, namedType.ToDisplayString(), property.Type.ToDisplayString(), lm.Name); - return null; - } + Diag(DiagDescriptors.LogPropertiesCycleDetected, paramSymbol.GetLocation(), paramName, namedType.ToDisplayString(), property.Type.ToDisplayString(), lm.Name); + return null; + } - var propName = property.Name; + var propName = property.Name; - extractedType = property.Type; - if (extractedType.IsNullableOfT()) - { - // extract the T from a Nullable - extractedType = ((INamedTypeSymbol)extractedType).TypeArguments[0]; - } + extractedType = property.Type; + if (extractedType.IsNullableOfT()) + { + // extract the T from a Nullable + extractedType = ((INamedTypeSymbol)extractedType).TypeArguments[0]; + } - _ = typesChain.Add(namedType); - var props = GetTypePropertiesToLog(extractedType, typesChain, symbols); - _ = typesChain.Remove(namedType); + _ = typesChain.Add(namedType); + var props = GetTypePropertiesToLog(extractedType, typesChain, symbols, ref foundDataClassificationAttributes); + _ = typesChain.Remove(namedType); - if (props != null) - { - lp.Properties.AddRange(props); - } + if (props != null) + { + lp.Properties.AddRange(props); } + } - if (tagProviderAttribute != null) + if (tagProviderAttribute != null) + { + if ((property.DeclaredAccessibility != Accessibility.Public || property.IsStatic) + || (property.GetMethod == null || property.GetMethod.DeclaredAccessibility != Accessibility.Public)) { - if ((property.DeclaredAccessibility != Accessibility.Public || property.IsStatic) - || (property.GetMethod == null || property.GetMethod.DeclaredAccessibility != Accessibility.Public)) - { - Diag(DiagDescriptors.InvalidAttributeUsage, tagProviderAttribute.ApplicationSyntaxReference?.GetSyntax(_cancellationToken).GetLocation(), "TagProvider"); - } - - if (!ProcessTagProviderForProperty(tagProviderAttribute, lp, property, symbols)) - { - lp.TagProvider = null; - } + Diag(DiagDescriptors.InvalidAttributeUsage, tagProviderAttribute.ApplicationSyntaxReference?.GetSyntax(_cancellationToken).GetLocation(), "TagProvider"); } - if (tagProviderAttribute == null && logPropertiesAttribute == null) + if (!ProcessTagProviderForProperty(tagProviderAttribute, lp, property, symbols)) { - if ((property.DeclaredAccessibility != Accessibility.Public || property.IsStatic) - || (property.GetMethod == null || property.GetMethod.DeclaredAccessibility != Accessibility.Public) - || (property.Type.TypeKind == TypeKind.Delegate) - || (logPropertyIgnoreAttribute != null)) - { - continue; - } + lp.TagProvider = null; } + } - if (lp.HasDataClassification && (lp.HasProperties || lp.HasTagProvider)) + if (tagProviderAttribute == null && logPropertiesAttribute == null) + { + if ((property.DeclaredAccessibility != Accessibility.Public || property.IsStatic) + || (property.GetMethod == null || property.GetMethod.DeclaredAccessibility != Accessibility.Public) + || (property.Type.TypeKind == TypeKind.Delegate) + || (logPropertyIgnoreAttribute != null)) { - Diag(DiagDescriptors.CantUseDataClassificationWithLogPropertiesOrTagProvider, property.GetLocation()); - lp.ClassificationAttributeTypes.Clear(); + continue; } + } - result.Add(lp); + if (lp.HasDataClassification && (lp.HasProperties || lp.HasTagProvider)) + { + Diag(DiagDescriptors.CantUseDataClassificationWithLogPropertiesOrTagProvider, property.GetLocation()); + lp.ClassificationAttributeTypes.Clear(); } + + result.Add(lp); } // This is to support inheritance, i.e. to fetch public properties of base class(-es) as well: diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.Records.cs b/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.Records.cs new file mode 100644 index 00000000000..c5f16008de7 --- /dev/null +++ b/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(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 typesChain, SymbolHolder symbols, CancellationToken token) + { + var overriddenMembers = new HashSet(); + 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 + 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; + } + } +} diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs b/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs index 257095c4a06..bc7b101fdcc 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs +++ b/src/Generators/Microsoft.Gen.Logging/Parsing/Parser.cs @@ -93,6 +93,7 @@ public IReadOnlyList GetLogTypes(IEnumerable parameterSymbols[lp] = paramSymbol; + var foundDataClassificationAttributesInProps = false; var logPropertiesAttribute = ParserUtilities.GetSymbolAttributeAnnotationOrDefault(symbols.LogPropertiesAttribute, paramSymbol); if (logPropertiesAttribute is not null) { @@ -101,7 +102,8 @@ public IReadOnlyList GetLogTypes(IEnumerable lm, lp, paramSymbol, - symbols)) + symbols, + ref foundDataClassificationAttributesInProps)) { lp.Properties.Clear(); } @@ -134,36 +136,49 @@ public IReadOnlyList GetLogTypes(IEnumerable } 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; } diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.Designer.cs b/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.Designer.cs index e5fb9fc6494..63fce874d64 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.Designer.cs +++ b/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.Designer.cs @@ -456,6 +456,24 @@ internal static string ParameterHasNoCorrespondingTemplateTitle { } } + /// + /// Looks up a localized string similar to Parameter "{0}" of logging method "{1}" has sensitive field/property in its type. + /// + internal static string RecordTypeSensitiveArgumentIsInTemplateMessage { + get { + return ResourceManager.GetString("RecordTypeSensitiveArgumentIsInTemplateMessage", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The logging method parameter leaks sensitive data. + /// + internal static string RecordTypeSensitiveArgumentIsInTemplateTitle { + get { + return ResourceManager.GetString("RecordTypeSensitiveArgumentIsInTemplateTitle", resourceCulture); + } + } + /// /// Looks up a localized string similar to Remove redundant qualifier (Info:, Warning:, Error:, etc) from the logging message since it is implicit in the specified log level.. /// diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx b/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx index 4dab52fa763..6273eee7246 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx +++ b/src/Generators/Microsoft.Gen.Logging/Parsing/Resources.resx @@ -327,4 +327,10 @@ Attribute can't be used in this context - + + Parameter "{0}" of logging method "{1}" has a sensitive field/property in its type + + + The logging method parameter leaks sensitive data + + \ No newline at end of file diff --git a/src/Generators/Microsoft.Gen.Logging/Parsing/SymbolLoader.cs b/src/Generators/Microsoft.Gen.Logging/Parsing/SymbolLoader.cs index 11af20b794a..4dce3561635 100644 --- a/src/Generators/Microsoft.Gen.Logging/Parsing/SymbolLoader.cs +++ b/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, diff --git a/src/Shared/DiagnosticIds/DiagnosticIds.cs b/src/Shared/DiagnosticIds/DiagnosticIds.cs index ff7508ff04b..dbc82065488 100644 --- a/src/Shared/DiagnosticIds/DiagnosticIds.cs +++ b/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 diff --git a/test/Generators/Microsoft.Gen.Logging/Generated/SensitiveRecordTests.cs b/test/Generators/Microsoft.Gen.Logging/Generated/SensitiveRecordTests.cs new file mode 100644 index 00000000000..3719271a3ce --- /dev/null +++ b/test/Generators/Microsoft.Gen.Logging/Generated/SensitiveRecordTests.cs @@ -0,0 +1,87 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using Microsoft.Extensions.Logging.Testing; +using TestClasses; +using Xunit; + +namespace Microsoft.Gen.Logging.Test; + +public class SensitiveRecordTests +{ + [Fact] + public void TestRecordInTemplate() + { + using var logger = Utils.GetLogger(); + SensitiveRecordExtensions.LogInTemplate(logger, new()); + + var logRecord = Assert.Single(logger.FakeLogCollector.GetSnapshot()); + Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, logRecord.Message); + Assert.NotNull(logRecord.StructuredState); + Assert.All(logRecord.StructuredState, x => Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, x.Value)); + } + + [Fact] + public void TestRecordStructured() + { + using var logger = Utils.GetLogger(); + SensitiveRecordExtensions.LogFullyStructured(logger, new()); + + var logRecord = Assert.Single(logger.FakeLogCollector.GetSnapshot()); + Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, logRecord.Message); + Assert.NotNull(logRecord.StructuredState); + Assert.All(logRecord.StructuredState, x => Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, x.Value)); + } + + [Fact] + public void TestSensitiveRecordWithLogPropsAndTemplate() + { + using var logger = Utils.GetLogger(); + var dataToLog = new SensitiveRecordExtensions.RecordWithSensitiveMembers(SensitiveRecordExtensions.Sensitive, SensitiveRecordExtensions.Sensitive) + { + PropGetSet = SensitiveRecordExtensions.Sensitive + }; + + SensitiveRecordExtensions.LogPropertiesWithTemplate(logger, dataToLog); + + var logRecord = Assert.Single(logger.FakeLogCollector.GetSnapshot()); + Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, logRecord.Message); + Assert.NotNull(logRecord.StructuredState); + Assert.All(logRecord.StructuredState, x => Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, x.Value)); + } + + [Fact] + public void TestSensitiveRecordWithLogPropsNoTemplate() + { + using var logger = Utils.GetLogger(); + var dataToLog = new SensitiveRecordExtensions.RecordWithSensitiveMembers(SensitiveRecordExtensions.Sensitive, SensitiveRecordExtensions.Sensitive) + { + PropGetSet = SensitiveRecordExtensions.Sensitive + }; + + SensitiveRecordExtensions.LogPropertiesFullyStructured(logger, dataToLog); + + var logRecord = Assert.Single(logger.FakeLogCollector.GetSnapshot()); + Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, logRecord.Message); + Assert.NotNull(logRecord.StructuredState); + Assert.All(logRecord.StructuredState, x => Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, x.Value)); + } + + [Fact] + public void TestSensitiveRecordWithInlineAnnotation() + { + using var logger = Utils.GetLogger(); + var dataToLog = new SensitiveRecordExtensions.RecordWithSensitiveMembers(SensitiveRecordExtensions.Sensitive, SensitiveRecordExtensions.Sensitive) + { + PropGetSet = SensitiveRecordExtensions.Sensitive + }; + + SensitiveRecordExtensions.LogInTemplateWithAnnotation(logger, dataToLog); + + var logRecord = Assert.Single(logger.FakeLogCollector.GetSnapshot()); + Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, logRecord.Message); + Assert.NotNull(logRecord.StructuredState); + Assert.All(logRecord.StructuredState, x => Assert.DoesNotContain(SensitiveRecordExtensions.Sensitive, x.Value)); + } +} diff --git a/test/Generators/Microsoft.Gen.Logging/TestClasses/SensitiveRecordExtensions.cs b/test/Generators/Microsoft.Gen.Logging/TestClasses/SensitiveRecordExtensions.cs new file mode 100644 index 00000000000..479fd935107 --- /dev/null +++ b/test/Generators/Microsoft.Gen.Logging/TestClasses/SensitiveRecordExtensions.cs @@ -0,0 +1,90 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics.CodeAnalysis; +using Microsoft.Extensions.Compliance.Testing; +using Microsoft.Extensions.Logging; + +namespace TestClasses +{ + [SuppressMessage("Performance", "CA1822:Mark members as static", Justification = "Test code")] + [SuppressMessage("CodeQuality", "IDE0051:Remove unused private members", Justification = "Test code")] + [SuppressMessage("CodeQuality", "IDE0052:Remove unread private members", Justification = "Test code")] + [SuppressMessage("Style", "IDE1006:Naming Styles", Justification = "Test code")] + [SuppressMessage("Major Code Smell", "S2376:Write-only properties should not be used", Justification = "Test code")] + internal static partial class SensitiveRecordExtensions + { + internal const string Sensitive = "SENSITIVE"; + + public record class BaseRecord + { + [PrivateData] public const string ConstFieldBase = Sensitive; + [PrivateData] public static string StaticFieldBase = Sensitive; + [PrivateData] public static string StaticPropBase => Sensitive; + +#pragma warning disable CS0414 // The field is assigned but its value is never used + [PrivateData] private readonly string _privateFieldBase = Sensitive; + [PrivateData] private string _privateReadonlyFieldBase = Sensitive; + [PrivateData] internal string InternalFieldBase = Sensitive; +#pragma warning restore CS0414 // The field is assigned but its value is never used + } + + public record class InterimRecord : BaseRecord + { + [PrivateData] public virtual string PropVirtual => Sensitive; + } + + // Even though this record and its base classes contain sensitive members, + // they won't be logged because the default record's ToString() implementation doesn't emit non-public and non-instance members. + internal record class MyRecord : InterimRecord + { + [PrivateData] public const string ConstField = Sensitive; + [PrivateData] public static string StaticField = Sensitive; + [PrivateData] public static string StaticProp => Sensitive; + +#pragma warning disable CS0414 // The field is assigned but its value is never used + [PrivateData] private string PrivateField = Sensitive; + [PrivateData] internal string InternalField = Sensitive; +#pragma warning restore CS0414 // The field is assigned but its value is never used + + [PrivateData] private string PrivatePropGetSet { get; set; } = Sensitive; + [PrivateData] internal string InternalPropGetSet { get; set; } = Sensitive; + [PrivateData] protected string ProtectedPropGetSet { get; set; } = Sensitive; + [PrivateData] private string PrivatePropGet => Sensitive; + [PrivateData] internal string InternalPropGet => Sensitive; + [PrivateData] protected string ProtectedPropGet => Sensitive; + + // This one overrides 'virtual' property declared in 'InterimRecord': + public override string PropVirtual => "Regular"; + } + + internal record RecordWithSensitiveMembers( + [PrivateData] string AnnotatedArgFromPrimaryCtor, + [property: PrivateData] string AnnotatedPropFromPrimaryCtor) + { + [PrivateData] + public string? PropGetSet { get; set; } + } + + [LoggerMessage(LogLevel.Debug, "Param is {p0}")] + public static partial void LogInTemplate(ILogger logger, MyRecord p0); + + [LoggerMessage(LogLevel.Debug)] + public static partial void LogFullyStructured(ILogger logger, MyRecord p0); + + [LoggerMessage(LogLevel.Information, "Data was obtained")] + public static partial void LogPropertiesWithTemplate( + ILogger logger, + [LogProperties] RecordWithSensitiveMembers data); + + [LoggerMessage(LogLevel.Information)] + public static partial void LogPropertiesFullyStructured( + ILogger logger, + [LogProperties] RecordWithSensitiveMembers data); + + [LoggerMessage(LogLevel.Information, "Data is {data}")] + public static partial void LogInTemplateWithAnnotation( + ILogger logger, + [PrivateData] RecordWithSensitiveMembers data); + } +} diff --git a/test/Generators/Microsoft.Gen.Logging/Unit/AttributeParserTests.cs b/test/Generators/Microsoft.Gen.Logging/Unit/AttributeParserTests.cs index 5343fc77740..a2364b2adf2 100644 --- a/test/Generators/Microsoft.Gen.Logging/Unit/AttributeParserTests.cs +++ b/test/Generators/Microsoft.Gen.Logging/Unit/AttributeParserTests.cs @@ -178,18 +178,17 @@ namespace Test {{ using Microsoft.Extensions.Compliance.Testing; using Microsoft.Extensions.Compliance.Redaction; using Microsoft.Extensions.Logging; - using Microsoft.Extensions.Logging; {code} }} "; var loggerAssembly = Assembly.GetAssembly(typeof(ILogger)); - var logMethodAssembly = Assembly.GetAssembly(typeof(LoggerMessageAttribute)); + var loggerMessageAssembly = Assembly.GetAssembly(typeof(LoggerMessageAttribute)); var enrichmentAssembly = Assembly.GetAssembly(typeof(IEnrichmentTagCollector)); var dataClassificationAssembly = Assembly.GetAssembly(typeof(DataClassification)); var simpleDataClassificationAssembly = Assembly.GetAssembly(typeof(PrivateDataAttribute)); var redactorProviderAssembly = Assembly.GetAssembly(typeof(IRedactorProvider)); - var refs = new[] { loggerAssembly!, logMethodAssembly!, enrichmentAssembly!, dataClassificationAssembly!, simpleDataClassificationAssembly!, redactorProviderAssembly! }; + var refs = new[] { loggerAssembly!, loggerMessageAssembly!, enrichmentAssembly!, dataClassificationAssembly!, simpleDataClassificationAssembly!, redactorProviderAssembly! }; var (d, _) = await RoslynTestUtils.RunGenerator( new LoggingGenerator(), diff --git a/test/Generators/Microsoft.Gen.Logging/Unit/LogParserUtilitiesTests.cs b/test/Generators/Microsoft.Gen.Logging/Unit/LogParserUtilitiesTests.cs index c1a6e6cde57..1db966137c3 100644 --- a/test/Generators/Microsoft.Gen.Logging/Unit/LogParserUtilitiesTests.cs +++ b/test/Generators/Microsoft.Gen.Logging/Unit/LogParserUtilitiesTests.cs @@ -1,9 +1,16 @@ // 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.Collections.Immutable; +using System.Threading; using Microsoft.CodeAnalysis; +using Microsoft.Gen.Logging.Model; +using Microsoft.Gen.Logging.Parsing; using Microsoft.Gen.Shared; using Moq; +using Moq.Protected; using Xunit; namespace Microsoft.Gen.Logging.Test; @@ -34,4 +41,154 @@ public void ShouldNotDetectNullableOfT() var result = typeSymbolMock.Object.IsNullableOfT(); Assert.False(result); } + + [Fact] + public void RecordHasSensitivePublicMembers_ShouldReturnFalse_WhenNoDataClasses() + { + var symbolMock = new Mock(); + symbolMock + .Setup(x => x.GetMembers()) + .Returns(ImmutableArray.Empty); + + var symbolHolder = new SymbolHolder( + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!); + + var diagMock = new Mock>(); + var parser = new Parser(null!, diagMock.Object, CancellationToken.None); + var result = parser.RecordHasSensitivePublicMembers(symbolMock.Object, symbolHolder); + Assert.False(result); + symbolMock.VerifyNoOtherCalls(); + } + + [Theory] + [CombinatorialData] + public void RecordHasSensitivePublicMembers_ShouldNotThrow_WhenNoMembersOnType(bool isNull) + { + var symbolMock = new Mock(); + symbolMock + .Setup(x => x.GetMembers()) + .Returns(isNull + ? default + : ImmutableArray.Empty); + + symbolMock + .Setup(x => x.GetAttributes()) + .Returns(Array.Empty().ToImmutableArray()); + + symbolMock + .SetupGet(x => x.BaseType) + .Returns((INamedTypeSymbol?)null); + + symbolMock + .SetupGet(x => x.SpecialType) + .Returns(SpecialType.None); + + var symbolHolder = new SymbolHolder( + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + Mock.Of()); + + var diagMock = new Mock>(); + var parser = new Parser(null!, diagMock.Object, CancellationToken.None); + var result = parser.RecordHasSensitivePublicMembers(symbolMock.Object, symbolHolder); + Assert.False(result); + symbolMock.VerifyAll(); + symbolMock.VerifyNoOtherCalls(); + } + + [Theory] + [CombinatorialData] + public void ProcessLogPropertiesForParameter_ShouldNotThrow_WhenNoMembersOnType(bool isNull) + { + var symbolHolder = new SymbolHolder( + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + null!, + new HashSet(SymbolEqualityComparer.Default), + null!, + null!, + null!, + null!); + + const string ParamType = "param type"; + + var paramTypeMock = new Mock() + .As(); + + paramTypeMock.SetupGet(x => x.Kind).Returns(SymbolKind.NamedType); + paramTypeMock.SetupGet(x => x.TypeKind).Returns(TypeKind.Class); + paramTypeMock.SetupGet(x => x.SpecialType).Returns(SpecialType.None); + paramTypeMock.SetupGet(x => x.OriginalDefinition).Returns(paramTypeMock.Object); + paramTypeMock.Setup(x => x.ToDisplayString(It.IsAny())) + .Returns(ParamType); + + paramTypeMock + .SetupGet(x => x.BaseType) + .Returns((INamedTypeSymbol?)null); + + paramTypeMock + .Setup(x => x.GetMembers()) + .Returns(isNull + ? default + : ImmutableArray.Empty); + + var paramSymbolMock = new Mock(); + paramSymbolMock.SetupGet(x => x.Type) + .Returns(paramTypeMock.Object); + + var logPropertiesAttribute = new Mock(); + logPropertiesAttribute + .Protected() + .SetupGet>>("CommonNamedArguments") + .Returns(ImmutableArray>.Empty); + + logPropertiesAttribute + .Protected() + .SetupGet>("CommonConstructorArguments") + .Returns(ImmutableArray.Empty); + + var diagMock = new Mock>(); + var parser = new Parser(null!, diagMock.Object, CancellationToken.None); + bool unused = false; + var result = parser.ProcessLogPropertiesForParameter( + logPropertiesAttribute.Object, + null!, + new LoggingMethodParameter(), + paramSymbolMock.Object, + symbolHolder, + ref unused); + + diagMock.Verify(x => x.Invoke(It.Is(d => d.Id == DiagDescriptors.LogPropertiesParameterSkipped.Id)), Times.Once); + Assert.True(result); + } } diff --git a/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.LogProperties.cs b/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.LogProperties.cs index 96bb274aada..98d30ccdd99 100644 --- a/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.LogProperties.cs +++ b/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.LogProperties.cs @@ -76,20 +76,23 @@ partial class C }"); } - [Fact] - public static async Task LogProperties_AllowsRecordTypes() + [Theory] + [InlineData("record class")] + [InlineData("record struct")] + [InlineData("readonly record struct")] + public async Task LogProperties_AllowsRecordTypes(string type) { - await RunGenerator(@" - internal record class MyRecord(int Value) - { + await RunGenerator(@$" + internal {type} MyRecord(int Value) + {{ public int GetOnlyValue => Value + 1; - } + }} partial class C - { + {{ [LoggerMessage(LogLevel.Debug)] public static partial void LogFunc(ILogger logger, [LogProperties] MyRecord p0); - }"); + }}"); } [Theory] diff --git a/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.cs b/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.cs index 1fa12390444..bfd7dc44c6c 100644 --- a/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.cs +++ b/test/Generators/Microsoft.Gen.Logging/Unit/ParserTests.cs @@ -18,6 +18,8 @@ namespace Microsoft.Gen.Logging.Test; public partial class ParserTests { + private const int TotalSensitiveCases = 21; + [Fact] public async Task IncompatibleAttributes() { @@ -675,6 +677,174 @@ partial class C await RunGenerator(source, DiagDescriptors.LoggingMethodParameterRefKind); } + [Theory] + [CombinatorialData] + public async Task LogMethod_DetectsSensitiveMembersInRecord([CombinatorialRange(0, TotalSensitiveCases)] int positionNumber) + { + var sb = new System.Text.StringBuilder(@" + using Microsoft.Extensions.Compliance.Testing; + using System.Collections; + + {17} + public record class BaseRecord + { + {0}public string PropBase { get; } + {1}public string FieldBase; + } + + {18} + public record struct InlineStruct({2}string InlineProp); + + {19} + public record class InterimRecord : BaseRecord + { + {3}public string PropInterim { get; } + {4}public string FieldInterim; + + // Hiding on purpose: + {5}public new string FieldBase; + public virtual string PropVirt => nameof(PropVirt); + } + + {20} + // 'internal' on purpose + internal record struct EnumerableStructRecord : IEnumerable + { + {6}public string EnumProp => nameof(EnumProp); + public IEnumerator GetEnumerator() => null!; + } + + {16} + record MyRecord : InterimRecord + { + {7}public string Field; + {8}public string PropGet { get; } + {9}public string PropGetSet { get; set; } + {10}public string PropPrivateGet { private get; set; } + {11}public string PropInternalGet { internal get; set; } + {12}public string PropProtectedGet { protected get; set; } + {13}public override string PropVirt => nameof(MyRecord); + {14}public string PropGetInit { get; init; } + public EnumerableStructRecord PropRecord { get; } = new(); + {15}public InlineStruct FieldRecord = new(nameof(FieldRecord)); + } + + partial class C + { + [LoggerMessage(LogLevel.Debug, ""Param is {p0}"")] + public static partial void LogTemplate(ILogger logger, IRedactorProvider provider, MyRecord /*0+*/p0/*-0*/); + + [LoggerMessage(LogLevel.Debug, ""No param here"")] + public static partial void LogStructured(ILogger logger, MyRecord /*1+*/p0/*-1*/); + + [LoggerMessage(LogLevel.Debug)] + public static partial void LogFullyStructured(ILogger logger, MyRecord /*2+*/p0/*-2*/); + + [LoggerMessage(LogLevel.Debug, ""Param is {p0}"")] + public static partial void LogProperties(ILogger logger, IRedactorProvider rp, [LogProperties] MyRecord /*3+*/p0/*-3*/); + }"); + + for (int i = 0; i < TotalSensitiveCases; i++) + { + var template = "{" + i.ToString() + "}"; + var replacement = i switch + { + _ when positionNumber == i + => "[PrivateData] ", + _ => string.Empty, + }; + + sb.Replace(template, replacement); + } + + await RunGenerator( + sb.ToString(), + DiagDescriptors.RecordTypeSensitiveArgumentIsInTemplate, + ignoreDiag: DiagDescriptors.ParameterHasNoCorrespondingTemplate); + } + + [Theory] + [InlineData("System.Nullable")] + [InlineData("int?")] + public async Task LogMethod_DetectsSensitiveNullableMembersInRecord(string type) + { + await RunGenerator(@$" + using Microsoft.Extensions.Compliance.Testing; + + record MyRecord + {{ + [PrivateData] + public {type} Field; + }} + + partial class C + {{ + [LoggerMessage(LogLevel.Debug, ""Param is {{p0}}"")] + public static partial void LogTemplate(ILogger logger, MyRecord /*0+*/p0/*-0*/); + }}", + DiagDescriptors.RecordTypeSensitiveArgumentIsInTemplate); + } + + [Theory] + [InlineData("[PrivateData]")] + [InlineData("[property: PrivateData]")] + public async Task LogMethod_DetectsSensitiveMembersInRecord_WithPrimaryCtor(string annotation) + { + await RunGenerator(@$" + using Microsoft.Extensions.Compliance.Testing; + + record MyRecord({annotation} string userIp); + + partial class C + {{ + [LoggerMessage(LogLevel.Debug, ""Param is {{p0}}"")] + public static partial void LogTemplate(ILogger logger, MyRecord /*0+*/p0/*-0*/); + }}", + DiagDescriptors.RecordTypeSensitiveArgumentIsInTemplate); + } + + [Fact] + public async Task LogMethod_SkipsNonSensitiveMembersInRecord() + { + await RunGenerator(@" + using Microsoft.Extensions.Compliance.Testing; + + public record class BaseRecord + { + [PrivateData] public const int ConstFieldBase = 99; + [PrivateData] public static int StaticFieldBase; + [PrivateData] public static int StaticPropBase => 100; + [PrivateData] private int PrivateFieldBase; + [PrivateData] internal int InternalFieldBase; + } + + public record class InterimRecord : BaseRecord + { + [PrivateData] public virtual decimal PropVirt => decimal.MinusOne; + } + + internal record class MyRecord : InterimRecord + { + [PrivateData] public const int ConstField = 99; + [PrivateData] public static int StaticField; + [PrivateData] public static int StaticProp => 100; + [PrivateData] private int PrivateField; + [PrivateData] internal int InternalField; + [PrivateData] private string PrivatePropGetSet { get; set; } + [PrivateData] internal string InternalPropGetSet { get; set; } + [PrivateData] protected string ProtectedPropGetSet { get; set; } + + // This one overrides 'virtual' property declared in 'InterimRecord': + public override decimal PropVirt => decimal.One; + } + + partial class C + { + [LoggerMessage(LogLevel.Debug, ""Param is {p0}"")] + public static partial void LogFunc(ILogger logger, IRedactorProvider provider, MyRecord /*0+*/p0/*-0*/); + }"); + } + [Fact] public async Task Templates() { @@ -773,7 +943,6 @@ private static async Task RunGenerator( text = $@" {nspaceStart} using Microsoft.Extensions.Logging; - using Microsoft.Extensions.Logging; {code} {nspaceEnd}"; } diff --git a/test/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/AcceptanceTest.cs b/test/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/AcceptanceTest.cs index 6ff29196f7e..c0e22bdb1c2 100644 --- a/test/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/AcceptanceTest.cs +++ b/test/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Linux/AcceptanceTest.cs @@ -155,12 +155,18 @@ public Task ResourceUtilizationTracker_Reports_The_Same_Values_As_One_Can_Observ var memoryFromGauge = 0.0d; using var e = new ManualResetEventSlim(); - listener.InstrumentPublished = (i, m) => + object? meterScope = null; + listener.InstrumentPublished = (instrument, meterListener) => { - if (i.Name == ResourceUtilizationInstruments.CpuUtilization - || i.Name == ResourceUtilizationInstruments.MemoryUtilization) + if (!ReferenceEquals(instrument.Meter.Scope, meterScope)) { - m.EnableMeasurementEvents(i); + return; + } + + if (instrument.Name == ResourceUtilizationInstruments.CpuUtilization || + instrument.Name == ResourceUtilizationInstruments.MemoryUtilization) + { + meterListener.EnableMeasurementEvents(instrument); } }; @@ -178,15 +184,17 @@ public Task ResourceUtilizationTracker_Reports_The_Same_Values_As_One_Can_Observ listener.Start(); - using var host = FakeHost.CreateBuilder().ConfigureServices(x => - x.AddLogging() - .AddSingleton(clock) - .AddSingleton(new FakeUserHz(100)) - .AddSingleton(fileSystem) - .AddSingleton(new GenericPublisher(_ => e.Set())) - .AddResourceMonitoring()) + using var host = FakeHost.CreateBuilder() + .ConfigureServices(x => + x.AddLogging() + .AddSingleton(clock) + .AddSingleton(new FakeUserHz(100)) + .AddSingleton(fileSystem) + .AddSingleton(new GenericPublisher(_ => e.Set())) + .AddResourceMonitoring()) .Build(); + meterScope = host.Services.GetRequiredService(); var tracker = host.Services.GetService(); Assert.NotNull(tracker); diff --git a/test/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Windows/WindowsCountersTests.cs b/test/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Windows/WindowsCountersTests.cs index cbd1ecee25c..2be00da8718 100644 --- a/test/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Windows/WindowsCountersTests.cs +++ b/test/Libraries/Microsoft.Extensions.Diagnostics.ResourceMonitoring.Tests/Windows/WindowsCountersTests.cs @@ -40,7 +40,10 @@ public void WindowsCounters_Registers_Instruments() { InstrumentPublished = (instrument, listener) => { - listener.EnableMeasurementEvents(instrument); + if (ReferenceEquals(meter, instrument.Meter)) + { + listener.EnableMeasurementEvents(instrument); + } } }; @@ -79,7 +82,7 @@ public void WindowsCounters_Got_Unsuccessful() { InstrumentPublished = (instrument, listener) => { - if (instrument.Meter == meter) + if (ReferenceEquals(meter, instrument.Meter)) { listener.EnableMeasurementEvents(instrument); }