From a3a9287498381f7155a2ff7b7a94b1b17994251f Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Wed, 5 Aug 2026 03:12:25 +0100 Subject: [PATCH 1/3] perf: skip logging scans without secrets --- .../Console/ModuleOutputBuffer.cs | 9 ++++++-- .../Engine/ISecretObfuscator.cs | 11 ++++++++- .../Engine/SecretObfuscator.cs | 3 +++ .../Logging/FormattedLogValuesObfuscator.cs | 10 ++++++-- .../Logging/ObfuscatedLogException.cs | 16 +++++++------ .../Console/ModuleOutputBufferTests.cs | 3 +++ .../FormattedLogValuesObfuscatorTests.cs | 23 +++++++++++++++++++ .../Logging/PipelineLevelLoggerTests.cs | 21 +++++++++++++++++ .../Logging/SecretObfuscatorCachingTests.cs | 19 +++++++++++++++ 9 files changed, 103 insertions(+), 12 deletions(-) diff --git a/src/ModularPipelines/Console/ModuleOutputBuffer.cs b/src/ModularPipelines/Console/ModuleOutputBuffer.cs index df990b54f9c..31e80969ce2 100644 --- a/src/ModularPipelines/Console/ModuleOutputBuffer.cs +++ b/src/ModularPipelines/Console/ModuleOutputBuffer.cs @@ -813,10 +813,15 @@ public void WriteTo(ILogger logger) public string? FormatException() => _obfuscatedException is null ? null - : secretObfuscator.Obfuscate(_obfuscatedException.ToString(), null); + : Obfuscate(_obfuscatedException.ToString()); private string Format(object? state, Exception? logException) - => secretObfuscator.Obfuscate(_rawFormattedMessage.Value, null) ?? string.Empty; + => Obfuscate(_rawFormattedMessage.Value); + + private string Obfuscate(string value) + => secretObfuscator.HasSecrets + ? secretObfuscator.Obfuscate(value, null) + : value; private string FormatTyped(TState state, Exception? logException) => Format(state!, logException); diff --git a/src/ModularPipelines/Engine/ISecretObfuscator.cs b/src/ModularPipelines/Engine/ISecretObfuscator.cs index 06dcbd92acb..5cda1ec1bb4 100644 --- a/src/ModularPipelines/Engine/ISecretObfuscator.cs +++ b/src/ModularPipelines/Engine/ISecretObfuscator.cs @@ -5,6 +5,15 @@ namespace ModularPipelines.Engine; /// public interface ISecretObfuscator { + /// + /// Gets whether any secrets are currently registered for global masking. + /// + /// + /// The conservative default preserves masking for custom implementations that do not + /// expose their registration state. + /// + bool HasSecrets => true; + /// /// Obfuscates sensitive information in the provided input. /// @@ -12,4 +21,4 @@ public interface ISecretObfuscator /// An options object that may contain sensitive properties. /// The input with sensitive information obfuscated. string Obfuscate(string? input, object? optionsObject); -} \ No newline at end of file +} diff --git a/src/ModularPipelines/Engine/SecretObfuscator.cs b/src/ModularPipelines/Engine/SecretObfuscator.cs index 1092cb0c714..e1ef91cf2d0 100644 --- a/src/ModularPipelines/Engine/SecretObfuscator.cs +++ b/src/ModularPipelines/Engine/SecretObfuscator.cs @@ -34,6 +34,9 @@ internal class SecretObfuscator : ISecretObfuscator, IInitializer public int Order => int.MaxValue; + public bool HasSecrets => + GetRegisteredSecretCache(_maskingOptions.Value.CaseInsensitive).SearchValues is not null; + public SecretObfuscator( ISecretProvider secretProvider, IOptions maskingOptions) diff --git a/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs b/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs index 74e9d64be60..f934313acf9 100644 --- a/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs +++ b/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs @@ -31,6 +31,11 @@ public FormattedLogValuesObfuscator(ISecretObfuscator secretObfuscator) public object TryObfuscateValues(object state) { + if (!_secretObfuscator.HasSecrets) + { + return state; + } + if (state is not IReadOnlyList> values) { return ObfuscateValue(state); @@ -69,7 +74,7 @@ private object ObfuscateValue(object value) string originalValue; try { - originalValue = value.ToString() ?? string.Empty; + originalValue = value as string ?? value.ToString() ?? string.Empty; } catch (Exception) { @@ -77,7 +82,8 @@ private object ObfuscateValue(object value) } var obfuscatedValue = _secretObfuscator.Obfuscate(originalValue, null); - return obfuscatedValue.Equals(originalValue, StringComparison.Ordinal) + return ReferenceEquals(obfuscatedValue, originalValue) + || obfuscatedValue.Equals(originalValue, StringComparison.Ordinal) ? value : obfuscatedValue; } diff --git a/src/ModularPipelines/Logging/ObfuscatedLogException.cs b/src/ModularPipelines/Logging/ObfuscatedLogException.cs index 8a2e6173e4c..bc676c3dbe9 100644 --- a/src/ModularPipelines/Logging/ObfuscatedLogException.cs +++ b/src/ModularPipelines/Logging/ObfuscatedLogException.cs @@ -31,13 +31,15 @@ private ObfuscatedLogException(Exception exception, ISecretObfuscator secretObfu } public static Exception? Create(Exception? exception, ISecretObfuscator secretObfuscator) - => exception switch - { - null => null, - AggregateException aggregateException => - new ObfuscatedAggregateLogException(aggregateException, secretObfuscator), - _ => new ObfuscatedLogException(exception, secretObfuscator), - }; + => exception is null || !secretObfuscator.HasSecrets + ? exception + : exception switch + { + null => null, + AggregateException aggregateException => + new ObfuscatedAggregateLogException(aggregateException, secretObfuscator), + _ => new ObfuscatedLogException(exception, secretObfuscator), + }; public override string? StackTrace => _obfuscatedStackTrace; diff --git a/test/ModularPipelines.UnitTests/Console/ModuleOutputBufferTests.cs b/test/ModularPipelines.UnitTests/Console/ModuleOutputBufferTests.cs index c3751aae665..47ade244ffe 100644 --- a/test/ModularPipelines.UnitTests/Console/ModuleOutputBufferTests.cs +++ b/test/ModularPipelines.UnitTests/Console/ModuleOutputBufferTests.cs @@ -203,6 +203,7 @@ public async Task BufferedLogEvent_FormatsOnceAndObfuscatesEveryTime() { var formatterCalls = 0; var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => value?.Replace("secret", "***") ?? string.Empty); @@ -236,6 +237,7 @@ public async Task BufferedLogEvent_ReobfuscatesMessageWithCurrentSecrets() const string secret = "late-registered-secret"; var redactSecret = false; var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(() => redactSecret); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => redactSecret @@ -268,6 +270,7 @@ public async Task BufferedLogEvent_ReobfuscatesExceptionWithCurrentSecrets() const string secret = "late-registered-secret"; var redactSecret = false; var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(() => redactSecret); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => redactSecret diff --git a/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs b/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs index 893db56a1c1..c8340c82c17 100644 --- a/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs +++ b/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs @@ -7,6 +7,22 @@ namespace ModularPipelines.UnitTests.Logging; public class FormattedLogValuesObfuscatorTests { + [Test] + public async Task TryObfuscateValues_DoesNotInspectStateWhenNoSecretsAreRegistered() + { + var state = new ThrowingToStringState(); + var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(false); + + var obfuscatedState = new FormattedLogValuesObfuscator(secretObfuscator.Object) + .TryObfuscateValues(state); + + await Assert.That(obfuscatedState).IsSameReferenceAs(state); + secretObfuscator.Verify( + x => x.Obfuscate(It.IsAny(), It.IsAny()), + Times.Never); + } + [Test] public async Task TryObfuscateValues_MasksSecretsInOriginalFormat() { @@ -17,6 +33,7 @@ public async Task TryObfuscateValues_MasksSecretsInOriginalFormat() var state = logger.Invocations.Single(x => x.Method.Name == nameof(ILogger.Log)).Arguments[2]; var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => (value ?? string.Empty).Replace(secret, "********", StringComparison.Ordinal)); @@ -43,6 +60,7 @@ public async Task TryObfuscateValues_PreservesUnmaskedStructuredValueTypes() var state = logger.Invocations.Single(x => x.Method.Name == nameof(ILogger.Log)).Arguments[2]; var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => value == "secret" ? "********" : value ?? string.Empty); @@ -66,6 +84,7 @@ public async Task TryObfuscateValues_MasksValueTypeSecrets() var state = logger.Invocations.Single(x => x.Method.Name == nameof(ILogger.Log)).Arguments[2]; var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => value == secret.ToString() ? "********" : value ?? string.Empty); @@ -82,6 +101,7 @@ public async Task TryObfuscateValues_MasksCustomStructuredLogStates() { var state = new ModuleCompletionLogState("secret", TimeSpan.FromSeconds(1), "(none)", 0, 0); var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => value == "secret" ? "********" : value ?? string.Empty); @@ -98,6 +118,7 @@ public async Task TryObfuscateValues_MasksUnstructuredState() { const string secret = "plain-state-secret"; var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => @@ -114,6 +135,7 @@ public async Task TryObfuscateValues_PreservesStateWhenToStringThrows() { var state = new ThrowingToStringState(); var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); var obfuscatedState = new FormattedLogValuesObfuscator(secretObfuscator.Object) .TryObfuscateValues(state); @@ -128,6 +150,7 @@ public async Task TryObfuscateValues_PreservesStateWhenToStringThrows() public async Task TryObfuscateValues_DoesNotRescanPreObfuscatedValues() { var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); var state = new[] { new KeyValuePair( diff --git a/test/ModularPipelines.UnitTests/Logging/PipelineLevelLoggerTests.cs b/test/ModularPipelines.UnitTests/Logging/PipelineLevelLoggerTests.cs index 7831be28de3..89e94617b43 100644 --- a/test/ModularPipelines.UnitTests/Logging/PipelineLevelLoggerTests.cs +++ b/test/ModularPipelines.UnitTests/Logging/PipelineLevelLoggerTests.cs @@ -144,6 +144,26 @@ public async Task Log_ObfuscatesStateMessageAndExceptionBeforeDelegating() await Assert.That(underlyingLogger.Exception?.ToString()).DoesNotContain(secret); } + [Test] + public async Task Log_PreservesOriginalExceptionWhenNoSecretsAreRegistered() + { + var underlyingLogger = new RecordingLogger(); + var originalException = new InvalidOperationException("Failure"); + var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(false); + var pipelineLevelLogger = new PipelineLevelLogger( + underlyingLogger, + secretObfuscator.Object, + new FormattedLogValuesObfuscator(secretObfuscator.Object)); + + pipelineLevelLogger.LogError(originalException, "Failure"); + + await Assert.That(underlyingLogger.Exception).IsSameReferenceAs(originalException); + secretObfuscator.Verify( + x => x.Obfuscate(It.IsAny(), It.IsAny()), + Times.Never); + } + [Test] public async Task Log_PreservesSanitizedExceptionDiagnostics() { @@ -318,6 +338,7 @@ private static PipelineLevelLogger CreateLogger( Func? obfuscate = null) { var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => obfuscate?.Invoke(value) ?? value ?? string.Empty); diff --git a/test/ModularPipelines.UnitTests/Logging/SecretObfuscatorCachingTests.cs b/test/ModularPipelines.UnitTests/Logging/SecretObfuscatorCachingTests.cs index c67a79e1d43..45729b24ef5 100644 --- a/test/ModularPipelines.UnitTests/Logging/SecretObfuscatorCachingTests.cs +++ b/test/ModularPipelines.UnitTests/Logging/SecretObfuscatorCachingTests.cs @@ -8,6 +8,25 @@ namespace ModularPipelines.UnitTests.Logging; public class SecretObfuscatorCachingTests { + [Test] + public async Task HasSecrets_TracksDynamicSecretRegistration() + { + var optionsProvider = new Mock(); + optionsProvider.Setup(x => x.GetOptions()).Returns([]); + var secretProvider = new SecretProvider( + optionsProvider.Object, + Mock.Of(), + Microsoft.Extensions.Options.Options.Create(new SecretMaskingOptions()), + Mock.Of>()); + var obfuscator = CreateObfuscator(secretProvider); + + await Assert.That(obfuscator.HasSecrets).IsFalse(); + + secretProvider.AddSecret("dynamic-secret"); + + await Assert.That(obfuscator.HasSecrets).IsTrue(); + } + [Test] public async Task ReusesSecretSnapshotUntilProviderChanges() { From e5c82690aa5044ed84911a589a24cba8153cfe0b Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Mon, 10 Aug 2026 03:29:22 +0100 Subject: [PATCH 2/3] fix(logging): keep zero-secret paths safe Refs #3756 --- .../Console/ModuleOutputBuffer.cs | 4 +- .../Engine/ISecretObfuscator.cs | 4 +- .../Logging/FormattedLogValuesObfuscator.cs | 22 +++++--- .../Logging/ObfuscatedLogException.cs | 16 +++--- .../FormattedLogValuesObfuscatorTests.cs | 22 ++++++++ .../Logging/PipelineLevelLoggerTests.cs | 50 ++++++++++++++++--- 6 files changed, 91 insertions(+), 27 deletions(-) diff --git a/src/ModularPipelines/Console/ModuleOutputBuffer.cs b/src/ModularPipelines/Console/ModuleOutputBuffer.cs index 31e80969ce2..92f99339d6c 100644 --- a/src/ModularPipelines/Console/ModuleOutputBuffer.cs +++ b/src/ModularPipelines/Console/ModuleOutputBuffer.cs @@ -819,9 +819,7 @@ private string Format(object? state, Exception? logException) => Obfuscate(_rawFormattedMessage.Value); private string Obfuscate(string value) - => secretObfuscator.HasSecrets - ? secretObfuscator.Obfuscate(value, null) - : value; + => secretObfuscator.Obfuscate(value, null); private string FormatTyped(TState state, Exception? logException) => Format(state!, logException); diff --git a/src/ModularPipelines/Engine/ISecretObfuscator.cs b/src/ModularPipelines/Engine/ISecretObfuscator.cs index 5cda1ec1bb4..4abfaafad08 100644 --- a/src/ModularPipelines/Engine/ISecretObfuscator.cs +++ b/src/ModularPipelines/Engine/ISecretObfuscator.cs @@ -9,8 +9,8 @@ public interface ISecretObfuscator /// Gets whether any secrets are currently registered for global masking. /// /// - /// The conservative default preserves masking for custom implementations that do not - /// expose their registration state. + /// This is a performance hint only. Callers must not use it to bypass safety or masking + /// behavior that custom implementations may provide. /// bool HasSecrets => true; diff --git a/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs b/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs index f934313acf9..c81b54916e2 100644 --- a/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs +++ b/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs @@ -31,14 +31,11 @@ public FormattedLogValuesObfuscator(ISecretObfuscator secretObfuscator) public object TryObfuscateValues(object state) { - if (!_secretObfuscator.HasSecrets) - { - return state; - } + var hasSecrets = _secretObfuscator.HasSecrets; if (state is not IReadOnlyList> values) { - return ObfuscateValue(state); + return hasSecrets ? ObfuscateValue(state) : state; } KeyValuePair[]? obfuscatedValues = null; @@ -51,7 +48,20 @@ public object TryObfuscateValues(object state) continue; } - var obfuscatedValue = ObfuscateValue(property.Value); + object obfuscatedValue; + if (property.Value is PreObfuscatedLogValue preObfuscatedValue) + { + obfuscatedValue = preObfuscatedValue.Value; + } + else if (hasSecrets) + { + obfuscatedValue = ObfuscateValue(property.Value); + } + else + { + continue; + } + if (ReferenceEquals(obfuscatedValue, property.Value)) { continue; diff --git a/src/ModularPipelines/Logging/ObfuscatedLogException.cs b/src/ModularPipelines/Logging/ObfuscatedLogException.cs index bc676c3dbe9..8a2e6173e4c 100644 --- a/src/ModularPipelines/Logging/ObfuscatedLogException.cs +++ b/src/ModularPipelines/Logging/ObfuscatedLogException.cs @@ -31,15 +31,13 @@ private ObfuscatedLogException(Exception exception, ISecretObfuscator secretObfu } public static Exception? Create(Exception? exception, ISecretObfuscator secretObfuscator) - => exception is null || !secretObfuscator.HasSecrets - ? exception - : exception switch - { - null => null, - AggregateException aggregateException => - new ObfuscatedAggregateLogException(aggregateException, secretObfuscator), - _ => new ObfuscatedLogException(exception, secretObfuscator), - }; + => exception switch + { + null => null, + AggregateException aggregateException => + new ObfuscatedAggregateLogException(aggregateException, secretObfuscator), + _ => new ObfuscatedLogException(exception, secretObfuscator), + }; public override string? StackTrace => _obfuscatedStackTrace; diff --git a/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs b/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs index c8340c82c17..7c13e5523d1 100644 --- a/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs +++ b/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs @@ -168,6 +168,28 @@ public async Task TryObfuscateValues_DoesNotRescanPreObfuscatedValues() Times.Never); } + [Test] + public async Task TryObfuscateValues_UnwrapsPreObfuscatedValuesWithoutSecrets() + { + var secretObfuscator = new Mock(); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(false); + var state = new[] + { + new KeyValuePair( + "CommandOutput", + new PreObfuscatedLogValue("already-masked")), + }; + + var obfuscatedState = new FormattedLogValuesObfuscator(secretObfuscator.Object) + .TryObfuscateValues(state); + var value = ((IReadOnlyList>) obfuscatedState)[0].Value; + + await Assert.That(value).IsEqualTo("already-masked"); + secretObfuscator.Verify( + x => x.Obfuscate(It.IsAny(), It.IsAny()), + Times.Never); + } + private sealed class ThrowingToStringState { public override string ToString() => throw new InvalidOperationException("Cannot format state."); diff --git a/test/ModularPipelines.UnitTests/Logging/PipelineLevelLoggerTests.cs b/test/ModularPipelines.UnitTests/Logging/PipelineLevelLoggerTests.cs index 89e94617b43..c1171d3939e 100644 --- a/test/ModularPipelines.UnitTests/Logging/PipelineLevelLoggerTests.cs +++ b/test/ModularPipelines.UnitTests/Logging/PipelineLevelLoggerTests.cs @@ -145,12 +145,15 @@ public async Task Log_ObfuscatesStateMessageAndExceptionBeforeDelegating() } [Test] - public async Task Log_PreservesOriginalExceptionWhenNoSecretsAreRegistered() + public async Task Log_WrapsExceptionWhenNoSecretsAreRegistered() { var underlyingLogger = new RecordingLogger(); var originalException = new InvalidOperationException("Failure"); var secretObfuscator = new Mock(); secretObfuscator.SetupGet(x => x.HasSecrets).Returns(false); + secretObfuscator + .Setup(x => x.Obfuscate(It.IsAny(), null)) + .Returns((string? value, object? _) => value ?? string.Empty); var pipelineLevelLogger = new PipelineLevelLogger( underlyingLogger, secretObfuscator.Object, @@ -158,10 +161,7 @@ public async Task Log_PreservesOriginalExceptionWhenNoSecretsAreRegistered() pipelineLevelLogger.LogError(originalException, "Failure"); - await Assert.That(underlyingLogger.Exception).IsSameReferenceAs(originalException); - secretObfuscator.Verify( - x => x.Obfuscate(It.IsAny(), It.IsAny()), - Times.Never); + await Assert.That(underlyingLogger.Exception).IsNotSameReferenceAs(originalException); } [Test] @@ -219,6 +219,20 @@ await Assert.That(underlyingLogger.Exception?.ToString()) } } + [Test] + public async Task Log_GuardsHostileExceptionDiagnosticsWithoutSecrets() + { + var underlyingLogger = new RecordingLogger(); + var pipelineLevelLogger = CreateLogger(underlyingLogger, hasSecrets: false); + + await Assert.That( + () => pipelineLevelLogger.LogError(new ThrowingDiagnosticException(), "Failure")) + .ThrowsNothing(); + + await Assert.That(underlyingLogger.Exception?.Message) + .IsEqualTo(LoggingConstants.SecretMask); + } + [Test] public async Task Log_GuardsHostileStructuredTraversal() { @@ -240,6 +254,27 @@ await Assert.That(() => pipelineLevelLogger.Log( } } + [Test] + public async Task Log_GuardsHostileStructuredTraversalWithoutSecrets() + { + var underlyingLogger = new RecordingLogger(); + var pipelineLevelLogger = CreateLogger(underlyingLogger, hasSecrets: false); + + await Assert.That(() => pipelineLevelLogger.Log( + LogLevel.Information, + new EventId(3, "HostileState"), + new ThrowingCountStructuredState(), + null, + static (_, _) => throw new InvalidOperationException("Cannot format state."))) + .ThrowsNothing(); + + using (Assert.Multiple()) + { + await Assert.That(underlyingLogger.State).IsEqualTo(LoggingConstants.SecretMask); + await Assert.That(underlyingLogger.Message).IsEqualTo(LoggingConstants.SecretMask); + } + } + [Test] public async Task Log_PreservesEverySanitizedAggregateExceptionBranch() { @@ -335,10 +370,11 @@ await Assert.That( private static PipelineLevelLogger CreateLogger( ILogger logger, - Func? obfuscate = null) + Func? obfuscate = null, + bool hasSecrets = true) { var secretObfuscator = new Mock(); - secretObfuscator.SetupGet(x => x.HasSecrets).Returns(true); + secretObfuscator.SetupGet(x => x.HasSecrets).Returns(hasSecrets); secretObfuscator .Setup(x => x.Obfuscate(It.IsAny(), null)) .Returns((string? value, object? _) => obfuscate?.Invoke(value) ?? value ?? string.Empty); From 5caccd44c526d1c23b7b10a5ba5019ec3ea3759e Mon Sep 17 00:00:00 2001 From: Tom Longhurst <30480171+thomhurst@users.noreply.github.com> Date: Mon, 10 Aug 2026 03:56:40 +0100 Subject: [PATCH 3/3] fix(logging): make secret hint fail-safe --- .../Engine/SecretObfuscator.cs | 11 ++- .../Logging/FormattedLogValuesObfuscator.cs | 29 +++++- .../FormattedLogValuesObfuscatorTests.cs | 88 +++++++++++++++++-- 3 files changed, 118 insertions(+), 10 deletions(-) diff --git a/src/ModularPipelines/Engine/SecretObfuscator.cs b/src/ModularPipelines/Engine/SecretObfuscator.cs index e1ef91cf2d0..3e709839e33 100644 --- a/src/ModularPipelines/Engine/SecretObfuscator.cs +++ b/src/ModularPipelines/Engine/SecretObfuscator.cs @@ -34,8 +34,7 @@ internal class SecretObfuscator : ISecretObfuscator, IInitializer public int Order => int.MaxValue; - public bool HasSecrets => - GetRegisteredSecretCache(_maskingOptions.Value.CaseInsensitive).SearchValues is not null; + public bool HasSecrets => GetRegistrationState().HasSecrets; public SecretObfuscator( ISecretProvider secretProvider, @@ -81,6 +80,12 @@ public string Obfuscate(string? input, object? optionsObject) caseInsensitive ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal); } + internal SecretRegistrationState GetRegistrationState() + { + var cache = GetRegisteredSecretCache(_maskingOptions.Value.CaseInsensitive); + return new SecretRegistrationState(cache.Version, cache.SearchValues is not null); + } + internal SecretCache GetSecretCache( object? optionsObject, SecretMaskingOptions options, @@ -260,4 +265,6 @@ internal sealed record SecretCache( string[] Secrets, IReadOnlySet ExactSecrets, SearchValues? SearchValues); + + internal readonly record struct SecretRegistrationState(long Version, bool HasSecrets); } diff --git a/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs b/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs index c81b54916e2..055171d4037 100644 --- a/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs +++ b/src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs @@ -31,11 +31,34 @@ public FormattedLogValuesObfuscator(ISecretObfuscator secretObfuscator) public object TryObfuscateValues(object state) { - var hasSecrets = _secretObfuscator.HasSecrets; + // HasSecrets is only a hint; custom obfuscators may still apply policy-based masking. + if (_secretObfuscator is not SecretObfuscator builtInObfuscator) + { + return TryObfuscateValues(state, skipOrdinaryValues: false); + } + + var registrationState = builtInObfuscator.GetRegistrationState(); + var skipOrdinaryValues = !registrationState.HasSecrets; + var obfuscatedState = TryObfuscateValues(state, skipOrdinaryValues); + if (!skipOrdinaryValues) + { + return obfuscatedState; + } + + // A secret may be registered while the zero-secret fast path traverses the state. + var currentRegistrationState = builtInObfuscator.GetRegistrationState(); + return currentRegistrationState.Version != registrationState.Version + && currentRegistrationState.HasSecrets + ? TryObfuscateValues(state, skipOrdinaryValues: false) + : obfuscatedState; + } + + private object TryObfuscateValues(object state, bool skipOrdinaryValues) + { if (state is not IReadOnlyList> values) { - return hasSecrets ? ObfuscateValue(state) : state; + return skipOrdinaryValues ? state : ObfuscateValue(state); } KeyValuePair[]? obfuscatedValues = null; @@ -53,7 +76,7 @@ public object TryObfuscateValues(object state) { obfuscatedValue = preObfuscatedValue.Value; } - else if (hasSecrets) + else if (!skipOrdinaryValues) { obfuscatedValue = ObfuscateValue(property.Value); } diff --git a/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs b/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs index 7c13e5523d1..17ff3d31892 100644 --- a/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs +++ b/test/ModularPipelines.UnitTests/Logging/FormattedLogValuesObfuscatorTests.cs @@ -1,6 +1,7 @@ using Microsoft.Extensions.Logging; using ModularPipelines.Engine; using ModularPipelines.Logging; +using ModularPipelines.Options; using Moq; namespace ModularPipelines.UnitTests.Logging; @@ -10,17 +11,72 @@ public class FormattedLogValuesObfuscatorTests [Test] public async Task TryObfuscateValues_DoesNotInspectStateWhenNoSecretsAreRegistered() { - var state = new ThrowingToStringState(); + var state = new CountingToStringState(); + var secretObfuscator = CreateBuiltInObfuscator(); + + var obfuscatedState = new FormattedLogValuesObfuscator(secretObfuscator) + .TryObfuscateValues(state); + + await Assert.That(obfuscatedState).IsSameReferenceAs(state); + await Assert.That(state.ToStringCalls).IsEqualTo(0); + } + + [Test] + public async Task TryObfuscateValues_PreservesCustomMaskingWhenHintIsFalse() + { + const string secret = "policy-secret"; var secretObfuscator = new Mock(); secretObfuscator.SetupGet(x => x.HasSecrets).Returns(false); + secretObfuscator + .Setup(x => x.Obfuscate(It.IsAny(), null)) + .Returns((string? value, object? _) => + (value ?? string.Empty).Replace(secret, "********", StringComparison.Ordinal)); + var state = new[] + { + new KeyValuePair("PolicyValue", secret), + }; var obfuscatedState = new FormattedLogValuesObfuscator(secretObfuscator.Object) .TryObfuscateValues(state); + var value = ((IReadOnlyList>) obfuscatedState)[0].Value; - await Assert.That(obfuscatedState).IsSameReferenceAs(state); - secretObfuscator.Verify( - x => x.Obfuscate(It.IsAny(), It.IsAny()), - Times.Never); + await Assert.That(value).IsEqualTo("********"); + } + + [Test] + public async Task TryObfuscateValues_RetriesWhenSecretIsRegisteredDuringFastPath() + { + const string secret = "dynamic-secret"; + var version = 0L; + IReadOnlyList secrets = []; + var secretProvider = new Mock(); + secretProvider.SetupGet(x => x.Version).Returns(() => version); + secretProvider.Setup(x => x.GetSnapshot()) + .Returns(() => new SecretSnapshot(version, secrets)); + var secretObfuscator = CreateBuiltInObfuscator(secretProvider.Object); + var values = new[] { new KeyValuePair("Value", secret) }; + var registered = false; + var state = new Mock>>(); + state.SetupGet(x => x.Count).Returns(() => + { + if (!registered) + { + registered = true; + secrets = [secret]; + version += 2; + } + + return values.Length; + }); + state.Setup(x => x[0]).Returns(values[0]); + state.Setup(x => x.GetEnumerator()) + .Returns(() => ((IEnumerable>) values).GetEnumerator()); + + var obfuscatedState = new FormattedLogValuesObfuscator(secretObfuscator) + .TryObfuscateValues(state.Object); + var value = ((IReadOnlyList>) obfuscatedState)[0].Value; + + await Assert.That(value).IsEqualTo("**********"); } [Test] @@ -194,4 +250,26 @@ private sealed class ThrowingToStringState { public override string ToString() => throw new InvalidOperationException("Cannot format state."); } + + private sealed class CountingToStringState + { + public int ToStringCalls { get; private set; } + + public override string ToString() + { + ToStringCalls++; + return "state"; + } + } + + private static SecretObfuscator CreateBuiltInObfuscator(ISecretProvider? secretProvider = null) + { + secretProvider ??= Mock.Of(provider => + provider.Version == 0 && + provider.GetSnapshot() == new SecretSnapshot(0, Array.Empty())); + + return new SecretObfuscator( + secretProvider, + Microsoft.Extensions.Options.Options.Create(new SecretMaskingOptions())); + } }