From 4a7b76a23f6163caca85013db8af42730bb858f1 Mon Sep 17 00:00:00 2001 From: Sean Marchetti Date: Tue, 8 Sep 2026 12:34:07 -0400 Subject: [PATCH] fix(marketplace): copy an in-tree symlink instead of refusing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RelativeFetcher's copyDir refused every symlink inside a marketplace tree, while GitFetcher's rejectEscapingSymlinks permits one that resolves in-tree. The same repository was therefore registrable as `github:` but refused as a local path: relative fetcher: /.claude/skills/next-best-practices is a symlink (refusing — marketplace trees must contain only regular files and directories) That asymmetry bites hardest on a PRIVATE marketplace. The git fetcher builds go-git CloneOptions with no Auth, and go-git derives HTTP basic auth only from URL userinfo — no credential helper, no GITHUB_TOKEN, no .netrc — so a private repo cannot be fetched as `github:` at all, and a local clone registered as a path is the only route. A repo that keeps one component tree and links the per-agent views at it (.claude/skills/x -> .agents/skills/x) had no route left. copyDir now resolves a link and judges it, rather than refusing outright: - in-tree -> dereferenced and copied (dir or file) - escapes the tree -> refused, unchanged; this is the hole the guard exists to close, since copyFile's os.Open follows links - unresolvable -> refused, failing closed as the git fetcher does - own ancestor -> refused; dereferencing would recurse. No counterpart in the git fetcher, which preserves links rather than following them, so it never faces the case Dereferencing rather than recreating the link leaves the cache with no symlinks at all, so nothing reading it later can be redirected by one, and an absolute in-tree link needs no rewriting to stay valid under the new root. copyDir takes the tree root explicitly, since the boundary must not shift as the walk descends. extractSubdir passes the extracted subdir, not the clone root: the subdir is all that survives the swap, so a link reaching into a discarded part of the clone would dangle. TestRelativeFetcher_RejectsSymlinkEntry (the original leak regression) is an escaping link and still passes unchanged. --- internal/marketplace/fetch_git.go | 6 +- internal/marketplace/fetch_relative.go | 81 +++++++-- .../fetch_relative_symlink_test.go | 154 ++++++++++++++++++ 3 files changed, 228 insertions(+), 13 deletions(-) create mode 100644 internal/marketplace/fetch_relative_symlink_test.go 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") + } + }) + } +}