From bea7b7d4af8525fe71fe37bf482deba6f626a240 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh Date: Thu, 1 Oct 2026 12:52:06 +0100 Subject: [PATCH] Improve MCP skill archive loading consistency Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Skills/AgentMcpSkillsSource.cs | 5 + .../Loaders/AgentMcpSkillArchiveExtractor.cs | 67 ++++- .../Skills/Loaders/ArchiveEntryLoader.cs | 28 +-- .../AgentMcpSkillsSourceArchiveTests.cs | 236 ++++++++++++++++++ .../packages/core/agent_framework/_skills.py | 21 ++ .../core/tests/core/test_mcp_skills.py | 164 +++++++++++- 6 files changed, 486 insertions(+), 35 deletions(-) diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs index 139efd117e3..141b812262f 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/AgentMcpSkillsSource.cs @@ -48,6 +48,11 @@ namespace Microsoft.Agents.AI; /// does not apply to lazily fetched skill-md entries or their supporting resources. /// /// +/// Archive members resolving to the same file keep the first file's content. Later colliding members +/// are skipped with a warning, and discovery continues with the remaining archive content. +/// This applies whether or not an archive supplies a matching digest. +/// +/// /// Thread safety and archive reconciliation. For archive-type skills, every call to /// reconciles a shared on-disk directory: it extracts newly advertised /// skills, re-extracts existing ones, and prunes those the server no longer advertises. Because that diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs index 2f4c3105132..f9d5dcc5328 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs @@ -2,8 +2,11 @@ using System; using System.Buffers; +using System.Collections.Generic; using System.IO; using System.IO.Compression; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; namespace Microsoft.Agents.AI; @@ -13,8 +16,9 @@ namespace Microsoft.Agents.AI; /// /// Supports ZIP payloads. Extraction is guarded against path-traversal ("zip-slip") attacks and /// bounded by a maximum file count and total uncompressed size to mitigate decompression-bomb attacks. +/// Later entries resolving to an existing file are skipped, preserving the first file's content. /// -internal static class AgentMcpSkillArchiveExtractor +internal static partial class AgentMcpSkillArchiveExtractor { /// /// The default maximum number of files that may be extracted from a single archive, sized for a @@ -77,6 +81,7 @@ internal static ArchiveFormat DetectFormat(byte[] bytes, string? mediaType, stri /// The directory the archive is unpacked into. Created if missing. /// The maximum number of files that may be extracted from the archive. /// The maximum total uncompressed size, in bytes, of all extracted files. + /// An optional logger reporting each skipped colliding archive member. /// The format is not . /// The archive exceeds one of the supplied limits. internal static void Extract( @@ -84,7 +89,8 @@ internal static void Extract( ArchiveFormat format, string targetDirectory, int? maxFileCount = null, - long? maxUncompressedSizeBytes = null) + long? maxUncompressedSizeBytes = null, + ILogger? logger = null) { maxFileCount ??= DefaultMaxFileCount; maxUncompressedSizeBytes ??= DefaultMaxUncompressedSizeBytes; @@ -98,15 +104,17 @@ internal static void Extract( string fullTarget = Path.GetFullPath(targetDirectory); using var source = new MemoryStream(bytes, writable: false); - ExtractZip(source, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value); + ExtractZip(source, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value, logger ?? NullLogger.Instance); } - private static void ExtractZip(Stream source, string fullTarget, int maxFileCount, long maxUncompressedSizeBytes) + private static void ExtractZip(Stream source, string fullTarget, int maxFileCount, long maxUncompressedSizeBytes, ILogger logger) { using var archive = new ZipArchive(source, ZipArchiveMode.Read, leaveOpen: true); long remainingBytes = maxUncompressedSizeBytes; int fileCount = 0; + var destinations = new HashSet(OperatingSystem.IsWindows() ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); + string skillName = SanitizePathForLog(Path.GetFileName(fullTarget)); foreach (ZipArchiveEntry entry in archive.Entries) { @@ -127,11 +135,34 @@ private static void ExtractZip(Stream source, string fullTarget, int maxFileCoun continue; } + // Normalization can map distinct archive names to one path; keep the first file. + if (!destinations.Add(destination)) + { + LogArchiveMemberCollision(logger, skillName, SanitizePathForLog(entry.FullName)); + continue; + } + Directory.CreateDirectory(Path.GetDirectoryName(destination)!); - using Stream entryStream = entry.Open(); - using FileStream output = File.Create(destination); - CopyWithLimit(entryStream, output, ref remainingBytes); + // CreateNew also prevents overwrites through filesystem aliases, such as trailing dots on Windows. + FileStream output; + try + { +#pragma warning disable CA5389 // ResolveDestination canonicalizes the path and rejects destinations outside fullTarget. + output = new FileStream(destination, FileMode.CreateNew, FileAccess.Write, FileShare.None); +#pragma warning restore CA5389 + } + catch (IOException) when (File.Exists(destination)) + { + LogArchiveMemberCollision(logger, skillName, SanitizePathForLog(entry.FullName)); + continue; + } + + using (output) + { + using Stream entryStream = entry.Open(); + CopyWithLimit(entryStream, output, ref remainingBytes); + } } } @@ -212,4 +243,26 @@ internal static bool IsPathContainedIn(string parentDirectory, string candidateP prefix, OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal); } + + /// + /// Replaces control characters in a file-system path with ? so the path is safe to include + /// in log messages without risking terminal-escape injection. + /// + internal static string SanitizePathForLog(string path) + { + char[]? chars = null; + for (int i = 0; i < path.Length; i++) + { + if (char.IsControl(path[i])) + { + chars ??= path.ToCharArray(); + chars[i] = '?'; + } + } + + return chars is null ? path : new string(chars); + } + + [LoggerMessage(LogLevel.Warning, "Skipping duplicate archive member '{MemberPath}' for skill '{SkillName}'; keeping the first file.")] + private static partial void LogArchiveMemberCollision(ILogger logger, string skillName, string memberPath); } diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs index 9b4791747f8..3145554132c 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs @@ -26,6 +26,8 @@ namespace Microsoft.Agents.AI; /// inside an archive are surfaced as readable resources only; they are never discovered as /// executable scripts. Supplied SHA-256 digests are verified before extraction; archives without /// a digest remain supported. +/// Archive members resolving to the same file keep the first file; later colliding members are +/// skipped with a warning. /// internal sealed partial class ArchiveEntryLoader : IMcpSkillEntryLoader, IDisposable { @@ -146,7 +148,7 @@ private void ReconcileArchiveSkillDirectories(string? archiveSkillsDirectory, Li try { Directory.Delete(directory, recursive: true); - LogArchiveSkillPruned(this._logger, name, SanitizePathForLog(directory)); + LogArchiveSkillPruned(this._logger, name, AgentMcpSkillArchiveExtractor.SanitizePathForLog(directory)); } catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { @@ -240,7 +242,8 @@ private async Task> TryDownloadAndExtractSkillAsync(McpSki format, skillDirectory, this._options?.ArchiveMaxFileCount, - this._options?.ArchiveMaxUncompressedSizeBytes); + this._options?.ArchiveMaxUncompressedSizeBytes, + logger: this._logger); } catch (Exception ex) { @@ -252,7 +255,7 @@ private async Task> TryDownloadAndExtractSkillAsync(McpSki return []; } - LogArchiveExtracted(this._logger, entry.Name!, SanitizePathForLog(skillDirectory)); + LogArchiveExtracted(this._logger, entry.Name!, AgentMcpSkillArchiveExtractor.SanitizePathForLog(skillDirectory)); return [skillDirectory]; } @@ -393,25 +396,6 @@ static bool IsValidDirectoryName(string value, char[] invalidChars) } } - /// - /// Replaces control characters in a file-system path with ? so the path is safe to include - /// in log messages without risking terminal-escape injection. - /// - private static string SanitizePathForLog(string path) - { - char[]? chars = null; - for (int i = 0; i < path.Length; i++) - { - if (char.IsControl(path[i])) - { - chars ??= path.ToCharArray(); - chars[i] = '?'; - } - } - - return chars is null ? path : new string(chars); - } - /// /// Attempts to recursively delete a skill directory. /// diff --git a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs index f9754c34960..6a4ed11b222 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs @@ -242,6 +242,164 @@ public async Task GetSkillsAsync_ArchiveExistingMetadataScalarFormats_PreservedA Assert.Contains("Body.", await skill.GetContentAsync()); } + /// + /// Archive member aliases and ordering, content, and digest combinations for collision tests. + /// + public static TheoryData ArchiveCollisionCases + { + get + { + var aliases = new List + { + "refs/policy.md", + ".//refs//policy.md", + "refs\\policy.md", + "refs/./policy.md", + "/refs/policy.md", + }; + if (OperatingSystem.IsWindows()) + { + aliases.Add("refs/POLICY.md"); + aliases.Add("refs/policy.md."); + } + + var cases = new TheoryData(); + foreach (string alias in aliases) + { + foreach (bool aliasFirst in new[] { false, true }) + { + foreach (bool identical in new[] { false, true }) + { + foreach (bool withDigest in new[] { false, true }) + { + cases.Add(alias, aliasFirst, identical, withDigest); + } + } + } + } + + return cases; + } + } + + [Theory] + [MemberData(nameof(ArchiveCollisionCases))] + public async Task GetSkillsAsync_CollidingArchiveResources_KeepsFirstFileAndLogsWarningAsync( + string alias, bool aliasFirst, bool identical, bool withDigest) + { + // Arrange + string firstName = aliasFirst ? alias : "refs/policy.md"; + string secondName = aliasFirst ? "refs/policy.md" : alias; + byte[] archive = BuildZip( + ("SKILL.md", ArchivedSkillMd), + ("refs/", ""), + (firstName, "First resource."), + (secondName, identical ? "First resource." : "Substituted resource."), + (".//refs//other.md", "Unaffected resource.")); + string index = withDigest + ? ArchiveIndexWithDigest(ArchiveDigest(archive)) + : ArchiveIndex("archived-skill", "skill://archives/archived-skill.zip"); + await using var server = CreateArchiveServer(index, new Dictionary { ["archived-skill"] = archive }); + await using var client = await server.CreateClientAsync(); + var logger = new Mock(); + logger.Setup(l => l.IsEnabled(It.IsAny())).Returns(true); + var loggerFactory = new Mock(); + loggerFactory.Setup(f => f.CreateLogger(It.IsAny())).Returns(logger.Object); + var source = new AgentMcpSkillsSource( + client, new() { ArchiveSkillsDirectory = this._extractionRoot }, loggerFactory.Object); + + // Act / Assert + for (int i = 0; i < 2; i++) + { + var skill = Assert.Single(await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create())); + string resourceName = aliasFirst && alias == "refs/POLICY.md" ? alias : "refs/policy.md"; + var resource = await skill.GetResourceAsync(resourceName); + Assert.NotNull(resource); + Assert.Equal("First resource.", await resource.ReadAsync()); + var other = await skill.GetResourceAsync("refs/other.md"); + Assert.NotNull(other); + Assert.Equal("Unaffected resource.", await other.ReadAsync()); + } + + logger.Verify( + l => l.Log( + LogLevel.Warning, + It.IsAny(), + It.Is((state, _) => + state.ToString()!.Contains("Skipping duplicate archive member", StringComparison.Ordinal) && + state.ToString()!.Contains("keeping the first file", StringComparison.Ordinal) && + !state.ToString()!.Contains("Substituted resource.", StringComparison.Ordinal)), + It.IsAny(), + It.IsAny>()), + Times.Exactly(2)); + } + + [Theory] + [InlineData("SKILL.md", false)] + [InlineData("SKILL.md", true)] + [InlineData(".//SKILL.md", false)] + [InlineData(".//SKILL.md", true)] + [InlineData("/SKILL.md", false)] + [InlineData("/SKILL.md", true)] + public async Task GetSkillsAsync_CollidingArchiveSkillMd_KeepsFirstFileAndLogsWarningAsync(string alias, bool aliasFirst) + { + // Arrange + byte[] archive = BuildZip( + (aliasFirst ? alias : "SKILL.md", ArchivedSkillMd), + (aliasFirst ? "SKILL.md" : alias, ArchivedSkillMd.Replace("name: archived-skill", "name: other-skill", StringComparison.Ordinal)), + ("reference.md", "Unaffected resource.")); + await using var server = CreateArchiveServer( + ArchiveIndex("archived-skill", "skill://archives/archived-skill.zip"), + new Dictionary { ["archived-skill"] = archive }); + await using var client = await server.CreateClientAsync(); + var logger = new Mock(); + logger.Setup(l => l.IsEnabled(It.IsAny())).Returns(true); + var loggerFactory = new Mock(); + loggerFactory.Setup(f => f.CreateLogger(It.IsAny())).Returns(logger.Object); + var source = new AgentMcpSkillsSource( + client, new() { ArchiveSkillsDirectory = this._extractionRoot }, loggerFactory.Object); + + // Act + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + + // Assert + var skill = Assert.Single(skills); + Assert.Equal("archived-skill", skill.Frontmatter.Name); + Assert.Contains("Body from the archive.", await skill.GetContentAsync()); + logger.Verify( + l => l.Log( + LogLevel.Warning, + It.IsAny(), + It.Is((state, _) => + state.ToString()!.Contains("Skipping duplicate archive member", StringComparison.Ordinal)), + It.IsAny(), + It.IsAny>()), + Times.Once); + } + + [Fact] + public async Task GetSkillsAsync_ArchiveCollision_DoesNotSkipOtherSkillsAsync() + { + // Arrange + byte[] archiveA = BuildZip(("SKILL.md", SkillAMd), ("reference.md", "First."), ("./reference.md", "Second.")); + byte[] archiveB = BuildZip(("SKILL.md", SkillBMd)); + JsonNode index = JsonNode.Parse(ArchiveIndex("skill-a", "skill://archives/skill-a.zip"))!; + index["skills"]!.AsArray().Add( + JsonNode.Parse(ArchiveIndex("skill-b", "skill://archives/skill-b.zip"))!["skills"]![0]!.DeepClone()); + await using var server = CreateArchiveServer( + index.ToJsonString(), new Dictionary { ["skill-a"] = archiveA, ["skill-b"] = archiveB }); + await using var client = await server.CreateClientAsync(); + var source = new AgentMcpSkillsSource(client, new() { ArchiveSkillsDirectory = this._extractionRoot }); + + // Act + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + + // Assert + Assert.Equal(2, skills.Count); + Assert.Contains(skills, skill => skill.Frontmatter.Name == "skill-a"); + Assert.Contains(skills, skill => skill.Frontmatter.Name == "skill-b"); + } + /// /// Malformed, unsupported, and mismatched archive digests that must be rejected. /// @@ -763,6 +921,72 @@ public void Extract_ArchiveExceedsDefaultFileCount_Throws() () => AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target)); } + [Fact] + public void Extract_DuplicateMembers_CountTowardFileLimit() + { + // Arrange + byte[] zip = BuildZip(("reference.md", "First."), ("reference.md", "Second.")); + string target = Path.Combine(this._extractionRoot, "skill"); + + // Act / Assert + Assert.Throws( + () => AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target, maxFileCount: 1)); + } + + [Fact] + public void Extract_SkippedDuplicate_IsNotDecompressed() + { + // Arrange + byte[] zip = BuildZip(("reference.md", "First."), ("./reference.md", new string('x', 100))); + string target = Path.Combine(this._extractionRoot, "skill"); + var logger = new Mock(); + logger.Setup(l => l.IsEnabled(It.IsAny())).Returns(true); + + // Act + AgentMcpSkillArchiveExtractor.Extract( + zip, ArchiveFormat.Zip, target, maxUncompressedSizeBytes: Encoding.UTF8.GetByteCount("First."), logger: logger.Object); + + // Assert + Assert.Equal("First.", File.ReadAllText(Path.Combine(target, "reference.md"))); + VerifyArchiveMemberCollision(logger, "./reference.md", Times.Once()); + } + + [Fact] + public void Extract_ExistingFile_IsNotOverwritten() + { + // Arrange + string target = Path.Combine(this._extractionRoot, "skill"); + Directory.CreateDirectory(target); + File.WriteAllText(Path.Combine(target, "reference.md"), "Existing."); + byte[] zip = BuildZip(("reference.md", "Substituted.")); + var logger = new Mock(); + logger.Setup(l => l.IsEnabled(It.IsAny())).Returns(true); + + // Act + AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target, logger: logger.Object); + + // Assert + Assert.Equal("Existing.", File.ReadAllText(Path.Combine(target, "reference.md"))); + VerifyArchiveMemberCollision(logger, "reference.md", Times.Once()); + } + + [Fact] + public void Extract_FileDirectoryConflict_IsNotSkippedAsDuplicate() + { + // Arrange + string target = Path.Combine(this._extractionRoot, "skill"); + Directory.CreateDirectory(Path.Combine(target, "reference.md")); + byte[] zip = BuildZip(("reference.md", "Content.")); + var logger = new Mock(); + logger.Setup(l => l.IsEnabled(It.IsAny())).Returns(true); + + // Act / Assert + Exception? exception = Record.Exception( + () => AgentMcpSkillArchiveExtractor.Extract(zip, ArchiveFormat.Zip, target, logger: logger.Object)); + Assert.True(exception is IOException or UnauthorizedAccessException); + VerifyArchiveMemberCollision(logger, "reference.md", Times.Never()); + } + [Fact] public void Extract_ArchiveExceedsDefaultUncompressedSize_Throws() { @@ -1043,6 +1267,18 @@ public void Dispose() } } + private static void VerifyArchiveMemberCollision(Mock logger, string memberPath, Times times) => + logger.Verify( + l => l.Log( + LogLevel.Warning, + It.IsAny(), + It.Is((state, _) => + state.ToString()!.Contains($"Skipping duplicate archive member '{memberPath}'", StringComparison.Ordinal) && + state.ToString()!.Contains("keeping the first file", StringComparison.Ordinal)), + It.IsAny(), + It.IsAny>()), + times); + private static byte[] BuildZip(params (string Path, string Content)[] entries) { using var ms = new MemoryStream(); diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index 71973281c25..3bef5b39424 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -4993,6 +4993,8 @@ def _extract_archive_to_memory( ``..`` parent-traversal ("zip-slip") aborts extraction of the whole archive by raising. Extraction is bounded by a maximum file count and total uncompressed size to mitigate decompression-bomb attacks. No filesystem is touched. + Later members whose normalized paths match case-insensitively are skipped + with a warning; the first member's spelling and content are retained. Args: data: The raw archive bytes. @@ -5023,6 +5025,7 @@ def _extract_zip_to_memory( """Read regular files from a ZIP archive into memory. See :func:`_extract_archive_to_memory`.""" remaining_bytes = max_uncompressed_size_bytes files: dict[str, bytes] = {} + seen_names: set[str] = set() file_count = 0 for info in archive.infolist(): @@ -5037,9 +5040,21 @@ def _extract_zip_to_memory( if name is None: continue + # Match FileSkill's case-insensitive lookup; retain the first member's spelling. + lookup_name = name.lower() + if lookup_name in seen_names: + logger.warning( + "Skipping duplicate archive member %r: normalized path %r matches an earlier member; " + "keeping the first file", + info.filename, + name, + ) + continue + with archive.open(info) as source: content, remaining_bytes = _read_member_with_limit(source, remaining_bytes) files[name] = content + seen_names.add(lookup_name) return files @@ -5059,6 +5074,8 @@ class _ArchiveEntryLoader: Extraction is hardened against path-traversal ("zip-slip") member names, oversized downloads, excessive file counts, and decompression bombs. + Member paths are compared case-insensitively; collisions are skipped with a + warning, keeping the first file's spelling and content. Supplied SHA-256 digests are verified before extraction; archives without a digest remain supported. """ @@ -5333,6 +5350,10 @@ class MCPSkillsSource(SkillsSource): archive are surfaced as read-only resources only; MCP-delivered scripts are never discovered as runnable. + Archive member paths are compared case-insensitively, matching resource lookup. + The first file's spelling and content are retained; later colliding members + are skipped with a warning. + Entries whose type has no registered handler (e.g. ``mcp-resource-template``) are skipped. diff --git a/python/packages/core/tests/core/test_mcp_skills.py b/python/packages/core/tests/core/test_mcp_skills.py index aecdd6f0ced..6b9bf0f9060 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -8,8 +8,9 @@ import hashlib import io import json +import warnings import zipfile -from collections.abc import Iterator +from collections.abc import Iterator, Mapping, Sequence from datetime import timedelta from unittest.mock import AsyncMock, patch from urllib.parse import unquote @@ -895,12 +896,15 @@ async def test_index_timeout_error_propagates(self) -> None: # --------------------------------------------------------------------------- -def _make_zip(files: dict[str, bytes]) -> bytes: - """Build an in-memory ZIP archive from a ``{path: content}`` mapping.""" +def _make_zip(files: Mapping[str, bytes] | Sequence[tuple[str, bytes]]) -> bytes: + """Build an in-memory ZIP from a mapping or ordered entries, including duplicate names.""" buffer = io.BytesIO() - with zipfile.ZipFile(buffer, "w", zipfile.ZIP_DEFLATED) as archive: - for name, data in files.items(): - archive.writestr(name, data) + entries = files.items() if isinstance(files, Mapping) else files + with warnings.catch_warnings(): + warnings.filterwarnings("ignore", message="Duplicate name:", category=UserWarning, module="zipfile") + with zipfile.ZipFile(buffer, "w", zipfile.ZIP_DEFLATED) as archive: + for name, data in entries: + archive.writestr(name, data) return buffer.getvalue() @@ -1354,6 +1358,100 @@ async def test_zip_slip_archive_skips_whole_skill(self) -> None: assert skills == [] +class TestMCPSkillsSourceArchiveCollisions: + """Colliding archive members warn and preserve the first file through discovery.""" + + @pytest.mark.parametrize( + "alias", + [ + "refs/policy.md", + ".//refs//policy.md", + "refs\\policy.md", + "refs/./policy.md", + "/refs/policy.md", + "refs/POLICY.md", + "REFS\\POLICY.MD", + ], + ) + @pytest.mark.parametrize("alias_first", [False, True]) + @pytest.mark.parametrize("identical", [False, True]) + @pytest.mark.parametrize("with_digest", [False, True]) + async def test_colliding_resources_keep_first_file_and_warn( + self, alias: str, alias_first: bool, identical: bool, with_digest: bool, caplog: pytest.LogCaptureFixture + ) -> None: + url = "skill://archives/packaged-skill.zip" + first_name, second_name = (alias, "refs/policy.md") if alias_first else ("refs/policy.md", alias) + archive = _make_zip([ + ("SKILL.md", ARCHIVE_SKILL_MD.encode()), + ("refs/", b""), + (first_name, b"First resource."), + (second_name, b"First resource." if identical else b"Substituted resource."), + (".//refs//other.md", b"Unaffected resource."), + ]) + digest = f"sha256:{hashlib.sha256(archive).hexdigest()}" if with_digest else None + index = _make_archive_index("packaged-skill", url, digest=digest) + source = MCPSkillsSource(client=_archive_client(index, url, archive, "application/zip")) + + for _ in range(2): + skills = await source.get_skills(_SOURCE_CTX) + + assert len(skills) == 1 + for resource_name in ("refs/policy.md", "REFS/POLICY.MD"): + resource = await skills[0].get_resource(resource_name) + assert resource is not None + assert await resource.read() == "First resource." + other = await skills[0].get_resource("refs/other.md") + assert other is not None + assert await other.read() == "Unaffected resource." + + collisions = [record for record in caplog.records if "duplicate archive member" in record.getMessage()] + assert len(collisions) == 2 + assert all(record.levelname == "WARNING" for record in collisions) + assert all("keeping the first file" in record.getMessage() for record in collisions) + assert all("Substituted resource." not in record.getMessage() for record in collisions) + + @pytest.mark.parametrize("alias", ["SKILL.md", ".//SKILL.md", "/SKILL.md", "skill.md", "Skill.MD"]) + @pytest.mark.parametrize("alias_first", [False, True]) + async def test_colliding_skill_md_keeps_first_file_and_warns( + self, alias: str, alias_first: bool, caplog: pytest.LogCaptureFixture + ) -> None: + url = "skill://archives/packaged-skill.zip" + first_name, second_name = (alias, "SKILL.md") if alias_first else ("SKILL.md", alias) + archive = _make_zip([ + (first_name, ARCHIVE_SKILL_MD.encode()), + (second_name, ARCHIVE_SKILL_MD.replace("name: packaged-skill", "name: other-skill").encode()), + ("reference.md", b"Unaffected resource."), + ]) + index = _make_archive_index("packaged-skill", url) + + skills = await MCPSkillsSource(client=_archive_client(index, url, archive, "application/zip")).get_skills( + _SOURCE_CTX + ) + + assert len(skills) == 1 + assert skills[0].frontmatter.name == "packaged-skill" + assert "Instructions from an archive." in await skills[0].get_content() + assert len(caplog.records) == 1 + assert caplog.records[0].levelname == "WARNING" + assert "duplicate archive member" in caplog.text + + async def test_collision_does_not_skip_other_skills(self, caplog: pytest.LogCaptureFixture) -> None: + url = "skill://archives/packaged-skill.zip" + index_data = json.loads(SAMPLE_SKILL_INDEX) + index_data["skills"].extend(json.loads(_make_archive_index("packaged-skill", url))["skills"]) + archive = _make_zip([ + ("SKILL.md", ARCHIVE_SKILL_MD.encode()), + ("reference.md", b"First."), + ("./reference.md", b"Second."), + ]) + client = _archive_client(json.dumps(index_data), url, archive, "application/zip") + + skills = await MCPSkillsSource(client=client).get_skills(_SOURCE_CTX) + + assert sorted(skill.frontmatter.name for skill in skills) == ["packaged-skill", "unit-converter"] + assert "duplicate archive member" in caplog.text + + class TestMCPSkillsSourceArchiveDigest: """Tests for archive digest verification through the MCP discovery pipeline.""" @@ -1658,6 +1756,60 @@ def test_file_count_limit_is_enforced(self) -> None: with pytest.raises(ValueError, match="file count"): _extract_archive_to_memory(archive, _ArchiveFormat.ZIP, 2, 1024 * 1024) + def test_duplicate_members_count_toward_file_limit(self) -> None: + from agent_framework._skills import _ArchiveFormat, _extract_archive_to_memory + + archive = _make_zip([("reference.md", b"First."), ("reference.md", b"Second.")]) + + with pytest.raises(ValueError, match="file count"): + _extract_archive_to_memory(archive, _ArchiveFormat.ZIP, 1, 1024 * 1024) + + def test_skipped_duplicate_is_not_decompressed(self, caplog: pytest.LogCaptureFixture) -> None: + from agent_framework._skills import _ArchiveFormat, _extract_archive_to_memory, _read_member_with_limit + + archive = _make_zip([("reference.md", b"First."), ("./reference.md", b"x" * 100)]) + with patch("agent_framework._skills._read_member_with_limit", wraps=_read_member_with_limit) as read: + files = _extract_archive_to_memory(archive, _ArchiveFormat.ZIP, 20, len(b"First.")) + + assert files == {"reference.md": b"First."} + read.assert_called_once() + assert "duplicate archive member" in caplog.text + + @pytest.mark.parametrize("first_name", ["reference.md", "REFERENCE.md"]) + def test_case_collision_keeps_first_name_and_warns(self, first_name: str, caplog: pytest.LogCaptureFixture) -> None: + from agent_framework._skills import _ArchiveFormat, _extract_archive_to_memory + + second_name = "REFERENCE.md" if first_name == "reference.md" else "reference.md" + archive = _make_zip([(first_name, b"First."), (second_name, b"Second.")]) + + files = _extract_archive_to_memory(archive, _ArchiveFormat.ZIP, 20, 1024 * 1024) + + assert files == {first_name: b"First."} + assert len(caplog.records) == 1 + assert caplog.records[0].levelname == "WARNING" + assert "duplicate archive member" in caplog.text + + def test_distinct_lowercase_resource_names_are_preserved(self, caplog: pytest.LogCaptureFixture) -> None: + from agent_framework._skills import _ArchiveFormat, _extract_archive_to_memory + + # Resource lookup uses lower(), not Unicode casefold(). + archive = _make_zip({"stra\u00dfe.md": b"First.", "STRASSE.md": b"Second."}) + + files = _extract_archive_to_memory(archive, _ArchiveFormat.ZIP, 20, 1024 * 1024) + + assert files == {"stra\u00dfe.md": b"First.", "STRASSE.md": b"Second."} + assert not caplog.records + + def test_distinct_trailing_dot_names_are_preserved(self, caplog: pytest.LogCaptureFixture) -> None: + from agent_framework._skills import _ArchiveFormat, _extract_archive_to_memory + + archive = _make_zip({"reference.md": b"First.", "reference.md.": b"Second."}) + + files = _extract_archive_to_memory(archive, _ArchiveFormat.ZIP, 20, 1024 * 1024) + + assert files == {"reference.md": b"First.", "reference.md.": b"Second."} + assert not caplog.records + def test_uncompressed_size_limit_is_enforced(self) -> None: from agent_framework._skills import _ArchiveFormat, _extract_archive_to_memory