From 693480083d8e837b7f97efe0f5963dfb3bbece5d Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Tue, 4 Aug 2026 14:54:37 +0000 Subject: [PATCH 1/2] Add windows junction detection for skills --- .../core/agent_framework/_filesystem.py | 23 ++++++++ .../agent_framework/_harness/_file_access.py | 23 ++------ .../packages/core/agent_framework/_skills.py | 40 ++++++++----- python/packages/core/tests/core/conftest.py | 28 ++++++++- .../tests/core/test_harness_file_access.py | 46 +++++++-------- .../packages/core/tests/core/test_skills.py | 59 +++++++++++++++---- 6 files changed, 147 insertions(+), 72 deletions(-) create mode 100644 python/packages/core/agent_framework/_filesystem.py diff --git a/python/packages/core/agent_framework/_filesystem.py b/python/packages/core/agent_framework/_filesystem.py new file mode 100644 index 00000000000..ac907621da5 --- /dev/null +++ b/python/packages/core/agent_framework/_filesystem.py @@ -0,0 +1,23 @@ +# Copyright (c) Microsoft. All rights reserved. + +"""Private filesystem security helpers.""" + +from __future__ import annotations + +import stat +from pathlib import Path + + +def is_link_or_reparse_point(path: Path) -> bool: + """Return whether ``path`` is a symbolic link, junction, or other reparse point.""" + path_stat = path.lstat() + if stat.S_ISLNK(path_stat.st_mode): + return True + + is_junction = getattr(path, "is_junction", None) + if callable(is_junction) and is_junction(): + return True + + reparse_attribute = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) + file_attributes = getattr(path_stat, "st_file_attributes", 0) + return bool(reparse_attribute and file_attributes & reparse_attribute) diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index b70d78081ed..2a5fad18ce7 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -27,7 +27,6 @@ import logging import os import re -import stat from abc import ABC, abstractmethod from collections.abc import Callable, Mapping, MutableMapping from pathlib import Path @@ -36,6 +35,7 @@ from pydantic import BaseModel, Field from .._feature_stage import ExperimentalFeature, experimental +from .._filesystem import is_link_or_reparse_point from .._serialization import SerializationMixin from .._sessions import AgentSession, ContextProvider, SessionContext from .._telemetry import FeatureIndex, mark_feature_used @@ -83,21 +83,6 @@ _ELOOP = errno.ELOOP -def _is_link_or_reparse_point(path: Path) -> bool: - """Return whether ``path`` is a symbolic link, junction, or other reparse point.""" - path_stat = path.lstat() - if stat.S_ISLNK(path_stat.st_mode): - return True - - is_junction = getattr(path, "is_junction", None) - if callable(is_junction) and is_junction(): - return True - - reparse_attribute = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) - file_attributes = getattr(path_stat, "st_file_attributes", 0) - return bool(reparse_attribute and file_attributes & reparse_attribute) - - def _compile_search_regex(pattern: str) -> re.Pattern[str]: """Compile a case-insensitive search regex, enforcing the length cap. @@ -889,7 +874,7 @@ def _throw_if_contains_symlink(self, candidate: Path) -> None: for segment in relative_parts: current = current / segment try: - is_link = _is_link_or_reparse_point(current) + is_link = is_link_or_reparse_point(current) except FileNotFoundError: break except OSError as exc: @@ -1003,7 +988,7 @@ def _list_sync(full_dir: Path) -> list[FileStoreEntry]: files: list[FileStoreEntry] = [] for entry in full_dir.iterdir(): try: - is_link = _is_link_or_reparse_point(entry) + is_link = is_link_or_reparse_point(entry) except OSError: # Fail closed when an entry cannot be inspected. continue @@ -1062,7 +1047,7 @@ def _enumerate_search_files(full_dir: Path, recursive: bool) -> list[tuple[str, current = directories.pop() for entry in current.iterdir(): try: - is_link = _is_link_or_reparse_point(entry) + is_link = is_link_or_reparse_point(entry) except OSError: # Fail closed when an entry cannot be inspected. continue diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index e749510d948..bea18061d72 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -66,6 +66,7 @@ from typing import IO, TYPE_CHECKING, Any, ClassVar, Final, Protocol, TypeAlias, TypeVar, cast, runtime_checkable from ._feature_stage import ExperimentalFeature, experimental +from ._filesystem import is_link_or_reparse_point from ._sessions import ContextProvider from ._telemetry import FeatureIndex, mark_feature_used from ._tools import ApprovalMode, FunctionTool @@ -2989,8 +2990,8 @@ def _is_path_within_directory(path: str, directory: str) -> bool: return False @staticmethod - def _has_symlink_in_path(path: str, directory: str) -> bool: - """Detect symlinks in the portion of *path* below *directory*. + def _has_link_or_reparse_point_in_path(path: str, directory: str) -> bool: + """Detect links or reparse points in the portion of *path* below *directory*. Only segments below *directory* are inspected; the directory itself and anything above it are not checked. @@ -3003,7 +3004,8 @@ def _has_symlink_in_path(path: str, directory: str) -> bool: directory: Root directory; segments above it are not checked. Returns: - ``True`` if any intermediate segment below *directory* is a symlink. + ``True`` if any segment below *directory* is a symbolic link, + junction, other reparse point, or cannot be safely inspected. Raises: ValueError: If *path* is not relative to *directory*. @@ -3017,7 +3019,11 @@ def _has_symlink_in_path(path: str, directory: str) -> bool: current = dir_path for part in relative.parts: current = current / part - if current.is_symlink(): + try: + is_link = is_link_or_reparse_point(current) + except OSError: + return True + if is_link: return True return False @@ -3098,9 +3104,10 @@ def _scan_directory_for_resources( ) return - if FileSkillsSource._has_symlink_in_path(resolved_target, root_directory_path): + if FileSkillsSource._has_link_or_reparse_point_in_path(resolved_target, root_directory_path): logger.warning( - "Skipping resource directory '%s': symlink detected in path under skill directory '%s'", + "Skipping resource directory '%s': symbolic link or reparse point detected in path under " + "skill directory '%s'", target_dir, root_directory_path, ) @@ -3143,9 +3150,10 @@ def _scan_directory_for_resources( ) continue - if FileSkillsSource._has_symlink_in_path(resource_full_path, root_directory_path): + if FileSkillsSource._has_link_or_reparse_point_in_path(resource_full_path, root_directory_path): logger.warning( - "Skipping resource '%s': symlink detected in path under skill directory '%s'", + "Skipping resource '%s': symbolic link or reparse point detected in path under " + "skill directory '%s'", entry, root_directory_path, ) @@ -3251,9 +3259,10 @@ def _scan_directory_for_scripts( ) return - if FileSkillsSource._has_symlink_in_path(resolved_target, root_directory_path): + if FileSkillsSource._has_link_or_reparse_point_in_path(resolved_target, root_directory_path): logger.warning( - "Skipping script directory '%s': symlink detected in path under skill directory '%s'", + "Skipping script directory '%s': symbolic link or reparse point detected in path under " + "skill directory '%s'", target_dir, root_directory_path, ) @@ -3293,9 +3302,9 @@ def _scan_directory_for_scripts( ) continue - if FileSkillsSource._has_symlink_in_path(script_full_path, root_directory_path): + if FileSkillsSource._has_link_or_reparse_point_in_path(script_full_path, root_directory_path): logger.warning( - "Skipping script '%s': symlink detected in path under skill directory '%s'", + "Skipping script '%s': symbolic link or reparse point detected in path under skill directory '%s'", entry, root_directory_path, ) @@ -3359,8 +3368,11 @@ def _get_validated_resource_path(skill_dir: str, resource_name: str) -> str: if not Path(resource_full_path).is_file(): raise ValueError(f"Resource file '{resource_name}' not found in skill directory '{skill_dir}'.") - if FileSkillsSource._has_symlink_in_path(resource_full_path, root_directory_path): - raise ValueError(f"Resource file '{resource_name}' has a symlink in its path; symlinks are not allowed.") + if FileSkillsSource._has_link_or_reparse_point_in_path(resource_full_path, root_directory_path): + raise ValueError( + f"Resource file '{resource_name}' has a symbolic link or reparse point in its path; " + "links and reparse points are not allowed." + ) return resource_full_path diff --git a/python/packages/core/tests/core/conftest.py b/python/packages/core/tests/core/conftest.py index d67b9ef223c..70361935082 100644 --- a/python/packages/core/tests/core/conftest.py +++ b/python/packages/core/tests/core/conftest.py @@ -2,15 +2,19 @@ import asyncio import logging +import os +import stat +import subprocess import sys import warnings from collections.abc import AsyncIterable, Awaitable, MutableSequence, Sequence +from pathlib import Path from typing import Any, Generic from typing import TypedDict as TypedDict # noqa: F401 # pydantic mypy plugin needs TypedDict in module scope from unittest.mock import patch from uuid import uuid4 -from pytest import fixture +from pytest import fixture, skip warnings.filterwarnings( "ignore", @@ -46,6 +50,28 @@ logger = logging.getLogger(__name__) +def create_junction_or_skip(*, link: Path, target: Path) -> None: + """Create a Windows directory junction or skip when the environment cannot.""" + if sys.platform != "win32": + skip("Windows directory junctions are only available on Windows") + + result = subprocess.run( + [os.environ.get("COMSPEC", "cmd"), "/c", "mklink", "/J", str(link), str(target)], + capture_output=True, + text=True, + check=False, + ) + if result.returncode != 0: + skip(f"Could not create Windows directory junction: {result.stderr or result.stdout}") + + is_junction = getattr(link, "is_junction", None) + reparse_attribute = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) + is_reparse_point = bool(reparse_attribute and getattr(link.lstat(), "st_file_attributes", 0) & reparse_attribute) + if not (callable(is_junction) and is_junction()) and not is_reparse_point: + link.rmdir() + skip("Created junction was not reported as a reparse point") + + @fixture(scope="function") def chat_history() -> list[Message]: return [] diff --git a/python/packages/core/tests/core/test_harness_file_access.py b/python/packages/core/tests/core/test_harness_file_access.py index b966e5366d8..19497be97d3 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -6,10 +6,9 @@ import os import re import stat -import subprocess -import sys import time from pathlib import Path +from types import SimpleNamespace import pytest @@ -29,6 +28,7 @@ Message, SupportsChatGetResponse, ) +from agent_framework._filesystem import is_link_or_reparse_point from agent_framework._harness import _file_access as _file_access_module from agent_framework._harness._file_access import ( DEFAULT_FILE_ACCESS_INSTRUCTIONS, @@ -38,6 +38,8 @@ _run_search_with_timeout, ) +from .conftest import create_junction_or_skip + async def _list_files(store: AgentFileStore, directory: str = "") -> list[str]: """Return only the file names from a combined ``store.list_children`` call.""" @@ -62,29 +64,6 @@ def _text(content: Content) -> str: return content.text -def _create_junction_or_skip(*, link: Path, target: Path) -> None: - if sys.platform != "win32": - pytest.skip("Windows directory junctions are only available on Windows") - - result = subprocess.run( - [os.environ.get("COMSPEC", "cmd"), "/c", "mklink", "/J", str(link), str(target)], - capture_output=True, - text=True, - check=False, - ) - if result.returncode != 0: - pytest.skip(f"Could not create Windows directory junction: {result.stderr or result.stdout}") - - is_junction = getattr(link, "is_junction", None) - is_reparse_point = bool( - getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) - and getattr(link.lstat(), "st_file_attributes", 0) & getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) - ) - if not (callable(is_junction) and is_junction()) and not is_reparse_point: - link.rmdir() - pytest.skip("Created junction was not reported as a reparse point") - - def test_normalize_relative_path_collapses_and_validates() -> None: """The path normalizer should accept relative forward/backslash paths and reject unsafe ones.""" assert _normalize_relative_path("foo/bar.txt") == "foo/bar.txt" @@ -457,7 +436,7 @@ async def test_filesystem_store_search_and_list_skip_junctioned_directories(tmp_ root.mkdir() (root / "inside.md").write_text("ERROR inside", encoding="utf-8") junction = root / "linked" - _create_junction_or_skip(link=junction, target=outside) + create_junction_or_skip(link=junction, target=outside) try: store = FileSystemAgentFileStore(root) @@ -843,6 +822,21 @@ def fail_for_target(self: Path) -> os.stat_result: await store.read("same/same/ok.txt") +def test_link_probe_detects_windows_reparse_attribute(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """The Python 3.10/3.11 Windows fallback should detect the reparse-point file attribute.""" + path = tmp_path / "entry" + path.write_text("content", encoding="utf-8") + reparse_attribute = getattr(stat, "FILE_ATTRIBUTE_REPARSE_POINT", 0) + assert reparse_attribute + + def fake_lstat(self: Path) -> SimpleNamespace: + return SimpleNamespace(st_mode=stat.S_IFREG, st_file_attributes=reparse_attribute) + + monkeypatch.setattr(Path, "lstat", fake_lstat) + + assert is_link_or_reparse_point(path) is True + + def test_file_access_harness_classes_are_marked_experimental() -> None: """File-access harness public classes should expose HARNESS experimental metadata.""" assert getattr(AgentFileStore, "__feature_id__", None) == ExperimentalFeature.HARNESS.value diff --git a/python/packages/core/tests/core/test_skills.py b/python/packages/core/tests/core/test_skills.py index 48c5cb548cb..8308ea54439 100644 --- a/python/packages/core/tests/core/test_skills.py +++ b/python/packages/core/tests/core/test_skills.py @@ -47,7 +47,7 @@ _FileSkillResource, ) -from .conftest import MockAgent, MockAgentSession +from .conftest import MockAgent, MockAgentSession, create_junction_or_skip # Cross-platform absolute path prefix for tests _ABS = "C:\\skills" if os.name == "nt" else "/skills" @@ -885,7 +885,7 @@ async def test_xml_escaping_in_prompt(self, tmp_path: Path) -> None: # --------------------------------------------------------------------------- -# Tests: symlink detection (_has_symlink_in_path and end-to-end guards) +# Tests: link detection (_has_link_or_reparse_point_in_path and end-to-end guards) # --------------------------------------------------------------------------- @@ -898,7 +898,7 @@ def _requires_symlinks(tmp_path: Path) -> None: @pytest.mark.usefixtures("_requires_symlinks") class TestSymlinkDetection: - """Tests for _has_symlink_in_path and the symlink guards in validation/read.""" + """Tests for link detection and the guards in validation/read.""" def test_detects_symlinked_file(self, tmp_path: Path) -> None: """A symlink to a file outside the directory should be detected.""" @@ -913,7 +913,7 @@ def test_detects_symlinked_file(self, tmp_path: Path) -> None: full_path = str(symlink_path) directory_path = str(skill_dir) + os.sep - assert FileSkillsSource._has_symlink_in_path(full_path, directory_path) is True + assert FileSkillsSource._has_link_or_reparse_point_in_path(full_path, directory_path) is True def test_detects_symlinked_directory(self, tmp_path: Path) -> None: """A symlink to a directory outside should be detected for paths through it.""" @@ -929,7 +929,7 @@ def test_detects_symlinked_directory(self, tmp_path: Path) -> None: full_path = str(skill_dir / "linked-dir" / "data.txt") directory_path = str(skill_dir) + os.sep - assert FileSkillsSource._has_symlink_in_path(full_path, directory_path) is True + assert FileSkillsSource._has_link_or_reparse_point_in_path(full_path, directory_path) is True def test_returns_false_for_regular_files(self, tmp_path: Path) -> None: """Regular (non-symlinked) files should not be flagged.""" @@ -941,7 +941,7 @@ def test_returns_false_for_regular_files(self, tmp_path: Path) -> None: full_path = str(regular_file) directory_path = str(skill_dir) + os.sep - assert FileSkillsSource._has_symlink_in_path(full_path, directory_path) is False + assert FileSkillsSource._has_link_or_reparse_point_in_path(full_path, directory_path) is False async def test_discover_skips_symlinked_resource(self, tmp_path: Path) -> None: """get_skills() should skip a symlinked resource but keep the skill.""" @@ -1004,6 +1004,30 @@ def test_discover_skips_symlinked_script(self, tmp_path: Path) -> None: assert "scripts/leak.py" not in discovered +class TestJunctionDetection: + """Tests for Windows junction guards in file-based skills.""" + + def test_junction_is_detected_and_excluded(self, tmp_path: Path) -> None: + skill_dir = _write_skill(tmp_path, "my-skill") + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + (outside_dir / "leak.md").write_text("secret", encoding="utf-8") + (outside_dir / "leak.py").write_text("print('secret')", encoding="utf-8") + + junction = skill_dir / "linked" + create_junction_or_skip(link=junction, target=outside_dir) + + try: + linked_resource = str(junction / "leak.md") + assert FileSkillsSource._has_link_or_reparse_point_in_path(linked_resource, str(skill_dir)) is True + assert "linked/leak.md" not in _discover_resources(str(skill_dir)) + assert "linked/leak.py" not in _discover_scripts(str(skill_dir)) + with pytest.raises(ValueError, match="symbolic link or reparse point"): + FileSkillsSource._get_validated_resource_path(str(skill_dir), "linked/leak.md") + finally: + junction.rmdir() + + # --------------------------------------------------------------------------- # Tests: SkillResource # --------------------------------------------------------------------------- @@ -1870,21 +1894,32 @@ def test_similar_prefix_not_matched(self, tmp_path: Path) -> None: # --------------------------------------------------------------------------- -# Tests: _has_symlink_in_path edge cases +# Tests: _has_link_or_reparse_point_in_path edge cases # --------------------------------------------------------------------------- -class TestHasSymlinkInPathEdgeCases: - """Edge-case tests for _has_symlink_in_path.""" +class TestHasLinkOrReparsePointInPathEdgeCases: + """Edge-case tests for _has_link_or_reparse_point_in_path.""" def test_raises_when_path_not_relative(self, tmp_path: Path) -> None: unrelated = str(tmp_path.parent / "other" / "file.txt") with pytest.raises(ValueError, match="does not start with directory"): - FileSkillsSource._has_symlink_in_path(unrelated, str(tmp_path)) + FileSkillsSource._has_link_or_reparse_point_in_path(unrelated, str(tmp_path)) def test_returns_false_for_empty_relative(self, tmp_path: Path) -> None: """When path equals directory, relative is empty so no symlinks.""" - assert FileSkillsSource._has_symlink_in_path(str(tmp_path), str(tmp_path)) is False + assert FileSkillsSource._has_link_or_reparse_point_in_path(str(tmp_path), str(tmp_path)) is False + + def test_fails_closed_when_path_cannot_be_inspected(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + target = tmp_path / "resource.md" + target.write_text("content", encoding="utf-8") + + def fail_probe(path: Path) -> bool: + raise PermissionError(path) + + monkeypatch.setattr("agent_framework._skills.is_link_or_reparse_point", fail_probe) + + assert FileSkillsSource._has_link_or_reparse_point_in_path(str(target), str(tmp_path)) is True # --------------------------------------------------------------------------- @@ -2166,7 +2201,7 @@ def test_rejects_symlink_in_path(self, tmp_path: Path) -> None: (real_subdir / "data.md").write_text("external data") link = skill_dir / "linked" link.symlink_to(real_subdir) - with pytest.raises(ValueError, match="symlink"): + with pytest.raises(ValueError, match="symbolic link or reparse point"): FileSkillsSource._get_validated_resource_path(str(skill_dir), "linked/data.md") From ee7da3a6d5e742cf364e5038dcecae0f613e82cc Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Wed, 5 Aug 2026 18:06:20 +0000 Subject: [PATCH 2/2] Address PR comment --- python/packages/core/AGENTS.md | 1 + .../packages/core/agent_framework/_skills.py | 33 +++++- .../packages/core/tests/core/test_skills.py | 109 +++++++++++++++++- 3 files changed, 141 insertions(+), 2 deletions(-) diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 8e633a9334d..11005268543 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -95,6 +95,7 @@ agent_framework/ - **`SkillScriptRunner`** - Protocol for file-based script execution. Any callable matching `(skill, script, args) -> Any` satisfies it. Code-defined scripts do not use a runner. - **`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`. 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. - **`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. diff --git a/python/packages/core/agent_framework/_skills.py b/python/packages/core/agent_framework/_skills.py index bea18061d72..d1fb1fb397a 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -3548,6 +3548,15 @@ def _discover_skill_directories(skill_paths: Sequence[str]) -> list[str]: search does not descend into its subdirectories: everything beneath a skill boundary is part of that skill, not an independent skill root. + Discovery fails closed on links: any entry below a configured root that is a + symbolic link, junction, other reparse point, or cannot be inspected is skipped + and never adopted as a skill root, and a directory whose ``SKILL.md`` is itself + such a link is skipped too. Without this check a link below a root would become + the skill root, and because every later guard treats the skill root as the trust + boundary and only inspects segments below it, the link itself would never be + inspected. The configured root paths are not checked: the host chose them + explicitly, so they define the trust boundary rather than sit inside it. + Args: skill_paths: Root directory paths to search. @@ -3556,11 +3565,26 @@ def _discover_skill_directories(skill_paths: Sequence[str]) -> list[str]: """ discovered: list[str] = [] + def _is_unsafe_link(path: Path) -> bool: + try: + return is_link_or_reparse_point(path) + except OSError: + return True + def _search(directory: str, current_depth: int) -> None: dir_path = Path(directory) - if (dir_path / SKILL_FILE_NAME).is_file(): + skill_file = dir_path / SKILL_FILE_NAME + if skill_file.is_file(): # This directory is a skill root. Subdirectories are part of this # skill and must not be treated as independent skill roots. + if _is_unsafe_link(skill_file): + logger.warning( + "Skipping skill directory '%s': '%s' is a symbolic link or reparse point, " + "or could not be inspected", + directory, + SKILL_FILE_NAME, + ) + return discovered.append(str(dir_path.absolute())) return @@ -3573,6 +3597,13 @@ def _search(directory: str, current_depth: int) -> None: return for entry in entries: + if _is_unsafe_link(entry): + logger.warning( + "Skipping discovery entry '%s': symbolic link or reparse point detected, " + "or the entry could not be inspected", + entry, + ) + continue if entry.is_dir(): _search(str(entry), current_depth + 1) diff --git a/python/packages/core/tests/core/test_skills.py b/python/packages/core/tests/core/test_skills.py index 8308ea54439..fab0a95dafc 100644 --- a/python/packages/core/tests/core/test_skills.py +++ b/python/packages/core/tests/core/test_skills.py @@ -10,7 +10,7 @@ from datetime import timedelta from pathlib import Path from typing import Any -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, patch import pytest @@ -1003,6 +1003,65 @@ def test_discover_skips_symlinked_script(self, tmp_path: Path) -> None: assert "scripts/safe.py" in discovered assert "scripts/leak.py" not in discovered + async def test_discover_skips_symlinked_skill_directory(self, tmp_path: Path) -> None: + """A symlinked directory below a configured root must not become a skill root.""" + root = tmp_path / "root" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + _write_skill(outside, "evil-skill") + + (root / "evil-skill").symlink_to(outside / "evil-skill", target_is_directory=True) + _write_skill(root, "good-skill") + + assert FileSkillsSource._discover_skill_directories([str(root)]) == [str((root / "good-skill").absolute())] + + skills = await _discover_file_skills_for_test([str(root)]) + assert "evil-skill" not in skills + assert "good-skill" in skills + + def test_discover_skips_directory_with_symlinked_skill_file(self, tmp_path: Path) -> None: + """A real directory whose SKILL.md is a symlink must not be discovered.""" + root = tmp_path / "root" + root.mkdir() + outside_skill_file = tmp_path / "outside-SKILL.md" + outside_skill_file.write_text( + "---\nname: evil-skill\ndescription: Evil.\n---\nEvil instructions.", + encoding="utf-8", + ) + + evil_dir = root / "evil-skill" + evil_dir.mkdir() + (evil_dir / "SKILL.md").symlink_to(outside_skill_file) + + assert FileSkillsSource._discover_skill_directories([str(root)]) == [] + + def test_discover_keeps_nested_real_skill_directories(self, tmp_path: Path) -> None: + """Nested real skill directories are still discovered when links are present.""" + root = tmp_path / "root" + nested = root / "group" + nested.mkdir(parents=True) + _write_skill(nested, "nested-skill") + + outside = tmp_path / "outside" + outside.mkdir() + (root / "linked").symlink_to(outside, target_is_directory=True) + + assert FileSkillsSource._discover_skill_directories([str(root)]) == [str((nested / "nested-skill").absolute())] + + def test_configured_root_may_itself_be_a_link(self, tmp_path: Path) -> None: + """The host-configured root defines the trust boundary and is not link-checked.""" + real_root = tmp_path / "real-root" + real_root.mkdir() + _write_skill(real_root, "my-skill") + + linked_root = tmp_path / "linked-root" + linked_root.symlink_to(real_root, target_is_directory=True) + + assert FileSkillsSource._discover_skill_directories([str(linked_root)]) == [ + str((linked_root / "my-skill").absolute()) + ] + class TestJunctionDetection: """Tests for Windows junction guards in file-based skills.""" @@ -1027,6 +1086,54 @@ def test_junction_is_detected_and_excluded(self, tmp_path: Path) -> None: finally: junction.rmdir() + async def test_junctioned_skill_directory_is_not_discovered(self, tmp_path: Path) -> None: + """A junction below a configured root must not be adopted as a skill root.""" + root = tmp_path / "root" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + _write_skill(outside, "evil-skill") + _write_skill(root, "good-skill") + + junction = root / "evil-skill" + create_junction_or_skip(link=junction, target=outside / "evil-skill") + + try: + assert FileSkillsSource._discover_skill_directories([str(root)]) == [str((root / "good-skill").absolute())] + skills = await _discover_file_skills_for_test([str(root)]) + assert "evil-skill" not in skills + assert "good-skill" in skills + finally: + junction.rmdir() + + +class TestSkillDiscoveryFailsClosed: + """Discovery must fail closed when an entry cannot be inspected.""" + + def test_entry_that_cannot_be_inspected_is_skipped(self, tmp_path: Path) -> None: + root = tmp_path / "root" + root.mkdir() + _write_skill(root, "my-skill") + + def _raise(path: Path) -> bool: + raise OSError("cannot inspect") + + with patch("agent_framework._skills.is_link_or_reparse_point", side_effect=_raise): + assert FileSkillsSource._discover_skill_directories([str(root)]) == [] + + def test_skill_file_that_cannot_be_inspected_is_skipped(self, tmp_path: Path) -> None: + root = tmp_path / "root" + root.mkdir() + _write_skill(root, "my-skill") + + def _raise_for_skill_file(path: Path) -> bool: + if path.name == "SKILL.md": + raise OSError("cannot inspect") + return False + + with patch("agent_framework._skills.is_link_or_reparse_point", side_effect=_raise_for_skill_file): + assert FileSkillsSource._discover_skill_directories([str(root)]) == [] + # --------------------------------------------------------------------------- # Tests: SkillResource