From 0ca939d191f973c238fec3464d1792398c0762c7 Mon Sep 17 00:00:00 2001 From: Greg Lamberson Date: Wed, 27 May 2026 13:42:03 -0500 Subject: [PATCH] feat(connector): surface ShareDataPdu variant in unexpected-PDU errors When the server sends a ShareControlPdu::Data wrapping a ShareDataPdu the client did not expect, the three error sites in legacy.rs and connection_activation.rs previously reported only "Data" via the outer ShareControlPdu::as_short_name(). For the common case where the server rejects a session by sending ServerSetErrorInfo, this hid the information the client actually needed to diagnose the failure. A new pub(crate) helper describe_unexpected_share_control_pdu drills into the Data wrapper and surfaces the inner ShareDataPdu variant name. When the inner variant is ServerSetErrorInfo, the ErrorInfo description is appended so callers can see why the server rejected the session without substring matching on the Reason string. The three sites in decode_share_data, decode_io_channel, and the ConnectionActivation::CapabilitiesExchange handler route through the helper. Non-Data variants continue to use the outer as_short_name(), so diagnostics for ServerDeactivateAll and ClientConfirmActive are preserved verbatim. Three unit tests cover the helper: a non-Data variant, a Data wrapper around a non-SetErrorInfo inner, and a Data wrapper around ServerSetErrorInfo carrying a ProtocolIndependentCode error. Refs #1232. --- .../src/connection_activation.rs | 2 +- crates/ironrdp-connector/src/legacy.rs | 78 ++++++++++++++++++- 2 files changed, 77 insertions(+), 3 deletions(-) diff --git a/crates/ironrdp-connector/src/connection_activation.rs b/crates/ironrdp-connector/src/connection_activation.rs index fa053676e..71b034558 100644 --- a/crates/ironrdp-connector/src/connection_activation.rs +++ b/crates/ironrdp-connector/src/connection_activation.rs @@ -147,7 +147,7 @@ impl Sequence for ConnectionActivationSequence { return Err(reason_err!( "ConnectionActivation::CapabilitiesExchange", "unexpected Share Control PDU during capabilities exchange: got {} (expected Server Demand Active PDU)", - share_control_ctx.pdu.as_short_name(), + legacy::describe_unexpected_share_control_pdu(&share_control_ctx.pdu), )); }; diff --git a/crates/ironrdp-connector/src/legacy.rs b/crates/ironrdp-connector/src/legacy.rs index e5da73b1c..f80fce18e 100644 --- a/crates/ironrdp-connector/src/legacy.rs +++ b/crates/ironrdp-connector/src/legacy.rs @@ -145,6 +145,29 @@ pub struct ShareDataCtx { pub pdu: rdp::headers::ShareDataPdu, } +/// Format an unexpected `ShareControlPdu` for error context. +/// +/// Drills into the `Data` wrapper to surface the inner `ShareDataPdu` variant +/// name; for `ServerSetErrorInfo` the `ErrorInfo` description is appended so +/// callers can see *why* the server rejected the session without resorting to +/// substring matching on the `Reason` string. +pub(crate) fn describe_unexpected_share_control_pdu(pdu: &rdp::headers::ShareControlPdu) -> String { + let rdp::headers::ShareControlPdu::Data(header) = pdu else { + return pdu.as_short_name().to_owned(); + }; + + let inner = &header.share_data_pdu; + if let rdp::headers::ShareDataPdu::ServerSetErrorInfo(payload) = inner { + format!( + "Data PDU wrapping {} ({})", + inner.as_short_name(), + payload.0.description(), + ) + } else { + format!("Data PDU wrapping {}", inner.as_short_name()) + } +} + pub fn decode_share_data(ctx: SendDataIndicationCtx<'_>) -> ConnectorResult { let ctx = decode_share_control(ctx)?; @@ -152,7 +175,7 @@ pub fn decode_share_data(ctx: SendDataIndicationCtx<'_>) -> ConnectorResult) -> ConnectorResult Err(reason_err!( "decode_io_channel", "received unexpected Share Control PDU: got {} (expected Data PDU or Server Deactivate All PDU)", - other.as_short_name(), + describe_unexpected_share_control_pdu(&other), )), } } + +#[cfg(test)] +mod tests { + use ironrdp_pdu::rdp::client_info::CompressionType; + use ironrdp_pdu::rdp::headers::{ + CompressionFlags, ServerDeactivateAll, ShareControlPdu, ShareDataHeader, ShareDataPdu, StreamPriority, + }; + use ironrdp_pdu::rdp::server_error_info::{ErrorInfo, ProtocolIndependentCode, ServerSetErrorInfoPdu}; + + use super::describe_unexpected_share_control_pdu; + + fn wrap_in_data(inner: ShareDataPdu) -> ShareControlPdu { + ShareControlPdu::Data(ShareDataHeader { + share_data_pdu: inner, + stream_priority: StreamPriority::Medium, + compression_flags: CompressionFlags::empty(), + compression_type: CompressionType::K8, + }) + } + + #[test] + fn non_data_variant_uses_outer_short_name() { + let pdu = ShareControlPdu::ServerDeactivateAll(ServerDeactivateAll); + assert_eq!(describe_unexpected_share_control_pdu(&pdu), "Server Deactivate All PDU"); + } + + #[test] + fn data_wrapping_non_set_error_info_surfaces_inner_short_name() { + let pdu = wrap_in_data(ShareDataPdu::Update(Vec::new())); + assert_eq!( + describe_unexpected_share_control_pdu(&pdu), + "Data PDU wrapping Update PDU" + ); + } + + #[test] + fn data_wrapping_set_error_info_surfaces_description() { + let error = ErrorInfo::ProtocolIndependentCode(ProtocolIndependentCode::ServerDeniedConnection); + let pdu = wrap_in_data(ShareDataPdu::ServerSetErrorInfo(ServerSetErrorInfoPdu(error))); + let described = describe_unexpected_share_control_pdu(&pdu); + + assert!( + described.starts_with("Data PDU wrapping Server Set Error Info PDU ("), + "unexpected prefix: {described}", + ); + assert!( + described.contains("Protocol independent error"), + "missing error category in description: {described}", + ); + } +}