canonical / react-components

A set of components based on Vanilla Framework
https://canonical.github.io/react-components
99 stars 55 forks source link

Unsanitized Inner HTML in Chip component #1091

Open jmuzina opened 4 months ago

jmuzina commented 4 months ago

I noticed when reviewing the [TICS report for react-components ](https://canonical.tiobe.com/tiobeweb/TICS/TqiDashboard.html#axes=ProjectGroup(publicRepo),Project(react-components),Sub()&metric=tqi) that there are some uses of dangerouslySetInnerHTML that were flagged as XSS vulnerabilities.

Flag 1,File(Path(HIE,react-components,master,src,components,Chip,Chip.tsx))&diff=false&metrics=Annotations(SECURITY)): Chip (src) Flag 2,File(Path(HIE,react-components,master,src,components,SearchAndFilter,FilterPanelSection,FilterPanelSection.tsx))&diff=false&metrics=Annotations(AI),Annotations(CS),Annotations(CW),Annotations(COMPLEXITY),Annotations(FANOUT),Annotations(SECURITY),Annotations(DUP),Annotations(UNITCOVERAGE)): 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: https://github.com/jmuzina/react-components/commit/f3371c6ae423dc5f80e4d10430c4c99c9b71a95f

bartaz commented 4 months ago

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: https://github.com/canonical/react-components/commit/1d160ee4b7dd3f3faa3784d9913135840f713055

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.