Resolve comments

cmd/picoclaw/main.go

@@ -37,6 +37,7 @@ import (
 	"github.com/sipeed/picoclaw/pkg/skills"
 	"github.com/sipeed/picoclaw/pkg/state"
 	"github.com/sipeed/picoclaw/pkg/tools"
+	"github.com/sipeed/picoclaw/pkg/utils"
 	"github.com/sipeed/picoclaw/pkg/voice"
 )
 
@@ -1300,6 +1301,18 @@ func skillsInstallCmd(installer *skills.SkillInstaller, cfg *config.Config) {
 
 // skillsInstallFromRegistry installs a skill from a named registry (e.g. clawhub).
 func skillsInstallFromRegistry(cfg *config.Config, registryName, slug string) {
+	err := utils.ValidateSkillIdentifier(registryName)
+	if err != nil {
+		fmt.Printf("\u2717 Invalid registry name: %v\n", err)
+		os.Exit(1)
+	}
+
+	err = utils.ValidateSkillIdentifier(slug)
+	if err != nil {
+		fmt.Printf("\u2717 Invalid slug: %v\n", err)
+		os.Exit(1)
+	}
+
 	fmt.Printf("Installing skill '%s' from %s registry...\n", slug, registryName)
 
 	registryMgr := skills.NewRegistryManagerFromConfig(skills.RegistryConfig{

pkg/config/config.go

@@ -244,14 +244,15 @@ type SkillsRegistriesConfig struct {
 }
 
 type ClawHubRegistryConfig struct {
-	Enabled      bool   `json:"enabled" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_ENABLED"`
+	Enabled         bool   `json:"enabled" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_ENABLED"`
-	BaseURL      string `json:"base_url" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_BASE_URL"`
+	BaseURL         string `json:"base_url" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_BASE_URL"`
-	AuthToken    string `json:"auth_token" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_AUTH_TOKEN"`
+	AuthToken       string `json:"auth_token" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_AUTH_TOKEN"`
-	SearchPath   string `json:"search_path" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_SEARCH_PATH"`
+	SearchPath      string `json:"search_path" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_SEARCH_PATH"`
-	SkillsPath   string `json:"skills_path" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_SKILLS_PATH"`
+	SkillsPath      string `json:"skills_path" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_SKILLS_PATH"`
-	DownloadPath string `json:"download_path" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_DOWNLOAD_PATH"`
+	DownloadPath    string `json:"download_path" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_DOWNLOAD_PATH"`
-	Timeout      int    `json:"timeout" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_TIMEOUT"`
+	Timeout         int    `json:"timeout" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_TIMEOUT"`
-	MaxZipSize   int    `json:"max_zip_size" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_MAX_ZIP_SIZE"`
+	MaxZipSize      int    `json:"max_zip_size" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_MAX_ZIP_SIZE"`
+	MaxResponseSize int    `json:"max_response_size" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_MAX_RESPONSE_SIZE"`
 }
 
 func DefaultConfig() *Config {

pkg/skills/clawhub_registry.go

@@ -14,19 +14,21 @@ import (
 )
 
 const (
-	defaultClawHubTimeout = 30 * time.Second
+	defaultClawHubTimeout  = 30 * time.Second
-	defaultMaxZipSize     = 50 * 1024 * 1024 // 50 MB
+	defaultMaxZipSize      = 50 * 1024 * 1024 // 50 MB
+	defaultMaxResponseSize = 2 * 1024 * 1024  // 2 MB
 )
 
 // ClawHubRegistry implements SkillRegistry for the ClawhHub platform.
 type ClawHubRegistry struct {
-	baseURL      string
+	baseURL         string
-	authToken    string // Optional - for elevated rate limits
+	authToken       string // Optional - for elevated rate limits
-	searchPath   string // Search API
+	searchPath      string // Search API
-	skillsPath   string // For retrieving skill metadata
+	skillsPath      string // For retrieving skill metadata
-	downloadPath string // For fetching ZIP files for download
+	downloadPath    string // For fetching ZIP files for download
-	maxZipSize   int
+	maxZipSize      int
-	client       *http.Client
+	maxResponseSize int
+	client          *http.Client
 }
 
 // NewClawHubRegistry creates a new ClawhHub registry client from config.
@@ -58,13 +60,19 @@ func NewClawHubRegistry(cfg ClawHubConfig) *ClawHubRegistry {
 		maxZip = cfg.MaxZipSize
 	}
 
+	maxResp := defaultMaxResponseSize
+	if cfg.MaxResponseSize > 0 {
+		maxResp = cfg.MaxResponseSize
+	}
+
 	return &ClawHubRegistry{
-		baseURL:      baseURL,
+		baseURL:         baseURL,
-		authToken:    cfg.AuthToken,
+		authToken:       cfg.AuthToken,
-		searchPath:   searchPath,
+		searchPath:      searchPath,
-		skillsPath:   skillsPath,
+		skillsPath:      skillsPath,
-		downloadPath: downloadPath,
+		downloadPath:    downloadPath,
-		maxZipSize:   maxZip,
+		maxZipSize:      maxZip,
+		maxResponseSize: maxResp,
 		client: &http.Client{
 			Timeout: timeout,
 			Transport: &http.Transport{
@@ -290,7 +298,7 @@ func (c *ClawHubRegistry) doGet(ctx context.Context, urlStr string) ([]byte, err
 	defer resp.Body.Close()
 
 	// Limit response body read to prevent memory issues.
-	body, err := io.ReadAll(io.LimitReader(resp.Body, int64(c.maxZipSize)+1024))
+	body, err := io.ReadAll(io.LimitReader(resp.Body, int64(c.maxResponseSize)))
 	if err != nil {
 		return nil, fmt.Errorf("failed to read response: %w", err)
 	}

pkg/skills/registry.go

@@ -66,14 +66,15 @@ type RegistryConfig struct {
 
 // ClawHubConfig configures the ClawhHub registry.
 type ClawHubConfig struct {
-	Enabled      bool
+	Enabled         bool
-	BaseURL      string
+	BaseURL         string
-	AuthToken    string
+	AuthToken       string
-	SearchPath   string // e.g. "/api/v1/search"
+	SearchPath      string // e.g. "/api/v1/search"
-	SkillsPath   string // e.g. "/api/v1/skills"
+	SkillsPath      string // e.g. "/api/v1/skills"
-	DownloadPath string // e.g. "/api/v1/download"
+	DownloadPath    string // e.g. "/api/v1/download"
-	Timeout      int    // seconds, 0 = default (30s)
+	Timeout         int    // seconds, 0 = default (30s)
-	MaxZipSize   int    // bytes, 0 = default (50MB)
+	MaxZipSize      int    // bytes, 0 = default (50MB)
+	MaxResponseSize int    // bytes, 0 = default (2MB)
 }
 
 // RegistryManager coordinates multiple skill registries.

pkg/tools/skills_install.go

@@ -20,6 +20,7 @@ import (
 type InstallSkillTool struct {
 	registryMgr *skills.RegistryManager
 	workspace   string
+	mu          sync.Mutex
 }
 
 // NewInstallSkillTool creates a new InstallSkillTool.
@@ -29,6 +30,7 @@ func NewInstallSkillTool(registryMgr *skills.RegistryManager, workspace string)
 	return &InstallSkillTool{
 		registryMgr: registryMgr,
 		workspace:   workspace,
+		mu:          sync.Mutex{},
 	}
 }
 
@@ -68,9 +70,8 @@ func (t *InstallSkillTool) Parameters() map[string]interface{} {
 func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]interface{}) *ToolResult {
 	// Install lock to prevent concurrent directory operations.
 	// Ideally this should be done at a `slug` level, currently, its at a `workspace` level.
-	slugLock := sync.Mutex{}
+	t.mu.Lock()
-	slugLock.Lock()
+	defer t.mu.Unlock()
-	defer slugLock.Unlock()
 
 	// Validate slug
 	slug, _ := args["slug"].(string)

pkg/utils/zip.go

@@ -41,8 +41,9 @@ func ExtractZipFile(zipPath string, targetDir string) error {
 
 		destPath := filepath.Join(targetDir, cleanName)
 
-		// Double-check the resolved path is within target.
+		// Double-check the resolved path is within target directory (defense-in-depth).
-		if !strings.HasPrefix(filepath.Clean(destPath), filepath.Clean(targetDir)) {
+		targetDirClean := filepath.Clean(targetDir)
+		if !strings.HasPrefix(filepath.Clean(destPath), targetDirClean+string(filepath.Separator)) && filepath.Clean(destPath) != targetDirClean {
 			return fmt.Errorf("zip entry escapes target dir: %q", f.Name)
 		}
 
@@ -73,8 +74,15 @@ func ExtractZipFile(zipPath string, targetDir string) error {
 	return nil
 }
 
-// extractSingleFile extracts one zip.File entry to destPath.
+// extractSingleFile extracts one zip.File entry to destPath, with a size check.
 func extractSingleFile(f *zip.File, destPath string) error {
+	const maxFileSize = 5 * 1024 * 1024 // 5MB, adjust as appropriate
+
+	// Check the uncompressed size from the header, if available.
+	if f.UncompressedSize64 > maxFileSize {
+		return fmt.Errorf("zip entry %q is too large (%d bytes)", f.Name, f.UncompressedSize64)
+	}
+
 	rc, err := f.Open()
 	if err != nil {
 		return fmt.Errorf("failed to open zip entry %q: %w", f.Name, err)
@@ -85,16 +93,23 @@ func extractSingleFile(f *zip.File, destPath string) error {
 	if err != nil {
 		return fmt.Errorf("failed to create file %q: %w", destPath, err)
 	}
+	defer func() {
+		// Ensure file is closed in all paths.
+		if cerr := outFile.Close(); cerr != nil && err == nil {
+			err = fmt.Errorf("failed to close file %q: %w", destPath, cerr)
+			_ = os.Remove(destPath)
+		}
+	}()
 
-	if _, err := io.Copy(outFile, rc); err != nil {
+	// Streamed size check: prevent overruns and malicious/corrupt headers.
-		_ = outFile.Close()
+	written, err := io.CopyN(outFile, rc, maxFileSize+1)
+	if err != nil && err != io.EOF {
 		_ = os.Remove(destPath)
 		return fmt.Errorf("failed to extract %q: %w", f.Name, err)
 	}
-
+	if written > maxFileSize {
-	if err := outFile.Close(); err != nil {
 		_ = os.Remove(destPath)
-		return fmt.Errorf("failed to close file %q: %w", destPath, err)
+		return fmt.Errorf("zip entry %q exceeds max size (%d bytes)", f.Name, written)
 	}
 
 	return nil