feat(xmldsig): complete transform coverage - #103
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds Base64, XPath 1.0, and XPath Filter 2.0 XMLDSig transforms, exact node-set tracking, visibility-aware canonicalization, configurable ChangesXMLDSig transform and canonicalization flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Reference
participant parse_transforms
participant execute_transforms_with_options
participant XPathEngine
participant NodeSet
participant canonicalize_with_visibility
participant VerifyContext
Reference->>parse_transforms: parse Base64/XPath/XPathFilter2
VerifyContext->>execute_transforms_with_options: execute with TransformOptions
execute_transforms_with_options->>XPathEngine: evaluate XPath or Filter 2.0
XPathEngine->>NodeSet: return selected exact nodes
execute_transforms_with_options->>canonicalize_with_visibility: canonicalize visible node set
canonicalize_with_visibility-->>VerifyContext: return pre-digest bytes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| src/xmldsig/xpath.rs | Adds bounded XPath evaluation and exact tree, attribute, and namespace node projection. |
| src/xmldsig/transforms.rs | Adds Base64, XPath, Filter 2.0, octet-stream adaptation, and node-set finalization. |
| src/xmldsig/builder.rs | Aligns template construction and XPath validation with parser and executor limits. |
| src/c14n/serialize.rs | Adds exact node visibility, subset axis processing, document separators, and XML attribute inheritance handling. |
| src/c14n/ns_common.rs | Makes namespace collection aware of exact namespace-node visibility. |
| src/c14n/ns_exclusive.rs | Adds visibility-aware prefix selection and namespace discontinuity tracking. |
| src/c14n/xml_base.rs | Limits C14N 1.1 base fixup to contiguous omitted ancestors. |
| Cargo.toml | Adds safe-Rust XPath dependencies to the XMLDSig feature. |
Reviews (16): Last reviewed commit: "docs(xmldsig): clarify xpath work ceilin..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/xmldsig/types.rs`:
- Around line 252-263: Restore set.with_comments to the requested with_comments
value after insert_subtree and comment filtering in collect_document, so
materializing nodes cannot override the caller’s setting. Preserve the existing
removal of comment Tree keys when comments are disabled and ensure
entire_document_without_comments() reports with_comments() == false.
In `@tests/c14n_golden.rs`:
- Around line 288-372: Update
merlin_xpath_subset_and_signed_info_match_all_28_golden_outputs so the final
signed_info_output comparison does not assert before the accumulated failures
are reported. Add any SignedInfo mismatch to the existing failures list with
consistent diagnostic context, or otherwise defer that assertion until after the
per-reference failures check, ensuring all mismatches are surfaced together.
In `@tests/xpath_transform_integration.rs`:
- Around line 1-12: Add end-to-end signing and verification cases in the
integration tests for plain Transform::XPath and XPathFilterOperation::Union,
alongside the existing XPathFilter2 Intersect/Subtract coverage. Reuse the
established fixtures, builders, and verification flow so both missing transform
cases are exercised without changing existing coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b254107a-d800-4c2d-a4d0-e8f72b84b1da
📒 Files selected for processing (27)
Cargo.tomlREADME.mdsrc/c14n/mod.rssrc/c14n/ns_common.rssrc/c14n/ns_exclusive.rssrc/c14n/ns_inclusive.rssrc/c14n/serialize.rssrc/c14n/xml_base.rssrc/xmldsig/builder.rssrc/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/transforms.rssrc/xmldsig/types.rssrc/xmldsig/verify.rssrc/xmldsig/xpath.rstests/base64_transform_integration.rstests/c14n_golden.rstests/donor_negative_vectors.rstests/fixtures/xmldsig/README.mdtests/fixtures/xmldsig/merlin-xpath-filter2/sign-spec-subset.xmltests/fixtures/xmldsig/phaos-xmldsig-three/certs/rsa-ca-cert.dertests/fixtures/xmldsig/phaos-xmldsig-three/certs/rsa-cert.dertests/fixtures_smoke.rstests/signature_builder.rstests/xmlsec1_interop.rstests/xpath_donor_vectors.rstests/xpath_transform_integration.rs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 335a8cc3b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Prove that selected attributes and namespaces emit no bytes when their owner element is absent from the canonical subset.
This reverts commit cac91b4.
- Default here() to the XMLDSig XPath parameter element - Add explicit libxmlsec Transform-node compatibility to sign and verify - Preserve comment exclusion metadata while materializing node sets
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1315dc17b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/transforms.rs (1)
326-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared XML-base64 normalization helper.
append_normalized_base64duplicates the same space/tab/CR/LF-only normalization policy fromsrc/xmldsig/whitespace.rs. Extract a shared byte-oriented helper usable fromtransforms.rsandverify.rs, and call it from the Base64 transform path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/xmldsig/transforms.rs` around lines 326 - 349, Move the shared space/tab/CR/LF Base64 normalization logic from append_normalized_base64 into a reusable byte-oriented helper in xmldsig::whitespace, preserving invalid-byte rejection and error behavior. Update the Base64 transform path and verify.rs callers to use that helper, then remove the duplicate local implementation from transforms.rs.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/xmldsig/transforms.rs`:
- Around line 326-349: Move the shared space/tab/CR/LF Base64 normalization
logic from append_normalized_base64 into a reusable byte-oriented helper in
xmldsig::whitespace, preserving invalid-byte rejection and error behavior.
Update the Base64 transform path and verify.rs callers to use that helper, then
remove the duplicate local implementation from transforms.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f6d7c4cf-ed65-46bf-a728-34b758bc8e13
📒 Files selected for processing (10)
src/c14n/serialize.rssrc/xmldsig/mod.rssrc/xmldsig/sign.rssrc/xmldsig/transforms.rssrc/xmldsig/types.rssrc/xmldsig/verify.rssrc/xmldsig/xpath.rstests/base64_transform_integration.rstests/c14n_golden.rstests/xpath_transform_integration.rs
Reproduce duplicate xml:base materialization when an omitted parent separates two included elements.
Stop C14N 1.1 xml:base materialization at the nearest included ancestor while preserving it as the URI resolution seed.
Reproduce malformed XPath elements when an expression namespace binding reuses the signature prefix for another URI.
Fail template validation when an XPath binding would move prefixed XMLDSig parameter elements into another namespace.
Reproduce the compatibility normalizer collapsing XPath word operators into function-like tokens.
Exclude XPath word operators from QName-to-function-call whitespace normalization.
Reproduce unbounded transform chains through both XML parsing and the public programmatic executor.
Reject references with more than 64 transforms at both XML parsing and public executor boundaries.
Route transform and signature verification byte streams through one XML-whitespace helper while retaining caller-specific alphabet validation.
|
@coderabbitai Fixed in deac05f: transform and SignatureValue verification now share a byte-oriented XML base64 normalizer with caller-specific alphabet validation; existing invalid-byte offsets and error mapping are preserved, with focused unit and integration coverage. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/xmldsig/types.rs (1)
96-124: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake the public
NodeSetconstructors budget-checked.
xmldsigre-exportsNodeSet, but the fully public constructors remain unbounded while the budget checks only cover thepub(crate)try_*variants. External callers can bypass the quadratic materialization guard throughentire_document_without_comments,entire_document_with_comments, orsubtree. Move those constructors topub(crate)withtry_*behavior, or embed the budget check in the public constructors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/xmldsig/types.rs` around lines 96 - 124, Make the public NodeSet constructors entire_document_without_comments, entire_document_with_comments, and subtree enforce the same materialization budget as the existing try_* variants. Either change them to crate-private try_* constructors that return the appropriate failure result, or add the budget checks directly while preserving their public API; ensure external callers cannot bypass the quadratic guard.src/xmldsig/xpath.rs (1)
441-470: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBound total XPath evaluation work.
MAX_XPATH_PER_NODE_EVALUATIONSonly limits how many independent filter evaluations run; eachxpath.evaluate(&context, node.clone())can still traverse the mirrored document, andsxd-xpath-no-unsafeoffers no step limit/interrupt API. In filter mode, add a combined budget such asnodes_evaluated_so_far * document_size, or run the ordinary XPath transform under a wall-clock/step ceiling, rather than only checking the outer loop count.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/xmldsig/xpath.rs` around lines 441 - 470, Bound total XPath work in the wrap_as_filter branch, not just the number of per-node evaluations. Use the mirrored document size and a cumulative budget such as evaluated_nodes multiplied by document_size, rejecting the transform with TransformError::XPath once the budget is exceeded; apply the check before each xpath.evaluate call while preserving the existing selected-node behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/xmldsig/types.rs`:
- Around line 96-124: Make the public NodeSet constructors
entire_document_without_comments, entire_document_with_comments, and subtree
enforce the same materialization budget as the existing try_* variants. Either
change them to crate-private try_* constructors that return the appropriate
failure result, or add the budget checks directly while preserving their public
API; ensure external callers cannot bypass the quadratic guard.
In `@src/xmldsig/xpath.rs`:
- Around line 441-470: Bound total XPath work in the wrap_as_filter branch, not
just the number of per-node evaluations. Use the mirrored document size and a
cumulative budget such as evaluated_nodes multiplied by document_size, rejecting
the transform with TransformError::XPath once the budget is exceeded; apply the
check before each xpath.evaluate call while preserving the existing
selected-node behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2bbf016d-9748-42c1-8af7-1b555d1c8109
📒 Files selected for processing (8)
src/xmldsig/builder.rssrc/xmldsig/transforms.rssrc/xmldsig/types.rssrc/xmldsig/uri.rssrc/xmldsig/xpath.rstests/base64_transform_integration.rstests/signature_builder.rstests/uri_integration.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebaef0678b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 066682f47f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baa19f6071
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21590a5f04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Share XPath work across transforms and references - Bound source mirrors, id scans, namespaces, and references - Clarify the standards-required cross-document here() error
Validate each SignedInfo child before applying the aggregate Reference cardinality bound.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/xpath.rs (1)
617-657: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRaise the whole-document XPath work ceiling or avoid per-node document-size charges.
The filter branch charges
document_sizeonce for the all-nodes scan and again before every input-node evaluation, soNwhole-document inputs cost2·N·D. WithMAX_XPATH_PER_NODE_EVALUATIONS = 4_096, a 737-node entire-document input can exhaustMAX_XPATH_CUMULATIVE_EVALUATION_WORK = 6_000_000beforeid()/here()even run, while a 2,500-node input fails on the trivialtrue()expression. For SAML/XAdES documents, either raise this constant to allow the full per-node cap on reasonable document sizes or scale the charge by expression shape/document relation so ordinaryURI=""transforms are not rejected under the 1,024-entry default per-transform limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/xmldsig/xpath.rs` around lines 617 - 657, Adjust the work-budget accounting in the wrap_as_filter branch so per-node XPath evaluation does not exhaust the cumulative limit merely because each input charges the full document size. Update the charges around all_nodes_xpath evaluation and the xpath.evaluate loop, or raise the relevant MAX_XPATH_CUMULATIVE_EVALUATION_WORK limit, while preserving protection against genuinely expensive expressions and allowing ordinary whole-document URI="" transforms up to MAX_XPATH_PER_NODE_EVALUATIONS.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/xmldsig/xpath.rs`:
- Around line 617-657: Adjust the work-budget accounting in the wrap_as_filter
branch so per-node XPath evaluation does not exhaust the cumulative limit merely
because each input charges the full document size. Update the charges around
all_nodes_xpath evaluation and the xpath.evaluate loop, or raise the relevant
MAX_XPATH_CUMULATIVE_EVALUATION_WORK limit, while preserving protection against
genuinely expensive expressions and allowing ordinary whole-document URI=""
transforms up to MAX_XPATH_PER_NODE_EVALUATIONS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7209b988-bcef-4bdf-b39d-e6866b834f13
📒 Files selected for processing (14)
src/xmldsig/builder.rssrc/xmldsig/parse.rssrc/xmldsig/sign.rssrc/xmldsig/transforms.rssrc/xmldsig/types.rssrc/xmldsig/uri.rssrc/xmldsig/verify.rssrc/xmldsig/xpath.rstests/base64_transform_integration.rstests/signature_builder.rstests/transforms_integration.rstests/uri_integration.rstests/xpath_donor_vectors.rstests/xpath_transform_integration.rs
Document why the context-count ceiling and signature-wide worst-case work budget are independent limits.
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 814d30e806
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // A user expression may scan the entire document on every call. | ||
| // Charge that worst-case cost before execution so a failure never | ||
| // occurs after performing work beyond the aggregate budget. | ||
| work_budget.charge(document_size)?; |
There was a problem hiding this comment.
When verifying untrusted XML with an ordinary XPath transform, this charges only one document traversal per context-node evaluation, but a short accepted expression such as //*[count(//*) > 0] can scan the document once for every element during each evaluation. Since the outer loop evaluates the expression for every input node, a roughly 1,000-node document can remain below the 6,000,000-unit meter while causing approximately cubic XPath work, enabling a CPU denial of service. The budget must account for intra-expression traversal multiplicity or use an evaluator with an interruptible step limit.
Useful? React with 👍 / 👎.
| self.nodes.insert(XmlNodeKey::Namespace { | ||
| owner: owner.id(), | ||
| prefix: prefix.to_owned(), | ||
| uri: uri.to_owned(), |
There was a problem hiding this comment.
When untrusted XML declares a long namespace URI on an ancestor with many descendants, every descendant's in-scope namespace entry clones that entire URI here. The 65,536-entry limit counts nodes but not string bytes, so a document containing one moderately large URI and thousands of small elements can amplify into hundreds of megabytes or more during ordinary URI dereferencing, causing an out-of-memory denial of service before transform execution. Charge the cloned prefix/URI bytes against a materialization budget or store shared references instead.
Useful? React with 👍 / 👎.
| self.nodes.retain(|key| { | ||
| document.get_node(key.owner_id()).is_none_or(|candidate| { | ||
| candidate != node && !candidate.ancestors().any(|ancestor| ancestor == node) | ||
| }) |
There was a problem hiding this comment.
For an enveloped signature nested near the bottom of a deeply nested untrusted document, most retained entries are ancestors or unrelated nodes, and this predicate walks each candidate's complete ancestor chain looking for the signature node. With tens of thousands of entries still permitted by the materialization cap, exclude_subtree therefore becomes quadratic and can consume hundreds of millions of ancestor steps before digesting. Remove entries using document-order subtree bounds or precompute the excluded owner IDs in one traversal.
Useful? React with 👍 / 👎.
1 participant
Summary
<XPath>semantics forhere()plus explicit libxmlsec1<Transform>compatibility in signing and verificationid()values, invalid reserved XML namespace bindings, and namespaced ID mismatches while preserving the implicit XPathxmlcontext and C14N 1.1xml:idnon-inheritancexml:basefixup, document-level separators, and all 28 Merlin canonical outputs byte-for-byteValidation
cargo check --all-targets --all-featurescargo check --no-default-features --features c14ncargo check --no-default-features --features xmldsigcargo clippy --all-targets --all-features -- -D warningscargo nextest run --all-features --no-fail-fast(714 passed)cargo test --doc --all-features(4 passed)cargo fmt --all -- --checkcargo package --allow-dirtycargo update --dry-run(aes0.9.2 andcc1.4.0 available; library lockfile is not tracked)Closes #100
Closes #101
Closes #102