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(refinementList): prevent XSS via routing #4344

Merged
merged 1 commit into from Mar 5, 2020

Conversation

@francoischalifour
Copy link
Member

francoischalifour commented Mar 5, 2020

There's a security issue with the refinementList widget when relying on its default template and routing.

Security issue demonstration →

Why this is an issue

Malicious users could inject some JavaScript code via the URL to get specific information. This is Cross-Site Scripting (XSS) vulnerability.

Why this happens

We use {{{highlighted}}} in the default item template of the refinementList widget. The triple braces in Hogan.js means that it doesn't escape the value, and uses the setInnerHTML method under the hood. Therefore, HTML can be injected. The issue comes from the fact that we allow users to select refinement list items from the URL, which means that it's not safe to use this method.

We use triple braces in the template because when SFFV (Searching For Facet Values), we highlight the matches with HTML tags. This specific case is not as dangerous because we don't synchronize the SFFV values in the URL.

This solution

We can switch to using {{highlighted}} by default. This means that HTML won't be interpreted. When we detect that we're coming from a search (props.isFromSearch in the RefinementList component), we can safely rely on {{{highlighted}}} because SFFV are not synced with the URL.

We therefore conditionally use the "dangerous" value when it cannot come from the URL.

Migration plan

This solution is the one that I think will be the less transparent for users. Very few (if not none) users must rely on facet values containing HTML and the triple brace Hogan syntax. If so, this fix is more important than considering this as a breaking change to speed up the adoption of the next minor version. We'll therefore be explicit about this security fix in the changelog if ever it breaks some apps, which again, is very unlikely.

Other considerations

Vue InstantSearch is vulnerable to the same attack. This happens because we rely on innerHTML in the ais-highlightcomponent.

We should be more careful with values generated from the URL. This will also be true with values injected in the UI state more generally. We should think about rejecting values that cannot happen, since you're not supposed to be able to generate facet values via the URL. This is out of scope for now.

We can delay the documentation for this new isFromSearch template property. We shouldn't wait for the documentation update in the templates section before releasing a new version.

@Haroenv
Haroenv approved these changes Mar 5, 2020
Copy link
Member

Haroenv left a comment

good test, good solution. I think we have the same issue in Vue & Angular (which always use highlight, and not only when from search)

@francoischalifour

This comment has been minimized.

Copy link
Member Author

francoischalifour commented Mar 5, 2020

@Haroenv As explained in the post, this happens in Vue InstantSearch, not in the other framework-based flavors.

Copy link
Contributor

eunjae-lee left a comment

It totally makes sense 👍

@francoischalifour francoischalifour merged commit 8552221 into master Mar 5, 2020
12 of 14 checks passed
12 of 14 checks passed
ci/circleci: e2e tests Your tests failed on CircleCI
Details
ci/circleci: type check js (optional) Your tests failed on CircleCI
Details
Header rules No header rules processed
Details
Pages changed 22 new files uploaded
Details
Mixed content No mixed content detected
Details
Redirect rules 3 redirect rules processed
Details
Semantic Pull Request ready to be squashed
Details
bundlesize Total bundle size is 213.94KB/223.5KB (-null)
Details
ci/circleci: build and size Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: release if needed Your tests passed on CircleCI!
Details
ci/circleci: type check algoliasearch v3 Your tests passed on CircleCI!
Details
ci/circleci: unit tests Your tests passed on CircleCI!
Details
deploy/netlify Deploy preview ready!
Details
@francoischalifour francoischalifour deleted the fix/refinement-list-xss branch Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.