Avoid using function constructor for alt format handling

[tschaub]

This allows rbush to be used where the Content Security Policy (or other) disallows the use of eval and related functions (e.g. browser extensions). Previously, the format supplied could contain function calls (e.g. ['.getMaxX()', ...]). With this change, only property names are accepted - though they can be dot prefixed or bracket wrapped (e.g. '.maxX' or '[0]').

[tschaub]

The tests and docs didn't show use of .getMaxX() style accessors. Not sure if you need to support those. If not, it would be nice to be able to avoid the Function constructor.

[mourner]

That's interesting, I've never heard about security policies blocking eval-like statements. Can you share more details on this? What was the original related issue that you encountered?

This could be a nice change, but doesn't support more complex data scenarios (e.g. ['.bbox[0]', '.bbox[1]', ...), and potentially introduces a performance hit due to closures inside extremely performance-sensitive methods (did you compare benchmark results?).

[mourner]

The security stuff was brought up in Leaflet too, so that's a valid point it seems. Leaflet/Leaflet#2209

An alternative solution (although requiring more verbose code) would be to make toBBox/compare methods public and don't run _initFormat if specified false for it explicitly, to allow simply defining all the methods manually. rbush.toBBox = function (...

[tschaub]

Yes, the Content Security Policy (CSP) for browser apps/extensions is the common case where eval and friends are disallowed.

Allowing people to override the toBBox and compare* methods makes sense.

FWIW, I couldn't find any significant difference in the benchmarks with and without this change. But I also didn't have the patience to run them enough times to get much confidence in the results :).

[tschaub]

@mourner here's an alternate implementation: https://github.com/tschaub/rbush/compare/less-evil-II

This is quite a bit more verbose.

If you do allow custom toBBox, it looks like the default implementation could avoid creating an extra array (reducing pressure on the garbage collector). I left in the extra array creation because that's what the current _toBBox does - but I didn't immediately see a place where you modify it.

[mourner]

Nice, thanks! I meant that we could keep the current format handling (instead of removing it completely), but make it possible to bypass it in vulnerable cases such as developing an extension.

Yeah, no need to slice the bbox — it was done this way for simplicity in format-handling code (I found no significant performance difference).

[tschaub]

I meant that we could keep the current format handling (instead of removing it completely)

Makes sense. I can update that branch.

[tschaub]

Ok, closing in favor of #14. Let me know if you had something else in mind.