Skip to content

feat(xmldsig): complete transform coverage - #103

Open
polaz wants to merge 82 commits into
mainfrom
test/#100-phaos-negative-vectors
Open

feat(xmldsig): complete transform coverage#103
polaz wants to merge 82 commits into
mainfrom
test/#100-phaos-negative-vectors

Conversation

@polaz

@polaz polaz commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • classify the imported Phaos negative XMLDSig artifacts at their actual digest, structure, weak-key, and certificate-validity boundaries
  • implement the XMLDSig Base64 transform for binary and exact node-set text input across parsing, building, signing, verification, and policy enforcement
  • implement safe-Rust XPath 1.0 and XPath Filter 2.0 with exact tree, attribute, and namespace nodes, input-scoped contexts, bounded expression complexity, namespace storage, and cumulative evaluation work, standard functions, and ordered subtree set operations
  • implement standards-default <XPath> semantics for here() plus explicit libxmlsec1 <Transform> compatibility in signing and verification
  • fail closed on ambiguous XPath id() values, invalid reserved XML namespace bindings, and namespaced ID mismatches while preserving the implicit XPath xml context and C14N 1.1 xml:id non-inheritance
  • bound exact node-set materialization through every public and internal constructor, preflight every string copied into temporary SXD mirrors, and exclude subtrees without materializing attacker-controlled removal sets
  • adapt UTF-8 and BOM-marked UTF-16 XML octet streams to comment-preserving node sets for subsequent XPath, Filter 2.0, and canonicalization transforms
  • make template builders reject transform chains and XPath parameters that parsing or execution cannot accept
  • extend subset canonicalization to preserve exact node visibility, contiguous omitted-ancestor xml:base fixup, document-level separators, and all 28 Merlin canonical outputs byte-for-byte
  • add end-to-end signing, negative-path, tampering, donor-vector, and reciprocal xmlsec1 1.3.12 interoperability coverage

Validation

  • cargo check --all-targets --all-features
  • cargo check --no-default-features --features c14n
  • cargo check --no-default-features --features xmldsig
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo nextest run --all-features --no-fail-fast (all passed)
  • cargo test --doc --all-features (all passed)
  • cargo fmt --all -- --check
  • cargo package --allow-dirty
  • cargo update --dry-run (aes 0.9.2 and cc 1.4.0 available; library lockfile is not tracked)

Closes #100
Closes #101
Closes #102

- classify Phaos negative signature and certificate vectors
- implement Base64 reference transforms end to end
- add exact XPath 1.0 and XPath Filter 2.0 node sets
- validate Merlin canonical subsets and reciprocal xmlsec1 interop

Closes #100
Closes #101
Closes #102
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@polaz, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 50 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 295c8744-e1da-44b8-a7a0-fd789bb3b133

📥 Commits

Reviewing files that changed from the base of the PR and between 610e646 and 903141d.

📒 Files selected for processing (1)
  • src/xmldsig/xpath.rs
📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added Base64, XPath 1.0, and XPath Filter 2.0 transform support for XML signatures.
    • Added configurable XPath here() behavior for signing and verification.
    • Expanded XMLDSig interoperability and canonicalization support.
  • Bug Fixes

    • Improved XML subset canonicalization, namespace handling, and inherited attributes.
    • Corrected xml:id behavior in Canonical XML 1.1 subsets.
  • Security

    • Added limits and stricter validation for references, transforms, XPath expressions, and node-set processing.
    • Improved rejection of malformed or unsafe signature inputs.

Walkthrough

The change adds Base64, XPath 1.0, and XPath Filter 2.0 XMLDSig transforms, exact node-set tracking, visibility-aware canonicalization, configurable here() semantics, and expanded validation and interoperability coverage.

Changes

XMLDSig transform and canonicalization flow

Layer / File(s) Summary
Visibility-aware canonicalization
src/c14n/*
Canonicalization now tracks element, attribute, and namespace visibility, renders orphan axis nodes, handles namespace discontinuities, and updates C14N 1.1 xml:* inheritance and xml:base behavior.
Exact node sets and XPath evaluation
src/xmldsig/types.rs, src/xmldsig/xpath.rs
NodeSet stores exact projected nodes. XPath evaluation supports here(), id(), lang(), namespace bindings, and ordered Filter 2.0 operations.
Transform parsing and execution
src/xmldsig/transforms.rs, src/xmldsig/whitespace.rs
Base64, XPath, and XPath Filter 2.0 transforms use strict parsing, bounded execution, binary-to-node-set adaptation, and visibility-aware canonicalization.
Public API and signature construction
src/xmldsig/mod.rs, src/xmldsig/parse.rs, src/xmldsig/builder.rs, Cargo.toml
New transform types and URIs are exported. Reference parsing is stricter. Safe XPath dependencies are enabled. Builder validation checks XPath expressions and namespace bindings.
Signing and verification options
src/xmldsig/sign.rs, src/xmldsig/verify.rs
Signing and verification carry TransformOptions, expose XPath here() semantics, update transform allowlisting, and normalize signature Base64 input as bytes.
Validation and fixtures
tests/*, README.md
Tests cover transform behavior, signing, verification, tampering, C14N golden vectors, xmlsec1 interoperability, Phaos rejection boundaries, fixture documentation, and corpus completeness.

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
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: completing XMLDSig transform coverage.
Description check ✅ Passed The description directly explains the XMLDSig transform, security, interoperability, testing, and validation changes.
Linked Issues check ✅ Passed The changes address the coding objectives and acceptance criteria for Phaos vectors, Base64 transforms, and XPath transforms [#100, #101, #102].
Out of Scope Changes check ✅ Passed The changes remain within the linked issue scope, including supporting canonicalization, fixtures, documentation, and interoperability tests.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch test/#100-phaos-negative-vectors

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR completes XMLDSig transform support and expands canonicalization behavior. The main changes are:

  • Base64, XPath 1.0, and XPath Filter 2.0 transforms.
  • Exact element, attribute, and namespace node-set handling.
  • Bounded XPath evaluation and node-set materialization.
  • Stricter parsing, template building, and namespace validation.
  • Improved C14N subset, XML base, and document separator handling.
  • Expanded negative-vector and xmlsec1 interoperability tests.

Confidence Score: 5/5

This looks safe to merge.

No blocking issues found in the changed code.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the focused test command from the repository; it completed with exit code 0 and reported no failed, ignored, measured, or filtered tests.
  • The xmlsec1 target ran and completed successfully; CLI-dependent paths printed the explicit skip condition.
  • A separate availability check confirmed xmlsec1 is not installed (exit code 127), blocking only external reciprocal CLI execution.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

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

Comment thread src/c14n/serialize.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 93ff852 and 335a8cc.

📒 Files selected for processing (27)
  • Cargo.toml
  • README.md
  • src/c14n/mod.rs
  • src/c14n/ns_common.rs
  • src/c14n/ns_exclusive.rs
  • src/c14n/ns_inclusive.rs
  • src/c14n/serialize.rs
  • src/c14n/xml_base.rs
  • src/xmldsig/builder.rs
  • src/xmldsig/mod.rs
  • src/xmldsig/parse.rs
  • src/xmldsig/transforms.rs
  • src/xmldsig/types.rs
  • src/xmldsig/verify.rs
  • src/xmldsig/xpath.rs
  • tests/base64_transform_integration.rs
  • tests/c14n_golden.rs
  • tests/donor_negative_vectors.rs
  • tests/fixtures/xmldsig/README.md
  • tests/fixtures/xmldsig/merlin-xpath-filter2/sign-spec-subset.xml
  • tests/fixtures/xmldsig/phaos-xmldsig-three/certs/rsa-ca-cert.der
  • tests/fixtures/xmldsig/phaos-xmldsig-three/certs/rsa-cert.der
  • tests/fixtures_smoke.rs
  • tests/signature_builder.rs
  • tests/xmlsec1_interop.rs
  • tests/xpath_donor_vectors.rs
  • tests/xpath_transform_integration.rs

Comment thread src/xmldsig/types.rs
Comment thread tests/c14n_golden.rs
Comment thread tests/xpath_transform_integration.rs

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/xmldsig/xpath.rs Outdated
Comment thread src/xmldsig/transforms.rs Outdated
Comment thread src/xmldsig/xpath.rs Outdated
Comment thread src/xmldsig/transforms.rs
Comment thread src/c14n/serialize.rs
polaz added 9 commits July 23, 2026 09:57
Prove that selected attributes and namespaces emit no bytes when their owner element is absent from the canonical subset.
- 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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/c14n/serialize.rs
Comment thread src/xmldsig/builder.rs
Comment thread src/xmldsig/xpath.rs Outdated
Comment thread src/xmldsig/transforms.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Reuse the shared XML-base64 normalization helper.

append_normalized_base64 duplicates the same space/tab/CR/LF-only normalization policy from src/xmldsig/whitespace.rs. Extract a shared byte-oriented helper usable from transforms.rs and verify.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

📥 Commits

Reviewing files that changed from the base of the PR and between 335a8cc and b1315dc.

📒 Files selected for processing (10)
  • src/c14n/serialize.rs
  • src/xmldsig/mod.rs
  • src/xmldsig/sign.rs
  • src/xmldsig/transforms.rs
  • src/xmldsig/types.rs
  • src/xmldsig/verify.rs
  • src/xmldsig/xpath.rs
  • tests/base64_transform_integration.rs
  • tests/c14n_golden.rs
  • tests/xpath_transform_integration.rs

polaz added 10 commits July 23, 2026 12:17
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.
@polaz

polaz commented Jul 23, 2026

Copy link
Copy Markdown
Member Author

@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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/xmldsig/transforms.rs
Comment thread src/xmldsig/types.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/xmldsig/xpath.rs
Comment thread src/xmldsig/xpath.rs
Comment thread src/xmldsig/xpath.rs Outdated
Comment thread src/xmldsig/xpath.rs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/xmldsig/builder.rs
polaz added 2 commits July 28, 2026 11:20
- 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Raise the whole-document XPath work ceiling or avoid per-node document-size charges.

The filter branch charges document_size once for the all-nodes scan and again before every input-node evaluation, so N whole-document inputs cost 2·N·D. With MAX_XPATH_PER_NODE_EVALUATIONS = 4_096, a 737-node entire-document input can exhaust MAX_XPATH_CUMULATIVE_EVALUATION_WORK = 6_000_000 before id()/here() even run, while a 2,500-node input fails on the trivial true() 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 ordinary URI="" 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

📥 Commits

Reviewing files that changed from the base of the PR and between 74e0b2b and 0a0921e.

📒 Files selected for processing (14)
  • src/xmldsig/builder.rs
  • src/xmldsig/parse.rs
  • src/xmldsig/sign.rs
  • src/xmldsig/transforms.rs
  • src/xmldsig/types.rs
  • src/xmldsig/uri.rs
  • src/xmldsig/verify.rs
  • src/xmldsig/xpath.rs
  • tests/base64_transform_integration.rs
  • tests/signature_builder.rs
  • tests/transforms_integration.rs
  • tests/uri_integration.rs
  • tests/xpath_donor_vectors.rs
  • tests/xpath_transform_integration.rs

Document why the context-count ceiling and signature-wide worst-case work budget are independent limits.
@polaz

polaz commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

@codex review

@polaz

polaz commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/xmldsig/xpath.rs Outdated
Comment thread src/xmldsig/types.rs
Comment thread src/xmldsig/types.rs Outdated
- Account for nested XPath traversal multiplicity before evaluation
- Bound owned node-set string materialization before cloning
- Exclude subtrees using contiguous roxmltree node ID ranges
@polaz

polaz commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@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 //*[count(//*) > 0]; the final all-features suite passes 718/718.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

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 || true

Length of output: 73301


@polaz Confirmed. Commit e2387e73f8183efeece495f5744dafc7c3270c5b is reachable and contains the stated fix.

xpath_evaluation_work precharges document-scanning paths and axes inside predicates. evaluate_expression applies that charge before SXD evaluation. The regression for //*[count(//*) > 0] verifies that the shared work budget rejects the nested-scan case.

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
Learnt from: polaz
URL: https://github.com/structured-world/xml-sec/pull/103

Timestamp: 2026-07-31T08:49:19.564Z
Learning: In `src/xmldsig/xpath.rs`, `MAX_XPATH_CONTEXT_EVALUATIONS` is an intentional CPU bound for the non-interruptible SXD XPath evaluator. Do not recommend raising this ceiling solely to admit all 4,096-node full-document per-node evaluations. Nested document-scanning paths and axes inside XPath predicates must instead be precharged by expression shape through `xpath_evaluation_work` before SXD evaluation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Add a regression test for trailing text/comment nodes in subtree exclusion.

exclude_subtree relies on subtree_node_id_range(node.descendants().next_back()) producing a single inclusive NodeId range. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 814d30e and e2387e7.

📒 Files selected for processing (2)
  • src/xmldsig/types.rs
  • src/xmldsig/xpath.rs

Verify trailing text and comment nodes are removed from canonical digest input while surrounding content remains visible.
@polaz

polaz commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai Fixed in 9cac88b. excluding_subtree_removes_trailing_text_and_comments_from_canonical_output now exercises the public exclusion path with trailing text and comment descendants, asserts both nodes are removed, and verifies exact canonical digest input while preserving surrounding text/elements. The final all-features suite passes 719/719.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

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 || true

Length of output: 121280


@polaz Confirmed. Commit 9cac88b5468751b2c3a1a0411256da47f911f0cd is reachable.

excluding_subtree_removes_trailing_text_and_comments_from_canonical_output calls the public NodeSet::exclude_subtree path. It verifies removal of both trailing text and comment nodes. It also verifies the exact canonical output after canonicalize_with_visibility.

The expected bytes retain the content before and after the excluded subtree. They exclude the subtree and its trailing descendants. Transform::Enveloped uses this same NodeSet::exclude_subtree path.

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread src/xmldsig/xpath.rs
Preflight every string copied into the temporary SXD document before allocating the mirror. Cover oversized text and attribute values plus the complete projection contract.
@polaz

polaz commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e2387e7 and 610e646.

📒 Files selected for processing (2)
  • src/xmldsig/types.rs
  • src/xmldsig/xpath.rs

Comment thread src/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.

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@polaz

polaz commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(xmldsig): support XPath transforms feat(xmldsig): support Base64 reference transforms test(xmldsig): classify Phaos negative vectors

1 participant