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")
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

@ -12,6 +12,7 @@ import (
"reflect"
"regexp"
"strconv"
"strings"
"sync"
"time"
@ -132,8 +133,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

@ -95,42 +95,11 @@ func handleRequestSessionApprovalRPC(session *Session) (any, error) {
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) {
sessionID, _ := params["sessionId"].(string)
nickname, _ := params["nickname"].(string)
if err := validateNickname(nickname); err != nil {
if err := sessionManager.validateNickname(nickname); err != nil {
return nil, err
}

View File

@ -2,6 +2,8 @@ package kvm
import (
"time"
"github.com/pion/webrtc/v4"
)
// 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)
// Returns true if any pending session was removed
// handlePendingSessionTimeout removes timed-out pending sessions only if disconnected
// 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 {
toDelete := make([]string, 0)
for id, session := range sm.sessions {
if session.Mode == SessionModePending &&
now.Sub(session.CreatedAt) > defaultPendingSessionTimeout {
websocketLogger.Debug().
Str("sessionId", id).
Dur("age", now.Sub(session.CreatedAt)).
Msg("Removing timed-out pending session")
toDelete = append(toDelete, id)
// Only remove if the connection is closed/failed
// This prevents resource leaks while keeping connected sessions visible
if session.peerConnection != nil {
connectionState := session.peerConnection.ConnectionState()
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 {

View File

@ -173,11 +173,8 @@ func (sm *SessionManager) AddSession(session *Session, clientSettings *SessionSe
}
if session.Nickname != "" {
if len(session.Nickname) < minNicknameLength {
return fmt.Errorf("nickname must be at least %d characters", minNicknameLength)
}
if len(session.Nickname) > maxNicknameLength {
return fmt.Errorf("nickname must be %d characters or less", maxNicknameLength)
if err := sm.validateNickname(session.Nickname); err != nil {
return err
}
}
if len(session.Identity) > maxIdentityLength {
@ -251,8 +248,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 +426,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 +440,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 +465,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 {
@ -1209,11 +1220,9 @@ func (sm *SessionManager) transferPrimaryRole(fromSessionID, toSessionID, transf
return fmt.Errorf("cannot promote: another primary session exists (%s)", existingPrimaryID)
}
// Promote target session
toSession.Mode = SessionModePrimary
toSession.hidRPCAvailable = false
// Reset LastActive for all emergency promotions to prevent immediate re-timeout
if strings.HasPrefix(transferType, "emergency_") {
if transferType == "emergency_timeout_promotion" {
toSession.LastActive = time.Now()
}
sm.primarySessionID = toSessionID
@ -1462,16 +1471,19 @@ func extractBrowserFromUserAgent(userAgent string) *string {
ua := strings.ToLower(userAgent)
// 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") {
return &BrowserEdge
}
if strings.Contains(ua, "firefox") {
return &BrowserFirefox
}
if strings.Contains(ua, "chrome") {
if hasChrome {
return &BrowserChrome
}
if strings.Contains(ua, "safari") && !strings.Contains(ua, "chrome") {
if strings.Contains(ua, "safari") {
return &BrowserSafari
}
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
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) {
// Skip if session already has a 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 { XCircleIcon } from "@heroicons/react/24/outline";
@ -30,6 +30,7 @@ export default function AccessDeniedOverlay({
const { maxRejectionAttempts } = useSettingsStore();
const [countdown, setCountdown] = useState(10);
const [isRetrying, setIsRetrying] = useState(false);
const hasCountedRef = useRef(false);
const handleLogout = useCallback(async () => {
try {
@ -50,7 +51,15 @@ export default function AccessDeniedOverlay({
}, [navigate, setUser, clearSession, clearNickname]);
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();

View File

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

View File

@ -29,7 +29,10 @@ export function useVersion() {
return new Promise<SystemVersionInfo>((resolve, reject) => {
send("getUpdateStatus", {}, (resp: JsonRpcResponse) => {
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"));
} else {
const result = resp.result as SystemVersionInfo;
@ -56,8 +59,11 @@ export function useVersion() {
console.warn("Failed to get device version, using legacy version");
return getVersionInfo().then(result => resolve(result.local)).catch(reject);
}
console.error("Failed to get device version N", resp.error);
notifications.error(`Failed to get device version: ${resp.error}`);
console.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"));
} else {
const result = resp.result as VersionInfo;

View File

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