avoid Object.prototype inheritance for col._byId

[brooswajne]

fixes issue #4224

collection._byId is an internal object used solely for mapping ids to models, and doesn't need any of the standard object prototype methods for its internal usage.
Unlikely to be a breaking change for any external uses, unless someone is really hacking into the core backbone functionality, and they can simply do things like Object.prototype.hasOwnProperty.apply(collection._byId).

Alternative solution could be to use a Map() object for collection._byId, but that would require many more code changes, would not be supported in old browsers, and is more likely to break external uses.

[jgonggrijp]

I just closed #4224 as invalid, but I'll give this PR some thought. I don't think supporting names of JavaScript's Object prototype properties as identifiers is important, but maybe this could make Backbone less sensitive to prototype pollution. Maybe. This has to be weighed against the obvious cost of requiring Object.create, which is almost certainly a breaking change.

[ogonkov]

I think #4224 was valid. Backbone should check for hasOwnProperty, when accessing model by id that is same as some builtin method. It could be possible attack vector aswell, if we write to object without checks. Like model with id __proto__.

[jgonggrijp]

I closed #4224 because it was motivated by a use case that I believe shouldn't be supported. However, I left the current PR open because I think it might improve security. So we don't really disagree on that count.

You really have to ask yourself, though: how does the id of a model end up being something like __proto__ or hasOwnProperty? Is that a scenario Backbone should cater to?

[ogonkov]

I think we should ask "do we handle it correctly when it happen"

[ogonkov]

Lets imagine that user app is somehow wanted __proto__ as model id. What would be correct behavior in that case? I think it would be correct to return model with such id.

I believe its not a breaking change, because we fixing internal behavior that never public advertised.

[jgonggrijp]

Backbone is not obliged to support whatever the user wants to do. We can set preconditions, such as "choose sensible values as model ids". __proto__ is not a sensible value for a model id in my book.

I'm more interested in scenarios where the model id ends up being __proto__ or something like that unintentionally, i.e., when an attacker it trying to hack a Backbone application. I can think of two ways this might happen:

1. The app developer might set the model id based on user input. This is always a bad idea, so the app developer just shouldn't do that.
2. The database backend has already been compromised by an attacker and they inserted __proto__ as a model id somewhere. While exotic, I currently cannot rule out that this might grant the attacker additional capabilities. This hypothetical scenario is the reason I've left this PR open. My question is basically this: would this grant the attacker any new capabilities, or would it only break things?

It is a breaking change because it requires Object.create, which is not backwards compatible with all environments currently supported by Backbone (such as Internet Explorer).

[ogonkov]

One of my project was once pwned with __proto__ poisoning
I had "wonderful" 4 hours of debugging trying to figure out how this works

[ogonkov]

Object.create supported since ie 9. I see that it is a minimal version in tests.

Where is IE support declared?

[ogonkov]

Seems like IE8 support was dropped since #4008

[jgonggrijp]

One of my project was once pwned with __proto__ poisoning
I had "wonderful" 4 hours of debugging trying to figure out how this works

Yes, that's the potential danger I'm interested in. I'm not sure it applies here, though, because prototype poisoning depends on assigning properties of __proto__ rather than __proto__ itself. This most commonly happens with recursive assignment functions like Lodash's merge and defaultsDeep. If somebody can provide a proof-of-concept (PoC) to show that it can actually be exploited in this case, that would be really helpful. That would also give us something to base a useful regression test on.

Object.create supported since ie 9. I see that it is a minimal version in tests.

Where is IE support declared?

Oops, I stand corrected. I was looking at https://caniuse.com/?search=object.create, not realizing that the top of the page is showing me Object.entries instead of Object.create. Strange order of search results...

Although #4008, which you linked to, also mentions workarounds for old IE that haven't been removed yet. Which means that IE8 probably still works, although it is untested. The situation is the same for Underscore, currently. That means it's still a breaking change.

[ogonkov]

IE8 support already broken for router, may be for other parts too, see #3623

Also #4008 clearly set vector for removing old ie hacks, not to support the further

[jgonggrijp]

Of course support for IE8 is eventually going to be dropped entirely, because it's extremely outdated. Still, that's something you do on a major version upgrade, not within the 1.x series. Note that @akre54 wrote "future" and "eventually", not "whenever we feel like it".

I'm fine with not fixing #3623, which happened accidentally, and with dropping IE8 support in tests, as has happened in #4008 (I did that in Underscore, too, simply because SauceLabs didn't support IE8 anymore), but technically it's still a breaking change.

Again, if we can establish that there is actually a security risk here, then I might be willing to make this breaking change already in the 1.x series. After all, then it is a bugfix, and bugfixes are always at least somewhat breaking changes by necessity.

[paulfalgout]

Personally I think IE8 should be dropped as a concern without worry about it affecting a major release. Backbone has not historically been as concerned with this, but I strongly support introducing breaking changes in major versions. However I think in this case dropping IE8 is forgivable. It's a unique situation for a breaking change and any cost of supporting IE8 is not worth the effort at this point. Particularly for a smaller community with no real reasonable way of actually testing any breaking changes on the platform.

[jgonggrijp]

I think we mostly agree. I would like to argue, however, that as long as this change is not provably urgent (I'm sceptical that it actually solves a problem), there is no real cost in postponing the merge until the next major release. In other words, saving a questionable bugfix for later is more forgivable than making a certain breaking change now.