From 587f8b5e4c84b87f6c0d675b41a027c90eb0ceed Mon Sep 17 00:00:00 2001
From: Alex P <dtk077@gmail.com>
Date: Thu, 23 Oct 2025 16:06:57 +0300
Subject: [PATCH] fix: resolve critical security and stability issues

- Fix dual-primary race condition in reconnection logic by verifying actual session mode
- Fix authentication bypass in cloud registration endpoint
- Fix grace period eviction DoS by preventing loop and adding defensive checks
- Improve RPC error logging to distinguish closed channels from actual errors
---
 cloud.go           | 12 ++++++++----
 jsonrpc.go         | 10 ++++++++--
 session_manager.go | 24 +++++++++++++++++++-----
 3 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/cloud.go b/cloud.go
index 30f5b42c..9d805a4a 100644
--- a/cloud.go
+++ b/cloud.go
@@ -200,15 +200,19 @@ func handleCloudRegister(c *gin.Context) {
 	sessionID, _ := c.Cookie("sessionId")
 	authToken, _ := c.Cookie("authToken")
 
-	if sessionID != "" && authToken != "" && authToken == config.LocalAuthToken {
+	// Require authentication for this endpoint
+	if authToken == "" || authToken != config.LocalAuthToken {
+		c.JSON(401, gin.H{"error": "Authentication required"})
+		return
+	}
+
+	// Check session permissions if session exists
+	if sessionID != "" {
 		session := sessionManager.GetSession(sessionID)
 		if session != nil && !session.HasPermission(PermissionSettingsWrite) {
 			c.JSON(403, gin.H{"error": "Permission denied: settings modify permission required"})
 			return
 		}
-	} else if sessionID != "" {
-		c.JSON(401, gin.H{"error": "Authentication required"})
-		return
 	}
 
 	var req CloudRegisterRequest
diff --git a/jsonrpc.go b/jsonrpc.go
index 14e3bcfa..8ecbbe2d 100644
--- a/jsonrpc.go
+++ b/jsonrpc.go
@@ -132,8 +132,14 @@ func writeJSONRPCEvent(event string, params any, session *Session) {
 
 	err = session.RPCChannel.SendText(requestString)
 	if err != nil {
-		// Only log at debug level - closed pipe errors are expected during reconnection
-		scopedLogger.Debug().Err(err).Str("event", event).Msg("Could not send JSONRPC event (channel may be closing)")
+		// Check if it's a closed/closing error (expected during reconnection)
+		errStr := err.Error()
+		if strings.Contains(errStr, "closed") || strings.Contains(errStr, "closing") {
+			scopedLogger.Debug().Err(err).Str("event", event).Msg("Could not send JSONRPC event (channel closing)")
+		} else {
+			// Other errors (buffer full, protocol errors) should be visible
+			scopedLogger.Warn().Err(err).Str("event", event).Msg("Failed to send JSONRPC event")
+		}
 		return
 	}
 }
diff --git a/session_manager.go b/session_manager.go
index 7295483b..024f3ab1 100644
--- a/session_manager.go
+++ b/session_manager.go
@@ -251,8 +251,13 @@ func (sm *SessionManager) AddSession(session *Session, clientSettings *SessionSe
 
 		if existing.Mode == SessionModePrimary {
 			isBlacklisted := sm.isSessionBlacklisted(session.ID)
-			// SECURITY: Prevent dual-primary - only restore if no other primary exists
-			primaryExists := sm.primarySessionID != "" && sm.sessions[sm.primarySessionID] != nil
+			// SECURITY: Prevent dual-primary - check actual mode, not just existence
+			primaryExists := false
+			if sm.primarySessionID != "" {
+				if existingPrimary, ok := sm.sessions[sm.primarySessionID]; ok && existingPrimary.Mode == SessionModePrimary {
+					primaryExists = true
+				}
+			}
 			if sm.lastPrimaryID == session.ID && !isBlacklisted && !primaryExists {
 				sm.primarySessionID = session.ID
 				sm.lastPrimaryID = ""
@@ -424,8 +429,8 @@ func (sm *SessionManager) RemoveSession(sessionID string) {
 	// Only add grace period if this is NOT an intentional logout
 	if !isIntentionalLogout {
 		// Limit grace period entries to prevent memory exhaustion
-		// Evict the entry that will expire soonest (oldest expiration time)
-		for len(sm.reconnectGrace) >= maxGracePeriodEntries {
+		// Evict entries ONLY when full, and only evict one entry
+		if len(sm.reconnectGrace) >= maxGracePeriodEntries {
 			var evictID string
 			var earliestExpiration time.Time
 			for id, graceTime := range sm.reconnectGrace {
@@ -438,8 +443,15 @@ func (sm *SessionManager) RemoveSession(sessionID string) {
 			if evictID != "" {
 				delete(sm.reconnectGrace, evictID)
 				delete(sm.reconnectInfo, evictID)
+				sm.logger.Debug().
+					Str("evictedSessionID", evictID).
+					Msg("Evicted oldest grace period entry due to limit")
 			} else {
-				break
+				// Defensive: if we couldn't evict, don't add grace period
+				sm.logger.Error().
+					Int("graceCount", len(sm.reconnectGrace)).
+					Msg("Failed to evict grace period entry, skipping grace period for this session")
+				goto skipGracePeriod
 			}
 		}
 
@@ -456,6 +468,8 @@ func (sm *SessionManager) RemoveSession(sessionID string) {
 		}
 	}
 
+skipGracePeriod:
+
 	// If this was the primary session, clear primary slot and track for grace period
 	if wasPrimary {
 		if isIntentionalLogout {