diff --git a/oci/skills/errors.go b/oci/skills/errors.go new file mode 100644 index 0000000..0437681 --- /dev/null +++ b/oci/skills/errors.go @@ -0,0 +1,37 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skills + +import "errors" + +// Sentinel errors returned (wrapped) by the packager so callers can classify +// failures with errors.Is instead of matching error message strings. The +// underlying error message is preserved at each call site via fmt.Errorf +// with %w; only the classification is added. +var ( + // ErrInvalidSkillDir indicates the skill directory is missing, not a + // directory, or otherwise unsafe to read (e.g. contains path traversal). + ErrInvalidSkillDir = errors.New("invalid skill directory") + + // ErrSkillMDMissing indicates SKILL.md is not present in the skill + // directory. + ErrSkillMDMissing = errors.New("SKILL.md missing") + + // ErrInvalidFrontmatter indicates the SKILL.md YAML frontmatter is + // missing, malformed, oversized, or missing required fields such as + // the skill name. + ErrInvalidFrontmatter = errors.New("invalid SKILL.md frontmatter") + + // ErrTooManyFiles indicates the skill directory exceeds the maximum + // allowed number of files. + ErrTooManyFiles = errors.New("too many files in skill directory") + + // ErrSkillTooLarge indicates the skill directory exceeds the maximum + // allowed total size. + ErrSkillTooLarge = errors.New("skill directory too large") + + // ErrInvalidSkillFile indicates a per-file issue inside the skill + // directory: a symlink, a non-regular file, or an unreadable entry. + ErrInvalidSkillFile = errors.New("invalid skill file") +) diff --git a/oci/skills/packager.go b/oci/skills/packager.go index 9a6f0e0..ab5643f 100644 --- a/oci/skills/packager.go +++ b/oci/skills/packager.go @@ -227,7 +227,7 @@ func readSkillDirectory(dir string) (*skillDirContent, error) { skillMD, err := os.ReadFile(skillMDPath) //#nosec G304 -- path constructed from user-provided skill directory if err != nil { if os.IsNotExist(err) { - return nil, fmt.Errorf("SKILL.md not found in skill directory") + return nil, fmt.Errorf("SKILL.md not found in skill directory: %w", ErrSkillMDMissing) } return nil, fmt.Errorf("reading SKILL.md: %w", err) } @@ -238,7 +238,7 @@ func readSkillDirectory(dir string) (*skillDirContent, error) { } if fm.Name == "" { - return nil, fmt.Errorf("skill name is required in SKILL.md frontmatter") + return nil, fmt.Errorf("skill name is required in SKILL.md frontmatter: %w", ErrInvalidFrontmatter) } files, err := collectSkillFiles(dir) @@ -258,17 +258,17 @@ func validateSkillDir(dir string) error { info, err := os.Stat(dir) if err != nil { if os.IsNotExist(err) { - return fmt.Errorf("skill directory not found: %s", dir) + return fmt.Errorf("skill directory not found: %s: %w", dir, ErrInvalidSkillDir) } - return fmt.Errorf("accessing skill directory: %w", err) + return fmt.Errorf("accessing skill directory: %w: %w", err, ErrInvalidSkillDir) } if !info.IsDir() { - return fmt.Errorf("path is not a directory: %s", dir) + return fmt.Errorf("path is not a directory: %s: %w", dir, ErrInvalidSkillDir) } cleanDir := filepath.Clean(dir) if strings.Contains(cleanDir, "..") { - return fmt.Errorf("invalid path: contains path traversal") + return fmt.Errorf("invalid path: contains path traversal: %w", ErrInvalidSkillDir) } return nil @@ -305,7 +305,7 @@ func collectSkillFiles(dir string) (map[string][]byte, error) { // Security: reject symlinked directories (WalkDir follows them silently) if d.Type()&os.ModeSymlink != 0 { - return fmt.Errorf("symlinks not allowed in skill directory: %s", relPath) + return fmt.Errorf("symlinks not allowed in skill directory: %s: %w", relPath, ErrInvalidSkillFile) } if d.IsDir() { @@ -322,7 +322,7 @@ func collectSkillFiles(dir string) (map[string][]byte, error) { } if len(files) >= maxSkillFiles { - return fmt.Errorf("skill directory exceeds maximum of %d files", maxSkillFiles) + return fmt.Errorf("skill directory exceeds maximum of %d files: %w", maxSkillFiles, ErrTooManyFiles) } content, err := os.ReadFile(path) //#nosec G304,G122 -- path from WalkDir, symlink-checked @@ -332,7 +332,7 @@ func collectSkillFiles(dir string) (map[string][]byte, error) { totalSize += int64(len(content)) if totalSize > maxSkillTotalSize { - return fmt.Errorf("skill directory exceeds maximum total size of %d bytes", maxSkillTotalSize) + return fmt.Errorf("skill directory exceeds maximum total size of %d bytes: %w", maxSkillTotalSize, ErrSkillTooLarge) } files[relPath] = content @@ -351,10 +351,10 @@ func validateSkillFile(absPath, relPath string) error { return fmt.Errorf("checking file type for %s: %w", relPath, err) } if fileInfo.Mode()&os.ModeSymlink != 0 { - return fmt.Errorf("symlinks not allowed in skill directory: %s", relPath) + return fmt.Errorf("symlinks not allowed in skill directory: %s: %w", relPath, ErrInvalidSkillFile) } if !fileInfo.Mode().IsRegular() { - return fmt.Errorf("non-regular file not allowed in skill directory: %s", relPath) + return fmt.Errorf("non-regular file not allowed in skill directory: %s: %w", relPath, ErrInvalidSkillFile) } return nil } @@ -365,7 +365,7 @@ func parseFrontmatter(content []byte) (*frontmatter, error) { delimiter := []byte("---") if !bytes.HasPrefix(content, delimiter) { - return nil, fmt.Errorf("SKILL.md must start with YAML frontmatter (---)") + return nil, fmt.Errorf("SKILL.md must start with YAML frontmatter (---): %w", ErrInvalidFrontmatter) } rest := content[len(delimiter):] @@ -373,18 +373,18 @@ func parseFrontmatter(content []byte) (*frontmatter, error) { endIdx := bytes.Index(rest, delimiter) if endIdx == -1 { - return nil, fmt.Errorf("SKILL.md frontmatter missing closing delimiter (---)") + return nil, fmt.Errorf("SKILL.md frontmatter missing closing delimiter (---): %w", ErrInvalidFrontmatter) } fmBytes := rest[:endIdx] if len(fmBytes) > maxFrontmatterSize { - return nil, fmt.Errorf("frontmatter exceeds maximum size of %d bytes", maxFrontmatterSize) + return nil, fmt.Errorf("frontmatter exceeds maximum size of %d bytes: %w", maxFrontmatterSize, ErrInvalidFrontmatter) } var fm frontmatter if err := yaml.Unmarshal(fmBytes, &fm); err != nil { - return nil, fmt.Errorf("parsing frontmatter YAML: %w", err) + return nil, fmt.Errorf("parsing frontmatter YAML: %w: %w", err, ErrInvalidFrontmatter) } return &fm, nil diff --git a/oci/skills/packager_test.go b/oci/skills/packager_test.go index 3825db1..f8d3b65 100644 --- a/oci/skills/packager_test.go +++ b/oci/skills/packager_test.go @@ -4,6 +4,7 @@ package skills import ( + "bytes" "context" "encoding/json" "fmt" @@ -331,14 +332,7 @@ func TestPackager_Package_RejectsSymlinks(t *testing.T) { t.Parallel() dir := t.TempDir() - skillMD := `--- -name: test-skill -description: A test skill -version: 1.0.0 ---- -# Test Skill -` - require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(skillMD), 0600)) + writeValidSkillMD(t, dir) require.NoError(t, os.Symlink("/etc/passwd", filepath.Join(dir, "evil_link"))) store, err := NewStore(t.TempDir()) @@ -356,14 +350,7 @@ func TestPackager_Package_RejectsSymlinkedDirectory(t *testing.T) { t.Parallel() dir := t.TempDir() - skillMD := `--- -name: test-skill -description: A test skill -version: 1.0.0 ---- -# Test Skill -` - require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(skillMD), 0600)) + writeValidSkillMD(t, dir) require.NoError(t, os.Symlink("/etc", filepath.Join(dir, "evil_dir"))) store, err := NewStore(t.TempDir()) @@ -571,6 +558,187 @@ version: 1.0.0 _, err := collectSkillFiles(dir) require.Error(t, err) assert.Contains(t, err.Error(), "exceeds maximum") + assert.ErrorIs(t, err, ErrTooManyFiles) +} + +func TestPackager_Package_SentinelErrors(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + setup func(t *testing.T) string + wantErr error + }{ + { + name: "missing skill directory", + setup: func(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "does-not-exist") + }, + wantErr: ErrInvalidSkillDir, + }, + { + name: "path is file not directory", + setup: func(t *testing.T) string { + t.Helper() + f := filepath.Join(t.TempDir(), "not-a-dir") + require.NoError(t, os.WriteFile(f, []byte("x"), 0600)) + return f + }, + wantErr: ErrInvalidSkillDir, + }, + { + name: "path contains traversal", + setup: func(_ *testing.T) string { + return "../no-such-skill-dir" + }, + wantErr: ErrInvalidSkillDir, + }, + { + name: "missing SKILL.md", + setup: func(t *testing.T) string { + t.Helper() + return t.TempDir() + }, + wantErr: ErrSkillMDMissing, + }, + { + name: "frontmatter missing opening delimiter", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(dir, "SKILL.md"), + []byte("# no frontmatter\n"), + 0600, + )) + return dir + }, + wantErr: ErrInvalidFrontmatter, + }, + { + name: "frontmatter missing closing delimiter", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(dir, "SKILL.md"), + []byte("---\nname: test\n# never closed"), + 0600, + )) + return dir + }, + wantErr: ErrInvalidFrontmatter, + }, + { + name: "frontmatter exceeds size limit", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + var buf bytes.Buffer + buf.WriteString("---\nname: test\nfiller: ") + buf.Write(bytes.Repeat([]byte("a"), maxFrontmatterSize+1)) + buf.WriteString("\n---\n# body\n") + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), buf.Bytes(), 0600)) + return dir + }, + wantErr: ErrInvalidFrontmatter, + }, + { + name: "frontmatter invalid YAML", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(dir, "SKILL.md"), + []byte("---\nname: [unclosed\n---\n# body\n"), + 0600, + )) + return dir + }, + wantErr: ErrInvalidFrontmatter, + }, + { + name: "frontmatter missing name", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile( + filepath.Join(dir, "SKILL.md"), + []byte("---\ndescription: nameless skill\n---\n# body\n"), + 0600, + )) + return dir + }, + wantErr: ErrInvalidFrontmatter, + }, + { + name: "symlinked file in skill directory", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + writeValidSkillMD(t, dir) + require.NoError(t, os.Symlink("/etc/passwd", filepath.Join(dir, "evil_link"))) + return dir + }, + wantErr: ErrInvalidSkillFile, + }, + { + name: "symlinked directory in skill directory", + setup: func(t *testing.T) string { + t.Helper() + dir := t.TempDir() + writeValidSkillMD(t, dir) + require.NoError(t, os.Symlink("/etc", filepath.Join(dir, "evil_dir"))) + return dir + }, + wantErr: ErrInvalidSkillFile, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + store, err := NewStore(t.TempDir()) + require.NoError(t, err) + packager := NewPackager(store) + opts := PackageOptions{Epoch: time.Unix(0, 0).UTC()} + + _, err = packager.Package(context.Background(), tt.setup(t), opts) + require.Error(t, err) + assert.ErrorIs(t, err, tt.wantErr) + }) + } +} + +// TestCollectSkillFiles_ExceedsMaxSize verifies that the total-size limit +// surfaces ErrSkillTooLarge. Kept separate from the table-driven test because +// it writes >100 MiB to disk. +func TestCollectSkillFiles_ExceedsMaxSize(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + writeValidSkillMD(t, dir) + + // Stream-write a single file just over maxSkillTotalSize using a 1 MiB + // buffer so we don't hold the whole payload in memory at once. + f, err := os.Create(filepath.Join(dir, "big.bin")) //#nosec G304 -- t.TempDir + require.NoError(t, err) + const chunkSize = 1 << 20 // 1 MiB + chunk := make([]byte, chunkSize) + written := int64(0) + for written <= maxSkillTotalSize { + n, werr := f.Write(chunk) + require.NoError(t, werr) + written += int64(n) + } + require.NoError(t, f.Close()) + + _, err = collectSkillFiles(dir) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds maximum total size") + assert.ErrorIs(t, err, ErrSkillTooLarge) } // Helper functions @@ -597,6 +765,19 @@ This is a test skill. return dir } +func writeValidSkillMD(t *testing.T, dir string) { + t.Helper() + + skillMD := `--- +name: test-skill +description: A test skill +version: 1.0.0 +--- +# Test Skill +` + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(skillMD), 0600)) +} + func createTestSkillDirWithScripts(t *testing.T) string { t.Helper()