fix: reenable indirect eval #26
Conversation
|
@erights, would you please have a look at this? |
|
Sorry! |
| // explain/spec | ||
| const scopeTarget = create(safeGlobal, { | ||
| ...getOwnPropertyDescriptors(endowments), | ||
| eval: { |
There was a problem hiding this comment.
Did you mean to have this eval override what may have been contributed as an eval in the endowments? I think it's better to enable the endowments to override this setting, in which case eval: safeEval would be an overridable default.
I do see in the todo comments above that we should ban eval from the endowments, in which case this isn't an observable issue. However, I can no longer reconstruct why we should prohibit it.
Did we not put eval: safeEval on the safeGlobal? Recently, I've been thinking that the scopeTarget is a better place anyway, for both the safe eval and the safe Function constructor. But that has not been the plan. Does the safe Function constructor appear on the safeGlobal? In any case, they should both appear in the same place.
Thus, if we do place Function on the scopeTarget, it should go either before or after the endowments, in the same position as eval.
There was a problem hiding this comment.
Today I experimented with putting safe eval and Function constructor on the scopeTarget. Some tests are now failing because they expect to do r.global.Function(...) or new r.global.Function(...), and cannot find the Function constructor there.
What should be the expected behaviour? I can certainly rewrite the tests to use r.evaluate('Function')(...) and new (r.evaluate('Function'))(...), but I need to make sure the plan for this whole scopeTarget versus safeGlobal thing is fully-baked.
| // todo (shim limitation): scan endowments, throw error if endowment | ||
| // overlaps with the const optimization (which would otherwise | ||
| // incorrectly shadow endowments), or if endowments includes 'eval'. Also | ||
| // prohibit accessor properties (to be able to consistently explain |
There was a problem hiding this comment.
This comment is stale. Nothing in this PR causes it to be stale, since it is in a todo anyway. However, our safe module plans depend on our ability to install accessors on the scopeTarget.
There was a problem hiding this comment.
I'll update the comment.
|
This turns out to be a bad interaction between SES doesn't whitelist the |
closes #25
The issue preventing indirect eval was that the scoped evaluator looks up
evalon the proxy target, but it is not defined in either the safeGlobal, nor endowments. This fix makes it work again (if it did before) but needs a careful review.