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

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, enhancement)

Description

[[e-mail]]

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.

Comment 1

[[e-mail]]

Attachment: Possible patch

I've based this on the OAuth2 window, although I've removed the tooltipText as that was removed back in Gecko 24.

Comment 2

[Jorg K (CEST = GMT+2)]

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.

Comment 3

[[e-mail]]

Attachment: Updated patch

(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.)

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to [e-mail] 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.

Comment 5

[[e-mail]]

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 6

[Geoff Lankow (:darktrojan)]

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.

Comment 7

[Richard Marti (:Paenglab)]

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.

Comment 8

[[e-mail]]

Attachment: Updated for review comments

Comment 9

[[e-mail]]

Attachment: Updated for review comments

Comment 10

[[e-mail]]

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.

Comment 11

[Richard Marti (:Paenglab)]

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.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: 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.

Comment 13

[[e-mail]]

Attachment: Updated for review comments

... I had no idea that Thunderbird opens so much chrome in content tabs...

Comment 14

[Richard Marti (:Paenglab)]

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.

Comment 15

[Ben Bucksch (:BenB)]

+        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"); ?

Comment 16

[Richard Marti (:Paenglab)]

(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.

Comment 17

[Jorg K (CEST = GMT+2)]

(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.

Comment 18

[Richard Marti (:Paenglab)]

Yes, until now where was no need.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/33b63fc7b450
Introduce URL and security display for content tab pages. r=darktrojan,Paenglab

Comment 20

[Jorg K (CEST = GMT+2)]

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.

Comment 21

[Ben Bucksch (:BenB)]

Filed bug 1499916 for styling.

Comment 22

[[e-mail]]

(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...