feat(ses): Shim compatible with Hermes compiler #2334
Conversation
leotm
changed the title
|
ready for review ^ tackling the testing strategy separately in a follow-up PR, to keep this change small |
There was a problem hiding this comment.
This is indeed far more surgical than I expected.
I only request that we capture the async generator function instance in a single try{eval}catch in commons.js so we can just use if blocks to decide whether to include async generators in the various points you’re currently using try/catch.
I would like to make sure @erights looks at these changes as well.
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 |
|
Glad to see this!
Hopefully tomorrow. |
leotm
force-pushed
the
ses-hermes
branch
2 times, most recently
from
02271af
to
a8e6593
Compare
@kriskowal 120ish commits groomed into 6 ^ and all requested changes addressed |
There was a problem hiding this comment.
This is hopefully only one more round of review away from merging. Until the merge, please append differential changes only, in anticipation of squashing them to your curated commits. Thank you for addressing all prior feedback.
Please add a draft of release notes to NEWS.md. I roll these up to a blog on hardenedjs.org now and this would likely be a headline.
I would like a confirmation from a CSP expert @Jack-Works or @naugtur that this is consistent with desired behavior on no-unsafe-eval environments.
Let’s build the Hermes dist file unconditionally. I’ve confirmed that the new file will be captured by npm pack with our current "files" in package.json.
.github/workflows/ci.yml
Outdated
| run: yarn build | ||
|
|
||
| - name: Run SES/Hermes smoke test | ||
| run: cd packages/ses && yarn test:hermes |
There was a problem hiding this comment.
Three options for this:
-
There’s a
working-directoryYAML property you can use instead of a compositerunproperty. -
Or, you could use
yarn lerna run test:hermesto run the test in any package that implements a script for it. I used this fortest:xssince there are XS-specific integration tests in other packages, and we might want that for Hermes maybe someday too. -
This works fine as-is. No changes are required.
I prefer 2 because it better prepares for a test:hermes script in packages/hardened262 (forthcoming).
There was a problem hiding this comment.
gonoe with 2., what do we think of opt 4. yarn test:hermes which includes compiler (or some other binary) output
packages/ses/package.json
Outdated
| "./hermes": { | ||
| "require": { | ||
| "types": "./dist/types.d.cts", | ||
| "default": "./dist/ses-hermes.cjs" | ||
| } | ||
| }, |
There was a problem hiding this comment.
I’m curious why this is not either "./hermes.js" or on "." with a "hermes" condition, as in:
| "./hermes": { | |
| "require": { | |
| "types": "./dist/types.d.cts", | |
| "default": "./dist/ses-hermes.cjs" | |
| } | |
| }, | |
| ".": { | |
| "require": { | |
| "type": "./dist/types.d.cts", | |
| "hermes": "./dist/ses-hermes.cjs", | |
| "default": "./dist/ses.cjs" | |
| }, | |
| "default": "./src/index.js" | |
| }, |
There was a problem hiding this comment.
i was having some trouble testing this change in LavaMoat/LavaMoat#1438 after npm pack or yarn pack then installing the .tar.gz due to our use of Yarn workspaces
npm error code EUNSUPPORTEDPROTOCOL
npm error Unsupported URL Type "workspace:": workspace:^
so i've been manually updating node_modules/ses/package.json to test
There was a problem hiding this comment.
unfortunately no bueno
Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './hermes' is not defined by "exports" in /Users/leo/Documents/GitHub/LavaMoat/node_modules/ses/package.jsonit seems the ./hermes export is required for require.resolve('ses/hermes') to work
unless you can see why "." with a "hermes" condition isn't working
| ]; | ||
| let terseFilePaths = ['dist/ses.umd.min.js', 'dist/lockdown.umd.min.js']; | ||
|
|
||
| if (buildType === 'hermes') { |
There was a problem hiding this comment.
Let’s simplify and make this unconditional.
There was a problem hiding this comment.
by making this unconditional, do we mean removing the if statement? if we do we'll only be building the ses-hermes shim, so i must have misunderstood ^
unless we mean applying the Hermes transforms by default, in which case we wouldn't need a separate Hermes shim 😛
| const bundle = await makeBundle( | ||
| read, | ||
| pathToFileURL(resolve('../index.js', import.meta.url)).toString(), | ||
| { moduleTransforms }, |
There was a problem hiding this comment.
makeBundle accepts a conditions option, which can be new Set(['hermes']) to use a "hermes" package condition in package.json. My hope is that you can simply import "ses" in Hermes applications instead of import "ses/hermes.js".
Regarding the ".js" extension in my causal misspelling of import "ses/hermes", we’ve been burned too many times by tools that do not yet pay attention to "exports" in package.json, so have a general rule that the public exports should be shadowed by a physical file with the ".js" extension, like ses/lockdown-shim.js. I can imagine beginning to relax that rule for any code that is provably unusable without "exports" support, but I want to frame that as a rule and not an exception before encouraging its use.
There was a problem hiding this comment.
makeBundleaccepts aconditionsoption, which can benew Set(['hermes'])to use a"hermes"package condition inpackage.json. My hope is that you can simplyimport "ses"in Hermes applications instead ofimport "ses/hermes.js".
it's great the conditions option exists, but i'm unsure how a Hermes app would import "ses" and somehow resolve this to dist/ses-hermes.cjs, perhaps i don't fully understand how the conditions option works
There was a problem hiding this comment.
Regarding the
".js"extension in my causal misspelling ofimport "ses/hermes", we’ve been burned too many times by tools that do not yet pay attention to"exports"inpackage.json, so have a general rule that the public exports should be shadowed by a physical file with the".js"extension, likeses/lockdown-shim.js. I can imagine beginning to relax that rule for any code that is provably unusable without"exports"support, but I want to frame that as a rule and not an exception before encouraging its use.
i can see "./hermes.js": "./dist/ses-hermes.cjs", working well
so in "./hermes.js": "./hermes.js", would the physical extra shadow file be a copy of dist/ses-hermes.cjs?
| @@ -135,6 +119,27 @@ export const getAnonymousIntrinsics = () => { | |||
| '%InertCompartment%': InertCompartment, | |||
| }; | |||
|
|
|||
| if (AsyncGeneratorFunctionInstance !== undefined) { | |||
There was a problem hiding this comment.
Please note that this block is conditional because Hermes does not provide these intrinsics.
There was a problem hiding this comment.
Is it possible for %AsyncGeneratorPrototype% to be absent while %AsyncIteratorPrototype% is present? If so, is there some means to discover %AsyncIteratorPrototype% without %AsyncGeneratorPrototype%?
I see the code below assumes that there might be a globalThis.AsyncIterator constructor. It seems possible for this to exist in a system without %AsyncGeneratorPrototype%, providing a distinct means of discovering %AsyncIteratorPrototype%.
packages/ses/scripts/hermes.sh
Outdated
| HERMES="../../node_modules/hermes-engine-cli/$OS_DIR/hermes" | ||
|
|
||
| echo "Concatenating: dist/ses-hermes.cjs + test/hermes-smoke.js" | ||
| cat dist/ses-hermes.cjs test/hermes-smoke.js > test/hermes-smoke-dist.js |
There was a problem hiding this comment.
How is dist/ses-hermes.cjs intended to be used in practice? Is Hermes code expected to ever import it or is concatenation (or bundling with Rollup or is @endo/bundle-source an option? Can you take advantage of -C package conditions? If it’s only concatenation, we might opt to not to export dist/ses-hermes.cjs in package.json since it would never be retrieved through module resolution.
There was a problem hiding this comment.
the intended use is require.resolve('ses/hermes') in
- feat: @lavamoat/react-native-lockdown LavaMoat/LavaMoat#1438
- build.js (stuff to export)
- makeGetPolyfills.js (example)
which plugs into the Metro serializer at getpolyfills
so that we lockdown earlier w/o needing to patch React Native core
There was a problem hiding this comment.
devil's advocate: for @lavamoat/react-native-lockdown i guess we could instead curl/wget the ses-hermes shim then require it w/o module resolution, but it becomes a pain to manually update it (vs exporting it for renovate/dependabot to pickup), and wouldn't be consistent resolving ses as a module but ses-hermes as a local file path
There was a problem hiding this comment.
however the concatenation is perfect here for our smoke test running in CI on Hermes VM, since Hermes doesn't support I/O
| } | ||
| } | ||
| }; | ||
| export const AsyncGeneratorFunctionInstance = |
There was a problem hiding this comment.
Please add at least a comment or a type that makes clear the conditional nature of this export. Maybe even a rename to imply its conditional nature?
Since this export is exported static state that isn't entered as an intrinsic and so not hardened by lockdown, should we freeze it here before exporting? Is there any other non FERAL_... values exported by commons.js but untouched by lockdown that should be frozen before export?
There was a problem hiding this comment.
Just checking: We do not have any such problem with async functions or generator functions? Only with async generator functions?
There was a problem hiding this comment.
Just checking: We do not have any such problem with async functions or generator functions? Only with async generator functions?
yes and we have a problemo with async arrow functions, it's abit of a mess, so i'll clarify:
generators are supported on (correct docs)
async functions are supported (comment)
despite the error in the codebase
despite listed in docs as In Progress
it's actually async arrow functions aren't supported (not in docs/code) so i filed the issue
(fixed since only in Static Hermes)
so we're handling these on Hermes in this PR packages/ses/scripts/hermes-transforms.js
for a Hermes flavoured SES shim
and then finally yes async generators are unsupported is accurate (not mentioned in docs)
so we're handling that here with AsyncGeneratorFunctionInstance delaying the error till runtime
then hardening Hermes accordingly
| @@ -135,6 +119,27 @@ export const getAnonymousIntrinsics = () => { | |||
| '%InertCompartment%': InertCompartment, | |||
| }; | |||
|
|
|||
| if (AsyncGeneratorFunctionInstance !== undefined) { | |||
There was a problem hiding this comment.
Is it possible for %AsyncGeneratorPrototype% to be absent while %AsyncIteratorPrototype% is present? If so, is there some means to discover %AsyncIteratorPrototype% without %AsyncGeneratorPrototype%?
I see the code below assumes that there might be a globalThis.AsyncIterator constructor. It seems possible for this to exist in a system without %AsyncGeneratorPrototype%, providing a distinct means of discovering %AsyncIteratorPrototype%.
leotm
force-pushed
the
ses-hermes
branch
2 times, most recently
from
c7f5c50
to
9a277a4
Compare
Closes: #2598 Refs: #2563 #2334 #1221 ## Description #1221 was supposed to make ses tolerate undeletable `func.prototype` properties that should be absent, so long as they could be set to `undefined` instead, making them harmless. This tolerance came with a warning to flag the remaining non-conformance. However #2598 explains why #1221 sometimes fails to do this. #1221 did come with a test, but it fell into the case where #1221 works, which is a non-toplevel function. #2563 (and #2334 ?) fell into the trap explained by #2598 and untested by #1221, which is an undeletable `func.prototype` on a top-level instrinsic. As a result, #2563 currently contains a workaround for #2598 which this PR would make unnecessary. This PR fixes the problem by factoring out the `func.prototype`-tolerant property deletion into a separate `cauterizeProperty` function which it calls from both places. This PR also adds the test that was missing from #1221 , having first checked that the test detects #2598 when run without the rest of this PR. If this PR gets merged before #2563, then #2563's workaround for #2598 can first be removed before it is merged. - [ ] TODO should pass a genuine reporter in to all calls to `cauterizeProperty`. @kriskowal , please advise how intrinsics.js should arrange to do so. ### Security Considerations Allowing a `func.prototype` property that really shouldn't be there seems safe, so long as it is safely set to `undefined` first, which this PR does, and then checks that it has done so. ### Scaling Considerations none ### Documentation Considerations generally, this would be one less thing to worry about, and thus one less thing that needs to be documented for most users. ### Testing Considerations Adds the test that was missing from #1221 that let #2598 go unnoticed until #2563 ### Compatibility Considerations Should be none. ### Upgrade Considerations Should be none.
Description
Add lockdown shim compatible with HermesMake 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=releasevia RN CLIcalls Gradle's
bundleReleasetask under the hood (bundlebuild task onreleasevariant)which calls RNGP (React Native Gradle Plugin) React task
createBundleReleaseJsAndAssetsand failsafter Metro finishes writing the release bundle (bundle, sourcemaps, assets)
Gradle emits these Hermes errors
async functions are unsupportedasync arrow functions are unsupportedfacebook/hermes#1395async generators are unsupported(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
/srcfilesNb: clean before bundling to see changes reflected (avoid cache)
i.e.
./gradlew clean :app:bundleReleaseNb:
async function* a() {};alone won't emit an errorbut using/referencing it and beyond
const b = a;willNb: 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 runtimeTODO
./gradlew :app:createBundleReleaseJsAndAssets./gradlew :app:installRelease -PreactNativeArchitectures=arm64-v8ayarn <android/ios> --mode releaseFollow-up, CI testing options discussed
cd android && ./gradlew :app:bundleReleaseCI(macos): RN app test + SES, Xcode releasetest262:hermesscript (liketest262:xs)Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Compatibility Considerations
Upgrade Considerations