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): shim ArrayBuffer.prototype.transfer #2417

Merged
merged 5 commits into from
Sep 4, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Aug 19, 2024

Staged on #2419

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

Description

#2414 by itself does not work on Node 18 and Node 20 because

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.

@erights erights self-assigned this Aug 19, 2024
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch 3 times, most recently from 4822621 to 2911043 Compare August 19, 2024 19:54
@erights
Copy link
Contributor Author

erights commented Aug 19, 2024

See #2418

The platform-compatability-test tests are not marked as required, so solving #2418 does not actually block this PR. But it would be nice to get that fixed first.

@erights erights marked this pull request as ready for review August 19, 2024 20:34
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch from 2911043 to 4dd9bf8 Compare August 19, 2024 22:20
@erights erights changed the base branch from master to markm-repair-arraybuffer-transfer-2 August 19, 2024 22:36
@erights erights force-pushed the markm-repair-arraybuffer-transfer-2 branch from b324de6 to fc32323 Compare August 22, 2024 01:05
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch from 4dd9bf8 to d19e509 Compare August 22, 2024 01:08
@erights erights force-pushed the markm-repair-arraybuffer-transfer-2 branch from fc32323 to 4825656 Compare August 26, 2024 20:30
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch from d19e509 to 585042f Compare August 26, 2024 20:48
@erights erights force-pushed the markm-repair-arraybuffer-transfer-2 branch from a3840c7 to ca4b5fb Compare August 27, 2024 21:09
Base automatically changed from markm-repair-arraybuffer-transfer-2 to master August 27, 2024 21:19
erights added a commit that referenced this pull request Aug 27, 2024
Closes: #2418 
Refs: #2417 #1308 

## Description

Adapted from
#2419 (review)
below

The platform compatibility test specifically validates that SES works on
Node.js 12 and can be deleted since it has vanished into history.
Node.js 12 required special consideration because of its experimental
ESM support. Delete the `test:platform-compatibility` script in the ses
`package.json`, as well as the `test/package` fixture in ci.yml

Immediate motivation explained in #2418 , #2417 broken the
platform-compatibility-test tests because it depends on the platform
providing either `Array.prototype.transfer` or `structuredClone`. Node
supports `transfer` starting with Node 22, but supports
`structuredClone` starting in Node 18. That should be fine, since that
is our declared support floor. This PR changes our one known remaining
dependence on Node 12.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

none beyond the need to explain our platform requirements, which this PR
does not change.
### Testing Considerations

The point. This PR only affects tests, not production code.

Reviewers, should this PR be labeled `test:` instead of `fix:`?

### Compatibility and Upgrade Considerations

After this PR, we will no longer notice further breakage on Node < 18.
That fits with our declared support floor.
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch from 585042f to c874e3c Compare August 27, 2024 21:20
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch from 13e80da to 8dce401 Compare September 2, 2024 21:28
@erights erights force-pushed the markm-repair-arraybuffer-transfer branch from 8dce401 to 06e15fe Compare September 2, 2024 21:30
Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Please consider removing the unnecessary slice call as brand check, and improve support for when the new length doesn't match.

packages/ses/src/lockdown.js Outdated Show resolved Hide resolved
packages/ses/src/shim-arraybuffer-transfer.js Outdated Show resolved Hide resolved
packages/ses/src/shim-arraybuffer-transfer.js Outdated Show resolved Hide resolved
packages/ses/src/shim-arraybuffer-transfer.js Outdated Show resolved Hide resolved
@erights
Copy link
Contributor Author

erights commented Sep 3, 2024

PTAL

@erights
Copy link
Contributor Author

erights commented Sep 4, 2024

Please consider removing the unnecessary slice call as brand check, and improve support for when the new length doesn't match.

Done

Copy link
Contributor

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the previous feedback. There is indeed a better way to copy an array buffer,

packages/ses/src/shim-arraybuffer-transfer.js Outdated Show resolved Hide resolved
packages/ses/src/shim-arraybuffer-transfer.js Outdated Show resolved Hide resolved
packages/ses/test/shim-arraybuffer-transfer.test.js Outdated Show resolved Hide resolved
@erights erights requested a review from mhofman September 4, 2024 01:07
@erights erights changed the title feat(ses): shim ArrayBuffer.p.transfer feat(ses): shim ArrayBuffer.prototype.transfer Sep 4, 2024
@erights erights merged commit 4e5e30e into master Sep 4, 2024
15 checks passed
@erights erights deleted the markm-repair-arraybuffer-transfer branch September 4, 2024 21:20
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.

3 participants