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

Render personal_sign messages as utf-8 text #1177

Merged
merged 7 commits into from
Mar 7, 2017

Conversation

danfinlay
Copy link
Contributor

Calls to personal_sign are now:

  • When hex encoded, preserved as hex encoded, but displayed as utf-8 text.
  • When not hex encoded, decoded as utf-8 text as hex for signing.
  • The messages proposed for signing are displayed as UTF-8 text.
  • When the message cannot be rendered as UTF-8 text, it is displayed as hexadecimal.

screen shot 2017-03-06 at 2 58 55 pm

Fixes #1173

Calls to `personal_sign` are now:

- When hex encoded, preserved as hex encoded, but displayed as utf-8 text.
- When not hex encoded, decoded as utf-8 text as hex for signing.
- The messages proposed for signing are displayed as UTF-8 text.
- When the message cannot be rendered as UTF-8 text, it is displayed as hexadecimal.

Fixes #1173
@kumavis
Copy link
Member

kumavis commented Mar 6, 2017

When not hex encoded

I think we should hard fail here, as hex encoding is a valid utf8 encoding

@danfinlay
Copy link
Contributor Author

Sample exception: 0x1 is valid hex but is not utf8 decodable.

@danfinlay
Copy link
Contributor Author

What would a hard fail look like here? Throw an error to the sign request, and not show it to the user?

@kumavis
Copy link
Member

kumavis commented Mar 6, 2017

throw an error to the sign request

@kumavis
Copy link
Member

kumavis commented Mar 6, 2017

One example of unexpected behavior would be signing a number (e.g. a unix timestamp) -- in this implementation it would be interpreted as hex instead of utf8

@danfinlay
Copy link
Contributor Author

The old eth_sign implementation hard-requires a hex-prefix 0x, and otherwise interpretes as utf8. That also seemed weird. What if a utf8 string started with 0x?

The safest move would maybe be to only allow hex input, try to decode it, but fall back to the original input.

@@ -21,6 +22,7 @@ PendingMsgDetails.prototype.render = function () {
var account = state.accounts[address] || { address: address }

var { data } = msgParams
console.dir({ msgParams })
Copy link
Member

Choose a reason for hiding this comment

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

plz remove

@kumavis
Copy link
Member

kumavis commented Mar 6, 2017

The safest move would maybe be to only allow hex input, try to decode it, but fall back to the original input.

this isnt safe if the developer is expecting the fallback case but (accidently) ends up with valid hex

@danfinlay
Copy link
Contributor Author

This is what MIME types are for, and what I was originally postponing this feature for. Maybe we should just wait for that, I don't see a correct way forward here.

@kumavis
Copy link
Member

kumavis commented Mar 6, 2017

my philosophy here is:
for developer input, have one correct way of doing things
for user input, be flexible

@kumavis
Copy link
Member

kumavis commented Mar 7, 2017

I look forward to the MIME type spec - but that feature actually happens "after" this intial decoding

we are dealing with the first layer of this stack:

  • decode raw RPC params
  • parse for MIME type
  • display message signing UI

@chafey
Copy link

chafey commented Mar 7, 2017

In many cases, a timestamp will be included in the message to be signed to prevent replay attacks. The timestamp is not necessarily something we want to display to the user though - yet this scheme would do so. I would love to be able to pass a "display string" that is presented to the user in the confirmation window to help guide them through the process. For example "Please press "Confirm" if you would like to do X". We can make the actual message available to the user for inspection if they so desire, but it may not be meaningful to them

@danfinlay
Copy link
Contributor Author

I think mime content headers would adequately allow specifying portions of the signed text to render vs to hide: #925

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.

4 participants