diff --git a/pkg/channels/pico/pico.go b/pkg/channels/pico/pico.go index 8d8b62a67..7247f9db2 100644 --- a/pkg/channels/pico/pico.go +++ b/pkg/channels/pico/pico.go @@ -251,7 +251,14 @@ func (c *PicoChannel) handleWebSocket(w http.ResponseWriter, r *http.Request) { return } - conn, err := c.upgrader.Upgrade(w, r, nil) + // If the client authenticated via a subprotocol (e.g. "token.xxx"), echo + // it back in the upgrade response so the browser accepts the connection. + var responseHeader http.Header + if proto := c.matchedSubprotocol(r); proto != "" { + responseHeader = http.Header{"Sec-WebSocket-Protocol": {proto}} + } + + conn, err := c.upgrader.Upgrade(w, r, responseHeader) if err != nil { logger.ErrorCF("pico", "WebSocket upgrade failed", map[string]any{ "error": err.Error(), @@ -282,8 +289,11 @@ func (c *PicoChannel) handleWebSocket(w http.ResponseWriter, r *http.Request) { go c.readLoop(pc) } -// authenticate checks the Bearer token from the Authorization header. -// Query parameter authentication is only allowed when AllowTokenQuery is explicitly enabled. +// authenticate checks for a valid token in the following order: +// 1. Authorization: Bearer header (preferred) +// 2. Sec-WebSocket-Protocol subprotocol with prefix "token." — this lets +// browser-based clients pass the token without putting it in the URL +// 3. Query parameter "token" — only when AllowTokenQuery is explicitly on func (c *PicoChannel) authenticate(r *http.Request) bool { token := c.config.Token if token == "" { @@ -298,6 +308,11 @@ func (c *PicoChannel) authenticate(r *http.Request) bool { } } + // Check Sec-WebSocket-Protocol subprotocol ("token.") + if c.matchedSubprotocol(r) != "" { + return true + } + // Check query parameter only when explicitly allowed if c.config.AllowTokenQuery { if r.URL.Query().Get("token") == token { @@ -308,6 +323,18 @@ func (c *PicoChannel) authenticate(r *http.Request) bool { return false } +// matchedSubprotocol returns the first Sec-WebSocket-Protocol value that +// carries a valid token (format: "token."), or "" if none match. +func (c *PicoChannel) matchedSubprotocol(r *http.Request) string { + token := c.config.Token + for _, proto := range websocket.Subprotocols(r) { + if after, ok := strings.CutPrefix(proto, "token."); ok && after == token { + return proto + } + } + return "" +} + // readLoop reads messages from a WebSocket connection. func (c *PicoChannel) readLoop(pc *picoConn) { defer func() { diff --git a/web/backend/api/pico.go b/web/backend/api/pico.go index a4590dcde..b6a1c3cd9 100644 --- a/web/backend/api/pico.go +++ b/web/backend/api/pico.go @@ -65,8 +65,23 @@ func (h *Handler) handleRegenPicoToken(w http.ResponseWriter, r *http.Request) { }) } +// defaultPicoOrigins is a restricted set of localhost origins used when the +// user hasn't configured any explicit allow_origins. This covers the common +// local-dev scenarios (Vite on 5173, launcher on 18800) without opening the +// WebSocket to arbitrary cross-origin pages. +var defaultPicoOrigins = []string{ + "http://localhost:5173", + "http://127.0.0.1:5173", + "http://localhost:18800", + "http://127.0.0.1:18800", +} + // ensurePicoChannel checks if the Pico Channel is properly configured and -// enables it with sensible defaults if not. Returns true if config was changed. +// enables it with minimal secure defaults if not. Returns true if config was changed. +// +// Setup only enables the channel and creates a token. It deliberately does not +// turn on allow_token_query (prefer header-based auth) or set wildcard origins +// (prefer a restricted localhost allowlist). func (h *Handler) ensurePicoChannel() (bool, error) { cfg, err := config.LoadConfig(h.configPath) if err != nil { @@ -85,14 +100,10 @@ func (h *Handler) ensurePicoChannel() (bool, error) { changed = true } - if !cfg.Channels.Pico.AllowTokenQuery { - cfg.Channels.Pico.AllowTokenQuery = true - changed = true - } - - // Make sure origins are allowed (frontend might be running on a different port like 5173 during dev) + // Only populate origins when the user hasn't configured any. Use a + // restricted localhost allowlist instead of "*" to limit the attack surface. if len(cfg.Channels.Pico.AllowOrigins) == 0 { - cfg.Channels.Pico.AllowOrigins = []string{"*"} + cfg.Channels.Pico.AllowOrigins = defaultPicoOrigins changed = true } diff --git a/web/backend/api/pico_test.go b/web/backend/api/pico_test.go new file mode 100644 index 000000000..d0deebe89 --- /dev/null +++ b/web/backend/api/pico_test.go @@ -0,0 +1,176 @@ +package api + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + + "github.com/sipeed/picoclaw/pkg/config" +) + +func TestEnsurePicoChannel_FreshConfig(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + h := NewHandler(configPath) + + changed, err := h.ensurePicoChannel() + if err != nil { + t.Fatalf("ensurePicoChannel() error = %v", err) + } + if !changed { + t.Fatal("ensurePicoChannel() should report changed on a fresh config") + } + + 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") + } +} + +func TestEnsurePicoChannel_DoesNotEnableTokenQuery(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + h := NewHandler(configPath) + + if _, err := h.ensurePicoChannel(); err != nil { + t.Fatalf("ensurePicoChannel() error = %v", err) + } + + cfg, err := config.LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + + if cfg.Channels.Pico.AllowTokenQuery { + t.Error("setup must not enable allow_token_query by default") + } +} + +func TestEnsurePicoChannel_DoesNotSetWildcardOrigins(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + h := NewHandler(configPath) + + if _, err := h.ensurePicoChannel(); err != nil { + t.Fatalf("ensurePicoChannel() error = %v", err) + } + + cfg, err := config.LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + + for _, origin := range cfg.Channels.Pico.AllowOrigins { + if origin == "*" { + t.Error("setup must not set wildcard origin '*'") + } + } + + if len(cfg.Channels.Pico.AllowOrigins) == 0 { + t.Error("expected default localhost origins to be populated") + } +} + +func TestEnsurePicoChannel_PreservesUserSettings(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + + // Pre-configure with custom user settings + cfg := config.DefaultConfig() + cfg.Channels.Pico.Enabled = true + cfg.Channels.Pico.Token = "user-custom-token" + cfg.Channels.Pico.AllowTokenQuery = true + cfg.Channels.Pico.AllowOrigins = []string{"https://myapp.example.com"} + if err := config.SaveConfig(configPath, cfg); err != nil { + t.Fatalf("SaveConfig() error = %v", err) + } + + h := NewHandler(configPath) + + changed, err := h.ensurePicoChannel() + if err != nil { + t.Fatalf("ensurePicoChannel() error = %v", err) + } + if changed { + t.Error("ensurePicoChannel() should not change a fully configured config") + } + + cfg, err = config.LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + + if cfg.Channels.Pico.Token != "user-custom-token" { + t.Errorf("token = %q, want %q", cfg.Channels.Pico.Token, "user-custom-token") + } + if !cfg.Channels.Pico.AllowTokenQuery { + t.Error("user's allow_token_query=true must be preserved") + } + if len(cfg.Channels.Pico.AllowOrigins) != 1 || cfg.Channels.Pico.AllowOrigins[0] != "https://myapp.example.com" { + t.Errorf("allow_origins = %v, want [https://myapp.example.com]", cfg.Channels.Pico.AllowOrigins) + } +} + +func TestEnsurePicoChannel_Idempotent(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + h := NewHandler(configPath) + + // First call sets things up + if _, err := h.ensurePicoChannel(); err != nil { + t.Fatalf("first ensurePicoChannel() error = %v", err) + } + + cfg1, _ := config.LoadConfig(configPath) + token1 := cfg1.Channels.Pico.Token + + // Second call should be a no-op + changed, err := h.ensurePicoChannel() + if err != nil { + t.Fatalf("second ensurePicoChannel() error = %v", err) + } + if changed { + t.Error("second ensurePicoChannel() should not report changed") + } + + cfg2, _ := config.LoadConfig(configPath) + if cfg2.Channels.Pico.Token != token1 { + t.Error("token should not change on subsequent calls") + } +} + +func TestHandlePicoSetup_Response(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + h := NewHandler(configPath) + + req := httptest.NewRequest("POST", "/api/pico/setup", nil) + rec := httptest.NewRecorder() + + h.handlePicoSetup(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK) + } + + var resp map[string]any + if err := json.NewDecoder(rec.Body).Decode(&resp); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + + if resp["token"] == nil || resp["token"] == "" { + t.Error("response should contain a non-empty token") + } + if resp["ws_url"] == nil || resp["ws_url"] == "" { + t.Error("response should contain ws_url") + } + if resp["enabled"] != true { + t.Error("response should have enabled=true") + } + if resp["changed"] != true { + t.Error("response should have changed=true on first setup") + } +} diff --git a/web/frontend/src/lib/pico-chat-controller.ts b/web/frontend/src/lib/pico-chat-controller.ts index be3397bae..27efdcc59 100644 --- a/web/frontend/src/lib/pico-chat-controller.ts +++ b/web/frontend/src/lib/pico-chat-controller.ts @@ -165,8 +165,10 @@ export async function connectChat() { console.warn("Could not parse ws_url:", error) } - const url = `${finalWsUrl}?token=${encodeURIComponent(token)}&session_id=${encodeURIComponent(activeSessionIdRef)}` - const socket = new WebSocket(url) + const url = `${finalWsUrl}?session_id=${encodeURIComponent(activeSessionIdRef)}` + // Pass the token via the Sec-WebSocket-Protocol header instead of a query + // parameter to avoid leaking it in logs, browser history, and proxies. + const socket = new WebSocket(url, [`token.${token}`]) if (generation !== connectionGeneration) { socket.close()