ironrdp-server: button mouse PDUs drop their coordinates — clicks land at the stale cursor position for clients that send a button with no preceding MOVE

[clintcan]

Summary

In ironrdp-server, a mouse button event delivered to RdpServerInputHandler::mouse carries no position, even though the wire PDU always does. As a result, a click lands wherever the server's cursor happened to be — not where the client actually clicked — for any client that sends a button PDU without a preceding move PDU.

Most clients (mstsc, Microsoft Remote Desktop / the desktop Windows App) send a separate MOVE PDU immediately before the button, so their clicks already land correctly and the bug is masked. It bites clients that send a tap as a single button PDU. A concrete repro is below.

Root cause

The MouseEvent variants for buttons are position-less unit variants — only Move/Scroll/RelMove carry coordinates:

crates/ironrdp-server/src/handler.rs:27

pub enum MouseEvent {
    Move { x: u16, y: u16 },
    RightPressed, RightReleased,
    LeftPressed,  LeftReleased,
    MiddlePressed, MiddleReleased,
    Button4Pressed, Button4Released,
    Button5Pressed, Button5Released,
    VerticalScroll { value: i16 },
    Scroll { x: i32, y: i32 },
    RelMove { x: i32, y: i32 },
}

The From<MousePdu> / From<MouseXPdu> conversions therefore discard the PDU's x_position/y_position whenever a button flag is set — the coordinates survive only when the PDU degenerates to a Move:

crates/ironrdp-server/src/handler.rs:149 (From<MousePdu>) and :176 (From<MouseXPdu>) both do

if flags.contains(LEFT_BUTTON) { /* -> LeftPressed / LeftReleased, x/y dropped */ }
else if flags.contains(RIGHT_BUTTON) { /* -> Right..., x/y dropped */ }
...
else { MouseEvent::Move { x, y } }   // position only survives here

The dispatch then forwards that position-less event straight through — every mouse arm calls handler.mouse(mouse.into()):

crates/ironrdp-server/src/server.rs (fast-path :1548/:1552/:1556, slow-path :1708/:1712).

So the position a button PDU carried is lost before the handler ever sees it, on both the regular (MousePdu) and extended (MouseXPdu) absolute-position paths.

Reproduction

• Client: Windows App on iOS/iPadOS, touch mode.
• A tap is sent as a single button PDU (button DOWN then UP at the tap coordinates) with no preceding MOVE PDU.
• Result: the click registers at the previous cursor position, not where the user tapped — taps feel "off" / "not working."
• A mouse-driven client (mstsc, desktop Windows App) is unaffected because it emits MOVE → button, so the position is already current when the button arrives.

Impact

Any ironrdp-server consumer serving a client that sends button-without-preceding-move gets mis-placed clicks. It's silent (no error) and easy to misattribute to coordinate scaling.

Suggested fixes

Two shapes, and I think the choice is a maintainer call because one touches the public API:

1. Minimal / non-breaking — synthesize a Move before the button at the point of conversion/dispatch (apply the PDU's x_position/y_position as a Move, then the button). No public-API change. Downside: it has to be repeated at each dispatch arm (or each From), it emits a redundant Move for the common move-then-click client, and the "which flags are position-less" knowledge ends up duplicated alongside the From impls.

2. Root-cause / breaking — give the button events a position. e.g. a single MouseEvent::Button { x, y, button, pressed }, or coordinate fields on the existing pressed/released variants. This fixes MousePdu, MouseXPdu (and structurally the MouseRelPdu button branches) uniformly in one place, with no synthetic event and no duplicated flag knowledge. The cost is a breaking change to the public MouseEvent enum / RdpServerInputHandler contract, which affects downstream server implementations — hence flagging it rather than sending a patch.

Downstream reference

Observed and worked around in a downstream ironrdp-server consumer (macrdp) via option (1) — a small helper that prepends the Move on the regular MousePdu path — which confirms the fix but also surfaced why (2) is cleaner (the same gap is reachable through MousePdu, MouseXPdu, and the MouseRel button branches, so (1) is a per-arm band-aid). Happy to open a PR in whichever shape you prefer.

Environment

• Upstream master @ 079b4842 (verified the code above against it).
• Not related to Mouse position looks like wrong when connect to windows on macos #507, which is the native client showing a wrong position to a Windows host (A-native-client); this is the server dropping an incoming button PDU's position.

[glamberson]

Confirmed against master: From<MousePdu> and From<MouseXPdu> both branch on the button flags before the Move fallthrough, so the PDU's x_position / y_position is dropped on both absolute paths. We hit the same thing in lamco-rdp-server with the iOS/iPadOS Windows App touch client.

I'd go with option 2. Option 1's synthesized Move duplicates the position-less-flag knowledge across three From impls and emits a redundant Move for the common move-then-click client. Putting the position on the button event fixes MousePdu and MouseXPdu in one place.

The unified variant you floated reads best:

MouseEvent::Button { x: u16, y: u16, button: MouseButton, pressed: bool }

It also collapses the button variants and simplifies downstream matches. Two things to settle with it:

• MouseRelPdu buttons have no absolute position (it accumulates from RelMove), so scope the absolute Button to the MousePdu / MouseXPdu paths and leave the relative branch position-less.
• MouseEvent isn't #[non_exhaustive]; since this breaks anyway, worth adding so future variant additions don't break downstream again.

Maintainer call given it's public API, but pre-crates.io is the cheap time to take it. Happy to validate whichever shape lands against the same iOS-touch repro.