Skip to content

Commit 846b975

Browse files
mcuelenaereclaude
andcommitted
fix(mdns): defer setup-state mutation until validated; drop dead setters
Two cursorbot findings on 57968fd: 1. handleSetup assigned config.LocalAuthMode before validating the password (length, bcrypt). On validation failure it returned without SaveConfig, but the in-memory LocalAuthMode was already dirty — so a subsequent restartMdns (e.g. from networkStateChanged) would publish `setup=true` over Bonjour even though the device hadn't actually been set up, and the "Device is already set up" guard at the top of handleSetup would refuse the user's retry. Move the assignment after all validation succeeds, and roll back the in-memory state if SaveConfig itself fails. 2. SetService / SetLocalNames / SetListenOptions (and their inner set* helpers) are unused — only SetOptions is called from restartMdns. Since they could even mislead future maintainers into bypassing the atomic SetOptions path, drop them along with the now-unused `reflect` import. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 57968fd commit 846b975

2 files changed

Lines changed: 27 additions & 69 deletions

File tree

internal/mdns/mdns.go

Lines changed: 0 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ package mdns
2020
import (
2121
"fmt"
2222
"net"
23-
"reflect"
2423
"strings"
2524

2625
"github.com/jetkvm/kvm/internal/logging"
@@ -327,60 +326,6 @@ func (m *MDNS) Stop() error {
327326
return err
328327
}
329328

330-
func (m *MDNS) setLocalNames(localNames []string) {
331-
m.lock.Lock()
332-
defer m.lock.Unlock()
333-
334-
if reflect.DeepEqual(m.localNames, localNames) {
335-
return
336-
}
337-
338-
m.localNames = localNames
339-
}
340-
341-
func (m *MDNS) setListenOptions(listenOptions *MDNSListenOptions) {
342-
m.lock.Lock()
343-
defer m.lock.Unlock()
344-
345-
if m.listenOptions != nil &&
346-
listenOptions != nil &&
347-
m.listenOptions.IPv4 == listenOptions.IPv4 &&
348-
m.listenOptions.IPv6 == listenOptions.IPv6 {
349-
return
350-
}
351-
352-
m.listenOptions = listenOptions
353-
}
354-
355-
func (m *MDNS) setService(service *MDNSService) {
356-
m.lock.Lock()
357-
defer m.lock.Unlock()
358-
359-
if reflect.DeepEqual(m.service, service) {
360-
return
361-
}
362-
363-
m.service = service
364-
}
365-
366-
// SetLocalNames sets the local names and restarts the mDNS server
367-
func (m *MDNS) SetLocalNames(localNames []string) error {
368-
m.setLocalNames(localNames)
369-
return m.Restart()
370-
}
371-
372-
// SetListenOptions sets the listen options and restarts the mDNS server
373-
func (m *MDNS) SetListenOptions(listenOptions *MDNSListenOptions) error {
374-
m.setListenOptions(listenOptions)
375-
return m.Restart()
376-
}
377-
378-
// SetService sets the advertised DNS-SD service and restarts the mDNS server
379-
func (m *MDNS) SetService(service *MDNSService) error {
380-
m.setService(service)
381-
return m.Restart()
382-
}
383-
384329
// SetOptions sets the local names, listen options, and service, then
385330
// restarts the mDNS server. All three fields are updated under a
386331
// single lock acquisition so a concurrent restart can't observe a

web.go

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -870,8 +870,13 @@ func handleSetup(c *gin.Context) {
870870
return
871871
}
872872

873-
config.LocalAuthMode = req.LocalAuthMode
874-
873+
// Validate the full request and compute the new state BEFORE
874+
// touching config. If any check fails we return early and leave
875+
// the device in its pre-setup state — otherwise an aborted setup
876+
// would leave LocalAuthMode set in memory and trip the
877+
// "already set up" guard on retry (and leak `setup=true` over
878+
// mDNS on the next restartMdns).
879+
var newHashedPassword, newAuthToken string
875880
if req.LocalAuthMode == "password" {
876881
if req.Password == "" {
877882
passwordRateLimiter.RecordFailure(ip)
@@ -890,30 +895,38 @@ func handleSetup(c *gin.Context) {
890895
return
891896
}
892897

893-
// Hash the password
894898
hashedPassword, err := bcrypt.GenerateFromPassword([]byte(req.Password), bcrypt.DefaultCost)
895899
if err != nil {
896900
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to hash password"})
897901
return
898902
}
899903

900-
config.HashedPassword = string(hashedPassword)
901-
config.LocalAuthToken = uuid.New().String()
902-
903-
// Set the cookie
904-
c.SetCookie("authToken", config.LocalAuthToken, authTokenMaxAge, "/", "", false, true)
905-
} else {
906-
// For noPassword mode, ensure the password field is empty
907-
config.HashedPassword = ""
908-
config.LocalAuthToken = ""
904+
newHashedPassword = string(hashedPassword)
905+
newAuthToken = uuid.New().String()
909906
}
910907

911-
err := SaveConfig()
912-
if err != nil {
908+
// All validation passed — commit. If SaveConfig fails, roll the
909+
// in-memory state back so a retry can succeed.
910+
prevAuthMode := config.LocalAuthMode
911+
prevHashedPassword := config.HashedPassword
912+
prevAuthToken := config.LocalAuthToken
913+
914+
config.LocalAuthMode = req.LocalAuthMode
915+
config.HashedPassword = newHashedPassword
916+
config.LocalAuthToken = newAuthToken
917+
918+
if err := SaveConfig(); err != nil {
919+
config.LocalAuthMode = prevAuthMode
920+
config.HashedPassword = prevHashedPassword
921+
config.LocalAuthToken = prevAuthToken
913922
c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to save config"})
914923
return
915924
}
916925

926+
if req.LocalAuthMode == "password" {
927+
c.SetCookie("authToken", config.LocalAuthToken, authTokenMaxAge, "/", "", false, true)
928+
}
929+
917930
// Refresh the mDNS advertisement so the `setup=` TXT record
918931
// flips from false to true now that the device is provisioned.
919932
restartMdns()

0 commit comments

Comments
 (0)