Merged
Conversation
- parse and process ds:Manifest references under ds:Object when enabled - preserve non-fatal manifest semantics via manifest_references diagnostics - add regression tests for enabled/disabled and mismatch behavior
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduce per-origin reference tracking via a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds optional processing of <ds:Manifest> references under <ds:Object> during XMLDSig verification, surfacing those outcomes on VerifyResult without changing the overall signature validity decision unless manifest processing is enabled.
Changes:
- Implement manifest reference discovery + processing behind
VerifyContext::process_manifests(true). - Extend
VerifyResultto return collected manifestReferenceResults. - Add tests covering enabled/disabled manifest processing and manifest digest mismatch reporting.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/xmldsig/verify.rs |
Implements manifest reference parsing/processing in the verification pipeline and adds regression tests. |
src/xmldsig/parse.rs |
Exposes parse_reference as pub(crate) so manifest parsing can reuse existing reference parsing logic. |
Member
Author
|
@coderabbitai re-review |
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/xmldsig/verify.rs`:
- Around line 665-669: The loop over manifest_node currently uses
object_node.descendants() which matches <ds:Manifest> at any depth; change to
object_node.children() so only direct child <ds:Manifest> elements (per XMLDSig
§5.1) are considered, keeping the existing namespace/name checks using
XMLDSIG_NS and tag_name().name() == "Manifest"; if the broader behavior was
intentional, instead add a comment documenting the permissive design choice
rather than changing the traversal.
🪄 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: 34689a70-1662-4b4a-93b0-258bc13fe685
📒 Files selected for processing (2)
src/xmldsig/parse.rssrc/xmldsig/verify.rs
- map manifest reference parse failures to explicit DsigError variant - track reference origin/index for SignedInfo vs Manifest processing - process only direct ds:Manifest children under ds:Object Closes #41
There was a problem hiding this comment.
Actionable comments posted: 1
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)
597-610:⚠️ Potential issue | 🟡 MinorManifest diagnostics disappear on
ReferenceDigestMismatch.The return on Line 599 exits before Lines 606-610 compute
manifest_references, soprocess_manifests(true)keeps manifest results on laterSignatureMismatch/KeyNotFoundpaths but drops them when a<SignedInfo>reference fails. That makes the new diagnostics inconsistent across invalid outcomes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 597 - 610, The early return when references.first_failure is Some skips computing manifest_references, causing manifest diagnostics to be lost on ReferenceDigestMismatch; compute manifest_references before returning (i.e., evaluate process_manifest_references(signature_node, &resolver, ctx) when ctx.process_manifests is true) so that whether you return early from the first_failure branch or continue, you still populate VerifyResult.manifest_references; update the code around references.first_failure, VerifyResult construction, and the manifest_references binding to ensure process_manifest_references is invoked unconditionally when ctx.process_manifests is true.
🤖 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 669-684: The code currently uses `?` on
`enforce_reference_policies` and `process_reference` so any non-parse manifest
error aborts `verify()`; instead, when processing manifests (the
`manifest_references` loop, `ReferenceSet::Manifest`), capture errors from
`enforce_reference_policies` and from each `process_reference` call and convert
them into entries stored in the `results` collection (the same way
`ParseManifestReference` is handled) rather than propagating with `?`;
specifically, stop using `?` for these calls inside the loop (and for policy
checks for manifest processing), match on the Result, push Ok(result) or
Err(error) into `results` (ensuring the error type matches what
`VerifyResult::manifest_references` expects), and only propagate fatal errors
outside of optional manifest processing so optional manifest failures are
recorded in `VerifyResult::manifest_references` instead of aborting `verify()`.
---
Outside diff comments:
In `@src/xmldsig/verify.rs`:
- Around line 597-610: The early return when references.first_failure is Some
skips computing manifest_references, causing manifest diagnostics to be lost on
ReferenceDigestMismatch; compute manifest_references before returning (i.e.,
evaluate process_manifest_references(signature_node, &resolver, ctx) when
ctx.process_manifests is true) so that whether you return early from the
first_failure branch or continue, you still populate
VerifyResult.manifest_references; update the code around
references.first_failure, VerifyResult construction, and the manifest_references
binding to ensure process_manifest_references is invoked unconditionally when
ctx.process_manifests is true.
🪄 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: 5350e646-9cbc-4612-97b4-d285a46faec4
📒 Files selected for processing (3)
src/xmldsig/mod.rssrc/xmldsig/verify.rstests/reference_integration.rs
- preserve manifest diagnostics even when SignedInfo reference fails - record manifest policy/processing failures as invalid reference statuses - add regression tests for optional manifest failure handling
- document ReferenceSet and reference_index semantics - remove stale ref_index wording tied to SignedInfo only
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/verify.rs`:
- Around line 162-166: Update the docstring on manifest handling in verify.rs to
avoid overstating non-fatal semantics: state that only per-reference policy
violations and reference-processing failures are captured in
VerifyResult::manifest_references, while structural or parsing errors (e.g.,
empty <ds:Manifest>, children that are not <ds:Reference>, or XML parse errors)
will cause verify() to return Err(...); mention that structural/parse errors
abort verification and are not converted to per-reference statuses. Reference
the VerifyResult::manifest_references field and the verify() function so
reviewers can locate where behavior is described.
- Around line 683-712: The code currently discards error details from
enforce_reference_policies and process_reference; update the Err arms that push
manifest_reference_invalid_result to first log the error details with
tracing::debug (including the reference index and the error) before pushing the
generic FailureReason (e.g., in the Err branch after enforce_reference_policies
and the Err(_) branch of process_reference); reference the functions
enforce_reference_policies, process_reference and the helper
manifest_reference_invalid_result and include the
FailureReason::ReferencePolicyViolation and
FailureReason::ReferenceProcessingFailure variants in the debug log to preserve
observability without changing the returned result.
🪄 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: fe3d917a-f8f0-4cf4-b1cb-eced2f75b720
📒 Files selected for processing (1)
src/xmldsig/verify.rs
- document that structural/parse manifest errors are fatal in verify() - add debug logs for non-fatal manifest policy and processing failures - keep per-reference invalid statuses unchanged for API compatibility
- map missing URI/non-policy precheck errors to ReferenceProcessingFailure - keep DisallowedUri/DisallowedTransform mapped to ReferencePolicyViolation - preserve omitted URI diagnostics as <omitted> and add regression coverage
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/verify.rs`:
- Around line 1451-1455: Format the blocked call to object_suffix.replacen by
running rustfmt (cargo fmt) so the mutated_object_suffix assignment conforms to
project style; specifically reformat the object_suffix.replacen(...) invocation
(the mutated_object_suffix binding) to satisfy rustfmt's expected line
breaks/indentation.
- Around line 1293-1309: signature_with_manifest_xml() currently calls
parse_manifest_references() and then uses the parsed manifest_reference, which
couples the fixture to the parser under test; instead, remove the
parse_manifest_references() call and hardcode the known manifest values
(URI="#target", no transforms, digest method = SHA-1) when constructing the
manifest_reference used to compute the digest. Use the same
UriReferenceResolver::new(&doc) to dereference "#target" to get the raw target
bytes, run execute_transforms with an empty transforms list (or skip
transforms), then call compute_digest with the explicit SHA-1 digest method to
produce computed_manifest_digest_b64; this ensures signature_with_manifest_xml()
does not rely on parse_manifest_references().
🪄 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: bf56e313-29bc-4874-8741-90db75cb2670
📒 Files selected for processing (2)
Cargo.tomlsrc/xmldsig/verify.rs
- compute manifest digest from known fixture constants (#target, no transforms, SHA-1) - apply rustfmt formatting on manifest test helper blocks
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/verify.rs`:
- Around line 1309-1323: Update the test
verify_context_processes_manifest_references_when_enabled() to also exercise the
“manifests disabled” path with a malformed <ds:Manifest> so regressions in
parse_manifest_references() escaping the ctx.process_manifests guard are caught;
reuse the broken fixture from
verify_context_reports_manifest_reference_parse_errors_explicitly() (or
construct a malformed signature_with_manifest_xml variant), call
VerifyContext::new().key(&RejectingKey).verify(&xml) with ctx.process_manifests
disabled, and assert that result.manifest_references remains empty and
result.status still matches
DsigStatus::Invalid(FailureReason::SignatureMismatch), ensuring
parse_manifest_references() does not run when ctx.process_manifests is false.
- Around line 1413-1454: Update the two tests
verify_context_records_manifest_policy_violations_without_aborting and
verify_context_records_manifest_missing_uri_as_processing_failure to also assert
the top-level VerifyResult::status remains driven by signature verification:
after calling
VerifyContext::new().key(&RejectingKey).process_manifests(true).verify(...), add
an assertion that result.status ==
DsigStatus::Invalid(FailureReason::SignatureMismatch) so that
ReferencePolicyViolation and ReferenceProcessingFailure remain per-reference
only and are not promoted to the top-level status.
🪄 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: 492e791e-43b8-4019-803b-1f46ee3a8d33
📒 Files selected for processing (1)
src/xmldsig/verify.rs
- cover malformed Manifest while process_manifests is disabled - assert top-level SignatureMismatch for manifest policy/processing failures
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/xmldsig/verify.rs (2)
1412-1454: 🧹 Nitpick | 🔵 TrivialAssert top-level
statuson non-fatal Manifest failure tests.Both
verify_context_records_manifest_policy_violations_without_abortingandverify_context_records_manifest_missing_uri_as_processing_failureverify the per-reference result is recorded, but don't assert thatresult.statusremains driven by SignedInfo/signature verification. Addassert!(matches!(result.status, DsigStatus::Invalid(FailureReason::SignatureMismatch)))to pin the non-fatal contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 1412 - 1454, Add an assertion in both tests verify_context_records_manifest_policy_violations_without_aborting and verify_context_records_manifest_missing_uri_as_processing_failure to ensure the top-level result.status remains driven by SignedInfo/signature verification (i.e. non-fatal manifest problems do not override it); specifically, after the existing checks add assert!(matches!(result.status, DsigStatus::Invalid(FailureReason::SignatureMismatch))) so the tests pin the contract that manifest failures are recorded per-reference but do not abort or change the overall signature status.
1308-1347: 🧹 Nitpick | 🔵 TrivialConsider adding a malformed-Manifest regression with manifest processing disabled.
The test exercises the disabled path with a well-formed
<ds:Manifest>. To catchparse_manifest_references()accidentally moving outside thectx.process_manifestsguard, consider adding a case that uses the broken fixture fromverify_context_reports_manifest_reference_parse_errors_explicitly()with manifest processing disabled, asserting that verification still succeeds (or fails only due to signature mismatch, not manifest parse errors).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 1308 - 1347, Add a regression case that uses the malformed manifest fixture (the same broken XML used by verify_context_reports_manifest_reference_parse_errors_explicitly()) and runs VerifyContext::new().key(&RejectingKey).verify(&xml) with process_manifests left false to ensure parse_manifest_references() does not run; assert that result.manifest_references is empty and that result.status still matches DsigStatus::Invalid(FailureReason::SignatureMismatch) (i.e., no manifest parse errors are reported when process_manifests is not enabled).
🤖 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 710-725: The catch-all Err(err) branch handling
enforce_reference_policies results masks any new error variants by converting
them to FailureReason::ReferenceProcessingFailure; update the match so you
explicitly handle Reference(MissingUri) (or other known variants) and make the
fallback unreachable (e.g., replace the generic Err arm with an explicit match
or add a debug_assert/unreachable! comment) and/or add a clear comment that the
fallback is defensive; specifically adjust the match that calls
enforce_reference_policies in verify.rs so Reference(MissingUri) is handled in
the same way as process_reference does, keep
manifest_reference_invalid_result(...) and the tracing::debug call for known
cases, and remove or mark the generic Err(err) branch as unreachable/intentional
to avoid silently hiding future errors.
---
Duplicate comments:
In `@src/xmldsig/verify.rs`:
- Around line 1412-1454: Add an assertion in both tests
verify_context_records_manifest_policy_violations_without_aborting and
verify_context_records_manifest_missing_uri_as_processing_failure to ensure the
top-level result.status remains driven by SignedInfo/signature verification
(i.e. non-fatal manifest problems do not override it); specifically, after the
existing checks add assert!(matches!(result.status,
DsigStatus::Invalid(FailureReason::SignatureMismatch))) so the tests pin the
contract that manifest failures are recorded per-reference but do not abort or
change the overall signature status.
- Around line 1308-1347: Add a regression case that uses the malformed manifest
fixture (the same broken XML used by
verify_context_reports_manifest_reference_parse_errors_explicitly()) and runs
VerifyContext::new().key(&RejectingKey).verify(&xml) with process_manifests left
false to ensure parse_manifest_references() does not run; assert that
result.manifest_references is empty and that result.status still matches
DsigStatus::Invalid(FailureReason::SignatureMismatch) (i.e., no manifest parse
errors are reported when process_manifests is not enabled).
🪄 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: ebc1b3cb-5715-426c-92fb-2ea71c81a0d3
📒 Files selected for processing (1)
src/xmldsig/verify.rs
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/verify.rs`:
- Around line 1403-1423: Add a parallel test case using an accepting key so
manifest digest mismatches remain non-fatal: introduce or reuse an AcceptingKey
fixture that returns Ok(true) and call
VerifyContext::new().key(&AcceptingKey).process_manifests(true).verify(&xml);
then assert that result.manifest_references still reports the specific
FailureReason variants (e.g., ReferenceDigestMismatch / ReferencePolicyViolation
/ ReferenceProcessingFailure) for the manifest entry while result.status equals
DsigStatus::Valid; mirror this pattern wherever RejectingKey-only checks exist
(e.g., around the blocks referencing VerifyContext, RejectingKey,
result.manifest_references, DsigStatus and FailureReason in the tests at the
other location).
- Around line 160-168: The documentation for manifest processing is ambiguous
about the acceptance contract: update the docblock for process_manifests to
explicitly state that enabling manifests (process_manifests(true)) makes
manifest digest mismatches, policy violations, and processing failures non-fatal
— they are reported per-reference in VerifyResult::manifest_references and do
not change VerifyResult::status; therefore callers of verify() must inspect
VerifyResult::manifest_references in addition to VerifyResult::status to
determine overall acceptance of manifests and to handle individual reference
failures. Ensure the text names process_manifests, verify(),
VerifyResult::status, and VerifyResult::manifest_references so readers know
where to look.
🪄 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: b380abc5-137c-45b6-94f6-c860e076764e
📒 Files selected for processing (1)
src/xmldsig/verify.rs
- document non-fatal manifest acceptance semantics in process_manifests docs - add accepting-key regression tests for digest, policy, and missing-uri manifest failures - remove tracing dependency and manifest debug logging to keep dependency surface lean
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 614-618: Currently process_manifest_references is invoked when
ctx.process_manifests is true and processes every <ds:Manifest> under the
Signature regardless of whether SignedInfo actually references it; change the
logic in verify() to first collect the set of Reference URIs from SignedInfo
(use the SignedInfo parsing code / symbol that produces reference URIs), then
call process_manifest_references(signature_node, &resolver, ctx) only for
Manifest/Object nodes whose ID/URI is present in that referenced set, or
alternatively return unsigned manifests separately (e.g.,
manifest_references_verified vs manifest_references_unverified) so callers
cannot treat unreferenced Manifests as authenticated; update all sites that rely
on manifest_references (including code around process_manifests, the verify()
return path, and any logic in or around process_manifest_references) to use the
verified set.
🪄 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: ab294501-027a-4aae-a0f2-c55a15f669a2
📒 Files selected for processing (1)
src/xmldsig/verify.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/xmldsig/verify.rs (1)
1570-1593:⚠️ Potential issue | 🟠 MajorThese accepting-key tests still invalidate the signed
#manifestsubtree.Mutating the Manifest reference URI after
signature_with_manifest_xml(true)has already computed the outerURI="#manifest"digest means these cases still produce a top-levelReferenceDigestMismatch. They therefore do not prove thatReferencePolicyViolation/ReferenceProcessingFailurestay non-fatal for an otherwise-valid signature. Recompute the SignedInfo digest after the mutation, or build the mutated fixture before computing it, and keep the accepting-key assertion onDsigStatus::Valid.Also applies to: 1623-1647
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/xmldsig/verify.rs` around lines 1570 - 1593, The test verify_context_records_manifest_policy_violations_with_accepting_key mutates the Manifest reference URI after the fixture’s SignedInfo digest was already computed by signature_with_manifest_xml(true), which causes a ReferenceDigestMismatch; fix by producing the mutated fixture before SignedInfo is generated (or by recomputing the SignedInfo digest after performing the URI mutation) so the signature remains valid, then run VerifyContext::new().key(&AcceptingKey).process_manifests(true).verify(...) and assert result.status is DsigStatus::Valid while keeping the manifest_references[0].status assertion for ReferencePolicyViolation; apply the same change to the related test duplicated in the other block (the one referenced as also applies to 1623-1647).
🤖 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 162-175: Update the docblock for process_manifests(true) to state
that only direct-child <ds:Object>/<ds:Manifest> elements that are themselves
referenced from SignedInfo are processed and returned in
VerifyResult::manifest_references; explicitly note that direct-child Manifests
which are not referenced (i.e., unsigned/unreferenced) are skipped and will not
appear in the list so an empty manifest_references does not imply no Manifest
elements existed. Also add the same clarifying sentence to the documentation for
VerifyResult::manifest_references and ensure the wording references verify() and
process_manifests so callers know to inspect manifest_references in addition to
VerifyResult::status.
---
Duplicate comments:
In `@src/xmldsig/verify.rs`:
- Around line 1570-1593: The test
verify_context_records_manifest_policy_violations_with_accepting_key mutates the
Manifest reference URI after the fixture’s SignedInfo digest was already
computed by signature_with_manifest_xml(true), which causes a
ReferenceDigestMismatch; fix by producing the mutated fixture before SignedInfo
is generated (or by recomputing the SignedInfo digest after performing the URI
mutation) so the signature remains valid, then run
VerifyContext::new().key(&AcceptingKey).process_manifests(true).verify(...) and
assert result.status is DsigStatus::Valid while keeping the
manifest_references[0].status assertion for ReferencePolicyViolation; apply the
same change to the related test duplicated in the other block (the one
referenced as also applies to 1623-1647).
🪄 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: c88eede6-c268-4269-a80d-c6c053fa91e7
📒 Files selected for processing (1)
src/xmldsig/verify.rs
Member
Author
|
@coderabbitai Addressed duplicate item about accepting-key manifest tests. In commit 31cb760, fixture generation now supports manifest mutation before SignedInfo digest finalization ( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5928
Both accepting-key tests (
The doc comment updates to 🧠 Learnings used |
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 631-633: collect_signed_info_reference_ids currently reduces
SignedInfo URIs to plain strings and
parse_manifest_references/process_manifest_references uses string membership to
mark direct-child <Object>/<Manifest> as signed; change this to resolve each
SignedInfo reference through the same resolver used for signature verification
and compare the resolved Node identity (e.g., pointer/address/unique node id or
canonicalized node) instead of raw Id strings in process_manifest_references and
parse_manifest_references; additionally, before processing manifests detect and
reject ambiguous collisions where multiple direct-child nodes resolve to the
same referenced node (or treat them as unsigned) to avoid aliasing attacks,
update collect_signed_info_reference_ids usage to return resolved node
identifiers or a mapping, and add a regression test that creates two
direct-child manifest/object nodes with the same Id to assert the function
rejects or handles the collision as specified.
🪄 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: e64f51a0-ede9-4f44-a27e-69771305b6dc
📒 Files selected for processing (1)
src/xmldsig/verify.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 1327-1338: Add a new test fixture where SignedInfo's Reference URI
points at the enclosing ds:Object (e.g., <ds:Reference URI="#object-id">) while
the nested <ds:Manifest> inside that ds:Object has no ID; this will exercise the
object_is_signed branch in parse_manifest_references(). Update/ add a fixture
file used by the verify.rs tests to include a ds:Object with Id set and a nested
ds:Manifest without an Id, ensure the signature Reference targets the ds:Object
Id, and run the existing tests so parse_manifest_references() is exercised for
the signed-object path.
🪄 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: f2969f51-dba0-411c-98e5-02048b30526e
📒 Files selected for processing (2)
src/xmldsig/uri.rssrc/xmldsig/verify.rs
- clarify process_manifests docs to require ID-based same-document references - document whole-document URI references as non-signing for direct child selection - add regression test for SignedInfo reference to ds:Object with nested Manifest without ID Refs #41
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
Validation