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.

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

feat(ses,module-source): Add ModuleSource shim #2463

Merged
merged 3 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions packages/module-source/NEWS.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
User-visible changes in `@endo/module-source`:

# Next release

- Adds `@endo/module-source/shim.js` to shim `globalThis.ModuleSource`.
The shim currently replaces the native `globalThis.ModuleSource` if present.

# v1.0.0 (2024-07-30)

- Renamed from `@endo/static-module-record` to `@endo/module-source` exporting
Expand Down
1 change: 1 addition & 0 deletions packages/module-source/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"main": "./index.js",
"exports": {
".": "./index.js",
"./shim.js": "./shim.js",
"./package.json": "./package.json"
},
"scripts": {
Expand Down
10 changes: 10 additions & 0 deletions packages/module-source/shim.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/* global globalThis */

import { ModuleSource } from './index.js';

Object.defineProperty(globalThis, 'ModuleSource', {
value: ModuleSource,
enumerable: false,
writable: true,
configurable: true,
});
29 changes: 29 additions & 0 deletions packages/module-source/src/module-source.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,32 @@ export function ModuleSource(source, opts = {}) {
this.__needsImportMeta__ = needsImportMeta;
freeze(this);
}

// AbstractModuleSource
// https://github.com/tc39/proposal-source-phase-imports?tab=readme-ov-file#js-module-source
//
// We are attempting to ensure that a JavaScript shim (particularly ses) is
// forward-compatible as the engine evolves beneath it, with or without this
// ModuleSource shim, and with our without a native AbstractModuleSource which
// remains undecided.
// Lockdown does not gracefully handle the presence of an unexpected prototype,
// but can tolerate the absence of an expected prototype.
// So, we are providing AbstractModuleSource since we can better tolerate the
// various uncertain futures.
//
// WebAssembly and ModuleSource are both in motion.
// The Source Phase Imports proposal implies an additional AbstractModuleSource
// layer above the existing WebAssembly.Module that would be shared by
// the JavaScript ModuleSource prototype chains.
// At time of writing, no version of WebAssembly provides the shared base
// class, and the ModuleSource *shim* gains nothing from sharing one when that
// prototype when it comes into being.
// So, we do not attempt to entangle our AbstractModuleSource with
// WebAssembly.Module.

function AbstractModuleSource() {
// no-op, safe to super()
}

Object.setPrototypeOf(ModuleSource, AbstractModuleSource);
Object.setPrototypeOf(ModuleSource.prototype, AbstractModuleSource.prototype);
gibson042 marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 2 additions & 0 deletions packages/ses/NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ User-visible changes in `ses`:
- Node 18, Node 20, and all browsers have `structuredClone`
- Node <= 16 have neither, but are also no longer supported by Endo.
- Now exports separate layer for console shim: `ses/console-shim.js`.
- Adds permits for `ModuleSource` if present, either the native implementation
or from `@endo/module-source/shim.js`.

# v1.8.0 (2024-08-27)

Expand Down
14 changes: 14 additions & 0 deletions packages/ses/src/get-anonymous-intrinsics.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,5 +161,19 @@ export const getAnonymousIntrinsics = () => {
);
}

if (globalThis.ModuleSource) {
const AbstractModuleSourcePrototype = getPrototypeOf(
globalThis.ModuleSource.prototype,
);
intrinsics['%AbstractModuleSourcePrototype%'] =
AbstractModuleSourcePrototype;
intrinsics['%AbstractModuleSource%'] =
AbstractModuleSourcePrototype.constructor;
}
Comment on lines +164 to +172
Copy link
Member Author

Choose a reason for hiding this comment

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

A valid but annoying feedback reviewers might provide here is that we can’t anticipate whether ModuleSource will land in the language with or without an AbstractModuleSource on its prototype chain, so we could hedge our bets and add a repair for ModuleSource to force it to appear one way or the other, so that lockdown() doesn’t break if the AbstractModuleSource is absent. Or, we could go the other way and not have AbstractModuleSource by default, in which case SES would just delete it and issue a warning if it showed up.

Copy link
Contributor

Choose a reason for hiding this comment

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

You read my mind, but that's just the risk with front-running proposals. No suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Capturing here my reasoning for keeping AbstractModuleSource since it appears to be the path of least regret.

  1. We shim AbstractModuleSource and include it in SES permits:
    1. The application includes module-source/shim.js:
      1. The VM implements AbstractModuleSource: ✅ Lockdown’s fine
      2. The VM does not implement AbstractModuleSource: ✅ Lockdown’s fine. The shim currently stomps the native implementation. If it didn’t stomp the native implementation, Lockdown would tolerate the incomplete intrinsics.
    2. The application excludes module-source/shim.js:
      1. The VM implements AbstractModuleSource: ✅ Lockdown’s fine
      2. The VM does not implement AbstractModuleSource: ✅ Lockdown’s fine, but intrinsics are incomplete
  2. We do not shim AbstractModuleSource and exclude it in SES permits:
    1. The application includes module-source/shim.js:
      1. The VM implements AbstractModuleSource: ✅ :man-shrugging: The shim currently stomps the native implementation, so this works out. But, if the shim surfaced the native implementation (and perhaps feature-checks that the native implementation behaves just like the shim deigning to fall through to native)
      2. The VM does not implement AbstractModuleSource: ✅ Lockdown’s fine
    2. The application excludes module-source/shim.js:
      1. The VM implements AbstractModuleSource: 💔 Lockdown is surprised to find AbstractModuleSource and chooses throw rather than attempt a repair. We don’t have the module source shim in play, so there’s nothing it can do to mitigate. We can’t require the module-source/shim.js because it entrains Babel.
      2. The VM does not implement AbstractModuleSource: ✅ Lockdown’s fine


if (globalThis.ModuleSource) {
intrinsics['%ModuleSourcePrototype%'] = globalThis.ModuleSource.prototype;
}

return intrinsics;
};
10 changes: 7 additions & 3 deletions packages/ses/src/permits-intrinsics.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,8 +156,10 @@ export default function whitelistIntrinsics(
return;
}

// We can't clean [[prototype]], therefore abort.
throw TypeError(`Unexpected intrinsic ${path}.__proto__ at ${protoName}`);
// We can't clean [[Prototype]], therefore abort.
throw TypeError(
`Unexpected [[Prototype]] at ${path}.__proto__ (expected ${protoName || '%ObjectPrototype%'})`,
);
}

/*
Expand Down Expand Up @@ -212,7 +214,9 @@ export default function whitelistIntrinsics(
}
}

throw TypeError(`Unexpected whitelist permit ${permit} at ${path}`);
throw TypeError(
`Unexpected property ${prop} with permit ${permit} at ${path}`,
);
}

/*
Expand Down
23 changes: 23 additions & 0 deletions packages/ses/src/permits.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,12 @@ export const universalPropertyNames = {

// ESNext

// https://github.com/tc39/proposal-source-phase-imports?tab=readme-ov-file#js-module-source
ModuleSource: 'ModuleSource',

lockdown: 'lockdown',
harden: 'harden',

HandledPromise: 'HandledPromise', // TODO: Until Promise.delegate (see below).
};

Expand Down Expand Up @@ -1505,6 +1509,25 @@ export const permitted = {
resolve: fn,
},

// https://github.com/tc39/proposal-source-phase-imports?tab=readme-ov-file#js-module-source
'%AbstractModuleSourcePrototype%': {
constructor: '%AbstractModuleSource%',
'@@toStringTag': getter,
},
'%AbstractModuleSource%': {
'[[Proto]]': '%FunctionPrototype%',
prototype: '%AbstractModuleSourcePrototype%',
},
'%ModuleSourcePrototype%': {
'[[Proto]]': '%AbstractModuleSourcePrototype%',
constructor: 'ModuleSource',
'@@toStringTag': getter,
},
ModuleSource: {
'[[Proto]]': '%AbstractModuleSource%',
prototype: '%ModuleSourcePrototype%',
},

Promise: {
// Properties of the Promise Constructor
'[[Proto]]': '%FunctionPrototype%',
Expand Down
21 changes: 20 additions & 1 deletion packages/ses/test/module-source.test.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,24 @@
/// <reference types="ses">

import test from 'ava';
import '../index.js';
import { ModuleSource } from '@endo/module-source';
import '@endo/module-source/shim.js';

lockdown();

test('module source property/prototype graph and hardening', t => {
const AbstractModuleSource = Object.getPrototypeOf(ModuleSource);
t.is(
Object.getPrototypeOf(ModuleSource.prototype),
AbstractModuleSource.prototype,
);

t.truthy(Object.isFrozen(ModuleSource));
t.truthy(Object.isFrozen(AbstractModuleSource));
t.truthy(Object.isFrozen(ModuleSource.prototype));
t.truthy(Object.isFrozen(AbstractModuleSource.prototype));
});

test('module source constructor', t => {
const msr = new ModuleSource(`
import foo from 'import-default-export-from-me.js';
Expand Down Expand Up @@ -42,3 +57,7 @@ test('module source constructor', t => {
'ModuleSource imports should be frozen',
);
});

test('ModuleSource is a shared intrinsic', t => {
t.truthy(ModuleSource === new Compartment().globalThis.ModuleSource);
});
Loading