olifolkerd / tabulator

Interactive Tables and Data Grids for JavaScript
http://tabulator.info
MIT License
6.7k stars 818 forks source link

Built in formatters, tooltips, and headers should escape HTML by default to avoid XSS problems #4387

Open rathboma opened 8 months ago

rathboma commented 8 months ago

It's great that Tabulator allows such customization and the ability to render HTML is a big part of that.

Unfortunately it's really easy to introduce XSS errors into an application by forgetting to escape tooltips, cell values, header titles, and basically anywhere data is rendered.

I'd like to propose that by default all the built-in components escape HTML, but that raw HTML mode can be enabled with a flag.

An example of what it is possible to do with Tabulator right now:

tooltip: "<img src='#' onerror=alert(1) />",
headerTooltip: "<img src='#' onerror=alert(1) />"

Also - settting that as the value of a cell, the name of a column, etc.

olifolkerd commented 8 months ago

Hey, the built in formatters already strip tags so you should be covered there, you have to use the html formatter or a custom formatter to inject.

But you are right about the header titles and tooltips.

The header titles are by design as people often use that to inject some basic tags into their title and those generally are hard coded so not vulnerable to that, but I could add that restriction to auto columns if that is where your concern arises from

Totally with you on the tooltips when they auto generate on from the cell value. But people may choose to setup their table with tags in the tooltips from the table settings which is a perfectly valid configuration. The setup config is only vulnerable to injection if it has been configured in a unusually dynamic way.

As a general rule for tabulator it should be automatically protecting against injection for data that is dynamically loaded into the table like table data. But the table settings cannot be externaly loaded without developers intervention shouldn't be default blocked because a lot of people use that functionality specifically to setup table UI.

The for the vast majority of users the data is the only dynamic element loaded into the table. Though I understand your usage case is much more complex

rathboma commented 8 months ago

Hey!

I like your way of framing it -- when the content can be dynamic -> Tabulator should protect (but also allow customization).

I'm not 100% aligned with you on what 'dynamic' means. I think it should also include the result of functions provided in the table constructor. Like for tooltips, formatters, etc.

Take for example someone providing a function for the tooltip. The result of that function is dynamic and should be escaped too, unless they override a setting. This could be user data also!

I totally agree that values which are just 'figured out' by tabulator from the data (column titles, tooltips, etc) should be escaped.

Beekeeper's use case is extreme, but I think it also likely reflects a common integration path -- we store a lot of data state outside of tabulator, and use the great tabulator API to override a bunch of behaviors. I think by default Tabulator should be really skeptical of this data :-)