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

DApps: Address the security concern to show eTLD+1 of max 255 characters (site+domain) and show site origin in Panels #21798

Closed
Pavneet-Sing opened this issue Mar 21, 2022 · 5 comments · Fixed by brave/brave-core#13398

Comments

@Pavneet-Sing
Copy link

Pavneet-Sing commented Mar 21, 2022

Description

Steps to reproduce

  1. Create a DApp wallet connectivity request with lengthy eTLD+1
  2. A eTLD+1 with 255 chars (max) is not bold and origin is not displayed

Actual result

A eTLD+1 is not bold in DApps permission panel and origin is not displayed

Expected result

Issue reproduces how often

Not yet merged in master

Version/Channel Information:

  • Can you reproduce this issue with the current Play Store version? NA
  • Can you reproduce this issue with the current Play Store Beta version? NA
  • Can you reproduce this issue with the current Play Store Nightly version? NA

Additional information

Also add/replace the root view with ScrollView for lengthy values.

cc: @diracdeltas

@Pavneet-Sing Pavneet-Sing added the OS/Android Fixes related to Android browser functionality label Mar 21, 2022
@diracdeltas
Copy link
Member

@Pavneet-Sing actually the maximium allowed length for a label in DNS is 63 characters. so eTLD+1 won't be much more than ~70 characters total.

we should always make sure to show the eTLD+1 clearly so that it's visible without any scrolling.

for subdomains, which can be a lot longer, scrolling makes sense to me

cc @darkdh @nuo-xu

@diracdeltas
Copy link
Member

and actually the total length of the domain should be <255 characters according to the DNS spec; however i'm not sure if there are workarounds. https://serverfault.com/questions/580249/is-there-a-maximum-subdomain-depth

@Pavneet-Sing
Copy link
Author

Pavneet-Sing commented Mar 21, 2022

Got it, then length of 255 character will not be a concern on Android without scroll. Possible UI adaption could be is to show a warning or something, when the length exceeds then we can make the whole screen scroll able and keep the whole domain and subdomain sections visible (instead of defining a specific height of the domain and subdomain section).

@Pavneet-Sing Pavneet-Sing changed the title DApps: Fix the security concern to show arbitrary length of eTLD+1 (site+domain) in brave_permission_dialog layout DApps: Address the security concern to show eTLD+1 of max 255 characters (site+domain) in brave_permission_dialog layout Mar 22, 2022
@Pavneet-Sing Pavneet-Sing changed the title DApps: Address the security concern to show eTLD+1 of max 255 characters (site+domain) in brave_permission_dialog layout DApps: Address the security concern to show eTLD+1 of max 255 characters (site+domain) and show site origin in Panels Apr 14, 2022
@qamarngr qamarngr self-assigned this May 12, 2022
@Pavneet-Sing Pavneet-Sing added the feature/web3/wallet Integrating Ethereum+ wallet support label May 19, 2022
@kjozwiak kjozwiak added this to the 1.40.x - Beta milestone May 28, 2022
@kjozwiak
Copy link
Member

The above requires 1.40.78 or higher for 1.40.x verification 👍

@srirambv
Copy link
Contributor

Verification passed on Oppo Reno 5 with Android 12 running 1.40.101

Connect Account Approve Tx Approve Network Switch Network Sign Message Encryption message
image image image image image image

Verification passed on Samsung Tab A with Android 10 running 1.40.101

Connect Account Approve Network Switch Network Connected State Sign Message Encryption message
image image image image image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
6 participants