feat(xmldsig): add signature template builders#74
Conversation
- Add quick-xml 0.38 for deterministic XML output - Raise the time dev-dependency floor to 0.3.53 - Resolve num-bigint 0.4.7 through the updated dependency graph
- Add deterministic SignatureBuilder and ReferenceBuilder APIs - Validate signing algorithms, namespace prefixes, and required references - Cover XML structure, escaping, transform order, and error paths Closes #73
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds a new ChangesXMLDSig signature builder
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 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 |
Describe the current RustCrypto stack and quick-xml writer instead of the removed ring dependency.
|
| Filename | Overview |
|---|---|
| src/xmldsig/builder.rs | Adds the XMLDSig template builder implementation and validation logic. |
| src/xmldsig/mod.rs | Exports the new builder module and public builder types. |
| tests/signature_builder.rs | Adds tests for generated template structure, transform round-tripping, Unicode prefixes, and invalid IDs. |
| Cargo.toml | Adds quick-xml and updates the time dev-dependency. |
| README.md | Updates the project description to include the XML writer and crypto stack. |
Reviews (3): Last reviewed commit: "style(xmldsig): format NCName regression..." | 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: 2
🤖 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/builder.rs`:
- Line 16: The XPath transform in the XML signature builder uses the ds prefix
without guaranteeing it is bound in scope. Update the XPath emission in the
builder around XPATH_EXCLUDE_ALL_SIGNATURES so the generated <XPath> element
includes a local xmlns:ds binding, or uses the same configured prefix emitted by
ns_prefix("ds"), so not(ancestor-or-self::ds:Signature) always resolves
correctly.
In `@tests/signature_builder.rs`:
- Around line 16-29: Reject non-NCName values for both
SignatureBuilder::signature_id and ReferenceBuilder::id before build_template()
emits XML. Add the same NCName validation already used for ns_prefix into
SignatureBuilder::validate() (or the build path it calls) and ensure the check
covers Signature/@Id and each Reference/@Id so values like sig&1 are rejected
early with a clear error.
🪄 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: 9159d5d0-6ccc-4c1b-ac0a-3a6b651d496e
📒 Files selected for processing (5)
Cargo.tomlREADME.mdsrc/xmldsig/builder.rssrc/xmldsig/mod.rstests/signature_builder.rs
- Reproduce XPath transform parser incompatibility - Cover Unicode NCName prefixes - Require NCName validation for Signature and Reference IDs
- Bind the parser-compatible dsig prefix for XPath transforms - Accept Unicode XML namespace prefixes - Reject schema-invalid Signature and Reference IDs
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/builder.rs (1)
192-233: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTighten
is_ncnameto reject non-round-tripping inputs.Document::parse("<{value}/>")accepts strings that only parse because they introduce comment/PI markup before the root, so malformedSignature/@IdandReference/@Idvalues can pass validation. Sincequick-xmlwrites attribute values verbatim, those inputs produce invalid XML.🤖 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/builder.rs` around lines 192 - 233, The `validate` method in `SignatureBuilder` only checks `Signature/@Id` and `Reference/@Id` with `is_ncname`, which lets through values that parse in `Document::parse` but do not round-trip safely when serialized by quick-xml. Update the ID validation used in `validate` so it rejects inputs that would not serialize back as a valid XML attribute value, and apply the stricter check consistently to both `signature_id` and each `reference.id` by using the existing `SignatureBuilderError::InvalidId` paths.
🤖 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/builder.rs`:
- Around line 192-233: The `validate` method in `SignatureBuilder` only checks
`Signature/@Id` and `Reference/@Id` with `is_ncname`, which lets through values
that parse in `Document::parse` but do not round-trip safely when serialized by
quick-xml. Update the ID validation used in `validate` so it rejects inputs that
would not serialize back as a valid XML attribute value, and apply the stricter
check consistently to both `signature_id` and each `reference.id` by using the
existing `SignatureBuilderError::InvalidId` paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8c96f6c4-a1ee-45fc-abd4-4a32cedfc5d5
📒 Files selected for processing (2)
src/xmldsig/builder.rstests/signature_builder.rs
Reproduce parser-based validation accepting comment and processing-instruction markup for Signature and Reference IDs.
Require parser-based XML names to round-trip exactly, preventing comment or processing-instruction markup from passing Signature and Reference ID validation.
|
@coderabbitai Re: NCName round-trip validation outside-diff finding — fixed in ee25df4. The parsed root local name must now equal the original input exactly, preventing comment/PI markup injection for both Signature/@id and Reference/@id. Regression coverage reproduces both bypass forms; full suite passes 511/511. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /tmp && rm -rf repo && git clone --quiet https://github.com/structured-world/xml-sec.git repo && cd repo
git log --oneline -1 ee25df4 2>/dev/null || echo "commit not found"
git show ee25df4 --stat
echo "---diff---"
git show ee25df4 -- src/xmldsig/builder.rsLength of output: 1980
|
|
@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 `@tests/signature_builder.rs`:
- Around line 199-213: The current invalid ID payloads in SignatureBuilder tests
fail the initial NCName character check instead of exercising the parser
round-trip path. Update the cases in
SignatureBuilder::new(...).signature_id(...) and
ReferenceBuilder::new(...).id(...) to use payloads that start as valid NCNames
but cross the boundary later, so the validation in build_template() covers the
intended injection scenario.
🪄 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: 7dc78bf3-e133-4604-bf13-4a7a41215485
📒 Files selected for processing (2)
src/xmldsig/builder.rstests/signature_builder.rs
|
|
||
| let injected_signature_id = | ||
| SignatureBuilder::new(exclusive_c14n(), SignatureAlgorithm::RsaSha256) | ||
| .signature_id("!--comment--><valid") | ||
| .add_reference(ReferenceBuilder::new(DigestAlgorithm::Sha256)) | ||
| .build_template() | ||
| .expect_err("markup must not satisfy Signature Id validation"); | ||
| assert!(injected_signature_id.to_string().contains("Signature Id")); | ||
|
|
||
| let injected_reference_id = | ||
| SignatureBuilder::new(exclusive_c14n(), SignatureAlgorithm::RsaSha256) | ||
| .add_reference(ReferenceBuilder::new(DigestAlgorithm::Sha256).id("?check?><valid")) | ||
| .build_template() | ||
| .expect_err("markup must not satisfy Reference Id validation"); | ||
| assert!(injected_reference_id.to_string().contains("Reference Id")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fn is_ncname" -A 30 src/xmldsig/builder.rsRepository: structured-world/xml-sec
Length of output: 882
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline tests/signature_builder.rs --view expanded
sed -n '160,240p' tests/signature_builder.rs | cat -n
rg -n 'foo/><!--x|foo\?>|NCName|signature_id\(|ReferenceBuilder::new\(DigestAlgorithm::Sha256\)\.id' tests/signature_builder.rsRepository: structured-world/xml-sec
Length of output: 4228
Use payloads that cross the NCName boundary. tests/signature_builder.rs:199-213 currently uses !--comment--><valid and ?check?><valid, which fail the first-character NCName check outright. Swap them for values like foo/><!--x and foo?><?pi? so this test actually covers the parser round-trip case the fix is meant to protect.
🤖 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 `@tests/signature_builder.rs` around lines 199 - 213, The current invalid ID
payloads in SignatureBuilder tests fail the initial NCName character check
instead of exercising the parser round-trip path. Update the cases in
SignatureBuilder::new(...).signature_id(...) and
ReferenceBuilder::new(...).id(...) to use payloads that start as valid NCNames
but cross the boundary later, so the validation in build_template() covers the
intended injection scenario.
1 participant
Summary
Testing
Closes #73