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

Move node async_hooks taming to ses #2711

Open
mhofman opened this issue Feb 5, 2025 · 1 comment
Open

Move node async_hooks taming to ses #2711

mhofman opened this issue Feb 5, 2025 · 1 comment
Labels
enhancement New feature or request

Comments

@mhofman
Copy link
Contributor

mhofman commented Feb 5, 2025

What is the Problem Being Solved?

Node's async_hooks are pretty intrusive and attempt to assign properties onto promise instances, sometimes after those promise objects have been observed by user code. #1115 introduced a monkey patch to async_hooks that made sure async_hooks would continue working with frozen promises, and not use any values that would break the inert properties of passStyleOf. However this was done as a sort of shim in @endo/init, while really the problem occurs as soon as anyone attempts to harden a promise, and/or use the promise with @endo/marshal, meaning it's a taming that belongs along with the other tamings for SES. Using SES without async_hooks has been identified as a concern in #1298 and re-discovered as the cause in #2700.

Furthermore the async_hooks patch runs before SES lockdown, and @endo/init doesn't have the same double init protection than SES has, resulting in obscure errors (#2653).

Description of the Design

Move the async_hooks taming as a SES lockdown operation. This however requires some further shenanigans: the async_hooks patch relies on importing the node:async_hooks module, and enabling temporarily the async_hooks system. Unfortunately, importing a module is an async operation if done imperatively. One possible workaround would be to import createRequire from node:module, and synchronously require('node:async_hooks') during lockdown based on options. That still mandates the usage of a node specific entrypoint for the SES shim that can declaratively import node:module, but all the taming logic could be moved to lockdown execution, which include double lockdown protection, or skipped altogether depending on taming options (which could then be used by the unsafe-fast init entrypoint, resolving #1688)

Security Considerations

None, we've been trusting the properties of the async_hooks taming.

Scaling Considerations

None

Test Plan

New test covering the lockdown options and possible non Node envs.

Compatibility Considerations

This is technically an additive feature, but given the changes in entrypoints, we may need to be extra careful here.

Upgrade Considerations

See above

@mhofman
Copy link
Contributor Author

mhofman commented Feb 5, 2025

We should actually double check that importing async_hooks has side effects, orelse we may be able to simply statically import it, and just enabled it during lockdown

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

No branches or pull requests

1 participant