-
Notifications
You must be signed in to change notification settings - Fork 77
feat(ses): update lockdown init and enable Hermes VM #2723
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
Conversation
69a97f5
to
7e2b07a
Compare
fa54256
to
5793270
Compare
1e87397
to
063ec33
Compare
8cf7a5e
to
d0269ff
Compare
@Jack-Works , this PR is now proceeding under the assumption that it would be ok with you to change the behavior of Would that change in |
Yes, it's ok with me. I'm only using this on platforms with direct eval available |
1bfed4d
to
13f4d10
Compare
13f4d10
to
1a348f9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
@erights i've rebased the 9 commits into 6 and ready for merge, unless any further feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
packages/ses/NEWS.md
Outdated
@@ -1,4 +1,12 @@ | |||
User-visible changes in `ses`: | |||
<!-- markdownlint-disable MD041 --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this line mean? Does the need for this have something to do with this PR, or is it a drive-by? Did you use some kind of yarn format
-like or prettier-like thing for .md files? Should we bundle that in to yarn format
(though not in this PR)? If so, please at least file an issue.
In any case, no change suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry i resolved #2723 (review) too early, exactly we should add a script (or include it in yarn format
) then ensure it's run in CI (example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right it's out of scope of this PR, filed issue here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted formatting changes in the end to keep this PR relevant/minimal and not step on any toes
all .md docs in the codebase better updated when addressing the filed issue then reviewed separetely
packages/ses/NEWS.md
Outdated
```sh | ||
$ export LOCKDOWN_ERROR_TAMING=unsafe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the $
removal manual or by the .md fixer thing?
I do mildly prefer keeping the $
prefix so readers can easily see this is shell and not js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep .md fixer https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md014.md
Rationale: It is easier to copy/paste and less noisy if the dollar signs are omitted when they are not needed. See https://cirosantilli.com/markdown-style-guide#dollar-signs-in-shell-code for more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because some commands do not produce output, it is not a violation if some commands do not have output:
$ mkdir test
mkdir: created directory 'test'
$ ls test
seems a bit silly that some doesn't include one command with no output 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do mildly prefer keeping the
$
prefix so readers can easily see this is shell and not js.
agree ^ added exception <!-- markdownlint-disable MD014 -->
1a348f9
to
224bac2
Compare
224bac2
to
b59c100
Compare
officially closes non-blocking compat improvements now tracked in updated |
nb: next release ([email protected]) to plug into Metro for bundling React Native apps |
happy to expand current NEWS.md update in follow-up a PR to be more newsworthy (; which i imagine would land on https://github.com/endojs/hardenedjs.org/tree/main/src/content/docs/blog i foresee the blog post could detail
|
b59c100
to
093bdcb
Compare
Closes: #1891 (tracker), tested on #2334, Endo Sync: 2025-01-29
Previous iterations: #2723 (comment)
TODO
legacyHermesTaming: safe (default), unsafe4th option undefined, warn (with new error reporter) to use the new lockdown optionsince getEnvironmentOption default must be a string ('all'), warn instead of SES_DIRECT_EVAL error with a strict CSPdisable compartment-shim when bundling ses for hermesdisable: globalThis Compartment and testCompartmentHooksvs camelCase--disallow-code-generation-from-strings
useful node flag to emulate a strict csp runtime in testsFollow-up: new Compartment() fails on removeUnpermittedIntrinsics at
Tolerating undeletable intrinsics.%CompartmentPrototype%.importNow.prototype === undefined
Uncaught TypeError: property is not configurable
test branch https://github.com/endojs/endo/tree/ses-hermes-p2 (from #2334)
yarn build:hermes
bundle ses for hermesyarn test:hermes
run ses/test/_hermes-smoke.jsHermes eval behaviour on
bin/hermesc
(standalone compiler) andbin/hermes
(vm, eshost)