From e11dc4a66efbc27cc8274f066155f6a326ce319a Mon Sep 17 00:00:00 2001 From: SergeyMenshykh Date: Wed, 23 Sep 2026 14:23:33 +0100 Subject: [PATCH 1/5] Refine MCP skill resource handling in .NET and Python Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Skills/AgentMcpSkill.cs | 61 ++++++++- .../Skills/AgentMcpSkillsSource.cs | 6 + .../Skills/Loaders/SkillMdEntryLoader.cs | 2 +- .../Skills/AgentMcpSkillsSourceTests.cs | 122 ++++++++++++++++-- .../packages/core/agent_framework/_skills.py | 48 ++++++- .../core/tests/core/test_mcp_skills.py | 99 +++++++++++++- 6 files changed, 320 insertions(+), 18 deletions(-) diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs index 53ecb4d09ff..434a43cf399 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs @@ -1,9 +1,11 @@ // Copyright (c) Microsoft. All rights reserved. using System; +using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; using Microsoft.Shared.Diagnostics; using ModelContextProtocol.Client; using ModelContextProtocol.Protocol; @@ -25,11 +27,12 @@ namespace Microsoft.Agents.AI; /// with pre-fetched content. /// /// -internal sealed class AgentMcpSkill : AgentSkill +internal sealed partial class AgentMcpSkill : AgentSkill { private const string SkillMdSuffix = "SKILL.md"; private readonly McpClient _client; + private readonly ILogger _logger; private readonly string _skillMdUri; private readonly string _skillRootUri; private string? _content; @@ -44,12 +47,14 @@ internal sealed class AgentMcpSkill : AgentSkill /// (used to resolve sibling resources) is derived by stripping the trailing SKILL.md segment. /// /// The MCP client used to fetch resources on demand. - public AgentMcpSkill(AgentSkillFrontmatter frontmatter, string skillMdUri, McpClient client) + /// The logger used to report rejected resource names. + public AgentMcpSkill(AgentSkillFrontmatter frontmatter, string skillMdUri, McpClient client, ILogger logger) { this.Frontmatter = Throw.IfNull(frontmatter); this._skillMdUri = Throw.IfNullOrWhitespace(skillMdUri); this._skillRootUri = ComputeSkillRootUri(skillMdUri); this._client = Throw.IfNull(client); + this._logger = Throw.IfNull(logger); } /// @@ -85,7 +90,9 @@ public override async ValueTask GetContentAsync(CancellationToken cancel /// /// Resolves as a relative path against the skill's root URI, issues a /// resources/read request to the MCP server, and returns an - /// with the pre-fetched content. Returns when the name is empty, the server + /// with the pre-fetched content. Absolute paths, parent traversal (including percent-encoded forms), + /// embedded URI schemes, and control characters are rejected before sending a request. + /// Returns when the name is empty or unsafe, the server /// returns no content, or the resource does not exist on the server. /// public override async ValueTask GetResourceAsync(string name, CancellationToken cancellationToken = default) @@ -95,6 +102,12 @@ public override async ValueTask GetContentAsync(CancellationToken cancel return null; } + if (!IsResourceNameSafe(name)) + { + LogUnsafeResourceName(this._logger); + return null; + } + string uri = this._skillRootUri + name; ReadResourceResult result; @@ -112,6 +125,48 @@ public override async ValueTask GetContentAsync(CancellationToken cancel return new AgentMcpSkillResource(name: name, result: result); } + private static bool IsResourceNameSafe(string name) + { + string normalized = name.Replace('\\', '/'); + return !GetDecodedForms(normalized).Any(IsUnsafePath); + } + + /// + /// Yields and each successive percent-decoded form until decoding is stable. + /// Every layer is validated so nested encodings (e.g. %252e) cannot smuggle traversal past + /// the check, while the original escaping is still preserved in the resource URI. + /// + private static IEnumerable GetDecodedForms(string name) + { + string current = name; + while (true) + { + yield return current; + + string decoded = Uri.UnescapeDataString(current).Replace('\\', '/'); + if (decoded == current) + { + yield break; + } + + current = decoded; + } + } + + private static bool IsUnsafePath(string form) + { + // URI parsers can trim trailing spaces before resolving dot segments. + form = form.TrimEnd(' '); + string path = form.Split(['?', '#'], 2)[0]; + return form.StartsWith('/') + || form.Contains("://", StringComparison.Ordinal) + || path.Split('/').Contains("..") + || form.Any(char.IsControl); + } + + [LoggerMessage(LogLevel.Debug, "Rejecting MCP skill resource name with unsafe path components.")] + private static partial void LogUnsafeResourceName(ILogger logger); + /// /// Strips the trailing SKILL.md from the URI to produce the skill's root directory URI. /// If the URI doesn't end with SKILL.md, ensures it ends with a trailing slash. diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs index ebbf4ec9793..139efd117e3 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs @@ -71,6 +71,12 @@ namespace Microsoft.Agents.AI; /// /// /// +/// Supporting resource names are checked for absolute paths, parent traversal (including +/// percent-encoded forms), and control characters before an MCP request is sent. Index URLs +/// retain their MCP resource schemes: they are sent to the connected server, not opened locally. +/// This client-side guard does not replace server-side authorization. +/// +/// /// Security considerations: This source discovers and loads skills — including full /// skill instructions and, for archive-type entries, files extracted to local disk — from a /// remote MCP server that the caller connects to explicitly (via UseMcpSkills); it is never diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/SkillMdEntryLoader.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/SkillMdEntryLoader.cs index fa8ff02bd9d..7cb7c7a696e 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/SkillMdEntryLoader.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/SkillMdEntryLoader.cs @@ -65,7 +65,7 @@ private bool TryLoadSkillMdEntry(McpSkillIndexEntry entry, [NotNullWhen(true)] o return false; } - skill = new AgentMcpSkill(frontmatter, entry.Url!, this._client); + skill = new AgentMcpSkill(frontmatter, entry.Url!, this._client, this._logger); LogSkillLoaded(this._logger, frontmatter.Name); diff --git a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs index 439e24bfdaa..fe4d8b71362 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs @@ -1,5 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. +using System.Collections.Generic; +using System.Text.Json; using System.Threading.Tasks; using Microsoft.Extensions.AI; using Microsoft.Extensions.DependencyInjection; @@ -141,21 +143,100 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() [InlineData("../escape.md")] [InlineData("references/../../escape.md")] [InlineData("..")] + [InlineData("..\\escape.md")] + [InlineData("/etc/passwd")] + [InlineData("http://attacker.example.com/payload")] + [InlineData("%2e%2e/escape.md")] + [InlineData("%2E./escape.md")] + [InlineData(".%2e/escape.md")] + [InlineData("references/%2e%2e/escape.md")] + [InlineData("references%2f..%2f..%2fescape.md")] + [InlineData("%2e%2e%5cescape.md")] + [InlineData("%252e%252e%252fescape.md")] + [InlineData("%25252e%25252e/escape.md")] + [InlineData("%2fescape.md")] + [InlineData("%5cescape.md")] + [InlineData("%68ttp%3a%2f%2fattacker.example.com/payload")] + [InlineData("..?download=1")] + [InlineData("..#fragment")] + [InlineData("%2e%2e%3fdownload=1")] + [InlineData("references%3f/../../escape.md")] + [InlineData(".\t./escape.md")] + [InlineData(".%09./escape.md")] + [InlineData("references/\0/guide.md")] + [InlineData(".. ")] + [InlineData(".%2e ")] + [InlineData("%2e%2e ")] + [InlineData("..%20")] + [InlineData("%252e%252e%2520")] + [InlineData("references/.. ")] public async Task GetResourceAsync_PathTraversalName_ReturnsNullAsync(string name) { - // Arrange - '..' segments result in URIs that don't match any server resource. - // The MCP server returns an error for unknown URIs, so GetResourceAsync returns null. - await using var server = new InMemoryMcpServer(builder => - builder.WithResources()); + foreach (string root in new[] + { + "skill://unit-converter/", + "skill://unit-converter/private/", + "https://example.com/skills/private/", + "file:///skills/private/", + "custom:skills/private/" + }) + { + // Arrange - accept every URI so rejection must happen before the MCP request. + List reads = []; + await using var server = CreatePermissiveSkillServer(root + "SKILL.md", reads); + await using var client = await server.CreateClientAsync(); + var source = new AgentMcpSkillsSource(client); + var skill = Assert.Single(await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create())); + reads.Clear(); + + // Act + var resource = await skill.GetResourceAsync(name); + + // Assert + Assert.Null(resource); + Assert.Empty(reads); + } + } + + [Theory] + [InlineData("skill://unit-converter/")] + [InlineData("skill://unit-converter/private/")] + [InlineData("https://example.com/skills/private/")] + [InlineData("file:///skills/private/")] + [InlineData("custom:skills/private/")] + public async Task GetResourceAsync_SafeNamesAndSchemes_ArePreservedAsync(string root) + { + // Arrange + List reads = []; + await using var server = CreatePermissiveSkillServer(root + "SKILL.md", reads); await using var client = await server.CreateClientAsync(); var source = new AgentMcpSkillsSource(client); + var skill = Assert.Single(await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create())); // Act - var skill = Assert.Single(await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create())); - var resource = await skill.GetResourceAsync(name); + Assert.Equal(SampleSkillMd, await skill.GetContentAsync()); - // Assert - resource does not exist on the server, so null is returned - Assert.Null(resource); + // Assert + Assert.Equal(root + "SKILL.md", reads[1]); + foreach (string name in new[] + { + "references/guide.md", + "references\\guide.md", + "references/guide%20one.md", + "references/v1.2/guide.md", + "references/%2520.md", + "references/guide.md?version=1#section", + "references/guide.md ", + "references/.. ?version=1" + }) + { + reads.Clear(); + var resource = await skill.GetResourceAsync(name); + Assert.NotNull(resource); + Assert.Equal(name, resource.Name); + Assert.Equal("safe content", await resource.ReadAsync()); + Assert.Equal(root + name, Assert.Single(reads)); + } } [Fact] @@ -242,6 +323,31 @@ public async Task GetSkillsAsync_IndexEntryWithTemplateType_IsSkippedAsync() Assert.Empty(skills); } + private static InMemoryMcpServer CreatePermissiveSkillServer(string skillMdUri, List reads) + { + string index = JsonSerializer.Serialize(new + { + skills = new[] + { + new { name = "unit-converter", type = "skill-md", description = "Convert between common units.", url = skillMdUri } + } + }); + return new InMemoryMcpServer(builder => builder.WithReadResourceHandler((request, cancellationToken) => + { + string uri = request.Params!.Uri; + reads.Add(uri); + return ValueTask.FromResult(new ReadResourceResult + { + Contents = [new TextResourceContents + { + Uri = uri, + Text = uri == "skill://index.json" ? index : uri == skillMdUri ? SampleSkillMd : "safe content", + MimeType = "text/plain" + }] + }); + })); + } + #region Resource classes (registered with the MCP server via WithResources) // CA1812 flags these classes as "never instantiated", which is technically correct - diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index eb29d6ab5b4..9f354c8370f 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -56,13 +56,14 @@ import time import zipfile from abc import ABC, abstractmethod -from collections.abc import Callable, Mapping, Sequence +from collections.abc import Callable, Iterator, Mapping, Sequence from dataclasses import dataclass from datetime import timedelta from enum import Enum from html import escape as xml_escape from pathlib import Path, PurePosixPath from typing import IO, TYPE_CHECKING, Any, ClassVar, Final, Protocol, TypeAlias, TypeVar, cast, runtime_checkable +from urllib.parse import unquote import yaml from yaml.nodes import MappingNode, Node, ScalarNode @@ -4407,6 +4408,38 @@ async def read(self, **kwargs: Any) -> Any: return text if text else None +_RESOURCE_CONTROL_CHARS: Final = re.compile(r"[\x00-\x1f\x7f-\x9f]") + + +def _decoded_resource_forms(name: str) -> Iterator[str]: + """Yield *name* and each successive percent-decoded form until decoding is stable. + + Every layer is validated so that nested encodings (e.g. ``%252e``) cannot + smuggle traversal past the check, while the original escaping is still + preserved in the resource identifier sent to the MCP server. + """ + current = name + while True: + yield current + decoded = unquote(current).replace("\\", "/") + if decoded == current: + return + current = decoded + + +def _is_unsafe_resource_path(form: str) -> bool: + """Return ``True`` if *form* is absolute, has a URI scheme, traverses upward, or has control characters.""" + # URI parsers can trim trailing spaces before resolving dot segments. + form = form.rstrip(" ") + path = re.split(r"[?#]", form, maxsplit=1)[0] + return ( + form.startswith("/") + or "://" in form + or ".." in path.split("/") + or _RESOURCE_CONTROL_CHARS.search(form) is not None + ) + + @experimental(feature_id=ExperimentalFeature.MCP_SKILLS) class MCPSkill(Skill): """A :class:`Skill` discovered from an MCP server exposing the Agent Skills convention. @@ -4502,7 +4535,7 @@ async def get_resource(self, name: str) -> SkillResource | None: Returns: An :class:`MCPSkillResource`, or ``None`` when the name is empty - or the resource does not exist on the server. + or unsafe, or the resource does not exist on the server. """ if not name or not name.strip(): return None @@ -4527,7 +4560,8 @@ def _validate_resource_name(name: str) -> str | None: """Validate a resource name and return the normalized form. Defense in depth: refuses names that could escape the skill root - (absolute paths, embedded URI schemes, parent-traversal segments). + (absolute paths, embedded URI schemes, parent-traversal segments, + or control characters), including percent-encoded forms. The MCP server is the authority on URI resolution, but rejecting obviously unsafe shapes client-side avoids leaking escape attempts upstream. @@ -4540,7 +4574,7 @@ def _validate_resource_name(name: str) -> str | None: or ``None`` if the name is unsafe. """ normalized = name.replace("\\", "/") - if normalized.startswith("/") or "://" in normalized or any(seg == ".." for seg in normalized.split("/")): + if any(_is_unsafe_resource_path(form) for form in _decoded_resource_forms(normalized)): logger.debug("Rejecting resource name with unsafe path components: %r", name) return None return normalized @@ -5050,6 +5084,12 @@ class MCPSkillsSource(SkillsSource): entries or their supporting resources. Security considerations: + Supporting resource names are checked for absolute paths, parent + traversal (including percent-encoded forms), and control characters + before an MCP request is sent. Index URLs retain their MCP resource + schemes: they are sent to the connected server, not opened locally. + This client-side guard does not replace server-side authorization. + Discovering skills over MCP means an *external* MCP server controls what skill content (including instructions and, for script-capable skills, the scripts the agent may run) reaches the agent. This source diff --git a/python/packages/core/tests/core/test_mcp_skills.py b/python/packages/core/tests/core/test_mcp_skills.py index 85638a036d0..46d68e36d57 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -295,9 +295,43 @@ async def test_get_resource_unknown_returns_none(self) -> None: "..\\escape.md", "/etc/passwd", "http://attacker.example.com/payload", + "%2e%2e/escape.md", + "%2E./escape.md", + ".%2e/escape.md", + "references/%2e%2e/escape.md", + "references%2f..%2f..%2fescape.md", + "%2e%2e%5cescape.md", + "%252e%252e%252fescape.md", + "%25252e%25252e/escape.md", + "%2fescape.md", + "%5cescape.md", + "%68ttp%3a%2f%2fattacker.example.com/payload", + "..?download=1", + "..#fragment", + "%2e%2e%3fdownload=1", + "references%3f/../../escape.md", + ".\t./escape.md", + ".%09./escape.md", + "references/\x00/guide.md", + ".. ", + ".%2e ", + "%2e%2e ", + "..%20", + "%252e%252e%2520", + "references/.. ", ], ) - async def test_get_resource_path_traversal_returns_none(self, name: str) -> None: + @pytest.mark.parametrize( + "skill_md_uri", + [ + "skill://unit-converter/SKILL.md", + "skill://unit-converter/private/SKILL.md", + "https://example.com/skills/private/SKILL.md", + "file:///skills/private/SKILL.md", + "custom:skills/private/SKILL.md", + ], + ) + async def test_get_resource_path_traversal_returns_none(self, name: str, skill_md_uri: str) -> None: # Register a permissive mock that would happily return content for any URI, # so the test fails unless the client-side validation rejects the name # before issuing the read. @@ -307,12 +341,50 @@ async def test_get_resource_path_traversal_returns_none(self, name: str) -> None from agent_framework import SkillFrontmatter fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") - skill = MCPSkill(frontmatter=fm, skill_md_uri="skill://unit-converter/SKILL.md", client=client) + skill = MCPSkill(frontmatter=fm, skill_md_uri=skill_md_uri, client=client) resource = await skill.get_resource(name) assert resource is None client.read_resource.assert_not_called() + @pytest.mark.parametrize( + "name", + [ + "references/guide.md", + "references\\guide.md", + "references/guide%20one.md", + "references/v1.2/guide.md", + "references/%2520.md", + "references/guide.md?version=1#section", + "references/guide.md ", + "references/.. ?version=1", + ], + ) + @pytest.mark.parametrize( + "root", + [ + "skill://unit-converter/", + "skill://unit-converter/private/", + "https://example.com/skills/private/", + "file:///skills/private/", + "custom:skills/private/", + ], + ) + async def test_get_resource_preserves_safe_names_and_schemes(self, name: str, root: str) -> None: + from agent_framework import SkillFrontmatter + + client = AsyncMock() + client.read_resource.return_value = _make_text_result("safe content") + fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") + skill = MCPSkill(frontmatter=fm, skill_md_uri=root + "SKILL.md", client=client) + + resource = await skill.get_resource(name) + + assert resource is not None + assert resource.name == name + assert await resource.read() == "safe content" + client.read_resource.assert_awaited_once_with(AnyUrl(root + name.replace("\\", "/"))) + async def test_get_resource_empty_name_returns_none(self) -> None: client = _make_client() from agent_framework import SkillFrontmatter @@ -390,6 +462,29 @@ def test_requires_exactly_one_of_client_or_session_provider(self) -> None: class TestMCPSkillsSource: """Tests for MCPSkillsSource.""" + @pytest.mark.parametrize( + "uri", + [ + "https://example.com/skills/SKILL.md", + "file:///skills/SKILL.md", + "custom:skills/SKILL.md", + ], + ) + async def test_index_preserves_mcp_resource_schemes(self, uri: str) -> None: + index = json.loads(SAMPLE_SKILL_INDEX) + index["skills"][0]["url"] = uri + client = _make_client(**{ + "skill://index.json": _make_text_result(json.dumps(index)), + uri: _make_text_result(SAMPLE_SKILL_MD), + }) + source = MCPSkillsSource(client=client) + + skills = await source.get_skills(_SOURCE_CTX) + + assert len(skills) == 1 + assert await skills[0].get_content() == SAMPLE_SKILL_MD + assert str(client.read_resource.call_args.args[0]) == uri + async def test_index_based_discovery_returns_skill(self) -> None: client = _make_client(**{ "skill://index.json": _make_text_result(SAMPLE_SKILL_INDEX, uri="skill://index.json"), From d7a64eb1db00b11405a5f0743ead2100eb3b1243 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh Date: Wed, 23 Sep 2026 15:08:15 +0100 Subject: [PATCH 2/5] Simplify MCP skill resource name validation Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Skills/AgentMcpSkill.cs | 48 ++++++++---------- .../Skills/AgentMcpSkillsSourceTests.cs | 24 +++++++-- .../packages/core/agent_framework/_skills.py | 50 +++++++++---------- .../core/tests/core/test_mcp_skills.py | 20 +++++++- 4 files changed, 82 insertions(+), 60 deletions(-) diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs index 434a43cf399..566d54e634f 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. using System; -using System.Collections.Generic; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -127,41 +126,36 @@ public override async ValueTask GetContentAsync(CancellationToken cancel private static bool IsResourceNameSafe(string name) { + // Treat backslashes as separators, e.g. "..\x" is checked as "../x". string normalized = name.Replace('\\', '/'); - return !GetDecodedForms(normalized).Any(IsUnsafePath); + + // Validate only the path before a literal "?"/"#", fully decoded; e.g. "a%3f/%2e%2e/x" stays one path, "a/b.md?q=/../x" ignores the query. + string path = FullyUnescape(normalized.Split(['?', '#'], 2)[0]); + + // Absolute path, e.g. "/etc/passwd" or "%2fetc/passwd". + return !path.StartsWith('/') + // Embedded URI, e.g. "http://example.com/other" or "%68ttp%3a%2f%2fexample.com". + && !path.Contains("://", StringComparison.Ordinal) + // Parent traversal, e.g. "../x", "%2e%2e/x", "%252e%252e/x", "a%3f/../../x", or ".. " (URI parsers can trim trailing spaces). + && !path.Split(['/', '?', '#']).Any(segment => segment.TrimEnd(' ') == "..") + // Control characters anywhere, e.g. "a/\0/b.md", ".\t./x", or ".%09./x". + && !FullyUnescape(normalized).Any(char.IsControl); } /// - /// Yields and each successive percent-decoded form until decoding is stable. - /// Every layer is validated so nested encodings (e.g. %252e) cannot smuggle traversal past - /// the check, while the original escaping is still preserved in the resource URI. + /// Percent-decodes until stable, normalizing backslashes to forward slashes. + /// Decoding never removes a literal ., /, :, or control character, so checking + /// the final form also covers every nested encoding layer (e.g. %252e). /// - private static IEnumerable GetDecodedForms(string name) + private static string FullyUnescape(string value) { - string current = name; - while (true) + string decoded; + while ((decoded = Uri.UnescapeDataString(value).Replace('\\', '/')) != value) { - yield return current; - - string decoded = Uri.UnescapeDataString(current).Replace('\\', '/'); - if (decoded == current) - { - yield break; - } - - current = decoded; + value = decoded; } - } - private static bool IsUnsafePath(string form) - { - // URI parsers can trim trailing spaces before resolving dot segments. - form = form.TrimEnd(' '); - string path = form.Split(['?', '#'], 2)[0]; - return form.StartsWith('/') - || form.Contains("://", StringComparison.Ordinal) - || path.Split('/').Contains("..") - || form.Any(char.IsControl); + return value; } [LoggerMessage(LogLevel.Debug, "Rejecting MCP skill resource name with unsafe path components.")] diff --git a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs index fe4d8b71362..0dcc2a092f1 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs @@ -145,7 +145,7 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() [InlineData("..")] [InlineData("..\\escape.md")] [InlineData("/etc/passwd")] - [InlineData("http://attacker.example.com/payload")] + [InlineData("http://example.com/other")] [InlineData("%2e%2e/escape.md")] [InlineData("%2E./escape.md")] [InlineData(".%2e/escape.md")] @@ -156,11 +156,21 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() [InlineData("%25252e%25252e/escape.md")] [InlineData("%2fescape.md")] [InlineData("%5cescape.md")] - [InlineData("%68ttp%3a%2f%2fattacker.example.com/payload")] + [InlineData("%68ttp%3a%2f%2fexample.com/other")] [InlineData("..?download=1")] [InlineData("..#fragment")] [InlineData("%2e%2e%3fdownload=1")] [InlineData("references%3f/../../escape.md")] + [InlineData("references%3f/%2e%2e/%2e%2e/escape.md")] + [InlineData("references%23/%2e%2e/%2e%2e/escape.md")] + [InlineData("references%3f%2f%2e%2e%2f%2e%2e%2fescape.md")] + [InlineData("references%23%5c%2e%2e%5c%2e%2e%5cescape.md")] + [InlineData("references%253f%252f%252e%252e%252f%252e%252e%252fescape.md")] + [InlineData("references%2523%252f%252e%252e%252f%252e%252e%252fescape.md")] + [InlineData("references%3f/%252e%252e/%252e%252e/escape.md")] + [InlineData("references%3f/%2e%2e/%2e%2e/escape.md?version=1")] + [InlineData("references%23/%2e%2e/%2e%2e/escape.md#section")] + [InlineData("references%3f%2f%2e%2e%20")] [InlineData(".\t./escape.md")] [InlineData(".%09./escape.md")] [InlineData("references/\0/guide.md")] @@ -170,6 +180,7 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() [InlineData("..%20")] [InlineData("%252e%252e%2520")] [InlineData("references/.. ")] + [InlineData("references/.. ?version=1")] public async Task GetResourceAsync_PathTraversalName_ReturnsNullAsync(string name) { foreach (string root in new[] @@ -226,8 +237,13 @@ public async Task GetResourceAsync_SafeNamesAndSchemes_ArePreservedAsync(string "references/v1.2/guide.md", "references/%2520.md", "references/guide.md?version=1#section", - "references/guide.md ", - "references/.. ?version=1" + "references/guide%3fname.md", + "references/guide%23name.md", + "references/guide%253fname.md", + "references/guide.md?example=/../../other.md", + "references/guide.md#example=/../../other.md", + "references/guide.md?example=%2e%2e%2f%2e%2e%2fother.md", + "references/guide.md " }) { reads.Clear(); diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 9f354c8370f..4b3f327e9df 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -56,7 +56,7 @@ import time import zipfile from abc import ABC, abstractmethod -from collections.abc import Callable, Iterator, Mapping, Sequence +from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass from datetime import timedelta from enum import Enum @@ -4411,33 +4411,15 @@ async def read(self, **kwargs: Any) -> Any: _RESOURCE_CONTROL_CHARS: Final = re.compile(r"[\x00-\x1f\x7f-\x9f]") -def _decoded_resource_forms(name: str) -> Iterator[str]: - """Yield *name* and each successive percent-decoded form until decoding is stable. +def _fully_unquote(value: str) -> str: + """Percent-decode *value* until stable, normalizing backslashes to forward slashes. - Every layer is validated so that nested encodings (e.g. ``%252e``) cannot - smuggle traversal past the check, while the original escaping is still - preserved in the resource identifier sent to the MCP server. + Decoding never removes a literal ``.``, ``/``, ``:``, or control character, so + checking the final form also covers every nested encoding layer (e.g. ``%252e``). """ - current = name - while True: - yield current - decoded = unquote(current).replace("\\", "/") - if decoded == current: - return - current = decoded - - -def _is_unsafe_resource_path(form: str) -> bool: - """Return ``True`` if *form* is absolute, has a URI scheme, traverses upward, or has control characters.""" - # URI parsers can trim trailing spaces before resolving dot segments. - form = form.rstrip(" ") - path = re.split(r"[?#]", form, maxsplit=1)[0] - return ( - form.startswith("/") - or "://" in form - or ".." in path.split("/") - or _RESOURCE_CONTROL_CHARS.search(form) is not None - ) + while (decoded := unquote(value).replace("\\", "/")) != value: + value = decoded + return value @experimental(feature_id=ExperimentalFeature.MCP_SKILLS) @@ -4573,8 +4555,22 @@ def _validate_resource_name(name: str) -> str | None: The normalized name with backslashes replaced by forward slashes, or ``None`` if the name is unsafe. """ + # Treat backslashes as separators, e.g. "..\x" is checked as "../x". normalized = name.replace("\\", "/") - if any(_is_unsafe_resource_path(form) for form in _decoded_resource_forms(normalized)): + # Validate only the path before a literal "?"/"#", fully decoded; e.g. "a%3f/%2e%2e/x" stays one path, + # "a/b.md?q=/../x" ignores the query. + path = _fully_unquote(re.split(r"[?#]", normalized, maxsplit=1)[0]) + if ( + # Absolute path, e.g. "/etc/passwd" or "%2fetc/passwd". + path.startswith("/") + # Embedded URI, e.g. "http://example.com/other" or "%68ttp%3a%2f%2fexample.com". + or "://" in path + # Parent traversal, e.g. "../x", "%2e%2e/x", "%252e%252e/x", "a%3f/../../x", or ".. " + # (URI parsers can trim trailing spaces). + or any(segment.rstrip(" ") == ".." for segment in re.split(r"[/?#]", path)) + # Control characters anywhere, e.g. "a/\0/b.md", ".\t./x", or ".%09./x". + or _RESOURCE_CONTROL_CHARS.search(_fully_unquote(normalized)) + ): logger.debug("Rejecting resource name with unsafe path components: %r", name) return None return normalized diff --git a/python/packages/core/tests/core/test_mcp_skills.py b/python/packages/core/tests/core/test_mcp_skills.py index 46d68e36d57..8aa90f51395 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -305,11 +305,21 @@ async def test_get_resource_unknown_returns_none(self) -> None: "%25252e%25252e/escape.md", "%2fescape.md", "%5cescape.md", - "%68ttp%3a%2f%2fattacker.example.com/payload", + "%68ttp%3a%2f%2fexample.com/other", "..?download=1", "..#fragment", "%2e%2e%3fdownload=1", "references%3f/../../escape.md", + "references%3f/%2e%2e/%2e%2e/escape.md", + "references%23/%2e%2e/%2e%2e/escape.md", + "references%3f%2f%2e%2e%2f%2e%2e%2fescape.md", + "references%23%5c%2e%2e%5c%2e%2e%5cescape.md", + "references%253f%252f%252e%252e%252f%252e%252e%252fescape.md", + "references%2523%252f%252e%252e%252f%252e%252e%252fescape.md", + "references%3f/%252e%252e/%252e%252e/escape.md", + "references%3f/%2e%2e/%2e%2e/escape.md?version=1", + "references%23/%2e%2e/%2e%2e/escape.md#section", + "references%3f%2f%2e%2e%20", ".\t./escape.md", ".%09./escape.md", "references/\x00/guide.md", @@ -319,6 +329,7 @@ async def test_get_resource_unknown_returns_none(self) -> None: "..%20", "%252e%252e%2520", "references/.. ", + "references/.. ?version=1", ], ) @pytest.mark.parametrize( @@ -356,8 +367,13 @@ async def test_get_resource_path_traversal_returns_none(self, name: str, skill_m "references/v1.2/guide.md", "references/%2520.md", "references/guide.md?version=1#section", + "references/guide%3fname.md", + "references/guide%23name.md", + "references/guide%253fname.md", + "references/guide.md?example=/../../other.md", + "references/guide.md#example=/../../other.md", + "references/guide.md?example=%2e%2e%2f%2e%2e%2fother.md", "references/guide.md ", - "references/.. ?version=1", ], ) @pytest.mark.parametrize( From e6501df49d0f790d80f2a1cee083ad153120df59 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh Date: Wed, 23 Sep 2026 15:19:20 +0100 Subject: [PATCH 3/5] Use normalized MCP skill resource names in .NET request URIs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs | 11 +++++------ .../Skills/AgentMcpSkillsSourceTests.cs | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs index 566d54e634f..3c88d1a033b 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs @@ -101,13 +101,15 @@ public override async ValueTask GetContentAsync(CancellationToken cancel return null; } - if (!IsResourceNameSafe(name)) + // Treat backslashes as separators, e.g. "..\x" is checked and requested as "../x". + string normalized = name.Replace('\\', '/'); + if (!IsResourceNameSafe(normalized)) { LogUnsafeResourceName(this._logger); return null; } - string uri = this._skillRootUri + name; + string uri = this._skillRootUri + normalized; ReadResourceResult result; try @@ -124,11 +126,8 @@ public override async ValueTask GetContentAsync(CancellationToken cancel return new AgentMcpSkillResource(name: name, result: result); } - private static bool IsResourceNameSafe(string name) + private static bool IsResourceNameSafe(string normalized) { - // Treat backslashes as separators, e.g. "..\x" is checked as "../x". - string normalized = name.Replace('\\', '/'); - // Validate only the path before a literal "?"/"#", fully decoded; e.g. "a%3f/%2e%2e/x" stays one path, "a/b.md?q=/../x" ignores the query. string path = FullyUnescape(normalized.Split(['?', '#'], 2)[0]); diff --git a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs index 0dcc2a092f1..dfb59b72432 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs @@ -251,7 +251,7 @@ public async Task GetResourceAsync_SafeNamesAndSchemes_ArePreservedAsync(string Assert.NotNull(resource); Assert.Equal(name, resource.Name); Assert.Equal("safe content", await resource.ReadAsync()); - Assert.Equal(root + name, Assert.Single(reads)); + Assert.Equal(root + name.Replace('\\', '/'), Assert.Single(reads)); } } From 8e3bad1cff875765a72e8316ed3c1b72ad834585 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh Date: Wed, 23 Sep 2026 15:32:12 +0100 Subject: [PATCH 4/5] Refine MCP skill resource decoding limits Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Skills/AgentMcpSkill.cs | 28 +++++++--- .../Skills/AgentMcpSkillsSourceTests.cs | 51 +++++++++++++++++++ .../packages/core/agent_framework/_skills.py | 23 +++++++-- .../core/tests/core/test_mcp_skills.py | 33 ++++++++++++ 4 files changed, 122 insertions(+), 13 deletions(-) diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs index 3c88d1a033b..d93d6afeeba 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs @@ -29,6 +29,7 @@ namespace Microsoft.Agents.AI; internal sealed partial class AgentMcpSkill : AgentSkill { private const string SkillMdSuffix = "SKILL.md"; + private const int MaxResourceNameDecodingDepth = 32; private readonly McpClient _client; private readonly ILogger _logger; @@ -91,6 +92,7 @@ public override async ValueTask GetContentAsync(CancellationToken cancel /// resources/read request to the MCP server, and returns an /// with the pre-fetched content. Absolute paths, parent traversal (including percent-encoded forms), /// embedded URI schemes, and control characters are rejected before sending a request. + /// Resource names requiring more than 32 percent-decoding passes are also rejected. /// Returns when the name is empty or unsafe, the server /// returns no content, or the resource does not exist on the server. /// @@ -129,16 +131,19 @@ public override async ValueTask GetContentAsync(CancellationToken cancel private static bool IsResourceNameSafe(string normalized) { // Validate only the path before a literal "?"/"#", fully decoded; e.g. "a%3f/%2e%2e/x" stays one path, "a/b.md?q=/../x" ignores the query. - string path = FullyUnescape(normalized.Split(['?', '#'], 2)[0]); + string? path = FullyUnescape(normalized.Split(['?', '#'], 2)[0]); + string? decodedName = FullyUnescape(normalized); - // Absolute path, e.g. "/etc/passwd" or "%2fetc/passwd". - return !path.StartsWith('/') + // Excessive encoding depth, e.g. a name requiring more than 32 decoding passes. + return path is not null && decodedName is not null + // Absolute path, e.g. "/etc/passwd" or "%2fetc/passwd". + && !path.StartsWith('/') // Embedded URI, e.g. "http://example.com/other" or "%68ttp%3a%2f%2fexample.com". && !path.Contains("://", StringComparison.Ordinal) // Parent traversal, e.g. "../x", "%2e%2e/x", "%252e%252e/x", "a%3f/../../x", or ".. " (URI parsers can trim trailing spaces). && !path.Split(['/', '?', '#']).Any(segment => segment.TrimEnd(' ') == "..") // Control characters anywhere, e.g. "a/\0/b.md", ".\t./x", or ".%09./x". - && !FullyUnescape(normalized).Any(char.IsControl); + && !decodedName.Any(char.IsControl); } /// @@ -146,15 +151,22 @@ private static bool IsResourceNameSafe(string normalized) /// Decoding never removes a literal ., /, :, or control character, so checking /// the final form also covers every nested encoding layer (e.g. %252e). /// - private static string FullyUnescape(string value) + /// The decoded value, or if the decoding depth exceeds the limit. + private static string? FullyUnescape(string value) { - string decoded; - while ((decoded = Uri.UnescapeDataString(value).Replace('\\', '/')) != value) + // The final pass only checks that decoding has stabilized. + for (int depth = 0; depth <= MaxResourceNameDecodingDepth; depth++) { + string decoded = Uri.UnescapeDataString(value).Replace('\\', '/'); + if (decoded == value) + { + return value; + } + value = decoded; } - return value; + return null; } [LoggerMessage(LogLevel.Debug, "Rejecting MCP skill resource name with unsafe path components.")] diff --git a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs index dfb59b72432..2be873647fc 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. using System.Collections.Generic; +using System.Linq; using System.Text.Json; using System.Threading.Tasks; using Microsoft.Extensions.AI; @@ -181,6 +182,9 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() [InlineData("%252e%252e%2520")] [InlineData("references/.. ")] [InlineData("references/.. ?version=1")] + [InlineData("references/guide.md?value=%00")] + [InlineData("references/guide.md#value=%2509")] + [InlineData("references/guide.md?value=%C2%85")] public async Task GetResourceAsync_PathTraversalName_ReturnsNullAsync(string name) { foreach (string root in new[] @@ -236,6 +240,7 @@ public async Task GetResourceAsync_SafeNamesAndSchemes_ArePreservedAsync(string "references/guide%20one.md", "references/v1.2/guide.md", "references/%2520.md", + "references/100%.md", "references/guide.md?version=1#section", "references/guide%3fname.md", "references/guide%23name.md", @@ -243,6 +248,7 @@ public async Task GetResourceAsync_SafeNamesAndSchemes_ArePreservedAsync(string "references/guide.md?example=/../../other.md", "references/guide.md#example=/../../other.md", "references/guide.md?example=%2e%2e%2f%2e%2e%2fother.md", + "references/guide.md?src=https://example.com/other", "references/guide.md " }) { @@ -255,6 +261,51 @@ public async Task GetResourceAsync_SafeNamesAndSchemes_ArePreservedAsync(string } } + [Theory] + [InlineData(1)] + [InlineData(31)] + [InlineData(32)] + [InlineData(33)] + [InlineData(4096)] + public async Task GetResourceAsync_DecodingDepthIsBoundedAsync(int depth) + { + // Arrange + const string Root = "skill://unit-converter/private/"; + List reads = []; + await using var server = CreatePermissiveSkillServer(Root + "SKILL.md", reads); + await using var client = await server.CreateClientAsync(); + var source = new AgentMcpSkillsSource(client); + var skill = Assert.Single(await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create())); + string encoded = "%" + string.Concat(Enumerable.Repeat("25", depth - 1)) + "41"; + + foreach (string name in new[] + { + $"references/{encoded}.md", + $"references/guide.md?value={encoded}", + $"references/guide.md#value={encoded}", + $"../{encoded}.md" + }) + { + reads.Clear(); + + // Act + var resource = await skill.GetResourceAsync(name); + + // Assert + if (depth <= 32 && !name.StartsWith("../", System.StringComparison.Ordinal)) + { + Assert.NotNull(resource); + Assert.Equal(name, resource.Name); + Assert.Equal(Root + name, Assert.Single(reads)); + } + else + { + Assert.Null(resource); + Assert.Empty(reads); + } + } + } + [Fact] public async Task GetSkillsAsync_DoesNotReadSkillMdAsync() { diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 4b3f327e9df..38b097c13d1 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -4409,17 +4409,25 @@ async def read(self, **kwargs: Any) -> Any: _RESOURCE_CONTROL_CHARS: Final = re.compile(r"[\x00-\x1f\x7f-\x9f]") +_MAX_RESOURCE_NAME_DECODING_DEPTH: Final[int] = 32 -def _fully_unquote(value: str) -> str: +def _fully_unquote(value: str) -> str | None: """Percent-decode *value* until stable, normalizing backslashes to forward slashes. Decoding never removes a literal ``.``, ``/``, ``:``, or control character, so checking the final form also covers every nested encoding layer (e.g. ``%252e``). + + Returns: + The decoded value, or ``None`` if the decoding depth exceeds the limit. """ - while (decoded := unquote(value).replace("\\", "/")) != value: + # The final pass only checks that decoding has stabilized. + for _ in range(_MAX_RESOURCE_NAME_DECODING_DEPTH + 1): + decoded = unquote(value).replace("\\", "/") + if decoded == value: + return value value = decoded - return value + return None @experimental(feature_id=ExperimentalFeature.MCP_SKILLS) @@ -4544,6 +4552,7 @@ def _validate_resource_name(name: str) -> str | None: Defense in depth: refuses names that could escape the skill root (absolute paths, embedded URI schemes, parent-traversal segments, or control characters), including percent-encoded forms. + Names requiring more than 32 percent-decoding passes are also refused. The MCP server is the authority on URI resolution, but rejecting obviously unsafe shapes client-side avoids leaking escape attempts upstream. @@ -4560,16 +4569,20 @@ def _validate_resource_name(name: str) -> str | None: # Validate only the path before a literal "?"/"#", fully decoded; e.g. "a%3f/%2e%2e/x" stays one path, # "a/b.md?q=/../x" ignores the query. path = _fully_unquote(re.split(r"[?#]", normalized, maxsplit=1)[0]) + decoded_name = _fully_unquote(normalized) if ( + # Excessive encoding depth, e.g. a name requiring more than 32 decoding passes. + path is None + or decoded_name is None # Absolute path, e.g. "/etc/passwd" or "%2fetc/passwd". - path.startswith("/") + or path.startswith("/") # Embedded URI, e.g. "http://example.com/other" or "%68ttp%3a%2f%2fexample.com". or "://" in path # Parent traversal, e.g. "../x", "%2e%2e/x", "%252e%252e/x", "a%3f/../../x", or ".. " # (URI parsers can trim trailing spaces). or any(segment.rstrip(" ") == ".." for segment in re.split(r"[/?#]", path)) # Control characters anywhere, e.g. "a/\0/b.md", ".\t./x", or ".%09./x". - or _RESOURCE_CONTROL_CHARS.search(_fully_unquote(normalized)) + or _RESOURCE_CONTROL_CHARS.search(decoded_name) ): logger.debug("Rejecting resource name with unsafe path components: %r", name) return None diff --git a/python/packages/core/tests/core/test_mcp_skills.py b/python/packages/core/tests/core/test_mcp_skills.py index 8aa90f51395..34c44155339 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -11,6 +11,7 @@ import zipfile from datetime import timedelta from unittest.mock import AsyncMock, patch +from urllib.parse import unquote import pytest from mcp.shared.exceptions import McpError @@ -330,6 +331,9 @@ async def test_get_resource_unknown_returns_none(self) -> None: "%252e%252e%2520", "references/.. ", "references/.. ?version=1", + "references/guide.md?value=%00", + "references/guide.md#value=%2509", + "references/guide.md?value=%C2%85", ], ) @pytest.mark.parametrize( @@ -366,6 +370,7 @@ async def test_get_resource_path_traversal_returns_none(self, name: str, skill_m "references/guide%20one.md", "references/v1.2/guide.md", "references/%2520.md", + "references/100%.md", "references/guide.md?version=1#section", "references/guide%3fname.md", "references/guide%23name.md", @@ -373,6 +378,7 @@ async def test_get_resource_path_traversal_returns_none(self, name: str, skill_m "references/guide.md?example=/../../other.md", "references/guide.md#example=/../../other.md", "references/guide.md?example=%2e%2e%2f%2e%2e%2fother.md", + "references/guide.md?src=https://example.com/other", "references/guide.md ", ], ) @@ -401,6 +407,33 @@ async def test_get_resource_preserves_safe_names_and_schemes(self, name: str, ro assert await resource.read() == "safe content" client.read_resource.assert_awaited_once_with(AnyUrl(root + name.replace("\\", "/"))) + @pytest.mark.parametrize("depth", [1, 31, 32, 33, 4096]) + @pytest.mark.parametrize( + "template", + ["references/{}.md", "references/guide.md?value={}", "references/guide.md#value={}", "../{}.md"], + ) + async def test_get_resource_decoding_depth_is_bounded(self, depth: int, template: str) -> None: + from agent_framework import SkillFrontmatter + + root = "skill://unit-converter/private/" + client = AsyncMock() + client.read_resource.return_value = _make_text_result("safe content") + fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") + skill = MCPSkill(frontmatter=fm, skill_md_uri=root + "SKILL.md", client=client) + name = template.format("%" + "25" * (depth - 1) + "41") + + with patch("agent_framework._skills.unquote", wraps=unquote) as decode: + resource = await skill.get_resource(name) + + assert decode.call_count <= 66 + if depth <= 32 and not name.startswith("../"): + assert resource is not None + assert resource.name == name + client.read_resource.assert_awaited_once_with(AnyUrl(root + name)) + else: + assert resource is None + client.read_resource.assert_not_called() + async def test_get_resource_empty_name_returns_none(self) -> None: client = _make_client() from agent_framework import SkillFrontmatter From bd78e939ffbcf93498878a393b75704684edb47e Mon Sep 17 00:00:00 2001 From: SergeyMenshykh Date: Thu, 24 Sep 2026 10:47:22 +0100 Subject: [PATCH 5/5] Decode MCP skill resource name parts once and cache Python decoding Split names at the first literal query/fragment delimiter, then decode the path and suffix independently. Make the Python decoder recursive with a bounded LRU cache that preserves the decoding depth limit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Skills/AgentMcpSkill.cs | 12 +++-- .../Skills/AgentMcpSkillsSourceTests.cs | 7 +++ .../packages/core/agent_framework/_skills.py | 34 +++++++----- .../core/tests/core/test_mcp_skills.py | 52 +++++++++++++++++-- 4 files changed, 83 insertions(+), 22 deletions(-) diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs index d93d6afeeba..d5453e7d9be 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs @@ -130,12 +130,14 @@ public override async ValueTask GetContentAsync(CancellationToken cancel private static bool IsResourceNameSafe(string normalized) { - // Validate only the path before a literal "?"/"#", fully decoded; e.g. "a%3f/%2e%2e/x" stays one path, "a/b.md?q=/../x" ignores the query. - string? path = FullyUnescape(normalized.Split(['?', '#'], 2)[0]); - string? decodedName = FullyUnescape(normalized); + // Split at the first literal "?"/"#" before decoding, so "a%3f/%2e%2e/x" stays one path while + // "a/b.md?q=/../x" leaves "/../x" in the query; then decode each part once. + string[] parts = normalized.Split(['?', '#'], 2); + string? path = FullyUnescape(parts[0]); + string? suffix = FullyUnescape(parts.Length > 1 ? parts[1] : string.Empty); // Excessive encoding depth, e.g. a name requiring more than 32 decoding passes. - return path is not null && decodedName is not null + return path is not null && suffix is not null // Absolute path, e.g. "/etc/passwd" or "%2fetc/passwd". && !path.StartsWith('/') // Embedded URI, e.g. "http://example.com/other" or "%68ttp%3a%2f%2fexample.com". @@ -143,7 +145,7 @@ private static bool IsResourceNameSafe(string normalized) // Parent traversal, e.g. "../x", "%2e%2e/x", "%252e%252e/x", "a%3f/../../x", or ".. " (URI parsers can trim trailing spaces). && !path.Split(['/', '?', '#']).Any(segment => segment.TrimEnd(' ') == "..") // Control characters anywhere, e.g. "a/\0/b.md", ".\t./x", or ".%09./x". - && !decodedName.Any(char.IsControl); + && !(path + suffix).Any(char.IsControl); } /// diff --git a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs index 2be873647fc..e123eef8a40 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceTests.cs @@ -185,6 +185,9 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() [InlineData("references/guide.md?value=%00")] [InlineData("references/guide.md#value=%2509")] [InlineData("references/guide.md?value=%C2%85")] + [InlineData("references/%2500guide.md?version=1")] + [InlineData("references/guide.md?version=1#value=%2509")] + [InlineData("references/guide.md#section?value=%2509")] public async Task GetResourceAsync_PathTraversalName_ReturnsNullAsync(string name) { foreach (string root in new[] @@ -249,6 +252,10 @@ public async Task GetResourceAsync_SafeNamesAndSchemes_ArePreservedAsync(string "references/guide.md#example=/../../other.md", "references/guide.md?example=%2e%2e%2f%2e%2e%2fother.md", "references/guide.md?src=https://example.com/other", + "references/guide%2520one.md?value=%2520#section%2520", + "references/guide%253fname.md#section?value=%2520", + "references/guide.md?", + "references/guide.md#", "references/guide.md " }) { diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 38b097c13d1..2efce44a0be 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -60,6 +60,7 @@ from dataclasses import dataclass from datetime import timedelta from enum import Enum +from functools import lru_cache from html import escape as xml_escape from pathlib import Path, PurePosixPath from typing import IO, TYPE_CHECKING, Any, ClassVar, Final, Protocol, TypeAlias, TypeVar, cast, runtime_checkable @@ -4412,22 +4413,27 @@ async def read(self, **kwargs: Any) -> Any: _MAX_RESOURCE_NAME_DECODING_DEPTH: Final[int] = 32 -def _fully_unquote(value: str) -> str | None: +@lru_cache(maxsize=128) +def _fully_unquote(value: str, *, remaining_depth: int = _MAX_RESOURCE_NAME_DECODING_DEPTH) -> str | None: """Percent-decode *value* until stable, normalizing backslashes to forward slashes. Decoding never removes a literal ``.``, ``/``, ``:``, or control character, so checking the final form also covers every nested encoding layer (e.g. ``%252e``). + Each layer is cached, keyed by the remaining depth so reuse cannot bypass the limit. + + Args: + value: The value to decode. + + Keyword Args: + remaining_depth: The number of further decoding layers allowed. Returns: The decoded value, or ``None`` if the decoding depth exceeds the limit. """ - # The final pass only checks that decoding has stabilized. - for _ in range(_MAX_RESOURCE_NAME_DECODING_DEPTH + 1): - decoded = unquote(value).replace("\\", "/") - if decoded == value: - return value - value = decoded - return None + decoded = unquote(value).replace("\\", "/") + if decoded == value: + return value + return _fully_unquote(decoded, remaining_depth=remaining_depth - 1) if remaining_depth > 0 else None @experimental(feature_id=ExperimentalFeature.MCP_SKILLS) @@ -4566,14 +4572,14 @@ def _validate_resource_name(name: str) -> str | None: """ # Treat backslashes as separators, e.g. "..\x" is checked as "../x". normalized = name.replace("\\", "/") - # Validate only the path before a literal "?"/"#", fully decoded; e.g. "a%3f/%2e%2e/x" stays one path, - # "a/b.md?q=/../x" ignores the query. - path = _fully_unquote(re.split(r"[?#]", normalized, maxsplit=1)[0]) - decoded_name = _fully_unquote(normalized) + # Split at the first literal "?"/"#" before decoding, so "a%3f/%2e%2e/x" stays one path while + # "a/b.md?q=/../x" leaves "/../x" in the query; then decode each part once. + raw_path, *raw_suffix = re.split(r"[?#]", normalized, maxsplit=1) + path, suffix = _fully_unquote(raw_path), _fully_unquote("".join(raw_suffix)) if ( # Excessive encoding depth, e.g. a name requiring more than 32 decoding passes. path is None - or decoded_name is None + or suffix is None # Absolute path, e.g. "/etc/passwd" or "%2fetc/passwd". or path.startswith("/") # Embedded URI, e.g. "http://example.com/other" or "%68ttp%3a%2f%2fexample.com". @@ -4582,7 +4588,7 @@ def _validate_resource_name(name: str) -> str | None: # (URI parsers can trim trailing spaces). or any(segment.rstrip(" ") == ".." for segment in re.split(r"[/?#]", path)) # Control characters anywhere, e.g. "a/\0/b.md", ".\t./x", or ".%09./x". - or _RESOURCE_CONTROL_CHARS.search(decoded_name) + or _RESOURCE_CONTROL_CHARS.search(path + suffix) ): logger.debug("Rejecting resource name with unsafe path components: %r", name) return None diff --git a/python/packages/core/tests/core/test_mcp_skills.py b/python/packages/core/tests/core/test_mcp_skills.py index 34c44155339..aecdd6f0ced 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -9,6 +9,7 @@ import io import json import zipfile +from collections.abc import Iterator from datetime import timedelta from unittest.mock import AsyncMock, patch from urllib.parse import unquote @@ -24,7 +25,7 @@ from pydantic import AnyUrl from agent_framework import CachingSkillsSource, MCPSkill, MCPSkillResource, MCPSkillsSource, SkillsSourceContext -from agent_framework._skills import _parse_mcp_skill_index +from agent_framework._skills import _fully_unquote, _parse_mcp_skill_index from .conftest import MockAgent @@ -100,6 +101,43 @@ async def _read_resource(uri: AnyUrl) -> ReadResourceResult: return client +@pytest.fixture(autouse=True) +def _clear_resource_name_decode_cache() -> Iterator[None]: + _fully_unquote.cache_clear() + yield + _fully_unquote.cache_clear() + + +def _encode(depth: int) -> str: + """Return ``"A"`` percent-encoded *depth* times, e.g. ``%2541`` for depth 2.""" + return "%" + "25" * (depth - 1) + "41" + + +# --------------------------------------------------------------------------- +# _fully_unquote tests +# --------------------------------------------------------------------------- + + +class TestFullyUnquote: + """Tests for recursive, cached resource-name decoding.""" + + def test_reuses_cached_layers(self) -> None: + assert _fully_unquote("guide%2520one.md") == "guide one.md" + + with patch("agent_framework._skills.unquote", wraps=unquote) as decode: + # A repeated name is served entirely from the cache. + assert _fully_unquote("guide%2520one.md") == "guide one.md" + decode.assert_not_called() + # A different name reaching a cached layer ("guide%20one.md") decodes only its first layer. + assert _fully_unquote("guide%25%32%30one.md") == "guide one.md" + decode.assert_called_once_with("guide%25%32%30one.md") + + @pytest.mark.parametrize("depths", [(32, 33), (33, 32)]) + def test_cached_layers_preserve_depth_limit(self, depths: tuple[int, int]) -> None: + for depth in depths: + assert _fully_unquote(_encode(depth)) == ("A" if depth <= 32 else None) + + # --------------------------------------------------------------------------- # _parse_mcp_skill_index tests # --------------------------------------------------------------------------- @@ -334,6 +372,9 @@ async def test_get_resource_unknown_returns_none(self) -> None: "references/guide.md?value=%00", "references/guide.md#value=%2509", "references/guide.md?value=%C2%85", + "references/%2500guide.md?version=1", + "references/guide.md?version=1#value=%2509", + "references/guide.md#section?value=%2509", ], ) @pytest.mark.parametrize( @@ -379,6 +420,10 @@ async def test_get_resource_path_traversal_returns_none(self, name: str, skill_m "references/guide.md#example=/../../other.md", "references/guide.md?example=%2e%2e%2f%2e%2e%2fother.md", "references/guide.md?src=https://example.com/other", + "references/guide%2520one.md?value=%2520#section%2520", + "references/guide%253fname.md#section?value=%2520", + "references/guide.md?", + "references/guide.md#", "references/guide.md ", ], ) @@ -420,12 +465,13 @@ async def test_get_resource_decoding_depth_is_bounded(self, depth: int, template client.read_resource.return_value = _make_text_result("safe content") fm = SkillFrontmatter(name="unit-converter", description="Convert between common units.") skill = MCPSkill(frontmatter=fm, skill_md_uri=root + "SKILL.md", client=client) - name = template.format("%" + "25" * (depth - 1) + "41") + name = template.format(_encode(depth)) with patch("agent_framework._skills.unquote", wraps=unquote) as decode: resource = await skill.get_resource(name) - assert decode.call_count <= 66 + # One pass decodes the unencoded part; the encoded part takes depth + 1 passes, capped at 33. + assert decode.call_count == min(depth + 1, 33) + 1 if depth <= 32 and not name.startswith("../"): assert resource is not None assert resource.name == name