Add default sanitizer for resource group name in URI#2386
Add default sanitizer for resource group name in URI#2386alzimmermsft wants to merge 2 commits intomicrosoft:mainfrom
Conversation
alzimmermsft
requested review from
Copilot,
fanyang-mono,
g2vinay,
hallipr,
jongio,
saikoumudi,
sandeep-sen and
vukelich
There was a problem hiding this comment.
Pull request overview
This PR updates the core recorded-test base class to improve record/playback stability by adding a default URI sanitizer that replaces Azure Resource Group names in request URIs.
Changes:
- Add a default
UriRegexSanitizerto sanitize resource group names found in URIs during recording and playback.
| public virtual List<UriRegexSanitizer> UriRegexSanitizers { get; } = [ | ||
| // Sanitize Resource Group name from the URI. This is needed as there is another default GeneralRegexSanitizer for | ||
| // Settings.ResourceBaseName. But there is two issues we hit with that: | ||
| // 1. Resource group names often have other characters added to it, like prepending or appending for uniqueness. | ||
| // 2. In playback, Settings.ResourceBaseName is configured to be 'Sanitized', so it won't find the right string. | ||
| new UriRegexSanitizer(new() | ||
| { | ||
| Value = "Sanitized", | ||
| Regex = "/resource[gG]roups/(?<rgname>[\\w\\-.()]{1,90})/", | ||
| GroupForReplace = "rgname" | ||
| }) | ||
| ]; |
There was a problem hiding this comment.
The new default UriRegexSanitizer is always registered via the property initializer, even when EnableDefaultSanitizerAdditions is set to false. That makes the opt-out flag semantics inconsistent for derived tests that disable default sanitizers (often to control sanitizer ordering). Consider only adding this default sanitizer when EnableDefaultSanitizerAdditions is enabled (e.g., populate it in PopulateDefaultSanitizers()), or update the flag/docs to reflect that URI sanitizers are always applied.
There was a problem hiding this comment.
I think this is probably pretty safe, but I also do agree with this github comment. I don't agree enough to block checkin though.
I will definitely like to kick a server run against this to make certain no one got broken by the new addition.
core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Tests/Client/RecordedCommandTestsBase.cs
Outdated
Show resolved
Hide resolved
core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Tests/Client/RecordedCommandTestsBase.cs
Outdated
Show resolved
Hide resolved
scbedd
left a comment
scbedd
left a comment
There was a problem hiding this comment.
Going to run Azure.Server against this pr.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Regex looks correct - character class covers Azure's allowed RG name chars, 1-90 length constraint matches, and the lookahead handles /, ?, and end-of-string terminators.
The property-initializer approach is consistent with how HeaderRegexSanitizers already provides defaults outside EnableDefaultSanitizerAdditions. The 7 test classes that disable that flag can override UriRegexSanitizers if needed since it's virtual.
LGTM.
What does this PR do?
Adds a default
UriRegexSanitizertoRecordedCommandTestsBasethat handles sanitizing resource group name in URIs in a way that won't have issues in playback after recording.The previous sanitization was done using a
GeneralRegexSanitizerlooking for the resource base name. This might not be the same as the resource group name, which is the first issue, but generally the resource group name does contain the base name. The second issue is only the base name specifically was being sanitized, so only part of the resource group name may be sanitized, which would be fine if the third issue wasn't present. The third problem is that the base name is defaulted toSantizedin playback, which would fail to match the resource group name if it was capture.GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline