endojs / endo

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

feat(ses): Shim compatible with Hermes compiler #2334

Open leotm opened 3 months ago

leotm commented 3 months ago

Description

Add lockdown shim compatible with Hermes Make current shim compatible with Hermes compiler for building React Native (RN) prod apps with SES

Generating the release AAB (Android App Bundle) i.e. npx react-native build-android --mode=release via RN CLI calls Gradle's bundleRelease task under the hood (bundle build task on release variant) which calls RNGP (React Native Gradle Plugin) React task createBundleReleaseJsAndAssets and fails after Metro finishes writing the release bundle (bundle, sourcemaps, assets)

Gradle emits these Hermes errors

(at runtime we can see both are SyntaxErrors) Resulting in vague java.lang.StackOverflowError (no error message)

The try/catch approach testing language ft support via a new fn works since RNGP no longer emits the Hermes errors in the task after Metro bundles and we're conditionally using parts of SES compatible with the Hermes engine

The initial approach involved building a new shim via an env var which involved a lot of duplicates /src files

Nb: clean before bundling to see changes reflected (avoid cache) i.e. ./gradlew clean :app:bundleRelease

Nb: async function* a() {}; alone won't emit an error but using/referencing it and beyond const b = a; will

Nb: eventually we hit RNGP BundleHermesCTask.kt > run > detectedHermesCommand > detectOSAwareHermesCommand from PathUtils.kt, which calls the Hermes compiler default command hermesc - the path of the binary file, to create the optimised bytecode bundle to load/exec at runtime

TODO

Follow-up, CI testing options discussed

Security Considerations

Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?

Scaling Considerations

Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated?

Documentation Considerations

Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?

Testing Considerations

Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?

Compatibility Considerations

Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?

Upgrade Considerations

What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed?

Include *BREAKING*: in the commit message with migration instructions for any breaking change.

Update NEWS.md for user-facing changes.

Delete guidance from pull request description before merge (including this!)

leotm commented 2 months ago

ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small

kriskowal commented 2 months ago

ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small

Are you familiar with stacked PRs? You can propose the test changes in a PR based on this branch so they can be reviewed together. I would hesitate to approve code that doesn’t come with tests in the same merge, though I’m fine with reviewing them separately. Much depends on your workflow.

One workflow that I like is individually reviewable commits in a single PR. That does require more commit grooming, and it looks like you intend to squash the 21 commits here when you merge.

Another workflow is to create “stacked” PRs for each review artifact, then merge them top to bottom, so that ultimately the feature and its tests arrive in master with a single merge commit.

erights commented 2 months ago

Glad to see this!

I would like to make sure @erights looks at these changes as well.

Hopefully tomorrow.

leotm commented 2 months ago

Nb: interestingly this isn't surfaced nor fixed running lint scripts within the ses folder

Screenshot 2024-07-19 at 12 16 09 PM Screenshot 2024-07-19 at 12 19 58 PM

edit: ah it's a Prettier problem, unrelated to ESLint, i.e. fixed with yarn format

leotm commented 2 months ago

ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small

Are you familiar with stacked PRs? You can propose the test changes in a PR based on this branch so they can be reviewed together. I would hesitate to approve code that doesn’t come with tests in the same merge, though I’m fine with reviewing them separately. Much depends on your workflow.

One workflow that I like is individually reviewable commits in a single PR. That does require more commit grooming, and it looks like you intend to squash the 21 commits here when you merge.

Another workflow is to create “stacked” PRs for each review artifact, then merge them top to bottom, so that ultimately the feature and its tests arrive in master with a single merge commit.

created the test changes in PR based on this branch to work on here

leotm commented 1 day ago

One workflow that I like is individually reviewable commits in a single PR. That does require more commit grooming, and it looks like you intend to squash the 21 commits here when you merge.

@kriskowal 120ish commits groomed into 6 ^ and all requested changes addressed