[cjbarth]

Summary

• Include document-level processing instructions in empty-URI reference digests during signing and verification.
• Canonicalize document children with the required processing-instruction newlines while excluding the XML declaration.
• Preserve existing element behavior for other references and transform inputs.
• Canonicalize document children in one pass to avoid quadratic work on untrusted PI-heavy XML.

Security impact

An empty-URI signature could verify after a document-level xml-stylesheet instruction was changed, because verification previously dereferenced only the document element. This issue remains at the merged head of #642 and is fixed here using library code only.

Regression and validation

The public-API regression test signs a document with a stylesheet instruction using the repository's test key, then changes only its stylesheet URL. Before the fix, the original signature verified and the tampered document also verified (expected true to be false). After the fix, tampering fails verification for both tested transform orders. Added-PI tampering also fails. All four canonicalizers match the C14N section 3.1 example.

• npm run build
• npm test -- --reporter dot --no-colors (550 passing)
• npm run lint
• git diff --check

Summary by CodeRabbit

• Bug Fixes
  • XML signatures now account for document-level processing instructions when signing and verifying empty-URI references.
  • Canonicalization handles whole documents and eligible document children, preserving processing instructions through supported transform sequences.
  • Processing-instruction formatting follows canonical XML spacing and line-break rules. Comment-preserving algorithms also retain eligible document-level comments, while doctype declarations are excluded.
• Documentation
  • Clarified how document-level processing instructions are handled in empty-URI references and custom transforms.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: dc6ba4f7-eb3b-4521-bbea-fc040de94035

📥 Commits

Reviewing files that changed from the base of the PR and between d16e04d and 28a02b4.

📒 Files selected for processing (1)

• src/utils.ts

💤 Files with no reviewable changes (1)

• src/utils.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Canonicalization now processes document nodes and their canonical children. For empty-URI references with document-level processing instructions, SignedXml uses and preserves the whole document through transform parsing.

Changes

Document-level processing instruction handling

Layer / File(s)	Summary
Document-node canonicalization src/utils.ts, src/c14n-canonicalization.ts, src/exclusive-canonicalization.ts, test/processing-instructions.spec.ts	Both canonicalizers process canonical document children and handle document-child comments. Exclusive canonicalization accepts Document inputs and uses the document element for ancestor namespace handling. Tests cover processing-instruction output for each algorithm.
Empty-URI reference pipeline src/signed-xml.ts, test/document-level-processing-instructions.spec.ts, README.md	For empty-URI references with document-level processing instructions, SignedXml selects and preserves the whole document through transform parsing. Tests cover both transform orders and changes to signed processing instructions. The README describes the reference behavior.

Priority: ➖ Normal

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

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant SignedXml
  participant Utils
  participant Canonicalizer
  SignedXml->>Utils: Check for a document-level processing instruction
  Utils-->>SignedXml: Return whether a non-XML instruction exists
  SignedXml->>Canonicalizer: Pass the Document for an affected empty-URI reference
  Canonicalizer->>Utils: Check which document children to canonicalize
  Canonicalizer-->>SignedXml: Return canonicalized document content

Loading

Merge Risk: ⚪ Minimal · up to 28a02

No actionable merge-blocking issue was identified in the document-level processing-instruction change. It is ready for normal merge checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 069cb

The change strengthens signature coverage for document-level processing instructions. The main remaining risk is compatibility: existing signatures containing these instructions and custom transforms that assume an element may require coordinated upgrades.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The directly affected security boundary is the content authenticated by each empty-URI reference in an XML document. Applications that consume document-level instructions after signature verification are downstream beneficiaries; their actual processing authority and deployment exposure are not established by the available source.

Security Findings and Attack Paths

• inferred — An attacker changing or inserting a stylesheet instruction previously left the empty-URI reference's selected element unchanged. The new flow includes that instruction in canonicalized reference bytes when transforms preserve it, causing digest comparison to reject the alteration. This supports a narrower authentication gap, not a claim that all downstream XML processing is safe.

Trust Boundaries and Controls

• observed — Reference bytes are not published as authenticated output until every reference digest validates and SignatureValue verification succeeds. The document-selection change feeds this existing authentication sequence rather than bypassing it.

Resilience and Maintainability Implications

• observed — Verification clears exposed authenticated-reference state before processing a new attempt. Digest failure, parsing interruption, or signature failure occurs before the current attempt is committed. Repeated successful checks append authenticated results under an unchanged cache policy.

Hardening Proposals

• proposed — Validate deployed custom transforms against Document input and PI preservation before migration, and plan handling for historical PI-bearing signatures without silently falling back to the earlier element-only verification behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ 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: including document-level processing instructions in empty-URI XML signatures.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

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 86.36%. Comparing base (24af0a7) to head (7bcc204).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #645      +/-   ##
==========================================
+ Coverage   85.20%   86.36%   +1.15%     
==========================================
  Files           9        9              
  Lines        1244     1283      +39     
  Branches      319      335      +16     
==========================================
+ Hits         1060     1108      +48     
+ Misses        111      100      -11     
- Partials       73       75       +2     

☔ 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.