From 61ccecc7be857bc0ee645693f7fbd70fdc1571af Mon Sep 17 00:00:00 2001 From: ChrAlpha <53332481+ChrAlpha@users.noreply.github.com> Date: Tue, 14 Apr 2026 15:14:23 +0000 Subject: [PATCH] fix(skills): harden managed skill path validation --- internal/handlers/skills.go | 8 ++--- internal/handlers/skills_test.go | 18 ++++++++++ internal/skills/skills.go | 24 ++++++++++++- internal/skills/skills_test.go | 58 ++++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 5 deletions(-) diff --git a/internal/handlers/skills.go b/internal/handlers/skills.go index f13741ef..2a378ad7 100644 --- a/internal/handlers/skills.go +++ b/internal/handlers/skills.go @@ -101,10 +101,10 @@ func (h *ContainerdHandler) UpsertSkills(c echo.Context) error { for _, raw := range req.Skills { parsed := skillset.ParseFile(raw, "") - if !skillset.IsValidName(parsed.Name) { + dirPath, dirErr := skillset.ManagedSkillDirForName(parsed.Name) + if dirErr != nil { return echo.NewHTTPError(http.StatusBadRequest, "skill must have a valid name in YAML frontmatter") } - dirPath := path.Join(skillset.ManagedDir(), parsed.Name) if err := client.Mkdir(ctx, dirPath); err != nil { return echo.NewHTTPError(http.StatusInternalServerError, fmt.Sprintf("mkdir failed: %v", err)) } @@ -149,10 +149,10 @@ func (h *ContainerdHandler) DeleteSkills(c echo.Context) error { for _, name := range req.Names { skillName := strings.TrimSpace(name) - if !skillset.IsValidName(skillName) { + managedDir, dirErr := skillset.ManagedSkillDirForName(skillName) + if dirErr != nil { return echo.NewHTTPError(http.StatusBadRequest, "invalid skill name") } - managedDir := path.Join(skillset.ManagedDir(), skillName) if _, statErr := client.Stat(ctx, managedDir); statErr != nil { return fsHTTPError(statErr) } diff --git a/internal/handlers/skills_test.go b/internal/handlers/skills_test.go index 9baf2c07..ac8b6f47 100644 --- a/internal/handlers/skills_test.go +++ b/internal/handlers/skills_test.go @@ -205,6 +205,24 @@ func TestDeleteSkillsAPIRejectsExternalOnlySkill(t *testing.T) { } } +func TestUpsertSkillsAPIRejectsTraversalName(t *testing.T) { + env := newSkillsTestEnv(t) + + _, err := env.callJSON(t, http.MethodPost, "/bots/:bot_id/container/skills", SkillsUpsertRequest{ + Skills: []string{"---\nname: ..\ndescription: Escape\n---\n\n# Escape"}, + }, env.handler.UpsertSkills) + if err == nil { + t.Fatal("expected upserting traversal skill name to fail") + } + var httpErr *echo.HTTPError + if !errors.As(err, &httpErr) { + t.Fatalf("expected echo.HTTPError, got %T", err) + } + if httpErr.Code != http.StatusBadRequest { + t.Fatalf("upsert traversal status = %d, want 400", httpErr.Code) + } +} + func TestLoadSkillsUsesEffectiveSetAndPromptReflectsOverrideFallback(t *testing.T) { env := newSkillsTestEnv(t) managedPath := path.Join(skillset.ManagedDir(), "alpha", "SKILL.md") diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 6d7d66bb..8a10e471 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -102,6 +102,19 @@ func ManagedDir() string { return ManagedDirPath } +func ManagedSkillDirForName(name string) (string, error) { + name = strings.TrimSpace(name) + if !IsValidName(name) { + return "", bridge.ErrBadRequest + } + + dirPath := path.Clean(path.Join(ManagedDirPath, name)) + if dirPath == ManagedDirPath || !strings.HasPrefix(dirPath, ManagedDirPath+"/") { + return "", bridge.ErrBadRequest + } + return dirPath, nil +} + func ContainerEnv() []string { return []string{ "HOME=" + config.DefaultDataMount, @@ -184,7 +197,10 @@ func ApplyAction(ctx context.Context, client fileClient, req ActionRequest) erro return bridge.ErrBadRequest } } - dirPath := path.Join(ManagedDirPath, target.Name) + dirPath, err := ManagedSkillDirForName(target.Name) + if err != nil { + return err + } if err := client.Mkdir(ctx, dirPath); err != nil { return err } @@ -247,6 +263,12 @@ func IsValidName(name string) bool { if name == "" { return false } + if name == "." || name == ".." { + return false + } + if strings.HasPrefix(name, ".") || strings.Contains(name, "..") { + return false + } for _, r := range name { switch { case r >= 'a' && r <= 'z': diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index f1ded6a7..bd32bd59 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -2,11 +2,13 @@ package skills import ( "context" + "errors" "io" "slices" "strings" "testing" + "github.com/memohai/memoh/internal/workspace/bridge" pb "github.com/memohai/memoh/internal/workspace/bridgepb" ) @@ -107,6 +109,62 @@ func TestApplyActionAdoptAndDisable(t *testing.T) { } } +func TestApplyActionAdoptRejectsInvalidManagedName(t *testing.T) { + client := newFakeClient() + externalPath := pathJoin("/data/.agents/skills", "escape", "SKILL.md") + client.listings["/data/.agents/skills"] = []*pb.FileEntry{{Path: "escape", IsDir: true}} + client.files[externalPath] = "---\nname: ..\ndescription: Escape\n---\n\n# Escape" + + err := ApplyAction(context.Background(), client, ActionRequest{ + Action: ActionAdopt, + TargetPath: externalPath, + }) + if !errors.Is(err, bridge.ErrBadRequest) { + t.Fatalf("adopt err = %v, want ErrBadRequest", err) + } + if _, ok := client.files[pathJoin(ManagedDirPath, "..", "SKILL.md")]; ok { + t.Fatalf("unexpected managed write for invalid adopted name") + } +} + +func TestIsValidNameRejectsTraversalPatterns(t *testing.T) { + for _, name := range []string{ + "", + ".", + "..", + ".hidden", + "alpha..beta", + "../escape", + "alpha/../beta", + } { + if IsValidName(name) { + t.Fatalf("IsValidName(%q) = true, want false", name) + } + } + + for _, name := range []string{"alpha", "alpha-beta", "alpha_beta", "alpha.beta"} { + if !IsValidName(name) { + t.Fatalf("IsValidName(%q) = false, want true", name) + } + } +} + +func TestManagedSkillDirForNameRejectsEscapingNames(t *testing.T) { + for _, name := range []string{".", "..", ".alpha", "alpha..beta"} { + if _, err := ManagedSkillDirForName(name); !errors.Is(err, bridge.ErrBadRequest) { + t.Fatalf("ManagedSkillDirForName(%q) err = %v, want ErrBadRequest", name, err) + } + } + + dirPath, err := ManagedSkillDirForName("alpha.beta") + if err != nil { + t.Fatalf("ManagedSkillDirForName(valid) returned error: %v", err) + } + if dirPath != pathJoin(ManagedDirPath, "alpha.beta") { + t.Fatalf("ManagedSkillDirForName(valid) = %q, want %q", dirPath, pathJoin(ManagedDirPath, "alpha.beta")) + } +} + func TestDiscoveryRootsMatchCurrentPolicy(t *testing.T) { roots := DiscoveryRoots() want := []Root{