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

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

Merged
merged 22 commits into from
Feb 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 27 additions & 1 deletion app/scripts/controllers/preferences.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ export default class PreferencesController {
useNonceField: false,
usePhishDetect: true,
dismissSeedBackUpReminder: false,
disabledRpcMethodPreferences: {
eth_sign: false,
},
409H marked this conversation as resolved.
Show resolved Hide resolved
useMultiAccountBalanceChecker: true,

// set to true means the dynamic list from the API is being used
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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
//
Expand Down
19 changes: 19 additions & 0 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
),
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions app/scripts/metamask-controller.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 3 additions & 0 deletions test/data/mock-send-state.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
"metamask": {
"ipfsGateway": "",
"dismissSeedBackUpReminder": false,
"disabledRpcMethodPreferences": {
"eth_sign": false
},
"usePhishDetect": true,
"participateInMetaMetrics": false,
"gasEstimateType": "fee-market",
Expand Down
52 changes: 43 additions & 9 deletions test/e2e/tests/eth-sign.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -21,6 +49,11 @@ describe('Eth sign', function () {
{
dapp: true,
fixtures: new FixtureBuilder()
.withPreferencesController({
disabledRpcMethodPreferences: {
eth_sign: true,
},
})
.withPermissionControllerConnectedToTestDapp()
.build(),
ganacheOptions,
Expand All @@ -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(
Expand Down
7 changes: 7 additions & 0 deletions ui/helpers/constants/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
2 changes: 1 addition & 1 deletion ui/helpers/utils/settings-search.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
38 changes: 38 additions & 0 deletions ui/pages/settings/advanced-tab/advanced-tab.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -711,6 +748,7 @@ export default class AdvancedTab extends PureComponent {
{this.renderRestoreUserData()}
{notUsingFirefox ? this.renderLedgerLiveControl() : null}
{this.renderDismissSeedBackupReminderControl()}
{this.renderToggleEthSignControl()}
</div>
);
}
Expand Down
6 changes: 6 additions & 0 deletions ui/pages/settings/advanced-tab/advanced-tab.container.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
setUseNonceField,
setLedgerTransportPreference,
setDismissSeedBackUpReminder,
setDisabledRpcMethodPreference,
backupUserData,
restoreUserData,
} from '../../../store/actions';
Expand All @@ -25,6 +26,7 @@ export const mapStateToProps = (state) => {
} = state;
const {
featureFlags: { sendHexData, advancedInlineGas } = {},
disabledRpcMethodPreferences,
useNonceField,
ledgerTransportType,
dismissSeedBackUpReminder,
Expand All @@ -48,6 +50,7 @@ export const mapStateToProps = (state) => {
ledgerTransportType,
dismissSeedBackUpReminder,
userHasALedgerAccount,
disabledRpcMethodPreferences,
};
};

Expand Down Expand Up @@ -78,6 +81,9 @@ export const mapDispatchToProps = (dispatch) => {
setDismissSeedBackUpReminder: (value) => {
return dispatch(setDismissSeedBackUpReminder(value));
},
setDisabledRpcMethodPreference: (methodName, isEnabled) => {
return dispatch(setDisabledRpcMethodPreference(methodName, isEnabled));
},
};
};

Expand Down
27 changes: 27 additions & 0 deletions ui/store/actions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down