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

console warn fallback for <b> when run with Happydom #903

Closed HugoPoi closed 7 months ago

HugoPoi commented 7 months ago

Summary

console.warn(element) is displaying a lot of stuff, in some context like HappyDom this displaying a lot of stuff because a recursive console output of the entire Dom.

Background & Context

I was thinking of the other solutions

Tasks

https://github.com/cure53/DOMPurify/blob/756cec35240cefe261aab06be1e0a7a1e66a6b19/src/purify.js#L114-L120

Could be Happydom not implementing the same way the API for <b> ?

As now it seems Dompurify doesn't support Happydom so might be an upstream issue in Happydom itself.

cure53 commented 7 months ago

Hey there, thanks for sending over the PR. Indeed, we believe, this is an issue in Happydom itself, here is more context:

https://github.com/cure53/DOMPurify/pull/878 https://github.com/capricorn86/happy-dom/issues/1165

I am hesitant to accept the PR since working with outerHTML doesn't really fix this and also adds additional complications, such as for elements that don't have an outerHTML etc.

I am not sure how active Happydom is, but I guess the best fix would be to address the root cause.

cure53 commented 7 months ago

I am wondering, do we even need the console.warn here? @vlad-borodaev

https://github.com/cure53/DOMPurify/blame/756cec35240cefe261aab06be1e0a7a1e66a6b19/src/utils.js#L179

HugoPoi commented 7 months ago

Ok thanks a lot to respond so quickly ! After re-thinking this, we're facing 3 issues here that's are not directly related to Dompurify

  1. As you mention the fallback occur because Happydom doesn't implement some part of the API https://github.com/capricorn86/happy-dom/issues/1165
  2. Second I need to open issue in Happydom because console.log(element) lead to recursively printing earth, not sure we want that
  3. Using console is a side effect functionally speaking, we probably want to either throw or remove the warn or inject a logger ?
cure53 commented 7 months ago

I hear you :smile: I want to get rid of the console.warn call entirely as well, hence my ping to the one who added it.

Would you mind reshaping the PR so we nuke the whole console.warn call? Then we can take it from there.

cure53 commented 7 months ago

The linter doesn't love it yet, but likely easy to fix :sweat_smile:

cure53 commented 7 months ago

Thanks :bow:

cure53 commented 7 months ago

In case that works and keeps Happydom happy, we could consider a release soon.

vlad-borodaev commented 7 months ago

I am wondering, do we even need the console.warn here? @vlad-borodaev

https://github.com/cure53/DOMPurify/blame/756cec35240cefe261aab06be1e0a7a1e66a6b19/src/utils.js#L179

Hi everyone, sorry for missing the message. Thanks for the fix, I suppose as it was a fallback to fix the issue with nullable element for Angular Universal on the server side (for Angular 11-12 as I remember), we could definitely remove the console there.

Probably it would be better then to leave a TODO/FIXME-comment to later review this fallback, probably we need to process more cases there.