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

Unsanitized Inner HTML in Chip component #1091

Open
jmuzina opened this issue May 17, 2024 · 1 comment
Open

Unsanitized Inner HTML in Chip component #1091

jmuzina opened this issue May 17, 2024 · 1 comment
Labels
Question ❓ Further information is requested

Comments

@jmuzina
Copy link
Member

jmuzina commented May 17, 2024

I noticed when reviewing the TICS report for react-components that there are some uses of dangerouslySetInnerHTML that were flagged as XSS vulnerabilities.

Flag 1: Chip (src)
Flag 2: FilterPanelSection of Search and Filter (src)

Are these left here intentionally so that our users have the freedom to place whatever they like in the chips, and thus they have the responsibility to sanitize contents? Otherwise, we could use something like dompurify to sanitize the inner HTML, i.e:

import { sanitize } from 'dompurify'
// 
//
const el = ({ text }) => {
    return (
        <h3
              dangerouslySetInnerHTML={{
                __html: sanitize(text)
              }}
        />
    );
}

Here's what a change to fix this might look like: jmuzina@f3371c6

@jmuzina jmuzina added the Question ❓ Further information is requested label May 17, 2024
@bartaz
Copy link
Member

bartaz commented May 24, 2024

I looked into history and it seems that passing HTML directly into Chip was introduced here, where I think it was meant for selecting part of chip value as a search result:
1d160ee

This functionality seems to be gone from the component and it kind of looks like we don't need it. Both properties that build the Chip value (lead and value itself) are type of string, so it doesn't seem that we really want to allow any custom HTML children there (in which case we should accept React node rather than string anyway).

I guess we could try to search for Chip component usage and see what people pass as value there, but I feel like we could try to enforce using safe values and remove the dangerouslySetInnerHTML from this component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question ❓ Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants