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

[Bug]: Site URL value is undefined for any Approve Transaction triggered by a Deeplink #5570

Closed
seaona opened this issue Jan 23, 2023 · 6 comments
Assignees
Labels
Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead type-bug Something isn't working type-security

Comments

@seaona
Copy link
Contributor

seaona commented Jan 23, 2023

Describe the bug
Site URL value is undefined for any Approve Transaction triggered by a Deeplink

Screenshots
If applicable, add screenshots or links to help explain your problem

image

undefined-site-url-deeplink.mp4

To Reproduce
Steps to reproduce the behavior

  1. Select Mainnet network
  2. Click on MM Scanner (top right button)
  3. Scan the following QR code

image

  1. Accept prompt
  2. Wait until MM opens the Approve Confirmation screen
  3. Click View details
  4. See how Site URL is undefined

Expected behavior

Not sure what should be the appropiate value though. Should we display something like Depplink or similar?

Smartphone (please complete the following information):

  • Device: Samsung SM-A217F
  • OS: Android 11.0
  • App Version: v5.12.3 (1033)

to be added after bug submission by internal support / PM
Severity

  • How critical is the impact of this bug on a user?
  • Add stats if available on % of customers impacted
  • Is this visible to all users?
  • Is this tech debt?
@seaona seaona added the type-bug Something isn't working label Jan 23, 2023
@seaona seaona changed the title [Bug]: Site URL is undefined for any Transaction triggered by a Deeplink [Bug]: Site URL value is undefined for any Approve Transaction triggered by a Deeplink Jan 23, 2023
@seaona seaona added the team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead label Jan 23, 2023
@bschorchit
Copy link

On the confirmation itself, it's showing as metamask.github.io which is also misleading, right? For me it showed up as etherscan so it might be using the last url you loaded a confirmation screen for.

@bschorchit bschorchit added needs-design Feature that requires UI/UX design type-security labels Jan 23, 2023
@bschorchit
Copy link

bschorchit commented Jan 23, 2023

Wondering what the fix might be - cc: @holantonela this will need design for the domain pill (should we just hide it in this case?) and is also security related.

Do we have docs on the deeplink and what parameters are passed on it?

@seaona
Copy link
Contributor Author

seaona commented Jan 24, 2023

@bschorchit from what I've seen, there are 3 different type of deeplinks:

  1. Open a Dapp: we pass the dapp URL
  • Example: https://metamask.app.link/dapp/uniswap.exchange
  1. Payment request:
  • ETH Payment: we pass the recipient address, amount (i.e. 1.5) and network (i.e. 5)
    https://metamask.app.link/send/0x07Be9763a718C0539017E2Ab6fC42853b4aEeb6B@5?value=1.5e18
  • Token Transfer: we pass token contract, token decimals, recipient, amount (i.e. 1) and chainId (i.e. 1)
    https://metamask.app.link/send/0x6B175474E89094C44Da98b954EedeAC495271d0F@1/transfer?address=0x1C53dc20D1E36ed8359250dE626ACAe36BD28a29&uint256=1e18
  1. Payment channel: we pass address, amount and and optional text field (not 100% how this one works though)

You can generate deeplinks here.

@holantonela
Copy link

holantonela commented Jan 24, 2023

Thanks for reporting this @seaona! Since we don't have origin the origin url does not reflect a Dapp or a website, we can hide/remove the entire Site URL line in this kind of request. We also may want to be consistent in the confirmation screen (at 0:12). Could that be possible?

Another option is allowing deeplink generators to use a specific label to expose as an "Requester/Origin name". It may be maliciously used by impersonators, opening another hell thread.

Orthogonal: Are we passing all the transaction parameters clean in that deeplink? Could be obfuscate that data? https://owasp.org/www-community/vulnerabilities/Information_exposure_through_query_strings_in_url

@andrepimenta @ezgi-cengiz

@bschorchit
Copy link

bschorchit commented Jan 24, 2023

Ok, so based on Antonela's comment above (thank you!!), here's the scope for the fix:
For confirmations screens created from deeplink requests,

  1. the origin pill (1st screenshot) should be hidden;
  2. the site url in transaction details (2nd screenshot) should be hidden.

1st screenshot (origin pill):
Screenshot 2023-01-24 at 11 18 08

2nd screenshot (site url):
Screenshot 2023-01-24 at 11 18 23

I'm leaving the obfuscating part out of scope as there might be needed some discussion and input from Andre and Ezgi that doesn't block the above fix.

@bschorchit bschorchit removed the needs-design Feature that requires UI/UX design label Jan 24, 2023
@jpuri jpuri self-assigned this Jan 24, 2023
@bschorchit bschorchit added the Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking label Jan 24, 2023
@jpuri jpuri removed their assignment Jan 25, 2023
@andreahaku
Copy link
Contributor

andreahaku commented Apr 19, 2023

@bschorchit @seaona A simple metamask.app.link universal link does not carry the information about the dapp that triggered it. it just carries the information about the transaction itself. So metamask.app.link is not the URL generating the transaction

@jpuri jpuri assigned jpuri and blackdevelopa and unassigned jpuri Apr 19, 2023
@seaona seaona closed this as completed May 16, 2023
blackdevelopa added a commit that referenced this issue Nov 6, 2023
## **Description**

<!--
Write a short description of the changes included in this pull request,
also include relevant motivation and context. Have in mind the following
questions:
1. What is the reason for the change?
2. What is the improvement/solution?
-->

When the transaction origin is `qr-code`, we shouldn't show the origin
pill. See ref
[5570](#5570 (comment))
## **Related issues**

Fixes: #7545 
Bitrise:
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/07a2a3fd-2fff-4242-a754-e8f45d455cb4

## **Manual testing steps**
1. Select Mainnet
2. Trigger a Deeplink
3. See domain pill appears with the selected address

![Screenshot from 2023-10-19
14-50-01](https://github.com/MetaMask/metamask-mobile/assets/54408225/04fc28b2-40df-4daf-84b6-cd193011a373)

## **Screenshots/Recordings**

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

### **Before**

![Screenshot from 2023-10-19
14-14-16](https://github.com/MetaMask/metamask-mobile/assets/54408225/05ded1e2-c0ec-4f2d-88d7-d3cc3551d401)

### **After**


![IMG_9310](https://github.com/MetaMask/metamask-mobile/assets/29962968/b4a834d4-3f99-4a50-89ae-aacd627a674b)


## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [x] I’ve included tests if applicable
- [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-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [x] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [x] 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sev2-normal An issue that may lead to users misunderstanding some limited risks they are taking team-confirmations-secure-ux DEPRECATED: please use "team-confirmations" label instead type-bug Something isn't working type-security
Projects
None yet
Development

No branches or pull requests

6 participants