Fix race condition, input validation, and code quality issues

- Add sync.RWMutex to protect global searchCache from concurrent access
- Replace O(n²) duplicate skill check with map-based O(1) lookup
- Add git ref validation to prevent malformed references in Checkout
- Add path traversal protection in buildRepoURL/buildRepoName
- Replace io.ReadAtLeast with io.ReadAll+LimitReader for safer reads
- Consolidate hardcoded HTTP timeouts into named constants
- Add OS signal handling for graceful sync cancellation
- Remove redundant helper functions, use strings stdlib instead

Author: yeasy

cmd/install.go

@@ -139,16 +139,14 @@ func runInstall(cmd *cobra.Command, args []string) {
 					count++
 				}
 				// Add from legacy skills list if not duplicate
+				seen := make(map[string]bool, len(skillArgs))
+				for _, existing := range skillArgs {
+					seen[existing] = true
+				}
 				for _, s := range cfg.Skills {
-					exists := false
-					for _, existing := range skillArgs {
-						if existing == s {
-							exists = true
-							break
-						}
-					}
-					if !exists {
+					if !seen[s] {
 						skillArgs = append(skillArgs, s)
+						seen[s] = true
 						count++
 					}
 				}

cmd/sync.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"os/signal"
 	"strings"
 	"sync"
 	"time"
@@ -80,7 +81,9 @@ If no repo name is specified, syncs all configured repositories.`,
 		bar := ui.NewProgressBar(len(targetRepos), "Syncing repositories")
 
 		// Use errgroup for parallel syncing with limit
-		ctx := context.Background()
+		// Support cancellation via OS signals (Ctrl+C)
+		ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
+		defer stop()
 		g, ctx := errgroup.WithContext(ctx)
 		g.SetLimit(5) // Limit concurrency to 5
 
@@ -169,28 +172,48 @@ If no repo name is specified, syncs all configured repositories.`,
 	},
 }
 
-// buildRepoURL constructs the git clone URL from repo config
-func buildRepoURL(url string) string {
+// buildRepoURL constructs the git clone URL from repo config.
+// Validates that owner/repo parts do not contain path traversal patterns.
+func buildRepoURL(repoURL string) string {
 	// Handle owner/repo format
-	if !strings.HasPrefix(url, "http") && !strings.HasPrefix(url, "git@") {
+	if !strings.HasPrefix(repoURL, "http") && !strings.HasPrefix(repoURL, "git@") {
 		// Extract owner/repo from path like "anthropics/skills/skills"
-		parts := strings.Split(url, "/")
+		parts := strings.Split(repoURL, "/")
 		if len(parts) >= 2 {
-			return fmt.Sprintf("https://github.com/%s/%s.git", parts[0], parts[1])
+			owner, repo := parts[0], parts[1]
+			// Reject path traversal or empty segments
+			if owner == ".." || repo == ".." || owner == "." || repo == "." || owner == "" || repo == "" {
+				return ""
+			}
+			return fmt.Sprintf("https://github.com/%s/%s.git", owner, repo)
+		}
+		if repoURL == ".." || repoURL == "." || repoURL == "" {
+			return ""
 		}
-		return "https://github.com/" + url + ".git"
+		return "https://github.com/" + repoURL + ".git"
 	}
-	return url
+	return repoURL
 }
 
-// buildRepoName constructs a filesystem-safe name from repo URL
-func buildRepoName(url string) string {
+// buildRepoName constructs a filesystem-safe name from repo URL.
+// Strips path traversal patterns to prevent directory escape.
+func buildRepoName(repoURL string) string {
 	// Handle owner/repo/path format
-	parts := strings.Split(url, "/")
+	parts := strings.Split(repoURL, "/")
 	if len(parts) >= 2 {
-		return parts[0] + "-" + parts[1]
+		owner := strings.ReplaceAll(parts[0], "..", "")
+		repo := strings.ReplaceAll(parts[1], "..", "")
+		if owner == "" || repo == "" {
+			return "unknown-repo"
+		}
+		return owner + "-" + repo
+	}
+	name := strings.ReplaceAll(repoURL, "/", "-")
+	name = strings.ReplaceAll(name, "..", "")
+	if name == "" {
+		return "unknown-repo"
 	}
-	return strings.ReplaceAll(url, "/", "-")
+	return name
 }
 
 func init() {

internal/git/git.go

@@ -172,15 +172,36 @@ func GetLatestTag(repoPath string) (string, error) {
 	return strings.TrimSpace(string(output)), nil
 }
 
-// Checkout checks out a specific tag or branch
+// Checkout checks out a specific tag or branch.
+// The ref is validated to prevent unexpected git behavior from malformed references.
 func Checkout(repoPath, ref string) error {
+	if err := validateGitRef(ref); err != nil {
+		return fmt.Errorf("invalid git ref: %w", err)
+	}
 	cmd := exec.Command("git", "checkout", ref)
 	cmd.Dir = repoPath
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	return cmd.Run()
 }
 
+// validateGitRef checks that a git reference string is safe to use.
+func validateGitRef(ref string) error {
+	if ref == "" {
+		return fmt.Errorf("ref cannot be empty")
+	}
+	if strings.Contains(ref, "..") {
+		return fmt.Errorf("ref cannot contain '..'")
+	}
+	if strings.ContainsAny(ref, " \t\n\r~^:?*[\\") {
+		return fmt.Errorf("ref contains invalid characters")
+	}
+	if strings.HasPrefix(ref, "-") {
+		return fmt.Errorf("ref cannot start with '-'")
+	}
+	return nil
+}
+
 // GetCurrentCommit returns the current commit hash of the repository
 func GetCurrentCommit(repoPath string) (string, error) {
 	cmd := exec.Command("git", "rev-parse", "HEAD")

internal/github/client.go

@@ -9,6 +9,7 @@ import (
 	"net/url"
 	"os"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/yeasy/ask/internal/cache"
@@ -20,10 +21,20 @@ const (
 	SkillTopic = "agent-skill"
 	// APIURL is the GitHub API endpoint for searching repositories
 	APIURL = "https://api.github.com/search/repositories"
+
+	// httpTimeoutDefault is the default timeout for GitHub API requests
+	httpTimeoutDefault = 10 * time.Second
+	// httpTimeoutShort is a shorter timeout for non-critical requests like fetching descriptions
+	httpTimeoutShort = 5 * time.Second
+	// maxDescriptionReadBytes limits how much of SKILL.md we read for description extraction
+	maxDescriptionReadBytes = 4096
 )
 
-// Global cache instance
-var searchCache *cache.Cache
+// Global cache instance, protected by cacheMu for concurrent access
+var (
+	searchCache *cache.Cache
+	cacheMu     sync.RWMutex
+)
 
 // OfflineMode returns whether the application is in offline mode.
 // Delegates to config.OfflineMode as the single source of truth.
@@ -41,6 +52,25 @@ func init() {
 	}
 }
 
+// cacheGet safely reads from the global cache under a read lock.
+func cacheGet(key string, dest interface{}) bool {
+	cacheMu.RLock()
+	defer cacheMu.RUnlock()
+	if searchCache == nil {
+		return false
+	}
+	return searchCache.Get(key, dest)
+}
+
+// cacheSet safely writes to the global cache under a write lock.
+func cacheSet(key string, value interface{}) {
+	cacheMu.Lock()
+	defer cacheMu.Unlock()
+	if searchCache != nil {
+		_ = searchCache.Set(key, value)
+	}
+}
+
 // SearchResult represents the response from GitHub search API
 type SearchResult struct {
 	TotalCount int          `json:"total_count"`
@@ -97,11 +127,9 @@ func SearchTopic(topic, keyword string) ([]Repository, error) {
 
 	// Try cache first
 	// In offline mode, we MUST find it in cache or return error
-	if searchCache != nil {
-		var cached []Repository
-		if searchCache.Get(cacheKey, &cached) {
-			return cached, nil
-		}
+	var cached []Repository
+	if cacheGet(cacheKey, &cached) {
+		return cached, nil
 	}
 
 	if isOffline() {
@@ -129,7 +157,7 @@ func SearchTopic(topic, keyword string) ([]Repository, error) {
 	req.Header.Set("Accept", "application/vnd.github.v3+json")
 	req.Header.Set("User-Agent", "ask-cli")
 
-	client := &http.Client{Timeout: 10 * time.Second}
+	client := &http.Client{Timeout: httpTimeoutDefault}
 	resp, err := client.Do(req)
 	if err != nil {
 		return nil, err
@@ -146,9 +174,7 @@ func SearchTopic(topic, keyword string) ([]Repository, error) {
 	}
 
 	// Cache the result
-	if searchCache != nil {
-		_ = searchCache.Set(cacheKey, result.Items)
-	}
+	cacheSet(cacheKey, result.Items)
 
 	return result.Items, nil
 }
@@ -165,11 +191,9 @@ func SearchDir(owner, repo, path string) ([]Repository, error) {
 	cacheKey := fmt.Sprintf("dir:%s/%s/%s", owner, repo, path)
 
 	// Try cache first
-	if searchCache != nil {
-		var cached []Repository
-		if searchCache.Get(cacheKey, &cached) {
-			return cached, nil
-		}
+	var cached []Repository
+	if cacheGet(cacheKey, &cached) {
+		return cached, nil
 	}
 
 	if isOffline() {
@@ -190,7 +214,7 @@ func SearchDir(owner, repo, path string) ([]Repository, error) {
 	req.Header.Set("Accept", "application/vnd.github.v3+json")
 	req.Header.Set("User-Agent", "ask-cli")
 
-	client := &http.Client{Timeout: 10 * time.Second}
+	client := &http.Client{Timeout: httpTimeoutDefault}
 	resp, err := client.Do(req)
 	if err != nil {
 		return nil, err
@@ -240,9 +264,7 @@ func SearchDir(owner, repo, path string) ([]Repository, error) {
 	}
 
 	// Cache the result
-	if searchCache != nil {
-		_ = searchCache.Set(cacheKey, skills)
-	}
+	cacheSet(cacheKey, skills)
 
 	return skills, nil
 }
@@ -251,11 +273,9 @@ func SearchDir(owner, repo, path string) ([]Repository, error) {
 func fetchSkillDescription(owner, repo, skillPath string) string {
 	// Check cache first
 	cacheKey := fmt.Sprintf("skill-desc:%s/%s/%s", owner, repo, skillPath)
-	if searchCache != nil {
-		var cached string
-		if searchCache.Get(cacheKey, &cached) {
-			return cached
-		}
+	var cached string
+	if cacheGet(cacheKey, &cached) {
+		return cached
 	}
 
 	// Fetch SKILL.md content
@@ -273,7 +293,7 @@ func fetchSkillDescription(owner, repo, skillPath string) string {
 	req.Header.Set("Accept", "application/vnd.github.v3.raw") // Get raw file content
 	req.Header.Set("User-Agent", "ask-cli")
 
-	client := &http.Client{Timeout: 5 * time.Second}
+	client := &http.Client{Timeout: httpTimeoutShort}
 	resp, err := client.Do(req)
 	if err != nil {
 		return ""
@@ -284,20 +304,19 @@ func fetchSkillDescription(owner, repo, skillPath string) string {
 		return ""
 	}
 
-	// Read the content (limit to 4KB to avoid huge files)
-	buf := make([]byte, 4096)
-	n, err := io.ReadAtLeast(resp.Body, buf, 1)
-	if err != nil && n == 0 {
+	// Read the content (limit to maxDescriptionReadBytes to avoid huge files)
+	data, err := io.ReadAll(io.LimitReader(resp.Body, maxDescriptionReadBytes))
+	if err != nil || len(data) == 0 {
 		return ""
 	}
-	content := string(buf[:n])
+	content := string(data)
 
 	// Parse description from SKILL.md (check both frontmatter and first paragraph)
 	desc := parseDescriptionFromSkillMD(content)
 
 	// Cache the description
-	if searchCache != nil && desc != "" {
-		_ = searchCache.Set(cacheKey, desc)
+	if desc != "" {
+		cacheSet(cacheKey, desc)
 	}
 
 	return desc
@@ -328,7 +347,7 @@ func parseDescriptionFromSkillMD(content string) string {
 
 	// If no frontmatter description, look for first non-empty non-heading line
 	for _, line := range lines {
-		line = trimSpace(line)
+		line = strings.TrimSpace(line)
 		if line == "" || line == "---" {
 			continue
 		}
@@ -342,36 +361,13 @@ func parseDescriptionFromSkillMD(content string) string {
 	return ""
 }
 
-// Helper functions to avoid importing strings package
+// splitLines splits a string into lines by newline character.
 func splitLines(s string) []string {
-	var lines []string
-	start := 0
-	for i := 0; i < len(s); i++ {
-		if s[i] == '\n' {
-			lines = append(lines, s[start:i])
-			start = i + 1
-		}
-	}
-	if start < len(s) {
-		lines = append(lines, s[start:])
-	}
-	return lines
-}
-
-func trimSpace(s string) string {
-	start := 0
-	end := len(s)
-	for start < end && (s[start] == ' ' || s[start] == '\t' || s[start] == '\r') {
-		start++
-	}
-	for end > start && (s[end-1] == ' ' || s[end-1] == '\t' || s[end-1] == '\r') {
-		end--
-	}
-	return s[start:end]
+	return strings.Split(s, "\n")
 }
 
 func trimQuotes(s string) string {
-	s = trimSpace(s)
+	s = strings.TrimSpace(s)
 	if len(s) >= 2 && ((s[0] == '"' && s[len(s)-1] == '"') || (s[0] == '\'' && s[len(s)-1] == '\'')) {
 		return s[1 : len(s)-1]
 	}
@@ -511,7 +507,7 @@ func FetchRepoDetails(owner, repo string) (*Repository, error) {
 	req.Header.Set("Accept", "application/vnd.github.v3+json")
 	req.Header.Set("User-Agent", "ask-cli")
 
-	client := &http.Client{Timeout: 10 * time.Second}
+	client := &http.Client{Timeout: httpTimeoutDefault}
 	resp, err := client.Do(req)
 	if err != nil {
 		return nil, err