fix(client): address PR #1115 review feedback

crates/ironrdp-client/Cargo.toml

@@ -84,7 +84,6 @@ smallvec = "1.15"
 tap = "1"
 semver = "1"
 raw-window-handle = "0.6"
-uuid = { version = "1.20" }
 x509-cert = { version = "0.2", default-features = false, features = ["std"] }
 url = "2"
 

crates/ironrdp-client/README.md

@@ -47,7 +47,7 @@ Property precedence is:
 2. `.rdp` file values
 3. Defaults and interactive prompts
 
-Unknown or unsupported `.rdp` properties are ignored and do not fail parsing. Parse issues and skipped properties are logged at debug level.
+Unknown or unsupported `.rdp` properties are ignored and do not fail parsing.
 
 ## Configuring log filter directives
 

crates/ironrdp-client/src/config.rs

@@ -12,78 +12,17 @@ use ironrdp::pdu::rdp::capability_sets::{client_codecs_capabilities, MajorPlatfo
 use ironrdp::pdu::rdp::client_info::{PerformanceFlags, TimezoneInfo};
 use ironrdp_mstsgu::GwConnectTarget;
 use tap::prelude::*;
-use tracing::debug;
 use url::Url;
 
 const DEFAULT_WIDTH: u16 = 1920;
 const DEFAULT_HEIGHT: u16 = 1080;
-const REDACTED_RDP_VALUE: &str = "*omitted*";
-const SUPPORTED_RDP_PROPERTIES: &[&str] = &[
-    "full address",
-    "alternate full address",
-    "server port",
-    "username",
-    "domain",
-    "enablecredsspsupport",
-    "gatewayhostname",
-    "gatewayusagemethod",
-    "gatewaycredentialssource",
-    "gatewayusername",
-    "gatewaypassword",
-    "kdcproxyname",
-    "kdcproxyurl",
-    "alternate shell",
-    "shell working directory",
-    "redirectclipboard",
-    "audiomode",
-    "desktopwidth",
-    "desktopheight",
-    "desktopscalefactor",
-    "compression",
-    "ClearTextPassword",
-];
-
-const SENSITIVE_RDP_PROPERTIES: &[&str] = &["ClearTextPassword", "GatewayPassword", "gatewaypassword"];
-
-fn is_supported_rdp_property(key: &str) -> bool {
-    SUPPORTED_RDP_PROPERTIES
-        .iter()
-        .any(|supported| supported.eq_ignore_ascii_case(key))
-}
-
-fn is_sensitive_rdp_property(key: &str) -> bool {
-    SENSITIVE_RDP_PROPERTIES
-        .iter()
-        .any(|sensitive| sensitive.eq_ignore_ascii_case(key))
-}
-
-fn redacted_rdp_line(line: &str) -> String {
-    let mut parts = line.splitn(3, ':');
-    let property = parts.next().unwrap_or("<unknown>");
-
-    if is_sensitive_rdp_property(property) {
-        format!("{property}:*:{REDACTED_RDP_VALUE}")
-    } else {
-        line.to_owned()
-    }
-}
 
-fn rdp_u16_property(value: Option<i64>, property: &'static str) -> Option<u16> {
-    value.and_then(|value| {
-        u16::try_from(value).ok().or_else(|| {
-            debug!(%property, value, "Ignored .RDP property with out-of-range value");
-            None
-        })
-    })
+fn rdp_u16_property(value: Option<i64>) -> Option<u16> {
+    value.and_then(|value| u16::try_from(value).ok())
 }
 
-fn rdp_u32_property(value: Option<i64>, property: &'static str) -> Option<u32> {
-    value.and_then(|value| {
-        u32::try_from(value).ok().or_else(|| {
-            debug!(%property, value, "Ignored .RDP property with out-of-range value");
-            None
-        })
-    })
+fn rdp_u32_property(value: Option<i64>) -> Option<u32> {
+    value.and_then(|value| u32::try_from(value).ok())
 }
 
 fn normalize_kdc_proxy_url_from_name(name: &str) -> String {
@@ -98,14 +37,7 @@ fn should_use_gateway_from_rdp(gateway_usage_method: Option<i64>, has_gateway_ho
     match gateway_usage_method {
         Some(0 | 4) => false,
         Some(1..=3) => true,
-        Some(value) => {
-            debug!(
-                property = "gatewayusagemethod",
-                value, "Ignored .RDP property with unsupported value"
-            );
-            has_gateway_host
-        }
-        None => has_gateway_host,
+        _ => has_gateway_host,
     }
 }
 
@@ -454,46 +386,7 @@ impl Config {
             let input =
                 std::fs::read_to_string(&rdp_file).with_context(|| format!("failed to read {}", rdp_file.display()))?;
 
-            if let Err(errors) = ironrdp_rdpfile::load(&mut properties, &input) {
-                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"
-                            );
-                        }
-                        ironrdp_rdpfile::ErrorKind::InvalidValue { ty, value } => {
-                            debug!(
-                                rdp_file = %rdp_file.display(),
-                                line = error.line,
-                                %ty,
-                                value = %value,
-                                "Skipped .RDP property with invalid value"
-                            );
-                        }
-                        ironrdp_rdpfile::ErrorKind::MalformedLine { line } => {
-                            let redacted_line = redacted_rdp_line(line);
-                            debug!(
-                                rdp_file = %rdp_file.display(),
-                                line = error.line,
-                                malformed_line = %redacted_line,
-                                "Skipped malformed .RDP property line"
-                            );
-                        }
-                    }
-                }
-            }
-
-            for (key, _) in properties.iter() {
-                let property = key.as_ref();
-
-                if !is_supported_rdp_property(property) {
-                    debug!(rdp_file = %rdp_file.display(), %property, "Ignored unsupported .RDP property");
-                }
-            }
+            let _ = ironrdp_rdpfile::load(&mut properties, &input);
         }
 
         let has_gateway_host = properties.gateway_hostname().is_some();
@@ -513,30 +406,9 @@ impl Config {
                 gw_pass: String::new(),
                 server: String::new(), // TODO: non-standard port? also dont use here?
             });
-        } else if has_gateway_host && !use_gateway_from_rdp {
-            debug!("Ignored gatewayhostname due to gatewayusagemethod policy");
         }
 
         if let Some(ref mut gw) = gw {
-            if let Some(gateway_credentials_source) = properties.gateway_credentials_source() {
-                match gateway_credentials_source {
-                    0 | 3 | 4 => {}
-                    1 | 2 | 5 => {
-                        debug!(
-                            property = "gatewaycredentialssource",
-                            value = gateway_credentials_source,
-                            "Unsupported gateway credential source; falling back to username/password"
-                        );
-                    }
-                    value => {
-                        debug!(
-                            property = "gatewaycredentialssource",
-                            value, "Ignored .RDP property with unsupported value"
-                        );
-                    }
-                }
-            }
-
             gw.gw_user = if let Some(gw_user) = args
                 .gw_user
                 .or_else(|| properties.gateway_username().map(str::to_owned))
@@ -649,18 +521,7 @@ impl Config {
             args.clipboard_type
         };
 
-        let enable_audio_playback = match properties.audio_mode() {
-            Some(0) => true,
-            Some(1 | 2) => false,
-            Some(value) => {
-                debug!(
-                    property = "audiomode",
-                    value, "Ignored .RDP property with unsupported value"
-                );
-                true
-            }
-            None => true,
-        };
+        let enable_audio_playback = !matches!(properties.audio_mode(), Some(1 | 2));
 
         let compression_enabled = if !args.compression_enabled {
             false
@@ -674,28 +535,21 @@ impl Config {
             None
         };
 
-        let desktop_width = rdp_u16_property(properties.desktop_width(), "desktopwidth").unwrap_or(DEFAULT_WIDTH);
-        let desktop_height = rdp_u16_property(properties.desktop_height(), "desktopheight").unwrap_or(DEFAULT_HEIGHT);
-        let desktop_scale_factor =
-            rdp_u32_property(properties.desktop_scale_factor(), "desktopscalefactor").unwrap_or(0);
+        let desktop_width = rdp_u16_property(properties.desktop_width()).unwrap_or(DEFAULT_WIDTH);
+        let desktop_height = rdp_u16_property(properties.desktop_height()).unwrap_or(DEFAULT_HEIGHT);
+        let desktop_scale_factor = rdp_u32_property(properties.desktop_scale_factor()).unwrap_or(0);
 
         let kdc_proxy_url = properties
             .kdc_proxy_url()
             .map(str::to_owned)
             .or_else(|| properties.kdc_proxy_name().map(normalize_kdc_proxy_url_from_name));
 
-        let kerberos_config = kdc_proxy_url.and_then(|kdc_proxy_url| {
-            Url::parse(&kdc_proxy_url)
-                .ok()
-                .map(|url| connector::credssp::KerberosConfig {
-                    kdc_proxy_url: Some(url),
-                    hostname: None,
-                })
-                .or_else(|| {
-                    debug!(%kdc_proxy_url, "Ignored invalid KDC proxy URL from .RDP property");
-                    None
-                })
-        });
+        let kerberos_config = kdc_proxy_url
+            .and_then(|kdc_proxy_url| Url::parse(&kdc_proxy_url).ok())
+            .map(|url| connector::credssp::KerberosConfig {
+                kdc_proxy_url: Some(url),
+                hostname: None,
+            });
 
         let connector = connector::Config {
             credentials: Credentials::UsernamePassword { username, password },

crates/ironrdp-client/src/rdp.rs

@@ -292,7 +292,7 @@ async fn connect(
     )
     .await?;
 
-    debug!("Connection finalized");
+    debug!(?connection_result);
 
     Ok((connection_result, upgraded_framed))
 }

crates/ironrdp-testsuite-extra/Cargo.toml

@@ -16,12 +16,14 @@ anyhow = "1.0"
 async-trait = "0.1"
 ironrdp = { path = "../ironrdp", features = ["server", "pdu", "connector", "session", "dvc", "echo"] }
 ironrdp-async.path = "../ironrdp-async"
+ironrdp-client = { path = "../ironrdp-client" }
 ironrdp-tokio.path = "../ironrdp-tokio"
 ironrdp-tls = { path = "../ironrdp-tls", features = ["rustls"] }
 semver = "1.0"
 tracing = { version = "0.1", features = ["log"] }
 tracing-subscriber = { version = "0.3", features = ["env-filter"] }
 tokio = { version = "1", features = ["sync", "time"] }
+uuid = { version = "1", features = ["v4"] }
 
 [lints]
 workspace = true

crates/ironrdp-client/tests/config_rdp_merge.rs → crates/ironrdp-testsuite-extra/tests/rdp_client_config.rs

@@ -1,32 +1,42 @@
 #![allow(unused_crate_dependencies)]
+#![allow(clippy::unwrap_used, reason = "unwrap is fine in tests")]
 
 use std::fs;
 use std::path::PathBuf;
 
 use ironrdp_client::config::{ClipboardType, Config};
 use uuid::Uuid;
 
-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
+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 }
+    }
+}
+
+impl Drop for TempRdpFile {
+    fn drop(&mut self) {
+        let _ = fs::remove_file(&self.path);
+    }
 }
 
 fn parse_config_from_rdp(content: &str, extra_args: &[&str]) -> Config {
-    let rdp_file = write_rdp_file(content);
+    let rdp_file = TempRdpFile::new(content);
 
     let mut args = vec![
         "ironrdp-client".to_owned(),
         "--rdp-file".to_owned(),
-        rdp_file.display().to_string(),
+        rdp_file.path.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);
-
-    result.expect("failed to parse client config")
+    Config::parse_from(args).expect("failed to parse client config")
 }
 
 #[test]
@@ -126,3 +136,41 @@ fn invalid_audiomode_falls_back_to_audio_playback_enabled() {
 
     assert!(config.connector.enable_audio_playback);
 }
+
+#[test]
+fn desktop_dimensions_are_parsed_from_rdp_file() {
+    let config = parse_config_from_rdp(
+        "full address:s:rdp.example.com\nusername:s:test-user\nClearTextPassword:s:test-pass\ndesktopwidth:i:1024\ndesktopheight:i:768\ndesktopscalefactor:i:125\n",
+        &[],
+    );
+
+    assert_eq!(config.connector.desktop_size.width, 1024);
+    assert_eq!(config.connector.desktop_size.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\nusername:s:test-user\nClearTextPassword:s:test-pass\n",
+        &[],
+    );
+
+    let invalid_config = parse_config_from_rdp(
+        "full address:s:rdp.example.com\nusername:s:test-user\nClearTextPassword:s:test-pass\ndesktopwidth:i:-1\ndesktopheight:i:-1\ndesktopscalefactor:i:99999999999\n",
+        &[],
+    );
+
+    assert_eq!(
+        invalid_config.connector.desktop_size.width,
+        default_config.connector.desktop_size.width
+    );
+    assert_eq!(
+        invalid_config.connector.desktop_size.height,
+        default_config.connector.desktop_size.height
+    );
+    assert_eq!(
+        invalid_config.connector.desktop_scale_factor,
+        default_config.connector.desktop_scale_factor
+    );
+}

xtask/src/cov.rs

@@ -53,7 +53,6 @@ pub fn report(sh: &Shell, html_report: bool) -> anyhow::Result<()> {
 
     if html_report {
         cmd!(sh, "{CARGO} llvm-cov --html")
-            .arg("--no-cfg-coverage")
             .arg("--ignore-filename-regex")
             .arg(COV_IGNORE_REGEX)
             .run()?;
@@ -284,7 +283,6 @@ impl CoverageReport {
         let output = cmd!(
             sh,
             "{CARGO} llvm-cov
-            --no-cfg-coverage
             --ignore-filename-regex {COV_IGNORE_REGEX}
             --json"
         )