fix: resolve critical concurrency and safety issues in session management

- Fix panic recovery in AddSession to log instead of re-throwing, preventing process crashes
- Fix integer overflow in trust score calculation by capping before int conversion
- Fix TOCTOU race condition in nickname uniqueness check with atomic UpdateSessionNickname method
This commit is contained in:
Alex P 2025-10-23 10:47:37 +03:00
parent 2e4a49feb6
commit 1b007b76d7
2 changed files with 37 additions and 12 deletions

View File

@ -143,16 +143,10 @@ func handleUpdateSessionNicknameRPC(params map[string]any, session *Session) (an
return nil, errors.New("permission denied: can only update own nickname") return nil, errors.New("permission denied: can only update own nickname")
} }
// Check nickname uniqueness if err := sessionManager.UpdateSessionNickname(sessionID, nickname); err != nil {
allSessions := sessionManager.GetAllSessions() return nil, err
for _, existingSession := range allSessions {
if existingSession.ID != sessionID && existingSession.Nickname == nickname {
return nil, fmt.Errorf("nickname '%s' is already in use by another session", nickname)
}
} }
targetSession.Nickname = nickname
// If session is pending and approval is required, send the approval request now that we have a nickname // If session is pending and approval is required, send the approval request now that we have a nickname
if targetSession.Mode == SessionModePending && currentSessionSettings != nil && currentSessionSettings.RequireApproval { if targetSession.Mode == SessionModePending && currentSessionSettings != nil && currentSessionSettings.RequireApproval {
if primary := sessionManager.GetPrimarySession(); primary != nil { if primary := sessionManager.GetPrimarySession(); primary != nil {

View File

@ -195,7 +195,7 @@ func (sm *SessionManager) AddSession(session *Session, clientSettings *SessionSe
delete(sm.nicknameIndex, session.Nickname) delete(sm.nicknameIndex, session.Nickname)
} }
} }
panic(r) sm.logger.Error().Interface("panic", r).Str("sessionID", session.ID).Msg("Recovered from panic in AddSession")
} }
}() }()
@ -987,6 +987,35 @@ func (sm *SessionManager) UpdateLastActive(sessionID string) {
sm.mu.Unlock() sm.mu.Unlock()
} }
// UpdateSessionNickname atomically updates a session's nickname with uniqueness check
func (sm *SessionManager) UpdateSessionNickname(sessionID, nickname string) error {
sm.mu.Lock()
defer sm.mu.Unlock()
targetSession, exists := sm.sessions[sessionID]
if !exists {
return errors.New("session not found")
}
// Check nickname uniqueness under lock
if existingSession, nicknameInUse := sm.nicknameIndex[nickname]; nicknameInUse {
if existingSession.ID != sessionID {
return fmt.Errorf("nickname '%s' is already in use by another session", nickname)
}
}
// Remove old nickname from index
if targetSession.Nickname != "" {
delete(sm.nicknameIndex, targetSession.Nickname)
}
// Update nickname and index atomically
targetSession.Nickname = nickname
sm.nicknameIndex[nickname] = targetSession
return nil
}
// Internal helper methods // Internal helper methods
// validateSinglePrimary ensures there's only one primary session and fixes any inconsistencies // validateSinglePrimary ensures there's only one primary session and fixes any inconsistencies
@ -1364,9 +1393,11 @@ func (sm *SessionManager) getSessionTrustScore(sessionID string) int {
// Longer session duration = more trust (up to 100 points for 100+ minutes) // Longer session duration = more trust (up to 100 points for 100+ minutes)
sessionAge := now.Sub(session.CreatedAt) sessionAge := now.Sub(session.CreatedAt)
score += int(sessionAge.Minutes()) sessionAgeMinutes := sessionAge.Minutes()
if score > 100 { if sessionAgeMinutes > 100 {
score = 100 // Cap age bonus at 100 points score += 100
} else {
score += int(sessionAgeMinutes)
} }
// Recently successful primary sessions get higher trust // Recently successful primary sessions get higher trust