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

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.

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

@eunjae-lee eunjae-lee left a comment

Choose a reason for hiding this comment

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

It totally makes sense 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants