[cjbarth]

Summary

When a reference's transforms ended in enveloped-signature, signing digested xmldom's node.toString() while verification appended inclusive C14N in loadReference. Any document whose serialization is not already canonical, such as one with a self-closing tag or attributes out of order, got a signature no version of this library could verify.

Both sides reach the digest through getCanonXml, so it now converts a node-set left after the last transform to octets with inclusive C14N, as the reference processing model requires. Signing and verification canonicalize under the same condition by construction, and verification of existing signatures using the built-in algorithms is unchanged. This also drops the getCanonXml TODO about transforms returning either a Node or a string. Transforms after one that returns a string are still skipped rather than getting the octet-to-node-set conversion; that is tracked in #111.

Compatibility

Documented in the README's Upgrading section.

• Signatures created for such references now verify with 6.1.2 as well.
• getCanonXml() is public. For a transform list ending in a node-set it now returns canonical XML (<y></y>) instead of xmldom's serialization (<y/>). Two tests pinned the old output and are updated.
• CanonicalizationOrTransformationAlgorithm.process() is typed Node | string. A custom transform, or a custom SignedInfo canonicalization algorithm, that returns a Node is now canonicalized on both sides, so its digest or signature differs from 6.1.x: signatures created with it by 6.1.x no longer verify, and the reverse. Signers and verifiers using one need to upgrade together.

Fixes #549

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Improved XML signature canonicalization when transformations return DOM nodes, producing consistent canonical output.
  • Updated empty-element handling to use explicit start and end tags where required.
  • Improved verification reliability for enveloped-signature transformations, including preservation of inherited namespaces.

• Documentation

  • Added guidance on DOM-node transformations, canonicalization behavior, verification, and compatibility differences affecting custom transformations and canonicalizers.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 7edfd778-0d0c-4836-b723-cafb95e7d43a

📥 Commits

Reviewing files that changed from the base of the PR and between 8c18cea and 5d14e3c.

📒 Files selected for processing (2)

• README.md
• test/signature-integration-tests.spec.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• README.md

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

---

📝 Walkthrough

Walkthrough

The signing path now canonicalizes DOM nodes returned by the final transform before digesting. Tests update canonical XML expectations, add enveloped-signature verification coverage, and document the changed behavior.

Changes

DOM result canonicalization

Layer / File(s)	Summary
Canonicalize DOM transform results src/signed-xml.ts	getCanonXml() applies non-exclusive XML C14N when transform processing leaves a DOM node.
Validate canonical signing output test/*, README.md	Canonicalization tests expect <y></y>. Integration coverage verifies enveloped-signature-only references and inherited namespaces. The README documents the behavior change.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant SignedXml
  participant getCanonXml
  participant C14N
  participant Verifier
  SignedXml->>getCanonXml: Apply enveloped-signature transform
  getCanonXml->>C14N: Canonicalize DOM node
  C14N-->>SignedXml: Return canonical octets
  SignedXml->>Verifier: Produce signed XML
  Verifier->>C14N: Canonicalize referenced node
  C14N-->>Verifier: Return matching octets

Loading

Suggested reviewers: shunkica

Merge Risk: ⚪ Minimal · up to 5d14e

The canonicalization change is covered by updated regression tests for empty elements, attribute order, enveloped signatures, and inherited namespaces. No unresolved merge risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The implementation satisfies the main coding requirements in #549. getCanonXml() canonicalizes a final DOM Node with inclusive C14N. The integration coverage includes an enveloped-signature-only…	Add an explanatory comment to both updated canonicalization tests. State that the final node-set is converted with inclusive C14N and that C14N renders an empty element as <y></y>, not <y/>.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: canonicalizing a node-set that remains after the final transform.
Out of Scope Changes check	✅ Passed	The source change, test updates, integration coverage, and README documentation support #549. They address final node-set canonicalization, signing and verification consistency, or the resulting seria…
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…

Full details: Linked Issues check

Explanation

The implementation satisfies the main coding requirements in #549. getCanonXml() canonicalizes a final DOM Node with inclusive C14N. The integration coverage includes an enveloped-signature-only reference and verifies the signature. The two canonicalization tests expect <y></y>. The required explanatory note is still missing. The incremental diffs for both test files are empty, so the previous assertion-only changes remain unchanged.

• 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.}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.50%. Comparing base (f5c4d22) to head (5d14e3c).
⚠️ Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #581      +/-   ##
==========================================
+ Coverage   75.95%   78.50%   +2.55%     
==========================================
  Files           9        9              
  Lines        1048     1098      +50     
  Branches      273      280       +7     
==========================================
+ Hits          796      862      +66     
+ Misses        144      135       -9     
+ Partials      108      101       -7     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.