feat(xmldsig): add XML mutation strategy#77
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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds XMLDSig mutation helpers, updates crypto dependencies, switches RSAKeyValue parsing to crypto-bigint, and changes ECDSA P-256/P-384 verification to use raw signed data. ChangesXML mutation and crypto updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant append_signature_to_root
participant fill_digest_values
participant fill_signature_values
participant roxmltree
participant quick_xml
Caller->>append_signature_to_root: xml, signature_template
append_signature_to_root->>roxmltree: validate template root
append_signature_to_root->>quick_xml: stream and append template
append_signature_to_root->>roxmltree: re-parse output
append_signature_to_root-->>Caller: mutated XML
Caller->>fill_digest_values: xml, values
fill_digest_values->>roxmltree: count DigestValue elements
fill_digest_values->>quick_xml: replace matching text
fill_digest_values->>roxmltree: validate output
fill_digest_values-->>Caller: mutated XML
Caller->>fill_signature_values: xml, values
fill_signature_values->>roxmltree: count SignatureValue elements
fill_signature_values->>quick_xml: replace matching text
fill_signature_values->>roxmltree: validate output
fill_signature_values-->>Caller: mutated XML
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 |
|---|---|
| Cargo.toml | Updated RustCrypto dependency versions and added crypto-bigint to the xmldsig feature. |
| src/xmldsig/keys.rs | Adapted RSA KeyValue SPKI construction and tests for crypto-bigint::BoxedUint with trimmed byte encoding. |
| src/xmldsig/mod.rs | Exposes the new XMLDSig mutation module. |
| src/xmldsig/mutation.rs | Adds namespace-aware XMLDSig mutation helpers with fail-closed count checks and focused edge-case tests. |
| src/xmldsig/signature.rs | Migrates ECDSA verification calls to the updated RustCrypto verifier APIs while preserving signature encoding behavior. |
Reviews (3): Last reviewed commit: "fix(xmldsig): fail closed on skipped val..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/mutation.rs`:
- Around line 155-165: In the replacing_depth handling inside mutation logic,
Event::Empty is being counted as nested content even though it has no matching
end event. Update the event match in the replacing state so only Event::Start(_)
increments depth, while Event::Empty(_) is left uncounted, preserving correct
recognition of the target element’s real Event::End and avoiding dropping it
from the output.
🪄 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: ff7d1c1a-9c88-4ab9-99fe-4e604b069e08
📒 Files selected for processing (5)
Cargo.tomlsrc/xmldsig/keys.rssrc/xmldsig/mod.rssrc/xmldsig/mutation.rssrc/xmldsig/signature.rs
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/mutation.rs (1)
155-195: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when the streaming pass consumes fewer values than were counted.
count_dsig_elementscounts descendant matches, but once the stream enters a matching target, nested matching elements are skipped as replacement content. A parseable nested target can therefore satisfy the upfront count with two values while only writing one and still returningOk. Add a post-loopvalue_indexcheck before returning.Suggested guard
loop { let (namespace, event) = reader.read_resolved_event_into(&mut buf)?; @@ } buf.clear(); } + if value_index != values.len() { + return Err(XmlMutationError::ValueCountMismatch { + element: local_name, + expected, + actual: value_index, + }); + } + let output = String::from_utf8(writer.into_inner())?; roxmltree::Document::parse(&output)?; Ok(output)🤖 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/mutation.rs` around lines 155 - 195, Ensure the streaming replacement in mutation.rs fails closed when fewer DSIG values are actually consumed than `count_dsig_elements` predicted. In the `replace_dsig_values` flow, after the event loop and before parsing/returning the output, compare `value_index` against the number of counted values and return an error if not all values were written, since nested matching elements can be skipped while still being counted upfront.
🤖 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/mutation.rs`:
- Around line 155-195: Ensure the streaming replacement in mutation.rs fails
closed when fewer DSIG values are actually consumed than `count_dsig_elements`
predicted. In the `replace_dsig_values` flow, after the event loop and before
parsing/returning the output, compare `value_index` against the number of
counted values and return an error if not all values were written, since nested
matching elements can be skipped while still being counted upfront.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c04e30c-c7c6-4012-a023-cff86ade7840
📒 Files selected for processing (1)
src/xmldsig/mutation.rs
|
@coderabbitai Re: outside-diff finding on |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd mutation.rs src/xmldsig | xargs cat -n | sed -n '1,400p'Length of output: 17790 Confirmed — the fix at Lines 193–199 correctly re-checks Thanks for the detailed validation summary, |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Testing
Closes #76