feat(xmldsig): complete transform coverage - #103
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 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: project selected nodes
execute_transforms_with_options->>canonicalize_with_visibility: canonicalize visible nodes
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/c14n/serialize.rs | Adds exact node visibility, orphan axis processing, document separators, and revised XML attribute inheritance. |
| src/c14n/ns_common.rs | Tracks visible namespace bindings separately from source namespace scope. |
| src/c14n/ns_exclusive.rs | Adds namespace visibility and binding reconciliation for exclusive canonicalization. |
| src/c14n/xml_base.rs | Limits C14N 1.1 XML base fixup to contiguous omitted ancestors. |
| src/xmldsig/xpath.rs | Implements bounded XPath evaluation, XMLDSig functions, namespace handling, and exact node projection. |
| src/xmldsig/transforms.rs | Adds Base64 and XPath transform execution across binary and node-set inputs. |
| src/xmldsig/types.rs | Adds exact axis-node membership, materialization limits, and subtree exclusion. |
| src/xmldsig/builder.rs | Aligns template validation and transform limits with parsing and execution. |
| Cargo.toml | Adds safe-Rust SXD dependencies to the XMLDSig feature. |
Reviews (19): Last reviewed commit: "fix(xmldsig): bound xpath mirror storage" | Re-trigger Greptile
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
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.
💡 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".
- Account for nested XPath traversal multiplicity before evaluation - Bound owned node-set string materialization before cloning - Exclude subtrees using contiguous roxmltree node ID ranges
|
@coderabbitai Regarding the outside-diff XPath ceiling item: declined. The stated 737-node exhaustion arithmetic does not match the implementation, and raising the ceiling to admit every 4,096-node full-document per-node evaluation would weaken the intentional CPU bound of a non-interruptible evaluator. Commit e2387e7 fixes the actual accounting blind spot instead: nested path/axis traversals inside predicates are precharged by expression shape before SXD execution, with regression coverage for |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="structured-world/xml-sec"
sha="e2387e7"
printf '%s\n' '=== Commit availability in checkout ==='
git show --format=fuller --stat --no-renames "$sha" || true
printf '%s\n' '=== Commit metadata and changed files from GitHub ==='
gh api "repos/${repo}/commits/${sha}" \
--jq '{sha: .sha, message: .commit.message, files: [.files[] | {filename, status, additions, deletions, patch}]}' \
|| true
printf '%s\n' '=== XPath module outline ==='
ast-grep outline src/xmldsig/xpath.rs --items all --type function,struct,impl,enum 2>/dev/null || true
printf '%s\n' '=== Budget accounting and stated regression ==='
rg -n -C 12 --type rust \
'XPathWorkBudget|precharg|charge\(|token.*complex|count\(//\*\)|nested|predicate|axis|work.*limit|signature-wide' \
src/xmldsig/xpath.rs tests || trueLength of output: 73301
The 4,096 context-node ceiling remains an intentional CPU bound. The original outside-diff finding does not apply. I withdraw it. Verification inspected the committed implementation and focused test source. This environment did not execute the test suite. 🐇 ✏️ Learnings added
You are interacting with an AI system. |
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/types.rs (1)
180-192: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a regression test for trailing text/comment nodes in subtree exclusion.
exclude_subtreerelies onsubtree_node_id_range(node.descendants().next_back())producing a single inclusiveNodeIdrange. This works when the excluded subtree is a contiguous document-order block, but the current coverage does not exercise trailing text or comment nodes as the excluded subtree’s last descendant. Add that case to avoid digest input including excluded content in pretty-printed signatures.🤖 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 180 - 192, Add a regression test covering exclude_subtree when the excluded subtree’s final descendant is trailing text or a comment, verifying excluded nodes do not contribute to the pretty-printed signature digest input. Exercise the public exclude_subtree path and assert the resulting digest input omits the entire excluded subtree while preserving surrounding content.
🤖 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 180-192: Add a regression test covering exclude_subtree when the
excluded subtree’s final descendant is trailing text or a comment, verifying
excluded nodes do not contribute to the pretty-printed signature digest input.
Exercise the public exclude_subtree path and assert the resulting digest input
omits the entire excluded subtree while preserving surrounding content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5a62ee71-90c6-4515-8d89-7e2a09028a71
📒 Files selected for processing (2)
src/xmldsig/types.rssrc/xmldsig/xpath.rs
Verify trailing text and comment nodes are removed from canonical digest input while surrounding content remains visible.
|
@coderabbitai Fixed in 9cac88b. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="structured-world/xml-sec"
sha="9cac88b"
printf '%s\n' '=== Commit availability in checkout ==='
git show --format=fuller --stat --no-renames "$sha" || true
printf '%s\n' '=== Commit metadata and changed files from GitHub ==='
gh api "repos/${repo}/commits/${sha}" \
--jq '{sha: .sha, message: .commit.message, files: [.files[] | {filename, status, additions, deletions, patch}]}' \
|| true
printf '%s\n' '=== Candidate test and implementation files ==='
fd -t f -e rs . src tests | sort | rg '(xpath|transform|c14n|types|integration)\.rs$' || true
printf '%s\n' '=== Focused test and public exclusion-path call sites ==='
rg -n -C 14 --type rust \
'excluding_subtree_removes_trailing_text_and_comments_from_canonical_output|exclude_subtree|excluding_subtree|exclude.*subtree|canonical.*digest|pre_digest' \
src tests || true
printf '%s\n' '=== NodeSet structure ==='
ast-grep outline src/xmldsig/types.rs --items all --type struct,impl,function --match 'NodeSet|exclude|subtree' 2>/dev/null || trueLength of output: 121280
The expected bytes retain the content before and after the excluded subtree. They exclude the subtree and its trailing descendants. This addresses the finding. I did not run the test suite in this environment. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cac88b546
ℹ️ 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".
Preflight every string copied into the temporary SXD document before allocating the mirror. Cover oversized text and attribute values plus the complete projection contract.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/xpath.rs`:
- Around line 257-316: Add a property-style test alongside
xpath_mirror_projection_counts_every_copied_string that builds a Mirror from a
fixture, calculates the total bytes of all strings actually copied into the
resulting SXD document, and asserts this equals projected_string_bytes for the
same source. Keep the test tied to Mirror::build behavior so future SXD
string-copy additions cause a failure unless the projection is updated.
🪄 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: ad73183e-2948-4d6f-b3e4-9e53ee1a3a2c
📒 Files selected for processing (2)
src/xmldsig/types.rssrc/xmldsig/xpath.rs
Measure the strings present in a built SXD mirror and compare them with the preflight projection so future builder changes cannot silently bypass the storage bound.
There was a problem hiding this comment.
Greptile has paused reviews on this repository — it used its 100 free open-source review credits for this billing period. Reviews resume automatically on August 15. To continue before then, an organization admin can keep reviews running past the free credits — those bill as normal usage.
|
@coderabbitai review |
✅ Action performedReview finished.
|
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(all passed)cargo test --doc --all-features(all 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