Skip to content

chore(xmldsig): introduce DsigError#37

Merged
polaz merged 2 commits intomainfrom
chore/#36-p1-024-dsig-error
Mar 30, 2026
Merged

chore(xmldsig): introduce DsigError#37
polaz merged 2 commits intomainfrom
chore/#36-p1-024-dsig-error

Conversation

@polaz
Copy link
Copy Markdown
Member

@polaz polaz commented Mar 29, 2026

Summary

  • rename XMLDSig pipeline error surface to DsigError
  • make TransformError::C14n carry typed C14nError via thiserror source chaining
  • update xmldsig exports and integration assertions to use DsigError

Testing

  • cargo fmt -- --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo nextest run --workspace
  • cargo test --doc --workspace
  • cargo build

Closes #36

- rename public XMLDSig pipeline error to DsigError
- make TransformError::C14n hold typed C14nError via thiserror source
- update xmldsig exports and integration tests to use DsigError

Closes #36
Copilot AI review requested due to automatic review settings March 29, 2026 20:59
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 84f9a815-3873-4002-a696-8edddc4fff8d

📥 Commits

Reviewing files that changed from the base of the PR and between f28a909 and 9d52d4c.

📒 Files selected for processing (1)
  • src/xmldsig/verify.rs

📝 Walkthrough

Summary by CodeRabbit

  • Refactor

    • Consolidated XML DSig verification errors by renaming the pipeline error type to a single exported error and updating verification APIs to use it.
    • Improved canonicalization/transform error propagation to preserve original error types for clearer error chaining.
  • Tests

    • Updated integration tests to assert against the consolidated error type.

Walkthrough

Replaces the public error type for the XMLDSig verification pipeline with DsigError (replacing SignatureVerificationPipelineError), updates public APIs and tests accordingly, and changes transform/canonicalization error propagation to carry typed C14nError instead of stringified messages.

Changes

Cohort / File(s) Summary
Verify API & Exports
src/xmldsig/mod.rs, src/xmldsig/verify.rs
Introduced DsigError as the public pipeline error type; updated trait and function Result<..., _> signatures (e.g., VerifyingKey::verify, KeyResolver::resolve, VerifyContext::verify, verify_signature_with_pem_key) to return DsigError; added internal alias type SignatureVerificationPipelineError = DsigError; re-exports adjusted.
Transform Error Type
src/xmldsig/types.rs
Changed TransformError::C14n payload from String to #[from] crate::c14n::C14nError, enabling typed error chaining and automatic conversion.
Transform Error Propagation
src/xmldsig/transforms.rs
Removed manual stringification of c14n errors; canonicalization calls now propagate C14nError directly via ? instead of .map_err(... to_string()).
Tests
tests/signature_pipeline_integration.rs
Updated imports and assertions to use and pattern-match xml_sec::xmldsig::DsigError variants instead of SignatureVerificationPipelineError.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'chore(xmldsig): introduce DsigError' is clear and directly related to the main change: renaming the XMLDSig pipeline error from SignatureVerificationPipelineError to DsigError.
Description check ✅ Passed The description is directly related to the changeset, outlining the three main changes (rename error type, update TransformError::C14n, update exports) and documenting testing steps.
Linked Issues check ✅ Passed The PR meets the acceptance criteria from issue #36: public verify APIs return Result<VerifyResult, DsigError> [verify.rs], TransformError::C14n is updated to carry typed C14nError with source chaining [types.rs], failure categories are preserved, and exports/tests are updated. CI requirements documented.
Out of Scope Changes check ✅ Passed All changes directly support the linked issue objectives: mod.rs updates exports, types.rs and transforms.rs align error handling with thiserror, verify.rs replaces the error type, and tests update assertions. No unrelated changes detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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 chore/#36-p1-024-dsig-error

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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces DsigError as the top-level XMLDSig verification pipeline error type, replacing the previous SignatureVerificationPipelineError naming and improving typed error chaining (notably for C14N/transform layers).

Changes:

  • Renamed the public XMLDSig pipeline error surface to DsigError and updated key/resolver hooks + verify APIs accordingly.
  • Updated TransformError::C14n to carry a typed C14nError source (instead of String) and simplified transform error propagation.
  • Updated integration tests and xmldsig module exports to use DsigError.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/signature_pipeline_integration.rs Updates assertions/imports to expect DsigError variants.
src/xmldsig/verify.rs Introduces pub enum DsigError and updates public verify/key/resolver error types.
src/xmldsig/types.rs Switches TransformError::C14n to typed C14nError with #[from] for source chaining.
src/xmldsig/transforms.rs Uses ? to propagate C14nError into TransformError via From.
src/xmldsig/mod.rs Re-exports DsigError and removes the old pipeline error export.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

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/verify.rs (1)

445-447: ⚠️ Potential issue | 🟠 Major

Add #[source] annotations to ReferenceProcessingError tuple fields to preserve the TransformError/C14nError source chain.

Line 447 wraps ReferenceProcessingError, but the inner variants UriDereference and Transform hold TransformError as plain tuple fields. In thiserror, plain tuple fields do not participate in Error::source() — callers cannot walk the error chain to inspect the underlying typed TransformError or C14nError.

Fix
 pub enum ReferenceProcessingError {
     /// URI dereference failed.
     #[error("URI dereference failed: {0}")]
-    UriDereference(super::types::TransformError),
+    UriDereference(#[source] super::types::TransformError),

     /// Transform execution failed.
     #[error("transform failed: {0}")]
-    Transform(super::types::TransformError),
+    Transform(#[source] super::types::TransformError),
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/xmldsig/verify.rs` around lines 445 - 447, The ReferenceProcessingError
enum's tuple fields aren't exposed as error sources; add #[source] to the tuple
fields in the enum variants that wrap underlying errors so Error::source()
returns the inner TransformError/C14nError. Specifically, update the
ReferenceProcessingError variants (e.g., UriDereference(#[source]
TransformError) and Transform(#[source] C14nError) — or whichever variants wrap
TransformError/C14nError) to annotate those tuple fields with #[source] so the
error chain is preserved; no other API changes required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/xmldsig/verify.rs`:
- Around line 445-447: The ReferenceProcessingError enum's tuple fields aren't
exposed as error sources; add #[source] to the tuple fields in the enum variants
that wrap underlying errors so Error::source() returns the inner
TransformError/C14nError. Specifically, update the ReferenceProcessingError
variants (e.g., UriDereference(#[source] TransformError) and Transform(#[source]
C14nError) — or whichever variants wrap TransformError/C14nError) to annotate
those tuple fields with #[source] so the error chain is preserved; no other API
changes required.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5bc3b52b-ea57-4c31-b2ad-bf157f72c55b

📥 Commits

Reviewing files that changed from the base of the PR and between 7e8952c and f28a909.

📒 Files selected for processing (5)
  • src/xmldsig/mod.rs
  • src/xmldsig/transforms.rs
  • src/xmldsig/types.rs
  • src/xmldsig/verify.rs
  • tests/signature_pipeline_integration.rs

@polaz
Copy link
Copy Markdown
Member Author

polaz commented Mar 29, 2026

@coderabbitai Fixed. Added #[source] on ReferenceProcessingError::UriDereference and ::Transform so Error::source() preserves the typed TransformError/C14nError chain.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 29, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me verify the fix in the codebase.


🏁 Script executed:

#!/bin/bash
# Find where ReferenceProcessingError is defined
rg -n "ReferenceProcessingError" --type rust -C 5

Length of output: 6524


🏁 Script executed:

#!/bin/bash
# Show the full ReferenceProcessingError enum definition
rg -n "ReferenceProcessingError" --type rust -A 30 "pub enum ReferenceProcessingError"

Length of output: 73


🏁 Script executed:

#!/bin/bash
# Show the full ReferenceProcessingError enum definition
rg -n --type rust -A 25 "pub enum ReferenceProcessingError"

Length of output: 1746


@polaz: Confirmed — the fix looks correct. Both ReferenceProcessingError::UriDereference and ::Transform now carry #[source] super::types::TransformError (Lines 394 and 398), so Error::source() correctly exposes the typed TransformError at that layer.

Using #[source] without #[from] is also the right call here: since both variants wrap the same type (TransformError), adding #[from] to both would be a compile error (ambiguous From<TransformError> impl). The explicit map_err call-sites at Lines 316 and 320 already handle the conversion.

The full error chain now traverses cleanly:

DsigError::Reference
  └─ ReferenceProcessingError::Transform  (#[source])
       └─ TransformError::C14n            (#[from] / #[source])
            └─ C14nError

🧠 Learnings used
Learnt from: CR
Repo: structured-world/xml-sec PR: 0
File: .github/instructions/rust.instructions.md:0-0
Timestamp: 2026-03-17T12:12:28.146Z
Learning: Applies to **/*.rs : Tier 1 - Flag logic bugs: error swallowing (`let _ = fallible_call()` silently dropping errors that affect correctness)

Learnt from: polaz
Repo: structured-world/xml-sec PR: 32
File: src/xmldsig/verify.rs:34-51
Timestamp: 2026-03-28T22:00:29.444Z
Learning: In `src/xmldsig/verify.rs` (xml-sec Rust project), the `VerifyingKey` and `KeyResolver` public traits intentionally omit `Send + Sync` supertraits to preserve lightweight single-threaded implementations. The single-threaded-only constraint is documented on both trait definitions. Do not flag the absence of `Send + Sync` on these traits as a review issue.

Learnt from: polaz
Repo: structured-world/xml-sec PR: 0
File: :0-0
Timestamp: 2026-03-28T20:47:05.798Z
Learning: In `src/xmldsig/verify.rs` (xml-sec Rust project), the ASCII-whitespace branch in `push_normalized_signature_text` uses `u8::try_from(u32::from(ch)).expect("ASCII whitespace always fits into u8")` for the explicit narrowing conversion, producing `base64::DecodeError::InvalidByte(normalized.len(), invalid_byte)` as the error payload. The conversion is provably infallible because `ch.is_ascii_whitespace()` guarantees the character is in the 0x00–0x7F range.

Learnt from: polaz
Repo: structured-world/xml-sec PR: 0
File: :0-0
Timestamp: 2026-03-28T15:50:07.840Z
Learning: In `src/xmldsig/verify.rs` (xml-sec Rust project), `push_normalized_signature_text` strips only the four XML whitespace characters (space 0x20, tab 0x09, CR 0x0D, LF 0x0A) from `SignatureValue` text before base64 decoding, and explicitly rejects any other ASCII whitespace (e.g., form feed 0x0C, vertical tab 0x0B) as `SignatureVerificationPipelineError::SignatureValueBase64(base64::DecodeError::InvalidByte(...))`. The regression test `push_normalized_signature_text_rejects_form_feed` covers the form-feed boundary case.

Learnt from: polaz
Repo: structured-world/xml-sec PR: 0
File: :0-0
Timestamp: 2026-03-28T18:51:49.919Z
Learning: In `src/xmldsig/parse.rs` (xml-sec Rust project), the private function `n` (formerly `base64_decode_digest`) strips only the four XML whitespace characters (space 0x20, tab 0x09, CR 0x0D, LF 0x0A) from `DigestValue` base64 text before decoding, and returns `ParseError::Base64(format!("invalid XML whitespace U+{:04X} ...", ...))` for any other `is_ascii_whitespace()` character (e.g. form feed 0x0C). Regression tests `n_accepts_xml_whitespace_chars` and `n_rejects_non_xml_ascii_whitespace` cover these cases.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@polaz polaz merged commit 561d6fd into main Mar 30, 2026
13 checks passed
@polaz polaz deleted the chore/#36-p1-024-dsig-error branch March 30, 2026 02:57
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.

chore(xmldsig): introduce DsigError for verify pipeline

2 participants