diff --git a/internal/declarative/claude/skill.go b/internal/declarative/claude/skill.go index 5e94a6c..12141e5 100644 --- a/internal/declarative/claude/skill.go +++ b/internal/declarative/claude/skill.go @@ -47,6 +47,9 @@ func loadSkillDir(dir string) (map[string]any, core.Payload, error) { // (display_title, title) are still accepted, and a skill with none is // shown under its `name`, or failing that its directory. name := firstString(fields, "name") + if err := checkSkillName(name); err != nil { + return nil, nil, fmt.Errorf("%s: %w", skillMD, err) + } displayName := cmp.Or(firstString(fields, "display_name", "display_title", "title"), name, filepath.Base(dir)) uploadDir := cmp.Or(name, filepath.Base(dir)) @@ -112,6 +115,19 @@ func skillFrontmatter(content []byte) ([]byte, error) { return []byte(strings.NewReplacer("\r\n", "\n", "\r", "\n").Replace(yaml)), nil } +// checkSkillName refuses a manifest name that is not one path segment. The +// name is the directory the skill is uploaded under, and path.Join would +// otherwise turn "../../other" into a multipart filename outside that folder. +func checkSkillName(name string) error { + if name == "" { + return nil + } + if strings.Contains(name, "/") || strings.Contains(name, `\`) || strings.Contains(name, "..") { + return fmt.Errorf("name %q cannot contain '/', '\\', or '..'; it is the directory the skill is uploaded under", name) + } + return nil +} + // firstString returns the first of keys whose value is a non-blank string, trimmed. func firstString(m map[string]any, keys ...string) string { for _, k := range keys { diff --git a/internal/declarative/claude/skill_test.go b/internal/declarative/claude/skill_test.go index 36aad0d..a1885cd 100644 --- a/internal/declarative/claude/skill_test.go +++ b/internal/declarative/claude/skill_test.go @@ -3,6 +3,7 @@ package claude import ( "os" "path/filepath" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -93,6 +94,26 @@ func TestSkillManifestProblemsAreNamedBeforeAnyUpload(t *testing.T) { } } +func TestSkillNameCannotBeAPath(t *testing.T) { + for _, name := range []string{"../../other", "/other", `..\other`, "foo/bar", "..", "foo..bar"} { + root := writeTree(t, map[string]string{ + "s/SKILL.md": "---\nname: " + strconv.Quote(name) + "\n---\nbody\n", + }) + _, _, err := loadSkillDir(filepath.Join(root, "s")) + require.ErrorContains(t, err, "cannot contain") + assert.ErrorContains(t, err, "SKILL.md") + } + + root := writeTree(t, map[string]string{ + "s/SKILL.md": "---\nname: pr-writer\n---\nbody\n", + }) + _, payload, err := loadSkillDir(filepath.Join(root, "s")) + require.NoError(t, err) + bundle := payload.(*skillBundle) + assert.Equal(t, "pr-writer", bundle.UploadDir) + assert.Equal(t, "pr-writer/SKILL.md", bundle.Files[0].UploadName(bundle.UploadDir)) +} + func TestSkillBodyCarriesTheDeclaredName(t *testing.T) { root := writeTree(t, map[string]string{ "named/SKILL.md": "---\nname: pdf-tools\ndescription: d\n---\n",