Globalize-runtime has unsafe runtime-key hash function

[mattyork]

The runtime key is generated from a JSON.stringify of the arguments (see here.

Problem is that JSON.stringify does not guarantee an order that the properties are stringified. See here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/stringify

This already bit me once where formatters were called with the exact same properties but were stringified differently. For example:

// Compile time
compile(Globalize.numberFormatter({
  minimumSignificantDigits: 1,
  maximumSignificantDigits: 3
}))

// Runtime
Globalize.numberFormatter({
  maximumSignificantDigits: 3,
  minimumSignificantDigits: 1
});

By making sure the properties are specified in the same order, it appears to work in the current browsers, but this is a bandaid because JSON.stringify is defined as unstable.

The solution is to use a stable hash of the arguments. Here's a fairly small, stable stringify: https://github.com/substack/json-stable-stringify. Or you can non-stringify hash.

[mattyork]

704 is related and may have the same solution

[nkovacs]

I ran into this issue as well a few months ago. According to this, the order should be stable as long as you create the object in the same way: http://2ality.com/2015/10/property-traversal-order-es6.html So I did not open an issue at the time.

However, it might be a good idea to use json-stable-stringify to avoid mistakes like this and to avoid duplicating runtime formatters.

There's also another issue. These two are identical, but they produce two different hashes:

Globalize.numberFormatter();
Globalize.numberFormatter({style: 'decimal'});

because runtimeBind gets the original arguments as-is, e.g. here: https://github.com/globalizejs/globalize/blob/1.2.3/src/number.js#L75

I think the argument hashing happens here during compilation: https://github.com/globalizejs/globalize/blob/1.2.3/src/common/runtime-bind.js#L8 The code you linked to is used during runtime to find the function. So it needs to be fixed in two places.

[rxaviers]

If someone works on a fix, we could include it in the upcoming 1.3.0... We need to evaluate different hash choices with respect to runtime and size footprint.

There's also another issue. These two are identical, but they produce two different hashes:

This can be solved similarly to dateFormatter https://github.com/globalizejs/globalize/blob/master/src/date-runtime.js#L38. In other words, the default options for numberFormatter needs to be fixed from {} to {style: 'decimal'} https://github.com/globalizejs/globalize/blob/master/src/number-runtime.js#L31.

[nkovacs]

json-stable-stringify requires jsonify if JSON doesn't exist . We could hack that out during the build.

There's also fast-stable-stringify: https://www.npmjs.com/package/fast-stable-stringify, which is faster, but it doesn't support replacer, which I used for the fix for #704, and it doesn't check circular references or toJSON methods (I don't know if those are needed).

[nkovacs]

I ran a benchmark on node:

Finished benchmarking: nickyout/fast-stable-stringify x 15,634 ops/sec ±0.75% (94 runs sampled) (cumulative string length: 221965828) Finished benchmarking: substack/json-stable-stringify x 12,959 ops/sec ±0.80% (100 runs sampled) (cumulative string length: 190507028) Finished benchmarking: JSON.stringify x 81,442 ops/sec ±0.23% (100 runs sampled) (cumulative string length: 1173618522)

[nkovacs]

I wrote a new benchmark to compare various options: https://github.com/nkovacs/stringify-benchmarks I modified ppaskaris/faster-stable-stringify to support the replacer function. It's much slower in Chrome than in Node for some reason.

Node.js:

Finished benchmarking: JSON.stringify x 894,885 ops/sec ±1.33% (89 runs sampled) (cumulative string length: 298628380)
Finished benchmarking: nickyout/fast-stable-stringify x 652,594 ops/sec ±0.78% (94 runs sampled) (cumulative string length: 268121016)
Finished benchmarking: ppaskaris/faster-stable-stringify x 650,882 ops/sec ±1.33% (87 runs sampled) (cumulative string length: 81900725)
Finished benchmarking: nkovacs/faster-stable-stringify-2 x 613,162 ops/sec ±0.63% (94 runs sampled) (cumulative string length: 205904601)
Finished benchmarking: substack/json-stable-stringify x 495,164 ops/sec ±0.65% (96 runs sampled) (cumulative string length: 165384274)

Google Chrome:

Finished benchmarking: JSON.stringify x 854,869 ops/sec ±1.03% (59 runs sampled) (cumulative string length: 289429146)
bundle.js:22003 Finished benchmarking: nickyout/fast-stable-stringify x 776,384 ops/sec ±1.07% (62 runs sampled) (cumulative string length: 323719335)
bundle.js:22003 Finished benchmarking: ppaskaris/faster-stable-stringify x 658,560 ops/sec ±1.07% (61 runs sampled) (cumulative string length: 83453475)
bundle.js:22003 Finished benchmarking: nkovacs/faster-stable-stringify-2 x 545,758 ops/sec ±1.06% (61 runs sampled) (cumulative string length: 186746688)
bundle.js:22003 Finished benchmarking: substack/json-stable-stringify x 475,906 ops/sec ±1.58% (62 runs sampled) (cumulative string length: 163903373)

[rxaviers]

Thanks @nkovacs what about file sizes? I mean how much byes would we add to the library by using them? Thanks

[nkovacs]

This compresses down to 995 bytes with uglify.js. However, maybe a full JSON.stringify isn't needed. All of these options objects have a simple, fixed structure, with no nested objects. So something like this could also work:

arr = [];
Object.keys(options).sort().forEach(function(val, key) {
    if (typeof val === 'function') {
        val = val.runtimeKey;
    }
    arr.push(key, val);
});
JSON.stringify(arr);

Or you could just write them out, but this would be a maintenance burden:

JSON.stringify([options.minimumSignificantDigits, options.maximumSignificantDigits]);