diff --git a/internal/skills/installer/installer.go b/internal/skills/installer/installer.go index 0ac9e182c..e27d35f5b 100644 --- a/internal/skills/installer/installer.go +++ b/internal/skills/installer/installer.go @@ -216,7 +216,7 @@ func installLocalSkill(sourceRoot string, skill discovery.Skill, baseDir string) if err != nil { var traversalErr safepaths.PathTraversalError if errors.As(err, &traversalErr) { - return nil + return fmt.Errorf("blocked path traversal in %q", relPath) } return fmt.Errorf("could not resolve destination path: %w", err) } @@ -273,7 +273,7 @@ func installSkill(opts *Options, skill discovery.Skill, baseDir string) error { if err != nil { var traversalErr safepaths.PathTraversalError if errors.As(err, &traversalErr) { - continue + return fmt.Errorf("blocked path traversal in %q", relPath) } return fmt.Errorf("could not resolve destination path: %w", err) } diff --git a/internal/skills/installer/installer_test.go b/internal/skills/installer/installer_test.go index 6334add85..e05a3541e 100644 --- a/internal/skills/installer/installer_test.go +++ b/internal/skills/installer/installer_test.go @@ -253,7 +253,7 @@ func TestInstallSkill(t *testing.T) { }, }, { - name: "skips path traversal from malicious tree", + name: "fails on path traversal from malicious tree", skill: discovery.Skill{Name: "code-review", Path: "skills/code-review", TreeSHA: "tree123"}, stubs: func(reg *httpmock.Registry) { reg.Register( @@ -280,10 +280,7 @@ func TestInstallSkill(t *testing.T) { }, verify: func(t *testing.T, destDir string) { t.Helper() - _, err := os.Stat(filepath.Join(destDir, "code-review", "SKILL.md")) - assert.NoError(t, err) - - _, err = os.Stat(filepath.Join(destDir, "..", "etc", "passwd")) + _, err := os.Stat(filepath.Join(destDir, "..", "etc", "passwd")) assert.True(t, os.IsNotExist(err), "traversal path should not be written") }, }, @@ -305,7 +302,12 @@ func TestInstallSkill(t *testing.T) { } err := installSkill(opts, tt.skill, destDir) - require.NoError(t, err) + if tt.name == "fails on path traversal from malicious tree" { + require.Error(t, err) + assert.Contains(t, err.Error(), "blocked path traversal") + } else { + require.NoError(t, err) + } tt.verify(t, destDir) }) } diff --git a/pkg/cmd/skills/preview/preview.go b/pkg/cmd/skills/preview/preview.go index 270912478..319288f17 100644 --- a/pkg/cmd/skills/preview/preview.go +++ b/pkg/cmd/skills/preview/preview.go @@ -216,7 +216,7 @@ func renderAllFiles(opts *PreviewOptions, cs *iostreams.ColorScheme, skill disco fmt.Fprintf(out, "\n%s\n", cs.Muted(fmt.Sprintf("(skipped remaining files, showing first %d)", maxFiles))) break } - if totalBytes+f.Size > maxTotalBytes && fetched > 0 { + if totalBytes+f.Size > maxTotalBytes { fmt.Fprintf(out, "\n%s\n", cs.Muted("(skipped remaining files, size limit reached)")) break } diff --git a/pkg/cmd/skills/update/update.go b/pkg/cmd/skills/update/update.go index 766f52515..ad7c647d5 100644 --- a/pkg/cmd/skills/update/update.go +++ b/pkg/cmd/skills/update/update.go @@ -510,7 +510,13 @@ func scanInstalledSkills(skillsDir string, host *registry.AgentHost, scope regis func parseInstalledSkill(data []byte, name, dir string, host *registry.AgentHost, scope registry.Scope) (installedSkill, bool) { result, err := frontmatter.Parse(string(data)) if err != nil { - return installedSkill{}, false + return installedSkill{ + name: name, + dir: dir, + host: host, + scope: scope, + metadataErr: fmt.Errorf("invalid SKILL.md: %w", err), + }, true } s := installedSkill{ diff --git a/pkg/cmd/skills/update/update_test.go b/pkg/cmd/skills/update/update_test.go index 0c224e9a0..86fdcaa80 100644 --- a/pkg/cmd/skills/update/update_test.go +++ b/pkg/cmd/skills/update/update_test.go @@ -199,7 +199,9 @@ func TestScanInstalledSkills(t *testing.T) { verify: func(t *testing.T, skills []installedSkill, err error) { t.Helper() require.NoError(t, err) - assert.Len(t, skills, 0) + require.Len(t, skills, 1) + assert.Equal(t, "corrupt", skills[0].name) + assert.ErrorContains(t, skills[0].metadataErr, "invalid SKILL.md") }, }, }