Need to see URL or domain of content tab pages, to know the trust anchor. Can't tell how secure content tab pages are.

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: neil, Assigned: neil)

Tracking

unspecified
Thunderbird 64.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

If an extension (now typically a WebExtension) opens a content tab, there's no indication of the security of the page being visited. This is particularly important if the page requires authentication of some sort.
Posted patch Possible patch (obsolete) β€” β€” Splinter Review
I've based this on the OAuth2 window, although I've removed the tooltipText as that was removed back in Gecko 24.
Attachment #9015545 - Flags: review?(jorgk)
Comment on attachment 9015545 [details] [diff] [review]
Possible patch

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

When have you last rebased ;-) ? onSecurityChange() has five parameters now. Please make the patch apply, remove the trailing spaces and the get review from Geoff (:darktrojan).

::: mail/themes/shared/mail/tabmail.css
@@ +384,5 @@
> +
> +.contentTabSecurity[loading="true"] {
> +  background-image: url("chrome://global/skin/icons/loading.png");
> +  background-position: 4px 2px;
> +}  

Nit: spaces.
Attachment #9015545 - Flags: review?(jorgk)
Posted patch Updated patch β€” β€” Splinter Review
(In reply to Jorg K from comment #2)
> When have you last rebased ;-) ?
Just now when I pushed to try ;-)

(I don't know whether I've used the most appropriate try options though.)
Attachment #9015545 - Attachment is obsolete: true
Attachment #9015941 - Flags: review?(geoff)
(In reply to neil@parkwaycc.co.uk from comment #3)
> (I don't know whether I've used the most appropriate try options though.)
Buildbot is dead, so lose the --buildbot. You can do -u xpcshell, -u mozmill or -u all.

-b do -p macosx64,linux64 is usually enough, unless you want to add win64.
New Try push https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f0723632732a497e6df188228f029bd82bab00e8 seems to have completed without major incident; I got the test-content-tab.js errors in other unrelated Try runs so I'm thinking they're intermittent?
Comment on attachment 9015941 [details] [diff] [review]
Updated patch

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

I don't like this. I can see why it's wanted, and it works, so I've given it r+ with comments, but it's ugly. You copied it from somewhere that is also ugly, so I'm not blaming you. Perhaps Richard would like to have a look at both. (Sorry, all four – why do we have three copies already?)

Also I can't help the feeling that TB is turning into a web browser with email capability. Will the next bug be somebody complaining they can't enter a URL in the textbox you just added?

::: mail/base/content/specialTabs.js
@@ +164,5 @@
> +                              wpl.STATE_SECURE_MED |
> +                              wpl.STATE_SECURE_LOW;
> +    switch (aState & wpl_security_bits) {
> +      case wpl.STATE_IS_SECURE | wpl.STATE_SECURE_HIGH:
> +        level = "high";

level is not defined.

::: mail/themes/shared/mail/tabmail.css
@@ +357,5 @@
>  }
> +
> +.contentTabHeader {
> +  overflow: hidden;
> +  border-bottom: 1px solid black;

This should be the same colour as the other borders, e.g. below toolbars. I think it also should line up with the Today Pane header beside it. It looks like we set the height of that explicitly, so that should be easy enough.
Attachment #9015941 - Flags: review?(geoff) → review+
Comment on attachment 9015941 [details] [diff] [review]
Updated patch

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

Is somewhere a extension I can try this?

::: mail/themes/shared/mail/tabmail.css
@@ +357,5 @@
>  }
> +
> +.contentTabHeader {
> +  overflow: hidden;
> +  border-bottom: 1px solid black;

Yes, shouldn't be needed as the bottom border of the toolbox is defined here: https://searchfox.org/comm-central/search?q=.contentTabToolbox%3A%3Aafter&case=false&regexp=false&path=

@@ +363,5 @@
> +
> +.contentTabAddress {
> +  -moz-appearance: textfield;
> +  overflow: hidden;
> +  margin: 0px 5px;

Please, only 0 because 0 has no unit.

@@ +368,5 @@
> +}
> +
> +.contentTabSecurity {
> +  width: 20px;
> +  padding-right: 5px;

Please use padding-inline-end to be RTL safe.
Posted patch Updated for review comments (obsolete) β€” β€” Splinter Review
Posted patch Updated for review comments (obsolete) β€” β€” Splinter Review
Attachment #9016666 - Attachment is obsolete: true
Comment on attachment 9016668 [details] [diff] [review]
Updated for review comments

(In reply to Richard Marti from comment #7)
> Is somewhere a extension I can try this?
You don't need an extension, just something that opens a content tab (e.g. evaluate openContentTab("https://www.thunderbird.net/"); in the Error Console).

> shouldn't be needed as the bottom border of the toolbox is defined
It's not a toolbox. Although there is a toolbox, it's explicitly hidden.
Attachment #9016668 - Flags: review?(richard.marti)
Comment on attachment 9016668 [details] [diff] [review]
Updated for review comments

Screenshot in the next comment.

I'm not happy with this patch. It adds on every content tab this box like the Add-on Manager the Troubleshooting Infos etc. No special content tab opening needed, and this is what I don't like. This box should only be shown when from a WebExtension opened and show then the address like FX does with showing the WX URL in the URL bar.

Then the new box looks weird because it doesn't blend with the tabs and the textbox uses the whole height and this makes that the left and right textbox border look a bit abandoned. Also is the text isn't aligned. A vertically centred text would look much better, also the textbox could only be around 20px tall.

Why don't you use the existing toolbox which gets the correct colours and styling automatically instead of a new toolbar imitating box? Then you could add in your code a display: -moz-box; on the .contentTabToolbox. Like this we show the address with its security info only where we want it.
Attachment #9016668 - Flags: review?(richard.marti) → review-
Posted image screenshot.png β€”
Screenshot on Windows.
- The box is shown also on internal content pages.
- The box colour doesn't fit to the tab colour.
- The textbox is almost not visible as textbox. The textbox could only have the half width, left aligned.
- The "about:support" text isn't vertically centred.
... I had no idea that Thunderbird opens so much chrome in content tabs...
Attachment #9016668 - Attachment is obsolete: true
Attachment #9017549 - Flags: review?(richard.marti)
Comment on attachment 9017549 [details] [diff] [review]
Updated for review comments

Thanks, it's now a lot better. Needs some work for LW- and WX-themes, but I'll do this in a new bug.
Attachment #9017549 - Flags: review?(richard.marti) → review+
+        this.mTab.urlbar.textContent = location;
+        this.mTab.root.removeAttribute("collapsed");
+      } else {
+        this.mTab.root.setAttribute("collapsed", "false");

This looks strange. Did you mean setAttribute("collapsed", "true"); ?
(In reply to Ben Bucksch (:BenB) from comment #15)
> +        this.mTab.urlbar.textContent = location;
> +        this.mTab.root.removeAttribute("collapsed");
> +      } else {
> +        this.mTab.root.setAttribute("collapsed", "false");
> 
> This looks strange. Did you mean setAttribute("collapsed", "true"); ?

It's because of this:
https://searchfox.org/comm-central/source/mail/themes/shared/mail/tabmail.css#348-349
When it will be changed to setAttribute("collapsed", "true"); , then the CSS rule needs also be adapted. Then it looks more logical, yes.
Status: NEW → ASSIGNED
Keywords: push-needed
Summary: Can't tell how secure content tab pages are → Need to see URL or domain of content tab pages, to know the trust anchor. Can't tell how secure content tab pages are.
Severity: normal → enhancement
(In reply to Richard Marti (:Paenglab) from comment #14)
> Thanks, it's now a lot better. Needs some work for LW- and WX-themes, but
> I'll do this in a new bug.
Indeed, I had a LW theme loaded for the review of another bug and I got and empty box on a http: page, https: had a green icon. I'll guess we'll take it for now, but it needs a follow-up.
Yes, until now where was no need.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/33b63fc7b450
Introduce URL and security display for content tab pages. r=darktrojan,Paenglab
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
I had to make up a commit message. Richard, please file a follow-up since displaying an empty box is not ready for prime-time.
Target Milestone: --- → Thunderbird 64.0
Blocks: 1499916
Filed bug 1499916 for styling.
(In reply to Jorg K from comment #20)
> I had to make up a commit message.
Oops, sorry, all the previous versions had had full headers on...

> Richard, please file a follow-up since
> displaying an empty box is not ready for prime-time.
Ugh, if only you'd found a better solution back in bug 1230250 comment 6...
You need to log in before you can comment on or make changes to this bug.