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

Fix signTypedData_v4 chainId validation #9552

Merged
merged 1 commit into from
Oct 12, 2020
Merged

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Oct 12, 2020

We were checking the chainId passed with signTypedData_v4 requests against NetworkController.network, which is wrong. This PR ensures that we check it against the stored chainId.

Fixes #8385

@metamaskbot
Copy link
Collaborator

Builds ready [876ce04]
Page Load Metrics (433 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3293522211
domContentLoaded32269143110952
load32369343310952
domInteractive32269043110952

@metamaskbot
Copy link
Collaborator

Builds ready [876ce04]
Page Load Metrics (377 ± 38 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint2910439167
domContentLoaded2945943768038
load2965953778038
domInteractive2945923758038

danfinlay
danfinlay previously approved these changes Oct 12, 2020
Copy link
Contributor

@danfinlay danfinlay left a comment

Choose a reason for hiding this comment

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

This looks good to me, glad we're finally using the semantic meaning of chainId instead of networkId's "usually the same" behavior.

@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@rekmarks rekmarks marked this pull request as ready for review October 12, 2020 18:12
@rekmarks rekmarks requested a review from a team as a code owner October 12, 2020 18:12
@rekmarks rekmarks requested review from whymarrh and removed request for a team October 12, 2020 18:12
@rekmarks rekmarks changed the base branch from remove-localhost-provider-type to develop October 12, 2020 18:12
@rekmarks rekmarks dismissed danfinlay’s stale review October 12, 2020 18:12

The base branch was changed.

@metamaskbot
Copy link
Collaborator

Builds ready [06c0da6]
Page Load Metrics (443 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30493842
domContentLoaded30969744110149
load31069944310149
domInteractive30969744110149

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@danfinlay danfinlay merged commit 45ba657 into develop Oct 12, 2020
@danfinlay danfinlay deleted the signTypedData-chainId-fix branch October 12, 2020 19:10
@github-actions github-actions bot locked and limited conversation to collaborators Oct 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metamask doesn't work with ganache with typed message
4 participants