fix(agentplugins): reject unsafe and conflicting skill selections (#2505)

## Summary

- reject skill selections that are empty, traversal-like, or contain
path separators
- reject duplicate skill names across standalone skills and selected
plugin skills before fetching artifacts
- canonicalize existing and not-yet-created paths during containment
checks so macOS `/var` aliases do not reject valid packages

This keeps Agent Plugin materialization deterministic and prevents one
selected skill from overwriting another. It does not change the Agent
Plugins schema or artifact source formats.

## Testing

- `cd go && go test ./core/v2/agentplugins ./adk/pkg/mcp
./core/v2/translator`
- `cd go && go vet ./core/v2/agentplugins`
- `git diff --check`

## Risk / Notes

- The new validation runs before any artifact is fetched or copied.
- Existing plugin and MCP behavior is unchanged for valid selections.
- The canonical-path handling preserves the existing symlink escape
checks while making containment portable across macOS
temporary-directory aliases.

Signed-off-by: Whxuan0701 <102815982+Whxuan0701@users.noreply.github.com>
Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Co-authored-by: Whxuan0701 <102815982+Whxuan0701@users.noreply.github.com>
Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io>

Author: 3 people

go/core/v2/agentplugins/materialize.go

@@ -68,6 +68,17 @@ func MaterializeAgentConfig(ctx context.Context, config *adk.AgentConfig, paths
 }
 
 func Materialize(ctx context.Context, config adk.AgentPluginConfig, paths Paths) (MCPConfig, error) {
+	selectedSkills := make([]string, 0, len(config.Skills))
+	for _, skill := range config.Skills {
+		selectedSkills = append(selectedSkills, skill.Name)
+	}
+	for _, plugin := range config.Plugins {
+		selectedSkills = append(selectedSkills, plugin.Skills...)
+	}
+	if err := validateSkillSelections(selectedSkills); err != nil {
+		return MCPConfig{}, err
+	}
+
 	if err := os.MkdirAll(paths.Skills, 0o755); err != nil {
 		return MCPConfig{}, fmt.Errorf("create skills directory: %w", err)
 	}
@@ -119,6 +130,27 @@ func Materialize(ctx context.Context, config adk.AgentPluginConfig, paths Paths)
 	return result, nil
 }
 
+func validateSkillSelections(names []string) error {
+	seen := make(map[string]struct{}, len(names))
+	for _, name := range names {
+		if err := validateSkillName(name); err != nil {
+			return err
+		}
+		if _, exists := seen[name]; exists {
+			return fmt.Errorf("duplicate skill name %q", name)
+		}
+		seen[name] = struct{}{}
+	}
+	return nil
+}
+
+func validateSkillName(name string) error {
+	if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\\`) {
+		return fmt.Errorf("skill name %q must be a single relative path component", name)
+	}
+	return nil
+}
+
 func fetchSource(ctx context.Context, source adk.AgentPluginSource, destination, requiredFile string) (string, error) {
 	selected := 0
 	if source.OCI != "" {
@@ -241,10 +273,24 @@ func containedPath(root, relative string) (string, error) {
 }
 
 func pathWithin(root, path string) bool {
-	relative, err := filepath.Rel(root, path)
+	root = canonicalPath(root)
+	path = canonicalPath(path)
+	relative, err := filepath.Rel(filepath.Clean(root), filepath.Clean(path))
 	return err == nil && relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator))
 }
 
+func canonicalPath(path string) string {
+	path = filepath.Clean(path)
+	if resolved, err := filepath.EvalSymlinks(path); err == nil {
+		return resolved
+	}
+	parent := filepath.Dir(path)
+	if parent == path {
+		return path
+	}
+	return filepath.Join(canonicalPath(parent), filepath.Base(path))
+}
+
 func copySkill(source, destination string) error {
 	info, err := os.Stat(filepath.Join(source, "SKILL.md"))
 	if err != nil || !info.Mode().IsRegular() {

go/core/v2/agentplugins/materialize_test.go

@@ -157,7 +157,7 @@ func TestParseMCPServerSupportsLocalAndRemoteTransports(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if stdio.Command != command || stdio.Args[0] != "--root="+root || stdio.Env["STATE"] != filepath.Join(data, "state") || stdio.CWD != filepath.Join(data, "work") {
+	if stdio.Command != canonicalPath(command) || stdio.Args[0] != "--root="+root || stdio.Env["STATE"] != filepath.Join(data, "state") || stdio.CWD != filepath.Join(data, "work") {
 		t.Fatalf("stdio server = %#v", stdio)
 	}
 
@@ -230,3 +230,34 @@ func TestCopySkillRejectsSymlinkOutsideSkill(t *testing.T) {
 		t.Fatal("copySkill() accepted a symlink outside the skill root")
 	}
 }
+
+func TestValidateSkillNameRejectsPathTraversal(t *testing.T) {
+	for _, name := range []string{"../escape", "nested/skill", `nested\skill`, ".", ".."} {
+		if err := validateSkillName(name); err == nil {
+			t.Fatalf("validateSkillName(%q) accepted a path-like name", name)
+		}
+	}
+}
+
+func TestValidateSkillSelectionsRejectsDuplicateNames(t *testing.T) {
+	err := validateSkillSelections([]string{"review", "lint", "review"})
+	if err == nil || !strings.Contains(err.Error(), `duplicate skill name "review"`) {
+		t.Fatalf("validateSkillSelections() error = %v, want duplicate skill error", err)
+	}
+}
+
+func TestPathWithinCanonicalizesRootAliases(t *testing.T) {
+	actualRoot := t.TempDir()
+	child := filepath.Join(actualRoot, "child")
+	if err := os.Mkdir(child, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	alias := filepath.Join(t.TempDir(), "root-alias")
+	if err := os.Symlink(actualRoot, alias); err != nil {
+		t.Fatal(err)
+	}
+
+	if !pathWithin(alias, child) {
+		t.Fatalf("pathWithin(%q, %q) rejected a path under the aliased root", alias, child)
+	}
+}