diff --git a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillPathScope.cs b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillPathScope.cs new file mode 100644 index 00000000000..ea02abbaa2e --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillPathScope.cs @@ -0,0 +1,76 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.IO; + +namespace Microsoft.Agents.AI; + +/// +/// The path trust boundary of a discovered file-backed skill: the host-configured discovery +/// root together with the skill directory that was found at or beneath it. +/// +/// +/// +/// The configured root is retained alongside the skill directory so that a file can be +/// revalidated immediately before use against every directory from the root down to the file, +/// rather than only the segments below the skill directory. Without the root, a skill directory +/// (or a directory between it and the root) that was swapped for a link after discovery would +/// never be inspected. +/// +/// +/// The configured root itself is never inspected: the host chose it explicitly, so it defines the +/// trust boundary rather than sitting inside it, and it is allowed to be a link. +/// +/// +internal sealed class AgentFileSkillPathScope +{ + private static readonly string s_directorySeparator = Path.DirectorySeparatorChar.ToString(); + + /// + /// Initializes a new instance of the class. + /// + /// The host-configured discovery root the skill was found under. + /// The discovered skill directory, at or beneath the configured root. + /// The skill directory does not reside at or beneath the configured root. + public AgentFileSkillPathScope(string trustedRootFullPath, string skillDirectoryFullPath) + { + this.SkillDirectoryPath = Path.GetFullPath(skillDirectoryFullPath); + this.SkillDirectoryPrefix = EnsureTrailingSeparator(this.SkillDirectoryPath); + this.TrustedRootPrefix = EnsureTrailingSeparator(Path.GetFullPath(trustedRootFullPath)); + + if (!this.SkillDirectoryPrefix.StartsWith(this.TrustedRootPrefix, StringComparison.OrdinalIgnoreCase)) + { + throw new ArgumentException( + "The skill directory must reside at or beneath the configured skill discovery root.", + nameof(skillDirectoryFullPath)); + } + } + + /// + /// Gets the absolute path of the skill directory. + /// + public string SkillDirectoryPath { get; } + + /// + /// Gets the skill directory with a trailing separator, for path-containment checks and for + /// computing paths relative to the skill directory. + /// + /// + /// The trailing separator stops containment checks from false-matching sibling directories. + /// e.g. "/skills/myskill" matches "/skills/myskill-evil/", but "/skills/myskill/" does not. + /// + public string SkillDirectoryPrefix { get; } + + /// + /// Gets the configured discovery root with a trailing separator, used as the base for + /// link and reparse point scans so that every segment beneath it is inspected. + /// + public string TrustedRootPrefix { get; } + + private static string EnsureTrailingSeparator(string fullPath) + { + string trimmedPath = fullPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + + return string.Concat(trimmedPath, s_directorySeparator); + } +} diff --git a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillPathValidator.cs b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillPathValidator.cs new file mode 100644 index 00000000000..d723ad5c281 --- /dev/null +++ b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillPathValidator.cs @@ -0,0 +1,91 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.IO; +using System.Security; + +namespace Microsoft.Agents.AI; + +/// +/// Validates paths used by file-backed skills. +/// +internal static class AgentFileSkillPathValidator +{ + /// + /// Revalidates a discovered file against its trusted path scope immediately before use. + /// + internal static string ValidateForUse(string fullPath, AgentFileSkillPathScope scope, string fileKind, string fileName) + { + string resolvedFilePath = Path.GetFullPath(fullPath); + + if (!resolvedFilePath.StartsWith(scope.SkillDirectoryPrefix, StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException($"{fileKind} file '{fileName}' references a path outside the skill directory."); + } + + if (!File.Exists(resolvedFilePath)) + { + throw new FileNotFoundException($"{fileKind} file '{fileName}' was not found in the skill directory.", resolvedFilePath); + } + + // Scan from the configured discovery root rather than from the skill directory, so that a + // skill directory - or any directory between it and the root - that was replaced with a + // link after discovery is rejected as well. + if (HasLinkOrReparsePointInPath(resolvedFilePath, scope.TrustedRootPrefix)) + { + throw new InvalidOperationException( + $"{fileKind} file '{fileName}' has a symbolic link or reparse point in its path; links and reparse points are not allowed."); + } + + return resolvedFilePath; + } + + /// + /// Checks whether any segment in the path below the trusted base is a link, + /// reparse point, or cannot be inspected. + /// + internal static bool HasLinkOrReparsePointInPath(string pathToCheck, string trustedBasePath) + { + string relativePath = pathToCheck.Substring(trustedBasePath.Length); + string[] segments = relativePath.Split( + [Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar], + StringSplitOptions.RemoveEmptyEntries); + + string currentPath = trustedBasePath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + + foreach (string segment in segments) + { + currentPath = Path.Combine(currentPath, segment); + + if (IsLinkOrReparsePointOrInaccessible(currentPath)) + { + return true; + } + } + + return false; + } + + /// + /// Checks whether a path is a link, reparse point, or cannot be safely inspected. + /// + internal static bool IsLinkOrReparsePointOrInaccessible(string path) + { + try + { + return (File.GetAttributes(path) & FileAttributes.ReparsePoint) != 0; + } + catch (Exception ex) when (IsFileSystemInspectionFailure(ex)) + { + return true; + } + } + + /// + /// Checks whether an exception indicates that a filesystem path could not be inspected. + /// + internal static bool IsFileSystemInspectionFailure(Exception exception) + { + return exception is IOException or UnauthorizedAccessException or SecurityException; + } +} diff --git a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillResource.cs b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillResource.cs index 9ba5b7e24ae..6e7e1415238 100644 --- a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillResource.cs +++ b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillResource.cs @@ -14,15 +14,19 @@ namespace Microsoft.Agents.AI; /// internal sealed class AgentFileSkillResource : AgentSkillResource { + private readonly AgentFileSkillPathScope _scope; + /// /// Initializes a new instance of the class. /// /// The resource name (relative path within the skill directory). /// The absolute file path to the resource. - public AgentFileSkillResource(string name, string fullPath) + /// The trusted path scope the resource was discovered in. + public AgentFileSkillResource(string name, string fullPath, AgentFileSkillPathScope scope) : base(name) { this.FullPath = Throw.IfNullOrWhitespace(fullPath); + this._scope = Throw.IfNull(scope); } /// @@ -33,10 +37,12 @@ public AgentFileSkillResource(string name, string fullPath) /// public override async Task ReadAsync(IServiceProvider? serviceProvider = null, CancellationToken cancellationToken = default) { + string validatedPath = AgentFileSkillPathValidator.ValidateForUse(this.FullPath, this._scope, "Resource", this.Name); + #if NET8_0_OR_GREATER - return await File.ReadAllTextAsync(this.FullPath, Encoding.UTF8, cancellationToken).ConfigureAwait(false); + return await File.ReadAllTextAsync(validatedPath, Encoding.UTF8, cancellationToken).ConfigureAwait(false); #else - using var reader = new StreamReader(this.FullPath, Encoding.UTF8); + using var reader = new StreamReader(validatedPath, Encoding.UTF8); return await reader.ReadToEndAsync().ConfigureAwait(false); #endif } diff --git a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillScript.cs b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillScript.cs index 9bb6fdd798d..1c1acd990a8 100644 --- a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillScript.cs +++ b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillScript.cs @@ -19,17 +19,20 @@ public sealed class AgentFileSkillScript : AgentSkillScript private static readonly JsonElement s_defaultSchema = CreateDefaultSchema(); private readonly AgentFileSkillScriptRunner? _runner; + private readonly AgentFileSkillPathScope _scope; /// /// Initializes a new instance of the class. /// /// The script name. /// The absolute file path to the script. + /// The trusted path scope the script was discovered in. /// Optional external runner for running the script. An is thrown from if no runner is provided. - internal AgentFileSkillScript(string name, string fullPath, AgentFileSkillScriptRunner? runner = null) + internal AgentFileSkillScript(string name, string fullPath, AgentFileSkillPathScope scope, AgentFileSkillScriptRunner? runner = null) : base(name) { this.FullPath = Throw.IfNullOrWhitespace(fullPath); + this._scope = Throw.IfNull(scope); this._runner = runner; } @@ -60,6 +63,8 @@ internal AgentFileSkillScript(string name, string fullPath, AgentFileSkillScript $"Supply a script runner when constructing {nameof(AgentFileSkillsSource)} to enable script execution."); } + AgentFileSkillPathValidator.ValidateForUse(this.FullPath, this._scope, "Script", this.Name); + return await this._runner(fileSkill, this, arguments, serviceProvider, cancellationToken).ConfigureAwait(false); } diff --git a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs index d28c969cb5e..b22ea74c93e 100644 --- a/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs +++ b/dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs @@ -5,7 +5,6 @@ using System.Diagnostics.CodeAnalysis; using System.IO; using System.Linq; -using System.Security; using System.Text; using System.Text.RegularExpressions; using System.Threading; @@ -26,6 +25,7 @@ namespace Microsoft.Agents.AI; /// Each file is validated for YAML frontmatter. Resource and script files are discovered by scanning the skill /// directory for files with matching extensions. Invalid resources are skipped with logged warnings. /// Resource and script paths are checked against path traversal and symlink escape attacks. +/// Discovered files are revalidated against their trusted skill directory immediately before use. /// public sealed partial class AgentFileSkillsSource : AgentSkillsSource { @@ -126,9 +126,9 @@ public override Task> GetSkillsAsync(AgentSkillsSourceContext var skills = new List(); - foreach (string skillPath in discoveredPaths) + foreach (AgentFileSkillPathScope scope in discoveredPaths) { - AgentFileSkill? skill = this.ParseSkillDirectory(skillPath); + AgentFileSkill? skill = this.ParseSkillDirectory(scope); if (skill is null) { continue; @@ -144,9 +144,9 @@ public override Task> GetSkillsAsync(AgentSkillsSourceContext return Task.FromResult(skills as IList); } - private List DiscoverSkillDirectories(IEnumerable skillPaths) + private List DiscoverSkillDirectories(IEnumerable skillPaths) { - var discoveredPaths = new List(); + var discoveredPaths = new List(); foreach (string rootDirectory in skillPaths) { @@ -155,18 +155,18 @@ private List DiscoverSkillDirectories(IEnumerable skillPaths) continue; } - this.SearchDirectoriesForSkills(rootDirectory, discoveredPaths, currentDepth: 0); + this.SearchDirectoriesForSkills(rootDirectory, Path.GetFullPath(rootDirectory), discoveredPaths, currentDepth: 0); } return discoveredPaths; } - private void SearchDirectoriesForSkills(string directory, List results, int currentDepth) + private void SearchDirectoriesForSkills(string directory, string trustedRootFullPath, List results, int currentDepth) { string skillFilePath = Path.Combine(directory, SkillFileName); if (File.Exists(skillFilePath)) { - if (IsLinkOrReparsePointOrInaccessible(skillFilePath)) + if (AgentFileSkillPathValidator.IsLinkOrReparsePointOrInaccessible(skillFilePath)) { LogUnsafeSkillDiscoveryPath(this._logger, SanitizePathForLog(skillFilePath)); return; @@ -174,7 +174,7 @@ private void SearchDirectoriesForSkills(string directory, List results, // Once a SKILL.md is found, this directory is the skill root. // Subdirectories are part of this skill and should not be treated as independent skill roots. - results.Add(Path.GetFullPath(directory)); + results.Add(new AgentFileSkillPathScope(trustedRootFullPath, directory)); return; } @@ -185,19 +185,19 @@ private void SearchDirectoriesForSkills(string directory, List results, foreach (string subdirectory in this.SafeEnumerateDirectories(directory, attributesToSkip: 0)) { - if (IsLinkOrReparsePointOrInaccessible(subdirectory)) + if (AgentFileSkillPathValidator.IsLinkOrReparsePointOrInaccessible(subdirectory)) { LogUnsafeSkillDiscoveryPath(this._logger, SanitizePathForLog(subdirectory)); continue; } - this.SearchDirectoriesForSkills(subdirectory, results, currentDepth + 1); + this.SearchDirectoriesForSkills(subdirectory, trustedRootFullPath, results, currentDepth + 1); } } - private AgentFileSkill? ParseSkillDirectory(string skillDirectoryFullPath) + private AgentFileSkill? ParseSkillDirectory(AgentFileSkillPathScope scope) { - string skillFilePath = Path.Combine(skillDirectoryFullPath, SkillFileName); + string skillFilePath = Path.Combine(scope.SkillDirectoryPath, SkillFileName); string content = File.ReadAllText(skillFilePath, Encoding.UTF8); if (!this.TryParseFrontmatter(content, skillFilePath, out AgentSkillFrontmatter? frontmatter)) @@ -205,18 +205,13 @@ private void SearchDirectoriesForSkills(string directory, List results, return null; } - // Append a trailing separator so path-containment checks don't false-match - // sibling directories. e.g. "/skills/myskill" matches "/skills/myskill-evil/", - // but "/skills/myskill/" does not. - string normalizedSkillDirectoryFullPath = skillDirectoryFullPath + Path.DirectorySeparatorChar; - - var resources = this.DiscoverResourceFiles(normalizedSkillDirectoryFullPath, frontmatter.Name); - var scripts = this.DiscoverScriptFiles(normalizedSkillDirectoryFullPath, frontmatter.Name); + var resources = this.DiscoverResourceFiles(scope, frontmatter.Name); + var scripts = this.DiscoverScriptFiles(scope, frontmatter.Name); return new AgentFileSkill( frontmatter: frontmatter, content: content, - path: skillDirectoryFullPath, + path: scope.SkillDirectoryPath, resources: resources, scripts: scripts); } @@ -323,29 +318,29 @@ private bool TryParseFrontmatter(string content, string skillFilePath, [NotNullW /// If a predicate is configured, files /// that do not satisfy it are excluded. /// - private List DiscoverResourceFiles(string skillDirectoryFullPath, string skillName) + private List DiscoverResourceFiles(AgentFileSkillPathScope scope, string skillName) { var resources = new List(); - this.ScanDirectoryForResources(skillDirectoryFullPath, skillDirectoryFullPath, skillName, resources, currentDepth: 1); + this.ScanDirectoryForResources(scope.SkillDirectoryPrefix, scope, skillName, resources, currentDepth: 1); return resources; } - private void ScanDirectoryForResources(string targetDirectory, string skillDirectoryFullPath, string skillName, List resources, int currentDepth) + private void ScanDirectoryForResources(string targetDirectory, AgentFileSkillPathScope scope, string skillName, List resources, int currentDepth) { if (currentDepth > this._searchDepth) { return; } - bool isRootDirectory = string.Equals(targetDirectory, skillDirectoryFullPath, StringComparison.OrdinalIgnoreCase); + bool isRootDirectory = string.Equals(targetDirectory, scope.SkillDirectoryPrefix, StringComparison.OrdinalIgnoreCase); // Directory-level symlink check: skip if targetDirectory (or any intermediate // segment) is a reparse point or cannot be inspected. The root directory is excluded — // it's a caller-supplied trusted path, and the security boundary guards files within it, // not the path itself. - if (!isRootDirectory && HasSymlinkInPath(targetDirectory, skillDirectoryFullPath)) + if (!isRootDirectory && AgentFileSkillPathValidator.HasLinkOrReparsePointInPath(targetDirectory, scope.SkillDirectoryPrefix)) { if (this._logger.IsEnabled(LogLevel.Warning)) { @@ -394,7 +389,7 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec // Path containment: reject if the resolved path escapes the skill directory. // e.g. "/etc/shadow".StartsWith("/skills/myskill/") → false → skip - if (!resolvedFilePath.StartsWith(skillDirectoryFullPath, StringComparison.OrdinalIgnoreCase)) + if (!resolvedFilePath.StartsWith(scope.SkillDirectoryPrefix, StringComparison.OrdinalIgnoreCase)) { if (this._logger.IsEnabled(LogLevel.Warning)) { @@ -407,7 +402,7 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec // Per-file symlink check: detects if the file (or any intermediate segment) // is a reparse point or cannot be inspected. // e.g. "references/secret.md" → symlink to "/etc/shadow" - if (HasSymlinkInPath(resolvedFilePath, skillDirectoryFullPath)) + if (AgentFileSkillPathValidator.HasLinkOrReparsePointInPath(resolvedFilePath, scope.SkillDirectoryPrefix)) { if (this._logger.IsEnabled(LogLevel.Warning)) { @@ -419,7 +414,7 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec // Compute relative path and normalize separators. // e.g. "/skills/myskill/references/guide.md" → "references/guide.md" - string relativePath = NormalizePath(resolvedFilePath.Substring(skillDirectoryFullPath.Length)); + string relativePath = NormalizePath(resolvedFilePath.Substring(scope.SkillDirectoryPrefix.Length)); // Apply user-provided filter predicate if (this._resourceFilter is not null && !this._resourceFilter(new AgentFileSkillFilterContext(skillName, relativePath))) @@ -427,7 +422,7 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec continue; } - resources.Add(new AgentFileSkillResource(relativePath, resolvedFilePath)); + resources.Add(new AgentFileSkillResource(relativePath, resolvedFilePath, scope)); } // Recurse into subdirectories if within depth limit @@ -435,7 +430,7 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec { foreach (string subdirectory in this.SafeEnumerateDirectories(targetDirectory, FileAttributes.ReparsePoint)) { - this.ScanDirectoryForResources(subdirectory, skillDirectoryFullPath, skillName, resources, currentDepth + 1); + this.ScanDirectoryForResources(subdirectory, scope, skillName, resources, currentDepth + 1); } } } @@ -449,29 +444,29 @@ private void ScanDirectoryForResources(string targetDirectory, string skillDirec /// If a predicate is configured, files /// that do not satisfy it are excluded. /// - private List DiscoverScriptFiles(string skillDirectoryFullPath, string skillName) + private List DiscoverScriptFiles(AgentFileSkillPathScope scope, string skillName) { var scripts = new List(); - this.ScanDirectoryForScripts(skillDirectoryFullPath, skillDirectoryFullPath, skillName, scripts, currentDepth: 1); + this.ScanDirectoryForScripts(scope.SkillDirectoryPrefix, scope, skillName, scripts, currentDepth: 1); return scripts; } - private void ScanDirectoryForScripts(string targetDirectory, string skillDirectoryFullPath, string skillName, List scripts, int currentDepth) + private void ScanDirectoryForScripts(string targetDirectory, AgentFileSkillPathScope scope, string skillName, List scripts, int currentDepth) { if (currentDepth > this._searchDepth) { return; } - bool isRootDirectory = string.Equals(targetDirectory, skillDirectoryFullPath, StringComparison.OrdinalIgnoreCase); + bool isRootDirectory = string.Equals(targetDirectory, scope.SkillDirectoryPrefix, StringComparison.OrdinalIgnoreCase); // Directory-level symlink check: skip if targetDirectory (or any intermediate // segment) is a reparse point or cannot be inspected. The root directory is excluded — // it's a caller-supplied trusted path, and the security boundary guards files within it, // not the path itself. - if (!isRootDirectory && HasSymlinkInPath(targetDirectory, skillDirectoryFullPath)) + if (!isRootDirectory && AgentFileSkillPathValidator.HasLinkOrReparsePointInPath(targetDirectory, scope.SkillDirectoryPrefix)) { if (this._logger.IsEnabled(LogLevel.Warning)) { @@ -507,7 +502,7 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto // Path containment: reject if the resolved path escapes the skill directory. // e.g. "/etc/shadow".StartsWith("/skills/myskill/") → false → skip - if (!resolvedFilePath.StartsWith(skillDirectoryFullPath, StringComparison.OrdinalIgnoreCase)) + if (!resolvedFilePath.StartsWith(scope.SkillDirectoryPrefix, StringComparison.OrdinalIgnoreCase)) { if (this._logger.IsEnabled(LogLevel.Warning)) { @@ -520,7 +515,7 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto // Per-file symlink check: detects if the file (or any intermediate segment) // is a reparse point or cannot be inspected. // e.g. "scripts/run.py" → symlink to "/etc/shadow" - if (HasSymlinkInPath(resolvedFilePath, skillDirectoryFullPath)) + if (AgentFileSkillPathValidator.HasLinkOrReparsePointInPath(resolvedFilePath, scope.SkillDirectoryPrefix)) { if (this._logger.IsEnabled(LogLevel.Warning)) { @@ -532,7 +527,7 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto // Compute relative path and normalize separators. // e.g. "/skills/myskill/scripts/parsepdf.py" → "scripts/parsepdf.py" - string relativePath = NormalizePath(resolvedFilePath.Substring(skillDirectoryFullPath.Length)); + string relativePath = NormalizePath(resolvedFilePath.Substring(scope.SkillDirectoryPrefix.Length)); // Apply user-provided filter predicate if (this._scriptFilter is not null && !this._scriptFilter(new AgentFileSkillFilterContext(skillName, relativePath))) @@ -540,7 +535,7 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto continue; } - scripts.Add(new AgentFileSkillScript(relativePath, resolvedFilePath, this._scriptRunner)); + scripts.Add(new AgentFileSkillScript(relativePath, resolvedFilePath, scope, this._scriptRunner)); } // Recurse into subdirectories if within depth limit @@ -548,52 +543,9 @@ private void ScanDirectoryForScripts(string targetDirectory, string skillDirecto { foreach (string subdirectory in this.SafeEnumerateDirectories(targetDirectory, FileAttributes.ReparsePoint)) { - this.ScanDirectoryForScripts(subdirectory, skillDirectoryFullPath, skillName, scripts, currentDepth + 1); - } - } - } - - /// - /// Checks whether any segment in the path (relative to the directory) is a symlink, - /// reparse point, or cannot be inspected. - /// - private static bool HasSymlinkInPath(string pathToCheck, string trustedBasePath) - { - string relativePath = pathToCheck.Substring(trustedBasePath.Length); - string[] segments = relativePath.Split( - [Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar], - StringSplitOptions.RemoveEmptyEntries); - - string currentPath = trustedBasePath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); - - foreach (string segment in segments) - { - currentPath = Path.Combine(currentPath, segment); - - if (IsLinkOrReparsePointOrInaccessible(currentPath)) - { - return true; + this.ScanDirectoryForScripts(subdirectory, scope, skillName, scripts, currentDepth + 1); } } - - return false; - } - - private static bool IsLinkOrReparsePointOrInaccessible(string path) - { - try - { - return (File.GetAttributes(path) & FileAttributes.ReparsePoint) != 0; - } - catch (Exception ex) when (IsFileSystemInspectionFailure(ex)) - { - return true; - } - } - - private static bool IsFileSystemInspectionFailure(Exception exception) - { - return exception is IOException or UnauthorizedAccessException or SecurityException; } /// @@ -619,7 +571,7 @@ private string[] SafeEnumerateDirectories(string path, FileAttributes attributes return Directory.GetDirectories(path); #endif } - catch (Exception ex) when (IsFileSystemInspectionFailure(ex)) + catch (Exception ex) when (AgentFileSkillPathValidator.IsFileSystemInspectionFailure(ex)) { if (this._logger.IsEnabled(LogLevel.Warning)) { diff --git a/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentFileSkillScriptTests.cs b/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentFileSkillScriptTests.cs index 0957ee0622e..b3b5dd1b8e9 100644 --- a/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentFileSkillScriptTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentFileSkillScriptTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. using System; +using System.IO; using System.Text.Json; using System.Threading; using System.Threading.Tasks; @@ -10,14 +11,27 @@ namespace Microsoft.Agents.AI.UnitTests.AgentSkills; /// /// Unit tests for . /// -public sealed class AgentFileSkillScriptTests +public sealed class AgentFileSkillScriptTests : IDisposable { + private readonly string _testRoot; + + public AgentFileSkillScriptTests() + { + this._testRoot = Path.Combine(Path.GetTempPath(), "file-skill-script-tests-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(this._testRoot); + } + + public void Dispose() + { + Directory.Delete(this._testRoot, recursive: true); + } + [Fact] public async Task RunAsync_SkillIsNotAgentFileSkill_ThrowsInvalidOperationExceptionAsync() { // Arrange static Task RunnerAsync(AgentFileSkill s, AgentFileSkillScript sc, JsonElement? a, IServiceProvider? sp, CancellationToken ct) => Task.FromResult("result"); - var script = CreateScript("test-script", "/path/to/script.py", RunnerAsync); + var script = this.CreateScript("test-script", "/path/to/script.py", RunnerAsync); var nonFileSkill = new TestAgentSkill("my-skill", "A skill", "Instructions."); // Act & Assert @@ -35,7 +49,7 @@ public async Task RunAsync_WithAgentFileSkill_DelegatesToRunnerAsync() runnerCalled = true; return Task.FromResult("executed"); } - var script = CreateScript("run-me", "/scripts/run-me.sh", runnerAsync); + var script = this.CreateScript("run-me", "/scripts/run-me.sh", runnerAsync); var fileSkill = new AgentFileSkill( new AgentSkillFrontmatter("my-skill", "A file skill"), "---\nname: my-skill\n---\nContent", @@ -61,7 +75,7 @@ public async Task RunAsync_RunnerReceivesCorrectArgumentsAsync() capturedScript = scriptArg; return Task.FromResult(null); } - var script = CreateScript("capture", "/scripts/capture.py", runnerAsync); + var script = this.CreateScript("capture", "/scripts/capture.py", runnerAsync); var fileSkill = new AgentFileSkill( new AgentSkillFrontmatter("owner-skill", "Owner"), "Content", @@ -80,11 +94,11 @@ public void Script_HasCorrectNameAndPath() { // Arrange & Act static Task RunnerAsync(AgentFileSkill s, AgentFileSkillScript sc, JsonElement? a, IServiceProvider? sp, CancellationToken ct) => Task.FromResult(null); - var script = CreateScript("my-script", "/path/to/my-script.py", RunnerAsync); + var script = this.CreateScript("my-script", "/path/to/my-script.py", RunnerAsync); // Assert Assert.Equal("my-script", script.Name); - Assert.Equal("/path/to/my-script.py", script.FullPath); + Assert.Equal(Path.Combine(this._testRoot, "my-script.py"), script.FullPath); } [Fact] @@ -92,7 +106,7 @@ public void ParametersSchema_ReturnsExpectedArraySchema() { // Arrange static Task RunnerAsync(AgentFileSkill s, AgentFileSkillScript sc, JsonElement? a, IServiceProvider? sp, CancellationToken ct) => Task.FromResult(null); - var script = CreateScript("my-script", "/path/to/script.py", RunnerAsync); + var script = this.CreateScript("my-script", "/path/to/script.py", RunnerAsync); // Act var schema = script.ParametersSchema; @@ -109,8 +123,8 @@ public async Task Content_WithScripts_AppendsPerScriptEntriesAsync() { // Arrange static Task RunnerAsync(AgentFileSkill s, AgentFileSkillScript sc, JsonElement? a, IServiceProvider? sp, CancellationToken ct) => Task.FromResult(null); - var script1 = CreateScript("build", "/scripts/build.sh", RunnerAsync); - var script2 = CreateScript("deploy", "/scripts/deploy.sh", RunnerAsync); + var script1 = this.CreateScript("build", "/scripts/build.sh", RunnerAsync); + var script2 = this.CreateScript("deploy", "/scripts/deploy.sh", RunnerAsync); var fileSkill = new AgentFileSkill( new AgentSkillFrontmatter("my-skill", "A skill"), "Original content", @@ -183,7 +197,7 @@ public async Task Content_WithResourcesAndScripts_AppendsResourcesBeforeScriptsA "Original content", "/skills/my-skill", resources: [new AgentInlineSkillResource("reference", "value")], - scripts: [CreateScript("build", "/scripts/build.sh", RunnerAsync)]); + scripts: [this.CreateScript("build", "/scripts/build.sh", RunnerAsync)]); // Act var content = await fileSkill.GetContentAsync(); @@ -200,7 +214,7 @@ public async Task Content_WithScripts_IsCachedAsync() { // Arrange static Task RunnerAsync(AgentFileSkill s, AgentFileSkillScript sc, JsonElement? a, IServiceProvider? sp, CancellationToken ct) => Task.FromResult(null); - var script = CreateScript("test", "/scripts/test.sh", RunnerAsync); + var script = this.CreateScript("test", "/scripts/test.sh", RunnerAsync); var fileSkill = new AgentFileSkill( new AgentSkillFrontmatter("my-skill", "A skill"), "Content", @@ -225,7 +239,7 @@ public async Task RunAsync_ForwardsJsonArrayArgumentsToRunnerAsync() capturedArgs = args; return Task.FromResult("done"); } - var script = CreateScript("array-test", "/scripts/test.sh", runnerAsync); + var script = this.CreateScript("array-test", "/scripts/test.sh", runnerAsync); var fileSkill = new AgentFileSkill( new AgentSkillFrontmatter("my-skill", "A skill"), "Content", @@ -252,7 +266,7 @@ public async Task RunAsync_ForwardsServiceProviderToRunnerAsync() capturedProvider = sp; return Task.FromResult("done"); } - var script = CreateScript("sp-test", "/scripts/test.sh", runnerAsync); + var script = this.CreateScript("sp-test", "/scripts/test.sh", runnerAsync); var fileSkill = new AgentFileSkill( new AgentSkillFrontmatter("my-skill", "A skill"), "Content", @@ -270,7 +284,7 @@ public async Task RunAsync_ForwardsServiceProviderToRunnerAsync() public async Task RunAsync_NoRunner_ThrowsInvalidOperationExceptionAsync() { // Arrange — create script without a runner - var script = CreateScript("no-runner", "/scripts/test.sh", runner: null); + var script = this.CreateScript("no-runner", "/scripts/test.sh", runner: null); var fileSkill = new AgentFileSkill( new AgentSkillFrontmatter("my-skill", "A skill"), "Content", @@ -286,7 +300,7 @@ public async Task Content_WithScripts_ContainsDefaultParametersSchemaAsync() { // Arrange static Task RunnerAsync(AgentFileSkill s, AgentFileSkillScript sc, JsonElement? a, IServiceProvider? sp, CancellationToken ct) => Task.FromResult(null); - var script = CreateScript("test", "/scripts/test.sh", RunnerAsync); + var script = this.CreateScript("test", "/scripts/test.sh", RunnerAsync); var fileSkill = new AgentFileSkill( new AgentSkillFrontmatter("my-skill", "A skill"), "Original content", @@ -301,17 +315,14 @@ public async Task Content_WithScripts_ContainsDefaultParametersSchemaAsync() } /// - /// Helper to create an via reflection since the constructor is internal. + /// Helper to create an rooted in the test directory. /// - private static AgentFileSkillScript CreateScript(string name, string fullPath, AgentFileSkillScriptRunner? runner) + private AgentFileSkillScript CreateScript(string name, string fullPath, AgentFileSkillScriptRunner? runner) { - var ctor = typeof(AgentFileSkillScript).GetConstructor( - System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance, - null, - [typeof(string), typeof(string), typeof(AgentFileSkillScriptRunner)], - null) ?? throw new InvalidOperationException("Could not find internal constructor."); + string resolvedPath = Path.Combine(this._testRoot, Path.GetFileName(fullPath)); + File.WriteAllText(resolvedPath, string.Empty); - return (AgentFileSkillScript)ctor.Invoke([name, fullPath, runner]); + return new AgentFileSkillScript(name, resolvedPath, new AgentFileSkillPathScope(this._testRoot, this._testRoot), runner); } /// diff --git a/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs b/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs index 41a772f1b18..6db43554f84 100644 --- a/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs @@ -597,6 +597,93 @@ public async Task ReadSkillResourceAsync_ValidResource_ReturnsContentAsync() Assert.Equal("Document content here.", content); } +#if NET + [Fact] + public async Task ReadSkillResourceAsync_ResourceReplacedWithSymlink_ThrowsAsync() + { + // Arrange + string skillDir = this.CreateSkillDirectory("read-symlink-skill", "A skill", "See docs."); + string refsDir = Path.Combine(skillDir, "references"); + Directory.CreateDirectory(refsDir); + string resourcePath = Path.Combine(refsDir, "doc.md"); + File.WriteAllText(resourcePath, "Safe content."); + string outsidePath = Path.Combine(this._testRoot, "secret.md"); + File.WriteAllText(outsidePath, "Secret content."); + var source = new AgentFileSkillsSource(this._testRoot, s_noOpExecutor); + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + var resource = skills[0].GetTestResources()!.Single(r => r.Name == "references/doc.md"); + + File.Delete(resourcePath); + if (!TryCreateFileSymbolicLink(resourcePath, outsidePath)) + { + Assert.Skip("Symbolic links are not supported in this environment."); + } + + // Act & Assert + await Assert.ThrowsAsync(() => resource.ReadAsync()); + } + + [Fact] + public async Task RunSkillScriptAsync_ScriptReplacedWithSymlink_DoesNotInvokeRunnerAsync() + { + // Arrange + string skillDir = this.CreateSkillDirectory("run-symlink-skill", "A skill", "Run scripts."); + string scriptsDir = Path.Combine(skillDir, "scripts"); + Directory.CreateDirectory(scriptsDir); + string scriptPath = Path.Combine(scriptsDir, "run.py"); + File.WriteAllText(scriptPath, "print('safe')"); + string outsidePath = Path.Combine(this._testRoot, "outside.py"); + File.WriteAllText(outsidePath, "print('outside')"); + bool runnerCalled = false; + var source = new AgentFileSkillsSource( + this._testRoot, + (skill, script, args, serviceProvider, cancellationToken) => + { + runnerCalled = true; + return Task.FromResult(null); + }); + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + var script = await skills[0].GetScriptAsync("scripts/run.py"); + + File.Delete(scriptPath); + if (!TryCreateFileSymbolicLink(scriptPath, outsidePath)) + { + Assert.Skip("Symbolic links are not supported in this environment."); + } + + // Act & Assert + await Assert.ThrowsAsync( + () => script!.RunAsync(skills[0], null, null)); + Assert.False(runnerCalled); + } + + [Fact] + public async Task ReadSkillResourceAsync_SkillDirectoryReplacedWithSymlink_ThrowsAsync() + { + // Arrange — a skill whose directory sits below the configured root + string skillDir = this.CreateSkillDirectory("swapped-skill", "A skill", "See docs."); + string resourcePath = Path.Combine(skillDir, "doc.md"); + File.WriteAllText(resourcePath, "Safe content."); + var source = new AgentFileSkillsSource(this._testRoot, s_noOpExecutor); + var skills = await source.GetSkillsAsync(TestAgentSkillsSourceContextFactory.Create()); + var resource = skills[0].GetTestResources()!.Single(r => r.Name == "doc.md"); + + // Replace the whole skill directory with a link to an attacker-controlled directory + // that mirrors the discovered layout. + string decoyDir = Path.Combine(this._testRoot, "decoy"); + Directory.CreateDirectory(decoyDir); + File.WriteAllText(Path.Combine(decoyDir, "doc.md"), "Attacker content."); + Directory.Delete(skillDir, recursive: true); + if (!TryCreateDirectorySymbolicLink(skillDir, decoyDir) && !TryCreateDirectoryJunction(skillDir, decoyDir)) + { + Assert.Skip("Directory links are not supported in this environment."); + } + + // Act & Assert + await Assert.ThrowsAsync(() => resource.ReadAsync()); + } +#endif + [Fact] public async Task GetSkillsAsync_NameExceedsMaxLength_ExcludesSkillAsync() { diff --git a/python/packages/core/AGENTS.md b/python/packages/core/AGENTS.md index 6363ff3ba00..6e778c5c3be 100644 --- a/python/packages/core/AGENTS.md +++ b/python/packages/core/AGENTS.md @@ -152,7 +152,7 @@ The vector store API is experimental under the shared `VECTOR_STORES` feature ID - **`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". +- **`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. - **`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 e118464438a..ad6dd5c3255 100644 --- a/python/packages/core/agent_framework/_skills.py +++ b/python/packages/core/agent_framework/_skills.py @@ -230,6 +230,45 @@ async def read(self, **kwargs: Any) -> Any: return result +@dataclass(frozen=True) +class _SkillPathScope: + """The path trust boundary of a discovered file-backed skill. + + Pairs the host-configured discovery root with the skill directory found at or + beneath it. The root is retained so that a file can be revalidated immediately + before use against every directory from the root down to the file, rather than + only the segments below the skill directory. Without the root, a skill directory + (or a directory between it and the root) that was swapped for a link after + discovery would never be inspected. + + The configured root itself is never inspected: the host chose it explicitly, so it + defines the trust boundary rather than sitting inside it, and it is allowed to be a link. + + Containment is enforced here rather than at use time so that a mismatched pair fails + at construction, and so that every later check can rely on the invariant. + + Attributes: + trusted_root: Absolute path of the host-configured discovery root. + skill_dir: Absolute path of the skill directory, at or beneath ``trusted_root``. + + Raises: + ValueError: If ``skill_dir`` does not reside at or beneath ``trusted_root``. + """ + + trusted_root: str + skill_dir: str + + def __post_init__(self) -> None: + within_root = FileSkillsSource._is_path_within_directory( # pyright: ignore[reportPrivateUsage] + os.path.normpath(self.skill_dir), + os.path.normpath(self.trusted_root), + ) + if not within_root: + raise ValueError( + f"skill_dir '{self.skill_dir}' must reside at or beneath trusted_root '{self.trusted_root}'." + ) + + class _FileSkillResource(SkillResource): """A file-path-backed skill resource that reads content from disk. @@ -248,6 +287,7 @@ def __init__( name: str, full_path: str, description: str | None = None, + scope: _SkillPathScope | None = None, ) -> None: """Initialize a _FileSkillResource. @@ -255,6 +295,7 @@ def __init__( name: Relative path of the resource within the skill directory. full_path: Absolute path to the resource file. description: Optional human-readable summary. + scope: Trusted path scope used to revalidate discovered resources before reading. Raises: ValueError: If ``full_path`` is empty. @@ -265,6 +306,7 @@ def __init__( raise ValueError("full_path cannot be empty.") self.full_path = full_path + self._scope = scope async def read(self, **kwargs: Any) -> Any: """Read the resource content from disk. @@ -278,11 +320,22 @@ async def read(self, **kwargs: Any) -> Any: Raises: ValueError: If the resource file does not exist. """ - if not await asyncio.to_thread(Path(self.full_path).is_file): + return await asyncio.to_thread(self._read_validated_resource) + + def _read_validated_resource(self) -> str: + validated_path = self.full_path + if self._scope is not None: + validated_path = FileSkillsSource._validate_file_path_for_use( # pyright: ignore[reportPrivateUsage] + self._scope, + self.full_path, + self.name, + "Resource", + ) + elif not Path(self.full_path).is_file(): raise ValueError(f"Resource file '{self.name}' not found at '{self.full_path}'.") logger.info("Reading resource '%s' from '%s'", self.name, self.full_path) - return await asyncio.to_thread(Path(self.full_path).read_text, encoding="utf-8") + return Path(validated_path).read_text(encoding="utf-8") class SkillScript(ABC): @@ -478,6 +531,8 @@ def __init__( description: str | None = None, full_path: str, runner: SkillScriptRunner | None = None, + skill_dir: str | None = None, + trusted_root: str | None = None, ) -> None: """Initialize a FileSkillScript. @@ -487,9 +542,15 @@ def __init__( full_path: Absolute path to the script file. runner: Strategy for running file-based scripts. Required for execution; an error is raised from :meth:`run` if not provided. + skill_dir: Trusted skill directory used to revalidate the script before execution. + trusted_root: Configured discovery root used to include the skill directory and + intermediate directories in revalidation. Requires ``skill_dir``. When omitted, + revalidation starts at ``skill_dir`` for backward compatibility. Raises: - ValueError: If ``full_path`` is empty or not an absolute path. + ValueError: If ``full_path`` is empty or not an absolute path, if + ``trusted_root`` is provided without ``skill_dir``, or if ``skill_dir`` + does not reside at or beneath ``trusted_root``. """ super().__init__(name=name, description=description) @@ -497,9 +558,19 @@ def __init__( raise ValueError("full_path cannot be empty.") if not os.path.isabs(full_path): raise ValueError(f"full_path must be an absolute path, got: '{full_path}'") + if trusted_root is not None and skill_dir is None: + raise ValueError("trusted_root requires skill_dir.") self.full_path = full_path self._runner = runner + self._scope = ( + _SkillPathScope( + trusted_root=trusted_root if trusted_root is not None else skill_dir, + skill_dir=skill_dir, + ) + if skill_dir is not None + else None + ) @property def parameters_schema(self) -> dict[str, Any] | None: @@ -533,6 +604,14 @@ async def run(self, skill: Skill, args: dict[str, Any] | list[str] | None = None ) if self._runner is None: raise ValueError(f"Script '{self.name}' requires a runner. Provide a script_runner for file-based scripts.") + if self._scope is not None: + await asyncio.to_thread( + FileSkillsSource._validate_file_path_for_use, # pyright: ignore[reportPrivateUsage] + self._scope, + self.full_path, + self.name, + "Script", + ) result = self._runner(skill, self, args) if inspect.isawaitable(result): return await result @@ -2915,7 +2994,8 @@ async def get_skills(self, context: SkillsSourceContext) -> list[Skill]: discovered = FileSkillsSource._discover_skill_directories(self._skill_paths) logger.info("Discovered %d potential skills", len(discovered)) - for skill_path in discovered: + for scope in discovered: + skill_path = scope.skill_dir parsed = FileSkillsSource._read_and_parse_skill_file(skill_path) if parsed is None: continue @@ -2933,14 +3013,22 @@ async def get_skills(self, context: SkillsSourceContext) -> list[Skill]: # Discover file-based resources resources: list[SkillResource] = [] for rn in self._discover_resource_files(skill_path, frontmatter.name): - resource_full_path = FileSkillsSource._get_validated_resource_path(skill_path, rn) - resources.append(_FileSkillResource(name=rn, full_path=resource_full_path)) + resource_full_path = FileSkillsSource._get_validated_resource_path(scope, rn) + resources.append(_FileSkillResource(name=rn, full_path=resource_full_path, scope=scope)) # Discover file-based scripts scripts: list[SkillScript] = [] for sn in self._discover_script_files(skill_path, frontmatter.name): script_full_path = os.path.normpath(os.path.join(skill_path, sn)) # ruff:ignore[blocking-path-method-in-async-function] - scripts.append(FileSkillScript(name=sn, full_path=script_full_path, runner=self._script_runner)) + scripts.append( + FileSkillScript( + name=sn, + full_path=script_full_path, + runner=self._script_runner, + skill_dir=scope.skill_dir, + trusted_root=scope.trusted_root, + ) + ) file_skill = FileSkill( frontmatter=frontmatter, @@ -3337,46 +3425,68 @@ def _scan_directory_for_scripts( ) @staticmethod - def _get_validated_resource_path(skill_dir: str, resource_name: str) -> str: + def _get_validated_resource_path(scope: _SkillPathScope, resource_name: str) -> str: """Resolve and validate a resource file path within a skill directory. - Normalizes *resource_name*, resolves it against *skill_dir*, and - validates that the result stays within the skill directory and does + Normalizes *resource_name*, resolves it against the scope's skill directory, + and validates that the result stays within the skill directory and does not traverse any symlinks. Args: - skill_dir: Absolute path to the owning skill directory. + scope: Trusted path scope of the owning skill. resource_name: Relative path of the resource within the skill directory. Returns: The validated absolute path to the resource file. Raises: - ValueError: If *skill_dir* is not an absolute path, the resolved path + ValueError: If the scope's paths are not absolute, the resolved path escapes the skill directory, the file does not exist, or a symlink is detected in the path. """ - if not os.path.isabs(skill_dir): - raise ValueError(f"skill_dir must be an absolute path, got: '{skill_dir}'") - resource_name = FileSkillsSource._normalize_resource_path(resource_name) + resource_full_path = os.path.normpath(Path(scope.skill_dir) / resource_name) + return FileSkillsSource._validate_file_path_for_use( + scope, + resource_full_path, + resource_name, + "Resource", + ) - resource_full_path = os.path.normpath(Path(skill_dir) / resource_name) - root_directory_path = os.path.normpath(skill_dir) - - if not FileSkillsSource._is_path_within_directory(resource_full_path, root_directory_path): - raise ValueError(f"Resource file '{resource_name}' references a path outside the skill directory.") - - 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_link_or_reparse_point_in_path(resource_full_path, root_directory_path): + @staticmethod + def _validate_file_path_for_use(scope: _SkillPathScope, full_path: str, file_name: str, file_kind: str) -> str: + """Validate a discovered file immediately before it is read or executed.""" + # Require anchored trust boundaries, e.g. "/skills/weather", not "skills/weather". + if not os.path.isabs(scope.skill_dir): + raise ValueError(f"skill_dir must be an absolute path, got: '{scope.skill_dir}'") + if not os.path.isabs(scope.trusted_root): + raise ValueError(f"trusted_root must be an absolute path, got: '{scope.trusted_root}'") + + # Collapse lexical segments, e.g. "/skills/weather/refs/../guide.md" -> "/skills/weather/guide.md". + normalized_full_path = os.path.normpath(full_path) + skill_directory_path = os.path.normpath(scope.skill_dir) + trusted_root_path = os.path.normpath(scope.trusted_root) + + # Reject lexical escapes, e.g. "/skills/weather/../secret.md" resolves outside the skill root. + if not FileSkillsSource._is_path_within_directory(normalized_full_path, skill_directory_path): + raise ValueError(f"{file_kind} file '{file_name}' references a path outside the skill directory.") + + # The skill directory sitting at or beneath the configured root is a scope invariant, + # so the file is transitively within the root and the scan below is well-anchored. + + # Reject files deleted or replaced with non-files after discovery. + if not Path(normalized_full_path).is_file(): + raise ValueError(f"{file_kind} file '{file_name}' not found in skill directory '{scope.skill_dir}'.") + + # Reject links in any segment below the configured root, e.g. the skill directory + # "weather" or the child segment "refs" in "/skills/weather/refs/guide.md". + if FileSkillsSource._has_link_or_reparse_point_in_path(normalized_full_path, trusted_root_path): raise ValueError( - f"Resource file '{resource_name}' has a symbolic link or reparse point in its path; " + f"{file_kind} file '{file_name}' has a symbolic link or reparse point in its path; " "links and reparse points are not allowed." ) - return resource_full_path + return normalized_full_path @staticmethod def _validate_skill_metadata( @@ -3542,8 +3652,8 @@ def _read_and_parse_skill_file( return frontmatter, content @staticmethod - def _discover_skill_directories(skill_paths: Sequence[str]) -> list[str]: - """Return absolute paths of all directories that contain a ``SKILL.md`` file. + def _discover_skill_directories(skill_paths: Sequence[str]) -> list[_SkillPathScope]: + """Return the path scopes of all directories that contain a ``SKILL.md`` file. Recursively searches each root path up to :data:`MAX_SEARCH_DEPTH`. Once a ``SKILL.md`` is found in a directory, that directory is the skill root and the @@ -3563,9 +3673,10 @@ def _discover_skill_directories(skill_paths: Sequence[str]) -> list[str]: skill_paths: Root directory paths to search. Returns: - Absolute paths to directories containing ``SKILL.md``. + One :class:`_SkillPathScope` per directory containing ``SKILL.md``, each pairing + the configured root it was found under with the skill directory itself. """ - discovered: list[str] = [] + discovered: list[_SkillPathScope] = [] def _is_unsafe_link(path: Path) -> bool: try: @@ -3573,7 +3684,7 @@ def _is_unsafe_link(path: Path) -> bool: except OSError: return True - def _search(directory: str, current_depth: int) -> None: + def _search(directory: str, trusted_root: str, current_depth: int) -> None: dir_path = Path(directory) skill_file = dir_path / SKILL_FILE_NAME if skill_file.is_file(): @@ -3587,7 +3698,7 @@ def _search(directory: str, current_depth: int) -> None: SKILL_FILE_NAME, ) return - discovered.append(str(dir_path.absolute())) + discovered.append(_SkillPathScope(trusted_root=trusted_root, skill_dir=str(dir_path.absolute()))) return if current_depth >= MAX_SEARCH_DEPTH: @@ -3607,12 +3718,12 @@ def _search(directory: str, current_depth: int) -> None: ) continue if entry.is_dir(): - _search(str(entry), current_depth + 1) + _search(str(entry), trusted_root, current_depth + 1) for root_dir in skill_paths: if not root_dir or not root_dir.strip() or not Path(root_dir).is_dir(): continue - _search(root_dir, current_depth=0) + _search(root_dir, str(Path(root_dir).absolute()), current_depth=0) return discovered diff --git a/python/packages/core/tests/core/test_skills.py b/python/packages/core/tests/core/test_skills.py index 237b723841b..94b98f2114e 100644 --- a/python/packages/core/tests/core/test_skills.py +++ b/python/packages/core/tests/core/test_skills.py @@ -5,6 +5,7 @@ from __future__ import annotations import os +import shutil from abc import ABC from collections.abc import Sequence from datetime import timedelta @@ -45,6 +46,7 @@ _create_resource_element, _create_script_element, _FileSkillResource, + _SkillPathScope, ) from .conftest import MockAgent, MockAgentSession, create_junction_or_skip @@ -167,6 +169,16 @@ def _write_skill( return skill_dir +def _scope(skill_dir: str | Path, trusted_root: str | Path | None = None) -> _SkillPathScope: + """Build a path scope, defaulting the trusted root to the skill directory itself.""" + return _SkillPathScope(str(trusted_root if trusted_root is not None else skill_dir), str(skill_dir)) + + +def _discovered_skill_dirs(skill_paths: list[str]) -> list[str]: + """Discover skills and return just the skill directory paths.""" + return [scope.skill_dir for scope in FileSkillsSource._discover_skill_directories(skill_paths)] + + def _read_and_parse_skill_file_for_test(skill_dir: Path) -> FileSkill: """Parse a SKILL.md file from the given directory, raising if invalid.""" result = FileSkillsSource._read_and_parse_skill_file(str(skill_dir)) @@ -979,6 +991,47 @@ async def test_discover_skips_symlinked_resource(self, tmp_path: Path) -> None: assert "references/leak.md" not in resource_names assert "references/safe.md" in resource_names + async def test_read_rejects_resource_replaced_with_symlink(self, tmp_path: Path) -> None: + """A resource replaced after discovery must be revalidated before reading.""" + skill_dir = _write_skill( + tmp_path, + "my-skill", + resources={"references/guide.md": "safe content"}, + ) + outside_file = tmp_path / "secret.md" + outside_file.write_text("secret content", encoding="utf-8") + skills = await _discover_file_skills_for_test([str(tmp_path)]) + resource = next(r for r in skills["my-skill"]._resources if r.name == "references/guide.md") + + resource_path = skill_dir / "references" / "guide.md" + resource_path.unlink() + resource_path.symlink_to(outside_file) + + with pytest.raises(ValueError, match="symbolic link or reparse point"): + await resource.read() + + async def test_read_rejects_skill_directory_replaced_with_symlink(self, tmp_path: Path) -> None: + """A skill directory replaced after discovery must be revalidated before reading.""" + if not _symlinks_supported(tmp_path): + pytest.skip("Symlinks not supported on this platform/environment") + + root = tmp_path / "root" + root.mkdir() + skill_dir = _write_skill(root, "my-skill", resources={"guide.md": "safe content"}) + skills = await _discover_file_skills_for_test([str(root)]) + resource = next(r for r in skills["my-skill"]._resources if r.name == "guide.md") + + # Swap the whole skill directory for a link to an attacker-controlled directory + # that mirrors the discovered layout. + decoy = tmp_path / "decoy" + decoy.mkdir() + (decoy / "guide.md").write_text("attacker content", encoding="utf-8") + shutil.rmtree(skill_dir) + skill_dir.symlink_to(decoy, target_is_directory=True) + + with pytest.raises(ValueError, match="symbolic link or reparse point"): + await resource.read() + def test_discover_resource_files_rejects_symlinked_resource(self, tmp_path: Path) -> None: """_discover_resource_files should exclude a symlinked resource file.""" skill_dir = tmp_path / "skill" @@ -1014,6 +1067,31 @@ 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_run_rejects_script_replaced_with_symlink(self, tmp_path: Path) -> None: + """A script replaced after discovery must be revalidated before its runner is invoked.""" + skill_dir = _write_skill(tmp_path, "my-skill") + script_path = skill_dir / "scripts" / "run.py" + script_path.parent.mkdir() + script_path.write_text("print('safe')", encoding="utf-8") + outside_script = tmp_path / "outside.py" + outside_script.write_text("print('outside')", encoding="utf-8") + runner_called = False + + def runner(skill: Skill, script: SkillScript, args: dict[str, Any] | list[str] | None = None) -> None: + nonlocal runner_called + runner_called = True + + skills = await _discover_file_skills_for_test([str(tmp_path)], script_runner=runner) + skill = skills["my-skill"] + script = next(s for s in skill._scripts if s.name == "scripts/run.py") + + script_path.unlink() + script_path.symlink_to(outside_script) + + with pytest.raises(ValueError, match="symbolic link or reparse point"): + await script.run(skill) + assert runner_called is False + 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" @@ -1025,7 +1103,7 @@ async def test_discover_skips_symlinked_skill_directory(self, tmp_path: Path) -> (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())] + assert _discovered_skill_dirs([str(root)]) == [str((root / "good-skill").absolute())] skills = await _discover_file_skills_for_test([str(root)]) assert "evil-skill" not in skills @@ -1045,7 +1123,7 @@ def test_discover_skips_directory_with_symlinked_skill_file(self, tmp_path: Path evil_dir.mkdir() (evil_dir / "SKILL.md").symlink_to(outside_skill_file) - assert FileSkillsSource._discover_skill_directories([str(root)]) == [] + assert _discovered_skill_dirs([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.""" @@ -1058,7 +1136,7 @@ def test_discover_keeps_nested_real_skill_directories(self, tmp_path: Path) -> N outside.mkdir() (root / "linked").symlink_to(outside, target_is_directory=True) - assert FileSkillsSource._discover_skill_directories([str(root)]) == [str((nested / "nested-skill").absolute())] + assert _discovered_skill_dirs([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.""" @@ -1069,9 +1147,7 @@ def test_configured_root_may_itself_be_a_link(self, tmp_path: Path) -> None: 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()) - ] + assert _discovered_skill_dirs([str(linked_root)]) == [str((linked_root / "my-skill").absolute())] class TestJunctionDetection: @@ -1093,7 +1169,7 @@ def test_junction_is_detected_and_excluded(self, tmp_path: Path) -> None: 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") + FileSkillsSource._get_validated_resource_path(_scope(skill_dir), "linked/leak.md") finally: junction.rmdir() @@ -1110,7 +1186,7 @@ async def test_junctioned_skill_directory_is_not_discovered(self, tmp_path: Path create_junction_or_skip(link=junction, target=outside / "evil-skill") try: - assert FileSkillsSource._discover_skill_directories([str(root)]) == [str((root / "good-skill").absolute())] + assert _discovered_skill_dirs([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 @@ -1130,7 +1206,7 @@ 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)]) == [] + assert _discovered_skill_dirs([str(root)]) == [] def test_skill_file_that_cannot_be_inspected_is_skipped(self, tmp_path: Path) -> None: root = tmp_path / "root" @@ -1143,7 +1219,7 @@ def _raise_for_skill_file(path: Path) -> bool: 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)]) == [] + assert _discovered_skill_dirs([str(root)]) == [] # --------------------------------------------------------------------------- @@ -2135,14 +2211,14 @@ class TestDiscoverSkillDirectories: def test_finds_skill_at_root(self, tmp_path: Path) -> None: (tmp_path / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n", encoding="utf-8") - dirs = FileSkillsSource._discover_skill_directories([str(tmp_path)]) + dirs = _discovered_skill_dirs([str(tmp_path)]) assert len(dirs) == 1 def test_finds_nested_skill(self, tmp_path: Path) -> None: sub = tmp_path / "sub" sub.mkdir() (sub / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n", encoding="utf-8") - dirs = FileSkillsSource._discover_skill_directories([str(tmp_path)]) + dirs = _discovered_skill_dirs([str(tmp_path)]) assert len(dirs) == 1 assert str(sub.absolute()) in dirs[0] @@ -2153,30 +2229,30 @@ def test_stops_searching_below_skill_boundary(self, tmp_path: Path) -> None: (skill_dir / "SKILL.md").write_text("---\nname: parent-skill\ndescription: d\n---\n", encoding="utf-8") (nested_skill_dir / "SKILL.md").write_text("---\nname: nested-skill\ndescription: d\n---\n", encoding="utf-8") - dirs = FileSkillsSource._discover_skill_directories([str(tmp_path)]) + dirs = _discovered_skill_dirs([str(tmp_path)]) assert dirs == [str(skill_dir.absolute())] def test_skips_empty_path_string(self) -> None: - dirs = FileSkillsSource._discover_skill_directories(["", " "]) + dirs = _discovered_skill_dirs(["", " "]) assert dirs == [] def test_skips_nonexistent_path(self) -> None: - dirs = FileSkillsSource._discover_skill_directories(["/nonexistent/does/not/exist"]) + dirs = _discovered_skill_dirs(["/nonexistent/does/not/exist"]) assert dirs == [] def test_depth_limit_excludes_deep_skill(self, tmp_path: Path) -> None: deep = tmp_path / "l1" / "l2" / "l3" deep.mkdir(parents=True) (deep / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n", encoding="utf-8") - dirs = FileSkillsSource._discover_skill_directories([str(tmp_path)]) + dirs = _discovered_skill_dirs([str(tmp_path)]) assert len(dirs) == 0 def test_depth_limit_includes_at_boundary(self, tmp_path: Path) -> None: at_boundary = tmp_path / "l1" / "l2" at_boundary.mkdir(parents=True) (at_boundary / "SKILL.md").write_text("---\nname: s\ndescription: d\n---\n", encoding="utf-8") - dirs = FileSkillsSource._discover_skill_directories([str(tmp_path)]) + dirs = _discovered_skill_dirs([str(tmp_path)]) assert len(dirs) == 1 @@ -2289,12 +2365,12 @@ def test_returns_valid_path(self, tmp_path: Path) -> None: skill_dir = tmp_path / "skill" skill_dir.mkdir() (skill_dir / "doc.md").write_text("hello") - result = FileSkillsSource._get_validated_resource_path(str(skill_dir), "doc.md") + result = FileSkillsSource._get_validated_resource_path(_scope(skill_dir), "doc.md") assert Path(result).is_file() def test_rejects_relative_skill_dir(self) -> None: with pytest.raises(ValueError, match="skill_dir must be an absolute path"): - FileSkillsSource._get_validated_resource_path("relative/path", "doc.md") + FileSkillsSource._get_validated_resource_path(_scope("relative/path"), "doc.md") def test_rejects_path_outside_skill_dir(self, tmp_path: Path) -> None: skill_dir = tmp_path / "skill" @@ -2302,13 +2378,13 @@ def test_rejects_path_outside_skill_dir(self, tmp_path: Path) -> None: outside_file = tmp_path / "secret.md" outside_file.write_text("secret") with pytest.raises(ValueError, match="outside the skill directory"): - FileSkillsSource._get_validated_resource_path(str(skill_dir), "../secret.md") + FileSkillsSource._get_validated_resource_path(_scope(skill_dir), "../secret.md") def test_rejects_nonexistent_file(self, tmp_path: Path) -> None: skill_dir = tmp_path / "skill" skill_dir.mkdir() with pytest.raises(ValueError, match="not found"): - FileSkillsSource._get_validated_resource_path(str(skill_dir), "missing.md") + FileSkillsSource._get_validated_resource_path(_scope(skill_dir), "missing.md") @pytest.mark.skipif(os.name == "nt", reason="symlinks require elevated privileges on Windows") def test_rejects_symlink_in_path(self, tmp_path: Path) -> None: @@ -2320,7 +2396,7 @@ def test_rejects_symlink_in_path(self, tmp_path: Path) -> None: link = skill_dir / "linked" link.symlink_to(real_subdir) with pytest.raises(ValueError, match="symbolic link or reparse point"): - FileSkillsSource._get_validated_resource_path(str(skill_dir), "linked/data.md") + FileSkillsSource._get_validated_resource_path(_scope(skill_dir), "linked/data.md") # --------------------------------------------------------------------------- @@ -3307,6 +3383,44 @@ def test_full_path_rejects_empty(self) -> None: with pytest.raises(ValueError, match="cannot be empty"): FileSkillScript(name="run.py", full_path="") + def test_skill_dir_remains_supported(self) -> None: + script = FileSkillScript( + name="run.py", + full_path=f"{_ABS}/test/run.py", + skill_dir=f"{_ABS}/test", + ) + + assert script.full_path == f"{_ABS}/test/run.py" + # Without a trusted root, revalidation must stay anchored at the skill directory. + assert script._scope == _SkillPathScope(trusted_root=f"{_ABS}/test", skill_dir=f"{_ABS}/test") + + def test_trusted_root_is_retained_alongside_skill_dir(self) -> None: + script = FileSkillScript( + name="run.py", + full_path=f"{_ABS}/root/test/run.py", + skill_dir=f"{_ABS}/root/test", + trusted_root=f"{_ABS}/root", + ) + + assert script._scope == _SkillPathScope(trusted_root=f"{_ABS}/root", skill_dir=f"{_ABS}/root/test") + + def test_trusted_root_without_skill_dir_raises(self) -> None: + with pytest.raises(ValueError, match="trusted_root requires skill_dir"): + FileSkillScript( + name="run.py", + full_path=f"{_ABS}/test/run.py", + trusted_root=_ABS, + ) + + def test_skill_dir_outside_trusted_root_raises(self) -> None: + with pytest.raises(ValueError, match="must reside at or beneath trusted_root"): + FileSkillScript( + name="run.py", + full_path=f"{_ABS}/elsewhere/run.py", + skill_dir=f"{_ABS}/elsewhere", + trusted_root=f"{_ABS}/root", + ) + # --------------------------------------------------------------------------- # @skill.script decorator tests