Disable eth_sign by default, allow users to toggle it back on (#17308)

* Added translation for eth sign toggle * Disabled the ability to call eth_sign by default. Added ability within advanced settings to renable support for eth_sign * Add test case for eth_sign being enabled and disabled * Modified copy * Moved rpc method preference to its own object within store * Complete work on moving rpc method preference into its own object within store * Fix with prettier * Fix lint * Fix a unit test * Fix test * Renamed function and object keys to be more intuitive * Fix e2e test * Enabled eth_sign through preferences fixture * Fix lint * Fix e2e test Wait for the notification popup to close, leaving 2 window handles the extension and the test dapp * Fix e2e test * Fix unit test Enable eth_sign method * Lint fix --------- Co-authored-by: PeterYinusa <[email protected]> Co-authored-by: Dan J Miller <[email protected]>
MetaMask · Feb 6, 2023 · b4ecc4c · b4ecc4c
1 parent 080d72a
commit b4ecc4c
Show file tree

Hide file tree

Showing 11 changed files with 181 additions and 11 deletions.
diff --git a/app/_locales/en/messages.json b/app/_locales/en/messages.json
diff --git a/app/scripts/controllers/preferences.js b/app/scripts/controllers/preferences.js
@@ -30,6 +30,9 @@ export default class PreferencesController {
       useNonceField: false,
       usePhishDetect: true,
       dismissSeedBackUpReminder: false,
+      disabledRpcMethodPreferences: {
+        eth_sign: false,
+      },
       useMultiAccountBalanceChecker: true,
 
       // set to true means the dynamic list from the API is being used
@@ -72,7 +75,7 @@ export default class PreferencesController {
 
     this.network = opts.network;
     this.store = new ObservableStore(initState);
-    this.store.setMaxListeners(12);
+    this.store.setMaxListeners(13);
     this.openPopup = opts.openPopup;
     this.tokenListController = opts.tokenListController;
 
@@ -544,6 +547,29 @@ export default class PreferencesController {
     });
   }
 
+  /**
+   * A setter for the user preference to enable/disable rpc methods
+   *
+   * @param {string} methodName - The RPC method name to change the setting of
+   * @param {bool} isEnabled - true to enable the rpc method
+   */
+  async setDisabledRpcMethodPreference(methodName, isEnabled) {
+    const currentRpcMethodPreferences =
+      this.store.getState().disabledRpcMethodPreferences;
+    const updatedRpcMethodPreferences = {
+      ...currentRpcMethodPreferences,
+      [methodName]: isEnabled,
+    };
+
+    this.store.updateState({
+      disabledRpcMethodPreferences: updatedRpcMethodPreferences,
+    });
+  }
+
+  getRpcMethodPreferences() {
+    return this.store.getState().disabledRpcMethodPreferences;
+  }
+
   //
   // PRIVATE METHODS
   //

diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js
@@ -1824,6 +1824,14 @@ export default class MetamaskController extends EventEmitter {
         preferencesController.setDismissSeedBackUpReminder.bind(
           preferencesController,
         ),
+      setDisabledRpcMethodPreference:
+        preferencesController.setDisabledRpcMethodPreference.bind(
+          preferencesController,
+        ),
+      getRpcMethodPreferences:
+        preferencesController.getRpcMethodPreferences.bind(
+          preferencesController,
+        ),
       setAdvancedGasFee: preferencesController.setAdvancedGasFee.bind(
         preferencesController,
       ),
@@ -3046,8 +3054,19 @@ export default class MetamaskController extends EventEmitter {
    * @param {object} [req] - The original request, containing the origin.
    */
   async newUnsignedMessage(msgParams, req) {
+    const { disabledRpcMethodPreferences } =
+      this.preferencesController.store.getState();
+    const { eth_sign } = disabledRpcMethodPreferences; // eslint-disable-line camelcase
     const data = normalizeMsgData(msgParams.data);
     let promise;
+
+    // eslint-disable-next-line camelcase
+    if (!eth_sign) {
+      throw ethErrors.rpc.methodNotFound(
+        'eth_sign has been disabled. You must enable it in the advanced settings',
+      );
+    }
+
     // 64 hex + "0x" at the beginning
     // This is needed because Ethereum's EcSign works only on 32 byte numbers
     // For 67 length see: https://github.com/MetaMask/metamask-extension/pull/12679/files#r749479607

diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js
@@ -872,6 +872,10 @@ describe('MetaMaskController', function () {
         data,
       };
 
+      metamaskController.preferencesController.setDisabledRpcMethodPreference(
+        'eth_sign',
+        true,
+      );
       const promise = metamaskController.newUnsignedMessage(msgParams);
       // handle the promise so it doesn't throw an unhandledRejection
       promise.then(noop).catch(noop);

diff --git a/test/data/mock-send-state.json b/test/data/mock-send-state.json
@@ -45,6 +45,9 @@
   "metamask": {
     "ipfsGateway": "",
     "dismissSeedBackUpReminder": false,
+    "disabledRpcMethodPreferences": {
+      "eth_sign": false
+    },
     "usePhishDetect": true,
     "participateInMetaMetrics": false,
     "gasEstimateType": "fee-market",

diff --git a/test/e2e/tests/eth-sign.spec.js b/test/e2e/tests/eth-sign.spec.js
@@ -2,17 +2,45 @@ const { strict: assert } = require('assert');
 const { convertToHexValue, withFixtures } = require('../helpers');
 const FixtureBuilder = require('../fixture-builder');
 
+const ganacheOptions = {
+  accounts: [
+    {
+      secretKey:
+        '0x7C9529A67102755B7E6102D6D950AC5D5863C98713805CEC576B945B15B71EAC',
+      balance: convertToHexValue(25000000000000000000),
+    },
+  ],
+};
+
 describe('Eth sign', function () {
+  it('will detect if eth_sign is disabled', async function () {
+    await withFixtures(
+      {
+        dapp: true,
+        fixtures: new FixtureBuilder()
+          .withPermissionControllerConnectedToTestDapp()
+          .build(),
+        ganacheOptions,
+        title: this.test.title,
+      },
+      async ({ driver }) => {
+        await driver.navigate();
+        await driver.fill('#password', 'correct horse battery staple');
+        await driver.press('#password', driver.Key.ENTER);
+
+        await driver.openNewPage('http://127.0.0.1:8080/');
+        await driver.clickElement('#ethSign');
+
+        const ethSignButton = await driver.findElement('#ethSign');
+        const exceptionString =
+          'ERROR: ETH_SIGN HAS BEEN DISABLED. YOU MUST ENABLE IT IN THE ADVANCED SETTINGS';
+
+        assert.equal(await ethSignButton.getText(), exceptionString);
+      },
+    );
+  });
+
   it('can initiate and confirm a eth sign', async function () {
-    const ganacheOptions = {
-      accounts: [
-        {
-          secretKey:
-            '0x7C9529A67102755B7E6102D6D950AC5D5863C98713805CEC576B945B15B71EAC',
-          balance: convertToHexValue(25000000000000000000),
-        },
-      ],
-    };
     const expectedPersonalMessage =
       '0x879a053d4800c6354e76c7985a865d2922c82fb5b3f4577b2fe08b998954f2e0';
     const expectedEthSignResult =
@@ -21,6 +49,11 @@ describe('Eth sign', function () {
       {
         dapp: true,
         fixtures: new FixtureBuilder()
+          .withPreferencesController({
+            disabledRpcMethodPreferences: {
+              eth_sign: true,
+            },
+          })
           .withPermissionControllerConnectedToTestDapp()
           .build(),
         ganacheOptions,
@@ -34,6 +67,7 @@ describe('Eth sign', function () {
         await driver.openNewPage('http://127.0.0.1:8080/');
         await driver.clickElement('#ethSign');
 
+        // Wait for Signature request popup
         await driver.waitUntilXWindowHandles(3);
         let windowHandles = await driver.getAllWindowHandles();
         await driver.switchToWindowWithTitle(

diff --git a/ui/helpers/constants/settings.js b/ui/helpers/constants/settings.js
@@ -132,6 +132,13 @@ export const SETTINGS_CONSTANTS = [
     route: `${ADVANCED_ROUTE}#dimiss-secretrecovery`,
     icon: 'fas fa-sliders-h',
   },
+  {
+    tabMessage: (t) => t('advanced'),
+    sectionMessage: (t) => t('toggleEthSignField'),
+    descriptionMessage: (t) => t('toggleEthSignDescriptionField'),
+    route: `${ADVANCED_ROUTE}#toggle-ethsign`,
+    icon: 'fas fa-sliders-h',
+  },
   {
     tabMessage: (t) => t('contacts'),
     sectionMessage: (t) => t('contacts'),

diff --git a/ui/helpers/utils/settings-search.test.js b/ui/helpers/utils/settings-search.test.js
@@ -159,7 +159,7 @@ describe('Settings Search Utils', () => {
     });
 
     it('should get good advanced section number', () => {
-      expect(getNumberOfSettingsInSection(t, t('advanced'))).toStrictEqual(13);
+      expect(getNumberOfSettingsInSection(t, t('advanced'))).toStrictEqual(14);
     });
 
     it('should get good contact section number', () => {

diff --git a/ui/pages/settings/advanced-tab/advanced-tab.component.js b/ui/pages/settings/advanced-tab/advanced-tab.component.js
@@ -56,6 +56,10 @@ export default class AdvancedTab extends PureComponent {
     userHasALedgerAccount: PropTypes.bool.isRequired,
     backupUserData: PropTypes.func.isRequired,
     restoreUserData: PropTypes.func.isRequired,
+    setDisabledRpcMethodPreference: PropTypes.func.isRequired,
+    disabledRpcMethodPreferences: PropTypes.shape({
+      eth_sign: PropTypes.bool.isRequired,
+    }),
   };
 
   state = {
@@ -580,6 +584,39 @@ export default class AdvancedTab extends PureComponent {
     );
   }
 
+  renderToggleEthSignControl() {
+    const { t } = this.context;
+    const { disabledRpcMethodPreferences, setDisabledRpcMethodPreference } =
+      this.props;
+
+    return (
+      <div
+        ref={this.settingsRefs[10]}
+        className="settings-page__content-row"
+        data-testid="advanced-setting-toggle-ethsign"
+      >
+        <div className="settings-page__content-item">
+          <span>{t('toggleEthSignField')}</span>
+          <div className="settings-page__content-description">
+            {t('toggleEthSignDescriptionField')}
+          </div>
+        </div>
+        <div className="settings-page__content-item">
+          <div className="settings-page__content-item-col">
+            <ToggleButton
+              value={disabledRpcMethodPreferences?.eth_sign || false}
+              onToggle={(value) =>
+                setDisabledRpcMethodPreference('eth_sign', !value)
+              }
+              offLabel={t('off')}
+              onLabel={t('on')}
+            />
+          </div>
+        </div>
+      </div>
+    );
+  }
+
   handleLockChange(time) {
     const { t } = this.context;
     const autoLockTimeLimit = Math.max(Number(time), 0);
@@ -711,6 +748,7 @@ export default class AdvancedTab extends PureComponent {
         {this.renderRestoreUserData()}
         {notUsingFirefox ? this.renderLedgerLiveControl() : null}
         {this.renderDismissSeedBackupReminderControl()}
+        {this.renderToggleEthSignControl()}
       </div>
     );
   }

diff --git a/ui/pages/settings/advanced-tab/advanced-tab.container.js b/ui/pages/settings/advanced-tab/advanced-tab.container.js
@@ -11,6 +11,7 @@ import {
   setUseNonceField,
   setLedgerTransportPreference,
   setDismissSeedBackUpReminder,
+  setDisabledRpcMethodPreference,
   backupUserData,
   restoreUserData,
 } from '../../../store/actions';
@@ -25,6 +26,7 @@ export const mapStateToProps = (state) => {
   } = state;
   const {
     featureFlags: { sendHexData, advancedInlineGas } = {},
+    disabledRpcMethodPreferences,
     useNonceField,
     ledgerTransportType,
     dismissSeedBackUpReminder,
@@ -48,6 +50,7 @@ export const mapStateToProps = (state) => {
     ledgerTransportType,
     dismissSeedBackUpReminder,
     userHasALedgerAccount,
+    disabledRpcMethodPreferences,
   };
 };
 
@@ -78,6 +81,9 @@ export const mapDispatchToProps = (dispatch) => {
     setDismissSeedBackUpReminder: (value) => {
       return dispatch(setDismissSeedBackUpReminder(value));
     },
+    setDisabledRpcMethodPreference: (methodName, isEnabled) => {
+      return dispatch(setDisabledRpcMethodPreference(methodName, isEnabled));
+    },
   };
 };
 

diff --git a/ui/store/actions.ts b/ui/store/actions.ts
@@ -3830,6 +3830,33 @@ export function setDismissSeedBackUpReminder(
   };
 }
 
+export function setDisabledRpcMethodPreference(
+  methodName: string,
+  value: number,
+): ThunkAction<void, MetaMaskReduxState, unknown, AnyAction> {
+  return async (dispatch) => {
+    dispatch(showLoadingIndication());
+    await submitRequestToBackground('setDisabledRpcMethodPreference', [
+      methodName,
+      value,
+    ]);
+    dispatch(hideLoadingIndication());
+  };
+}
+
+export function getRpcMethodPreferences(): ThunkAction<
+  void,
+  MetaMaskReduxState,
+  unknown,
+  AnyAction
+> {
+  return async (dispatch) => {
+    dispatch(showLoadingIndication());
+    await submitRequestToBackground('getRpcMethodPreferences', []);
+    dispatch(hideLoadingIndication());
+  };
+}
+
 export function setConnectedStatusPopoverHasBeenShown(): ThunkAction<
   void,
   MetaMaskReduxState,