Closed Bug 1204616 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 43
Iteration:
43.3 - Sep 21
Tracking Status
firefox43 --- fixed

People

(Reporter: javaun, Assigned: rbarnes)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(2 files, 2 obsolete files)

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).
Whiteboard: [fxprivacy][triage]
Attached patch bug-1204616.0.patch (obsolete) — Splinter Review
Assignee: nobody → rlb
Status: NEW → ASSIGNED
Attachment #8660901 - Flags: review?(MattN+bmo)
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 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
Attachment #8660901 - Flags: review?(MattN+bmo) → review-
You should also get Android aligned with this since they have copied the same code in mobile/android/chrome/content/browser.js
(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.
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.
Attached patch bug-1204616.1.patch (obsolete) — Splinter Review
* Makes the corresponding change in Android
* Changes to crop=start to better handle long names
Attachment #8660901 - Attachment is obsolete: true
Attachment #8661247 - Flags: review?(MattN+bmo)
Attachment #8661247 - Flags: feedback?(jmoradi)
Attached image before-after.1.png
Iteration: --- → 43.3 - Sep 21
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Reverts @crop=start for EV (which should still crop at the end)
Attachment #8661247 - Attachment is obsolete: true
Attachment #8661247 - Flags: review?(MattN+bmo)
Attachment #8661247 - Flags: feedback?(jmoradi)
Attachment #8661326 - Flags: review?(MattN+bmo)
Does this change the control center, the notifications or both?  If just control center, we should make a followup but for notifications.
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.
Attachment #8661326 - Flags: review?(liuche)
Attachment #8661326 - Flags: review?(MattN+bmo)
Attachment #8661326 - Flags: review+
(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.
Summary: Control Panel should show full host name (w/ subdomains) in security block → Control Center should show full host name (w/ subdomains) in security block
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 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.
Attachment #8661326 - Flags: review?(liuche) → review+
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?
Attachment #8661251 - Flags: feedback?(alam)
(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.
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!
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
Flags: qe-verify? → qe-verify-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef5b842c975a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8661251 [details]
before-after.1.png

Sure, let's give that a try. Seems reasonable to me.
Attachment #8661251 - Flags: feedback?(alam) → feedback+
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
QA Whiteboard: [bugday-20151209]
Blocks: 1400544
You need to log in before you can comment on or make changes to this bug.