Document or fix possible XSS vulnerability (via jquery)

[JesseDahl]

bootbox.confirm and alert use jquery's .html() (and other functions) that add content to html elements. These are a potential XSS security issue since jquery evaluates the content.

Here's a working example (scroll down to the bottom of the JS window for the example code, I just borrowed somebody's fiddle and modified)
https://jsfiddle.net/93sk1zeh/2/

Pass in the following string to the text input field

<script>alert('HELLO WORLD')</script>

it should show 3 separate alert boxes (which verifies it can potentially be used for XSS attacks).

I think there's two options:

1. Sanitize input before adding it to a DOM element using jquery, or build up the element in a safe manner (i'm not 100% sure the right way to do that just yet tbh)
2. Mention in the documentation the potential danger of passing in user-submitted data as the first parameter to bootbox.confirm() and bootbox.alert(), or, if using an object instead of a string message, as the title property. This way it's clear the library user is responsible for sanitizing any input that might be used with bootbox.

[tiesont]

I suppose I can see your point, although taking input from a user and then (safely) using it in a call to Bootbox as dialog content seems like something that's the responsibility of the developer, not this library.

I'll look into whether it's relatively easy to add something to sanitize message and title content, but I a) really don't want to write that myself, and b) really dislike adding another dependency to Bootbox.

[JesseDahl]

Yeah I get that.

Maybe just a quick note in the docs about sanitizing your inputs before passing it in to bootbox?

[tiesont]

The bootboxjs website is generated from the gh-pages branch, so if you would like to take a crack at that, I'd be happy to review a pull-request. Otherwise, it may be a little while before I get to something like that - what little free-time I have to work on projects like this has been spent trying to get the v5x branch ready for becoming the master branch.

[JesseDahl]

oh yeah, didn't know the docs were on github too. i'll take a crack at it.

[yonjah]

I'm not sure if this is very hard to fix but I think the current implementation is useful in many cases.
So from a user perspective I want to still be able to show a dialog with HTML elements inside.

From implementation perspective all that needs to be done is to use .text method instead of .html
And I think you can add a constructor called like Bootbox.Unsafe that if provided for message or title will be entered into the DOM using the html method. So unless user wraps the strings in this object explicitly the data will be parsed as text only.

If it sounds reasonable I can probably try to submit a pull request

[tiesont]

@yonjah I'm not entirely sure I understand what you're suggesting.

I won't be changing the internals to use text() instead of html() - that would break most of the existing functionality, since the message and title options have always allowed HTML.

I'd have to reiterate my previous statement: sanitizing content seems out of scope for this plugin. If I can get 5.0 squared away and pushed to master, maybe I could think about adding an option to allow a sanitizer to be passed in - that would allow devs to sanitize content, without having another large chunk of code for me to maintain.

[yonjah]

@tiesont Using text() method by default seem like the best fix to this problem.
But it is indeed a breaking change.

I wouldn't bother with implementing any solution that can't be activated by default since users will need to know to activate it. So it's not much better than changing the documentation and let users decide how they want to handle this issue.

[yonjah]

BTW it seem like SweetAlert decided to solve this issue by offering a content property
Which will contain an actual element to be injected into the dialog -
t4t5/sweetalert#845
It's also have the extra benefit of allowing easier integration with UI frameworks

[vedmant]

This makes problem as this package is reported as not secure by https://snyk.io/vuln/SNYK-JS-BOOTBOX-174704 . So there is no way to fix it without breaking existing projects? Then maybe major version bump?