RDMR-1452 - Apply every element of a multi-element XML fragment - #39
Merged
Merged
Conversation
usernuno
requested review from
Chuckytuh,
EiyuuZack,
OS-kepatotorica,
OS-ruialves,
benmccarty91 and
trevor-lambert
September 21, 2026 09:02
injectFragment applied only the first element of a fragment holding more than one, then exited with code 0 and reported the file as updated. An inject entry listing two permissions wrote one, and the build passed, so the app shipped without the second permission. The cause is the @xmldom/xmldom bump from 0.7.5 to 0.8.15. Version 0.8.4 stopped inserting DOM nodes that are not well-formed (CVE-2022-39353). A document has exactly one root element, so xmldom converts content past the first root into text and escapes its < and > characters. Version 0.9 throws instead of converting. That reading of the spec is correct and this commit keeps it. The defect is that trampoline handed a fragment to a document parser, which 0.7 tolerated. The previous commit described the mechanism correctly and drew the wrong conclusion from it. It filtered out the text following the first element as "the raw source a second root degraded to". The text is that, and it is also the caller's remaining elements, so the filter removed the evidence of the loss rather than the loss itself. parseXmlFragment now wraps the fragment in one temporary root before parsing. Every top-level node then arrives inside a well-formed document, and the parser never inserts anything that is not well-formed. A DOM fragment parser does the same thing with a context element, and xmldom exposes no such API. The wrapped form also parses on 0.9.12, where the bare fragment throws HierarchyRequestError, so the fix does not depend on 0.8 being lenient. parseXmlFragment strips a leading declaration before wrapping. A fragment pasted in from a whole file would otherwise carry its <?xml ?> inside the wrapper, where it is neither well-formed nor printable, and the run failed in the formatter at commit time rather than where the fragment was written. _mergeJson no longer assumes both trees have children. An element written with no children has no elements array at all, so merging into a self-closing <application /> threw "target.elements is not iterable". That bug predates the bump, and this commit fixes it because it needs the same guard. Checked against the previous build. A multi-element inject now applies every element in document order. Comments and CDATA survive in place. A fragment carrying a declaration commits. Merging into a childless element works. Merge still de-duplicates by name and attributes. This commit does not change setAttrs, deleteNodes, deleteAttributes or replaceFragment. References https://outsystemsrd.atlassian.net/browse/RDMR-1452
usernuno
force-pushed
the
fix/RDMR-1452/xml-related-fixes
branch
from
September 21, 2026 09:03
a8437d6 to
30ae8d4
Compare
benmccarty91
approved these changes
Sep 21, 2026
Chuckytuh
approved these changes
Sep 21, 2026
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.
Description
injectFragmentapplied only the first element of a fragment holding more than one, then exited with code 0 and reported the file as updated. Aninjectentry listing two permissions wrote one, and the build passed, so the app shipped without the second permission.Cause
The
@xmldom/xmldombump from 0.7.5 to 0.8.15. Version 0.8.4 stopped inserting DOM nodes that are not well-formed (CVE-2022-39353). A document has exactly one root element, so xmldom converts content past the first root into text and escapes its<and>characters. Version 0.9 throws instead of converting. That reading of the spec is correct and this PR keeps it. The defect is that trampoline handed a fragment to a document parser, which 0.7 tolerated.The previous commit described the mechanism correctly and drew the wrong conclusion from it. It filtered out the text following the first element as "the raw source a second root degraded to". The text is that, and it is also the caller's remaining elements, so the filter removed the evidence of the loss rather than the loss itself.
Fix
parseXmlFragmentwraps the fragment in one temporary root before parsing. Every top-level node then arrives inside a well-formed document, and the parser never inserts anything that is not well-formed. A DOM fragment parser does the same thing with a context element, and xmldom exposes no such API. The wrapped form also parses on 0.9.12, where the bare fragment throwsHierarchyRequestError, so the fix does not depend on 0.8 being lenient.parseXmlFragmentstrips a leading declaration before wrapping. A fragment pasted in from a whole file would otherwise carry its<?xml ?>inside the wrapper, where it is neither well-formed nor printable, and the run failed in the formatter at commit time rather than where the fragment was written._mergeJsonno longer assumes both trees have children. An element written with no children has noelementsarray at all, so merging into a self-closing<application />threwtarget.elements is not iterable. That bug predates the bump, and this PR fixes it because it needs the same guard.Checked against the previous build:
setAttrs,deleteNodes,deleteAttributesorreplaceFragment.References https://outsystemsrd.atlassian.net/browse/RDMR-1452
Change Type