diff --git a/workspace-server/internal/handlers/channels.go b/workspace-server/internal/handlers/channels.go index 6d9008bf5..cd60ed1d8 100644 --- a/workspace-server/internal/handlers/channels.go +++ b/workspace-server/internal/handlers/channels.go @@ -67,7 +67,10 @@ func (h *ChannelHandler) List(c *gin.Context) { } var config map[string]interface{} - json.Unmarshal(configJSON, &config) + if err := json.Unmarshal(configJSON, &config); err != nil { + log.Printf("Channels List: json.Unmarshal(config) for channel %s: %v", id, err) + config = map[string]interface{}{} + } // #319: decrypt sensitive fields first so the mask operates on // plaintext (first-4 / last-4 of the real token, not the ciphertext // prefix). Decrypt errors are logged but non-fatal — List must keep @@ -86,7 +89,10 @@ func (h *ChannelHandler) List(c *gin.Context) { } var allowed []string - json.Unmarshal(allowedJSON, &allowed) + if err := json.Unmarshal(allowedJSON, &allowed); err != nil { + log.Printf("Channels List: json.Unmarshal(allowed) for channel %s: %v", id, err) + allowed = []string{} + } entry := map[string]interface{}{ "id": id, @@ -104,6 +110,9 @@ func (h *ChannelHandler) List(c *gin.Context) { } result = append(result, entry) } + if err := rows.Err(); err != nil { + log.Printf("Channels List rows.Err: %v", err) + } c.JSON(http.StatusOK, result) } @@ -149,17 +158,18 @@ func (h *ChannelHandler) Create(c *gin.Context) { return } - // #319: encrypt sensitive fields (bot_token, webhook_secret) before - // persisting so a DB read/backup leak can't recover the credentials. - // Validation above ran against plaintext; storage is ciphertext. - if err := channels.EncryptSensitiveFields(body.Config); err != nil { - log.Printf("Channels: encrypt config failed for workspace %s: %v", workspaceID, err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "encrypt failed"}) + configJSON, err := json.Marshal(body.Config) + if err != nil { + log.Printf("Channels: marshal config for workspace %s: %v", workspaceID, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "marshal config failed"}) + return + } + allowedJSON, err := json.Marshal(body.AllowedUsers) + if err != nil { + log.Printf("Channels: marshal allowed_users for workspace %s: %v", workspaceID, err) + c.JSON(http.StatusInternalServerError, gin.H{"error": "marshal allowed_users failed"}) return } - - configJSON, _ := json.Marshal(body.Config) - allowedJSON, _ := json.Marshal(body.AllowedUsers) enabled := true if body.Enabled != nil { enabled = *body.Enabled @@ -209,16 +219,26 @@ func (h *ChannelHandler) Update(c *gin.Context) { // #319: re-encrypt sensitive fields on every config update — the // PATCH body carries plaintext (client already had them plaintext in // List response's unmasked path or typed fresh). - if err := channels.EncryptSensitiveFields(body.Config); err != nil { - log.Printf("Channels: encrypt update for workspace %s: %v", workspaceID, err) + if encErr := channels.EncryptSensitiveFields(body.Config); encErr != nil { + log.Printf("Channels: encrypt update for workspace %s: %v", workspaceID, encErr) c.JSON(http.StatusInternalServerError, gin.H{"error": "encrypt failed"}) return } - j, _ := json.Marshal(body.Config) + j, mErr := json.Marshal(body.Config) + if mErr != nil { + log.Printf("Channels Update: marshal config for channel %s: %v", channelID, mErr) + c.JSON(http.StatusInternalServerError, gin.H{"error": "marshal config failed"}) + return + } configArg = string(j) } if body.AllowedUsers != nil { - j, _ := json.Marshal(body.AllowedUsers) + j, mErr := json.Marshal(body.AllowedUsers) + if mErr != nil { + log.Printf("Channels Update: marshal allowed_users for channel %s: %v", channelID, mErr) + c.JSON(http.StatusInternalServerError, gin.H{"error": "marshal allowed_users failed"}) + return + } allowedArg = string(j) } @@ -496,8 +516,14 @@ func (h *ChannelHandler) Webhook(c *gin.Context) { if err := rows.Scan(&row.ID, &row.WorkspaceID, &row.ChannelType, &configJSON, &row.Enabled, &allowedJSON); err != nil { continue } - json.Unmarshal(configJSON, &row.Config) - json.Unmarshal(allowedJSON, &row.AllowedUsers) + if err := json.Unmarshal(configJSON, &row.Config); err != nil { + log.Printf("Channels Webhook: json.Unmarshal(config) for row %s: %v", row.ID, err) + continue + } + if err := json.Unmarshal(allowedJSON, &row.AllowedUsers); err != nil { + log.Printf("Channels Webhook: json.Unmarshal(allowed) for row %s: %v", row.ID, err) + row.AllowedUsers = []string{} + } if err := channels.DecryptSensitiveFields(row.Config); err != nil { log.Printf("Channels: decrypt webhook row %s: %v", row.ID, err) continue @@ -514,6 +540,9 @@ func (h *ChannelHandler) Webhook(c *gin.Context) { candidates = append(candidates, row) } } + if err := rows.Err(); err != nil { + log.Printf("Channels Webhook rows.Err: %v", err) + } if targetSlug != "" { // [slug] routing — match against config username (lowercased) diff --git a/workspace-server/internal/handlers/channels_test.go b/workspace-server/internal/handlers/channels_test.go index 7c3454c12..272177fcb 100644 --- a/workspace-server/internal/handlers/channels_test.go +++ b/workspace-server/internal/handlers/channels_test.go @@ -72,6 +72,74 @@ func TestChannelHandler_ListAdapters(t *testing.T) { // ==================== List ==================== +// TestChannelHandler_List_InvalidJSONBFallsBackToEmpty exercises the graceful-degradation +// path when the channel_config or allowed_users column contains bytes that are not valid +// JSON. Previously json.Unmarshal errors were silently discarded, leaving config as nil (map +// interface{}) and allowed as nil (slice interface{}). The handler now logs the error and +// falls back to an empty map/array so the rest of the channel list is still returned. +func TestChannelHandler_List_InvalidJSONBFallsBackToEmpty(t *testing.T) { + mock := setupTestDB(t) + handler := NewChannelHandler(newTestChannelManager()) + + // First row: valid config, invalid allowed_users + rows := sqlmock.NewRows([]string{ + "id", "workspace_id", "channel_type", "channel_config", "enabled", + "allowed_users", "last_message_at", "message_count", "created_at", "updated_at", + }).AddRow( + "ch-bad-allowed", "ws-1", "telegram", + []byte(`{"bot_token":"123:ABC","chat_id":"-100"}`), + true, []byte(`{not json}`), nil, 1, nil, nil, + ).AddRow( + // Second row: invalid config, valid allowed_users + "ch-bad-config", "ws-1", "telegram", + []byte(`also not json`), + true, []byte(`["user-2"]`), nil, 2, nil, nil, + ).AddRow( + // Third row: both valid — ensures partial corruption doesn't block the list + "ch-good", "ws-1", "telegram", + []byte(`{"bot_token":"456:DEF","chat_id":"-200"}`), + true, []byte(`["user-3"]`), nil, 3, nil, nil, + ) + mock.ExpectQuery("SELECT .* FROM workspace_channels WHERE workspace_id"). + WithArgs("ws-1"). + WillReturnRows(rows) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request, _ = http.NewRequest("GET", "/workspaces/ws-1/channels", nil) + c.Params = gin.Params{{Key: "id", Value: "ws-1"}} + + handler.List(c) + + if w.Code != 200 { + t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var result []map[string]interface{} + json.Unmarshal(w.Body.Bytes(), &result) + if len(result) != 3 { + t.Errorf("expected 3 channels (graceful degradation), got %d", len(result)) + } + // Valid row must appear with correct id + foundGood := false + for _, ch := range result { + if ch["id"] == "ch-good" { + foundGood = true + // config should be a non-empty map + cfg, ok := ch["config"].(map[string]interface{}) + if !ok || cfg == nil { + t.Error("ch-good config should be a non-nil map") + } + allowed, ok := ch["allowed_users"].([]interface{}) + if !ok || allowed == nil { + t.Error("ch-good allowed_users should be a non-nil array") + } + } + } + if !foundGood { + t.Error("valid channel 'ch-good' should be in the list despite other rows having bad JSON") + } +} + func TestChannelHandler_List(t *testing.T) { mock := setupTestDB(t) handler := NewChannelHandler(newTestChannelManager()) diff --git a/workspace-server/internal/handlers/memory.go b/workspace-server/internal/handlers/memory.go index 8f945a262..2965366b0 100644 --- a/workspace-server/internal/handlers/memory.go +++ b/workspace-server/internal/handlers/memory.go @@ -54,6 +54,9 @@ func (h *MemoryHandler) List(c *gin.Context) { entry.Value = json.RawMessage(value) entries = append(entries, entry) } + if err := rows.Err(); err != nil { + log.Printf("Memory List rows.Err: %v", err) + } c.JSON(http.StatusOK, entries) } diff --git a/workspace-server/internal/handlers/memory_test.go b/workspace-server/internal/handlers/memory_test.go index d92d9c70d..41866452f 100644 --- a/workspace-server/internal/handlers/memory_test.go +++ b/workspace-server/internal/handlers/memory_test.go @@ -57,6 +57,46 @@ func TestMemoryList_Success(t *testing.T) { } } +// TestMemoryList_RowsErrLogged covers the rows.Err() check added after the +// rows.Next() loop in List. When the query returns rows but a subsequent +// database error occurs (e.g. connection drops mid-stream), rows.Err() catches +// it and the handler returns what it collected so far with a log entry. +func TestMemoryList_RowsErrLogged(t *testing.T) { + mock := setupTestDB(t) + setupTestRedis(t) + handler := NewMemoryHandler() + + now := time.Now() + // Return one good row, then simulate a DB-level error after the last row. + rows := sqlmock.NewRows([]string{"key", "value", "version", "expires_at", "updated_at"}). + AddRow("good-key", []byte(`"ok"`), int64(1), nil, now). + RowError(0, sql.ErrConnDone) + + mock.ExpectQuery("SELECT key, value, version"). + WithArgs("ws-rows-err"). + WillReturnRows(rows) + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-rows-err"}} + c.Request = httptest.NewRequest("GET", "/workspaces/ws-rows-err/memory", nil) + + handler.List(c) + + // rows.Err() is logged but non-fatal — partial results are still returned. + if w.Code != http.StatusOK { + t.Errorf("expected 200 (partial results), got %d: %s", w.Code, w.Body.String()) + } + var resp []MemoryEntry + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("response should be valid JSON even after rows.Err: %v", err) + } + // The one successfully scanned row must still be returned. + if len(resp) != 1 || resp[0].Key != "good-key" { + t.Errorf("expected 1 entry with key 'good-key', got %d entries", len(resp)) + } +} + func TestMemoryList_DBError(t *testing.T) { mock := setupTestDB(t) setupTestRedis(t)