1204616 - Control Center should show full host name (w/ subdomains) in security block

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[Javaun Moradi [:javaun]]

Noticed by Richard Barnes, the current Control Center security block carried over the exact logic from the prior doorhanger where it only shows the 2nd level domain name.

For example, the sites:

* mail.google.com
* drive.google.com

simply show as: "google.com" in the security block. 

This is true for encrypted and unencrypted sites. 

Example 1: http://www.nytimes.com shows "nytimes.com" in the security block. 

Example 2: http://nytimes.tumblr.com/ shows "tumblr.com" in the security block.

Richard is working on a patch. We'll need to discuss w/ team re: UX impact and any other issues.

Requirements:
* Full hostname w/ subdomains (i.e. mail.google.com) should display in ID block
* "www" should show for sites that use it, since that's a subdomain
* No changes needed for sites with EV certs. We currently handle this correctly by showing the company name on the cert (i.e. Mozilla Foundation).

Comment 1

[Richard Barnes [:rbarnes]]

Attachment: bug-1204616.0.patch

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8660901 [details] [diff] [review]
bug-1204616.0.patch

Review of attachment 8660901 [details] [diff] [review]:
-----------------------------------------------------------------

Did you test what happens with really long domains? IIRC, part of the reason we don't show the whole domain is so an attacker can't spoof another site due to truncation. e.g. with myawsomeapp.facebook.com.evil.com we don't want to ever truncate to myawsomeapp.facebook.com[...].

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8660901 [details] [diff] [review]
bug-1204616.0.patch

Review of attachment 8660901 [details] [diff] [review]:
-----------------------------------------------------------------

You also need to update tests e.g. browser/base/content/test/general/browser_identity_UI.js

Comment 4

[Matthew N. [:MattN]]

You should also get Android aligned with this since they have copied the same code in mobile/android/chrome/content/browser.js

Comment 5

[Javaun Moradi [:javaun]]

(In reply to Matthew N. [:MattN] (behind on mail) from comment #2)
> Did you test what happens with really long domains? IIRC, part of the reason
> we don't show the whole domain is so an attacker can't spoof another site
> due to truncation. e.g. with myawsomeapp.facebook.com.evil.com we don't want
> to ever truncate to myawsomeapp.facebook.com[...].

We did have this conversation in one of our first iterations, possibly during 41 nightly. 

We would still show the URL in the URL bar, and one use case is a user is clicking on the CC panel because something already seems phishy. 

We probably need to rethink how to handle longer domains in general, because even without subdomains we already truncate long domain names like this one: 
http://www.thelongestdomainnameintheworldandthensomeandthensomemoreandmore.com/

Perhaps instead of truncating with elipses (...) we could do something like reduce the text and show it all. I don't know if we have a wrapping limitation.

Comment 6

[Matthew N. [:MattN]]

I found the original rationale for eTLD+1 in the popup:

(Quoting Johnathan Nightingale [:johnath] from bug 406612 comment #58)
> eTLD+1 in the popup was done as a way to keep the information in the popup
> maximally useful, to mitigate subdomain attacks
> (paypal.com.paypal.com.evil.com), and to keep the presentation simple and
> comprehensible, even if it is at the cost of strict accuracy.
==

I agree there are some things we should improve with long domain names but for this bug I just care about not introducing regressions. I think some easy mitigations we could do for this bug would be:
* wrap instead of truncating (we should be careful about hyphens in domains affecting wrapping and therefore spoofing)
* truncate from the left (ideally with different styling on the eTLD+1 like we do with the address bar)

I think dynamically reducing the font size for long strings would be nice in the long term but I don't think we have that ability easily in the platform so it would require some non-trivial work though probably possibly now.

Comment 7

[Richard Barnes [:rbarnes]]

Attachment: bug-1204616.1.patch

* Makes the corresponding change in Android
* Changes to crop=start to better handle long names

Comment 8

[Richard Barnes [:rbarnes]]

Attachment: before-after.1.png

Comment 9

[Richard Barnes [:rbarnes]]

Attachment: bug-1204616.2.patch

Reverts @crop=start for EV (which should still crop at the end)

Comment 10

[Tanvi Vyas[:tanvi]]

Does this change the control center, the notifications or both?  If just control center, we should make a followup but for notifications.

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8661326 [details] [diff] [review]
bug-1204616.2.patch

Review of attachment 8661326 [details] [diff] [review]:
-----------------------------------------------------------------

Flagging :liuche for Android review. I'm not sure if there are any Android tests for that.

We discussed this in our FxPrivacy meeting and it seemed like people were fine with this incremental improvement.

Comment 12

[Matthew N. [:MattN]]

(In reply to Tanvi Vyas [:tanvi] from comment #10)
> Does this change the control center, the notifications or both?  If just
> control center, we should make a followup but for notifications.

This is only about Control Center. Notifications already do the whole domain (at least on the one I tested) use a smaller font-size and crop in the center (since they also need to handle file paths.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8661326 [details] [diff] [review]
bug-1204616.2.patch

Review of attachment 8661326 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +7211,1 @@
>      this._identityPopupContentHost.setAttribute("value", host);

Could you also set the "tooltiptext" to `host` so that the user can see the whole origin if they choose. This was something Bryan Bell mentioned in our standup.

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8661326 [details] [diff] [review]
bug-1204616.2.patch

Review of attachment 8661326 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine to me. I don't think this would affect Android however, since we have our own browser.js.

I will flag antlam, so he can keep this on his radar, and make changes to our Android design.

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8661251 [details]
before-after.1.png

Anthony, Desktop is switching to include the full domain name, as opposed to just the base domain. This will help with some domains like drive.google.com, where the subdomain is pretty relevant.

Should we file a bug for Android to match this?

Comment 16

[Matthew N. [:MattN]]

(In reply to Chenxia Liu [:liuche] from comment #14)
> This seems fine to me. I don't think this would affect Android however,
> since we have our own browser.js.

This patch is also changing it since the code was copied.

Comment 17

[Chenxia Liu [:liuche]]

Derp, for some reason I only looked at the filepath for the first browser.js change.

That's fine - we should match between desktop and Android. We don't have any tests that would break for this, so this patch is good. Thanks, Matt!

Comment 18

[Matthew N. [:MattN]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99df5b856dc5 (just pushed so please check results)

Patch to push (with r=…): https://hg.mozilla.org/try/rev/294f5ae16549

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ef5b842c975a

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ef5b842c975a

Comment 21

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8661251 [details]
before-after.1.png

Sure, let's give that a try. Seems reasonable to me.

Comment 22

[Ashickur Rahman]

Successfully reproduce the bug on Mac OS X; Firefox 42.0; Build ID : 20151029151421; User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0 . 


The fix works for me on Mac OS X; Firefox 43.0b9 ; Build ID : 20151203163240; User Agent : Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:43.0) Gecko/20100101 Firefox/43.0