feat(client): improve .rdp merge support and startup sizing#1115
feat(client): improve .rdp merge support and startup sizing#1115Marc-André Moreau (mamoreau-devolutions) wants to merge 2 commits intomasterfrom
Conversation
- map additional supported .rdp properties into native client config - ignore unsupported properties safely and redact sensitive values in logs - initialize startup window from configured desktop size - add regression tests for .rdp merge precedence and mapping
Marc-André Moreau (mamoreau-devolutions)
force-pushed
the
rdp-file
branch
from
73baa0b to
2cb39d1
Compare
Pass --no-cfg-coverage to cargo llvm-cov in xtask coverage commands to avoid dependency breakage on crates using cfg_attr(coverage, no_coverage).
Coverage Report 🤖 ⚙️Past: New: Diff: -1.80% [this comment will be updated automatically] |
There was a problem hiding this comment.
Pull request overview
This pull request enhances .rdp file support in the native IronRDP client by adding comprehensive property mapping, improving error handling, and initializing the client window from configured desktop dimensions.
Changes:
- Expanded
.rdpproperty support with 20+ properties including gateway, Kerberos, desktop sizing, audio, and clipboard configuration - Made
.rdpparsing tolerant by logging unsupported properties at debug level with redaction of sensitive values - Fixed client window initialization to use configured desktop dimensions instead of hardcoded values
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| xtask/src/cov.rs | Added --no-cfg-coverage flag to llvm-cov commands for improved coverage reporting |
| crates/ironrdp-rdpfile/README.md | Added documentation clarifying that property support depends on consumers |
| crates/ironrdp-client/tests/config_rdp_merge.rs | Added 8 integration tests verifying .rdp merge precedence and property mapping behavior |
| crates/ironrdp-client/src/rdp.rs | Updated connection functions to pass Kerberos config and simplified debug logging |
| crates/ironrdp-client/src/main.rs | Changed to use configured desktop size for initial window dimensions instead of hardcoded values |
| crates/ironrdp-client/src/config.rs | Implemented comprehensive .rdp property parsing with validation, redaction, and CLI precedence |
| crates/ironrdp-client/src/app.rs | Updated App to accept and use initial window size from configuration |
| crates/ironrdp-client/README.md | Documented all 22 currently supported .rdp properties with precedence rules |
| crates/ironrdp-cfg/src/lib.rs | Extended PropertySetExt trait with 14 new accessor methods for .rdp properties |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn write_rdp_file(content: &str) -> PathBuf { | ||
| let path = std::env::temp_dir().join(format!("ironrdp-client-rdp-{}.rdp", Uuid::new_v4())); | ||
| fs::write(&path, content).expect("failed to write temporary .rdp file"); | ||
| path | ||
| } | ||
|
|
||
| fn parse_config_from_rdp(content: &str, extra_args: &[&str]) -> Config { | ||
| let rdp_file = write_rdp_file(content); | ||
|
|
||
| let mut args = vec![ | ||
| "ironrdp-client".to_owned(), | ||
| "--rdp-file".to_owned(), | ||
| rdp_file.display().to_string(), | ||
| ]; | ||
|
|
||
| args.extend(extra_args.iter().map(|arg| (*arg).to_owned())); | ||
|
|
||
| let result = Config::parse_from(args); | ||
| let _ = fs::remove_file(rdp_file); |
There was a problem hiding this comment.
The temporary .rdp test file is not guaranteed to be cleaned up if Config::parse_from panics or if the test is interrupted. Consider using a RAII-style guard or implementing Drop for cleanup to ensure files are removed even on panic.
| fn write_rdp_file(content: &str) -> PathBuf { | |
| let path = std::env::temp_dir().join(format!("ironrdp-client-rdp-{}.rdp", Uuid::new_v4())); | |
| fs::write(&path, content).expect("failed to write temporary .rdp file"); | |
| path | |
| } | |
| fn parse_config_from_rdp(content: &str, extra_args: &[&str]) -> Config { | |
| let rdp_file = write_rdp_file(content); | |
| let mut args = vec![ | |
| "ironrdp-client".to_owned(), | |
| "--rdp-file".to_owned(), | |
| rdp_file.display().to_string(), | |
| ]; | |
| args.extend(extra_args.iter().map(|arg| (*arg).to_owned())); | |
| let result = Config::parse_from(args); | |
| let _ = fs::remove_file(rdp_file); | |
| struct TempRdpFile { | |
| path: PathBuf, | |
| } | |
| impl TempRdpFile { | |
| fn new(content: &str) -> Self { | |
| let path = | |
| std::env::temp_dir().join(format!("ironrdp-client-rdp-{}.rdp", Uuid::new_v4())); | |
| fs::write(&path, content).expect("failed to write temporary .rdp file"); | |
| TempRdpFile { path } | |
| } | |
| fn path(&self) -> &PathBuf { | |
| &self.path | |
| } | |
| } | |
| impl Drop for TempRdpFile { | |
| fn drop(&mut self) { | |
| let _ = fs::remove_file(&self.path); | |
| } | |
| } | |
| fn write_rdp_file(content: &str) -> TempRdpFile { | |
| TempRdpFile::new(content) | |
| } | |
| fn parse_config_from_rdp(content: &str, extra_args: &[&str]) -> Config { | |
| let rdp_file = write_rdp_file(content); | |
| let mut args = vec![ | |
| "ironrdp-client".to_owned(), | |
| "--rdp-file".to_owned(), | |
| rdp_file.path().display().to_string(), | |
| ]; | |
| args.extend(extra_args.iter().map(|arg| (*arg).to_owned())); | |
| let result = Config::parse_from(args); |
| ); | ||
|
|
||
| assert!(config.connector.enable_audio_playback); | ||
| } |
There was a problem hiding this comment.
Consider adding test coverage for the desktop dimension properties (desktopwidth, desktopheight, desktopscalefactor) to verify they are correctly parsed from RDP files and applied to the client configuration, including edge cases like out-of-range values.
| } | |
| } | |
| #[test] | |
| fn desktop_dimensions_are_parsed_from_rdp_file() { | |
| let config = parse_config_from_rdp( | |
| "full address:s:rdp.example.com\n\ | |
| username:s:test-user\n\ | |
| ClearTextPassword:s:test-pass\n\ | |
| desktopwidth:i:1024\n\ | |
| desktopheight:i:768\n\ | |
| desktopscalefactor:i:125\n", | |
| &[], | |
| ); | |
| assert_eq!(config.connector.desktop_width, 1024); | |
| assert_eq!(config.connector.desktop_height, 768); | |
| assert_eq!(config.connector.desktop_scale_factor, 125); | |
| } | |
| #[test] | |
| fn out_of_range_desktop_dimensions_fall_back_to_defaults() { | |
| let default_config = parse_config_from_rdp( | |
| "full address:s:rdp.example.com\n\ | |
| username:s:test-user\n\ | |
| ClearTextPassword:s:test-pass\n", | |
| &[], | |
| ); | |
| let invalid_config = parse_config_from_rdp( | |
| "full address:s:rdp.example.com\n\ | |
| username:s:test-user\n\ | |
| ClearTextPassword:s:test-pass\n\ | |
| desktopwidth:i:0\n\ | |
| desktopheight:i:0\n\ | |
| desktopscalefactor:i:1000\n", | |
| &[], | |
| ); | |
| assert_eq!( | |
| invalid_config.connector.desktop_width, | |
| default_config.connector.desktop_width | |
| ); | |
| assert_eq!( | |
| invalid_config.connector.desktop_height, | |
| default_config.connector.desktop_height | |
| ); | |
| assert_eq!( | |
| invalid_config.connector.desktop_scale_factor, | |
| default_config.connector.desktop_scale_factor | |
| ); | |
| } |
|
|
||
| if html_report { | ||
| cmd!(sh, "{CARGO} llvm-cov --html") | ||
| .arg("--no-cfg-coverage") |
There was a problem hiding this comment.
question: Why this modification? I think this should be reverted.
| .await?; | ||
|
|
||
| debug!(?connection_result); | ||
| debug!("Connection finalized"); |
There was a problem hiding this comment.
question: Why this change? It seems we are losing the ability to see the debug representation of the connection result when enabling debug logs
There was a problem hiding this comment.
I’m not sure why it’s named with the "_merge" suffix since there is nothing related to merging in here. Seems to be solely about verifying the resulting Config based on various .RDP files. I would put these tests in the testsuite-extra crate.
| for e in errors { | ||
| #[expect(clippy::print_stderr)] | ||
| { | ||
| eprintln!("Error when reading {}: {e}", rdp_file.display()) | ||
| for error in errors { | ||
| match &error.kind { | ||
| ironrdp_rdpfile::ErrorKind::UnknownType { ty } => { | ||
| debug!( | ||
| rdp_file = %rdp_file.display(), | ||
| line = error.line, | ||
| %ty, | ||
| "Skipped .RDP property with unsupported type" | ||
| ); | ||
| } |
There was a problem hiding this comment.
issue: I think the initial code was good and correct as-is. This is basically re-implementing the existing logic but using debug!. It also does not make sense to use debug! at this point, the logger is not yet initialized.
Benoît Cortier (CBenoit)
changed the title
2 participants
Summary
Validation