Unblock eth_requestAccounts and add dynamic permission support

[FrederikBolding]

This PR unblocks eth_requestAccounts and add dynamic permission support to the SnapController. This will allow Snaps to request eth_requestAccounts and get the dynamic eth_accounts permission. These dynamic permissions can be revoked again via SnapController:revokeDynamicPermissions.

Unblocking eth_accounts gives snaps access to more RPC methods by default. We disable these as the extension confirmations aren't ready to support snaps quite yet.

This PR needs changes in the extension as well.

Fixes #1451
Fixes #1452

[firnprotocol]

could i ask for some context on this please? i noticed that eth_accounts returns null, and eth_requestAccounts throws MetaMask - RPC Error: The method does not exist / is not available..

knowing the user's account is necessary to perform various basic actions (e.g., calling eth_sign). thanks in advance.

[Montoya]

could i ask for some context on this please? i noticed that eth_accounts returns null, and eth_requestAccounts throws MetaMask - RPC Error: The method does not exist / is not available..

knowing the user's account is necessary to perform various basic actions (e.g., calling eth_sign). thanks in advance.

Hi, we had to disable eth_accounts and eth_requestAccounts in the current Snaps API. The reason for this is that these methods require dynamic permissions, and the current Snaps API does not support that. This PR is for a v2 feature (so, coming soon to Flask and not going to be available as part of the v1 launch of Snaps to the MetaMask extension) which will give Snaps a way to use these methods via dynamic permission.

In the meantime, if you need to do a signature for your snap, you will have to rely on a dapp. An example is here: https://github.com/ziad-saab/siwe-metamask-snap

[firnprotocol]

In the meantime, if you need to do a signature for your snap, you will have to rely on a dapp. An example is here: https://github.com/ziad-saab/siwe-metamask-snap

ok, but this means that the dapp will see the resulting signature, yes? and must "pass it back" somehow to the snap?

this is deeply problematic in situations in which the signature itself is secret, and must be kept within the snap (per the whole security / domain-separation goals we are trying to achieve in the first place by using snaps). an example in which this happens is in, say, services which use a (deterministic) ECDSA signature to derive an app-specific key. Aztec Connect is a very widely-used example where this happens. Firn is another.

if a service such as that were to make a snap, then that signature needs to be kept inside the snap. if a malicious dapp were to see it, and, say, exfiltrate it to the owners of the dapp, then the user would lose funds (and privacy to boot).

[Montoya]

ok, but this means that the dapp will see the resulting signature, yes? and must "pass it back" somehow to the snap?

this is deeply problematic in situations in which the signature itself is secret, and must be kept within the snap (per the whole security / domain-separation goals we are trying to achieve in the first place by using snaps). an example in which this happens is in, say, services which use a (deterministic) ECDSA signature to derive an app-specific key. Aztec Connect is a very widely-used example where this happens. Firn is another.

if a service such as that were to make a snap, then that signature needs to be kept inside the snap. if a malicious dapp were to see it, and, say, exfiltrate it to the owners of the dapp, then the user would lose funds (and privacy to boot).

Yes, I'm aware that this is a huge security concern. If the signature needs to be secret, you can either filter the domain of the dapp that requests the signature (using SIWE and at the RPC level), or I understand if you would rather wait until this new API lands in the Snaps API in stable.

[firnprotocol]

i have to admit i just truly don't catch the drift of this whole "static" vs. "dynamic permissions" distinction.

these methods require dynamic permissions

i have no idea why these methods in particular would require "dynamic" permissions, or why any method for that matter should.

can you just make them static for now?

[Montoya]

i have to admit i just truly don't catch the drift of this whole "static" vs. "dynamic permissions" distinction.

It has to do with how the underlying MetaMask system works.

i have no idea why these methods in particular would require "dynamic" permissions, or why any method for that matter should.

can you just make them static for now?

The Snaps platform is all about balancing developer convenience with user security. Snaps are very powerful programs, and while we could allow developers to do all sorts of things, there are many things that would come at the expense of user security. We made a decision to remove access to eth_accounts until we could figure out a way to do it the right way that ensures that users can consent-fully provide access to ethereum accounts and revoke that access if they wish to. This PR has to do with our exploration in that area.

So no, we cannot make them static for now.

[firnprotocol]

so what i'm gathering is that each snap is essentially entirely oblivious of which account the user is using, and can't perform any account-specific actions—even when the user wants it to! so it's basically restricted to performing pure functions, which have nothing to do with the user's ethereum state.

indeed, even the workaround where the dapp passes in account as a parameter—but the signing still happens fully within the snap—also doesn't work, it appears, since when i call eth_sign with the passed-in account, i'm getting MetaMask - RPC Error: The requested account and/or method has not been authorized by the user.

[Montoya]

so what i'm gathering is that each snap is essentially entirely oblivious of which account the user is using, and can't perform any account-specific actions—even when the user wants it to! so it's basically restricted to performing pure functions, which have nothing to do with the user's ethereum state.

indeed, even the workaround where the dapp passes in account as a parameter—but the signing still happens fully within the snap—also doesn't work, it appears, since when i call eth_sign with the passed-in account, i'm getting MetaMask - RPC Error: The requested account and/or method has not been authorized by the user.

Yes, this is a correct understanding of Snaps v1.

[firnprotocol]

i see, thanks. suffice it to say that i consider this to be a pretty restricted feature set, and am fairly surprised that anyone else can do anything useful with it. please let me know (perhaps in this thread) once this changes. thanks in advance.

[FrederikBolding]

@metamaskbot publish-preview

[github-actions]

Preview builds have been published. See these instructions for more information about preview builds.

Expand for full list of packages and versions.

{
  "@metamask/create-snap": "0.34.1-flask.1",
  "@metamask/multichain-provider": "0.34.1-preview.e0865bc",
  "@metamask/rpc-methods": "0.34.1-preview.e0865bc",
  "@metamask/snaps-browserify-plugin": "0.34.1-preview.e0865bc",
  "@metamask/snaps-cli": "0.34.1-preview.e0865bc",
  "@metamask/snaps-controllers": "0.34.1-preview.e0865bc",
  "@metamask/snaps-execution-environments": "0.34.1-preview.e0865bc",
  "@metamask/snaps-rollup-plugin": "0.34.1-preview.e0865bc",
  "@metamask/snaps-types": "0.34.1-preview.e0865bc",
  "@metamask/snaps-ui": "0.34.1-preview.e0865bc",
  "@metamask/snaps-utils": "0.34.1-preview.e0865bc",
  "@metamask/snaps-webpack-plugin": "0.34.1-preview.e0865bc"
}

[codecov]

Codecov Report

Merging #1421 (65c8f40) into main (53c4ebe) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1421      +/-   ##
==========================================
+ Coverage   95.12%   95.31%   +0.18%     
==========================================
  Files         219      219              
  Lines        4988     4995       +7     
  Branches      765      766       +1     
==========================================
+ Hits         4745     4761      +16     
+ Misses        243      234       -9     

Impacted Files	Coverage Δ
...ages/snaps-controllers/src/snaps/SnapController.ts	96.40% <100.00%> (+1.34%)	⬆️
...s/snaps-execution-environments/src/common/utils.ts	93.33% <100.00%> (ø)

[Mrtenz]

Should we just name this revokePermissions? Is there a need (in the long term) to make a difference between dynamic and static permissions?

[FrederikBolding]

I'm not sure. I named it like this to avoid confusion with the PermissionController.

If all snap permissions end up being dynamic we can probably just remove this again and use the PermissionController directly. But for now it needs to be managed by the SnapController 🤔

[firnprotocol]

hello, can i please ask when this will be released into a metamask flask build? alternatively, is there a way i can locally install this / start testing and developing now? thanks in advance.

[FrederikBolding]

hello, can i please ask when this will be released into a metamask flask build? alternatively, is there a way i can locally install this / start testing and developing now? thanks in advance.

@firnprotocol This has been merged to the MetaMask develop branch, so if you are comfortable building the extension yourself you can do that to leverage this new change! There should be instructions in the README on building.

This will be included in a future MetaMask Flask release, probably within a week or two.

[firnprotocol]

hi @FrederikBolding, thanks. confirmed that i am now able to call eth_requestAccounts as of MetaMask/metamask-extension@805cc31.

however, eth_sign still returns "The method does not exist / is not available.". any insight into this? note that this is personal sign I need; cf. #1420. thanks in advance.

edit: looks like eth_sign is still blocked; any insight into the timeline to unblock? thanks again.

[Montoya]

Hi @firnprotocol can you try using personal_sign method explicitly? Example here: https://docs.metamask.io/wallet/how-to/sign-data/#use-personal_sign

[firnprotocol]

hi @Montoya. same issue—and in fact you can check that personal_sign is also on the BLOCKED_RPC_METHODS list.

snaps/packages/snaps-execution-environments/src/common/utils.ts

Lines 105 to 124 in 0934b73

	export const BLOCKED_RPC_METHODS = Object.freeze([
	'wallet_requestSnaps',
	'wallet_requestPermissions',
	// We disallow all of these confirmations for now, since the screens are not ready for Snaps.
	'eth_sendRawTransaction',
	'eth_sendTransaction',
	'personal_sign',
	'eth_sign',
	'eth_signTypedData',
	'eth_signTypedData_v1',
	'eth_signTypedData_v3',
	'eth_signTypedData_v4',
	'eth_decrypt',
	'eth_getEncryptionPublicKey',
	'wallet_addEthereumChain',
	'wallet_switchEthereumChain',
	'wallet_watchAsset',
	'wallet_registerOnboarding',
	'wallet_scanQRCode',
	]);

.

[FrederikBolding]

@firnprotocol We are currently working on unblocking personal_sign. I don't think we will ever unblock eth_sign as that RPC method is deprecated for dapps and almost exclusively used for malicious purposes.

[firnprotocol]

@FrederikBolding cool. any rough time estimate available?

to clarify, it was only ever personal_sign that i needed; this was my mistake.