Fix CA1873: Potentially expensive logging (#1695)

* Fix CA1873: Potentially expensive logging

* Refactor UriExtensions.ToMaskedString into MaskedUri struct with implicit conversion

* Mask request URLs in HTTP exchanges actuator

Author: bart-vmware

shared-package.props

@@ -33,14 +33,6 @@
     <GeneratePackageOnBuild>True</GeneratePackageOnBuild>
   </PropertyGroup>
 
-  <PropertyGroup>
-    <!--
-      Temporary workaround for CA1873: Evaluation of this argument may be expensive and unnecessary if logging is disabled.
-      Should be revisited as part of https://github.com/SteeltoeOSS/Steeltoe/issues/969.
-      -->
-    <NoWarn>$(NoWarn);CA1873</NoWarn>
-  </PropertyGroup>
-
   <PropertyGroup Condition="'$(CI)' != ''">
     <!--
       While deterministic builds are enabled by default in .NET SDK projects, there is an extra property, ContinuousIntegrationBuild,

src/Common/src/Common/Extensions/MaskedUri.cs

@@ -0,0 +1,59 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the Apache 2.0 License.
+// See the LICENSE file in the project root for more information.
+
+namespace Steeltoe.Common.Extensions;
+
+/// <summary>
+/// Represents a <see cref="Uri" /> whose username and password are masked.
+/// </summary>
+internal readonly record struct MaskedUri
+{
+    private readonly Uri? _value;
+
+    public MaskedUri(Uri? value)
+    {
+        _value = value;
+    }
+
+    public override string ToString()
+    {
+        return _value == null ? string.Empty : ToMaskedString(_value);
+    }
+
+    private static string ToMaskedString(Uri source)
+    {
+        string uris = source.ToString();
+
+        if (uris.Contains(','))
+        {
+            return string.Join(',',
+                uris.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries).Select(uri => Mask(new Uri(uri)).ToString()));
+        }
+
+        return Mask(source).ToString();
+    }
+
+    private static Uri Mask(Uri source)
+    {
+        if (string.IsNullOrEmpty(source.UserInfo))
+        {
+            return source;
+        }
+
+        var builder = new UriBuilder(source)
+        {
+            UserName = "****",
+#pragma warning disable S2068 // Hard-coded credentials are security-sensitive
+            Password = "****"
+#pragma warning restore S2068 // Hard-coded credentials are security-sensitive
+        };
+
+        return builder.Uri;
+    }
+
+    public static implicit operator MaskedUri(Uri? uri)
+    {
+        return new MaskedUri(uri);
+    }
+}

src/Common/src/Common/Extensions/UriExtensions.cs

@@ -9,39 +9,6 @@ namespace Steeltoe.Common.Extensions;
 
 internal static class UriExtensions
 {
-    public static string ToMaskedString(this Uri source)
-    {
-        ArgumentNullException.ThrowIfNull(source);
-
-        string uris = source.ToString();
-
-        if (uris.Contains(','))
-        {
-            return string.Join(',',
-                uris.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries).Select(uri => ToMaskedUri(new Uri(uri)).ToString()));
-        }
-
-        return ToMaskedUri(source).ToString();
-    }
-
-    private static Uri ToMaskedUri(Uri source)
-    {
-        if (string.IsNullOrEmpty(source.UserInfo))
-        {
-            return source;
-        }
-
-        var builder = new UriBuilder(source)
-        {
-            UserName = "****",
-#pragma warning disable S2068 // Hard-coded credentials are security-sensitive
-            Password = "****"
-#pragma warning restore S2068 // Hard-coded credentials are security-sensitive
-        };
-
-        return builder.Uri;
-    }
-
     public static bool TryGetUsernamePassword(this Uri uri, [NotNullWhen(true)] out string? username, [NotNullWhen(true)] out string? password)
     {
         ArgumentNullException.ThrowIfNull(uri);

src/Common/src/Http/HttpClientExtensions.cs

@@ -73,7 +73,8 @@ public static async Task<string> GetAccessTokenAsync(this HttpClient httpClient,
 
         if (string.IsNullOrEmpty(accessToken))
         {
-            throw new HttpRequestException($"No access token was returned from '{accessTokenUri.ToMaskedString()}'.", null, response.StatusCode);
+            MaskedUri masked = accessTokenUri;
+            throw new HttpRequestException($"No access token was returned from '{masked}'.", null, response.StatusCode);
         }
 
         return accessToken;

src/Common/test/Common.Test/Extensions/MaskedUriTest.cs

@@ -0,0 +1,43 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the Apache 2.0 License.
+// See the LICENSE file in the project root for more information.
+
+using Steeltoe.Common.Extensions;
+
+namespace Steeltoe.Common.Test.Extensions;
+
+public sealed class MaskedUriTest
+{
+    [Fact]
+    public void MaskSingleBasicAuthentication()
+    {
+        const string source = "http://username:password@www.example.com/";
+        const string expected = "http://****:****@www.example.com/";
+
+        MaskedUri uri = new Uri(source);
+
+        uri.ToString().Should().Be(expected);
+    }
+
+    [Fact]
+    public void MaskMultiBasicAuthentication()
+    {
+        const string source = "http://username:password@www.example.com/,http://user2:pass2@www.other.com/";
+        const string expected = "http://****:****@www.example.com/,http://****:****@www.other.com/";
+
+        MaskedUri uri = new Uri(source);
+
+        uri.ToString().Should().Be(expected);
+    }
+
+    [Fact]
+    public void DoNotMaskIfNoBasicAuthentication()
+    {
+        const string source = "http://www.example.com/";
+        const string expected = "http://www.example.com/";
+
+        MaskedUri uri = new Uri(source);
+
+        uri.ToString().Should().Be(expected);
+    }
+}

src/Common/test/Common.Test/Extensions/UriExtensionsTest.cs

@@ -8,39 +8,6 @@ namespace Steeltoe.Common.Test.Extensions;
 
 public sealed class UriExtensionsTest
 {
-    [Fact]
-    public void MaskSingleBasicAuthentication()
-    {
-        var uri = new Uri("http://username:password@www.example.com/");
-        const string expected = "http://****:****@www.example.com/";
-
-        string masked = uri.ToMaskedString();
-
-        masked.Should().Be(expected);
-    }
-
-    [Fact]
-    public void MaskMultiBasicAuthentication()
-    {
-        var uri = new Uri("http://username:password@www.example.com/,http://user2:pass2@www.other.com/");
-        const string expected = "http://****:****@www.example.com/,http://****:****@www.other.com/";
-
-        string masked = uri.ToMaskedString();
-
-        masked.Should().Be(expected);
-    }
-
-    [Fact]
-    public void DoNotMaskIfNoBasicAuthentication()
-    {
-        var uri = new Uri("http://www.example.com/");
-        string expected = uri.ToString();
-
-        string masked = uri.ToMaskedString();
-
-        masked.Should().Be(expected);
-    }
-
     [Fact]
     public void TryGetUsernamePassword_AllowsColonInPassword()
     {

src/Configuration/src/Abstractions/CompositeConfigurationProvider.cs

@@ -56,12 +56,7 @@ public IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string?
         ArgumentNullException.ThrowIfNull(earlierKeys);
 
         string[] earlierKeysArray = earlierKeys as string[] ?? earlierKeys.ToArray();
-
-        if (_logger.IsEnabled(LogLevel.Trace))
-        {
-            string earlierKeyNames = string.Join(", ", earlierKeysArray.Select(key => $"'{key}'"));
-            LogGetChildKeys(GetType().Name, earlierKeyNames, parentPath);
-        }
+        ExpensiveLogGetChildKeys(earlierKeysArray, parentPath);
 
         IConfiguration? section = parentPath == null ? ConfigurationRoot : ConfigurationRoot?.GetSection(parentPath);
 
@@ -77,6 +72,15 @@ public IEnumerable<string> GetChildKeys(IEnumerable<string> earlierKeys, string?
         return keys;
     }
 
+    private void ExpensiveLogGetChildKeys(string[] earlierKeysArray, string? parentPath)
+    {
+        if (_logger.IsEnabled(LogLevel.Trace))
+        {
+            string earlierKeyNames = string.Join(", ", earlierKeysArray.Select(key => $"'{key}'"));
+            LogGetChildKeys(GetType().Name, earlierKeyNames, parentPath);
+        }
+    }
+
     public virtual bool TryGet(string key, out string? value)
     {
         ArgumentNullException.ThrowIfNull(key);

src/Configuration/src/ConfigServer/ConfigServerConfigurationProvider.cs

@@ -403,7 +403,7 @@ public override void Load()
                 // Update configuration Data dictionary with any results
                 if (env != null)
                 {
-                    LogEnvironmentLocated(env.Name, string.Join(", ", env.Profiles.Select(p => $"'{p}'")), env.Label, env.Version, env.State);
+                    ExpensiveLogEnvironmentLocated(env);
 
                     if (updateDictionary)
                     {
@@ -451,6 +451,15 @@ public override void Load()
         throw new ConfigServerException("Failed fetching remote configuration from server(s).", error);
     }
 
+    private void ExpensiveLogEnvironmentLocated(ConfigEnvironment environment)
+    {
+        if (_logger.IsEnabled(LogLevel.Debug))
+        {
+            string profiles = string.Join(", ", environment.Profiles.Select(profile => $"'{profile}'"));
+            LogEnvironmentLocated(environment.Name, profiles, environment.Label, environment.Version, environment.State);
+        }
+    }
+
     internal void ApplyLastDiscoveryLookupResultToClientOptions(ConfigServerClientOptions optionsSnapshot)
     {
         DiscoveryLookupResult? lastResult = _lastDiscoveryLookupResult;
@@ -601,7 +610,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
 
         if (requestUri.TryGetUsernamePassword(out string? username, out string? password) && password.Length > 0)
         {
-            LogAddingCredentials(requestUri.ToMaskedString());
+            LogAddingCredentials(requestUri);
 
             requestMessage.Headers.Authorization =
                 new AuthenticationHeaderValue("Basic", Convert.ToBase64String(Encoding.ASCII.GetBytes($"{username}:{password}")));
@@ -616,7 +625,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
                 string accessToken =
                     await httpClient.GetAccessTokenAsync(accessTokenUri, optionsSnapshot.ClientId, optionsSnapshot.ClientSecret, cancellationToken);
 
-                LogAccessTokenFetched(accessTokenUri.ToMaskedString());
+                LogAccessTokenFetched(accessTokenUri);
                 requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);
             }
         }
@@ -646,7 +655,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
                 // Make Config Server URI from settings
                 Uri uri = BuildConfigServerUri(optionsSnapshot, requestUri, label);
 
-                LogTryingToConnect(uri.ToMaskedString());
+                LogTryingToConnect(uri);
                 HttpRequestMessage request;
 
                 try
@@ -660,7 +669,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
                     if (!string.IsNullOrEmpty(optionsSnapshot.AccessTokenUri))
                     {
                         var accessTokenUri = new Uri(optionsSnapshot.AccessTokenUri);
-                        LogFailedToFetchAccessToken(exception, accessTokenUri.ToMaskedString());
+                        LogFailedToFetchAccessToken(exception, accessTokenUri);
 
                         continue;
                     }
@@ -672,7 +681,7 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
                 LogSendingHttpRequest();
                 using HttpResponseMessage response = await httpClient.SendAsync(request, cancellationToken);
 
-                LogConfigServerReturnedStatus(response.StatusCode, uri.ToMaskedString());
+                LogConfigServerReturnedStatus(uri, response.StatusCode);
 
                 if (response.StatusCode != HttpStatusCode.OK)
                 {
@@ -684,7 +693,8 @@ internal async Task<HttpRequestMessage> GetConfigServerRequestMessageAsync(Confi
                     // Throw if status >= 400
                     if (response.StatusCode >= HttpStatusCode.BadRequest)
                     {
-                        throw new HttpRequestException($"Config Server returned status: {response.StatusCode} invoking path: {uri.ToMaskedString()}");
+                        MaskedUri masked = uri;
+                        throw new HttpRequestException($"Config Server returned status: {response.StatusCode} invoking path: {masked}");
                     }
 
                     return null;
@@ -827,7 +837,7 @@ internal async Task RefreshVaultTokenAsync(ConfigServerClientOptions optionsSnap
             Uri uri = BuildVaultRenewUri(optionsSnapshot);
             HttpRequestMessage message = await GetVaultRenewRequestMessageAsync(optionsSnapshot, uri, cancellationToken);
 
-            LogRenewingVaultToken(obscuredToken, optionsSnapshot.TokenTtl, uri.ToMaskedString());
+            LogRenewingVaultToken(obscuredToken, optionsSnapshot.TokenTtl, uri);
             using HttpResponseMessage response = await httpClient.SendAsync(message, cancellationToken);
 
             if (response.StatusCode != HttpStatusCode.OK)
@@ -867,7 +877,7 @@ private async Task<HttpRequestMessage> GetVaultRenewRequestMessageAsync(ConfigSe
             string accessToken =
                 await httpClient.GetAccessTokenAsync(accessTokenUri, optionsSnapshot.ClientId, optionsSnapshot.ClientSecret, cancellationToken);
 
-            LogAccessTokenFetched(accessTokenUri.ToMaskedString());
+            LogAccessTokenFetched(accessTokenUri);
             requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);
         }
 
@@ -1019,7 +1029,7 @@ private void ShutdownTimers()
     [LoggerMessage(Level = LogLevel.Debug, Message = "Multiple Config Server uris listed.")]
     private partial void LogMultipleConfigServerUris();
 
-    [LoggerMessage(Level = LogLevel.Debug,
+    [LoggerMessage(Level = LogLevel.Debug, SkipEnabledCheck = true,
         Message = "Located environment with name {Name}, profiles {Profiles}, label {Label}, version {Version} and state {State}.")]
     private partial void LogEnvironmentLocated(string? name, string profiles, string? label, string? version, string? state);
 
@@ -1030,22 +1040,22 @@ private void ShutdownTimers()
     private partial void LogDataNotChanged();
 
     [LoggerMessage(Level = LogLevel.Warning, Message = "Failed fetching remote configuration from server(s).")]
-    private partial void LogFetchingRemoteConfigurationFailed(Exception? error);
+    private partial void LogFetchingRemoteConfigurationFailed(Exception error);
 
     [LoggerMessage(Level = LogLevel.Debug, Message = "Adding credentials from '{RequestUri}' to Authorization header.")]
-    private partial void LogAddingCredentials(string requestUri);
+    private partial void LogAddingCredentials(MaskedUri requestUri);
 
     [LoggerMessage(Level = LogLevel.Debug, Message = "Fetched access token from {AccessTokenUri}.")]
-    private partial void LogAccessTokenFetched(string accessTokenUri);
+    private partial void LogAccessTokenFetched(MaskedUri accessTokenUri);
 
     [LoggerMessage(Level = LogLevel.Warning, Message = "Failed to fetch access token from '{AccessTokenUri}'.")]
-    private partial void LogFailedToFetchAccessToken(Exception exception, string accessTokenUri);
+    private partial void LogFailedToFetchAccessToken(Exception exception, MaskedUri accessTokenUri);
 
     [LoggerMessage(Level = LogLevel.Trace, Message = "Entered {Method}.")]
     private partial void LogRemoteLoadEntered(string method);
 
     [LoggerMessage(Level = LogLevel.Debug, Message = "Trying to connect to Config Server at {RequestUri}.")]
-    private partial void LogTryingToConnect(string requestUri);
+    private partial void LogTryingToConnect(MaskedUri requestUri);
 
     [LoggerMessage(Level = LogLevel.Trace, Message = "Building HTTP request message.")]
     private partial void LogBuildingHttpRequest();
@@ -1054,7 +1064,7 @@ private void ShutdownTimers()
     private partial void LogSendingHttpRequest();
 
     [LoggerMessage(Level = LogLevel.Debug, Message = "Config Server returned status {StatusCode} for path {RequestUri}.")]
-    private partial void LogConfigServerReturnedStatus(HttpStatusCode statusCode, string requestUri);
+    private partial void LogConfigServerReturnedStatus(MaskedUri requestUri, HttpStatusCode statusCode);
 
     [LoggerMessage(Level = LogLevel.Trace, Message = "Parsing JSON response.")]
     private partial void LogParsingJsonResponse();
@@ -1066,7 +1076,7 @@ private void ShutdownTimers()
     private partial void LogConfigServerPropertyException(Exception exception, string key, Type type);
 
     [LoggerMessage(Level = LogLevel.Debug, Message = "Renewing Vault token {Token} for {Ttl} milliseconds at Uri {Uri}.")]
-    private partial void LogRenewingVaultToken(string token, int ttl, string uri);
+    private partial void LogRenewingVaultToken(string token, int ttl, MaskedUri uri);
 
     [LoggerMessage(Level = LogLevel.Warning, Message = "Renewing Vault token {Token} returned status {Status}.")]
     private partial void LogVaultTokenRenewalStatus(string token, HttpStatusCode status);