fix: enforce size cap on first preview file, surface corrupted skills, fail on path traversal
- preview: remove 'fetched > 0' guard so the 512KB size cap applies uniformly to all files including the first - update: return skills with corrupted YAML frontmatter with metadataErr set instead of silently dropping them from scan results - installer: fail installation when a path traversal is detected in remote or local skill files instead of silently skipping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
parent
6c7743eaf1
commit
a6f6ab330f
5 changed files with 21 additions and 11 deletions
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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{
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
},
|
||||
},
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue