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

Proof of Concept Insights - Removing Babel Plugins #4

Closed
bentobox19 opened this issue Feb 25, 2022 · 1 comment
Closed

Proof of Concept Insights - Removing Babel Plugins #4

bentobox19 opened this issue Feb 25, 2022 · 1 comment
Labels
babel error build/run time blocker lockdown

Comments

@bentobox19
Copy link
Contributor

bentobox19 commented Feb 25, 2022

PoC

https://github.com/LavaMoat/docs/blob/main/react-native-and-ses-lockdown.md

Discussion

The following errors while running the lockdown() function:

TypeError: (0 , _getPrototypeOf2.default) is not a function

or

ERROR  failed to delete intrinsics.%PromisePrototype%.then.prototype [TypeError: Unable to delete property.]

After a number of patching iterations, including:

It was concluded that is more straightforward to just forfeit some plugins at the React Native preset.

PoC Solution

Items of action

  • Investigate the minimum RN plugins needed to actually run the mobile application
  • Find out which procedure is actually calling to polyfill Promise
    • Suggestion (by @naugtur) Instead of patching RN we need to look at how they are checking the built-in promise on the global object because they shouldn't be overwriting this.
  • Maybe we want to create a transform for the build package to prevent babel regenerator to put these monkey patches.
    • So, if RN is changing things, we could change them back!
    • This approach avoids patches, avoids creating a custom preset, and require us to just add a plugin at babel.config.js just after the preset.
  • Add isHermes plugins.
@bentobox19
Copy link
Contributor Author

Patching the dependency metro-react-native-babel-preset allows us to get rid of two problematic (for SES) plugins, and only have to maintain a single patch:

  • metro-react-native-babel-preset+0.66.2.patch
diff --git a/node_modules/metro-react-native-babel-preset/src/configs/main.js b/node_modules/metro-react-native-babel-preset/src/configs/main.js
index caa45fe..8eeb742 100644
--- a/node_modules/metro-react-native-babel-preset/src/configs/main.js
+++ b/node_modules/metro-react-native-babel-preset/src/configs/main.js
@@ -174,16 +174,6 @@ const getPreset = (src, options) => {
     extraPlugins.push([require("@babel/plugin-transform-react-jsx-self")]);
   }

-  if (!options || options.enableBabelRuntime !== false) {
-    extraPlugins.push([
-      require("@babel/plugin-transform-runtime"),
-      {
-        helpers: true,
-        regenerator: !isHermes
-      }
-    ]);
-  }
-
   return {
     comments: false,
     compact: true,
@@ -196,7 +186,6 @@ const getPreset = (src, options) => {
       {
         plugins: [
           ...defaultPluginsBeforeRegenerator,
-          isHermes ? null : require("@babel/plugin-transform-regenerator"),
           ...defaultPluginsAfterRegenerator
         ].filter(Boolean)
       },

@leotm leotm added babel lockdown error build/run time blocker labels Mar 12, 2023
leotm added a commit to MetaMask/metamask-mobile that referenced this issue Nov 10, 2023
## **Description**

Problem being solved: prototype pollution/poisoning

SES lockdown (shim v0.18.8) on iOS JSC, baked early into RN core before
RN initialisation for the simplest minimal solution
as opposed to previous approach of shim'ing at the beginning of our
entry file requiring further complex lib patches

with SES lockdown on Android Hermes (introduced earlier in our [RN v0.71.6 upgrade](#6220))
being followed up separately
currently bundling successfully, but runtime not yet functional

_SES lockdown on Android JSC was also passing smoke tests after some work prior to Hermes_
_so a backup engine worth keeping on ice being followed up separately_ 

_Previous patches no longer required:
[`eth-keyring-controller`](https://github.com/MetaMask/metamask-mobile/pull/3794/files#diff-19aae36749eec9908c74557591fade5cc596e9a40422d88594c0fba456870389),
`ethjs-contract` (one not [two](https://github.com/MetaMask/metamask-mobile/pull/3794/files#diff-1970015453fca9583682c37a44695a3d26645329e586bb70ff6f05b74f936802)), [`web3-core-methods`](LavaMoat/docs#8), [`metro-react-native-babel-preset`](LavaMoat/docs#4), Sentry [config](LavaMoat/docs#6) (see previous PR: #3794

_Nb: `@babel/plugin-transform-regenerator` removed from
`metro-react-native-babel-preset` since initial investigation_
_Nb: `@babel/plugin-transform-runtime` config opt `regenerator: true`
previously caused iOS animated node assertion failures_
_Nb: default `@babel/plugin-transform-runtime` via
`metro-react-native-babel-preset` causes additional 4 SES warnings_

<details>
<summary>Nb: Current behaviour (not SES)</summary>

```
main, jsc
- import wallet via SRP
  - tap form field: disables cmd+D (app must be closed, no Metro restart)

main, v8
- (fresh) import wallet via SRP
  - if cmd+v paste ok, bot Import btn tappable when filled, but spinner hang (no error/warn), ~20s Metro dc
  - restart, enter pw, tap Unlock btn, spinner hang, still ~20s Metro dc
  - tap Reset Wallet, hang
- import (continued)
  - partial responsiveness
    - tap form field: dev menu disabled (cmd+d, app must be closed)
    - unresponsive: tap Back, tap Show, cannot tap Import, cmd+v disabled
    - responsive: cycle/input fields
    - still ~20s Metro dc
```

</details>

<details>
<summary>Previous SES warnings when locking down at entry file (not RN
InitializeCore)</summary>

https://www.diffchecker.com/fjj1iObp

```console
# JSC (26)
Removing intrinsics.Object.setPrototypeOf.default
Removing intrinsics.Object.setPrototypeOf.__esModule
Removing intrinsics.Object.assign.default
Removing intrinsics.Object.assign.__esModule
Removing intrinsics.Reflect.construct.default
Removing intrinsics.Reflect.construct.__esModule
Removing intrinsics.Reflect.decorate
Removing intrinsics.Reflect.metadata
Removing intrinsics.Reflect.defineMetadata
Removing intrinsics.Reflect.hasMetadata
Removing intrinsics.Reflect.hasOwnMetadata
Removing intrinsics.Reflect.getMetadata
Removing intrinsics.Reflect.getOwnMetadata
Removing intrinsics.Reflect.getMetadataKeys
Removing intrinsics.Reflect.getOwnMetadataKeys
Removing intrinsics.Reflect.deleteMetadata
Removing intrinsics.%ArrayPrototype%.toReversed
Removing intrinsics.%ArrayPrototype%.toSorted
Removing intrinsics.%ArrayPrototype%.toSpliced
Removing intrinsics.%ArrayPrototype%.with
Removing intrinsics.%ArrayPrototype%.@@unscopables.toReversed
Removing intrinsics.%ArrayPrototype%.@@unscopables.toSorted
Removing intrinsics.%ArrayPrototype%.@@unscopables.toSpliced
Removing intrinsics.%TypedArrayPrototype%.toReversed
Removing intrinsics.%TypedArrayPrototype%.toSorted
Removing intrinsics.%TypedArrayPrototype%.with
```

```console
# V8 (33)
Removing intrinsics.Object.assign.default
Removing intrinsics.Object.assign.__esModule
Removing intrinsics.Object.setPrototypeOf.default
Removing intrinsics.Object.setPrototypeOf.__esModule
Removing intrinsics.JSON.rawJSON
Removing intrinsics.JSON.isRawJSON
Removing intrinsics.Reflect.construct.default
Removing intrinsics.Reflect.construct.__esModule
Removing intrinsics.Reflect.decorate
Removing intrinsics.Reflect.metadata
Removing intrinsics.Reflect.defineMetadata
Removing intrinsics.Reflect.hasMetadata
Removing intrinsics.Reflect.hasOwnMetadata
Removing intrinsics.Reflect.getMetadata
Removing intrinsics.Reflect.getOwnMetadata
Removing intrinsics.Reflect.getMetadataKeys
Removing intrinsics.Reflect.getOwnMetadataKeys
Removing intrinsics.Reflect.deleteMetadata
Removing intrinsics.%ArrayPrototype%.toReversed
Removing intrinsics.%ArrayPrototype%.toSorted
Removing intrinsics.%ArrayPrototype%.toSpliced
Removing intrinsics.%ArrayPrototype%.with
Removing intrinsics.%ArrayPrototype%.@@unscopables.toReversed
Removing intrinsics.%ArrayPrototype%.@@unscopables.toSorted
Removing intrinsics.%ArrayPrototype%.@@unscopables.toSpliced
Removing intrinsics.%ArrayBufferPrototype%.transferToFixedLength
Removing intrinsics.%ArrayBufferPrototype%.detached
Removing intrinsics.%StringPrototype%.isWellFormed
Removing intrinsics.%StringPrototype%.toWellFormed
Removing intrinsics.%RegExpPrototype%.unicodeSets
Removing intrinsics.%TypedArrayPrototype%.toReversed
Removing intrinsics.%TypedArrayPrototype%.toSorted
Removing intrinsics.%TypedArrayPrototype%.with
```

</details>

<details>
<summary>Notes on patch creation</summary>

- `--exclude 'nothing'` to include `package.json` changes, then trim
patch
- `react-native` requires trimming majority of patch after initial diffs
- upon failure on symlinks, `git clean -fdx` and re-create

</details>

## **Related issues**

Fixes
- LavaMoat/docs#1 various issues
  - #3794 previous pr
- LavaMoat/docs#12 various issues

Worthy read for everyone on adding/upgrading libraries
- endojs/endo#1855

## **Manual testing steps**

App functions normally

## **Screenshots/Recordings**

### **Before**

Previously failing iOS (JSC) E2E tests have now been fixed

- LavaMoat/docs#9
  - https://github.com/MetaMask/metamask-mobile/assets/1881059/3cac630f-6ec8-4975-a273-8a115a4e8fe9
  - https://github.com/MetaMask/metamask-mobile/assets/1881059/67221672-d1a3-4346-a783-5f93b53dbbe9
  - https://github.com/MetaMask/metamask-mobile/assets/1881059/baa6e512-e307-4f0f-ae2f-77bfb4a29baf
  - https://github.com/MetaMask/metamask-mobile/assets/1881059/c21ffb0b-c9df-4223-ba73-5383dc8627f5

And more screenshots in related issues linked above

### **After**

App functions normally

---------

Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
babel error build/run time blocker lockdown
Projects
None yet
Development

No branches or pull requests

2 participants