crysalead-js / dom-layer

Virtual DOM implementation.
MIT License
30 stars 1 forks source link

escape attributes and text? #43

Closed ghost closed 9 years ago

ghost commented 9 years ago

I just noticed that some libs such as REACT, deko, ReactJS, JSBlocks etc. are also escaping the text nodes before they deal with it, and also htmlify attributes. But they are not escaping value attributes.

jails commented 9 years ago

It depends where the escaping is done and for what purpose but I didn't think some escaping is missing somewhere in dom-layer.

ghost commented 9 years ago

As one example. Try to mount something, and do this

"#mount-point "

This will throw because here is an extra space.

You can see if this is something usefull too: https://github.com/paldepind/snabbdom/pull/1

Anyway. Look at this code snippets.

This should be escaped? https://github.com/crysalead-js/dom-layer/blob/master/node/text.js#L10

And this? https://github.com/crysalead-js/dom-layer/blob/master/util/stringify-attrs.js#L26

The mention libraries are escaping this one to avoid double space, tab, line feed etc.

ghost commented 9 years ago

For htmlify attributes, a better solution could be this one? At least you skip also join, push and replace

 var html = "";
 for (var key in attrs) {
    value = attrs[key];
    if (key === "style") {
      value = stringifyStyle(value);
    }
    if(value === '') {
          html += ' ' + value ;
        } else if (value  != null) {
    if ( !(key === "value" && (/^(?:textarea|select)$/i.test(tagName) || attrs.contenteditable))) {
       html += ' ' + value + '="' + value + '"';
    }
    }
  }
jails commented 9 years ago

For https://github.com/crysalead-js/dom-layer/blob/master/node/text.js#L10, nope it doesn't need to be escaped. No XSS issue is possible on text node since all special char just displayed and not interpreted as html.

For https://github.com/crysalead-js/dom-layer/blob/master/util/stringify-attrs.js#L26 nope either. values inside an html template should be escaped but because of html. But if you set a value directly using the API it's not necessary.

So escaping is only needed for toHtml(). And I'm not so worried about join / push since toHtml() will only be used server side.