From efcfcadf7d6a2819d72a10aceaa3d21fd02ddb77 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 16 Mar 2026 23:43:41 +0200 Subject: [PATCH 01/16] feat(xmldsig): add enveloped signature transform and pipeline executor - Implement Transform enum (Enveloped, C14n) with apply_transform() - Add execute_transforms() sequential pipeline with default C14N fallback - Add parse_transforms() to read XML into Vec - Support InclusiveNamespaces PrefixList parsing for exclusive C14N - Wire NodeSet::exclude_subtree() for enveloped-signature semantics - Add EnvelopedSignatureNotFound error variant to TransformError Closes #13 --- src/xmldsig/mod.rs | 2 + src/xmldsig/transforms.rs | 571 ++++++++++++++++++++++++++++++++++++++ src/xmldsig/types.rs | 4 + 3 files changed, 577 insertions(+) create mode 100644 src/xmldsig/transforms.rs diff --git a/src/xmldsig/mod.rs b/src/xmldsig/mod.rs index c68dd30..36a5fbc 100644 --- a/src/xmldsig/mod.rs +++ b/src/xmldsig/mod.rs @@ -8,7 +8,9 @@ //! - ID attribute resolution with configurable attribute names //! - Node set types for the transform pipeline +pub mod transforms; pub mod types; pub mod uri; +pub use transforms::{execute_transforms, parse_transforms, Transform}; pub use types::{NodeSet, TransformData, TransformError}; diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs new file mode 100644 index 0000000..10282f9 --- /dev/null +++ b/src/xmldsig/transforms.rs @@ -0,0 +1,571 @@ +//! Transform pipeline for XMLDSig `` processing. +//! +//! Implements [XMLDSig §6.6](https://www.w3.org/TR/xmldsig-core1/#sec-Transforms): +//! each `` specifies a chain of transforms applied sequentially to +//! produce bytes for digest computation. +//! +//! The pipeline is a simple `Vec` iterated front-to-back — a dramatic +//! simplification of xmlsec1's bidirectional push/pop doubly-linked list with +//! auto-inserted type adapters. +//! +//! ## Supported transforms +//! +//! | Transform | Direction | Priority | +//! |-----------|-----------|----------| +//! | Enveloped signature | NodeSet → NodeSet | P0 (SAML) | +//! | Inclusive C14N 1.0/1.1 | NodeSet → Binary | P0 | +//! | Exclusive C14N 1.0 | NodeSet → Binary | P0 | +//! | Base64 decode | Binary → Binary | P1 (future) | + +use roxmltree::{Document, Node, NodeId}; + +use super::types::{TransformData, TransformError}; +use crate::c14n::{self, C14nAlgorithm}; + +/// The algorithm URI for the enveloped signature transform. +pub const ENVELOPED_SIGNATURE_URI: &str = "http://www.w3.org/2000/09/xmldsig#enveloped-signature"; + +/// A single transform in the pipeline. +#[derive(Debug, Clone)] +pub enum Transform { + /// Enveloped signature: removes the `` element subtree + /// that contains the `` being processed. + /// + /// Input: `NodeSet` → Output: `NodeSet` + Enveloped, + + /// XML Canonicalization (any supported variant). + /// + /// Input: `NodeSet` → Output: `Binary` + C14n(C14nAlgorithm), +} + +/// Apply a single transform to the pipeline data. +/// +/// `signature_node_id` is the `NodeId` of the `` element that +/// contains the `` being processed. It is used by the enveloped +/// transform to know which signature subtree to exclude. +pub fn apply_transform<'a>( + doc: &'a Document<'a>, + signature_node_id: NodeId, + transform: &Transform, + input: TransformData<'a>, +) -> Result, TransformError> { + match transform { + Transform::Enveloped => { + let mut nodes = input.into_node_set()?; + // Exclude the Signature element and all its descendants from + // the node set. This is the core mechanism of the enveloped + // signature transform: the digest is computed as if the + // were not present in the document. + // + // xmlsec1 equivalent: + // xmlSecNodeSetGetChildren(doc, signatureNode, 1, 1) // inverted tree + // xmlSecNodeSetAdd(inNodes, children, Intersection) // intersect = subtract + let signature_node = doc + .get_node(signature_node_id) + .ok_or(TransformError::EnvelopedSignatureNotFound)?; + nodes.exclude_subtree(signature_node); + Ok(TransformData::NodeSet(nodes)) + } + Transform::C14n(algo) => { + let nodes = input.into_node_set()?; + let mut output = Vec::new(); + // Build a predicate closure that checks node membership in + // the NodeSet. The C14N serializer calls this for each node + // during document-order traversal. + let predicate = |node: Node| nodes.contains(node); + c14n::canonicalize(nodes.document(), Some(&predicate), algo, &mut output) + .map_err(|e| TransformError::C14n(e.to_string()))?; + Ok(TransformData::Binary(output)) + } + } +} + +/// Execute a chain of transforms for a single ``. +/// +/// 1. Start with `initial_data` (from URI dereference). +/// 2. Apply each transform sequentially. +/// 3. If the result is still a `NodeSet`, apply default inclusive C14N 1.0 +/// to produce bytes (per [XMLDSig §4.3.3.2]). +/// +/// Returns the final byte sequence ready for digest computation. +pub fn execute_transforms<'a>( + doc: &'a Document<'a>, + signature_node_id: NodeId, + initial_data: TransformData<'a>, + transforms: &[Transform], +) -> Result, TransformError> { + let mut data = initial_data; + + for transform in transforms { + data = apply_transform(doc, signature_node_id, transform, data)?; + } + + // Final coercion: if the result is still a NodeSet, canonicalize with + // default inclusive C14N 1.0 per XMLDSig spec §4.3.3.2. + match data { + TransformData::Binary(bytes) => Ok(bytes), + TransformData::NodeSet(nodes) => { + let algo = C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315") + .expect("known URI"); + let mut output = Vec::new(); + let predicate = |node: Node| nodes.contains(node); + c14n::canonicalize(nodes.document(), Some(&predicate), &algo, &mut output) + .map_err(|e| TransformError::C14n(e.to_string()))?; + Ok(output) + } + } +} + +/// Parse a `` element into a `Vec`. +/// +/// Reads each `` child element and constructs +/// the corresponding [`Transform`] variant. Unrecognized algorithm URIs +/// produce an error. +/// +/// For Exclusive C14N, also parses the optional `` child element. +pub fn parse_transforms(transforms_node: Node) -> Result, TransformError> { + let mut chain = Vec::new(); + + for child in transforms_node.children() { + if !child.is_element() { + continue; + } + // Match on local name, ignoring namespace prefix (could be ds:Transform or Transform) + if child.tag_name().name() != "Transform" { + continue; + } + let uri = child.attribute("Algorithm").ok_or_else(|| { + TransformError::UnsupportedTransform( + "missing Algorithm attribute on ".into(), + ) + })?; + + let transform = if uri == ENVELOPED_SIGNATURE_URI { + Transform::Enveloped + } else if let Some(mut algo) = C14nAlgorithm::from_uri(uri) { + // For exclusive C14N, check for InclusiveNamespaces child + if algo.mode() == c14n::C14nMode::Exclusive1_0 { + if let Some(prefix_list) = parse_inclusive_prefixes(child) { + algo = algo.with_prefix_list(&prefix_list); + } + } + Transform::C14n(algo) + } else { + return Err(TransformError::UnsupportedTransform(uri.to_string())); + }; + chain.push(transform); + } + + Ok(chain) +} + +/// Parse the `PrefixList` attribute from an `` child +/// element, if present. +/// +/// The element is typically: +/// ```xml +/// +/// ``` +fn parse_inclusive_prefixes(transform_node: Node) -> Option { + for child in transform_node.children() { + if child.is_element() && child.tag_name().name() == "InclusiveNamespaces" { + return child.attribute("PrefixList").map(String::from); + } + } + None +} + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + use super::*; + use crate::xmldsig::NodeSet; + + // ── Enveloped transform ────────────────────────────────────────── + + #[test] + fn enveloped_excludes_signature_subtree() { + // Simulates a SAML-like document with an enveloped signature + let xml = r#" + hello + + + abc + + "#; + let doc = Document::parse(xml).unwrap(); + + // Find the Signature element + let sig_node = doc + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "Signature") + .unwrap(); + + // Start with entire document without comments (empty URI) + let node_set = NodeSet::entire_document_without_comments(&doc); + let data = TransformData::NodeSet(node_set); + + // Apply enveloped transform + let result = apply_transform(&doc, sig_node.id(), &Transform::Enveloped, data).unwrap(); + let node_set = result.into_node_set().unwrap(); + + // Root and data should be in the set + assert!(node_set.contains(doc.root_element())); + let data_elem = doc + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "data") + .unwrap(); + assert!(node_set.contains(data_elem)); + + // Signature and its children should be excluded + assert!( + !node_set.contains(sig_node), + "Signature element should be excluded" + ); + let signed_info = doc + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "SignedInfo") + .unwrap(); + assert!( + !node_set.contains(signed_info), + "SignedInfo (child of Signature) should be excluded" + ); + } + + #[test] + fn enveloped_requires_node_set_input() { + let xml = ""; + let doc = Document::parse(xml).unwrap(); + let sig_id = doc.root_element().id(); + + // Binary input should fail with TypeMismatch + let data = TransformData::Binary(vec![1, 2, 3]); + let result = apply_transform(&doc, sig_id, &Transform::Enveloped, data); + assert!(result.is_err()); + match result.unwrap_err() { + TransformError::TypeMismatch { expected, got } => { + assert_eq!(expected, "NodeSet"); + assert_eq!(got, "Binary"); + } + other => panic!("expected TypeMismatch, got: {other:?}"), + } + } + + // ── C14N transform ─────────────────────────────────────────────── + + #[test] + fn c14n_transform_produces_bytes() { + let xml = r#""#; + let doc = Document::parse(xml).unwrap(); + + let node_set = NodeSet::entire_document_without_comments(&doc); + let data = TransformData::NodeSet(node_set); + + let algo = + C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315").unwrap(); + let result = + apply_transform(&doc, doc.root_element().id(), &Transform::C14n(algo), data).unwrap(); + + let bytes = result.into_binary().unwrap(); + let output = String::from_utf8(bytes).unwrap(); + // Attributes sorted, empty element expanded + assert_eq!(output, r#""#); + } + + #[test] + fn c14n_transform_requires_node_set() { + let xml = ""; + let doc = Document::parse(xml).unwrap(); + + let algo = + C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315").unwrap(); + let data = TransformData::Binary(vec![1, 2, 3]); + let result = apply_transform(&doc, doc.root_element().id(), &Transform::C14n(algo), data); + + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + TransformError::TypeMismatch { .. } + )); + } + + // ── Pipeline execution ─────────────────────────────────────────── + + #[test] + fn pipeline_enveloped_then_c14n() { + // Standard SAML transform chain: enveloped-signature → exc-c14n + let xml = r#" + hello + + + abc + + "#; + let doc = Document::parse(xml).unwrap(); + + let sig_node = doc + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "Signature") + .unwrap(); + + let initial = TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); + let transforms = vec![ + Transform::Enveloped, + Transform::C14n( + C14nAlgorithm::from_uri("http://www.w3.org/2001/10/xml-exc-c14n#").unwrap(), + ), + ]; + + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + + let output = String::from_utf8(result).unwrap(); + // Signature subtree should be gone; attributes sorted + assert!(!output.contains("Signature")); + assert!(!output.contains("SignedInfo")); + assert!(!output.contains("SignatureValue")); + assert!(output.contains("hello")); + } + + #[test] + fn pipeline_no_transforms_applies_default_c14n() { + // No explicit transforms → pipeline falls back to inclusive C14N 1.0 + let xml = r#""#; + let doc = Document::parse(xml).unwrap(); + + let initial = TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); + let result = execute_transforms(&doc, doc.root_element().id(), initial, &[]).unwrap(); + + let output = String::from_utf8(result).unwrap(); + assert_eq!(output, r#""#); + } + + #[test] + fn pipeline_binary_passthrough() { + // If initial data is already binary (unusual, but spec-compliant) + // and no transforms, returns bytes directly + let xml = ""; + let doc = Document::parse(xml).unwrap(); + + let initial = TransformData::Binary(b"raw bytes".to_vec()); + let result = execute_transforms(&doc, doc.root_element().id(), initial, &[]).unwrap(); + + assert_eq!(result, b"raw bytes"); + } + + // ── Nested signatures ──────────────────────────────────────────── + + #[test] + fn enveloped_only_excludes_own_signature() { + // Nested signatures: enveloped transform should only exclude + // the specific being verified, not all signatures + let xml = r#" + hello + + + + + + + "#; + let doc = Document::parse(xml).unwrap(); + + // We are verifying the element, not + let sig_node = doc + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "Signature") + .unwrap(); + + let node_set = NodeSet::entire_document_without_comments(&doc); + let data = TransformData::NodeSet(node_set); + + let result = apply_transform(&doc, sig_node.id(), &Transform::Enveloped, data).unwrap(); + let node_set = result.into_node_set().unwrap(); + + // Sig1 should still be in the set + let sig1 = doc + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "Sig1") + .unwrap(); + assert!( + node_set.contains(sig1), + "other signature elements should NOT be excluded" + ); + + // Signature should be excluded + assert!( + !node_set.contains(sig_node), + "the specific Signature being verified should be excluded" + ); + } + + // ── parse_transforms ───────────────────────────────────────────── + + #[test] + fn parse_transforms_enveloped_and_exc_c14n() { + let xml = r#" + + + "#; + let doc = Document::parse(xml).unwrap(); + let transforms_node = doc.root_element(); + + let chain = parse_transforms(transforms_node).unwrap(); + assert_eq!(chain.len(), 2); + assert!(matches!(chain[0], Transform::Enveloped)); + assert!(matches!(chain[1], Transform::C14n(_))); + } + + #[test] + fn parse_transforms_with_inclusive_prefixes() { + let xml = r#" + + + + "#; + let doc = Document::parse(xml).unwrap(); + let transforms_node = doc.root_element(); + + let chain = parse_transforms(transforms_node).unwrap(); + assert_eq!(chain.len(), 1); + match &chain[0] { + Transform::C14n(algo) => { + assert!(algo.inclusive_prefixes().contains("ds")); + assert!(algo.inclusive_prefixes().contains("saml")); + assert!(algo.inclusive_prefixes().contains("")); // #default + } + other => panic!("expected C14n, got: {other:?}"), + } + } + + #[test] + fn parse_transforms_unsupported_algorithm() { + let xml = r#" + + "#; + let doc = Document::parse(xml).unwrap(); + + let result = parse_transforms(doc.root_element()); + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + TransformError::UnsupportedTransform(_) + )); + } + + #[test] + fn parse_transforms_missing_algorithm() { + let xml = r#" + + "#; + let doc = Document::parse(xml).unwrap(); + + let result = parse_transforms(doc.root_element()); + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + TransformError::UnsupportedTransform(_) + )); + } + + #[test] + fn parse_transforms_empty() { + let xml = r#""#; + let doc = Document::parse(xml).unwrap(); + + let chain = parse_transforms(doc.root_element()).unwrap(); + assert!(chain.is_empty()); + } + + #[test] + fn parse_transforms_inclusive_c14n_variants() { + let xml = r#" + + + + "#; + let doc = Document::parse(xml).unwrap(); + + let chain = parse_transforms(doc.root_element()).unwrap(); + assert_eq!(chain.len(), 3); + // All should be C14n variants + for t in &chain { + assert!(matches!(t, Transform::C14n(_))); + } + } + + // ── Integration: SAML-like full pipeline ───────────────────────── + + #[test] + fn saml_enveloped_signature_full_pipeline() { + // Realistic SAML Response with enveloped signature + let xml = r#" + + user@example.com + + + + + + + + + + + fakesig== + + "#; + let doc = Document::parse(xml).unwrap(); + + // Find the Signature element + let sig_node = doc + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "Signature") + .unwrap(); + + // Parse the transforms from the XML + let reference = doc + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "Reference") + .unwrap(); + let transforms_elem = reference + .children() + .find(|n| n.is_element() && n.tag_name().name() == "Transforms") + .unwrap(); + let transforms = parse_transforms(transforms_elem).unwrap(); + assert_eq!(transforms.len(), 2); + + // Execute the pipeline with empty URI (entire document) + let initial = TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + + let output = String::from_utf8(result).unwrap(); + + // Signature subtree must be completely absent + assert!(!output.contains("Signature"), "Signature should be removed"); + assert!( + !output.contains("SignedInfo"), + "SignedInfo should be removed" + ); + assert!( + !output.contains("SignatureValue"), + "SignatureValue should be removed" + ); + assert!( + !output.contains("fakesig"), + "signature value should be removed" + ); + + // Document content should be present and canonicalized + assert!(output.contains("samlp:Response")); + assert!(output.contains("saml:Assertion")); + assert!(output.contains("user@example.com")); + } +} diff --git a/src/xmldsig/types.rs b/src/xmldsig/types.rs index 8513bd6..3abfa11 100644 --- a/src/xmldsig/types.rs +++ b/src/xmldsig/types.rs @@ -224,4 +224,8 @@ pub enum TransformError { /// Canonicalization error during transform. #[error("C14N error: {0}")] C14n(String), + + /// The Signature node referenced by the enveloped transform was not found. + #[error("enveloped-signature transform: Signature node not found in document")] + EnvelopedSignatureNotFound, } From d7865b07139fd35bd66638de5fdce9dffa0e5b56 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Mon, 16 Mar 2026 23:47:11 +0200 Subject: [PATCH 02/16] test(xmldsig): add integration tests for transform pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 10 integration tests in tests/transforms_integration.rs covering: - Enveloped + exclusive C14N (standard SAML pattern) - Enveloped + inclusive C14N 1.0 and 1.1 - #id URI + enveloped (assertion-level signature) - Nested signatures (only own Signature excluded) - Default C14N fallback when no explicit C14N in chain - Exclusive C14N with InclusiveNamespaces PrefixList from XML - parse_transforms → execute_transforms roundtrip - Edge case: Signature as only child - Whitespace preservation around excluded Signature Closes #13 --- tests/transforms_integration.rs | 479 ++++++++++++++++++++++++++++++++ 1 file changed, 479 insertions(+) create mode 100644 tests/transforms_integration.rs diff --git a/tests/transforms_integration.rs b/tests/transforms_integration.rs new file mode 100644 index 0000000..02bcd95 --- /dev/null +++ b/tests/transforms_integration.rs @@ -0,0 +1,479 @@ +//! Integration tests: transform pipeline (enveloped signature + C14N). +//! +//! Tests the full chain: URI dereference → parse_transforms → execute_transforms +//! → verify canonical output matches expected bytes. +//! +//! These tests exercise multi-component interaction: +//! - `UriReferenceResolver` (URI dereference) +//! - `parse_transforms` (XML → Vec) +//! - `execute_transforms` (sequential pipeline) +//! - `NodeSet::exclude_subtree` (enveloped-signature) +//! - `c14n::canonicalize` (C14N serializer) + +use xml_sec::c14n::{C14nAlgorithm, C14nMode}; +use xml_sec::xmldsig::transforms::{execute_transforms, parse_transforms, Transform}; +use xml_sec::xmldsig::uri::UriReferenceResolver; +use xml_sec::xmldsig::NodeSet; + +// ── Helper ─────────────────────────────────────────────────────────────────── + +/// Find a descendant element by local name. +fn find_element<'a>(doc: &'a roxmltree::Document<'a>, name: &str) -> roxmltree::Node<'a, 'a> { + doc.descendants() + .find(|n| n.is_element() && n.tag_name().name() == name) + .unwrap_or_else(|| panic!("element <{name}> not found")) +} + +// ── Enveloped + Exclusive C14N (most common SAML pattern) ──────────────────── + +#[test] +fn enveloped_exc_c14n_saml_response() { + // Standard SAML Response with enveloped signature over entire document. + // Transform chain: enveloped-signature → exclusive C14N. + let xml = r#" + + user@example.com + + + + + + + + + + + + + placeholder== + + + fakesig== + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let resolver = UriReferenceResolver::new(&doc); + let sig_node = find_element(&doc, "Signature"); + + // Parse transforms from the XML + let reference = find_element(&doc, "Reference"); + let transforms_elem = reference + .children() + .find(|n| n.is_element() && n.tag_name().name() == "Transforms") + .unwrap(); + let transforms = parse_transforms(transforms_elem).unwrap(); + assert_eq!(transforms.len(), 2); + + // Dereference empty URI → entire document without comments + let initial = resolver.dereference("").unwrap(); + + // Execute pipeline + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // Signature must be completely absent + assert!(!output.contains("Signature"), "output: {output}"); + assert!(!output.contains("SignedInfo"), "output: {output}"); + assert!(!output.contains("SignatureValue"), "output: {output}"); + assert!(!output.contains("fakesig"), "output: {output}"); + assert!(!output.contains("DigestValue"), "output: {output}"); + + // Document content must be present + assert!(output.contains("samlp:Response"), "output: {output}"); + assert!(output.contains("saml:Assertion"), "output: {output}"); + assert!(output.contains("user@example.com"), "output: {output}"); + assert!(output.contains("_assert1"), "output: {output}"); + assert!(output.contains("_resp1"), "output: {output}"); + + // Exclusive C14N: only visibly-utilized namespaces. + // samlp: is used on Response, saml: is used on Assertion/Subject/Conditions + assert!( + output.contains("xmlns:samlp="), + "samlp ns should be present: {output}" + ); + assert!( + output.contains("xmlns:saml="), + "saml ns should be present: {output}" + ); +} + +// ── Enveloped + Inclusive C14N ──────────────────────────────────────────────── + +#[test] +fn enveloped_inclusive_c14n() { + // Less common but valid: enveloped + inclusive C14N 1.0 + let xml = r#" + content + + + sig + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let resolver = UriReferenceResolver::new(&doc); + let sig_node = find_element(&doc, "Signature"); + + let initial = resolver.dereference("").unwrap(); + let transforms = vec![ + Transform::Enveloped, + Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), + ]; + + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // Signature gone + assert!(!output.contains("Signature"), "output: {output}"); + assert!(!output.contains("SignedInfo"), "output: {output}"); + + // Content present + assert!(output.contains(" + content + + + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let sig_node = find_element(&doc, "Signature"); + + let initial = + xml_sec::xmldsig::TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); + let transforms = vec![ + Transform::Enveloped, + Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_1, false)), + ]; + + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + assert!(!output.contains("Signature"), "output: {output}"); + assert!(output.contains("content"), "output: {output}"); +} + +// ── #id URI + enveloped (signature inside assertion) ───────────────────────── + +#[test] +fn id_uri_enveloped_assertion_level_signature() { + // Signature inside the Assertion, signing only the Assertion (not the whole Response). + // URI="#_assert1" → subtree of Assertion, then enveloped removes Signature within it. + let xml = r#" + + user@example.com + + + sig + + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let resolver = UriReferenceResolver::new(&doc); + let sig_node = find_element(&doc, "Signature"); + + // Dereference #_assert1 → subtree of Assertion element + let initial = resolver.dereference("#_assert1").unwrap(); + + let transforms = vec![ + Transform::Enveloped, + Transform::C14n( + C14nAlgorithm::from_uri("http://www.w3.org/2001/10/xml-exc-c14n#").unwrap(), + ), + ]; + + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // Should contain the Assertion element and Subject + assert!(output.contains("saml:Assertion"), "output: {output}"); + assert!(output.contains("user@example.com"), "output: {output}"); + + // Signature subtree removed + assert!(!output.contains("Signature"), "output: {output}"); + assert!(!output.contains("SignedInfo"), "output: {output}"); + + // Response should NOT appear (we dereferenced only the assertion subtree) + assert!(!output.contains("samlp:Response"), "output: {output}"); +} + +// ── Nested signatures ──────────────────────────────────────────────────────── + +#[test] +fn nested_signatures_only_own_excluded() { + // Two signatures: one at Response level, one inside Assertion. + // When verifying the Response-level signature, only IT should be excluded. + let xml = r#" + + user + + + + + + + outer + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let resolver = UriReferenceResolver::new(&doc); + + // The outer signature (Id="outer-sig") is being verified + let outer_sig = doc + .descendants() + .find(|n| n.is_element() && n.attribute("Id") == Some("outer-sig")) + .unwrap(); + + let initial = resolver.dereference("").unwrap(); + let transforms = vec![ + Transform::Enveloped, + Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), + ]; + + let result = execute_transforms(&doc, outer_sig.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // Outer signature should be excluded + assert!(!output.contains("outer-sig"), "output: {output}"); + assert!(!output.contains("outer"), "output: {output}"); + + // Inner signature should REMAIN (it's not the one being verified) + assert!( + output.contains("inner-sig"), + "inner signature should remain: {output}" + ); + + // Document content should be present + assert!( + output.contains("user"), + "output: {output}" + ); + assert!(output.contains("Assertion"), "output: {output}"); +} + +// ── No explicit C14N → default inclusive C14N 1.0 ──────────────────────────── + +#[test] +fn enveloped_only_falls_back_to_default_c14n() { + // If transform chain has only enveloped (no explicit C14N), + // the pipeline applies default inclusive C14N 1.0 per XMLDSig spec. + let xml = r#" + content + + + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let sig_node = find_element(&doc, "Signature"); + + let initial = + xml_sec::xmldsig::TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); + let transforms = vec![Transform::Enveloped]; + + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // Signature gone + assert!(!output.contains("Signature"), "output: {output}"); + + // Attributes sorted (inclusive C14N) + assert!( + output.contains(r#"a="1" b="2""#), + "attributes should be sorted: {output}" + ); +} + +// ── Exclusive C14N with InclusiveNamespaces PrefixList ─────────────────────── + +#[test] +fn exc_c14n_with_prefix_list_from_xml() { + // Parse a transform chain that includes InclusiveNamespaces PrefixList, + // then execute the pipeline. + let xml = r#" + text + + + + + + + + + + + + sig + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let resolver = UriReferenceResolver::new(&doc); + let sig_node = find_element(&doc, "Signature"); + + // Parse transforms from the XML + let reference = find_element(&doc, "Reference"); + let transforms_elem = reference + .children() + .find(|n| n.is_element() && n.tag_name().name() == "Transforms") + .unwrap(); + let transforms = parse_transforms(transforms_elem).unwrap(); + assert_eq!(transforms.len(), 2); + + // Verify PrefixList was parsed + match &transforms[1] { + Transform::C14n(algo) => { + assert!( + algo.inclusive_prefixes().contains("ns2"), + "PrefixList should include ns2" + ); + } + other => panic!("expected C14n transform, got: {other:?}"), + } + + // Execute pipeline + let initial = resolver.dereference("").unwrap(); + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // Signature removed + assert!(!output.contains("Signature"), "output: {output}"); + + // ns1: is visibly utilized (attr on child) → should appear + assert!( + output.contains("xmlns:ns1="), + "ns1 should be present (visibly utilized): {output}" + ); + + // ns2: is forced via PrefixList → should appear even though not visibly utilized on child + // (It IS on root via inclusive ns rendering forced by PrefixList) + assert!( + output.contains("xmlns:ns2="), + "ns2 should be present (forced by PrefixList): {output}" + ); +} + +// ── parse_transforms end-to-end with realistic SAML ────────────────────────── + +#[test] +fn parse_and_execute_transforms_roundtrip() { + // Parse transforms from XML, execute them, verify output. + // This tests the full integration path that verification will use. + let xml = r#" + important data + + + + + + + + + + base64sig== + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let resolver = UriReferenceResolver::new(&doc); + let sig_node = find_element(&doc, "Signature"); + + // Step 1: Parse transforms from XML + let reference = find_element(&doc, "Reference"); + let transforms_elem = reference + .children() + .find(|n| n.is_element() && n.tag_name().name() == "Transforms") + .unwrap(); + let transforms = parse_transforms(transforms_elem).unwrap(); + + // Step 2: Dereference URI + let uri = reference.attribute("URI").unwrap_or(""); + let initial = resolver.dereference(uri).unwrap(); + + // Step 3: Execute transforms + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // Step 4: Verify + assert!(!output.contains("Signature")); + assert!(output.contains(r#"important data"#)); +} + +// ── Edge case: signature is the only child ─────────────────────────────────── + +#[test] +fn enveloped_signature_only_child() { + // Edge case: the Signature is the only child of root. + // After enveloped transform, only the root element (empty) remains. + let xml = r#" + + + sig + +"#; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let sig_node = find_element(&doc, "Signature"); + + let initial = + xml_sec::xmldsig::TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); + let transforms = vec![ + Transform::Enveloped, + Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), + ]; + + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // Only whitespace and root element should remain + assert!(!output.contains("Signature"), "output: {output}"); + assert!(output.contains(""), "output: {output}"); +} + +// ── Whitespace handling ────────────────────────────────────────────────────── + +#[test] +fn enveloped_preserves_surrounding_whitespace() { + // Whitespace text nodes between elements should be preserved + // (they are part of the document, not part of the Signature subtree) + let xml = "\n text\n \n \n \n"; + + let doc = roxmltree::Document::parse(xml).unwrap(); + let sig_node = find_element(&doc, "Signature"); + + let initial = + xml_sec::xmldsig::TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); + let transforms = vec![ + Transform::Enveloped, + Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), + ]; + + let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let output = String::from_utf8(result).unwrap(); + + // The text nodes ("\n ") around Signature are preserved — they're not + // part of the Signature subtree (they're siblings). C14N will emit them. + assert!(output.contains("text"), "output: {output}"); + assert!(!output.contains("SignedInfo"), "output: {output}"); +} From 50ccb154b307df6458e028e974c55872aeefde27 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 00:07:02 +0200 Subject: [PATCH 03/16] fix(xmldsig): replace expect() with error propagation, validate InclusiveNamespaces namespace - Replace .expect("known URI") with .ok_or_else() returning TransformError::C14n - Validate InclusiveNamespaces element namespace matches exc-c14n spec URI - Add EXCLUSIVE_C14N_NS_URI constant for namespace validation - Add test verifying InclusiveNamespaces in foreign namespace is ignored --- src/xmldsig/transforms.rs | 45 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 3 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 10282f9..c1bf7b6 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -25,6 +25,9 @@ use crate::c14n::{self, C14nAlgorithm}; /// The algorithm URI for the enveloped signature transform. pub const ENVELOPED_SIGNATURE_URI: &str = "http://www.w3.org/2000/09/xmldsig#enveloped-signature"; +/// Namespace URI for Exclusive C14N `` elements. +const EXCLUSIVE_C14N_NS_URI: &str = "http://www.w3.org/2001/10/xml-exc-c14n#"; + /// A single transform in the pipeline. #[derive(Debug, Clone)] pub enum Transform { @@ -108,7 +111,9 @@ pub fn execute_transforms<'a>( TransformData::Binary(bytes) => Ok(bytes), TransformData::NodeSet(nodes) => { let algo = C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315") - .expect("known URI"); + .ok_or_else(|| { + TransformError::C14n("unsupported default C14N algorithm URI".to_string()) + })?; let mut output = Vec::new(); let predicate = |node: Node| nodes.contains(node); c14n::canonicalize(nodes.document(), Some(&predicate), &algo, &mut output) @@ -165,6 +170,10 @@ pub fn parse_transforms(transforms_node: Node) -> Result, Transfo /// Parse the `PrefixList` attribute from an `` child /// element, if present. /// +/// Per the [Exclusive C14N spec](https://www.w3.org/TR/xml-exc-c14n/#def-InclusiveNamespaces-PrefixList), +/// the element MUST be in the `http://www.w3.org/2001/10/xml-exc-c14n#` namespace. +/// Elements with the same local name but a different namespace are ignored. +/// /// The element is typically: /// ```xml /// Result, Transfo /// ``` fn parse_inclusive_prefixes(transform_node: Node) -> Option { for child in transform_node.children() { - if child.is_element() && child.tag_name().name() == "InclusiveNamespaces" { - return child.attribute("PrefixList").map(String::from); + if child.is_element() { + let tag = child.tag_name(); + if tag.name() == "InclusiveNamespaces" && tag.namespace() == Some(EXCLUSIVE_C14N_NS_URI) + { + return child.attribute("PrefixList").map(String::from); + } } } None @@ -443,6 +456,32 @@ mod tests { } } + #[test] + fn parse_transforms_ignores_wrong_ns_inclusive_namespaces() { + // InclusiveNamespaces in a foreign namespace should be ignored — + // only elements in http://www.w3.org/2001/10/xml-exc-c14n# are valid. + let xml = r#" + + + + "#; + let doc = Document::parse(xml).unwrap(); + + let chain = parse_transforms(doc.root_element()).unwrap(); + assert_eq!(chain.len(), 1); + match &chain[0] { + Transform::C14n(algo) => { + // PrefixList from wrong namespace should NOT be honoured + assert!( + algo.inclusive_prefixes().is_empty(), + "should ignore InclusiveNamespaces in wrong namespace" + ); + } + other => panic!("expected C14n, got: {other:?}"), + } + } + #[test] fn parse_transforms_unsupported_algorithm() { let xml = r#" From d7f3edfcb6bcd4470ff40a36020c5a09706a7028 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 00:17:47 +0200 Subject: [PATCH 04/16] refactor(xmldsig): replace NodeId with Node in transform API for cross-document safety - Change apply_transform() and execute_transforms() to accept Node<'a, 'a> instead of bare NodeId (prevents cross-document index misuse) - Add ptr::eq guard in enveloped transform for defense-in-depth - Update all call sites in unit and integration tests --- src/xmldsig/transforms.rs | 45 ++++++++++++++++++--------------- tests/transforms_integration.rs | 20 +++++++-------- 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index c1bf7b6..a172c44 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -17,7 +17,7 @@ //! | Exclusive C14N 1.0 | NodeSet → Binary | P0 | //! | Base64 decode | Binary → Binary | P1 (future) | -use roxmltree::{Document, Node, NodeId}; +use roxmltree::{Document, Node}; use super::types::{TransformData, TransformError}; use crate::c14n::{self, C14nAlgorithm}; @@ -45,12 +45,17 @@ pub enum Transform { /// Apply a single transform to the pipeline data. /// -/// `signature_node_id` is the `NodeId` of the `` element that -/// contains the `` being processed. It is used by the enveloped -/// transform to know which signature subtree to exclude. +/// `signature_node` is the `` element that contains the +/// `` being processed. It is used by the enveloped transform +/// to know which signature subtree to exclude. +/// +/// Accepting `Node` instead of `NodeId` binds the signature target to a +/// specific document at the type level, preventing cross-document misuse +/// (a bare `NodeId` is an untyped `u32` index that could silently resolve +/// to the wrong node if passed with a different document). pub fn apply_transform<'a>( doc: &'a Document<'a>, - signature_node_id: NodeId, + signature_node: Node<'a, 'a>, transform: &Transform, input: TransformData<'a>, ) -> Result, TransformError> { @@ -65,9 +70,9 @@ pub fn apply_transform<'a>( // xmlsec1 equivalent: // xmlSecNodeSetGetChildren(doc, signatureNode, 1, 1) // inverted tree // xmlSecNodeSetAdd(inNodes, children, Intersection) // intersect = subtract - let signature_node = doc - .get_node(signature_node_id) - .ok_or(TransformError::EnvelopedSignatureNotFound)?; + if !std::ptr::eq(signature_node.document() as *const _, doc as *const _) { + return Err(TransformError::EnvelopedSignatureNotFound); + } nodes.exclude_subtree(signature_node); Ok(TransformData::NodeSet(nodes)) } @@ -95,14 +100,14 @@ pub fn apply_transform<'a>( /// Returns the final byte sequence ready for digest computation. pub fn execute_transforms<'a>( doc: &'a Document<'a>, - signature_node_id: NodeId, + signature_node: Node<'a, 'a>, initial_data: TransformData<'a>, transforms: &[Transform], ) -> Result, TransformError> { let mut data = initial_data; for transform in transforms { - data = apply_transform(doc, signature_node_id, transform, data)?; + data = apply_transform(doc, signature_node, transform, data)?; } // Final coercion: if the result is still a NodeSet, canonicalize with @@ -224,7 +229,7 @@ mod tests { let data = TransformData::NodeSet(node_set); // Apply enveloped transform - let result = apply_transform(&doc, sig_node.id(), &Transform::Enveloped, data).unwrap(); + let result = apply_transform(&doc, sig_node, &Transform::Enveloped, data).unwrap(); let node_set = result.into_node_set().unwrap(); // Root and data should be in the set @@ -254,11 +259,9 @@ mod tests { fn enveloped_requires_node_set_input() { let xml = ""; let doc = Document::parse(xml).unwrap(); - let sig_id = doc.root_element().id(); - // Binary input should fail with TypeMismatch let data = TransformData::Binary(vec![1, 2, 3]); - let result = apply_transform(&doc, sig_id, &Transform::Enveloped, data); + let result = apply_transform(&doc, doc.root_element(), &Transform::Enveloped, data); assert!(result.is_err()); match result.unwrap_err() { TransformError::TypeMismatch { expected, got } => { @@ -282,7 +285,7 @@ mod tests { let algo = C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315").unwrap(); let result = - apply_transform(&doc, doc.root_element().id(), &Transform::C14n(algo), data).unwrap(); + apply_transform(&doc, doc.root_element(), &Transform::C14n(algo), data).unwrap(); let bytes = result.into_binary().unwrap(); let output = String::from_utf8(bytes).unwrap(); @@ -298,7 +301,7 @@ mod tests { let algo = C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315").unwrap(); let data = TransformData::Binary(vec![1, 2, 3]); - let result = apply_transform(&doc, doc.root_element().id(), &Transform::C14n(algo), data); + let result = apply_transform(&doc, doc.root_element(), &Transform::C14n(algo), data); assert!(result.is_err()); assert!(matches!( @@ -334,7 +337,7 @@ mod tests { ), ]; - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature subtree should be gone; attributes sorted @@ -351,7 +354,7 @@ mod tests { let doc = Document::parse(xml).unwrap(); let initial = TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); - let result = execute_transforms(&doc, doc.root_element().id(), initial, &[]).unwrap(); + let result = execute_transforms(&doc, doc.root_element(), initial, &[]).unwrap(); let output = String::from_utf8(result).unwrap(); assert_eq!(output, r#""#); @@ -365,7 +368,7 @@ mod tests { let doc = Document::parse(xml).unwrap(); let initial = TransformData::Binary(b"raw bytes".to_vec()); - let result = execute_transforms(&doc, doc.root_element().id(), initial, &[]).unwrap(); + let result = execute_transforms(&doc, doc.root_element(), initial, &[]).unwrap(); assert_eq!(result, b"raw bytes"); } @@ -396,7 +399,7 @@ mod tests { let node_set = NodeSet::entire_document_without_comments(&doc); let data = TransformData::NodeSet(node_set); - let result = apply_transform(&doc, sig_node.id(), &Transform::Enveloped, data).unwrap(); + let result = apply_transform(&doc, sig_node, &Transform::Enveloped, data).unwrap(); let node_set = result.into_node_set().unwrap(); // Sig1 should still be in the set @@ -583,7 +586,7 @@ mod tests { // Execute the pipeline with empty URI (entire document) let initial = TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); diff --git a/tests/transforms_integration.rs b/tests/transforms_integration.rs index 02bcd95..11820d7 100644 --- a/tests/transforms_integration.rs +++ b/tests/transforms_integration.rs @@ -71,7 +71,7 @@ fn enveloped_exc_c14n_saml_response() { let initial = resolver.dereference("").unwrap(); // Execute pipeline - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature must be completely absent @@ -123,7 +123,7 @@ fn enveloped_inclusive_c14n() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), ]; - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature gone @@ -162,7 +162,7 @@ fn enveloped_inclusive_c14n_1_1() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_1, false)), ]; - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); assert!(!output.contains("Signature"), "output: {output}"); @@ -201,7 +201,7 @@ fn id_uri_enveloped_assertion_level_signature() { ), ]; - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Should contain the Assertion element and Subject @@ -250,7 +250,7 @@ fn nested_signatures_only_own_excluded() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), ]; - let result = execute_transforms(&doc, outer_sig.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, outer_sig, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Outer signature should be excluded @@ -291,7 +291,7 @@ fn enveloped_only_falls_back_to_default_c14n() { xml_sec::xmldsig::TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); let transforms = vec![Transform::Enveloped]; - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature gone @@ -354,7 +354,7 @@ fn exc_c14n_with_prefix_list_from_xml() { // Execute pipeline let initial = resolver.dereference("").unwrap(); - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature removed @@ -412,7 +412,7 @@ fn parse_and_execute_transforms_roundtrip() { let initial = resolver.dereference(uri).unwrap(); // Step 3: Execute transforms - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Step 4: Verify @@ -443,7 +443,7 @@ fn enveloped_signature_only_child() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), ]; - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Only whitespace and root element should remain @@ -469,7 +469,7 @@ fn enveloped_preserves_surrounding_whitespace() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), ]; - let result = execute_transforms(&doc, sig_node.id(), initial, &transforms).unwrap(); + let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // The text nodes ("\n ") around Signature are preserved — they're not From 4a2a4b1106bc4d2d24f1fd5a5a919da45aa9aabc Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 00:24:09 +0200 Subject: [PATCH 05/16] docs(xmldsig): add explicit URL to XMLDSig spec reference in rustdoc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Convert bracket-only markdown link to full URL for §4.3.3.2 - Correct test description for whitespace text node behavior --- src/xmldsig/transforms.rs | 2 +- tests/transforms_integration.rs | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index a172c44..12bef6d 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -95,7 +95,7 @@ pub fn apply_transform<'a>( /// 1. Start with `initial_data` (from URI dereference). /// 2. Apply each transform sequentially. /// 3. If the result is still a `NodeSet`, apply default inclusive C14N 1.0 -/// to produce bytes (per [XMLDSig §4.3.3.2]). +/// to produce bytes (per [XMLDSig §4.3.3.2](https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel)). /// /// Returns the final byte sequence ready for digest computation. pub fn execute_transforms<'a>( diff --git a/tests/transforms_integration.rs b/tests/transforms_integration.rs index 11820d7..00b9fa3 100644 --- a/tests/transforms_integration.rs +++ b/tests/transforms_integration.rs @@ -424,8 +424,10 @@ fn parse_and_execute_transforms_roundtrip() { #[test] fn enveloped_signature_only_child() { - // Edge case: the Signature is the only child of root. - // After enveloped transform, only the root element (empty) remains. + // Edge case: the Signature is the only element child of root. + // After the enveloped transform, the Signature subtree is removed and + // only the root element (plus any surrounding whitespace text nodes) + // remains in the NodeSet / canonical output. let xml = r#" @@ -446,7 +448,8 @@ fn enveloped_signature_only_child() { let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); - // Only whitespace and root element should remain + // The Signature subtree should be removed; only the root element and any + // surrounding whitespace text nodes should remain. assert!(!output.contains("Signature"), "output: {output}"); assert!(output.contains(""), "output: {output}"); } From 662c929933c0618c3850801a9bb3e6792f2454f1 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 00:28:17 +0200 Subject: [PATCH 06/16] fix(xmldsig): validate Transform namespace and require PrefixList attribute - Validate elements belong to XMLDSig namespace (fail-closed) - Reject without required PrefixList attribute - Change parse_inclusive_prefixes to return Result for error propagation - Add XMLDSIG_NS_URI constant for namespace validation - Add test for missing PrefixList producing error --- src/xmldsig/transforms.rs | 47 ++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 12bef6d..4113ee0 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -25,6 +25,9 @@ use crate::c14n::{self, C14nAlgorithm}; /// The algorithm URI for the enveloped signature transform. pub const ENVELOPED_SIGNATURE_URI: &str = "http://www.w3.org/2000/09/xmldsig#enveloped-signature"; +/// XMLDSig namespace URI for `` elements. +const XMLDSIG_NS_URI: &str = "http://www.w3.org/2000/09/xmldsig#"; + /// Namespace URI for Exclusive C14N `` elements. const EXCLUSIVE_C14N_NS_URI: &str = "http://www.w3.org/2001/10/xml-exc-c14n#"; @@ -143,8 +146,11 @@ pub fn parse_transforms(transforms_node: Node) -> Result, Transfo if !child.is_element() { continue; } - // Match on local name, ignoring namespace prefix (could be ds:Transform or Transform) - if child.tag_name().name() != "Transform" { + // Match on local name AND namespace — reject non-XMLDSig elements + // that happen to be named "Transform" (fail-closed parsing). + if child.tag_name().name() != "Transform" + || child.tag_name().namespace() != Some(XMLDSIG_NS_URI) + { continue; } let uri = child.attribute("Algorithm").ok_or_else(|| { @@ -158,7 +164,7 @@ pub fn parse_transforms(transforms_node: Node) -> Result, Transfo } else if let Some(mut algo) = C14nAlgorithm::from_uri(uri) { // For exclusive C14N, check for InclusiveNamespaces child if algo.mode() == c14n::C14nMode::Exclusive1_0 { - if let Some(prefix_list) = parse_inclusive_prefixes(child) { + if let Some(prefix_list) = parse_inclusive_prefixes(child)? { algo = algo.with_prefix_list(&prefix_list); } } @@ -179,23 +185,32 @@ pub fn parse_transforms(transforms_node: Node) -> Result, Transfo /// the element MUST be in the `http://www.w3.org/2001/10/xml-exc-c14n#` namespace. /// Elements with the same local name but a different namespace are ignored. /// +/// Returns `Ok(None)` if no `` child is present. +/// Returns `Err` if the element exists but lacks the required `PrefixList` attribute +/// (fail-closed: malformed control elements are rejected, not silently ignored). +/// /// The element is typically: /// ```xml /// /// ``` -fn parse_inclusive_prefixes(transform_node: Node) -> Option { +fn parse_inclusive_prefixes(transform_node: Node) -> Result, TransformError> { for child in transform_node.children() { if child.is_element() { let tag = child.tag_name(); if tag.name() == "InclusiveNamespaces" && tag.namespace() == Some(EXCLUSIVE_C14N_NS_URI) { - return child.attribute("PrefixList").map(String::from); + let prefix_list = child.attribute("PrefixList").ok_or_else(|| { + TransformError::UnsupportedTransform( + "missing PrefixList attribute on ".into(), + ) + })?; + return Ok(Some(prefix_list.to_string())); } } } - None + Ok(None) } #[cfg(test)] @@ -485,6 +500,26 @@ mod tests { } } + #[test] + fn parse_transforms_missing_prefix_list_is_error() { + // InclusiveNamespaces in correct namespace but without PrefixList + // attribute should be rejected (fail-closed), not silently ignored. + let xml = r#" + + + + "#; + let doc = Document::parse(xml).unwrap(); + + let result = parse_transforms(doc.root_element()); + assert!(result.is_err()); + assert!(matches!( + result.unwrap_err(), + TransformError::UnsupportedTransform(_) + )); + } + #[test] fn parse_transforms_unsupported_algorithm() { let xml = r#" From f04bc62fff66126fbe59d95474a865ad8d397a0e Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 00:48:09 +0200 Subject: [PATCH 07/16] refactor(xmldsig): remove doc param from transform API, rename error variant - Remove redundant doc parameter from apply_transform/execute_transforms (signature_node already carries document reference) - Validate signature_node against nodes.document() instead of separate doc - Rename EnvelopedSignatureNotFound to CrossDocumentSignatureNode (reflects actual failure: cross-document mismatch, not missing node) - Fail closed on with wrong namespace (error, not skip) --- src/xmldsig/transforms.rs | 60 ++++++++++++++++----------------- src/xmldsig/types.rs | 7 ++-- tests/transforms_integration.rs | 20 +++++------ 3 files changed, 44 insertions(+), 43 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 4113ee0..93588bc 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -17,7 +17,7 @@ //! | Exclusive C14N 1.0 | NodeSet → Binary | P0 | //! | Base64 decode | Binary → Binary | P1 (future) | -use roxmltree::{Document, Node}; +use roxmltree::Node; use super::types::{TransformData, TransformError}; use crate::c14n::{self, C14nAlgorithm}; @@ -50,14 +50,10 @@ pub enum Transform { /// /// `signature_node` is the `` element that contains the /// `` being processed. It is used by the enveloped transform -/// to know which signature subtree to exclude. -/// -/// Accepting `Node` instead of `NodeId` binds the signature target to a -/// specific document at the type level, preventing cross-document misuse -/// (a bare `NodeId` is an untyped `u32` index that could silently resolve -/// to the wrong node if passed with a different document). +/// to know which signature subtree to exclude. The node must belong to the +/// same document as the `NodeSet` in `input`; a cross-document mismatch +/// returns [`TransformError::CrossDocumentSignatureNode`]. pub fn apply_transform<'a>( - doc: &'a Document<'a>, signature_node: Node<'a, 'a>, transform: &Transform, input: TransformData<'a>, @@ -73,8 +69,11 @@ pub fn apply_transform<'a>( // xmlsec1 equivalent: // xmlSecNodeSetGetChildren(doc, signatureNode, 1, 1) // inverted tree // xmlSecNodeSetAdd(inNodes, children, Intersection) // intersect = subtract - if !std::ptr::eq(signature_node.document() as *const _, doc as *const _) { - return Err(TransformError::EnvelopedSignatureNotFound); + if !std::ptr::eq( + signature_node.document() as *const _, + nodes.document() as *const _, + ) { + return Err(TransformError::CrossDocumentSignatureNode); } nodes.exclude_subtree(signature_node); Ok(TransformData::NodeSet(nodes)) @@ -82,9 +81,6 @@ pub fn apply_transform<'a>( Transform::C14n(algo) => { let nodes = input.into_node_set()?; let mut output = Vec::new(); - // Build a predicate closure that checks node membership in - // the NodeSet. The C14N serializer calls this for each node - // during document-order traversal. let predicate = |node: Node| nodes.contains(node); c14n::canonicalize(nodes.document(), Some(&predicate), algo, &mut output) .map_err(|e| TransformError::C14n(e.to_string()))?; @@ -102,7 +98,6 @@ pub fn apply_transform<'a>( /// /// Returns the final byte sequence ready for digest computation. pub fn execute_transforms<'a>( - doc: &'a Document<'a>, signature_node: Node<'a, 'a>, initial_data: TransformData<'a>, transforms: &[Transform], @@ -110,7 +105,7 @@ pub fn execute_transforms<'a>( let mut data = initial_data; for transform in transforms { - data = apply_transform(doc, signature_node, transform, data)?; + data = apply_transform(signature_node, transform, data)?; } // Final coercion: if the result is still a NodeSet, canonicalize with @@ -146,13 +141,18 @@ pub fn parse_transforms(transforms_node: Node) -> Result, Transfo if !child.is_element() { continue; } - // Match on local name AND namespace — reject non-XMLDSig elements - // that happen to be named "Transform" (fail-closed parsing). - if child.tag_name().name() != "Transform" - || child.tag_name().namespace() != Some(XMLDSIG_NS_URI) - { + // Match on local name AND namespace. + // Ignore non-Transform elements, but fail closed if we see a + // element that is not in the XMLDSig namespace. + let tag = child.tag_name(); + if tag.name() != "Transform" { continue; } + if tag.namespace() != Some(XMLDSIG_NS_URI) { + return Err(TransformError::UnsupportedTransform( + "unexpected element with incorrect or missing XMLDSig namespace".into(), + )); + } let uri = child.attribute("Algorithm").ok_or_else(|| { TransformError::UnsupportedTransform( "missing Algorithm attribute on ".into(), @@ -218,6 +218,7 @@ fn parse_inclusive_prefixes(transform_node: Node) -> Result, Tran mod tests { use super::*; use crate::xmldsig::NodeSet; + use roxmltree::Document; // ── Enveloped transform ────────────────────────────────────────── @@ -244,7 +245,7 @@ mod tests { let data = TransformData::NodeSet(node_set); // Apply enveloped transform - let result = apply_transform(&doc, sig_node, &Transform::Enveloped, data).unwrap(); + let result = apply_transform(sig_node, &Transform::Enveloped, data).unwrap(); let node_set = result.into_node_set().unwrap(); // Root and data should be in the set @@ -276,7 +277,7 @@ mod tests { let doc = Document::parse(xml).unwrap(); // Binary input should fail with TypeMismatch let data = TransformData::Binary(vec![1, 2, 3]); - let result = apply_transform(&doc, doc.root_element(), &Transform::Enveloped, data); + let result = apply_transform(doc.root_element(), &Transform::Enveloped, data); assert!(result.is_err()); match result.unwrap_err() { TransformError::TypeMismatch { expected, got } => { @@ -299,8 +300,7 @@ mod tests { let algo = C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315").unwrap(); - let result = - apply_transform(&doc, doc.root_element(), &Transform::C14n(algo), data).unwrap(); + let result = apply_transform(doc.root_element(), &Transform::C14n(algo), data).unwrap(); let bytes = result.into_binary().unwrap(); let output = String::from_utf8(bytes).unwrap(); @@ -316,7 +316,7 @@ mod tests { let algo = C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315").unwrap(); let data = TransformData::Binary(vec![1, 2, 3]); - let result = apply_transform(&doc, doc.root_element(), &Transform::C14n(algo), data); + let result = apply_transform(doc.root_element(), &Transform::C14n(algo), data); assert!(result.is_err()); assert!(matches!( @@ -352,7 +352,7 @@ mod tests { ), ]; - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature subtree should be gone; attributes sorted @@ -369,7 +369,7 @@ mod tests { let doc = Document::parse(xml).unwrap(); let initial = TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); - let result = execute_transforms(&doc, doc.root_element(), initial, &[]).unwrap(); + let result = execute_transforms(doc.root_element(), initial, &[]).unwrap(); let output = String::from_utf8(result).unwrap(); assert_eq!(output, r#""#); @@ -383,7 +383,7 @@ mod tests { let doc = Document::parse(xml).unwrap(); let initial = TransformData::Binary(b"raw bytes".to_vec()); - let result = execute_transforms(&doc, doc.root_element(), initial, &[]).unwrap(); + let result = execute_transforms(doc.root_element(), initial, &[]).unwrap(); assert_eq!(result, b"raw bytes"); } @@ -414,7 +414,7 @@ mod tests { let node_set = NodeSet::entire_document_without_comments(&doc); let data = TransformData::NodeSet(node_set); - let result = apply_transform(&doc, sig_node, &Transform::Enveloped, data).unwrap(); + let result = apply_transform(sig_node, &Transform::Enveloped, data).unwrap(); let node_set = result.into_node_set().unwrap(); // Sig1 should still be in the set @@ -621,7 +621,7 @@ mod tests { // Execute the pipeline with empty URI (entire document) let initial = TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); diff --git a/src/xmldsig/types.rs b/src/xmldsig/types.rs index 3abfa11..e628201 100644 --- a/src/xmldsig/types.rs +++ b/src/xmldsig/types.rs @@ -225,7 +225,8 @@ pub enum TransformError { #[error("C14N error: {0}")] C14n(String), - /// The Signature node referenced by the enveloped transform was not found. - #[error("enveloped-signature transform: Signature node not found in document")] - EnvelopedSignatureNotFound, + /// The Signature node passed to the enveloped transform belongs to a + /// different `Document` than the input `NodeSet`. + #[error("enveloped-signature transform: invalid Signature node for this document")] + CrossDocumentSignatureNode, } diff --git a/tests/transforms_integration.rs b/tests/transforms_integration.rs index 00b9fa3..f394d83 100644 --- a/tests/transforms_integration.rs +++ b/tests/transforms_integration.rs @@ -71,7 +71,7 @@ fn enveloped_exc_c14n_saml_response() { let initial = resolver.dereference("").unwrap(); // Execute pipeline - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature must be completely absent @@ -123,7 +123,7 @@ fn enveloped_inclusive_c14n() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), ]; - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature gone @@ -162,7 +162,7 @@ fn enveloped_inclusive_c14n_1_1() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_1, false)), ]; - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); assert!(!output.contains("Signature"), "output: {output}"); @@ -201,7 +201,7 @@ fn id_uri_enveloped_assertion_level_signature() { ), ]; - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Should contain the Assertion element and Subject @@ -250,7 +250,7 @@ fn nested_signatures_only_own_excluded() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), ]; - let result = execute_transforms(&doc, outer_sig, initial, &transforms).unwrap(); + let result = execute_transforms(outer_sig, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Outer signature should be excluded @@ -291,7 +291,7 @@ fn enveloped_only_falls_back_to_default_c14n() { xml_sec::xmldsig::TransformData::NodeSet(NodeSet::entire_document_without_comments(&doc)); let transforms = vec![Transform::Enveloped]; - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature gone @@ -354,7 +354,7 @@ fn exc_c14n_with_prefix_list_from_xml() { // Execute pipeline let initial = resolver.dereference("").unwrap(); - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Signature removed @@ -412,7 +412,7 @@ fn parse_and_execute_transforms_roundtrip() { let initial = resolver.dereference(uri).unwrap(); // Step 3: Execute transforms - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // Step 4: Verify @@ -445,7 +445,7 @@ fn enveloped_signature_only_child() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), ]; - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // The Signature subtree should be removed; only the root element and any @@ -472,7 +472,7 @@ fn enveloped_preserves_surrounding_whitespace() { Transform::C14n(C14nAlgorithm::new(C14nMode::Inclusive1_0, false)), ]; - let result = execute_transforms(&doc, sig_node, initial, &transforms).unwrap(); + let result = execute_transforms(sig_node, initial, &transforms).unwrap(); let output = String::from_utf8(result).unwrap(); // The text nodes ("\n ") around Signature are preserved — they're not From 1ad6bdfb7e7d15bf620e1e855789d7ecd57c0101 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 01:58:31 +0200 Subject: [PATCH 08/16] feat(ci): add Rust code review instructions for automated PR analysis - Add .github/instructions/rust.instructions.md with tiered review priorities - Add unit test for CrossDocumentSignatureNode error path - Add explicit whitespace assertion in enveloped transform test --- .github/instructions/rust.instructions.md | 69 +++++++++++++++++++++++ src/xmldsig/transforms.rs | 23 ++++++++ tests/transforms_integration.rs | 5 ++ 3 files changed, 97 insertions(+) create mode 100644 .github/instructions/rust.instructions.md diff --git a/.github/instructions/rust.instructions.md b/.github/instructions/rust.instructions.md new file mode 100644 index 0000000..d1dbf41 --- /dev/null +++ b/.github/instructions/rust.instructions.md @@ -0,0 +1,69 @@ +--- +applyTo: "**/*.rs" +--- + +# Rust Code Review Instructions + +## Review Priority (HIGH → LOW) + +Focus review effort on real bugs, not cosmetics. Stop after finding issues in higher tiers — do not pad reviews with low-priority nitpicks. + +### Tier 1 — Logic Bugs and Correctness (MUST flag) +- Incorrect algorithm: wrong canonicalization order, wrong namespace handling, off-by-one, TOCTOU +- Missing validation: unchecked array index, unvalidated input from XML/base64 +- Resource leaks: unclosed handles, missing cleanup +- Concurrency: data races, lock ordering, shared mutable state without sync +- Error swallowing: `let _ = fallible_call()` silently dropping errors that affect correctness +- Integer overflow/truncation on security-critical paths (nonces, sizes, lengths) + +### Tier 2 — Safety and Security (MUST flag) +- `unsafe` without `// SAFETY:` invariant explanation +- `unwrap()`/`expect()` on I/O or network data (must use `Result` propagation) +- Sensitive data (keys, passwords) exposed in logs or error messages +- Constant-time comparison not used for digest/signature comparison +- Hardcoded secrets, credentials, or private URLs + +### Tier 3 — API Design and Robustness (flag if clear improvement) +- Public API missing `#[must_use]` on `Result`-returning methods +- `pub` visibility where `pub(crate)` suffices +- Missing `Send + Sync` bounds on types used across threads +- `Clone` on large types where a reference would work + +### Tier 4 — Style (ONLY flag if misleading or confusing) +- Variable/function names that actively mislead about behavior +- Dead code (unused functions, unreachable branches) + +## DO NOT Flag (Explicit Exclusions) + +These are not actionable review findings. Do not raise them: + +- **Comment wording vs code behavior**: If a comment describes intent that slightly differs from implementation details, the intent is clear. Do not suggest rewording comments to match implementation details. Comments describe intent and context, not repeat the code. +- **Comment precision**: "returns X" when it technically returns `Result` — the comment conveys meaning, not type signature. +- **Magic numbers with context**: Algorithm URI strings used once with a descriptive variable name or comment. Do not suggest extracting a named constant when the value is used once with clear context. +- **Minor naming preferences**: `algo` vs `algorithm`, `ns` vs `namespace` — these are team style, not bugs. +- **Import organization**: Single unused import that clippy would catch anyway. +- **Test code style**: Tests prioritize readability and explicitness over DRY. Repeated setup code in tests is acceptable. +- **`#[allow(clippy::...)]` with justification comment**: Respect the author's suppression if explained. +- **W3C spec section references**: Comments referencing W3C spec sections (e.g., "XMLDSig §4.3.3.2", "Exc-C14N §3") are documentation, not noise. Do not flag as unnecessary or suggest removal. + +## Scope Rules + +- **Review ONLY code within the PR's diff.** Do not suggest inline fixes for unchanged lines. +- For issues **outside the diff**, suggest opening a separate issue. +- **Read the PR description.** If it lists known limitations or deferred items, do not re-flag them. + +## Rust-Specific Standards + +- Prefer `#[expect(lint)]` over `#[allow(lint)]` — `#[expect]` warns when suppression becomes unnecessary +- `TryFrom`/`TryInto` for fallible conversions; `as` casts need justification +- No `unwrap()` / `expect()` on I/O paths — use `?` propagation +- `expect()` is acceptable for programmer invariants (e.g., `const` construction) with reason +- Code must pass `cargo clippy --all-features -- -D warnings` +- W3C/RFC compliance comments (e.g., "XMLDSig §4.3.3.2", "RFC 3986 §5.3") are documentation, not noise — preserve them + +## Testing Standards + +- Test naming: `fn test__()` or `fn test_()` +- Integration tests that require infrastructure use `#[ignore = "reason"]` +- Prefer `assert_eq!` with message over bare `assert!` for better failure output +- Hardcoded values in tests are fine when accompanied by explanatory comments or assertion messages diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 93588bc..330b078 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -288,6 +288,29 @@ mod tests { } } + #[test] + fn enveloped_rejects_cross_document_signature_node() { + // Signature node from a different Document must be rejected, + // not silently used to exclude wrong subtree. + let xml = r#""#; + let doc1 = Document::parse(xml).unwrap(); + let doc2 = Document::parse(xml).unwrap(); + + // NodeSet from doc1, Signature node from doc2 + let node_set = NodeSet::entire_document_without_comments(&doc1); + let input = TransformData::NodeSet(node_set); + let sig_from_doc2 = doc2 + .descendants() + .find(|n| n.is_element() && n.tag_name().name() == "Signature") + .unwrap(); + + let result = apply_transform(sig_from_doc2, &Transform::Enveloped, input); + assert!(matches!( + result, + Err(TransformError::CrossDocumentSignatureNode) + )); + } + // ── C14N transform ─────────────────────────────────────────────── #[test] diff --git a/tests/transforms_integration.rs b/tests/transforms_integration.rs index f394d83..f54f864 100644 --- a/tests/transforms_integration.rs +++ b/tests/transforms_integration.rs @@ -478,5 +478,10 @@ fn enveloped_preserves_surrounding_whitespace() { // The text nodes ("\n ") around Signature are preserved — they're not // part of the Signature subtree (they're siblings). C14N will emit them. assert!(output.contains("text"), "output: {output}"); + // Verify whitespace around removed Signature is actually in the output + assert!( + output.contains("\n text\n"), + "whitespace around Signature should be preserved: {output}" + ); assert!(!output.contains("SignedInfo"), "output: {output}"); } From 01097ec88a31b7f59ca55937e20ba37c991eaac5 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 02:29:28 +0200 Subject: [PATCH 09/16] refactor(xmldsig): restrict apply_transform visibility, use correct error variant - Change apply_transform from pub to pub(crate) (internal helper) - Use UnsupportedTransform for default C14N fallback URI failure (not a runtime C14N error, but an unsupported algorithm condition) --- src/xmldsig/transforms.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 330b078..606e062 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -53,7 +53,7 @@ pub enum Transform { /// to know which signature subtree to exclude. The node must belong to the /// same document as the `NodeSet` in `input`; a cross-document mismatch /// returns [`TransformError::CrossDocumentSignatureNode`]. -pub fn apply_transform<'a>( +pub(crate) fn apply_transform<'a>( signature_node: Node<'a, 'a>, transform: &Transform, input: TransformData<'a>, @@ -115,7 +115,9 @@ pub fn execute_transforms<'a>( TransformData::NodeSet(nodes) => { let algo = C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315") .ok_or_else(|| { - TransformError::C14n("unsupported default C14N algorithm URI".to_string()) + TransformError::UnsupportedTransform( + "unsupported default C14N algorithm URI".into(), + ) })?; let mut output = Vec::new(); let predicate = |node: Node| nodes.contains(node); From 5bc19d660b55068e35f5c66939ed9f97117e3b5b Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 02:31:55 +0200 Subject: [PATCH 10/16] test(xmldsig): use two real Signature elements in nested-signatures test - Replace placeholder with - Both elements now share XMLDSig namespace, differentiated by Id - Matches realistic SAML nested signature scenario --- src/xmldsig/transforms.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 606e062..7695539 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -417,23 +417,23 @@ mod tests { #[test] fn enveloped_only_excludes_own_signature() { - // Nested signatures: enveloped transform should only exclude - // the specific being verified, not all signatures - let xml = r#" + // Two real elements: enveloped transform should only + // exclude the specific one being verified, not the other. + let xml = r#" hello - - - - - - + + + + + + "#; let doc = Document::parse(xml).unwrap(); - // We are verifying the element, not + // We are verifying sig-target, not sig-other let sig_node = doc .descendants() - .find(|n| n.is_element() && n.tag_name().name() == "Signature") + .find(|n| n.is_element() && n.attribute("Id") == Some("sig-target")) .unwrap(); let node_set = NodeSet::entire_document_without_comments(&doc); @@ -442,14 +442,14 @@ mod tests { let result = apply_transform(sig_node, &Transform::Enveloped, data).unwrap(); let node_set = result.into_node_set().unwrap(); - // Sig1 should still be in the set - let sig1 = doc + // sig-other should still be in the set + let sig_other = doc .descendants() - .find(|n| n.is_element() && n.tag_name().name() == "Sig1") + .find(|n| n.is_element() && n.attribute("Id") == Some("sig-other")) .unwrap(); assert!( - node_set.contains(sig1), - "other signature elements should NOT be excluded" + node_set.contains(sig_other), + "other Signature elements should NOT be excluded" ); // Signature should be excluded From 277f3b423ee2e7dece1b0c729f2130c31d865325 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 03:00:19 +0200 Subject: [PATCH 11/16] refactor(xmldsig): mark TransformError non_exhaustive, simplify ptr::eq call - Add #[non_exhaustive] to TransformError for semver safety - Remove unnecessary raw pointer casts in cross-document check --- src/xmldsig/transforms.rs | 5 +---- src/xmldsig/types.rs | 1 + 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 7695539..06908dc 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -69,10 +69,7 @@ pub(crate) fn apply_transform<'a>( // xmlsec1 equivalent: // xmlSecNodeSetGetChildren(doc, signatureNode, 1, 1) // inverted tree // xmlSecNodeSetAdd(inNodes, children, Intersection) // intersect = subtract - if !std::ptr::eq( - signature_node.document() as *const _, - nodes.document() as *const _, - ) { + if !std::ptr::eq(signature_node.document(), nodes.document()) { return Err(TransformError::CrossDocumentSignatureNode); } nodes.exclude_subtree(signature_node); diff --git a/src/xmldsig/types.rs b/src/xmldsig/types.rs index e628201..eb85a01 100644 --- a/src/xmldsig/types.rs +++ b/src/xmldsig/types.rs @@ -199,6 +199,7 @@ fn collect_subtree_ids(node: Node<'_, '_>, ids: &mut HashSet) { /// Errors during transform processing. #[derive(Debug, thiserror::Error)] +#[non_exhaustive] pub enum TransformError { /// Data type mismatch between transforms. #[error("type mismatch: expected {expected}, got {got}")] From 9a8064b08db40497adaeb3e3ac3a0c652927833e Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 03:22:25 +0200 Subject: [PATCH 12/16] docs(xmldsig): add non_exhaustive rationale, add code-review instructions - Add doc comment explaining #[non_exhaustive] on TransformError (pre-1.0 crate) - Add .github/instructions/code-review.instructions.md for xml-sec --- .../instructions/code-review.instructions.md | 26 +++++++++++++++++++ src/xmldsig/types.rs | 3 +++ 2 files changed, 29 insertions(+) create mode 100644 .github/instructions/code-review.instructions.md diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md new file mode 100644 index 0000000..0849eb8 --- /dev/null +++ b/.github/instructions/code-review.instructions.md @@ -0,0 +1,26 @@ +--- +applyTo: "**/*.rs" +--- + +# Code Review Instructions for xml-sec + +## Scope Rules (CRITICAL) + +- **Review ONLY code within the PR's diff.** Do not suggest inline fixes for unchanged lines. +- For issues in code **outside the diff**, suggest creating a **separate issue** instead of proposing code changes. Example: "Consider opening an issue to add namespace validation here — this is outside this PR's scope." +- **Read the PR description carefully.** If the PR body has an "out of scope" section listing items handled by other PRs, do NOT flag those items. + +## Rust Standards + +- `unsafe` blocks require `// SAFETY:` comments explaining the invariant +- Prefer `#[expect(lint)]` over `#[allow(lint)]` — `#[expect]` warns when suppression becomes unnecessary +- Use `TryFrom`/`TryInto` for fallible conversions; `as` casts need `#[expect(clippy::cast_possible_truncation)]` with reason +- No `unwrap()` / `expect()` on I/O paths — use `Result` propagation +- `expect()` is acceptable for programmer invariants (lock poisoning) with `#[expect(clippy::expect_used, reason = "...")]` +- Code must pass `cargo clippy --all-features -- -D warnings` + +## Testing + +- Corruption/validation tests: tamper the relevant field (e.g., digest value, signature bytes, base64 encoding) and assert the error +- Use same serialization as production (e.g., canonical XML output, not shortcuts) +- Test naming: `fn __()` diff --git a/src/xmldsig/types.rs b/src/xmldsig/types.rs index eb85a01..115649a 100644 --- a/src/xmldsig/types.rs +++ b/src/xmldsig/types.rs @@ -198,6 +198,9 @@ fn collect_subtree_ids(node: Node<'_, '_>, ids: &mut HashSet) { } /// Errors during transform processing. +/// +/// Marked `#[non_exhaustive]` — new variants will be added as more transforms +/// are implemented (Base64, XPath). Crate is pre-1.0 so this is not a semver concern. #[derive(Debug, thiserror::Error)] #[non_exhaustive] pub enum TransformError { From f19c79b0cf5a9bd404c5763749a0374142433616 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 04:04:41 +0200 Subject: [PATCH 13/16] fix(xmldsig): validate Transforms element and reject unexpected children - Validate parse_transforms input is in XMLDSig namespace - Reject any child element that is not (fail-closed) - Prevents masking parsing bugs via silent empty transform chain --- src/xmldsig/transforms.rs | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 06908dc..11148ad 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -134,22 +134,32 @@ pub fn execute_transforms<'a>( /// For Exclusive C14N, also parses the optional `` child element. pub fn parse_transforms(transforms_node: Node) -> Result, TransformError> { + // Validate that we received a element. + if !transforms_node.is_element() { + return Err(TransformError::UnsupportedTransform( + "expected element but got non-element node".into(), + )); + } + let transforms_tag = transforms_node.tag_name(); + if transforms_tag.name() != "Transforms" || transforms_tag.namespace() != Some(XMLDSIG_NS_URI) { + return Err(TransformError::UnsupportedTransform( + "expected element in XMLDSig namespace".into(), + )); + } + let mut chain = Vec::new(); for child in transforms_node.children() { if !child.is_element() { continue; } - // Match on local name AND namespace. - // Ignore non-Transform elements, but fail closed if we see a - // element that is not in the XMLDSig namespace. + + // Only children are allowed; fail closed on any other element. let tag = child.tag_name(); - if tag.name() != "Transform" { - continue; - } - if tag.namespace() != Some(XMLDSIG_NS_URI) { + if tag.name() != "Transform" || tag.namespace() != Some(XMLDSIG_NS_URI) { return Err(TransformError::UnsupportedTransform( - "unexpected element with incorrect or missing XMLDSig namespace".into(), + "unexpected child element of ; only is allowed" + .into(), )); } let uri = child.attribute("Algorithm").ok_or_else(|| { From 2423b0e991d8cbdb9f987b2fe498a385b83e57ec Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 04:37:20 +0200 Subject: [PATCH 14/16] refactor(xmldsig): use #[expect] over #[allow] in test module, trim doc comment - Switch #[allow(clippy::unwrap_used)] to #[expect] with reason - Simplify TransformError doc comment (remove semver discussion) --- src/xmldsig/transforms.rs | 2 +- src/xmldsig/types.rs | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 11148ad..2b41cb3 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -223,7 +223,7 @@ fn parse_inclusive_prefixes(transform_node: Node) -> Result, Tran } #[cfg(test)] -#[allow(clippy::unwrap_used)] +#[expect(clippy::unwrap_used, reason = "tests use trusted XML fixtures")] mod tests { use super::*; use crate::xmldsig::NodeSet; diff --git a/src/xmldsig/types.rs b/src/xmldsig/types.rs index 115649a..27314e6 100644 --- a/src/xmldsig/types.rs +++ b/src/xmldsig/types.rs @@ -199,8 +199,7 @@ fn collect_subtree_ids(node: Node<'_, '_>, ids: &mut HashSet) { /// Errors during transform processing. /// -/// Marked `#[non_exhaustive]` — new variants will be added as more transforms -/// are implemented (Base64, XPath). Crate is pre-1.0 so this is not a semver concern. +/// New variants may be added as more transforms are implemented (Base64, XPath). #[derive(Debug, thiserror::Error)] #[non_exhaustive] pub enum TransformError { From fc508b99c364118d46ce2e886d0141ac8b2d0815 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 05:59:38 +0200 Subject: [PATCH 15/16] refactor(xmldsig): use expect() for hardcoded C14N URI invariant - Replace ok_or_else with expect() for known-constant URI lookup - Add #[expect(clippy::expect_used)] with reason --- src/xmldsig/transforms.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/xmldsig/transforms.rs b/src/xmldsig/transforms.rs index 2b41cb3..ce613c2 100644 --- a/src/xmldsig/transforms.rs +++ b/src/xmldsig/transforms.rs @@ -110,12 +110,9 @@ pub fn execute_transforms<'a>( match data { TransformData::Binary(bytes) => Ok(bytes), TransformData::NodeSet(nodes) => { + #[expect(clippy::expect_used, reason = "hardcoded URI is a known constant")] let algo = C14nAlgorithm::from_uri("http://www.w3.org/TR/2001/REC-xml-c14n-20010315") - .ok_or_else(|| { - TransformError::UnsupportedTransform( - "unsupported default C14N algorithm URI".into(), - ) - })?; + .expect("default C14N algorithm URI must be supported by C14nAlgorithm::from_uri"); let mut output = Vec::new(); let predicate = |node: Node| nodes.contains(node); c14n::canonicalize(nodes.document(), Some(&predicate), &algo, &mut output) From 70f6f180ef9d4db8ce4d8be556261cb3d21a6c27 Mon Sep 17 00:00:00 2001 From: Dmitry Prudnikov Date: Tue, 17 Mar 2026 13:55:40 +0200 Subject: [PATCH 16/16] docs(ci): add non_exhaustive and expect_used to DO NOT Flag exclusions - Exclude #[non_exhaustive] on pre-1.0 enums from review findings - Exclude #[expect(clippy::expect_used)] on hardcoded constant lookups --- .github/instructions/rust.instructions.md | 69 +++++++++++++---------- 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/.github/instructions/rust.instructions.md b/.github/instructions/rust.instructions.md index d1dbf41..7d7a51d 100644 --- a/.github/instructions/rust.instructions.md +++ b/.github/instructions/rust.instructions.md @@ -4,32 +4,37 @@ applyTo: "**/*.rs" # Rust Code Review Instructions -## Review Priority (HIGH → LOW) - -Focus review effort on real bugs, not cosmetics. Stop after finding issues in higher tiers — do not pad reviews with low-priority nitpicks. - -### Tier 1 — Logic Bugs and Correctness (MUST flag) -- Incorrect algorithm: wrong canonicalization order, wrong namespace handling, off-by-one, TOCTOU -- Missing validation: unchecked array index, unvalidated input from XML/base64 -- Resource leaks: unclosed handles, missing cleanup -- Concurrency: data races, lock ordering, shared mutable state without sync +## Review Priority (HIGH -> LOW) + +Focus review effort on real bugs, not cosmetics. Stop after finding issues in higher tiers -- do not pad reviews with low-priority nitpicks. + +### Tier 1 -- Logic Bugs and Correctness (MUST flag) +- Data corruption: wrong algorithm, incorrect ordering, dropped or duplicated data +- Off-by-one in boundaries, index lookups, or range operations +- Checksum/hash mismatches: computing over wrong byte range, verifying against stale value +- TOCTOU: checking state then acting on it without holding a lock or atomic operation +- Missing validation: unchecked index, unvalidated input from network/disk/external source +- Resource leaks: unclosed file handles, missing cleanup on error paths +- Concurrency: data races, lock ordering violations, missing synchronization - Error swallowing: `let _ = fallible_call()` silently dropping errors that affect correctness -- Integer overflow/truncation on security-critical paths (nonces, sizes, lengths) +- Integer overflow/truncation on sizes, offsets, lengths, or security-critical values -### Tier 2 — Safety and Security (MUST flag) +### Tier 2 -- Safety and Crash Recovery (MUST flag) - `unsafe` without `// SAFETY:` invariant explanation -- `unwrap()`/`expect()` on I/O or network data (must use `Result` propagation) -- Sensitive data (keys, passwords) exposed in logs or error messages -- Constant-time comparison not used for digest/signature comparison +- `unwrap()`/`expect()` on I/O, network, or deserialization paths (must use `Result` propagation) +- Crash safety: write ordering that leaves data unrecoverable after power loss +- Sensitive data (keys, passwords, tokens) not wrapped in `Zeroize`/`Zeroizing` +- Constant-time comparison not used for cryptographic MACs/checksums - Hardcoded secrets, credentials, or private URLs -### Tier 3 — API Design and Robustness (flag if clear improvement) -- Public API missing `#[must_use]` on `Result`-returning methods +### Tier 3 -- API Design and Robustness (flag if clear improvement) +- Public API missing `#[must_use]` on builder-style methods or non-`Result` types callers might discard - `pub` visibility where `pub(crate)` suffices - Missing `Send + Sync` bounds on types used across threads - `Clone` on large types where a reference would work +- Fallible operations returning `()` instead of `Result` -### Tier 4 — Style (ONLY flag if misleading or confusing) +### Tier 4 -- Style (ONLY flag if misleading or confusing) - Variable/function names that actively mislead about behavior - Dead code (unused functions, unreachable branches) @@ -37,14 +42,19 @@ Focus review effort on real bugs, not cosmetics. Stop after finding issues in hi These are not actionable review findings. Do not raise them: -- **Comment wording vs code behavior**: If a comment describes intent that slightly differs from implementation details, the intent is clear. Do not suggest rewording comments to match implementation details. Comments describe intent and context, not repeat the code. -- **Comment precision**: "returns X" when it technically returns `Result` — the comment conveys meaning, not type signature. -- **Magic numbers with context**: Algorithm URI strings used once with a descriptive variable name or comment. Do not suggest extracting a named constant when the value is used once with clear context. -- **Minor naming preferences**: `algo` vs `algorithm`, `ns` vs `namespace` — these are team style, not bugs. -- **Import organization**: Single unused import that clippy would catch anyway. +- **Comment wording vs code behavior**: If a comment says "flush when full" but the threshold uses `>=` not `>`, the intent is clear. Do not suggest rewording comments to match exact operators or implementation details. Comments describe intent, not repeat the code. +- **Comment precision**: "returns the key" when it technically returns `Result` -- the comment conveys meaning, not type signature. +- **Magic numbers with context**: `4` in `assert_eq!(header.len(), 4, "expected u32 checksum")` -- the assertion message provides context. Do not suggest a named constant when the value is used once in a test with an explanatory message. +- **Domain constants**: Specific numeric values for block sizes (e.g., `4096`), key sizes, protocol version numbers, port numbers, or configuration defaults are domain constants, not magic numbers, when used with surrounding context. +- **Minor naming preferences**: `lvl` vs `level`, `opts` vs `options`, `enc_part` vs `encrypted_part` -- these are team style, not bugs. +- **Import ordering**: Import grouping or ordering style. Unused imports are NOT cosmetic -- they cause `clippy -D warnings` failures and must be removed. - **Test code style**: Tests prioritize readability and explicitness over DRY. Repeated setup code in tests is acceptable. -- **`#[allow(clippy::...)]` with justification comment**: Respect the author's suppression if explained. -- **W3C spec section references**: Comments referencing W3C spec sections (e.g., "XMLDSig §4.3.3.2", "Exc-C14N §3") are documentation, not noise. Do not flag as unnecessary or suggest removal. +- **`#[allow(clippy::...)]` with justification comment**: When `#[allow]` has an adjacent comment explaining why it is used instead of `#[expect]`, do not suggest switching. Common reason: the lint does not fire on the current code but the suppression is kept defensively. `#[expect]` would fail the build with "unfulfilled expectation" in that case. +- **`#[allow(clippy::...)]` in existing/unchanged code**: Do not flag `#[allow]` suppressions in unchanged lines. New code should use `#[expect]` when the lint actually fires. +- **Temporary directory strategies in existing code**: Existing tests using manual temp paths are not a finding. New tests should prefer `tempfile::tempdir()`. +- **Semver concerns on pre-1.0 crates**: Adding required methods to public traits, changing public API signatures, or restructuring modules in a crate with version `0.x.y` is expected and does not require a default implementation or sealed trait pattern. Semver breakage only matters at `>= 1.0.0`. +- **Test infrastructure scripts (shell)**: Shell scripts in `tests/` that set up Docker containers, KDCs, databases, or other test infrastructure are not production code. Do not flag hardcoded test credentials (they are overridable via env vars), redundant principal creation (required by the specific technology), or non-production patterns in these scripts. +- **Previously resolved review threads**: If the same suggestion was already raised and resolved in a prior review round on this PR, do not re-raise it. Check the resolved threads before flagging. ## Scope Rules @@ -54,16 +64,17 @@ These are not actionable review findings. Do not raise them: ## Rust-Specific Standards -- Prefer `#[expect(lint)]` over `#[allow(lint)]` — `#[expect]` warns when suppression becomes unnecessary +- Prefer `#[expect(lint)]` over `#[allow(lint)]` when the lint actually fires -- `#[expect]` warns when suppression becomes unnecessary. Use `#[allow(lint)]` with a justification comment when the lint does not fire but defensive suppression is desired. - `TryFrom`/`TryInto` for fallible conversions; `as` casts need justification -- No `unwrap()` / `expect()` on I/O paths — use `?` propagation -- `expect()` is acceptable for programmer invariants (e.g., `const` construction) with reason +- No `unwrap()` / `expect()` on I/O paths -- use `?` propagation +- `expect()` is acceptable for programmer invariants (e.g., lock poisoning, `const` construction) with reason - Code must pass `cargo clippy --all-features -- -D warnings` -- W3C/RFC compliance comments (e.g., "XMLDSig §4.3.3.2", "RFC 3986 §5.3") are documentation, not noise — preserve them +- RFC/spec compliance comments (e.g., "RFC 4120 section 7.5.1") are documentation -- preserve them ## Testing Standards -- Test naming: `fn test__()` or `fn test_()` +- Test naming: `fn __()` or `fn test_()` +- Use `tempfile::tempdir()` for test directories -- ensures cleanup even on panic - Integration tests that require infrastructure use `#[ignore = "reason"]` - Prefer `assert_eq!` with message over bare `assert!` for better failure output - Hardcoded values in tests are fine when accompanied by explanatory comments or assertion messages