cure53 / DOMPurify

DOMPurify - a DOM-only, super-fast, uber-tolerant XSS sanitizer for HTML, MathML and SVG. DOMPurify works with a secure default, but offers a lot of configurability and hooks. Demo:
https://cure53.de/purify
Other
13.61k stars 695 forks source link

Return removed elements after sanitize call #888

Closed xleepy closed 8 months ago

xleepy commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

xleepy commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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

cure53 commented 8 months ago

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 :slightly_smiling_face:

xleepy commented 8 months ago

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

xleepy commented 8 months ago

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 commented 8 months ago

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