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

Return removed elements after sanitize call #888

Closed
xleepy opened this issue Dec 14, 2023 · 10 comments
Closed

Return removed elements after sanitize call #888

xleepy opened this issue Dec 14, 2023 · 10 comments

Comments

@xleepy
Copy link

xleepy commented Dec 14, 2023

Hi,
Is there better approach than overriding push https://stackoverflow.com/a/5100420 or using uponSanitizeElement hook to get removed elements after sanitize call?

const result = DOMPurify.sanitize(html, {
      FORCE_BODY: true,
      ADD_TAGS: ['script']
    });

With this config removed will be always in array but i need to get all scripts before html rendered and store them per component.

uponSanitizeElement can't be used because hook will fire globally and will update other elements as well

@cure53
Copy link
Owner

cure53 commented Dec 14, 2023

Hmm, not sure if I understand the use case fully. What is the problem with using uponSanitizeElement and how would overriding Array.push which we use to populate the DOMPurify.removed array make things different?

@xleepy
Copy link
Author

xleepy commented Dec 14, 2023

My idea to have something like

const { html, removed } = DOMPurify.sanitize(html, {
      FORCE_BODY: true,
      ADD_TAGS: ['script']
});

to check and return removed and sanitized elements in one component scope. Right now i'm trying to use Map where i collect elements by specific component id but this solution far from ideal 😅.
And i rely on uponSanitizeElement right now but this hook can fire in other subscribed components as well. Also i know that i shouldn't rely on removed 😅 but i need to display notification to user if user really wants to run scripts.
I can prepare sandbox if it will help

@cure53
Copy link
Owner

cure53 commented Dec 14, 2023

I see, can you not make the hook check for the component scope and treat elements conditionally?

@xleepy
Copy link
Author

xleepy commented Dec 14, 2023

Codesandbox here example.
2 issues:

  1. In react env uponSanitizeElement hook will not fire in any of effects on first render (this is solvable)
  2. In component scope we cannot say in which component sanitize really happened and in which component instance hook called

@cure53
Copy link
Owner

cure53 commented Dec 14, 2023

Ah, I see. And if we had a hook registered inside _forceRemove your problem would be solved? Like, somewhere here?

  const _forceRemove = function (node) {
    arrayPush(DOMPurify.removed, { element: node });

    try {
      // eslint-disable-next-line unicorn/prefer-dom-node-remove
      node.parentNode.removeChild(node);
    } catch (_) {
      node.remove();
    }
  };

@xleepy
Copy link
Author

xleepy commented Dec 14, 2023

Yep, i think so. at least we can give a try 😅

@cure53
Copy link
Owner

cure53 commented Dec 14, 2023

I am not against adding this, if you can confirm that it works and where exactly the hook should be placed in this method, I can add it. Or a PR from you 🙂

@xleepy
Copy link
Author

xleepy commented Dec 14, 2023

Ok, will fork project and give a try on local build. Thanks! Will let you know after

@xleepy
Copy link
Author

xleepy commented Dec 15, 2023

Due to some policies i cannot work on this by myself. Discovered yesterday 😅. And we will need some key for this hook as well otherwise removeNode hook will be called for each subscription as well. So it will be

const result = DOMPurify.sanitize(hmtl, {key: `some-unique-key`})

and then in hook

DOMPurify.addHook(`removeNode`,(node, data) => {
 if(data.key === 'some-unique-key') {
    ....do some logic here
 }
})

but not sure if you will like this change 😅
. Also i found another solution to just show notification globally and for now it will be fine 😅. So we can close this issue

@cure53
Copy link
Owner

cure53 commented Dec 15, 2023

Oh, haha - okay :) Will do that now, let me know if we can do anything.

@cure53 cure53 closed this as completed Dec 15, 2023
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

No branches or pull requests

2 participants