chore: Fully remove `eth_sign` #24756

adonesky1 · 2024-05-23T20:32:37Z

Explanation

Months ago, because of phishing risk, we disabled the eth_sign API method by default (users could manually enable it with a preference toggle). Now because of additional risk associated with potentially malicious EIP-3074 invokers we are fully removing support for this method.

This PR introduces the changes to @metamask/signature-controller and @metamask/preferences-controller from MetaMask/core#4319 which remove eth_sign related infrastructure.

Additionally it removes all instances of eth_sign components and references from the extension codebase.

References

Fixes https://github.com/MetaMask/MetaMask-planning/issues/2371

Manual testing steps

Go to the e2e test dapp
Connect the wallet
Scroll down to the Eth Sign card (https://metamask.github.io/test-dapp/#ethSign)
Click Sign
You should see Error: The method "eth_sign" does not exist / is not available.

Pre-merge author checklist

I’ve followed MetaMask Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-05-23T20:32:50Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

socket-security · 2024-07-18T20:36:30Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/logging-controller@5.0.0	None	`0`	84.6 kB	metamaskbot
npm/@metamask/signature-controller@17.0.0	None	`0`	221 kB	metamaskbot

🚮 Removed packages: npm/@metamask/logging-controller@3.0.1, npm/@metamask/signature-controller@16.0.0

View full report↗︎

adonesky1 · 2024-07-18T20:44:15Z

@metamaskbot update-policies

metamaskbot · 2024-07-18T20:59:07Z

Policies updated

…versions

FrederikBolding · 2024-07-30T14:46:05Z

app/scripts/metamask-controller.js

-        this.preferencesController.store.getState()
-          ?.disabledRpcMethodPreferences?.eth_sign,
+      // TODO remove this option
+      isEthSignEnabled: () => false,


Can we remove this now?

Yes. Not sure why but I thought I'd forgotten to remove it as an option so was going to loop back to it but it can be remove removed now.

FrederikBolding · 2024-07-30T14:47:30Z

ui/pages/confirmations/components/signature-request-original/signature-request-original.test.js

@@ -63,10 +27,21 @@ const props = {
  txData: {
    msgParams: {
      from: address,
-      data: MOCK_SIGN_DATA,
+      data: [


Why the change in test data here? IIRC the data is stringified like MOCK_SIGN_DATA

So I didn't change the way the data was formatted in the actual component for eth_signTypedData see here... I agree this doesn't seem right. But as you can see here, this data is expected to be mappable. So what I've done here is preserve the existing behavior but make the test pass because of expectations embedded in the component that I didn't necessarily want to dig further into because of 👇

A few things. This component is technically deprecated and slated for removal shortly. I'm not clear on the original API method signature for eth_signTypedData (v1) since EIP-712 doesn't have a changelog...

FrederikBolding · 2024-07-30T14:48:54Z

.../confirmations/components/signature-request-original/signature-request-original.component.js

@@ -313,10 +311,6 @@ export default class SignatureRequestOriginal extends Component {
          history.push(mostRecentOverviewPage);
        }}
        onSubmit={async () => {
-          if (txData.type === MESSAGE_TYPE.ETH_SIGN) {
-            return this.setState({ showSignatureRequestWarning: true });


Can't we delete the state and component for showSignatureRequestWarning too?

FrederikBolding · 2024-07-30T14:50:37Z

app/scripts/controllers/mmi-controller.ts

-      updatedMsgParams as PersonalMessageParams,
-      req as OriginalRequest,
-    );
+    return undefined;


Nit: Perhaps just throwing here is simpler?

gantunesr

Approved for team-accounts.

Just reviewed the file snap-keyring/metrics.test.ts

sonarcloud · 2024-07-31T15:02:44Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-07-31T15:22:28Z

Builds ready [7c1a3b8]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (207 ± 218 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	57	310	102	57	27
		domContentLoaded	9	60	21	15	7
		load	37	1600	207	454	218
		domInteractive	9	60	21	15	7

Bundle size diffs [🚀 Bundle size reduced!]

background: -8.63 KiB (-0.25%)
ui: -12.09 KiB (-0.17%)
common: -2.47 KiB (-0.04%)

gantunesr

Approved for team-accounts.

Just reviewed the file snap-keyring/metrics.test.ts

albertolive · 2024-08-01T08:53:04Z

app/scripts/controllers/mmi-controller.ts

      return await this.signatureController.newUnsignedPersonalMessage(
        updatedMsgParams as PersonalMessageParams,
        req as OriginalRequest,
      );
    }
-    return await this.signatureController.newUnsignedMessage(


@adonesky1, the only thing I'm afraid of in this PR is the removal of this newUnsignedMessage for the custodian accounts. Let me talk with the team first. cc @zone-live @shane-t

I'm not concerned about it, because it willy only be true if the method is eth_sign based on the other conditions in the block.

adonesky1 changed the title ~~Ad/remove eth sign~~ chore: Fully remove eth_sign May 23, 2024

adonesky1 added the team-wallet-api-platform label May 23, 2024

adonesky1 force-pushed the ad/remove-eth_sign branch from 036e0c6 to 2256e62 Compare June 25, 2024 17:28

Gudahtt mentioned this pull request Jul 18, 2024

chore: fix peer dependency warnings #25941

Closed

7 tasks

adonesky1 force-pushed the ad/remove-eth_sign branch from 641c3e5 to dd98d96 Compare July 18, 2024 20:25

adonesky1 and others added 10 commits July 18, 2024 16:15

fully remove eth_sign support

d144c05

remove disabledRpcMethodPreferences, need migration

b827c9f

remove more instances of eth_sign

2e5e349

fix up e2e tests

dfda3d8

lint

7004a8c

fix post rebase

bdb2f26

remove more instances

24867a7

bump @metamask/signature-controller and @metamask/logging-controller …

aad870b

…versions

Update LavaMoat policies

9db3cc7

fix mmi-controller

60271aa

adonesky1 force-pushed the ad/remove-eth_sign branch from 408b612 to 60271aa Compare July 18, 2024 21:16

adonesky1 added 6 commits July 18, 2024 16:26

more cleanup

d44feda

final fix

8eab5f8

update test

12b2ae2

more cleanup

29ed8da

further cleanup

22d099c

lint + tests fix

6c131d4

adonesky1 force-pushed the ad/remove-eth_sign branch from 9992fc4 to 6c131d4 Compare July 19, 2024 16:35

adonesky1 added 3 commits July 19, 2024 11:55

yet more cleanup

aee10a2

fix

07ecf4e

delete signature-request-original which is no longer used

8a6bca2

adonesky1 requested a review from jiexi July 29, 2024 19:15

jiexi previously approved these changes Jul 29, 2024

View reviewed changes

Gudahtt mentioned this pull request Jul 30, 2024

chore: Upgrade core packages to release 159.0.0 #26138

Merged

7 tasks

FrederikBolding reviewed Jul 30, 2024

View reviewed changes

address feedback + cleanup

343ee96

adonesky1 dismissed jiexi’s stale review via 343ee96 July 30, 2024 17:07

adonesky1 requested a review from FrederikBolding July 30, 2024 17:14

Merge branch 'develop' into ad/remove-eth_sign

e3f16cc

gantunesr previously approved these changes Jul 30, 2024

View reviewed changes

FrederikBolding previously approved these changes Jul 31, 2024

View reviewed changes

remove more unused localization strings

7c1a3b8

adonesky1 dismissed stale reviews from FrederikBolding and gantunesr via 7c1a3b8 July 31, 2024 14:44

FrederikBolding approved these changes Jul 31, 2024

View reviewed changes

adonesky1 requested a review from gantunesr July 31, 2024 16:51

gantunesr approved these changes Jul 31, 2024

View reviewed changes

sleepytanya approved these changes Jul 31, 2024

View reviewed changes

albertolive reviewed Aug 1, 2024

View reviewed changes

albertolive approved these changes Aug 1, 2024

View reviewed changes

adonesky1 merged commit 757b699 into develop Aug 1, 2024
75 checks passed

adonesky1 deleted the ad/remove-eth_sign branch August 1, 2024 14:15

github-actions bot locked and limited conversation to collaborators Aug 1, 2024

metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 1, 2024

gauthierpetetin added release-12.3.0 Issue or pull request that will be included in release 12.3.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Sep 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Fully remove `eth_sign` #24756

chore: Fully remove `eth_sign` #24756

adonesky1 commented May 23, 2024 •

edited

Loading

github-actions bot commented May 23, 2024

socket-security bot commented Jul 18, 2024 •

edited

Loading

adonesky1 commented Jul 18, 2024

metamaskbot commented Jul 18, 2024

FrederikBolding Jul 30, 2024

adonesky1 Jul 30, 2024

adonesky1 Jul 30, 2024

FrederikBolding Jul 30, 2024 •

edited

Loading

adonesky1 Jul 30, 2024

FrederikBolding Jul 30, 2024

adonesky1 Jul 30, 2024

FrederikBolding Jul 30, 2024

adonesky1 Jul 30, 2024

gantunesr left a comment

sonarcloud bot commented Jul 31, 2024

metamaskbot commented Jul 31, 2024

gantunesr left a comment

albertolive Aug 1, 2024

shane-t Aug 1, 2024

chore: Fully remove eth_sign #24756

chore: Fully remove eth_sign #24756

Conversation

adonesky1 commented May 23, 2024 • edited Loading

Explanation

References

Manual testing steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented May 23, 2024

socket-security bot commented Jul 18, 2024 • edited Loading

adonesky1 commented Jul 18, 2024

metamaskbot commented Jul 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrederikBolding Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gantunesr left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 31, 2024

Quality Gate passed

metamaskbot commented Jul 31, 2024

gantunesr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore: Fully remove `eth_sign` #24756

chore: Fully remove `eth_sign` #24756

adonesky1 commented May 23, 2024 •

edited

Loading

socket-security bot commented Jul 18, 2024 •

edited

Loading

FrederikBolding Jul 30, 2024 •

edited

Loading