endojs
`with (scopeProxy) { foo() }` leaks scopeProxy if foo mentions `this`.
Typing valueOf() into the evaluator shim playground (demo/index.html) and hitting the Evaluate button causes the error
please report internal shim error: unexpected scope handler trap called: ownKeys
Unlike other surprising scopeProxy traps, this does not look like an engine bug. We see identical symptoms on Brave, FF, and Safari.
[edit by @kriskowal 2020-08-12:]
Actions needed (to be independently tracked, and this issue closed)
- ~~Create a specification proposal that introduces a
hiddenWithsymbol thatwithwould respect.~~ - [ ] Document this known-limitation of SES-shim, ~~barring eventual engine support for a
hiddenWithsymbol.~~ - [ ] Confirm that ~~absent
hiddenWith,~~ revealing theScopeProxydoes not break containment.
Due to https://github.com/google/caja/wiki/SES#this-binding-of-global-function-calls
The weird trap isn't happening in evaluating the user code. Rather, the valueOf() causes the scopeProxy to leak as the completion value. The playground code then treats it as a more normal object, provoking the surprising trap.
The scopeProxy was never supposed to leak. The problem is only caused by with lookup of a function that mentions this, followed by a function-call-syntax invocation of that function.
At an unmodified normal JS prompt,
const obj = { foo() {'use strict'; return this;} };
with (obj) { foo() === obj; } // true
with (obj) { (1,foo)() === undefined; } // true
This leakage of obj on line 2 is according to the spec, and is an unrepairable deficiency emulating global scope with with. Had we noticed it in time, we likely could have fixed it in ES5 strict-mode. But even by https://github.com/google/caja/wiki/SES#this-binding-of-global-function-calls (2015) it was already too late. Attn @allenwb @jfparadis @caridy @warner @XmiliaH
What this means under the evaluator-shim and the realms-shim, and therefore under SES:
const e = new Evaluator();
function foo() { return this; }
e.evaluate('foo()', { foo }); // evaluates to the scopeProxy.
Amusingly, once Agoric/SES-beta#18 is fixed by Agoric/evaluator-shim#3 the following works
const e = new Evaluator();
function foo() { return this; }
e.evaluate('foo()', Object.freeze({ foo })); // undefined
because foo becomes an optimized const variable.
If we cannot practically prevent the scopeProxy from leaking, we need to ensure that no vulnerability follows from leaking the scopeProxy. We could probably make the scopeProxy no more powerful than the safeGlobal plus the values of the endowments. That should be both possible and adequate.
(grr...it's too easy to accidentally loose/close a github tab with an unsaved text entry field...trying again)
This should be easy (at least at the ECMA-262 spec. level) to fix. Object Environment Records already have a specification mechanism that controls whether or not they make their backing object available for inclusion in resolved References. This is controlled by the setting their withValue to true. With statements normally do that for all with objects. What we need to do is not set withValue to true when the with object is a ScopeProxy.
There are various ways this might be done in the spec, but I suggest doing all the work in with statement evaluation. Specifically we need to put a "not a ScopeProxy" guard on step 5 which currently unconditionally sets withValue.
What does the guard look like? We really don't want to build knowledge of a specific ScopeProxy definition into this level of ECMA-262. Instead, I suggest we define a new well-known symbol named @@hiddenWith. The guard would be: HasOwnProperty(obj, @@hiddenWith) is false. The definition of ScopeProxy (its proxy handler) would ensure that they always report that they have a @@hiddenWith own property. That's it!
Actually updating implementations to do this is probably not quite so simple, I assume that they don't have anything that exactly corresponds to an Object environment record with a withValue flag. But I suspect that adapting an implementation to conform to this updated specification would be a modest effort—and well worth it.
I left out of my redo: this ECMA-262 specification change appears be backwards compatible with all existing ES code.
This is a use of this, not a mention of this, no?
... function foo() { return this; } ...
Well, it is also a mention of this. But good point. I genuinely do not know what the least confusing short title would be. "Uses this" can be misunderstood in other ways (such as "uses the value that this evaluates to"). I'll just let these notes in this issue thread clarify, and leave the title as is. Thanks.
We've noticed that some Web APIs such as fetch are sensitive to the this / context. If it's null-ish or a real window object, it works fine, but fails with a TypeError if any other object: Failed to execute 'fetch' on 'Window': Illegal invocation (message on v8/Chrome) or 'fetch' called on an object that does not implement interface Window (message in SpiderMonkey/Gecko).
This fidelity bug of the shim causes code that would otherwise work to break if these functions are endowed to a compartment's global.
It still leaks, but only the globalThis regular object.
Main problem left is for module lexical created by the shim, as detailed in #912.
Should we rename the title of this issue? It might be good to keep tracking an engine level change allowing to disable the current with context behavior.
Narrowed scope and posted for follow-up: https://github.com/endojs/endo/issues/1954
