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)!: Remove support for globalLexicals #1343

Merged
merged 3 commits into from
Oct 28, 2022

Conversation

kriskowal
Copy link
Member

Fixes #904

BREAKING CHANGE: Removes support for globalLexicals. To our knowledge, there are no production uses for globalLexicals. They currently could leak because moduleLexicals and globalLexicals used the same scope object, so properties of one would leak to the other with crafted modules. We had an opportunity to plug the leak at the cost of a fifth scope in all evaluators, but elected to remove the unnecessary complexity instead.

@kriskowal
Copy link
Member Author

In lieu of #1341

@mhofman
Copy link
Contributor

mhofman commented Oct 26, 2022

@warner do we still use the meter transform in agoric-sdk, or is this safe to land?

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.

Looking good.

Layering question, should the second commit removing globalLexicals from compartment-mapper not come first before the removal from the shim?

packages/ses/src/compartment-evaluate.js Show resolved Hide resolved
packages/ses/src/make-safe-evaluator.js Show resolved Hide resolved
@kriskowal
Copy link
Member Author

@mhofman It’s perfectly sensible to reverse the commit order. Note that I’ve added another one to propagate the change to @endo/import-bundle.

@kriskowal kriskowal requested a review from warner October 27, 2022 00:31
@warner
Copy link
Contributor

warner commented Oct 27, 2022

@warner do we still use the meter transform in agoric-sdk, or is this safe to land?

Nope, we removed that over a year ago (in 5b9563849fa7ca2f26b4ca7c55f10d1d37334f46), at which point we stopped using the inescapableTransforms and inescapableGlobalLexicals options of importBundle(). This is safe to land.

Copy link
Contributor

@warner warner left a comment

Choose a reason for hiding this comment

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

LGTM, but consider the docs example

inescapableTransforms,
inescapableGlobalLexicals);
const endowments = {};
const WrappedCompartment = wrapInescapableCompartment(
Copy link
Contributor

Choose a reason for hiding this comment

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

note: the prose at the beginning of this file references the feature being removed, search for provide additional global lexicals to its environment and maybe remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, I think this example needs rethought.. the imposed transform will emit code that still works (it tries to call getOdometer(), which still works, but as a global global, not a global-lexical), but the user code can use globalThis['get'+'Odometer']().read(), which it could not do before

We don't need that restriction in agoric-sdk anymore, and I totally support removing it, but maybe the example we use here should emphasize the transform without also mentioning a confinement behavior that no longer exists.

…leGlobalLexicals

*BREAKING CHANGE*: Removes support for globalLexicals and inescapableGlobalLexicals, and consequently drops support for patterns like metering, where a hidden lexical and censoring transform were able to add invisible machinery to a compartment and all its descendants.
*BREAKING CHANGE*: Removes support for `globalLexicals`, consistent with changes to SES.
Fixes #904

*BREAKING CHANGE*: Removes support for `globalLexicals`.  To our knowledge, there are no production uses for `globalLexicals`.  They currently could leak because `moduleLexicals` and `globalLexicals` used the same scope object, so properties of one would leak to the other with crafted modules.  We had an opportunity to plug the leak at the cost of a fifth scope in all evaluators, but elected to remove the unnecessary complexity instead.
@kriskowal kriskowal merged commit 6df2e00 into master Oct 28, 2022
@kriskowal kriskowal deleted the kris-no-global-lexicals branch October 28, 2022 00:36
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.

Scope proxy leak allow constructive access to global lexicals
3 participants