Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add coverage for the backend callback and stale-state reset path.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes RDPDR handling when User Logged On arrives before Client ID Confirm.
Changes:
- Defers early logon notifications until client confirmation.
- Resets pending state on new server announcements.
- Adds regression coverage for deferred device handling.
| File | Summary |
|---|---|
crates/ironrdp-rdpdr/src/lib.rs |
Implements deferred logon handling and tests the sequence. |
| self.client_id_confirmed = true; | ||
| self.post_logon_devices_announced = announce_all_devices; | ||
| if core::mem::take(&mut self.user_logged_on_pending) { | ||
| messages.extend(self.handle_user_logged_on()?); |
There was a problem hiding this comment.
Added in bc64c6a: TrackingBackend now counts handle_user_logged_on calls. The test asserts zero calls while the logon is deferred and exactly one after Client ID Confirm. A second test, server_announce_discards_a_deferred_user_logged_on, checks that a new Server Announce drops a pending logon (no backend call, no post-logon announcement). I checked that each test fails if the corresponding line in the fix is removed.
|
Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes. |
There was a problem hiding this comment.
PR #2079 defers an RDPDR Server User Logged On that arrives before Server Client ID Confirm (as Windows 11 sends after re-announcing the channel) instead of terminating the connection with a PDU error. Verified in pr-head: a single user_logged_on_pending flag is set in handle_user_logged_on when client_id_confirmed is false, cleared on Server Announce in lockstep with the other sequence state, and consumed once via core::mem::take at the end of handle_client_id_confirm, so the backend callback and post-logon device announcements still run after confirmation. This matches MS-RDPEFS sequencing and the FreeRDP-precedent claim. Two new tests cover deferral and discard-on-reannounce, and the implementation is minimal with no compression concerns. The only valid specialist finding is a low-severity state/message coupling: within handle_client_id_confirm, device announcements are registered in pending_device_announcements and their messages built before the deferred backend handle_user_logge…
| if core::mem::take(&mut self.user_logged_on_pending) { | ||
| messages.extend(self.handle_user_logged_on()?); | ||
| } |
There was a problem hiding this comment.
[skeptical] Backend error during deferred User Logged On drops already-built device announcements — low 🟡 — The deferral folds User Logged On handling into the Client ID Confirm process() call. By the time the flushed backend.handle_user_logged_on runs, announce_devices has already registered the device IDs in pending_device_announcements and built the ClientDeviceListAnnounce messages. If the backend callback returns Err, the error propagates out of process() and the built messages are never returned to the transport, leaving pending_device_announcements stale relative to what the server actually received; the affected devices are never activated and re-announcing them later trips the announced-more-than-once check. Not a regression for the misordered flow (it was previously fatal) and requires a backend error at exactly this point; detaching the announcement messages before the flush or registering them only after the deferred call would avoid the coupling.
After logon, Windows re-announces the RDPDR channel and sends User Logged On before the new capability request and Client ID Confirm. Treating that as a protocol error ended the whole session a few seconds after login. Remember the logon and handle it once the client ID is confirmed, as FreeRDP tolerates the same order.
Count RdpdrBackend::handle_user_logged_on calls in the tracking backend: the deferred logon must invoke it exactly once after Client ID Confirm, and a new Server Announce must discard a logon that is still pending.
bc64c6a to
20c88f6
Compare
There was a problem hiding this comment.
🟢 PR #2079 is a minimal, well-scoped fix in crates/ironrdp-rdpdr/src/lib.rs: a Server User Logged On PDU arriving before Server Client ID Confirm no longer terminates the connection. Instead, a single user_logged_on_pending bool records the event; handle_client_id_confirm replays it via core::mem::take after setting client_id_confirmed = true, so the backend's handle_user_logged_on and post-logon device announcements run exactly once with the same semantics as the non-deferred path (the RDP51 post_logon_devices_announced guard in announce_post_logon_devices behaves identically). Resetting the flag in handle_server_announce is consistent with the existing full channel reset there, and Windows re-sends UserLoggedon per re-announce cycle, so no event is lost; repeated early PDUs coalesce harmlessly since the PDU is payload-free. All modified lines verified in pr-head: the field/init/reset, the deferred branch in handle_user_logged_on, the replay block in handle_client_id_confirm, and two t…

After the user logs in, Windows re-announces the RDPDR channel and sends Server User Logged On before the new Server Core Capability Request and Server Client ID Confirm.
Rdpdrtreated that order as a protocol error, so a connection to a Windows 11 host ended a few seconds after login with:Observed sequence (
ironrdp_rdpdr=debug):Rdpdrnow remembers a User Logged On that arrives before the Client ID Confirm and handles it right after the confirmation, so the backend'shandle_user_logged_onand the post-logon device announcement still run once the client ID is known. A new Server Announce clears the pending flag. FreeRDP accepts the same order: its state check allowsPAKID_CORE_USER_LOGGEDONbeforePAKID_CORE_CLIENTID_CONFIRM.Validation
user_logged_on_before_client_id_confirm_is_deferred: an early User Logged On yields no messages, the following Client ID Confirm announces the configured drive, and drive IRPs are dispatched afterwards.cargo test -p ironrdp-rdpdr,cargo clippy -p ironrdp-rdpdr --all-targets -- -D warnings,cargo fmt --check.