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.

Sign up for GitHub

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

Jump to bottom

Fully remove eth_sign #4319

Merged
merged 11 commits into from
May 30, 2024
Merged

Fully remove eth_sign #4319

merged 11 commits into from
May 30, 2024

Conversation

adonesky1
Copy link
Contributor

@adonesky1 adonesky1 commented May 24, 2024 •
edited
Loading

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.

References

Changelog

@metamask/signature-controller

  • REMOVED: Methods related to eth_sign signatures - unapprovedMsgCount, messages, newUnsignedMessage - have been removed.
  • REMOVED: constructor argument isEthSignEnabled is no longer expected

@metamask/preferences-controller

  • REMOVED: disabledRpcMethodPreferences removed from state
  • REMOVED: setDisabledRpcMethodPreference removed

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@adonesky1 adonesky1 changed the title Ad/remove eth sign Fully remove eth_sign May 27, 2024
@adonesky1 adonesky1 force-pushed the ad/remove-eth_sign branch 2 times, most recently from 7ef4e59 to fd4e876 Compare May 29, 2024 15:33
@adonesky1 adonesky1 marked this pull request as ready for review May 29, 2024 17:09
@adonesky1 adonesky1 requested a review from a team as a code owner May 29, 2024 17:09
@adonesky1 adonesky1 requested review from a team, shanejonas and jiexi May 29, 2024 17:11
jiexi
jiexi previously approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@jiexi jiexi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems correct to me

@adonesky1
Copy link
Contributor Author

Only question I had was whether we actually want to remove the disabledRpcMethodPreferences state in the PreferencesController in case we want to add different methods later on? Or perhaps that would be unnecessary and confusing

shanejonas
shanejonas previously approved these changes May 30, 2024
View reviewed changes
@adonesky1 adonesky1 dismissed stale reviews from shanejonas and jiexi via 0ea2ff0 May 30, 2024 17:23
@adonesky1 adonesky1 merged commit e6ecd95 into main May 30, 2024
147 checks passed
@adonesky1 adonesky1 deleted the ad/remove-eth_sign branch May 30, 2024 21:03
@mcmire mcmire mentioned this pull request May 30, 2024
Merged
3 tasks
@adonesky1 adonesky1 mentioned this pull request Jul 24, 2024
Merged
7 tasks
adonesky1 added a commit to MetaMask/metamask-extension that referenced this pull request Aug 1, 2024
@adonesky1
chore: Fully remove eth_sign (#24756)
757b699 
## 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](https://ethereum-magicians.org/t/eip-3074-is-unsafe-unnecessary-puts-user-funds-at-risk-while-fragmenting-ux-liquidity-and-the-wallet-stack/19662)
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 MetaMask/MetaMask-planning#2371

## **Manual testing steps**

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

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.
dawnseeker8 pushed a commit to MetaMask/metamask-extension that referenced this pull request Aug 12, 2024
@adonesky1 @dawnseeker8
chore: Fully remove eth_sign (#24756)
e0b68a9 
## 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](https://ethereum-magicians.org/t/eip-3074-is-unsafe-unnecessary-puts-user-funds-at-risk-while-fragmenting-ux-liquidity-and-the-wallet-stack/19662)
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 MetaMask/MetaMask-planning#2371

## **Manual testing steps**

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

## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] 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](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiexi jiexi jiexi approved these changes

@shanejonas shanejonas Awaiting requested review from shanejonas

Assignees
No one assigned
Labels
team-wallet-api-platform
Projects
PR review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adonesky1 @shanejonas @jiexi @vandan