[BUG] Fix query obfuscation culture invariant#8850
Merged
Conversation
The query-string obfuscation regex was compiled with IgnoreCase but without CultureInvariant. Case-insensitive matching therefore depended on the ambient culture, which caused two problems: - CPU spikes when obfuscating non-ASCII query strings on .NET Framework, because each character comparison performs culture-aware case folding (~2x overall, up to ~4x under tr-TR). - Incorrect redaction under Turkic cultures: the dotted/dotless 'I' does not fold to 'i', so keywords containing 'i' (api, public, signature, ...) were never redacted. Add RegexOptions.CultureInvariant and a regression test asserting redaction is identical across cultures (tr-TR, az-Latn-AZ, en-US). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Exercises the culture-folding fix through a customer-style custom obfuscation pattern carrying Korean secret values. Under tr-TR the upper-case keys containing 'I' (TICKET, EMPID) would otherwise leak their values unredacted. The pattern is linear (not backtracking); this guards correctness, not throughput. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Reproduces the reported Vault file-download scenario (URL-encoded Korean filename + auth params). The default pattern is linear on this input - URL-encoding makes it pure ASCII and the repetition groups' alternatives are mutually exclusive - so there is no catastrophic backtracking. The guard uses a tight regex timeout so any future change that reintroduces super-linear backtracking would time out and fail. Also documents that the Vault "ticket" parameter passes through unredacted by the default pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
Author
|
@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
Here are some automated review suggestions for this pull request.
Reviewed commit: 6826e2e57d
ℹ️ 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".
BenchmarksBenchmark execution time: 2026-06-30 18:13:06 Comparing candidate commit 26f8319 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 58 known flaky benchmarks, 68 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8850) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8850) - mean (73ms) : 70, 76
master - mean (70ms) : 68, 72
section Bailout
This PR (8850) - mean (77ms) : 73, 80
master - mean (76ms) : 72, 80
section CallTarget+Inlining+NGEN
This PR (8850) - mean (1,086ms) : 1025, 1146
master - mean (1,080ms) : 1034, 1126
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8850) - mean (113ms) : 107, 119
master - mean (109ms) : 106, 112
section Bailout
This PR (8850) - mean (111ms) : 107, 115
master - mean (115ms) : 109, 121
section CallTarget+Inlining+NGEN
This PR (8850) - mean (780ms) : 755, 804
master - mean (777ms) : 759, 795
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8850) - mean (100ms) : 94, 106
master - mean (97ms) : 94, 99
section Bailout
This PR (8850) - mean (99ms) : 96, 103
master - mean (98ms) : 97, 99
section CallTarget+Inlining+NGEN
This PR (8850) - mean (940ms) : 892, 988
master - mean (938ms) : 896, 979
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8850) - mean (94ms) : 92, 97
master - mean (99ms) : 94, 103
section Bailout
This PR (8850) - mean (99ms) : 93, 105
master - mean (97ms) : 94, 100
section CallTarget+Inlining+NGEN
This PR (8850) - mean (814ms) : 778, 849
master - mean (812ms) : 779, 844
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8850) - mean (205ms) : 201, 209
master - mean (211ms) : 205, 218
section Bailout
This PR (8850) - mean (208ms) : 204, 212
master - mean (219ms) : 209, 228
section CallTarget+Inlining+NGEN
This PR (8850) - mean (1,217ms) : 1173, 1262
master - mean (1,246ms) : 1185, 1307
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8850) - mean (295ms) : 288, 302
master - mean (306ms) : 293, 320
section Bailout
This PR (8850) - mean (298ms) : 293, 303
master - mean (304ms) : 293, 314
section CallTarget+Inlining+NGEN
This PR (8850) - mean (980ms) : 962, 999
master - mean (994ms) : 961, 1027
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8850) - mean (290ms) : 284, 295
master - mean (290ms) : 280, 300
section Bailout
This PR (8850) - mean (289ms) : 284, 294
master - mean (291ms) : 284, 299
section CallTarget+Inlining+NGEN
This PR (8850) - mean (1,173ms) : 1132, 1213
master - mean (1,183ms) : 1150, 1216
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8850) - mean (285ms) : 277, 293
master - mean (289ms) : 282, 296
section Bailout
This PR (8850) - mean (286ms) : 281, 291
master - mean (290ms) : 284, 296
section CallTarget+Inlining+NGEN
This PR (8850) - mean (1,054ms) : 1006, 1103
master - mean (1,061ms) : 1002, 1120
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
The new default-pattern tests construct the default obfuscation regex, which is documented to crash the RegexRunner on netcoreapp2.1/Linux/ARM64. Switch them to Skippable + SkipOn, matching ObfuscateWithDefaultPattern, so that target is skipped rather than failing the suite. The custom-pattern test does not use the default regex and is left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
dromanol
marked this pull request as ready for review
June 30, 2026 08:27
andrewlock
added
type:performance
andrewlock
approved these changes
Jun 30, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
LGTM, though I would add some extra unicode tests, and remove the timeout-related one (which will flake in CI)
Comment on lines
+161
to
+196
| // Regression guard for a reported production CPU spike on Vault file-download URLs carrying | ||
| // URL-encoded CJK filenames alongside auth-related parameters. URL-encoding makes the input pure | ||
| // ASCII ("%EC%84%A4..."), and the default pattern's repetition groups are linear (their | ||
| // alternatives are mutually exclusive by the second character), so this must NOT exhibit | ||
| // catastrophic backtracking. A deliberately tight timeout means any future change that | ||
| // reintroduces super-linear backtracking would time out (Obfuscate returns string.Empty) and | ||
| // fail the trailing-secret assertion below. | ||
| [SkippableTheory] | ||
| [InlineData(50)] | ||
| [InlineData(500)] | ||
| [InlineData(2000)] | ||
| public void DefaultPatternDoesNotBacktrackOnUrlEncodedCjk(int filenameRepetitions) | ||
| { | ||
| // the default regex seems to crash the regex engine on netcoreapp2.1 under arm64, with a null reference exception on the dotnet RegexRunner. Its ok as these arent supported in auto instrumentation, we just warn not to reuse this regex if 2.1&arm64 is the environment | ||
| #if NETCOREAPP2_1 | ||
| SkipOn.PlatformAndArchitecture(SkipOn.PlatformValue.Linux, SkipOn.ArchitectureValue.ARM64); | ||
| #endif | ||
| const double tightTimeoutMs = 2000; | ||
| var encodedCjkFilename = string.Concat(Enumerable.Repeat("%EC%84%A4%EA%B3%84%EC%9E%90%EB%A3%8C", filenameRepetitions)); | ||
| // Mirrors the reported URL shape, with a genuinely redactable secret appended so a successful | ||
| // (non-timed-out) run is observable in the output. | ||
| var queryString = $"/Vault/vaultserver.aspx?fileName={encodedCjkFilename}_.xlsx&vaultId=67BBB9204FE84A8981ED8313049BA06C&password=hunter2"; | ||
|
|
||
| var logger = new Mock<IDatadogLogger>(); | ||
| var queryStringObfuscator = ObfuscatorFactory.GetObfuscator(tightTimeoutMs, TracerSettingsConstants.DefaultObfuscationQueryStringRegex, logger.Object); | ||
|
|
||
| var result = queryStringObfuscator.Obfuscate(queryString); | ||
|
|
||
| // Completed within the timeout (no RegexMatchTimeoutException, which would yield string.Empty) | ||
| // and still redacted the trailing secret while leaving the encoded CJK bytes untouched. | ||
| result.Should().Be($"/Vault/vaultserver.aspx?fileName={encodedCjkFilename}_.xlsx&vaultId=67BBB9204FE84A8981ED8313049BA06C&<redacted>"); | ||
| } | ||
|
|
||
| // The reported Vault "ticket" parameter is not actually matched by the default pattern (there is | ||
| // no "ticket" keyword and the value does not satisfy any keyword suffix), so the URL passes | ||
| // through unchanged - and, crucially, quickly. |
Member
There was a problem hiding this comment.
This test is absolutely asking for flake in CI - I would remove it
Suggested change
| // Regression guard for a reported production CPU spike on Vault file-download URLs carrying | |
| // URL-encoded CJK filenames alongside auth-related parameters. URL-encoding makes the input pure | |
| // ASCII ("%EC%84%A4..."), and the default pattern's repetition groups are linear (their | |
| // alternatives are mutually exclusive by the second character), so this must NOT exhibit | |
| // catastrophic backtracking. A deliberately tight timeout means any future change that | |
| // reintroduces super-linear backtracking would time out (Obfuscate returns string.Empty) and | |
| // fail the trailing-secret assertion below. | |
| [SkippableTheory] | |
| [InlineData(50)] | |
| [InlineData(500)] | |
| [InlineData(2000)] | |
| public void DefaultPatternDoesNotBacktrackOnUrlEncodedCjk(int filenameRepetitions) | |
| { | |
| // the default regex seems to crash the regex engine on netcoreapp2.1 under arm64, with a null reference exception on the dotnet RegexRunner. Its ok as these arent supported in auto instrumentation, we just warn not to reuse this regex if 2.1&arm64 is the environment | |
| #if NETCOREAPP2_1 | |
| SkipOn.PlatformAndArchitecture(SkipOn.PlatformValue.Linux, SkipOn.ArchitectureValue.ARM64); | |
| #endif | |
| const double tightTimeoutMs = 2000; | |
| var encodedCjkFilename = string.Concat(Enumerable.Repeat("%EC%84%A4%EA%B3%84%EC%9E%90%EB%A3%8C", filenameRepetitions)); | |
| // Mirrors the reported URL shape, with a genuinely redactable secret appended so a successful | |
| // (non-timed-out) run is observable in the output. | |
| var queryString = $"/Vault/vaultserver.aspx?fileName={encodedCjkFilename}_.xlsx&vaultId=67BBB9204FE84A8981ED8313049BA06C&password=hunter2"; | |
| var logger = new Mock<IDatadogLogger>(); | |
| var queryStringObfuscator = ObfuscatorFactory.GetObfuscator(tightTimeoutMs, TracerSettingsConstants.DefaultObfuscationQueryStringRegex, logger.Object); | |
| var result = queryStringObfuscator.Obfuscate(queryString); | |
| // Completed within the timeout (no RegexMatchTimeoutException, which would yield string.Empty) | |
| // and still redacted the trailing secret while leaving the encoded CJK bytes untouched. | |
| result.Should().Be($"/Vault/vaultserver.aspx?fileName={encodedCjkFilename}_.xlsx&vaultId=67BBB9204FE84A8981ED8313049BA06C&<redacted>"); | |
| } | |
| // The reported Vault "ticket" parameter is not actually matched by the default pattern (there is | |
| // no "ticket" keyword and the value does not satisfy any keyword suffix), so the URL passes | |
| // through unchanged - and, crucially, quickly. |
The timeout-based ReDoS guard relied on a tight regex timeout and could flake under CI load. Replace it with RedactsSecretsWhilePreservingUnicodeValues, which deterministically checks that a secret is redacted while surrounding multi-byte values (CJK, RTL Arabic, surrogate-pair emoji, accents, Cyrillic) are left byte-for-byte intact. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Co-authored-by: Andrew Lock <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
Add
RegexOptions.CultureInvariantto the query-string obfuscation regex (Obfuscator.cs), plus regression tests covering culture-independent redaction and a ReDoS guard for URL-encoded CJK query strings.Reason for change
The obfuscation regex was compiled with
IgnoreCasebut withoutCultureInvariant, so case-insensitive matching depended on the ambient culture. This caused two problems:%-dense query strings (.NET Framework). Without culture invariance, each character comparison uses a culture-aware case-folding path. Investigating a reported production incident (Vault file-download URLs with URL-encoded Korean filenames + auth params), the obfuscation regex consumed ~16 s/min of CPU and pushedsystem.cpu.userfrom ~1.6% to ~90%.DD_HTTP_SERVER_TAG_QUERY_STRING_SIZE=5000, a CJK-encoded query string costs ~1.5–2.3 ms/call on .NET Framework;CultureInvariantcuts that ~2x. (URL-encoding makes the input pure ASCII, but the faster case-folding path still applies.)Idoes not fold toi, so undertr-TR/az-Latn-AZkeywords containing ani(api,public,signature, …) were never redacted, leaking secrets. Reproduces on all runtimes (correctness, not just perf).Implementation details
One-line change: add
RegexOptions.CultureInvariantto the options inObfuscator. Redaction output is byte-identical for ASCII input; matching is now locale-independent, which is the correct behavior for a security obfuscator.Explored but rejected: explicit atomic groups
(?>…)made it ~2x slower (the cost is the unanchored start-position scan, not internal backtracking). Boundary-anchoring keyword matches is ~2.3x faster but changes redaction semantics (e.g.access_token=<jwt>), so it is out of scope for this fix — see follow-up note below.Test coverage
ObfuscatesIndependentlyOfCurrentCulture(tr-TR / az-Latn-AZ / en-US) — fails before the fix under Turkic cultures, passes after.ObfuscatesNonAsciiValuesWithCustomPatternIndependentlyOfCulture— customer-style custom pattern over Korean secret values.DefaultPatternDoesNotBacktrackOnUrlEncodedCjk(50/500/2000 reps) — ReDoS guard with a tight regex timeout; large CJK payload must complete and still redact a trailing secret.DefaultPatternPassesThroughUnmatchedVaultUrl— documents that the Vaultticketparam is not matched by the default pattern.All existing obfuscator + IAST
EvidenceRedactortests continue to pass.Other details
The default pattern is a cross-tracer canonical regex; this change keeps its matches identical and only removes culture sensitivity. A separate, behavior-changing proposal (boundary-anchoring to further cut CPU ~2.3x) should be discussed cross-team rather than bundled here. Lowering
DD_HTTP_SERVER_TAG_QUERY_STRING_SIZEremains an immediate linear mitigation, stackable with this fix.