feat(agent): add clipboard file transfer - #1899
Greg Lamberson (glamberson) wants to merge 4 commits into
Conversation
Extends the daemon clipboard-get/clipboard-set operations added in Devolutions#1877 to files: clipboard-set-files offers one or more local files to the remote via the CLIPRDR file-list mechanism (FileGroupDescriptorW), clipboard-list-files lists what the remote currently offers (metadata only, matching the delayed-render model MS-RDPECLIP itself uses), and clipboard-get-file fetches one file's full contents by its position in that list. Independent of Devolutions#1878 (HTML support, still open): This branches directly off master rather than off Devolutions#1878, and touches none of its code. Both extend the ClipboardContent enum Devolutions#1877 introduced, so whichever of the two merges first will need the other rebased, same as the earlier Devolutions#1877/Devolutions#1878 rebase. Extends ClipboardContent with a Files variant (wire-shaped metadata, symmetric for what is offered locally and what the remote last advertised); a parallel local_file_paths list on ClipboardState carries the on-disk paths a local offer maps to, since FileDescriptor itself carries no filesystem path and file transfer is a structurally different CLIPRDR mechanism from the FormatDataRequest path text/image/HTML share. Advertises STREAM_FILECLIP_ENABLED and CAN_LOCK_CLIPDATA in client_capabilities. Cliprdr::initiate_file_copy and request_file_contents both hard-error when file transfer was not negotiated, and that error is session-fatal by the time it reaches ironrdp-client's dispatcher, so the daemon tracks the negotiated capabilities from on_process_negotiated_capabilities and checks them before sending either message, failing cleanly at the IPC layer instead. Cliprdr already owns the whole clipboard-lock lifecycle for file transfer (snapshotting the local file list on an incoming LockData, auto-locking when a remote file list arrives, timeout-driven expiry); this backend's on_lock/on_unlock stay informational. on_outgoing_locks_expired is not: It aborts an in-progress clipboard-get-file fetch bound to the expiring lock rather than let it continue issuing requests against a remote clipboard that has since changed underneath it. on_remote_copy clears an in-progress fetch the same way when the remote clipboard changes again mid-fetch, so a clipboard-get-file caller left waiting is told the fetch was interrupted instead of silently blocked until its own timeout. clipboard-get-file drives ironrdp_cliprdr::chunked_fetch::ChunkedFetch to completion inside the daemon, bounded at MAX_CLIPBOARD_FILE_BYTES (derived from the RPC transport's own frame limit, the same reasoning MAX_CLIPBOARD_IMAGE_BYTES already uses) before issuing any wire request, not just before framing the IPC response. The IPC handler and the CLIPRDR backend's on_file_contents_response coordinate through a per-session Notify, the same wait-and-recheck shape rail_wait already uses for RAIL evidence, including re-checking state once more after a timeout races against a same-instant completion rather than assuming a timeout means nothing arrived. Serving a remote's request for a file we offered (on_file_contents_request) reads from the local path clipboard-set-files recorded, seeking to the requested range and capping a single RANGE response at 4 MiB regardless of what the peer's own cbRequested asks for: MS-RDPECLIP defines cbRequested as an upper bound on what a responder may return, not a guarantee, and ChunkedFetch on the requesting side already handles a shorter-than-asked response by issuing another RANGE request for the remainder. This read happens synchronously on the session's own current-thread runtime (Cliprdr callbacks are not async), bounded per response by the same 4 MiB cap: A slow local read stalls that session briefly rather than the whole daemon, but it is a real tradeoff against a fully async read path, disclosed rather than silently accepted. Directory entries are rejected, not silently mishandled: clipboard-set-files refuses a directory path outright rather than support recursive folder copy (out of scope here), and a remote directory entry (real Windows Explorer folder copies advertise one per subfolder, mixed in with the files) is shown as such in clipboard-list-files and refused by clipboard-get-file rather than attempt a byte fetch against it. New Request::ClipboardSetFiles/ClipboardListFiles/ClipboardGetFile and Payload::ClipboardFileList/ClipboardFile wire variants, plus the shared ClipboardFileEntry metadata type, bounded at MAX_CLIPBOARD_FILE_BYTES and MAX_CLIPBOARD_FILE_LIST_ENTRIES on wire decode. Extends ironrdp-activex's exhaustive match on Request with the same treatment as the other clipboard operations. Extends the wire round-trip and Debug-redaction test coverage added in Devolutions#1877 to the three new variants. cargo xtask check fmt/lints/tests/typos/locks all pass. Not yet verified against a live remote client, same as Devolutions#1877/Devolutions#1878.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical deadlock and stream-correlation issues, plus multiple moderate safety concerns, block approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds CLIPRDR clipboard file listing, upload, and bounded download support across the daemon, RPC, and CLI.
Changes:
- Adds file-transfer RPC types, limits, commands, and tests.
- Implements chunked fetching, local file serving, locking, and capability checks.
- Adds CLI help and ActiveX rejection handling.
Review findings:
- Critical: Fixed stream ID reuse can associate delayed responses with newer fetches in
clipboard.rs. - Critical: Inconsistent mutex ordering in
daemon.rscan deadlock concurrent fetch and set requests. - Moderate: Session creation does not clear negotiated capabilities and remote file state.
- Moderate: File offers accept non-regular files, potentially blocking the session thread.
- Moderate: Remote paths printed verbatim allow terminal control-sequence injection.
- Nit: Help text incorrectly states that setting files always requires an active session.
File summaries
| File | Description |
|---|---|
crates/ironrdp-testsuite-extra/tests/agent.rs |
Tests RPC round trips and redaction. |
crates/ironrdp-rpc/src/ipc.rs |
Defines file-transfer RPC types and codecs. |
crates/ironrdp-daemon/src/daemon.rs |
Implements file operations and fetch coordination. |
crates/ironrdp-daemon/src/clipboard.rs |
Integrates CLIPRDR file-transfer callbacks. |
crates/ironrdp-agent/src/help.rs |
Documents clipboard file commands. |
crates/ironrdp-agent/src/cli.rs |
Adds clipboard file CLI handling. |
crates/ironrdp-activex/src/rpc.rs |
Rejects unsupported ActiveX operations. |
Review details
Suppressed comments (6)
crates/ironrdp-daemon/src/clipboard.rs:562
- This lock callback cannot remain informational:
Cliprdrsnapshots only the descriptors, whilebuild_file_contents_responseresolves the request index against the currentlocal_file_paths. Ifclipboard-set-filesreplaces the local offer while the peer holds an olderclipDataId, a request for the locked list can read the newly offered path at that index and disclose the wrong file. Snapshot the backing paths byLockDataIdhere and resolverequest.data_idagainst that snapshot until unlock.
fn on_lock(&mut self, data_id: LockDataId) {
// `Cliprdr` already snapshots `local_file_list` for this id and releases it on the
// matching unlock; nothing for this backend to do beyond logging.
debug!(?data_id, "Remote locked local clipboard file list");
crates/ironrdp-daemon/src/clipboard.rs:590
- Only expiration is handled here. Starting a local file offer calls
Cliprdr::release_outgoing_locks, which reports the removed IDs throughon_outgoing_locks_cleared; with the default no-op callback, an active download keeps issuing chunks using an ID that was just unlocked. Handle cleared IDs with the same abort-and-notify path.
fn on_outgoing_locks_expired(&mut self, clip_data_ids: &[LockDataId]) {
let mut state = self.state.lock().expect("clipboard state poisoned");
let Some(active_id) = state.active_fetch_lock_id else {
return;
};
if clip_data_ids.iter().any(|id| id.0 == active_id) {
crates/ironrdp-daemon/src/clipboard.rs:385
- Removing
active_fetchbefore its waiting IPC handler consumes this failure reopens the slot too early. After the new file list arrives, another caller can start a fetch and clearactive_fetch_result; the original waiter can then miss its failure or consume the new fetch’s result. Keep the failed fetch reserved until its owning handler cleans it up, or correlate state/results with a generation ID.
if state.active_fetch.take().is_some() {
state.active_fetch_lock_id = None;
state.active_fetch_result = Some(ChunkedFetchProgress::Failed);
drop(state);
self.file_fetch_notify.notify_waiters();
crates/ironrdp-daemon/src/clipboard.rs:573
- The IPC limit is not enforced when accepting a remote file list.
ironrdp_cliprdrpermits up to 100,000 descriptors, butPayload::ClipboardFileListdecoding rejects more than 10,000; storing (for example) 20,000 entries makesclipboard-list-filesemit a response the CLI cannot decode (and larger lists can exceed the frame limit). Reject or truncate the list at this boundary before storing it.
fn on_remote_file_list(&mut self, files: &[FileDescriptor], clip_data_id: Option<u32>) {
debug!(file_count = files.len(), ?clip_data_id, "Received remote file list");
let mut state = self.state.lock().expect("clipboard state poisoned");
state.remote = Some(ClipboardContent::Files(files.to_vec()));
state.remote_file_lock_id = clip_data_id;
crates/ironrdp-daemon/src/daemon.rs:1345
ironrdp_cliprdraccepts remote lists up to 100,000 entries, but the new RPC decoder rejects lists above 10,000. Collecting the entire list here means 10,001–65,535 entries are encoded into a response the CLI cannot decode, while larger lists fail encoding outright. Check this bound before constructing the payload and return a typed error instead.
let files = match clipboard.remote.as_ref() {
Some(crate::clipboard::ClipboardContent::Files(files)) => Some(
files
.iter()
.map(|descriptor| ClipboardFileEntry {
name: descriptor.name.clone(),
crates/ironrdp-daemon/src/daemon.rs:1300
- Validate the basename before storing this parallel descriptor/path list.
Cliprdr::initiate_file_copysilently filters descriptors whose wire name exceeds 259 characters; if one is filtered, the remote’s compacted indices no longer matchlocal_file_paths, so requesting a visible file can serve a different local file.
let mut descriptor = FileDescriptor::new(name)
- Files reviewed: 7/7 changed files
- Comments generated: 6
- Review effort level: Balanced
Replaces the fixed FileContentsRequest stream_id with a per-fetch counter on ClipboardState: Cliprdr retains an outstanding request until its own timeout even after clipboard_get_file gives up on it, so a fixed id let a stale response from an abandoned fetch be accepted into a later one. Fixes clipboard_get_file locking state then clipboard while clipboard_set_files and set_local_and_advertise lock clipboard then state, which could deadlock two concurrent IPC calls; clones what it needs from the session while state is held instead. Clears the remote file list, its lock id, and negotiated_capabilities when a new session starts, alongside the existing active_fetch reset, so a stale lock id from a defunct CLIPRDR channel cannot be used against the new one. Rejects any non-regular file in clipboard_set_files (metadata.is_file() instead of !is_dir()), since a FIFO or similar can block the session thread on a later synchronous read. Escapes remote-controlled file names and relative paths with str::escape_debug() before printing them in clipboard-list-files, so a crafted name cannot inject terminal control sequences. Corrects the clipboard-set-files help text: it works before a session connects, same as clipboard-set/clipboard-set-image, with the negotiated-file-transfer requirement applying only once a session is active.
There was a problem hiding this comment.
The PR adds CLIPRDR file transfer to the agent daemon. The core machinery (capability gating, monotonic stream ids, ChunkedFetch driving, lock-expiry aborts) is sound. Verified defects: SendInitiateFileCopy reaches Cliprdr::initiate_file_copy, whose require_ready guard errors outside Ready and is session-fatal, so a pre-connect offer advertised at Monitor Ready kills the session before the initial Format List (a set-files call in the negotiation window hits the same path); incoming FileContentsRequests are served from the current local offer rather than the clipDataId-locked snapshot; a timed-out fetch waiter clears a later fetch's state; and clipboard-set-files releases an active fetch's lock via on_outgoing_locks_cleared, which the backend never overrides. The rest is low severity: a whole-transfer rather than idle-based fetch deadline, and localized duplication.
- [protocol] Pre-connect file offer is advertised via initiate_file_copy during Initialization, aborting the session — high 🔴 — crates/ironrdp-daemon/src/clipboard.rs
handle_monitor_ready calls on_request_format_list while the channel is in Initialization (lib.rs:596-618); the Files branch forwards SendInitiateFileCopy, which ironrdp-client maps to Cliprdr::initiate_file_copy (rdp.rs:4141-4145). initiate_file_copy's require_ready guard (lib.rs:1462-1463) errors outside Ready, and the dispatcher's custom_err! makes that session-fatal, so the mandatory initial Format List is never sent and the session terminates. The same path is hit by an explicit clipboard-set-files between capability negotiation (on_process_negotiated_capabilities) and channel Ready. Any pre-connect offer plus a file-transfer-capable server triggers this on the next connection, contradicting the PR's advertised pre-connect support. - [code-compressor] on_request_format_list repeats drop(state) + send across three arms — low 🟡 — crates/ironrdp-daemon/src/clipboard.rs
All three match arms end with drop(state) followed by one send_clipboard_message, and the un-negotiated-files and None arms both send SendInitiateCopy(Vec::new()) since advertised_formats already returns empty for Files. Collapsing to a negotiated-Files arm producing SendInitiateFileCopy(files.clone()) plus a catch-all computing SendInitiateCopy(match local { Some(content) => advertised_formats(content), None => Vec::new() }), with a single drop(state) and send after the match, is behavior-identical and removes roughly 15 lines of repeated control flow. - [code-compressor] FILETIME epoch constant declared twice in adjacent converters — low 🟡 — crates/ironrdp-daemon/src/daemon.rs
system_time_to_filetime (daemon.rs:292) and filetime_to_unix_secs (daemon.rs:310) each declare a private EPOCH_DIFFERENCE_SECS = 11_644_473_600 with near-identical wording. The filetime_round_trips_through_unix_seconds test depends on the two agreeing, so one module-level constant is the more honest encoding of that invariant; hoisting it removes the duplication with no behavior change. - [code-compressor] active_fetch_lock_id exists only because ChunkedFetch does not expose its bound id — low 🟡 — crates/ironrdp-daemon/src/clipboard.rs
The field copies the clip_data_id already stored inside the active ChunkedFetch and is synchronized in five places: connect()'s reset, clipboard_get_file's set and clears, and the abort paths in on_remote_copy and on_outgoing_locks_expired. A read-only ChunkedFetch::clip_data_id() accessor (small public API addition to ironrdp-cliprdr, a crate this PR does not otherwise modify) would eliminate the field and its drift risk. Without that API change the parallel field is a reasonable local choice, so this is an optional follow-up rather than a required fix.
build_file_contents_response ignored a FileContentsRequest's clipDataId and always read the current local offer, so a clipboard-set-files call that replaces the offer while a remote lock is still active served the wrong file's bytes under the locked descriptor's identity. on_lock now snapshots the offered list per clipDataId (capped like Cliprdr's own locked-list bound) and build_file_contents_response serves from it when a request carries a data_id, falling back to the current offer otherwise; on_unlock clears the snapshot. clipboard_get_file's timeout path unconditionally cleared the shared active_fetch slot without checking it still belonged to this call. A session transition (connect) clears active_fetch on disconnect without resolving the waiting caller or notifying it, so a stale timeout could clear a different, later fetch that had since started in the same slot. The timeout path now only clears the slot when its stream_id still matches, and connect also clears active_fetch_result and wakes the outgoing session's waiter so it does not sit for its full deadline for no reason. on_outgoing_locks_cleared was unimplemented, so a clipboard-set-files call that released an outgoing lock immediately left an active fetch bound to that lock running against a clipboard that had already moved on; it now aborts the fetch the same way the inactivity-timeout path already does. The fixed 60-second fetch timeout also covered the whole transfer rather than resetting on progress, so a large file over a slow link could time out even while making steady progress; it is now an idle timeout, matching Cliprdr's own per-request transfer_timeout. Consolidated the active-fetch abort sequence (three call sites) and the fetch-result take/match logic (two call sites) into shared helpers.
There was a problem hiding this comment.
The clipboard file-transfer feature is coherently implemented: wire bounds, stream-id reuse prevention, locked-snapshot serving, and fetch-abort consolidation all check out. Eight of nine specialist candidates verified; the two unmatched-clipDataId findings merge into one fallback finding. Three daemon coordination defects stand: per-chunk progress never wakes the waiter, so the 60s deadline is a fixed whole-transfer budget; connect()'s reset records no Failed result, so its wake cannot release a stale waiter; and the unkeyed active_fetch_result slot lets a stale waiter consume another call's bytes. One SHOULD-level capability-advertisement deviation and three optional cleanups remain. No critical or high-severity issues.
- [code-compressor] on_request_format_list repeats the drop-lock-and-send tail in all four arms — low 🟡 — crates/ironrdp-daemon/src/clipboard.rs
Each match arm ends with the same drop-then-send pair, and two arms (Files without negotiated file transfer, and no local content) send identical SendInitiateCopy(Vec::new()). Computing the ClipboardMessage inside the arms and issuing a single send after the match removes three duplicated send sites and keeps the release-lock-before-send rule visible in exactly one place, with identical messages and ordering.
Wakes the fetch waiter on every response, not only Complete/Failed, so an idle-timeout deadline actually resets on real chunk progress. Keys active_fetch_result by stream_id so a stale waiter cannot consume a later, unrelated fetch's bytes; reuses the same guard for the session- transition reset so an abandoned fetch resolves promptly instead of running out its own timeout. A present-but-unmatched clipDataId now errors instead of falling back to the current offer, per MS-RDPECLIP 3.1.5.4.6. Surfaces last_write_time in the CLI file listing instead of leaving it unread. Consolidates the two near-identical lock-abort callbacks into one helper. Six new/updated regression tests, each verified to fail against the prior code and pass against the fix.
|
This pull request may overlap with #1878. Both PRs extend the same clipboard stack with a new content type: this PR adds file-list transfer (clipboard-set-files/list-files/get-file) while This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
Extends the daemon clipboard-get/clipboard-set operations added in #1877 to
files: clipboard-set-files offers one or more local files to the remote via
the CLIPRDR file-list mechanism (FileGroupDescriptorW), clipboard-list-files
lists what the remote currently offers (metadata only, matching the
delayed-render model MS-RDPECLIP itself uses), and clipboard-get-file
fetches one file's full contents by its position in that list.
Independent of #1878 (HTML support, still open): This branches directly off
master rather than off #1878, and touches none of its code. Both extend the
ClipboardContent enum #1877 introduced, so whichever of the two merges first
will need the other rebased, same as the earlier #1877/#1878 rebase.
Extends ClipboardContent with a Files variant (wire-shaped metadata,
symmetric for what is offered locally and what the remote last advertised);
a parallel local_file_paths list on ClipboardState carries the on-disk paths
a local offer maps to, since FileDescriptor itself carries no filesystem
path and file transfer is a structurally different CLIPRDR mechanism from
the FormatDataRequest path text/image/HTML share.
Advertises STREAM_FILECLIP_ENABLED and CAN_LOCK_CLIPDATA in
client_capabilities. Cliprdr::initiate_file_copy and request_file_contents
both hard-error when file transfer was not negotiated, and that error is
session-fatal by the time it reaches ironrdp-client's dispatcher, so the
daemon tracks the negotiated capabilities from
on_process_negotiated_capabilities and checks them before sending either
message, failing cleanly at the IPC layer instead.
Cliprdr already owns the whole clipboard-lock lifecycle for file transfer
(snapshotting the local file list on an incoming LockData, auto-locking when
a remote file list arrives, timeout-driven expiry); this backend's
on_lock/on_unlock stay informational. on_outgoing_locks_expired is not: It
aborts an in-progress clipboard-get-file fetch bound to the expiring lock
rather than let it continue issuing requests against a remote clipboard that
has since changed underneath it. on_remote_copy clears an in-progress fetch
the same way when the remote clipboard changes again mid-fetch, so a
clipboard-get-file caller left waiting is told the fetch was interrupted
instead of silently blocked until its own timeout.
clipboard-get-file drives ironrdp_cliprdr::chunked_fetch::ChunkedFetch to
completion inside the daemon, bounded at MAX_CLIPBOARD_FILE_BYTES (derived
from the RPC transport's own frame limit, the same reasoning
MAX_CLIPBOARD_IMAGE_BYTES already uses) before issuing any wire request, not
just before framing the IPC response. The IPC handler and the CLIPRDR
backend's on_file_contents_response coordinate through a per-session
Notify, the same wait-and-recheck shape rail_wait already uses for RAIL
evidence, including re-checking state once more after a timeout races
against a same-instant completion rather than assuming a timeout means
nothing arrived.
Serving a remote's request for a file we offered (on_file_contents_request)
reads from the local path clipboard-set-files recorded, seeking to the
requested range and capping a single RANGE response at 4 MiB regardless of
what the peer's own cbRequested asks for: MS-RDPECLIP defines cbRequested as
an upper bound on what a responder may return, not a guarantee, and
ChunkedFetch on the requesting side already handles a shorter-than-asked
response by issuing another RANGE request for the remainder. This read
happens synchronously on the session's own current-thread runtime (Cliprdr
callbacks are not async), bounded per response by the same 4 MiB cap: A slow
local read stalls that session briefly rather than the whole daemon, but it
is a real tradeoff against a fully async read path, disclosed rather than
silently accepted.
Directory entries are rejected, not silently mishandled: clipboard-set-files
refuses a directory path outright rather than support recursive folder copy
(out of scope here), and a remote directory entry (real Windows Explorer
folder copies advertise one per subfolder, mixed in with the files) is shown
as such in clipboard-list-files and refused by clipboard-get-file rather
than attempt a byte fetch against it.
New Request::ClipboardSetFiles/ClipboardListFiles/ClipboardGetFile and
Payload::ClipboardFileList/ClipboardFile wire variants, plus the shared
ClipboardFileEntry metadata type, bounded at MAX_CLIPBOARD_FILE_BYTES and
MAX_CLIPBOARD_FILE_LIST_ENTRIES on wire decode. Extends ironrdp-activex's
exhaustive match on Request with the same treatment as the other clipboard
operations. Extends the wire round-trip and Debug-redaction test coverage
added in #1877 to the three new variants.
cargo xtask check fmt/lints/tests/typos/locks all pass. Not yet verified
against a live remote client, same as #1877/#1878.