fix eval(<non‑string>) and eval.length
#306
Conversation
|
Before proceeding on this, we should be aware of @mikesamuel's proposals to change this aspect of the EcmaScript standard to give hosts (and CSP) more control over the response to non-string Whatever is done here should also be done to the compartment shim. See endojs/endo#481 |
|
I think you're referring to https://github.com/tc39/proposal-dynamic-code-brand-checks |
|
@mikesamuel yes, thanks. |
|
Please pardon the very late review. I’m cleaning out the skeletons in my review queue. This change looks good to me. |
src/safeEval.js
Outdated
|
|
||
| // safeEval's prototype RootRealm's value and instanceof Function | ||
| // is true inside the realm. It doesn't point at the primal realm | ||
| // value, and there is no defense against leaking primal realm | ||
| // intrinsics. | ||
|
|
||
| defineProperties(safeEval, { | ||
| // `eval.length` is already initialized correctly. | ||
| // Ensure that `eval.name` is also correct: | ||
| name: { value: 'eval' }, |
There was a problem hiding this comment.
It might be worth making explicit that this property is non-writable, non-enumerable, and non-configurable.
There was a problem hiding this comment.
This property has the defaults of { writeable: false, enumerable: false, configurable: true }, as it’s defined by ECMAScript:
| @@ -166,6 +166,26 @@ test('degenerate-pattern-match-argument', t => { | |||
| t.end(); | |||
| }); | |||
|
|
|||
| test('eval-own-properties', t => { | |||
| const r = Realm.makeRootRealm(); | |||
There was a problem hiding this comment.
This doesn’t exist any more. This might be a hard rebase.
| `), | ||
| true, | ||
| 'eval(<non-string>) returns <non-string>' | ||
| ); |
There was a problem hiding this comment.
So much time has passed, we will probably want a test that verifies the immutability of the eval inside a Compartment.
| value: 'eval', | ||
| writable: false, | ||
| enumerable: false, | ||
| configurable: true |
There was a problem hiding this comment.
This might be fine, but I want to be sure that the safe evaluator gets hardened later. That said, I don’t think we ever need safe eval to have configurable name, so making this false should be fine.
See https://tc39.es/ecma262/#sec-performeval: