Address CodeRabbit review and fix CI on embedded-vnc

client/internal/debug/debug_test.go

@@ -863,6 +863,7 @@ func TestAddConfig_AllFieldsCovered(t *testing.T) {
 		RosenpassPermissive:           true,
 		ServerSSHAllowed:              &bTrue,
 		ServerVNCAllowed:              &bTrue,
+		DisableVNCApproval:            &bTrue,
 		EnableSSHRoot:                 &bTrue,
 		EnableSSHSFTP:                 &bTrue,
 		EnableSSHLocalPortForwarding:  &bTrue,

client/internal/profilemanager/config.go

@@ -433,7 +433,7 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
 			updated = true
 		}
 	} else if config.ServerVNCAllowed == nil {
-		config.ServerVNCAllowed = util.True()
+		config.ServerVNCAllowed = util.False()
 		updated = true
 	}
 

client/server/capture.go

@@ -111,7 +111,7 @@ func (s *Server) StartCapture(req *proto.StartCaptureRequest, stream proto.Daemo
 		return status.Errorf(codes.Internal, "create capture session: %v", err)
 	}
 
-	engine, err := s.claimCapture(sess)
+	engine, err := s.claimCapture(sess, func() { pw.Close() })
 	if err != nil {
 		sess.Stop()
 		pw.Close()
@@ -305,7 +305,7 @@ func (s *Server) cleanupBundleCapture() {
 // capture is already running it is evicted: a previous streaming session
 // whose gRPC client died and never freed the slot stays stuck otherwise,
 // and a bundle capture is just informational state.
-func (s *Server) claimCapture(sess *capture.Session) (*internal.Engine, error) {
+func (s *Server) claimCapture(sess *capture.Session, cancel func()) (*internal.Engine, error) {
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
 
@@ -315,6 +315,7 @@ func (s *Server) claimCapture(sess *capture.Session) (*internal.Engine, error) {
 		return nil, err
 	}
 	s.activeCapture = sess
+	s.activeCaptureCancel = cancel
 	return engine, nil
 }
 
@@ -331,13 +332,18 @@ func (s *Server) evictActiveCaptureLocked() {
 	}
 	log.Infof("evicting previous streaming capture to start a new one")
 	prev := s.activeCapture
+	cancel := s.activeCaptureCancel
 	if engine, err := s.getCaptureEngineLocked(); err == nil {
 		if err := engine.SetCapture(nil); err != nil {
 			log.Debugf("clear previous capture: %v", err)
 		}
 	}
 	s.activeCapture = nil
+	s.activeCaptureCancel = nil
 	prev.Stop()
+	if cancel != nil {
+		cancel()
+	}
 }
 
 // releaseCapture clears the active-capture owner if it still matches sess.
@@ -346,6 +352,7 @@ func (s *Server) releaseCapture(sess *capture.Session) {
 	defer s.mutex.Unlock()
 	if s.activeCapture == sess {
 		s.activeCapture = nil
+		s.activeCaptureCancel = nil
 	}
 }
 
@@ -360,6 +367,7 @@ func (s *Server) clearCaptureIfOwner(sess *capture.Session, engine *internal.Eng
 		log.Debugf("clear capture: %v", err)
 	}
 	s.activeCapture = nil
+	s.activeCaptureCancel = nil
 }
 
 func (s *Server) getCaptureEngineLocked() (*internal.Engine, error) {

client/server/server.go

@@ -93,8 +93,12 @@ type Server struct {
 	captureEnabled         bool
 	bundleCapture          *bundleCapture
 	// activeCapture is the session currently installed on the engine; guarded by s.mutex.
-	activeCapture    *capture.Session
-	networksDisabled bool
+	activeCapture *capture.Session
+	// activeCaptureCancel tears down the streaming pipe/cancel for the
+	// active streaming capture so eviction unblocks the StartCapture RPC
+	// handler. Nil for bundle captures (they own their own context).
+	activeCaptureCancel func()
+	networksDisabled    bool
 
 	sleepHandler *sleephandler.SleepHandler
 

client/ui/approval.go

@@ -113,15 +113,17 @@ func (s *serviceClient) showApprovalUI(req approvalRequest) {
 				decide(outcome{})
 				return
 			}
-			updateCountdown()
+			fyne.Do(updateCountdown)
 		}
 	}()
 
 	go func() {
 		o := <-decided
 		s.sendApprovalResponse(req.requestID, o.accept, o.viewOnly)
-		w.Close()
-		s.app.Quit()
+		fyne.Do(func() {
+			w.Close()
+			s.app.Quit()
+		})
 	}()
 
 	w.Show()

client/vnc/server/agent_windows.go

@@ -47,8 +47,6 @@ var (
 	procWTSEnumerateSessionsW      = wtsapi32.NewProc("WTSEnumerateSessionsW")
 	procWTSFreeMemory              = wtsapi32.NewProc("WTSFreeMemory")
 	procWTSQuerySessionInformation = wtsapi32.NewProc("WTSQuerySessionInformationW")
-
-	iphlpapi                = windows.NewLazySystemDLL("iphlpapi.dll")
 )
 
 // GetCurrentSessionID returns the session ID of the current process.
@@ -514,6 +512,8 @@ func (m *sessionManager) reapExitedAgent() {
 		log.Debugf("close agent handle: %v", err)
 	}
 	m.agentProc = 0
+	m.authToken = ""
+	m.socketPath = ""
 }
 
 // scheduleNextSpawn applies an exponential backoff on fast crashes (<5s) and
@@ -586,6 +586,8 @@ func (m *sessionManager) killAgent() {
 	_ = windows.TerminateProcess(m.agentProc, 0)
 	_ = windows.CloseHandle(m.agentProc)
 	m.agentProc = 0
+	m.authToken = ""
+	m.socketPath = ""
 	log.Info("killed old agent")
 }
 

client/vnc/server/console_user_darwin.go

@@ -1,19 +1,10 @@
 package server
 
-import "errors"
-
-// consoleHasInteractiveUser returns true when a user is logged into the
-// console (i.e. an Aqua session is active). At the loginwindow there is
-// nobody to display an approval prompt to, so callers can decline
-// without waiting on the broker.
-func consoleHasInteractiveUser() bool {
-	if _, err := consoleUserID(); err != nil {
-		if errors.Is(err, errNoConsoleUser) {
-			return false
-		}
-		// Unknown error: fail closed so a probe-time glitch does not
-		// silently let an unattended console accept VNC sessions.
-		return false
-	}
-	return true
+// interactiveUserError returns nil when a user is logged into the console
+// (i.e. an Aqua session is active). At the loginwindow there is nobody to
+// display an approval prompt to, so callers can decline without waiting on
+// the broker. Any error (including errNoConsoleUser) is treated as decline.
+func interactiveUserError() error {
+	_, err := consoleUserID()
+	return err
 }

client/vnc/server/console_user_other.go

@@ -2,6 +2,6 @@
 
 package server
 
-// consoleHasInteractiveUser is unused outside service mode (darwin/windows)
-// but the symbol must exist so gateApproval compiles on all platforms.
-func consoleHasInteractiveUser() bool { return true }
+// interactiveUserError is unused outside service mode (darwin/windows) but
+// the symbol must exist so gateApproval compiles on all platforms.
+func interactiveUserError() error { return nil }

client/vnc/server/console_user_windows.go

@@ -1,13 +1,15 @@
 package server
 
-// consoleHasInteractiveUser returns true when there is a logged-in user
-// session on the box. At the lock/login screen WTSQueryUserName is empty,
-// which means there is nobody to display an approval prompt to. Callers
-// should decline without waiting on the broker in that case.
-func consoleHasInteractiveUser() bool {
+// interactiveUserError returns nil when there is a logged-in user session
+// on the box. At the lock/login screen WTSQueryUserName is empty, which
+// means there is nobody to display an approval prompt to.
+func interactiveUserError() error {
 	sid := getActiveSessionID()
 	if sid == 0 {
-		return false
+		return errNoConsoleUser
 	}
-	return wtsSessionHasUser(sid)
+	if !wtsSessionHasUser(sid) {
+		return errNoConsoleUser
+	}
+	return nil
 }

client/vnc/server/server.go

@@ -431,10 +431,12 @@ func (s *Server) gateApproval(conn net.Conn, header *connectionHeader, connLog *
 		connLog.Warn("VNC connection rejected: approval required but no approver")
 		return false, ApprovalDecision{}
 	}
-	if s.serviceMode && !consoleHasInteractiveUser() {
-		rejectConnection(conn, codeMessage(RejectCodeNoConsoleUser, "no interactive user session"))
-		connLog.Info("VNC connection rejected: no interactive user session to approve")
-		return false, ApprovalDecision{}
+	if s.serviceMode {
+		if err := interactiveUserError(); err != nil {
+			rejectConnection(conn, codeMessage(RejectCodeNoConsoleUser, "no interactive user session"))
+			connLog.Infof("VNC connection rejected: no interactive user session to approve: %v", err)
+			return false, ApprovalDecision{}
+		}
 	}
 	info := ApprovalInfo{
 		SourceIP: sourceIPString(conn.RemoteAddr()),
@@ -449,7 +451,7 @@ func (s *Server) gateApproval(conn net.Conn, header *connectionHeader, connLog *
 	}
 	decision, err := s.approver.Request(s.ctx, info)
 	if err != nil {
-		rejectConnection(conn, codeMessage(RejectCodeApprovalDenied, err.Error()))
+		rejectConnection(conn, codeMessage(RejectCodeApprovalDenied, "approval denied"))
 		connLog.Infof("VNC connection rejected: approval %v", err)
 		return false, ApprovalDecision{}
 	}
@@ -737,9 +739,13 @@ func (s *Server) validateCapturer(capturer ScreenCapturer) error {
 func (s *Server) isAllowedSource(addr net.Addr) bool {
 	// Unix-socket remotes (the agent path) are local IPC, gated by the
 	// token, not by overlay membership.
+	if _, ok := addr.(*net.UnixAddr); ok {
+		return true
+	}
 	tcpAddr, ok := addr.(*net.TCPAddr)
 	if !ok {
-		return true
+		s.log.Warnf("connection rejected: unsupported remote address type %T", addr)
+		return false
 	}
 
 	remoteIP, ok := netip.AddrFromSlice(tcpAddr.IP)

client/vnc/server/server_windows.go

@@ -269,4 +269,3 @@ func (s *Server) serviceAcceptLoop() {
 		}(conn)
 	}
 }
-

client/vnc/server/session.go

@@ -545,6 +545,9 @@ func (s *session) handleFBUpdateRequest() error {
 // in sync with the active session (e.g. username changes after login on
 // a virtual session).
 func (s *session) SendDesktopName(name string) error {
+	if s.viewOnly {
+		name = ViewOnlyDesktopNamePrefix + name
+	}
 	s.encMu.RLock()
 	supported := s.clientSupportsDesktopName
 	s.encMu.RUnlock()
@@ -629,13 +632,13 @@ func (s *session) handlePointerEvent() error {
 		mask = (mask & 0x7f) | uint16(hi[0])<<7
 	}
 
+	if s.viewOnly {
+		return nil
+	}
 	s.pointerMu.Lock()
 	s.lastPointerX = x
 	s.lastPointerY = y
 	s.pointerMu.Unlock()
-	if s.viewOnly {
-		return nil
-	}
 	s.injector.InjectPointer(mask, x, y, s.serverW, s.serverH)
 	return nil
 }
@@ -661,6 +664,9 @@ var stickyModifierKeysyms = [...]uint32{
 // when the client disconnects mid-press. Mouse coordinates are reused
 // from the last PointerEvent so we don't warp the cursor.
 func (s *session) releaseStickyInput() {
+	if s.viewOnly {
+		return
+	}
 	for _, ks := range stickyModifierKeysyms {
 		s.injector.InjectKey(ks, false)
 	}

management/server/http/handlers/peers/peers_handler.go

@@ -475,6 +475,37 @@ func (h *Handler) CreateTemporaryAccess(w http.ResponseWriter, r *http.Request)
 	newPeer := &nbpeer.Peer{}
 	newPeer.FromAPITemporaryAccessRequest(&req)
 
+	parsedRules := make([]struct {
+		raw       string
+		protocol  types.PolicyRuleProtocolType
+		portRange types.RulePortRange
+	}, 0, len(req.Rules))
+	needsVNCKey := false
+	for _, rule := range req.Rules {
+		protocol, portRange, err := types.ParseRuleString(rule)
+		if err != nil {
+			util.WriteError(r.Context(), err, w)
+			return
+		}
+		if protocol == types.PolicyRuleProtocolNetbirdVNC {
+			needsVNCKey = true
+		}
+		parsedRules = append(parsedRules, struct {
+			raw       string
+			protocol  types.PolicyRuleProtocolType
+			portRange types.RulePortRange
+		}{rule, protocol, portRange})
+	}
+
+	var vncSessionPubKey string
+	if needsVNCKey {
+		vncSessionPubKey, err = validateVNCSessionPubKey(req.SessionPubKey)
+		if err != nil {
+			util.WriteError(r.Context(), err, w)
+			return
+		}
+	}
+
 	targetPeer, err := h.accountManager.GetPeer(r.Context(), userAuth.AccountId, peerID, userAuth.UserId)
 	if err != nil {
 		util.WriteError(r.Context(), err, w)
@@ -487,12 +518,7 @@ func (h *Handler) CreateTemporaryAccess(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
-	for _, rule := range req.Rules {
-		protocol, portRange, err := types.ParseRuleString(rule)
-		if err != nil {
-			util.WriteError(r.Context(), err, w)
-			return
-		}
+	for _, pr := range parsedRules {
 		policy := &types.Policy{
 			AccountID:   userAuth.AccountId,
 			Description: "Temporary access policy for peer " + peer.Name,
@@ -512,34 +538,28 @@ func (h *Handler) CreateTemporaryAccess(w http.ResponseWriter, r *http.Request)
 					ID:   targetPeer.ID,
 				},
 				Bidirectional: false,
-				Protocol:      protocol,
-				PortRanges:    []types.RulePortRange{portRange},
+				Protocol:      pr.protocol,
+				PortRanges:    []types.RulePortRange{pr.portRange},
 			}},
 		}
-		if protocol == types.PolicyRuleProtocolNetbirdSSH || protocol == types.PolicyRuleProtocolNetbirdVNC {
+		if pr.protocol == types.PolicyRuleProtocolNetbirdSSH || pr.protocol == types.PolicyRuleProtocolNetbirdVNC {
 			policy.Rules[0].AuthorizedUser = userAuth.UserId
 		}
-		if protocol == types.PolicyRuleProtocolNetbirdVNC {
-			pubKey, err := validateVNCSessionPubKey(req.SessionPubKey)
-			if err != nil {
-				util.WriteError(r.Context(), err, w)
-				return
-			}
-			policy.Rules[0].SessionPubKey = pubKey
+		if pr.protocol == types.PolicyRuleProtocolNetbirdVNC {
+			policy.Rules[0].SessionPubKey = vncSessionPubKey
 			policy.Rules[0].SessionDisplayName = h.displayNameForUser(r.Context(), userAuth)
 		}
 
-		_, err = h.accountManager.SavePolicy(r.Context(), userAuth.AccountId, userAuth.UserId, policy, true)
-		if err != nil {
+		if _, err = h.accountManager.SavePolicy(r.Context(), userAuth.AccountId, userAuth.UserId, policy, true); err != nil {
 			util.WriteError(r.Context(), err, w)
 			return
 		}
 	}
 
 	resp := &api.PeerTemporaryAccessResponse{
-		Id:             peer.ID,
-		Name:           peer.Name,
-		Rules:          req.Rules,
+		Id:           peer.ID,
+		Name:         peer.Name,
+		Rules:        req.Rules,
 		TargetPubKey: targetPeer.Key,
 	}