Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: reenable indirect eval #26

Closed
wants to merge 1 commit into from
Closed

fix: reenable indirect eval #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 22 additions & 16 deletions src/evaluators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,22 +96,7 @@ export function createSafeEvaluatorFactory(unsafeRec, safeGlobal, transforms) {
);
endowments = endowmentState.endowments;

// 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
// things in terms of shimming the global lexical scope).
// writeable-vs-nonwritable == let-vs-const, but there's no
// global-lexical-scope equivalent of an accessor, outside what we can
// explain/spec
const scopeTarget = create(
safeGlobal,
getOwnPropertyDescriptors(endowments)
);
const scopeProxy = new Proxy(scopeTarget, scopeHandler);
const scopedEvaluator = apply(scopedEvaluatorFactory, safeGlobal, [
scopeProxy
]);
let scopedEvaluator;

// We use the the concise method syntax to create an eval without a
// [[Construct]] behavior (such that the invocation "new eval()" throws
Expand Down Expand Up @@ -155,6 +140,27 @@ export function createSafeEvaluatorFactory(unsafeRec, safeGlobal, transforms) {
// of Function in any compartment of the same root realm.
setPrototypeOf(safeEval, unsafeFunction.prototype);

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the comment.

// things in terms of shimming the global lexical scope).
// writeable-vs-nonwritable == let-vs-const, but there's no
// global-lexical-scope equivalent of an accessor, outside what we can
// explain/spec
const scopeTarget = create(safeGlobal, {
...getOwnPropertyDescriptors(endowments),
eval: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// We need this on the scopeTarget to permit indirect eval.
value: safeEval,
writable: false,
enumerable: false,
configurable: true
}
});
const scopeProxy = new Proxy(scopeTarget, scopeHandler);
scopedEvaluator = apply(scopedEvaluatorFactory, safeGlobal, [scopeProxy]);

assert(getPrototypeOf(safeEval).constructor !== Function, 'hide Function');
assert(
getPrototypeOf(safeEval).constructor !== unsafeFunction,
Expand Down
5 changes: 4 additions & 1 deletion test/module/evaluators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ test('createSafeEvaluatorWhichTakesEndowments - options.transforms', t => {
});

test('createSafeEvaluatorWhichTakesEndowments', t => {
t.plan(9);
michaelfig marked this conversation as resolved.
Show resolved Hide resolved
t.plan(11);

// Mimic repairFunctions.
// eslint-disable-next-line no-proto
Expand All @@ -150,6 +150,9 @@ test('createSafeEvaluatorWhichTakesEndowments', t => {
t.equal(safeEval('foo', endowments), 1);
t.equal(safeEval('bar', endowments), 4);

t.equal(safeEval(`(1, eval)('foo')`, endowments), 1);
t.equal(safeEval(`(1, eval)('bar')`, endowments), 4);

t.throws(() => safeEval('foo = 5', {}), TypeError);
safeEval('bar = 6', {});

Expand Down