Refactor plugin telemetry allowlists and validate tool name before emitting#2291
Merged
saikoumudi merged 1 commit intomicrosoft:release/azure/2.xfrom Mar 30, 2026
Conversation
…itting telemetry (microsoft#2282) * Refactor PluginTelemetryCommand to use constructor injection - Inject IPluginFileReferenceAllowlistProvider and IPluginSkillNameAllowlistProvider via constructor - Remove service resolution from ExecuteAsync method - Move validation before host creation for better performance - Improves testability by allowing easy dependency mocking * Add tool name validation to plugin telemetry using CommandFactory - Validate tool names directly using ICommandFactory.FindCommandByName() in PluginTelemetryCommand - Remove IPluginToolNameAllowlistProvider abstraction (too thin, CommandFactory is already mockable) - Tool name validation automatically stays in sync with registered commands - Update CHANGELOG with cleaner validation approach * Refactor allowlist providers to use simpler interface design - Change IPluginFileReferenceAllowlistProvider from GetAllowedPaths() to IsPathAllowed(string) - Change IPluginSkillNameAllowlistProvider from GetAllowedSkillNames() to IsSkillNameAllowed(string) - Providers now encapsulate the check logic instead of exposing collections - Simplifies caller code - single method call instead of get + contains - Better encapsulation - implementation details (HashSet) hidden from callers - Tests now directly test behavior (true/false) like testing a HashSet - All three validation mechanisms now follow consistent patterns - Add comprehensive unit tests for all three validation providers * remove circular dependency, added toolname validation by stripping the prefix * updated unit tests and changelog * Make plugin telemetry no-op for fabric and template * update the test and make checks case sensitive
kvenkatrajan
approved these changes
Mar 30, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors plugin telemetry validation to use constructor-injected allowlist providers and adds tool-name validation/normalization against the registered command set before emitting telemetry, aiming to prevent logging unrecognized/custom tool identifiers.
Changes:
- Refactored
PluginTelemetryCommandto use constructor injection for allowlist providers and added tool-name validation viaICommandFactory(including client prefix stripping and extension-tool allowlist). - Simplified allowlist provider interfaces from “get collection + contains” to
Is*Allowed(...), and introduced “null” (deny-all) provider implementations. - Added unit tests covering allowlist behavior and tool-name normalization; updated Azure server changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| servers/Template.Mcp.Server/src/Program.cs | Registers deny-all allowlist providers for plugin telemetry scenarios in Template server. |
| servers/Fabric.Mcp.Server/src/Program.cs | Registers deny-all allowlist providers for plugin telemetry scenarios in Fabric server. |
| servers/Azure.Mcp.Server/CHANGELOG.md | Documents plugin telemetry validation updates. |
| core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ToolLoading/PluginTelemetryCommand.cs | Adds tool-name validation/normalization and switches allowlist access to injected providers. |
| core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ToolLoading/IPluginSkillNameAllowlistProvider.cs | Changes interface to IsSkillNameAllowed and adds implementations. |
| core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ToolLoading/IPluginFileReferenceAllowlistProvider.cs | Changes interface to IsPathAllowed and adds implementations. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/Commands/ToolLoading/PluginTelemetryCommandTests.cs | Adds unit tests for allowlists and tool-name validation/normalization. |
Comments suppressed due to low confidence (3)
core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ToolLoading/IPluginSkillNameAllowlistProvider.cs:37
- This file now contains both the interface and concrete implementations (Null* and Resource*). Since the filename is IPluginSkillNameAllowlistProvider.cs, this makes navigation harder and encourages further type pile-up. Consider moving each implementation to its own file (e.g., NullPluginSkillNameAllowlistProvider.cs and ResourcePluginSkillNameAllowlistProvider.cs).
/// <summary>
/// No-op implementation that rejects all skill names.
/// Used by servers that don't support plugin telemetry (e.g., Fabric).
/// </summary>
public class NullPluginSkillNameAllowlistProvider : IPluginSkillNameAllowlistProvider
{
public bool IsSkillNameAllowed(string skillName) => false;
}
/// <summary>
/// Provides skill name validation using an embedded JSON resource allowlist.
/// The resource should contain a JSON array of skill names.
/// </summary>
public sealed class ResourcePluginSkillNameAllowlistProvider : IPluginSkillNameAllowlistProvider
core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ToolLoading/IPluginFileReferenceAllowlistProvider.cs:37
- This file now contains both the interface and concrete implementations (Null* and Resource*). Since the filename is IPluginFileReferenceAllowlistProvider.cs, this makes navigation harder and encourages further type pile-up. Consider moving each implementation to its own file (e.g., NullPluginFileReferenceAllowlistProvider.cs and ResourcePluginFileReferenceAllowlistProvider.cs).
/// <summary>
/// No-op implementation that rejects all file references.
/// Used by servers that don't support plugin telemetry (e.g., Fabric).
/// </summary>
public class NullPluginFileReferenceAllowlistProvider : IPluginFileReferenceAllowlistProvider
{
public bool IsPathAllowed(string pluginRelativePath) => false;
}
/// <summary>
/// Provides file reference validation using an embedded JSON resource allowlist.
/// The resource should contain a JSON array of plugin-relative file references.
/// </summary>
public sealed class ResourcePluginFileReferenceAllowlistProvider : IPluginFileReferenceAllowlistProvider
servers/Azure.Mcp.Server/CHANGELOG.md:117
- Typo in changelog: "enahnce" should be "enhance".
- Improved tool descriptions to enahnce LLM selection accuracy for the following tools: [[#2131](https://github.com/microsoft/mcp/pull/2131)]
Contributor
Author
|
dont want to address any of copilot feedback on this since this is the change in main branch. |
21 tasks
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
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.
…itting telemetry (#2282)
remove circular dependency, added toolname validation by stripping the prefix
updated unit tests and changelog
Make plugin telemetry no-op for fabric and template
update the test and make checks case sensitive
What does this PR do?
[Provide a clear, concise description of the changes][Add additional context, screenshots, or information that helps reviewers]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