feat(xmldsig): enveloped signature transform and pipeline executor#14
feat(xmldsig): enveloped signature transform and pipeline executor#14
Conversation
- Implement Transform enum (Enveloped, C14n) with apply_transform() - Add execute_transforms() sequential pipeline with default C14N fallback - Add parse_transforms() to read <Transforms> XML into Vec<Transform> - Support InclusiveNamespaces PrefixList parsing for exclusive C14N - Wire NodeSet::exclude_subtree() for enveloped-signature semantics - Add EnvelopedSignatureNotFound error variant to TransformError Closes #13
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new xmldsig transforms module implementing Enveloped and C14n transforms, parsing of , functions to apply a single transform and execute a transform pipeline (with default C14N fallback), a new TransformError variant for missing Signature, and extensive unit/integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser as parse_transforms()
participant Executor as execute_transforms()
participant Applier as apply_transform()
participant C14n as canonicalize()
participant Doc as Document
Client->>Parser: provide <Transforms> node
Parser-->>Client: Vec<Transform>
Client->>Executor: (doc, signature_node_id, initial_data, transforms)
loop transforms
Executor->>Applier: apply_transform(transform_i, data, signature_node_id, doc)
alt Enveloped
Applier->>Doc: locate & exclude target Signature subtree
Doc-->>Applier: NodeSet without Signature
else C14n
Applier->>C14n: canonicalize(NodeSet, algorithm, inclusive_prefixes?)
C14n-->>Applier: bytes
end
Applier-->>Executor: TransformData (NodeSet | Binary)
end
alt remaining NodeSet
Executor->>C14n: default inclusive C14N 1.0 canonicalize(NodeSet)
C14n-->>Executor: Vec<u8>
end
Executor-->>Client: digest-ready bytes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline to the xmldsig module (enveloped-signature + C14N variants), plus end-to-end integration tests to validate URI dereference → transform parsing → transform execution → canonical output.
Changes:
- Introduces
xmldsig::transformswithTransform,parse_transforms,apply_transform, andexecute_transforms(including default C14N fallback). - Extends
TransformErrorwithEnvelopedSignatureNotFoundfor invalid/foreignNodeIdinputs to enveloped processing. - Adds integration tests covering common SAML-style transform chains, nested signatures,
#iddereference, and Exclusive C14NInclusiveNamespacesprefix lists.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/xmldsig/transforms.rs |
New transform pipeline implementation + unit tests for parsing/execution behavior. |
src/xmldsig/types.rs |
Adds a dedicated error variant for missing signature node during enveloped processing. |
src/xmldsig/mod.rs |
Exposes the new transforms module and re-exports the primary API. |
tests/transforms_integration.rs |
New integration tests validating real-world transform pipelines (SAML-like cases). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…siveNamespaces 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/transforms.rs`:
- Around line 129-181: The parser should reject Transform/InclusiveNamespaces
elements that are not in the expected XMLDSig namespaces and should fail when an
InclusiveNamespaces element is present but lacks a PrefixList; update
parse_transforms to check the child's tag_name().namespace() (or equivalent) and
only accept Transform elements in the XMLDSig namespace before reading the
Algorithm attribute, and update parse_inclusive_prefixes to check that the
InclusiveNamespaces element is in the exclusive C14N namespace and return a
Result (or propagate an error) instead of Option so that absence of PrefixList
yields a TransformError (e.g. TransformError::MalformedInclusiveNamespaces or
similar) which parse_transforms must handle when constructing C14n transforms
for C14nAlgorithm::Exclusive1_0; reference functions/idents: parse_transforms,
parse_inclusive_prefixes, TransformError, C14nAlgorithm,
ENVELOPED_SIGNATURE_URI, and Transform.
- Around line 48-53: The public API should take a bound Node instead of a raw
NodeId to prevent cross-document misuse: change the signature of apply_transform
to accept signature_node: roxmltree::Node<'a, 'a> (and similarly update the
other affected functions called at the same sites) instead of signature_node_id:
NodeId; remove any doc.get_node(signature_node_id) lookups and use the passed
signature_node directly when computing enveloped/exclusion checks, and update
all callers to pass the Node obtained from doc (ensuring lifetimes match). Keep
existing protections by leaving NodeSet::contains() and exclude_subtree()
unchanged (they already use pointer equality), but ensure their callers now
receive the Node reference rather than a NodeId so cross-document mistakes
cannot occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29e8604e-f765-4fec-a483-af4d07c9c0a9
📒 Files selected for processing (4)
src/xmldsig/mod.rssrc/xmldsig/transforms.rssrc/xmldsig/types.rstests/transforms_integration.rs
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/xmldsig/transforms.rs (2)
141-147:⚠️ Potential issue | 🟠 MajorMake transform parsing fail-closed for malformed/foreign control elements.
Line 142 matches only local name, so non-XMLDSig
*:Transformcan be accepted. Also, whenInclusiveNamespacesexists but lacksPrefixList(Line 189), parsing silently treats it as absent. Both are permissive in signature-verification code.Fail-closed parsing sketch
+const XMLDSIG_NS_URI: &str = "http://www.w3.org/2000/09/xmldsig#"; pub fn parse_transforms(transforms_node: Node) -> Result<Vec<Transform>, TransformError> { @@ - if child.tag_name().name() != "Transform" { + if child.tag_name().name() != "Transform" + || child.tag_name().namespace() != Some(XMLDSIG_NS_URI) + { continue; } @@ - 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); } }-fn parse_inclusive_prefixes(transform_node: Node) -> Option<String> { +fn parse_inclusive_prefixes(transform_node: Node) -> Result<Option<String>, TransformError> { for child in transform_node.children() { @@ - return child.attribute("PrefixList").map(String::from); + let prefix_list = child.attribute("PrefixList").ok_or_else(|| { + TransformError::UnsupportedTransform( + "missing PrefixList attribute on <InclusiveNamespaces>".into(), + ) + })?; + return Ok(Some(prefix_list.to_string())); } } } - None + Ok(None) }Also applies to: 156-157, 183-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/transforms.rs` around lines 141 - 147, The parser is currently accepting any element with local name "Transform" and silently tolerating an InclusiveNamespaces element missing a PrefixList; update the checks to fail-closed: when matching Transform elements (where code uses child.tag_name().name() and child.attribute("Algorithm")), require the XMLDSIG namespace (e.g. child.tag_name().namespace() == Some("http://www.w3.org/2000/09/xmldsig#")) and reject/return a TransformError for any Transform in a different namespace or missing/empty Algorithm attribute; likewise, when handling an InclusiveNamespaces element (look for the element named "InclusiveNamespaces"), require the PrefixList attribute to exist and be non-empty and return a proper TransformError (e.g. TransformError::UnsupportedTransform or a MalformedTransform variant) if it is absent/empty; apply the same namespace-and-attribute strictness to the other Transform-matching sites (the other uses of child.tag_name().name() and the InclusiveNamespaces handling blocks).
51-53:⚠️ Potential issue | 🟠 MajorBind enveloped target to the same document instead of accepting raw
NodeId.At Line 53 / Line 98, accepting
NodeIdallows accidental cross-document IDs; Line 69 then resolves it againstdocwith no provenance check. This can exclude the wrong subtree silently.Suggested API hardening
-use roxmltree::{Document, Node, NodeId}; +use roxmltree::{Document, Node}; pub fn apply_transform<'a>( doc: &'a Document<'a>, - signature_node_id: NodeId, + signature_node: Node<'a, 'a>, transform: &Transform, input: TransformData<'a>, ) -> Result<TransformData<'a>, TransformError> { match transform { Transform::Enveloped => { let mut nodes = input.into_node_set()?; - 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)) }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<Vec<u8>, TransformError> { @@ - data = apply_transform(doc, signature_node_id, transform, data)?; + data = apply_transform(doc, signature_node, transform, data)?; }In roxmltree 0.21, does Document::get_node(NodeId) verify that the NodeId originated from the same Document?Also applies to: 68-70, 96-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/transforms.rs` around lines 51 - 53, The function apply_transform currently accepts a raw NodeId which can refer to nodes from a different Document; change its API to accept a roxmltree::Node tied to the same Document (e.g., replace the signature apply_transform(doc: &Document<'a>, signature_node_id: NodeId) with apply_transform(doc: &Document<'a>, signature_node: Node<'a>)) and use that Node directly instead of calling doc.get_node(signature_node_id); update any internal uses that resolve NodeId (the places that call get_node/resolve the enveloped target) to use the provided Node so provenance is enforced at the type level, and update all call sites accordingly.
🧹 Nitpick comments (1)
src/xmldsig/transforms.rs (1)
381-386: Strengthen the nested-signatures test with two actual<Signature>elements.The secondary element is
<Sig1>at Line 381, so Line 409 doesn’t validate behavior when two real dsig<Signature>siblings exist. Consider making both elements<Signature>and selecting by position/ID.Also applies to: 402-410
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/transforms.rs` around lines 381 - 386, The test in src/xmldsig/transforms.rs that builds nested signature elements uses a non-dsig element name "<Sig1>" instead of a second "<Signature>", so it doesn’t exercise behavior with two real dsig Signature siblings; update the test XML construction to create two "<Signature xmlns=\"http://www.w3.org/2000/09/xmldsig#\">...<SignedInfo><Reference URI=\"\"/></SignedInfo>...</Signature>" elements (replace the "<Sig1>" node) and then adjust the selection/assertion logic in the test (the nested-signatures test) to pick the correct Signature by position or by assigning an ID attribute and selecting by that ID to validate the intended behavior. Ensure any helper used to parse/transform signatures (e.g., the test function name or helper that locates Signature elements) is updated to expect two true dsig Signature siblings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/xmldsig/transforms.rs`:
- Around line 141-147: The parser is currently accepting any element with local
name "Transform" and silently tolerating an InclusiveNamespaces element missing
a PrefixList; update the checks to fail-closed: when matching Transform elements
(where code uses child.tag_name().name() and child.attribute("Algorithm")),
require the XMLDSIG namespace (e.g. child.tag_name().namespace() ==
Some("http://www.w3.org/2000/09/xmldsig#")) and reject/return a TransformError
for any Transform in a different namespace or missing/empty Algorithm attribute;
likewise, when handling an InclusiveNamespaces element (look for the element
named "InclusiveNamespaces"), require the PrefixList attribute to exist and be
non-empty and return a proper TransformError (e.g.
TransformError::UnsupportedTransform or a MalformedTransform variant) if it is
absent/empty; apply the same namespace-and-attribute strictness to the other
Transform-matching sites (the other uses of child.tag_name().name() and the
InclusiveNamespaces handling blocks).
- Around line 51-53: The function apply_transform currently accepts a raw NodeId
which can refer to nodes from a different Document; change its API to accept a
roxmltree::Node tied to the same Document (e.g., replace the signature
apply_transform(doc: &Document<'a>, signature_node_id: NodeId) with
apply_transform(doc: &Document<'a>, signature_node: Node<'a>)) and use that Node
directly instead of calling doc.get_node(signature_node_id); update any internal
uses that resolve NodeId (the places that call get_node/resolve the enveloped
target) to use the provided Node so provenance is enforced at the type level,
and update all call sites accordingly.
---
Nitpick comments:
In `@src/xmldsig/transforms.rs`:
- Around line 381-386: The test in src/xmldsig/transforms.rs that builds nested
signature elements uses a non-dsig element name "<Sig1>" instead of a second
"<Signature>", so it doesn’t exercise behavior with two real dsig Signature
siblings; update the test XML construction to create two "<Signature
xmlns=\"http://www.w3.org/2000/09/xmldsig#\">...<SignedInfo><Reference
URI=\"\"/></SignedInfo>...</Signature>" elements (replace the "<Sig1>" node) and
then adjust the selection/assertion logic in the test (the nested-signatures
test) to pick the correct Signature by position or by assigning an ID attribute
and selecting by that ID to validate the intended behavior. Ensure any helper
used to parse/transform signatures (e.g., the test function name or helper that
locates Signature elements) is updated to expect two true dsig Signature
siblings.
…s-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
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline to the xmldsig module, covering parsing and execution of enveloped-signature + C14N transform chains and validating behavior via integration tests.
Changes:
- Introduces
xmldsig::transformswithTransform,parse_transforms,apply_transform, andexecute_transforms(including default C14N fallback). - Extends
TransformErrorwith a specificEnvelopedSignatureNotFounderror for invalid signature node references. - Adds comprehensive integration tests exercising URI dereference → transform parsing → transform execution across common SAML/XMLDSig scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/xmldsig/transforms.rs |
Implements transform parsing/execution (enveloped + C14N) plus unit tests. |
src/xmldsig/types.rs |
Adds a dedicated error variant for missing signature node during enveloped transform. |
src/xmldsig/mod.rs |
Exposes the new transforms module and re-exports primary APIs. |
tests/transforms_integration.rs |
Adds end-to-end integration coverage for realistic transform chains and edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/xmldsig/transforms.rs (1)
147-149:⚠️ Potential issue | 🟠 MajorFail closed for transform control-element parsing.
Line 147 currently accepts any
*:Transformby local name, and Line 194 treats<InclusiveNamespaces>withoutPrefixListas if absent. In signature verification code, both cases should reject malformed/foreign control metadata instead of silently proceeding.🔒 Proposed fail-closed patch
@@ const EXCLUSIVE_C14N_NS_URI: &str = "http://www.w3.org/2001/10/xml-exc-c14n#"; +const XMLDSIG_NS_URI: &str = "http://www.w3.org/2000/09/xmldsig#"; @@ - // Match on local name, ignoring namespace prefix (could be ds:Transform or Transform) - if child.tag_name().name() != "Transform" { + if child.tag_name().name() != "Transform" + || child.tag_name().namespace() != Some(XMLDSIG_NS_URI) + { continue; } @@ - 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); } } @@ -fn parse_inclusive_prefixes(transform_node: Node) -> Option<String> { +fn parse_inclusive_prefixes(transform_node: Node) -> Result<Option<String>, 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 <InclusiveNamespaces>".into(), + ) + })?; + return Ok(Some(prefix_list.to_string())); } } } - None + Ok(None) }Also applies to: 161-163, 188-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/transforms.rs` around lines 147 - 149, The transform parsing currently accepts elements by local name only and silently ignores malformed/missing attributes; change it to strictly validate namespace and required attributes: when iterating child.tag_name(), require the element's namespace URI equals the XMLDSIG namespace (not just local name "Transform") and reject (return an error) if a control-element is in a different namespace; additionally, when handling the InclusiveNamespaces element (the code that reads "InclusiveNamespaces" / PrefixList), treat an InclusiveNamespaces without a PrefixList attribute as a parsing error instead of as absent—update the parsing logic in the transform processing function(s) that reference child.tag_name(), the Transform branch, and the InclusiveNamespaces handling so they return an explicit error on namespace mismatch or missing PrefixList.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/xmldsig/transforms.rs`:
- Around line 147-149: The transform parsing currently accepts elements by local
name only and silently ignores malformed/missing attributes; change it to
strictly validate namespace and required attributes: when iterating
child.tag_name(), require the element's namespace URI equals the XMLDSIG
namespace (not just local name "Transform") and reject (return an error) if a
control-element is in a different namespace; additionally, when handling the
InclusiveNamespaces element (the code that reads "InclusiveNamespaces" /
PrefixList), treat an InclusiveNamespaces without a PrefixList attribute as a
parsing error instead of as absent—update the parsing logic in the transform
processing function(s) that reference child.tag_name(), the Transform branch,
and the InclusiveNamespaces handling so they return an explicit error on
namespace mismatch or missing PrefixList.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50eb1ff0-809a-4bcf-a70b-633b512c5f36
📒 Files selected for processing (2)
src/xmldsig/transforms.rstests/transforms_integration.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/transforms_integration.rs
- Convert bracket-only markdown link to full URL for §4.3.3.2 - Correct test description for whitespace text node behavior
…ribute - Validate <Transform> elements belong to XMLDSig namespace (fail-closed) - Reject <InclusiveNamespaces> 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
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline to the xmldsig module, including parsing <Transforms> from XML and executing enveloped-signature + C14N chains end-to-end (with tests validating canonical output).
Changes:
- Introduces
xmldsig::transformswithTransform,parse_transforms,apply_transform, andexecute_transforms. - Extends
TransformErrorwith an enveloped-signature-specific error variant. - Adds integration tests covering common SAML and edge-case transform scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/transforms_integration.rs | New integration tests covering dereference → parse → execute → canonical output assertions. |
| src/xmldsig/types.rs | Adds a new TransformError variant related to enveloped-signature processing. |
| src/xmldsig/transforms.rs | New transform pipeline implementation + unit tests (enveloped, C14N, prefix lists, defaults). |
| src/xmldsig/mod.rs | Exposes the new transforms module and re-exports key transform APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…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 <Transform> with wrong namespace (error, not skip)
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline to the xmldsig module, including parsing <Transforms> elements and executing enveloped-signature + C14N chains end-to-end.
Changes:
- Introduce
xmldsig::transformswithTransform,parse_transforms, andexecute_transforms. - Add a new
TransformError::CrossDocumentSignatureNodefor safer enveloped-signature handling. - Add integration tests covering common XMLDSig/SAML transform chains and edge cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/transforms_integration.rs | New integration tests validating transform parsing + execution across multiple scenarios. |
| src/xmldsig/types.rs | Adds a dedicated error for cross-document signature node misuse in enveloped transforms. |
| src/xmldsig/transforms.rs | Implements transform parsing and sequential execution, including default C14N fallback. |
| src/xmldsig/mod.rs | Exposes the new transforms module and re-exports the public API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- 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
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline to the xmldsig module, including parsing <Transforms> from XML and executing an enveloped-signature + C14N chain end-to-end, with thorough unit/integration test coverage.
Changes:
- Introduces
xmldsig::transformsmodule withTransform,parse_transforms, andexecute_transforms. - Extends
TransformErrorwith a cross-document Signature node validation error for the enveloped transform. - Adds integration tests covering common SAML-style transform chains, nested signatures, prefix lists, and whitespace behavior; also adds Rust review instructions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/transforms_integration.rs | New end-to-end integration tests for URI dereference → transform parsing → pipeline execution → canonical output assertions. |
| src/xmldsig/types.rs | Adds TransformError::CrossDocumentSignatureNode to fail closed on document mismatches in enveloped transform. |
| src/xmldsig/transforms.rs | New transform pipeline implementation + unit tests (enveloped, C14N, parsing, default C14N fallback). |
| src/xmldsig/mod.rs | Exposes transforms module and re-exports key transform APIs from xmldsig. |
| .github/instructions/rust.instructions.md | Adds repository-specific Rust code review guidance for future PRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…rror 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)
- Replace <Sig1> placeholder with <ds:Signature Id="sig-other"> - Both elements now share XMLDSig namespace, differentiated by Id - Matches realistic SAML nested signature scenario
…eq call - Add #[non_exhaustive] to TransformError for semver safety - Remove unnecessary raw pointer casts in cross-document check
There was a problem hiding this comment.
Pull request overview
Introduces an XMLDSig <Reference> transform pipeline implementation (enveloped-signature + C14N variants), including parsing <Transforms> from XML and executing them end-to-end in verification flows.
Changes:
- Added
xmldsig::transformsmodule withTransform,parse_transforms, andexecute_transforms(including default C14N fallback and InclusiveNamespaces PrefixList parsing for exc-c14n). - Extended
TransformErrorwith a cross-document Signature-node error and marked it#[non_exhaustive]. - Added extensive integration tests covering common SAML patterns and edge cases for the transform pipeline.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/xmldsig/transforms.rs |
New transform pipeline: apply/enveloped exclusion, C14N transform, default C14N fallback, and XML parsing of <Transforms> (+ unit tests). |
src/xmldsig/types.rs |
Extends TransformError (new variant) and marks the enum #[non_exhaustive]. |
src/xmldsig/mod.rs |
Exposes the new transforms module and re-exports transform API. |
tests/transforms_integration.rs |
New integration tests exercising dereference → parse → execute across realistic and edge-case inputs. |
.github/instructions/rust.instructions.md |
Adds Rust-focused review instructions for .rs files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…ions - Add doc comment explaining #[non_exhaustive] on TransformError (pre-1.0 crate) - Add .github/instructions/code-review.instructions.md for xml-sec
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline implementation (enveloped-signature + C14N) and end-to-end tests to validate parsing and execution of transform chains across realistic XMLDSig/SAML scenarios.
Changes:
- Introduces
xmldsig::transformswithTransform,parse_transforms, andexecute_transforms(including default C14N fallback). - Extends
TransformErrorwith#[non_exhaustive]and a new cross-document signature-node error. - Adds comprehensive unit + integration coverage for transform parsing/execution and edge cases (nested signatures, PrefixList, whitespace).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/xmldsig/transforms.rs |
New transform pipeline implementation + unit tests (enveloped + C14N, PrefixList parsing). |
src/xmldsig/types.rs |
Makes TransformError non-exhaustive and adds CrossDocumentSignatureNode. |
src/xmldsig/mod.rs |
Exposes the new transforms module and re-exports public APIs. |
tests/transforms_integration.rs |
New integration tests covering real-world transform chains and behavior. |
.github/instructions/rust.instructions.md |
Adds Rust-focused review guidance for this repo. |
.github/instructions/code-review.instructions.md |
Adds repo-specific code review scope/standards guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline implementation and integration tests to validate end-to-end behavior (URI dereference → transform parsing → transform execution → canonicalized bytes), including enveloped-signature handling and C14N variants.
Changes:
- Introduces
xmldsig::transformswithTransform,parse_transforms, andexecute_transforms(enveloped-signature + C14N; default C14N fallback). - Expands
TransformErrorwith#[non_exhaustive]and a cross-document signature-node error variant. - Adds integration tests covering common SAML patterns, nested signatures, PrefixList parsing, and whitespace behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/transforms_integration.rs | New integration tests exercising the full transform pipeline across common XMLDSig/SAML scenarios. |
| src/xmldsig/types.rs | Extends transform error handling (#[non_exhaustive], new cross-document signature-node error). |
| src/xmldsig/transforms.rs | Implements transform parsing/execution (enveloped-signature exclusion, C14N, default C14N fallback) plus unit tests. |
| src/xmldsig/mod.rs | Exposes the new transforms module and re-exports key transform APIs. |
| .github/instructions/rust.instructions.md | Adds Rust-focused review guidance for the repository. |
| .github/instructions/code-review.instructions.md | Adds repo-specific code review scope/standards guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Validate parse_transforms input is <ds:Transforms> in XMLDSig namespace - Reject any child element that is not <ds:Transform> (fail-closed) - Prevents masking parsing bugs via silent empty transform chain
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline implementation (parse + execute) with coverage for common SAML flows (enveloped-signature + C14N), plus accompanying integration/unit tests and repo review guidance.
Changes:
- Introduces
xmldsig::transformswithTransform,parse_transforms, andexecute_transforms(including default C14N fallback). - Extends
TransformError(new variant + marked#[non_exhaustive]). - Adds comprehensive integration tests for transform parsing/execution scenarios (SAML-like, nested signatures, prefix lists, whitespace).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/transforms_integration.rs | New end-to-end integration tests for dereference → parse → execute → canonical output assertions |
| src/xmldsig/types.rs | Updates transform error type (non-exhaustive + cross-document signature node error) |
| src/xmldsig/transforms.rs | Implements transform parsing/execution pipeline + unit tests |
| src/xmldsig/mod.rs | Exposes the new transforms module and re-exports pipeline API |
| .github/instructions/rust.instructions.md | Adds Rust-focused review guidance for the repo |
| .github/instructions/code-review.instructions.md | Adds repo-specific code review scope/standards guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
…oc comment - Switch #[allow(clippy::unwrap_used)] to #[expect] with reason - Simplify TransformError doc comment (remove semver discussion)
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline to the xmldsig module, enabling end-to-end parsing/execution of common transform chains (notably enveloped-signature + C14N) and validating behavior via new integration tests.
Changes:
- Introduce
xmldsig::transformswithTransform,parse_transforms, andexecute_transforms(including Exclusive C14N PrefixList support). - Extend
TransformErrorwith a cross-document signature-node error and mark the enum#[non_exhaustive]. - Add comprehensive integration tests covering enveloped transform behavior, C14N variants, nested signatures, URI dereferencing, and whitespace behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/xmldsig/transforms.rs |
New transform pipeline implementation + parsing of <ds:Transforms> and Exclusive C14N PrefixList + unit tests. |
src/xmldsig/mod.rs |
Exposes the new transforms module and re-exports key APIs. |
src/xmldsig/types.rs |
Updates transform error type to support new failure modes and future extensibility. |
tests/transforms_integration.rs |
New integration tests exercising the full dereference → parse → execute pipeline. |
.github/instructions/rust.instructions.md |
Adds Rust review guidance for this repo. |
.github/instructions/code-review.instructions.md |
Adds additional repo-specific review scope/testing guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Replace ok_or_else with expect() for known-constant URI lookup - Add #[expect(clippy::expect_used)] with reason
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline (enveloped-signature + C14N variants) and verifies it end-to-end with integration tests, aligning the xmldsig module with how real SAML signature verification builds canonical bytes for digesting.
Changes:
- Introduce
xmldsig::transformswithTransform,parse_transforms, andexecute_transforms(including default C14N fallback). - Extend
TransformErrorfor transform-pipeline failures and add a cross-document guard for the enveloped transform. - Add comprehensive integration tests covering common SAML patterns, PrefixList parsing, default C14N fallback, and nested signatures.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/xmldsig/transforms.rs |
Implements transform parsing/execution (enveloped + C14N) and unit tests for key behaviors. |
src/xmldsig/types.rs |
Updates transform error handling (new variant + #[non_exhaustive]). |
src/xmldsig/mod.rs |
Exposes the new transforms module and re-exports transform APIs. |
tests/transforms_integration.rs |
Adds integration tests validating realistic multi-component transform flows. |
.github/instructions/rust.instructions.md |
Adds Rust-focused review guidance (tiers, clippy, testing expectations). |
.github/instructions/code-review.instructions.md |
Adds repo-specific review scope/standards for Rust PRs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Exclude #[non_exhaustive] on pre-1.0 enums from review findings - Exclude #[expect(clippy::expect_used)] on hardcoded constant lookups
There was a problem hiding this comment.
Pull request overview
Adds an XMLDSig <Reference> transform pipeline to the xmldsig module (enveloped-signature + C14N variants) and validates it with unit + integration tests, strengthening end-to-end reference processing prior to digest computation.
Changes:
- Introduce
xmldsig::transformswithTransform,parse_transforms, andexecute_transforms(including default C14N fallback). - Extend
TransformErrorwith#[non_exhaustive]and a cross-document signature-node error. - Add comprehensive integration tests for common SAML/XMLDSig transform scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/transforms_integration.rs | New end-to-end integration tests covering dereference → parse → execute → canonicalized bytes. |
| src/xmldsig/types.rs | Makes TransformError non-exhaustive and adds CrossDocumentSignatureNode. |
| src/xmldsig/transforms.rs | New transform pipeline implementation + unit tests (enveloped + C14N + parsing). |
| src/xmldsig/mod.rs | Exposes the new transforms module and re-exports key APIs. |
| .github/instructions/rust.instructions.md | Adds Rust-specific review guidance for the repo. |
| .github/instructions/code-review.instructions.md | Adds general Rust code review scope/standards guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Summary
Transform::Enveloped) that excludes the<Signature>subtree from the node setexecute_transforms) with default inclusive C14N 1.0 fallbackparse_transforms()to read<Transforms>XML elements includingInclusiveNamespaces PrefixListc14n::canonicalize.github/instructions/for automated code review (rust.instructions.md+code-review.instructions.md)Technical Details
The transform pipeline is a simple
Vec<Transform>iterated front-to-back — replacing xmlsec1's bidirectional push/pop doubly-linked list. Each transform takesTransformData(NodeSet or Binary) and producesTransformData:Enveloped: NodeSet → NodeSet (excludes Signature subtree viaNodeSet::exclude_subtree)C14n: NodeSet → Binary (delegates toc14n::canonicalizewith node set predicate)The
signature_node: Node<'a, 'a>parameter (not rawNodeId) ensures nested signatures work correctly — only the specific<Signature>being verified is excluded. Cross-document misuse is prevented by pointer-equality validation against the NodeSet's owning document.Security hardening:
<Transform>and<Transforms>elements validated against XMLDSig namespace (fail-closed)<InclusiveNamespaces>validated against Exclusive C14N namespacePrefixListattribute on<InclusiveNamespaces>produces an error (not silently ignored)<Transforms>produce an error (not silently skipped)apply_transformispub(crate)— onlyexecute_transformsis the public entry pointKnown Limitations
Test Plan
tests/transforms_integration.rs:#idURI + enveloped (assertion-level signature inside Response)InclusiveNamespaces PrefixListparsed from XMLparse_transforms→execute_transformsfull roundtripCloses #13