From 3febb44037bb06aa4e9572d6e19f7a50bab912a2 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 9 Mar 2026 09:56:04 +0200 Subject: [PATCH 1/4] Add git reference support for skill install Users can now install skills directly from git repositories using: thv skill install git://github.com/org/repo#path/to/skill This eliminates the "pending forever" problem where plain skill names created dead-end records. Unresolved plain names now return an actionable 404 error suggesting valid installation methods. New package pkg/skills/gitresolver/ provides: - Reference parsing with SSRF prevention (private IP/localhost rejection) - Path traversal validation (reject .., absolute paths, backslashes) - Host-scoped auth (GITHUB_TOKEN only sent to github.com, etc.) - Clone timeout (2 minutes) to prevent slowloris attacks - Supply chain defense (SKILL.md name must match git reference) - File writer with symlink checks and permission sanitization (0644 cap) Security reviewed: credential exfiltration prevented by scoping tokens to their respective hosts. DNS rebinding mitigation deferred as follow-up (requires custom DialContext). Relates to #4015 Co-Authored-By: Claude Opus 4.6 (1M context) --- cmd/thv/app/skill_install.go | 9 +- pkg/api/server.go | 2 + pkg/skills/gitresolver/auth.go | 82 ++++++++ pkg/skills/gitresolver/auth_test.go | 110 +++++++++++ pkg/skills/gitresolver/reference.go | 186 +++++++++++++++++++ pkg/skills/gitresolver/reference_test.go | 217 ++++++++++++++++++++++ pkg/skills/gitresolver/resolver.go | 226 +++++++++++++++++++++++ pkg/skills/gitresolver/resolver_test.go | 205 ++++++++++++++++++++ pkg/skills/gitresolver/writer.go | 98 ++++++++++ pkg/skills/gitresolver/writer_test.go | 123 ++++++++++++ pkg/skills/skillsvc/git_install.go | 150 +++++++++++++++ pkg/skills/skillsvc/skillsvc.go | 49 +++-- pkg/skills/skillsvc/skillsvc_test.go | 154 ++++++--------- 13 files changed, 1485 insertions(+), 126 deletions(-) create mode 100644 pkg/skills/gitresolver/auth.go create mode 100644 pkg/skills/gitresolver/auth_test.go create mode 100644 pkg/skills/gitresolver/reference.go create mode 100644 pkg/skills/gitresolver/reference_test.go create mode 100644 pkg/skills/gitresolver/resolver.go create mode 100644 pkg/skills/gitresolver/resolver_test.go create mode 100644 pkg/skills/gitresolver/writer.go create mode 100644 pkg/skills/gitresolver/writer_test.go create mode 100644 pkg/skills/skillsvc/git_install.go diff --git a/cmd/thv/app/skill_install.go b/cmd/thv/app/skill_install.go index 592c1039ce..06ef1d38c3 100644 --- a/cmd/thv/app/skill_install.go +++ b/cmd/thv/app/skill_install.go @@ -20,8 +20,13 @@ var ( var skillInstallCmd = &cobra.Command{ Use: "install [skill-name]", Short: "Install a skill", - Long: `Install a skill by name or OCI reference. -The skill will be fetched from a remote registry and installed locally.`, + Long: `Install a skill by name, OCI reference, or git reference. + +Examples: + thv skill install my-skill # from local build + thv skill install ghcr.io/org/my-skill:v1 # from OCI registry + thv skill install git://github.com/org/repo#skills/my-skill # from git repo + thv skill install git://github.com/org/repo@v1.0#skills/my-skill # from git ref`, Args: cobra.ExactArgs(1), PreRunE: chainPreRunE( validateSkillScope(&skillInstallScope), diff --git a/pkg/api/server.go b/pkg/api/server.go index 72c2a01813..4ce962a201 100644 --- a/pkg/api/server.go +++ b/pkg/api/server.go @@ -40,6 +40,7 @@ import ( "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/recovery" "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/gitresolver" "github.com/stacklok/toolhive/pkg/skills/skillsvc" "github.com/stacklok/toolhive/pkg/storage/sqlite" "github.com/stacklok/toolhive/pkg/updates" @@ -263,6 +264,7 @@ func (b *ServerBuilder) createDefaultManagers(ctx context.Context) error { skillsvc.WithPackager(packager), skillsvc.WithRegistryClient(registry), skillsvc.WithGroupManager(b.groupManager), + skillsvc.WithGitResolver(gitresolver.NewResolver()), ) } diff --git a/pkg/skills/gitresolver/auth.go b/pkg/skills/gitresolver/auth.go new file mode 100644 index 0000000000..d3f73ffe83 --- /dev/null +++ b/pkg/skills/gitresolver/auth.go @@ -0,0 +1,82 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package gitresolver + +import ( + "log/slog" + "net/url" + "os" + "strings" + + "github.com/go-git/go-git/v5/plumbing/transport" + githttp "github.com/go-git/go-git/v5/plumbing/transport/http" +) + +// tokenMapping maps environment variable names to the git hosts they are scoped to. +// Tokens are only sent to their matching host to prevent credential exfiltration. +var tokenMapping = []struct { + envVar string + hosts []string // empty means the token is sent to any host (user opt-in) +}{ + {envVar: "GITHUB_TOKEN", hosts: []string{"github.com"}}, + {envVar: "GITLAB_TOKEN", hosts: []string{"gitlab.com"}}, + {envVar: "GIT_TOKEN", hosts: nil}, // fallback: sent to any host +} + +// EnvFunc is a function that looks up an environment variable. +// The default is os.Getenv; tests can inject a custom implementation. +type EnvFunc func(string) string + +// ResolveAuth attempts to find authentication credentials from the environment +// scoped to the given clone URL. Returns nil if no credentials match. +// +// Security: tokens are only sent to their designated hosts. GITHUB_TOKEN is +// only sent to github.com, GITLAB_TOKEN only to gitlab.com. GIT_TOKEN is a +// fallback sent to any host. +func ResolveAuth(cloneURL string) transport.AuthMethod { + return ResolveAuthWith(os.Getenv, cloneURL) +} + +// ResolveAuthWith is like ResolveAuth but uses the provided function to look up +// environment variables, making it testable without modifying process state. +func ResolveAuthWith(getenv EnvFunc, cloneURL string) transport.AuthMethod { + host := extractHost(cloneURL) + + for _, mapping := range tokenMapping { + token := getenv(mapping.envVar) + if token == "" { + continue + } + // If hosts are specified, only send the token to matching hosts. + if len(mapping.hosts) > 0 && !hostMatches(host, mapping.hosts) { + continue + } + slog.Debug("Using git authentication from environment", "env_var", mapping.envVar) + return &githttp.BasicAuth{ + Username: "x-access-token", + Password: token, + } + } + + return nil +} + +// extractHost returns the lowercase hostname from a URL, or empty string on failure. +func extractHost(rawURL string) string { + parsed, err := url.Parse(rawURL) + if err != nil { + return "" + } + return strings.ToLower(parsed.Hostname()) +} + +// hostMatches checks if host matches any of the allowed hosts (case-insensitive). +func hostMatches(host string, allowed []string) bool { + for _, h := range allowed { + if strings.EqualFold(host, h) { + return true + } + } + return false +} diff --git a/pkg/skills/gitresolver/auth_test.go b/pkg/skills/gitresolver/auth_test.go new file mode 100644 index 0000000000..fe0d08119c --- /dev/null +++ b/pkg/skills/gitresolver/auth_test.go @@ -0,0 +1,110 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package gitresolver + +import ( + "testing" + + githttp "github.com/go-git/go-git/v5/plumbing/transport/http" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeEnv builds an EnvFunc that returns values from the given map. +func fakeEnv(vars map[string]string) EnvFunc { + return func(key string) string { + return vars[key] + } +} + +func TestResolveAuthWith(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + cloneURL string + envVars map[string]string + expectNil bool + expectToken string + }{ + { + name: "no env vars set", + cloneURL: "https://github.com/org/repo", + envVars: map[string]string{}, + expectNil: true, + }, + { + name: "GITHUB_TOKEN sent to github.com", + cloneURL: "https://github.com/org/repo", + envVars: map[string]string{"GITHUB_TOKEN": "ghp_test123"}, + expectToken: "ghp_test123", + }, + { + name: "GITHUB_TOKEN NOT sent to gitlab.com", + cloneURL: "https://gitlab.com/org/repo", + envVars: map[string]string{"GITHUB_TOKEN": "ghp_test123"}, + expectNil: true, + }, + { + name: "GITHUB_TOKEN NOT sent to evil host", + cloneURL: "https://evil.com/org/repo", + envVars: map[string]string{"GITHUB_TOKEN": "ghp_secret"}, + expectNil: true, + }, + { + name: "GITLAB_TOKEN sent to gitlab.com", + cloneURL: "https://gitlab.com/org/repo", + envVars: map[string]string{"GITLAB_TOKEN": "glpat-test123"}, + expectToken: "glpat-test123", + }, + { + name: "GITLAB_TOKEN NOT sent to github.com", + cloneURL: "https://github.com/org/repo", + envVars: map[string]string{"GITLAB_TOKEN": "glpat-test123"}, + expectNil: true, + }, + { + name: "GIT_TOKEN sent to any host", + cloneURL: "https://custom-git.example.com/org/repo", + envVars: map[string]string{"GIT_TOKEN": "token123"}, + expectToken: "token123", + }, + { + name: "GITHUB_TOKEN takes precedence over GIT_TOKEN on github.com", + cloneURL: "https://github.com/org/repo", + envVars: map[string]string{ + "GITHUB_TOKEN": "ghp_first", + "GIT_TOKEN": "fallback", + }, + expectToken: "ghp_first", + }, + { + name: "GIT_TOKEN used on github.com when GITHUB_TOKEN absent", + cloneURL: "https://github.com/org/repo", + envVars: map[string]string{ + "GIT_TOKEN": "fallback", + }, + expectToken: "fallback", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + auth := ResolveAuthWith(fakeEnv(tt.envVars), tt.cloneURL) + + if tt.expectNil { + assert.Nil(t, auth) + return + } + + require.NotNil(t, auth) + basicAuth, ok := auth.(*githttp.BasicAuth) + require.True(t, ok, "expected *githttp.BasicAuth") + assert.Equal(t, "x-access-token", basicAuth.Username) + assert.Equal(t, tt.expectToken, basicAuth.Password) + }) + } +} diff --git a/pkg/skills/gitresolver/reference.go b/pkg/skills/gitresolver/reference.go new file mode 100644 index 0000000000..f9ac13411e --- /dev/null +++ b/pkg/skills/gitresolver/reference.go @@ -0,0 +1,186 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package gitresolver + +import ( + "fmt" + "net" + "path" + "strings" +) + +const gitScheme = "git://" + +// GitReference represents a parsed git:// skill reference. +type GitReference struct { + // URL is the HTTPS clone URL (e.g., https://github.com/org/repo) + URL string + // Path is the subdirectory within repo (e.g., "path/to/skill"), empty = repo root + Path string + // Ref is the git ref: branch, tag, or commit (e.g., "v1.0.0"), empty = default branch + Ref string +} + +// IsGitReference returns true if name starts with "git://". +func IsGitReference(name string) bool { + return strings.HasPrefix(name, gitScheme) +} + +// ParseGitReference parses a git:// skill reference. +// +// Format: git://host/owner/repo[@ref][#path/to/skill] +// +// Examples: +// - git://github.com/org/repo +// - git://github.com/org/repo@v1.0.0 +// - git://github.com/org/repo#skills/my-skill +// - git://github.com/org/repo@main#skills/my-skill +func ParseGitReference(raw string) (*GitReference, error) { + if !IsGitReference(raw) { + return nil, fmt.Errorf("not a git reference: must start with %q", gitScheme) + } + + // Strip scheme + rest := raw[len(gitScheme):] + + // Split off fragment (#path) + var skillPath string + if idx := strings.Index(rest, "#"); idx >= 0 { + skillPath = rest[idx+1:] + rest = rest[:idx] + } + + // Split off ref (@ref) + var ref string + if idx := strings.Index(rest, "@"); idx >= 0 { + ref = rest[idx+1:] + rest = rest[:idx] + } + + // rest is now "host/owner/repo" (or "host/owner/repo/...") + if rest == "" { + return nil, fmt.Errorf("invalid git reference: empty host/path") + } + + // Extract host + slashIdx := strings.Index(rest, "/") + if slashIdx < 0 { + return nil, fmt.Errorf("invalid git reference: no repository path after host") + } + host := rest[:slashIdx] + repoPath := rest[slashIdx+1:] + + // Validate host + if err := validateHost(host); err != nil { + return nil, fmt.Errorf("invalid git reference: %w", err) + } + + // Validate repo path has at least owner/repo + if repoPath == "" || !strings.Contains(repoPath, "/") { + return nil, fmt.Errorf("invalid git reference: repository path must be at least owner/repo") + } + + // Validate ref + if err := validateRef(ref); err != nil { + return nil, fmt.Errorf("invalid git reference: %w", err) + } + + // Validate skill path + if err := validateSkillPath(skillPath); err != nil { + return nil, fmt.Errorf("invalid git reference: %w", err) + } + + // Build HTTPS clone URL + cloneURL := "https://" + host + "/" + repoPath + + return &GitReference{ + URL: cloneURL, + Path: skillPath, + Ref: ref, + }, nil +} + +// SkillName extracts the expected skill name from the reference. +// Uses the last component of Path if set, otherwise the last component of the repo URL. +func (r *GitReference) SkillName() string { + if r.Path != "" { + return path.Base(r.Path) + } + // Extract from URL: "https://github.com/org/repo" -> "repo" + trimmed := strings.TrimSuffix(r.URL, ".git") + return path.Base(trimmed) +} + +// validateHost checks the host is not localhost, a private IP, or empty. +func validateHost(host string) error { + if host == "" { + return fmt.Errorf("host must not be empty") + } + + // Strip port if present + hostname := host + if h, _, err := net.SplitHostPort(host); err == nil { + hostname = h + } + + // Reject localhost variants + lower := strings.ToLower(hostname) + if lower == "localhost" || lower == "127.0.0.1" || lower == "::1" || lower == "[::1]" || lower == "0.0.0.0" { + return fmt.Errorf("host %q is not allowed: localhost is rejected for SSRF prevention", host) + } + + // Reject private IPs + ip := net.ParseIP(hostname) + if ip != nil && (ip.IsPrivate() || ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast()) { + return fmt.Errorf("host %q is not allowed: private/loopback IPs are rejected for SSRF prevention", host) + } + + return nil +} + +// validateRef checks that the ref doesn't contain shell metacharacters. +func validateRef(ref string) error { + if ref == "" { + return nil + } + // Reject characters that could be used in shell injection or path traversal + for _, c := range ref { + switch { + case c >= 'a' && c <= 'z', + c >= 'A' && c <= 'Z', + c >= '0' && c <= '9', + c == '.', c == '-', c == '_', c == '/': + continue + default: + return fmt.Errorf("ref %q contains invalid character %q", ref, c) + } + } + if strings.Contains(ref, "..") { + return fmt.Errorf("ref %q must not contain '..' segments", ref) + } + return nil +} + +// validateSkillPath checks that the path doesn't contain traversal, null bytes, +// absolute paths, or backslashes. +func validateSkillPath(p string) error { + if p == "" { + return nil + } + if strings.ContainsRune(p, 0) { + return fmt.Errorf("path contains null bytes") + } + if strings.HasPrefix(p, "/") || strings.HasPrefix(p, "\\") { + return fmt.Errorf("path %q must be relative", p) + } + if strings.Contains(p, "\\") { + return fmt.Errorf("path %q must not contain backslashes", p) + } + for _, segment := range strings.Split(p, "/") { + if segment == ".." { + return fmt.Errorf("path %q must not contain '..' traversal segments", p) + } + } + return nil +} diff --git a/pkg/skills/gitresolver/reference_test.go b/pkg/skills/gitresolver/reference_test.go new file mode 100644 index 0000000000..2793e08d29 --- /dev/null +++ b/pkg/skills/gitresolver/reference_test.go @@ -0,0 +1,217 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package gitresolver + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIsGitReference(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + expected bool + }{ + {name: "valid git scheme", input: "git://github.com/org/repo", expected: true}, + {name: "with ref and path", input: "git://github.com/org/repo@v1#skills/foo", expected: true}, + {name: "plain name", input: "my-skill", expected: false}, + {name: "OCI reference", input: "ghcr.io/org/skill:v1", expected: false}, + {name: "https URL", input: "https://github.com/org/repo", expected: false}, + {name: "empty string", input: "", expected: false}, + {name: "git prefix but not scheme", input: "github.com/org/repo", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.expected, IsGitReference(tt.input)) + }) + } +} + +func TestParseGitReference(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + expected *GitReference + expectError string + }{ + { + name: "simple repo", + input: "git://github.com/org/repo", + expected: &GitReference{ + URL: "https://github.com/org/repo", + }, + }, + { + name: "with tag ref", + input: "git://github.com/org/repo@v1.0.0", + expected: &GitReference{ + URL: "https://github.com/org/repo", + Ref: "v1.0.0", + }, + }, + { + name: "with path", + input: "git://github.com/org/repo#skills/my-skill", + expected: &GitReference{ + URL: "https://github.com/org/repo", + Path: "skills/my-skill", + }, + }, + { + name: "with ref and path", + input: "git://github.com/org/repo@main#skills/my-skill", + expected: &GitReference{ + URL: "https://github.com/org/repo", + Ref: "main", + Path: "skills/my-skill", + }, + }, + { + name: "gitlab host", + input: "git://gitlab.com/org/repo", + expected: &GitReference{ + URL: "https://gitlab.com/org/repo", + }, + }, + { + name: "deep repo path", + input: "git://github.com/org/suborg/repo", + expected: &GitReference{ + URL: "https://github.com/org/suborg/repo", + }, + }, + { + name: "not a git reference", + input: "my-skill", + expectError: "not a git reference", + }, + { + name: "empty after scheme", + input: "git://", + expectError: "empty host/path", + }, + { + name: "host only no repo", + input: "git://github.com", + expectError: "no repository path after host", + }, + { + name: "host with single path component", + input: "git://github.com/org", + expectError: "repository path must be at least owner/repo", + }, + { + name: "localhost rejected", + input: "git://localhost/org/repo", + expectError: "SSRF prevention", + }, + { + name: "127.0.0.1 rejected", + input: "git://127.0.0.1/org/repo", + expectError: "SSRF prevention", + }, + { + name: "private IP rejected", + input: "git://10.0.0.1/org/repo", + expectError: "SSRF prevention", + }, + { + name: "192.168 rejected", + input: "git://192.168.1.1/org/repo", + expectError: "SSRF prevention", + }, + { + name: "path traversal in skill path", + input: "git://github.com/org/repo#../../../etc/passwd", + expectError: "'..' traversal", + }, + { + name: "absolute skill path rejected", + input: "git://github.com/org/repo#/etc/passwd", + expectError: "must be relative", + }, + { + name: "backslash in skill path rejected", + input: "git://github.com/org/repo#skills\\my-skill", + expectError: "must not contain backslashes", + }, + { + name: "ref with shell metacharacters", + input: "git://github.com/org/repo@v1;rm -rf /", + expectError: "invalid character", + }, + { + name: "ref with double dots", + input: "git://github.com/org/repo@main..HEAD", + expectError: "must not contain '..'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + result, err := ParseGitReference(tt.input) + + if tt.expectError != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectError) + assert.Nil(t, result) + return + } + + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, tt.expected.URL, result.URL) + assert.Equal(t, tt.expected.Path, result.Path) + assert.Equal(t, tt.expected.Ref, result.Ref) + }) + } +} + +func TestGitReference_SkillName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + ref GitReference + expected string + }{ + { + name: "name from path", + ref: GitReference{URL: "https://github.com/org/repo", Path: "skills/my-skill"}, + expected: "my-skill", + }, + { + name: "name from repo URL", + ref: GitReference{URL: "https://github.com/org/my-skill"}, + expected: "my-skill", + }, + { + name: "name from repo URL with .git suffix", + ref: GitReference{URL: "https://github.com/org/my-skill.git"}, + expected: "my-skill", + }, + { + name: "single path component", + ref: GitReference{URL: "https://github.com/org/repo", Path: "my-skill"}, + expected: "my-skill", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.expected, tt.ref.SkillName()) + }) + } +} diff --git a/pkg/skills/gitresolver/resolver.go b/pkg/skills/gitresolver/resolver.go new file mode 100644 index 0000000000..d096f57ea7 --- /dev/null +++ b/pkg/skills/gitresolver/resolver.go @@ -0,0 +1,226 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package gitresolver resolves skill installations from git repositories. +package gitresolver + +import ( + "context" + "fmt" + "io/fs" + "path" + "time" + + "github.com/stacklok/toolhive/pkg/git" + "github.com/stacklok/toolhive/pkg/skills" +) + +// cloneTimeout is the maximum time allowed for cloning a git repository. +const cloneTimeout = 2 * time.Minute + +// Resolver clones a git repository and extracts skill files. +type Resolver interface { + // Resolve clones the repo, validates the skill, and returns the skill + // directory contents as files ready for installation. + Resolve(ctx context.Context, ref *GitReference) (*ResolveResult, error) +} + +// ResolveResult contains the outcome of resolving a git skill reference. +type ResolveResult struct { + // SkillConfig is the parsed SKILL.md + SkillConfig *skills.ParseResult + // Files is all files in the skill directory + Files []FileEntry + // CommitHash is the git commit hash (for digest/upgrade detection) + CommitHash string +} + +// FileEntry represents a single file from the cloned repository. +type FileEntry struct { + Path string + Content []byte + Mode fs.FileMode +} + +// ResolverOption configures a defaultResolver. +type ResolverOption func(*defaultResolver) + +// WithGitClient sets a fixed git client, bypassing per-clone auth resolution. +// Primarily used for testing with mock clients. +func WithGitClient(client git.Client) ResolverOption { + return func(r *defaultResolver) { + r.fixedClient = client + } +} + +// NewResolver creates a new git skill resolver. +func NewResolver(opts ...ResolverOption) Resolver { + r := &defaultResolver{} + for _, o := range opts { + o(r) + } + return r +} + +type defaultResolver struct { + // fixedClient, when set, is used for all clones (testing). + // When nil, a new client is created per-clone with host-scoped auth. + fixedClient git.Client +} + +// clientForURL returns a git client appropriate for the given clone URL. +// If a fixed client was provided (testing), it is returned as-is. +// Otherwise, a new client is created with host-scoped auth from the environment. +func (r *defaultResolver) clientForURL(cloneURL string) git.Client { + if r.fixedClient != nil { + return r.fixedClient + } + auth := ResolveAuth(cloneURL) + var opts []git.ClientOption + if auth != nil { + opts = append(opts, git.WithAuth(auth)) + } + return git.NewDefaultGitClient(opts...) +} + +// Resolve clones a git repository and extracts skill files from it. +func (r *defaultResolver) Resolve(ctx context.Context, ref *GitReference) (*ResolveResult, error) { + // Enforce a clone timeout to prevent indefinite hangs from slow/malicious servers. + ctx, cancel := context.WithTimeout(ctx, cloneTimeout) + defer cancel() + + // Build clone config from the git reference + cloneConfig := &git.CloneConfig{ + URL: ref.URL, + } + if ref.Ref != "" { + // Try as branch/tag first (go-git will determine the correct one). + // If the ref looks like a full commit hash, use commit checkout instead. + if len(ref.Ref) == 40 && isHex(ref.Ref) { + cloneConfig.Commit = ref.Ref + } else { + cloneConfig.Branch = ref.Ref + } + } + + client := r.clientForURL(ref.URL) + + repoInfo, err := client.Clone(ctx, cloneConfig) + if err != nil { + return nil, fmt.Errorf("cloning repository: %w", err) + } + defer client.Cleanup(ctx, repoInfo) //nolint:errcheck // best-effort cleanup + + // Get commit hash for digest tracking + commitHash, err := git.HeadCommitHash(repoInfo) + if err != nil { + return nil, fmt.Errorf("getting commit hash: %w", err) + } + + // Read SKILL.md from the skill path + skillMDPath := path.Join(ref.Path, "SKILL.md") + if ref.Path == "" { + skillMDPath = "SKILL.md" + } + + skillContent, err := client.GetFileContent(repoInfo, skillMDPath) + if err != nil { + return nil, fmt.Errorf("reading SKILL.md at %q: %w", skillMDPath, err) + } + + // Parse the skill definition + parsed, err := skills.ParseSkillMD(skillContent) + if err != nil { + return nil, fmt.Errorf("parsing SKILL.md: %w", err) + } + + // Validate skill name + if err := skills.ValidateSkillName(parsed.Name); err != nil { + return nil, fmt.Errorf("invalid skill name in SKILL.md: %w", err) + } + + // Collect all files in the skill directory. + // For now, we read SKILL.md as the primary file. Additional files in the + // skill directory are discovered by listing the tree entries. + files, err := r.collectFiles(repoInfo, ref.Path) + if err != nil { + return nil, fmt.Errorf("collecting skill files: %w", err) + } + + return &ResolveResult{ + SkillConfig: parsed, + Files: files, + CommitHash: commitHash, + }, nil +} + +// collectFiles reads all files from the given path in the repository. +func (*defaultResolver) collectFiles(repoInfo *git.RepositoryInfo, basePath string) ([]FileEntry, error) { + ref, err := repoInfo.Repository.Head() + if err != nil { + return nil, fmt.Errorf("getting HEAD: %w", err) + } + + commit, err := repoInfo.Repository.CommitObject(ref.Hash()) + if err != nil { + return nil, fmt.Errorf("getting commit: %w", err) + } + + tree, err := commit.Tree() + if err != nil { + return nil, fmt.Errorf("getting tree: %w", err) + } + + // Navigate to subdirectory if specified + if basePath != "" { + tree, err = tree.Tree(basePath) + if err != nil { + return nil, fmt.Errorf("navigating to path %q: %w", basePath, err) + } + } + + var files []FileEntry + for _, entry := range tree.Entries { + // Skip directories — we only want files at the top level of the skill dir. + // Nested subdirectories are not part of the skill spec. + if entry.Mode == 0040000 { + continue + } + + file, fileErr := tree.File(entry.Name) + if fileErr != nil { + return nil, fmt.Errorf("reading file %q: %w", entry.Name, fileErr) + } + + content, contentErr := file.Contents() + if contentErr != nil { + return nil, fmt.Errorf("reading content of %q: %w", entry.Name, contentErr) + } + + // All files are capped to 0644 by the writer; set a uniform mode here. + mode := fs.FileMode(0644) + + files = append(files, FileEntry{ + Path: entry.Name, + Content: []byte(content), + Mode: mode, + }) + } + + return files, nil +} + +// isHex checks if a string is a valid hexadecimal string. +func isHex(s string) bool { + for _, c := range s { + switch { + case c >= '0' && c <= '9', + c >= 'a' && c <= 'f', + c >= 'A' && c <= 'F': + continue + default: + return false + } + } + return true +} diff --git a/pkg/skills/gitresolver/resolver_test.go b/pkg/skills/gitresolver/resolver_test.go new file mode 100644 index 0000000000..709449c43b --- /dev/null +++ b/pkg/skills/gitresolver/resolver_test.go @@ -0,0 +1,205 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package gitresolver + +import ( + "context" + "os" + "path/filepath" + "testing" + + gogit "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/plumbing/object" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/git" +) + +// createTestRepo creates a local git repo with a skill at the given path. +// Returns the repo directory path. +func createTestRepo(t *testing.T, skillPath string, skillMD string) string { + t.Helper() + + dir := t.TempDir() + repo, err := gogit.PlainInit(dir, false) + require.NoError(t, err) + + wt, err := repo.Worktree() + require.NoError(t, err) + + // Create SKILL.md at the specified path + fullDir := dir + if skillPath != "" { + fullDir = filepath.Join(dir, skillPath) + require.NoError(t, os.MkdirAll(fullDir, 0755)) + } + + skillMDPath := filepath.Join(fullDir, "SKILL.md") + require.NoError(t, os.WriteFile(skillMDPath, []byte(skillMD), 0644)) + + // Add a companion file + readmePath := filepath.Join(fullDir, "README.md") + require.NoError(t, os.WriteFile(readmePath, []byte("# Test Skill"), 0644)) + + // Stage and commit + _, err = wt.Add(".") + require.NoError(t, err) + + _, err = wt.Commit("Add test skill", &gogit.CommitOptions{ + Author: &object.Signature{ + Name: "Test Author", + Email: "test@example.com", + }, + }) + require.NoError(t, err) + + return dir +} + +func TestResolver_Resolve(t *testing.T) { + t.Parallel() + + validSkillMD := `--- +name: my-skill +description: A test skill +version: "1.0.0" +--- +# My Skill + +This is a test skill. +` + + tests := []struct { + name string + skillPath string + skillMD string + ref *GitReference + expectError string + expectName string + expectFiles int + }{ + { + name: "skill at repo root", + skillPath: "", + skillMD: validSkillMD, + ref: &GitReference{Path: ""}, + expectName: "my-skill", + expectFiles: 2, // SKILL.md + README.md + }, + { + name: "skill in subdirectory", + skillPath: "skills/my-skill", + skillMD: validSkillMD, + ref: &GitReference{Path: "skills/my-skill"}, + expectName: "my-skill", + expectFiles: 2, + }, + { + name: "invalid SKILL.md", + skillPath: "", + skillMD: "not valid frontmatter", + ref: &GitReference{Path: ""}, + expectError: "parsing SKILL.md", + }, + { + name: "invalid skill name", + skillPath: "", + skillMD: `--- +name: INVALID +description: bad name +--- +`, + ref: &GitReference{Path: ""}, + expectError: "invalid skill name", + }, + { + name: "nonexistent path", + skillPath: "", + skillMD: validSkillMD, + ref: &GitReference{Path: "does/not/exist"}, + expectError: "reading SKILL.md", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + repoDir := createTestRepo(t, tt.skillPath, tt.skillMD) + gitClient := git.NewDefaultGitClient() + resolver := NewResolver(WithGitClient(gitClient)) + + // Override the URL to point to the local repo + ref := *tt.ref + ref.URL = repoDir + + result, err := resolver.Resolve(t.Context(), &ref) + + if tt.expectError != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectError) + assert.Nil(t, result) + return + } + + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, tt.expectName, result.SkillConfig.Name) + assert.Len(t, result.Files, tt.expectFiles) + assert.NotEmpty(t, result.CommitHash) + assert.Len(t, result.CommitHash, 40) + }) + } +} + +func TestResolver_Resolve_MissingSkillMD(t *testing.T) { + t.Parallel() + + // Create a repo without SKILL.md + dir := t.TempDir() + repo, err := gogit.PlainInit(dir, false) + require.NoError(t, err) + + wt, err := repo.Worktree() + require.NoError(t, err) + + readmePath := filepath.Join(dir, "README.md") + require.NoError(t, os.WriteFile(readmePath, []byte("# No skill here"), 0644)) + + _, err = wt.Add(".") + require.NoError(t, err) + + _, err = wt.Commit("No skill", &gogit.CommitOptions{ + Author: &object.Signature{ + Name: "Test", + Email: "test@example.com", + }, + }) + require.NoError(t, err) + + resolver := NewResolver(WithGitClient(git.NewDefaultGitClient())) + ref := &GitReference{URL: dir} + + result, err := resolver.Resolve(t.Context(), ref) + require.Error(t, err) + assert.Contains(t, err.Error(), "reading SKILL.md") + assert.Nil(t, result) +} + +func TestResolver_Resolve_ContextCancellation(t *testing.T) { + t.Parallel() + + resolver := NewResolver(WithGitClient(git.NewDefaultGitClient())) + ref := &GitReference{URL: "https://github.com/nonexistent/nonexistent-repo-12345"} + + // Create a context derived from the test context and cancel it immediately. + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + // Should fail from context cancellation (or network error). + result, err := resolver.Resolve(ctx, ref) + require.Error(t, err) + assert.Nil(t, result) +} diff --git a/pkg/skills/gitresolver/writer.go b/pkg/skills/gitresolver/writer.go new file mode 100644 index 0000000000..ae8d75fab2 --- /dev/null +++ b/pkg/skills/gitresolver/writer.go @@ -0,0 +1,98 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package gitresolver + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +const ( + // dirPermissions is the permission mode for created directories. + dirPermissions os.FileMode = 0750 + // filePermissionMask caps file permissions at 0644 (strips setuid/setgid/sticky). + filePermissionMask os.FileMode = 0644 +) + +// WriteFiles writes resolved skill files to the target directory. +// If force is true, any existing directory is removed before writing. +func WriteFiles(files []FileEntry, targetDir string, force bool) error { + // Handle existing directory + if _, statErr := os.Stat(targetDir); statErr == nil { + if !force { + return fmt.Errorf("target directory %q already exists; use force to overwrite", targetDir) + } + if err := os.RemoveAll(targetDir); err != nil { + return fmt.Errorf("removing existing directory: %w", err) + } + } + + // Pre-extraction: validate that no existing path components are symlinks. + if err := validatePathNoSymlinks(targetDir); err != nil { + return fmt.Errorf("target path validation: %w", err) + } + + if err := os.MkdirAll(targetDir, dirPermissions); err != nil { + return fmt.Errorf("creating target directory: %w", err) + } + + cleanTarget := filepath.Clean(targetDir) + string(os.PathSeparator) + + for _, f := range files { + destPath := filepath.Clean(filepath.Join(targetDir, filepath.FromSlash(f.Path))) + + // Containment check: ensure destPath is beneath targetDir. + if !strings.HasPrefix(destPath, cleanTarget) { + return fmt.Errorf("path traversal detected: file %q escapes target directory", f.Path) + } + + parentDir := filepath.Dir(destPath) + if err := os.MkdirAll(parentDir, dirPermissions); err != nil { + return fmt.Errorf("creating directory %q: %w", parentDir, err) + } + + // Sanitize file permissions: strip setuid/setgid/sticky, cap at 0644 + mode := (f.Mode & 0o777) & filePermissionMask + + if err := os.WriteFile(destPath, f.Content, mode); err != nil { + return fmt.Errorf("writing file %q: %w", f.Path, err) + } + } + + return nil +} + +// validatePathNoSymlinks walks up from the target path checking each existing +// path component for symlinks. +func validatePathNoSymlinks(targetDir string) error { + absTarget, err := filepath.Abs(targetDir) + if err != nil { + return fmt.Errorf("resolving absolute path: %w", err) + } + + current := func() string { + if vol := filepath.VolumeName(absTarget); vol != "" { + return vol + string(os.PathSeparator) + } + return string(os.PathSeparator) + }() + for _, component := range strings.Split(absTarget, string(os.PathSeparator)) { + if component == "" { + continue + } + current = filepath.Join(current, component) + + info, err := os.Lstat(current) + if err != nil { + // Path doesn't exist yet — remaining components will be created by MkdirAll. + break + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("symlink found at %q: refusing to write through symlinks", current) + } + } + return nil +} diff --git a/pkg/skills/gitresolver/writer_test.go b/pkg/skills/gitresolver/writer_test.go new file mode 100644 index 0000000000..ba934339e9 --- /dev/null +++ b/pkg/skills/gitresolver/writer_test.go @@ -0,0 +1,123 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package gitresolver + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func resolvedTempDir(t *testing.T) string { + t.Helper() + dir := t.TempDir() + resolved, err := filepath.EvalSymlinks(dir) + require.NoError(t, err) + return resolved +} + +func TestWriteFiles(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + files []FileEntry + force bool + preExist bool + expectError string + expectFiles int + }{ + { + name: "write single file", + files: []FileEntry{ + {Path: "SKILL.md", Content: []byte("# Skill"), Mode: 0644}, + }, + expectFiles: 1, + }, + { + name: "write multiple files", + files: []FileEntry{ + {Path: "SKILL.md", Content: []byte("# Skill"), Mode: 0644}, + {Path: "README.md", Content: []byte("# Readme"), Mode: 0644}, + }, + expectFiles: 2, + }, + { + name: "existing directory without force", + files: []FileEntry{ + {Path: "SKILL.md", Content: []byte("# Skill"), Mode: 0644}, + }, + preExist: true, + expectError: "already exists", + }, + { + name: "existing directory with force", + files: []FileEntry{ + {Path: "SKILL.md", Content: []byte("# New Skill"), Mode: 0644}, + }, + preExist: true, + force: true, + expectFiles: 1, + }, + { + name: "path traversal rejected", + files: []FileEntry{ + {Path: "../../../etc/passwd", Content: []byte("evil"), Mode: 0644}, + }, + expectError: "path traversal detected", + }, + { + name: "permissions capped at 0644", + files: []FileEntry{ + {Path: "script.sh", Content: []byte("#!/bin/bash"), Mode: 0755}, + }, + expectFiles: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + baseDir := resolvedTempDir(t) + targetDir := filepath.Join(baseDir, "my-skill") + + if tt.preExist { + require.NoError(t, os.MkdirAll(targetDir, 0750)) + require.NoError(t, os.WriteFile(filepath.Join(targetDir, "old.txt"), []byte("old"), 0644)) + } + + err := WriteFiles(tt.files, targetDir, tt.force) + + if tt.expectError != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.expectError) + return + } + + require.NoError(t, err) + + entries, err := os.ReadDir(targetDir) + require.NoError(t, err) + assert.Len(t, entries, tt.expectFiles) + + // Verify file contents + for _, f := range tt.files { + content, readErr := os.ReadFile(filepath.Join(targetDir, f.Path)) + require.NoError(t, readErr) + assert.Equal(t, f.Content, content) + } + + // Verify permissions are capped + for _, f := range tt.files { + info, statErr := os.Stat(filepath.Join(targetDir, f.Path)) + require.NoError(t, statErr) + mode := info.Mode().Perm() + assert.True(t, mode <= 0644, "file %q has mode %o, expected <= 0644", f.Path, mode) + } + }) + } +} diff --git a/pkg/skills/skillsvc/git_install.go b/pkg/skills/skillsvc/git_install.go new file mode 100644 index 0000000000..49cf9a0026 --- /dev/null +++ b/pkg/skills/skillsvc/git_install.go @@ -0,0 +1,150 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "errors" + "fmt" + "net/http" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/gitresolver" + "github.com/stacklok/toolhive/pkg/storage" +) + +// gitInstallOpts extends InstallOptions with resolved git files. +type gitInstallOpts struct { + skills.InstallOptions + gitFiles []gitresolver.FileEntry +} + +// installFromGit resolves a git:// reference, clones the repository, validates +// the skill, writes files to disk, and creates a DB record. +func (s *service) installFromGit( + ctx context.Context, + opts skills.InstallOptions, + scope skills.Scope, +) (*skills.InstallResult, error) { + if s.gitResolver == nil { + return nil, httperr.WithCode( + errors.New("git resolver is not configured"), + http.StatusInternalServerError, + ) + } + if s.pathResolver == nil { + return nil, httperr.WithCode( + errors.New("path resolver is required for git installs"), + http.StatusInternalServerError, + ) + } + + resolvedOpts, err := s.resolveGitReference(ctx, &opts) + if err != nil { + return nil, err + } + + unlock := s.locks.lock(resolvedOpts.Name, scope, resolvedOpts.ProjectRoot) + defer unlock() + + clientType := s.resolveClient(resolvedOpts.Client) + + targetDir, pathErr := s.pathResolver.GetSkillPath(clientType, resolvedOpts.Name, scope, resolvedOpts.ProjectRoot) + if pathErr != nil { + return nil, fmt.Errorf("resolving skill path: %w", pathErr) + } + + // Write files from git to the target directory. + if writeErr := gitresolver.WriteFiles(resolvedOpts.gitFiles, targetDir, resolvedOpts.Force); writeErr != nil { + return nil, fmt.Errorf("writing skill files: %w", writeErr) + } + + return s.upsertGitSkill(ctx, *resolvedOpts, scope, clientType, targetDir) +} + +// resolveGitReference parses a git:// reference, clones the repo, validates +// the skill, and hydrates install options from the resolved content. +func (s *service) resolveGitReference(ctx context.Context, opts *skills.InstallOptions) (*gitInstallOpts, error) { + originalRef := opts.Name + + gitRef, err := gitresolver.ParseGitReference(opts.Name) + if err != nil { + return nil, httperr.WithCode( + fmt.Errorf("invalid git reference %q: %w", opts.Name, err), + http.StatusBadRequest, + ) + } + + resolved, err := s.gitResolver.Resolve(ctx, gitRef) + if err != nil { + return nil, httperr.WithCode( + fmt.Errorf("resolving git skill: %w", err), + http.StatusBadGateway, + ) + } + + // Supply chain defense: the declared skill name must match what the reference expects. + expectedName := gitRef.SkillName() + if resolved.SkillConfig.Name != expectedName { + return nil, httperr.WithCode( + fmt.Errorf( + "skill name %q in SKILL.md does not match expected name %q from git reference", + resolved.SkillConfig.Name, expectedName, + ), + http.StatusUnprocessableEntity, + ) + } + + result := &gitInstallOpts{ + InstallOptions: *opts, + gitFiles: resolved.Files, + } + result.Name = resolved.SkillConfig.Name + result.Digest = resolved.CommitHash + result.Reference = originalRef + if result.Version == "" && resolved.SkillConfig.Version != "" { + result.Version = resolved.SkillConfig.Version + } + + return result, nil +} + +// upsertGitSkill creates or updates a DB record for a git-installed skill. +func (s *service) upsertGitSkill( + ctx context.Context, + opts gitInstallOpts, + scope skills.Scope, + clientType, targetDir string, +) (*skills.InstallResult, error) { + existing, storeErr := s.store.Get(ctx, opts.Name, scope, opts.ProjectRoot) + isNotFound := errors.Is(storeErr, storage.ErrNotFound) + + switch { + case storeErr != nil && !isNotFound: + return nil, fmt.Errorf("checking existing skill: %w", storeErr) + + case storeErr == nil && existing.Digest == opts.Digest: + // Same commit hash — already installed, no-op. + return &skills.InstallResult{Skill: existing}, nil + + case storeErr == nil: + // Different commit — upgrade. + sk := buildInstalledSkill(opts.InstallOptions, scope, clientType, existing.Clients) + if err := s.store.Update(ctx, sk); err != nil { + _ = s.installer.Remove(targetDir) // rollback + return nil, err + } + return &skills.InstallResult{Skill: sk}, nil + + default: + // Not found — fresh install. + sk := buildInstalledSkill(opts.InstallOptions, scope, clientType, nil) + if err := s.store.Create(ctx, sk); err != nil { + _ = s.installer.Remove(targetDir) // rollback + return nil, err + } + return &skills.InstallResult{Skill: sk}, nil + } +} diff --git a/pkg/skills/skillsvc/skillsvc.go b/pkg/skills/skillsvc/skillsvc.go index 2273c8f969..7f2acaadf4 100644 --- a/pkg/skills/skillsvc/skillsvc.go +++ b/pkg/skills/skillsvc/skillsvc.go @@ -26,6 +26,7 @@ import ( ociskills "github.com/stacklok/toolhive-core/oci/skills" "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/gitresolver" "github.com/stacklok/toolhive/pkg/storage" ) @@ -78,6 +79,13 @@ func WithGroupManager(mgr groups.Manager) Option { } } +// WithGitResolver sets the git resolver for git:// skill references. +func WithGitResolver(r gitresolver.Resolver) Option { + return func(s *service) { + s.gitResolver = r + } +} + // skillLock provides per-skill mutual exclusion keyed by scope/name/projectRoot. // Entries are never evicted. This is acceptable because the number of distinct // skills on a single machine is expected to remain small (< 1000). @@ -114,6 +122,7 @@ type service struct { ociStore *ociskills.Store packager ociskills.SkillPackager registry ociskills.RegistryClient + gitResolver gitresolver.Resolver } // New creates a new SkillService backed by the given store. @@ -193,6 +202,15 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski // the same lock key and DB record. opts.ProjectRoot = projectRoot + // Check for git:// reference first (before OCI, since git:// contains '/') + if gitresolver.IsGitReference(opts.Name) { + result, err := s.installFromGit(ctx, opts, scope) + if err != nil { + return nil, err + } + return result, s.registerSkillInGroup(ctx, opts.Group, result.Skill.Metadata.Name) + } + ref, isOCI, err := parseOCIReference(opts.Name) if err != nil { return nil, httperr.WithCode( @@ -232,11 +250,12 @@ func (s *service) Install(ctx context.Context, opts skills.InstallOptions) (*ski } } if !resolved { - result, err := s.installPending(ctx, opts, scope) - if err != nil { - return nil, err - } - return result, s.registerSkillInGroup(ctx, opts.Group, opts.Name) + return nil, httperr.WithCode( + fmt.Errorf("skill %q not found; use an OCI reference (ghcr.io/org/foo:v1), "+ + "a git reference (git://github.com/org/repo#path), "+ + "or build locally first (thv skill build ./foo)", opts.Name), + http.StatusNotFound, + ) } // resolved: opts hydrated, fall through to installWithExtraction } @@ -653,26 +672,6 @@ func (s *service) extractOCIContent(ctx context.Context, d digest.Digest) ([]byt return layerData, skillConfig, nil } -// installPending creates a pending skill record (no extraction). -func (s *service) installPending( - ctx context.Context, opts skills.InstallOptions, scope skills.Scope, -) (*skills.InstallResult, error) { - sk := skills.InstalledSkill{ - Metadata: skills.SkillMetadata{ - Name: opts.Name, - Version: opts.Version, - }, - Scope: scope, - ProjectRoot: opts.ProjectRoot, - Status: skills.InstallStatusPending, - InstalledAt: time.Now().UTC(), - } - if err := s.store.Create(ctx, sk); err != nil { - return nil, err - } - return &skills.InstallResult{Skill: sk}, nil -} - // installWithExtraction handles the full install flow: managed/unmanaged // detection, extraction, and DB record creation or update. func (s *service) installWithExtraction( diff --git a/pkg/skills/skillsvc/skillsvc_test.go b/pkg/skills/skillsvc/skillsvc_test.go index c3fa4fa059..687cf76754 100644 --- a/pkg/skills/skillsvc/skillsvc_test.go +++ b/pkg/skills/skillsvc/skillsvc_test.go @@ -151,86 +151,35 @@ func TestList(t *testing.T) { } } -func TestInstallPending(t *testing.T) { +func TestInstallPlainName(t *testing.T) { t.Parallel() - projectRoot := makeProjectRoot(t) - tests := []struct { - name string - opts skills.InstallOptions - setupMock func(*storemocks.MockSkillStore) - wantCode int - wantName string - wantScope skills.Scope + name string + opts skills.InstallOptions + wantCode int + wantErr string }{ { - name: "creates pending record with defaults", - opts: skills.InstallOptions{Name: "my-skill"}, - setupMock: func(s *storemocks.MockSkillStore) { - s.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, sk skills.InstalledSkill) error { - assert.Equal(t, "my-skill", sk.Metadata.Name) - assert.Equal(t, skills.ScopeUser, sk.Scope) - assert.Equal(t, skills.InstallStatusPending, sk.Status) - assert.False(t, sk.InstalledAt.IsZero()) - return nil - }) - }, - wantName: "my-skill", - wantScope: skills.ScopeUser, - }, - { - name: "propagates version", - opts: skills.InstallOptions{Name: "my-skill", Version: "2.1.0"}, - setupMock: func(s *storemocks.MockSkillStore) { - s.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, sk skills.InstalledSkill) error { - assert.Equal(t, "2.1.0", sk.Metadata.Version) - return nil - }) - }, - wantName: "my-skill", - }, - { - name: "respects explicit scope", - opts: skills.InstallOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}, - setupMock: func(s *storemocks.MockSkillStore) { - s.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, sk skills.InstalledSkill) error { - assert.Equal(t, skills.ScopeProject, sk.Scope) - assert.Equal(t, projectRoot, sk.ProjectRoot) - return nil - }) - }, - wantName: "my-skill", - wantScope: skills.ScopeProject, - }, - { - name: "rejects project scope without root", - opts: skills.InstallOptions{Name: "my-skill", Scope: skills.ScopeProject}, - setupMock: func(_ *storemocks.MockSkillStore) {}, - wantCode: http.StatusBadRequest, + name: "plain name without local build returns actionable error", + opts: skills.InstallOptions{Name: "my-skill"}, + wantCode: http.StatusNotFound, + wantErr: "not found", }, { - name: "rejects invalid name", - opts: skills.InstallOptions{Name: "A"}, - setupMock: func(_ *storemocks.MockSkillStore) {}, - wantCode: http.StatusBadRequest, + name: "rejects project scope without root", + opts: skills.InstallOptions{Name: "my-skill", Scope: skills.ScopeProject}, + wantCode: http.StatusBadRequest, }, { - name: "rejects empty name", - opts: skills.InstallOptions{Name: ""}, - setupMock: func(_ *storemocks.MockSkillStore) {}, - wantCode: http.StatusBadRequest, + name: "rejects invalid name", + opts: skills.InstallOptions{Name: "A"}, + wantCode: http.StatusBadRequest, }, { - name: "returns conflict on duplicate", - opts: skills.InstallOptions{Name: "my-skill"}, - setupMock: func(s *storemocks.MockSkillStore) { - s.EXPECT().Create(gomock.Any(), gomock.Any()).Return(storage.ErrAlreadyExists) - }, - wantCode: http.StatusConflict, + name: "rejects empty name", + opts: skills.InstallOptions{Name: ""}, + wantCode: http.StatusBadRequest, }, } @@ -239,17 +188,13 @@ func TestInstallPending(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) store := storemocks.NewMockSkillStore(ctrl) - tt.setupMock(store) - result, err := New(store).Install(t.Context(), tt.opts) - if tt.wantCode != 0 { - require.Error(t, err) - assert.Equal(t, tt.wantCode, httperr.Code(err)) - return + _, err := New(store).Install(t.Context(), tt.opts) + require.Error(t, err) + assert.Equal(t, tt.wantCode, httperr.Code(err)) + if tt.wantErr != "" { + assert.Contains(t, err.Error(), tt.wantErr) } - require.NoError(t, err) - assert.Equal(t, tt.wantName, result.Skill.Metadata.Name) - assert.Equal(t, skills.InstallStatusPending, result.Skill.Status) }) } } @@ -889,7 +834,7 @@ func TestInstallFromLocalStore(t *testing.T) { wantErr: "does not match install name", }, { - name: "tag not found falls back to pending", + name: "tag not found returns actionable error", opts: skills.InstallOptions{Name: "no-such-skill"}, setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { t.Helper() @@ -898,30 +843,22 @@ func TestInstallFromLocalStore(t *testing.T) { require.NoError(t, err) store := storemocks.NewMockSkillStore(ctrl) - store.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, sk skills.InstalledSkill) error { - assert.Equal(t, skills.InstallStatusPending, sk.Status) - return nil - }) pr := skillsmocks.NewMockPathResolver(ctrl) return ociStore, store, pr }, - wantStatus: string(skills.InstallStatusPending), + wantCode: http.StatusNotFound, + wantErr: "not found", }, { - name: "nil ociStore falls back to pending", + name: "nil ociStore returns actionable error", opts: skills.InstallOptions{Name: "some-skill"}, setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { t.Helper() store := storemocks.NewMockSkillStore(ctrl) - store.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, sk skills.InstalledSkill) error { - assert.Equal(t, skills.InstallStatusPending, sk.Status) - return nil - }) return nil, store, nil }, - wantStatus: string(skills.InstallStatusPending), + wantCode: http.StatusNotFound, + wantErr: "not found", }, { name: "corrupt manifest propagates error", @@ -1799,9 +1736,10 @@ func TestListFiltersByGroup(t *testing.T) { func TestInstallAddsSkillToGroup(t *testing.T) { t.Parallel() - // These tests use plain skill names with no LayerData, so Install takes the - // pending path (installPending) which only calls store.Create — no store.Get, - // no path resolver needed. + layerData := makeLayerData(t) + + // These tests use LayerData so Install goes through the extraction path, + // which requires a path resolver and store interactions. tests := []struct { name string opts skills.InstallOptions @@ -1811,8 +1749,9 @@ func TestInstallAddsSkillToGroup(t *testing.T) { }{ { name: "install with group registers skill", - opts: skills.InstallOptions{Name: "my-skill", Group: "mygroup"}, + opts: skills.InstallOptions{Name: "my-skill", Group: "mygroup", LayerData: layerData, Digest: "sha256:aaa"}, setupStoreMock: func(s *storemocks.MockSkillStore) { + s.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) s.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) }, setupGroupMock: func(gm *groupmocks.MockManager) { @@ -1823,8 +1762,9 @@ func TestInstallAddsSkillToGroup(t *testing.T) { }, { name: "install without group skips group registration", - opts: skills.InstallOptions{Name: "my-skill"}, + opts: skills.InstallOptions{Name: "my-skill", LayerData: layerData, Digest: "sha256:bbb"}, setupStoreMock: func(s *storemocks.MockSkillStore) { + s.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) s.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) }, setupGroupMock: func(_ *groupmocks.MockManager) { @@ -1833,8 +1773,9 @@ func TestInstallAddsSkillToGroup(t *testing.T) { }, { name: "group registration error propagates", - opts: skills.InstallOptions{Name: "my-skill", Group: "badgroup"}, + opts: skills.InstallOptions{Name: "my-skill", Group: "badgroup", LayerData: layerData, Digest: "sha256:ccc"}, setupStoreMock: func(s *storemocks.MockSkillStore) { + s.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) s.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) }, setupGroupMock: func(gm *groupmocks.MockManager) { @@ -1852,11 +1793,26 @@ func TestInstallAddsSkillToGroup(t *testing.T) { ctrl := gomock.NewController(t) store := storemocks.NewMockSkillStore(ctrl) gm := groupmocks.NewMockManager(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) tt.setupStoreMock(store) tt.setupGroupMock(gm) - svc := New(store, WithGroupManager(gm)) + targetDir := tempDir(t) + pr.EXPECT().GetSkillPath(gomock.Any(), "my-skill", skills.ScopeUser, ""). + Return(filepath.Join(targetDir, "my-skill"), nil).AnyTimes() + pr.EXPECT().ListSkillSupportingClients().Return([]string{"claude-code"}).AnyTimes() + inst.EXPECT().Extract(gomock.Any(), gomock.Any(), gomock.Any()). + Return(&skills.ExtractResult{SkillDir: filepath.Join(targetDir, "my-skill"), Files: 1}, nil).AnyTimes() + // Allow rollback Remove on error paths. + inst.EXPECT().Remove(gomock.Any()).Return(nil).AnyTimes() + + svc := New(store, + WithGroupManager(gm), + WithPathResolver(pr), + WithInstaller(inst), + ) _, err := svc.Install(t.Context(), tt.opts) if tt.wantErr != "" { From ece3390b2a45990e25bfc0d9c59ea08267aac1c1 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 9 Mar 2026 10:06:55 +0200 Subject: [PATCH 2/4] Sanitize targetDir at WriteFiles entry for CodeQL Add filepath.Clean at function entry so CodeQL can trace the sanitized path through all downstream os calls. Add #nosec annotations for gosec consistency with the existing installer.go patterns. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/skills/gitresolver/writer.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/skills/gitresolver/writer.go b/pkg/skills/gitresolver/writer.go index ae8d75fab2..906168ad06 100644 --- a/pkg/skills/gitresolver/writer.go +++ b/pkg/skills/gitresolver/writer.go @@ -19,13 +19,20 @@ const ( // WriteFiles writes resolved skill files to the target directory. // If force is true, any existing directory is removed before writing. +// +// Security: targetDir is produced by PathResolver.GetSkillPath (a trusted +// internal source that builds paths from known base directories). File paths +// within the archive are validated via containment check against targetDir. func WriteFiles(files []FileEntry, targetDir string, force bool) error { + // Sanitize targetDir early so all downstream os calls use the clean path. + targetDir = filepath.Clean(targetDir) + // Handle existing directory - if _, statErr := os.Stat(targetDir); statErr == nil { + if _, statErr := os.Stat(targetDir); statErr == nil { //#nosec G304 -- targetDir is cleaned and produced by PathResolver if !force { return fmt.Errorf("target directory %q already exists; use force to overwrite", targetDir) } - if err := os.RemoveAll(targetDir); err != nil { + if err := os.RemoveAll(targetDir); err != nil { //#nosec G304 -- targetDir is cleaned above return fmt.Errorf("removing existing directory: %w", err) } } @@ -35,7 +42,7 @@ func WriteFiles(files []FileEntry, targetDir string, force bool) error { return fmt.Errorf("target path validation: %w", err) } - if err := os.MkdirAll(targetDir, dirPermissions); err != nil { + if err := os.MkdirAll(targetDir, dirPermissions); err != nil { //#nosec G304 -- targetDir is cleaned above return fmt.Errorf("creating target directory: %w", err) } @@ -85,7 +92,7 @@ func validatePathNoSymlinks(targetDir string) error { } current = filepath.Join(current, component) - info, err := os.Lstat(current) + info, err := os.Lstat(current) //#nosec G304 -- current is built from filepath.Abs of the cleaned targetDir if err != nil { // Path doesn't exist yet — remaining components will be created by MkdirAll. break From 55d5e8073a22fed2a135980ed49a64dfeb9674c4 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 9 Mar 2026 10:23:51 +0200 Subject: [PATCH 3/4] Add CodeQL inline suppression for go/path-injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The targetDir parameter in WriteFiles is produced by PathResolver.GetSkillPath which builds paths from known base directories — not directly from user input. Add codeql[go/path-injection] inline suppression comments to document this. This is the same false-positive pattern as the existing alerts in pkg/skills/installer.go and pkg/skills/skillsvc/skillsvc.go on main. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/skills/gitresolver/writer.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/skills/gitresolver/writer.go b/pkg/skills/gitresolver/writer.go index 906168ad06..a8595e53d5 100644 --- a/pkg/skills/gitresolver/writer.go +++ b/pkg/skills/gitresolver/writer.go @@ -28,11 +28,11 @@ func WriteFiles(files []FileEntry, targetDir string, force bool) error { targetDir = filepath.Clean(targetDir) // Handle existing directory - if _, statErr := os.Stat(targetDir); statErr == nil { //#nosec G304 -- targetDir is cleaned and produced by PathResolver + if _, statErr := os.Stat(targetDir); statErr == nil { // lgtm[go/path-injection] #nosec G304 if !force { return fmt.Errorf("target directory %q already exists; use force to overwrite", targetDir) } - if err := os.RemoveAll(targetDir); err != nil { //#nosec G304 -- targetDir is cleaned above + if err := os.RemoveAll(targetDir); err != nil { // lgtm[go/path-injection] #nosec G304 -- targetDir is cleaned above return fmt.Errorf("removing existing directory: %w", err) } } @@ -42,7 +42,7 @@ func WriteFiles(files []FileEntry, targetDir string, force bool) error { return fmt.Errorf("target path validation: %w", err) } - if err := os.MkdirAll(targetDir, dirPermissions); err != nil { //#nosec G304 -- targetDir is cleaned above + if err := os.MkdirAll(targetDir, dirPermissions); err != nil { // lgtm[go/path-injection] #nosec G304 return fmt.Errorf("creating target directory: %w", err) } @@ -92,7 +92,7 @@ func validatePathNoSymlinks(targetDir string) error { } current = filepath.Join(current, component) - info, err := os.Lstat(current) //#nosec G304 -- current is built from filepath.Abs of the cleaned targetDir + info, err := os.Lstat(current) // lgtm[go/path-injection] #nosec G304 -- built from filepath.Abs of cleaned targetDir if err != nil { // Path doesn't exist yet — remaining components will be created by MkdirAll. break From bd0f454450ca47906abe2ee6240e00fe8726e7f3 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 9 Mar 2026 12:19:16 +0200 Subject: [PATCH 4/4] Fix E2E skills tests for new plain-name 404 behavior The git reference support PR replaced dead-end "pending" records with an actionable 404 for unresolvable plain skill names. The E2E tests were still installing plain names without building first. Each affected test now calls buildTestSkill() before installSkill() to place the artifact in the local OCI store, matching the real build-then-install workflow. A new test explicitly covers the 404 path for unresolvable names. Co-Authored-By: Claude Opus 4.6 --- test/e2e/api_skills_test.go | 58 ++++++++++++++++++++++++++++++++----- 1 file changed, 50 insertions(+), 8 deletions(-) diff --git a/test/e2e/api_skills_test.go b/test/e2e/api_skills_test.go index 10b953598a..bdbbc053db 100644 --- a/test/e2e/api_skills_test.go +++ b/test/e2e/api_skills_test.go @@ -159,6 +159,16 @@ func buildSkill(server *e2e.Server, path, tag string) *http.Response { // createTestSkillDir creates a temporary directory with a valid SKILL.md file. // The directory name matches the skill name (validator requirement). +// buildTestSkill creates a test skill directory, builds it via the API (so it +// lands in the local OCI store), and returns the skill directory path. The +// caller can then install the skill by plain name. +func buildTestSkill(server *e2e.Server, skillName, description string) { + skillDir := createTestSkillDir(skillName, description) + resp := buildSkill(server, skillDir, "") + defer resp.Body.Close() + ExpectWithOffset(1, resp.StatusCode).To(Equal(http.StatusOK)) +} + func createTestSkillDir(skillName, description string) string { parentDir := GinkgoT().TempDir() skillDir := filepath.Join(parentDir, skillName) @@ -460,7 +470,8 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { }) It("should include installed skills", func() { - By("Installing a skill") + By("Building and installing a skill") + buildTestSkill(apiServer, "list-test-skill", "A skill for list test") installResp := installSkill(apiServer, installSkillRequest{Name: "list-test-skill"}) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -496,18 +507,21 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { } }) - It("should install a skill with pending status", func() { - By("Installing a skill by name") + It("should install a locally-built skill", func() { + By("Building a skill so it is available in the local store") + buildTestSkill(apiServer, "install-test-skill", "A skill for install test") + + By("Installing the skill by name") resp := installSkill(apiServer, installSkillRequest{Name: "install-test-skill"}) defer resp.Body.Close() By("Verifying response status is 201 Created") Expect(resp.StatusCode).To(Equal(http.StatusCreated)) - By("Verifying the skill has pending status") + By("Verifying the skill has installed status") var result installSkillResponse Expect(json.NewDecoder(resp.Body).Decode(&result)).To(Succeed()) - Expect(result.Skill.Status).To(Equal("pending")) + Expect(result.Skill.Status).To(Equal("installed")) Expect(result.Skill.Metadata.Name).To(Equal("install-test-skill")) Expect(result.Skill.InstalledAt).ToNot(BeZero(), "InstalledAt should be a valid timestamp") @@ -515,6 +529,15 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { Expect(resp.Header.Get("Location")).To(Equal("/api/v1beta/skills/install-test-skill")) }) + It("should return 404 for unresolvable plain name", func() { + By("Attempting to install a skill that has not been built or published") + resp := installSkill(apiServer, installSkillRequest{Name: "no-such-skill"}) + defer resp.Body.Close() + + By("Verifying response status is 404 Not Found") + Expect(resp.StatusCode).To(Equal(http.StatusNotFound)) + }) + It("should reject empty name", func() { By("Attempting to install with empty name") resp := installSkill(apiServer, installSkillRequest{Name: ""}) @@ -534,7 +557,8 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { }) It("should reject duplicate install", func() { - By("Installing a skill") + By("Building and installing a skill") + buildTestSkill(apiServer, "dup-test-skill", "A skill for dup test") resp := installSkill(apiServer, installSkillRequest{Name: "dup-test-skill"}) defer resp.Body.Close() Expect(resp.StatusCode).To(Equal(http.StatusCreated)) @@ -569,7 +593,8 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { }) It("should return info for an installed skill", func() { - By("Installing a skill") + By("Building and installing a skill") + buildTestSkill(apiServer, "info-test-skill", "A skill for info test") installResp := installSkill(apiServer, installSkillRequest{Name: "info-test-skill"}) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -608,7 +633,8 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { Describe("DELETE /api/v1beta/skills/{name} - Uninstall a skill", func() { It("should uninstall an installed skill", func() { - By("Installing a skill") + By("Building and installing a skill") + buildTestSkill(apiServer, "uninstall-test", "A skill for uninstall test") installResp := installSkill(apiServer, installSkillRequest{Name: "uninstall-test"}) defer installResp.Body.Close() Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) @@ -670,6 +696,9 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { It("should register the skill in the group on install", func() { skillName := "group-install-skill" + By("Building the skill") + buildTestSkill(apiServer, skillName, "A skill for group install test") + By("Installing a skill into the group") resp := installSkill(apiServer, installSkillRequest{Name: skillName, Group: groupName}) defer resp.Body.Close() @@ -693,6 +722,10 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { skillInGroup := "group-filter-in" skillOutGroup := "group-filter-out" + By("Building both skills") + buildTestSkill(apiServer, skillInGroup, "A skill for group filter test (in)") + buildTestSkill(apiServer, skillOutGroup, "A skill for group filter test (out)") + By("Installing a skill into the group") r1 := installSkill(apiServer, installSkillRequest{Name: skillInGroup, Group: groupName}) defer r1.Body.Close() @@ -722,6 +755,9 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { It("should remove the skill from the group on uninstall", func() { skillName := "group-uninstall-skill" + By("Building the skill") + buildTestSkill(apiServer, skillName, "A skill for group uninstall test") + By("Installing a skill into the group") r1 := installSkill(apiServer, installSkillRequest{Name: skillName, Group: groupName}) defer r1.Body.Close() @@ -747,6 +783,9 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { }) It("should return error when installing into a non-existent group", func() { + By("Building the skill so the name resolves") + buildTestSkill(apiServer, "group-noexist-skill", "A skill for non-existent group test") + By("Attempting to install a skill into a non-existent group") resp := installSkill(apiServer, installSkillRequest{ Name: "group-noexist-skill", @@ -763,6 +802,9 @@ var _ = Describe("Skills API", Label("api", "skills", "e2e"), func() { It("should support install → list → info → uninstall → list → info", func() { skillName := "lifecycle-test" + By("Building the skill") + buildTestSkill(apiServer, skillName, "A skill for lifecycle test") + By("Installing the skill") installResp := installSkill(apiServer, installSkillRequest{Name: skillName}) defer installResp.Body.Close()