Skip to content

keyring-controller: validate from-address in signTypedMessage - #1293

Merged
legobeat merged 2 commits into
MetaMask:mainfrom
legobeat:fix-sign-empty-address
May 3, 2023
Merged

keyring-controller: validate from-address in signTypedMessage#1293
legobeat merged 2 commits into
MetaMask:mainfrom
legobeat:fix-sign-empty-address

Conversation

@legobeat

@legobeatlegobeat commented May 2, 2023

Copy link
Copy Markdown
Contributor

The normalizeAddress function is can return undefined even if the interface says otherwise.

This adds explicit handling of that case in keyring-controller. Required for #1289 .

@legobeatlegobeat mentioned this pull request May 2, 2023
Closed
@legobeat
legobeatforce-pushed the fix-sign-empty-address branch 2 times, most recently from 35b9b1d to 4705ae4CompareMay 2, 2023 02:35
@legobeat
legobeat marked this pull request as ready for review May 2, 2023 02:36
@legobeat
legobeat requested a review from a team as a code ownerMay 2, 2023 02:36
mikesposito
mikesposito reviewed May 2, 2023
View reviewed changes

@mikespositomikesposito left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Left just a small suggestion

Comment threadpackages/keyring-controller/src/KeyringController.ts Outdated

@mikespositomikespositoMay 2, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: maybe we can use isValidAddress from ethereumjs-util to be sure that what we get is an address

@legobeatlegobeatMay 2, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated along with PR title.

@legobeat
legobeat requested a review from mikespositoMay 2, 2023 22:58
@legobeat
legobeatforce-pushed the fix-sign-empty-address branch from cdf3025 to 8b42427CompareMay 2, 2023 23:00
@legobeat
legobeatforce-pushed the fix-sign-empty-address branch from 8b42427 to 67d33efCompareMay 2, 2023 23:00
@legobeatlegobeat changed the title keyring-controller: throw explicit error on undefined result from normalizeAddresskeyring-controller: validate from address in signTypedMessageMay 2, 2023
@legobeatlegobeat changed the title keyring-controller: validate from address in signTypedMessagekeyring-controller: validate from-address in signTypedMessageMay 2, 2023
Gudahtt
Gudahtt approved these changes May 3, 2023
View reviewed changes

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@legobeat
legobeat merged commit bdeee5a into MetaMask:mainMay 3, 2023
Gudahtt added a commit that referenced this pull request May 3, 2023
@Gudahtt
Merge remote-tracking branch 'origin/main' into added-network-clients…
32736e8
…-rebased
* origin/main:
docs: update controller-utils/isValidHexAddress to match reality (#1308)
keyring-controller: validate from-address in signTypedMessage (#1293)
NetworkController: Fix chain IDs in tests (#1307)
NetworkController: Normalize INFURA_NETWORKS array (#1306)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GudahttGudahttGudahtt approved these changes
@mikespositomikespositoAwaiting requested review from mikesposito

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@legobeat@Gudahtt@mikesposito