statgen / locuszoom

A Javascript/d3 embeddable plugin for interactively visualizing statistical genetic data from customizable sources.
https://statgen.github.io/locuszoom/
MIT License
154 stars 29 forks source link

Should dashboard buttons allow html rather than just text? #104

Closed pjvandehaar closed 7 years ago

pjvandehaar commented 7 years ago

Right now,

Should all of these allow arbitrary HTML?

Frencil commented 7 years ago

All of the above should be safe to be interpreted as HTML. In general, any element being defined by the layout that's outside of the SVG context (so dashboard elements, tooltips, etc.) should accept arbitrary HTML where they accept arbitrary stings.

The only exception would be places where strings are being used for title attributes (e.g. LocusZoom.Dashboard.Component.Button.title) as HTML doesn't make sense in that context.

XSS is not a concern here... any <script> tags pumped through LZ's HTML generation methods would not be executed as browsers don't automatically execute dynamic script tags. And since everything's client-side anyway if somebody really wanted to execute arbitrary JS they certainly could without needing to use LZ's HTML passthroughs.

pjvandehaar commented 7 years ago

Thanks for explaining about the XSS. I always assumed the browser evaluated scripts on node.innerHTML = string, but apparently that's jquery here, not documented.

Frencil commented 7 years ago

Fixed in a39f647, will be included in v0.5.7.