template/route/controller/etc lookups fail when lookup name exists on Object.prototype

[machty]

See this Twitter thread: https://twitter.com/nathanhammond/status/461525108394577920

TL;DR there's a Firefox-only bug that prevents you from naming a route watch because the Ember.TEMPLATES.watch is detected as a function and tries to render with that function.

Here's another JSBin where a route named hasOwnProperty fails: http://emberjs.jsbin.com/ucanam/4812/edit

There's probably some limit of what we want to support but let's discuss how we can be more careful with our lookup logic.

/cc @nathanhammond

[stefanpenner]

o_create(null) fixes this in modern browsers.

Once we move away from Ember.TEMPLATES completely (for the eak/ecli users), this wont effect us, as templates are stored as modules, and retrieved via the loader)

[nathanhammond]

Until we do either Object.create(null); or a .hasOwnProperty() check, any property that exists on Object.prototype is not valid as a template name. And worst, this is different on a per-browser basis: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/watch

[nathanhammond]

Even if we catch the error with an Ember.assert the user won't know until they've tested in the failing browser. I'm of the opinion that, since we don't care for Ember.TEMPLATES to have a prototype, we:

• Allow any and all property names save for hasOwnProperty.
• Add a .hasOwnProperty() check here.
• And add a new assert for templates named "hasOwnProperty" so that we don't troll our ability to check for it.

[nathanhammond]

Speaking of subtle trolls:

App.Router.map(function() {
  this.route('hasOwnProperty');
  // Or anything on Object.prototype.
});

This will blow up no matter what we do with Ember.TEMPLATES unless we have a null prototype.

EDIT: just noticed that this is what @machty references in his first post. :smile:

[machty]

We need to make sure that stuff like ember-inspector doesn't iterate over Ember.TEMPLATES and expect hasOwnProperty to exist on it.

[nathanhammond]

Recent discussion on creating dictionaries using just plain objects.

Short version on trying to come up with a solution for this appears to be "don't try." It appears that there are a ton of subtle bugs on a per-browser basis. Are people opposed to creating our own dictionary implementation/adopting one that we know is correct?

Pros:

• Guaranteed correctness.
• ???

Cons:

• More code.
• Slower (likely not deal-breakingly slow).
• Will require tweaks in lots of places inside of Ember.
• ???

/cc @ebryn

[GetContented]

@nathanhammond I've felt the pain of this when seeing several error messages referring to hashes and I was like "Oh? I know JS doesn't have Hashes, so Ember must have implemented its own, so I should be using a Hash object then", and then I couldn't find an implementation. It confused me for about 5 minutes until I realised the error message was referring to a POJO that the source was using as a hash. The only trouble with that is that they're not. (ie you can't say "keys" or "values"), and it'd be very useful if they were for debugging purposes (or even just because sometimes you want a hash). :+1: for a "native hash" implementation very much.

Sure you're aware of these, but both have the rationale: http://www.mojavelinux.com/articles/javascript_hashes.html http://www.timdown.co.uk/jshashtable/

One of the main cons you seemed to have missed out is that people will need to be informed because there's a prevalent tendency in JS devs to "just use an object" and then implement your own values or keys finders "on the fly". In other words, we'll need to think about and possibly guard against the cases when people use a POJO where we're actually expecting a dict/hash. Annoying.

[nathanhammond]

@JulianLeviston Native is supposed to be Object.create(null); but that won't work until __proto__ goes away. In the future that will probably work just fine but right now we've got legacy browsers and stubborn developers to deal with (on both sides of the issue). :/

Throughout the Ember codebase I believe that it's okay for us to use objects as dictionaries as long as we're not expecting dynamic keys. Once we begin expecting dynamic keys (e.g. Ember.TEMPLATES) I'm of the opinion that we should move to a safe implementation.

[stefanpenner]

@nathanhammond I don't think __proto__ is going away. It is part of the es6 spec.

@nathanhammond meta's cache is a scary object then :(

[nathanhammond]

@stefanpenner __proto__ has been demoted to Appendix B and its implementation is described in terms of using Object.setPrototypeOf(). The trending direction is away from using that property. That being said, it remains an unreliable thing which we can't really work around until it is truly gone.

Can "real/safe dictionaries" make the agenda for the next core team meeting? I would like to implement it if it's something that we decide should be done. (I want more familiarity with Ember internals so that I can contribute in more ways.)

Aside: if I were actively looking for XSS in Ember I'd probably try and abuse the fact that we're using full objects as dictionaries. It seems like a reasonable attack vector.

/cc @webreflection

[stefanpenner]

@nathanhammond you are correct

[WebReflection]

I'm not sure I'm over-simplyfing in here but this is the way I'd go:

Ember.TEMPLATES = (function(hasOwnProperty, cache, prefix){
  return (Object.freeze || Object)({
    del: function (k) {
      return delete cache[prefix + k];
    },
    get: function (k) {
      return cache[prefix + k];
    },
    has: function (k) {
      return hasOwnProperty.call(cache, prefix + k);
    },
    set: function (k, v) {
      return (cache[prefix + k] = v);
    }
  });
}(
  // avoid further changes of hasOwnProperty
  // causing troubles with this logic
  Object.prototype.hasOwnProperty,
  // use Object.create(null) where available
  (Object.create || Object)(null),
  // use a non common prefix to avoid
  // __proto__ or Object.prototype names clashes
  '\x01'
));

Performances won't be compromised that much being a template related object and having just one level of indirection.

[stefanpenner]

interesting, we have a similar construct already: https://github.com/emberjs/ember.js/blob/master/packages_es6/container/lib/inheriting_dict.js

[WebReflection]

you need to make it safe adding a prefix that is not an underscore (or _proto__ will fail)

maybe you can improve the constructor.

Also, that will be slightly slower in getting and returns no useful info on deleting (why?) plus it does too many hasOwnProperty calls my example won't actually need.

As summary: If raw performance was a concern, seeing that class I would say you guys are good to go with that kind of indirect logic ;-)

[nathanhammond]

@WebReflection My hope is to focus on correctness. As a result I would fully expect to go through all of Ember and make sure that it played nicely with a safe dictionary implementation. Done that way we don't have to worry about weird edge conditions. :smile:

Rather than reinventing the wheel I would probably adopt and adapt your HashMap implementation from this post. Then, so we don't trigger a major version bump, for any "public" API I'd go back and add checks that cast an object to a dictionary when not passed a dictionary. Since our current state is broken, it will be exactly as broken as it was previously–meaning no backwards incompatible changes to Ember.

I appreciate the incredibly detailed research that you've done on this topic–it makes the next steps in the process all implementation-oriented instead of having to do a bunch of research.

@stefanpenner Hey look at what @rjackson snuck in! InheritingDict could be a start, but right now we only appear to be using that in the container. Seems to handle container lookups with the added clever feature that, if it doesn't find it in the current container, it hops up to the parent container. Could be easily built as a "subclass" of a safe dictionary.

[stefanpenner]

Inheriting dict has been around for over a year. I wonder if we should extract it to be shared with the rest of ember

[WebReflection]

@nathanhammond by "you guys are good to go with that kind of indirect logic" I meant with what @stefanpenner found out already adding a fix for the key that could be optional.

Here a pull request example

[nathanhammond]

I guess that's what I get for relying on GitHub's history. I always forget that GitHub doesn't really understand git mv very well.

And I do believe that we should split it out, create a "Dict" and "InheritingDict", and start using it everywhere.

[stefanpenner]

The annoying part of the prefix is the cost of interned vs non interned string lookup on a fast object. For some use cases like meta cache the overhead is pretty high. As is x in y or hasOwnProp. Atleast until v8 optimizes those paths

Obviously for other things and other runtimes the impact may be tolerable. Ember.TEMPLATES and meta cache have different performance needs.

It seems we should lobby vendors to provide us with a fast safe object. As current Object.create(null) is much slower then allocating {}, as is forcing a object to be a dictionary over a map.

[WebReflection]

@stefanpenner you are dynamically accessing properties either ways, if you don't chose a prefix bigger than 1 or 2 chars I don't see any relevant/concrete performance impact against just key.

I am also not sure just filtering by __proto__ would give you better performance than just prefix concatenation and you need that check, IMO, or even hasOwnProperty could become compromised the first time some template gets called __proto__ (I know, it should not happen)

However, since I got that TEMPLATES is a special case, it's not by accident that I've already suggested that very specific piece of code.

Last, but not least, creation of objects in JS is fast enough to never be the real concern in real-world apps, but if the usage of Object.create(null) worries you you can simply go through:

function Dict() {}
Dict.prototype = Object.create(null);

then use new Dict any time you want same as you create any other non object instance.

take care

[stefanpenner]

Again my thoughts are not related to TEMPLATE object, but other objects that should be "safe" and fast.

@WebReflection

function Dict() {}
Dict.prototype = Object.create(null);

Approach is a good idea, I'll play with some benchmarks later to see the cost cache misses, I suspect much better.

On a modern browser, new Dict() should be just as cheap as {}, its about 20%-30% slower on older browsers. But I don't think I care, if the cache miss cost drops nicely.

It would be interesting to see if dictionary only mode objects, can inherit that behavior. (will investigate tonight).

The issue with concat (in hotspots) is the concat'd string (even if it is short enough) doesn't immediately get interned. The concat itself isn't free, and causes extra allocations but more importantly non interned string lookup on fast objects is more costly, then interned key lookups. Eventually those concat'd strings may be interned if they meet the correct criteria, but if we do the concat immediately before the lookup this will never happen.

[WebReflection]

bear in mind this might improve performance over Object.create(null) but will not fix __proto__ shenanigans with null objects in old browsers, most Android mobile, and some broken engine (nashorn as example)

[stefanpenner]

@WebReflection confirm, but I am somewhat alright with a slightly slower fallback for older engines.

[WebReflection]

please post the code here or cc me once pushed, thanks

[GetContented]

@stefanpenner & @nathanhammond this issue outlines my confusion/initial annoyance about Hash/Dicts in error messages: https://github.com/emberjs/data/issues/1892 I realise the discussion here is about "Hash" object pollution & performance. Just FWIW.

[wagenet]

@stefanpenner what's the status on this? Are there any specific actions that someone else can take?

[stefanpenner]

many of these scenarios are fixed, as we are using Object.create(null) in places where this should matter. If we are missing some those can be reported as separate issues