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

The ses-shim tests on Node 12 despite our min support being Node 18 #2418

Closed
erights opened this issue Aug 19, 2024 · 2 comments · Fixed by #2419
Closed

The ses-shim tests on Node 12 despite our min support being Node 18 #2418

erights opened this issue Aug 19, 2024 · 2 comments · Fixed by #2419
Assignees
Labels
bug Something isn't working

Comments

@erights
Copy link
Contributor

erights commented Aug 19, 2024

#2417 as of this writing fails both "platform-compatibility-test"s, allegedly for Node 18 and Node 20. However, these tests also depend on Node 12, which #2417 does not support.

The culprit seems to be

- name: 'switch to node v12'
uses: actions/setup-node@v3
with:
node-version: '12.x'

where the Node v12 dependency seems to be intentional.

@erights erights added the bug Something isn't working label Aug 19, 2024
@erights
Copy link
Contributor Author

erights commented Aug 19, 2024

The Node 12 dependency comes from @kriskowal 's #1308 , which upgraded it from Node 10 to Node 12.

@kriskowal can we just upgrade this again to 18 and call it a day? Since you authored #1308 , can I assign to you?

@erights
Copy link
Contributor Author

erights commented Aug 19, 2024

Experiment at #2419

erights added a commit that referenced this issue Sep 4, 2024
Staged on #2419 

Closes: #XXXX
Refs: #2414 #2418 #2419 

## Description

#2414  by itself does not work on Node 18 and Node 20 because
- those platforms do not have `Array.prototype.transfer`, so #2414 must
use `structuredClone` instead
- `structuredClone` does exist on Node >= 18, so it should be on
supported platforms (though see #2418 ). However, `structuredClone`
itself is dangerous and so must not be added to the shared intrinsics.
As a result, in #2414 , when `@endo/pass-style` is initialized in a
created compartment, it fails to find either `Array.prototype.transfer`
and `structuredClone

To solve this, @kriskowal suggested that we also shim
`Array.prototype.transfer` if needed during `lockdown`, along with other
repairs. We are avoiding similarly shimming
`Array.prototype.transferToImmutable` because it is not yet standard.
But `Array.prototype.transfer` is standard, and so `lockdown` can
globally shim it before hardening the shared intrinsics.

This PR implements @kriskowal 's suggestion.

### Security Considerations

none
### Scaling Considerations

by itself, none
### Documentation Considerations

nothing signicant.
### Testing Considerations

See #2418 . Aside from that, none
### Compatibility and Upgrade Considerations

On platforms with neither `Array.prototype.transfer` nor a global
`structuredClone`, the ses-shim will *currently* not install an
emulation of `Array.prototype.transfer`. However, once we verify that
endo is not intended to support platforms without both, we may change
lockdown to throw, failing to lock down.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants