snabbdom / snabbdom-to-html

Render Snabbdom Vnode’s to HTML strings
94 stars 21 forks source link

HTML tags should be encoded #36

Closed zkochan closed 6 years ago

zkochan commented 7 years ago

When using snabbdom directly, it encodes text passed to the node:

'use strict'
const h = require('snabbdom/h').default

h('div', {}, '<br/>')
// will be rendered as <div>&lt;br/&gt;</div>

To insert raw HTML to a node with snabbdom, the innerHTML prop should be set, see explanation in this issue.

However, when rendering with snabbdom-to-html, text is not encoded, so the previous example with snabbdom-to-html will be:

'use strict'
const h = require('snabbdom/h').default
const toHtml = require('snabbdom-to-html')

console.log(toHtml(h('div', {}, '<br/>')))
//> <div><br/></div>

This is inconsistent and dangerous.

acstll commented 7 years ago

Thanks for pointing this out.

Indeed snabbdom uses document.createTextNode, which I guess does the encoding. See https://github.com/snabbdom/snabbdom/blob/master/src/htmldomapi.ts#L27-L29

What do you think would be the simplest way to do the encoding and apply it to vnode.text? https://github.com/snabbdom/snabbdom-to-html/blob/master/init.js#L33

zkochan commented 7 years ago

I am not aware of built-in Node.js modules that would encode HTML. I searched on npm and seems like the he is the most popular package for encoding/decoding HTML

zkochan commented 7 years ago

Actually, it is enough to encode just the <>"&. This is how preact does it:

https://github.com/developit/preact-render-to-string/blob/1a04dc098a24dd916db79990d50a3ebb0c7a6976/src/util.js#L21

acstll commented 7 years ago

This module is actually using lodash.escape already haha, which does that exactly:

https://lodash.com/docs/#escape https://github.com/snabbdom/snabbdom-to-html/blob/master/package.json#L29

I'll try and fix this over the weekend. PR's are welcome too :-)

acstll commented 7 years ago

Another hand-made take https://github.com/stasm/innerself/blob/master/sanitize.js

rbelouin commented 6 years ago

@zkochan, @acstll: Hi! This does not seem to be fixed, so I've tried to address the issue in #38