fix: Support nested enveloped signature location (#525)

[shunkica]

Description

This PR fixes an issue where the enveloped signature transformation was unable to find and remove Signature elements that were nested within other elements in the document.

Changes

• Updated the XPath query in from to to search for Signature elements at any depth, not just direct children
• Added a test case demonstrating the issue with nested signatures
• computeSignature sets signatureNode before processing references, and restores it if reference processing throws
• getCanonXml maps the signature element into the cloned subtree so the enveloped-signature transform removes exactly the signature being created, at any depth
• EnvelopedSignature removes the mapped node by identity during signing; verification still matches by SignatureValue
• Tests for nested locations, counter-signing at /root and /root/container, tampering, and state recovery after a failed sign

Related Issue

Fixes #525

Testing

Added a test case that verifies signatures can be validated when the Signature element is nested within other elements in the document structure.

Summary by CodeRabbit

• Bug Fixes

  • Improved XML signature handling for signatures located within nested elements.
  • Fixed canonicalization so the intended signature is removed while nested signatures remain intact.
  • Improved recovery when signature generation or reference processing fails.
  • Ensured signatures remain verifiable across subsequent signing operations and correctly detect tampering.

• Tests

  • Added coverage for nested signature locations, canonicalization behavior, repeated signing, and tampered signatures.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: cab7513e-fa06-4fa1-a036-87322ae19938

📥 Commits

Reviewing files that changed from the base of the PR and between 8c1af97 and a0375d2.

📒 Files selected for processing (4)

• src/enveloped-signature.ts
• src/signed-xml.ts
• test/canonicalization-unit-tests.spec.ts
• test/signature-integration-tests.spec.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.

---

📝 Walkthrough

Walkthrough

This change fixes nested enveloped-signature processing. It updates signature-node state handling, maps nested signatures into cloned XML during canonicalization, restricts SignatureValue lookup to direct children, and adds integration and unit tests for nested signatures and tampering.

Changes

Nested enveloped-signature handling

Layer / File(s)	Summary
Signature state and cloned-node mapping src/signed-xml.ts	computeSignature sets and restores the active signature node around reference processing. getCanonXml maps nested signature ancestors into the cloned XML before applying the enveloped-signature transform.
Nested signature removal and SignatureValue lookup src/enveloped-signature.ts	The transform removes a configured signature node when it is nested under the processing node. SignatureValue lookup now reads the first direct child.
Nested signature and canonicalization validation test/signature-integration-tests.spec.ts, test/canonicalization-unit-tests.spec.ts	Tests cover nested signature placement, failed computation recovery, parent and child verification, tampering, and preservation of deeper nested signatures.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Merge Risk: ⚪ Minimal · up to a0375

Nested enveloped signatures can now be removed from signed content at their actual location while preserving deeper signatures and restoring signing state after failures. The covered signing, verification, and tamper scenarios indicate no current merge-blocking risk.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant SignedXml
  participant EnvelopedSignature
  participant Verifier
  Client->>SignedXml: computeSignature with nested location
  SignedXml->>EnvelopedSignature: canonicalize referenced node
  EnvelopedSignature->>EnvelopedSignature: remove nested Signature node
  SignedXml-->>Client: return signed XML
  Client->>Verifier: checkSignature
  Verifier-->>Client: return verification result

Loading

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The rollback behavior in computeSignature and its related test address signature-node state after a failed reference-processing operation. This requirement is not stated in issue #525 and is separat…	Remove the rollback change and its dedicated test, or link an issue that explicitly requires restoring signatureNode after computeSignature fails.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue #525 by locating nested signature nodes, removing the correct signature during canonicalization, and adding tests for nested signing and verification.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 4…
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: support for nested enveloped signature locations.

Full details: Out of Scope Changes check

Explanation

The rollback behavior in computeSignature and its related test address signature-node state after a failed reference-processing operation. This requirement is not stated in issue #525 and is separate from nested enveloped-signature handling.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[cjbarth]

@shunkica , I've fixed a few regressions here. I think this is ready to merge. What do you think?

[shunkica]

LGTM. Updated PR body.