diff --git a/README.md b/README.md index 7d3cd2e..a09b018 100644 --- a/README.md +++ b/README.md @@ -671,7 +671,7 @@ Only the given path is searched. The [agentskills.io implementation guide](https - Subdirectories are searched up to 6 levels deep by default. Use `--max-depth=N` to change this; `--max-depth=1` searches only the immediate subdirectories, and `--max-depth=0` treats the given path as a single skill and nothing else. - A directory that contains a `SKILL.md` is a skill, and nothing below it is searched, so a `SKILL.md` inside a skill's examples or references does not count as a second skill. - Hidden directories (names starting with `.`, which covers `.git/`) and dependency directories (`node_modules/`, `__pycache__/`) are skipped. `.gitignore` is not consulted: honoring it would mean locating the repository root and evaluating nested ignore rules, and the directory budget already bounds the cost of scanning build output. -- Symlinks are followed, so a skills directory assembled from links to skill folders kept elsewhere works. Each real directory is examined once, which makes symlink cycles harmless and keeps a skill from being reported twice under two paths. +- Symlinks and, on Windows, directory junctions are followed, so a skills directory assembled from links to skill folders kept elsewhere works. Each real directory is examined once, which makes link cycles harmless and keeps a skill from being reported twice under two paths. - At most 2000 directories are listed. A skill is found as soon as its parent directory is listed, and the search is breadth-first, so shallow skills are found before a large subtree can use up that budget. If the limit is reached, a warning on stderr says that skills below that point were not found. Each skill is validated independently. The text output separates skills with a line and appends an overall summary. The JSON output wraps individual skill reports in a `skills` array: diff --git a/cmd/junction_windows_integration_test.go b/cmd/junction_windows_integration_test.go new file mode 100644 index 0000000..53e8f23 --- /dev/null +++ b/cmd/junction_windows_integration_test.go @@ -0,0 +1,102 @@ +//go:build windows + +package cmd_test + +import ( + "bytes" + "encoding/json" + "errors" + "io/fs" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/agent-ecosystem/skill-validator/internal/testutil" +) + +// copySkill copies the skill fixture at src to dst and renames it after its +// new directory, since a skill's name must match the directory it lives in. +func copySkill(t *testing.T, src, dst string) { + t.Helper() + err := filepath.WalkDir(src, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + rel, err := filepath.Rel(src, path) + if err != nil { + return err + } + target := filepath.Join(dst, rel) + if d.IsDir() { + return os.MkdirAll(target, 0o755) + } + data, err := os.ReadFile(path) + if err != nil { + return err + } + if rel == "SKILL.md" { + data = bytes.Replace(data, []byte("name: "+filepath.Base(src)), []byte("name: "+filepath.Base(dst)), 1) + } + return os.WriteFile(target, data, 0o644) + }) + if err != nil { + t.Fatal(err) + } +} + +// Issue #101 end to end: a skills directory made of junctions, the layout +// that was reported, must validate every skill rather than silently only +// the ones that are real directories. +func TestCheckJunctionedCatalog(t *testing.T) { + bin := buildBinary(t) + valid := fixture(t, "valid-skill") + + source := t.TempDir() + catalog := t.TempDir() + names := []string{"alpha", "beta", "gamma"} + for _, name := range names { + copySkill(t, valid, filepath.Join(source, name)) + testutil.Junction(t, filepath.Join(source, name), filepath.Join(catalog, name)) + } + copySkill(t, valid, filepath.Join(catalog, "plain")) + wantSkills := len(names) + 1 + + out, err := exec.Command(bin, "check", "-o", "json", catalog).Output() + var exitErr *exec.ExitError + if err != nil && !errors.As(err, &exitErr) { + t.Fatalf("running check: %v", err) + } + + var report struct { + Skills []struct { + SkillDir string `json:"skill_dir"` + Errors int `json:"errors"` + Results []struct { + Level string `json:"level"` + Message string `json:"message"` + } `json:"results"` + } `json:"skills"` + } + if err := json.Unmarshal(out, &report); err != nil { + t.Fatalf("parsing report: %v\n%s", err, out) + } + if len(report.Skills) != wantSkills { + var dirs []string + for _, s := range report.Skills { + dirs = append(dirs, s.SkillDir) + } + t.Fatalf("validated %d skills %v, want %d", len(report.Skills), dirs, wantSkills) + } + for _, s := range report.Skills { + if s.Errors == 0 { + continue + } + t.Errorf("%s: %d errors", s.SkillDir, s.Errors) + for _, r := range s.Results { + if r.Level == "error" { + t.Logf(" %s", r.Message) + } + } + } +} diff --git a/internal/testutil/junction_windows.go b/internal/testutil/junction_windows.go new file mode 100644 index 0000000..9551cab --- /dev/null +++ b/internal/testutil/junction_windows.go @@ -0,0 +1,19 @@ +//go:build windows + +package testutil + +import ( + "os/exec" + "testing" +) + +// Junction creates a directory junction at link leading to target. Unlike a +// symlink, a junction needs no privilege, which is why it is the common way +// to alias a directory on Windows and why Windows CI can always exercise it. +func Junction(t testing.TB, target, link string) { + t.Helper() + out, err := exec.Command("cmd", "/c", "mklink", "/J", link, target).CombinedOutput() + if err != nil { + t.Fatalf("mklink /J %s %s: %v\n%s", link, target, err, out) + } +} diff --git a/internal/testutil/privilege_other.go b/internal/testutil/privilege_other.go new file mode 100644 index 0000000..68fbcd0 --- /dev/null +++ b/internal/testutil/privilege_other.go @@ -0,0 +1,5 @@ +//go:build !windows + +package testutil + +func isPrivilegeError(error) bool { return false } diff --git a/internal/testutil/privilege_windows.go b/internal/testutil/privilege_windows.go new file mode 100644 index 0000000..314249b --- /dev/null +++ b/internal/testutil/privilege_windows.go @@ -0,0 +1,12 @@ +//go:build windows + +package testutil + +import ( + "errors" + "syscall" +) + +func isPrivilegeError(err error) bool { + return errors.Is(err, syscall.ERROR_PRIVILEGE_NOT_HELD) +} diff --git a/internal/testutil/symlink.go b/internal/testutil/symlink.go new file mode 100644 index 0000000..6781aa7 --- /dev/null +++ b/internal/testutil/symlink.go @@ -0,0 +1,24 @@ +// Package testutil holds helpers shared by the test suites. +package testutil + +import ( + "os" + "testing" +) + +// Symlink creates link pointing at target, or skips the test when the host +// refuses. Windows grants symlink creation only to elevated processes and to +// accounts with Developer Mode on. The suites used to skip every symlink +// test on Windows outright, which kept the platform's link handling out of +// CI even though the hosted Windows runners are elevated (#101). +func Symlink(t testing.TB, target, link string) { + t.Helper() + err := os.Symlink(target, link) + if err == nil { + return + } + if isPrivilegeError(err) { + t.Skipf("cannot create symlinks on this host: %v", err) + } + t.Fatal(err) +} diff --git a/skill/skill_test.go b/skill/skill_test.go index 87c96a9..ab843ab 100644 --- a/skill/skill_test.go +++ b/skill/skill_test.go @@ -4,9 +4,9 @@ import ( "errors" "os" "path/filepath" - "runtime" "testing" + "github.com/agent-ecosystem/skill-validator/internal/testutil" "github.com/agent-ecosystem/skill-validator/util" ) @@ -127,17 +127,12 @@ func TestLoad(t *testing.T) { }) t.Run("refuses SKILL.md symlink", func(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("symlinks require admin on Windows") - } dir := t.TempDir() secret := filepath.Join(dir, "secret.md") if err := os.WriteFile(secret, []byte("---\nname: x\ndescription: x\n---\nLEAK"), 0o644); err != nil { t.Fatal(err) } - if err := os.Symlink(secret, filepath.Join(dir, "SKILL.md")); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, secret, filepath.Join(dir, "SKILL.md")) _, err := Load(dir) if !errors.Is(err, util.ErrUnsafeFile) { t.Fatalf("expected ErrUnsafeFile, got %v", err) diff --git a/skillcheck/discovery_windows_test.go b/skillcheck/discovery_windows_test.go new file mode 100644 index 0000000..efd7420 --- /dev/null +++ b/skillcheck/discovery_windows_test.go @@ -0,0 +1,107 @@ +//go:build windows + +package skillcheck + +import ( + "os" + "path/filepath" + "reflect" + "testing" + + "github.com/agent-ecosystem/skill-validator/internal/testutil" +) + +// Issue #101: Go reports a directory junction as an irregular file, neither +// a directory nor a symlink, so discovery dropped every junctioned skill +// without a word. Junctions need no privilege, so these run on any Windows +// host. +func TestDiscoverSkills_Junctions(t *testing.T) { + t.Run("junctioned catalog is discovered", func(t *testing.T) { + // The reported layout: a real source tree elsewhere and a skills + // directory made of junctions into it, beside one ordinary skill. + source := t.TempDir() + dir := t.TempDir() + var want []string + for _, name := range []string{"alpha", "beta", "gamma"} { + writeSkill(t, filepath.Join(source, name), "---\nname: "+name+"\n---\n") + testutil.Junction(t, filepath.Join(source, name), filepath.Join(dir, name)) + want = append(want, filepath.Join(dir, name)) + } + writeSkill(t, filepath.Join(dir, "plain"), "---\nname: plain\n---\n") + want = append(want, filepath.Join(dir, "plain")) + + d := DiscoverSkills(dir, DefaultMaxDepth) + if !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("junction to a grouping directory is searched", func(t *testing.T) { + source := t.TempDir() + writeSkill(t, filepath.Join(source, "team", "skill"), "---\nname: s\n---\n") + dir := t.TempDir() + testutil.Junction(t, source, filepath.Join(dir, "linked")) + + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "linked", "team", "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("alias of a skill is reported once", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "real"), "---\nname: s\n---\n") + testutil.Junction(t, filepath.Join(dir, "real"), filepath.Join(dir, "alias")) + + d := DiscoverSkills(dir, DefaultMaxDepth) + if len(d.Dirs) != 1 { + t.Errorf("dirs = %v, want exactly one", d.Dirs) + } + }) + + t.Run("cycle to root", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "team", "skill"), "---\nname: s\n---\n") + testutil.Junction(t, dir, filepath.Join(dir, "loop")) + + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "team", "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + if d.Truncated { + t.Error("a cycle must not consume the directory budget") + } + }) + + t.Run("symlink to a skill behind a junction", func(t *testing.T) { + source := t.TempDir() + writeSkill(t, filepath.Join(source, "skill"), "---\nname: s\n---\n") + junction := filepath.Join(t.TempDir(), "junction") + testutil.Junction(t, source, junction) + dir := t.TempDir() + testutil.Symlink(t, filepath.Join(junction, "skill"), filepath.Join(dir, "alias")) + + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "alias")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) + + t.Run("dangling junction is ignored", func(t *testing.T) { + dir := t.TempDir() + writeSkill(t, filepath.Join(dir, "skill"), "---\nname: s\n---\n") + gone := filepath.Join(t.TempDir(), "gone") + if err := os.Mkdir(gone, 0o755); err != nil { + t.Fatal(err) + } + testutil.Junction(t, gone, filepath.Join(dir, "dangling")) + if err := os.Remove(gone); err != nil { + t.Fatal(err) + } + + d := DiscoverSkills(dir, DefaultMaxDepth) + if want := []string{filepath.Join(dir, "skill")}; !reflect.DeepEqual(d.Dirs, want) { + t.Errorf("dirs = %v, want %v", d.Dirs, want) + } + }) +} diff --git a/skillcheck/validator.go b/skillcheck/validator.go index 447cefa..9465211 100644 --- a/skillcheck/validator.go +++ b/skillcheck/validator.go @@ -116,7 +116,7 @@ func discoverSkills(root string, maxDepth, maxDirs int) Discovery { if err != nil { return Discovery{Mode: types.NoSkill} } - realRoot, err := filepath.EvalSymlinks(absRoot) + realRoot, err := util.RealPath(absRoot) if err != nil { return Discovery{Mode: types.NoSkill} } @@ -152,10 +152,12 @@ func discoverSkills(root string, maxDepth, maxDirs int) Discovery { switch { case entry.IsDir(): real = filepath.Join(cur.real, name) - case entry.Type()&os.ModeSymlink != 0: - // Follow the link, but only into a directory that exists. + case entry.Type()&(os.ModeSymlink|os.ModeIrregular) != 0: + // A symlink, or on Windows a directory junction, which Go + // reports as irregular rather than as a symlink (#101). + // Follow it, but only into a directory that exists. // Resolving through cur.real keeps the key absolute. - resolved, err := filepath.EvalSymlinks(filepath.Join(cur.real, name)) + resolved, err := util.RealPath(filepath.Join(cur.real, name)) if err != nil { continue } diff --git a/skillcheck/validator_test.go b/skillcheck/validator_test.go index 82b52fe..c24e3d3 100644 --- a/skillcheck/validator_test.go +++ b/skillcheck/validator_test.go @@ -5,10 +5,10 @@ import ( "os" "path/filepath" "reflect" - "runtime" "strings" "testing" + "github.com/agent-ecosystem/skill-validator/internal/testutil" "github.com/agent-ecosystem/skill-validator/skill" "github.com/agent-ecosystem/skill-validator/types" ) @@ -91,18 +91,13 @@ func TestReadReferencesMarkdownFiles(t *testing.T) { // skill tree, the regular files inside pass a leaf-only check. Their content // must not be read and shipped to the LLM judge. func TestReadReferencesMarkdownFiles_SkipsSymlinkedDirectory(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("symlinks require admin on Windows") - } outside := t.TempDir() if err := os.WriteFile(filepath.Join(outside, "leak.md"), []byte("SHOULD-NOT-LEAK"), 0o644); err != nil { t.Fatal(err) } dir := t.TempDir() - if err := os.Symlink(outside, filepath.Join(dir, "references")); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, outside, filepath.Join(dir, "references")) files := ReadReferencesMarkdownFiles(dir) for name, content := range files { @@ -113,9 +108,6 @@ func TestReadReferencesMarkdownFiles_SkipsSymlinkedDirectory(t *testing.T) { } func TestReadReferencesMarkdownFiles_SkipsSymlinks(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("symlinks require admin on Windows") - } dir := t.TempDir() refsDir := filepath.Join(dir, "references") if err := os.MkdirAll(refsDir, 0o755); err != nil { @@ -128,9 +120,7 @@ func TestReadReferencesMarkdownFiles_SkipsSymlinks(t *testing.T) { if err := os.WriteFile(secret, []byte("SHOULD-NOT-LEAK"), 0o600); err != nil { t.Fatal(err) } - if err := os.Symlink(secret, filepath.Join(refsDir, "leaky.md")); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, secret, filepath.Join(refsDir, "leaky.md")) files := ReadReferencesMarkdownFiles(dir) if _, ok := files["leaky.md"]; ok { @@ -350,9 +340,7 @@ func TestDetectSkills(t *testing.T) { if err := os.MkdirAll(parent, 0o755); err != nil { t.Fatal(err) } - if err := os.Symlink(realDir, filepath.Join(parent, "linked")); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, realDir, filepath.Join(parent, "linked")) mode, dirs := DetectSkills(parent) if mode != types.MultiSkill { t.Errorf("expected MultiSkill, got %d", mode) @@ -506,20 +494,10 @@ func TestDiscoverSkills_Nested(t *testing.T) { } func TestDiscoverSkills_Symlinks(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("symlinks require admin on Windows") - } - mustSymlink := func(t *testing.T, target, link string) { - t.Helper() - if err := os.Symlink(target, link); err != nil { - t.Fatal(err) - } - } - t.Run("cycle to root", func(t *testing.T) { dir := t.TempDir() writeSkill(t, filepath.Join(dir, "team", "skill"), "---\nname: s\n---\n") - mustSymlink(t, dir, filepath.Join(dir, "loop")) + testutil.Symlink(t, dir, filepath.Join(dir, "loop")) d := DiscoverSkills(dir, DefaultMaxDepth) if want := []string{filepath.Join(dir, "team", "skill")}; !reflect.DeepEqual(d.Dirs, want) { t.Errorf("dirs = %v, want %v", d.Dirs, want) @@ -532,7 +510,7 @@ func TestDiscoverSkills_Symlinks(t *testing.T) { t.Run("cycle to parent", func(t *testing.T) { dir := t.TempDir() writeSkill(t, filepath.Join(dir, "team", "skill"), "---\nname: s\n---\n") - mustSymlink(t, "..", filepath.Join(dir, "team", "up")) + testutil.Symlink(t, "..", filepath.Join(dir, "team", "up")) d := DiscoverSkills(dir, DefaultMaxDepth) if want := []string{filepath.Join(dir, "team", "skill")}; !reflect.DeepEqual(d.Dirs, want) { t.Errorf("dirs = %v, want %v", d.Dirs, want) @@ -542,7 +520,7 @@ func TestDiscoverSkills_Symlinks(t *testing.T) { t.Run("alias of a skill is reported once", func(t *testing.T) { dir := t.TempDir() writeSkill(t, filepath.Join(dir, "real"), "---\nname: s\n---\n") - mustSymlink(t, filepath.Join(dir, "real"), filepath.Join(dir, "alias")) + testutil.Symlink(t, filepath.Join(dir, "real"), filepath.Join(dir, "alias")) d := DiscoverSkills(dir, DefaultMaxDepth) if len(d.Dirs) != 1 { t.Errorf("dirs = %v, want exactly one", d.Dirs) @@ -556,7 +534,7 @@ func TestDiscoverSkills_Symlinks(t *testing.T) { // a second skill. Reported paths stay relative, as given. base := t.TempDir() writeSkill(t, filepath.Join(base, "catalog", "real"), "---\nname: s\n---\n") - mustSymlink(t, filepath.Join(base, "catalog", "real"), filepath.Join(base, "catalog", "alias")) + testutil.Symlink(t, filepath.Join(base, "catalog", "real"), filepath.Join(base, "catalog", "alias")) t.Chdir(base) d := DiscoverSkills("catalog", DefaultMaxDepth) if want := []string{filepath.Join("catalog", "alias")}; !reflect.DeepEqual(d.Dirs, want) { @@ -573,7 +551,7 @@ func TestDiscoverSkills_Symlinks(t *testing.T) { if err := os.MkdirAll(filepath.Join(dir, "team"), 0o755); err != nil { t.Fatal(err) } - mustSymlink(t, filepath.Join(outside, "shared"), filepath.Join(dir, "team", "shared")) + testutil.Symlink(t, filepath.Join(outside, "shared"), filepath.Join(dir, "team", "shared")) d := DiscoverSkills(dir, DefaultMaxDepth) if want := []string{filepath.Join(dir, "team", "shared")}; !reflect.DeepEqual(d.Dirs, want) { t.Errorf("dirs = %v, want %v", d.Dirs, want) @@ -583,8 +561,8 @@ func TestDiscoverSkills_Symlinks(t *testing.T) { t.Run("broken link and link to file are ignored", func(t *testing.T) { dir := t.TempDir() writeSkill(t, filepath.Join(dir, "skill"), "---\nname: s\n---\n") - mustSymlink(t, filepath.Join(dir, "missing"), filepath.Join(dir, "dangling")) - mustSymlink(t, filepath.Join(dir, "skill", "SKILL.md"), filepath.Join(dir, "filelink")) + testutil.Symlink(t, filepath.Join(dir, "missing"), filepath.Join(dir, "dangling")) + testutil.Symlink(t, filepath.Join(dir, "skill", "SKILL.md"), filepath.Join(dir, "filelink")) d := DiscoverSkills(dir, DefaultMaxDepth) if want := []string{filepath.Join(dir, "skill")}; !reflect.DeepEqual(d.Dirs, want) { t.Errorf("dirs = %v, want %v", d.Dirs, want) @@ -601,7 +579,7 @@ func TestDiscoverSkills_Symlinks(t *testing.T) { dir := t.TempDir() // "aaa-link" sorts before "team", so a depth-first scan would spend // its budget inside the linked tree and report no skills. - mustSymlink(t, big, filepath.Join(dir, "aaa-link")) + testutil.Symlink(t, big, filepath.Join(dir, "aaa-link")) writeSkill(t, filepath.Join(dir, "team", "skill"), "---\nname: s\n---\n") d := discoverSkills(dir, DefaultMaxDepth, 40) if want := []string{filepath.Join(dir, "team", "skill")}; !reflect.DeepEqual(d.Dirs, want) { diff --git a/structure/links_test.go b/structure/links_test.go index 019377d..b23739a 100644 --- a/structure/links_test.go +++ b/structure/links_test.go @@ -3,9 +3,9 @@ package structure import ( "os" "path/filepath" - "runtime" "testing" + "github.com/agent-ecosystem/skill-validator/internal/testutil" "github.com/agent-ecosystem/skill-validator/types" ) @@ -28,9 +28,6 @@ func TestCheckInternalLinks(t *testing.T) { // Issue #86: a link that looks internal syntactically may be a symlink // whose target resolves outside the skill directory. t.Run("symlink escaping skill directory is rejected", func(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("symlinks require admin on Windows") - } outside := t.TempDir() secret := filepath.Join(outside, "secret.md") if err := os.WriteFile(secret, []byte("OUT-OF-TREE"), 0o644); err != nil { @@ -41,9 +38,7 @@ func TestCheckInternalLinks(t *testing.T) { if err := os.MkdirAll(filepath.Join(dir, "references"), 0o755); err != nil { t.Fatal(err) } - if err := os.Symlink(secret, filepath.Join(dir, "references", "guide.md")); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, secret, filepath.Join(dir, "references", "guide.md")) body := "See [guide](references/guide.md)." results := CheckInternalLinks(dir, body) @@ -51,14 +46,9 @@ func TestCheckInternalLinks(t *testing.T) { }) t.Run("symlink resolving inside skill directory passes", func(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("symlinks require admin on Windows") - } dir := t.TempDir() writeFile(t, dir, "references/real.md", "content") - if err := os.Symlink(filepath.Join(dir, "references", "real.md"), filepath.Join(dir, "references", "alias.md")); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, filepath.Join(dir, "references", "real.md"), filepath.Join(dir, "references", "alias.md")) body := "See [alias](references/alias.md)." results := CheckInternalLinks(dir, body) diff --git a/structure/links_windows_test.go b/structure/links_windows_test.go new file mode 100644 index 0000000..bce5eea --- /dev/null +++ b/structure/links_windows_test.go @@ -0,0 +1,42 @@ +//go:build windows + +package structure + +import ( + "os" + "path/filepath" + "testing" + + "github.com/agent-ecosystem/skill-validator/internal/testutil" + "github.com/agent-ecosystem/skill-validator/types" +) + +func TestCheckInternalLinks_Junctions(t *testing.T) { + // Issue #86 for junctions: a link that looks internal may cross a + // junction that leads outside the skill. + t.Run("junction escaping skill directory is rejected", func(t *testing.T) { + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "secret.md"), []byte("OUT-OF-TREE"), 0o644); err != nil { + t.Fatal(err) + } + dir := t.TempDir() + testutil.Junction(t, outside, filepath.Join(dir, "references")) + + body := "See [secret](references/secret.md)." + results := CheckInternalLinks(dir, body) + requireResult(t, results, types.Error, "internal link escapes skill directory: references/secret.md (resolves outside the skill package)") + }) + + // Issue #101: a skill that is itself reached through a junction has + // every internal link resolve through that junction, so they are inside. + t.Run("links inside a junctioned skill pass", func(t *testing.T) { + source := t.TempDir() + writeFile(t, source, "references/guide.md", "content") + dir := filepath.Join(t.TempDir(), "skill") + testutil.Junction(t, source, dir) + + body := "See [guide](references/guide.md)." + results := CheckInternalLinks(dir, body) + requireResult(t, results, types.Pass, "internal link: references/guide.md (exists)") + }) +} diff --git a/util/realpath.go b/util/realpath.go new file mode 100644 index 0000000..a9a094a --- /dev/null +++ b/util/realpath.go @@ -0,0 +1,13 @@ +package util + +// RealPath returns path with every symlink and, on Windows, every directory +// junction resolved, in whichever order they appear. Elsewhere it is +// filepath.EvalSymlinks. Since Go 1.23 the standard library reports a +// junction as an irregular file rather than a symlink, leaves it unresolved, +// and fails with "not a directory" when one sits above the last element of a +// path, including inside a symlink's target. Without this, a skill reached +// through a junction, the usual way to alias a directory on Windows (#101), +// is found by discovery and then cannot be read. The path must exist. +func RealPath(path string) (string, error) { + return evalLinks(path) +} diff --git a/util/realpath_other.go b/util/realpath_other.go new file mode 100644 index 0000000..d8f8eb8 --- /dev/null +++ b/util/realpath_other.go @@ -0,0 +1,9 @@ +//go:build !windows + +package util + +import "path/filepath" + +func evalLinks(path string) (string, error) { + return filepath.EvalSymlinks(path) +} diff --git a/util/realpath_windows.go b/util/realpath_windows.go new file mode 100644 index 0000000..ec72d31 --- /dev/null +++ b/util/realpath_windows.go @@ -0,0 +1,72 @@ +//go:build windows + +package util + +import ( + "fmt" + "os" + "path/filepath" +) + +// maxLinkHops bounds the links followed, as filepath.EvalSymlinks does, so +// a link that leads back into its own path cannot loop forever. +const maxLinkHops = 255 + +// evalLinks walks path from its volume, replacing each symlink or junction +// with its target as it is met and restarting from the volume, so a link +// found inside another link's target is resolved too. Once no element is a +// link, filepath.EvalSymlinks normalizes case and short names, which is all +// it has left to do. Other reparse points, such as cloud file placeholders, +// are not links and are left to it as well. +func evalLinks(path string) (string, error) { + path, err := filepath.Abs(path) + if err != nil { + return "", err + } + hops := 0 + vol := len(filepath.VolumeName(path)) + for i := vol + 1; i <= len(path); i++ { + if i < len(path) && !os.IsPathSeparator(path[i]) { + continue + } + prefix := path[:i] + fi, err := os.Lstat(prefix) + if err != nil { + return "", err + } + var target string + switch { + case fi.Mode()&os.ModeSymlink != 0: + target, err = os.Readlink(prefix) + if err != nil { + return "", err + } + switch { + case filepath.IsAbs(target): + case len(target) > 0 && os.IsPathSeparator(target[0]): + // Relative to the volume of the link. + target = filepath.VolumeName(prefix) + target + default: + // Relative to the directory holding the link. + target = filepath.Join(filepath.Dir(prefix), target) + } + case fi.Mode()&os.ModeIrregular != 0: + // A junction's target is always absolute. Readlink fails for + // reparse points that are not links. + target, err = os.Readlink(prefix) + if err != nil { + continue + } + default: + continue + } + hops++ + if hops > maxLinkHops { + return "", fmt.Errorf("too many links: %s", path) + } + path = filepath.Join(target, path[i:]) + vol = len(filepath.VolumeName(path)) + i = vol + } + return filepath.EvalSymlinks(path) +} diff --git a/util/util.go b/util/util.go index 7395f52..1bd0fc9 100644 --- a/util/util.go +++ b/util/util.go @@ -103,14 +103,14 @@ func checkSafePath(root, path string) error { return nil } -// ResolvesWithin reports whether path, after resolving all symlinks in both -// arguments, remains inside root. The path must exist. +// ResolvesWithin reports whether path, after resolving all symlinks and +// junctions in both arguments, remains inside root. The path must exist. func ResolvesWithin(root, path string) (bool, error) { - resolvedRoot, err := filepath.EvalSymlinks(root) + resolvedRoot, err := RealPath(root) if err != nil { return false, err } - resolved, err := filepath.EvalSymlinks(path) + resolved, err := RealPath(path) if err != nil { return false, err } diff --git a/util/util_test.go b/util/util_test.go index 840a8b6..a183338 100644 --- a/util/util_test.go +++ b/util/util_test.go @@ -5,8 +5,9 @@ import ( "math" "os" "path/filepath" - "runtime" "testing" + + "github.com/agent-ecosystem/skill-validator/internal/testutil" ) func TestRoundTo(t *testing.T) { @@ -107,22 +108,20 @@ func TestSafeReadFile(t *testing.T) { t.Errorf("IsRegularFile(regular) = false, want true") } - if runtime.GOOS != "windows" { + t.Run("symlink is refused", func(t *testing.T) { secret := filepath.Join(dir, "secret.txt") if err := os.WriteFile(secret, []byte("PRIVATE"), 0o644); err != nil { t.Fatal(err) } link := filepath.Join(dir, "link.txt") - if err := os.Symlink(secret, link); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, secret, link) if _, err := SafeReadFile(dir, link); !errors.Is(err, ErrUnsafeFile) { t.Errorf("SafeReadFile(symlink) error = %v, want ErrUnsafeFile", err) } if IsRegularFile(link) { t.Errorf("IsRegularFile(symlink) = true, want false") } - } + }) missing := filepath.Join(dir, "missing.txt") if _, err := SafeReadFile(dir, missing); err == nil { @@ -141,19 +140,13 @@ func TestSafeReadFile(t *testing.T) { // a symlinked directory passes a leaf-only Lstat check, but its resolved path // escapes the skill root and must be refused. func TestSafeReadFile_SymlinkedDirectory(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("symlinks require admin on Windows") - } - outside := t.TempDir() if err := os.WriteFile(filepath.Join(outside, "secret.md"), []byte("OUT-OF-TREE"), 0o644); err != nil { t.Fatal(err) } root := t.TempDir() - if err := os.Symlink(outside, filepath.Join(root, "references")); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, outside, filepath.Join(root, "references")) // The leaf is a regular file, so only the resolved-path containment // check can catch the escape. @@ -169,9 +162,7 @@ func TestSafeReadFile_SymlinkedDirectory(t *testing.T) { if err := os.WriteFile(filepath.Join(root, "assets", "real", "ok.md"), []byte("fine"), 0o644); err != nil { t.Fatal(err) } - if err := os.Symlink(filepath.Join(root, "assets", "real"), filepath.Join(root, "alias")); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, filepath.Join(root, "assets", "real"), filepath.Join(root, "alias")) if _, err := SafeReadFile(root, filepath.Join(root, "alias", "ok.md")); err != nil { t.Errorf("SafeReadFile(file in in-tree symlinked dir) error = %v, want nil", err) } @@ -223,15 +214,13 @@ func TestSafeReadFileN(t *testing.T) { } } - if runtime.GOOS != "windows" { + t.Run("symlink is refused", func(t *testing.T) { link := filepath.Join(dir, "link.txt") - if err := os.Symlink(path, link); err != nil { - t.Fatal(err) - } + testutil.Symlink(t, path, link) if _, _, err := SafeReadFileN(dir, link, 4); !errors.Is(err, ErrUnsafeFile) { t.Errorf("SafeReadFileN(symlink) error = %v, want ErrUnsafeFile", err) } - } + }) } // TestSafeReadFile_TooLarge covers issue #87: a file past MaxSkillFileBytes diff --git a/util/util_windows_test.go b/util/util_windows_test.go new file mode 100644 index 0000000..3719723 --- /dev/null +++ b/util/util_windows_test.go @@ -0,0 +1,149 @@ +//go:build windows + +package util + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/agent-ecosystem/skill-validator/internal/testutil" +) + +func TestRealPath_Junction(t *testing.T) { + source := t.TempDir() + if err := os.MkdirAll(filepath.Join(source, "sub"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(source, "sub", "file.txt"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(t.TempDir(), "link") + testutil.Junction(t, source, link) + + // filepath.EvalSymlinks alone fails here with "not a directory" since + // Go 1.23, because the junction above file.txt is neither a directory + // nor a symlink to it. + got, err := RealPath(filepath.Join(link, "sub", "file.txt")) + if err != nil { + t.Fatalf("RealPath through junction: %v", err) + } + want, err := filepath.EvalSymlinks(filepath.Join(source, "sub", "file.txt")) + if err != nil { + t.Fatal(err) + } + if got != want { + t.Errorf("RealPath = %q, want %q", got, want) + } +} + +// A skill reached through a junction (#101) must read like any other: the +// root and the files under it resolve through the same junction. +func TestSafeReadFile_JunctionedRoot(t *testing.T) { + source := t.TempDir() + files := map[string]string{ + "SKILL.md": "skill", + filepath.Join("references", "guide.md"): "guide", + } + for rel, content := range files { + path := filepath.Join(source, rel) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + root := filepath.Join(t.TempDir(), "skill") + testutil.Junction(t, source, root) + + for rel, want := range files { + got, err := SafeReadFile(root, filepath.Join(root, rel)) + if err != nil { + t.Errorf("SafeReadFile(%s): %v", rel, err) + continue + } + if string(got) != want { + t.Errorf("SafeReadFile(%s) = %q, want %q", rel, got, want) + } + } +} + +// Issue #88 for junctions: a junction inside the skill that leads outside it +// must be refused, just as a symlinked directory is. +func TestSafeReadFile_JunctionEscapesRoot(t *testing.T) { + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "secret.md"), []byte("OUT-OF-TREE"), 0o644); err != nil { + t.Fatal(err) + } + root := t.TempDir() + testutil.Junction(t, outside, filepath.Join(root, "references")) + + leaked := filepath.Join(root, "references", "secret.md") + if _, err := SafeReadFile(root, leaked); !errors.Is(err, ErrUnsafeFile) { + t.Errorf("SafeReadFile through escaping junction error = %v, want ErrUnsafeFile", err) + } +} + +// Links of both kinds can chain: a symlink may lead into a junction and a +// junction may lead to a symlink. Each element is resolved in turn. +func TestRealPath_MixedLinkChains(t *testing.T) { + source := t.TempDir() + if err := os.MkdirAll(filepath.Join(source, "sub"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(source, "sub", "file.txt"), []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + want, err := filepath.EvalSymlinks(filepath.Join(source, "sub", "file.txt")) + if err != nil { + t.Fatal(err) + } + base := t.TempDir() + junction := filepath.Join(base, "junction") + testutil.Junction(t, source, junction) + + check := func(t *testing.T, path string) { + t.Helper() + got, err := RealPath(path) + if err != nil { + t.Fatalf("RealPath(%s): %v", path, err) + } + if got != want { + t.Errorf("RealPath(%s) = %q, want %q", path, got, want) + } + } + + t.Run("symlink into a junction", func(t *testing.T) { + link := filepath.Join(base, "via-symlink") + testutil.Symlink(t, filepath.Join(junction, "sub"), link) + check(t, filepath.Join(link, "file.txt")) + }) + + t.Run("relative symlink into a junction", func(t *testing.T) { + link := filepath.Join(base, "via-relative") + testutil.Symlink(t, filepath.Join("junction", "sub"), link) + check(t, filepath.Join(link, "file.txt")) + }) + + t.Run("junction to a symlink", func(t *testing.T) { + link := filepath.Join(base, "symlinked-source") + testutil.Symlink(t, source, link) + second := filepath.Join(base, "junction-to-symlink") + testutil.Junction(t, link, second) + check(t, filepath.Join(second, "sub", "file.txt")) + }) + + t.Run("reads through a symlink into a junction", func(t *testing.T) { + root := filepath.Join(base, "skill-alias") + testutil.Symlink(t, filepath.Join(junction, "sub"), root) + got, err := SafeReadFile(root, filepath.Join(root, "file.txt")) + if err != nil { + t.Fatalf("SafeReadFile: %v", err) + } + if string(got) != "x" { + t.Errorf("SafeReadFile = %q, want %q", got, "x") + } + }) +}