Open
Conversation
- parse KeyInfo child sources: KeyName, KeyValue, X509Data, DEREncodedKeyValue - wire KeyInfo parsing into signature verification pipeline with explicit ParseKeyInfo error - add parser and pipeline coverage tests for order and malformed input Closes #46
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 1 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughParses ds:KeyInfo (KeyName, KeyValue, X509Data, dsig11:DEREncodedKeyValue); adds KeyInfo result types and parse_key_info(); introduces an internal XML-whitespace helper; integrates KeyInfo parsing into the verify pipeline; and tightens child-order and mixed-content validation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Verify as "verify.rs"
participant ParseSigned as "parse.rs (parse_signed_info)"
participant ParseKey as "parse.rs (parse_key_info)"
participant Whitespace as "whitespace.rs"
Caller->>Verify: verify_signature_with_context(signature_node)
Verify->>ParseSigned: parse_signed_info(signed_info_node)
ParseSigned-->>Verify: SignedInfo
Verify->>Verify: parse_signature_value()
alt KeyInfo present
Verify->>ParseKey: parse_key_info(key_info_node)
ParseKey->>Whitespace: is_xml_whitespace_only(text)
ParseKey->>ParseKey: dispatch children (KeyName, KeyValue, X509Data, DEREncodedKeyValue)
ParseKey-->>Verify: KeyInfo
else KeyInfo absent
Note over Verify: continue without KeyInfo
end
Verify-->>Caller: verification result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds initial XMLDSig <KeyInfo> child dispatch parsing (ROADMAP P2-001) and wires it into the verification pipeline so malformed/unsupported structures can be surfaced as explicit pipeline parse errors.
Changes:
- Introduces
KeyInfo/KeyInfoSourceparsing with dispatch forKeyName,KeyValue,X509Data, anddsig11:DEREncodedKeyValue. - Integrates
<KeyInfo>parsing into the verify pipeline and enforces<Signature>child ordering with optionalKeyInfoas the 3rd element child. - Re-exports the new parsing API from the
xmldsigmodule and adds targeted unit tests for parser + pipeline behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/xmldsig/parse.rs |
Adds KeyInfo model types and a dispatch parser for supported KeyInfo child elements + unit tests. |
src/xmldsig/verify.rs |
Parses optional KeyInfo in the verify pipeline, adds ParseKeyInfo error mapping, and enforces Signature child ordering + pipeline tests. |
src/xmldsig/mod.rs |
Re-exports new KeyInfo parsing types/functions as part of the public API. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/verify.rs (1)
964-1043:⚠️ Potential issue | 🟠 MajorReject unexpected
<Signature>children after the allowed prefix.This index tracking only proves where
SignedInfo,SignatureValue, andKeyInfoappear; it still accepts"<Signature><SignedInfo/><SignatureValue/><Foo/></Signature>"and"...<KeyInfo/><Foo/>"because any other trailing element is ignored. The content model after the optionalKeyInfois onlyObject*, so this should fail closed and get a regression test.As per coding guidelines,
Tier 1 - Flag logic bugs: missing validation (unvalidated input from network/disk/external source).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 964 - 1043, The parser currently ignores unexpected XMLDSIG element children after the allowed prefix, so "<Signature><SignedInfo/><SignatureValue/><Foo/></Signature>" wrongly succeeds; fix parse_signature_children to count only XMLDSIG element children (use a xml_element_index counter incremented only when child.tag_name().namespace() == Some(XMLDSIG_NS)) and enforce the content model: require SignedInfo at xml index 1 and SignatureValue at 2, allow optional KeyInfo at 3, and permit only "Object" elements after that; treat any other XMLDSIG element name at any position as SignatureVerificationPipelineError::InvalidStructure (include the element name in the reason) and fail if an unexpected element appears after the allowed prefix. Reference: parse_signature_children, signed_info_index, signature_value_index, key_info_index, and SignatureChildNodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/parse.rs`:
- Around line 357-383: The loop over element_children(key_info_node) accepts
malformed mixed-content like "<KeyValue>junk<RSAKeyValue/></KeyValue>" because
element_children() and collect_text_content() silently drop unexpected node
kinds; update the KeyInfo parsing to explicitly reject non-whitespace mixed
content and unexpected child elements: for KeyName (in the KeyName arm) assert
the node contains no element children and only text (trim() non-empty allowed,
any embedded element nodes or non-whitespace text before/after elements =>
return Err(ParseError::InvalidStructure(...))); for KeyValue and X509Data ensure
the node has no non-whitespace text nodes mixed with element children before
calling parse_key_value_dispatch or parse_x509_data_dispatch; for
DEREncodedKeyValue ensure there are no element children and the text node is the
only content before calling decode_base64_xml_text; return a clear
ParseError::InvalidStructure on violations. Apply the same validation pattern to
the other parsing locations you mentioned (around the
parse_key_value_dispatch/parse_x509_data_dispatch and DER decoding sites).
- Around line 115-161: Add forward-compatibility by marking the public parse
model as non-exhaustive and tighten field visibility: add #[non_exhaustive] to
KeyInfo, KeyInfoSource, KeyValueInfo, and X509DataInfo; change internal fields
that need not be publicly mutable (KeyInfo.sources and the X509DataInfo *_count
fields) from pub to pub(crate) so consumers cannot match exhaustively or rely on
concrete variants/fields; leave enum variants and public types exported but rely
on non_exhaustive to allow adding new variants/fields in future.
---
Outside diff comments:
In `@src/xmldsig/verify.rs`:
- Around line 964-1043: The parser currently ignores unexpected XMLDSIG element
children after the allowed prefix, so
"<Signature><SignedInfo/><SignatureValue/><Foo/></Signature>" wrongly succeeds;
fix parse_signature_children to count only XMLDSIG element children (use a
xml_element_index counter incremented only when child.tag_name().namespace() ==
Some(XMLDSIG_NS)) and enforce the content model: require SignedInfo at xml index
1 and SignatureValue at 2, allow optional KeyInfo at 3, and permit only "Object"
elements after that; treat any other XMLDSIG element name at any position as
SignatureVerificationPipelineError::InvalidStructure (include the element name
in the reason) and fail if an unexpected element appears after the allowed
prefix. Reference: parse_signature_children, signed_info_index,
signature_value_index, key_info_index, and SignatureChildNodes.
🪄 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
Run ID: f04a3c91-55a7-4840-8c05-39edae4ff650
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/verify.rs
Member
Author
|
Addressed the outside-diff item as well in commit 2004cf8: now fails closed for unexpected XMLDSIG children after the allowed SignedInfo/SignatureValue/(optional KeyInfo) prefix, and the regression suite passes. |
Member
Author
|
Correction: outside-diff item is fixed in commit 2004cf8. Function parse_signature_children now rejects unexpected XMLDSIG children after the allowed SignedInfo/SignatureValue/(optional KeyInfo) prefix, and full regression tests are green. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai Outside-diff item (verify.rs 964-1043) is already addressed: parse_signature_children now fails closed on unexpected XMLDSIG children after SignedInfo/SignatureValue/(optional KeyInfo), and workspace tests are green after latest commit 5238c92. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/xmldsig/verify.rs (1)
976-984:⚠️ Potential issue | 🟠 MajorReject foreign and mixed-content children under
<Signature>.This scan only tracks XMLDSig element children and silently skips everything else, so malformed inputs like
<foo:Bar/>or stray non-whitespace text after the allowed prefix still pass the new suffix checks. If this function is meant to enforce theSignedInfo,SignatureValue, optionalKeyInfo, thenObject*shape, it needs to fail closed on any non-XMLDSig element child and any non-whitespace mixed content under<Signature>.🔒 Suggested hardening
- for (zero_based_index, child) in signature_node - .children() - .filter(|node| node.is_element()) - .enumerate() - { - let element_index = zero_based_index + 1; - if child.tag_name().namespace() != Some(XMLDSIG_NS) { - continue; - } + let mut element_index = 0usize; + for child in signature_node.children() { + if child.is_text() + && child.text().is_some_and(|text| { + text.chars().any(|c| !matches!(c, ' ' | '\t' | '\n' | '\r')) + }) + { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must not contain non-whitespace mixed content", + }); + } + if !child.is_element() { + continue; + } + element_index += 1; + if child.tag_name().namespace() != Some(XMLDSIG_NS) { + return Err(SignatureVerificationPipelineError::InvalidStructure { + reason: "Signature must contain only ds:SignedInfo, ds:SignatureValue, optional ds:KeyInfo, and ds:Object element children", + }); + }As per coding guidelines,
Tier 1 - Flag logic bugs: missing validation (unchecked index, unvalidated input from network/disk/external source).Also applies to: 1050-1072
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 976 - 984, The loop over signature_node children in verify.rs currently ignores non-XMLDSIG elements and mixed content; change it to fail-closed: inside the iteration over signature_node.children() (the block around the for ... in signature_node.children().filter(|node| node.is_element()).enumerate()), remove the filter that hides non-element nodes and instead inspect every child node; if a child is an element and child.tag_name().namespace() != Some(XMLDSIG_NS) return an error (reject foreign elements), and if a child is not an element but contains non-whitespace text or non-trivial nodes (comments/PIs as disallowed per spec) return an error (reject mixed content); apply the same stricter checks in the corresponding later block referenced at lines ~1050-1072 so any unexpected non-XMLDSIG element or non-whitespace text under <Signature> causes verification to fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/parse.rs`:
- Around line 381-384: The DEREncodedKeyValue handler currently calls
collect_text_content(child) which clones the entire text payload before
decode_base64_xml_text enforces MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN /
MAX_DER_ENCODED_KEY_VALUE_LEN; change this to stream/validate the text nodes
first by replacing collect_text_content+decode_base64_xml_text with a new
decode_base64_xml_children(child, ...) helper that iterates
child.children().filter(|n| n.is_text()), enforces the existing
length/whitespace caps (MAX_DER_ENCODED_KEY_VALUE_TEXT_LEN and
MAX_DER_ENCODED_KEY_VALUE_LEN) as it appends, and returns the decoded bytes so
you can still push KeyInfoSource::DerEncodedKeyValue(der); keep
ensure_no_element_children(child, "DEREncodedKeyValue") and reuse the same
validation logic elsewhere (e.g., the ranges noted at lines ~550-594).
In `@src/xmldsig/verify.rs`:
- Around line 2510-2529: The test pipeline_reports_keyinfo_parse_error currently
corrupts KeyInfo but leaves <SignedInfo/> invalid, making the failure dependent
on parser order; update the test fixture so <SignedInfo> is a minimally valid
SignedInfo (e.g., include CanonicalizationMethod, SignatureMethod and at least
one Reference with DigestMethod and a DigestValue) and keep only the
dsig11:DEREncodedKeyValue corrupted, then call verify_signature_with_pem_key and
assert the returned error is SignatureVerificationPipelineError::ParseKeyInfo(_)
to make the regression independent of element parse order.
---
Outside diff comments:
In `@src/xmldsig/verify.rs`:
- Around line 976-984: The loop over signature_node children in verify.rs
currently ignores non-XMLDSIG elements and mixed content; change it to
fail-closed: inside the iteration over signature_node.children() (the block
around the for ... in signature_node.children().filter(|node|
node.is_element()).enumerate()), remove the filter that hides non-element nodes
and instead inspect every child node; if a child is an element and
child.tag_name().namespace() != Some(XMLDSIG_NS) return an error (reject foreign
elements), and if a child is not an element but contains non-whitespace text or
non-trivial nodes (comments/PIs as disallowed per spec) return an error (reject
mixed content); apply the same stricter checks in the corresponding later block
referenced at lines ~1050-1072 so any unexpected non-XMLDSIG element or
non-whitespace text under <Signature> causes verification to fail.
🪄 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
Run ID: 20e18548-9962-42e0-aaa9-80cd3a6c2bc8
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/verify.rs
Member
Author
|
@coderabbitai Outside-diff item (verify.rs 976-984 / 1050-1072) fixed in eec05eb: parse_signature_children now iterates all child nodes and fails closed on foreign element children, non-whitespace text mixed content, and non-element children under . |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/parse.rs`:
- Around line 625-628: The helper is_xml_whitespace_only is duplicated; extract
it into a shared internal module (e.g., xmldsig::utils or
xmldsig::internal::whitespace) as a single pub(crate) function, replace the
duplicate definitions in the parse and verify modules to call the new shared
function, and remove the redundant implementation so both parse's and verify's
code reference the same is_xml_whitespace_only symbol.
In `@src/xmldsig/verify.rs`:
- Around line 1123-1126: The function is_xml_whitespace_only is duplicated (one
in verify.rs and one in parse.rs); extract it into a single shared internal
utility module (e.g., xml_utils or utils) as a pub(crate) function named
is_xml_whitespace_only, remove the duplicate definitions, and update both
modules (verify.rs and parse.rs) to import and call the shared function; ensure
module visibility and imports are corrected and run tests to verify nothing
breaks.
🪄 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
Run ID: bf4a51e6-3bdd-45dc-a152-5b985c143e1a
📒 Files selected for processing (2)
src/xmldsig/parse.rssrc/xmldsig/verify.rs
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/verify.rs`:
- Around line 629-631: The call to parse_key_info(key_info_node) validates
KeyInfo but the returned parsed key material is intentionally not consumed yet;
update the block around parse_key_info (and the surrounding
signature_children.key_info_node handling) to include a short comment stating
this is deliberate for the current P2-001 dispatch-only phase and that future
work will wire the parsed KeyInfo into the KeyResolver for actual key resolution
(preserve the existing error mapping to
SignatureVerificationPipelineError::ParseKeyInfo).
🪄 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
Run ID: a173c093-c3c4-41ed-870c-3ceff37a47f9
📒 Files selected for processing (4)
src/xmldsig/mod.rssrc/xmldsig/parse.rssrc/xmldsig/verify.rssrc/xmldsig/whitespace.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/verify.rs`:
- Around line 2559-2617: These three tests mutate Signature but leave SignedInfo
malformed and call verify_signature_with_pem_key, so sibling checks run before
SignedInfo/crypto; instead, build each test XML by starting from
signature_with_target_reference() so SignedInfo is valid, then run the
verification using VerifyContext::new().key(&RejectingKey) (or equivalent
VerifyContext invocation) to isolate the child-order/content check; replace the
verify_signature_with_pem_key(...) calls in
pipeline_rejects_foreign_element_children_under_signature,
pipeline_rejects_non_whitespace_mixed_content_under_signature, and
pipeline_rejects_keyinfo_out_of_order with code that uses
signature_with_target_reference() to create the base signature and
VerifyContext::new().key(&RejectingKey) to assert the specific Signature child
validation 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
Run ID: 04985649-7531-4319-b827-ac0a6f141f01
📒 Files selected for processing (1)
src/xmldsig/verify.rs
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Details
src/xmldsig/parse.rssrc/xmldsig/mod.rsparse.rsandverify.rsValidation
cargo fmt -- --checkcargo clippy --all-targets --all-features -- -D warningscargo nextest run --workspacecargo test --doc --workspaceCloses #46