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".
Fall back to a fully masked event when structured state inspection throws, avoiding both unsafe traversal and the original formatter. Refs #3476
There was a problem hiding this comment.
Code review
Re-reviewed the current head (c702dd7). This PR has gone through a lot of hardening rounds now (IsEnabled short-circuits, null-state handling, structured-scope rendering, hostile-exception diagnostics), and each previously-flagged issue I can find genuinely holds up on re-check — no secret-leak regressions spotted in the diff since the last review.
One real gap, plus one architectural observation for the pattern that's produced most of this PR's churn:
1. The just-added "hostile structured state" guard only covers PipelineLevelLogger, not ModuleLogger<T> — the far more common path
ModularPipelines/src/ModularPipelines/Logging/ModuleLogger.cs
Lines 108 to 138 in c702dd7
The latest commit (c702dd7, "guard hostile log state") wraps _formattedLogValuesObfuscator.TryObfuscateValues(state) in a try/catch inside PipelineLevelLogger.Log, falling back to a fully-masked event when a structured state's Count/indexer throws — with Log_GuardsHostileStructuredTraversal covering it. But ModuleLogger<T>.Log, the other IModuleLogger implementation of the same contract, calls the exact same _formattedLogValuesObfuscator.TryObfuscateValues(state) unguarded (ModuleLogger.cs:122-124) before building its BufferedLogEvent. There's no equivalent test in ModuleLoggerTests.cs.
This isn't just an inconsistency — ModuleLoggerProvider.GetLogger() (ModuleLoggerProvider.cs:63-100) only falls back to PipelineLevelLogger when there's no module context; every log call made from inside a module (i.e. nearly all real-world logging in a pipeline) goes through the unguarded ModuleLogger<T> path. A module that logs a custom IReadOnlyList<KeyValuePair<string,object?>> state whose Count or indexer throws (or any other structured state FormattedLogValuesObfuscator.TryObfuscateValues can't safely traverse) will throw straight out of the module's LogInformation/LogWarning call, which is exactly the failure mode the last several commits were dedicated to eliminating for pipeline-level logging.
Why this keeps happening: this is the third or fourth time in this PR's history that a fix landed in one of ModuleLogger/PipelineLevelLogger/CommandLogger and had to be re-applied to a sibling later (IsEnabled short-circuit, null-state handling, and now this). All three duplicate the same "obfuscate state defensively before handing it to the underlying ILogger" logic independently. Rather than adding a fourth try/catch to ModuleLogger.Log, it'd be worth pulling this into one shared helper (e.g. have IFormattedLogValuesObfuscator.TryObfuscateValues itself catch and return a masked fallback, or add a single SafeObfuscateState(state) used by both Log implementations) so hardening added in one place can't be forgotten in the other. That would also shrink the surface area future secret-leak fixes need to touch.
2. (Non-blocking) ObfuscatedLogException's reliance on UnsafeAccessor into private BCL fields is worth watching
ModularPipelines/src/ModularPipelines/Logging/ObfuscatedLogException.cs
Lines 204 to 219 in c702dd7
TryCopyTargetSite/TryCopyNativeAotTargetSiteState reach into Exception's private _exceptionMethod, _corDbgStackTrace, and _idxFirstFreeStackTraceEntry fields via [UnsafeAccessor] to preserve TargetSite across the wrap. It's already defensively caught (MissingFieldException), so it degrades gracefully rather than crashing — but these are undocumented CoreCLR/NativeAOT implementation details, not a public contract, so they can silently stop copying (falling back to null TargetSite) on a future runtime or on Mono without any signal. Given how much of this file's complexity (244 lines, most of it added across the last ~8 commits) exists to preserve exception structure field-by-field, it may be worth reconsidering the overall approach — e.g. wrapping/decorating the original exception (keep it as InnerException or similar, only override the rendered text) instead of reconstructing its internal state — rather than continuing to chase individual fields as they're noticed missing. Not asking for a change in this PR; flagging it since the pattern of "one more field to copy" has been the shape of several commits here already.
Nothing else stood out — dry-run masking, the ObfuscatedScopeState fix for templated BeginScope, and the DI wiring all look correct and are covered by tests that exercise real outcomes rather than re-asserting prior behavior.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c702dd741c
ℹ️ 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".
Replay null buffered states with the original generic type so logger providers observe the same event contract.
There was a problem hiding this comment.
Code review
Re-reviewed the current head (13b31e0a). The newest commit (13b31e0aae, "preserve null state type") correctly fixes the last outstanding Codex finding — buffered null states now replay through logger.Log<TState> with the original generic type (ModuleOutputBuffer.cs:776-784), verified by ModuleOutputBufferTests. No new secret-leak regressions in this round.
One point from my previous review round does not appear to have been addressed, and it's the same architectural issue this PR keeps running into, so flagging it again as the one thing worth resolving before merge:
ModuleLogger<T>.Log still lacks the hostile-state guard that PipelineLevelLogger.Log has
src/ModularPipelines/Logging/PipelineLevelLogger.cs:42-61 wraps _formattedLogValuesObfuscator.TryObfuscateValues(state) in a try/catch (added in c702dd741c, "guard hostile log state") — if a custom structured state's Count or indexer throws, it falls back to a fully masked BufferedLogEvent instead of propagating.
src/ModularPipelines/Logging/ModuleLogger.cs:122-124 calls the exact same method, completely unguarded:
var obfuscatedState = state is null
? null
: _formattedLogValuesObfuscator.TryObfuscateValues(state);And FormattedLogValuesObfuscator.TryObfuscateValues (FormattedLogValuesObfuscator.cs:32-58) iterates values.Count / values[index] directly, with no try/catch around that traversal — only the inner ObfuscateValue's ToString() call is guarded. A module that logs a custom IReadOnlyList<KeyValuePair<string,object?>> state whose Count or indexer throws will now throw straight out of LogInformation/LogWarning, aborting the module — exactly the failure mode the last several commits were dedicated to eliminating, just not for this path.
This isn't a minor oversight: per ModuleLoggerProvider.GetLogger(), ModuleLogger<T> is the logger used for essentially all in-module logging, while PipelineLevelLogger only backs the narrow pre/post-module-context window (init, hooks, condition evaluation). The guard landed on the rarer path and not the common one, and there's no equivalent regression test in ModuleLoggerTests.cs (that file isn't touched by this PR at all) — the closest existing test, PipelineLevelLoggerTests.Log_GuardsHostileStructuredTraversal, only exercises PipelineLevelLogger.
Why this keeps recurring: this is now the fourth piece of hardening (IsEnabled short-circuit, null-state handling, structured-scope rendering, and now this) that landed on one of ModuleLogger/PipelineLevelLogger/CommandLogger and had to be independently re-discovered for a sibling. Rather than adding a fifth try/catch to ModuleLogger.Log, it would close off this whole class of gap to push the defensive behavior into IFormattedLogValuesObfuscator.TryObfuscateValues itself (catch internally, return a safe fallback) so both Log implementations get the hardening for free and future changes only need to touch one place. Given how much of this PR's churn has been chasing the same fix across duplicated call sites, that consolidation would pay for itself quickly.
Non-blocking, previously raised, still true
ObfuscatedLogException's TargetSite preservation relies on [UnsafeAccessor] into private Exception fields (_exceptionMethod, _corDbgStackTrace, _idxFirstFreeStackTraceEntry). It degrades gracefully (caught MissingFieldException) so it's not a blocker, but these are undocumented runtime internals that could silently stop working on a future CoreCLR/NativeAOT/Mono version. Worth keeping in mind if this file needs another round of "preserve one more field" fixes — a wrap-don't-reconstruct approach (keep the original exception reachable, e.g. as InnerException, and only override the rendered text) would be more resilient than continuing to mirror BCL internals field-by-field.
Everything else — dry-run input masking, the ObfuscatedScopeState fix for templated BeginScope, AggregateException handling, Data copying, and the DI wiring in ModuleLoggerProvider — looks correct and is exercised by tests that check real outcomes rather than re-asserting prior behavior.
1 participant
Summary
Test plan
ModularPipelines.slnRelease build (0 errors)Closes #3476