SergeyMenshykh · Sep 24, 2026 · Sep 23, 2026 · Sep 23, 2026 · Sep 23, 2026 · Sep 23, 2026
@@ -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.
 /// </para>
 /// </remarks>
-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 <c>SKILL.md</c> segment.
     /// </param>
     /// <param name="client">The MCP client used to fetch resources on demand.</param>
-    public AgentMcpSkill(AgentSkillFrontmatter frontmatter, string skillMdUri, McpClient client)
+    /// <param name="logger">The logger used to report rejected resource names.</param>
+    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);
     }
 
     /// <inheritdoc/>
@@ -85,7 +90,10 @@ public override async ValueTask<string> GetContentAsync(CancellationToken cancel
     /// <remarks>
     /// Resolves <paramref name="name"/> as a relative path against the skill's root URI, issues a
     /// <c>resources/read</c> request to the MCP server, and returns an <see cref="AgentMcpSkillResource"/>
-    /// with the pre-fetched content. Returns <see langword="null"/> 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 <see langword="null"/> when the name is empty or unsafe, the server
     /// returns no content, or the resource does not exist on the server.
     /// </remarks>
     public override async ValueTask<AgentSkillResource?> GetResourceAsync(string name, CancellationToken cancellationToken = default)
@@ -95,7 +103,15 @@ public override async ValueTask<string> 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<string> 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);
+    }
+
+    /// <summary>
+    /// Percent-decodes <paramref name="value"/> until stable, normalizing backslashes to forward slashes.
+    /// Decoding never removes a literal <c>.</c>, <c>/</c>, <c>:</c>, or control character, so checking
+    /// the final form also covers every nested encoding layer (e.g. <c>%252e</c>).
+    /// </summary>
+    /// <returns>The decoded value, or <see langword="null"/> if the decoding depth exceeds the limit.</returns>
+    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);
+
     /// <summary>
     /// Strips the trailing <c>SKILL.md</c> from the URI to produce the skill's root directory URI.
     /// If the URI doesn't end with <c>SKILL.md</c>, ensures it ends with a trailing slash.

@@ -71,6 +71,12 @@ namespace Microsoft.Agents.AI;
 /// </list>
 /// </para>
 /// <para>
+/// 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.
+/// </para>
+/// <para>
 /// <strong>Security considerations:</strong> This source discovers and loads skills — including full
 /// skill instructions and, for <c>archive</c>-type entries, files extracted to local disk — from a
 /// remote MCP server that the caller connects to explicitly (via <c>UseMcpSkills</c>); it is never

@@ -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);
 

@@ -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<IndexAndSkill>());
+        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<string> 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<string> 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<string> 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<string> 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<T>)
 
     // CA1812 flags these classes as "never instantiated", which is technically correct -