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

Don't allow sanitize in BS popover #728

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

duzun
Copy link

@duzun duzun commented Jul 2, 2019

In BS v3.4.1 tooltips and popovers sanitize HTML by default, thus removing navigation buttons from the tour.

In [BS v3.4.1](https://github.com/twbs/bootstrap/blob/v3.4.1/js/popover.js#L51) tooltips and popovers sanitize HTML by default, thus removing navigation buttons from the tour.
@IGreatlyDislikeJavascript
Copy link

IGreatlyDislikeJavascript commented Jul 8, 2019

Sorry, I feel like I'm repeatedly posting the same thing on this issue. Turning off the sanitizer as per your patch will fix the issue, but will introduce a potential security vulnerability due to XSS etc depending on how your code works.

I strongly suggest that you change your patch to specifically whitelist the button elements, or use my fork which exposes options to do this for you.

See #729

@duzun
Copy link
Author

duzun commented Jul 8, 2019

My bad, I did not investigate the code thoroughly enough.
I thought the source of HTML is internal only, but now I see user can pass contents and the template of the popover through options.
In my project, I've just disabled BS sanitization globally, because of this issue.
I know what I'm doing and can afford this (a browser extension with strict CSP and the only source of HTML being my compiled JS).

But I can imagine other projects, where this issue arises and someone disables BS sanitization globally as a quick fix, with far more bad consequences than good ones.

IMHO it should work out of the box, at least with BS 3.4. And only sanitize user-supplied contents of templates, if not explicitly requested otherwise.

@ibastevan
Copy link

I would recommend using @IGreatlyDislikeJavascript fork It is working much better.

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.

None yet

4 participants