[dotnet-port-fixes] Revalidate file skill paths before use

agent/skills/fsskills/path_scope.go

@@ -0,0 +1,88 @@
+// Copyright (c) Microsoft. All rights reserved.
+
+package fsskills
+
+import (
+	"errors"
+	"fmt"
+	"io/fs"
+	"path"
+	"strings"
+)
+
+type skillPathScope struct {
+	rootFS         fs.FS
+	skillDirPath   string
+	skillDirPrefix string
+}
+
+func newSkillPathScope(rootFS fs.FS, skillDirPath string) skillPathScope {
+	skillDirPath = path.Clean(skillDirPath)
+	skillDirPrefix := ""
+	if skillDirPath != "." {
+		skillDirPrefix = skillDirPath + "/"
+	}
+	return skillPathScope{
+		rootFS:         rootFS,
+		skillDirPath:   skillDirPath,
+		skillDirPrefix: skillDirPrefix,
+	}
+}
+
+func (s skillPathScope) validateDiscoveredPathForUse(relativePath, kind string) (string, error) {
+	fullPath := path.Clean(relativePath)
+	if s.skillDirPath != "." {
+		fullPath = path.Clean(path.Join(s.skillDirPath, relativePath))
+	}
+	if !s.contains(fullPath) {
+		return "", fmt.Errorf("%s file %q references a path outside the skill directory", kind, relativePath)
+	}
+	if hasLinkOrInspectionFailureInPath(s.rootFS, fullPath) {
+		return "", fmt.Errorf("%s file %q has a symbolic link or inspection failure in its path; symbolic links are not allowed", kind, relativePath)
+	}
+	if _, err := fs.Stat(s.rootFS, fullPath); err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
+			return "", fmt.Errorf("%s file %q was not found in the skill directory", kind, relativePath)
+		}
+		return "", err
+	}
+	return fullPath, nil
+}
+
+func (s skillPathScope) contains(fullPath string) bool {
+	if s.skillDirPath == "." {
+		return fullPath != ".." && !strings.HasPrefix(fullPath, "../")
+	}
+	return fullPath == s.skillDirPath || strings.HasPrefix(fullPath, s.skillDirPrefix)
+}
+
+func hasLinkOrInspectionFailureInPath(filesystem fs.FS, pathToCheck string) bool {
+	currentPath := ""
+	for _, segment := range strings.Split(path.Clean(pathToCheck), "/") {
+		if segment == "" || segment == "." {
+			continue
+		}
+		if currentPath == "" {
+			currentPath = segment
+		} else {
+			currentPath = path.Join(currentPath, segment)
+		}
+		if isUnsafePath(filesystem, currentPath) {
+			return true
+		}
+	}
+	return false
+}
+
+func isUnsafePath(filesystem fs.FS, filePath string) bool {
+	readLinkFS, ok := filesystem.(fs.ReadLinkFS)
+	if !ok {
+		return true
+	}
+
+	info, err := readLinkFS.Lstat(filePath)
+	if err == nil {
+		return info.Mode()&fs.ModeSymlink != 0
+	}
+	return !errors.Is(err, fs.ErrNotExist)
+}

agent/skills/fsskills/source.go

@@ -189,7 +189,7 @@ func (s *Source) Skills(ctx context.Context) ([]*skills.Skill, error) {
 		if err := ctx.Err(); err != nil {
 			return nil, err
 		}
-		skill := s.parseSkillDirectory(directory.fsys, directory.path)
+		skill := s.parseSkillDirectory(directory)
 		if skill == nil {
 			continue
 		}
@@ -237,7 +237,7 @@ func searchForSkills(filesystem fs.FS, dir string, logger *slog.Logger, results
 			sub, subErr = fs.Sub(filesystem, dir)
 		}
 		if subErr == nil {
-			*results = append(*results, discoveredSkillDir{fsys: sub, path: dir})
+			*results = append(*results, discoveredSkillDir{rootFS: filesystem, skillFS: sub, path: dir})
 		}
 		return
 	}
@@ -258,7 +258,10 @@ func searchForSkills(filesystem fs.FS, dir string, logger *slog.Logger, results
 	}
 }
 
-func (s *Source) parseSkillDirectory(skillFS fs.FS, logPath string) *skills.Skill {
+func (s *Source) parseSkillDirectory(directory discoveredSkillDir) *skills.Skill {
+	scope := newSkillPathScope(directory.rootFS, directory.path)
+	skillFS := directory.skillFS
+	logPath := directory.path
 	data, err := fs.ReadFile(skillFS, skillFileName)
 	if err != nil {
 		s.logger.Error("Failed to read SKILL.md", "path", logPath, "error", err)
@@ -271,8 +274,8 @@ func (s *Source) parseSkillDirectory(skillFS fs.FS, logPath string) *skills.Skil
 		return nil
 	}
 
-	resources := s.discoverResourceFiles(skillFS, frontmatter.Name)
-	scripts := s.discoverScriptFiles(skillFS, frontmatter.Name)
+	resources := s.discoverResourceFiles(skillFS, scope, frontmatter.Name)
+	scripts := s.discoverScriptFiles(skillFS, scope, frontmatter.Name)
 	var (
 		contentOnce   sync.Once
 		cachedContent string
@@ -467,7 +470,7 @@ func leadingWhitespaceCount(line string) int {
 	return count
 }
 
-func (s *Source) discoverResourceFiles(skillFS fs.FS, skillName string) []skills.Resource {
+func (s *Source) discoverResourceFiles(skillFS fs.FS, scope skillPathScope, skillName string) []skills.Resource {
 	seen := make(map[string]bool)
 	var resources []skills.Resource
 	s.scanForFiles(skillFS, ".", skillName, 1, s.allowedResourceExtensions, s.resourceFilter, "resource", func(filePath string) {
@@ -482,7 +485,11 @@ func (s *Source) discoverResourceFiles(skillFS fs.FS, skillName string) []skills
 		resources = append(resources, skills.Resource{
 			Name: filePath,
 			Read: func(context.Context) (any, error) {
-				data, err := fs.ReadFile(skillFS, filePath)
+				validatedPath, err := scope.validateDiscoveredPathForUse(filePath, "resource")
+				if err != nil {
+					return nil, err
+				}
+				data, err := fs.ReadFile(scope.rootFS, validatedPath)
 				if err != nil {
 					return nil, err
 				}
@@ -493,7 +500,7 @@ func (s *Source) discoverResourceFiles(skillFS fs.FS, skillName string) []skills
 	return resources
 }
 
-func (s *Source) discoverScriptFiles(skillFS fs.FS, skillName string) []skills.Script {
+func (s *Source) discoverScriptFiles(skillFS fs.FS, scope skillPathScope, skillName string) []skills.Script {
 	seen := make(map[string]bool)
 	var scripts []skills.Script
 	s.scanForFiles(skillFS, ".", skillName, 1, s.allowedScriptExtensions, s.scriptFilter, "script", func(filePath string) {
@@ -504,7 +511,7 @@ func (s *Source) discoverScriptFiles(skillFS fs.FS, skillName string) []skills.S
 			return
 		}
 		seen[filePath] = true
-		scripts = append(scripts, newScript(filePath, skillFS, s.scriptRunner))
+		scripts = append(scripts, newScript(filePath, skillFS, scope, s.scriptRunner))
 	})
 	return scripts
 }
@@ -609,26 +616,29 @@ func validateExtensions(extensions []string) {
 	}
 }
 
-func newScript(name string, fsys fs.FS, runner skills.ScriptRunner) skills.Script {
+func newScript(name string, fsys fs.FS, scope skillPathScope, runner skills.ScriptRunner) skills.Script {
 	additionalProperties := map[string]any{
 		"fsskills.scriptFS": fsys,
 	}
 	return skills.Script{
 		Name:                 name,
 		ParametersSchema:     defaultFileScriptSchema,
-		Run:                  newFileScriptRunFunc(name, runner, additionalProperties),
+		Run:                  newFileScriptRunFunc(name, scope, runner, additionalProperties),
 		AdditionalProperties: additionalProperties,
 	}
 }
 
-func newFileScriptRunFunc(name string, runner skills.ScriptRunner, additionalProperties map[string]any) func(context.Context, *skills.Skill, []string) (any, error) {
+func newFileScriptRunFunc(name string, scope skillPathScope, runner skills.ScriptRunner, additionalProperties map[string]any) func(context.Context, *skills.Skill, []string) (any, error) {
 	return func(ctx context.Context, owner *skills.Skill, arguments []string) (any, error) {
 		if err := requireFileSkill(name, owner); err != nil {
 			return nil, err
 		}
 		if runner == nil {
 			return nil, fmt.Errorf("script %q cannot be executed because no file script runner was provided", name)
 		}
+		if _, err := scope.validateDiscoveredPathForUse(name, "script"); err != nil {
+			return nil, err
+		}
 		// Hand the runner a script carrying the same metadata the discovered
 		// Script exposes (parameters schema and the backing fs.FS), so runners
 		// that inspect them to locate/execute the file see the real values
@@ -650,8 +660,9 @@ func requireFileSkill(scriptName string, skill *skills.Skill) error {
 }
 
 type discoveredSkillDir struct {
-	fsys fs.FS
-	path string
+	rootFS  fs.FS
+	skillFS fs.FS
+	path    string
 }
 
 func buildAvailableResourcesBlock(resources []skills.Resource) string {

agent/skills/fsskills/source_script_test.go

@@ -156,6 +156,67 @@ func TestFileSource_WithRunner_ScriptsCanRun(t *testing.T) {
 	}
 }
 
+func TestFileSource_ScriptExecution_RevalidatesParentDirectoriesBeforeRun(t *testing.T) {
+	root := t.TempDir()
+	createSkillDir(t, filepath.Join(root, "trusted"), "exec-skill", "Executor test", "Body.")
+	createRelativeFile(t, filepath.Join(root, "trusted", "exec-skill"), "scripts/test.py", "print('trusted')")
+	createSkillDir(t, filepath.Join(root, "outside", "trusted"), "exec-skill", "Executor test", "Body.")
+	createRelativeFile(t, filepath.Join(root, "outside", "trusted", "exec-skill"), "scripts/test.py", "print('outside')")
+
+	runnerCalled := false
+	source := fsskills.NewSourceOptions(fsskills.SourceOptions{ScriptRunner: func(_ context.Context, _ *skills.Skill, _ *skills.Script, _ []string) (any, error) {
+		runnerCalled = true
+		return "executed", nil
+	}}, os.DirFS(root))
+
+	loaded, err := source.Skills(t.Context())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(loaded) != 1 || len(loaded[0].Scripts) != 1 {
+		t.Fatalf("expected one skill with one script, got %d skills and %d scripts", len(loaded), len(loaded[0].Scripts))
+	}
+
+	if err := os.Rename(filepath.Join(root, "trusted"), filepath.Join(root, "trusted-real")); err != nil {
+		t.Fatal(err)
+	}
+	createSymlink(t, filepath.Join(root, "trusted"), filepath.Join(root, "outside", "trusted"))
+
+	_, err = loaded[0].Scripts[0].Run(t.Context(), loaded[0], nil)
+	if err == nil {
+		t.Fatal("expected script run to fail after the discovered path was replaced with a symlink")
+	}
+	if runnerCalled {
+		t.Fatal("expected script runner not to be called after path revalidation failed")
+	}
+}
+
+func TestFileSource_ScriptExecution_FailsWithoutLinkInspection(t *testing.T) {
+	runnerCalled := false
+	source := fsskills.NewSourceOptions(fsskills.SourceOptions{ScriptRunner: func(_ context.Context, _ *skills.Skill, _ *skills.Script, _ []string) (any, error) {
+		runnerCalled = true
+		return "executed", nil
+	}}, fsWithoutLinkInspection{fstest.MapFS{
+		"exec-skill/SKILL.md":        {Data: []byte("---\nname: exec-skill\ndescription: A skill\n---\nBody.")},
+		"exec-skill/scripts/test.py": {Data: []byte("print('test')")},
+	}})
+
+	loaded, err := source.Skills(t.Context())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(loaded) != 1 || len(loaded[0].Scripts) != 1 {
+		t.Fatalf("expected one skill with one script, got %d skills and %d scripts", len(loaded), len(loaded[0].Scripts))
+	}
+
+	if _, err := loaded[0].Scripts[0].Run(t.Context(), loaded[0], nil); err == nil {
+		t.Fatal("expected script run to fail when the filesystem does not support link inspection")
+	}
+	if runnerCalled {
+		t.Fatal("expected script runner not to be called when link inspection is unavailable")
+	}
+}
+
 func TestFileSource_NullRunner_DoesNotPanic(t *testing.T) {
 	_ = fsskills.NewSource(os.DirFS(t.TempDir()))
 }

agent/skills/fsskills/source_test.go

@@ -3,14 +3,20 @@
 package fsskills_test
 
 import (
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
+	"testing/fstest"
 
 	"github.com/microsoft/agent-framework-go/agent/skills/fsskills"
 )
 
+type fsWithoutLinkInspection struct {
+	fs.FS
+}
+
 func TestFileSource_EmptyPaths_ReturnsEmptyList(t *testing.T) {
 	source := fsskills.NewSource()
 
@@ -300,6 +306,50 @@ func TestFileSource_ReadResource_ValidResource_ReturnsContent(t *testing.T) {
 	}
 }
 
+func TestFileSource_ReadResource_RevalidatesParentDirectoriesBeforeUse(t *testing.T) {
+	root := t.TempDir()
+	createSkillDirWithResource(t, filepath.Join(root, "trusted"), "read-skill", "A skill", "See docs.", "references/doc.md", "trusted content")
+	createSkillDirWithResource(t, filepath.Join(root, "outside", "trusted"), "read-skill", "A skill", "See docs.", "references/doc.md", "outside content")
+
+	source := fsskills.NewSource(os.DirFS(root))
+	loaded, err := source.Skills(t.Context())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(loaded) != 1 || len(loaded[0].Resources) != 1 {
+		t.Fatalf("expected one skill with one resource, got %d skills and %d resources", len(loaded), len(loaded[0].Resources))
+	}
+
+	if err := os.Rename(filepath.Join(root, "trusted"), filepath.Join(root, "trusted-real")); err != nil {
+		t.Fatal(err)
+	}
+	createSymlink(t, filepath.Join(root, "trusted"), filepath.Join(root, "outside", "trusted"))
+
+	_, err = loaded[0].Resources[0].Read(t.Context())
+	if err == nil {
+		t.Fatal("expected resource read to fail after the discovered path was replaced with a symlink")
+	}
+}
+
+func TestFileSource_ReadResource_FailsWithoutLinkInspection(t *testing.T) {
+	source := fsskills.NewSource(fsWithoutLinkInspection{fstest.MapFS{
+		"read-skill/SKILL.md":          {Data: []byte("---\nname: read-skill\ndescription: A skill\n---\nSee docs.")},
+		"read-skill/references/doc.md": {Data: []byte("content")},
+	}})
+
+	loaded, err := source.Skills(t.Context())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(loaded) != 1 || len(loaded[0].Resources) != 1 {
+		t.Fatalf("expected one skill with one resource, got %d skills and %d resources", len(loaded), len(loaded[0].Resources))
+	}
+
+	if _, err := loaded[0].Resources[0].Read(t.Context()); err == nil {
+		t.Fatal("expected resource read to fail when the filesystem does not support link inspection")
+	}
+}
+
 func TestFileSource_MetadataWithQuotedValues_ParsedCorrectly(t *testing.T) {
 	root := t.TempDir()
 	createSkillDirRaw(t, root, "quoted-meta", strings.Join([]string{