fix(marketplace): copy an in-tree symlink instead of refusing it

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)
 	}

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 {

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")
+			}
+		})
+	}
+}