Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Properly check nodes before replacement #457

Merged
merged 1 commit into from Nov 3, 2022
Merged

fix: Properly check nodes before replacement #457

merged 1 commit into from Nov 3, 2022

Conversation

karfau
Copy link
Member

@karfau karfau commented Nov 3, 2022

which is slightly different from the checks required when inserting a node.

fixes #455 (only on the master branch for now, I want to test if this also takes care of #456)

https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity https://dom.spec.whatwg.org/#concept-node-replace

@karfau karfau merged commit 3bc6ccf into master Nov 3, 2022
@karfau karfau deleted the 455 branch November 3, 2022 07:53
@karfau karfau linked an issue Nov 4, 2022 that may be closed by this pull request
karfau added a commit that referenced this pull request Nov 5, 2022
which is slightly different from the checks required when inserting a node.
Also fixes the missing `Document.ownerDocument`, which prevented proper deletion of childNodes from `Document`.

fixes #455 (only on the master branch for now, I want to test if this
also takes care of #456)

https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity
https://dom.spec.whatwg.org/#concept-node-replace
karfau added a commit that referenced this pull request Nov 5, 2022
which is slightly different from the checks required when inserting a node.
Also fixes the missing `Document.ownerDocument`, which prevented proper deletion of childNodes from `Document`.
Also fixes properly disconnecting removed children from it's previous parent.

fixes #455 (only on the master branch for now, I want to test if this
also takes care of #456)

https://dom.spec.whatwg.org/#concept-node-ensure-pre-insertion-validity
https://dom.spec.whatwg.org/#concept-node-replace
mastermatt added a commit to mastermatt/samlify that referenced this pull request Feb 14, 2023
`@xmldom/xmldom` 0.8.6 included: fix: Properly check nodes before replacement.
xmldom/xmldom#457

Which caused the `.replaceChild` calls in `encryptAssertion` and `decryptAssertion` to error with:
> "Not found: child not in parent"

The root cause is a subtle difference in `@xmldom/xmldom` `Document` vs `Element` instances.
The previous code was asking xmldom to replace an element from a top-level Document with another top-level Document. The patch to xmldom 0.8.6 started the enforcement of `.replaceChild` being passed `Nodes` who share a common parent Node.

e.g. in the case of `encryptAssertion`: `doc.replaceChild(encryptAssertionNode, rawAssertionNode)`.
It's important to distinguish that neither `doc` nor `encryptAssertionNode` are `Element` nodes, but instead `Document` Nodes. Meaning `doc` does _not_ refer to the `<samlp:Response>` node, but instead a meta object one level up. To reference the Response tag, you instead use the `Document#documentElement` attribute.
Changing that line to the following as the same intended affect using the correct node references.
`doc.documentElement.replaceChild(encryptAssertionNode.documentElement, rawAssertionNode)`
I renamed a few of the variables in an attempt to clarify which are `Documents`.

Also fixes an issue where the `ERR_NO_ASSERTION` and `ERR_UNDEFINED_ENCRYPTED_ASSERTION` errors were not being thrown if exactly zero nodes were found.

fixes: tngan#495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document doesn't track lost child Document.replaceChild fails
1 participant