Alright, let's run some tests :)

It's not ready yet; this is still a draft. See my previous comment above

Oh, sorry, no worries.

I think it would be better to have it always call importNode if RETURN_DOM_IMPORT is set to true, so that it works as expected when set explicitly, but have it default to false when shadowroot is not in the ALLOWED_ATTR attribute, rather than overriding it and preventing importNode from getting called even when RETURN_DOM_IMPORT is true. Does that sound appropriate?

So, the goal of whatever we do here is to avoid having people configure themselves to be prone to XSS when they are not aware of it. No matter how we do it, this needs to be prio number one.

So, the goal of whatever we do here is to avoid having people configure themselves to be prone to XSS when they are not aware of it. No matter how we do it, this needs to be prio number one.

In that case, perhaps it would be better to discard the RETURN_DOM_IMPORT option entirely, in favor of forcing it to always importNode when the shadowroot attribute is allowed? It doesn't seem there's any benefit in allowing people to disable it when doing so necessarily opens a vulnerability if you have shadowroot allowed.

Hmmm, not sure how many people already use it. And not sure how many of those allow shadowroot :D

Of the people who are using it, there are two possible cases:

1. They allow shadowroot. Then removing the option fixes an XSS vulnerability for them that they likely weren't aware of.
2. They don't allow shadowroot. Then removing the option changes nothing and the config option is a no-op for them.

Thus, I don't see any issue with removing it.

I think you are right, let's nuke it.

Oh! I realize the reason it's likely there despite your concern about users unwittingly opening themselves to vulnerability. It's useful if all of the following conditions are true for your use case:

1. You have RETURN_DOM or RETURN_DOM_FRAGMENT enabled (obviously).
2. You allow the shadowroot attribute.
3. You are NOT appending the DOM node outputted by DOMPurify into any document. (This one is the one we forgot about.)
4. You want to improve performance by disabling the importNode call.

I still don't think it's worth keeping though, because it's a minor non-security benefit (a performance improvement for very large node trees) and only in very rare conditions (shadowroot allowed, not appending to any document). And removing it also has the security benefit of users not needing to worry about disabling it unwittingly.

But it's up to you; you know about web security and DOMPurify's goals better than I do. Let me know what you think.

Let's remove it for now and see if people complain. Less complexity is better in the long run.

Alright, all ready now. I've changed up a couple of the tests which used to pertain to DOM importing. Needless to say, review my changes to the tests especially carefully. I tried to ensure that my changes kept the purpose of each test--what possible issue each test was trying to prevent--the same.

Looks like there are merge conflicts now that you've accepted a PR from someone else and made some changes of your own in the meantime. I hope it's easier for you to take care of that from your end. If not, then I'm not sure how to handle it from my end, and I may need some help.

There should be nothing that conflicts with your changes afaics, just generated stuff

Alright, conflicts resolved, let's see what the tests say!

This looks great, thank you very much 🙇 😄