Fix signing method bugs #8833

rekmarks · 2020-06-18T18:11:35Z

This PR fixes a number of bugs related to our signing methods. Most of them are fixed by updating eth-json-rpc-middleware.

Summary of Changes

Add [email protected]
Add personal_ecRecover to permissions safe methods
Minor refactor of sign typed data validation
Remove now-redundant normalization of transaction from address

Changes in Detail

Add [email protected]
- For a detailed summary of what this does, see: Normalize and require keyholder address eth-json-rpc-middleware#29 (review)
- We have other dependencies that depend on 4.1.1 of this package, but 5.0.0 did not change the parts that they consume.
- Some tests had to be updated now that bad parameters are rejected at the middleware layer
Add personal_ecRecover to permissions safe methods
- The method will now work again
Minor refactor of sign typed data validation
- It's now more DRY, and with better error messages
Remove now-redundant normalization of transaction from address
- The from address is already normalized by the time it hits addUnapprovedTransaction, and the tx-state-manager does not lowercase addresses. Thus, it's useless.
- The to address is still lowercased by addUnapprovedTransacation

metamaskbot · 2020-06-18T18:28:19Z

Builds ready [b09a391]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (589 ± 27 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	29	81	39	16	8
		domContentLoaded	344	624	588	58	28
		load	347	625	589	57	27
		domInteractive	344	623	587	58	28

rekmarks · 2020-06-23T04:50:19Z

app/scripts/controllers/permissions/enums.js

@@ -75,6 +75,7 @@ export const SAFE_METHODS = [
  'eth_sendTransaction',
  'eth_sign',
  'personal_sign',
+  'personal_ecRecover',


We'd forgotten to add it, so it wasn't working!

rekmarks · 2020-06-23T04:54:46Z

test/unit/app/controllers/metamask-controller-test.js

+let loggerMiddlewareMock
+const initializeMockMiddlewareLog = () => {
+  loggerMiddlewareMock = {
+    requests: [],
+    responses: [],
+  }
+}
+const tearDownMockMiddlewareLog = () => {
+  loggerMiddlewareMock = undefined
+}
+
+const createLoggerMiddlewareMock = () => (req, res, next) => {
+  if (loggerMiddlewareMock) {
+    loggerMiddlewareMock.requests.push(req)
+    next((cb) => {
+      loggerMiddlewareMock.responses.push(res)
+      cb()
+    })
+  } else {
+    next()
+  }
+}
+


This mock was added to get around a new test failure.

We were relying on the lack of validation in eth-json-rpc-middleware to test middleware behavior by means of a bogus transaction. Bogus transactions are now rejected by the middleware, so that no longer works.

This mock allows us to arbitrarily inspect RPC requests sent during tests. I'm open to moving it elsewhere, but couldn't think of a better solution.

app/scripts/lib/typed-message-manager.js

Gudahtt · 2020-06-23T05:23:26Z

Nit: normalizeTxParams is still doing a redundant lower-case of the address. It should probably be updated to validate that the address is already lower-cased, instead of calling toLowerCase

metamaskbot · 2020-06-23T05:30:31Z

Builds ready [f80a3a6]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (839 ± 95 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	39	139	74	31	15
		domContentLoaded	363	1489	837	198	95
		load	364	1490	839	198	95
		domInteractive	362	1488	836	198	95

metamaskbot · 2020-06-23T05:37:39Z

Builds ready [4063984]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (691 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	33	107	46	20	9
		domContentLoaded	426	810	689	77	37
		load	427	812	691	78	37
		domInteractive	425	809	689	77	37

Gudahtt · 2020-06-23T05:44:12Z

Nit: normalizeTxParams is still doing a redundant lower-case of the address. It should probably be updated to validate that the address is already lower-cased, instead of calling toLowerCase

Actually, after looking at this further, it would probably make more sense for this validation to be in validateTxParams. It's only called in one place - immediately after the parameters are normalized. That would still ensure it blew up.

metamaskbot · 2020-06-23T06:43:45Z

Builds ready [00d7807]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (1201 ± 86 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	NaN	NaN	NaN	NaN	NaN
		domContentLoaded	612	1577	1198	179	86
		load	615	1578	1201	179	86
		domInteractive	612	1576	1197	179	86

app/scripts/controllers/transactions/lib/util.js

Gudahtt

LGTM!

metamaskbot · 2020-06-23T16:06:11Z

Builds ready [a194919]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
dep viz: background
all artifacts

Page Load Metrics (671 ± 42 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	86	42	15	7
		domContentLoaded	374	845	670	87	42
		load	376	847	671	87	42
		domInteractive	374	844	670	87	42

* origin/develop: Fix signing method bugs (#8833) replace icons with Checkbox component (#8830) Use [email protected] (#8845) Use [email protected] (#8844) Call getMethodDataAsync when knownMethodData[fourBytePrefix] object is empty (#8836) Update connected status popover content (#8834) Use @metamask/[email protected] (#8832) ParseInt nextworkNextNonce correction (#8827) Fix first time onboarding popup position (#8829) fix overflowing contract names and origins (#8823) Hide 'Expand view' button in fullscreen (#8826)

rekmarks added 2 commits June 17, 2020 14:44

update signTypedData validation

1cb651c

update tests for new eth-json-rpc-middleware

48e857a

rekmarks added the DO-NOT-MERGE Pull requests that should not be merged label Jun 18, 2020

update eth-json-rpc-middleware

2c2a21e

rekmarks force-pushed the sign-typed-message-fix branch from b09a391 to 2c2a21e Compare June 23, 2020 04:49

rekmarks commented Jun 23, 2020

View reviewed changes

rekmarks changed the title ~~Sign typed message fix~~ Fix signing method bugs Jun 23, 2020

rekmarks commented Jun 23, 2020

View reviewed changes

fixup! update eth-json-rpc-middleware

f80a3a6

Gudahtt reviewed Jun 23, 2020

View reviewed changes

app/scripts/lib/typed-message-manager.js Show resolved Hide resolved

add back params.data typecheck

4063984

rekmarks marked this pull request as ready for review June 23, 2020 05:14

rekmarks requested a review from a team as a code owner June 23, 2020 05:14

Gudahtt reviewed Jun 23, 2020

View reviewed changes

app/scripts/controllers/transactions/lib/util.js Outdated Show resolved Hide resolved

rekmarks force-pushed the sign-typed-message-fix branch from 00d7807 to 3635190 Compare June 23, 2020 15:43

remove lowercasing of tx 'from' addresses

a194919

rekmarks force-pushed the sign-typed-message-fix branch from 3635190 to a194919 Compare June 23, 2020 15:43

Gudahtt approved these changes Jun 23, 2020

View reviewed changes

rekmarks removed the DO-NOT-MERGE Pull requests that should not be merged label Jun 23, 2020

rekmarks merged commit 04de9a9 into develop Jun 23, 2020

rekmarks deleted the sign-typed-message-fix branch June 23, 2020 16:12

Gudahtt mentioned this pull request Jun 25, 2020

Message: Cannot read property #8808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix signing method bugs #8833

Fix signing method bugs #8833

rekmarks commented Jun 18, 2020 •

edited

Loading

metamaskbot commented Jun 18, 2020

rekmarks Jun 23, 2020

rekmarks Jun 23, 2020 •

edited

Loading

Gudahtt commented Jun 23, 2020 •

edited

Loading

metamaskbot commented Jun 23, 2020

metamaskbot commented Jun 23, 2020

Gudahtt commented Jun 23, 2020

metamaskbot commented Jun 23, 2020

Gudahtt left a comment

metamaskbot commented Jun 23, 2020

Fix signing method bugs #8833

Fix signing method bugs #8833

Conversation

rekmarks commented Jun 18, 2020 • edited Loading

Summary of Changes

Changes in Detail

metamaskbot commented Jun 18, 2020

rekmarks Jun 23, 2020

Choose a reason for hiding this comment

rekmarks Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Gudahtt commented Jun 23, 2020 • edited Loading

metamaskbot commented Jun 23, 2020

metamaskbot commented Jun 23, 2020

Gudahtt commented Jun 23, 2020

metamaskbot commented Jun 23, 2020

Gudahtt left a comment

Choose a reason for hiding this comment

metamaskbot commented Jun 23, 2020

rekmarks commented Jun 18, 2020 •

edited

Loading

rekmarks Jun 23, 2020 •

edited

Loading

Gudahtt commented Jun 23, 2020 •

edited

Loading