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(ses): tolerate omitted species #2108

Merged
merged 3 commits into from
Feb 28, 2024
Merged

Conversation

erights
Copy link
Contributor

@erights erights commented Feb 28, 2024

closes: #XXXX
refs: #XXXX

Description

MetaMask is targeting React Native, that run on the Hermes JS engine. Thus, it would be good for the ses-shim to work on Hermes. Once problem we ran into is that Hermes omits Symbol.species. (Good riddance!) With this PR, the ses-shim should tolerate this difference from standard JS.

Security Considerations

Assuming that the semantics of the JS they implement is adjusted from the standard semantics in any of the obvious ways to cope with the absence of Symbol.species, this should have little effect on security.

Those "obvious ways" are

  • Just act as if the species is always the base constructor itself. IOW, when asking a subclass of Array to .map, return an array rather than an instance of this or a related subclass of array.
  • Just use constructor for what Symbol.species would have been used for. IOW, when asking a subclass of Array to .map, return an instance of this subclass rather than a somehow related subclass of array.

(FWIW, I further assume they make the first choice. Otherwise, what's the point of the omission? However, this PR should be compat with either.)

I say "little effect on security" above because this absence can in theory mislead correct JS code into silently doing the wrong thing.

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

I leave that to those set up to test on Hermes, since that is the point. Attn @naugtur @kumavis

Compatibility Considerations

It only causes an observable difference on platforms that omit Symbol.species, on which the ses shim previously refused to run. So none.

Upgrade Considerations

None. Nothing Breaking certainly. Nothing newsworthy yet. But when the ses-shim as a whole is know to support Hermes, that will be newsworthy ;)

  • Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
  • Updates NEWS.md for user-facing changes.

@erights erights self-assigned this Feb 28, 2024
@erights erights marked this pull request as ready for review February 28, 2024 19:55
packages/ses/src/tame-regexp-constructor.js Outdated Show resolved Hide resolved
@erights erights merged commit 70c85ef into master Feb 28, 2024
14 checks passed
@erights erights deleted the markm-tolerate-omitted-species branch February 28, 2024 21:32
mhofman pushed a commit that referenced this pull request May 7, 2024
…j literal short-hand methods (#2206)

Fix SES compat with Hermes when calling
`addIntrinsics(tameSymbolConstructor());`
- #1891

Follow-up to
- #2108
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.

2 participants