test: pin the hoisted ancestor namespace behaviour #541 fixed - #572
Conversation
Three branches were open against defects in the ancestor-namespace path of non-exclusive C14N: a hoisted ancestor default namespace not becoming the default its descendants were canonicalized against, an ancestor declaration being appended alongside one the node made itself, and `process()` marking ancestor prefixes in scope before anything had been rendered so the apex's own redeclaration was skipped as redundant. node-saml#541 fixed all three by a different route — `localDefaultNs`/`nodeDefaultNs` in `renderNs`, `newDefaultNs` seeded from the hoisted entry, and `findSubsetNSPrefixes` collecting every prefix the subset declares rather than the first. The fixes those branches carried are obsolete, so only their tests land here, parameterized over both non-exclusive canonicalizers. Each case fails against the implementation as it stood before node-saml#541 and passes now, so they are guards rather than documentation. Two spot checks confirm they bite: dropping the `newDefaultNs` assignment from the ancestor merge fails three of them, and reverting `findSubsetNSPrefixes` to the first declaration only fails another. The integration case is the one worth having. A hoisted default namespace leaking into a descendant leaves the signature verifying while `getSignedReferences()` reports an element in a namespace the sender never signed it in, and nothing else in the suite covers that. Refs node-saml#538, node-saml#541 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 20 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #572 +/- ##
==========================================
+ Coverage 75.95% 76.86% +0.91%
==========================================
Files 9 9
Lines 1048 1059 +11
Branches 273 275 +2
==========================================
+ Hits 796 814 +18
+ Misses 144 139 -5
+ Partials 108 106 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Refs #538, #541.
Three branches were open against defects in the ancestor-namespace path of non-exclusive C14N:
xmlns=""read as redundant and was dropped;xmlnsattributes on one element;process()marking ancestor prefixes in scope before anything had been rendered, so the apex's own redeclaration of such a prefix was skipped as redundant and resolved to the outer binding.All three are already fixed on
master. #541 got there by a different route —localDefaultNs/nodeDefaultNsinrenderNs,newDefaultNsseeded from the hoisted entry in the ancestor merge, andfindSubsetNSPrefixescollecting every prefix the subset declares rather than the first. So the fixes those branches carried are obsolete and are dropped.Their tests are not obsolete.
mastersatisfies these spec requirements without asserting any of them, in an area that has now produced four separate defects.These are guards, not documentation
Each of the six unit cases fails against
src/c14n-canonicalization.tsas it stood before #541 and passes onmaster. Two spot checks confirm they bite against plausible regressions:masternewDefaultNs = ancestorNamespace.namespaceURIfrom the ancestor mergefindSubsetNSPrefixesto the first declaration onlyThe cases are parameterized over both
C14nCanonicalizationandC14nCanonicalizationWithComments, which the original branches did not do.The integration case
The one worth having. A hoisted default namespace leaking into a descendant leaves the signature verifying while
getSignedReferences()— the API the README designates as the secure replacement forgetValidatedNode()— reports the element in a namespace the sender never signed it in. A caller that dispatches on element namespace then acts on an identity that was never signed, and the sender picks it. Nothing else in the suite covers that.Cited inline: C14N 1.0 §2.3, the §4.7 rationale for preserving
xmlns="", and Namespaces in XML §2.2 for prefix scoping.Milestone
v6.2, matching #541 — test-only, no behaviour change.
Verification
npm run build && npm test && npm run lintclean; 254 passing (241 + 13; 6 cases × 2 canonicalizers + 1 integration).🤖 Generated with Claude Code