diff --git a/internal/marketplace/fetch_git.go b/internal/marketplace/fetch_git.go index 8db3735a..ea772a9d 100644 --- a/internal/marketplace/fetch_git.go +++ b/internal/marketplace/fetch_git.go @@ -268,7 +268,11 @@ func extractSubdir(dir, subPath string) error { _ = os.RemoveAll(tmp) _ = os.RemoveAll(old) - if err := copyDir(resolvedSub, tmp); err != nil { + // resolvedSub, not the clone root, is the symlink boundary: the extracted + // tree is all that survives the swap below, so a link reaching into a part + // of the clone that is about to be discarded would dangle. Before in-tree + // links were copied at all this was moot — copyDir refused every symlink. + if err := copyDir(resolvedSub, resolvedSub, tmp); err != nil { _ = os.RemoveAll(tmp) return fmt.Errorf("copy subdir to tmp: %w", err) } diff --git a/internal/marketplace/fetch_relative.go b/internal/marketplace/fetch_relative.go index 745baa33..9a988f80 100644 --- a/internal/marketplace/fetch_relative.go +++ b/internal/marketplace/fetch_relative.go @@ -93,7 +93,7 @@ func (f *RelativeFetcher) Fetch(src Source, into string) (FetchResult, error) { copySrc = resolvedAbs } - if err := copyDir(copySrc, into); err != nil { + if err := copyDir(copySrc, copySrc, into); err != nil { from := copySrc if copySrc != abs { // Name the user-recognizable path too — the resolved spelling alone @@ -128,8 +128,38 @@ func pathContains(parent, child string) bool { return true } -// copyDir recursively copies src directory tree into dst, creating dst if needed. -func copyDir(src, dst string) error { +// resolveInTreeSymlink resolves the symlink at path and returns the target it +// may be copied from, requiring that target to stay inside root. It fails +// closed, mirroring the git fetcher's rejectEscapingSymlinks: a dangling or +// otherwise unresolvable link is refused rather than guessed. +// +// The ancestor check has no counterpart in the git fetcher, which preserves +// links instead of following them and so never faces the case: dereferencing a +// link that points at one of its own ancestors would recurse until the +// filesystem ran out of path. +func resolveInTreeSymlink(root, path string) (string, error) { + resolvedRoot, err := filepath.EvalSymlinks(root) + if err != nil { + return "", fmt.Errorf("relative fetcher: resolve tree root %s: %w", root, err) + } + target, err := filepath.EvalSymlinks(path) + if err != nil { + return "", fmt.Errorf("relative fetcher: cannot resolve symlink %s (refusing): %w", path, err) + } + if !pathContains(resolvedRoot, target) { + return "", fmt.Errorf("relative fetcher: %s is a symlink pointing outside the marketplace tree (refusing — would copy host files into the plugin cache)", path) + } + if pathContains(target, path) { + return "", fmt.Errorf("relative fetcher: %s is a symlink to its own ancestor %s (refusing — dereferencing it would recurse)", path, target) + } + return target, nil +} + +// copyDir recursively copies src directory tree into dst, creating dst if +// needed. root is the top of the tree being copied; it does NOT change across +// the recursion, because it is the boundary every symlink target discovered in +// the walk must stay inside. +func copyDir(root, src, dst string) error { if err := os.MkdirAll(dst, 0o755); err != nil { return err } @@ -140,18 +170,45 @@ func copyDir(src, dst string) error { for _, entry := range entries { srcPath := filepath.Join(src, entry.Name()) dstPath := filepath.Join(dst, entry.Name()) - // Reject symlinks rather than dereferencing them. copyFile does - // os.Open (which follows the link), so a marketplace tree with a - // symlink to /etc/passwd (or a dir symlink escaping the root) would - // otherwise have its target's content copied into the plugin cache - // and projected into agent config. The RootDir containment check - // only validates the top-level source path, not links discovered - // during the walk — mirror the npm fetcher's loud reject. + // A symlink is resolved and then judged, not refused outright. An + // ESCAPING link is still the hole this guard exists to close: copyFile + // does os.Open (which follows the link), so a tree with a symlink to + // /etc/passwd would otherwise have that content copied into the plugin + // cache and projected into agent config, and the RootDir containment + // check only validates the top-level source path, never links found + // during the walk. But an IN-TREE link is legitimate and must be + // copied, mirroring the git fetcher's in-tree-symlink policy + // (rejectEscapingSymlinks) and the same policy this fetcher already + // applies to a symlinked SOURCE path: one repo must not be registrable + // as `github:` yet refused as a local path. A repo that keeps one + // component tree and links the per-agent views at it (.claude/skills/x + // -> .agents/skills/x) is the shape that motivated this. if entry.Type()&os.ModeSymlink != 0 { - return fmt.Errorf("relative fetcher: %s is a symlink (refusing — marketplace trees must contain only regular files and directories)", srcPath) + target, terr := resolveInTreeSymlink(root, srcPath) + if terr != nil { + return terr + } + info, serr := os.Stat(target) + if serr != nil { + return fmt.Errorf("relative fetcher: stat symlink target of %s: %w", srcPath, serr) + } + // Dereference rather than recreate the link: the cache is left with + // no symlinks at all, so nothing reading it later can be redirected + // by one, and an absolute in-tree link does not have to be rewritten + // to stay valid under the new root. + if info.IsDir() { + if err := copyDir(root, target, dstPath); err != nil { + return err + } + } else { + if err := copyFile(target, dstPath); err != nil { + return err + } + } + continue } if entry.IsDir() { - if err := copyDir(srcPath, dstPath); err != nil { + if err := copyDir(root, srcPath, dstPath); err != nil { return err } } else { diff --git a/internal/marketplace/fetch_relative_symlink_test.go b/internal/marketplace/fetch_relative_symlink_test.go new file mode 100644 index 00000000..fc1985ef --- /dev/null +++ b/internal/marketplace/fetch_relative_symlink_test.go @@ -0,0 +1,154 @@ +package marketplace_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/spxrogers/agentsync/internal/marketplace" +) + +// TestRelativeFetcher_CopiesInTreeSymlinkEntry is the regression for a local-path +// marketplace being STRICTER than the same repo fetched over git: copyDir refused +// every symlink inside the tree, while the git fetcher's rejectEscapingSymlinks +// permits one that resolves in-tree. A repo that keeps one component tree and +// links the per-agent views at it (.claude/skills/x -> .agents/skills/x) was +// therefore registrable as `github:` but refused as a local path — the shape a +// private repo is reduced to, since the git fetcher passes no credentials. +func TestRelativeFetcher_CopiesInTreeSymlinkEntry(t *testing.T) { + src := t.TempDir() + skill := filepath.Join(src, ".agents", "skills", "shadcn") + if err := os.MkdirAll(skill, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skill, "SKILL.md"), []byte("shadcn"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Join(src, ".claude", "skills"), 0o755); err != nil { + t.Fatal(err) + } + // Directory link, relative — the shape a real repo uses. + if err := os.Symlink( + filepath.Join("..", "..", ".agents", "skills", "shadcn"), + filepath.Join(src, ".claude", "skills", "shadcn"), + ); err != nil { + t.Skipf("symlink unsupported on this platform: %v", err) + } + // File link, absolute — must be dereferenced without rewriting. + if err := os.WriteFile(filepath.Join(src, "README.md"), []byte("readme"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(src, "README.md"), filepath.Join(src, "AGENTS.md")); err != nil { + t.Skipf("symlink unsupported on this platform: %v", err) + } + + dst := t.TempDir() + source := marketplace.Source{Relative: src} + if _, err := marketplace.Dispatch(source).Fetch(source, dst); err != nil { + t.Fatalf("in-tree symlinks must be copied, got: %v", err) + } + + linked := filepath.Join(dst, ".claude", "skills", "shadcn", "SKILL.md") + data, err := os.ReadFile(linked) + if err != nil { + t.Fatalf("symlinked skill dir not copied: %v", err) + } + if string(data) != "shadcn" { + t.Errorf("%s content = %q, want %q", linked, data, "shadcn") + } + if data, err := os.ReadFile(filepath.Join(dst, "AGENTS.md")); err != nil { + t.Fatalf("symlinked file not copied: %v", err) + } else if string(data) != "readme" { + t.Errorf("AGENTS.md content = %q, want %q", data, "readme") + } + + // The cache must contain no symlinks: dereferencing, not recreating, is what + // keeps anything reading the cache later from being redirected by a link. + if err := filepath.WalkDir(dst, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.Type()&os.ModeSymlink != 0 { + t.Errorf("cache contains a symlink: %s", path) + } + return nil + }); err != nil { + t.Fatal(err) + } +} + +// TestRelativeFetcher_RefusesBadSymlinkEntry pins the three shapes that must +// still be refused now that an in-tree link is copied. +func TestRelativeFetcher_RefusesBadSymlinkEntry(t *testing.T) { + tests := []struct { + name string + build func(t *testing.T, src string) + wantErr string + }{ + { + // The original hole: a link whose target is a host file outside the + // tree. copyFile follows links, so this would leak the target. + name: "escapes the tree", + build: func(t *testing.T, src string) { + outside := filepath.Join(t.TempDir(), "secret.txt") + if err := os.WriteFile(outside, []byte("TOP SECRET HOST FILE"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(src, "leak.txt")); err != nil { + t.Skipf("symlink unsupported on this platform: %v", err) + } + }, + wantErr: "outside the marketplace tree", + }, + { + // Fail closed, as the git fetcher does: an unresolvable link is + // refused rather than guessed. + name: "dangling", + build: func(t *testing.T, src string) { + if err := os.Symlink(filepath.Join(src, "nope"), filepath.Join(src, "dangling.txt")); err != nil { + t.Skipf("symlink unsupported on this platform: %v", err) + } + }, + wantErr: "cannot resolve symlink", + }, + { + // No counterpart in the git fetcher, which preserves links instead + // of following them: dereferencing this would recurse forever. + name: "points at its own ancestor", + build: func(t *testing.T, src string) { + sub := filepath.Join(src, "sub") + if err := os.MkdirAll(sub, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(src, filepath.Join(sub, "loop")); err != nil { + t.Skipf("symlink unsupported on this platform: %v", err) + } + }, + wantErr: "own ancestor", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + src := t.TempDir() + if err := os.WriteFile(filepath.Join(src, "README.md"), []byte("ok"), 0o644); err != nil { + t.Fatal(err) + } + tc.build(t, src) + + dst := t.TempDir() + source := marketplace.Source{Relative: src} + _, err := marketplace.Dispatch(source).Fetch(source, dst) + if err == nil { + t.Fatal("expected a refusal") + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("error = %v, want it to mention %q", err, tc.wantErr) + } + if _, statErr := os.Stat(filepath.Join(dst, "leak.txt")); statErr == nil { + t.Fatal("symlink target leaked into the cache") + } + }) + } +}