From cf9b0e7d30a73b4821031e603af00f169fd09885 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 11 Aug 2026 03:15:38 +0000 Subject: [PATCH] agent/skills/fsskills: harden file skill discovery Port .NET file-skill discovery hardening so symlinked skill files, resources, and scripts are skipped while a symlinked configured root remains supported. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- agent/skills/fsskills/source.go | 63 ++++++++++++++---- agent/skills/fsskills/source_script_test.go | 26 ++++++++ agent/skills/fsskills/source_test.go | 74 +++++++++++++++++++++ 3 files changed, 151 insertions(+), 12 deletions(-) diff --git a/agent/skills/fsskills/source.go b/agent/skills/fsskills/source.go index 95e01f58..d714965c 100644 --- a/agent/skills/fsskills/source.go +++ b/agent/skills/fsskills/source.go @@ -170,7 +170,7 @@ func NewSourceOptions(opts SourceOptions, filesystems ...fs.FS) *Source { // Skills discovers and loads valid skills from the configured filesystems. func (s *Source) Skills(ctx context.Context) ([]*skills.Skill, error) { - directories := discoverSkillDirectories(s.filesystems) + directories := discoverSkillDirectories(s.filesystems, s.logger) s.logger.Info("Discovered potential skills", "count", len(directories)) skills := make([]*skills.Skill, 0, len(directories)) @@ -190,10 +190,10 @@ func (s *Source) Skills(ctx context.Context) ([]*skills.Skill, error) { return skills, nil } -func discoverSkillDirectories(filesystems []fs.FS) []discoveredSkillDir { +func discoverSkillDirectories(filesystems []fs.FS, logger *slog.Logger) []discoveredSkillDir { var results []discoveredSkillDir for _, filesystem := range filesystems { - searchForSkills(filesystem, ".", &results, 0) + searchForSkills(filesystem, ".", logger, &results, 0) } return results } @@ -203,9 +203,23 @@ func discoverSkillDirectories(filesystems []fs.FS) []discoveredSkillDir { // independent of SourceOptions.SearchDepth, which governs only resource and // script discovery within an already-discovered skill directory. This matches // the .NET SDK, which bounds the two concerns separately. -func searchForSkills(filesystem fs.FS, dir string, results *[]discoveredSkillDir, currentDepth int) { - skillPath := path.Join(dir, skillFileName) - if _, err := fs.Stat(filesystem, skillPath); err == nil { +func searchForSkills(filesystem fs.FS, dir string, logger *slog.Logger, results *[]discoveredSkillDir, currentDepth int) { + entries, err := fs.ReadDir(filesystem, dir) + if err != nil { + return + } + + for _, entry := range entries { + if entry.Name() != skillFileName { + continue + } + + skillPath := path.Join(dir, entry.Name()) + if isUnsafeDirEntry(filesystem, skillPath, entry) { + logger.Warn("Skipping skill discovery path: symbolic link or inspection failure", "path", skillPath) + return + } + sub := filesystem var subErr error if dir != "." { @@ -213,19 +227,22 @@ func searchForSkills(filesystem fs.FS, dir string, results *[]discoveredSkillDir } if subErr == nil { *results = append(*results, discoveredSkillDir{fsys: sub, path: dir}) - return } - } - if currentDepth >= defaultSearchDepth { return } - entries, err := fs.ReadDir(filesystem, dir) - if err != nil { + + if currentDepth >= defaultSearchDepth { return } + for _, entry := range entries { + entryPath := path.Join(dir, entry.Name()) + if isUnsafeDirEntry(filesystem, entryPath, entry) { + logger.Warn("Skipping skill discovery path: symbolic link or inspection failure", "path", entryPath) + continue + } if entry.IsDir() { - searchForSkills(filesystem, path.Join(dir, entry.Name()), results, currentDepth+1) + searchForSkills(filesystem, entryPath, logger, results, currentDepth+1) } } } @@ -502,6 +519,10 @@ func (s *Source) scanForFiles( for _, entry := range entries { entryPath := path.Join(dir, entry.Name()) + if isUnsafeDirEntry(skillFS, entryPath, entry) { + s.logger.Warn("Skipping file skill path: symbolic link or inspection failure", "skillName", skillName, "filePath", entryPath, "kind", kind) + continue + } if entry.IsDir() { if currentDepth < s.searchDepth { s.scanForFiles(skillFS, entryPath, skillName, currentDepth+1, allowedExtensions, filter, kind, collect) @@ -534,6 +555,24 @@ func (s *Source) scanForFiles( } } +func isUnsafeDirEntry(filesystem fs.FS, filePath string, entry fs.DirEntry) bool { + if entry.Type()&fs.ModeSymlink != 0 { + return true + } + + readLinkFS, ok := filesystem.(fs.ReadLinkFS) + if !ok { + return false + } + + info, err := readLinkFS.Lstat(filePath) + if err == nil { + return info.Mode()&fs.ModeSymlink != 0 + } + + return !errors.Is(err, fs.ErrNotExist) +} + func buildExtensionSet(extensions []string, defaults []string) map[string]bool { if extensions == nil { extensions = defaults diff --git a/agent/skills/fsskills/source_script_test.go b/agent/skills/fsskills/source_script_test.go index 744fd889..d9762e94 100644 --- a/agent/skills/fsskills/source_script_test.go +++ b/agent/skills/fsskills/source_script_test.go @@ -342,6 +342,32 @@ func TestFileSource_ScriptFilter_IncludesOnlyMatchingScripts(t *testing.T) { } } +func TestFileSource_SymlinkedScript_IsSkipped(t *testing.T) { + root := t.TempDir() + createSkillDir(t, root, "script-link-skill", "Symlinked script", "Body.") + outsideScript := filepath.Join(root, "outside.py") + if err := os.WriteFile(outsideScript, []byte("print('outside')"), 0o644); err != nil { + t.Fatal(err) + } + createSymlink(t, filepath.Join(root, "script-link-skill", "scripts", "run.py"), outsideScript) + source := fsskills.NewSourceOptions(fsskills.SourceOptions{ + ScriptRunner: func(context.Context, *skills.Skill, *skills.Script, []string) (any, error) { + return nil, nil + }, + }, os.DirFS(root)) + + loaded, err := source.Skills(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(loaded) != 1 { + t.Fatalf("expected 1 skill, got %d", len(loaded)) + } + if len(loaded[0].Scripts) != 0 { + t.Fatalf("expected symlinked script to be skipped, got %d scripts", len(loaded[0].Scripts)) + } +} + func TestFileScript_RunWithNonFileSkill_ReturnsError(t *testing.T) { root := t.TempDir() createSkillDir(t, root, "script-owner", "Script owner", "Body.") diff --git a/agent/skills/fsskills/source_test.go b/agent/skills/fsskills/source_test.go index 06b69d1f..c61be2ef 100644 --- a/agent/skills/fsskills/source_test.go +++ b/agent/skills/fsskills/source_test.go @@ -142,6 +142,48 @@ func TestFileSource_RootSkillFileWithNestedSkillFile_DoesNotAbortDiscovery(t *te } } +func TestFileSource_SymlinkedSkillFile_IsSkipped(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "linked-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + outsideSkillFile := filepath.Join(root, "outside-SKILL.md") + if err := os.WriteFile(outsideSkillFile, []byte("---\nname: linked-skill\ndescription: Linked skill file\n---\nBody."), 0o644); err != nil { + t.Fatal(err) + } + createSymlink(t, filepath.Join(skillDir, "SKILL.md"), outsideSkillFile) + + source := fsskills.NewSource(os.DirFS(root)) + loaded, err := source.Skills(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(loaded) != 0 { + t.Fatalf("expected symlinked SKILL.md to be skipped, got %d skills", len(loaded)) + } +} + +func TestFileSource_ConfiguredRootSymlink_StillDiscoversSkills(t *testing.T) { + root := t.TempDir() + realRoot := filepath.Join(root, "real-root") + linkedRoot := filepath.Join(root, "linked-root") + createSkillDir(t, realRoot, "my-skill", "A skill", "Body.") + createSymlink(t, linkedRoot, realRoot) + + source := fsskills.NewSource(os.DirFS(linkedRoot)) + loaded, err := source.Skills(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(loaded) != 1 { + t.Fatalf("expected 1 skill, got %d", len(loaded)) + } + if loaded[0].Frontmatter.Name != "my-skill" { + t.Fatalf("expected my-skill, got %q", loaded[0].Frontmatter.Name) + } +} + func TestFileSource_NestedSkillFileUnderSkillRoot_NotDiscoveredAsIndependentSkill(t *testing.T) { root := t.TempDir() createSkillDir(t, root, "parent-skill", "Parent", "Parent body.") @@ -548,6 +590,28 @@ func TestFileSource_NoDuplicateResourcesFromSamePath(t *testing.T) { } } +func TestFileSource_SymlinkedResource_IsSkipped(t *testing.T) { + root := t.TempDir() + createSkillDir(t, root, "resource-link-skill", "Symlinked resource", "Body.") + outsideResource := filepath.Join(root, "outside.md") + if err := os.WriteFile(outsideResource, []byte("secret"), 0o644); err != nil { + t.Fatal(err) + } + createSymlink(t, filepath.Join(root, "resource-link-skill", "references", "secret.md"), outsideResource) + + source := fsskills.NewSource(os.DirFS(root)) + loaded, err := source.Skills(t.Context()) + if err != nil { + t.Fatal(err) + } + if len(loaded) != 1 { + t.Fatalf("expected 1 skill, got %d", len(loaded)) + } + if len(loaded[0].Resources) != 0 { + t.Fatalf("expected symlinked resource to be skipped, got %d resources", len(loaded[0].Resources)) + } +} + func createSkillDir(t *testing.T, root, name, description, body string) { t.Helper() skillDir := filepath.Join(root, name) @@ -594,3 +658,13 @@ func createRelativeFile(t *testing.T, root, relativePath, content string) { t.Fatal(err) } } + +func createSymlink(t *testing.T, linkPath, targetPath string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(linkPath), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(targetPath, linkPath); err != nil { + t.Skipf("symlink creation unavailable: %v", err) + } +}