Repository navigation
Enhance RDP features and expose additional WASM options - #1764
Tô Nguyễn Đức Mạnh (tonguyenducmanh) wants to merge 16 commits into
Conversation
…ên trình duyệt mong muốn render cả con trỏ chuột cho tiện thao tác
Enhance RDP features, improve documentation, and update dependencies
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
needs-review
GitHub Actions
Inactive
github-actions
Bot
added
breaking-change
|
Automated review will not run because this contributor is not yet eligible under the automation policy. Contributors become eligible after one qualifying IronRDP pull request is merged into |
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you for the PR.
As per the design principle documented here, I think these should be provided as extensions instead of be part of the common abstraction.
GitHub Actions
Inactive
github-actions
Bot
removed
scope/cross-cutting
github-actions
Bot
added
the
needs-review
GitHub Actions
Inactive
github-actions
Bot
added
size/S
github-actions
Bot
removed
the
needs-review
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR adds four browser-facing session builder options (pointer_software_rendering, desktop_scale_factor, performance_flags, timezone_info) via the extension mechanism, as the maintainer requested, and wires them into connector::Config. The design and protocol framing are sound: DST-date-zeroed TimezoneInfo is a valid 'no DST' encoding and the bias sign convention comment matches MS-RDPBCGR. Independent verification confirmed three concrete defects: timezone name truncation counts Unicode scalars instead of UTF-16 code units, so astral characters overflow the fixed 64-byte PDU name fields and shift subsequent Client Info PDU fields (the encoder pads but never truncates); the invalid performance_flags fallback stores an empty flag set while logging and previously shipping a non-empty default; and desktop_scale_factor silently floors fractional values and forwards values the connector itself treats as out of range. Remaining candidates are low severity: an inaccurate doc-comment premis…
github-actions
Bot
added
ai-reviewed/1
GitHub Actions
Inactive
github-actions
Bot
removed
needs-author-action
The desktop_scale_factor, performance_flags and timezone_info extensions accepted any JS number and truncated it on the way to the wire. - desktop_scale_factor now honours the MS-RDPBCGR 2.2.1.13.1.2 window of 100% to 500%, keeps 0 as the "server picks" default, and rejects a fractional devicePixelRatio instead of flooring 1.5 to the out-of-range value 1. - performance_flags falls back to PerformanceFlags::default() rather than an empty set, so a type-invalid value keeps the behaviour the client advertised before the extension existed. - Timezone names are truncated on UTF-16 code units, not char scalars. StandardName and DaylightName are fixed at 64 bytes and the encoder only pads them, so an astral-plane name overflowed the field and shifted the rest of the Client Info PDU. build_config keeps its original signature and the four values are assigned on the returned config, which drops the too_many_arguments suppression and unbreaks the test helper that was still calling the old signature. Also states the DST trade-off accurately and covers the new pure helpers with tests.
GitHub Actions
Inactive
github-actions
Bot
added
size/M
|
Benoît Cortier (@CBenoit) hi, i'm changed the code follow your review. |
GitHub Actions
Inactive
GitHub Actions
Inactive
github-actions
Bot
added
the
automation-failed
There was a problem hiding this comment.
The PR wires four existing connector Config options (pointer_software_rendering, desktop_scale_factor, performance_flags, timezone_info) through the WASM extension mechanism in ironrdp-web session.rs, matching the maintainer-requested extension design. Independent verification: all new SessionBuilderInner defaults equal the build_config defaults they override, so existing sessions keep identical behaviour; the connector already consumes all four fields; the new parsers match MS-RDPBCGR constraints (100-500 integer percentages with 0 as the server default, whole-number bitmask in 0..=u32::MAX, bias within plus/minus 1440 with zeroed transition dates and biases); and truncate_timezone_name is genuinely required because TimezoneInfo::encode only pads the 64-byte name field. The get_property/get_str refactor is a small same-file dedup used by the new code. Two valid low-severity issues remain: the performance_flags extension leaks the internal bitflag layout to JS with no exported constan…
Reduced coverage: optional reviewer code-compressor was unavailable.
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
| /// Reads the `performance_flags` extension value as a `PerformanceFlags` bitmask. | ||
| /// | ||
| /// JS numbers are doubles, so a value that is not a whole number in | ||
| /// `0..=u32::MAX` (a ratio, a negative number, or something past `u32::MAX`) | ||
| /// cannot be a bitmask, and casting it would silently truncate it. Such a value | ||
| /// falls back to `PerformanceFlags::default()` rather than to an empty set, so | ||
| /// a type-invalid value keeps advertising the behaviour the client had before | ||
| /// this extension existed. | ||
| /// | ||
| /// Bits outside the known set are dropped rather than rejected, matching how | ||
| /// the connector treats the flags it forwards. | ||
| fn parse_performance_flags(value: f64) -> PerformanceFlags { | ||
| let is_bitmask = value.is_finite() && value >= 0.0 && value.fract() == 0.0 && value <= f64::from(u32::MAX); | ||
|
|
||
| if !is_bitmask { | ||
| warn!(value, "Invalid performance flags; using default"); | ||
| return PerformanceFlags::default(); | ||
| } | ||
|
|
||
| #[expect(clippy::as_conversions, clippy::cast_possible_truncation, clippy::cast_sign_loss)] | ||
| let bits = value as u32; | ||
|
|
||
| PerformanceFlags::from_bits_truncate(bits) |
There was a problem hiding this comment.
[skeptical] performance_flags extension exposes a raw bitflag layout with silently dropped unknown bits — low 🟡 — The extension hands the internal PerformanceFlags bit layout to JS as a bare f64 with no named constants exported anywhere in the repository, so consumers must hardcode magic numbers. parse_performance_flags uses PerformanceFlags::from_bits_truncate without logging dropped bits; the warn path only fires for type-invalid values. A transposed or guessed bit (e.g. 0x0200 instead of 0x0100) is silently discarded, so a browser client can believe it disabled wallpaper and theming while silently advertising them - the bandwidth-sensitive scenario the doc comment motivates. A warn on dropped bits or named flag arguments would make the failure observable; as written the misconfiguration is undetectable from the JS side.
| |timezone_info: JsValue| { | ||
| match parse_timezone_info(timezone_info) { | ||
| Ok(info) => self.0.borrow_mut().timezone_info = info, | ||
| Err(error) => { | ||
| let reason = iron_remote_desktop::IronError::backtrace(&error); | ||
| warn!(%reason, "Invalid timezone_info; falling back to default"); | ||
| } | ||
| } |
There was a problem hiding this comment.
[skeptical] timezone_info error path logs 'falling back to default' but keeps the previously set value — low 🟡 — On parse failure the Err arm only warns; self.0.borrow_mut().timezone_info is not assigned, so if the timezone_info extension was called earlier with a valid payload the session keeps that stale value, contradicting the 'falling back to default' log line. The sibling parsers match their logs: parse_desktop_scale_factor returns 0 and parse_performance_flags returns PerformanceFlags::default(). The inconsistency is diagnostic-level (a misleading warn during failed setup) plus a behavioural divergence between the three new extensions that maintainers must reason about for every future fallback. Applying the same fall-back-to-default semantics, or rewording the log, resolves it.
github-actions
Bot
added
ai-reviewed/2
No description provided.