fix: xss vulnerability in creating choice labels

[hrahman-miovision]

Description

This addresses a XSS vulnerability when creating labels for choices. For example, if we set the label property of a choice to the HTML string <img src=x onerror=alert()> like so:

var singleNoSearch = new Choices('#choices-single-no-search', {
          searchEnabled: false,
          removeItemButton: true,
          choices: [
            { value: 'One', label: '<img src=x onerror=alert()>' },
            { value: 'Two', label: 'Label Two', disabled: true },
            { value: 'Three', label: 'Label Three' },
          ],
        });

It will execute the JavaScript function defined in the onerror attribute since the src does not exist. An attacker can exploit this.

The issue was that when creating the choice / placeholder elements, it would set the innerHTML content of the element. Instead, since these are just string labels, we should be setting the content as a string using the innerText property.

Screenshots (if appropriate)

Choices executing JavaScript functions (alert()) defined in a label string.

Types of changes

• Chore (tooling change or documentation change)
• Refactor (non-breaking change which maintains existing functionality)
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[mtriff]

Thanks for the contribution, Hasib. However, this would be a breaking change for anyone using HTML in their choices (for icons, font styling, etc.) I understand this is a XSS vulnerability, but only if you are loading your choices from an untrusted/third-party source. That said, I think it's a good idea to have a default safe option for users. I'd propose the following:

1. Add an allowHTML option. If that option is true, set innerHTML. If it's false set innerText.
2. For now, default to allowHTML: true so that we aren't introducing a breaking change.
3. If allowHTML is not explicitly set, log a deprecation warning.

In a future release we'll change the default to allowHTML: false.

What do you think? Would you be able to implement this change?

[mason-rogers]

@mtriff I have created a new PR which completes these 3 things you have asked for. See #984

[mtriff]

Thanks for flagging this @hrahman-miovision, closing in favour of #984