powmedia / backbone-forms

Form framework for BackboneJS with nested forms, editable lists and validation
MIT License
2.17k stars 415 forks source link

itemToString does not escape values by default #516

Closed wesvetter closed 7 years ago

wesvetter commented 8 years ago

It seems like there is a possible vulnerability in using the List editor with the Object item-type. Modal.prototype.itemToString does not escape the values by default which makes it vulnerable to XSS attacks.

Here is a simple example: https://jsfiddle.net/wesvetter/sh57z7ba/2/

If this is the intended behavior then a note should be added to the README so that users are aware of the behavior.

glenpike commented 7 years ago

Half inclined to ignore this because you're fiddle is written in CoffeeScript and I have to learn something else just to grok the problem... :p

There is a workaround - you can supply your own itemToString function as part of the schema if it's possible that you need to escape data.

I will also mention in the documents.

I've done a JavaScript JSFiddle of the workaround ;)

wesvetter commented 7 years ago

If you're displaying any user-submitted data, I'd advocate overriding it at the top level with the safe (escaped) version, and passing an unescaped version at the schema-level should you need an unescaped itemToString.

In other words:

// main.js
window.Backbone.Form.editors.List.Modal.prototype.itemToString =
 function() {
   ... // escaped version
}

// foo.js
new Backbone.Form({
  schema: {
    itemToString: function() { ... } // unescaped version
  }
})

It's worth noting that if you want markup in your itemToString method you still need to remember to escape user-submitted values. e.g.:

itemToString: function(value) { return key + ": <strong>" + _.escape(value) + "</strong>" })

I'd also consider using _.escape rather than $('div').html(value).text().

glenpike commented 7 years ago

Okay, it probably makes sense to "play safe" by default. As this would be a breaking change, I will probably slate it for later - if we are to release non-breaking changes soon.