anthonyshort / deku

Render interfaces using pure functions and virtual DOM
https://github.com/anthonyshort/deku/tree/master/docs
3.41k stars 130 forks source link

HTML escaping in renderString #256

Open jasonhansel opened 8 years ago

jasonhansel commented 8 years ago

Since renderString does not escape HTML tags, the HTML generated by renderString can look very different in the browser from the DOM generated by render.

For instance, given the following code:

var deku = require('deku'),
  virtualElement = require('virtual-element'),
  app = deku.tree(virtualElement( 'div', null, '<b>Hello!</b>' ));

The following lines of code will produce very different results:

document.body.innerHTML = deku.renderString(app);
deku.render(app, document.body);

This inconsistency can create problems for isomorphic applications, which expect to be rendered in the same way on both the server and the client.

Furthermore, this opens up the possibility for XSS issues, if the data is untrusted. This problem was already noted in issue #55, but that issue discussed escaping input, while this is really an issue of escaping output (namely, the output produced by renderString and displayed to the client).

anthonyshort commented 8 years ago

Thanks @jasonhansel. I'll write some tests and documentation for this. Any sort of escaping we've left up to the user depending on what they're trying to do.

It looks like you here you want to display the html as a string on both sides, so I would have used another module to just escape that string to get the correct output. I can see how this would be confusing though. Even just a warning in development if we detect that a string looks like HTML but isn't escaped would be useful.

jasonhansel commented 8 years ago

Thanks for looking into this! To clarify, I actually don't want to display the HTML as a string on both sides. Rather, I want using renderString (on the server) and using render (on the client) to produce visually similar results (though the output from renderString will of course not be interactive).

Adding a warning in development would be an excellent idea, though it wouldn't be much more work to just incorporate the escaping functionality into deku itself; I'd be glad to submit a PR for that, if it would be helpful. The code necessary for doing the escaping is less than 80 lines.

I'm also still somewhat concerned about the security implications of not escaping potentially untrusted data; OWASP, for instance, recommends always escaping user data inserted into HTML elements.

kesla commented 8 years ago

FWIW, this would be valuable for us as well!