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

EIP-712: Sign typed data #4803

Merged
merged 4 commits into from
Sep 17, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- [#4606](https://github.com/MetaMask/metamask-extension/pull/4606): Add new metamask_watchAsset method.
- [#5189](https://github.com/MetaMask/metamask-extension/pull/5189): Fix bug where Ropsten loading message is shown when connecting to Kovan.
- [#4752](https://github.com/MetaMask/metamask-extension/issues/4752): Implement latest `eth_signTypedData` specification.

## 4.9.3 Wed Aug 15 2018

Expand Down
74 changes: 52 additions & 22 deletions app/scripts/lib/typed-message-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const createId = require('./random-id')
const assert = require('assert')
const sigUtil = require('eth-sig-util')
const log = require('loglevel')
const jsonschema = require('jsonschema')

/**
* Represents, and contains data about, an 'eth_signTypedData' type signature request. These are created when a
Expand All @@ -17,7 +18,7 @@ const log = require('loglevel')
* @property {Object} msgParams.from The address that is making the signature request.
* @property {string} msgParams.data A hex string conversion of the raw buffer data of the signature request
* @property {number} time The epoch time at which the this message was created
* @property {string} status Indicates whether the signature request is 'unapproved', 'approved', 'signed' or 'rejected'
* @property {string} status Indicates whether the signature request is 'unapproved', 'approved', 'signed', 'rejected', or 'errored'
* @property {string} type The json-prc signing method for which a signature request has been made. A 'Message' will
* always have a 'eth_signTypedData' type.
*
Expand All @@ -26,17 +27,10 @@ const log = require('loglevel')
module.exports = class TypedMessageManager extends EventEmitter {
/**
* Controller in charge of managing - storing, adding, removing, updating - TypedMessage.
*
* @typedef {Object} TypedMessage
* @param {Object} opts @deprecated
* @property {Object} memStore The observable store where TypedMessage are saved.
* @property {Object} memStore.unapprovedTypedMessages A collection of all TypedMessages in the 'unapproved' state
* @property {number} memStore.unapprovedTypedMessagesCount The count of all TypedMessages in this.memStore.unapprobedMsgs
* @property {array} messages Holds all messages that have been created by this TypedMessage
*
*/
constructor (opts) {
constructor ({ networkController }) {
super()
this.networkController = networkController
this.memStore = new ObservableStore({
unapprovedTypedMessages: {},
unapprovedTypedMessagesCount: 0,
Expand Down Expand Up @@ -76,15 +70,17 @@ module.exports = class TypedMessageManager extends EventEmitter {
* @returns {promise} When the message has been signed or rejected
*
*/
addUnapprovedMessageAsync (msgParams, req) {
addUnapprovedMessageAsync (msgParams, req, version) {
return new Promise((resolve, reject) => {
const msgId = this.addUnapprovedMessage(msgParams, req)
const msgId = this.addUnapprovedMessage(msgParams, req, version)
this.once(`${msgId}:finished`, (data) => {
switch (data.status) {
case 'signed':
return resolve(data.rawSig)
case 'rejected':
return reject(new Error('MetaMask Message Signature: User denied message signature.'))
case 'errored':
return reject(new Error(`MetaMask Message Signature: ${data.error}`))
default:
return reject(new Error(`MetaMask Message Signature: Unknown problem: ${JSON.stringify(msgParams)}`))
}
Expand All @@ -102,7 +98,8 @@ module.exports = class TypedMessageManager extends EventEmitter {
* @returns {number} The id of the newly created TypedMessage.
*
*/
addUnapprovedMessage (msgParams, req) {
addUnapprovedMessage (msgParams, req, version) {
msgParams.version = version
this.validateParams(msgParams)
// add origin from request
if (req) msgParams.origin = req.origin
Expand Down Expand Up @@ -132,14 +129,33 @@ module.exports = class TypedMessageManager extends EventEmitter {
*
*/
validateParams (params) {
assert.equal(typeof params, 'object', 'Params should ben an object.')
assert.ok('data' in params, 'Params must include a data field.')
assert.ok('from' in params, 'Params must include a from field.')
assert.ok(Array.isArray(params.data), 'Data should be an array.')
assert.equal(typeof params.from, 'string', 'From field must be a string.')
assert.doesNotThrow(() => {
sigUtil.typedSignatureHash(params.data)
}, 'Expected EIP712 typed data')
switch (params.version) {
Copy link
Contributor Author

@bitpshr bitpshr Sep 10, 2018

Choose a reason for hiding this comment

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

For now, since there are only two versions of the proposal, we include both validation logic paths in the same TypedMessageManager class. If there ends up being even more iterations of the proposal with different validation schemes (unlikely, but possible), we can break this class down into a base Manager class and multiple implementations that extend it with a custom validateParams method.

case 'V1':
assert.equal(typeof params, 'object', 'Params should ben an object.')
assert.ok('data' in params, 'Params must include a data field.')
assert.ok('from' in params, 'Params must include a from field.')
assert.ok(Array.isArray(params.data), 'Data should be an array.')
assert.equal(typeof params.from, 'string', 'From field must be a string.')
assert.doesNotThrow(() => {
sigUtil.typedSignatureHash(params.data)
}, 'Expected EIP712 typed data')
break
case 'V3':
let data
assert.equal(typeof params, 'object', 'Params should be an object.')
assert.ok('data' in params, 'Params must include a data field.')
assert.ok('from' in params, 'Params must include a from field.')
assert.equal(typeof params.from, 'string', 'From field must be a string.')
assert.equal(typeof params.data, 'string', 'Data must be passed as a valid JSON string.')
assert.doesNotThrow(() => { data = JSON.parse(params.data) }, 'Data must be passed as a valid JSON string.')
const validation = jsonschema.validate(data, sigUtil.TYPED_MESSAGE_SCHEMA)
assert.ok(data.primaryType in data.types, `Primary type of "${data.primaryType}" has no type definition.`)
assert.equal(validation.errors.length, 0, 'Data must conform to EIP-712 schema. See https://git.io/fNtcx.')
const chainId = data.domain.chainId
const activeChainId = parseInt(this.networkController.getNetworkState())
chainId && assert.equal(chainId, activeChainId, `Provided chainId (${chainId}) must match the active chainId (${activeChainId})`)
break
}
}

/**
Expand Down Expand Up @@ -214,6 +230,7 @@ module.exports = class TypedMessageManager extends EventEmitter {
*/
prepMsgForSigning (msgParams) {
delete msgParams.metamaskId
delete msgParams.version
return Promise.resolve(msgParams)
}

Expand All @@ -227,6 +244,19 @@ module.exports = class TypedMessageManager extends EventEmitter {
this._setMsgStatus(msgId, 'rejected')
}

/**
* Sets a TypedMessage status to 'errored' via a call to this._setMsgStatus.
*
* @param {number} msgId The id of the TypedMessage to error
*
*/
errorMessage (msgId, error) {
const msg = this.getMsg(msgId)
msg.error = error
this._updateMsg(msg)
this._setMsgStatus(msgId, 'errored')
}

//
// PRIVATE METHODS
//
Expand All @@ -250,7 +280,7 @@ module.exports = class TypedMessageManager extends EventEmitter {
msg.status = status
this._updateMsg(msg)
this.emit(`${msgId}:${status}`, msg)
if (status === 'rejected' || status === 'signed') {
if (status === 'rejected' || status === 'signed' || status === 'errored') {
Copy link
Contributor

Choose a reason for hiding this comment

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

unclear when status gets set to 'errored' due to errors thrown in validateParams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I follow; what's unclear? The conditions that cause a message to be considered invalid? Error descriptions should be fine; are you suggesting to improve the documentation for the validateParams method to explicitly state what conditions are validated?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry I was being dense, I didn't see the call to set the message status in metamask-controller

this.emit(`${msgId}:finished`, msg)
}
}
Expand Down
77 changes: 60 additions & 17 deletions app/scripts/metamask-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ const log = require('loglevel')
const TrezorKeyring = require('eth-trezor-keyring')
const LedgerBridgeKeyring = require('eth-ledger-bridge-keyring')
const EthQuery = require('eth-query')
const ethUtil = require('ethereumjs-util')
const sigUtil = require('eth-sig-util')

module.exports = class MetamaskController extends EventEmitter {

Expand Down Expand Up @@ -205,7 +207,7 @@ module.exports = class MetamaskController extends EventEmitter {
this.networkController.lookupNetwork()
this.messageManager = new MessageManager()
this.personalMessageManager = new PersonalMessageManager()
this.typedMessageManager = new TypedMessageManager()
this.typedMessageManager = new TypedMessageManager({ networkController: this.networkController })
this.publicConfigStore = this.initPublicConfigStore()

this.store.updateStructure({
Expand Down Expand Up @@ -266,7 +268,6 @@ module.exports = class MetamaskController extends EventEmitter {
// msg signing
processEthSignMessage: this.newUnsignedMessage.bind(this),
processPersonalMessage: this.newUnsignedPersonalMessage.bind(this),
processTypedMessage: this.newUnsignedTypedMessage.bind(this),
}
const providerProxy = this.networkController.initializeProvider(providerOpts)
return providerProxy
Expand Down Expand Up @@ -975,22 +976,31 @@ module.exports = class MetamaskController extends EventEmitter {
* @param {Object} msgParams - The params passed to eth_signTypedData.
* @returns {Object} Full state update.
*/
signTypedMessage (msgParams) {
log.info('MetaMaskController - signTypedMessage')
async signTypedMessage (msgParams) {
log.info('MetaMaskController - eth_signTypedData')
const msgId = msgParams.metamaskId
// sets the status op the message to 'approved'
// and removes the metamaskId for signing
return this.typedMessageManager.approveMessage(msgParams)
.then((cleanMsgParams) => {
// signs the message
return this.keyringController.signTypedMessage(cleanMsgParams)
})
.then((rawSig) => {
// tells the listener that the message has been signed
// and can be returned to the dapp
this.typedMessageManager.setMsgStatusSigned(msgId, rawSig)
return this.getState()
})
const version = msgParams.version
try {
Copy link
Contributor Author

@bitpshr bitpshr Sep 10, 2018

Choose a reason for hiding this comment

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

Previously, we called down into the KeyringController, which in turn called into a specific keyring, which then called called signTypedData. This dependency depth made changes very difficult and it seemed like potentially-unnecessary indirection. To get around this, the MetaMaskController just does the signing directly without traversing down to a specific keyring. This also lends itself well to a future of signature-specific controllers at the extension level.

const cleanMsgParams = await this.typedMessageManager.approveMessage(msgParams)
const address = sigUtil.normalize(cleanMsgParams.from)
const keyring = await this.keyringController.getKeyringForAccount(address)
const wallet = keyring._getWalletForAccount(address)
const privKey = ethUtil.toBuffer(wallet.getPrivateKey())
let signature
switch (version) {
case 'V1':
signature = sigUtil.signTypedDataLegacy(privKey, { data: cleanMsgParams.data })
break
case 'V3':
signature = sigUtil.signTypedData(privKey, { data: JSON.parse(cleanMsgParams.data) })
break
}
this.typedMessageManager.setMsgStatusSigned(msgId, signature)
return this.getState()
} catch (error) {
log.info('MetaMaskController - eth_signTypedData failed.', error)
this.typedMessageManager.errorMessage(msgId, error)
}
}

/**
Expand Down Expand Up @@ -1241,6 +1251,9 @@ module.exports = class MetamaskController extends EventEmitter {
engine.push(createLoggerMiddleware({ origin }))
engine.push(filterMiddleware)
engine.push(this.preferencesController.requestWatchAsset.bind(this.preferencesController))
engine.push(this.createTypedDataMiddleware('eth_signTypedData', 'V1').bind(this))
engine.push(this.createTypedDataMiddleware('eth_signTypedData_v1', 'V1').bind(this))
engine.push(this.createTypedDataMiddleware('eth_signTypedData_v3', 'V3').bind(this))
engine.push(createProviderMiddleware({ provider: this.provider }))

// setup connection
Expand Down Expand Up @@ -1474,4 +1487,34 @@ module.exports = class MetamaskController extends EventEmitter {
set isClientOpenAndUnlocked (active) {
this.tokenRatesController.isActive = active
}

/**
* Creates RPC engine middleware for processing eth_signTypedData requests
*
* @param {Object} req - request object
* @param {Object} res - response object
* @param {Function} - next
* @param {Function} - end
*/
createTypedDataMiddleware (methodName, version) {
return async (req, res, next, end) => {
const { method, params } = req
if (method === methodName) {
const promise = this.typedMessageManager.addUnapprovedMessageAsync({
data: params.length >= 1 && params[0],
from: params.length >= 2 && params[1],
}, req, version)
this.sendUpdate()
this.opts.showUnconfirmedMessage()
try {
res.result = await promise
end()
} catch (error) {
end(error)
}
} else {
next()
}
}
}
}
3 changes: 2 additions & 1 deletion old-ui/app/components/pending-typed-msg-details.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ PendingMsgDetails.prototype.render = function () {
var identity = state.identities[address] || { address: address }
var account = state.accounts[address] || { address: address }

var { data } = msgParams
var { data, version } = msgParams

return (
h('div', {
Expand All @@ -48,6 +48,7 @@ PendingMsgDetails.prototype.render = function () {
h('label.font-small', { style: { display: 'block' } }, 'YOU ARE SIGNING'),
h(TypedMessageRenderer, {
value: data,
version,
style: {
height: '215px',
},
Expand Down
27 changes: 25 additions & 2 deletions old-ui/app/components/typed-message-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const Component = require('react').Component
const h = require('react-hyperscript')
const inherits = require('util').inherits
const extend = require('xtend')
const { ObjectInspector } = require('react-inspector')

module.exports = TypedMessageRenderer

Expand All @@ -12,8 +13,16 @@ function TypedMessageRenderer () {

TypedMessageRenderer.prototype.render = function () {
const props = this.props
const { value, style } = props
const text = renderTypedData(value)
const { value, version, style } = props
let text
switch (version) {
case 'V1':
text = renderTypedData(value)
break
case 'V3':
text = renderTypedDataV3(value)
break
}

const defaultStyle = extend({
width: '315px',
Expand Down Expand Up @@ -44,3 +53,17 @@ function renderTypedData (values) {
])
})
}

function renderTypedDataV3 (values) {
const { domain, message } = JSON.parse(values)
return [
domain ? h('div', [
h('h1', 'Domain'),
h(ObjectInspector, { data: domain, expandLevel: 1, name: 'domain' }),
]) : '',
message ? h('div', [
h('h1', 'Message'),
h(ObjectInspector, { data: message, expandLevel: 1, name: 'message' }),
]) : '',
]
}
Loading