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

Permissions: Do not display HTTP/HTTPS URL schemes for unique hosts #8768

Merged
merged 4 commits into from
Jun 9, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Jun 8, 2020

Terminology

URL components recap:

  • origin: https://metamask.io:9000
  • scheme or protocol: https
  • port: 9000
  • host: metamask.io:9000
    • Note that in our URLs, there is usually no port displayed, as almost all HTTP/HTTPS websites use default ports, and the Chrome APIs don't even bother giving the default ports to us.
    • Therefore, if the port is there at all, it's some non-default value, and I think we should display it.
  • hostname: metamask.io

Changes

These changes required updates to permissions controller and selector tests. The majority of the changes (by lines) are due to changing a commonly used constant in the permissions controller tests.

Updates the permission origin string display to conform to the following:

  • Show the origin on the connect and confirmation screens
    • We were already doing this.
  • Show the host on the 'Connected accounts' modal title
    • Originally, it was suggested that we only show the hostname here. However, I decided to follow the conventions established by Chrome, and display the host. See also notes above on when the port is displayed.
  • Show non-HTTP/HTTPS schemes on the connected sites list, but hide HTTP/HTTPS (except for the purpose of disambiguation, e.g. we'll show the scheme if the same host is connected via both HTTP and HTTPS)
    • We do this if the same host, not hostname, is connected to any set of accounts, under different schemes.
    • So, http://foo.bar can be connected to Account 1, and https://foo.bar can be connected to Account 2, and the schemes will be displayed for both sites, even if they'll never appear in the same connected sites list.
    • https://foo.bar:9000 and https://foo.bar:9001 will be displayed as foo.bar:9000 and foo.bar:9001, respectively.

These changes do not affect external domains that are extension (i.e., their domain metadata has a truthy extensionId property).

@rekmarks rekmarks requested a review from a team as a code owner June 8, 2020 22:57
@rekmarks rekmarks changed the title Strip scheme from domain URL origins for unique hosts Permissions: Do not display HTTP/HTTPS URL schemes for unique hosts Jun 8, 2020
@rekmarks rekmarks force-pushed the perm-origin-scheme-display branch from 5afbed1 to 72e3533 Compare June 8, 2020 23:00
@metamaskbot
Copy link
Collaborator

Builds ready [72e3533]
Page Load Metrics (626 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint319243178
domContentLoaded34581062514469
load34681262614469
domInteractive34481062414469

rekmarks and others added 2 commits June 9, 2020 13:30
…t.component.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@rekmarks rekmarks requested a review from Gudahtt June 9, 2020 20:32
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [3559a53]
Page Load Metrics (618 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3088482010
domContentLoaded37486561714971
load37686661814971
domInteractive37386461614971

@rekmarks rekmarks merged commit a84eedb into develop Jun 9, 2020
@rekmarks rekmarks deleted the perm-origin-scheme-display branch June 9, 2020 20:56
Gudahtt added a commit that referenced this pull request Jun 10, 2020
* origin/develop: (35 commits)
  Delete unused InfuraController & tests (#8773)
  Permissions: Do not display HTTP/HTTPS URL schemes for unique hosts (#8768)
  Refactor confirm approve page (#8757)
  blocklisted -> blocked
  Update app/scripts/contentscript.js
  blacklist -> blocklist; whitelist -> safelist
  replace blacklist with blocklist
  Delete unused transaction history test state (#8769)
  fix-formatting-of-gif (#8767)
  Order accounts on connect page (#8762)
  add gif for loading dev build (#8766)
  Bump websocket-extensions from 0.1.3 to 0.1.4 (#8759)
  Fix prop type mismatch (#8754)
  use grid template to position list item (#8753)
  Fix account menu entry for imported accounts (#8747)
  Fix permissions connect close and redirect behavior (#8751)
  Refactor `TokenBalance` component (#8752)
  Fix 'Remove account' in Account Options menu (#8748)
  move activation logic into token rates controller (#8744)
  asset outdated warning inline on full screen (#8734)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants