inconsistent order of host and type checks for eval

[mikesamuel]

JS engines are not consistent in the order in which they apply type checks and host callouts during eval.

eval applied to non string arguments is the identity function because of step 2 of the definition of PerformEval:

18.2.1.1. Runtime Semantics: PerformEval ( x, evalRealm, strictCaller, direct )

The abstract operation PerformEval with arguments *x*, *evalRealm*, *strictCaller*, and *direct* performs the following steps:

1. Assert: If direct is false, then strictCaller is also false.
2. If Type(x) is not String, return x.
3. ...

This type check happens after the call to HostEnsureCanCompileStrings:

18.2.1 eval ( x )

The `eval` function is the %eval% intrinsic object. When the `eval` function is called with one argument *x*, the following steps are taken:

1. Assert: The execution context stack has at least two elements.
2. Let callerContext be the second to top element of the execution context stack.
3. Let callerRealm be callerContext's Realm.
4. Let calleeRealm be the current Realm Record.
5. Perform ? HostEnsureCanCompileStrings(callerRealm, calleeRealm).
6. Return ? PerformEval(x, calleeRealm, false, false).

but when the argument is not a string, some engines do not seem to perform HostEnsureCanCompileStrings.

$ npx node@10 --disallow_code_generation_from_strings -e 'console.log(eval(0))'; \
   echo Exit status $?
0
Exit status 0

$ npx node@10 --disallow_code_generation_from_strings -e 'console.log(eval("0"))'; \
  echo Exit status $?
[eval]:1
console.log(eval("0"))
        ^

EvalError: Code generation from strings disallowed for this context
    at [eval]:1:9
    at Script.runInThisContext (vm.js:119:20)
    at Object.runInThisContext (vm.js:326:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:701:30)
    at evalScript (internal/bootstrap/node.js:589:27)
    at startup (internal/bootstrap/node.js:265:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3)
Exit status 1

---

I tested this in various browsers via

<meta http-equiv="Content-Security-Policy" content="script-src 'nonce-test'">
<script nonce="test">
  console.group('eval number');
  try {
    console.log(eval(0));
  } finally {
    console.groupEnd();
  }
</script>
<script nonce="test">
  console.group('eval string');
  try {
    console.log(eval("0"));
  } finally {
    console.groupEnd();
  }
</script>

Browser	eval(0)
Chrome 73.0	allowed
Firefox 66.0	rejected
Safari 12.0	allowed

I don't have IE / Edge handy.

[devsnek]

It kinda makes sense to allow eval on non-strings, as it's just identity.

[mikesamuel]

Yeah. I could file bugs against Chrome and Safari or we could treat it as a web reality issue.

[ljharb]

Given that it’s a host-defined operation, i don’t see how the spec gets any opinion on it. Your test case isn’t necessarily evidence that HostCanCompileStrings isn’t called, it might also be that it chooses not to throw on non-strings.

[devsnek]

@ljharb HostEnsureCanCompileStrings doesn't know what is passed to eval, it just has the callee and caller context.

[ljharb]

Sure, but it’s host-defined. They can easily track what was passed to eval in a side channel and check it in the abstract op; there’s nothing in the spec i can see that prohibits that.

[devsnek]

reading the source, v8 checks for can compile strings after it checks if the input is a string, while the spec says before.

[ljharb]

Sure, but what i mean is, i don’t think the spec currently mandates doing the same thing in that abstract op regardless of input type. v8 could certainly swap the ordering but I’m not sure it’s really a bug.

[zenparsing]

Chakra also returns non-strings before checking whether eval is disabled. Perhaps @domenic has some thoughts?

For reference, HostEnsureCanCompileStrings was introduced in #451.

[mikesamuel]

@ljharb, @devsnek Somewhat off topic, but I'm proposing that HostEnsureCanCompileStrings get that context.

[mikesamuel]

@ljharb, if you think this is not a web reality issue, I can file issues against Chrome & Safari and they can always choose to close them WAI.

[ljharb]

I don’t have a confident opinion formed :-) just not sure if this is something host-dependent or not. If so, then it’d be up to each engine to decide to conform or not; if not, then i think the spec language might need to be more explicit to make it clear.

[annevk]

Sure, but it’s host-defined. They can easily track what was passed to eval in a side channel and check it in the abstract op; there’s nothing in the spec i can see that prohibits that.

That's generally not how we write specifications. If an algorithm has access to such a value we'd make that explicit. This line of reasoning would mean we don't even need a hook for hosts at all.

Given the name of the hook, it would make more sense for Firefox to change and to slightly adjust when this hook is called in the specification.

[ljharb]

@annevk in that case, it seems clear that there’s only two options to move forward - either change the spec to do this check after returning for non-strings, or change engines to do the check before.

If Firefox changes so that the table in the OP is “allowed”, then we’d want to change the spec?

(please confirm im reading things right)

[annevk]

Yeah, unless there's a compelling reason to run this check for non-strings, but I can't think of any.

[devsnek]

strawman linked above wants to be called for every value, not just strings: https://github.com/mikesamuel/proposal-hostensurecancompilestrings-passthru

[mikesamuel]

@annevk @devsnek If that proceeds it might incorporate an editorial change to s/CanCompileStrings/CanCompile/, but I wanted to keep the spec diff minimal early on.

The non-string portion is there only to prove prior-attestation.

[ljharb]

Can you elaborate on what prior attestation means?

[allenwb]

This is a specification bug introduced post ES6.

In the ES6 spec there are no observable actions that happen before the the type check on the argument. This behavior dates all the way back to the ES1 spec:

It seems like the current spec. has an (unintentional??) breaking change.

[mikesamuel]

@allenwb Thanks for explaining.

Should I prep an editorial change that consolidates the callouts in both indirect and direct eval into PerformEval after the type check?

[jmdyck]

consolidates the callouts in both indirect and direct eval into PerformEval after the type check?

In #450, @domenic said "[putting the hook in] PerformEval is not sufficient since we need both the calleeRealm and the callerRealm to match web reality."

That is, PerformEval has the parameter _evalRealm_, which is the callee realm, but it does not currently have a handle on the caller realm, to pass to HECCS. So a couple possibilities:

• do the above consolidation if PerformEval is given another parameter for the caller realm, or
• leave calls to HECCS as-is, and hoist the identity-for-non-strings step out of PerformEval, putting it somewhere before each call to HECCS. (And PerformEval can then assert that _x_ is a string.)

Personally, I think I prefer the second.

[zenparsing]

I would probably lean toward the first option, as keeping all of the "eval" logic in one place makes it less likely for readers to miss all of the important semantics.

Either way, it appears that we need to decide whether we want to require all implementations to always allow non-strings.

[mikesamuel]

@jmdyck Good point.

@zenparsing I'll ecmarkupize both so we can see how they look in context.

[mikesamuel]

https://mikesamuel.github.io/eval-in-order/ uses <ins> and <del> to show the relevant diffs.

[jmdyck]

In option 1, PerformEval's preamble will need to be tweaked.

In option 2, in the direct eval code, you need to change x to evalText. Also, in both spots, you could move the type-test earlier (to avoid code that isn't relevant to the non-string case).

[mikesamuel]

@jmdyck

In option 1, edited the argument list description.

In option 2, changed to "1. If Type(evalText) is not String, return evalText.".
Moved the type check in the indirect case to after the initial assert, and in the direct case, moved it to directly after "Let evalText be the first element of argList."

[ljharb]

Option 2 seems much cleaner and simpler to me; ie, it no longer overloads “perform eval” with the unrelated task of “if it’s not a string, don’t try to eval it”.

[jmdyck]

@mikesamuel: Thanks.

[option 2 eval] Moved the type check in the indirect case to after the initial assert

The initial assert (that there's at least two elements on the execution context stack) is there so reassure the reader that it's valid for step 3 to refer to the "second to top" element. It doesn't make much sense to separate those two steps, so I'd suggest moving the type-check up one more spot.

[mikesamuel]

@jmdyck Assuming you're talking about the below, maybe I'm getting too DbC, but something about fast-pathing out of caller/callee contract check seems off to me.

eval ( x )

The eval function is the %eval% intrinsic object. When the eval function is called with one argument x, the following steps are taken:

1. Assert: The execution context stack has at least two elements.
2. If Type(x) is not String, return x.

[littledan]

Swapping the order of the tests in the spec as a web reality change seems reasonable, or updating browsers also seems fine if they are interested. I agree with others above that it's important that the behavior be well-defined. It'd be great to add a web-platform-tests test for whatever semantics we decide on, to avoid divergence in the future. cc @domenic

[mikesamuel]

@littledan, Would https://github.com/web-platform-tests/wpt/tree/master/js/behaviours be an appropriate place to put them?

The other content in https://github.com/web-platform-tests/wpt/tree/master/content-security-policy seems an awkward fit.

[littledan]

@mikesamuel Seems good to me. We can discuss further in a code review. I'd recommend cc'ing @Ms2ger on that, who's more of a wpt expert than me.

[mikesamuel]

@littledan 2 separate PRs for spec change and test out. Will schedule a few minutes next meeting to request consensus.