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

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

Closed
pjvandehaar opened this issue May 30, 2017 · 3 comments
Closed

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

pjvandehaar opened this issue May 30, 2017 · 3 comments

Comments

@pjvandehaar
Copy link
Contributor

pjvandehaar commented May 30, 2017

Right now,

  • component.text is plain text. I'd like to use HTML for glyphicons but don't need it.
  • component.title is plain text
  • title_component.title is html
  • menu_component.button_html is plain text
  • menu_component.menu_html is html
  • covariates_component.button_html is plain text

Should all of these allow arbitrary HTML?

@pjvandehaar pjvandehaar changed the title Should dashboard buttons allow html from layout? Should dashboard buttons allow html rather than just text? May 30, 2017
@Frencil
Copy link
Contributor

Frencil commented May 30, 2017

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
Copy link
Contributor Author

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
Copy link
Contributor

Frencil commented Jun 9, 2017

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

@Frencil Frencil closed this as completed Jun 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants