Skip to content

feat(cliprdr,web,ffi): implement clipboard file transfer support#1166

Open
Gabriel Bauman (gabrielbauman) wants to merge 1 commit intoDevolutions:masterfrom
gabrielbauman:gbauman/clipboard-file-transfer
Open

feat(cliprdr,web,ffi): implement clipboard file transfer support#1166
Gabriel Bauman (gabrielbauman) wants to merge 1 commit intoDevolutions:masterfrom
gabrielbauman:gbauman/clipboard-file-transfer

Conversation

@gabrielbauman
Copy link
Copy Markdown
Contributor

@gabrielbauman Gabriel Bauman (gabrielbauman) commented Mar 13, 2026

Add end-to-end clipboard file transfer (upload and download) across the CLIPRDR channel per MS-RDPECLIP. This spans the protocol layer, WASM/web backend, FFI bindings, and adds a TypeScript FileTransferManager.

This is a big PR. I've squashed the history because it was... a journey.

Key changes:

  • File list format negotiation, capability exchange, and lock management
  • File contents request/response handling with streaming chunk transfer
  • Proactive locking strategy with timeout, expiry, and cleanup logic
  • Path traversal sanitization and wire-format validation
  • WASM clipboard backend integration with drag-and-drop folder uploads
  • FFI accessors for file transfer message variants and .NET bindings
  • Fuzz targets for PackedFileList, FileContentsRequest, and channel state
  • Comprehensive test coverage for delayed rendering, capabilities, and file contents validation

I suggest you start with crates/ironrdp-cliprdr/README.md - it gives a complete overview of the design, API, and lock lifecycle.

Some other review entry points that might help:

  • Protocol layer - crates/ironrdp-cliprdr/src/lib.rs is the core. Focus on handle_format_list(), lock_clipboard(), request_file_contents(), and handle_file_contents_response() for the main flows
  • PDU changes - crates/ironrdp-cliprdr/src/pdu/ for wire format changes (file_contents.rs, format_data/file_list.rs, capabilities.rs)
  • Backend trait - crates/ironrdp-cliprdr/src/backend.rs defines the contract between the protocol processor and platform implementations. New methods all have default impls.
  • WASM bridge - crates/iron-remote-desktop/src/session.rs and src/lib.rs for how Rust traits map to WASM
  • TypeScript frontend - web-client/iron-remote-desktop/src/FileTransferManager.ts for the high-level user-facing API
  • FFI - ffi/src/clipboard/ for .NET binding changes (I think I did this right)
  • Tests - The #[cfg(test)] module in lib.rs and the .test.ts

There are a bunch of breaking changes here. I can summarize if need be. ironrdp-cliprdr, -ffi and web stuff will want new releases.

You probably want tests in the test crate, but the cliprdr tests refer to private structures so that was going to be hairy. I do see some inline tests throughout the project. Let me know what you'd like moved.

This code is working well with our internal browser RDP product. I hope you like it.

@gabrielbauman Gabriel Bauman (gabrielbauman) force-pushed the gbauman/clipboard-file-transfer branch 2 times, most recently from c8ce2e7 to 8111eb2 Compare March 13, 2026 04:43
@CBenoit
Copy link
Copy Markdown
Member

Benoît Cortier (CBenoit) commented Mar 13, 2026 •
edited
Loading

Thank you!! I appreciate the review hints, this will help with the big diff. I would prefer it if the work can be split a bit narrower, multiple PRs with smaller scopes, but if it's too late it's okay, and I understand it's not always easy to hide all "journey" behind the sausage making. The work seems to still remain focused. I'll start by launching a quick review with Copilot to get rid of the nitpicks!

Copilot AI reviewed Mar 13, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements end-to-end clipboard file transfer support (upload + download) over the CLIPRDR channel per MS-RDPECLIP, spanning the Rust protocol implementation, WASM/web bridge, TypeScript client API, FFI bindings, and test/fuzz coverage.

Changes:

  • Extends ironrdp-cliprdr PDUs/state machine for FileGroupDescriptorW, file-contents streaming, capability negotiation, and clipboard lock lifecycle management.
  • Adds web-facing file transfer APIs (Session/SessionBuilder callbacks + FileTransferManager), integrates clipboard monitoring suppression during uploads, and introduces Vitest-based tests.
  • Updates FFI (Rust + generated .NET) to expose new clipboard/file-transfer message variants and adds fuzz targets + expanded testsuite coverage.

Reviewed changes

Copilot reviewed 55 out of 86 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
web-client/iron-remote-desktop/vite.config.ts Adds Vitest configuration (jsdom, setup file).
web-client/iron-remote-desktop/src/test/setup.ts Vitest global setup hook.
web-client/iron-remote-desktop/src/services/remote-desktop.service.ts Wires file-transfer callbacks + enableFileTransfer() and adds Ctrl+C/Ctrl+V combos.
web-client/iron-remote-desktop/src/services/enableFileTransfer.test.ts Adds integration tests for enableFileTransfer() behavior.
web-client/iron-remote-desktop/src/services/clipboard.service.ts Adds clipboard monitoring suppression during uploads.
web-client/iron-remote-desktop/src/services/PublicAPI.ts Exposes ctrlC/ctrlV + enableFileTransfer with monitoring suppression.
web-client/iron-remote-desktop/src/main.ts Exports file transfer types/APIs for consumers.
web-client/iron-remote-desktop/src/interfaces/UserInteraction.ts Adds ctrlC/ctrlV + enableFileTransfer to the public interface.
web-client/iron-remote-desktop/src/interfaces/SessionBuilder.ts Adds file-transfer callback registrations on SessionBuilder.
web-client/iron-remote-desktop/src/interfaces/Session.ts Adds low-level file transfer methods to Session interface + docs.
web-client/iron-remote-desktop/src/interfaces/FileTransfer.ts Introduces file transfer data model types (FileInfo/Request/Response).
web-client/iron-remote-desktop/src/enums/SpecialCombination.ts Adds CTRL_C/CTRL_V.
web-client/iron-remote-desktop/src/enums/FileContentsFlags.ts Adds FileContentsFlags constants and typing.
web-client/iron-remote-desktop/package.json Adds Vitest + jsdom dependencies and scripts.
web-client/iron-remote-desktop/README.md Documents FileTransferManager and low-level APIs.
web-client/iron-remote-desktop/.prettierignore Ignores /pkg output.
web-client/iron-remote-desktop-rdp/tsconfig.json Adjusts TS types and lib checking.
web-client/iron-remote-desktop-rdp/package.json Adds @types/node + normalizes deps.
web-client/iron-remote-desktop-rdp/package-lock.json Lockfile updates for node typings and deps.
web-client/README.md Documents Node prerequisites for web builds.
typos.toml Updates typos-cli dictionary/ignores.
fuzz/fuzz_targets/cliprdr_channel_processing.rs Adds CLIPRDR channel-processing fuzz target.
fuzz/Cargo.toml Registers the new fuzz target binary.
ffi/src/session/mod.rs Uses mutable SVC processor access for clipboard operations.
ffi/src/clipboard/windows.rs Fixes proxy type name + improves mpsc disconnect handling.
ffi/src/clipboard/mod.rs Renames/provides correct FFI clipboard proxy + svc message type.
ffi/src/clipboard/message.rs Exposes new clipboard message variants (unlock, file contents, errors) via Diplomat.
ffi/dotnet/Devolutions.IronRdp/Generated/RawSessionFfiResultMultitransportRequestBoxIronRdpError.cs New generated result type for multitransport.
ffi/dotnet/Devolutions.IronRdp/Generated/RawMultitransportRequest.cs New raw struct for multitransport request.
ffi/dotnet/Devolutions.IronRdp/Generated/RawFfiLockDataId.cs New raw handle wrapper for lock data id.
ffi/dotnet/Devolutions.IronRdp/Generated/RawFfiFileContentsResponse.cs New raw handle wrapper for file-contents response.
ffi/dotnet/Devolutions.IronRdp/Generated/RawFfiFileContentsRequest.cs New raw handle wrapper for file-contents request.
ffi/dotnet/Devolutions.IronRdp/Generated/RawConnectorResultFfiResultU32BoxIronRdpError.cs New generated result type (u32).
ffi/dotnet/Devolutions.IronRdp/Generated/RawConnectionResult.cs Adds share-id getter to raw ConnectionResult.
ffi/dotnet/Devolutions.IronRdp/Generated/RawConnectionActivationStateFinalized.cs Adds share-id getter to raw activation state.
ffi/dotnet/Devolutions.IronRdp/Generated/RawClipboardSvcMessage.cs Renames raw clipboard svc message wrapper.
ffi/dotnet/Devolutions.IronRdp/Generated/RawClipboardMessageType.cs Extends clipboard message type enum for new variants.
ffi/dotnet/Devolutions.IronRdp/Generated/RawClipboardMessageFfiResultU32BoxIronRdpError.cs New generated result type for clipboard message API.
ffi/dotnet/Devolutions.IronRdp/Generated/RawClipboardMessage.cs Adds new getters for unlock/file-contents/error.
ffi/dotnet/Devolutions.IronRdp/Generated/RawActiveStageOutputType.cs Adds multitransport output type.
ffi/dotnet/Devolutions.IronRdp/Generated/RawActiveStageOutput.cs Adds getter for multitransport request output.
ffi/dotnet/Devolutions.IronRdp/Generated/RawActiveStage.cs Extends fastpath processor setter signature (shareId).
ffi/dotnet/Devolutions.IronRdp/Generated/MultitransportRequest.cs New managed wrapper for multitransport request.
ffi/dotnet/Devolutions.IronRdp/Generated/FfiLockDataId.cs New managed wrapper for lock data id.
ffi/dotnet/Devolutions.IronRdp/Generated/FfiFileContentsResponse.cs New managed wrapper for file-contents response.
ffi/dotnet/Devolutions.IronRdp/Generated/FfiFileContentsRequest.cs New managed wrapper for file-contents request.
ffi/dotnet/Devolutions.IronRdp/Generated/ConnectionResult.cs Exposes ShareId on managed ConnectionResult.
ffi/dotnet/Devolutions.IronRdp/Generated/ConnectionActivationStateFinalized.cs Exposes ShareId on managed activation state.
ffi/dotnet/Devolutions.IronRdp/Generated/ClipboardSvcMessage.cs Renames managed clipboard svc message wrapper.
ffi/dotnet/Devolutions.IronRdp/Generated/ClipboardMessageType.cs Extends managed clipboard message type enum.
ffi/dotnet/Devolutions.IronRdp/Generated/ClipboardMessage.cs Adds managed properties/getters for new clipboard message variants.
ffi/dotnet/Devolutions.IronRdp/Generated/ActiveStageOutputType.cs Adds multitransport output type (managed).
ffi/dotnet/Devolutions.IronRdp/Generated/ActiveStageOutput.cs Adds multitransport request accessor (managed).
ffi/dotnet/Devolutions.IronRdp/Generated/ActiveStage.cs Extends managed fastpath processor setter signature.
crates/ironrdp-web/src/session.rs Wires CLIPRDR file transfer messages between WASM and RDP core; adds tests and safer event sending.
crates/ironrdp-web/Cargo.toml Enables needed web-sys features for timing/window access.
crates/ironrdp-testsuite-core/tests/clipboard/mod.rs Registers new clipboard/file-transfer test modules and updates expectations.
crates/ironrdp-testsuite-core/tests/clipboard/file_transfer_capabilities.rs Adds capability negotiation/encoding tests.
crates/ironrdp-testsuite-core/tests/clipboard/file_list_format.rs Adds file list format and round-trip tests.
crates/ironrdp-testsuite-core/tests/clipboard/file_contents_validation.rs Adds strict spec-validation tests for requests/responses.
crates/ironrdp-testsuite-core/tests/clipboard/delayed_rendering_integration.rs Adds integration tests for delayed rendering file transfer flows.
crates/ironrdp-testsuite-core/tests/clipboard/delayed_rendering.rs Adds unit tests for delayed rendering and boundaries.
crates/ironrdp-testsuite-core/Cargo.toml Adds dependency needed by new tests.
crates/ironrdp-server/src/server.rs Updates clipboard message handling for new enum variants.
crates/ironrdp-fuzzing/src/oracles/mod.rs Adds decoders/oracles for new CLIPRDR PDUs + channel oracle.
crates/ironrdp-cliprdr/src/pdu/mod.rs Adjusts error text for unknown msg types.
crates/ironrdp-cliprdr/src/pdu/format_list.rs Normalizes error message casing.
crates/ironrdp-cliprdr/src/pdu/format_data/mod.rs Adds decode rationale comment re: bounds/borrowing.
crates/ironrdp-cliprdr/src/pdu/format_data/file_list.rs Adds relative paths, bounds checks, and stronger decode safety for file lists.
crates/ironrdp-cliprdr/src/pdu/file_contents.rs Renames DATA->RANGE, adds validation, and tightens request/response parsing.
crates/ironrdp-cliprdr/src/pdu/capabilities.rs Fixes downgrade logic and supports unknown protocol versions.
crates/ironrdp-cliprdr/src/backend.rs Extends backend API for file lists, lock cleanup hooks, and timing hooks.
crates/ironrdp-cliprdr/README.md Adds detailed design documentation and usage for file transfer/locking.
crates/ironrdp-cliprdr/Cargo.toml Enables tests for the crate (removes test=false).
crates/ironrdp-cliprdr-native/src/windows/cliprdr_backend.rs Implements monotonic time functions for lock housekeeping.
crates/ironrdp-cliprdr-native/src/stub.rs Implements monotonic time functions for stub backend.
crates/ironrdp-cliprdr-native/src/lib.rs Adds shared monotonic clock helper (native_now_ms).
crates/ironrdp-client/src/rdp.rs Updates clipboard message dispatch for new enum variants.
crates/iron-remote-desktop/src/session.rs Extends WASM-facing Session/Builder traits with file transfer APIs.
crates/iron-remote-desktop/src/lib.rs Exposes new WASM bindings for file transfer methods/callbacks with position validation.
Cargo.lock Adds new crate dependencies to workspace lockfile.
Files not reviewed (2)
  • web-client/iron-remote-desktop-rdp/package-lock.json: Language not supported
  • web-client/iron-remote-desktop/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

CBenoit
Benoît Cortier (CBenoit) reviewed Mar 13, 2026
View reviewed changes
@gabrielbauman
Copy link
Copy Markdown
Contributor Author

Gabriel Bauman (gabrielbauman) commented Mar 13, 2026

Also Benoît Cortier (@CBenoit), I'm willing to split this into more-reviewable diffs; it'd be artificially done, since the implementation evolved over time and there are inter-module interface changes and internal deps. Each diff would not necessarily be buildable or testable in isolation.

If you can handle the big diff, that'd be less work for me, but I do want to be friendly and share the load, so let me know what you need!

@CBenoit
Copy link
Copy Markdown
Member

Benoît Cortier (CBenoit) commented Mar 13, 2026 •
edited
Loading

Also Benoît Cortier (Benoît Cortier (@CBenoit)), I'm willing to split this into more-reviewable diffs; it'd be artificially done, since the implementation evolved over time and there are inter-module interface changes and internal deps. Each diff would not necessarily be buildable or testable in isolation.

If you can handle the big diff, that'd be less work for me, but I do want to be friendly and share the load, so let me know what you need!

I think it’s okay; you gave me enough entry points, and I’ll trust you and Copilot on the smaller details.

Speaking of which, you don’t need to address every comment it leaves. Sometimes it’s not that great, but surprisingly, it’s getting pretty good at making project-specific suggestions. I’ll run it again because it’s not deterministic and actually catches new stuff with more rounds, especially for large PRs.

I can also see you already spend a lot of time thinking and tuning things, the code seems to be high quality even without scrutinizing everything.

On my side, I’ll focus on the higher-level aspects, such as the new API for iron-remote-desktop.

To be honest, since it’s still a big diff, it may take me a bit more time, and there’s definitely a psychological effect that makes the task feel more daunting, but I can handle it 🙂

Copilot AI reviewed Mar 13, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements end-to-end clipboard-based file transfer over CLIPRDR (per MS-RDPECLIP), spanning the Rust protocol layer, WASM/web bridge, TypeScript API surface, and FFI/.NET bindings, with added fuzzing and test coverage.

Changes:

  • Adds CLIPRDR file-transfer capabilities/PDUs (file list + file contents, locking lifecycle, stricter wire validation) and backend hooks.
  • Integrates file transfer into the web stack: new session callbacks/methods, a TypeScript FileTransferManager, and clipboard-monitoring suppression during uploads.
  • Updates FFI bindings (.NET generated surfaces included), adds fuzz targets, and expands the Rust testsuite coverage for file-transfer and delayed-rendering flows.

Reviewed changes

Copilot reviewed 55 out of 86 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
web-client/iron-remote-desktop/vite.config.ts Enables Vitest (jsdom + setup file) for the web client package.
web-client/iron-remote-desktop/src/test/setup.ts Adds Vitest global test setup scaffold.
web-client/iron-remote-desktop/src/services/remote-desktop.service.ts Wires FileTransferManager + file-transfer callbacks into session connect lifecycle; adds Ctrl+C/Ctrl+V helpers.
web-client/iron-remote-desktop/src/services/enableFileTransfer.test.ts Adds integration tests for enableFileTransfer() behavior and PublicAPI clipboard monitoring suppression wiring.
web-client/iron-remote-desktop/src/services/clipboard.service.ts Adds monitoring suppression/resume to avoid clobbering file-upload clipboard formats.
web-client/iron-remote-desktop/src/services/PublicAPI.ts Exposes ctrlC/ctrlV + enableFileTransfer; composes upload hooks with clipboard suppression.
web-client/iron-remote-desktop/src/main.ts Exports new file-transfer/public API types and helpers.
web-client/iron-remote-desktop/src/interfaces/UserInteraction.ts Extends public interaction contract with ctrlC/ctrlV and enableFileTransfer.
web-client/iron-remote-desktop/src/interfaces/SessionBuilder.ts Adds file-transfer callback registration methods to builder interface.
web-client/iron-remote-desktop/src/interfaces/Session.ts Adds file-transfer session methods (lock/unlock, request/submit contents, initiate copy).
web-client/iron-remote-desktop/src/interfaces/FileTransfer.ts Introduces TS types for file metadata and file contents request/response.
web-client/iron-remote-desktop/src/enums/SpecialCombination.ts Adds CTRL_C / CTRL_V to special-combination enum.
web-client/iron-remote-desktop/src/enums/FileContentsFlags.ts Adds TS FileContentsFlags constants/types.
web-client/iron-remote-desktop/package.json Adds test scripts and Vitest/jsdom deps.
web-client/iron-remote-desktop/README.md Documents FileTransferManager usage + low-level APIs; updates limitations section.
web-client/iron-remote-desktop/.prettierignore Ignores /pkg output directory.
web-client/iron-remote-desktop-rdp/tsconfig.json Adjusts TS types config (ua-parser-js) and enables skipLibCheck.
web-client/iron-remote-desktop-rdp/package.json Adds @types/node and adjusts deps ordering (ua-parser-js).
web-client/iron-remote-desktop-rdp/package-lock.json Lockfile updates for new TS/Node typings deps.
web-client/README.md Adds Node prerequisite note for the web workspace.
typos.toml Adjusts typos config to avoid false positives (extend-words).
fuzz/fuzz_targets/cliprdr_channel_processing.rs Adds a libFuzzer target for CLIPRDR channel processing.
fuzz/Cargo.toml Registers the new fuzz target binary.
ffi/src/session/mod.rs Switches to mutable SVC processor access for CLIPRDR actions.
ffi/src/clipboard/windows.rs Fixes proxy type name and improves mpsc disconnected handling.
ffi/src/clipboard/mod.rs Renames/cleans up clipboard message proxy + Diplomat type name.
ffi/src/clipboard/message.rs Expands clipboard message FFI accessors for unlock + file contents + error.
ffi/dotnet/Devolutions.IronRdp/Generated/RawSessionFfiResultMultitransportRequestBoxIronRdpError.cs Adds generated .NET raw result for multitransport request.
ffi/dotnet/Devolutions.IronRdp/Generated/RawMultitransportRequest.cs Adds generated .NET raw multitransport struct.
ffi/dotnet/Devolutions.IronRdp/Generated/RawFfiLockDataId.cs Adds generated .NET raw lock ID handle.
ffi/dotnet/Devolutions.IronRdp/Generated/RawFfiFileContentsResponse.cs Adds generated .NET raw file contents response handle.
ffi/dotnet/Devolutions.IronRdp/Generated/RawFfiFileContentsRequest.cs Adds generated .NET raw file contents request handle.
ffi/dotnet/Devolutions.IronRdp/Generated/RawConnectorResultFfiResultU32BoxIronRdpError.cs Adds generated .NET raw result wrapper for u32 returns.
ffi/dotnet/Devolutions.IronRdp/Generated/RawConnectionResult.cs Exposes share_id getter in generated raw connection result.
ffi/dotnet/Devolutions.IronRdp/Generated/RawConnectionActivationStateFinalized.cs Exposes share_id getter in generated raw activation state.
ffi/dotnet/Devolutions.IronRdp/Generated/RawClipboardSvcMessage.cs Renames generated clipboard SVC message type.
ffi/dotnet/Devolutions.IronRdp/Generated/RawClipboardMessageType.cs Updates generated clipboard message type enum variants for file transfer.
ffi/dotnet/Devolutions.IronRdp/Generated/RawClipboardMessageFfiResultU32BoxIronRdpError.cs Adds generated result wrapper used by file contents request DataId accessor.
ffi/dotnet/Devolutions.IronRdp/Generated/RawClipboardMessage.cs Adds generated raw accessors for unlock/file-contents/error variants.
ffi/dotnet/Devolutions.IronRdp/Generated/RawActiveStageOutputType.cs Adds MultitransportRequest output type.
ffi/dotnet/Devolutions.IronRdp/Generated/RawActiveStageOutput.cs Adds accessor for multitransport request (without sensitive cookie).
ffi/dotnet/Devolutions.IronRdp/Generated/RawActiveStage.cs Updates fastpath processor setter signature to include shareId.
ffi/dotnet/Devolutions.IronRdp/Generated/MultitransportRequest.cs Adds managed wrapper for multitransport request.
ffi/dotnet/Devolutions.IronRdp/Generated/FfiLockDataId.cs Adds managed wrapper for lock data ID handle.
ffi/dotnet/Devolutions.IronRdp/Generated/FfiFileContentsResponse.cs Adds managed wrapper for file contents response handle.
ffi/dotnet/Devolutions.IronRdp/Generated/FfiFileContentsRequest.cs Adds managed wrapper for file contents request handle.
ffi/dotnet/Devolutions.IronRdp/Generated/ConnectionResult.cs Exposes ShareId in managed ConnectionResult wrapper.
ffi/dotnet/Devolutions.IronRdp/Generated/ConnectionActivationStateFinalized.cs Exposes ShareId in managed activation state wrapper.
ffi/dotnet/Devolutions.IronRdp/Generated/ClipboardSvcMessage.cs Renames managed clipboard SVC message wrapper.
ffi/dotnet/Devolutions.IronRdp/Generated/ClipboardMessageType.cs Updates managed clipboard message type enum variants for file transfer.
ffi/dotnet/Devolutions.IronRdp/Generated/ClipboardMessage.cs Adds managed accessors for file-transfer variants and error.
ffi/dotnet/Devolutions.IronRdp/Generated/ActiveStageOutputType.cs Adds MultitransportRequest output type (managed).
ffi/dotnet/Devolutions.IronRdp/Generated/ActiveStageOutput.cs Adds managed accessor for multitransport request.
ffi/dotnet/Devolutions.IronRdp/Generated/ActiveStage.cs Updates managed fastpath processor setter signature to include shareId.
crates/ironrdp-web/src/session.rs Adds JS callback plumbing + session methods for file transfer, clipboard lock orchestration, and safer marshalling; adds tests.
crates/ironrdp-web/Cargo.toml Enables needed web-sys features for timing/window integration.
crates/ironrdp-testsuite-core/tests/clipboard/mod.rs Adds new clipboard/file-transfer test modules and updates existing expectations.
crates/ironrdp-testsuite-core/tests/clipboard/file_transfer_capabilities.rs Adds capability negotiation/encoding tests for file transfer flags + versions.
crates/ironrdp-testsuite-core/tests/clipboard/file_list_format.rs Adds packed file list and file descriptor format tests.
crates/ironrdp-testsuite-core/tests/clipboard/file_contents_validation.rs Adds decode/validation tests for file contents requests/responses per spec.
crates/ironrdp-testsuite-core/tests/clipboard/delayed_rendering_integration.rs Adds integration coverage for delayed rendering flows incl. file lists.
crates/ironrdp-testsuite-core/tests/clipboard/delayed_rendering.rs Adds unit tests for delayed rendering behavior, boundaries, and capability gating.
crates/ironrdp-testsuite-core/Cargo.toml Adds ironrdp-svc dependency for clipboard integration tests.
crates/ironrdp-server/src/server.rs Updates clipboard backend message handling for new unlock/file-transfer message shapes.
crates/ironrdp-fuzzing/src/oracles/mod.rs Extends decode fuzzing and adds a CLIPRDR channel processing oracle.
crates/ironrdp-cliprdr/src/pdu/mod.rs Tweaks unknown PDU error string.
crates/ironrdp-cliprdr/src/pdu/format_list.rs Tweaks invalid msgFlags error string.
crates/ironrdp-cliprdr/src/pdu/format_data/mod.rs Adds rationale comment about bounds/borrowing for format data payloads.
crates/ironrdp-cliprdr/src/pdu/format_data/file_list.rs Adds relative path support, count/length hardening, and descriptor builder APIs.
crates/ironrdp-cliprdr/src/pdu/file_contents.rs Renames DATA->RANGE, validates flags/constraints, and tightens request/response parsing.
crates/ironrdp-cliprdr/src/pdu/capabilities.rs Fixes flag negotiation bug and preserves unknown protocol versions.
crates/ironrdp-cliprdr/src/backend.rs Extends backend contract/messages for file lists and lock cleanup events; refines semantics docs.
crates/ironrdp-cliprdr/README.md Adds comprehensive CLIPRDR + file transfer design/API documentation.
crates/ironrdp-cliprdr/Cargo.toml Enables crate tests by removing test = false.
crates/ironrdp-cliprdr-native/src/windows/cliprdr_backend.rs Implements monotonic clock hooks for lock timeout tracking.
crates/ironrdp-cliprdr-native/src/stub.rs Implements monotonic clock hooks for stub backend.
crates/ironrdp-cliprdr-native/src/lib.rs Adds a shared native monotonic clock helper (native_now_ms).
crates/ironrdp-client/src/rdp.rs Switches to mutable CLIPRDR processor access and updates unlock message handling.
crates/iron-remote-desktop/src/session.rs Extends WASM-facing session traits with file-transfer callbacks/methods (default unimplemented).
crates/iron-remote-desktop/src/lib.rs Exposes new WASM bindings for file transfer session APIs and callbacks; validates numeric marshalling.
Cargo.lock Adds ironrdp-svc as a dependency where needed.
Files not reviewed (2)
  • web-client/iron-remote-desktop-rdp/package-lock.json: Language not supported
  • web-client/iron-remote-desktop/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

CBenoit
Benoît Cortier (CBenoit) reviewed Mar 13, 2026
View reviewed changes
crates/ironrdp-fuzzing/src/oracles/mod.rs
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: Even the state machine was fuzzed 👏

@glamberson
Copy link
Copy Markdown
Contributor

Greg Lamberson (glamberson) commented Mar 13, 2026 •
edited
Loading

Nice work on this. Given that I maintain a server-side IronRDP implementation (CliprdrBackend consumer on the Linux/Wayland side), I wanted to share some observations from that perspective.

Default trait impls: the approach of adding on_remote_file_list, on_outgoing_locks_cleared, now_ms, and elapsed_ms with default implementations is clean. Our existing backend compiles without changes, and we can adopt the new callbacks incrementally. Good API evolution pattern.

Path sanitization at the protocol layer: we have ~550 lines of path sanitization on the server side (Linux-specific filename concerns, Windows reserved names, file URI parsing). Having sanitize_file_path() at the protocol layer as a baseline is good defense-in-depth. The protocol handles the universal concerns (traversal, UNC, drive letters); application layers can add platform-specific rules on top.

LockStrategy question: what informed the choice of OnDemand as the default over Proactive? FreeRDP uses proactive locking (auto-lock when FileGroupDescriptorW is detected in the format list), which is what most Windows RDP servers expect to see. Was OnDemand chosen deliberately for browser/WASM constraints, or is there a correctness argument for deferring the lock until the first file contents request?

Server-side use case: on our end, the "remote" side is the RDP client advertising files for paste into the Linux desktop. We use a FUSE virtual filesystem so Linux apps can read clipboard files on demand -- each FUSE read() triggers an async FileContentsRequest back to the RDP client.. The on_remote_file_list callback is useful for this: we get file metadata upfront to populate the FUSE directory entries before any read happens.

These are the things that jump out at me. I'm sure there are others..

Greg Lamberson

@gabrielbauman
Copy link
Copy Markdown
Contributor Author

Gabriel Bauman (gabrielbauman) commented Mar 13, 2026

Hi Greg Lamberson (@glamberson)! Thanks for the kind words; good to see a fellow linux-desktop person on here, and glad this works out-of-the-box for you.

LockStrategy question: what informed the choice of OnDemand as the default over Proactive? FreeRDP uses proactive locking (auto-lock when FileGroupDescriptorW is detected in the format list), which is what most Windows RDP servers expect to see. Was OnDemand chosen deliberately for browser/WASM constraints, or is there a correctness argument for deferring the lock until the first file contents request?

Good call-out. OnDemand was the first thing I implemented while reading the spec, and when I realized FreeRDP behaved differently I was unsure which was correct... So I did both just in case and moved on. The default is just a default.

You've been in the trenches with RDP for awhile; can you think of any reason not to nuke LockStrategy and adopt Proactive as the only behaviour? I'd rather lose the complexity than merge something that's effectively useless. I think my Typescript layer basically implements Proactive on top of default Ondemand, now that I look at it. facepalm

@gabrielbauman Gabriel Bauman (gabrielbauman) force-pushed the gbauman/clipboard-file-transfer branch 2 times, most recently from a851470 to c86f040 Compare March 13, 2026 20:31
@gabrielbauman
Copy link
Copy Markdown
Contributor Author

Gabriel Bauman (gabrielbauman) commented Mar 13, 2026

Thinking about this more I'm leaning towards removing OnDemand and making Proactive the only locking behavior. Here's the reasoning:

Locks are per-clipboard-snapshot, not per-file. The Lock/Unlock PDUs in MS-RDPECLIP 1.3.2.3 lock the entire clipboard state so it doesn't change during reads. There's no scenario where you'd want to lock "one file" - the clipDataId identifies a snapshot, not an individual transfer.

OnDemand creates redundant locks. With OnDemand, the TypeScript FileTransferManager acquires a separate lock per download, even though all downloads reference the same clipboard state. The server has to track N locks when 1 suffices. Proactive already handles clipboard changes correctly by detaching the old lock (keeping it alive for in-flight transfers) and creating a new one.

The TS layer is reimplementing Proactive manually. FileTransferManager has a lockClipboard() call with a 30-second timeout and 13 separate unlockClipboard() calls across every error/success/abort path. With Proactive, all of that goes away - the lock is already held when filesAvailable fires, and lifecycle is managed by the Rust layer.

FreeRDP uses proactive locking, and it's what Windows RDP servers expect per the spec's Figure 3 sequence diagram.

Server-side consumers (FUSE, etc.) also benefit. Proactive means the lock is already held when on_remote_file_list() fires, so backends can immediately issue FileContentsRequests against a stable snapshot without a lock-acquisition round-trip.

I'll remove LockStrategy, always auto-lock in handle_format_list(), simplify the TS layer, and expose the proactive clipDataId through the files-available callback. I want to do this on this MR so we aren't committing complexity that doesn't need to exist.

Tagging Greg Lamberson (@glamberson) since you raised the question - any concerns with this from the server side?

@glamberson
Copy link
Copy Markdown
Contributor

Greg Lamberson (glamberson) commented Mar 13, 2026

Gabriel Bauman (@gabrielbauman) No concerns from the server side. This is the right move.

The per-snapshot semantics are the key point. With Proactive, the lock is already held when on_remote_file_list() fires, so backends can start issuing FileContentsRequests against a stable snapshot immediately. No lock-acquisition round-trip, no window where the clipboard could change between seeing the file list and requesting content.

One thing worth considering: server-side backends may not request file contents immediately. A backend might expose the file list to the host OS and defer the actual FileContentsRequests until an application reads the data, potentially minutes later. The lock needs to stay alive until the backend explicitly releases it or the clipboard changes via a new FormatList. As long as the auto-detach on new FormatList keeps the old lock alive for in-flight transfers (which it sounds like it does) this works well for deferred-read patterns.

Agreed on nuking the enum. Proactive matches the spec's Figure 3 sequence, matches FreeRDP, and removes a configuration surface that nobody should need to change.

Greg

@gabrielbauman Gabriel Bauman (gabrielbauman) force-pushed the gbauman/clipboard-file-transfer branch 2 times, most recently from bcba7e5 to fd29fef Compare March 14, 2026 00:34
@gabrielbauman
Copy link
Copy Markdown
Contributor Author

Gabriel Bauman (gabrielbauman) commented Mar 14, 2026

Okay, I've nuked the enum and made proactive locking the default and only lock strategy. I've also tested with our internal browser RDP client and the new locking works flawlessly against a Windows rdp server. Nice drop in complexity in the wasm and typescript layers.

I've also done some drive-by fixes for a few minor style and quality-of-life issues etc. I'm done tweaking until I hear back on the review front.

Sorry for the churn! We're in good shape now, I think. Thanks for the discussion, Greg Lamberson (@glamberson)! Have a nice weekend, everyone.

@gabrielbauman
Copy link
Copy Markdown
Contributor Author

Gabriel Bauman (gabrielbauman) commented Mar 16, 2026

Found and fixed a regression from our locking refactor above.

@gabrielbauman
Copy link
Copy Markdown
Contributor Author

Gabriel Bauman (gabrielbauman) commented Mar 19, 2026

Hi Benoît Cortier (@CBenoit), anything I can help with?

CBenoit
Benoît Cortier (CBenoit) reviewed Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

I’m really sorry for the delay!

The bottom line for me: the final handling seems to be the right one. Thank you Greg Lamberson (@glamberson) for also double checking the protocol part, and suggesting proactive clipboard locking. That’s a valid and pragmatic choice. I totally endorse the direction.

I have extra remarks and suggestions before we merge.

praise: I think FileTransferManager is a good, high-level abstraction, well-designed. Very nice. We should take inspiration from it in the future to provide a nice higher interface to the consumers.

praise: Well documented.

praise: Thank you for following IronRDP’s STYLE.md!

praise: Test coverage is excellent.

fuzz/Cargo.toml
bench = false

[[bin]]
name = "cliprdr_channel_processing"
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Can you add this target in the fuzz workflow too?

Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IronRDP/.github/workflows/fuzz.yml

Line 44 in a6b4109

target: [pdu_decoding, rle_decompression, bitmap_stream, cliprdr_format, channel_processing]

crates/ironrdp-cliprdr/README.md
For advanced use cases where you need explicit control, you can call `cleanup_expired_locks()` manually:

```rust
// Optional: Force cleanup check before automatic interval
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I think this wording is easier to understand (at least for me)

Suggested change
// Optional: Force cleanup check before automatic interval
// Optional: Trigger cleanup check before automatic interval

crates/ironrdp-cliprdr/README.md
Comment on lines +166 to +174
#### Automatic Lock Behavior

When a FormatList containing `FileGroupDescriptorW` is received and `CAN_LOCK_CLIPDATA` was negotiated, the cliprdr processor automatically:

1. Sends a Lock Clipboard Data PDU with a new `clipDataId`
2. Passes the `clipDataId` to the backend via `on_remote_file_list(files, clip_data_id)`
3. Manages the lock lifecycle (expiry, cleanup, Unlock PDUs) internally

Backends receive the `clipDataId` and can pass it to `request_file_contents()` via the `data_id` field. No explicit lock/unlock calls are needed.
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The lazy piggyback pattern is a pragmatic shortcut, but it has real costs in my opinion.

rationale:

  1. It forces &mut self on semantically read-only operations (initiate_paste, submit_format_data), so callers and type system can no longer distinguish between "mutating" and "just builds a PDU".
  2. The return type becomes semantically muddled: initiate_paste may return [UNLOCK_CLIPDATA, FORMAT_DATA_REQUEST], which can be surprising, it’s documented but it’s unrelated side-effects bundled in.
  3. Cleanup is only triggered by user activity, and is unpredictable, although I understand it’s not a big issue in practice.

suggestion: I think there is a more idiomatic alternative here. I’m thinking about a dedicated poll/drive called from the event loop. I’ve seen this pattern in crates such as rustls, quinn or tokio. Some return a Duration hint to let the caller scheduler the next drive without polling blindly. In our case, I think we don’t really need much more than an interval though:

// A persistent interval stream.
let cleanup_interval_ms = 5_000;
let mut cleanup_timer = gloo_timers::future::IntervalStream::new(cleanup_interval_ms);

let disconnect_reason = 'outer: loop {
   let outputs = select! {
       /* ... */

       _ = cleanup_timer.next() => {
           if let Some(cliprdr) = active_stage.get_svc_processor_mut::<CliprdrClient>() {
               match cliprdr.cleanup_expired_locks() {
                   Ok(svc_messages) => {
                       let frame = active_stage.process_svc_processor_messages(svc_messages)?;
                       vec![ActiveStageOutput::ResponseFrame(frame)]
                   }
                   Err(e) => {
                       error!(error = %e, "Clipboard lock cleanup failed");
                       Vec::new()
                   }
               }
           } else {
               Vec::new()
           }
       }
   };
};

That way Cliprdr can drop the internal auto_cleanup_interval / last_cleanup_check / maybe_cleanup_locks machinery entirely and the scheduling responsibility moves to the caller, making it configurable in the session setup code without coupling it to the protocol processor.

With that in place:

  • initiate_copy, initiate_paste, submit_format_data go back to &self (they just encode PDUs)
  • maybe_cleanup_locks() disappears entirely
  • cleanup_expired_locks() is the single, explicit, testable cleanup path
  • get_svc_processor suffices for most call sites; only genuinely mutating operations (request_file_contents, initiate_file_copy) need get_svc_processor_mut

crates/ironrdp-cliprdr/src/backend.rs
Comment on lines +176 to +193
/// Called when outgoing clipboard locks are automatically cleared.
///
/// This is triggered when a new FormatList PDU is received, indicating that the clipboard
/// content has changed. Per [MS-RDPECLIP] 2.2.4.2, all existing locks are no longer
/// relevant and must be released.
///
/// **Use case**: Backends can use this to clean up any associated lock state, cancel
/// ongoing downloads, or update UI to reflect that the remote clipboard has changed.
///
/// ## Parameters
///
/// - `clip_data_ids`: List of clipDataIds for locks that were automatically cleared.
///
/// **Note**: Unlock PDUs are automatically sent for each cleared lock.
fn on_outgoing_locks_cleared(&mut self, clip_data_ids: &[LockDataId]) {
let _ = clip_data_ids;
// Default implementation does nothing - backends can override to handle lock cleanup
}
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The trigger description here is wrong, and it conflicts with the on_outgoing_locks_expired doc directly below.

It says "triggered when a new FormatList PDU is received", but that is precisely what triggers on_outgoing_locks_expired. on_outgoing_locks_cleared is triggered later, by cleanup_expired_locks(), after the inactivity/max-lifetime timeout elapses on an expired lock.

Additionally, the spec citation [MS-RDPECLIP] 2.2.4.2 is the structure definition of CLIPRDR_UNLOCK_CLIPDATA, just a PDU layout, and it contains no directive about when locks must be released on clipboard change.

crates/ironrdp-cliprdr/README.md
**Maximum Lifetime (default: 2 hours)**
- Locks are force-cleaned after 2 hours regardless of activity
- This prevents indefinite resource accumulation from slow or stalled transfers
- Measured from when the lock transitioned to Expired state (not from lock creation)
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: It seems this line contradicts the code, cleanup_expired_locks_impl computes total_lifetime_ms = now.saturating_sub(lock.created_at_ms).

crates/iron-remote-desktop/src/session.rs
/// // Then request data in chunks
/// requestFileContents(1, 0, 0x2, 0, 4096, clipDataId);
/// // Wait for fileContentsResponseCallback with data
/// unlockClipboard(clipDataId);
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I don’t see this function (or maybe I’m missing it?) (EDIT: I think it was part of the initial implementation)

web-client/iron-remote-desktop/README.md
Comment on lines +222 to +232
// Lock clipboard for file transfer
const clipDataId = await session.lockClipboard();

// Request file size
session.requestFileContents(streamId, fileIndex, FileContentsFlags.SIZE, 0, 8, clipDataId);

// Request file data
session.requestFileContents(streamId, fileIndex, FileContentsFlags.RANGE, offset, chunkSize, clipDataId);

// Unlock clipboard when done
session.unlockClipboard(clipDataId);
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue:  Same as above, lockClipboard() and unlockClipboard() were part of the initial iteration, before the proactive locking refactor.

web-client/iron-remote-desktop/src/interfaces/Session.ts
* @param flags - FileContentsFlags: 0x1 (SIZE) or 0x2 (RANGE)
* @param position - Byte offset for RANGE requests
* @param size - Number of bytes requested for RANGE requests
* @param clipDataId - Optional lock ID from lockClipboard()
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Another reference to lockClipboard()

@CBenoit
Copy link
Copy Markdown
Member

Benoît Cortier (CBenoit) commented Mar 30, 2026 •
edited
Loading

I have more suggestions but it’s getting late so I’ll wrap up the review tomorrow!

CBenoit
Benoît Cortier (CBenoit) reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi again!

For this second pass, I’m focusing on the new API surface.

It looks like it was not clearly written anywhere, so I opened a PR to document the API design philosophy of iron-remote-desktop: #1192

crates/iron-remote-desktop/src/session.rs
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The Session/SessionBuilder traits (both Rust and TypeScript) now expose raw RDPECLIP wire concepts: streamId, FileContentsFlags, clipDataId, etc. These don't exist in any other remote protocol and violate the design invariant that iron-remote-desktop is protocol-agnostic.

suggestion: Push requestFileContents, submitFileContents, etc, into the RDP backend layer and wire them through the existing invokeExtension / extension mechanism. For that, you can refer to existing extensions such as displayControl or kdcProxyUrl.

You should use the extension_match! macro to match on the extension, and extract the value. The current extensions are trivial, only using primitive types. In this case, you will need to use js_sys::Reflect module in order to extract the parameters from the JavaScript object / JsValue, and Function::try_from_js_value to convert the JsValue into a js_sys::Function.

Example:

    fn invoke_extension(&self, ext: Extension) -> Result<JsValue, Self::Error> {
        use js_sys::{JsString, Object};
        use wasm_bindgen::convert::TryFromJsValue as _;

        iron_remote_desktop::extension_match! {
            match ext;
            |request_file_contents: JsValue| {
                let obj = into_object(request_file_contents)?;
                let stream_id = get_u32(&obj, "stream_id")?;
                let file_index = get_i32(&obj, "file_index")?;
                let flags = get_u32(&obj, "flags")?;
                let position = get_u64(&obj, "position")?;
                let size = get_u32(&obj, "size")?;
                let clip_data_id = get_u32_opt(&obj, "clip_data_id")?;

                self.input_events_tx
                    .unbounded_send(RdpInputEvent::ClipboardBackend(
                        WasmClipboardBackendMessage::FileContentsRequestSend {
                            stream_id,
                            index: file_index,
                            flags: FileContentsFlags::from_bits_truncate(flags),
                            position,
                            size,
                            clip_data_id,
                        },
                    ))
                    .context("send file contents request")
                    .map_err(Into::into)?;

                return Ok(JsValue::NULL);
            };
        }

        return Err(
            IronError::from(anyhow::Error::msg(format!("unknown extension: {}", ext.ident())))
                .with_kind(IronErrorKind::General),
        );

        fn into_object(val: JsValue) -> Result<Object, IronError> {
            let obj = Object::try_from_js_value(val).ok().context("invalid value")?;
            Ok(obj)
        }

        fn get_u32(obj: &Object, key: &str) -> Result<u32, IronError> {
            let property = js_sys::Reflect::get_str(obj, &JsString::from(key))
                .map_err(|e| anyhow::anyhow!("get property `{key}`: {e:?}"))?
                .with_context(|| format!("object is missing property `{key}`"))?;

            let value = property
                .as_f64()
                .map(f64_to_u32_saturating_cast)
                .with_context(|| format!("invalid type for property `{key}`"))?;

            Ok(value)
        }

        fn get_u64(obj: &Object, key: &str) -> Result<u64, IronError> {
            let property = js_sys::Reflect::get_str(obj, &JsString::from(key))
                .map_err(|e| anyhow::anyhow!("get property `{key}`: {e:?}"))?
                .with_context(|| format!("object is missing property `{key}`"))?;

            let value = property
                .as_f64()
                .map(f64_to_u64_saturating_cast)
                .with_context(|| format!("invalid type for property `{key}`"))?;

            Ok(value)
        }

        fn get_i32(obj: &Object, key: &str) -> Result<i32, IronError> {
            let property = js_sys::Reflect::get_str(obj, &JsString::from(key))
                .map_err(|e| anyhow::anyhow!("get property `{key}`: {e:?}"))?
                .with_context(|| format!("object is missing property `{key}`"))?;

            let value = property
                .as_f64()
                .map(f64_to_i32_saturating_cast)
                .with_context(|| format!("invalid type for property `{key}`"))?;

            Ok(value)
        }

        fn get_u32_opt(obj: &Object, key: &str) -> Result<Option<u32>, IronError> {
            match js_sys::Reflect::get_str(obj, &JsString::from(key))
                .map_err(|e| anyhow::anyhow!("get property `{key}`: {e:?}"))?
            {
                Some(property) => {
                    let value = property
                        .as_f64()
                        .map(f64_to_u32_saturating_cast)
                        .with_context(|| format!("invalid type for property `{key}`"))?;

                    Ok(Some(value))
                }
                None => Ok(None),
            }
        }
    }

We may refactor the helpers, but it would be good enough as-is for now!

crates/ironrdp-web/src/session.rs
clip_data_id,
},
))
.context("Send file contents request")
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style:

Suggested change
.context("Send file contents request")
.context("send file contents request")

web-client/iron-remote-desktop/src/FileTransferManager.ts
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: I really like this high-level abstraction.

suggestion: Based on my previous suggestion, this should go into iron-remote-desktop-rdp because it will rely on the new RDP extensions.

thought: At some point in the future we may consider writing a common abstraction using some form of dependency injection again, but I think it’s not necessary for now.

CBenoit
Benoît Cortier (CBenoit) reviewed Mar 31, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit pass!

I confirm I reviewed everything I intended to review before we merge 🙂

web-client/iron-remote-desktop/src/interfaces/SessionBuilder.ts
/**
* Optional
*
* Called when remote locks their clipboard for file transfer. The dataId associates
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I think this is the other way around?

Suggested change
* Called when remote locks their clipboard for file transfer. The dataId associates
* Called when remote locks our clipboard for file transfer. The dataId associates

crates/ironrdp-cliprdr/src/lib.rs
Comment on lines +610 to +618
// SAFETY (ordering): `handle_server_capabilities()` runs
// synchronously during `process()` for the Capabilities PDU,
// which always precedes the Monitor Ready PDU in the server's
// initialization sequence. By the time any backend callback
// triggers `initiate_copy()`, `self.capabilities` has already
// been downgraded against the server's capabilities. This holds
// regardless of whether the backend responds synchronously or
// asynchronously, because `process()` for the Capabilities PDU
// completes before `process()` for Monitor Ready begins.
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I think it may be best to keep SAFETY: comments for unsafe blocks, as a convention.

crates/ironrdp-cliprdr/src/lib.rs
/// [2.2.5.3]: https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpeclip/cbc851d3-4e68-45f4-9292-26872a9209f2
pub fn request_file_contents(&self, request: FileContentsRequest) -> PduResult<CliprdrSvcMessages<R>> {
ready_guard!(self, request_file_contents);
pub fn request_file_contents(&mut self, mut request: FileContentsRequest) -> PduResult<CliprdrSvcMessages<R>> {
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: It could be good to enforce the CB_HUGE_FILE_SUPPORT_ENABLED (0x00000020) constraint on nPositionHigh and nPositionLow. What do you think?

crates/ironrdp-cliprdr/src/lib.rs
}

// [MS-RDPECLIP] 2.2.5.3 - Validate flags are spec-compliant
if let Err(e) = request.flags.validate() {
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Just a note about "Parse Don’t Validate" pattern (https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/); but it’s probably fine as-is as well

crates/ironrdp-cliprdr/src/lib.rs
/// These methods are gated behind the `__test` feature and exist solely
/// so that tests in `ironrdp-testsuite-core` can set up / inspect internal
/// fields without making them part of the public API.
#[cfg(feature = "__test")]
Copy link
Copy Markdown
Member

@CBenoit Benoît Cortier (CBenoit) Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Also add #[doc(hidden)] so it’s never shown in docs.rs

Suggested change
#[cfg(feature = "__test")]
#[cfg(feature = "__test")]
#[doc(hidden)]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CBenoit Benoît Cortier (CBenoit) CBenoit left review comments

Copilot code review Copilot Copilot left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gabrielbauman @CBenoit @glamberson