Skip to content

Commit

Permalink
fix: Make QR scanner more strict (#28521)
Browse files Browse the repository at this point in the history
## **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](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28521?quickstart=1)

## **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**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **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**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [x] I’ve included tests if applicable
- We don't yet have any way to test QR scanning. We will follow up later
with tests, and rely on manual testing for now. Later test automation
work tracked in #28528
- [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [x] 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.
  • Loading branch information
Gudahtt authored Nov 18, 2024
1 parent ec8e5fb commit fd78a56
Showing 1 changed file with 13 additions and 3 deletions.
16 changes: 13 additions & 3 deletions ui/components/app/modals/qr-scanner/qr-scanner.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ const READY_STATE = {
READY: 'READY',
};

const ethereumPrefix = 'ethereum:';
// A 0x-prefixed Ethereum address is 42 characters (2 prefix + 40 address)
const addressLength = 42;

const parseContent = (content) => {
let type = 'unknown';
let values = {};
Expand All @@ -31,12 +35,18 @@ const parseContent = (content) => {
// For ex. EIP-681 (https://eips.ethereum.org/EIPS/eip-681)

// Ethereum address links - fox ex. ethereum:0x.....1111
if (content.split('ethereum:').length > 1) {
if (
content.split(ethereumPrefix).length > 1 &&
content.length === ethereumPrefix.length + addressLength
) {
type = 'address';
// uses regex capture groups to match and extract address while ignoring everything else
// uses regex capture groups to match and extract address
values = { address: parseScanContent(content) };
// Regular ethereum addresses - fox ex. 0x.....1111
} else if (content.substring(0, 2).toLowerCase() === '0x') {
} else if (
content.substring(0, 2).toLowerCase() === '0x' &&
content.length === addressLength
) {
type = 'address';
values = { address: content };
}
Expand Down

0 comments on commit fd78a56

Please sign in to comment.