Repository navigation
feat: drive redirection for the viewer, .rdp files and Windows hotplug - #2178
Marc-André Moreau (mamoreau-devolutions) wants to merge 9 commits into
Conversation
Parse the mstsc drive selection (`*`, `DynamicDrives`, drive letters separated by `;`) into `DriveRedirection`, with a nonempty `drivestoredirect` taking precedence over the legacy `redirectdrives:i:1` as in FreeRDP. `ConfigBuilder` exposes the selection on `RdpdrConfig::drives` for frontends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Allow redirected roots below a volume root (`C:\Users\name`), still opened without following reparse points. `with_logical_drives` registers device IDs 1-26 for drive letters and announces the selected letters present at each connection. `LogicalDriveWatcher` reports volumes mounted and removed from `WM_DEVICECHANGE` broadcasts and a `GetLogicalDrives` poll for network drives. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add `--drive NAME,PATH`, `--drives`, `--home-drive` and `--dynamic-drives`, honor the `.rdp` drive properties, attach the Windows RDPDR backend whenever a drive is requested, and forward hotplugged drives to the session. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add the selected drive letters to the per-connect RDPDR factory and watch for dynamic drives for the session's lifetime. Configured volumes now use device IDs from 27 so they cannot collide with drive letters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Active
github-actions
Bot
added
breaking-change
There was a problem hiding this comment.
🟡 Changes recommended
Root-path and gateway-policy bypasses remain, alongside hotplug delivery and readiness issues.
5 open issues
What changed in this PR
Adds drive redirection to the viewer and .rdp configuration, backed by Windows drive discovery and hotplug support.
Changes:
- Parses drive selections and exposes viewer CLI options.
- Supports directory roots, stable drive-letter IDs, and Windows hotplug notifications.
- Integrates daemon sessions and adds tests and documentation.
| File | Description |
|---|---|
| crates/ironrdp-viewer/src/main.rs | Attaches drive backend and retains watcher. |
| crates/ironrdp-viewer/src/lib.rs | Exports Windows drive support. |
| crates/ironrdp-viewer/src/drives.rs | Builds factories and forwards hotplug events. |
| crates/ironrdp-viewer/src/cli.rs | Adds drive selection options. |
| crates/ironrdp-viewer/README.md | Documents drive options and properties. |
| crates/ironrdp-testsuite-extra/tests/client/config.rs | Tests viewer drive configuration. |
| crates/ironrdp-testsuite-core/tests/cfg.rs | Tests parsing, precedence, and serialization. |
| crates/ironrdp-rdpdr-native/src/windows/mod.rs | Exports drive APIs. |
| crates/ironrdp-rdpdr-native/src/windows/logical_drives.rs | Implements discovery, selection, and hotplug watching. |
| crates/ironrdp-rdpdr-native/src/windows/handles.rs | Accepts directory roots and validates paths. |
| crates/ironrdp-rdpdr-native/src/windows/factory.rs | Registers and selects logical drives. |
| crates/ironrdp-rdpdr-native/src/lib.rs | Exposes Windows drive APIs publicly. |
| crates/ironrdp-rdpdr-native/README.md | Documents roots and hotplug integration. |
| crates/ironrdp-rdpdr-native/Cargo.toml | Enables required Windows APIs. |
| crates/ironrdp-daemon/src/daemon.rs | Integrates selections, watchers, and reserved IDs. |
| crates/ironrdp-daemon/README.md | Documents connect-time drive selection. |
| crates/ironrdp-client/src/rdp.rs | Updates test configuration literals. |
| crates/ironrdp-client/src/config.rs | Carries drive selection through configuration. |
| crates/ironrdp-cfg/src/lib.rs | Adds drive parsing and property accessors. |
Review effort: Balanced
- Keep OBJ_DONT_REPARSE on redirected-root opens even when the server asks for FILE_OPEN_REPARSE_POINT, so a junction above a directory root is not followed. - Ignore dynamic drive additions on a channel built without drive capability, so a queued event cannot bypass gateway drive policy. - Probe only the selected letters when a connection starts, and re-probe mapped but unready letters periodically so reconnected network shares appear. - Let the watcher callback report undelivered changes, which are retried on the next poll instead of being lost when the input queue is full. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Active
github-actions
Bot
added
automation-failed
- Signal watcher readiness before probing volumes, so starting a session with dynamic drives does not wait on unreachable network shares. - Treat a letter mapped before the session as connected later only when it is selected, unmapped and remapped, or announced by a volume arrival broadcast, so a preexisting share that becomes reachable is not redirected or reprobed. - Reject invalid drive roots in RedirectedDrive::new instead of failing at announcement time. - Canonicalize viewer named drive paths, which resolves junctions in the user profile path, and reject network paths, duplicate names or roots, and names reserved for drive letters. - Drop .rdp drive letters in the viewer when ironrdp_rdpdr:i:0 disables device redirection, and warn on other platforms for any drive request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Active
github-actions
Bot
added
risk/high
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR makes drive redirection configurable (drivestoredirect/redirectdrives) for the viewer and daemon, adds Windows logical-drive enumeration and hotplug watching, and renumbers daemon volumes to device IDs 27+. Verification confirms the two Windows frontends carry near-verbatim copies of the selection-to-factory wiring and the try_send forwarding callback (with a subtle Full/Closed retry contract), define the reserved named-drive device-ID base 27 twice while the complementary 1-26 letter layout lives in ironrdp-rdpdr-native, and diverge on display-name policy: the viewer rejects drive-letter names while the daemon does not. The daemon also ships a public DriveRedirection::contains_letter helper with no production caller. The claimed watcher startup arrival gap does not hold: in both frontends the watcher's initial snapshot precedes connection setup, so any selected volume present at snapshot time is announced by the factory's initial_drives() (selection intersected with currently …
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
github-actions
Bot
added
ai-reviewed/1
- Move the drive-selection-to-factory wiring and the hotplug forwarding callback into ironrdp_daemon::drives, used by both the daemon and the viewer. - Export FIRST_NAMED_DRIVE_DEVICE_ID and LogicalDriveSelection::from_letters from ironrdp-rdpdr-native, which owns the drive-letter device ID layout. - Reject configured drives with drive-letter names in with_logical_drives, so both frontends refuse a name like C: only when drive letters are redirected. - Remove the unused DriveRedirection::contains_letter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Active
github-actions
Bot
removed
the
needs-author-action
There was a problem hiding this comment.
Drive-redirection PR adding mstsc-syntax drivestoredirect parsing, viewer/daemon wiring, and a Windows hotplug watcher. The core design is sound: device-ID layout avoids collisions, hotplug additions are gated on drive capability, and the earlier duplication was consolidated in response to review. All six candidates were independently verified against pr-head and are published. Two are questions kept open: a nonempty drivestoredirect unconditionally overrides an explicit redirectdrives:i:0 opt-out (a deliberate, tested but protocol-semantics-sensitive precedence), and the watcher's live behavior against real broadcasts and hardware is unproven. The remaining candidates are a silent drop of unrecognized drivestoredirect entries and three low-severity simplifications (duplicated ineligible computation, single-use helper, fallible usize conversions).
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
| fn run_watcher( | ||
| selection: LogicalDriveSelection, | ||
| mut on_change: ChangeCallback, | ||
| ready: mpsc::SyncSender<io::Result<u32>>, | ||
| ) { | ||
| let window = match create_watcher_window() { | ||
| Ok(window) => window, | ||
| Err(error) => { | ||
| let _ = ready.send(Err(error)); | ||
| return; | ||
| } | ||
| }; | ||
| // SAFETY: `window` is a live window owned by this thread, and no timer callback is used. | ||
| if unsafe { SetTimer(Some(window), 1, LOGICAL_DRIVE_POLL_INTERVAL_MS, None) } == 0 { | ||
| warn!(error = %io::Error::last_os_error(), "Unable to start logical drive polling"); | ||
| } | ||
|
|
||
| // SAFETY: `GetLogicalDrives` takes no arguments. | ||
| let mut letters = unsafe { GetLogicalDrives() }; | ||
| // Letters mapped before the session are not "connected later" until they are unmapped or | ||
| // announced by a volume arrival broadcast. | ||
| let mut preexisting = LogicalDriveSet::from_mask(letters); | ||
| // Report readiness before probing volumes, which can block on unreachable network shares. | ||
| // SAFETY: `GetCurrentThreadId` takes no arguments. The thread has a message queue because it | ||
| // owns `window`, so `WM_QUIT` posted to this ID reaches the loop below. | ||
| let _ = ready.send(Ok(unsafe { GetCurrentThreadId() })); | ||
| let mut present = LogicalDriveSet::present(); | ||
| debug!(drives = ?present.iter().map(LogicalDrive::letter).collect::<String>(), "Watching logical drives"); | ||
|
|
||
| let mut polls = 0u32; | ||
| let mut undelivered = false; | ||
| let mut message = MSG::default(); | ||
| loop { | ||
| // SAFETY: `message` is a valid writable `MSG`. | ||
| let result = unsafe { GetMessageW(&mut message, None, 0, 0) }; | ||
| if result.0 == 0 { | ||
| break; | ||
| } | ||
| if result.0 == -1 { | ||
| warn!(error = %io::Error::last_os_error(), "Logical drive watcher message loop failed"); | ||
| break; | ||
| } | ||
|
|
||
| let current = match message.message { | ||
| WM_RESCAN_LOGICAL_DRIVES => { | ||
| // SAFETY: `GetLogicalDrives` takes no arguments. | ||
| letters = unsafe { GetLogicalDrives() }; | ||
| let arrived = LogicalDriveSet::from_mask(u32::try_from(message.wParam.0).unwrap_or(0)); | ||
| preexisting = preexisting | ||
| .intersection(LogicalDriveSet::from_mask(letters)) | ||
| .difference(arrived); | ||
| Some(LogicalDriveSet::present()) | ||
| } | ||
| WM_TIMER => { | ||
| // Probe every volume only when the letter mask changes or a change is undelivered: | ||
| // the probe can block on slow network drives, and media changes are broadcast. | ||
| // SAFETY: `GetLogicalDrives` takes no arguments. | ||
| let current = unsafe { GetLogicalDrives() }; | ||
| preexisting = preexisting.intersection(LogicalDriveSet::from_mask(current)); | ||
| polls = polls.wrapping_add(1); | ||
| if core::mem::replace(&mut letters, current) != current || undelivered { | ||
| Some(LogicalDriveSet::present()) | ||
| } else if polls.is_multiple_of(UNREADY_RETRY_POLLS) { | ||
| let ineligible = preexisting.difference(selection.get()); | ||
| let unready = LogicalDriveSet::from_mask(current) | ||
| .difference(present) | ||
| .difference(ineligible); | ||
| (!unready.is_empty()).then(|| present.union(unready.present_subset())) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| _ => { | ||
| // SAFETY: `message` was filled by `GetMessageW`. | ||
| unsafe { DispatchMessageW(&message) }; | ||
| None | ||
| } | ||
| }; | ||
| if let Some(current) = current { | ||
| let ineligible = preexisting.difference(selection.get()); | ||
| (present, undelivered) = report_changes(present, current, ineligible, &selection, &mut on_change); | ||
| } | ||
| } | ||
|
|
||
| // SAFETY: `window` is owned by this thread and has not been destroyed. | ||
| if let Err(error) = unsafe { DestroyWindow(window) } { | ||
| debug!(%error, "Unable to destroy the logical drive watcher window"); | ||
| } | ||
| } |
There was a problem hiding this comment.
[skeptical] Core hotplug behavior of the new Win32 watcher is unproven against live sessions and hardware — medium 🟠 ❓ — run_watcher combines WM_DEVICECHANGE broadcasts (queued via PostMessageW), a 2 s GetLogicalDrives poll, an every-8th-poll unready-volume retry, and preexisting-letter tracking feeding report_changes. Unit tests cover the pure state machine and one opt-in subst test covers arrival/removal locally, but the PR states no live session against a server and no physical USB stick was tested. Failure modes that only manifest live (broadcast timing, GetVolumeInformationW blocking on unreachable shares during retries, reconnect announcement ordering) are exactly what the changed lines implement; the interaction with real Windows behavior is asserted by documentation comments rather than evidence.
| let arrived = if usize::try_from(DBT_DEVICEARRIVAL).is_ok_and(|event| wparam.0 == event) { | ||
| // SAFETY: for `DBT_DEVICEARRIVAL`, `lparam` is null or points to a broadcast header that | ||
| // stays valid while this message is processed. | ||
| unsafe { arrived_volume_mask(lparam) } | ||
| } else { | ||
| 0 | ||
| }; | ||
| // Broadcasts are sent, not posted, so queue the rescan for the message loop. | ||
| // SAFETY: `window` is the live window receiving this message. | ||
| if let Err(error) = unsafe { | ||
| PostMessageW( | ||
| Some(window), | ||
| WM_RESCAN_LOGICAL_DRIVES, | ||
| WPARAM(usize::try_from(arrived).unwrap_or(0)), | ||
| LPARAM(0), | ||
| ) | ||
| } { | ||
| warn!(%error, "Unable to queue a logical drive rescan"); | ||
| } | ||
| return LRESULT(1); | ||
| } | ||
| // SAFETY: forwards the unmodified message parameters to the default window procedure. | ||
| unsafe { DefWindowProcW(window, message, wparam, lparam) } | ||
| } | ||
|
|
||
| /// Returns the drive letters announced by a `DBT_DEVICEARRIVAL` volume broadcast. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// `lparam` must be null or point to a valid `DEV_BROADCAST_HDR` followed by `dbch_size` bytes. | ||
| unsafe fn arrived_volume_mask(lparam: LPARAM) -> u32 { | ||
| let header = core::ptr::with_exposed_provenance::<DEV_BROADCAST_HDR>(usize::from_ne_bytes(lparam.0.to_ne_bytes())); | ||
| if header.is_null() { | ||
| return 0; | ||
| } | ||
| // SAFETY: the caller guarantees that a non-null `header` points to a broadcast header. | ||
| let header_value = unsafe { header.read_unaligned() }; | ||
| let is_volume = header_value.dbch_devicetype == DBT_DEVTYP_VOLUME | ||
| && usize::try_from(header_value.dbch_size).is_ok_and(|size| size_of::<DEV_BROADCAST_VOLUME>() <= size); |
There was a problem hiding this comment.
[code-compressor] usize::try_from(u32) with unwrap_or/is_ok_and where usize::from is lossless — low 🟡 — Line 505 (usize::try_from(DBT_DEVICEARRIVAL).is_ok_and(...)), line 518 (usize::try_from(arrived).unwrap_or(0)), and line 543 (usize::try_from(header_value.dbch_size).is_ok_and(...)) convert u32 to usize. This crate compiles only for Windows, where usize is at least 32 bits, so usize::from is infallible and behavior-preserving; the current form adds fallible plumbing whose failure arm (0) is unreachable and, for dbch_size, would silently skip the malformed-header check. Replacing the three sites with usize::from changes no reachable behavior.
github-actions
Bot
added
ai-reviewed/2
- An explicit redirectdrives:i:0 now disables drive redirection even when drivestoredirect still lists drives, so a stale list cannot be exposed. - Add DriveRedirection::unrecognized_entries; the viewer and daemon warn about drivestoredirect entries they ignore, such as FreeRDP folder paths. - Compute the watcher's ineligible drive set once per message and inline a single-use letter helper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Active
github-actions
Bot
removed
the
needs-author-action
There was a problem hiding this comment.
PR #2178 adds drive redirection for the viewer, .rdp properties and Windows hotplug. The core design is sound: DriveRedirection parsing follows mstsc semantics with documented precedence, the path hardening keeps OBJ_DONT_REPARSE and rejects reparse/UNC/stream forms, device IDs 1-26 for letters and 27+ for named drives are collision-checked, and the drive_hotplug_available gate prevents queued events from adding capability after negotiation. All six validated specialist candidates were independently verified in the head revision and are published: the connect path synchronously probes every selected volume with no timeout, the shared drive wiring lives in the daemon crate's public API with client-session types, --rpc silently drops the new drive flags, a hotplug-watcher startup failure aborts the daemon connect, and two small duplication spots remain in ironrdp-cfg and the daemon's error responses.
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
| use ironrdp_client::rdp::{RdpInputEvent, RdpInputSender}; | ||
| use ironrdp_rdpdr_native::{ | ||
| LogicalDriveChange, LogicalDriveSelection, LogicalDriveWatcher, WindowsRdpdrBackendFactory, | ||
| }; | ||
| use tokio::sync::mpsc::error::TrySendError; | ||
| use tracing::warn; |
There was a problem hiding this comment.
[skeptical] Frontend-shared drive wiring is exported from the daemon crate and depends on client-session types — low 🟡 — logical_drive_factory and watch_logical_drives are generic viewer/daemon helpers but live as public API in ironrdp-daemon and import ironrdp_client::rdp::{RdpInputEvent, RdpInputSender}; the viewer consumes them through its pre-existing daemon dependency. This inverts ownership: the daemon crate's public API carries client input-event types and serves viewer needs, so client-side changes to RdpInputSender ripple through it. No new crate edges and the alternatives (a new crate for ~100 lines, or client-session types in ironrdp-rdpdr-native) are worse, so this is a low-severity architectural note rather than a defect.
github-actions
Bot
added
ai-reviewed/3
- Announce selected drive letters from the logical-drive mask at connect, without probing volumes, so an unreachable share cannot stall session setup. - Reject viewer drive options with --rpc, where connect properties select drives. - Fall back to the drives present at connect when the hotplug watcher cannot start, instead of failing the session. - Share one drivestoredirect entry classifier between parse and unrecognized_entries. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Active
github-actions
Bot
added
needs-review
Drive redirection used to be reachable only through ActiveX and the daemon's fixed volume list. This makes it usable from the viewer and from standard
.rdpfiles, and adds automatic drive hotplug on Windows.Changes
ironrdp-cfg:DriveRedirectionparsesdrivestoredirectwith mstsc semantics (*= every drive including later ones,DynamicDrives,C:\;E:\). A nonemptydrivestoredirectwins; otherwiseredirectdrives:i:1selects every drive, as in FreeRDP. NewPropertySetExtaccessors for both keys plusdrive_redirection().ironrdp-client:RdpdrConfig::drivesandConfigBuilder::with_drive_redirection, filled from the property set. The client only carries the selection; frontends build the native factory.ironrdp-rdpdr-native(Windows):C:\Users\name), not onlyC:\. Paths with..,\?\/device/UNC prefixes,:or NUL are rejected, and the root is still opened withOBJ_DONT_REPARSE.WindowsRdpdrBackendFactory::with_logical_drivesregisters drive letters as device IDs 1–26 and announces the selected letters present when each connection (including reconnects) starts.LogicalDriveWatcherwatchesWM_DEVICECHANGEon a hidden top-level window, plus a 2 sGetLogicalDrivespoll for network drives, which are not broadcast. It reports arrivals and removals; arrivals are added to the shared selection.ironrdp-viewer:--drive NAME,PATH,--drives,--home-drive,--dynamic-drives; honors the.rdpkeys; attaches the RDPDR backend whenever any drive, smartcard or printer redirection is requested; forwards watcher changes asAddRdpdrDrive/RemoveRdpdrDrive.ironrdp-daemon: honorsdrivestoredirect/redirectdriveson connect (an explicitironrdp_rdpdr:i:0disables them) and keeps a watcher for the session's lifetime. Configured daemon volumes now use device IDs from 27, so they cannot collide with drive letters.Testing
cargo xtask check fmt -vandcargo xtask check lints -vpass..rdpparsing and precedence (testsuite-core/tests/cfg.rs), viewer CLI and.rdpmapping (testsuite-extra/tests/client/config.rs), root path validation, logical-drive factory, daemon factory.watcher_reports_subst_drive_arrival_and_removal(--ignored) maps a free letter withsubstand passes locally: arrival and removal are both reported.e2e::egfx_moves_onto_the_udp_tunnel_with_soft_synctimed out once under a parallel run and passed 3/3 alone; it is unrelated to this change.Merge note
This overlaps the printer redirection PR #2177 in
ironrdp-rdpdr-native(factory.rs, exports, README),ironrdp-clientconfig, and the viewer'scli.rs/main.rs. Whichever lands second needs a rebase. The device ID layout is: 0 smartcard, 1–26 drive letters, 27+ named drives, printers fromu32::MAX - 1down.🤖 Generated with Claude Code