From 1f9d390a6414e5dd3fad662c094c8207e058000d Mon Sep 17 00:00:00 2001 From: Kristjan Kruus Date: Mon, 23 Mar 2026 14:26:51 +0200 Subject: [PATCH 1/3] fix: apply security credentials before config validation in web handlers - Move SecurityCopyFrom() before validateConfig() in PUT and PATCH handlers - Make SecurityCopyFrom() call applySecurityConfig() to populate private fields - Add tests for config save with security-only channel tokens Without this fix, saving config via the web UI fails with 'channels.pico.token is required' (and similar for Telegram/Discord) when tokens are stored in .security.yml, because the validation ran before security credentials were copied to the config struct. --- pkg/config/config.go | 5 ++ web/backend/api/config.go | 23 ++++--- web/backend/api/config_test.go | 116 +++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 9 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 33919d9d7..b58069472 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1942,6 +1942,11 @@ func (c *Config) ValidateModelList() error { func (c *Config) SecurityCopyFrom(cfg *Config) { c.security = cfg.security + if c.security != nil { + if err := applySecurityConfig(c, c.security); err != nil { + logger.Errorf("failed to apply security config in SecurityCopyFrom: %v", err) + } + } } func MergeAPIKeys(apiKey string, apiKeys []string) []string { diff --git a/web/backend/api/config.go b/web/backend/api/config.go index 7cdfde174..fa2e91dec 100644 --- a/web/backend/api/config.go +++ b/web/backend/api/config.go @@ -54,6 +54,15 @@ func (h *Handler) handleUpdateConfig(w http.ResponseWriter, r *http.Request) { cfg.Tools.Exec.AllowRemote = config.DefaultConfig().Tools.Exec.AllowRemote } + // Load existing config and copy security credentials before validation, + // so that security-managed fields (e.g. pico token) are available. + oldCfg, err := config.LoadConfig(h.configPath) + if err != nil { + http.Error(w, fmt.Sprintf("Failed to load config: %v", err), http.StatusInternalServerError) + return + } + cfg.SecurityCopyFrom(oldCfg) + if errs := validateConfig(&cfg); len(errs) > 0 { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusBadRequest) @@ -64,13 +73,7 @@ func (h *Handler) handleUpdateConfig(w http.ResponseWriter, r *http.Request) { return } - logger.Infof("new config: %+v", cfg) - oldCfg, err := config.LoadConfig(h.configPath) - if err != nil { - http.Error(w, fmt.Sprintf("Failed to load config: %v", err), http.StatusInternalServerError) - return - } - cfg.SecurityCopyFrom(oldCfg) + logger.Infof("configuration updated successfully") if err := config.SaveConfig(h.configPath, &cfg); err != nil { http.Error(w, fmt.Sprintf("Failed to save config: %v", err), http.StatusInternalServerError) @@ -149,6 +152,10 @@ func (h *Handler) handlePatchConfig(w http.ResponseWriter, r *http.Request) { return } + // Copy security credentials before validation so security-managed + // fields (e.g. pico token) are available for validation checks. + newCfg.SecurityCopyFrom(cfg) + if errs := validateConfig(&newCfg); len(errs) > 0 { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusBadRequest) @@ -159,8 +166,6 @@ func (h *Handler) handlePatchConfig(w http.ResponseWriter, r *http.Request) { return } - newCfg.SecurityCopyFrom(cfg) - if err := config.SaveConfig(h.configPath, &newCfg); err != nil { http.Error(w, fmt.Sprintf("Failed to save config: %v", err), http.StatusInternalServerError) return diff --git a/web/backend/api/config_test.go b/web/backend/api/config_test.go index bbf285e14..cf8cd505e 100644 --- a/web/backend/api/config_test.go +++ b/web/backend/api/config_test.go @@ -4,6 +4,8 @@ import ( "bytes" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/sipeed/picoclaw/pkg/config" @@ -141,6 +143,120 @@ func TestHandlePatchConfig_AllowsInvalidExecRegexPatternsWhenExecDisabled(t *tes } } +// setupPicoEnabledEnv creates a test environment with Pico channel enabled and +// its token stored only in .security.yml (not in the JSON payload). +func setupPicoEnabledEnv(t *testing.T) (string, func()) { + t.Helper() + + tmp := t.TempDir() + oldHome := os.Getenv("HOME") + oldPicoHome := os.Getenv("PICOCLAW_HOME") + + if err := os.Setenv("HOME", tmp); err != nil { + t.Fatalf("set HOME: %v", err) + } + if err := os.Setenv("PICOCLAW_HOME", filepath.Join(tmp, ".picoclaw")); err != nil { + t.Fatalf("set PICOCLAW_HOME: %v", err) + } + + cfg := config.DefaultConfig() + cfg.ModelList = []*config.ModelConfig{{ + ModelName: "custom-default", + Model: "openai/gpt-4o", + }} + cfg.Agents.Defaults.ModelName = "custom-default" + cfg.Channels.Pico.Enabled = true + cfg.WithSecurity(&config.SecurityConfig{ + ModelList: map[string]config.ModelSecurityEntry{ + "custom-default": {APIKeys: []string{"sk-default"}}, + }, + Channels: config.ChannelsSecurity{ + Pico: &config.PicoSecurity{Token: "test-pico-token"}, + }, + }) + + configPath := filepath.Join(tmp, "config.json") + if err := config.SaveConfig(configPath, cfg); err != nil { + t.Fatalf("SaveConfig error: %v", err) + } + + cleanup := func() { + _ = os.Setenv("HOME", oldHome) + if oldPicoHome == "" { + _ = os.Unsetenv("PICOCLAW_HOME") + } else { + _ = os.Setenv("PICOCLAW_HOME", oldPicoHome) + } + } + return configPath, cleanup +} + +func TestHandleUpdateConfig_SucceedsWhenPicoTokenInSecurityOnly(t *testing.T) { + configPath, cleanup := setupPicoEnabledEnv(t) + defer cleanup() + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + // PUT request with pico enabled but no token in JSON — token is in .security.yml + req := httptest.NewRequest(http.MethodPut, "/api/config", bytes.NewBufferString(`{ + "version": 1, + "agents": { + "defaults": { + "workspace": "~/.picoclaw/workspace", + "model_name": "custom-default" + } + }, + "channels": { + "pico": { + "enabled": true, + "ping_interval": 30, + "read_timeout": 60, + "write_timeout": 10, + "max_connections": 100 + } + }, + "model_list": [ + { + "model_name": "custom-default", + "model": "openai/gpt-4o", + "api_keys": ["sk-default"] + } + ] + }`)) + req.Header.Set("Content-Type", "application/json") + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("PUT /api/config status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } +} + +func TestHandlePatchConfig_SucceedsWhenPicoTokenInSecurityOnly(t *testing.T) { + configPath, cleanup := setupPicoEnabledEnv(t) + defer cleanup() + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + // PATCH request changing an unrelated field — pico token still in .security.yml + req := httptest.NewRequest(http.MethodPatch, "/api/config", bytes.NewBufferString(`{ + "gateway": { + "log_level": "info" + } + }`)) + req.Header.Set("Content-Type", "application/json") + + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("PATCH /api/config status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } +} + func TestHandlePatchConfig_AllowsInvalidDenyRegexPatternsWhenDenyPatternsDisabled(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup() From d921bbb66727519d769df707f67817b6b3579c43 Mon Sep 17 00:00:00 2001 From: Cytown Date: Tue, 24 Mar 2026 16:24:12 +0800 Subject: [PATCH 2/3] bug fix for security initial cause can't save model in launcher (#1952) --- pkg/config/config.go | 1 + pkg/config/security.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index f0d9aa580..a943fb2eb 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1946,6 +1946,7 @@ func SaveConfig(path string, cfg *Config) error { if err != nil { return err } + logger.Infof("saving config to %s", path) return fileutil.WriteFileAtomic(path, data, 0o600) } diff --git a/pkg/config/security.go b/pkg/config/security.go index 816d465c7..1fda89bf0 100644 --- a/pkg/config/security.go +++ b/pkg/config/security.go @@ -31,7 +31,7 @@ type SecurityConfig struct { // Model API keys. Map key is model_name, can include suffix like "abc:0", "abc:1" // for load balancing with same model_name. The suffix ":N" is used to distinguish // multiple configs that share the same base model_name. - ModelList map[string]ModelSecurityEntry `yaml:"model_list,omitempty"` + ModelList map[string]ModelSecurityEntry `yaml:"model_list"` // Channel tokens/secrets Channels *ChannelsSecurity `yaml:"channels,omitempty"` From d23c24ce72977f3c87072813bde412ee7e8b9821 Mon Sep 17 00:00:00 2001 From: wenjie Date: Tue, 24 Mar 2026 17:03:28 +0800 Subject: [PATCH 3/3] fix(config): normalize empty security config before save/load (#1956) Normalize missing security sections when attaching, loading, and saving security config so existing config files without `.security.yml` can still be updated safely. This fixes Pico channel setup for legacy/existing configs and adds coverage for the missing security file path and unexported JSON field behavior. --- pkg/config/config.go | 2 ++ pkg/config/security.go | 23 +++++++++++++-- pkg/config/security_integration_test.go | 10 +++---- pkg/config/security_test.go | 3 ++ web/backend/api/config_test.go | 2 +- web/backend/api/pico_test.go | 39 +++++++++++++++++++++++++ 6 files changed, 70 insertions(+), 9 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 00f587159..8073dc723 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -106,6 +106,7 @@ func (c *Config) WithSecurity(sec *SecurityConfig) *Config { c.security = sec return c } + sec = normalizeSecurityConfig(sec) err := applySecurityConfig(c, sec) if err != nil { return nil @@ -1768,6 +1769,7 @@ func SaveConfig(path string, cfg *Config) error { logger.ErrorC("config", "security is nil") return fmt.Errorf("security is nil") } + cfg.security = normalizeSecurityConfig(cfg.security) // Ensure version is always set when saving if cfg.Version == 0 { cfg.Version = CurrentVersion diff --git a/pkg/config/security.go b/pkg/config/security.go index 1fda89bf0..5c71bf8c3 100644 --- a/pkg/config/security.go +++ b/pkg/config/security.go @@ -25,6 +25,25 @@ const ( SecurityConfigFile = ".security.yml" ) +func normalizeSecurityConfig(sec *SecurityConfig) *SecurityConfig { + if sec == nil { + sec = &SecurityConfig{} + } + if sec.ModelList == nil { + sec.ModelList = map[string]ModelSecurityEntry{} + } + if sec.Channels == nil { + sec.Channels = &ChannelsSecurity{} + } + if sec.Web == nil { + sec.Web = &WebToolsSecurity{} + } + if sec.Skills == nil { + sec.Skills = &SkillsSecurity{} + } + return sec +} + // SecurityConfig stores all sensitive data (API keys, tokens, secrets, passwords) // This data is loaded from security.yml and kept separate from the main config type SecurityConfig struct { @@ -191,7 +210,7 @@ func loadSecurityConfig(securityPath string) (*SecurityConfig, error) { data, err := os.ReadFile(securityPath) if err != nil { if os.IsNotExist(err) { - return &SecurityConfig{}, nil + return normalizeSecurityConfig(nil), nil } return nil, fmt.Errorf("failed to read security config: %w", err) } @@ -210,7 +229,7 @@ func loadSecurityConfig(securityPath string) (*SecurityConfig, error) { return nil, err } - return &sec, nil + return normalizeSecurityConfig(&sec), nil } // saveSecurityConfig saves the security configuration to security.yml diff --git a/pkg/config/security_integration_test.go b/pkg/config/security_integration_test.go index c1e1a2340..218914590 100644 --- a/pkg/config/security_integration_test.go +++ b/pkg/config/security_integration_test.go @@ -17,13 +17,12 @@ import ( // Test JSON unmarshal of private fields func TestJSONUnmarshalPrivateFields(t *testing.T) { - //nolint: govet type testStruct struct { PublicField string `json:"public"` - privateField string `json:"private"` + privateField string } - data := `{"public": "pub", "private": "priv"}` + data := `{"public": "pub", "privateField": "priv"}` var s testStruct if err := json.Unmarshal([]byte(data), &s); err != nil { t.Fatalf("JSON unmarshal failed: %v", err) @@ -35,9 +34,8 @@ func TestJSONUnmarshalPrivateFields(t *testing.T) { if s.PublicField != "pub" { t.Errorf("PublicField = %q, want 'pub'", s.PublicField) } - // This should fail because privateField is unexported - if s.privateField != "priv" { - t.Logf("privateField = %q, want 'priv' - THIS IS EXPECTED TO FAIL", s.privateField) + if s.privateField != "" { + t.Errorf("privateField = %q, want empty because unexported fields are ignored", s.privateField) } } diff --git a/pkg/config/security_test.go b/pkg/config/security_test.go index af08a67db..0f260ed59 100644 --- a/pkg/config/security_test.go +++ b/pkg/config/security_test.go @@ -20,6 +20,9 @@ func TestSecurityConfig(t *testing.T) { require.NoError(t, err) assert.NotNil(t, sec) assert.Empty(t, sec.ModelList) + assert.NotNil(t, sec.Channels) + assert.NotNil(t, sec.Web) + assert.NotNil(t, sec.Skills) }) } diff --git a/web/backend/api/config_test.go b/web/backend/api/config_test.go index cf8cd505e..9b05546f9 100644 --- a/web/backend/api/config_test.go +++ b/web/backend/api/config_test.go @@ -170,7 +170,7 @@ func setupPicoEnabledEnv(t *testing.T) (string, func()) { ModelList: map[string]config.ModelSecurityEntry{ "custom-default": {APIKeys: []string{"sk-default"}}, }, - Channels: config.ChannelsSecurity{ + Channels: &config.ChannelsSecurity{ Pico: &config.PicoSecurity{Token: "test-pico-token"}, }, }) diff --git a/web/backend/api/pico_test.go b/web/backend/api/pico_test.go index 263253cb2..b59878bf3 100644 --- a/web/backend/api/pico_test.go +++ b/web/backend/api/pico_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" "path/filepath" "strconv" "testing" @@ -154,6 +155,44 @@ func TestEnsurePicoChannel_PreservesUserSettings(t *testing.T) { } } +func TestEnsurePicoChannel_ExistingConfigWithoutSecurityFile(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + + cfg := config.DefaultConfig() + raw, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + if err = os.WriteFile(configPath, raw, 0o600); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + h := NewHandler(configPath) + + changed, err := h.ensurePicoChannel("") + if err != nil { + t.Fatalf("ensurePicoChannel() error = %v", err) + } + if !changed { + t.Fatal("ensurePicoChannel() should report changed when pico is missing") + } + + cfg, err = config.LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + + if !cfg.Channels.Pico.Enabled { + t.Error("expected Pico to be enabled after setup") + } + if cfg.Channels.Pico.Token() == "" { + t.Error("expected a non-empty token after setup") + } + if _, err := os.Stat(filepath.Join(filepath.Dir(configPath), config.SecurityConfigFile)); err != nil { + t.Fatalf("expected .security.yml to be created: %v", err) + } +} + func TestEnsurePicoChannel_Idempotent(t *testing.T) { configPath := filepath.Join(t.TempDir(), "config.json") h := NewHandler(configPath)