endojs
fix(ses): completePrototypes on Hermes
Description
- Refs: https://github.com/endojs/endo/issues/1891
- completePrototypes
- Follow-up to
- https://github.com/endojs/endo/pull/2334
Calling lockdown on Hermes, in the VM or React Native runtime, we reach
https://github.com/endojs/endo/blob/7b8c677b364bb076700f376362022fbd9d109c6e/packages/ses/src/intrinsics.js#L101-L104
# Hermes VM
Uncaught TypeError: lockdown.prototype property not whitelisted
checking the lockdown intrinsic descriptor
{"length":{"value":1,"writable":false,"enumerable":false,"configurable":true},"name":{"value":"","writable":false,"enumerable":false,"configurable":true},"caller":{"enumerable":false,"configurable":false},"arguments":{"enumerable":false,"configurable":false},"prototype":{"value":{},"writable":true,"enumerable":false,"configurable":false}}
we see it's because the prototype is non-standard
https://github.com/endojs/endo/blob/7b8c677b364bb076700f376362022fbd9d109c6e/packages/ses/src/permits.js#L1644
https://github.com/endojs/endo/blob/7b8c677b364bb076700f376362022fbd9d109c6e/packages/ses/src/permits.js#L282-L283
https://github.com/endojs/endo/blob/7b8c677b364bb076700f376362022fbd9d109c6e/packages/ses/src/permits.js#L265-L273
so we should be terminating the current loop iteration instead of throwing the error, ideally at
https://github.com/endojs/endo/blob/7b8c677b364bb076700f376362022fbd9d109c6e/packages/ses/src/intrinsics.js#L93-L96
the initial approach fixed our 3 problematic intrinsics on Hermes by terminating the current loop iteration earlier before throwing the error as intended i.e. skip completing prototypes that shouldn't be there
Repro branch including proposed fix and detailed Hermes VM print output (no Babel)
- https://github.com/endojs/endo/pull/2334
- https://github.com/endojs/endo/tree/ses-hermes-completePrototypes-repro
- https://github.com/endojs/endo/tree/ses-hermes-whitelistIntrinsics-repro
yarn clean && yarn build:hermes && test:hermes
TODO: check v0.13.0 for RN0.75.x ✅ partially fixed: lockdown intrinsic only
we can easily test this in https://github.com/endojs/endo/pull/2334 by downloading the .tar.gz locally then updating both binary paths
// Hermes release v0.13.0 for RN0.75.x
// lockdown, fixed - no prototype
{"length":{"value":1,"writable":false,"enumerable":false,"configurable":true},"name":{"value":"","writable":false,"enumerable":false,"configurable":true},"caller":{"enumerable":false,"configurable":false},"arguments":{"enumerable":false,"configurable":false}}
// - Uncaught TypeError: harden.prototype property not whitelisted
{"length":{"value":1,"writable":false,"enumerable":false,"configurable":true},"name":{"value":"harden","writable":false,"enumerable":false,"configurable":true},"caller":{"enumerable":false,"configurable":false},"arguments":{"enumerable":false,"configurable":false},"prototype":{"value":{},"writable":true,"enumerable":false,"configurable":false}}
// - Uncaught TypeError: %InitialGetStackString%.prototype property not whitelisted
{"length":{"value":1,"writable":false,"enumerable":false,"configurable":true},"name":{"value":"getStackString","writable":false,"enumerable":false,"configurable":true},"caller":{"enumerable":false,"configurable":false},"arguments":{"enumerable":false,"configurable":false},"prototype":{"value":{},"writable":true,"enumerable":false,"configurable":false}}
nb:
https://github.com/endojs/endo/blob/7b8c677b364bb076700f376362022fbd9d109c6e/packages/ses/src/permits.js#L1644-L1647
TODO: build Static Hermes with cmake and ninja then test (i.e. is it fully fixed upstream?)
- https://github.com/facebook/hermes/tree/sh_stable
- https://github.com/facebook/hermes/tree/static_h
yes ✅ object literals short hand method
- https://github.com/facebook/hermes/issues/1371
- https://github.com/facebook/hermes/commit/00f18c89c720e1c34592bb85a1a8d311e6e99599
the current proposed change accounts for Hermes' non-standard fn instance prototype (fixed in Static Hermes), by setting it to undefined when completing the prototypes
another approach: https://github.com/endojs/endo/pull/2563#issuecomment-2420363381
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?
- proposed pr change is minimal and less intrusive than alternate solution below (Compatibility Considerations)
- new assumptions: if the JS engine contains the 3 unexpected prototypes, tolerate them, if not - great!
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?
- no further resources consumed
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?
- the solution is backwards compatible with older versions of Hermes (v0.12.0) and React Native to be safe
- intrinsic
lockdownis fixed on v0.13.0 for RN0.75.x, so this part of the condition can be sunset when we want to drop support for v0.12.0 - React Native Releases Support Policy
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?
- see repro to see completePrototypes finishing successfully on Hermes VM with detailed print output
Compatibility Considerations
Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?
Considered alternative solution
if (
!objectHasOwnProperty(intrinsic, 'prototype') ||
(typeof intrinsicPrototype === 'object' &&
// eslint-disable-next-line
Object.keys(intrinsicPrototype).length === 0)
) {
which accounts for { ... "prototype":{"value":{},"writable":true,"enumerable":false,"configurable":false} }
but this breaks building the ses bundle when calling whitelistIntrinsics after completePrototypes
SES_UNCAUGHT_EXCEPTION: (TypeError#1) Unexpected property prototype with permit %ArrayPrototype% at intrinsics.Array.prototype`
which appears to be a regression, so no bueno
(See also Documentation Considerations RE backward compatibility)
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.mdfor user-facing changes.
Delete guidance from pull request description before merge (including this!)
(See also Documentation Considerations RE backward compatibility)
we considered implementing
https://github.com/endojs/endo/blob/a13a879a8ea647d3e13bc6cb223de1b413e06632/packages/ses/src/intrinsics.js#L103-L110
instead in
// packages/ses/src/tame-function-tostring.js
// ...
if (
!/^[A-Z]/.test(func.name) &&
func.prototype !== undefined
) {
func.prototype = undefined;
}
// ...
but at this point it's too late, so went with the current approach
https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563
Run npx playwright install --with-deps
npx playwright install --with-deps
shell: /usr/bin/bash -e {0}
Installing dependencies...
Switching to root user to install dependencies...
Hit:1 http://azure.archive.ubuntu.com/ubuntu noble InRelease
Get:[2](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:2) http://azure.archive.ubuntu.com/ubuntu noble-updates InRelease [126 kB]
Get:[3](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:3) http://azure.archive.ubuntu.com/ubuntu noble-backports InRelease [126 kB]
Get:[4](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:5) http://azure.archive.ubuntu.com/ubuntu noble-security InRelease [126 kB]
Hit:[5](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:6) https://packages.microsoft.com/repos/azure-cli noble InRelease
Get:[6](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:7) https://packages.microsoft.com/ubuntu/24.04/prod noble InRelease [3600 B]
Get:7 http://azure.archive.ubuntu.com/ubuntu noble-updates/main amd64 Packages [59[7](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:8) kB]
Get:8 http://azure.archive.ubuntu.com/ubuntu noble-updates/main Translation-en [146 kB]
Get:9 http://azure.archive.ubuntu.com/ubuntu noble-updates/main amd64 c-n-f Metadata [10.3 kB]
Get:10 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe amd64 Packages [706 kB]
Get:11 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe Translation-en [209 kB]
Get:12 http://azure.archive.ubuntu.com/ubuntu noble-updates/universe amd64 c-n-f Metadata [19.[8](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:9) kB]
Get:13 http://azure.archive.ubuntu.com/ubuntu noble-updates/restricted amd64 Packages [388 kB]
Get:14 http://azure.archive.ubuntu.com/ubuntu noble-updates/restricted Translation-en [74.8 kB]
Get:15 http://azure.archive.ubuntu.com/ubuntu noble-updates/multiverse amd64 Packages [14.8 kB]
Get:16 http://azure.archive.ubuntu.com/ubuntu noble-updates/multiverse Translation-en [3820 B]
Get:17 http://azure.archive.ubuntu.com/ubuntu noble-updates/multiverse amd64 c-n-f Metadata [552 B]
Get:18 http://azure.archive.ubuntu.com/ubuntu noble-backports/universe amd64 Packages [10.6 kB]
Get:1[9](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:10) http://azure.archive.ubuntu.com/ubuntu noble-backports/universe Translation-en [10.8 kB]
Get:20 http://azure.archive.ubuntu.com/ubuntu noble-backports/universe amd64 c-n-f Metadata [1[10](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:11)4 B]
Get:21 http://azure.archive.ubuntu.com/ubuntu noble-security/main amd64 Packages [410 kB]
Get:22 http://azure.archive.ubuntu.com/ubuntu noble-security/main Translation-en [90.4 kB]
Get:23 http://azure.archive.ubuntu.com/ubuntu noble-security/main amd64 c-n-f Metadata [5788 B]
Get:24 http://azure.archive.ubuntu.com/ubuntu noble-security/universe amd64 Packages [553 kB]
Get:25 http://azure.archive.ubuntu.com/ubuntu noble-security/universe Translation-en [147 kB]
Get:26 http://azure.archive.ubuntu.com/ubuntu noble-security/universe amd64 c-n-f Metadata [13.5 kB]
Get:27 https://packages.microsoft.com/ubuntu/24.04/prod noble/main armhf Packages [5057 B]
Get:28 https://packages.microsoft.com/ubuntu/24.04/prod noble/main amd64 Packages [12.2 kB]
Get:29 https://packages.microsoft.com/ubuntu/24.04/prod noble/main arm64 Packages [8451 B]
Fetched 3820 kB in 1s (7439 kB/s)
Reading package lists...
Reading package lists...
Building dependency tree...
Reading state information...
Package libicu70 is not available, but is referred to by another package.
This may mean that the package is missing, has been obsoleted, or
is only available from another source
Package libasound2 is a virtual package provided by:
liboss4-salsa-asound2 4.2-build2020-1ubuntu3
libasound2t64 1.2.[11](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:12)-1build2 (= 1.2.11-1build2)
E: Package 'libasound2' has no installation candidate
E: Package 'libicu70' has no installation candidate
E: Unable to locate package libffi7
E: Unable to locate package libx264-163
Failed to install browsers
Error: Installation process exited with code: 100
Error: Process completed with exit code 1.
flakey CI, re-run
edit: fail persisting after re-run and happening elsewhere https://github.com/endojs/endo/pull/2583#issuecomment-2408260818
Definitely unrelated. We saw this once before when one of the browsers shifted underneath us. I reran the job and it persisted. It’s not a required job.
Another thought: concise functions in Hermes may incorrectly have a prototype property, but bound functions do not:
$ eshost -se '[()=>{}, (()=>{}).bind()].map(fn => Object.hasOwnProperty.call(fn, "prototype"))'
#### engine262, GraalJS, JavaScriptCore, Moddable XS, QuickJS, SpiderMonkey, V8
false,false
#### Hermes
true,false
So any receiver-insensitive function that needs be presented as non-newable (which I think describes all functions that we want to appear native) can be wrapped with bind.
Package libasound2 is a virtual package provided by: liboss4-salsa-asound2 4.2-build2020-1ubuntu3 libasound2t64 1.2.[11](https://github.com/endojs/endo/actions/runs/11366289654/job/31616329403?pr=2563#step:9:12)-1build2 (= 1.2.11-1build2)
@leotm I believe this is because GitHub runner images switched from 22.04 to 24.04 (I see noble there).
This should fix it:
- #2587
Another thought: concise functions in Hermes may incorrectly have a
prototypeproperty, but bound functions do not:$ eshost -se '[()=>{}, (()=>{}).bind()].map(fn => Object.hasOwnProperty.call(fn, "prototype"))' #### engine262, GraalJS, JavaScriptCore, Moddable XS, QuickJS, SpiderMonkey, V8 false,false #### Hermes true,falseSo any receiver-insensitive function that needs be presented as non-newable (which I think describes all functions that we want to appear native) can be wrapped with
bind.
good point ^ same with normal functions
# Hermes release version: 0.12.0, HBC bytecode version: 89
>> [function(){}, (function(){}).bind()].map(fn => Object.hasOwnProperty.call(fn, "prototype"))
[ true, false, [length]: 2 ]
so binding the intrinsic works too after restoring the permits
https://github.com/endojs/endo/tree/ses-hermes-completePrototypes-bind
instead of setting the prototype to undefined
LGTM module suggestions, especially #2563 (comment)
If all this is true, then it makes sense to permit prototype: undefined on these three. So I'll approve on that basis. But before merging, please let us know if this is indeed all true.
all questions answered and addressed ^ in comments and commits
Now that #2624 is merged, I'm changing to "Request changes".
Does #2624 indeed enable this to be simplified? What does it simplify down to?
after testing https://github.com/endojs/endo/pull/2624 with git rebase master on https://github.com/endojs/endo/pull/2334
can confirm https://github.com/endojs/endo/pull/2624 fixes completePrototypes on Hermes - i.e. this PR simplifies to no longer necessary - thank you!
results
Removing lockdown.prototype
Tolerating undeletable lockdown.prototype === undefined
Removing harden.prototype
Tolerating undeletable harden.prototype === undefined
Removing %InitialGetStackString%.prototype
Tolerating undeletable %InitialGetStackString%.prototype === undefined