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

improve xss security #840

Merged
merged 1 commit into from
Dec 20, 2020
Merged

improve xss security #840

merged 1 commit into from
Dec 20, 2020

Conversation

mojoaxel
Copy link
Member

I added the xss library to improve security.

fixes #838

@mojoaxel mojoaxel requested review from Thomaash and yotamberk and removed request for Thomaash December 19, 2020 21:48
Copy link
Member

@yotamberk yotamberk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some of the files util is not imported

lib/timeline/component/CustomTime.js Show resolved Hide resolved
@yotamberk yotamberk merged commit a7ca349 into master Dec 20, 2020
@yotamberk yotamberk deleted the xss-security branch December 20, 2020 13:42
@vis-bot
Copy link
Collaborator

vis-bot commented Dec 20, 2020

🎉 This PR is included in version 7.4.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Donov4n
Copy link

Donov4n commented Dec 20, 2020

Hello :)

Its seems that this PR has introduced a regression because the xss dep. has only been added
to devDeps and is therefore not available when consuming vis-timeline.

The error :

This dependency was not found:

* xss in ./node_modules/vis-timeline/esnext/esm/vis-timeline-graph2d.js

To install it, you can run: npm install --save xss

Should i open a new issue for this ?

@Thomaash
Copy link
Member

Hi there,

I know this is already out as a fix but this is a breaking change. XSS is basically nuking the HTML away with an extremely constrained set of exceptions. There are no forms, no classes, no styles etc. Also the thing has like 30K and for what? So that people can have the convenience of <h2>Heading</h2><p>Text…</p>? This doesn't seem worth it to me (though as always, this is up for discussion). If we really want to sanitize this and since we allow for HTMLElement to be used as a title, I would just use innerText for strings and changed the docs so that the people who want HTML in there would be instructed to pass HTMLElement (maybe using innerHTML there, if they choose to) instead of a string.

Another thing is that the configurator (which is except for reformatting identical to the code in Vis Network and really should be moved to Vis Util) doesn't really use HTML (apart from some simple wrapping, but we don't need innerHTML for that), it can be quite easily converted to innerText and no sanitation is necessary at all.

Donov4n added a commit to Robert-2/Robert2 that referenced this pull request Dec 20, 2020
Donov4n added a commit to Robert-2/Robert2 that referenced this pull request Dec 20, 2020
Donov4n added a commit to Robert-2/Robert2 that referenced this pull request Dec 20, 2020
@mojoaxel
Copy link
Member Author

@Donov4n Sorry!
@Thomaash Thanks!

@ckeboss
Copy link

ckeboss commented Jan 4, 2021

I agree with @Thomaash, shouldn't sanitization be up to the user?

For example, setting a tooltip template is now pretty useless, as you can not input any attributes to elements.

@ge0ffrey
Copy link

Solution to allow classes, styles etc in elements (based on the changes that they made resolving this issue)

const options = {
    xss: {disabled: true}
}

Be wary of XSS in this case, of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security Cleanup
7 participants