Fix macOS input permissions, Caps Lock, scroll and layout-independent typing, reconnect the X11 injector, and release VNC resources when start fails

client/internal/engine_vnc.go

@@ -194,6 +194,10 @@ func (e *Engine) startVNCServer(authConfig *sshauth.Config) error {
 	listenAddr := netip.AddrPortFrom(netbirdIP, vnc.InternalPort)
 	network := e.wgInterface.Address().Network
 	if err := srv.Start(e.ctx, listenAddr, network); err != nil {
+		// The server never took ownership, so nothing else will release what
+		// newPlatformVNC opened: the X11 injector's display connection, the
+		// uinput device, the framebuffer mapping.
+		closeVNCResources(capturer, injector)
 		return fmt.Errorf("start VNC server: %w", err)
 	}
 
@@ -394,3 +398,14 @@ func (e *Engine) persistVNCProcesses(state *vncserver.ShutdownState) {
 		log.Debugf("update VNC session state: %v", err)
 	}
 }
+
+// closeVNCResources releases a capturer and an injector that implement Close.
+// Either may be a stub that holds nothing.
+func closeVNCResources(capturer vncserver.ScreenCapturer, injector vncserver.InputInjector) {
+	if c, ok := capturer.(interface{ Close() }); ok {
+		c.Close()
+	}
+	if i, ok := injector.(interface{ Close() }); ok {
+		i.Close()
+	}
+}

client/internal/engine_vnc_darwin.go

@@ -13,18 +13,22 @@ import (
 func newPlatformVNC() (vncserver.ScreenCapturer, vncserver.InputInjector, bool) {
 	capturer := vncserver.NewMacPoller()
 
-	// Ask only when this process is the one that will capture. Screen Recording
-	// is a user-scope TCC service, so the request is dropped from a
+	// Screen Recording is asked for only when this process is the one that will
+	// capture. It is a user-scope TCC service, so the request is dropped from a
 	// LaunchDaemon: no prompt appears and NetBird never even reaches the Screen
 	// Recording list. In that case the per-user agent asks instead, see
-	// newAgentResources.
-	//
-	// Without service mode there is no agent, so this process captures and
-	// nothing else will ever raise the prompt — the client would serve a
-	// windowless desktop with no indication why.
-	if !vncNeedsServiceMode() {
-		vncserver.RequestScreenRecording()
+	// newAgentResources. Without service mode there is no agent, so nothing
+	// else will ever raise the prompt and the client would serve a windowless
+	// desktop with no indication why.
+	if vncNeedsServiceMode() {
+		// The per-user agent owns capture and input in service mode, so this
+		// process needs neither. A real injector here would still hold its
+		// PreventUserIdleDisplaySleep assertion from construction, keeping
+		// the display awake for the daemon's whole life with no VNC session
+		// in sight.
+		return capturer, &vncserver.StubInputInjector{}, true
 	}
+	vncserver.RequestScreenRecording()
 
 	injector, err := vncserver.NewMacInputInjector()
 	if err != nil {

client/vnc/server/input_darwin.go

@@ -9,6 +9,7 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
+	"unicode/utf16"
 	"unsafe"
 
 	"github.com/ebitengine/purego"
@@ -92,6 +93,10 @@ var (
 	cgEventSetFlags             func(uintptr, uint64)
 	cgEventSetType              func(uintptr, int32)
 	cgEventCreateForInput       func(uintptr) uintptr
+	// cgEventKeyboardSetUnicodeString attaches literal text to a keyboard
+	// event, so the receiving app gets those characters whatever the active
+	// keyboard layout would have produced for the keycode.
+	cgEventKeyboardSetUnicodeString func(uintptr, uintptr, *uint16)
 
 	// CGEventCreateScrollWheelEvent is variadic, call via SyscallN.
 	cgEventCreateScrollWheelEventAddr uintptr
@@ -163,6 +168,9 @@ func initDarwinInput() {
 		purego.RegisterLibFunc(&cgEventSetFlags, cg, "CGEventSetFlags")
 		purego.RegisterLibFunc(&cgEventSetType, cg, "CGEventSetType")
 		purego.RegisterLibFunc(&cgEventCreateForInput, cg, "CGEventCreate")
+		if sym, err := purego.Dlsym(cg, "CGEventKeyboardSetUnicodeString"); err == nil {
+			purego.RegisterFunc(&cgEventKeyboardSetUnicodeString, sym)
+		}
 
 		sym, err := purego.Dlsym(cg, "CGEventCreateScrollWheelEvent")
 		if err == nil {
@@ -341,7 +349,13 @@ type MacInputInjector struct {
 	// made so far. Input arrives continuously, so the cold path is paced by time
 	// rather than by event count.
 	axNextTry atomic.Int64
-	axAsks    atomic.Int32
+	// axFirstAsk is when the first ask was made, the start of axAskWindow.
+	axFirstAsk atomic.Int64
+	// keyMu serializes keyboard emission. One injector is shared by every
+	// attach-mode session, and a modifier transition and the key that follows
+	// it have to reach the event stream as one step: interleaved with another
+	// client's, one client's Shift lands on the other's keystroke.
+	keyMu sync.Mutex
 	// modifiers is the CGEventFlags state the remote client has built up with
 	// its modifier key events, stamped onto everything posted afterwards.
 	modifiers atomic.Uint64
@@ -393,9 +407,13 @@ const (
 	// dropped without a trace. Asking again is the only way to land it, since
 	// nothing in this process can observe either the dialog or the answer.
 	axAskRetry = 8 * time.Second
-	// axMaxAsks bounds that, so a user who wants neither is left alone. A later
-	// connection asks again from a fresh process anyway.
-	axMaxAsks = 3
+	// axAskWindow bounds that, so a user who wants neither is left alone. It is
+	// a span of time rather than a count of asks because the thing being waited
+	// out is a person answering the Screen Recording dialog: a fixed three asks
+	// ran out after about sixteen seconds, and a user slower than that lost
+	// remote input for the whole agent. A later connection asks again from a
+	// fresh process anyway.
+	axAskWindow = 2 * time.Minute
 )
 
 // postEventAllowed reports whether injected events are allowed to land, reading
@@ -406,6 +424,16 @@ const (
 // It is deliberately not used anywhere else: AXIsProcessTrusted has the side
 // effect of filing the caller in the Accessibility list with the box unchecked,
 // and a decision on file, even that one, stops macOS from ever showing the dialog.
+// requestPostEvent asks for kTCCServicePostEvent where the call exists, and
+// reports whether it is granted. Where it does not exist, Accessibility is the
+// gate and there is nothing further to ask for.
+func requestPostEvent() bool {
+	if cgRequestPostEventAccess == nil {
+		return true
+	}
+	return cgRequestPostEventAccess()
+}
+
 func postEventAllowed() bool {
 	if cgPreflightPostEventAccess == nil {
 		return axProcessTrusted()
@@ -438,7 +466,8 @@ func (m *MacInputInjector) askAccessibility() {
 		return
 	}
 	m.axNextTry.Store(now.Add(axAskRetry).UnixNano())
-	if m.axAsks.Add(1) >= axMaxAsks {
+	m.axFirstAsk.CompareAndSwap(0, now.UnixNano())
+	if now.Sub(time.Unix(0, m.axFirstAsk.Load())) >= axAskWindow {
 		m.axDone.Store(true)
 	}
 
@@ -448,7 +477,11 @@ func (m *MacInputInjector) askAccessibility() {
 	// when the permission is already there, so this cannot produce a stray dialog.
 	switch {
 	case axIsProcessTrustedWithOptions != nil:
-		if axProcessIsTrusted() {
+		// Accessibility is what puts the dialog up, but CGEventPost is judged
+		// by kTCCServicePostEvent. A host can have the first and not the
+		// second, and returning on Accessibility alone would then never ask
+		// for the one that is missing, leaving input dead for the session.
+		if axProcessIsTrusted() && (postEventAllowed() || requestPostEvent()) {
 			return
 		}
 	case cgRequestPostEventAccess != nil:
@@ -517,6 +550,8 @@ func (m *MacInputInjector) InjectKey(keysym uint32, down bool) {
 	if keycode == 0xFFFF {
 		return
 	}
+	m.keyMu.Lock()
+	defer m.keyMu.Unlock()
 	m.postMacKey(src, keycode, down)
 }
 
@@ -537,6 +572,8 @@ func (m *MacInputInjector) InjectKeyScancode(scancode, keysym uint32, down bool)
 		m.InjectKey(keysym, down)
 		return
 	}
+	m.keyMu.Lock()
+	defer m.keyMu.Unlock()
 	m.postMacKey(src, vk, down)
 }
 
@@ -572,11 +609,23 @@ func (m *MacInputInjector) postMacKey(src uintptr, keycode uint16, down bool) {
 // bits off each event they receive, so the state has to be attached to
 // everything posted afterwards, which is what m.modifiers is for.
 func (m *MacInputInjector) postModifier(src uintptr, keycode uint16, down bool, bit uint64) {
+	// Caps Lock is a toggle, not a held modifier: each press flips it and the
+	// release changes nothing. Treating it like Shift clears it again on
+	// key-up, so the remote Caps Lock could never stay on.
+	capsLock := bit == kCGEventFlagMaskAlphaShift
+	if capsLock && !down {
+		return
+	}
+
 	var flags uint64
 	for {
 		old := m.modifiers.Load()
-		flags = old | bit
-		if !down {
+		switch {
+		case capsLock:
+			flags = old ^ bit
+		case down:
+			flags = old | bit
+		default:
 			flags = old &^ bit
 		}
 		if m.modifiers.CompareAndSwap(old, flags) {
@@ -714,7 +763,7 @@ func (m *MacInputInjector) dispatchPointer(src uintptr, buttonMask uint16, x, y
 	prev := m.lastButtons
 	m.postMoveOrDrag(src, prev&0x01 != 0, prev&0x04 != 0, x, y)
 	m.postButtonTransitions(src, buttonMask, x, y)
-	m.postScrollWheel(src, buttonMask)
+	m.postScrollWheel(src, prev, buttonMask)
 }
 
 func (m *MacInputInjector) postMoveOrDrag(src uintptr, leftDown, rightDown bool, x, y float64) {
@@ -762,11 +811,16 @@ func (m *MacInputInjector) postButtonTransitions(src uintptr, buttonMask uint16,
 	emit(1<<8, 1<<8, kCGEventOtherMouseDown, kCGEventOtherMouseUp, 4, 4)
 }
 
-func (m *MacInputInjector) postScrollWheel(src uintptr, buttonMask uint16) {
-	if buttonMask&0x08 != 0 {
+// postScrollWheel posts one tick per press of a wheel button. RFB spells a
+// notch as button 4 or 5 going down; a client that keeps the bit set across
+// several pointer samples is still describing that one notch, so only the
+// rising edge scrolls, as on the Windows and uinput backends.
+func (m *MacInputInjector) postScrollWheel(src uintptr, prev, buttonMask uint16) {
+	pressed := buttonMask &^ prev
+	if pressed&0x08 != 0 {
 		m.postScroll(src, scrollPixelsPerWheelTick)
 	}
-	if buttonMask&0x10 != 0 {
+	if pressed&0x10 != 0 {
 		m.postScroll(src, -scrollPixelsPerWheelTick)
 	}
 }
@@ -853,6 +907,8 @@ func (m *MacInputInjector) TypeText(text string) {
 	}
 	const maxChars = 4096
 	count := 0
+	m.keyMu.Lock()
+	defer m.keyMu.Unlock()
 	for _, r := range text {
 		if count >= maxChars {
 			break
@@ -865,6 +921,9 @@ func (m *MacInputInjector) TypeText(text string) {
 // typeRune emits the press/release events for a single ASCII rune, framing
 // the keystroke with Shift-down/up when required by the keysym.
 func (m *MacInputInjector) typeRune(src uintptr, r rune) {
+	if m.typeUnicodeRune(src, r) {
+		return
+	}
 	keysym, shift, ok := keysymForASCIIRune(r)
 	if !ok {
 		return
@@ -1062,3 +1121,31 @@ var specialKeyMap = map[uint32]uint16{
 }
 
 var _ InputInjector = (*MacInputInjector)(nil)
+
+// typeUnicodeRune types r as literal text rather than as a key on a US
+// layout, reporting false when that path is unavailable or r is a control
+// character that has to arrive as its own key (Return, Tab). Mapping a rune to
+// a keycode assumes the host's layout matches the table: on AZERTY or QWERTZ
+// that types the wrong characters, including into password fields. Text
+// attached to the event is inserted as-is, and covers non-ASCII as well.
+func (m *MacInputInjector) typeUnicodeRune(src uintptr, r rune) bool {
+	if cgEventKeyboardSetUnicodeString == nil || r < 0x20 || r == 0x7f {
+		return false
+	}
+	units := utf16.Encode([]rune{r})
+	for _, down := range []bool{true, false} {
+		event := cgEventCreateKeyboardEvent(src, 0, down)
+		if event == 0 {
+			return false
+		}
+		// No modifiers: a Shift or Option the remote client is holding would
+		// otherwise be applied on top of the literal character.
+		if cgEventSetFlags != nil {
+			cgEventSetFlags(event, 0)
+		}
+		cgEventKeyboardSetUnicodeString(event, uintptr(len(units)), &units[0])
+		cgEventPost(kCGHIDEventTap, event)
+		cfRelease(event)
+	}
+	return true
+}

client/vnc/server/input_x11.go

@@ -8,6 +8,7 @@ import (
 	"os/exec"
 	"strings"
 	"sync"
+	"time"
 
 	log "github.com/sirupsen/logrus"
 
@@ -22,11 +23,16 @@ type X11InputInjector struct {
 	// between every session, so without it two clients interleave their button
 	// transitions, and a keystroke can land between another session's
 	// Shift-down and Shift-up and come out as the wrong character.
-	inputMu           sync.Mutex
-	conn              *xgb.Conn
-	root              xproto.Window
-	screen            *xproto.ScreenInfo
-	display           string
+	inputMu sync.Mutex
+	conn    *xgb.Conn
+	root    xproto.Window
+	screen  *xproto.ScreenInfo
+	display string
+	// cookieHex is kept so the connection can be re-established the same way
+	// it was first made.
+	cookieHex string
+	// lastLiveCheck paces the liveness probe in ensureConnLocked.
+	lastLiveCheck     time.Time
 	keysymMap         map[uint32]byte
 	lastButtons       uint16
 	clipboardTool     string
@@ -49,35 +55,19 @@ func NewX11InputInjector(display, cookieHex, authFile string) (*X11InputInjector
 		return nil, fmt.Errorf("DISPLAY not set and no Xorg process found")
 	}
 
-	var conn *xgb.Conn
-	var err error
-	if cookieHex != "" {
-		conn, err = dialXUnixWithCookie(display, cookieHex)
-	} else {
-		conn, err = xgb.NewConnDisplay(display)
-	}
+	conn, screen, err := dialX11Input(display, cookieHex)
 	if err != nil {
-		return nil, fmt.Errorf("connect to X11 display %s: %w", display, err)
+		return nil, err
 	}
 
-	if err := xtest.Init(conn); err != nil {
-		conn.Close()
-		return nil, fmt.Errorf("init XTest extension: %w", err)
-	}
-
-	setup := xproto.Setup(conn)
-	if len(setup.Roots) == 0 {
-		conn.Close()
-		return nil, fmt.Errorf("no X11 screens")
-	}
-	screen := setup.Roots[0]
-
 	inj := &X11InputInjector{
-		conn:     conn,
-		root:     screen.Root,
-		screen:   &screen,
-		display:  display,
-		authFile: authFile,
+		conn:          conn,
+		root:          screen.Root,
+		screen:        &screen,
+		display:       display,
+		cookieHex:     cookieHex,
+		authFile:      authFile,
+		lastLiveCheck: time.Now(),
 	}
 	inj.cacheKeyboardMapping()
 	inj.resolveClipboardTool()
@@ -86,10 +76,71 @@ func NewX11InputInjector(display, cookieHex, authFile string) (*X11InputInjector
 	return inj, nil
 }
 
+// x11InjectorLiveCheck is how often the injector confirms its X connection is
+// still alive before injecting. A restarted X server leaves the old connection
+// dead, and XTest requests on it fail silently, so without a probe input never
+// comes back even after capture has recovered on its own.
+const x11InjectorLiveCheck = 2 * time.Second
+
+// dialX11Input opens an X connection for input injection and initialises XTest
+// on it, returning the first screen.
+func dialX11Input(display, cookieHex string) (*xgb.Conn, xproto.ScreenInfo, error) {
+	var conn *xgb.Conn
+	var err error
+	if cookieHex != "" {
+		conn, err = dialXUnixWithCookie(display, cookieHex)
+	} else {
+		conn, err = xgb.NewConnDisplay(display)
+	}
+	if err != nil {
+		return nil, xproto.ScreenInfo{}, fmt.Errorf("connect to X11 display %s: %w", display, err)
+	}
+
+	if err := xtest.Init(conn); err != nil {
+		conn.Close()
+		return nil, xproto.ScreenInfo{}, fmt.Errorf("init XTest extension: %w", err)
+	}
+
+	setup := xproto.Setup(conn)
+	if len(setup.Roots) == 0 {
+		conn.Close()
+		return nil, xproto.ScreenInfo{}, fmt.Errorf("no X11 screens")
+	}
+	return conn, setup.Roots[0], nil
+}
+
+// ensureConnLocked probes the X connection at most once per
+// x11InjectorLiveCheck and reconnects when it has died, refreshing the screen
+// and the keyboard mapping, both of which belong to the new server. A failed
+// reconnect keeps the dead connection and tries again on the next interval.
+// Caller must hold inputMu.
+func (x *X11InputInjector) ensureConnLocked() {
+	if time.Since(x.lastLiveCheck) < x11InjectorLiveCheck {
+		return
+	}
+	x.lastLiveCheck = time.Now()
+	if _, err := xproto.GetInputFocus(x.conn).Reply(); err == nil {
+		return
+	}
+
+	conn, screen, err := dialX11Input(x.display, x.cookieHex)
+	if err != nil {
+		log.Debugf("X11 input connection lost, reconnect to %s: %v", x.display, err)
+		return
+	}
+	x.conn.Close()
+	x.conn = conn
+	x.root = screen.Root
+	x.screen = &screen
+	x.cacheKeyboardMapping()
+	log.Infof("X11 input injector reconnected (display=%s)", x.display)
+}
+
 // InjectKey simulates a key press or release. keysym is an X11 KeySym.
 func (x *X11InputInjector) InjectKey(keysym uint32, down bool) {
 	x.inputMu.Lock()
 	defer x.inputMu.Unlock()
+	x.ensureConnLocked()
 	x.injectKeyLocked(keysym, down)
 }
 
@@ -111,6 +162,7 @@ func (x *X11InputInjector) injectKeyLocked(keysym uint32, down bool) {
 func (x *X11InputInjector) InjectKeyScancode(scancode, keysym uint32, down bool) {
 	x.inputMu.Lock()
 	defer x.inputMu.Unlock()
+	x.ensureConnLocked()
 
 	linuxKey := qemuScancodeToLinuxKey(scancode)
 	if linuxKey == 0 {
@@ -147,6 +199,7 @@ func (x *X11InputInjector) InjectPointer(buttonMask uint16, px, py, serverW, ser
 	// lastButtons and the write closes the sequence.
 	x.inputMu.Lock()
 	defer x.inputMu.Unlock()
+	x.ensureConnLocked()
 
 	// Scale to actual screen coordinates.
 	screenW := int(x.screen.WidthInPixels)
@@ -262,6 +315,18 @@ func (x *X11InputInjector) SetClipboard(text string) {
 // are skipped: a paste workflow for them needs Wayland-aware text input
 // or layout introspection that this path does not implement.
 func (x *X11InputInjector) TypeText(text string) {
+	x.inputMu.Lock()
+	x.ensureConnLocked()
+	restoreCaps := x.clearCapsLockLocked()
+	x.inputMu.Unlock()
+	if restoreCaps {
+		defer func() {
+			x.inputMu.Lock()
+			defer x.inputMu.Unlock()
+			x.toggleCapsLockLocked()
+		}()
+	}
+
 	const maxChars = 4096
 	count := 0
 	for _, r := range text {
@@ -303,6 +368,34 @@ func (x *X11InputInjector) typeRune(keysym uint32, shift bool) {
 	}
 }
 
+// xLockMask is the core-protocol modifier bit for Lock, which is Caps Lock on
+// every standard keymap.
+const xLockMask = 1 << 1
+
+// clearCapsLockLocked turns Caps Lock off when it is on and reports whether it
+// did, so the caller can put it back. The typed runes carry their case in the
+// Shift framing, and with Caps Lock engaged the server inverts it: pasted text
+// comes out with every letter's case flipped. Caller must hold inputMu.
+func (x *X11InputInjector) clearCapsLockLocked() bool {
+	reply, err := xproto.QueryPointer(x.conn, x.root).Reply()
+	if err != nil || reply.Mask&xLockMask == 0 {
+		return false
+	}
+	return x.toggleCapsLockLocked()
+}
+
+// toggleCapsLockLocked presses and releases Caps Lock, reporting whether the
+// keymap has one to press. Caller must hold inputMu.
+func (x *X11InputInjector) toggleCapsLockLocked() bool {
+	keycode := x.keysymToKeycode(0xffe5) // Caps_Lock
+	if keycode == 0 {
+		return false
+	}
+	xtest.FakeInput(x.conn, xproto.KeyPress, keycode, 0, x.root, 0, 0, 0)
+	xtest.FakeInput(x.conn, xproto.KeyRelease, keycode, 0, x.root, 0, 0, 0)
+	return true
+}
+
 func (x *X11InputInjector) resolveClipboardTool() {
 	for _, name := range []string{"xclip", "xsel"} {
 		path, err := exec.LookPath(name)