search

endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
825 stars 71 forks source link

Some endo tests are not using @endo/init #1298

Open mhofman opened 2 years ago

mhofman commented 2 years ago

It looks like some endo tests like the marshal package are using @endo/lockdown directly bypassing the @endo/init vetted shims, in particular the node-async_hooks patch. This caused @erights to believe the async_hooks symbols workaround were sometimes not added to Promise.prototype (if debugger opened after lockdown), and a confusing attempt at working around the symptoms in #1297.

Furthermore, we discovered the async_hooks patch is currently running before the SES-shim/lockdown even has a chance to be executed and capture what it can of the initial environment, placing it in a even more sensitive position than trusted shims (which already have too much power since the evaluators are not tamed by the time they execute).

Given the tight coupling of the async-hooks patch with the SES-shim, I'm wondering if a better place for it wouldn't have been in the lockdown package itself.

Something along the lines of in packages/lockdown/src/pre-node.js:

import '../pre.js';
import './node-async_hooks-patch.js';

// eslint-disable-next-line import/export
export * from '../pre.js';
erights commented 2 years ago

Partially addressed by https://github.com/endojs/endo/pull/1299

tgrecojs commented 5 months ago

@kriskowal is this something that still needs attention? if so, then i'd be happy to take it on.

I did a quick investigation and I've found several tests inside of @endo/compartment-mapper that import ses directly.

Below are 2 of such test files.

https://github.com/endojs/endo/blob/a77a1cd8c8063e3bf84b0cb348177dbed7bf5ea5/packages/compartment-mapper/test/test-error-handling.js#L1-L3

https://github.com/endojs/endo/blob/a77a1cd8c8063e3bf84b0cb348177dbed7bf5ea5/packages/compartment-mapper/test/test-transform.js#L1-L2

In the event that this work is still desired then i'll end with the following questions:

  1. Is the ask here to swap out import 'ses' for import '@endo/init' directly within each test that contains the ses import (such as the ones above)?
  2. Is there any scenario in which using @endo/ses-ava would be preferred over @endo/init? If one is required over the other, then additional input as to why it should be used, or why it shouldn't be used would be greatly appreciated 🙂