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

sanitize template option for tooltip/popover plugins #28236

Merged
merged 3 commits into from Feb 13, 2019

Conversation

Projects
7 participants
@Johann-S
Copy link
Member

commented Feb 11, 2019

XSS was possible in the tooltip or popover data-template, data-content and data-title attributes.

Fixes CVE-2019-8331.

@Johann-S Johann-S added js v4 labels Feb 11, 2019

@Johann-S Johann-S requested a review from twbs/js-review as a code owner Feb 11, 2019

@XhmikosR XhmikosR added this to Inbox in v4.3.1 via automation Feb 11, 2019

@Johann-S Johann-S force-pushed the v4-dev-jo-sanitize branch from 3b200ed to 5b074dd Feb 11, 2019

Show resolved Hide resolved js/src/tooltip.js Outdated
Show resolved Hide resolved js/src/tooltip.js Outdated
Show resolved Hide resolved site/docs/4.3/components/popovers.md Outdated
Show resolved Hide resolved js/src/tools/sanitizer.js
Show resolved Hide resolved js/src/tools/sanitizer.js Outdated
Show resolved Hide resolved js/src/tools/sanitizer.js Outdated
Show resolved Hide resolved js/src/tooltip.js Outdated
@XhmikosR

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

@markcarver: you need to tone down the discussion.

@Johann-S Johann-S force-pushed the v4-dev-jo-sanitize branch from cb1299a to 50b9795 Feb 12, 2019

@Johann-S

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

Ok things I have to do:

  • Update the docs
  • Sanitize title and content if html is at true
  • Not allowing sanitize to be set by data attributes in HTML

And I think it'll be good, do not hesite @markcarver if you have any feedbacks, I think I heard you 👍

@Johann-S Johann-S force-pushed the v4-dev-jo-sanitize branch 2 times, most recently from ae35b82 to e780c0a Feb 12, 2019

@markcarver

This comment has been minimized.

Copy link

commented Feb 12, 2019

I think the title option still needs to be sanitized, didn't see that in the latest code changes yet.

@markcarver

This comment has been minimized.

Copy link

commented Feb 12, 2019

  • Not allowing sanitize to be set by data attributes in HTML

Not allowing sanitize or whiteList to be set by data attributes in HTML

@Johann-S

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

The title is sanitized by the same methods which sanitized content

@markcarver

This comment has been minimized.

Copy link

commented Feb 12, 2019

Ah, you're right. Forgot that BS4 added setElementContent.

@Johann-S Johann-S force-pushed the v4-dev-jo-sanitize branch 3 times, most recently from 2f8ab60 to e47107e Feb 12, 2019

@markcarver

This comment has been minimized.

Copy link

commented Feb 12, 2019

Just that tiny doc nit, but other than that I think this looks good!

@Johann-S++

Ty and amazing work!

@Johann-S Johann-S force-pushed the v4-dev-jo-sanitize branch from cbe3a32 to 0cd53fc Feb 12, 2019

@XhmikosR XhmikosR force-pushed the v4-dev-jo-sanitize branch 3 times, most recently from f62e91f to 1bcb21a Feb 12, 2019

@MaPePeR

This comment has been minimized.

Copy link

commented Jun 12, 2019

Is there any special reason why the style attribute is not present in the default attribute whitelist? Is there some security issue with it or can it be safely added to the whitelist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.