diff --git a/cmd/picoclaw/main.go b/cmd/picoclaw/main.go index 3bba58ba0..a7f85eb4d 100644 --- a/cmd/picoclaw/main.go +++ b/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{ diff --git a/pkg/config/config.go b/pkg/config/config.go index 0f7d45137..d73fddecc 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -244,14 +244,15 @@ type SkillsRegistriesConfig struct { } type ClawHubRegistryConfig struct { - Enabled bool `json:"enabled" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_ENABLED"` - BaseURL string `json:"base_url" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_BASE_URL"` - AuthToken string `json:"auth_token" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_AUTH_TOKEN"` - SearchPath string `json:"search_path" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_SEARCH_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"` - Timeout int `json:"timeout" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_TIMEOUT"` - MaxZipSize int `json:"max_zip_size" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_MAX_ZIP_SIZE"` + Enabled bool `json:"enabled" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_ENABLED"` + BaseURL string `json:"base_url" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_BASE_URL"` + AuthToken string `json:"auth_token" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_AUTH_TOKEN"` + SearchPath string `json:"search_path" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_SEARCH_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"` + Timeout int `json:"timeout" env:"PICOCLAW_SKILLS_REGISTRIES_CLAWHUB_TIMEOUT"` + 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 { diff --git a/pkg/skills/clawhub_registry.go b/pkg/skills/clawhub_registry.go index 07867e709..3f5e27ba0 100644 --- a/pkg/skills/clawhub_registry.go +++ b/pkg/skills/clawhub_registry.go @@ -14,19 +14,21 @@ import ( ) const ( - defaultClawHubTimeout = 30 * time.Second - defaultMaxZipSize = 50 * 1024 * 1024 // 50 MB + defaultClawHubTimeout = 30 * time.Second + defaultMaxZipSize = 50 * 1024 * 1024 // 50 MB + defaultMaxResponseSize = 2 * 1024 * 1024 // 2 MB ) // ClawHubRegistry implements SkillRegistry for the ClawhHub platform. type ClawHubRegistry struct { - baseURL string - authToken string // Optional - for elevated rate limits - searchPath string // Search API - skillsPath string // For retrieving skill metadata - downloadPath string // For fetching ZIP files for download - maxZipSize int - client *http.Client + baseURL string + authToken string // Optional - for elevated rate limits + searchPath string // Search API + skillsPath string // For retrieving skill metadata + downloadPath string // For fetching ZIP files for download + maxZipSize int + 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, - authToken: cfg.AuthToken, - searchPath: searchPath, - skillsPath: skillsPath, - downloadPath: downloadPath, - maxZipSize: maxZip, + baseURL: baseURL, + authToken: cfg.AuthToken, + searchPath: searchPath, + skillsPath: skillsPath, + downloadPath: downloadPath, + 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) } diff --git a/pkg/skills/registry.go b/pkg/skills/registry.go index cb4e6f5a1..3be27cd11 100644 --- a/pkg/skills/registry.go +++ b/pkg/skills/registry.go @@ -66,14 +66,15 @@ type RegistryConfig struct { // ClawHubConfig configures the ClawhHub registry. type ClawHubConfig struct { - Enabled bool - BaseURL string - AuthToken string - SearchPath string // e.g. "/api/v1/search" - SkillsPath string // e.g. "/api/v1/skills" - DownloadPath string // e.g. "/api/v1/download" - Timeout int // seconds, 0 = default (30s) - MaxZipSize int // bytes, 0 = default (50MB) + Enabled bool + BaseURL string + AuthToken string + SearchPath string // e.g. "/api/v1/search" + SkillsPath string // e.g. "/api/v1/skills" + DownloadPath string // e.g. "/api/v1/download" + Timeout int // seconds, 0 = default (30s) + MaxZipSize int // bytes, 0 = default (50MB) + MaxResponseSize int // bytes, 0 = default (2MB) } // RegistryManager coordinates multiple skill registries. diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index be52f4ca5..b6284c8f6 100644 --- a/pkg/tools/skills_install.go +++ b/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{} - slugLock.Lock() - defer slugLock.Unlock() + t.mu.Lock() + defer t.mu.Unlock() // Validate slug slug, _ := args["slug"].(string) diff --git a/pkg/utils/zip.go b/pkg/utils/zip.go index 501c8cdbc..1b4807c6e 100644 --- a/pkg/utils/zip.go +++ b/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. - if !strings.HasPrefix(filepath.Clean(destPath), filepath.Clean(targetDir)) { + // Double-check the resolved path is within target directory (defense-in-depth). + 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 { - _ = outFile.Close() + // Streamed size check: prevent overruns and malicious/corrupt headers. + 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 err := outFile.Close(); err != nil { + if written > maxFileSize { _ = 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