Compare commits

...

9 Commits

Author SHA1 Message Date
Alex P e7bdabb667 fix: suppress expected datachannel closure errors during logout 2025-10-23 22:48:32 +03:00
Alex P f7a5ed6d2e fix: only auto-remove disconnected pending sessions 2025-10-23 20:56:15 +03:00
Alex P 6898ede8e5 refactor: deduplicate nickname validation logic 2025-10-23 20:43:11 +03:00
Alex P d7a37b5eb3 fix: prevent timeout ping-pong loop in emergency promotions 2025-10-23 18:47:58 +03:00
Alex P 906c5cf4b7 fix: defer version check until session is approved 2025-10-23 18:45:25 +03:00
Alex P 192a470e67 fix: display error message instead of [Object object] in version info 2025-10-23 16:32:27 +03:00
Alex P 94d24e7725 fix: add missing strings import in jsonrpc.go 2025-10-23 16:25:53 +03:00
Alex P 79b5ec3e29 fix: resolve activity tracking and UI race conditions
- Remove LastActive reset on emergency promotion to preserve actual activity state
- Fix UI rejection count race by tracking whether rejection was already counted
- Optimize browser detection to avoid redundant string searches
2025-10-23 16:10:37 +03:00
Alex P 587f8b5e4c 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
2025-10-23 16:06:57 +03:00
9 changed files with 128 additions and 69 deletions

View File

@ -200,15 +200,19 @@ func handleCloudRegister(c *gin.Context) {
sessionID, _ := c.Cookie("sessionId") sessionID, _ := c.Cookie("sessionId")
authToken, _ := c.Cookie("authToken") 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) session := sessionManager.GetSession(sessionID)
if session != nil && !session.HasPermission(PermissionSettingsWrite) { if session != nil && !session.HasPermission(PermissionSettingsWrite) {
c.JSON(403, gin.H{"error": "Permission denied: settings modify permission required"}) c.JSON(403, gin.H{"error": "Permission denied: settings modify permission required"})
return return
} }
} else if sessionID != "" {
c.JSON(401, gin.H{"error": "Authentication required"})
return
} }
var req CloudRegisterRequest var req CloudRegisterRequest

View File

@ -12,6 +12,7 @@ import (
"reflect" "reflect"
"regexp" "regexp"
"strconv" "strconv"
"strings"
"sync" "sync"
"time" "time"
@ -132,8 +133,14 @@ func writeJSONRPCEvent(event string, params any, session *Session) {
err = session.RPCChannel.SendText(requestString) err = session.RPCChannel.SendText(requestString)
if err != nil { if err != nil {
// Only log at debug level - closed pipe errors are expected during reconnection // Check if it's a closed/closing error (expected during reconnection)
scopedLogger.Debug().Err(err).Str("event", event).Msg("Could not send JSONRPC event (channel may be closing)") 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 return
} }
} }

View File

@ -95,42 +95,11 @@ func handleRequestSessionApprovalRPC(session *Session) (any, error) {
return map[string]interface{}{"status": "requested"}, nil return map[string]interface{}{"status": "requested"}, nil
} }
func validateNickname(nickname string) error {
if len(nickname) < minNicknameLength {
return fmt.Errorf("nickname must be at least %d characters", minNicknameLength)
}
if len(nickname) > maxNicknameLength {
return fmt.Errorf("nickname must be %d characters or less", maxNicknameLength)
}
if !isValidNickname(nickname) {
return errors.New("nickname can only contain letters, numbers, spaces, and - _ . @")
}
for i, r := range nickname {
if r < 32 || r == 127 {
return fmt.Errorf("nickname contains control character at position %d", i)
}
if r >= 0x200B && r <= 0x200D {
return errors.New("nickname contains zero-width character")
}
}
trimmed := ""
for _, r := range nickname {
trimmed += string(r)
}
if trimmed != nickname {
return errors.New("nickname contains disallowed unicode")
}
return nil
}
func handleUpdateSessionNicknameRPC(params map[string]any, session *Session) (any, error) { func handleUpdateSessionNicknameRPC(params map[string]any, session *Session) (any, error) {
sessionID, _ := params["sessionId"].(string) sessionID, _ := params["sessionId"].(string)
nickname, _ := params["nickname"].(string) nickname, _ := params["nickname"].(string)
if err := validateNickname(nickname); err != nil { if err := sessionManager.validateNickname(nickname); err != nil {
return nil, err return nil, err
} }

View File

@ -2,6 +2,8 @@ package kvm
import ( import (
"time" "time"
"github.com/pion/webrtc/v4"
) )
// emergencyPromotionContext holds context for emergency promotion attempts // emergencyPromotionContext holds context for emergency promotion attempts
@ -216,18 +218,29 @@ func (sm *SessionManager) promoteAfterGraceExpiration(expiredSessionID string, n
} }
} }
// handlePendingSessionTimeout removes timed-out pending sessions (DoS protection) // handlePendingSessionTimeout removes timed-out pending sessions only if disconnected
// Returns true if any pending session was removed // Connected pending sessions remain visible for approval (consistent UX)
// This prevents resource leaks while maintaining good user experience
func (sm *SessionManager) handlePendingSessionTimeout(now time.Time) bool { func (sm *SessionManager) handlePendingSessionTimeout(now time.Time) bool {
toDelete := make([]string, 0) toDelete := make([]string, 0)
for id, session := range sm.sessions { for id, session := range sm.sessions {
if session.Mode == SessionModePending && if session.Mode == SessionModePending &&
now.Sub(session.CreatedAt) > defaultPendingSessionTimeout { now.Sub(session.CreatedAt) > defaultPendingSessionTimeout {
websocketLogger.Debug(). // Only remove if the connection is closed/failed
Str("sessionId", id). // This prevents resource leaks while keeping connected sessions visible
Dur("age", now.Sub(session.CreatedAt)). if session.peerConnection != nil {
Msg("Removing timed-out pending session") connectionState := session.peerConnection.ConnectionState()
toDelete = append(toDelete, id) if connectionState == webrtc.PeerConnectionStateClosed ||
connectionState == webrtc.PeerConnectionStateFailed ||
connectionState == webrtc.PeerConnectionStateDisconnected {
websocketLogger.Debug().
Str("sessionId", id).
Dur("age", now.Sub(session.CreatedAt)).
Str("connectionState", connectionState.String()).
Msg("Removing timed-out disconnected pending session")
toDelete = append(toDelete, id)
}
}
} }
} }
for _, id := range toDelete { for _, id := range toDelete {

View File

@ -173,11 +173,8 @@ func (sm *SessionManager) AddSession(session *Session, clientSettings *SessionSe
} }
if session.Nickname != "" { if session.Nickname != "" {
if len(session.Nickname) < minNicknameLength { if err := sm.validateNickname(session.Nickname); err != nil {
return fmt.Errorf("nickname must be at least %d characters", minNicknameLength) return err
}
if len(session.Nickname) > maxNicknameLength {
return fmt.Errorf("nickname must be %d characters or less", maxNicknameLength)
} }
} }
if len(session.Identity) > maxIdentityLength { if len(session.Identity) > maxIdentityLength {
@ -251,8 +248,13 @@ func (sm *SessionManager) AddSession(session *Session, clientSettings *SessionSe
if existing.Mode == SessionModePrimary { if existing.Mode == SessionModePrimary {
isBlacklisted := sm.isSessionBlacklisted(session.ID) isBlacklisted := sm.isSessionBlacklisted(session.ID)
// SECURITY: Prevent dual-primary - only restore if no other primary exists // SECURITY: Prevent dual-primary - check actual mode, not just existence
primaryExists := sm.primarySessionID != "" && sm.sessions[sm.primarySessionID] != nil 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 { if sm.lastPrimaryID == session.ID && !isBlacklisted && !primaryExists {
sm.primarySessionID = session.ID sm.primarySessionID = session.ID
sm.lastPrimaryID = "" sm.lastPrimaryID = ""
@ -424,8 +426,8 @@ func (sm *SessionManager) RemoveSession(sessionID string) {
// Only add grace period if this is NOT an intentional logout // Only add grace period if this is NOT an intentional logout
if !isIntentionalLogout { if !isIntentionalLogout {
// Limit grace period entries to prevent memory exhaustion // Limit grace period entries to prevent memory exhaustion
// Evict the entry that will expire soonest (oldest expiration time) // Evict entries ONLY when full, and only evict one entry
for len(sm.reconnectGrace) >= maxGracePeriodEntries { if len(sm.reconnectGrace) >= maxGracePeriodEntries {
var evictID string var evictID string
var earliestExpiration time.Time var earliestExpiration time.Time
for id, graceTime := range sm.reconnectGrace { for id, graceTime := range sm.reconnectGrace {
@ -438,8 +440,15 @@ func (sm *SessionManager) RemoveSession(sessionID string) {
if evictID != "" { if evictID != "" {
delete(sm.reconnectGrace, evictID) delete(sm.reconnectGrace, evictID)
delete(sm.reconnectInfo, evictID) delete(sm.reconnectInfo, evictID)
sm.logger.Debug().
Str("evictedSessionID", evictID).
Msg("Evicted oldest grace period entry due to limit")
} else { } 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 +465,8 @@ func (sm *SessionManager) RemoveSession(sessionID string) {
} }
} }
skipGracePeriod:
// If this was the primary session, clear primary slot and track for grace period // If this was the primary session, clear primary slot and track for grace period
if wasPrimary { if wasPrimary {
if isIntentionalLogout { if isIntentionalLogout {
@ -1209,11 +1220,9 @@ func (sm *SessionManager) transferPrimaryRole(fromSessionID, toSessionID, transf
return fmt.Errorf("cannot promote: another primary session exists (%s)", existingPrimaryID) return fmt.Errorf("cannot promote: another primary session exists (%s)", existingPrimaryID)
} }
// Promote target session
toSession.Mode = SessionModePrimary toSession.Mode = SessionModePrimary
toSession.hidRPCAvailable = false toSession.hidRPCAvailable = false
// Reset LastActive for all emergency promotions to prevent immediate re-timeout if transferType == "emergency_timeout_promotion" {
if strings.HasPrefix(transferType, "emergency_") {
toSession.LastActive = time.Now() toSession.LastActive = time.Now()
} }
sm.primarySessionID = toSessionID sm.primarySessionID = toSessionID
@ -1462,16 +1471,19 @@ func extractBrowserFromUserAgent(userAgent string) *string {
ua := strings.ToLower(userAgent) ua := strings.ToLower(userAgent)
// Check for common browsers (order matters - Chrome contains Safari, etc.) // Check for common browsers (order matters - Chrome contains Safari, etc.)
// Optimize Safari check by caching Chrome detection
hasChrome := strings.Contains(ua, "chrome")
if strings.Contains(ua, "edg/") || strings.Contains(ua, "edge") { if strings.Contains(ua, "edg/") || strings.Contains(ua, "edge") {
return &BrowserEdge return &BrowserEdge
} }
if strings.Contains(ua, "firefox") { if strings.Contains(ua, "firefox") {
return &BrowserFirefox return &BrowserFirefox
} }
if strings.Contains(ua, "chrome") { if hasChrome {
return &BrowserChrome return &BrowserChrome
} }
if strings.Contains(ua, "safari") && !strings.Contains(ua, "chrome") { if strings.Contains(ua, "safari") {
return &BrowserSafari return &BrowserSafari
} }
if strings.Contains(ua, "opera") || strings.Contains(ua, "opr/") { if strings.Contains(ua, "opera") || strings.Contains(ua, "opr/") {
@ -1514,6 +1526,37 @@ func generateNicknameFromUserAgent(userAgent string) string {
} }
// ensureNickname ensures session has a nickname, auto-generating if needed // ensureNickname ensures session has a nickname, auto-generating if needed
func (sm *SessionManager) validateNickname(nickname string) error {
if len(nickname) < minNicknameLength {
return fmt.Errorf("nickname must be at least %d characters", minNicknameLength)
}
if len(nickname) > maxNicknameLength {
return fmt.Errorf("nickname must be %d characters or less", maxNicknameLength)
}
if !isValidNickname(nickname) {
return errors.New("nickname can only contain letters, numbers, spaces, and - _ . @")
}
for i, r := range nickname {
if r < 32 || r == 127 {
return fmt.Errorf("nickname contains control character at position %d", i)
}
if r >= 0x200B && r <= 0x200D {
return errors.New("nickname contains zero-width character")
}
}
trimmed := ""
for _, r := range nickname {
trimmed += string(r)
}
if trimmed != nickname {
return errors.New("nickname contains disallowed unicode")
}
return nil
}
func (sm *SessionManager) ensureNickname(session *Session) { func (sm *SessionManager) ensureNickname(session *Session) {
// Skip if session already has a nickname // Skip if session already has a nickname
if session.Nickname != "" { if session.Nickname != "" {

View File

@ -1,4 +1,4 @@
import { useEffect, useState, useCallback } from "react"; import { useEffect, useState, useCallback, useRef } from "react";
import { useNavigate } from "react-router"; import { useNavigate } from "react-router";
import { XCircleIcon } from "@heroicons/react/24/outline"; import { XCircleIcon } from "@heroicons/react/24/outline";
@ -30,6 +30,7 @@ export default function AccessDeniedOverlay({
const { maxRejectionAttempts } = useSettingsStore(); const { maxRejectionAttempts } = useSettingsStore();
const [countdown, setCountdown] = useState(10); const [countdown, setCountdown] = useState(10);
const [isRetrying, setIsRetrying] = useState(false); const [isRetrying, setIsRetrying] = useState(false);
const hasCountedRef = useRef(false);
const handleLogout = useCallback(async () => { const handleLogout = useCallback(async () => {
try { try {
@ -50,7 +51,15 @@ export default function AccessDeniedOverlay({
}, [navigate, setUser, clearSession, clearNickname]); }, [navigate, setUser, clearSession, clearNickname]);
useEffect(() => { useEffect(() => {
if (!show) return; if (!show) {
hasCountedRef.current = false;
setCountdown(10);
return;
}
// Only count rejection once per showing
if (hasCountedRef.current) return;
hasCountedRef.current = true;
const newCount = incrementRejectionCount(); const newCount = incrementRejectionCount();

View File

@ -31,9 +31,16 @@ export default function InfoBar() {
useEffect(() => { useEffect(() => {
if (!rpcDataChannel) return; if (!rpcDataChannel) return;
rpcDataChannel.onclose = () => console.log("rpcDataChannel has closed"); rpcDataChannel.onclose = () => {
rpcDataChannel.onerror = (e: Event) => if (rpcDataChannel.readyState === "closed") {
console.error(`Error on DataChannel '${rpcDataChannel.label}': ${e}`); console.debug("rpcDataChannel closed");
}
};
rpcDataChannel.onerror = (e: Event) => {
if (rpcDataChannel.readyState === "open" || rpcDataChannel.readyState === "connecting") {
console.error(`Error on DataChannel '${rpcDataChannel.label}':`, e);
}
};
}, [rpcDataChannel]); }, [rpcDataChannel]);
const { keyboardLedState, usbState } = useHidStore(); const { keyboardLedState, usbState } = useHidStore();

View File

@ -29,7 +29,10 @@ export function useVersion() {
return new Promise<SystemVersionInfo>((resolve, reject) => { return new Promise<SystemVersionInfo>((resolve, reject) => {
send("getUpdateStatus", {}, (resp: JsonRpcResponse) => { send("getUpdateStatus", {}, (resp: JsonRpcResponse) => {
if ("error" in resp) { if ("error" in resp) {
notifications.error(`Failed to check for updates: ${resp.error}`); const errorMsg = typeof resp.error === 'object' && resp.error.message
? resp.error.message
: String(resp.error);
notifications.error(`Failed to check for updates: ${errorMsg}`);
reject(new Error("Failed to check for updates")); reject(new Error("Failed to check for updates"));
} else { } else {
const result = resp.result as SystemVersionInfo; const result = resp.result as SystemVersionInfo;
@ -56,8 +59,11 @@ export function useVersion() {
console.warn("Failed to get device version, using legacy version"); console.warn("Failed to get device version, using legacy version");
return getVersionInfo().then(result => resolve(result.local)).catch(reject); return getVersionInfo().then(result => resolve(result.local)).catch(reject);
} }
console.error("Failed to get device version N", resp.error); console.error("Failed to get device version:", resp.error);
notifications.error(`Failed to get device version: ${resp.error}`); const errorMsg = typeof resp.error === 'object' && resp.error.message
? resp.error.message
: String(resp.error);
notifications.error(`Failed to get device version: ${errorMsg}`);
reject(new Error("Failed to get device version")); reject(new Error("Failed to get device version"));
} else { } else {
const result = resp.result as VersionInfo; const result = resp.result as VersionInfo;

View File

@ -965,10 +965,11 @@ export default function KvmIdRoute() {
useEffect(() => { useEffect(() => {
if (appVersion) return; if (appVersion) return;
if (rpcDataChannel?.readyState !== "open") return; if (rpcDataChannel?.readyState !== "open") return;
if (currentMode === "pending") return;
getLocalVersion(); getLocalVersion();
// eslint-disable-next-line react-hooks/exhaustive-deps // eslint-disable-next-line react-hooks/exhaustive-deps
}, [appVersion, rpcDataChannel?.readyState]); }, [appVersion, rpcDataChannel?.readyState, currentMode]);
const ConnectionStatusElement = useMemo(() => { const ConnectionStatusElement = useMemo(() => {
const isOtherSession = location.pathname.includes("other-session"); const isOtherSession = location.pathname.includes("other-session");