Obfuscate secrets in a single scan - #3847
Conversation
There was a problem hiding this comment.
Code review
Summary: Replaces the per-secret StringBuilder.Replace / manual IndexOf loops with a single forward scan over the input using a cached SearchValues<string>, finding the earliest match position and then resolving it against the (length-descending) secret list to preserve longest-match precedence.
Verification performed:
- Checked out the PR branch and ran
test/ModularPipelines.UnitTestsfiltered toSecretObfuscatorCachingTests— all 13 tests pass, including the two new tests (UsesLongestSecretWhenPatternsMatchAtSamePosition,DoesNotRescanMaskReplacement). - Traced the matching algorithm by hand against several overlapping/substring scenarios (shorter secret is a substring of a longer one at the same start index, secrets with different lengths starting at different indices, secrets equal in length).
IndexOfAny(searchValues)locates the leftmost position where any secret starts, and the subsequentStartsWithscan (secrets pre-sorted longest-first viaOrderByDescending(secret => secret.Length)inCreateSecretCache) always resolves to the longest secret matching at that position — this correctly reproduces the "longest match wins" behavior the old two-branch implementation aimed for, while also fixing the case-insensitive path's leftmost-match tie-breaking. - Confirmed the new single StringBuilder never re-scans previously appended mask text (it only walks the original
inputspan and advancesinputOffsetpast the matched secret), which is what the newDoesNotRescanMaskReplacementtest targets and is a real behavioral improvement over the old case-sensitive path (StringBuilder.Replaceper secret could, in principle, re-scan across replacement boundaries between separate secrets). - No CLAUDE.md violations found — the change is confined to
src/ModularPipelines/Engine/SecretObfuscator.csand its existing unit test file, doesn't touch generated code, and the updated XML doc comment on the class accurately reflects the new implementation.
Design note (non-blocking): ObfuscateMatches still does a linear StartsWith scan over secrets at every match position to determine which secret matched. For the typical secret-set sizes here (tokens/credentials registered per pipeline run) this is fine and is a clear net win over the old N-passes-over-the-whole-input approach. If secret counts ever grow large (hundreds+) and profiling shows this loop becoming hot, a follow-up could group secrets by first character (or build a small trie) to cut the per-match resolution cost — but that's speculative and not warranted by this PR's scope.
No issues found. Good, well-tested performance improvement.
|
Ubuntu pipeline failed only in the known retry timing race |
Summary
Validation
Closes #3753