From b8af0d8fa2f169c7d3a470bd83af3f57b60e344b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Sch=C3=A4fer?= Date: Fri, 20 Dec 2024 08:57:46 +0100 Subject: [PATCH] xmpp_parsers: `--features disable-validation` It was broken in multiple ways: - xso did not honour it: unknown children and attributes would cause a parse error even with `--features disable-validation` set on parsers. For this, we introduce a new feature flag on xso, `non-pedantic`, which defaults unknown children and attributes to discard instead of fail. Note that individual XSOs can still choose to be always pedantic or always lenient by explicitly declaring the intent via the `on_unknown_child` and `on_unknown_attribute` metas. - Many tests in `xmpp_parsers` were broken with `--features disable-validation`. They now all pass while *still* being rn with `disable-validation` set: In that case, they test that parsing in fact succeeds. --- parsers/Cargo.toml | 2 +- parsers/src/bind.rs | 1 + parsers/src/blocking.rs | 1 + parsers/src/bob.rs | 1 + parsers/src/data_forms_validate.rs | 8 +++++--- parsers/src/delay.rs | 1 + parsers/src/disco.rs | 1 + parsers/src/ecaps2.rs | 1 + parsers/src/eme.rs | 1 + parsers/src/forwarding.rs | 3 ++- parsers/src/hashes.rs | 1 + parsers/src/idle.rs | 1 + parsers/src/jingle.rs | 19 +++++++++---------- parsers/src/jingle_ft.rs | 26 +++++++++++++++++++++++--- parsers/src/media_element.rs | 1 + parsers/src/message_correct.rs | 1 + parsers/src/muc/muc.rs | 1 + parsers/src/muc/user.rs | 2 ++ parsers/src/occupant_id.rs | 1 + parsers/src/roster.rs | 9 ++++++++- parsers/src/sasl.rs | 2 +- parsers/src/stanza_id.rs | 1 + parsers/src/util/macro_tests.rs | 24 ++++++++++++++++++++++++ xso/Cargo.toml | 1 + xso/src/lib.rs | 18 ++++++++++++++---- 25 files changed, 104 insertions(+), 24 deletions(-) diff --git a/parsers/Cargo.toml b/parsers/Cargo.toml index 6f233c395bd3437f6fc4c0622cfbc10b56fea949..3b070eb31d5d39730f7024df17af204f3489ec26 100644 --- a/parsers/Cargo.toml +++ b/parsers/Cargo.toml @@ -32,7 +32,7 @@ uuid = { version = "1.9.1", features = ["v4"] } # Build xmpp-parsers to make components instead of clients. component = [] # Disable validation of unknown attributes. -disable-validation = [] +disable-validation = [ "xso/non-pedantic" ] # Enable serde support in jid crate serde = [ "jid/serde" ] # Enable some additional logging in helpers diff --git a/parsers/src/bind.rs b/parsers/src/bind.rs index d84866928aa39cbd3964c9db282d13c5a5cbec83..4ea5e4fa05b61db363af7a4edc3a91c5805ba353 100644 --- a/parsers/src/bind.rs +++ b/parsers/src/bind.rs @@ -77,6 +77,7 @@ impl From for Jid { mod tests { use super::*; use minidom::Element; + #[cfg(not(feature = "disable-validation"))] use xso::error::{Error, FromElementError}; #[cfg(target_pointer_width = "32")] diff --git a/parsers/src/blocking.rs b/parsers/src/blocking.rs index d41fe11c99040f46de85889d10905f2698da2b01..897f86572fb1546aae0acba6448efe6231240caf 100644 --- a/parsers/src/blocking.rs +++ b/parsers/src/blocking.rs @@ -62,6 +62,7 @@ pub struct Blocked; #[cfg(test)] mod tests { + #[cfg(not(feature = "disable-validation"))] use xso::error::{Error, FromElementError}; use super::*; diff --git a/parsers/src/bob.rs b/parsers/src/bob.rs index 1f09c8ec6f685f36ac5c743f7421b2b187627832..40cbd3df7e2457cc0684726baea325c3716a8f32 100644 --- a/parsers/src/bob.rs +++ b/parsers/src/bob.rs @@ -194,6 +194,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn unknown_child() { let elem: Element = "" .parse() diff --git a/parsers/src/data_forms_validate.rs b/parsers/src/data_forms_validate.rs index 373659310cb3b679caf1c632189225f0fc186839..57d8ce212e3fe462db76b4b2751ca2f513232ab9 100644 --- a/parsers/src/data_forms_validate.rs +++ b/parsers/src/data_forms_validate.rs @@ -448,7 +448,11 @@ mod tests { } #[test] - fn test_fails_with_invalid_children() -> Result<(), Error> { + #[cfg_attr( + feature = "disable-validation", + should_panic = "Validate::try_from(element).is_err()" + )] + fn test_fails_with_invalid_children() { let cases = [ r#""#, r#""#, @@ -460,7 +464,5 @@ mod tests { .expect(&format!("Failed to parse {}", case)); assert!(Validate::try_from(element).is_err()); } - - Ok(()) } } diff --git a/parsers/src/delay.rs b/parsers/src/delay.rs index 717115c2d84ea0fd9547acc08c51ef0e34739c49..c103fe5f90ddd1c7873951ea2ff5167078cc1234 100644 --- a/parsers/src/delay.rs +++ b/parsers/src/delay.rs @@ -81,6 +81,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = "" diff --git a/parsers/src/disco.rs b/parsers/src/disco.rs index 8428bd437249c90fd89d0149300f39524b236549..1cf51063e3f30f6cd03c6ebd0b6b331c8a8a5083 100644 --- a/parsers/src/disco.rs +++ b/parsers/src/disco.rs @@ -258,6 +258,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid() { let elem: Element = "" diff --git a/parsers/src/ecaps2.rs b/parsers/src/ecaps2.rs index e31dc3cfb0793bff274ec6b3bd495e6127ea73e8..04f4c0998be98019511c3d5b934f46c246f538e4 100644 --- a/parsers/src/ecaps2.rs +++ b/parsers/src/ecaps2.rs @@ -219,6 +219,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = "K1Njy3HZBThlo4moOD5gBGhn0U0oK7/CbfLlIUDi6o4=+sDTQqBmX6iG/X3zjt06fjZMBBqL/723knFIyRf0sg8=".parse().unwrap(); let error = ECaps2::try_from(elem).unwrap_err(); diff --git a/parsers/src/eme.rs b/parsers/src/eme.rs index fafba2b45ba11834919c7d9d17dc4358fa18bb9e..ca75d0d0c9ff54366e4919de57d89d4c4211ae05 100644 --- a/parsers/src/eme.rs +++ b/parsers/src/eme.rs @@ -72,6 +72,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = "" diff --git a/parsers/src/forwarding.rs b/parsers/src/forwarding.rs index 4963b2a705352405b5094afe7f1d2e00c6e24dfa..2584fc8006233dae93a3da2606f51f7f81b4dd57 100644 --- a/parsers/src/forwarding.rs +++ b/parsers/src/forwarding.rs @@ -56,8 +56,9 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { - let elem: Element = "" + let elem: Element = "" .parse() .unwrap(); let error = Forwarded::try_from(elem).unwrap_err(); diff --git a/parsers/src/hashes.rs b/parsers/src/hashes.rs index 332e69afcf4f7fda44f3432c42d01165fbac3618..0febefc0cb0d58fd409c8d96cc302bfabee49658 100644 --- a/parsers/src/hashes.rs +++ b/parsers/src/hashes.rs @@ -305,6 +305,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = "" .parse() diff --git a/parsers/src/idle.rs b/parsers/src/idle.rs index 08b14ef84995cbcc09ce69586cd0cecd3575e0bd..54cb235dd7a459ea84b253ca6c46fd53f4ba391f 100644 --- a/parsers/src/idle.rs +++ b/parsers/src/idle.rs @@ -42,6 +42,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = "" diff --git a/parsers/src/jingle.rs b/parsers/src/jingle.rs index 9f14da0a2890719c8fe723ec4ab79ce607deeb36..f216d6d23ddf4464bb2eb02deff2b11a9134f2e1 100644 --- a/parsers/src/jingle.rs +++ b/parsers/src/jingle.rs @@ -754,7 +754,7 @@ mod tests { } #[test] - fn test_invalid_reason() { + fn test_missing_reason_text() { let elem: Element = "".parse().unwrap(); let error = Jingle::try_from(elem).unwrap_err(); let message = match error { @@ -765,23 +765,22 @@ mod tests { message, "Missing child field 'reason' in ReasonElement element." ); + } - let elem: Element = "".parse().unwrap(); - let error = Jingle::try_from(elem).unwrap_err(); - let message = match error { - FromElementError::Invalid(Error::Other(string)) => string, - _ => panic!(), - }; - assert_eq!(message, "Unknown child in ReasonElement element."); - - let elem: Element = "".parse().unwrap(); + #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] + fn test_invalid_child_in_reason() { + let elem: Element = "".parse().unwrap(); let error = Jingle::try_from(elem).unwrap_err(); let message = match error { FromElementError::Invalid(Error::Other(string)) => string, _ => panic!(), }; assert_eq!(message, "Unknown child in ReasonElement element."); + } + #[test] + fn test_multiple_reason_children() { let elem: Element = "".parse().unwrap(); let error = Jingle::try_from(elem).unwrap_err(); let message = match error { diff --git a/parsers/src/jingle_ft.rs b/parsers/src/jingle_ft.rs index 746581cca09792671560f0e712d8489e21bea884..23b9d1557f22b0a3072c35e88c861174b0257df3 100644 --- a/parsers/src/jingle_ft.rs +++ b/parsers/src/jingle_ft.rs @@ -304,7 +304,7 @@ mod tests { } #[test] - fn test_received() { + fn test_received_valid() { let elem: Element = "".parse().unwrap(); let received = Received::try_from(elem).unwrap(); assert_eq!(received.name, ContentId(String::from("coucou"))); @@ -313,7 +313,11 @@ mod tests { let received2 = Received::try_from(elem2).unwrap(); assert_eq!(received2.name, ContentId(String::from("coucou"))); assert_eq!(received2.creator, Creator::Initiator); + } + #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] + fn test_received_unknown_child() { let elem: Element = "".parse().unwrap(); let error = Received::try_from(elem).unwrap_err(); let message = match error { @@ -321,7 +325,10 @@ mod tests { _ => panic!(), }; assert_eq!(message, "Unknown child in Received element."); + } + #[test] + fn test_received_missing_name() { let elem: Element = "" .parse() @@ -335,7 +342,10 @@ mod tests { message, "Required attribute field 'name' on Received element missing." ); + } + #[test] + fn test_received_invalid_creator() { let elem: Element = "".parse().unwrap(); let error = Received::try_from(elem).unwrap_err(); let message = match error { @@ -361,7 +371,7 @@ mod tests { } #[test] - fn test_checksum() { + fn test_checksum_valid() { let elem: Element = "w0mcJylzCn+AfvuGdqkty2+KP48=".parse().unwrap(); let hash = vec![ 195, 73, 156, 39, 41, 115, 10, 127, 128, 126, 251, 134, 118, 169, 45, 203, 111, 138, @@ -388,15 +398,22 @@ mod tests { hash: hash.clone() }) ); + } - let elem: Element = "".parse().unwrap(); + #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] + fn test_checksum_unknown_child() { + let elem: Element = "w0mcJylzCn+AfvuGdqkty2+KP48=".parse().unwrap(); let error = Checksum::try_from(elem).unwrap_err(); let message = match error { FromElementError::Invalid(Error::Other(string)) => string, other => panic!("unexpected error: {:?}", other), }; assert_eq!(message, "Unknown child in Checksum element."); + } + #[test] + fn test_checksum_missing_name() { let elem: Element = "w0mcJylzCn+AfvuGdqkty2+KP48=".parse().unwrap(); let error = Checksum::try_from(elem).unwrap_err(); let message = match error { @@ -407,7 +424,10 @@ mod tests { message, "Required attribute field 'name' on Checksum element missing." ); + } + #[test] + fn test_checksum_invalid_creator() { let elem: Element = "w0mcJylzCn+AfvuGdqkty2+KP48=".parse().unwrap(); let error = Checksum::try_from(elem).unwrap_err(); let message = match error { diff --git a/parsers/src/media_element.rs b/parsers/src/media_element.rs index 44c617ec5043553f3025f0e07a46135534ff7715..d17b0da190aa9a7dc0c6c2ec090a6e067f3df71c 100644 --- a/parsers/src/media_element.rs +++ b/parsers/src/media_element.rs @@ -154,6 +154,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_unknown_child() { let elem: Element = "" .parse() diff --git a/parsers/src/message_correct.rs b/parsers/src/message_correct.rs index 9b3c2c94f44f4ac092dbf00c1fcdde3f2e8b22d3..200a00a850c83cafd530f7b15eb056c5a41f916e 100644 --- a/parsers/src/message_correct.rs +++ b/parsers/src/message_correct.rs @@ -62,6 +62,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = "" diff --git a/parsers/src/muc/muc.rs b/parsers/src/muc/muc.rs index c9aa0b08bb0eabadc3f538b61977aafa1d7c4ff0..613980372c5fd7d5afab123b3659fdb081201b58 100644 --- a/parsers/src/muc/muc.rs +++ b/parsers/src/muc/muc.rs @@ -113,6 +113,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_muc_invalid_child() { let elem: Element = "" .parse() diff --git a/parsers/src/muc/user.rs b/parsers/src/muc/user.rs index 50cba62560a145307ce90d4710e1d3f400da979a..9835de8717e20abc2de1dd5acb4adf594a7f4eb1 100644 --- a/parsers/src/muc/user.rs +++ b/parsers/src/muc/user.rs @@ -317,6 +317,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = " @@ -490,6 +491,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_continue_invalid() { let elem: Element = "" diff --git a/parsers/src/occupant_id.rs b/parsers/src/occupant_id.rs index a175f64780cada40bf4afbfcb962fb3790aab364..040fbf96ff7eb06e30754fe91c041854b3309ee7 100644 --- a/parsers/src/occupant_id.rs +++ b/parsers/src/occupant_id.rs @@ -53,6 +53,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = "" diff --git a/parsers/src/roster.rs b/parsers/src/roster.rs index 6722642305ae0ebff68d83564012a6f5a8c636fa..6e044bb8d382fa8d9bf001928bd7ee7ca5c57532 100644 --- a/parsers/src/roster.rs +++ b/parsers/src/roster.rs @@ -289,7 +289,7 @@ mod tests { } #[test] - fn test_invalid_item() { + fn test_item_missing_jid() { let elem: Element = "" .parse() .unwrap(); @@ -302,7 +302,10 @@ mod tests { message, "Required attribute field 'jid' on Item element missing." ); + } + #[test] + fn test_item_invalid_jid() { let elem: Element = "" .parse() .unwrap(); @@ -311,7 +314,11 @@ mod tests { format!("{error}"), "text parse error: no domain found in this JID" ); + } + #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] + fn test_item_unknown_child() { let elem: Element = "" .parse() diff --git a/parsers/src/sasl.rs b/parsers/src/sasl.rs index de4abb1bd0bdeddd7bf14a01c83f5d29f90690f6..b18c83438f0c0f14a359448cc552ea51abe631bf 100644 --- a/parsers/src/sasl.rs +++ b/parsers/src/sasl.rs @@ -157,7 +157,7 @@ pub struct Failure { /// A human-readable explanation for the failure. #[xml(extract(n = .., name = "text", fields( - attribute(type_ = String, name = "xml:lang"), + attribute(type_ = String, name = "xml:lang", default), text(type_ = String), )))] pub texts: BTreeMap, diff --git a/parsers/src/stanza_id.rs b/parsers/src/stanza_id.rs index 04575d3d90c6d17bc20777f446baaa090087cbeb..ae65cc8434b10ad702cde7faa73f325f0ad0a4c2 100644 --- a/parsers/src/stanza_id.rs +++ b/parsers/src/stanza_id.rs @@ -76,6 +76,7 @@ mod tests { } #[test] + #[cfg_attr(feature = "disable-validation", should_panic = "Result::unwrap_err")] fn test_invalid_child() { let elem: Element = "" diff --git a/parsers/src/util/macro_tests.rs b/parsers/src/util/macro_tests.rs index 0da7e6d2a837fa08364dc174d8641a05b4927148..c08c5919141efe48046992b896ca62a133ba1b9d 100644 --- a/parsers/src/util/macro_tests.rs +++ b/parsers/src/util/macro_tests.rs @@ -121,6 +121,10 @@ fn empty_namespace_mismatch() { } #[test] +#[cfg_attr( + feature = "disable-validation", + should_panic = "unexpected result: Ok(" +)] fn empty_unexpected_attribute() { #[allow(unused_imports)] use core::{ @@ -136,6 +140,10 @@ fn empty_unexpected_attribute() { } #[test] +#[cfg_attr( + feature = "disable-validation", + should_panic = "unexpected result: Ok(" +)] fn empty_unexpected_child() { #[allow(unused_imports)] use core::{ @@ -869,6 +877,10 @@ fn text_extract_negative_absent_child() { } #[test] +#[cfg_attr( + feature = "disable-validation", + should_panic = "unexpected result: Ok(" +)] fn text_extract_negative_unexpected_attribute_in_child() { #[allow(unused_imports)] use core::{ @@ -886,6 +898,10 @@ fn text_extract_negative_unexpected_attribute_in_child() { } #[test] +#[cfg_attr( + feature = "disable-validation", + should_panic = "unexpected result: Ok(" +)] fn text_extract_negative_unexpected_child_in_child() { #[allow(unused_imports)] use core::{ @@ -1807,6 +1823,10 @@ fn ignore_unknown_attributes_positive() { } #[test] +#[cfg_attr( + feature = "disable-validation", + should_panic = "unexpected result: Ok(" +)] fn ignore_unknown_attributes_negative_unexpected_child() { #[allow(unused_imports)] use core::{ @@ -1849,6 +1869,10 @@ fn ignore_unknown_children_positive() { } #[test] +#[cfg_attr( + feature = "disable-validation", + should_panic = "unexpected result: Ok(" +)] fn ignore_unknown_children_negative_unexpected_attribute() { #[allow(unused_imports)] use core::{ diff --git a/xso/Cargo.toml b/xso/Cargo.toml index 7096307ea5f837afc7ed190456f06ad347ed9de4..4a0e5f36f7f123fe3bfe4e807b1ab238cf4c4981 100644 --- a/xso/Cargo.toml +++ b/xso/Cargo.toml @@ -29,6 +29,7 @@ default = [ "std" ] macros = [ "dep:xso_proc", "rxml/macros" ] minidom = [ "xso_proc/minidom"] panicking-into-impl = ["xso_proc/panicking-into-impl"] +non-pedantic = [] std = [] [package.metadata.docs.rs] diff --git a/xso/src/lib.rs b/xso/src/lib.rs index 568e1cd1ab3c7a1984ee5068c7f20e644ed5ba7b..3417936ead9c07c6038e2c647f2ced599b60db88 100644 --- a/xso/src/lib.rs +++ b/xso/src/lib.rs @@ -324,13 +324,18 @@ impl AsOptionalXmlText for Option { #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Default)] pub enum UnknownAttributePolicy { /// All unknown attributes are discarded. + /// + /// This is the default policy if the crate is built with the + /// `non-pedantic` feature. + #[cfg_attr(feature = "non-pedantic", default)] Discard, /// The first unknown attribute which is encountered generates a fatal /// parsing error. /// - /// This is the default policy. - #[default] + /// This is the default policy if the crate is built **without** the + /// `non-pedantic` feature. + #[cfg_attr(not(feature = "non-pedantic"), default)] Fail, } @@ -356,13 +361,18 @@ impl UnknownAttributePolicy { #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Default)] pub enum UnknownChildPolicy { /// All unknown children are discarded. + /// + /// This is the default policy if the crate is built with the + /// `non-pedantic` feature. + #[cfg_attr(feature = "non-pedantic", default)] Discard, /// The first unknown child which is encountered generates a fatal /// parsing error. /// - /// This is the default policy. - #[default] + /// This is the default policy if the crate is built **without** the + /// `non-pedantic` feature. + #[cfg_attr(not(feature = "non-pedantic"), default)] Fail, }