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: Make QR scanner more strict #28521

Merged
merged 1 commit into from
Nov 18, 2024
Merged

fix: Make QR scanner more strict #28521

merged 1 commit into from
Nov 18, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Nov 18, 2024

Description

The QR scanner is now more strict about the contents it allows to be scanned. If the scanned QR code deviates at all from the supported formats, it will return "unknown" as the result (as it always has for completely unrecognized QR codes).

Previously we would accept QR codes with a recognized prefix even if the complete contents did not match our expectations, which has resulted in unexpected behavior.

Open in GitHub Codespaces

Related issues

Fixes #28527

Manual testing steps

  • Open the MetaMask extension and select 'Send'
  • Click on the QR scanner icon in the "Send To" field and enable webcam
  • Scan a ERC-20 wallet receive QR from a mobile app, which follows the EIP-681 standard and contains a valid token contract and account address
  • ERC-20 Token Contract Address, which is the first address in the string, populates the "Send To" field instead of the intended recipient address

Screenshots/Recordings

Before

We didn't record this, but multiple people on the team reproduced the problem.

After

https://www.loom.com/share/be8822e872a14ec98a47547cf6198603

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@metamaskbot
Copy link
Collaborator

Builds ready [84f9979]
Page Load Metrics (2137 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint19442302213410450
domContentLoaded1934227120989144
load19432293213710450
domInteractive29218624019
backgroundConnect10102332914
firstReactRender612931025325
getState562091023416
initialActions01000
loadScripts1414175215748742
setupStore65313136
uiStartup22592832248816378
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 34 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt marked this pull request as ready for review November 18, 2024 19:43
@Gudahtt Gudahtt requested a review from a team as a code owner November 18, 2024 19:43
@danjm
Copy link
Contributor

danjm commented Nov 18, 2024

I manually tested on v12.6.1 and repro'd the bug, and then tested on this branch and saw the same as the above video. I also reviewed the code

The QR scanner is now more strict about the contents it allows to be
scanned. If the scanned QR code deviates at all from the supported
formats, it will retur "unknown" as the result (as it always has for
completely unrecognized QR codes).

Previously we would accept QR codes with a recognized prefix even if
the complete contents did not match our expectations, which has
resulted in unexpected behavior.
@Gudahtt Gudahtt added this pull request to the merge queue Nov 18, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [adc5f3c]
Page Load Metrics (2150 ± 60 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint66523652066343165
domContentLoaded18822331211011756
load19212378215012560
domInteractive31214523919
backgroundConnect10119393014
firstReactRender632931125024
getState26150713115
initialActions00000
loadScripts1398176815889747
setupStore65914157
uiStartup217030722485208100
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 34 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Merged via the queue into develop with commit fd78a56 Nov 18, 2024
77 checks passed
@Gudahtt Gudahtt deleted the strict-qr-scanner branch November 18, 2024 21:54
@github-actions github-actions bot locked and limited conversation to collaborators Nov 18, 2024
@metamaskbot metamaskbot added the release-12.9.0 Issue or pull request that will be included in release 12.9.0 label Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.9.0 Issue or pull request that will be included in release 12.9.0 team-wallet-framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: QR scanner can set recipient to wrong address during Send flow
4 participants