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
This commit is contained in:
Alex P 2025-10-23 16:06:57 +03:00
parent 15963d39ef
commit 587f8b5e4c
3 changed files with 35 additions and 11 deletions

View File

@ -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

View File

@ -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
}
}

View File

@ -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 {