fix: address code review IMPORTANT issues — atomic TOCTOU, updateKeyHolder race, dead keyReceived var

Client/tauri-client/src/lib/e2eeCrypto.ts

@@ -24,9 +24,11 @@ if (typeof crypto === "undefined" || !crypto.subtle) {
 }
 
 const ECDH_CURVE = "P-256";
+// UTF-8 bytes of "owncord-voice-e2ee-v1"
 const HKDF_SALT = new Uint8Array([
   111, 119, 110, 99, 111, 114, 100, 45, 118, 111, 105, 99, 101, 45, 101, 50, 101, 101, 45, 118, 49,
 ]);
+// UTF-8 bytes of "room-key-wrap"
 const HKDF_INFO = new Uint8Array([114, 111, 111, 109, 45, 107, 101, 121, 45, 119, 114, 97, 112]);
 const ROOM_KEY_BYTES = 32; // 256-bit AES key for LiveKit SFrame
 

Client/tauri-client/src/lib/livekitSession.ts

@@ -868,10 +868,8 @@ export class LiveKitSession {
           new Promise<void>((_, reject) => {
             timeoutId = setTimeout(() => reject(new Error("E2EE key exchange timeout")), ms);
           });
-        let keyReceived = false;
         try {
           await Promise.race([roomKeyPromise, makeTimeout(10_000)]);
-          keyReceived = true;
         } catch {
           // First attempt timed out — re-announce and retry once.
           if (timeoutId !== null) clearTimeout(timeoutId);
@@ -882,7 +880,6 @@ export class LiveKitSession {
           });
           try {
             await Promise.race([roomKeyPromise, makeTimeout(5_000)]);
-            keyReceived = true;
           } catch {
             log.error("E2EE: key exchange timed out after retry — disconnecting", { channelId });
             this._roomKeyResolver = null;

Server/ws/voice_e2ee.go

@@ -7,9 +7,11 @@ import (
 	"log/slog"
 )
 
-// decodeBase64Loose accepts both standard (padded) and raw (unpadded) base64.
-// ECDH public keys exported from WebCrypto use raw base64 (no '=' padding);
-// we accept both to avoid breaking existing clients.
+// decodeBase64Loose accepts both padded (StdEncoding) and unpadded (RawStdEncoding)
+// standard-alphabet base64. ECDH public keys exported from WebCrypto omit '='
+// padding; we accept both forms to avoid breaking existing clients.
+// Note: URL-safe base64 (alphabet '-_') is not accepted; clients must use the
+// standard alphabet ('+/').
 func decodeBase64Loose(s string) ([]byte, error) {
 	if b, err := base64.StdEncoding.DecodeString(s); err == nil {
 		return b, nil
@@ -20,8 +22,14 @@ func decodeBase64Loose(s string) ([]byte, error) {
 // updateKeyHolder scans connected clients to find the one with the lowest
 // userID currently in channelID, and records them as the key holder.
 // If no clients remain in the channel the entry is deleted.
-// Must NOT be called while h.mu is held (it acquires h.mu.RLock internally).
+// Must NOT be called while h.mu or h.keyHolderMu is held.
+// Lock order: keyHolderMu (write) → h.mu (read). Holding keyHolderMu for
+// the entire scan+write prevents two concurrent calls from racing and
+// overwriting each other with a stale result.
 func (h *Hub) updateKeyHolder(channelID int64) {
+	h.keyHolderMu.Lock()
+	defer h.keyHolderMu.Unlock()
+
 	h.mu.RLock()
 	var minUserID int64
 	found := false
@@ -35,13 +43,11 @@ func (h *Hub) updateKeyHolder(channelID int64) {
 	}
 	h.mu.RUnlock()
 
-	h.keyHolderMu.Lock()
 	if found {
 		h.voiceKeyHolders[channelID] = minUserID
 	} else {
 		delete(h.voiceKeyHolders, channelID)
 	}
-	h.keyHolderMu.Unlock()
 }
 
 // isVoiceKeyHolder reports whether userID is the current key holder for channelID.
@@ -149,9 +155,11 @@ func (h *Hub) handleVoiceE2EEOffer(_ context.Context, c *Client, payload json.Ra
 		return
 	}
 
-	// Verify the target is in the same voice channel — lookup and channel
-	// check must be atomic (under the same lock hold) to prevent TOCTOU races
-	// where the target leaves between lookup and the channel comparison.
+	// Verify the target is in the same voice channel, then relay — all under
+	// one h.mu.RLock hold so the check and send are atomic. A concurrent
+	// voice_leave cannot remove the target from h.clients between the lookup
+	// and the channel comparison, nor between the comparison and the send.
+	msg := buildVoiceE2EEOffer(c.userID, p.EncryptedKey, p.IV)
 	h.mu.RLock()
 	target, ok := h.clients[p.TargetUserID]
 	if !ok {
@@ -159,16 +167,13 @@ func (h *Hub) handleVoiceE2EEOffer(_ context.Context, c *Client, payload json.Ra
 		c.sendMsg(buildErrorMsg(ErrCodeBadPayload, "target user not connected"))
 		return
 	}
-	targetChID := target.getVoiceChID()
-	h.mu.RUnlock()
-	if targetChID != voiceChID {
+	if target.getVoiceChID() != voiceChID {
+		h.mu.RUnlock()
 		c.sendMsg(buildErrorMsg(ErrCodeForbidden, "target user not in your voice channel"))
 		return
 	}
-
-	// Relay the encrypted key offer to the target.
-	msg := buildVoiceE2EEOffer(c.userID, p.EncryptedKey, p.IV)
 	target.sendMsg(msg)
+	h.mu.RUnlock()
 
 	slog.Debug("voice e2ee: offer relayed",
 		"from_user_id", c.userID, "to_user_id", p.TargetUserID, "channel_id", voiceChID)

Server/ws/voice_e2ee_test.go

@@ -387,16 +387,6 @@ func TestE2EE_GetPubKey_ReturnsKeyAfterAnnounce(t *testing.T) {
 	}
 }
 
-// ─── I-7: login rate limit ──────────────────────────────────────────────────
-
-func TestLoginRateLimit_Is5(t *testing.T) {
-	// I-7: loginRateLimitPerMinute should be 5, not 60.
-	// We can't directly access the constant from ws_test, but we test the
-	// behavior via the API package test. This test is a placeholder that
-	// verifies the constant value via the api package's exported test helper.
-	// (See constants_test.go for the actual value assertion.)
-}
-
 // ─── is_key_holder in voice_token payload ────────────────────────────────────
 
 func TestE2EE_VoiceToken_IncludesIsKeyHolder(t *testing.T) {