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

fix: allow configuring the XSS protection behavior #1010

Conversation

dennissterzenbach
Copy link
Contributor

I tried different ways on how to implement this and thought about the ideas previously discussed in the issue #846.
After some fiddling around I decided to keep things quite simple here, to get this off and rolling for everyone.

The basic concept includes the following:

  • keep util.xss() usages unchanged
  • allow to disable xss protection completely
  • allow to control xss library's behavior using its public interfaces

Note: This still means, that the current default behavior will remain unchanged, but users will be able to gain control over it and have alternative options. See below.

To achieve things I basically did the following:

  1. replaced the xss export in util.js with a dynamic getter using Object.defineProperty, because this allows to exchange the XSS protection algorithms behind it, while keeping that an internal detail

  2. add configuration options to Timeline, which allow to:
    a) disable XSS protection completely and
    b) optionally also change the xss library's behavior using its XSS.IFilterXSSOptions

  3. hand these configuration options to a setup function at instanciation time, to define how that particular Timeline instance will behave with regards to XSS filtering

  4. if someone disabled the xss protection, that makes Timelien log a warning, because... the protection is in for good reason, so why should you disable it completely (other than you know what you are doing)!?

The "disabled" must be set explictly, otherwise the xss protection will remain in place.
The protection also defaults to the current state, using the xss filter with no option at all.

This solution might not be perfect, because the dynamic getter could impact performance; also this solution does not account for treating XSS differently for loading screen, item contents, groups etc.

However I think this is at least a good intermediate solution. This might also be worth shipping, because it is a relief for both: us and the people out there, as they now have the option to get control over the XSS protection.

Here's the examples I used to verify its behavior:

With Options set to:

  ...
  xss: {
    disabled: false,
    filterOptions: {
      whiteList: { p: ['class', 'data-from-template'], div: 'class' },
    },
  },

CASE 1:

  template: function (item, element, data) {
    return `<div class="not-xss-filtered-html" data-from-template="yes">${item.content}</div>`;
  }

output keeps the element, but only the class attribute passes through:

<div class="not-xss-filtered-html">This is the item.content</div>

CASE 2:

  template: function (item, element, data) {
    return `<p class="not-xss-filtered-html" data-from-template="yes">${item.content}</p>`;
  }

output keeps element and both attributes:

<p class="not-xss-filtered-html" data-from-template="yes">This is the item.content</p>

CASE 3:

  template: function (item, element, data) {
    return `<span class="not-xss-filtered-html" data-from-template="yes">${item.content}</span>`;
  }

output is esacped, because span is no valid element:

&lt;span class="not-xss-filtered-html" data-from-template="yes"&gt;This is the item.content&lt;/span&gt;

With Options set to:

  ...
  xss: {
    disabled: true,
  },

The startup of Timeline produces a console warning:

You disabled XSS protection for vis-Timeline. I sure hope you know what you're doing!

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.

Wow! Thanks for the PR. this is definitely better solution than the current application.
Thanks for the contribution of code!

@yotamberk yotamberk merged commit 47b8bed into visjs:master Apr 10, 2021
@vis-bot
Copy link
Collaborator

vis-bot commented Apr 10, 2021

🎉 This PR is included in version 7.4.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jladbury
Copy link

Adding the xss option via setOptions does not seem to work. Is that intentional?

Background: hitherto I have initialised a timeline like this:
timeline = new vis.Timeline(document.getElementById(presentation.name))
timeline.setOptions(visOptions);
timeline.setItems(data.items);

Doing this, even when xss was set in visOptions, it did not take effect.
I can circumvent this by initialising the timeline as follows:
timeline = new vis.Timeline(document.getElementById(presentation.name),{},visOptions);

@melloware
Copy link
Contributor

Also nice to have is we should have a disableWarnings boolean option to turn off all warnings generated by the Timeline. If I want to disable XSS entirely i don't want my user's panicking that they see the alert You disabled XSS protection for vis-Timeline. I sure hope you know what you're doing! but I know exactly why I am disabling it.

@dennissterzenbach dennissterzenbach deleted the allow-users-to-configure-xss-protection branch April 13, 2021 17:35
@dennissterzenbach
Copy link
Contributor Author

Yes, it is intended to only accept manipulating the xss protection upon instantiation.
This is to avoid side effects you could cause by overriding the protection via setOptions.

srowen pushed a commit to apache/spark that referenced this pull request Jun 24, 2023
### What changes were proposed in this pull request?
Upgrade vis timeline to 7.7.2
Have to add xss option with whitelisting to make the timeline work after the xss protection was added in vis-timeline.
(Refer to visjs/vis-timeline#1010)

### Why are the changes needed?
To remediate CVE-2020-28487
GHSA-9mrv-456v-pf22

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Manually by running spark-shell and checking History Server UI.
Timeline rendered successfully and no change in style.
Even after following operation:
(1 to 1000).foreach(_ => sc.parallelize(1 to 10).collect)
UI loaded in 3 seconds faster than it loaded with 4.21.

Closes #41613 from shrprasa/upgrade_vis.

Authored-by: Shrikant Prasad <shrprasa@visa.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
@daattali
Copy link
Contributor

daattali commented Dec 2, 2023

The documentation for the xss feature does not mention that it can only be set during initialization. It should be mentioned.

senthh pushed a commit to acceldata-io/spark3 that referenced this pull request Sep 2, 2024
…020-28487

### What changes were proposed in this pull request?
Upgrade vis timeline to 7.7.2
Have to add xss option with whitelisting to make the timeline work after the xss protection was added in vis-timeline.
(Refer to visjs/vis-timeline#1010)

### Why are the changes needed?
To remediate CVE-2020-28487
GHSA-9mrv-456v-pf22

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Manually by running spark-shell and checking History Server UI.
Timeline rendered successfully and no change in style.
Even after following operation:
(1 to 1000).foreach(_ => sc.parallelize(1 to 10).collect)
UI loaded in 3 seconds faster than it loaded with 4.21.

Closes apache#41613 from shrprasa/upgrade_vis.

Authored-by: Shrikant Prasad <shrprasa@visa.com>
Signed-off-by: Sean Owen <srowen@gmail.com>

(cherry picked from commit a8ea35f)
senthh pushed a commit to acceldata-io/spark3 that referenced this pull request Nov 12, 2024
…020-28487

### What changes were proposed in this pull request?
Upgrade vis timeline to 7.7.2
Have to add xss option with whitelisting to make the timeline work after the xss protection was added in vis-timeline.
(Refer to visjs/vis-timeline#1010)

### Why are the changes needed?
To remediate CVE-2020-28487
GHSA-9mrv-456v-pf22

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Manually by running spark-shell and checking History Server UI.
Timeline rendered successfully and no change in style.
Even after following operation:
(1 to 1000).foreach(_ => sc.parallelize(1 to 10).collect)
UI loaded in 3 seconds faster than it loaded with 4.21.

Closes apache#41613 from shrprasa/upgrade_vis.

Authored-by: Shrikant Prasad <shrprasa@visa.com>
Signed-off-by: Sean Owen <srowen@gmail.com>

(cherry picked from commit a8ea35f)
senthh pushed a commit to acceldata-io/spark3 that referenced this pull request Nov 13, 2024
…020-28487

### What changes were proposed in this pull request?
Upgrade vis timeline to 7.7.2
Have to add xss option with whitelisting to make the timeline work after the xss protection was added in vis-timeline.
(Refer to visjs/vis-timeline#1010)

### Why are the changes needed?
To remediate CVE-2020-28487
GHSA-9mrv-456v-pf22

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Manually by running spark-shell and checking History Server UI.
Timeline rendered successfully and no change in style.
Even after following operation:
(1 to 1000).foreach(_ => sc.parallelize(1 to 10).collect)
UI loaded in 3 seconds faster than it loaded with 4.21.

Closes apache#41613 from shrprasa/upgrade_vis.

Authored-by: Shrikant Prasad <shrprasa@visa.com>
Signed-off-by: Sean Owen <srowen@gmail.com>

(cherry picked from commit a8ea35f)
senthh pushed a commit to acceldata-io/spark3 that referenced this pull request Nov 13, 2024
…020-28487

### What changes were proposed in this pull request?
Upgrade vis timeline to 7.7.2
Have to add xss option with whitelisting to make the timeline work after the xss protection was added in vis-timeline.
(Refer to visjs/vis-timeline#1010)

### Why are the changes needed?
To remediate CVE-2020-28487
GHSA-9mrv-456v-pf22

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Manually by running spark-shell and checking History Server UI.
Timeline rendered successfully and no change in style.
Even after following operation:
(1 to 1000).foreach(_ => sc.parallelize(1 to 10).collect)
UI loaded in 3 seconds faster than it loaded with 4.21.

Closes apache#41613 from shrprasa/upgrade_vis.

Authored-by: Shrikant Prasad <shrprasa@visa.com>
Signed-off-by: Sean Owen <srowen@gmail.com>

(cherry picked from commit a8ea35f)
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.

6 participants