search

endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
761 stars 68 forks source link

fix: endow with original unstructured `assert` #2323

Closed erights closed 2 weeks ago

erights commented 2 weeks ago

Closes: #XXXX Refs: https://github.com/Agoric/agoric-sdk/issues/9515 https://github.com/Agoric/agoric-sdk/pull/9514

Description

Addresses https://github.com/Agoric/agoric-sdk/issues/9515 as applied to endo, by doing for endo what https://github.com/Agoric/agoric-sdk/pull/9514 does for agoric-sdk

To address https://github.com/Agoric/agoric-sdk/issues/5672 for endo , we should change all applicable uses of assert to obtain assert by importing it from the @endo/errors package. However, attempts to do so ran into the symptoms reported at https://github.com/Agoric/agoric-sdk/issues/9515 for the reasons reported there -- accidentally using the imported assert as the endowment for the global assert of new Compartments.

This PR prepares the ground for these fixes to https://github.com/Agoric/agoric-sdk/issues/5672 for endo by unambiguously endowing with the original unstructured globalThis.assert, which will remain the correct endowment even when other uses of assert have migrated to the one imported from @endo/errors. By itself, this PR, preceding those fixes to https://github.com/Agoric/agoric-sdk/issues/5672 for endo , is not actually fixing a bug in the sense of a behavioral change. Reviewers, let me know if you think this PR should be labeled a "refactor" because of this.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

anyone gathering endowments for a new compartment should be aware of this issue, and be sure to use globalThis.assert as the assert endowment. Fortunately, this will only be of concern to advanced developers.

Testing Considerations

Since this PR doesn't actually cause any behavioral change, it cannot be tested in place. Rather, its test will be whether #2324 still passes CI once rebased on this PR.

Update: #2324 is now passing CI well enough to consider this PR (#2323) to be adequately tested.

Compatibility Considerations

The point. This PR itself only prepares the ground for the equivalent of https://github.com/Agoric/agoric-sdk/pull/9513 for endo. By itself it has no other effect, and so no other compat issues.

Upgrade Considerations

none

erights commented 2 weeks ago

Reviewers, if you see a way to refactor this change so there is less redundancy and fewer sites that need to be coordinated, please let me know. I do not like the maintenance burden produced by this PR. Or rather, produced by any naive solution to https://github.com/Agoric/agoric-sdk/issues/9515.