show eTLD+1 prominently in request panel #12701

darkdh · 2022-03-22T18:37:18Z

Resolves brave/brave-browser#21787

Request panel from dApp now will show full origin with eTLD+1 emphasized

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Go to https://app.uniswap.org/#/swap and click Connect Wallet
The eTLD+1 part of the URL should be Bold in the Connect with site panel

Go to https://app.uniswap.org/#/swap and choose a token to swap, click Allow spend
The eTLD+1 part of the URL should be Bold in the Allow Spend panel

Go to chainlist.org and connect your wallet, choose a Network to add.
The eTLD+1 part of the URL should be Bold in the Allow Add Network panel

You should see another prompt to switch networks
The eTLD+1 part of the URL should be Bold in the Allow Switch Network panel

Open the Wallet Panel and click on the Connected button to get the Site Permissions panel
The eTLD+1 part of the URL should be Bold in the Site Permissions panel

Go to app.skiff.org and connect your wallet
You will then be prompt to Sign, sign the the transaction
You will then be prompt to provide your public encryption key.
The eTLD+1 part of the URL should be Bold in the Public Encryption Key panel

Go to http://www.reddit.com______________________________________________.________________________._________ethereum.defiapplication.wormholenetwork.com______________________._.______________wwwwwwwwwwwwwwwwwwwwwwwwwwwwwww.xdv.cz.srv21.endora.cz/metapoc/index.html and connect your wallet
The URL should be wrapped and the eTLD+1 part of the URL should be Bold in the Connect with site panel

Open the Wallet Panel and click on the Connected button to get the Site Permissions panel
The URL should be wrapped and the eTLD+1 part of the URL should be Bold in the Site Permissions panel

diracdeltas · 2022-03-22T18:46:50Z

lgtm

brave-builds · 2022-03-23T23:06:39Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-03-25T19:00:27Z

A Storybook has been deployed to preview UI for the latest push

josheleonard

LGTM, other than some useMemo performance gotchas

components/brave_wallet_ui/components/extension/connected-panel/index.tsx

components/brave_wallet_ui/components/shared/create-site-origin/index.tsx

diracdeltas · 2022-03-26T00:02:18Z

components/brave_wallet_ui/components/extension/connected-panel/index.tsx

@@ -122,6 +123,13 @@ const ConnectedPanel = (props: Props) => {

  const onClickViewOnBlockExplorer = useExplorer(selectedNetwork)

+  const showConnectButton = React.useMemo((): boolean => {
+    if (originInfo.origin) {
+      return originInfo.origin.startsWith('chrome://')


can this be an exact check for a set of whitelisted origins? i'm wondering why all of chrome:// needs to be whitelisted.

While addressing this issue brave/brave-browser#20973 it was decided there is no reason to allow a user to give/remove a Site Permission to chrome:// origins.
cc: @bbondy @srirambv

yrliou · 2022-03-26T01:23:04Z

components/brave_wallet/browser/ethereum_permission_utils.cc

@@ -135,6 +136,11 @@ bool GetSubRequestOrigin(const GURL& old_origin,
  return AddAccountToHost(old_origin, account, new_origin);
 }

+std::string eTLDPlusOne(const GURL& url) {


I think this one don't belong to ethereum_permission_utils.cc but belong to something more general, like brave_wallet_utils.cc.

fixed in e926740

brave-builds · 2022-03-26T01:30:39Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-03-26T02:49:45Z

A Storybook has been deployed to preview UI for the latest push

yrliou

Backend overall LGTM, just a few minor comments.

browser/brave_wallet/BUILD.gn

browser/brave_wallet/brave_wallet_service_delegate_impl.cc

brave-builds · 2022-03-28T21:28:40Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2022-03-29T00:44:56Z

A Storybook has been deployed to preview UI for the latest push

components/brave_wallet_ui/common/constants/action_types.ts

josheleonard

LGTM

brave-builds · 2022-03-31T18:30:26Z

A Storybook has been deployed to preview UI for the latest push

darkdh assigned darkdh and Douglashdaniel Mar 22, 2022

github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Mar 22, 2022

darkdh requested a review from yrliou March 23, 2022 22:12

darkdh marked this pull request as ready for review March 23, 2022 22:12

darkdh requested a review from a team as a code owner March 23, 2022 22:12

github-actions bot added rebase and removed rebase labels Mar 23, 2022

brave-builds force-pushed the request-etld-plus-one branch from 0865cbc to 6b272d5 Compare March 23, 2022 22:15

Douglashdaniel force-pushed the request-etld-plus-one branch 2 times, most recently from 54d417d to 562e4e2 Compare March 25, 2022 18:10

josheleonard reviewed Mar 25, 2022

View reviewed changes

diracdeltas reviewed Mar 26, 2022

View reviewed changes

Douglashdaniel force-pushed the request-etld-plus-one branch from 562e4e2 to b6d79d9 Compare March 26, 2022 00:36

Douglashdaniel requested a review from josheleonard March 26, 2022 00:46

yrliou reviewed Mar 26, 2022

View reviewed changes

yrliou approved these changes Mar 26, 2022

View reviewed changes

browser/brave_wallet/BUILD.gn Outdated Show resolved Hide resolved

browser/brave_wallet/brave_wallet_service_delegate_impl.cc Outdated Show resolved Hide resolved

darkdh force-pushed the request-etld-plus-one branch from e926740 to 3e94a70 Compare March 28, 2022 18:40

darkdh added this to the 1.39.x - Nightly milestone Mar 29, 2022

darkdh and others added 3 commits March 30, 2022 11:24

Pass eTLD+1 info to front end when requesting connect

476e69e

Add eTLD+1 to GetActiveOrigin and OnActiveOriginChanged

c5c8087

Update UI to fix URL wrapping and highlight eTDL+1

c9a600e

Douglashdaniel force-pushed the request-etld-plus-one branch from 3e94a70 to 0bba717 Compare March 30, 2022 17:30

josheleonard suggested changes Mar 30, 2022

View reviewed changes

components/brave_wallet_ui/common/constants/action_types.ts Outdated Show resolved Hide resolved

Douglashdaniel force-pushed the request-etld-plus-one branch from 0bba717 to a369c51 Compare March 30, 2022 18:16

Douglashdaniel requested a review from josheleonard March 30, 2022 18:18

josheleonard approved these changes Mar 30, 2022

View reviewed changes

nuo-xu mentioned this pull request Mar 31, 2022

Show eTLD+1 prominently in request panel brave/brave-ios#5188

Closed

Move eTLDPlusOne to brave_wallet_utils

91cf091

Douglashdaniel force-pushed the request-etld-plus-one branch from a369c51 to 91cf091 Compare March 31, 2022 17:40

darkdh merged commit 664bc3f into master Mar 31, 2022

darkdh deleted the request-etld-plus-one branch March 31, 2022 19:55

yrliou mentioned this pull request Apr 1, 2022

Store initiator origin info for DAPP requests brave/brave-browser#21915

Closed

srirambv mentioned this pull request May 12, 2022

[hackerone] show eTLD+1 prominently in wallet brave/brave-browser#21787

Closed

This was referenced May 18, 2022

Address security concern etld dapps #13385

Closed

Address security concern dapps #13398

Merged

srirambv mentioned this pull request Jun 16, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

show eTLD+1 prominently in request panel #12701

show eTLD+1 prominently in request panel #12701

darkdh commented Mar 22, 2022 •

edited by Douglashdaniel

Loading

diracdeltas commented Mar 22, 2022

brave-builds commented Mar 23, 2022

brave-builds commented Mar 25, 2022

josheleonard left a comment

diracdeltas Mar 26, 2022

Douglashdaniel Mar 26, 2022

yrliou Mar 26, 2022

darkdh Mar 26, 2022

brave-builds commented Mar 26, 2022

brave-builds commented Mar 26, 2022

yrliou left a comment

brave-builds commented Mar 28, 2022

brave-builds commented Mar 29, 2022

josheleonard left a comment

brave-builds commented Mar 31, 2022

show eTLD+1 prominently in request panel #12701

show eTLD+1 prominently in request panel #12701

Conversation

darkdh commented Mar 22, 2022 • edited by Douglashdaniel Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

diracdeltas commented Mar 22, 2022

brave-builds commented Mar 23, 2022

brave-builds commented Mar 25, 2022

josheleonard left a comment

Choose a reason for hiding this comment

diracdeltas Mar 26, 2022

Choose a reason for hiding this comment

Douglashdaniel Mar 26, 2022

Choose a reason for hiding this comment

yrliou Mar 26, 2022

Choose a reason for hiding this comment

darkdh Mar 26, 2022

Choose a reason for hiding this comment

brave-builds commented Mar 26, 2022

brave-builds commented Mar 26, 2022

yrliou left a comment

Choose a reason for hiding this comment

brave-builds commented Mar 28, 2022

brave-builds commented Mar 29, 2022

josheleonard left a comment

Choose a reason for hiding this comment

brave-builds commented Mar 31, 2022

darkdh commented Mar 22, 2022 •

edited by Douglashdaniel

Loading