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 3957aaf0c8b..2f4c3105132 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/AgentMcpSkillArchiveExtractor.cs @@ -2,7 +2,6 @@ using System; using System.Buffers; -using System.Formats.Tar; using System.IO; using System.IO.Compression; @@ -12,11 +11,8 @@ namespace Microsoft.Agents.AI; /// Unpacks skill archives downloaded from an MCP server into a local directory. /// /// -/// Supports ZIP, TAR, and gzip-compressed TAR payloads. Extraction is guarded against path-traversal -/// ("zip-slip") attacks: every entry must resolve to a path beneath the target directory. Non-regular -/// TAR entries (symbolic links, hard links, device nodes, etc.) are skipped so an archive cannot -/// create links that escape the target directory. Extraction is also bounded by a maximum file count -/// and total uncompressed size to mitigate decompression-bomb attacks. +/// 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. /// internal static class AgentMcpSkillArchiveExtractor { @@ -45,9 +41,10 @@ internal static class AgentMcpSkillArchiveExtractor internal static ArchiveFormat DetectFormat(byte[] bytes, string? mediaType, string? url) { // Magic-number sniffing is the most reliable signal. + // Reject gzip by signature before considering potentially incorrect MIME type or URL hints. if (bytes.Length >= 2 && bytes[0] == 0x1F && bytes[1] == 0x8B) { - return ArchiveFormat.TarGz; + return ArchiveFormat.Unknown; } if (bytes.Length >= 4 && bytes[0] == 0x50 && bytes[1] == 0x4B && @@ -63,35 +60,12 @@ internal static ArchiveFormat DetectFormat(byte[] bytes, string? mediaType, stri return ArchiveFormat.Zip; } - if (string.Equals(media, "application/gzip", StringComparison.OrdinalIgnoreCase) || - string.Equals(media, "application/x-gzip", StringComparison.OrdinalIgnoreCase) || - string.Equals(media, "application/x-compressed-tar", StringComparison.OrdinalIgnoreCase)) - { - return ArchiveFormat.TarGz; - } - - if (string.Equals(media, "application/x-tar", StringComparison.OrdinalIgnoreCase) || - string.Equals(media, "application/tar", StringComparison.OrdinalIgnoreCase)) - { - return ArchiveFormat.Tar; - } - string u = url ?? string.Empty; if (u.EndsWith(".zip", StringComparison.OrdinalIgnoreCase)) { return ArchiveFormat.Zip; } - if (u.EndsWith(".tar.gz", StringComparison.OrdinalIgnoreCase) || u.EndsWith(".tgz", StringComparison.OrdinalIgnoreCase)) - { - return ArchiveFormat.TarGz; - } - - if (u.EndsWith(".tar", StringComparison.OrdinalIgnoreCase)) - { - return ArchiveFormat.Tar; - } - return ArchiveFormat.Unknown; } @@ -103,7 +77,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. - /// The format is . + /// The format is not . /// The archive exceeds one of the supplied limits. internal static void Extract( byte[] bytes, @@ -115,28 +89,16 @@ internal static void Extract( maxFileCount ??= DefaultMaxFileCount; maxUncompressedSizeBytes ??= DefaultMaxUncompressedSizeBytes; + if (format != ArchiveFormat.Zip) + { + throw new NotSupportedException($"Unsupported skill archive format '{format}'. Use ZIP instead."); + } + Directory.CreateDirectory(targetDirectory); string fullTarget = Path.GetFullPath(targetDirectory); using var source = new MemoryStream(bytes, writable: false); - - switch (format) - { - case ArchiveFormat.Zip: - ExtractZip(source, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value); - break; - case ArchiveFormat.Tar: - ExtractTar(source, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value); - break; - case ArchiveFormat.TarGz: - { - using var gzip = new GZipStream(source, CompressionMode.Decompress); - ExtractTar(gzip, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value); - break; - } - default: - throw new NotSupportedException($"Unsupported skill archive format '{format}'."); - } + ExtractZip(source, fullTarget, maxFileCount.Value, maxUncompressedSizeBytes.Value); } private static void ExtractZip(Stream source, string fullTarget, int maxFileCount, long maxUncompressedSizeBytes) @@ -173,39 +135,6 @@ private static void ExtractZip(Stream source, string fullTarget, int maxFileCoun } } - private static void ExtractTar(Stream source, string fullTarget, int maxFileCount, long maxUncompressedSizeBytes) - { - using var reader = new TarReader(source, leaveOpen: true); - - long remainingBytes = maxUncompressedSizeBytes; - int fileCount = 0; - - while (reader.GetNextEntry() is { } entry) - { - // Only regular files are materialized. Skipping links/devices avoids both unsupported - // entry types and link-based escapes outside the target directory. - if (entry.EntryType is not (TarEntryType.RegularFile or TarEntryType.V7RegularFile)) - { - continue; - } - - if (++fileCount > maxFileCount) - { - throw new InvalidDataException($"Skill archive exceeds the maximum allowed file count ({maxFileCount})."); - } - - string? destination = ResolveDestination(fullTarget, entry.Name); - if (destination is null || entry.DataStream is null) - { - continue; - } - - Directory.CreateDirectory(Path.GetDirectoryName(destination)!); - using FileStream output = File.Create(destination); - CopyWithLimit(entry.DataStream, output, ref remainingBytes); - } - } - /// /// Copies to while decrementing a shared /// uncompressed-byte budget, throwing once it is exhausted. This is the authoritative defense against 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 1b309a31cc7..1bc1ff2667a 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveEntryLoader.cs @@ -15,10 +15,9 @@ namespace Microsoft.Agents.AI; /// /// Loads archive index entries: each entry's url points to a single archive resource -/// (application/zip, application/x-tar, or gzip-compressed TAR) whose content unpacks -/// into the skill's namespace. Archives are downloaded, extracted to a local directory, and the -/// resulting files are discovered via an internal that this -/// loader proxies to. +/// in ZIP format whose content unpacks into the skill's namespace. Archives are downloaded, +/// extracted to a local directory, and the resulting files are discovered via an internal +/// that this loader proxies to. /// /// /// Because MCP-delivered skills are treated strictly as instructor-format text, scripts bundled diff --git a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveFormat.cs b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveFormat.cs index 07d2c5f858c..99a510b3051 100644 --- a/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveFormat.cs +++ b/dotnet/src/Microsoft.Agents.AI.Mcp/Skills/Loaders/ArchiveFormat.cs @@ -12,10 +12,4 @@ internal enum ArchiveFormat /// A ZIP archive. Zip, - - /// An uncompressed TAR archive. - Tar, - - /// A gzip-compressed TAR archive (.tar.gz/.tgz). - TarGz, } 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 22956da27c9..f80798d38a7 100644 --- a/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.Mcp.UnitTests/Skills/AgentMcpSkillsSourceArchiveTests.cs @@ -2,7 +2,6 @@ using System; using System.Collections.Generic; -using System.Formats.Tar; using System.IO; using System.IO.Compression; using System.Linq; @@ -78,7 +77,7 @@ public async Task GetSkillsAsync_ZipArchive_DiscoversSkillAsync() } [Fact] - public async Task GetSkillsAsync_TarGzArchive_DiscoversSkillAsync() + public async Task GetSkillsAsync_TarGzArchive_SkipsSkillAsync() { // Arrange await using var server = new InMemoryMcpServer(builder => builder.WithResources()); @@ -90,9 +89,26 @@ public async Task GetSkillsAsync_TarGzArchive_DiscoversSkillAsync() 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()); + Assert.Empty(skills); + } + + [Fact] + public void DetectFormat_TarSignals_ReturnUnknown() + { + // Arrange / Act / Assert - TAR signals never become a supported archive format, + // including when weaker metadata claims that gzip data is ZIP. + Assert.Equal( + ArchiveFormat.Unknown, + AgentMcpSkillArchiveExtractor.DetectFormat([0x1F, 0x8B], "application/zip", "skill://archive.zip")); + Assert.Equal( + ArchiveFormat.Unknown, + AgentMcpSkillArchiveExtractor.DetectFormat([], "application/x-tar", null)); + Assert.Equal( + ArchiveFormat.Unknown, + AgentMcpSkillArchiveExtractor.DetectFormat([], null, "skill://archive.tar")); + Assert.Equal( + ArchiveFormat.Unknown, + AgentMcpSkillArchiveExtractor.DetectFormat([], null, "skill://archive.tgz")); } [Fact] @@ -329,42 +345,16 @@ public void Extract_ArchiveExceedsDefaultUncompressedSize_Throws() } [Fact] - public void Extract_TarGzExceedsUncompressedSize_Throws() + public void Extract_UnknownFormat_ThrowsBeforeCreatingTarget() { - // Arrange - a gzip-compressed tar whose expansion exceeds the default budget. The ZIP pre-gate - // does not apply here, so this exercises the authoritative streaming cap (CopyWithLimit). - string oversized = new('x', (int)AgentMcpSkillArchiveExtractor.DefaultMaxUncompressedSizeBytes + 1); - byte[] tarGz = BuildTarGz(("SKILL.md", oversized)); + // Arrange string target = Path.Combine(this._extractionRoot, "skill"); // Act / Assert - Assert.Throws( - () => AgentMcpSkillArchiveExtractor.Extract(tarGz, ArchiveFormat.TarGz, target)); - } - - [Fact] - public void Extract_TarWithLinkEntries_SkipsLinksAndExtractsRegularFiles() - { - // Arrange - a tar.gz containing symbolic-link and hard-link entries whose targets escape the - // target directory, alongside a regular file. Link entries must be skipped so an archive cannot - // create links that point outside the target directory. - byte[] tarGz = BuildTarGzFromEntries( - new PaxTarEntry(TarEntryType.SymbolicLink, "evil-symlink") { LinkName = "../../escaped.txt" }, - new PaxTarEntry(TarEntryType.HardLink, "evil-hardlink") { LinkName = "../../escaped.txt" }, - new PaxTarEntry(TarEntryType.RegularFile, "SKILL.md") - { - DataStream = new MemoryStream(Encoding.UTF8.GetBytes(ArchivedSkillMd)), - }); - string target = Path.Combine(this._extractionRoot, "skill"); - - // Act - AgentMcpSkillArchiveExtractor.Extract(tarGz, ArchiveFormat.TarGz, target); - - // Assert - only the regular file is materialized; neither link entry is written. - Assert.True(File.Exists(Path.Combine(target, "SKILL.md"))); - Assert.False(File.Exists(Path.Combine(target, "evil-symlink"))); - Assert.False(File.Exists(Path.Combine(target, "evil-hardlink"))); - Assert.Single(Directory.GetFileSystemEntries(target)); + var exception = Assert.Throws( + () => AgentMcpSkillArchiveExtractor.Extract([], ArchiveFormat.Unknown, target)); + Assert.Contains("Use ZIP instead", exception.Message); + Assert.False(Directory.Exists(target)); } [Fact] @@ -638,40 +628,6 @@ private static byte[] BuildZip(params (string Path, string Content)[] entries) return ms.ToArray(); } - private static byte[] BuildTarGz(params (string Path, string Content)[] entries) - { - using var ms = new MemoryStream(); - using (var gzip = new GZipStream(ms, CompressionMode.Compress, leaveOpen: true)) - using (var writer = new TarWriter(gzip, leaveOpen: true)) - { - foreach (var (path, content) in entries) - { - var entry = new PaxTarEntry(TarEntryType.RegularFile, path) - { - DataStream = new MemoryStream(Encoding.UTF8.GetBytes(content)), - }; - writer.WriteEntry(entry); - } - } - - return ms.ToArray(); - } - - private static byte[] BuildTarGzFromEntries(params TarEntry[] entries) - { - using var ms = new MemoryStream(); - using (var gzip = new GZipStream(ms, CompressionMode.Compress, leaveOpen: true)) - using (var writer = new TarWriter(gzip, leaveOpen: true)) - { - foreach (var entry in entries) - { - writer.WriteEntry(entry); - } - } - - return ms.ToArray(); - } - private static string ArchiveIndex(string skillName, string url) => $$""" { "$schema": "https://schemas.agentskills.io/discovery/0.2.0/schema.json", @@ -711,7 +667,7 @@ private sealed class TarGzArchiveServer [McpServerResource(UriTemplate = "skill://archives/archived-skill.tar.gz", Name = "archive", MimeType = "application/gzip")] public static BlobResourceContents Archive() => BlobResourceContents.FromBytes( - BuildTarGz(("SKILL.md", ArchivedSkillMd)), + new byte[] { 0x1F, 0x8B }, "skill://archives/archived-skill.tar.gz", "application/gzip"); } diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 6e778c5c3be..471883c9fdc 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -153,7 +153,7 @@ The vector store API is experimental under the shared `VECTOR_STORES` feature ID - **`SkillScriptArgumentParser`** - Public type alias for an optional callable `(raw args: dict | list[str] | str | None) -> dict | None` that converts the raw `args` value before an `InlineSkillScript` runs (applied before the inline list-args guard). It is an opt-in customization hook (port of .NET PR #6498) that lets callers support backends sending tool-call arguments in a non-conforming shape (e.g. vLLM JSON strings). The output is constrained to a `dict` (named keyword arguments) or `None`, because inline scripts bind arguments by keyword name. Supply it via the `argument_parser=` constructor arg on `InlineSkillScript`, `InlineSkill` (default for scripts added via `@skill.script`), or `ClassSkill` (default for scripts discovered via `@ClassSkill.script`). When `None` (the default), the raw value is used unchanged. File-based scripts are unaffected (their runner owns arg handling). - **`SkillsProvider`** - Context provider (extends `ContextProvider`) that discovers file-based skills from `SKILL.md` files and/or accepts code-defined `Skill` instances. Follows progressive disclosure: advertise → load → read resources / run scripts. By default all three tools it exposes (`load_skill`, `read_skill_resource`, `run_skill_script`) are registered with `approval_mode="always_require"`, so every skill operation needs approval. To run unattended, pass one of the static auto-approval rules to `ToolApprovalMiddleware` (via `auto_approval_rules`): `SkillsProvider.read_only_tools_auto_approval_rule` approves only the read-only tools (`load_skill`, `read_skill_resource`) while still prompting for `run_skill_script`, and `SkillsProvider.all_tools_auto_approval_rule` approves every skill tool including script execution. Both rules reject any call carrying a `server_label` so they stay scoped to this provider's local tools and never auto-approve a same-named hosted tool. Alternatively, for trusted skills, the constructor / `from_paths` kwargs `disable_load_skill_approval`, `disable_read_skill_resource_approval`, and `disable_run_skill_script_approval` (all default `False`) opt individual tools out of approval entirely by registering them with `approval_mode="never_require"` (the auto-approval rules only apply to tools that still require approval). The tool names are also exposed as class constants (`LOAD_SKILL_TOOL_NAME`, `READ_SKILL_RESOURCE_TOOL_NAME`, `RUN_SKILL_SCRIPT_TOOL_NAME`). - **`FileSkillsSource`** - `SkillsSource` that discovers file-based skills by scanning configured root paths for `SKILL.md`. The **configured root paths define the trust boundary** and are used as given (a root may itself be a symlink); everything discovered *below* a root is link-checked and fails closed. `_discover_skill_directories` rejects any entry that is a symbolic link, junction, or other reparse point (via the shared `agent_framework._filesystem.is_link_or_reparse_point` helper) before descending into it, and rejects a directory whose `SKILL.md` is itself such a link — otherwise a link planted under a root would be adopted as the skill root, and since every later guard treats the skill root as the boundary and only inspects segments below it, the link itself would never be inspected. Resource and script discovery apply the same rule per path segment via `_has_link_or_reparse_point_in_path`; source-discovered resources and scripts retain a `_SkillPathScope` pairing their configured root with their skill directory, and repeat containment, regular-file, and link checks immediately before reading or execution — scanning every segment from the configured root down to the file, so a skill directory (or any directory between it and the root) swapped for a link after discovery is rejected too. An `OSError` while inspecting an entry is treated as unsafe (skip / reject), never as "safe". -- **`MCPSkillsSource`** - `SkillsSource` that discovers Agent Skills served over MCP by reading the well-known `skill://index.json` (SEP-2640). Index entries are dispatched by their `type` (case-insensitive): `skill-md` entries become one `MCPSkill` each (its `SKILL.md` body and sibling resources are fetched on demand via `resources/read`), and `archive` entries are downloaded as a single ZIP / TAR / gzip-TAR blob and unpacked **entirely in memory** (via the private `_ArchiveEntryLoader`) into a `FileSkill` whose `SKILL.md` body drives it and whose sibling files (matching the resource extensions, within the search depth) become in-memory `InlineSkillResource` resources. **Nothing is written to disk** — there are no temporary directories to create, own, or prune (this is a deliberate divergence from .NET, which extracts archives to disk; it removes the temp-dir leak and the dangerous prune-of-unowned-subdirs footgun). Entries whose type has no handler (e.g. `mcp-resource-template`) are skipped. **MCP-delivered scripts are never runnable**: the loader emits no `SkillScript`s, so a bundled script can at most surface as a readable resource (and only if it matches the resource extensions — `.py` is not a default resource extension). The archive `SKILL.md` frontmatter `name` must match the advertised index-entry `name` or the skill is skipped. Extraction is hardened: a `..` path-traversal ("zip-slip") member name raises via `_normalize_archive_member_name` and aborts the whole skill (like the file-count/size limits), non-regular TAR members (links/devices) are skipped, and file-count / uncompressed-size (`_read_member_with_limit`) / download-size limits are enforced. Archive behavior is configured with `archive_*` constructor kwargs (`archive_resource_extensions`, `archive_resource_search_depth`, `archive_max_file_count`, `archive_max_size_bytes`, `archive_max_uncompressed_size_bytes`) — Python uses plain kwargs, not a `*Options` object as in .NET. A non-"resource not found" error while downloading an archive propagates (so a failed `CachingSkillsSource` refresh does not overwrite a cached list with a partial result). Unlike .NET's `AgentMcpSkillsSourceOptions.RefreshInterval`, this source has no built-in refresh interval; wrap it in `CachingSkillsSource(..., refresh_interval=...)` for caching/refresh. This is a port of .NET PR #6631; the `FileSkillsSource` `script_extensions`/`resource_extensions` kwargs default to the built-in tuples and treat `None` as "use defaults" and an empty tuple as "discover none" (an empty tuple previously fell back to defaults). `FoundryToolbox.as_skills_provider()` forwards matching `archive_*` kwargs to this source. +- **`MCPSkillsSource`** - `SkillsSource` that discovers Agent Skills served over MCP by reading the well-known `skill://index.json` (SEP-2640). Index entries are dispatched by their `type` (case-insensitive): `skill-md` entries become one `MCPSkill` each (its `SKILL.md` body and sibling resources are fetched on demand via `resources/read`), and `archive` entries are downloaded as a single ZIP blob and unpacked **entirely in memory** (via the private `_ArchiveEntryLoader`) into a `FileSkill` whose `SKILL.md` body drives it and whose sibling files (matching the resource extensions, within the search depth) become in-memory `InlineSkillResource` resources. **Nothing is written to disk** — there are no temporary directories to create, own, or prune (this is a deliberate divergence from .NET, which extracts ZIP archives to disk; it removes the temp-dir leak and the dangerous prune-of-unowned-subdirs footgun). Entries whose type has no handler (e.g. `mcp-resource-template`) are skipped. **MCP-delivered scripts are never runnable**: the loader emits no `SkillScript`s, so a bundled script can at most surface as a readable resource (and only if it matches the resource extensions — `.py` is not a default resource extension). The archive `SKILL.md` frontmatter `name` must match the advertised index-entry `name` or the skill is skipped. ZIP extraction is hardened: a `..` path-traversal ("zip-slip") member name raises via `_normalize_archive_member_name` and aborts the whole skill (like the file-count/size limits), and file-count / uncompressed-size (`_read_member_with_limit`) / download-size limits are enforced. Archive behavior is configured with `archive_*` constructor kwargs (`archive_resource_extensions`, `archive_resource_search_depth`, `archive_max_file_count`, `archive_max_size_bytes`, `archive_max_uncompressed_size_bytes`) — Python uses plain kwargs, not a `*Options` object as in .NET. A non-"resource not found" error while downloading an archive propagates (so a failed `CachingSkillsSource` refresh does not overwrite a cached list with a partial result). Unlike .NET's `AgentMcpSkillsSourceOptions.RefreshInterval`, this source has no built-in refresh interval; wrap it in `CachingSkillsSource(..., refresh_interval=...)` for caching/refresh. This is a port of .NET PR #6631; the `FileSkillsSource` `script_extensions`/`resource_extensions` kwargs default to the built-in tuples and treat `None` as "use defaults" and an empty tuple as "discover none" (an empty tuple previously fell back to defaults). `FoundryToolbox.as_skills_provider()` forwards matching `archive_*` kwargs to this source. - **`SkillsSource` decorators** - Skill sources are composable: `SkillsSource` is the abstract base, with concrete sources (`InMemorySkillsSource`, `FileSkillsSource`, `MCPSkillsSource`) and decorators that wrap an inner source — `AggregatingSkillsSource` (concatenate several sources), `FilteringSkillsSource` (predicate filter), `DeduplicatingSkillsSource` (first-wins by name), and `CachingSkillsSource` (cache the inner source's skills list). `DelegatingSkillsSource` is the abstract base for decorators. **`get_skills` takes a `SkillsSourceContext`**: every source/decorator implements `async def get_skills(self, context: SkillsSourceContext) -> list[Skill]` and forwards `context` to inner sources. `SkillsSourceContext` (frozen) carries the invoking `agent` (`SupportsAgentRun`) and optional `session` (`AgentSession | None`); `SkillsProvider` builds it from `before_run`'s `agent`/`session` and passes it into the pipeline. `FilteringSkillsSource`'s predicate is context-aware: `Callable[[Skill, SkillsSourceContext], bool]` (port of .NET #6797). **Default caching is applied only to the built-in, context-independent leaf sources**: for the `Skill` / sequence-of-skills / `from_paths` constructors, `SkillsProvider` builds `DeduplicatingSkillsSource(CachingSkillsSource())` so expensive filesystem/network discovery runs once. A **caller-supplied `SkillsSource` is used as-is — never auto-wrapped in caching or deduplication** — because auto-caching a context-aware caller source in a single shared bucket would replay the first invocation's skills for later `SkillsSourceContext`s and leak skills across agents/tenants (matches .NET, whose custom-source constructor also adds no caching/dedup). Callers who want caching on a custom pipeline compose `CachingSkillsSource(inner, cache_isolation_key_selector=...)` themselves. `disable_caching=True` only affects the built-in leaf caching (it has no effect on a caller-supplied source, which is never cached). `CachingSkillsSource` shares a single in-flight fetch across concurrent callers (per cache key) and does not update its cache on a failed fetch, so the next call retries (an initial failure leaves the cache empty; a refresh failure keeps the previously cached list). By default all callers share one cache bucket; pass `cache_isolation_key_selector=Callable[[SkillsSourceContext], str | None]` to cache separately per key (e.g. per agent name) for context-aware inner sources — the key should be low-cardinality and stable, and returning `None` (or leaving the selector `None`) uses the shared bucket. By default a cached list never expires; pass `refresh_interval=timedelta(...)` (port of .NET `CachingAgentSkillsSourceOptions.RefreshInterval`) to treat a cached list as stale once it is older than the interval so the next call re-queries the inner source (useful when an inner source such as `MCPSkillsSource` changes over the process lifetime; a zero/negative interval makes every result immediately stale, and a failed refresh keeps the prior list and retries). Freshness is measured with a monotonic clock (`time.monotonic()`). `SkillsProvider.__init__` / `from_paths` expose a `cache_refresh_interval` kwarg that is threaded into the built-in `CachingSkillsSource` (it has no effect on a caller-supplied source or when `disable_caching=True`). **`MCPSkillsSource` and `MCPSkill` accept exactly one of `client` (a fixed `ClientSession`) or `session_provider` (`Callable[[], ClientSession]`, resolved on every fetch); providing both/neither raises `ValueError`.** Use `session_provider` when the underlying session may be swapped over time — e.g. a reconnecting `MCPTool`/`FoundryToolbox` whose `session` is replaced on reconnect — so cached `MCPSkill`s keep fetching against the live session instead of a closed one (`MCPSkillsSource` forwards its provider to every `MCPSkill` it creates). A fixed `client` is safe only when the session outlives the skills. ### Model Context Protocol (`_mcp.py`) diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index ad6dd5c3255..4c14e5b07b5 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -46,14 +46,12 @@ import asyncio import base64 -import gzip import inspect import io import json import logging import os import re -import tarfile import time import zipfile from abc import ABC, abstractmethod @@ -4502,12 +4500,10 @@ def _compute_skill_root_uri(skill_md_uri: str) -> str: class _ArchiveFormat(Enum): - """The archive container formats supported by :func:`_extract_archive`.""" + """The archive container formats supported during archive skill discovery.""" UNKNOWN = "unknown" ZIP = "zip" - TAR = "tar" - TAR_GZ = "tar_gz" def _detect_archive_format(data: bytes, media_type: str | None, url: str | None) -> _ArchiveFormat: @@ -4524,8 +4520,9 @@ def _detect_archive_format(data: bytes, media_type: str | None, url: str | None) Returns: The detected :class:`_ArchiveFormat`, or :attr:`_ArchiveFormat.UNKNOWN`. """ + # Reject gzip by signature before considering potentially incorrect MIME type or URL hints. if len(data) >= 2 and data[0] == 0x1F and data[1] == 0x8B: - return _ArchiveFormat.TAR_GZ + return _ArchiveFormat.UNKNOWN if len(data) >= 4 and data[0] == 0x50 and data[1] == 0x4B and data[2] in (0x03, 0x05, 0x07): return _ArchiveFormat.ZIP @@ -4533,18 +4530,10 @@ def _detect_archive_format(data: bytes, media_type: str | None, url: str | None) media = (media_type or "").strip().lower() if media in ("application/zip", "application/x-zip-compressed"): return _ArchiveFormat.ZIP - if media in ("application/gzip", "application/x-gzip", "application/x-compressed-tar"): - return _ArchiveFormat.TAR_GZ - if media in ("application/x-tar", "application/tar"): - return _ArchiveFormat.TAR lowered = (url or "").lower() if lowered.endswith(".zip"): return _ArchiveFormat.ZIP - if lowered.endswith(".tar.gz") or lowered.endswith(".tgz"): - return _ArchiveFormat.TAR_GZ - if lowered.endswith(".tar"): - return _ArchiveFormat.TAR return _ArchiveFormat.UNKNOWN @@ -4623,13 +4612,10 @@ def _extract_archive_to_memory( ) -> dict[str, bytes]: """Extract an archive's regular files into an in-memory ``{relative-path: bytes}`` mapping. - Supports ZIP, TAR, and gzip-compressed TAR payloads. Non-regular TAR entries - (symbolic links, hard links, device nodes, etc.) are skipped so an archive cannot - smuggle in a link, and absolute member names are neutralized to relative. A member - that attempts to escape the skill namespace via a ``..`` 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. + Supports ZIP payloads. A member that attempts to escape the skill namespace via a + ``..`` 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. Args: data: The raw archive bytes. @@ -4644,20 +4630,12 @@ def _extract_archive_to_memory( ValueError: If the format is unknown, a limit is exceeded, or a member attempts a path-traversal ("zip-slip") escape. OSError: If the payload cannot be read. - tarfile.TarError: If a TAR payload is malformed. zipfile.BadZipFile: If a ZIP payload is malformed. - gzip.BadGzipFile: If a gzip payload is malformed. """ if archive_format is _ArchiveFormat.ZIP: with zipfile.ZipFile(io.BytesIO(data)) as archive: return _extract_zip_to_memory(archive, max_file_count, max_uncompressed_size_bytes) - if archive_format is _ArchiveFormat.TAR: - with tarfile.open(fileobj=io.BytesIO(data), mode="r:") as archive: - return _extract_tar_to_memory(archive, max_file_count, max_uncompressed_size_bytes) - if archive_format is _ArchiveFormat.TAR_GZ: - with tarfile.open(fileobj=io.BytesIO(data), mode="r:gz") as archive: - return _extract_tar_to_memory(archive, max_file_count, max_uncompressed_size_bytes) - raise ValueError(f"Unsupported skill archive format '{archive_format}'.") + raise ValueError(f"Unsupported skill archive format '{archive_format}'. Use ZIP instead.") def _extract_zip_to_memory( @@ -4689,46 +4667,10 @@ def _extract_zip_to_memory( return files -def _extract_tar_to_memory( - archive: tarfile.TarFile, - max_file_count: int, - max_uncompressed_size_bytes: int, -) -> dict[str, bytes]: - """Read regular files from a TAR archive into memory. See :func:`_extract_archive_to_memory`.""" - remaining_bytes = max_uncompressed_size_bytes - files: dict[str, bytes] = {} - file_count = 0 - - for member in archive: - # Only regular files are materialized. Skipping links/devices avoids both - # unsupported entry types and link-based escapes outside the skill namespace. - if not member.isreg(): - continue - - file_count += 1 - if file_count > max_file_count: - raise ValueError(f"Skill archive exceeds the maximum allowed file count ({max_file_count}).") - - name = _normalize_archive_member_name(member.name) - if name is None: - continue - - source = archive.extractfile(member) - if source is None: - continue - - with source: - content, remaining_bytes = _read_member_with_limit(source, remaining_bytes) - files[name] = content - - return files - - class _ArchiveEntryLoader: """Loads ``archive``-type ``skill://index.json`` entries entirely in memory. - Each entry's ``url`` points to a single archive resource (ZIP, TAR, or - gzip-compressed TAR). The archive is downloaded and unpacked **in memory** into a + Each entry's ``url`` points to a ZIP archive resource, which is unpacked **in memory** into a :class:`FileSkill` whose ``SKILL.md`` body drives the skill and whose sibling files (matching the configured resource extensions, within the configured depth) become in-memory :class:`InlineSkillResource` resources. Nothing is written to disk, so @@ -4738,9 +4680,8 @@ class _ArchiveEntryLoader: resource extensions become readable resources; a script file is at most a readable resource, never a :class:`SkillScript`. - Extraction is hardened against path-traversal ("zip-slip") member names, non-regular - TAR members (links/devices), oversized downloads, excessive file counts, and - decompression bombs. + Extraction is hardened against path-traversal ("zip-slip") member names, + oversized downloads, excessive file counts, and decompression bombs. """ def __init__( @@ -4871,7 +4812,7 @@ def _build_skill(self, entry: _McpSkillIndexEntry, data: bytes, mime_type: str | files = _extract_archive_to_memory( data, archive_format, self._max_file_count, self._max_uncompressed_size_bytes ) - except (OSError, ValueError, EOFError, tarfile.TarError, zipfile.BadZipFile, gzip.BadGzipFile): + except (OSError, ValueError, EOFError, zipfile.BadZipFile): logger.warning("Failed to extract archive for skill '%s'.", entry.name, exc_info=True) return None @@ -4984,8 +4925,8 @@ class MCPSkillsSource(SkillsSource): ``name``, ``description``, and ``url`` fields. The referenced ``SKILL.md`` resource is **not** read during discovery; the host fetches its body on demand via ``resources/read`` when the skill content is needed. - * ``archive`` — the entry's ``url`` points to a single archive resource - (ZIP, TAR, or gzip-compressed TAR) whose content unpacks into the skill's + * ``archive`` — the entry's ``url`` points to a ZIP archive resource + whose content unpacks into the skill's namespace. The archive is downloaded and unpacked **in memory** into a skill whose ``SKILL.md`` body drives it and whose sibling files become in-memory resources; nothing is written to disk. Scripts bundled inside an @@ -5015,8 +4956,8 @@ class MCPSkillsSource(SkillsSource): script-capable skills, executed. Only connect this source to MCP servers you have vetted and trust, and treat their responses as untrusted input. Archive extraction is hardened against path-traversal - ("zip-slip"), link-based escapes, and decompression bombs, but the - skill *content* is still untrusted. + ("zip-slip") and decompression bombs, but the skill *content* is still + untrusted. Examples: .. code-block:: python diff --git a/python/packages/core/tests/core/test_mcp_skills.py b/python/packages/core/tests/core/test_mcp_skills.py index a20551540fb..647981f0da4 100644 --- a/python/packages/core/tests/core/test_mcp_skills.py +++ b/python/packages/core/tests/core/test_mcp_skills.py @@ -7,7 +7,6 @@ import base64 import io import json -import tarfile import zipfile from unittest.mock import AsyncMock @@ -454,7 +453,6 @@ async def test_missing_required_fields_is_skipped(self) -> None: skills = await source.get_skills(_SOURCE_CTX) assert skills == [] - @pytest.mark.asyncio async def test_archive_missing_resource_is_skipped(self) -> None: # An archive entry whose archive resource is not available on the server # is skipped (the index is read, but the archive download fails). @@ -714,25 +712,6 @@ def _make_zip(files: dict[str, bytes]) -> bytes: return buffer.getvalue() -def _make_tar(files: dict[str, bytes], *, gzipped: bool) -> bytes: - """Build an in-memory TAR (optionally gzip-compressed) archive.""" - buffer = io.BytesIO() - - def _write(archive: tarfile.TarFile) -> None: - for name, data in files.items(): - info = tarfile.TarInfo(name=name) - info.size = len(data) - archive.addfile(info, io.BytesIO(data)) - - if gzipped: - with tarfile.open(fileobj=buffer, mode="w:gz") as archive: - _write(archive) - else: - with tarfile.open(fileobj=buffer, mode="w:") as archive: - _write(archive) - return buffer.getvalue() - - ARCHIVE_SKILL_MD = """\ --- name: packaged-skill @@ -775,7 +754,6 @@ def _archive_client(index_json: str, archive_url: str, archive_bytes: bytes, mim class TestMCPSkillsSourceArchive: """Tests for archive-type skill discovery via MCPSkillsSource (in-memory).""" - @pytest.mark.asyncio async def test_zip_archive_discovered_as_file_skill(self) -> None: from agent_framework import FileSkill @@ -794,33 +772,28 @@ async def test_zip_archive_discovered_as_file_skill(self) -> None: content = await skill.get_content() assert "Instructions from an archive." in content - @pytest.mark.asyncio - async def test_targz_archive_discovered(self) -> None: + async def test_targz_archive_is_rejected(self) -> None: url = "skill://archives/packaged-skill.tar.gz" index = _make_archive_index("packaged-skill", url) - archive = _make_tar({"SKILL.md": ARCHIVE_SKILL_MD.encode()}, gzipped=True) + archive = b"\x1f\x8b" client = _archive_client(index, url, archive, "application/gzip") source = MCPSkillsSource(client=client) skills = await source.get_skills(_SOURCE_CTX) - assert len(skills) == 1 - assert skills[0].frontmatter.name == "packaged-skill" + assert skills == [] - @pytest.mark.asyncio - async def test_tar_archive_discovered(self) -> None: + async def test_tar_archive_is_rejected(self) -> None: url = "skill://archives/packaged-skill.tar" index = _make_archive_index("packaged-skill", url) - archive = _make_tar({"SKILL.md": ARCHIVE_SKILL_MD.encode()}, gzipped=False) + archive = b"tar" client = _archive_client(index, url, archive, "application/x-tar") source = MCPSkillsSource(client=client) skills = await source.get_skills(_SOURCE_CTX) - assert len(skills) == 1 - assert skills[0].frontmatter.name == "packaged-skill" + assert skills == [] - @pytest.mark.asyncio async def test_archive_reference_resource_is_readable(self) -> None: # A bundled reference file is served as an in-memory resource, read on demand. url = "skill://archives/packaged-skill.zip" @@ -838,7 +811,6 @@ async def test_archive_reference_resource_is_readable(self) -> None: assert resource is not None assert "REF-CANARY-9001" in await resource.read() - @pytest.mark.asyncio async def test_wrapped_archive_root_is_discovered(self) -> None: # An archive whose SKILL.md sits under a top-level folder is still discovered, # and resources are resolved relative to the SKILL.md's directory. @@ -858,7 +830,6 @@ async def test_wrapped_archive_root_is_discovered(self) -> None: assert resource is not None assert "REF-CANARY-42" in await resource.read() - @pytest.mark.asyncio async def test_bundled_script_is_never_runnable(self) -> None: # An archive that bundles a .py script must not expose it as a runnable script, # nor (with default resource extensions) as a resource. @@ -878,7 +849,6 @@ async def test_bundled_script_is_never_runnable(self) -> None: content = await skill.get_content() assert "" in content - @pytest.mark.asyncio async def test_oversized_archive_download_is_skipped(self) -> None: url = "skill://archives/packaged-skill.zip" index = _make_archive_index("packaged-skill", url) @@ -889,7 +859,6 @@ async def test_oversized_archive_download_is_skipped(self) -> None: skills = await source.get_skills(_SOURCE_CTX) assert skills == [] - @pytest.mark.asyncio async def test_archive_exceeding_file_count_is_skipped(self) -> None: url = "skill://archives/packaged-skill.zip" index = _make_archive_index("packaged-skill", url) @@ -904,7 +873,6 @@ async def test_archive_exceeding_file_count_is_skipped(self) -> None: skills = await source.get_skills(_SOURCE_CTX) assert skills == [] - @pytest.mark.asyncio async def test_frontmatter_name_mismatch_is_skipped(self) -> None: # The SKILL.md frontmatter name must match the advertised entry name. url = "skill://archives/packaged-skill.zip" @@ -934,7 +902,6 @@ async def test_frontmatter_name_mismatch_is_logged_as_warning(self, caplog: pyte for record in caplog.records ) - @pytest.mark.asyncio async def test_archive_without_skill_md_is_skipped(self) -> None: url = "skill://archives/packaged-skill.zip" index = _make_archive_index("packaged-skill", url) @@ -945,7 +912,6 @@ async def test_archive_without_skill_md_is_skipped(self) -> None: skills = await source.get_skills(_SOURCE_CTX) assert skills == [] - @pytest.mark.asyncio async def test_unsupported_archive_format_is_skipped(self) -> None: url = "skill://archives/packaged-skill.bin" index = _make_archive_index("packaged-skill", url) @@ -955,7 +921,6 @@ async def test_unsupported_archive_format_is_skipped(self) -> None: skills = await source.get_skills(_SOURCE_CTX) assert skills == [] - @pytest.mark.asyncio async def test_archive_download_internal_error_propagates(self) -> None: # A non-"not found" MCP error while downloading an archive must propagate, # not silently drop the skill (which would corrupt a CachingSkillsSource refresh). @@ -975,7 +940,6 @@ async def _read_resource(uri: AnyUrl) -> ReadResourceResult: with pytest.raises(McpError): await source.get_skills(_SOURCE_CTX) - @pytest.mark.asyncio async def test_archive_download_connection_error_propagates(self) -> None: # A plain ConnectionError while downloading an archive must propagate. url = "skill://archives/packaged-skill.zip" @@ -994,7 +958,6 @@ async def _read_resource(uri: AnyUrl) -> ReadResourceResult: with pytest.raises(ConnectionError): await source.get_skills(_SOURCE_CTX) - @pytest.mark.asyncio async def test_mixed_skill_md_and_archive_entries(self) -> None: archive_url = "skill://archives/packaged-skill.zip" index = json.dumps({ @@ -1027,7 +990,6 @@ async def test_mixed_skill_md_and_archive_entries(self) -> None: names = sorted(s.frontmatter.name for s in skills) assert names == ["packaged-skill", "unit-converter"] - @pytest.mark.asyncio async def test_zip_slip_archive_skips_whole_skill(self) -> None: # An archive with a path-traversal member is treated as hostile: the whole # skill is dropped (extraction raises, and _build_skill skips it). @@ -1055,22 +1017,22 @@ class TestArchiveExtractor: def test_detect_format_from_magic_bytes(self) -> None: from agent_framework._skills import _ArchiveFormat, _detect_archive_format - assert _detect_archive_format(b"\x1f\x8b\x08\x00", None, None) is _ArchiveFormat.TAR_GZ + assert _detect_archive_format(b"\x1f\x8b\x08\x00", None, None) is _ArchiveFormat.UNKNOWN assert _detect_archive_format(b"PK\x03\x04rest", None, None) is _ArchiveFormat.ZIP def test_detect_format_from_media_type(self) -> None: from agent_framework._skills import _ArchiveFormat, _detect_archive_format assert _detect_archive_format(b"xx", "application/zip", None) is _ArchiveFormat.ZIP - assert _detect_archive_format(b"xx", "application/x-tar", None) is _ArchiveFormat.TAR - assert _detect_archive_format(b"xx", "application/gzip", None) is _ArchiveFormat.TAR_GZ + assert _detect_archive_format(b"xx", "application/x-tar", None) is _ArchiveFormat.UNKNOWN + assert _detect_archive_format(b"xx", "application/gzip", None) is _ArchiveFormat.UNKNOWN def test_detect_format_from_url_suffix(self) -> None: from agent_framework._skills import _ArchiveFormat, _detect_archive_format assert _detect_archive_format(b"xx", None, "skill://a.zip") is _ArchiveFormat.ZIP - assert _detect_archive_format(b"xx", None, "skill://a.tgz") is _ArchiveFormat.TAR_GZ - assert _detect_archive_format(b"xx", None, "skill://a.tar") is _ArchiveFormat.TAR + assert _detect_archive_format(b"xx", None, "skill://a.tgz") is _ArchiveFormat.UNKNOWN + assert _detect_archive_format(b"xx", None, "skill://a.tar") is _ArchiveFormat.UNKNOWN def test_detect_format_unknown(self) -> None: from agent_framework._skills import _ArchiveFormat, _detect_archive_format @@ -1127,21 +1089,8 @@ def test_uncompressed_size_limit_is_enforced(self) -> None: with pytest.raises(ValueError, match="uncompressed size"): _extract_archive_to_memory(archive, _ArchiveFormat.ZIP, 20, 10) - def test_tar_symlink_member_is_skipped(self) -> None: + def test_unknown_format_is_rejected(self) -> None: from agent_framework._skills import _ArchiveFormat, _extract_archive_to_memory - buffer = io.BytesIO() - with tarfile.open(fileobj=buffer, mode="w:") as archive: - link = tarfile.TarInfo(name="link") - link.type = tarfile.SYMTYPE - link.linkname = "/etc/passwd" - archive.addfile(link) - data = b"regular" - reg = tarfile.TarInfo(name="regular.md") - reg.size = len(data) - archive.addfile(reg, io.BytesIO(data)) - - files = _extract_archive_to_memory(buffer.getvalue(), _ArchiveFormat.TAR, 20, 1024 * 1024) - - assert "link" not in files - assert files == {"regular.md": b"regular"} + with pytest.raises(ValueError, match="Unsupported skill archive format"): + _extract_archive_to_memory(b"", _ArchiveFormat.UNKNOWN, 20, 1024 * 1024) diff --git a/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py b/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py index f0df9f92ad6..0696e70d8c1 100644 --- a/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py +++ b/python/packages/foundry_hosting/agent_framework_foundry_hosting/_toolbox.py @@ -288,7 +288,7 @@ def as_skills_provider( the agent via ``tools=`` -- set ``load_tools=False`` if you want skills only and no tools -- or by entering it as an ``async with`` context manager. - Skills served as ``archive`` entries (a packaged ZIP / TAR) are downloaded and + Skills served as ``archive`` entries (a packaged ZIP) are downloaded and unpacked **in memory** and served like file-based skills; nothing is written to disk. The ``archive_*`` keyword arguments configure that behavior; see :class:`~agent_framework.MCPSkillsSource` for their full semantics. Any left