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

feat: Introduce allowHTML option to allow people to disable injecting HTML into choices. #984

Merged
merged 9 commits into from
Dec 26, 2021

Conversation

mason-rogers
Copy link
Contributor

@mason-rogers mason-rogers commented Dec 23, 2021

Description

This is a continuation of #968 to address an XSS vulnerability when creating labels for choices.

Screenshots (if appropriate)

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

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
Copy link
Member

mtriff commented Dec 24, 2021

Awesome! Thanks for taking this up. There are a few test and linting errors that need to be resolved. Can you also add Cypress tests for the three states of allowHTML (true, false, undefined)?

@mason-rogers
Copy link
Contributor Author

Should be done 👍Let me know if there are any other changes you would like to be done.

Copy link
Member

@mtriff mtriff left a comment

Choose a reason for hiding this comment

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

Almost there, still a couple of linting issues. It also looks like we have an issue with custom templates. You can see in the demo page that the custom template example is no longer rendering correctly (there's a unit test failure that should point you in the right direction too).

cypress/integration/text.spec.ts Show resolved Hide resolved
Copy link
Member

@mtriff mtriff left a comment

Choose a reason for hiding this comment

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

This is excellent, one minor nitpick. Also, the callbackOnCreateTemplates advanced example in the README needs to be updated too. Then we are good to merge.

src/scripts/templates.ts Outdated Show resolved Hide resolved
@mtriff mtriff added breaking change Pull request that introduces a breaking change feature request labels Dec 26, 2021
@mason-rogers
Copy link
Contributor Author

All done! 👍

@mtriff
Copy link
Member

mtriff commented Dec 26, 2021

Excellent, thanks for this! I made some small changes to the wording around allowHTML and set allowHTML: false on our remote fetching examples (we should probably model good behaviour 😄).

@mtriff mtriff merged commit 0b6973b into Choices-js:master Dec 26, 2021
@mason-rogers mason-rogers deleted the feat/allowHTML branch December 26, 2021 18:12
@mason-rogers
Copy link
Contributor Author

Thanks for merging this, is there any chance this will be included in a release soon? Also, while I'm here I have a question regarding the typings because Im not able to import choices.js with typings through import Choices from 'choices.js or import { Choices } from 'choices.js'.

Seems like the typings file package.json points to isn't shipped with the module on npm so I can't make it work for me in a TS vue project.

@mtriff
Copy link
Member

mtriff commented Dec 26, 2021

I'd expect a release in the next week or two. There's at least one more breaking change I'd like to get merged first.

Thanks for flagging the types issue, looks like this was missed when the project was converted to TypeScript. It has been fixed on master (see #985). import Choices from 'choices.js' should work again.

@mason-rogers
Copy link
Contributor Author

Just had another go with the new version with the supposed typings fix - still no luck on my end.

Judging off of this diff, the npm module should ship with a public/types folder, which it doesn't seem to do.

image

@mtriff
Copy link
Member

mtriff commented Dec 27, 2021

Are you installing from the master branch?

npm install Choices-js/Choices#master

There won't be a new version pushed to npm until the next release.

@mason-rogers
Copy link
Contributor Author

Yes, that's the exact command I ran to install it (I also uninstalled the release version beforehand)

    "choices.js": "github:Choices-js/Choices#master",

This is the line in my package.json, and the contents of the module are the same as above still.

@mason-rogers
Copy link
Contributor Author

I've created a PR which fixes this issue: #986

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull request that introduces a breaking change feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants