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

Conversation

kriskowal
Copy link
Member

Refs: #2252

Description

To test feature the degree of compatibility between a version of XS, XS with Endo shims, and Node.js with Endo shims, we need a shim for ModuleSource proper. This lets us create an environment with a global ModuleSource, where ModuleSource is a shared intrinsic of all Compartments.

This change both introduces the shim and the necessary accommodations for the existence of a global ModuleSource shared intrinsic in SES.

Security Considerations

The new ModuleSource is subject to hardening of shared intrinsics during SES lockdown. A failure to harden the shared intrinsic would lead to a potential for interference or communication between isolated compartments.

Scaling Considerations

None.

Documentation Considerations

None.

Testing Considerations

This includes a test for SES that verifies that ModuleSource is properly propagated and hardened.

Compatibility Considerations

None.

Upgrade Considerations

None.

@kriskowal kriskowal changed the title Kriskowal module source shim feat(ses,module-source): Add ModuleSource shim Sep 20, 2024
Comment on lines +164 to +172
if (globalThis.ModuleSource) {
const AbstractModuleSourcePrototype = getPrototypeOf(
globalThis.ModuleSource.prototype,
);
intrinsics['%AbstractModuleSourcePrototype%'] =
AbstractModuleSourcePrototype;
intrinsics['%AbstractModuleSource%'] =
AbstractModuleSourcePrototype.constructor;
}
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

@kriskowal kriskowal force-pushed the kriskowal-module-source-shim branch 2 times, most recently from fb9a58e to 928cd19 Compare September 20, 2024 19:03
@kriskowal kriskowal merged commit 349f0e8 into master Sep 24, 2024
15 checks passed
@kriskowal kriskowal deleted the kriskowal-module-source-shim branch September 24, 2024 00:53
kriskowal added a commit that referenced this pull request Sep 24, 2024
This reverts commit 0d210a3.

## Description

This change caused a failure for Lockdown on XS in Agoric SDK,
indicating that it is a breaking change.

### Testing Considerations

Reverting this change is a temporary mitigation.
We will first address the gap in XS CI coverage and then persue a fix.

### Compatibility Considerations

We will pursue a follow-up change that addresses XS coverage in CI #2463
and then consider repairing the native `ModuleSource` intrinsic under
`repairIntrinsics` in SES.
kriskowal added a commit that referenced this pull request Sep 26, 2024
Refs: #2463, #2252

## Description

In #2463 we introduced ModuleSource to the SES permits, but this
interacted catastrophically with the native XS ModuleSource. We reverted
this change #2468 to unbreak Agoric SDK integration. This change
reintroduces the ModuleSource permits, such that they are compatible
with both XS and the `@endo/module-source/shim.js`, which anticipates
the introduction of an AbstractModuleSource base class. Because SES can
more gracefully tolerate the absence of an permitted [[Proto]] than the
presence of a non-permitted [[Proto]], this adjusts the shim to ensure
that the AbstractModuleSource shape exists as a side-effect of
repairs/taming, before permits are applied.

### Security Considerations

Increase in memory safety exposure in native code implementation of ModuleSource where applicable.

### Scaling Considerations

None.

### Documentation Considerations

This change reintroduces a note in NEWS.md for the next release.

### Testing Considerations

The prior regression went unnoticed because we did not yet have CI for
XS #2465. With this change, `yarn test:xs` in SES validates the shim on
XS. We also test `@endo/module-source/shim.js` in
`ses/test/module-source.test.js` on Node.js.

### Compatibility Considerations

This change is designed to tolerate either path forward for the
language, whether or not it accepts an AbstractModuleSource base class.

### Upgrade Considerations

None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants