feat: clipboard support for web client #259

pacmancoder · 2023-11-02T17:39:37Z

This PR adds clipboard support for IronRDP-web

Features:

Most complete support could be achieved on chromium-based browsers
- Formats: text/plain, text/html, text/png
- Clipboard monitoring is implemented via monitoring loop
Firefox has reduced clipboard support due to browser restrictions:
- Copy: text/plain
- Paste: text/plain, text/html, text/png
- Periodic clipboard monitoring is absent, copy/paste actions executed on demand, may introduce input freezes, the there is no known to me way to overcome this (synchronous onPaste event API is used; clipboardWrite action could only be performed as a response to user input)
Internally, the following CLIPRDR formats are handled and converted to/from their MIME counterparts:
- CF_UNICODETEXT <=> text/plain
- Registered(CF_HTML<HTML Format>), Registered(text/html) <=> text/html
- CF_DIB, CF_DIBV5, Registered(PNG), Registered(image/png) <=> image/png

Closes #107

Issue: ARC-125

github-actions · 2023-11-02T17:55:53Z

Coverage Report 🤖 ⚙️

Past:
Total lines: 25239
Covered lines: 15675 (62.11%)

New:
Total lines: 25828
Covered lines: 16187 (62.67%)

Diff: +0.57%

[this comment will be updated automatically]

web-client/iron-remote-gui/src/services/wasm-bridge.service.ts

pacmancoder · 2023-11-02T18:41:06Z

FYI: I was clippy lints regarding arithmetic side-effects, I'll fix that, other than that PR is ready for the review

crates/ironrdp-cliprdr-format/src/html.rs

CBenoit

Truly excellent work!
I appreciate you created a re-usable crate ironrdp-cliprdr-format 👍
~~In addition to the comments below, you may want to do a typos pass.~~

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

crates/ironrdp-cliprdr-format/src/bitmap.rs

crates/ironrdp-fuzzing/src/oracles/mod.rs

web-client/iron-remote-gui/src/iron-remote-gui.svelte

crates/ironrdp-web/src/clipboard/mod.rs

CBenoit · 2023-11-06T16:08:38Z

crates/ironrdp-cliprdr-format/src/bitmap.rs

+        check_invariant(self.width.abs() <= u16::MAX.into())
+            .ok_or_else(|| invalid_message_err!("biWidth", "width is too big"))?;
+        check_invariant(self.height.abs() <= u16::MAX.into())
+            .ok_or_else(|| invalid_message_err!("biHeight", "height is too big"))?;


Invariants should be checked when constructing the struct (when decoding or calling new), so that remaining of the code (encoding, casting…) may rely on this and just assume this to be true without checking again

CBenoit

In general, INVARIANT: comments should come where the variable is declared, either in the type definition, or above the let for local variables. The rationale is that invariants should be uphold at all times, and it’s useful to keep them in mind when analyzing the flow of the code.

// INVARIANT: end_pos < headers_cursor.len() - 1
let end_pos = /* … */;

Typically, when a local invariant is defined using INVARIANT:, the reader should not have to backtrack in order to verify it, but read forward instead.

Example in rust-analyzer code:
https://github.com/rust-lang/rust-analyzer/blob/c1c9e10f72ffd2e829d20ff1439ff49c2e121731/crates/hir-ty/src/lower.rs#L1118-L1127

When it’s not obvious why some piece of code is indeed ensuring the invariant holds, a comment may be added:
https://github.com/rust-lang/rust-analyzer/blob/c1c9e10f72ffd2e829d20ff1439ff49c2e121731/crates/hir-ty/src/lower.rs#L1143-L1144

And when explaining an assumption for doing something non obvious such as disabling a lint locally, the invariant may be referenced to without prefixing the comment with INVARIANT: (because it is already defined elsewhere)

// Given the invariant on `end_pos`, this code can’t cause integer overflow
#[allow(clippy::arithmetic_side_effects)]
{
    /* … */
}

CBenoit · 2023-11-06T16:28:55Z

crates/ironrdp-cliprdr-format/src/bitmap.rs

+
+    let components = color_type.samples();
+    debug_assert!(components <= 4);
+    // INVARIANT: height * width * components <= usize::MAX


Invariant bounds are better expressed as

Suggested change

// INVARIANT: height * width * components <= usize::MAX

// INVARIANT: height * width * components <= u16::MAX * u16::MAX * 4 < usize::MAX

CBenoit · 2023-11-06T16:34:12Z

crates/ironrdp-cliprdr-format/src/bitmap.rs

+        _ => unreachable!("possible values are restricted by header validation and logic above"),
+    };
+
+    // INVARIANT: width * components <= usize::MAX


Stronger:

Suggested change

// INVARIANT: width * components <= usize::MAX

// INVARIANT: width * components <= u16::MAX * 4 < usize::MAX

CBenoit · 2023-11-06T16:36:32Z

crates/ironrdp-cliprdr-format/src/bitmap.rs

+
+    // INVARIANT: stride * height_unsigned <= usize::MAX.
+    //
+    // This should be always true, because stide can't be greater than `width_unsigned * 4`,


Suggested change

// This should be always true, because stide can't be greater than `width_unsigned * 4`,

// This never overflows, because stride can't be greater than `width_unsigned * 4`,

CBenoit · 2023-11-06T16:37:11Z

crates/ironrdp-cliprdr-format/src/bitmap.rs

+    // INVARIANT: stride * height_unsigned <= usize::MAX.
+    //
+    // This should be always true, because stide can't be greater than `width_unsigned * 4`,
+    // and `width_unsigned * height_unsigned * 4` is guaranteed to be less or equal to `usize::MAX`.


The guarantee is even stronger

Suggested change

// and `width_unsigned * height_unsigned * 4` is guaranteed to be less or equal to `usize::MAX`.

// and `width_unsigned * height_unsigned * 4` is guaranteed to be lesser to `usize::MAX`.

CBenoit · 2023-11-06T16:38:14Z

crates/ironrdp-cliprdr-format/src/bitmap.rs

+    let width_unsigned = u16::try_from(info.width).unwrap();
+    let height_unsigned = u16::try_from(info.height).unwrap();
+
+    // INVARIANT: stride * height_unsigned <= usize::MAX.


Suggested change

// INVARIANT: stride * height_unsigned <= usize::MAX.

// INVARIANT: stride * height_unsigned < usize::MAX.

CBenoit · 2023-11-06T16:43:17Z

crates/ironrdp-cliprdr-format/src/bitmap.rs

+    check_invariant(u16::try_from(info.width).is_ok()).ok_or_else(|| BitmapError::WidthTooBig)?;
+    check_invariant(u16::try_from(info.height).is_ok()).ok_or_else(|| BitmapError::HeightTooBig)?;
+
+    let width_unsigned = u16::try_from(info.width).unwrap();
+    let height_unsigned = u16::try_from(info.height).unwrap();


It’s better style to not split the precondition check from the precondition use when possible

Suggested change

check_invariant(u16::try_from(info.width).is_ok()).ok_or_else(|| BitmapError::WidthTooBig)?;

check_invariant(u16::try_from(info.height).is_ok()).ok_or_else(|| BitmapError::HeightTooBig)?;

let width_unsigned = u16::try_from(info.width).unwrap();

let height_unsigned = u16::try_from(info.height).unwrap();

let width_unsigned: u16 = u16::try_from(info.width).map_err(|_| BitmapError::WidthTooBig)?;

let height_unsigned: u16 = u16::try_from(info.height).map_err(|_| BitmapError::HeightTooBig)?;

CBenoit · 2023-11-06T16:45:57Z

crates/ironrdp-cliprdr-format/src/bitmap.rs

+    };
+
+    // Row is in RGBA format
+    // INVARIANT: width_unsigned * 4 <= usize::MAX


Stronger

Suggested change

// INVARIANT: width_unsigned * 4 <= usize::MAX

// INVARIANT: width_unsigned * 4 <= u16::MAX * 4 < usize::MAX

CBenoit · 2023-11-06T17:05:27Z

crates/ironrdp-cliprdr-format/src/html.rs

+        // INVARIANT: end_pos < headers_cursor.len() - 1
+        #[allow(clippy::arithmetic_side_effects)]


The INVARIANT: comment should come where end_pos is declared so the reader knows what is being enforced for this value before reading the logic itself.

// INVARIANT: end_pos < headers_cursor.len() - 1 let end_pos = /* … */;

Then, the invariant can be used to explain why it’s okay to do something non obvious such as disabling a lint:

// Given the invariant on `end_pos`, this code can’t cause integer overflow #[allow(clippy::arithmetic_side_effects)] { /* … */ }

CBenoit · 2023-11-06T17:24:50Z

crates/ironrdp-cliprdr-format/src/html.rs

+    let start_fragment_pos_value = format!("{:0>10}", start_fragment_pos);
+    let end_fragment_pos_value = format!("{:0>10}", end_fragment_pos);
+
+    // INVARIANT: buffer.len() > placeholder_pos + POS_PLACEHOLDER.len()


It’s generally better to use < and <= over > and >=

placeholder_pos + POS_PLACEHOLDER.len() < buffer.len()

CBenoit

You’ll have to rebase on master to fix conflicts on iron-remote-gui.
It’s probably not much.

FYI, formatting and lints checks for TypeScript were added recently.
Here are the commands you should run when checking the code:

$ npm run check
$ npm run format
$ npm run lint

CBenoit

You can address the remaining comments and I’ll leave you merge yourself

CBenoit reviewed Nov 2, 2023

View reviewed changes

web-client/iron-remote-gui/src/services/wasm-bridge.service.ts Outdated Show resolved Hide resolved

pacmancoder changed the title ~~[DRAFT] Implemented clipboard for IronrRDP-Web~~ Implemented clipboard for IronrRDP-Web Nov 2, 2023

pacmancoder marked this pull request as ready for review November 2, 2023 18:39

pacmancoder requested review from awakecoding, thenextman and CBenoit November 2, 2023 18:41

CBenoit reviewed Nov 2, 2023

View reviewed changes

crates/ironrdp-cliprdr-format/src/html.rs Outdated Show resolved Hide resolved

CBenoit self-assigned this Nov 2, 2023

CBenoit reviewed Nov 3, 2023

View reviewed changes

pacmancoder requested a review from CBenoit November 6, 2023 15:13

CBenoit reviewed Nov 6, 2023

View reviewed changes

CBenoit enabled auto-merge (squash) November 6, 2023 17:39

CBenoit approved these changes Nov 6, 2023

View reviewed changes

CBenoit disabled auto-merge November 6, 2023 17:41

CBenoit changed the title ~~Implemented clipboard for IronrRDP-Web~~ feat: clipboard support for web client Nov 6, 2023

pacmancoder added 6 commits November 8, 2023 12:50

Implemented clipboard for IronrRDP-web

7a7b885

CI fixes

00db478

more docs and lint fixes

76012cb

Refactoring

8a7c2ec

Review changes part 2

703a395

Fixed JS lints

e8773d8

pacmancoder force-pushed the feat/clipboard-wasm branch from 5d2bc54 to e8773d8 Compare November 8, 2023 11:47

pacmancoder merged commit 2b50149 into master Nov 8, 2023
6 checks passed

pacmancoder deleted the feat/clipboard-wasm branch November 8, 2023 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: clipboard support for web client #259

feat: clipboard support for web client #259

pacmancoder commented Nov 2, 2023 •

edited by CBenoit

github-actions bot commented Nov 2, 2023 •

edited

pacmancoder commented Nov 2, 2023

CBenoit left a comment •

edited

CBenoit Nov 6, 2023 •

edited

CBenoit left a comment •

edited

CBenoit Nov 6, 2023 •

edited

CBenoit Nov 6, 2023 •

edited

CBenoit Nov 6, 2023

CBenoit Nov 6, 2023

CBenoit Nov 6, 2023

CBenoit Nov 6, 2023 •

edited

CBenoit Nov 6, 2023

CBenoit Nov 6, 2023 •

edited

CBenoit Nov 6, 2023

CBenoit left a comment

CBenoit left a comment •

edited

	// INVARIANT: height * width * components <= usize::MAX
	// INVARIANT: height * width * components <= u16::MAX * u16::MAX * 4 < usize::MAX

	// INVARIANT: width * components <= usize::MAX
	// INVARIANT: width * components <= u16::MAX * 4 < usize::MAX

	// This should be always true, because stide can't be greater than `width_unsigned * 4`,
	// This never overflows, because stride can't be greater than `width_unsigned * 4`,

	// and `width_unsigned * height_unsigned * 4` is guaranteed to be less or equal to `usize::MAX`.
	// and `width_unsigned * height_unsigned * 4` is guaranteed to be lesser to `usize::MAX`.

	// INVARIANT: stride * height_unsigned <= usize::MAX.
	// INVARIANT: stride * height_unsigned < usize::MAX.

	// INVARIANT: width_unsigned * 4 <= usize::MAX
	// INVARIANT: width_unsigned * 4 <= u16::MAX * 4 < usize::MAX

		// INVARIANT: end_pos < headers_cursor.len() - 1
		#[allow(clippy::arithmetic_side_effects)]

feat: clipboard support for web client #259

feat: clipboard support for web client #259

Conversation

pacmancoder commented Nov 2, 2023 • edited by CBenoit

github-actions bot commented Nov 2, 2023 • edited

Coverage Report 🤖 ⚙️

pacmancoder commented Nov 2, 2023

CBenoit left a comment • edited

Choose a reason for hiding this comment

CBenoit Nov 6, 2023 • edited

Choose a reason for hiding this comment

CBenoit left a comment • edited

Choose a reason for hiding this comment

CBenoit Nov 6, 2023 • edited

Choose a reason for hiding this comment

CBenoit Nov 6, 2023 • edited

Choose a reason for hiding this comment

CBenoit Nov 6, 2023

Choose a reason for hiding this comment

CBenoit Nov 6, 2023

Choose a reason for hiding this comment

CBenoit Nov 6, 2023

Choose a reason for hiding this comment

CBenoit Nov 6, 2023 • edited

Choose a reason for hiding this comment

CBenoit Nov 6, 2023

Choose a reason for hiding this comment

CBenoit Nov 6, 2023 • edited

Choose a reason for hiding this comment

CBenoit Nov 6, 2023

Choose a reason for hiding this comment

CBenoit left a comment

Choose a reason for hiding this comment

CBenoit left a comment • edited

Choose a reason for hiding this comment

pacmancoder commented Nov 2, 2023 •

edited by CBenoit

github-actions bot commented Nov 2, 2023 •

edited

CBenoit left a comment •

edited

CBenoit Nov 6, 2023 •

edited

CBenoit left a comment •

edited

CBenoit Nov 6, 2023 •

edited

CBenoit Nov 6, 2023 •

edited

CBenoit Nov 6, 2023 •

edited

CBenoit Nov 6, 2023 •

edited

CBenoit left a comment •

edited