Skip to content

Add default sanitizer for resource group name in URI #2386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alzimmermsft wants to merge 2 commits into
base: main
Choose a base branch
from
+13 −1
Open

Add default sanitizer for resource group name in URI #2386

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd2e5ce
Add default sanitizer for resource group name in URI
alzimmermsft Apr 10, 2026
741d9ff
Take on some feedback
alzimmermsft Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Tests/Client/RecordedCommandTestsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,19 @@ public abstract class RecordedCommandTestsBase(ITestOutputHelper output, TestPro
/// <summary>
/// Sanitizers that apply a regex replacement to URIs. This sanitization is applied to to recorded data at rest and during recording, and against test requests during playback.
/// </summary>
public virtual List<UriRegexSanitizer> UriRegexSanitizers { get; } = [];
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 are 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 and Settings.ResourceGroupName are both 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"
})
];
Comment on lines +58 to +70
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@scbedd scbedd Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// <summary>
/// Sanitizers that will apply a regex replacement to a specific json body key. This sanitization is applied to to recorded data at rest and during recording, and against test requests during playback.
Expand Down