test: pin the hoisted ancestor namespace behaviour #541 fixed - #565
Closed
cjbarth wants to merge 1 commit into
Closed
test: pin the hoisted ancestor namespace behaviour #541 fixed#565cjbarth wants to merge 1 commit into
cjbarth wants to merge 1 commit into
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. #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 #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 #538, #541 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 59 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 |
Contributor
Author
|
Closed in favour of #572 — the same commits, opened from 🤖 Generated with Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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