xml: check a fragment against the prefixes its ancestors declare - #312
Merged
Conversation
Four parses were exempt from the namespace rule for the same reason: they read
bytes lifted out of a larger document, whose prefixes are bound by declarations
that stayed behind on an ancestor. Holding them to the part-level rule would
have rejected valid content, so they were excused entirely.
They no longer need to be. CheckNamespacePrefixesInScope takes the prefixes an
ancestor declares and seeds them as bound; everything else is checked as before.
The in-scope set is prefixes rather than URIs because that is what the decoder
leaves visible — a bound prefix arrives as its URI, an unbound one as the prefix
string itself, and a fragment's inherited prefixes are the latter.
Two of the four turned out not to need it. CT_Scenarios.parse and
CT_OleObjects.parse rebuild their element with encodeUnknownElement and passed
nil where the sibling call one line above passes the root's prefix map, so they
were discarding declarations already in hand; they now pass it, and take the
in-scope set as well because encodeUnknownElement declares only what the start
tag uses while the inner content is carried across as raw bytes.
The pivot-caches fragment is the case that could not be fixed any other way:
<pivotCaches><pivotCache cacheId="1" r:id="rId5"/></pivotCaches>
r: is used on a child, so no reconstruction declares it, and the bytes are
re-emitted verbatim — giving them the missing declaration would change what an
untouched round trip writes.
The exemption list drops from thirteen to nine. What is left is six deep copies
of this library's own marshal output, the crypto descriptor, a validation probe
that reports decodability as a bool, and one synthesized wrapper: it is
self-contained for w:, but content using another prefix would silently stop
parsing, and no gate would catch that, so a real check is not traded for a
theoretical one.
Tests hold the line the change could have crossed: an unrelated in-scope prefix
does not excuse an unbound one, an empty set behaves exactly like the strict
check, and a fragment with trailing content is still refused.
mgilbir
marked this pull request as ready for review
August 19, 2026 10:00
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
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.
Closes the fragment half of the parse-entry-point exemptions. 13 → 9.
The problem
Four parses were exempt from the namespace rule for one reason: they read bytes lifted out of a larger document, whose prefixes are bound by declarations that stayed behind on an ancestor. The part-level rule would reject valid content, so they were excused entirely.
The API had to be prefixes, not URIs
The check tracks declared URIs, because that is what the decoder leaves visible: a bound prefix arrives as its URI, an unbound one as the prefix string itself. A fragment's inherited prefixes are the latter — so an in-scope URI set would have been inert.
CheckNamespacePrefixesInScopeseeds in-scope prefixes.Two of the four did not need it
CT_Scenarios.parseandCT_OleObjects.parserebuild their element withencodeUnknownElementand passednil— where the sibling call one line above passes the root's prefix map. They were discarding declarations already in hand. They now pass it, and take the in-scope set, becauseencodeUnknownElementdeclares only what the start tag uses while inner content is carried across as raw bytes.One could not be fixed any other way
r:is used on a child, so no reconstruction declares it — and these bytes are re-emitted verbatim, so giving them the missing declaration would change what an untouched round trip writes. This is the case the in-scope set exists for.What remains exempt, and why it should
Six deep copies of this library's own marshal output; the crypto descriptor (not an OPC part); a validation probe that reports decodability as a bool; and one synthesized wrapper — self-contained for
w:, but content using another prefix would silently stop parsing and no gate would catch that, so a real check is not traded for a theoretical one.Verification
The tests hold the line this change could have crossed: an unrelated in-scope prefix does not excuse an unbound one, an empty set behaves exactly like the strict check, a self-contained fragment needs no help, and trailing content is still refused.
golangci-lintThe repo's own
maporderguard caught map-order iteration in the prefix collection on the way in; sorted.