[tbb-bugs] #23247 [Applications/Tor Browser]: Communicating security expectations for .onion: what to say about different padlock states for .onion services

[Tor Bug Tracker & Wiki]

#23247: Communicating security expectations for .onion: what to say about different
padlock states for .onion services
-------------------------------------------------+-------------------------
 Reporter:  isabela                              |          Owner:
                                                 |  pospeselr
     Type:  project                              |         Status:
                                                 |  needs_revision
 Priority:  High                                 |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  ux-team, tor-hs,                     |  Actual Points:
  TorBrowserTeam201805                           |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by gk):

 * keywords:  ux-team, tor-hs, TorBrowserTeam201805R => ux-team, tor-hs,
     TorBrowserTeam201805
 * status:  needs_review => needs_revision


Comment:

 Overall, looks good to me! Some minor things:

 1) I had trouble parsing your commit message (there are some typos and
 things in the "-" lines missing etc.). https://chris.beams.io/posts/git-
 commit/ might be a useful resource. At least it helped me a lot.

 2) There is code in `browser.js` regarding the PointerLock API that might
 require patching as well (IIRC this or similar code is used or was used to
 show the same info when entering into fullscreen, at least on some
 platforms):
 {{{
  get pointerlockFsWarningClassName() {
     // Note that the fullscreen warning does not handle
 _isSecureInternalUI.
     if (this._uriHasHost && this._isEV) {
       return "verifiedIdentity";
     }
     if (this._uriHasHost && this._isSecure) {
       return "verifiedDomain";
     }
     return "unknownIdentity";
 }}}

 3) Could you explain why you needed to add `this._sslStatus != null`  in
 {{{
 } else if (this._uriHasHost && this._isSecure && this._sslStatus != null)
 {
 }}}
 If that's not obvious I think adding a comment would be useful.

 4) The `const bool` does not really fit into the surrounding code (where
 just `bool` is used). I think we should stick to the latter. Or maybe even
 better just get rid of this variable and do things like
 {{{
 if (!StringEndsWith(host, NS_LITERAL_CSTRING(".onion")))
 }}}
 in both places. (In the `parentIsOnion` case, maybe rename `host` to
 `parentHost` then as this makes the context clearer.

 5) I like the strings added to the Page Info window. However, doing it
 that way we have trouble translating them until we got our code upstreamed
 and Mozilla takes care of the translation. This has the unfortunate side-
 effect of mixed english $LANG text.

 We had a similar issue in #20244 (see in particular
 comment:2:ticket:20244) and worked around it with a Torbutton overlay. I
 wonder if that could be a solution for this case as well.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23247#comment:51>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online