From 62b4d6f9463278a099406b09c59e936e4fcdc884 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 25 Jun 2026 12:38:25 -0400 Subject: [PATCH 1/6] test(winrm): negative tests for FromXml malformed-input rejection Pin the rejection behavior the hardening round added: mixed-content leaf rejection, Empty non-empty rejection, single-child descend (zero/multiple/ wrong-name), duplicate singleton (Command/CommandState/derived ExitCode), and duplicate selector/option keys. --- crates/ironposh-winrm/src/cores/tag.rs | 29 ++++++++++++++++ crates/ironposh-winrm/src/cores/tag_value.rs | 33 +++++++++++++++++++ crates/ironposh-winrm/src/rsp/commandline.rs | 25 ++++++++++++++ crates/ironposh-winrm/src/rsp/receive.rs | 27 +++++++++++++++ .../src/ws_management/header.rs | 26 +++++++++++++++ 5 files changed, 140 insertions(+) diff --git a/crates/ironposh-winrm/src/cores/tag.rs b/crates/ironposh-winrm/src/cores/tag.rs index e9efbba..b49c5b9 100644 --- a/crates/ironposh-winrm/src/cores/tag.rs +++ b/crates/ironposh-winrm/src/cores/tag.rs @@ -298,4 +298,33 @@ mod tests { .expect("nested CommandResponse/CommandId should parse"); assert_eq!(tag.value.value.0.to_string().to_uppercase(), uuid); } + + /// The descend branch must reject a wrapper with no element child rather + /// than silently yielding a default. + #[test] + fn nested_tag_rejects_no_child() { + let xml = format!(r#""#); + let doc = parse(&xml).unwrap(); + assert!(CommandResponse::from_xml(doc.root_element()).is_err()); + } + + /// More than one child element is ambiguous; the descend branch must reject + /// it rather than silently picking the first. + #[test] + fn nested_tag_rejects_multiple_children() { + let uuid = "2D6534D0-6B12-40E3-B773-CBA26459CFA8"; + let xml = format!( + r#"{uuid}{uuid}"# + ); + let doc = parse(&xml).unwrap(); + assert!(CommandResponse::from_xml(doc.root_element()).is_err()); + } + + #[test] + fn nested_tag_rejects_wrong_child_name() { + let xml = + format!(r#""#); + let doc = parse(&xml).unwrap(); + assert!(CommandResponse::from_xml(doc.root_element()).is_err()); + } } diff --git a/crates/ironposh-winrm/src/cores/tag_value.rs b/crates/ironposh-winrm/src/cores/tag_value.rs index 39ea8ce..05eb4d0 100644 --- a/crates/ironposh-winrm/src/cores/tag_value.rs +++ b/crates/ironposh-winrm/src/cores/tag_value.rs @@ -211,3 +211,36 @@ impl<'a> FromXml<'a> for ReadOnlyUnParsed<'a> { Ok(ReadOnlyUnParsed::Node(node)) } } + +#[cfg(test)] +mod tests { + use super::*; + use ironposh_xml::parser::parse; + + #[test] + fn text_leaf_rejects_mixed_content() { + let doc = parse("text").unwrap(); + assert!(Text::from_xml(doc.root_element()).is_err()); + } + + #[test] + fn text_leaf_trims_surrounding_whitespace() { + let doc = parse(" hello ").unwrap(); + assert_eq!( + Text::from_xml(doc.root_element()).unwrap().as_ref(), + "hello" + ); + } + + #[test] + fn empty_rejects_text_content() { + let doc = parse("nope").unwrap(); + assert!(Empty::from_xml(doc.root_element()).is_err()); + } + + #[test] + fn empty_accepts_whitespace_only() { + let doc = parse(" ").unwrap(); + assert!(Empty::from_xml(doc.root_element()).is_ok()); + } +} diff --git a/crates/ironposh-winrm/src/rsp/commandline.rs b/crates/ironposh-winrm/src/rsp/commandline.rs index edb2894..ab41c58 100644 --- a/crates/ironposh-winrm/src/rsp/commandline.rs +++ b/crates/ironposh-winrm/src/rsp/commandline.rs @@ -65,3 +65,28 @@ impl<'a> FromXml<'a> for CommandLineValue { Ok(Self { command, arguments }) } } + +#[cfg(test)] +mod tests { + use super::*; + use ironposh_xml::parser::parse; + + const RSP: &str = "http://schemas.microsoft.com/wbem/wsman/1/windows/shell"; + + #[test] + fn rejects_duplicate_command() { + let xml = format!( + r#"ab"# + ); + let doc = parse(&xml).unwrap(); + assert!(CommandLineValue::from_xml(doc.root_element()).is_err()); + } + + #[test] + fn empty_command_element_is_none() { + let xml = format!(r#""#); + let doc = parse(&xml).unwrap(); + let value = CommandLineValue::from_xml(doc.root_element()).unwrap(); + assert!(value.command.is_none()); + } +} diff --git a/crates/ironposh-winrm/src/rsp/receive.rs b/crates/ironposh-winrm/src/rsp/receive.rs index aa39cd8..a553613 100644 --- a/crates/ironposh-winrm/src/rsp/receive.rs +++ b/crates/ironposh-winrm/src/rsp/receive.rs @@ -129,3 +129,30 @@ impl<'a> FromXml<'a> for ReceiveResponseValue<'a> { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use ironposh_xml::parser::parse; + + const RSP: &str = "http://schemas.microsoft.com/wbem/wsman/1/windows/shell"; + + #[test] + fn rejects_duplicate_command_state() { + let xml = format!( + r#""# + ); + let doc = parse(&xml).unwrap(); + assert!(ReceiveResponseValue::from_xml(doc.root_element()).is_err()); + } + + /// The `#[derive(FromXml)]` singleton field must reject a second binding. + #[test] + fn derive_rejects_duplicate_exit_code() { + let xml = format!( + r#"01"# + ); + let doc = parse(&xml).unwrap(); + assert!(CommandStateValue::from_xml(doc.root_element()).is_err()); + } +} diff --git a/crates/ironposh-winrm/src/ws_management/header.rs b/crates/ironposh-winrm/src/ws_management/header.rs index 738cd50..20cf156 100644 --- a/crates/ironposh-winrm/src/ws_management/header.rs +++ b/crates/ironposh-winrm/src/ws_management/header.rs @@ -141,3 +141,29 @@ impl<'a> FromXml<'a> for OptionSetValue { Ok(Self { options }) } } + +#[cfg(test)] +mod tests { + use super::*; + use ironposh_xml::parser::parse; + + const W: &str = "http://schemas.dmtf.org/wbem/wsman/1/wsman.xsd"; + + #[test] + fn rejects_duplicate_selector_name() { + let xml = format!( + r#"ab"# + ); + let doc = parse(&xml).unwrap(); + assert!(SelectorSetValue::from_xml(doc.root_element()).is_err()); + } + + #[test] + fn rejects_duplicate_option_name() { + let xml = format!( + r#"TRUEFALSE"# + ); + let doc = parse(&xml).unwrap(); + assert!(OptionSetValue::from_xml(doc.root_element()).is_err()); + } +} From e51feac85f29f9f7682eb780f26e2e0ea2f37b8c Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 25 Jun 2026 12:38:25 -0400 Subject: [PATCH 2/6] test(e2e): command-not-found case in the command matrix A non-existent command surfaces as an error record (non-fatal), printed via render_concise to stdout while the process still exits 0. Asserts both, across all auth methods. --- .../tests/e2e/command_matrix.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/crates/ironposh-client-tokio/tests/e2e/command_matrix.rs b/crates/ironposh-client-tokio/tests/e2e/command_matrix.rs index c080f8f..c4a37d0 100644 --- a/crates/ironposh-client-tokio/tests/e2e/command_matrix.rs +++ b/crates/ironposh-client-tokio/tests/e2e/command_matrix.rs @@ -100,6 +100,23 @@ fn real_server_command_matrix_all_auths() { "C3 retry marker missing for auth={auth}. stdout={stdout} stderr={stderr}" ); } + + // C4: a non-existent command. PowerShell reports CommandNotFound as an + // error record (not a process-fatal failure), so the client prints it via + // `render_concise` to stdout and still exits 0. The message echoes the + // command name, which is what we assert on (locale-independent). + { + let bogus = "Get-IronPoshDefinitelyNotARealCommand"; + let (ok, stdout, stderr) = run_noninteractive(auth, bogus); + assert!( + ok, + "C4 process should exit 0 (error record is non-fatal) for auth={auth}. stdout={stdout} stderr={stderr}" + ); + assert!( + stdout.contains(bogus), + "C4 command-not-found error missing the command name for auth={auth}. stdout={stdout} stderr={stderr}" + ); + } } let _ = Instant::now(); // keep `Instant` import used without adding behavior } From 5e395ac5977fb2443bdbd28ae4af049ca409d3ef Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 25 Jun 2026 13:41:17 -0400 Subject: [PATCH 3/6] refactor(winrm): address review feedback across the stack Polish from the independent review pass (no behavior change except a clearer error message): remove dead commented-out visitor block in ws_addressing; correct the tag! marker-visibility doc; drop two what/how comments; loosen the paste version pin; clearer 'found none' diagnostic for an empty tag wrapper; restore optional-permutation derive tests and add mixed-content rejection tests for numeric/uuid leaves. --- crates/ironposh-winrm/Cargo.toml | 2 +- crates/ironposh-winrm/src/cores/tag.rs | 16 ++--- crates/ironposh-winrm/src/cores/tag_name.rs | 4 +- crates/ironposh-winrm/src/cores/tag_value.rs | 12 ++++ crates/ironposh-winrm/src/macros.rs | 3 +- crates/ironposh-winrm/src/rsp/send.rs | 6 +- crates/ironposh-winrm/src/test_macro.rs | 19 ++++++ .../ironposh-winrm/src/ws_addressing/mod.rs | 59 ------------------- 8 files changed, 46 insertions(+), 75 deletions(-) diff --git a/crates/ironposh-winrm/Cargo.toml b/crates/ironposh-winrm/Cargo.toml index a28fbc8..f61e54b 100644 --- a/crates/ironposh-winrm/Cargo.toml +++ b/crates/ironposh-winrm/Cargo.toml @@ -13,7 +13,7 @@ ironposh-xml = { path = "../ironposh-xml" } ironposh-macros = { path = "../ironposh-macros" } byteorder = "1.5.0" base64 = "0.22.1" -paste = "1.0.15" +paste = "1.0" uuid = { version = "1.0", features = ["v4"] } [dev-dependencies] diff --git a/crates/ironposh-winrm/src/cores/tag.rs b/crates/ironposh-winrm/src/cores/tag.rs index b49c5b9..8f038bf 100644 --- a/crates/ironposh-winrm/src/cores/tag.rs +++ b/crates/ironposh-winrm/src/cores/tag.rs @@ -179,12 +179,13 @@ where let mut elements = node .children() .filter(ironposh_xml::parser::Node::is_element); - let only = elements - .next() - .ok_or_else(|| ironposh_xml::XmlError::XmlInvalidTag { - expected: N::TAG_NAME.to_string(), - found: node.tag_name().name().to_string(), - })?; + let only = elements.next().ok_or_else(|| { + ironposh_xml::XmlError::InvalidXml(format!( + "expected a <{}> child in <{}>, found none", + N::TAG_NAME, + node.tag_name().name() + )) + })?; if elements.next().is_some() { return Err(ironposh_xml::XmlError::InvalidXml(format!( "expected exactly one child element in <{}>", @@ -299,8 +300,7 @@ mod tests { assert_eq!(tag.value.value.0.to_string().to_uppercase(), uuid); } - /// The descend branch must reject a wrapper with no element child rather - /// than silently yielding a default. + /// A wrapper that yields no usable child must be rejected, not defaulted. #[test] fn nested_tag_rejects_no_child() { let xml = format!(r#""#); diff --git a/crates/ironposh-winrm/src/cores/tag_name.rs b/crates/ironposh-winrm/src/cores/tag_name.rs index 69d9f73..3eab122 100644 --- a/crates/ironposh-winrm/src/cores/tag_name.rs +++ b/crates/ironposh-winrm/src/cores/tag_name.rs @@ -43,14 +43,14 @@ tag!(DesiredStream = Text<'a> => WsmanShell); tag!(Stream = Text<'a> => WsmanShell); tag!(ExitCode = I32 => WsmanShell); tag!(CommandId = WsUuid => WsmanShell); -tag!(CommandResponse = CommandId<'a> => WsmanShell); // wraps a single CommandId child +tag!(CommandResponse = CommandId<'a> => WsmanShell); tag!(Command = Text<'a> => WsmanShell); tag!(Arguments = Text<'a> => WsmanShell); tag!(DisconnectResponse = Empty => WsmanShell); tag!(Reconnect = Empty => WsmanShell); tag!(ReconnectResponse = Empty => WsmanShell); tag!(SignalCode = "Code": Text<'a> => WsmanShell); -tag!(Signal = SignalCode<'a> => WsmanShell); // wraps a single Code child +tag!(Signal = SignalCode<'a> => WsmanShell); tag!(SignalResponse = Empty => WsmanShell); tag!(CreationXml = "creationXml": Text<'a> => PowerShellRemoting); diff --git a/crates/ironposh-winrm/src/cores/tag_value.rs b/crates/ironposh-winrm/src/cores/tag_value.rs index 05eb4d0..3fadf52 100644 --- a/crates/ironposh-winrm/src/cores/tag_value.rs +++ b/crates/ironposh-winrm/src/cores/tag_value.rs @@ -232,6 +232,18 @@ mod tests { ); } + #[test] + fn numeric_leaf_rejects_mixed_content() { + let doc = parse("1").unwrap(); + assert!(U32::from_xml(doc.root_element()).is_err()); + } + + #[test] + fn wsuuid_leaf_rejects_mixed_content() { + let doc = parse("uuid:2d6534d0-6b12-40e3-b773-cba26459cfa8").unwrap(); + assert!(WsUuid::from_xml(doc.root_element()).is_err()); + } + #[test] fn empty_rejects_text_content() { let doc = parse("nope").unwrap(); diff --git a/crates/ironposh-winrm/src/macros.rs b/crates/ironposh-winrm/src/macros.rs index fa495c5..381a732 100644 --- a/crates/ironposh-winrm/src/macros.rs +++ b/crates/ironposh-winrm/src/macros.rs @@ -3,7 +3,8 @@ /// Generates a zero-sized `TagName` marker (`Tag`) that pins the element's /// wire name + namespace at the type level, plus an ergonomic type alias /// `Alias<'a> = Tag<'a, Value, AliasTag>` used in struct fields and construction. -/// The marker stays internal; everything else writes the alias. +/// The alias is the normal spelling; the `Tag` marker is also public, +/// written directly at the few `Tag::from_name` call sites. /// /// Forms: /// - `tag!(Get = Text<'a> => DmtfWsmanSchema)` — wire name is the alias ident. diff --git a/crates/ironposh-winrm/src/rsp/send.rs b/crates/ironposh-winrm/src/rsp/send.rs index 3606e63..f494eda 100644 --- a/crates/ironposh-winrm/src/rsp/send.rs +++ b/crates/ironposh-winrm/src/rsp/send.rs @@ -6,10 +6,8 @@ use ironposh_xml::{ mapping::{FromXml, NodeExt}, }; -// NOTE: this `Send` type alias shares its spelling with `std::marker::Send`. -// They live in different namespaces (type alias vs trait), so they don't -// collide here — but avoid writing a bare `Send` trait bound in modules that -// import this alias. +// `Send` here is this type alias, not `std::marker::Send` — don't write a bare +// `Send` trait bound in modules that import it. tag!(Send = SendValue<'a> => WsmanShell); /// Value for Send element containing multiple Stream elements diff --git a/crates/ironposh-winrm/src/test_macro.rs b/crates/ironposh-winrm/src/test_macro.rs index 048ccb6..4a23473 100644 --- a/crates/ironposh-winrm/src/test_macro.rs +++ b/crates/ironposh-winrm/src/test_macro.rs @@ -51,6 +51,25 @@ mod tests { assert!(parsed.opt_b.is_none()); } + #[test] + fn test_deserialize_optional_permutations() { + // Both optionals present. + let xml = format!( + r#"aboaob"# + ); + let doc = ironposh_xml::parser::parse(&xml).expect("parse"); + let parsed = TestStruct::from_xml(doc.root_element()).expect("deserialize"); + assert_eq!(parsed.opt_a.unwrap().value.as_ref(), "oa"); + assert_eq!(parsed.opt_b.unwrap().value.as_ref(), "ob"); + + // Neither optional present. + let xml = format!(r#"ab"#); + let doc = ironposh_xml::parser::parse(&xml).expect("parse"); + let parsed = TestStruct::from_xml(doc.root_element()).expect("deserialize"); + assert!(parsed.opt_a.is_none()); + assert!(parsed.opt_b.is_none()); + } + #[test] fn test_deserialize_missing_required_field() { let xml = format!(r#"only-a"#); diff --git a/crates/ironposh-winrm/src/ws_addressing/mod.rs b/crates/ironposh-winrm/src/ws_addressing/mod.rs index 4ab1396..4d733f0 100644 --- a/crates/ironposh-winrm/src/ws_addressing/mod.rs +++ b/crates/ironposh-winrm/src/ws_addressing/mod.rs @@ -9,62 +9,3 @@ tag!(ReplyTo = AddressValue<'a> => WsAddressing2004); pub struct AddressValue<'a> { pub url: Address<'a>, } - -// impl<'a> TagValue<'a> for AddressValue<'a> { -// fn append_to_element(self, element: ironposh_xml::builder::Element<'a>) -> ironposh_xml::builder::Element<'a> { -// let inner_element = self.url.into_element(); -// element.add_child(inner_element) -// } -// } - -// pub struct AddressVisitor<'a> { -// address: Option>, -// } - -// impl<'a> XmlVisitor<'a> for AddressVisitor<'a> { -// type Value = AddressValue<'a>; - -// fn visit_children( -// &mut self, -// children: impl Iterator>, -// ) -> Result<(), ironposh_xml::XmlError> { -// for child in children { -// if !child.is_element() { -// continue; -// } - -// match (child.tag_name().name(), child.tag_name().namespace()) { -// (Address::TAG_NAME, Address::NAMESPACE) => { -// let tag = Tag::from_node(child)?; -// self.address = Some(AddressValue { url: tag }); -// } -// _ => { -// warn!( -// "Unexpected child element in AddressValue: {}", -// child.tag_name().name() -// ); -// } -// } -// } - -// Ok(()) -// } - -// fn finish(self) -> Result { -// Ok(AddressValue { -// url: self.address.ok_or_else(|| { -// ironposh_xml::XmlError::NotSupposeToBeCalled { -// extra_info: "AddressValue must contain an Address element".to_string(), -// } -// }?), -// }) -// } -// } - -// impl<'a> XmlDeserialize<'a> for AddressValue<'a> { -// type Visitor = AddressVisitor<'a>; - -// fn visitor() -> Self::Visitor { -// AddressVisitor { address: None } -// } -// } From 2e0938bf9e48a37a29376cd4d597dfcfb7f14b24 Mon Sep 17 00:00:00 2001 From: Junyi Ou Date: Thu, 25 Jun 2026 14:09:48 -0400 Subject: [PATCH 4/6] harden(winrm): strict, namespace-correct XML rejection (review round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the independent Codex pass — reject malformed input uniformly rather than silently absorbing it: - attributes matched by (namespace-URI, local-name) with parse errors on known attributes propagated instead of dropped (cores/attribute.rs, cores/tag.rs) - /