endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
825 stars 71 forks source link

Number.prototype.toLocaleString complains about radix #852

Closed dckc closed 2 years ago

dckc commented 3 years ago

I put (42).toLocaleString('en') into https://ses-demo.agoric.app/demos/console/ expecting to get "42" but instead it went kerflewey:

threw "[RangeError: toString() radix argument must be between 2 and 36]"

Moddable recently made a fix in this area: https://github.com/Moddable-OpenSource/moddable/issues/674

It shows that this can't be right:

test/test-tame-locale-methods.js
8:  t.is(Number.prototype.toString, Number.prototype.toLocaleString);
kriskowal commented 3 years ago

@erights What do you figure is an acceptable behavior here? Should we treat all languages as Arabic numerals, ignoring the argument outright? Should we just remove this method?

kriskowal commented 3 years ago

Perhaps we should offer a repaired version of this method. The repair would make the niladic form, 1..toLocaleString() throw an error, since respecting the default would reveal the locale, and otherwise permit all the other forms that take an explicit locale list. This is variadic, so we’d have to take care to also disallow the form that only provides an options bag.

Alternately, this might be an opportunity to suggest a use for a Compartment locale hook.

erights commented 3 years ago

Even with a locale hook, we still need an answer for the default case, which must be safe, and should be as compat as possible without being unsafe.

I don't like having the no-argument form throw. But I don't know what you mean by revealing the locale. If we alias the no-argument toLocaleString() behavior to the no argument toString() behavior, then we're defining that the SES locale is the locale where everything prints the way the locale independent standard printing prints. That's what I intended with the default aliasing behavior, but I missed this radix issue.

Given the radix issue, I think toLocaleString(arg) should error if a first non-undefined argument is present.

Separately, we should add a locale hook to our host-hook wish list, for us to start thinking about.

kriskowal commented 3 years ago

By revealing the locale, I mean leaking the system or user agent’s currently configured locale, which varies by place, time, and whimsy.

I like making n.toLocaleString() behave as n.toString().

Is the harm in allowing n.toLocaleString("en") or generally n.toLocaleString(locale) that the range of known locales varies by host?

I think it might be sensible for all forms of n.toLocaleString() to behave as n.toString() since that is the behavior of n.toLocaleString("liliput", "endor", "middle-earth"). It does not err out but falls back to n.toString() if none of the suggested locales are known.

erights commented 3 years ago

Oh that's interesting. What does the spec say the fallback behavior should be if the caller asks for a locale that is not known?

kriskowal commented 3 years ago

https://tc39.es/ecma262/#sec-number.prototype.tolocalestring

This function is implementation-defined, and it is permissible, but not encouraged, for it to return the same thing as toString.

kriskowal commented 3 years ago

I saw a note somewhere, not in 262, that it may also throw RangeError if it does not know a locale and this could be used for feature detection, but I don’t believe that to be true. In Node and Chrome, it seems to fallback to the user locale.

1_000..toLocaleString('liliput', 'gondor')
'1,000'
erights commented 3 years ago

Awesome. In that case we say the default SES behavior is one that knows no locales and always false back to toString behavior. Then we can treat toLocaleString(whatever) as toString(). Cool.