diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs index 53ecb4d09ff..d5453e7d9be 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkill.cs @@ -4,6 +4,7 @@ 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 +26,13 @@ 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 const int MaxResourceNameDecodingDepth = 32; 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,10 @@ 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. + /// 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. /// public override async ValueTask GetResourceAsync(string name, CancellationToken cancellationToken = default) @@ -95,7 +103,15 @@ public override async ValueTask GetContentAsync(CancellationToken cancel return null; } - string uri = this._skillRootUri + 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 + normalized; ReadResourceResult result; try @@ -112,6 +128,52 @@ public override async ValueTask GetContentAsync(CancellationToken cancel return new AgentMcpSkillResource(name: name, result: result); } + private static bool IsResourceNameSafe(string 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 && 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". + && !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". + && !(path + suffix).Any(char.IsControl); + } + + /// + /// 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). + /// + /// The decoded value, or if the decoding depth exceeds the limit. + private static string? FullyUnescape(string 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 null; + } + + [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..e123eef8a40 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,8 @@ // 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; using Microsoft.Extensions.DependencyInjection; @@ -141,21 +144,173 @@ public async Task GetResourceAsync_UnknownName_ReturnsNullAsync() [InlineData("../escape.md")] [InlineData("references/../../escape.md")] [InlineData("..")] + [InlineData("..\\escape.md")] + [InlineData("/etc/passwd")] + [InlineData("http://example.com/other")] + [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%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")] + [InlineData(".. ")] + [InlineData(".%2e ")] + [InlineData("%2e%2e ")] + [InlineData("..%20")] + [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")] + [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) { - // 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 + Assert.Equal(SampleSkillMd, await skill.GetContentAsync()); + + // 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/100%.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?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 " + }) + { + 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.Replace('\\', '/'), Assert.Single(reads)); + } + } + + [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())); - var resource = await skill.GetResourceAsync(name); + string encoded = "%" + string.Concat(Enumerable.Repeat("25", depth - 1)) + "41"; - // Assert - resource does not exist on the server, so null is returned - Assert.Null(resource); + 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] @@ -242,6 +397,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..2efce44a0be 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -60,9 +60,11 @@ 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 +from urllib.parse import unquote import yaml from yaml.nodes import MappingNode, Node, ScalarNode @@ -4407,6 +4409,33 @@ async def read(self, **kwargs: Any) -> Any: return text if text else None +_RESOURCE_CONTROL_CHARS: Final = re.compile(r"[\x00-\x1f\x7f-\x9f]") +_MAX_RESOURCE_NAME_DECODING_DEPTH: Final[int] = 32 + + +@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. + """ + 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) class MCPSkill(Skill): """A :class:`Skill` discovered from an MCP server exposing the Agent Skills convention. @@ -4502,7 +4531,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 +4556,9 @@ 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. + 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. @@ -4539,8 +4570,26 @@ 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 normalized.startswith("/") or "://" in normalized or any(seg == ".." for seg in normalized.split("/")): + # 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 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". + 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(path + suffix) + ): logger.debug("Rejecting resource name with unsafe path components: %r", name) return None return normalized @@ -5050,6 +5099,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..aecdd6f0ced 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -9,8 +9,10 @@ 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 import pytest from mcp.shared.exceptions import McpError @@ -23,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 @@ -99,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 # --------------------------------------------------------------------------- @@ -295,9 +334,60 @@ 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%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", + ".. ", + ".%2e ", + "%2e%2e ", + "..%20", + "%252e%252e%2520", + "references/.. ", + "references/.. ?version=1", + "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( + "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) -> None: + 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 +397,89 @@ 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/100%.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?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 ", + ], + ) + @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("\\", "/"))) + + @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(_encode(depth)) + + with patch("agent_framework._skills.unquote", wraps=unquote) as decode: + resource = await skill.get_resource(name) + + # 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 + 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 @@ -390,6 +557,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"),