Skip to content

feat: optionally display personal_sign data as hex#4039

Closed
bwheeler96 wants to merge 1 commit intoMetaMask:masterfrom
bwheeler96:hex-display-option-in-personal_sign
Closed

feat: optionally display personal_sign data as hex#4039
bwheeler96 wants to merge 1 commit intoMetaMask:masterfrom
bwheeler96:hex-display-option-in-personal_sign

Conversation

@bwheeler96
Copy link
Copy Markdown

Allows developers to specify the display encoding of a personal_sign request in the RPC call. Fixes #3931.

For example, requesting users to sign a sha3 hash with personal_sign would result in ugly UTF-8 renderings of hex data. Developers can now sign this data using web3.personal.sign('0xabcdef', web3.eth.coinbase, { encoding: 'hex' }, callback) and the data will be displayed as hex.

I attempted to add a test case in the confirm-sig-requests.js integration test, but I was having difficulty getting the request I added in the development/states/confirm-sig-requests.json file to appear in the final test. Maybe someone can point me in the right direction?

Allows developers to specify the display encoding of a personal_sign request in the RPC call. Closes MetaMask#3931.

For example, requesting users to sign a sha3 hash with personal_sign would result in ugly UTF-8 renderings of hex data. Developers can now sign this data using `web3.personal.sign('0xabcdef', web3.eth.coinbase, { encoding: 'hex' }, callback)` and the data will be displayed as hex.
@danfinlay
Copy link
Copy Markdown
Contributor

Like I mentioned in #4117, I think this opens some phishing vulnerabilities, since a site could present another site's challenge in hex encoding, obscuring its actual meaning.

You replied on that issue:

Personally I think the ultimate solution is for the client application to send a list of items and their ABI encoding to MM Array<Item<String,enum{uint,string,etc...}>> and have metamask show that data to the user. Then the user can sign the more-readable data, and MM can hash the data using the soliditySha3 function, then MM will produce a signature for prefix + soliditySha3(data).

To that I reply:

It sounds like you're waiting for EIP 712 to finalize.

In the meanwhile, you can use a temporary beta implementation that is sure to break, to see how it feels:
https://medium.com/metamask/scaling-web3-with-signtypeddata-91d6efc8b290

Also please do go over there, review the spec, and see if you can help solidify it! I can't wait for a good version to be produced.

My ideal result would include a version byte or parameter, so the signer could add new different signing strategies without breaking existing dapps.

Closing this for now b/c the security concern and the better alternative available.

@danfinlay danfinlay closed this May 2, 2018
@bwheeler96
Copy link
Copy Markdown
Author

@danfinlay EIP 712 is not backwards compatible which sucks. I would argue that the phishing argument is not very valid as pretty much 100% of users are clicking right through the massive warning that could be used to sign arbitrary data (including other sites TOS, transactions, etc).

I understand your decision, but I think individual wallet clients need to start spurring innovation independently, otherwise improvements (like EIP 712) are going to sit in the queue forever.

@danfinlay
Copy link
Copy Markdown
Contributor

100% of users are clicking right through the massive warning that could be used to sign arbitrary data (including other sites TOS, transactions, etc).

People click through the hex blob, but people read the legible text in signatures like on CryptoKitties. We need to tend towards more comprehensibility, not less.

individual wallet clients need to start spurring innovation independently, otherwise improvements (like EIP 712) are going to sit in the queue forever.

Take some ownership of the ecosystem and contribute, or you'll wait for centralized client devs to do all your innovating for you. We'll get there, just slower than you'd like. PRs welcome.

@danfinlay
Copy link
Copy Markdown
Contributor

Btw we're hiring as fast as we reasonably can (we're on a training cycle now). We are accelerating, and we will get there, but incentivized parties are always welcome to accelerate the features they are passionate about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants