Polaris-react: [Navigation Item] Render inline svg in an image tag

[theodoretan]

Problem

In the Item component svg strings are rendered inline with dangerouslySetInnerHTML. This can lead to an XSS vulnerability where if an svg with scripts is rendered on a page, they will be executed.

Solution

We can convert the svg string to a data URI and display it in an img tag where it doesn’t execute scripts when rendered.

I propose that the logic that implements this lives within the Icon component. This way other users that render svg strings can do so safely. The Icon component would have a color prop to recolour the image when the item’s state changes (hover/focus/selected).

Doing this also decouples Navigation Item from Icon as Item relies on the implementation Icon returning an svg tag to re-style. By passing in the fill colour instead of having these states in Navigation.scss, the icon that Item is rendering doesn’t break if it’s not an svg tag.

[probot-shopify[bot]]

👋 Thanks for opening your first issue. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

[theodoretan]

cc @ragalie

[nwtn]

The Icon component would have a color prop to recolour the image when the item’s state changes (hover/focus/selected).

Just a heads-up that this has the potential to send you down a rabbit-hole. Luckily, it’s one I’ve already explored and prepared you for. When icons are rendered inline in an img tag, CSS can’t be applied to elements within the SVG. This means that things like color styling for :hover, :focus, and :selected get complicated.

The best solution we’ve come up with so far is to apply CSS filters on the entire img the change the icon’s color when selected. polaris-tokens include filter values for all our supported colors to achieve this. More info in the README.

[kaelig]

if an svg with scripts is rendered on a page, they will be executed.

Is that something that can happen / has happened?

We can convert the svg string to a data URI and display it in an img tag where it doesn’t execute scripts when rendered.

Data URIs don't compress well. Just mentioning it so we can take this performance cost into account during the decision-making process.

[theodoretan]

Is that something that can happen / has happened?

Yes, the scrubber that sanitizes our sales channel icons has failed twice already and if we rely on this, we'll always be playing catch-up to new ways of XSS. More context: https://github.com/Shopify/partners/issues/7098

[kaelig]

Thanks - I wish this could be solved with a CSP directive that specifically declares scripts in inline SVGs are unsafe, but from a brief research, it seems like such fine-grained control isn't possible.

[theodoretan]

@kaelig Are we okay with changing how Icon handles SVGSource and let the Navigation Item use it instead of having dangerouslySetInnerHTML for user uploaded svgs?

[kaelig]

That would probably be a question for @tmlayton!

[kaelig]

I should have mentioned that Chrome’s handling of custom color profiles is buggy on hardware-accelerated elements (in this case, acceleration would be triggered by the presence of a CSS filter).

This means icons will have the right color in Safari and Firefox, but the resulting color will be wrong in Chrome.

For this reason, we may want to only use this technique in areas where source SVGs are possibly risky?

[theodoretan]

For this reason, we may want to only use this technique in areas where source SVGs are possibly risky?

I think having the Icon component render SVGs inline is potentially risky, as when other teams need to render 3rd party SVGs without knowing this vulnerability and use this component or use dangerouslySetInnerHTML, we reintroduce the risk of XSS.

Would it make sense to have a separate component that handles this use case instead of trying to merge it with the current Icon component?

I noticed there were a couple other Polaris components that use Icon and would not work with the filter technique since they're not monochromatic SVGs.

[amrocha]

I agree with Theodore that it's risky to have this as is. I also don't know if a separate component would work out, because people might just misuse them if they're unaware of the difference. I think I can help out though.

Something I'm going to be looking into next week is replacing our SVG loader with SVGR.

What that means for this issue is that all local icons would be passed into the Icon component as React nodes, instead of strings. This would allow us to work with the assumption that any React nodes are safe, and can be rendered inline, while strings are not safe and need to be rendered using the technique suggested.

What do you think of this? If you can hold off on addressing this issue for a week then we can add SVGR as a workaround, and ship a security fix that addresses this issue at the same time.

[theodoretan]

👍 This works for me. Thanks for the context @amrocha!

I'll hold of on my implementation til SVGR is added.

[tmlayton]

Are we okay with changing how Icon handles SVGSource and let the Navigation Item use it instead of having dangerouslySetInnerHTML for user uploaded svgs?

Maybe, it depends on the reason why we didn't end up using Icon with the nav Item in the first place, which I don't know the answer to without digging into the code more.

It seems though, it would be good to offer plugs for these two holes (Item and Icon) as those are the only 2 spots in Polaris React we potentially can be passed and render untrusted content.

We should be able to isolate this and offer an API change that 1) is not breaking and 2) only concerns itself with changing how we render unsafe SVGs.

Seems like options are CSS filters, color profiles, and sandboxed iframes. Anything else?

I would propose adding an untrusted prop to Icon which uses whichever alternative approach. If we can refactor Item to use Icon, just pass the prop through otherwise do the same conditionally rendering in Item.

Thoughts?

[amrocha]

I would propose adding an untrusted prop to Icon which uses whichever alternative approach. If we can refactor Item to use Icon, just pass the prop through otherwise do the same conditionally rendering in Item.

I like this since it allows us to ship non-breaking changes quickly to address the security issue.

That being said, I don't feel confident in "opt-in security" features like that long-term though, since they're prone to human error. My proposal would be that for a 4.0 release we remove the untrusted prop, and treat any strings passed into Icon as unsafe, along with a shift to SVGR and a recommendation that consuming projects use it too. That way internal, trusted icons are rendered as usual, and we isolate the impact of the change.

[theodoretan]

By adding untrusted as a prop, this would mean source would be optional right?

I'm thinking that if untrusted and source are both undefined we should render the placeholder icon. Are we okay with this?