chore: deprecate signing and verifying a document that has a DOCTYPE by cjbarth · Pull Request #661 · node-saml/xml-crypto

cjbarth · 2026-10-13T18:00:27Z

Refs #524.

xml-crypto parses with xmldom, which does not process a document type declaration. XML 1.0 §5.1 requires even a non-validating processor to "include the replacement text of internal entities, and supply default attribute values", and Canonical XML §1.1 builds on that: "Character and parsed entity references are replaced" and "Default attributes are added to each element". So for a document whose DOCTYPE declares either, the canonical form xml-crypto signs or verifies is not the one the specifications define.

With xmldom 0.8.15 and xml-crypto 6.3.3:

Document	Canonical form
`<!DOCTYPE foo [<!ENTITY bar "admin">]><foo>&bar;</foo>`	`<foo>&bar;</foo>`
`<foo>&bar;</foo>`	`<foo>&bar;</foo>`
`<!DOCTYPE foo [<!ATTLIST foo role CDATA "admin">]><foo>x</foo>`	`<foo>x</foo>`

The first two documents mean different things and get the same digest. The third loses the default attribute.

7.0 will refuse a document that has a DOCTYPE (#524). This PR is the warning that comes first:

Warning: computeSignature() and checkSignature() print the DeprecationWarning XML_CRYPTO_DOCUMENT_TYPE_DECLARATION, once per process, when the document they are given has a DOCTYPE. Any DOCTYPE counts, with or without an internal subset. The document is signed or verified exactly as before.
README: a section under "Upgrading", "Documents with a DOCTYPE".

The warning is in those two methods because they are where xml-crypto parses a document itself. The canonicalization classes take a DOM the caller parsed, so what the parser did with the declaration is the caller's.

Tests

The warning has no test, like the other deprecations. One test is new, in test/document-type-declaration.spec.ts: a document whose DOCTYPE declares a default attribute is still signed and verified, and the attribute is not in what is signed. Nothing exercised a DOCTYPE through these two methods before.

Checks

npm run build && npm test && npm run lint pass (551 tests).

🤖 Generated with Claude Code

Summary by CodeRabbit

Documentation
- Documented that DOCTYPE declarations are not processed during signing or signature verification. Their entities and default attributes are not applied.
Deprecation
- Signing or verifying documents with a DOCTYPE now triggers a warning. This behavior is deprecated and is expected to throw an error in version 7.0.

xmldom does not process a document type declaration, so an entity or a default attribute it declares is missing from the canonical form, which XML 1.0 and Canonical XML require to include both. 7.0 will refuse such a document. Print a DeprecationWarning from computeSignature() and checkSignature() until then. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-13T18:00:57Z

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 49 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID: c0528c8c-bc21-484a-a9f5-4b34f611b242

📥 Commits

Reviewing files that changed from the base of the PR and between 89373d9 and 91e240d.

📒 Files selected for processing: 2

README.md
src/signed-xml.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 2b9164c5-9fef-4a09-be56-b49f587633f1

📥 Commits

Reviewing files that changed from the base of the PR and between f844ded and 89373d9.

📒 Files selected for processing: 2

README.md
src/signed-xml.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

checkSignature() and computeSignature() now emit a deprecation warning when the parsed document has a DOCTYPE declaration. The README documents that declarations are not processed and that signing or verification with a DOCTYPE will throw in version 7.0.

Changes: DOCTYPE Warning

Layer / File(s)	Summary
Signing and verification warning `src/signed-xml.ts`, `README.md`	`checkSignature()` and `computeSignature()` emit a warning when the parsed document has a DOCTYPE. The README describes the warning and notes that entities are not expanded and default attributes are not added.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Suggested reviewers: shunkica

Merge Risk: ⚪ Minimal · up to 89373

DOCTYPE inputs receive the intended deprecation warning, while signing and verification remain unchanged before version 7.0. No material merge risk remains.

Pre-merge checks: 5

Passed checks: 5 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: deprecating signing and verification for documents that contain a DOCTYPE.
Docstring Coverage		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 1…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

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

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

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

…ocuments-with-doctype

codecov · 2026-10-13T18:13:03Z

Codecov Report

❌ Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.05%. Comparing base (b4f1335) to head (91e240d).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/signed-xml.ts	20.00%	2 Missing and 2 partials ⚠️

❌ Your patch status has failed because the patch coverage (20.00%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #661      +/-   ##
==========================================
- Coverage   86.30%   86.05%   -0.25%     
==========================================
  Files           9        9              
  Lines        1336     1341       +5     
  Branches      344      346       +2     
==========================================
+ Hits         1153     1154       +1     
- Misses        104      106       +2     
- Partials       79       81       +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.

…ribute Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

cjbarth added this to the v6.4 milestone Oct 13, 2026

cjbarth added the deprecation Marks public API as deprecated; nothing is removed yet label Oct 13, 2026

cjbarth mentioned this pull request Oct 13, 2026

Xml-crypto doesnt properly handle dtd #524

Open

Merge remote-tracking branch 'upstream/master' into chore/deprecate-d…

b61aae8

…ocuments-with-doctype

cjbarth and others added 3 commits October 13, 2026 14:30

test: sign and verify a document whose DOCTYPE declares a default att…

a0f1886

…ribute Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

chore: drop the test of signing and verifying a document with a DOCTYPE

89373d9

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Merge branch 'master' into chore/deprecate-documents-with-doctype

91e240d

cjbarth merged commit 8a7e264 into node-saml:master Oct 13, 2026
11 of 13 checks passed

cjbarth deleted the chore/deprecate-documents-with-doctype branch October 13, 2026 23:20

cjbarth mentioned this pull request Oct 14, 2026

chore: merge master into 6.x #662

Merged

Navigation Menu

Repository navigation

chore: deprecate signing and verifying a document that has a DOCTYPE - #661

chore: deprecate signing and verifying a document that has a DOCTYPE#661
cjbarth merged 5 commits into
node-saml:masterfrom
cjbarth:chore/deprecate-documents-with-doctype

cjbarth commented Oct 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading

Review limit reached

Uh oh!

codecov Bot commented Oct 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

cjbarth commented Oct 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Checks

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

codecov Bot commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

cjbarth commented Oct 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 13, 2026 •

edited

Loading

codecov Bot commented Oct 13, 2026 •

edited

Loading