sstephenson / eco

Embedded CoffeeScript templates
MIT License
1.71k stars 70 forks source link

Escape backbone model attributes causes error #36

Closed marlun closed 12 years ago

marlun commented 12 years ago

I've been trying out using stitch.js and eco with backbone.js but I'm having some troubles.

I'm sending a backbone model to a Eco template with:

$(this.el).html(template(this.model));

Then in my template, all I'm trying is this:

<%= @get('text') %>

Now what I've been able to figure out is that eco first checks if there's already an escape function on the object in question and since it's a backbone model there is a method called escape so eco calls that method instead of its own. The backbone escape method looks likes this:

escape : function(attr) {
    var html;
    if (html = this._escapedAttributes[attr]) return html;
    var val = this.attributes[attr];
    return this._escapedAttributes[attr] = escapeHTML(val == null ? '' : '' + val);
   },

Now since when this is called this is DOMWindow the this._escapedAttributes[attr] part fails with an error:

Uncaught TypeError: Cannot read property 'Finish some great tasks' of undefined
_.extend.escape
__sanitize
__obj.safe
__obj.safe
module.exports
module.exports.Backbone.View.extend.render
module.exports.Backbone.View.extend.add
_.each._.forEach
_.each.Backbone.Collection.(anonymous function)
module.exports.Backbone.View.extend.addAll
Backbone.Events.trigger
_.extend.reset
_.extend.fetch.options.success
jQuery.Callbacks.fire
jQuery.Callbacks.self.fireWith
done
jQuery.ajaxTransport.send.callback

'Finish some great tasks' is what's in my models 'text' attribute.

I tought maybe I could change __esacpe(value) in your code to __escape.call(__obj, value) but I havn't got it working yet. Any ideas?

Also I can change the template to <%- @escape('text') %> and it works but I'm not sure what I think is best and I thought I would tell you about this problem.

SpoBo commented 12 years ago

Eco uses an internal escape method to pass anything it renders to the page through it in order to prevent XSS attacks etc. This is all very nice and it works out of the box.

The problem however is that eco uses the escape method of the object itself it's trying to render if one is preset.

In the case of backbone there is an escape function on the models and to top it off it interprets its job a little bit differently. It only works if you pass along a valid attribute name and it'll return the escaped content for that attribute. This isn't what eco expects because it'll pass just about anything through the escape method.

So there are 2 ways to fix this in your app.

1) Don't pass along the native backbone model but pass something resembling it. One option could be to use model_instance.toJSON instead of the model_instance. Just make sure that the model you pass along has no escape method that misbehaves. Preferably no escape method at all.

2) If you want to use the full model and all its logic simply delete the escape function in the View prototype. I'm under the impression that it's not used anywhere else. It seems like a convenience method to me since backbone internally uses the _ library to escape. You can do this easily with a bit of code you load before everything starts rendering.

here's what I did in CoffeeScript: I made a lib folder I load right after backbone has loaded. Added a coffeescript file with:

delete Backbone.Model.prototype.escape

So should this be fixed in eco? Probably not. I think it's a neat idea that it can use the escape method of the object that's being rendered.

fson commented 12 years ago

In addition to overriding the internal escape similarly named property in the data object, eco also modifies the object by overriding safe property. So if you pass any object to eco template, after this you will have safe and escape methods in it, which IMHO is horrible.

This kind of messing with the object (stored in __obj) doesn't really make sense:

      }, __safe, __objSafe = __obj.safe, __escape = __obj.escape;
      __safe = __obj.safe = function(value) {
        if (value && value.ecoSafe) {
          return value;
        } else {
          if (!(typeof value !== 'undefined' && value != null)) value = '';
          var result = new String(value);
          result.ecoSafe = true;
          return result;
        }
      };
      if (!__escape) {
        __escape = __obj.escape = function(value) {

It would be more sane to at least make a clone of the object before messing with it.

sstephenson commented 12 years ago

Always pass an object literal of your own creation to Eco templates. Never pass a Backbone model directly.

In other words, do this:

$(this.el).html(template({ person: person }));

…not this:

$(this.el).html(template(person));

We don't want to make a copy of the passed object on each render—that would be bad for performance.

fson commented 12 years ago

Thanks for pointing this out. Maybe there should be a mention about this in the documentation.