rejectSomeDirectEvalExpressions is too aggressive

[caridy]

When it comes to evaluate arbitrary code inside a realm, sometimes throwing when direct eval is used (intentionally or not), makes the adoption of the shim more complicated. Even though executing the direct eval declaration as indirect eval is terrible, sometimes that compromise is desirable (e.g.: legacy code).

As today, this is one of the remaining issues for us to use the shim as it is, instead, it forces us to fork and change the logic in:
https://github.com/Agoric/realms-shim/blob/master/src/sourceParser.js#L97

I see few options:

1. find ways to support direct eval (I haven't think much about this one since we put in place the mechanism to replace the eval ref after the initial evaluation, I imagine that this one is going to be almost impossible).
2. provide some transpilation/transformation mechanism (via some configuration)
3. provide a way to control whether or not that rule should be applied during evaluation.
4. relax the rule or remove it entirely in favor of documentation (as it was before).

/cc @jdalton

[erights]

1. find ways to support direct eval (I haven't think much about this one since we put in place the mechanism to replace the eval ref after the initial evaluation, I imagine that this one is going to be almost impossible).

I believe it is possible, but terrifying to contemplate. Somewhere there's a thread between @SMotaal and me exploring a static verification + rewrite + enhanced proxy behavior that looks like it might be able to do this safely. However, the benefit it would produce would not pay for the gargantuan task of verifying that this pattern is actually safe. It would be a noble experiment, but not one we should accept back into the shim.

2. provide some transpilation/transformation mechanism (via some configuration)

Necessary for (1) and must be combined with (1) to achieve the goal. Direct eval is hard to emulate without using direct eval.

3. provide a to control whether or not that rule should be applied during evaluation.

If this is what I think you mean, I agree with (3). To restate: We should add an option, default off, that if on suppresses the rejection, allowing the previous indirect eval behavior.

4. relax the rule or remove it entirely in favor of documentation (as it was before).

I think the current behavior, where it defaults to rejection, is necessary for future compat. However, (3) for present kinda-compat by optional flag, with documentation, is a nice addition.

(3) fits with my general stance on unifying the shims between the major players: Currently Agoric, Salesforce, MetaMask, Figma. We will each demand somewhat different detailed policy decisions. However, we should avoid having our differences in details force any of us to fork, if we can avoid this at reasonable cost. Best would be to accommodate such differences with proper policy mechanism separation, where the shims (and the std they shim) provides the mechanism, such that policy variations can be built on top. However, when we can't achieve this cleanly, as here, we should add an option enabling switching between plausible policy choices. In all cases, such options should default to the safer setting.

[erights]

Currently Agoric, Salesforce, MetaMask, Figma

Meaning, major users of the shim. We should of course also coordinate Moddable on their direct SES implementation. We should ensure that all of these will correspond together to a reasonable draft spec.

[caridy]

Ok, I also think 3 is the way to do. Now, considering that this is a shim (not a polyfill [1]), I will propose that the execution of the shim code does not install the global Realm reference, and instead, provides a way to get access to it via a custom mechanism, e.g.:

import factory from "realms-shim";
globalThis.Realm = factory({ optionFoo: 1, optionBar: 2 });
const r = Realm.makeRootRealm();

Pros:

• The Realms' API will not need to differ from the spec (as spec evolves).
• All the limitations of the shim can be controlled and tweak via factory options rather than extending the API.
• Eventually, transition to the native implementation can be easy, you just need to remove the factory call.
• Allow you to use the realms without having to define the global reference Realm.

Cons:

• slightly more cumbersome to get you rolling with the shim.
• proprietary API with custom configurations that are driven by the limitations of the shim are usually not ideal (still powerful).

[1]* a polyfill is supposed to be transparent and undetectable once executed, while a shim can have a custom initialization and can differ from the specification.

[caridy]

any thoughts on the proposal?

[SMotaal]

@caridy I think a somewhat different take here could be:

import {Realm} from 'realms-shim/globals.js'

My thinking here is that globals are the exported set of whitelisted globals and globals that would be exposed (but are not by the shim).

This does not account for making multiple root realms instances of the Realm constructor using the factory (in essence).

[erights]

On the first question, I still favor (3): provide an option for suppressing the rejection behavior.

On the polyfill vs shim issue, two questions:

Given a polyfill, can one build a shim as a trivial wrapper around the polyfill?

In your example

import factory from "realms-shim";
globalThis.Realm = factory({ optionFoo: 1, optionBar: 2 });
const r = Realm.makeRootRealm();

The new realm r already has its own fully shimmed Realm constructor, right? And any further realms it creates, etc...?

[caridy]

yes, that's the normal progression from a shim to a polyfill.

as for the other question, yes, that is correct. we can also be more aggressive and let the factory to set the globalThis.Realm (just like today) but throw if factory is called more than once.

[erights]

I think the factory should remain a pure factory. Given this factoring between polyfill and shim, leave it to the shim to install the result of the polyfill factory. But I say this in ignorance of polyfill common practice.

Altogether, I support this.

[kumavis]

Encountered an issue where the // eslint-disable-line no-eval was triggering this
https://github.com/Agoric/harden/blob/57758759a578d537916a4d35df3de45bc2d0b0bf/src/index.js#L21

came from my own use of harden, and not SES's internal use of harden

[erights]

What transpilers are translating this line first? Might something be
translating

(0,eval)('this')

into

eval('this')

?

If so, that is a horribly incorrect translation. If not, then I am confused
because the regexp should not match that string.