Normative: Coerce evaluate's argument to string

[leobalter]

Ref #318

[leobalter]

It's important to highlight @caridy's PoV from #304 (comment) here:

it is that strict to avoid the confusion with eval, which has a bizarre behavior by returning the value of the first argument if it is not a string value. As for the second one, I believe it is a trying to match the semantics of dynamic import, which does not exhibit any bizarre behavior. An alternative here is to make evaluate to perform type coercion, which I'm open to, which will mean people moving from eval(value) to myRealm.evaluate(value) will not result in an error, but the behavior will not match the original behavior of eval either.

[Jack-Works]

Why not keep it throw? 👀

[leobalter]

This came in as a suggestion from @gibson042's spec review (#304).

As I pointed out in the comment above, @caridy shared some agreement to this change in #304 (comment) and #318.

The high level overview for the change is to make the evaluate method consistent with importValue and other general methods often seen in ECMAScript.

The main reason it throws today is to make it slightly consistent with eval, but the consistency is not perfect. eval will just return the argument if it's not a string:

const obj = { toString() { return "40 + 2"; } };

eval(obj) === obj; // and not 42

With the Realms API, we have the current outcome:

const obj = { toString() { return "40 + 2"; } };
const r = new Realm();

r.evaluate(obj); // throws TypeError
r.evaluate(new String('Math.PI')); // TypeError

With this PR, we could have some better usage of argument:

const obj = { toString() { return "40 + 2"; } };
const r = new Realm();

r.evaluate(obj); // 42
r.evaluate(new String('Math.PI')) === Math.PI; // true, considering Math.PI is not altered in both realms

---

We do have precedent for things like this today, such as the Function constructor coercing args and body to string. The approach from this PR only makes evaluate a bit more consistent with functionality of other methods and putting it slightly farther from eval. Anyway, I believe the weird behavior from eval on non string arguments is undesirable legacy, but part of web reality.

WDYT?

[littledan]

I think it'd make sense to close this PR and leave the behavior as is. importValue should be analogous to import() (which coerces) and evaluate should be analogous to eval (which returns its input when passed not a string--the natural analogue is to throw, since that behavior is actually surprising).

[Jack-Works]

Is it possible to accidentally enables some attack surface?

[leobalter]

I find validity in both sides, I don't have a strong preference and I can see a good reason either way.

[littledan]

That example falls into what I mentioned above, that there's similar behavior elsewhere, so I don't think it expands the "vulnerability" meaningfully.

[erights]

@mikesamuel where is your proposal to use a non-string eval argument for a trusted-type-like branding for approved evaluation in a CSP-no-eval context? If you could answer quickly, that'd be great because we're in the midst of a tc39 meeting where this may become relevant. Thanks!

[ljharb]

@erights https://github.com/tc39/proposal-dynamic-code-brand-checks iirc

[erights]

@ljharb that's it. Thanks!

[mhofman]

I'm not sure I understand the benefit in coercing the source argument to a string, besides making it more similar to the Function constructor. And given that the callable boundary prevents both access to the other Realm's Function.[[Construct]] and passing non primitive arguments, it should be impossible to coerce to string and evaluate an object in another isolated realm using that route.

[gibson042]

I found @caridy's explanation satisfactory, and don't have strong feelings about throwing vs. coercing. But I do think a callout of the behavioral difference should remain visible in the rendered spec.

[leobalter]

After some discussion with @caridy, we agreed to hold on and not move this PR forward for now. The reasons to keep the API as is have a good weight to also avoid such a noise before this TC39 meeting.

I'm gonna hold on this PR and I'm pretty sure I can do a better recap to previous discussion and works such as those pointed out by @erights and @ljharb.

Thanks everyone for the quick feedback!