From 12040b1be0ff5ead38d0290c61922bc25e7ee922 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Sat, 29 Aug 2026 17:25:02 +0200 Subject: [PATCH] Refuse an ambiguous X display and settle the approval timeout race under one claim Claude-Session: https://claude.ai/code/session_01QKDYfH4WKLbpNQHccpVo3P --- client/internal/approval/broker.go | 86 +++++++++++++++---- client/internal/approval/broker_test.go | 74 ++++++++++++++++ client/proto/daemon.pb.go | 7 +- client/proto/daemon.proto | 7 +- client/ui/services/approval.go | 11 +-- client/vnc/server/capture_x11.go | 90 +++++++++++++++----- client/vnc/server/capture_x11_select_test.go | 88 ++++++++++++------- client/vnc/server/session_cursor.go | 38 ++++++--- 8 files changed, 307 insertions(+), 94 deletions(-) diff --git a/client/internal/approval/broker.go b/client/internal/approval/broker.go index b2cf8b5bf..cac0d8430 100644 --- a/client/internal/approval/broker.go +++ b/client/internal/approval/broker.go @@ -128,19 +128,27 @@ type Decision struct { ViewOnly bool } +// pendingRequest is one in-flight prompt. resolved records that somebody has +// already claimed it, so the user's decision and the caller giving up race for +// the same entry under Broker.mu and exactly one of them wins. +type pendingRequest struct { + resp chan Decision + resolved bool +} + // Broker holds in-flight approval requests keyed by request ID. type Broker struct { pub EventPublisher mu sync.Mutex - pending map[string]chan Decision + pending map[string]*pendingRequest } // New returns a broker that publishes prompts via pub. func New(pub EventPublisher) *Broker { return &Broker{ pub: pub, - pending: make(map[string]chan Decision), + pending: make(map[string]*pendingRequest), } } @@ -161,7 +169,7 @@ func (b *Broker) Request(ctx context.Context, p Prompt) (Decision, error) { resp := make(chan Decision, 1) b.mu.Lock() - b.pending[id] = resp + b.pending[id] = &pendingRequest{resp: resp} b.mu.Unlock() defer b.dropPending(id) @@ -195,17 +203,61 @@ func (b *Broker) Request(ctx context.Context, p Prompt) (Decision, error) { select { case d := <-resp: - if !d.Accept { - return zero, ErrDenied - } - return d, nil + return decisionResult(d) case <-timer.C: + if d, answered := b.giveUp(id, resp); answered { + return decisionResult(d) + } return zero, ErrTimeout case <-ctx.Done(): + if d, answered := b.giveUp(id, resp); answered { + return decisionResult(d) + } return zero, ctx.Err() } } +// giveUp abandons the request and reports whether the user's decision landed +// first, in which case it is returned and must be honoured. +// +// Respond and this path claim the same entry under the same lock, so exactly +// one of them wins. Without that claim, a click arriving as the timer fires +// would be told it matched a live prompt while this caller had already denied +// the connection: the user would see their accept confirmed and the session +// dropped anyway. +func (b *Broker) giveUp(id string, resp <-chan Decision) (Decision, bool) { + if b.claim(id) { + return Decision{}, false + } + // Respond claimed the entry and sent while still holding the lock, so the + // decision is already buffered and this receive cannot block. + return <-resp, true +} + +// claim marks the request resolved so nothing else can take it, reporting +// whether the caller got there first. Callers must not already hold b.mu. +func (b *Broker) claim(id string) bool { + b.mu.Lock() + defer b.mu.Unlock() + + p, ok := b.pending[id] + if !ok || p.resolved { + return false + } + p.resolved = true + delete(b.pending, id) + return true +} + +// decisionResult maps a decision the user actually made onto the Request +// contract: a deny is an error, an accept carries the view-only flag back. +func decisionResult(d Decision) (Decision, error) { + if !d.Accept { + return Decision{}, ErrDenied + } + return d, nil +} + // Respond delivers the user's decision for id. Returns true when a pending // request matched and was woken, false when id was unknown or already done. func (b *Broker) Respond(id string, d Decision) bool { @@ -213,18 +265,18 @@ func (b *Broker) Respond(id string, d Decision) bool { return false } b.mu.Lock() - ch, ok := b.pending[id] - if ok { - delete(b.pending, id) - } - b.mu.Unlock() - if !ok { + defer b.mu.Unlock() + + p, ok := b.pending[id] + if !ok || p.resolved { return false } - select { - case ch <- d: - default: - } + p.resolved = true + delete(b.pending, id) + // The channel is buffered and claimed exactly once, so this never blocks. + // Sent under the lock so a Request that loses the claim race finds the + // decision already waiting instead of racing this send. + p.resp <- d return true } diff --git a/client/internal/approval/broker_test.go b/client/internal/approval/broker_test.go index 7b0c114e3..0310f40e1 100644 --- a/client/internal/approval/broker_test.go +++ b/client/internal/approval/broker_test.go @@ -458,3 +458,77 @@ func TestRequestViewOnly(t *testing.T) { t.Fatal("view-only request did not resolve") } } + +// TestGiveUpAndRespondAreMutuallyExclusive pins the claim that makes +// RespondApprovalResponse.matched truthful: for one request, exactly one of +// "the user answered" and "the caller gave up" wins, in either arrival order. +// Without it a click landing as the timer fires is told it matched a live +// prompt while the connection has already been denied. +func TestGiveUpAndRespondAreMutuallyExclusive(t *testing.T) { + t.Run("caller gives up first", func(t *testing.T) { + b := New(&fakePublisher{subscribers: true}) + resp := make(chan Decision, 1) + b.pending["req"] = &pendingRequest{resp: resp} + + d, answered := b.giveUp("req", resp) + assert.False(t, answered, "nothing had answered yet") + assert.False(t, d.Accept) + assert.False(t, b.Respond("req", Decision{Accept: true}), + "a response arriving after the caller gave up must not report a match") + }) + + t.Run("user answers first", func(t *testing.T) { + b := New(&fakePublisher{subscribers: true}) + resp := make(chan Decision, 1) + b.pending["req"] = &pendingRequest{resp: resp} + + require.True(t, b.Respond("req", Decision{Accept: true, ViewOnly: true})) + + d, answered := b.giveUp("req", resp) + require.True(t, answered, "giving up after the user answered must surface their decision") + assert.True(t, d.Accept) + assert.True(t, d.ViewOnly, "the view-only grant must survive the race") + }) +} + +// TestRespondRacingTimeoutIsConsistent aims a Respond at the deadline and +// requires the reported match to agree with the outcome the caller saw. A +// stress check on the invariant above rather than a reproduction: the losing +// window is a few instructions wide, so this does not reliably fail without the +// claim, but it does catch an outcome pair that should never occur. +func TestRespondRacingTimeoutIsConsistent(t *testing.T) { + defaultTimeout(t, 20*time.Millisecond) + defer defaultTimeout(t, DefaultTimeout) + + for i := 0; i < 50; i++ { + pub := &fakePublisher{subscribers: true} + b := New(pub) + + done := make(chan error, 1) + go func() { + done <- requestErr(b, context.Background(), Prompt{Kind: KindVNC}) + }() + id := waitForRequestID(t, pub) + + matchedCh := make(chan bool, 1) + go func() { + // Aim at the deadline so the claim lands on either side of it. + time.Sleep(20 * time.Millisecond) + matchedCh <- b.Respond(id, Decision{Accept: true}) + }() + + var err error + select { + case err = <-done: + case <-time.After(2 * time.Second): + t.Fatal("prompt neither resolved nor timed out") + } + matched := <-matchedCh + + if matched { + require.NoErrorf(t, err, "iteration %d: Respond reported the prompt matched, so the accept must be honoured", i) + continue + } + require.ErrorIsf(t, err, ErrTimeout, "iteration %d: Respond reported no match, so the request must have timed out", i) + } +} diff --git a/client/proto/daemon.pb.go b/client/proto/daemon.pb.go index c60f0e3c6..c19de50f8 100644 --- a/client/proto/daemon.pb.go +++ b/client/proto/daemon.pb.go @@ -7271,9 +7271,10 @@ func (x *RespondApprovalRequest) GetViewOnly() bool { type RespondApprovalResponse struct { state protoimpl.MessageState `protogen:"open.v1"` // matched is true when request_id named a prompt that was still waiting. - // False means the prompt had already been answered, or had expired and the - // connection was denied: the click had no effect, and the UI should say so - // rather than reporting the outcome the user picked. + // False covers three cases and does not distinguish them: request_id was + // never known, the prompt had already been answered, or it had expired and + // the connection was denied. In all three this call changed nothing, so the + // UI must not report the outcome the user picked as having taken effect. Matched bool `protobuf:"varint,1,opt,name=matched,proto3" json:"matched,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache diff --git a/client/proto/daemon.proto b/client/proto/daemon.proto index 291721ea5..2dd7f3ebe 100644 --- a/client/proto/daemon.proto +++ b/client/proto/daemon.proto @@ -1127,8 +1127,9 @@ message RespondApprovalRequest { message RespondApprovalResponse { // matched is true when request_id named a prompt that was still waiting. - // False means the prompt had already been answered, or had expired and the - // connection was denied: the click had no effect, and the UI should say so - // rather than reporting the outcome the user picked. + // False covers three cases and does not distinguish them: request_id was + // never known, the prompt had already been answered, or it had expired and + // the connection was denied. In all three this call changed nothing, so the + // UI must not report the outcome the user picked as having taken effect. bool matched = 1; } diff --git a/client/ui/services/approval.go b/client/ui/services/approval.go index 05348681a..6c5fff64d 100644 --- a/client/ui/services/approval.go +++ b/client/ui/services/approval.go @@ -38,11 +38,12 @@ func (a *Approval) Respond(ctx context.Context, requestID string, accept, viewOn return err } if !resp.GetMatched() { - // Not an error to the caller: the dialog closes either way, and the - // connection has already been denied by the broker's timeout. Logged so - // a report of "I clicked accept and it still disconnected" has a record - // showing the click landed after the prompt had expired. - log.Infof("approval %s was no longer pending; the daemon had already answered it", requestID) + // Not an error to the caller: the dialog closes either way. The daemon + // does not say which of "already answered", "expired" or "never known" + // applies, so neither does this line; it exists so a report of "I + // clicked and nothing happened" has a record that the click reached a + // prompt that was no longer waiting. + log.Infof("approval %s was no longer pending; this response changed nothing", requestID) } return nil } diff --git a/client/vnc/server/capture_x11.go b/client/vnc/server/capture_x11.go index ac6b287af..43f83a73c 100644 --- a/client/vnc/server/capture_x11.go +++ b/client/vnc/server/capture_x11.go @@ -65,15 +65,32 @@ func detectX11Display() { return } - // Try /proc first (Linux), then ps fallback (FreeBSD and others). - if detectX11FromProc() { + // Try /proc first (Linux), then the socket scan (FreeBSD and others). + switch detectX11FromProc() { + case x11Detected: return - } - if detectX11FromSockets() { + case x11Ambiguous: + // /proc listed X servers and none of them could be shown to be the one + // on the console. Falling through to the socket scan would only make + // the same guess with less information, so stop here: no capture at all + // beats capturing a session that may not be the caller's. return + case x11NotFound: } + detectX11FromSockets() } +// x11Detection is the outcome of one display-detection pass. Ambiguous is +// deliberately distinct from NotFound: it means candidates existed and were +// refused, which must not be retried by a less discriminating fallback. +type x11Detection int + +const ( + x11NotFound x11Detection = iota + x11Detected + x11Ambiguous +) + // xorgCandidate is one X server found in /proc, with the pieces that decide // whether it is the one a remote user should be attached to. type xorgCandidate struct { @@ -94,10 +111,10 @@ type xorgCandidate struct { // delivering the remote user's input to it. /sys/class/tty/tty0/active names the // session actually on the console and X records its own VT in argv, so the two // can be matched. -func detectX11FromProc() bool { +func detectX11FromProc() x11Detection { entries, err := os.ReadDir("/proc") if err != nil { - return false + return x11NotFound } var candidates []xorgCandidate @@ -117,40 +134,69 @@ func detectX11FromProc() bool { candidates = append(candidates, xorgCandidate{display: display, auth: auth, vt: parseXorgVT(args)}) } - best, ok := pickXorgCandidate(candidates, activeVT()) - if !ok { - return false + best, outcome := pickXorgCandidate(candidates, activeVT()) + if outcome != x11Detected { + return outcome } setDisplayEnv(best.display, best.auth) - return true + return x11Detected } -// pickXorgCandidate chooses which X server to attach to: the one on the active -// VT when that is known, and otherwise the lowest display number, so the choice -// is at least stable across runs instead of following readdir order. -func pickXorgCandidate(candidates []xorgCandidate, activeVT int) (xorgCandidate, bool) { +// pickXorgCandidate chooses which X server to attach to. +// +// The one on the active VT wins outright. Failing that the choice has to be +// unambiguous, because guessing wrong means handing a remote user another local +// user's screen and keyboard: a single candidate is taken, and anything else is +// refused. The lowest display number only breaks ties among servers that all +// record no VT, where there is no active-session signal to go on at all. +func pickXorgCandidate(candidates []xorgCandidate, activeVT int) (xorgCandidate, x11Detection) { if len(candidates) == 0 { - return xorgCandidate{}, false + return xorgCandidate{}, x11NotFound } if activeVT > 0 { for _, c := range candidates { if c.vt == activeVT { - return c, true + return c, x11Detected } } } + if len(candidates) == 1 { + return candidates[0], x11Detected + } - best := candidates[0] - for _, c := range candidates[1:] { + // Several servers and no way to tell which is on the console. One that + // records a VT is on some seat other than the active one, so it is a + // session this connection has no business showing. + var vtless []xorgCandidate + for _, c := range candidates { + if c.vt < 0 { + vtless = append(vtless, c) + } + } + if len(vtless) == 0 { + log.Warnf("found %d X servers, all on virtual terminals and none of them the active one (%s); not attaching to any", + len(candidates), describeActiveVT(activeVT)) + return xorgCandidate{}, x11Ambiguous + } + + best := vtless[0] + for _, c := range vtless[1:] { if displayNumber(c.display) < displayNumber(best.display) { best = c } } - if len(candidates) > 1 { - log.Warnf("found %d X servers and none on the active VT (%d); attaching to DISPLAY=%s", - len(candidates), activeVT, best.display) + log.Warnf("found %d X servers and none on the active VT (%s); attaching to the lowest VT-less display %s", + len(candidates), describeActiveVT(activeVT), best.display) + return best, x11Detected +} + +// describeActiveVT renders the active VT for a log line, keeping "we could not +// read it" distinct from a VT number that simply matched nothing. +func describeActiveVT(vt int) string { + if vt <= 0 { + return "unknown" } - return best, true + return "tty" + strconv.Itoa(vt) } // activeVT reports the virtual terminal currently on the console, or -1 when diff --git a/client/vnc/server/capture_x11_select_test.go b/client/vnc/server/capture_x11_select_test.go index eba094692..b9cd183f6 100644 --- a/client/vnc/server/capture_x11_select_test.go +++ b/client/vnc/server/capture_x11_select_test.go @@ -11,25 +11,26 @@ import ( // TestPickXorgCandidate covers which X server a service-mode capturer attaches // to when several are running. Picking the wrong one shows a remote user -// another local user's screen, so the active VT wins whenever it is known. +// another local user's screen and delivers their input to it, so the active VT +// wins whenever it is known and an unresolvable choice is refused outright. func TestPickXorgCandidate(t *testing.T) { tests := []struct { - name string - candidates []xorgCandidate - activeVT int - want string - wantOK bool + name string + candidates []xorgCandidate + activeVT int + want string + wantOutcome x11Detection }{ { - name: "no X server found", - wantOK: false, + name: "no X server found", + wantOutcome: x11NotFound, }, { - name: "single server is used whatever its VT", - candidates: []xorgCandidate{{display: ":3", vt: 9}}, - activeVT: 2, - want: ":3", - wantOK: true, + name: "single server is used whatever its VT", + candidates: []xorgCandidate{{display: ":3", vt: 9}}, + activeVT: 2, + want: ":3", + wantOutcome: x11Detected, }, { name: "active VT wins over the lower display number", @@ -37,19 +38,37 @@ func TestPickXorgCandidate(t *testing.T) { {display: ":0", vt: 2}, {display: ":1", vt: 7}, }, - activeVT: 7, - want: ":1", - wantOK: true, + activeVT: 7, + want: ":1", + wantOutcome: x11Detected, }, { - name: "unknown active VT falls back to the lowest display", + name: "several servers on other VTs are refused, not guessed at", candidates: []xorgCandidate{ - {display: ":1", vt: 7}, {display: ":0", vt: 2}, + {display: ":1", vt: 3}, }, - activeVT: -1, - want: ":0", - wantOK: true, + activeVT: 7, + wantOutcome: x11Ambiguous, + }, + { + name: "unknown active VT does not license a guess between seats", + candidates: []xorgCandidate{ + {display: ":0", vt: 2}, + {display: ":1", vt: 3}, + }, + activeVT: -1, + wantOutcome: x11Ambiguous, + }, + { + name: "a VT-less server is preferred over one on an inactive VT", + candidates: []xorgCandidate{ + {display: ":0", vt: 2}, + {display: ":99", vt: -1}, + }, + activeVT: 7, + want: ":99", + wantOutcome: x11Detected, }, { name: "display numbers compare numerically, not lexically", @@ -57,9 +76,9 @@ func TestPickXorgCandidate(t *testing.T) { {display: ":10", vt: -1}, {display: ":2", vt: -1}, }, - activeVT: -1, - want: ":2", - wantOK: true, + activeVT: -1, + want: ":2", + wantOutcome: x11Detected, }, { name: "an unparseable display is only picked when it is alone", @@ -67,17 +86,18 @@ func TestPickXorgCandidate(t *testing.T) { {display: ":bogus", vt: -1}, {display: ":4", vt: -1}, }, - activeVT: -1, - want: ":4", - wantOK: true, + activeVT: -1, + want: ":4", + wantOutcome: x11Detected, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got, ok := pickXorgCandidate(tc.candidates, tc.activeVT) - require.Equal(t, tc.wantOK, ok) - if !tc.wantOK { + got, outcome := pickXorgCandidate(tc.candidates, tc.activeVT) + require.Equal(t, tc.wantOutcome, outcome) + if tc.wantOutcome != x11Detected { + assert.Empty(t, got.display, "a refused choice must not leak a display") return } assert.Equal(t, tc.want, got.display) @@ -92,3 +112,11 @@ func TestParseXorgVT(t *testing.T) { assert.Equal(t, -1, parseXorgVT([]string{"/usr/bin/Xvfb", ":99", "-screen", "0", "1920x1080x24"})) assert.Equal(t, -1, parseXorgVT([]string{"/usr/lib/Xorg", ":0", "vtconsole"})) } + +// TestDescribeActiveVT keeps "we could not read the active VT" legible in the +// log line, rather than printing a -1 that reads like a real terminal. +func TestDescribeActiveVT(t *testing.T) { + assert.Equal(t, "unknown", describeActiveVT(-1)) + assert.Equal(t, "unknown", describeActiveVT(0)) + assert.Equal(t, "tty7", describeActiveVT(7)) +} diff --git a/client/vnc/server/session_cursor.go b/client/vnc/server/session_cursor.go index 0971d8673..61e10232a 100644 --- a/client/vnc/server/session_cursor.go +++ b/client/vnc/server/session_cursor.go @@ -30,13 +30,17 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte { // cannot produce one, or the sprite failed to encode. Say which, once per // session, so the next report of a missing cursor names its own cause. if !supported || failed || composite { - s.logCursorSkip("no cursor rect: client_requested=%v source_failed=%v compositing=%v", + // The three flags form the key so a client that negotiates the encoding + // mid-session, or a source that starts failing, still gets a line: eight + // combinations at most. + s.logCursorSkip(fmt.Sprintf("state:%t%t%t", supported, failed, composite), + "no cursor rect: client_requested=%v source_failed=%v compositing=%v", supported, failed, composite) return nil } src, ok := s.capturer.(cursorSource) if !ok { - s.logCursorSkip("no cursor rect: capturer %T reports no cursor source", s.capturer) + s.logCursorSkip("no-source", "no cursor rect: capturer %T reports no cursor source", s.capturer) return nil } img, hotX, hotY, serial, err := src.Cursor() @@ -48,7 +52,7 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte { return nil } if img == nil { - s.logCursorSkip("no cursor rect: capturer returned no sprite") + s.logCursorSkip("no-sprite", "no cursor rect: capturer returned no sprite") return nil } if serial == lastSerial { @@ -57,7 +61,10 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte { buf := encodeCursorPseudoRect(img, hotX, hotY, pf) if buf == nil { b := img.Bounds() - s.logCursorSkip("no cursor rect: sprite %dx%d stride=%d pix=%d could not be encoded", + // Fixed key, varying detail: a source returning junk dimensions must not + // be able to mint a new throttle entry per frame. + s.logCursorSkip("encode-failed", + "no cursor rect: sprite %dx%d stride=%d pix=%d could not be encoded", b.Dx(), b.Dy(), img.Stride, len(img.Pix)) return nil } @@ -83,25 +90,28 @@ func (s *session) pendingCursorRect(pf clientPixelFormat) []byte { } // logCursorSkip reports why this update carries no cursor rect, once per -// distinct line. pendingCursorRect runs per framebuffer update, so an -// unthrottled log would flood; throttling once per session instead would let -// whichever reason came first hide every later one, and the reasons do change -// as the client negotiates encodings and the cursor source starts failing. -func (s *session) logCursorSkip(format string, args ...any) { - msg := fmt.Sprintf(format, args...) - +// reason. pendingCursorRect runs per framebuffer update, so an unthrottled log +// would flood; throttling once per session instead would let whichever reason +// came first hide every later one, and the reasons do change as the client +// negotiates encodings and the cursor source starts failing. +// +// reason keys the throttle and must come from a fixed set, never from the +// message: a cursor source handing back varying junk dimensions would otherwise +// mint a new key per frame, growing the map for the life of the session and +// logging every one of them. +func (s *session) logCursorSkip(reason, format string, args ...any) { s.cursorSkipMu.Lock() if s.cursorSkipSeen == nil { s.cursorSkipSeen = make(map[string]struct{}) } - _, seen := s.cursorSkipSeen[msg] - s.cursorSkipSeen[msg] = struct{}{} + _, seen := s.cursorSkipSeen[reason] + s.cursorSkipSeen[reason] = struct{}{} s.cursorSkipMu.Unlock() if seen { return } - s.log.Debug(msg) + s.log.Debugf(format, args...) } // maxCursorDim caps the cursor sprite size we'll encode. Real platform