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): Tame ModuleSource #2469

Merged
merged 4 commits into from
Sep 26, 2024
Merged

feat(ses): Tame ModuleSource #2469

merged 4 commits into from
Sep 26, 2024

Conversation

kriskowal
Copy link
Member

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

None.

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.

@kriskowal
Copy link
Member Author

Agoric SDK integration probe Agoric/agoric-sdk#10144

@kriskowal kriskowal force-pushed the kriskowal-tame-module-source branch from 1bfee1b to dba7bec Compare September 25, 2024 01:09
Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

One thing I don't understand. lockdown calls the repair, to repair things once we have a ModuleSource, whether natively or via the shim. But what brings in the shim? I expected lockdown to do that as well.

packages/ses/NEWS.md Outdated Show resolved Hide resolved
packages/ses/src/commons.js Outdated Show resolved Hide resolved
packages/ses/src/error/console.js Outdated Show resolved Hide resolved
packages/ses/src/get-anonymous-intrinsics.js Outdated Show resolved Hide resolved
packages/ses/src/lockdown.js Outdated Show resolved Hide resolved
packages/ses/src/permits.js Outdated Show resolved Hide resolved
@erights
Copy link
Contributor

erights commented Sep 25, 2024

Why is everything failing under CI?

@kriskowal
Copy link
Member Author

Why is everything failing under CI?

It looks like adding = undefined to the ModuleSource = undefined destructure caused an error in the ModuleSource transform when applied to build dist/ses.*.js. I’m removing the = undefined to verify that’s the root cause, but that is not the end of that investigation. I have probably stumbled into a bug in the module-to-program transform.

One thing I don't understand. lockdown calls the repair, to repair things once we have a ModuleSource, whether natively or via the shim. But what brings in the shim? I expected lockdown to do that as well.

We do not bring in the ModuleSource shim unconditionally because it entrains Babel, so Lockdown must work whether there’s a native ModuleSource, shimmed ModuleSource, or none at all.

@kriskowal kriskowal force-pushed the kriskowal-tame-module-source branch from 36e96c1 to 635e630 Compare September 25, 2024 22:42
@kriskowal kriskowal force-pushed the kriskowal-tame-module-source branch from 635e630 to 4702335 Compare September 25, 2024 22:43
@kriskowal kriskowal requested a review from erights September 25, 2024 22:43
@kriskowal
Copy link
Member Author

I’ve restructured this change around addIntrinsics and tameModuleSource instead of any code in getAnonymousIntrinsics. I learned that we need to bind globalThis.ModuleSource at time of lockdown or we get too picky about whether the ModuleSource shim initializes before or after SES. This is ready for another review.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM!

@kriskowal kriskowal merged commit c1b8b03 into master Sep 26, 2024
16 checks passed
@kriskowal kriskowal deleted the kriskowal-tame-module-source branch September 26, 2024 15:34
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