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

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

Merged
merged 1 commit into from
Jan 18, 2024

Conversation

HugoPoi
Copy link
Contributor

@HugoPoi HugoPoi commented Jan 17, 2024

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

  • Change the way of displaying a element in Happydom
  • Remove the display of the element in the warn

Tasks

  • Could someone give me some context when this fallback function is called
    fallback value for <b>by SMS</b> when calling DOMPurify.sanitize('The patient will receive a link <b>by SMS</b> to join the video consultation') so why <b>by SMS</b> will be an element considered as a fallback ?

DOMPurify/src/purify.js

Lines 114 to 120 in 756cec3

const ElementPrototype = Element.prototype;
const cloneNode = lookupGetter(ElementPrototype, 'cloneNode');
const getNextSibling = lookupGetter(ElementPrototype, 'nextSibling');
const getChildNodes = lookupGetter(ElementPrototype, 'childNodes');
const getParentNode = lookupGetter(ElementPrototype, 'parentNode');

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
Copy link
Owner

cure53 commented Jan 17, 2024

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

#878
capricorn86/happy-dom#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
Copy link
Owner

cure53 commented Jan 17, 2024

@HugoPoi
Copy link
Contributor Author

HugoPoi commented Jan 18, 2024

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 Expose properties on Node prototype to support DOMPurify anti-clobbering capricorn86/happy-dom#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
Copy link
Owner

cure53 commented Jan 18, 2024

I hear you 😄 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
Copy link
Owner

cure53 commented Jan 18, 2024

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

console.warn(element) is displaying a lot stuff, in some context like HappyDom this display a lot stuff because a recursive console output of the entire Dom.
@cure53 cure53 merged commit a21a128 into cure53:main Jan 18, 2024
8 checks passed
@cure53
Copy link
Owner

cure53 commented Jan 18, 2024

Thanks 🙇

@cure53
Copy link
Owner

cure53 commented Jan 18, 2024

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

@vlad-borodaev
Copy link
Contributor

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.

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.

3 participants