Mask secrets in pipeline-level logging - #3581
Conversation
GitHub Actions
Failure
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review
This is a solid, well-targeted fix for #3476 — it closes all three holes the issue called out (dry-run input, PipelineLevelLogger bypassing the obfuscation pipeline entirely, and raw exceptions leaking into downstream providers), and the new tests (Masks_Secret_Values_From_Dry_Run_Command, PipelineLevelLoggerTests, FormattedLogValuesObfuscatorTests) exercise the new behavior directly rather than just re-asserting the old ones. No bugs or CLAUDE.md violations found. Two design points worth considering as follow-ups, neither blocking:
1. PipelineLevelLogger.Log lost the IsEnabled short-circuit that its sibling has
ModularPipelines/src/ModularPipelines/Logging/PipelineLevelLogger.cs
Lines 32 to 45 in 2cc8225
Log<TState> now unconditionally calls TryObfuscateValues and builds a BufferedLogEvent<TState> (which itself eagerly obfuscates exception.ToString() in ObfuscatedLogException's constructor) before ever checking whether the log level is enabled. Compare with ModuleLogger<T>.Log, which is the other implementation of the same IModuleLogger.Log contract and guards with if (!IsEnabled(logLevel)) return; before doing any obfuscation work — and with CommandLogger.LogDryRunCommand in this very PR, which adds exactly that !logger.IsEnabled(LogLevel.Information) guard for the same reason (avoid obfuscation cost when nothing will be written).
Since PipelineLevelLogger is what backs all logging outside a module context (pipeline init, hooks, condition evaluation — per ModuleLoggerProvider.GetLogger()), any Trace/Debug-level calls there now pay for a full state/exception obfuscation pass even when filtered out by LoggerFilterOptions. Adding the same IsEnabled guard at the top of Log<TState> would make the two IModuleLogger implementations consistent and avoid the wasted work — the same fix already applied to CommandLogger in this PR.
2. ObfuscatedLogException discards the original exception's type identity
Every exception that reaches a downstream ILogger (module or pipeline-level) is now replaced with a plain ObfuscatedLogException. Text-based sinks are fine — ToString() is overridden and still embeds the original type name (covered by the nameof(InvalidOperationException) assertion in ModuleOutputBufferTests) — but anything that inspects the exception object itself (is OperationCanceledException checks, Sentry/Application Insights grouping/sampling by exception type, custom retry or alerting logic keyed off exception type in a downstream logging provider) will now see every exception as the same ObfuscatedLogException type, silently changing that behavior for consumers of this library's logs.
This is a reasonable tradeoff for closing the secret leak (and matches what the issue itself suggested — "an obfuscated-ToString exception decorator"), so not asking for a change here, but it's worth a one-line doc comment on ObfuscatedLogException noting that type identity is intentionally not preserved, so a future maintainer doesn't assume GetType() still reflects the original exception when debugging a downstream integration.
|
Addressed review notes: disabled pipeline log levels now short-circuit before obfuscation, with regression coverage; exception type-identity replacement is documented. @codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Preserve formatted scope rendering after masking
When a structured scope such as BeginScope("Processing {Token}", secret) contains a value that is masked, this replaces its FormattedLogValues state with a raw KeyValuePair[]. Because BeginScope has no formatter, text-oriented providers render that array's type name instead of the original scope text (for example, Processing ********). Preserve a sanitized state object with the original formatted ToString() behavior while exposing the masked key/value pairs.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code review
Both follow-ups from the last review round are genuinely resolved now, not just cosmetically:
PipelineLevelLogger.Log(src/ModularPipelines/Logging/PipelineLevelLogger.cs:220-223) now short-circuits withIsEnabled(logLevel)before doing any obfuscation work, matchingModuleLogger<T>and the newCommandLogger.LogDryRunCommandguard — and it's covered byLog_DoesNotObfuscateWhenDisabled, which asserts the obfuscator is never invoked.ObfuscatedLogException(src/ModularPipelines/Logging/ObfuscatedLogException.cs:163-168) now carries a<remarks>doc noting type identity is intentionally dropped.
I re-read the rest of the diff (BufferedLogEvent's new typed-state branch, FormattedLogValuesObfuscator.ObfuscateValue, the dry-run input masking, ModuleLoggerProvider DI wiring) and didn't find anything that reintroduces a secret leak or breaks the existing behavior. No blocking issues. Two minor, non-blocking follow-ups worth a look:
1. ObfuscatedLogException loses stack trace, inner-exception chain, and Data — not just type identity
Since the wrapper is only ever new'd and never thrown, ObfuscatedLogException.StackTrace is always null, and InnerException/Data are never populated from the source exception. The full text (including the original stack trace) is preserved in the overridden ToString(), so nothing about the secret-leak fix is weakened — but any downstream sink that inspects the exception object directly rather than calling ToString() (Application Insights TrackException, Sentry, structured JSON exporters that walk InnerException/StackTrace for grouping and alerting) will lose that structure for every exception that passes through pipeline or module logging, which is a bigger observability hit than the type-identity change already called out. Worth either mentioning this explicitly alongside the existing type-identity remark, or considering copying Data/wrapping via InnerException = exception (the outer Message/ToString() behavior wouldn't need to change) so structured sinks at least retain the original exception in the chain.
2. FormattedLogValuesObfuscator.ObfuscateValue assumes a non-null state
TryObfuscateValues(object state) now falls into ObfuscateValue(state) for any non-structured state, which unconditionally calls value.ToString(). IModuleLogger.Log<TState> (like the BCL ILogger.Log<TState>) doesn't constrain TState : notnull, unlike BeginScope, which does. If a caller ever logs with a null state, this now throws an NRE from inside the obfuscation path itself — previously it was a harmless no-op (return state). Low likelihood in practice since MEL's own LogXxx extensions always build a non-null FormattedLogValues, but given this method's entire job is to safely pre-process arbitrary logger state before it reaches a sink, a value?.ToString() ?? string.Empty guard would make it robust against that edge case for free.
Nice test coverage on the new behavior (Masks_Secret_Values_From_Dry_Run_Command, TryObfuscateValues_MasksUnstructuredState, Log_ObfuscatesStateMessageAndExceptionBeforeDelegating, BeginScope_ObfuscatesStateBeforeDelegating) — these exercise the actual masking outcome rather than just re-asserting old behavior.
Sanitize dry-run commands, pipeline-level structured state, scopes, and exceptions before downstream providers can render them. Closes #3476
7229143 to
99a80fc
Compare
There was a problem hiding this comment.
Code review
Re-reviewed the current head (99a80fc19). Both fixes from the last two review rounds hold up:
PipelineLevelLogger.Logshort-circuits onIsEnabled(logLevel)before doing any obfuscation work (src/ModularPipelines/Logging/PipelineLevelLogger.cs:244-248), covered byLog_DoesNotObfuscateWhenDisabled.- The null-state NRE is fixed —
ModuleLogger.LogandPipelineLevelLogger.Logboth guardstate is nullbefore callingTryObfuscateValues(ModuleLogger.cs:129-131,PipelineLevelLogger.cs:249-251), andFormattedLogValuesObfuscatorTests/Log_PreservesNullStateexercise it. BufferedLogEvent<TState>.WriteTonow wraps the exception viaObfuscatedLogException.Createbefore it ever reaches a downstreamILogger.Logcall, closing the raw-exception leak the issue called out, andFormatstill routes the fully-rendered message throughsecretObfuscator.Obfuscateregardless of which exception/state variant is used — so the message text stays safe even thoughFormatintentionally formats off the original (unwrapped) exception via closure rather than thelogExceptionparameter.
Codex's P2 (structured scope rendering) still looks unaddressed
FormattedLogValuesObfuscator.TryObfuscateValues (src/ModularPipelines/Logging/FormattedLogValuesObfuscator.cs:35-58) still replaces a masked structured state with a raw KeyValuePair<string, object?>[] when any value changes:
obfuscatedValues ??= values.ToArray();
obfuscatedValues[index] = new KeyValuePair<string, object?>(property.Key, obfuscatedValue);
...
return obfuscatedValues ?? state;For logger.BeginScope("Processing {Token}", secret), the underlying state MEL builds is FormattedLogValues, which implements IReadOnlyList<KeyValuePair<string, object?>> and overrides ToString() to render the templated message (e.g. "Processing ********" if it had been given obfuscated args). Once a secret is found, PipelineLevelLogger.BeginScope (PipelineLevelLogger.cs:264-273) hands the downstream logger a bare array instead of that FormattedLogValues-shaped object. Any provider that renders scopes via scope.ToString() (console/file scope prefixes are the common case) loses the templated rendering and instead prints the array's default ToString() (its type name) — not a secret leak, but a real regression in scope legibility that's easy to miss since BeginScope_ObfuscatesStateBeforeDelegating only exercises the plain-interpolated-string path ($"Scope: {secret}"), not the templated BeginScope(format, args) overload that actually produces a FormattedLogValues. Worth either preserving a sanitized state object that still renders correctly via ToString() (as Codex suggested), or adding a regression test for the templated BeginScope overload if the current array-return behavior is intentional.
Non-blocking, previously raised
ObfuscatedLogException (src/ModularPipelines/Logging/ObfuscatedLogException.cs) still only preserves HResult and a rendered ToString() — StackTrace, InnerException, and Data from the original exception are dropped for every exception that passes through module/pipeline logging. Text sinks are unaffected (the full original text, including stack trace, survives in ToString()), but anything that inspects the exception object structurally (APM exception grouping, structured JSON exporters walking InnerException) loses that structure. Already flagged as an acceptable tradeoff in the prior round, so raising only as a reminder, not a blocker.
Nothing else stood out — the DI wiring in ModuleLoggerProvider, the dry-run input masking in CommandLogger, and the new/updated tests all look consistent with the stated goal of closing #3476.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99a80fc194
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19d825017e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Preserved sanitized exception diagnostics in 53d6bf7: recursive inner exceptions, original stack/target frame, Source, HelpLink, and HResult now reach downstream providers without raw secret-bearing exception references. Validation: PipelineLevelLoggerTests 10/10; ModuleOutputBufferTests 23/23; scoped whitespace clean. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53d6bf70f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66783d052b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef1c2784cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8838cc53f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@claude review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfa851e0c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08072259a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var obfuscatedState = state is null | ||
| ? null | ||
| : _formattedLogValuesObfuscator.TryObfuscateValues(state); |
There was a problem hiding this comment.
For an enabled pipeline-level Log<TState> whose state implements IReadOnlyList<KeyValuePair<string, object?>> but throws from Count, indexing, or enumeration, this preprocessing call propagates the exception before the underlying logger is reached. The previous implementation delegated such states unchanged, and the scope path now guards the same traversal failure, so a provider that would merely store or ignore the state can instead have logging abort pipeline initialization or condition evaluation; catch inspection failures here and fall back to a safe masked state.
Useful? React with 👍 / 👎.
1 participant
Summary
Test plan
ModularPipelines.slnRelease build (0 errors)Closes #3476