Repository navigation
feat(agent): add clipboard HTML support - #1878
Marc-André Moreau (mamoreau-devolutions) merged 3 commits into
Conversation
f2812d3 to
b648bc0
Compare
|
The |
Depends on Devolutions#1877 (clipboard image support), which this branch is stacked on: it introduces the ClipboardContent enum this PR extends with an Html variant. Diff is cumulative against master per the usual stacking note; the image-only diff is Devolutions#1877's own. Extends the daemon clipboard-get/clipboard-set operations to HTML fragments: clipboard-get-html prints the last HTML fragment received from the remote clipboard, clipboard-set-html sets one and advertises it to the remote as the registered HTML Format. Stores the fragment as plain text (not yet CF_HTML-wrapped) and converts at the point of use via ironrdp-cliprdr-format's existing plain_html_to_cf_html/cf_html_to_plain_html, so no new markup handling is needed here, mirroring the image PR's use of the crate's existing bitmap conversions. HTML Format is a registered, not fixed-ID, clipboard format: the offering side picks a private-range ID (0xC000 here) and pairs it with the name; the receiver learns the ID-to-name mapping from the FormatList. When receiving, matches the remote's offer by name and uses whatever ID it assigned, not the locally-chosen one, since those only apply to formats this backend itself offers. Extends the remote-copy priority order to image, then HTML, then text (richest representation first), and the PendingPaste tracking added in Devolutions#1877 with an Html variant so a returned FormatDataResponse is decoded correctly regardless of which of the three was requested. A remote HTML fragment over the same 256 KiB bound is dropped and logged rather than stored, mirroring the oversized-image drop Devolutions#1877 added for the same reason: an unbounded stored value would otherwise fail encoding once handed back to clipboard-get-html over the RPC transport. New Request::ClipboardGetHtml/ClipboardSetHtml and Payload::ClipboardHtml wire variants, bounded at 256 KiB in the CLI parser and 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 two new variants. cargo xtask check fmt/lints/tests/typos/locks all pass. Not yet verified against a live remote client, same as Devolutions#1877.
b648bc0 to
f22625b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Decode diagnostics violate the repository’s offset convention, and several messages misstate the new HTML behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds daemon clipboard HTML support using registered HTML Format negotiation and existing CF_HTML conversions.
Changes:
- Adds HTML clipboard RPC and CLI variants with 256 KiB bounds.
- Handles HTML advertisement, transfer, conversion, and prioritization.
- Extends ActiveX handling and wire/redaction tests.
File summaries
| File | Description |
|---|---|
crates/ironrdp-testsuite-extra/tests/agent.rs |
Tests HTML RPC variants and redaction. |
crates/ironrdp-rpc/src/ipc.rs |
Defines bounded HTML requests and payloads. |
crates/ironrdp-daemon/src/daemon.rs |
Implements HTML get/set handlers. |
crates/ironrdp-daemon/src/clipboard.rs |
Adds CF_HTML CLIPRDR behavior. |
crates/ironrdp-agent/src/help.rs |
Documents HTML commands. |
crates/ironrdp-agent/src/cli.rs |
Adds HTML CLI parsing and output. |
crates/ironrdp-activex/src/rpc.rs |
Marks HTML operations unavailable through ActiveX. |
Review details
Suppressed comments (2)
crates/ironrdp-rpc/src/ipc.rs:2643
- This decode error also omits the cursor context required by
STYLE.md:110-114, which drops the failing byte offset from diagnostics. Passin: srchere as well.
return Err(ironrdp_core::invalid_field_err!("clipboard html", "too large"));
crates/ironrdp-daemon/src/daemon.rs:1152
- The new HTML outcome is missing from this method's documentation, which says
Noneonly covers no content or text. Update it so callers know to queryclipboard_get_htmlwhen the current item is HTML.
Some(crate::clipboard::ClipboardContent::Text(_) | crate::clipboard::ClipboardContent::Html(_)) | None => {
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Balanced
Thank you! There is chance it may be looking at this, but the fix is to open the PRs as draft until they are rebased and ready to be reviewed; the classifier will not run against it until it is marked as ready. It’s probably better so it actually reviews the diff without the stacked work. |
Renames the HTML-oversized debug message to name the actual 256 KiB policy bound instead of the transport limit, which the message had copy-adapted from the image case where that name is accurate. Updates clipboard_get and clipboard_get_image's doc comments to name all three ClipboardContent outcomes now that Html exists; both were left describing a two-way split from before it was added. Adds in: src cursor context to the two HTML too-large decode errors, matching STYLE.md's convention for a field read off the wire and then bound-checked. Rewords clipboard-get-html's help text to state that (empty) covers both no-content-yet and current-item-is-not-html, not just the former.
There was a problem hiding this comment.
HTML clipboard support added across CLI, daemon CLIPRDR backend, RPC wire layer, ActiveX stub, and tests. Independent review confirms protocol-conformant registered-format handling (self-chosen 0xC000 ID when advertising, by-name match using the peer's ID when receiving), symmetric bounded wire codecs (Request tags 33/34, Payload tag 15), and plumbing consistent with the #1877 precedent. Only two low-severity compression candidates existed: one accepted (duplicate print arm), one rejected (inlining remote_html_id would diverge from the sibling precompute-then-branch style). No correctness, safety, protocol, or maintainability defects found.
Both arms printed the string or "(empty)" with identical logic; folded into one or-pattern arm per review feedback.
7442a63
into
Devolutions:master
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.
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. Rebased onto master after #1878 (HTML support) merged. The two were built in parallel and both extend the ClipboardContent enum from #1877, so this PR now carries both: A remote copy is requested files over image over HTML over text, and the file-transfer IPC tags sit after HTML's (Payload 16 and 17, Request 35 to 37) rather than reusing the numbers HTML took. 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.
#1877 (clipboard image support) merged: This PR now rebases cleanly onto
master and its own diff is HTML-only. It still depends on the
ClipboardContent enum #1877 introduced.
Extends the daemon clipboard-get/clipboard-set operations to HTML fragments:
clipboard-get-html prints the last HTML fragment received from the remote
clipboard, clipboard-set-html sets one and advertises it to the remote as
the registered HTML Format.
Stores the fragment as plain text (not yet CF_HTML-wrapped) and converts at
the point of use via ironrdp-cliprdr-format's existing
plain_html_to_cf_html/cf_html_to_plain_html, so no new markup handling is
needed here, mirroring the image PR's use of the crate's existing bitmap
conversions.
HTML Format is a registered, not fixed-ID, clipboard format: The offering
side picks a private-range ID (0xC000 here) and pairs it with the name; the
receiver learns the ID-to-name mapping from the FormatList. When receiving,
matches the remote's offer by name and uses whatever ID it assigned, not the
locally-chosen one, since those only apply to formats this backend itself
offers.
Extends the remote-copy priority order to image, then HTML, then text
(richest representation first), and the PendingPaste tracking added in
#1877 with an Html variant so a returned FormatDataResponse is decoded
correctly regardless of which of the three was requested. A remote HTML
fragment over the 256 KiB bound is dropped and logged rather than stored,
mirroring the oversized-image drop #1877's review round added, for the same
reason: An unbounded stored value would otherwise fail encoding once handed
back over the RPC transport.
New Request::ClipboardGetHtml/ClipboardSetHtml and Payload::ClipboardHtml
wire variants, bounded at 256 KiB in the CLI parser and 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 two new variants.
cargo xtask check fmt/lints/tests/typos/locks all pass. Not yet verified
against a live remote client, same as #1877.