Skip to content

validate addresses in qr codes#9916

Merged
brad-decker merged 1 commit intodevelopfrom
fix-9889
Nov 19, 2020
Merged

validate addresses in qr codes#9916
brad-decker merged 1 commit intodevelopfrom
fix-9889

Conversation

@brad-decker
Copy link
Copy Markdown
Contributor

@brad-decker brad-decker commented Nov 19, 2020

Fixes: #9889

Explanation: We didn't do any validation of the to address, and this would incorrectly interpret ERC-20 token send encoded uris as being sent to the token address. This PR just checks the validity of the address.

Valid QR code (ethereum:xxxx)
Invalid QR code (ethereum:xxxx/transfer?address=xxxx&uint256=1) vis a vis eip-681

Manual testing steps:

  • Open extension
  • Click send
  • Click the QR code scanner button in ENS input field
  • Scan the valid QR code example
  • See that an address is populated.
  • Repeat steps above with the invalid QR code
  • See an error message.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [9ae0c2f]
Page Load Metrics (420 ± 68 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint29544073
domContentLoaded26379441914268
load26479542014268
domInteractive26379341914268

@brad-decker brad-decker marked this pull request as ready for review November 19, 2020 15:21
@brad-decker brad-decker requested a review from a team as a code owner November 19, 2020 15:21
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.

Wow, that is an.... interesting method name and signature 😬. It does the trick though at least!

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.

Though actually, it does give a pretty strange error on testnets 🤔

Not ETH network, set to lowercase seems inappropriate here

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.

It might be worth using isValidAddress and INVALID_RECIPIENT_ADDRESS_ERROR directly perhaps, instead of using that rather confusing helper function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Gudahtt -- I was following the pattern in the file and had it working before I realized how... wonky it was. 🤦‍♂️

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [7a6e82f]
Page Load Metrics (404 ± 56 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31104512110
domContentLoaded28469940111656
load28870540411656
domInteractive28469940111656

Copy link
Copy Markdown
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!

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [ba51649]
Page Load Metrics (428 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint338947189
domContentLoaded28172642714771
load28272842814771
domInteractive28172642614771

@brad-decker brad-decker merged commit 3ebba0d into develop Nov 19, 2020
@brad-decker brad-decker deleted the fix-9889 branch November 19, 2020 21:44
@github-actions github-actions bot locked and limited conversation to collaborators Nov 19, 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.

QR code scans interpretting payment requests as token addresses

4 participants