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: non-security mode for create-react-scripts compat. (KLUDGE) #642

Merged
merged 8 commits into from
Mar 26, 2021

Conversation

dckc
Copy link
Contributor

@dckc dckc commented Mar 26, 2021

We temporarily introduce a new __unsafeKludgeForReact__ option whose
'unsafe' setting that unsafely allows React to work under SES in the browser
even before React is fixed. Once React is fixed, this option will disappear.

Enables https://github.com/Agoric/dapp-token-economy/issues/159 to be fixed by injecting SES on the page first, as shown at https://github.com/Agoric/dapp-token-economy/pull/155

@dckc @kriskowal This is now ready for review. @dckc Because you created this PR I cannot add you as an official reviewer but please look anyway.

@dckc dckc requested a review from erights March 26, 2021 18:45
@erights erights force-pushed the create-react-app-compat branch from 84249a6 to 05714aa Compare March 26, 2021 20:04
@erights erights requested a review from kriskowal March 26, 2021 20:44
@erights erights self-assigned this Mar 26, 2021
Copy link
Contributor Author

@dckc dckc left a comment

Choose a reason for hiding this comment

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

LGTM, modulo "seems to be" typo?

- No changes yet
- We temporarily introduced a new `__unsafeKludgeForReact__` option whose
`'unsafe'` setting that unsafely allows React to work under SES in the browser
even before React is fixed. Once React is fixed, this option will disappear.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I shy away from future tense. I tend toward "We plan to remove this once React is fixed." And I would much prefer to have an open issue to represent this plan.

Copy link
Contributor

Choose a reason for hiding this comment

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

typo fixed. Creating the issue next.

packages/ses/NEWS.md Outdated Show resolved Hide resolved
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

The substance is fine. Calling out React specifically is unnecessary and casts unfair judgement. React is working as designed and ascribing brokenness is subjective to use with SES. The problem this addresses also applies equally well to any framework that includes a shim for any of the intrinsics. That is probably all of them.

The name of the option should express the solution more than the problem. In prose, this is to enable the use of the harden and assert shims without creating a SES environment, an environment that Agoric frontends using CapTP marshaling depend upon.

I would agree that “don’t harden intrinsics” fails to communicate the scope of the lack of safety, or that this is not a sound foundation in the long term. But, perhaps __immediatelyHardenedSharedIntrinsics__: "unsafe" has enough qualifiers. Maybe __hardenedIntrinsics__: "unsafe" for the sake of typos.

@erights
Copy link
Contributor

erights commented Mar 26, 2021

The substance is fine. Calling out React specifically is unnecessary and casts unfair judgement. React is working as designed and ascribing brokenness is subjective to use with SES. The problem this addresses also applies equally well to any framework that includes a shim for any of the intrinsics. That is probably all of them.

The name of the option should express the solution more than the problem. In prose, this is to enable the use of the harden and assert shims without creating a SES environment, an environment that Agoric frontends using CapTP marshaling depend upon.

I would agree that “don’t harden intrinsics” fails to communicate the scope of the lack of safety, or that this is not a sound foundation in the long term. But, perhaps __immediatelyHardenedSharedIntrinsics__: "unsafe" has enough qualifiers. Maybe __hardenedIntrinsics__: "unsafe" for the sake of typos.

I get your point about removing references to React other than as an example. I will do that.

However, this only happens to work for react because react happens to do all its monkey patching in ways that don't conflict with the freezing of primordials caused by transitive hardening. In that sense, this really is react specific, in that anything else suffering the same problem only slightly differently may not be rescued by this kludge. Hence I want to keep "unsafeKludge" in the name of the option.

@kriskowal
Copy link
Member

Hence I want to keep "unsafeKludge" in the name of the option.

I like having unsafeKludge in the name. Sounds good to me.

Copy link
Contributor Author

@dckc dckc left a comment

Choose a reason for hiding this comment

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

LGTM

@erights erights merged commit 6bd9f03 into master Mar 26, 2021
@erights erights deleted the create-react-app-compat branch March 26, 2021 22:42
@erights
Copy link
Contributor

erights commented Mar 26, 2021

Hence I want to keep "unsafeKludge" in the name of the option.

I like having unsafeKludge in the name. Sounds good to me.

I kept the "unsafe". It is now __allowUnsafeMonkeyPatching__ . Done.

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