From 60f3b7419f913f79411a35c281dd0a721798602e Mon Sep 17 00:00:00 2001 From: stevef Date: Tue, 24 Mar 2026 11:27:04 +0100 Subject: [PATCH] azure skills whitelisting: fix skills loader, security config, and tests --- Makefile | 2 +- cmd/picoclaw/internal/skills/command.go | 2 +- pkg/agent/context.go | 2 +- pkg/agent/loop.go | 18 ++++++++++++++-- pkg/config/config.go | 4 ++-- pkg/config/security_integration_test.go | 3 ++- pkg/gateway/gateway.go | 1 - pkg/health/server.go | 1 + pkg/providers/factory_provider.go | 1 - pkg/providers/http_provider.go | 1 - pkg/providers/openai_compat/provider.go | 12 +++++------ pkg/skills/loader.go | 28 +++++++++++++++---------- pkg/skills/loader_test.go | 1 + pkg/tools/skills_install.go | 15 ++++++++----- pkg/tools/skills_install_test.go | 1 + pkg/tools/skills_search.go | 8 +++++-- web/Makefile | 6 +++++- web/backend/api/models.go | 22 ++++++++++++------- web/backend/api/skills.go | 2 ++ 19 files changed, 86 insertions(+), 44 deletions(-) diff --git a/Makefile b/Makefile index 9581fa633..b7662b560 100644 --- a/Makefile +++ b/Makefile @@ -254,7 +254,7 @@ test: generate ## fmt: Format Go code fmt: - @$(GOLANGCI_LINT) fmt + @gofmt -s -w $$(find . -name "*.go" -not -path "./web/*" -not -path "./vendor/*") ## lint: Run linters lint: diff --git a/cmd/picoclaw/internal/skills/command.go b/cmd/picoclaw/internal/skills/command.go index e8b884977..4df257140 100644 --- a/cmd/picoclaw/internal/skills/command.go +++ b/cmd/picoclaw/internal/skills/command.go @@ -43,7 +43,7 @@ func NewSkillsCommand() *cobra.Command { globalDir := filepath.Dir(internal.GetConfigPath()) globalSkillsDir := filepath.Join(globalDir, "skills") builtinSkillsDir := filepath.Join(globalDir, "picoclaw", "skills") - d.skillsLoader = skills.NewSkillsLoader(d.workspace, globalSkillsDir, builtinSkillsDir) + d.skillsLoader = skills.NewSkillsLoader(d.workspace, globalSkillsDir, builtinSkillsDir, nil, false) return nil }, diff --git a/pkg/agent/context.go b/pkg/agent/context.go index c3fcc9fff..033bd8327 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -81,7 +81,7 @@ func NewContextBuilder(workspace string) *ContextBuilder { return &ContextBuilder{ workspace: workspace, - skillsLoader: skills.NewSkillsLoader(workspace, globalSkillsDir, builtinSkillsDir), + skillsLoader: skills.NewSkillsLoader(workspace, globalSkillsDir, builtinSkillsDir, nil, false), memory: NewMemoryStore(workspace), } } diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 5357d26df..1b5b6f360 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -272,11 +272,25 @@ func registerSharedTools( cfg.Tools.Skills.SearchCache.MaxSize, time.Duration(cfg.Tools.Skills.SearchCache.TTLSeconds)*time.Second, ) - agent.Tools.Register(tools.NewFindSkillsTool(registryMgr, searchCache, cfg.Tools.Skills.Whitelist, cfg.Tools.Skills.WhitelistEnabled)) + agent.Tools.Register( + tools.NewFindSkillsTool( + registryMgr, + searchCache, + cfg.Tools.Skills.Whitelist, + cfg.Tools.Skills.WhitelistEnabled, + ), + ) } if install_skills_enable { - agent.Tools.Register(tools.NewInstallSkillTool(registryMgr, agent.Workspace, cfg.Tools.Skills.Whitelist, cfg.Tools.Skills.WhitelistEnabled)) + agent.Tools.Register( + tools.NewInstallSkillTool( + registryMgr, + agent.Workspace, + cfg.Tools.Skills.Whitelist, + cfg.Tools.Skills.WhitelistEnabled, + ), + ) } } diff --git a/pkg/config/config.go b/pkg/config/config.go index 27acdfe71..6744a4be0 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -706,7 +706,7 @@ func (c *ModelConfig) UnmarshalJSON(data []byte) error { return err } - c.apiKeys = MergeAPIKeys(aux.APIKey, aux.APIKeys) + c.APIKeys = SimpleSecureStrings(MergeAPIKeys(aux.APIKey, aux.APIKeys)...) return nil } @@ -938,7 +938,7 @@ type ToolsConfig struct { MediaCleanup MediaCleanupConfig `json:"media_cleanup" yaml:"-"` Whitelist FlexibleStringSlice `json:"whitelist,omitempty" yaml:"-" env:"PICOCLAW_TOOLS_WHITELIST"` WhitelistEnabled bool `json:"whitelist_enabled,omitempty" yaml:"-" env:"PICOCLAW_TOOLS_WHITELIST_ENABLED"` - MCP MCPConfig `json:"mcp" yaml:"-""` + MCP MCPConfig `json:"mcp" yaml:"-"` AppendFile ToolConfig `json:"append_file" yaml:"-" envPrefix:"PICOCLAW_TOOLS_APPEND_FILE_"` EditFile ToolConfig `json:"edit_file" yaml:"-" envPrefix:"PICOCLAW_TOOLS_EDIT_FILE_"` FindSkills ToolConfig `json:"find_skills" yaml:"-" envPrefix:"PICOCLAW_TOOLS_FIND_SKILLS_"` diff --git a/pkg/config/security_integration_test.go b/pkg/config/security_integration_test.go index 24170f84b..287bd9e68 100644 --- a/pkg/config/security_integration_test.go +++ b/pkg/config/security_integration_test.go @@ -34,8 +34,9 @@ func TestJSONUnmarshalPrivateFields(t *testing.T) { if s.PublicField != "pub" { t.Errorf("PublicField = %q, want 'pub'", s.PublicField) } + // Private fields cannot be unmarshaled from JSON if s.privateField != "" { - t.Errorf("privateField = %q, want empty because unexported fields are ignored", s.privateField) + t.Errorf("privateField = %q, want empty string (private fields are not unmarshaled)", s.privateField) } } diff --git a/pkg/gateway/gateway.go b/pkg/gateway/gateway.go index bbbb2f149..640aa81b5 100644 --- a/pkg/gateway/gateway.go +++ b/pkg/gateway/gateway.go @@ -168,7 +168,6 @@ func Run(debug bool, homePath, configPath string, allowEmptyStartup bool) error }) } - fmt.Printf("✓ Gateway started on %s:%d\n", cfg.Gateway.Host, cfg.Gateway.Port) fmt.Println("Press Ctrl+C to stop") diff --git a/pkg/health/server.go b/pkg/health/server.go index f3f941bdc..5262dbb1d 100644 --- a/pkg/health/server.go +++ b/pkg/health/server.go @@ -9,6 +9,7 @@ import ( "os" "sync" "time" + "github.com/sipeed/picoclaw/pkg/logger" ) diff --git a/pkg/providers/factory_provider.go b/pkg/providers/factory_provider.go index 113ac5de8..0bcc08630 100644 --- a/pkg/providers/factory_provider.go +++ b/pkg/providers/factory_provider.go @@ -206,7 +206,6 @@ func CreateProviderFromConfig(cfg *config.ModelConfig) (LLMProvider, string, err cfg.RequestTimeout, ), modelID, nil - case "minimax": // Minimax requires reasoning_split: true in the request body if cfg.APIKey() == "" && cfg.APIBase == "" { diff --git a/pkg/providers/http_provider.go b/pkg/providers/http_provider.go index 4ed78f860..444499c91 100644 --- a/pkg/providers/http_provider.go +++ b/pkg/providers/http_provider.go @@ -90,4 +90,3 @@ func (p *HTTPProvider) SetUseAzureHeaders(use bool) { func (p *HTTPProvider) SupportsNativeSearch() bool { return p.delegate.SupportsNativeSearch() } - diff --git a/pkg/providers/openai_compat/provider.go b/pkg/providers/openai_compat/provider.go index 25c0310ff..682139aca 100644 --- a/pkg/providers/openai_compat/provider.go +++ b/pkg/providers/openai_compat/provider.go @@ -31,12 +31,12 @@ type ( ) type Provider struct { - apiKey string - apiBase string - maxTokensField string // Field name for max tokens (e.g., "max_completion_tokens" for o1/glm models) - httpClient *http.Client - extraBody map[string]any // Additional fields to inject into request body - useAzureHeaders bool // Use api-key header instead of Authorization: Bearer + apiKey string + apiBase string + maxTokensField string // Field name for max tokens (e.g., "max_completion_tokens" for o1/glm models) + httpClient *http.Client + extraBody map[string]any // Additional fields to inject into request body + useAzureHeaders bool // Use api-key header instead of Authorization: Bearer } type Option func(*Provider) diff --git a/pkg/skills/loader.go b/pkg/skills/loader.go index d30018e45..bdabd63b8 100644 --- a/pkg/skills/loader.go +++ b/pkg/skills/loader.go @@ -59,10 +59,10 @@ func (info SkillInfo) validate() error { } type SkillsLoader struct { - workspace string - workspaceSkills string // workspace skills (project-level) - globalSkills string // global skills (~/.picoclaw/skills) - builtinSkills string // builtin skills + workspace string + workspaceSkills string // workspace skills (project-level) + globalSkills string // global skills (~/.picoclaw/skills) + builtinSkills string // builtin skills whitelist []string whitelistEnabled bool } @@ -90,13 +90,19 @@ func (sl *SkillsLoader) SkillRoots() []string { return out } -func NewSkillsLoader(workspace string, globalSkills string, builtinSkills string, whitelist []string, whitelistEnabled bool) *SkillsLoader { +func NewSkillsLoader( + workspace string, + globalSkills string, + builtinSkills string, + whitelist []string, + whitelistEnabled bool, +) *SkillsLoader { return &SkillsLoader{ - workspace: workspace, - workspaceSkills: filepath.Join(workspace, "skills"), - globalSkills: globalSkills, // ~/.picoclaw/skills - builtinSkills: builtinSkills, - whitelist: whitelist, + workspace: workspace, + workspaceSkills: filepath.Join(workspace, "skills"), + globalSkills: globalSkills, // ~/.picoclaw/skills + builtinSkills: builtinSkills, + whitelist: whitelist, whitelistEnabled: whitelistEnabled, } } @@ -196,7 +202,7 @@ func (sl *SkillsLoader) LoadSkill(name string) (string, bool) { return sl.stripFrontmatter(string(content)), true } } -// ... + // ... // 2. then load from global skills (~/.picoclaw/skills) if sl.globalSkills != "" { diff --git a/pkg/skills/loader_test.go b/pkg/skills/loader_test.go index 69d8b99db..4d0610160 100644 --- a/pkg/skills/loader_test.go +++ b/pkg/skills/loader_test.go @@ -417,6 +417,7 @@ func TestGetSkillMetadata_IgnoresHTMLCommentBlocks(t *testing.T) { assert.Equal(t, "biomed-skill", meta.Name) assert.Equal(t, "Summarize biomedical papers.", meta.Description) } + func TestListSkillsWithWhitelist(t *testing.T) { tmp := t.TempDir() ws := filepath.Join(tmp, "workspace") diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index 77eb44655..562809803 100644 --- a/pkg/tools/skills_install.go +++ b/pkg/tools/skills_install.go @@ -26,13 +26,18 @@ type InstallSkillTool struct { // NewInstallSkillTool creates a new InstallSkillTool. // registryMgr is the shared registry manager (same instance as FindSkillsTool). // workspace is the root workspace directory; skills install to {workspace}/skills/{slug}/. -func NewInstallSkillTool(registryMgr *skills.RegistryManager, workspace string, whitelist []string, whitelistEnabled bool) *InstallSkillTool { +func NewInstallSkillTool( + registryMgr *skills.RegistryManager, + workspace string, + whitelist []string, + whitelistEnabled bool, +) *InstallSkillTool { return &InstallSkillTool{ - registryMgr: registryMgr, - workspace: workspace, - whitelist: whitelist, + registryMgr: registryMgr, + workspace: workspace, + whitelist: whitelist, whitelistEnabled: whitelistEnabled, - mu: sync.Mutex{}, + mu: sync.Mutex{}, } } diff --git a/pkg/tools/skills_install_test.go b/pkg/tools/skills_install_test.go index 4d90b7fcc..5c12f0029 100644 --- a/pkg/tools/skills_install_test.go +++ b/pkg/tools/skills_install_test.go @@ -102,6 +102,7 @@ func TestInstallSkillToolMissingRegistry(t *testing.T) { assert.True(t, result.IsError) assert.Contains(t, result.ForLLM, "invalid registry") } + func TestInstallSkillToolWhitelist(t *testing.T) { workspace := t.TempDir() rm := skills.NewRegistryManager() diff --git a/pkg/tools/skills_search.go b/pkg/tools/skills_search.go index bf5c8e8e9..f4d440bc7 100644 --- a/pkg/tools/skills_search.go +++ b/pkg/tools/skills_search.go @@ -19,7 +19,12 @@ type FindSkillsTool struct { // NewFindSkillsTool creates a new FindSkillsTool. // registryMgr is the shared registry manager (built from config in createToolRegistry). // cache is the search cache for deduplicating similar queries. -func NewFindSkillsTool(registryMgr *skills.RegistryManager, cache *skills.SearchCache, whitelist []string, enabled bool) *FindSkillsTool { +func NewFindSkillsTool( + registryMgr *skills.RegistryManager, + cache *skills.SearchCache, + whitelist []string, + enabled bool, +) *FindSkillsTool { return &FindSkillsTool{ registryMgr: registryMgr, cache: cache, @@ -98,7 +103,6 @@ func (t *FindSkillsTool) Execute(ctx context.Context, args map[string]any) *Tool results = filtered } - // Cache the results. if t.cache != nil && len(results) > 0 { t.cache.Put(query, results) diff --git a/web/Makefile b/web/Makefile index 06717f2b9..62c03a0ae 100644 --- a/web/Makefile +++ b/web/Makefile @@ -83,7 +83,11 @@ build: # Run all tests test: cd backend && ${WEB_GO} test ./... - cd frontend && pnpm lint + @if command -v pnpm >/dev/null 2>&1; then \ + cd frontend && pnpm lint; \ + else \ + echo "pnpm not found, skipping frontend linting"; \ + fi # Lint and format lint: diff --git a/web/backend/api/models.go b/web/backend/api/models.go index 38a55948b..09b46b08e 100644 --- a/web/backend/api/models.go +++ b/web/backend/api/models.go @@ -126,8 +126,12 @@ func (h *Handler) handleAddModel(w http.ResponseWriter, r *http.Request) { return } - if mc.APIKey != "" { - mc.ModelConfig.SetAPIKey(mc.APIKey) + apiKey := mc.APIKey + if apiKey == "" { + apiKey = mc.ModelConfig.APIKey() + } + if apiKey != "" { + mc.ModelConfig.SetAPIKey(apiKey) } cfg, err := config.LoadConfig(h.configPath) @@ -197,13 +201,15 @@ func (h *Handler) handleUpdateModel(w http.ResponseWriter, r *http.Request) { return } - // Preserve the existing API key when the caller omits it (empty string). - // This lets the UI update api_base / proxy without clearing the stored secret. - if mc.APIKey == "" { - mc.ModelConfig.SetAPIKey(cfg.ModelList[idx].APIKey()) - } else { - mc.ModelConfig.SetAPIKey(mc.APIKey) + apiKey := mc.APIKey + if apiKey == "" { + apiKey = mc.ModelConfig.APIKey() } + if apiKey == "" { + apiKey = cfg.ModelList[idx].APIKey() + } + mc.ModelConfig.SetAPIKey(apiKey) + // Preserve existing ExtraBody when omitted (nil), but clear it when // the frontend sends an empty object {} to indicate the field should // be removed. diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index 3c2fb57dd..a1d7f13b8 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -194,6 +194,8 @@ func newSkillsLoader(workspace string) *skills.SkillsLoader { workspace, filepath.Join(globalConfigDir(), "skills"), builtinSkillsDir(), + nil, + false, ) }