[Feedback] react octicons should avoid`dangerouslySetInnerHtml`

Describe the topic

When consuming in an environment that requires trusted types these icons become unusable, since 'sinks' like innerHTML are limited to only consuming trusted types, every instance of an icons usage becomes an error

In order to support usage in more secure environments, moving these components to apis that utilize react children instead of 'dangerouslySetInnerHtml' is required. (Well, not necessarily required, as every icon could also accept a trusted type creator to wrap these calls to, or consumer one via context, but this would incur the performance hit of reading and running some trusted type creation on every render of the icon, where consuming children instead would not incur these penalties)

Anything else?

I think there might be a low-overhead path to just pre-evaluating the 'path' html strings and writing them as jsx to be passed as children from svgHeightMaps at build time, but didn't have time to dig into it much

octicons/lib/octicons_react/script/build.js

Lines 20 to 21 in 6014f5d

    
             const svgDataByHeight = ${JSON.stringify(octicon.heights)} 
        
             return <svg {...getSvgProps({...props, svgDataByHeight})} />

Something here where JSON.stringify(octicon.heights) becomes more like a

JSON.strinfigy(Object.fromEntries(Object.entries(octicon.heights).map(([key, config]) => {
   const {path, ...restConfig} = config
   return [key, {...restConfig, children: convertToJsx(path)}
})))

and then getSvgProps is modified to return children: children instead of dangerously....: path

While we're changing how octicons work partly for performance reasons, would it make sense to consider adopting some of the ideas from https://github.com/github/primer/issues/424 (i.e. rendering a spritesheet and having individual octicons consist of just a <use href="..."> call)?

@lesliecdubs - how do you feel about bringing this into PRC planning?

@rezrah I'm open to that as this seems like a reasonable suggestion, but I'd first like to understand a bit more about the use case.

@mattcosta7 can you tell us a bit more about where you're looking to use Octicons that requires trusted types? Is there a particular project or part of the product where this is needed on a particular timeline?

@rezrah I'm open to that as this seems like a reasonable suggestion, but I'd first like to understand a bit more about the use case.

@mattcosta7 can you tell us a bit more about where you're looking to use Octicons that requires trusted types? Is there a particular project or part of the product where this is needed on a particular timeline?

https://github.com/github/react-lang/issues/52 which came about from https://github.com/github/pse-architecture/issues/659 contain a good bit of information about the usecases

@KyFaSt might have a better idea on what timeline might look like for some of these, but since octicons are used internally in primer - there's a pretty firm dependency on octicons being updated to avoid dangerous html before any of that work could land

Thank you for the context, this is helpful!

I'm adding this back to the Primer React inbox for further review.

Thanks @mattcosta7 for looping me in here.

Hi @lesliecdubs trusted types is my team's next prioritized work but we want to work with frontend developers so that may look like enabling the trusted types directive as report-only, or "allowlisting" these existing sinks until we can catch all of them. I'm afraid that's not really a definitive timeline, but does it help with your planning and work?

Thanks for the additional info, @KyFaSt! We'll take a look at sizing the effort of this change at our next team sync and get back to you.

@KyFaSt we reviewed this and were wondering if the changes seemed feasible for someone on your team to contribute and we'd be happy to review? Let us know how we can support you in this!

	const svgDataByHeight = ${JSON.stringify(octicon.heights)}
	return <svg {...getSvgProps({...props, svgDataByHeight})} />