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: Browser external application alert on trusted deeplink protocols #6742

Merged
merged 6 commits into from
Jul 24, 2023

Conversation

jpcloureiro
Copy link
Contributor

@jpcloureiro jpcloureiro commented Jul 5, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

With the introduction of Improve deeplinks experience feature, all URI are intercepted & matched with an allowlist.
URIs with protocols on this allowlist are opened by in-app browser with no restriction.

All protocols not present in this allowlist will trigger an alert box that prompts the user to allow or ignore the action.
When the user allows, this URI is forwarded to the OS deep-link handler.

We've seen this alert box might disrupt the user experience when specific deeplink protocols are used, such as wc://... or metamask://...

In order to improve such user experience, we have created a list of trusted deeplink protocols where these URI won't be opened by in-app browser but won't prompt the user to allow or reject, being automatically redirected to the OS deeplink handler.

This changes the old behavior described here

Issue

Progresses # https://github.com/MetaMask/mobile-planning/issues/779

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

allow the OS to deeplink trusted protocols such as `wc:`
and `metamask:`
@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@jpcloureiro
Copy link
Contributor Author

Hey @sethkfman @NicholasEllul would love your thoughts on this approach

@jpcloureiro jpcloureiro self-assigned this Jul 5, 2023
@jpcloureiro jpcloureiro added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 5, 2023
Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

Overall no concerns with this. However, do we have documentation anywhere that highlights the use-cases for each deeplink protocol? Is there a reason these sites deeplink into MetaMask rather than interact with the in-page provider?

@sethkfman
Copy link
Contributor

sethkfman commented Jul 11, 2023

@jpcloureiro The purpose of the alert box is to make sure users are aware that the navigation is coming from an external source.

I think it make sense to add wc:// because the user is actively initiating that connection. The other protocols metamask:// & dapps:// can be triggered outside of a secure session and should be confirmed by the user.

WDYT?

@jpcloureiro jpcloureiro requested a review from a team as a code owner July 13, 2023 15:36
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

@sethkfman sethkfman added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Jul 13, 2023
@jpcloureiro
Copy link
Contributor Author

Overall no concerns with this. However, do we have documentation anywhere that highlights the use-cases for each deeplink protocol? Is there a reason these sites deeplink into MetaMask rather than interact with the in-page provider?

Aside from metamask: URI protocol, they are Ethereum Improvement Proposals

wc: eip-1328
ethereum: eip-831
dapp: ERC-1710

Dapps should always use the in-page provider when available. In the event of dapps not detecting / using the in-page provider, we want to support other forms of communication so we don't leave the dapp user in the dark. (for example, Uniswap does not detect in-page provider, using in-app browser on Android)

@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Jul 19, 2023
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

This is QA passed 👍

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed QA in Progress QA has started on the feature. labels Jul 24, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jul 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

16.7% 16.7% Coverage
0.0% 0.0% Duplication

@cortisiko cortisiko merged commit 7760842 into main Jul 24, 2023
13 checks passed
@cortisiko cortisiko deleted the fix/browser-allowed-protocol-wc branch July 24, 2023 22:19
@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-7.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants