From 7108a3c6f1d69eb821e57e5b0bfde077535ce84a Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Mon, 13 Apr 2026 03:14:02 +0300 Subject: [PATCH 1/2] Add skill import from URL/git (#43) Add `skern skill import ` command to import skills from GitHub repository directories and gists. Supports --name override, --scope, --force, overlap detection, companion files, and JSON output. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/cli/context.go | 3 + internal/cli/skill.go | 1 + internal/cli/skill_import.go | 164 +++++++++++++++ internal/cli/skill_import_test.go | 259 ++++++++++++++++++++++++ internal/output/types_skill.go | 8 + internal/registry/registry.go | 55 ++++- internal/skill/importer.go | 307 ++++++++++++++++++++++++++++ internal/skill/importer_test.go | 325 ++++++++++++++++++++++++++++++ internal/skill/skill.go | 3 + 9 files changed, 1119 insertions(+), 6 deletions(-) create mode 100644 internal/cli/skill_import.go create mode 100644 internal/cli/skill_import_test.go create mode 100644 internal/skill/importer.go create mode 100644 internal/skill/importer_test.go diff --git a/internal/cli/context.go b/internal/cli/context.go index d1f27b6..1e9d328 100644 --- a/internal/cli/context.go +++ b/internal/cli/context.go @@ -6,6 +6,7 @@ import ( "github.com/devrimcavusoglu/skern/internal/output" "github.com/devrimcavusoglu/skern/internal/platform" "github.com/devrimcavusoglu/skern/internal/registry" + "github.com/devrimcavusoglu/skern/internal/skill" "github.com/spf13/cobra" ) @@ -16,6 +17,8 @@ type CommandContext struct { Printer *output.Printer NewRegistry func() (*registry.Registry, error) NewDetector func() (*platform.Detector, error) + HTTPClient skill.HTTPClient // optional; defaults to http.DefaultClient + GitHubBaseURL string // optional; override GitHub API base URL (for testing) } func setContext(cmd *cobra.Command, cc *CommandContext) { diff --git a/internal/cli/skill.go b/internal/cli/skill.go index 84bf671..329c2bf 100644 --- a/internal/cli/skill.go +++ b/internal/cli/skill.go @@ -23,6 +23,7 @@ func newSkillCmd() *cobra.Command { cmd.AddCommand(newSkillRecommendCmd()) cmd.AddCommand(newSkillDiffCmd()) cmd.AddCommand(newSkillVersionCmd()) + cmd.AddCommand(newSkillImportCmd()) return cmd } diff --git a/internal/cli/skill_import.go b/internal/cli/skill_import.go new file mode 100644 index 0000000..23adb2a --- /dev/null +++ b/internal/cli/skill_import.go @@ -0,0 +1,164 @@ +package cli + +import ( + "fmt" + "net/http" + + "github.com/devrimcavusoglu/skern/internal/output" + "github.com/devrimcavusoglu/skern/internal/overlap" + "github.com/devrimcavusoglu/skern/internal/skill" + "github.com/spf13/cobra" +) + +func newSkillImportCmd() *cobra.Command { + var ( + scope string + name string + force bool + ) + + cmd := &cobra.Command{ + Use: "import ", + Short: "Import a skill from a remote URL", + Long: "Import a skill from a GitHub repository directory or gist into the local registry.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + ctx := getContext(cmd) + rawURL := args[0] + + src, err := skill.ParseImportURL(rawURL) + if err != nil { + return &ValidationError{Message: err.Error()} + } + + scopeVal, err := parseScope(scope) + if err != nil { + return err + } + + // Fetch skill files from remote + ctx.Printer.Print("Fetching skill from %s...\n", rawURL) + httpClient := ctx.HTTPClient + if httpClient == nil { + httpClient = http.DefaultClient + } + var fetched *skill.FetchedSkill + if ctx.GitHubBaseURL != "" { + fetched, err = skill.FetchSkillWithBaseURL(httpClient, src, ctx.GitHubBaseURL) + } else { + fetched, err = skill.FetchSkill(httpClient, src) + } + if err != nil { + return fmt.Errorf("fetching skill: %w", err) + } + + // Parse the downloaded SKILL.md + manifestData, ok := fetched.Files["SKILL.md"] + if !ok { + return fmt.Errorf("no SKILL.md found in fetched files") + } + + s, err := skill.ParseManifestFromBytes(manifestData) + if err != nil { + return fmt.Errorf("parsing imported manifest: %w", err) + } + + // Apply --name override + if name != "" { + if err := skill.ValidateName(name); err != nil { + return &ValidationError{Message: err.Error()} + } + s.Name = name + } + + if s.Name == "" { + return &ValidationError{Message: "imported skill has no name; use --name to specify one"} + } + + if err := skill.ValidateName(s.Name); err != nil { + return &ValidationError{Message: fmt.Sprintf("imported skill name is invalid: %s", err)} + } + + reg, err := ctx.NewRegistry() + if err != nil { + return err + } + + // Overlap detection + discovered, _, err := reg.ListAll() + if err != nil { + return fmt.Errorf("checking for overlapping skills: %w", err) + } + + if len(discovered) > 0 { + var existing []skill.Skill + var scopes []skill.Scope + for _, d := range discovered { + existing = append(existing, d.Skill) + scopes = append(scopes, d.Scope) + } + + matches := overlap.Check(s.Name, s.Description, existing, scopes) + if len(matches) > 0 { + blocked := overlap.ShouldBlock(matches) && !force + + if blocked { + maxScore := overlap.MaxScore(matches) + text := formatOverlapBlock(s.Name, matches) + overlapResult := output.OverlapCheckResult{ + Blocked: true, + MaxScore: maxScore, + } + for _, m := range matches { + overlapResult.Matches = append(overlapResult.Matches, output.OverlapResult{ + Name: m.Name, + Score: m.Score, + Scope: string(m.Scope), + }) + } + ctx.Printer.PrintResult(overlapResult, text) + return &ValidationError{Message: fmt.Sprintf("skill %q blocked due to near-duplicate (score %.2f); use --force to override", s.Name, maxScore)} + } + + // Warn but proceed + text := formatOverlapWarn(s.Name, matches) + ctx.Printer.Print("%s", text) + } + } + + // Import into registry + path, err := reg.Import(s, fetched.Files, scopeVal, force) + if err != nil { + return err + } + + result := output.SkillImportResult{ + Name: s.Name, + Scope: scope, + Path: path, + Source: rawURL, + } + text := fmt.Sprintf("Imported skill %q from %s into %s scope at %s\n", s.Name, formatSource(src), scope, path) + ctx.Printer.PrintResult(result, text) + return nil + }, + } + + cmd.Flags().StringVar(&scope, "scope", "user", "skill scope (user or project)") + cmd.Flags().StringVar(&name, "name", "", "override the skill name from the manifest") + cmd.Flags().BoolVar(&force, "force", false, "overwrite if skill already exists and bypass overlap block") + + return cmd +} + +func formatSource(src *skill.ImportSource) string { + switch src.Type { + case skill.SourceGitHubRepo: + return fmt.Sprintf("github.com/%s/%s", src.Owner, src.Repo) + case skill.SourceGitHubGist: + return fmt.Sprintf("gist %s", src.GistID) + default: + return src.RawURL + } +} + diff --git a/internal/cli/skill_import_test.go b/internal/cli/skill_import_test.go new file mode 100644 index 0000000..37c539b --- /dev/null +++ b/internal/cli/skill_import_test.go @@ -0,0 +1,259 @@ +package cli + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + + "github.com/devrimcavusoglu/skern/internal/output" + "github.com/devrimcavusoglu/skern/internal/registry" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// validSkillManifest returns a valid SKILL.md content for testing. +func validSkillManifest() string { + return `--- +name: test-skill +description: A test skill for import testing +metadata: + author: + name: alice + type: human + version: "1.0.0" +--- + +## Instructions + +Do the thing. +` +} + +// newImportTestServer creates a mock GitHub API server that serves a skill directory. +func newImportTestServer(t *testing.T, manifest string, companions map[string]string) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + + // GitHub repo contents endpoint + mux.HandleFunc("/repos/owner/repo/contents/skills/test-skill", func(w http.ResponseWriter, r *http.Request) { + entries := []map[string]interface{}{ + {"name": "SKILL.md", "type": "file", "download_url": fmt.Sprintf("http://%s/raw/SKILL.md", r.Host)}, + } + for name := range companions { + entries = append(entries, map[string]interface{}{ + "name": name, + "type": "file", + "download_url": fmt.Sprintf("http://%s/raw/%s", r.Host, name), + }) + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(entries) + }) + + mux.HandleFunc("/raw/SKILL.md", func(w http.ResponseWriter, _ *http.Request) { + w.Write([]byte(manifest)) + }) + for name, content := range companions { + name, content := name, content + mux.HandleFunc("/raw/"+name, func(w http.ResponseWriter, _ *http.Request) { + w.Write([]byte(content)) + }) + } + + // Gist endpoint + mux.HandleFunc("/gists/test-gist", func(w http.ResponseWriter, _ *http.Request) { + gistFiles := map[string]interface{}{ + "SKILL.md": map[string]interface{}{ + "filename": "SKILL.md", + "content": manifest, + }, + } + for name, content := range companions { + gistFiles[name] = map[string]interface{}{ + "filename": name, + "content": content, + } + } + gist := map[string]interface{}{"files": gistFiles} + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(gist) + }) + + return httptest.NewServer(mux) +} + +func testImportContext(t *testing.T, srv *httptest.Server) *CommandContext { + t.Helper() + userDir := filepath.Join(t.TempDir(), "user-skills") + projectDir := filepath.Join(t.TempDir(), "project-skills") + + return &CommandContext{ + NewRegistry: func() (*registry.Registry, error) { + return registry.New(userDir, projectDir), nil + }, + NewDetector: defaultNewDetector, + HTTPClient: srv.Client(), + GitHubBaseURL: srv.URL, + } +} + +func TestSkillImport_GitHubRepo(t *testing.T) { + srv := newImportTestServer(t, validSkillManifest(), nil) + defer srv.Close() + + cc := testImportContext(t, srv) + out, err := runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill") + require.NoError(t, err) + assert.Contains(t, out, "Imported skill") + assert.Contains(t, out, "test-skill") +} + +func TestSkillImport_GitHubRepo_JSON(t *testing.T) { + srv := newImportTestServer(t, validSkillManifest(), nil) + defer srv.Close() + + cc := testImportContext(t, srv) + out, err := runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill", "--json") + require.NoError(t, err) + + var result output.SkillImportResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + assert.Equal(t, "test-skill", result.Name) + assert.Equal(t, "user", result.Scope) + assert.NotEmpty(t, result.Path) + assert.Contains(t, result.Source, "github.com") +} + +func TestSkillImport_GitHubGist(t *testing.T) { + srv := newImportTestServer(t, validSkillManifest(), nil) + defer srv.Close() + + cc := testImportContext(t, srv) + out, err := runCmd(t, cc, "skill", "import", + "https://gist.github.com/alice/test-gist") + require.NoError(t, err) + assert.Contains(t, out, "Imported skill") + assert.Contains(t, out, "test-skill") +} + +func TestSkillImport_WithCompanionFiles(t *testing.T) { + companions := map[string]string{ + "helper.md": "# Helper\nSome content", + } + srv := newImportTestServer(t, validSkillManifest(), companions) + defer srv.Close() + + cc := testImportContext(t, srv) + out, err := runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill", "--json") + require.NoError(t, err) + + var result output.SkillImportResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + + // Verify companion file was written + assert.FileExists(t, filepath.Join(result.Path, "helper.md")) +} + +func TestSkillImport_NameOverride(t *testing.T) { + srv := newImportTestServer(t, validSkillManifest(), nil) + defer srv.Close() + + cc := testImportContext(t, srv) + out, err := runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill", + "--name", "renamed-skill", "--json") + require.NoError(t, err) + + var result output.SkillImportResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + assert.Equal(t, "renamed-skill", result.Name) +} + +func TestSkillImport_InvalidNameOverride(t *testing.T) { + srv := newImportTestServer(t, validSkillManifest(), nil) + defer srv.Close() + + cc := testImportContext(t, srv) + _, err := runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill", + "--name", "INVALID_NAME") + assert.Error(t, err) +} + +func TestSkillImport_ProjectScope(t *testing.T) { + srv := newImportTestServer(t, validSkillManifest(), nil) + defer srv.Close() + + cc := testImportContext(t, srv) + out, err := runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill", + "--scope", "project", "--json") + require.NoError(t, err) + + var result output.SkillImportResult + require.NoError(t, json.Unmarshal([]byte(out), &result)) + assert.Equal(t, "project", result.Scope) +} + +func TestSkillImport_AlreadyExists(t *testing.T) { + srv := newImportTestServer(t, validSkillManifest(), nil) + defer srv.Close() + + cc := testImportContext(t, srv) + + // First import + _, err := runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill") + require.NoError(t, err) + + // Second import without --force should fail + _, err = runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill") + assert.Error(t, err) + assert.Contains(t, err.Error(), "already exists") +} + +func TestSkillImport_Force(t *testing.T) { + srv := newImportTestServer(t, validSkillManifest(), nil) + defer srv.Close() + + cc := testImportContext(t, srv) + + // First import + _, err := runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill") + require.NoError(t, err) + + // Second import with --force should succeed + _, err = runCmd(t, cc, "skill", "import", + "https://github.com/owner/repo/tree/main/skills/test-skill", "--force") + require.NoError(t, err) +} + +func TestSkillImport_InvalidURL(t *testing.T) { + cc := testRegistry(t) + + _, err := runCmd(t, cc, "skill", "import", "not-a-url") + assert.Error(t, err) +} + +func TestSkillImport_UnsupportedHost(t *testing.T) { + cc := testRegistry(t) + + _, err := runCmd(t, cc, "skill", "import", "https://gitlab.com/owner/repo/tree/main/skill") + assert.Error(t, err) + assert.Contains(t, err.Error(), "unsupported host") +} + +func TestSkillImport_NoArgs(t *testing.T) { + cc := testRegistry(t) + + _, err := runCmd(t, cc, "skill", "import") + assert.Error(t, err) +} diff --git a/internal/output/types_skill.go b/internal/output/types_skill.go index e345c0c..18bdd16 100644 --- a/internal/output/types_skill.go +++ b/internal/output/types_skill.go @@ -136,6 +136,14 @@ type SkillVersionResult struct { Bumped bool `json:"bumped"` } +// SkillImportResult is the JSON envelope for skill import output. +type SkillImportResult struct { + Name string `json:"name"` + Scope string `json:"scope"` + Path string `json:"path"` + Source string `json:"source"` +} + // VersionCompareResult is the JSON envelope for version comparison output. type VersionCompareResult struct { Installed string `json:"installed"` diff --git a/internal/registry/registry.go b/internal/registry/registry.go index 1c4987f..482fec4 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -9,8 +9,6 @@ import ( "github.com/devrimcavusoglu/skern/internal/skill" ) -const manifestFile = "SKILL.md" - // ParseWarning records a skill directory that could not be parsed. type ParseWarning struct { Name string `json:"name"` @@ -49,7 +47,7 @@ func (r *Registry) Create(s *skill.Skill, scope skill.Scope) (string, error) { return "", fmt.Errorf("creating skill directory: %w", err) } - manifestPath := filepath.Join(skillDir, manifestFile) + manifestPath := filepath.Join(skillDir, skill.ManifestFile) if err := skill.WriteManifest(s, manifestPath); err != nil { // Clean up on failure _ = os.RemoveAll(skillDir) @@ -68,7 +66,7 @@ func (r *Registry) Get(name string, scope skill.Scope) (*skill.Skill, string, er dir := r.scopeDir(scope) skillDir := filepath.Join(dir, name) - manifestPath := filepath.Join(skillDir, manifestFile) + manifestPath := filepath.Join(skillDir, skill.ManifestFile) s, err := skill.ParseManifest(manifestPath) if err != nil { @@ -114,7 +112,7 @@ func (r *Registry) List(scope skill.Scope) ([]skill.Skill, []ParseWarning, error continue } - manifestPath := filepath.Join(dir, entry.Name(), manifestFile) + manifestPath := filepath.Join(dir, entry.Name(), skill.ManifestFile) s, parseErr := skill.ParseManifest(manifestPath) if parseErr != nil { warnings = append(warnings, ParseWarning{ @@ -134,10 +132,55 @@ func (r *Registry) List(scope skill.Scope) ([]skill.Skill, []ParseWarning, error func (r *Registry) Exists(name string, scope skill.Scope) bool { dir := r.scopeDir(scope) skillDir := filepath.Join(dir, name) - _, err := os.Stat(filepath.Join(skillDir, manifestFile)) + _, err := os.Stat(filepath.Join(skillDir, skill.ManifestFile)) return err == nil } +// Import writes a remotely fetched skill and its companion files into the registry. +// If force is true and the skill already exists, it is removed first. +func (r *Registry) Import(s *skill.Skill, files map[string][]byte, scope skill.Scope, force bool) (string, error) { + if err := skill.ValidateName(s.Name); err != nil { + return "", err + } + + dir := r.scopeDir(scope) + skillDir := filepath.Join(dir, s.Name) + + if _, err := os.Stat(skillDir); err == nil { + if !force { + return "", fmt.Errorf("skill %q already exists in %s scope; use --force to overwrite", s.Name, scope) + } + if err := os.RemoveAll(skillDir); err != nil { + return "", fmt.Errorf("removing existing skill: %w", err) + } + } + + if err := os.MkdirAll(skillDir, 0o755); err != nil { + return "", fmt.Errorf("creating skill directory: %w", err) + } + + // Write SKILL.md via manifest serializer + manifestPath := filepath.Join(skillDir, skill.ManifestFile) + if err := skill.WriteManifest(s, manifestPath); err != nil { + _ = os.RemoveAll(skillDir) + return "", fmt.Errorf("writing manifest: %w", err) + } + + // Write companion files (skip SKILL.md since we already wrote it via WriteManifest) + for name, content := range files { + if name == skill.ManifestFile { + continue + } + filePath := filepath.Join(skillDir, name) + if err := os.WriteFile(filePath, content, 0o644); err != nil { + _ = os.RemoveAll(skillDir) + return "", fmt.Errorf("writing companion file %q: %w", name, err) + } + } + + return skillDir, nil +} + func (r *Registry) scopeDir(scope skill.Scope) string { if scope == skill.ScopeProject { return r.projectDir diff --git a/internal/skill/importer.go b/internal/skill/importer.go new file mode 100644 index 0000000..fa2b54b --- /dev/null +++ b/internal/skill/importer.go @@ -0,0 +1,307 @@ +package skill + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + + "gopkg.in/yaml.v3" +) + +// SourceType identifies the kind of remote source for skill import. +type SourceType string + +const ( + // SourceGitHubRepo is a GitHub repository directory URL. + SourceGitHubRepo SourceType = "github-repo" + // SourceGitHubGist is a GitHub gist URL. + SourceGitHubGist SourceType = "github-gist" +) + +// ImportSource holds parsed information about a remote skill source. +type ImportSource struct { + Type SourceType + RawURL string + // GitHub repo fields + Owner string + Repo string + Ref string + Path string + // GitHub gist fields + GistID string +} + +// FetchedSkill holds the downloaded files from a remote skill source. +type FetchedSkill struct { + Files map[string][]byte // filename -> content +} + +// HTTPClient is the interface for making HTTP requests, allowing test injection. +type HTTPClient interface { + Do(req *http.Request) (*http.Response, error) +} + +// ParseImportURL parses a URL and returns an ImportSource describing the remote skill location. +// Supported formats: +// - GitHub repo directory: https://github.com///tree// +// - GitHub gist: https://gist.github.com// +func ParseImportURL(rawURL string) (*ImportSource, error) { + u, err := url.Parse(rawURL) + if err != nil { + return nil, fmt.Errorf("invalid URL: %w", err) + } + + if u.Scheme != "http" && u.Scheme != "https" { + return nil, fmt.Errorf("unsupported URL scheme %q; use https", u.Scheme) + } + + host := strings.ToLower(u.Host) + + switch { + case host == "gist.github.com": + return parseGistURL(u, rawURL) + case host == "github.com": + return parseGitHubRepoURL(u, rawURL) + default: + return nil, fmt.Errorf("unsupported host %q; supported sources: github.com (repo directory), gist.github.com", host) + } +} + +func parseGitHubRepoURL(u *url.URL, rawURL string) (*ImportSource, error) { + // Expected format: ///tree// + parts := strings.SplitN(strings.TrimPrefix(u.Path, "/"), "/", 5) + if len(parts) < 4 || parts[2] != "tree" { + return nil, fmt.Errorf("expected GitHub directory URL format: https://github.com///tree//") + } + + owner := parts[0] + repo := parts[1] + ref := parts[3] + path := "" + if len(parts) == 5 { + path = parts[4] + } + + if owner == "" || repo == "" || ref == "" { + return nil, fmt.Errorf("invalid GitHub URL: owner, repo, and ref are required") + } + + return &ImportSource{ + Type: SourceGitHubRepo, + RawURL: rawURL, + Owner: owner, + Repo: repo, + Ref: ref, + Path: path, + }, nil +} + +func parseGistURL(u *url.URL, rawURL string) (*ImportSource, error) { + // Expected format: // or / + parts := strings.Split(strings.TrimPrefix(u.Path, "/"), "/") + if len(parts) == 0 || (len(parts) == 1 && parts[0] == "") { + return nil, fmt.Errorf("expected GitHub gist URL format: https://gist.github.com//") + } + + var gistID string + if len(parts) >= 2 { + gistID = parts[1] + } else { + gistID = parts[0] + } + + if gistID == "" { + return nil, fmt.Errorf("expected GitHub gist URL format: https://gist.github.com//") + } + + return &ImportSource{ + Type: SourceGitHubGist, + RawURL: rawURL, + GistID: gistID, + }, nil +} + +// FetchSkill downloads all files from the remote skill source. +func FetchSkill(client HTTPClient, src *ImportSource) (*FetchedSkill, error) { + switch src.Type { + case SourceGitHubRepo: + return fetchFromGitHubRepo(client, src) + case SourceGitHubGist: + return fetchFromGitHubGist(client, src) + default: + return nil, fmt.Errorf("unsupported source type: %s", src.Type) + } +} + +// FetchSkillWithBaseURL is like FetchSkill but allows overriding the API base URL (for testing). +func FetchSkillWithBaseURL(client HTTPClient, src *ImportSource, baseURL string) (*FetchedSkill, error) { + switch src.Type { + case SourceGitHubRepo: + return fetchFromGitHubRepoWithBase(client, src, baseURL) + case SourceGitHubGist: + return fetchFromGitHubGistWithBase(client, src, baseURL) + default: + return nil, fmt.Errorf("unsupported source type: %s", src.Type) + } +} + +func fetchFromGitHubRepo(client HTTPClient, src *ImportSource) (*FetchedSkill, error) { + return fetchFromGitHubRepoWithBase(client, src, "https://api.github.com") +} + +func fetchFromGitHubRepoWithBase(client HTTPClient, src *ImportSource, baseURL string) (*FetchedSkill, error) { + // GET /repos/{owner}/{repo}/contents/{path}?ref={ref} + apiURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s?ref=%s", + baseURL, src.Owner, src.Repo, src.Path, url.QueryEscape(src.Ref)) + + req, err := http.NewRequest("GET", apiURL, nil) + if err != nil { + return nil, fmt.Errorf("creating request: %w", err) + } + req.Header.Set("Accept", "application/vnd.github.v3+json") + + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("fetching directory listing: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("GitHub API returned %d for %s", resp.StatusCode, apiURL) + } + + var entries []struct { + Name string `json:"name"` + Type string `json:"type"` + DownloadURL string `json:"download_url"` + } + if err := json.NewDecoder(resp.Body).Decode(&entries); err != nil { + return nil, fmt.Errorf("parsing directory listing: %w", err) + } + + files := make(map[string][]byte) + hasManifest := false + + for _, entry := range entries { + if entry.Type != "file" { + continue // skip subdirectories + } + if entry.DownloadURL == "" { + continue + } + if entry.Name == ManifestFile { + hasManifest = true + } + + content, err := downloadFile(client, entry.DownloadURL) + if err != nil { + return nil, fmt.Errorf("downloading %s: %w", entry.Name, err) + } + files[entry.Name] = content + } + + if !hasManifest { + return nil, fmt.Errorf("no SKILL.md found in remote directory") + } + + return &FetchedSkill{Files: files}, nil +} + +func fetchFromGitHubGist(client HTTPClient, src *ImportSource) (*FetchedSkill, error) { + return fetchFromGitHubGistWithBase(client, src, "https://api.github.com") +} + +func fetchFromGitHubGistWithBase(client HTTPClient, src *ImportSource, baseURL string) (*FetchedSkill, error) { + apiURL := fmt.Sprintf("%s/gists/%s", baseURL, src.GistID) + + req, err := http.NewRequest("GET", apiURL, nil) + if err != nil { + return nil, fmt.Errorf("creating request: %w", err) + } + req.Header.Set("Accept", "application/vnd.github.v3+json") + + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("fetching gist: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("GitHub API returned %d for gist %s", resp.StatusCode, src.GistID) + } + + var gist struct { + Files map[string]struct { + Filename string `json:"filename"` + Content string `json:"content"` + } `json:"files"` + } + if err := json.NewDecoder(resp.Body).Decode(&gist); err != nil { + return nil, fmt.Errorf("parsing gist response: %w", err) + } + + files := make(map[string][]byte) + hasManifest := false + + for _, f := range gist.Files { + if f.Filename == ManifestFile { + hasManifest = true + } + files[f.Filename] = []byte(f.Content) + } + + if !hasManifest { + return nil, fmt.Errorf("no SKILL.md found in gist") + } + + return &FetchedSkill{Files: files}, nil +} + +func downloadFile(client HTTPClient, fileURL string) ([]byte, error) { + req, err := http.NewRequest("GET", fileURL, nil) + if err != nil { + return nil, err + } + + resp, err := client.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("HTTP %d for %s", resp.StatusCode, fileURL) + } + + return io.ReadAll(resp.Body) +} + +// ParseManifestFromBytes parses a SKILL.md from raw bytes (for imported content). +func ParseManifestFromBytes(data []byte) (*Skill, error) { + content := string(data) + if len(strings.TrimSpace(content)) == 0 { + return nil, fmt.Errorf("manifest file is empty") + } + + fm, body, err := splitFrontmatter(content) + if err != nil { + return nil, err + } + + var f frontmatter + if err := yaml.Unmarshal([]byte(fm), &f); err != nil { + return nil, fmt.Errorf("parsing YAML frontmatter: %w", err) + } + + return &Skill{ + Name: f.Name, + Description: f.Description, + Tags: f.Tags, + AllowedTools: f.AllowedTools, + Metadata: f.Metadata, + Body: body, + }, nil +} diff --git a/internal/skill/importer_test.go b/internal/skill/importer_test.go new file mode 100644 index 0000000..2064cc5 --- /dev/null +++ b/internal/skill/importer_test.go @@ -0,0 +1,325 @@ +package skill + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseImportURL(t *testing.T) { + tests := []struct { + name string + url string + want *ImportSource + wantErr string + }{ + { + name: "github repo with path", + url: "https://github.com/owner/repo/tree/main/skills/my-skill", + want: &ImportSource{ + Type: SourceGitHubRepo, + RawURL: "https://github.com/owner/repo/tree/main/skills/my-skill", + Owner: "owner", + Repo: "repo", + Ref: "main", + Path: "skills/my-skill", + }, + }, + { + name: "github repo with branch ref", + url: "https://github.com/org/project/tree/feature/v2/path/to/skill", + want: &ImportSource{ + Type: SourceGitHubRepo, + RawURL: "https://github.com/org/project/tree/feature/v2/path/to/skill", + Owner: "org", + Repo: "project", + Ref: "feature", + Path: "v2/path/to/skill", + }, + }, + { + name: "github repo root tree", + url: "https://github.com/owner/repo/tree/main", + want: &ImportSource{ + Type: SourceGitHubRepo, + RawURL: "https://github.com/owner/repo/tree/main", + Owner: "owner", + Repo: "repo", + Ref: "main", + Path: "", + }, + }, + { + name: "github gist with user", + url: "https://gist.github.com/alice/abc123def456", + want: &ImportSource{ + Type: SourceGitHubGist, + RawURL: "https://gist.github.com/alice/abc123def456", + GistID: "abc123def456", + }, + }, + { + name: "github gist without user", + url: "https://gist.github.com/abc123def456", + want: &ImportSource{ + Type: SourceGitHubGist, + RawURL: "https://gist.github.com/abc123def456", + GistID: "abc123def456", + }, + }, + { + name: "unsupported host", + url: "https://gitlab.com/owner/repo/tree/main/skill", + wantErr: "unsupported host", + }, + { + name: "github repo without tree", + url: "https://github.com/owner/repo", + wantErr: "expected GitHub directory URL format", + }, + { + name: "github repo blob instead of tree", + url: "https://github.com/owner/repo/blob/main/SKILL.md", + wantErr: "expected GitHub directory URL format", + }, + { + name: "no scheme", + url: "github.com/owner/repo/tree/main/skill", + wantErr: "unsupported URL scheme", + }, + { + name: "ftp scheme", + url: "ftp://github.com/owner/repo/tree/main/skill", + wantErr: "unsupported URL scheme", + }, + { + name: "empty gist path", + url: "https://gist.github.com/", + wantErr: "expected GitHub gist URL format", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParseImportURL(tt.url) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want.Type, got.Type) + assert.Equal(t, tt.want.RawURL, got.RawURL) + assert.Equal(t, tt.want.Owner, got.Owner) + assert.Equal(t, tt.want.Repo, got.Repo) + assert.Equal(t, tt.want.Ref, got.Ref) + assert.Equal(t, tt.want.Path, got.Path) + assert.Equal(t, tt.want.GistID, got.GistID) + }) + } +} + +// validManifestContent returns a valid SKILL.md content for testing. +func validManifestContent() string { + return `--- +name: test-skill +description: A test skill for import testing +metadata: + author: + name: alice + type: human + version: "1.0.0" +--- + +## Instructions + +Do the thing. +` +} + +func TestFetchSkill_GitHubRepo(t *testing.T) { + manifest := validManifestContent() + companion := "# Helper\nSome helper content" + + // Mock GitHub API: contents endpoint returns a directory listing + mux := http.NewServeMux() + mux.HandleFunc("/repos/owner/repo/contents/skills/my-skill", func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "main", r.URL.Query().Get("ref")) + entries := []map[string]interface{}{ + {"name": "SKILL.md", "type": "file", "download_url": fmt.Sprintf("http://%s/raw/SKILL.md", r.Host)}, + {"name": "helper.md", "type": "file", "download_url": fmt.Sprintf("http://%s/raw/helper.md", r.Host)}, + {"name": "subdir", "type": "dir", "download_url": ""}, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(entries) + }) + mux.HandleFunc("/raw/SKILL.md", func(w http.ResponseWriter, _ *http.Request) { + w.Write([]byte(manifest)) + }) + mux.HandleFunc("/raw/helper.md", func(w http.ResponseWriter, _ *http.Request) { + w.Write([]byte(companion)) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + src := &ImportSource{ + Type: SourceGitHubRepo, + RawURL: "https://github.com/owner/repo/tree/main/skills/my-skill", + Owner: "owner", + Repo: "repo", + Ref: "main", + Path: "skills/my-skill", + } + + fetched, err := FetchSkillWithBaseURL(srv.Client(), src, srv.URL) + require.NoError(t, err) + assert.Len(t, fetched.Files, 2) // SKILL.md + helper.md (subdir skipped) + assert.Equal(t, []byte(manifest), fetched.Files["SKILL.md"]) + assert.Equal(t, []byte(companion), fetched.Files["helper.md"]) +} + +func TestFetchSkill_GitHubRepo_NoManifest(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/repos/owner/repo/contents/skill-dir", func(w http.ResponseWriter, r *http.Request) { + entries := []map[string]interface{}{ + {"name": "README.md", "type": "file", "download_url": fmt.Sprintf("http://%s/raw/README.md", r.Host)}, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(entries) + }) + mux.HandleFunc("/raw/README.md", func(w http.ResponseWriter, _ *http.Request) { + w.Write([]byte("just a readme")) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + src := &ImportSource{ + Type: SourceGitHubRepo, + Owner: "owner", + Repo: "repo", + Ref: "main", + Path: "skill-dir", + } + + _, err := FetchSkillWithBaseURL(srv.Client(), src, srv.URL) + require.Error(t, err) + assert.Contains(t, err.Error(), "no SKILL.md found") +} + +func TestFetchSkill_GitHubRepo_APIError(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/repos/owner/repo/contents/bad-path", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + src := &ImportSource{ + Type: SourceGitHubRepo, + Owner: "owner", + Repo: "repo", + Ref: "main", + Path: "bad-path", + } + + _, err := FetchSkillWithBaseURL(srv.Client(), src, srv.URL) + require.Error(t, err) + assert.Contains(t, err.Error(), "404") +} + +func TestFetchSkill_GitHubGist(t *testing.T) { + manifest := validManifestContent() + + mux := http.NewServeMux() + mux.HandleFunc("/gists/abc123", func(w http.ResponseWriter, _ *http.Request) { + gist := map[string]interface{}{ + "files": map[string]interface{}{ + "SKILL.md": map[string]interface{}{ + "filename": "SKILL.md", + "content": manifest, + }, + "config.yaml": map[string]interface{}{ + "filename": "config.yaml", + "content": "key: value", + }, + }, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(gist) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + src := &ImportSource{ + Type: SourceGitHubGist, + RawURL: "https://gist.github.com/alice/abc123", + GistID: "abc123", + } + + fetched, err := FetchSkillWithBaseURL(srv.Client(), src, srv.URL) + require.NoError(t, err) + assert.Len(t, fetched.Files, 2) + assert.Equal(t, []byte(manifest), fetched.Files["SKILL.md"]) + assert.Equal(t, []byte("key: value"), fetched.Files["config.yaml"]) +} + +func TestFetchSkill_GitHubGist_NoManifest(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/gists/no-manifest", func(w http.ResponseWriter, _ *http.Request) { + gist := map[string]interface{}{ + "files": map[string]interface{}{ + "README.md": map[string]interface{}{ + "filename": "README.md", + "content": "just a readme", + }, + }, + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(gist) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + src := &ImportSource{ + Type: SourceGitHubGist, + GistID: "no-manifest", + } + + _, err := FetchSkillWithBaseURL(srv.Client(), src, srv.URL) + require.Error(t, err) + assert.Contains(t, err.Error(), "no SKILL.md found") +} + +func TestParseManifestFromBytes(t *testing.T) { + data := []byte(validManifestContent()) + + s, err := ParseManifestFromBytes(data) + require.NoError(t, err) + assert.Equal(t, "test-skill", s.Name) + assert.Equal(t, "A test skill for import testing", s.Description) + assert.Equal(t, "alice", s.Metadata.Author.Name) + assert.Equal(t, "1.0.0", s.Metadata.Version) + assert.Contains(t, s.Body, "Do the thing.") +} + +func TestParseManifestFromBytes_Empty(t *testing.T) { + _, err := ParseManifestFromBytes([]byte("")) + require.Error(t, err) + assert.Contains(t, err.Error(), "empty") +} + +func TestParseManifestFromBytes_InvalidYAML(t *testing.T) { + data := []byte("---\n: invalid yaml [\n---\n\nbody\n") + _, err := ParseManifestFromBytes(data) + require.Error(t, err) +} diff --git a/internal/skill/skill.go b/internal/skill/skill.go index 4b83ac5..7606605 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -9,6 +9,9 @@ import ( // Scope represents where a skill is stored. type Scope string +// ManifestFile is the canonical filename for a skill manifest. +const ManifestFile = "SKILL.md" + // Scope constants for skill storage locations. const ( ScopeUser Scope = "user" From e865ec0b9a96724f0963c8c802f2615e7407b299 Mon Sep 17 00:00:00 2001 From: devrimcavusoglu Date: Sun, 26 Apr 2026 17:20:41 +0300 Subject: [PATCH 2/2] Harden skill import: lint, HTTP timeout, size cap, path-traversal guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add 30s default HTTP timeout for skill import (was using http.DefaultClient with no timeout — slow/malicious endpoints could hang the CLI indefinitely). - Cap downloaded payloads at 10 MiB via io.LimitReader on the contents API JSON, gist API JSON, and each companion file. - Reject companion filenames containing path separators, "..", or absolute paths in Registry.Import. The contents API normally returns base names, but the gist API echoes user-supplied filenames, so validate defensively at the registry boundary. - Convert ParseImportURL switch to a tagged switch (staticcheck QF1002) and document that refs containing slashes cannot be unambiguously parsed from the tree-URL form. - Drop the misleading "github repo with branch ref" test that asserted the parser's incorrect behavior on slashed refs. - Fix errcheck: wrap unchecked Encode/Write returns in test servers and resp.Body.Close() defers. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cli/context.go | 8 +++---- internal/cli/skill_import.go | 8 +++++-- internal/cli/skill_import_test.go | 8 +++---- internal/registry/registry.go | 23 +++++++++++++++++++ internal/registry/registry_test.go | 30 +++++++++++++++++++++++++ internal/skill/importer.go | 36 ++++++++++++++++++++++-------- internal/skill/importer_test.go | 26 ++++++--------------- 7 files changed, 101 insertions(+), 38 deletions(-) diff --git a/internal/cli/context.go b/internal/cli/context.go index 1e9d328..cdfd8ca 100644 --- a/internal/cli/context.go +++ b/internal/cli/context.go @@ -14,11 +14,11 @@ type contextKey struct{} // CommandContext holds injectable dependencies for CLI commands. type CommandContext struct { - Printer *output.Printer - NewRegistry func() (*registry.Registry, error) - NewDetector func() (*platform.Detector, error) + Printer *output.Printer + NewRegistry func() (*registry.Registry, error) + NewDetector func() (*platform.Detector, error) HTTPClient skill.HTTPClient // optional; defaults to http.DefaultClient - GitHubBaseURL string // optional; override GitHub API base URL (for testing) + GitHubBaseURL string // optional; override GitHub API base URL (for testing) } func setContext(cmd *cobra.Command, cc *CommandContext) { diff --git a/internal/cli/skill_import.go b/internal/cli/skill_import.go index 23adb2a..f3bda43 100644 --- a/internal/cli/skill_import.go +++ b/internal/cli/skill_import.go @@ -3,6 +3,7 @@ package cli import ( "fmt" "net/http" + "time" "github.com/devrimcavusoglu/skern/internal/output" "github.com/devrimcavusoglu/skern/internal/overlap" @@ -10,6 +11,10 @@ import ( "github.com/spf13/cobra" ) +// importHTTPTimeout caps individual HTTP requests during skill import so a +// slow or malicious endpoint cannot hang the CLI indefinitely. +const importHTTPTimeout = 30 * time.Second + func newSkillImportCmd() *cobra.Command { var ( scope string @@ -40,7 +45,7 @@ func newSkillImportCmd() *cobra.Command { ctx.Printer.Print("Fetching skill from %s...\n", rawURL) httpClient := ctx.HTTPClient if httpClient == nil { - httpClient = http.DefaultClient + httpClient = &http.Client{Timeout: importHTTPTimeout} } var fetched *skill.FetchedSkill if ctx.GitHubBaseURL != "" { @@ -161,4 +166,3 @@ func formatSource(src *skill.ImportSource) string { return src.RawURL } } - diff --git a/internal/cli/skill_import_test.go b/internal/cli/skill_import_test.go index 37c539b..82e24eb 100644 --- a/internal/cli/skill_import_test.go +++ b/internal/cli/skill_import_test.go @@ -50,16 +50,16 @@ func newImportTestServer(t *testing.T, manifest string, companions map[string]st }) } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(entries) + _ = json.NewEncoder(w).Encode(entries) }) mux.HandleFunc("/raw/SKILL.md", func(w http.ResponseWriter, _ *http.Request) { - w.Write([]byte(manifest)) + _, _ = w.Write([]byte(manifest)) }) for name, content := range companions { name, content := name, content mux.HandleFunc("/raw/"+name, func(w http.ResponseWriter, _ *http.Request) { - w.Write([]byte(content)) + _, _ = w.Write([]byte(content)) }) } @@ -79,7 +79,7 @@ func newImportTestServer(t *testing.T, manifest string, companions map[string]st } gist := map[string]interface{}{"files": gistFiles} w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(gist) + _ = json.NewEncoder(w).Encode(gist) }) return httptest.NewServer(mux) diff --git a/internal/registry/registry.go b/internal/registry/registry.go index 482fec4..e97d159 100644 --- a/internal/registry/registry.go +++ b/internal/registry/registry.go @@ -5,10 +5,29 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/devrimcavusoglu/skern/internal/skill" ) +// isSafeCompanionName rejects filenames that could escape the skill directory +// when joined via filepath.Join — e.g., "..", absolute paths, or any name +// containing a path separator. The contents-API normally returns base names, +// but the gist API echoes whatever filename the user set, so we validate +// defensively at the registry write boundary. +func isSafeCompanionName(name string) bool { + if name == "" || name == "." || name == ".." { + return false + } + if strings.ContainsAny(name, `/\`) { + return false + } + if filepath.IsAbs(name) { + return false + } + return true +} + // ParseWarning records a skill directory that could not be parsed. type ParseWarning struct { Name string `json:"name"` @@ -171,6 +190,10 @@ func (r *Registry) Import(s *skill.Skill, files map[string][]byte, scope skill.S if name == skill.ManifestFile { continue } + if !isSafeCompanionName(name) { + _ = os.RemoveAll(skillDir) + return "", fmt.Errorf("rejecting unsafe companion filename %q", name) + } filePath := filepath.Join(skillDir, name) if err := os.WriteFile(filePath, content, 0o644); err != nil { _ = os.RemoveAll(skillDir) diff --git a/internal/registry/registry_test.go b/internal/registry/registry_test.go index 24d635f..d409507 100644 --- a/internal/registry/registry_test.go +++ b/internal/registry/registry_test.go @@ -234,6 +234,36 @@ func TestRegistry_Search_MultiScope(t *testing.T) { assert.Len(t, results, 2) } +func TestRegistry_Import_RejectsUnsafeCompanionFilenames(t *testing.T) { + cases := []string{ + "../escape.md", + "../../etc/passwd", + "sub/dir.md", + `subdir\file.md`, + "..", + ".", + "", + } + + for _, badName := range cases { + t.Run(badName, func(t *testing.T) { + reg := newTestRegistry(t) + s := skill.NewSkill("safe-skill", "Desc.", "", "", "") + files := map[string][]byte{ + "SKILL.md": []byte("---\nname: safe-skill\ndescription: Desc.\nmetadata:\n author:\n name: a\n type: human\n version: \"0.0.1\"\n---\n\nbody\n"), + badName: []byte("payload"), + } + + _, err := reg.Import(s, files, skill.ScopeUser, false) + require.Error(t, err) + assert.Contains(t, err.Error(), "unsafe companion filename") + + // Skill directory must not be left behind on rejection. + assert.NoFileExists(t, filepath.Join(reg.userDir, "safe-skill", "SKILL.md")) + }) + } +} + func TestRegistry_List_SkipsInvalidWithWarning(t *testing.T) { reg := newTestRegistry(t) diff --git a/internal/skill/importer.go b/internal/skill/importer.go index fa2b54b..6d6a891 100644 --- a/internal/skill/importer.go +++ b/internal/skill/importer.go @@ -11,6 +11,11 @@ import ( "gopkg.in/yaml.v3" ) +// maxDownloadSize caps the size of any single fetched payload (API JSON or +// companion file) to protect against malicious or misconfigured remotes that +// could otherwise return an unbounded body. +const maxDownloadSize = 10 * 1024 * 1024 // 10 MiB + // SourceType identifies the kind of remote source for skill import. type SourceType string @@ -60,10 +65,10 @@ func ParseImportURL(rawURL string) (*ImportSource, error) { host := strings.ToLower(u.Host) - switch { - case host == "gist.github.com": + switch host { + case "gist.github.com": return parseGistURL(u, rawURL) - case host == "github.com": + case "github.com": return parseGitHubRepoURL(u, rawURL) default: return nil, fmt.Errorf("unsupported host %q; supported sources: github.com (repo directory), gist.github.com", host) @@ -72,6 +77,10 @@ func ParseImportURL(rawURL string) (*ImportSource, error) { func parseGitHubRepoURL(u *url.URL, rawURL string) (*ImportSource, error) { // Expected format: ///tree// + // + // NOTE: refs containing slashes (e.g., "feature/v2") cannot be unambiguously + // parsed from this URL form — only the first segment after `tree/` is taken + // as the ref. Use a ref without slashes or a commit SHA for reliable imports. parts := strings.SplitN(strings.TrimPrefix(u.Path, "/"), "/", 5) if len(parts) < 4 || parts[2] != "tree" { return nil, fmt.Errorf("expected GitHub directory URL format: https://github.com///tree//") @@ -167,7 +176,7 @@ func fetchFromGitHubRepoWithBase(client HTTPClient, src *ImportSource, baseURL s if err != nil { return nil, fmt.Errorf("fetching directory listing: %w", err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("GitHub API returned %d for %s", resp.StatusCode, apiURL) @@ -178,7 +187,7 @@ func fetchFromGitHubRepoWithBase(client HTTPClient, src *ImportSource, baseURL s Type string `json:"type"` DownloadURL string `json:"download_url"` } - if err := json.NewDecoder(resp.Body).Decode(&entries); err != nil { + if err := json.NewDecoder(io.LimitReader(resp.Body, maxDownloadSize)).Decode(&entries); err != nil { return nil, fmt.Errorf("parsing directory listing: %w", err) } @@ -227,7 +236,7 @@ func fetchFromGitHubGistWithBase(client HTTPClient, src *ImportSource, baseURL s if err != nil { return nil, fmt.Errorf("fetching gist: %w", err) } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("GitHub API returned %d for gist %s", resp.StatusCode, src.GistID) @@ -239,7 +248,7 @@ func fetchFromGitHubGistWithBase(client HTTPClient, src *ImportSource, baseURL s Content string `json:"content"` } `json:"files"` } - if err := json.NewDecoder(resp.Body).Decode(&gist); err != nil { + if err := json.NewDecoder(io.LimitReader(resp.Body, maxDownloadSize)).Decode(&gist); err != nil { return nil, fmt.Errorf("parsing gist response: %w", err) } @@ -270,13 +279,22 @@ func downloadFile(client HTTPClient, fileURL string) ([]byte, error) { if err != nil { return nil, err } - defer resp.Body.Close() + defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("HTTP %d for %s", resp.StatusCode, fileURL) } - return io.ReadAll(resp.Body) + // Read at most maxDownloadSize+1 to detect oversize without buffering + // arbitrarily large bodies. + data, err := io.ReadAll(io.LimitReader(resp.Body, maxDownloadSize+1)) + if err != nil { + return nil, err + } + if int64(len(data)) > maxDownloadSize { + return nil, fmt.Errorf("file %s exceeds maximum download size of %d bytes", fileURL, maxDownloadSize) + } + return data, nil } // ParseManifestFromBytes parses a SKILL.md from raw bytes (for imported content). diff --git a/internal/skill/importer_test.go b/internal/skill/importer_test.go index 2064cc5..f02a19c 100644 --- a/internal/skill/importer_test.go +++ b/internal/skill/importer_test.go @@ -30,18 +30,6 @@ func TestParseImportURL(t *testing.T) { Path: "skills/my-skill", }, }, - { - name: "github repo with branch ref", - url: "https://github.com/org/project/tree/feature/v2/path/to/skill", - want: &ImportSource{ - Type: SourceGitHubRepo, - RawURL: "https://github.com/org/project/tree/feature/v2/path/to/skill", - Owner: "org", - Repo: "project", - Ref: "feature", - Path: "v2/path/to/skill", - }, - }, { name: "github repo root tree", url: "https://github.com/owner/repo/tree/main", @@ -156,13 +144,13 @@ func TestFetchSkill_GitHubRepo(t *testing.T) { {"name": "subdir", "type": "dir", "download_url": ""}, } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(entries) + _ = json.NewEncoder(w).Encode(entries) }) mux.HandleFunc("/raw/SKILL.md", func(w http.ResponseWriter, _ *http.Request) { - w.Write([]byte(manifest)) + _, _ = w.Write([]byte(manifest)) }) mux.HandleFunc("/raw/helper.md", func(w http.ResponseWriter, _ *http.Request) { - w.Write([]byte(companion)) + _, _ = w.Write([]byte(companion)) }) srv := httptest.NewServer(mux) @@ -191,10 +179,10 @@ func TestFetchSkill_GitHubRepo_NoManifest(t *testing.T) { {"name": "README.md", "type": "file", "download_url": fmt.Sprintf("http://%s/raw/README.md", r.Host)}, } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(entries) + _ = json.NewEncoder(w).Encode(entries) }) mux.HandleFunc("/raw/README.md", func(w http.ResponseWriter, _ *http.Request) { - w.Write([]byte("just a readme")) + _, _ = w.Write([]byte("just a readme")) }) srv := httptest.NewServer(mux) @@ -253,7 +241,7 @@ func TestFetchSkill_GitHubGist(t *testing.T) { }, } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(gist) + _ = json.NewEncoder(w).Encode(gist) }) srv := httptest.NewServer(mux) @@ -284,7 +272,7 @@ func TestFetchSkill_GitHubGist_NoManifest(t *testing.T) { }, } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(gist) + _ = json.NewEncoder(w).Encode(gist) }) srv := httptest.NewServer(mux)