Comprehensive code review and feature completion

api/internal/db/database.go

@@ -183,10 +183,10 @@
 
 	// Configure connection pool for optimal performance
 	// These settings balance performance with resource usage
-	db.SetMaxOpenConns(25)                // Maximum number of open connections to the database
-	db.SetMaxIdleConns(5)                 // Maximum number of connections in the idle connection pool
-	db.SetConnMaxLifetime(5 * 60 * 1000)  // Maximum amount of time a connection may be reused (5 minutes)
-	db.SetConnMaxIdleTime(1 * 60 * 1000)  // Maximum amount of time a connection may be idle (1 minute)
+	db.SetMaxOpenConns(25)                     // Maximum number of open connections to the database
+	db.SetMaxIdleConns(5)                      // Maximum number of connections in the idle connection pool
+	db.SetConnMaxLifetime(5 * time.Minute)     // Maximum amount of time a connection may be reused (5 minutes)
+	db.SetConnMaxIdleTime(1 * time.Minute)     // Maximum amount of time a connection may be idle (1 minute)
 
 	// Test connection
 	if err := db.Ping(); err != nil {

api/internal/db/users.go

@@ -199,6 +199,8 @@ func (u *UserDB) GetUser(ctx context.Context, userID string) (*models.User, erro
 // GetUserByUsername retrieves a user by username
 func (u *UserDB) GetUserByUsername(ctx context.Context, username string) (*models.User, error) {
 	user := &models.User{}
+	// SECURITY FIX: Select password_hash only - this method is used for authentication
+	// Note: password_hash is needed here for VerifyPassword() to work
 	query := `
 		SELECT id, username, email, full_name, role, provider, password_hash, active, created_at, updated_at, last_login
 		FROM users
@@ -223,15 +225,17 @@ func (u *UserDB) GetUserByUsername(ctx context.Context, username string) (*model
 // GetUserByEmail retrieves a user by email address
 func (u *UserDB) GetUserByEmail(ctx context.Context, email string) (*models.User, error) {
 	user := &models.User{}
+	// SECURITY FIX: Don't expose password_hash unless absolutely necessary
+	// This method may be used for user lookups where password is not needed
 	query := `
-		SELECT id, username, email, full_name, role, provider, password_hash, active, created_at, updated_at, last_login
+		SELECT id, username, email, full_name, role, provider, active, created_at, updated_at, last_login
 		FROM users
 		WHERE email = $1
 	`
 
 	err := u.db.QueryRowContext(ctx, query, email).Scan(
 		&user.ID, &user.Username, &user.Email, &user.FullName,
-		&user.Role, &user.Provider, &user.PasswordHash, &user.Active,
+		&user.Role, &user.Provider, &user.Active,
 		&user.CreatedAt, &user.UpdatedAt, &user.LastLogin,
 	)
 	if err != nil {
@@ -293,17 +297,23 @@ func (u *UserDB) ListUsers(ctx context.Context, role, provider string, activeOnl
 	users := []*models.User{}
 	for rows.Next() {
 		user := &models.User{}
+		// BUG FIX: Return error instead of continuing - fail fast on database errors
 		err := rows.Scan(
 			&user.ID, &user.Username, &user.Email, &user.FullName,
 			&user.Role, &user.Provider, &user.Active,
 			&user.CreatedAt, &user.UpdatedAt, &user.LastLogin,
 		)
 		if err != nil {
-			continue
+			return nil, fmt.Errorf("failed to scan user row: %w", err)
 		}
 		users = append(users, user)
 	}
 
+	// BUG FIX: Check rows.Err() to catch any errors that occurred during iteration
+	if err := rows.Err(); err != nil {
+		return nil, fmt.Errorf("error iterating user rows: %w", err)
+	}
+
 	return users, nil
 }
 
@@ -356,21 +366,37 @@ func (u *UserDB) UpdateUser(ctx context.Context, userID string, req *models.Upda
 
 // DeleteUser deletes a user
 func (u *UserDB) DeleteUser(ctx context.Context, userID string) error {
+	// BUG FIX: Use transaction to ensure atomicity - all deletes succeed or all fail
+	tx, err := u.db.BeginTx(ctx, nil)
+	if err != nil {
+		return fmt.Errorf("failed to begin transaction: %w", err)
+	}
+	defer tx.Rollback() // Rollback if we don't commit
+
 	// Delete quota first
-	_, err := u.db.ExecContext(ctx, "DELETE FROM user_quotas WHERE user_id = $1", userID)
+	_, err = tx.ExecContext(ctx, "DELETE FROM user_quotas WHERE user_id = $1", userID)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to delete user quotas: %w", err)
 	}
 
 	// Delete group memberships
-	_, err = u.db.ExecContext(ctx, "DELETE FROM group_memberships WHERE user_id = $1", userID)
+	_, err = tx.ExecContext(ctx, "DELETE FROM group_memberships WHERE user_id = $1", userID)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to delete group memberships: %w", err)
 	}
 
 	// Delete user
-	_, err = u.db.ExecContext(ctx, "DELETE FROM users WHERE id = $1", userID)
-	return err
+	_, err = tx.ExecContext(ctx, "DELETE FROM users WHERE id = $1", userID)
+	if err != nil {
+		return fmt.Errorf("failed to delete user: %w", err)
+	}
+
+	// Commit transaction
+	if err := tx.Commit(); err != nil {
+		return fmt.Errorf("failed to commit transaction: %w", err)
+	}
+
+	return nil
 }
 
 // UpdateLastLogin updates the user's last login timestamp

api/internal/handlers/integrations.go

@@ -42,8 +42,10 @@ package handlers
 import (
 	"bytes"
 	"crypto/hmac"
+	"crypto/rand"
 	"crypto/sha256"
 	"database/sql"
+	"encoding/base64"
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
@@ -886,8 +888,14 @@ func (h *IntegrationsHandler) validateWebhookURL(urlStr string) error {
 }
 
 func (h *IntegrationsHandler) generateWebhookSecret() string {
-	// Generate a random 32-byte secret
-	return fmt.Sprintf("whsec_%d", time.Now().UnixNano())
+	// SECURITY FIX: Use crypto/rand for secure random generation
+	// Previous implementation used timestamp which is predictable
+	b := make([]byte, 32)
+	if _, err := rand.Read(b); err != nil {
+		// Should never happen, but fail safely if it does
+		panic("failed to generate secure random secret: " + err.Error())
+	}
+	return "whsec_" + base64.URLEncoding.EncodeToString(b)
 }
 
 func (h *IntegrationsHandler) deliverWebhook(webhook Webhook, event WebhookEvent) (bool, int, string, error) {

api/internal/handlers/notifications.go

@@ -638,9 +638,12 @@
 	payloadJSON, _ := json.Marshal(payload)
 
 	// Create signature (HMAC-SHA256)
+	// SECURITY: WEBHOOK_SECRET must be set for production use
 	webhookSecret := os.Getenv("WEBHOOK_SECRET")
 	if webhookSecret == "" {
-		webhookSecret = "default-secret"
+		// CRITICAL: Never use default secrets in production
+		log.Println("ERROR: WEBHOOK_SECRET not set - webhook signatures are insecure!")
+		return fmt.Errorf("WEBHOOK_SECRET environment variable must be set for security")
 	}
 
 	mac := hmac.New(sha256.New, []byte(webhookSecret))

api/internal/websocket/hub.go

@@ -171,17 +171,30 @@ func (h *Hub) Run() {
 			h.mu.Unlock()
 
 		case message := <-h.broadcast:
+			// BUG FIX: Collect clients to close first, then modify map with write lock
+			// Using RLock while iterating, but need write lock to modify map
 			h.mu.RLock()
+			clientsToClose := make([]*Client, 0)
 			for client := range h.clients {
 				select {
 				case client.send <- message:
+					// Successfully sent
 				default:
-					// Client's send buffer is full, close it
+					// Client's send buffer is full, mark for closing
+					clientsToClose = append(clientsToClose, client)
+				}
+			}
+			h.mu.RUnlock()
+
+			// Now close and remove blocked clients with write lock
+			if len(clientsToClose) > 0 {
+				h.mu.Lock()
+				for _, client := range clientsToClose {
 					close(client.send)
 					delete(h.clients, client)
 				}
+				h.mu.Unlock()
 			}
-			h.mu.RUnlock()
 		}
 	}
 }

controller/api/v1alpha1/groupversion_info.go

@@ -1,6 +1,6 @@
 // Package v1alpha1 contains API Schema definitions for the stream v1alpha1 API group
 // +kubebuilder:object:generate=true
-// +groupName=stream.streamspace.io
+// +groupName=stream.space
 package v1alpha1
 
 import (
@@ -10,7 +10,8 @@ import (
 
 var (
 	// GroupVersion is group version used to register these objects
-	GroupVersion = schema.GroupVersion{Group: "stream.streamspace.io", Version: "v1alpha1"}
+	// IMPORTANT: Must match the API group in CRD manifests (stream.space)
+	GroupVersion = schema.GroupVersion{Group: "stream.space", Version: "v1alpha1"}
 
 	// SchemeBuilder is used to add go types to the GroupVersionKind scheme
 	SchemeBuilder = &scheme.Builder{GroupVersion: GroupVersion}