1587132 - Add-on homepage link is not opened in browser

Status: VERIFIED FIXED

(Thunderbird :: Add-Ons: General, defect)

Description

[Andrei Hajdukewycz [:sancus]]

See also: Bug 1042280 and Bug 1204317.

Steps to reproduce:

1. Install any add-on with an external homepage.
2. Go to the add-on listed in your add-on manager and click on the homepage link.
3. Note that the link opens in Thunderbird, when it should open in the user's default browser.

Opening arbitrary links not on a Mozilla/Thunderbird domain inside of Thunderbird is incorrect behaviour, in my opinion.

Comment 1

[Andrei Hajdukewycz [:sancus]]

Attachment: screenshot of link

Added a screenshot of the specific link I'm referring to.

Comment 2

[Jorg K (CEST = GMT+2)]

Khushil is the man to fix any link issues ;-)

Comment 3

[Ralf Jung]

Toggling network.protocol-handler.expose.http and network.protocol-handler.expose.https to false fixes the bug for be (Thunderbird 60.9.0). However, I think I used to have those set to false earlier already and reverted for some reason, but forgot why.

Also, I think the default behavior is still a bug that should be fixed (toggling the default of these flags might or might not be a reasonable solution).

Comment 4

[Khushil Mistry [:khushil324]]

Attachment: Bug-1587132_add-on-homepage-link-fix.patch

It's kind of a hack right now. This page is coming from Firefox, so we can only make changes through aboutAddonExtra.js.
This was the default behavior in TB 60. In Firefox 69 onward, they have converted this label into <a>(during de-grid effort of Firefox). Thus in TB 69 onward, this link is opening up in a browser window.
It's debatable that we want to keep this behavior in TB 68 as this was also the same in TB 60 or use this kind of hack to make this work.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9099987 [details] [diff] [review]
Bug-1587132_add-on-homepage-link-fix.patch

Well, Andrei can try it. I'm not signing off on JS hacks, only C++ ones ;-) - I'll let Magnus make the call here.

Comment 6

[Khushil Mistry [:khushil324]]

Attachment: Bug-1587132_add-on-homepage-link-fix.patch

Comment 7

[Andrei Hajdukewycz [:sancus]]

(In reply to Khushil Mistry [:khushil324] from comment #4)

It's kind of a hack right now. This page is coming from Firefox, so we can only make changes through aboutAddonExtra.js.
This was the default behavior in TB 60. In Firefox 69 onward, they have converted this label into <a>(during de-grid effort of Firefox). Thus in TB 69 onward, this link is opening up in a browser window.
It's debatable that we want to keep this behavior in TB 68 as this was also the same in TB 60 or use this kind of hack to make this work.

I think the behaviour in TB60 is also a bug. Someone can correct me if I'm wrong, but the rule is that we should always open links to external websites in the user's browser. For example, links in any email.

The only exception to this is when we are using pages on domains we explicitly control as "part of the client" like with the Start Page and the Add-ons Manager. This is really just remote hosting content we own, we have full control over it, so it is known safe.

In general, any case where an arbitrary user or developer-entered url opens in Thunderbird is a bug, because Thunderbird isn't intended to be used as a browser by end users, it's not 100% safe. We have made other decisions like Bug 1549562 with the assumption that we do not expose any attack surface to external websites.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9100022 [details] [diff] [review]
Bug-1587132_add-on-homepage-link-fix.patch

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

Why not just add the event listeners to the exiting element?

Comment 9

[Khushil Mistry [:khushil324]]

I tried that but text-link component has a click event listener on the bubbling phase. So If I add the event listener to the component, event listener from the CE itself will take precedence so the link will be opened up in TB and browser both, event.stopPropagation() will have no significance. Also, we can't remove the event listener from the aboutAddonExtra.js.

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9100022 [details] [diff] [review]
Bug-1587132_add-on-homepage-link-fix.patch

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

I guess this is alright. Would add a code comment explaining that we hide the original and add another link to make sure we open in the browser instead of internally.

Comment 11

[Jorg K (CEST = GMT+2)]

I might add the comment myself unless Khushil beats me to it.

Comment 12

[Khushil Mistry [:khushil324]]

I was just on it :)

Comment 13

[Khushil Mistry [:khushil324]]

Attachment: Bug-1587132_add-on-homepage-link-fix.patch

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 9100749 [details] [diff] [review]
Bug-1587132_add-on-homepage-link-fix.patch

I guess you want this backported.

Comment 15

[Jorg K (CEST = GMT+2)]

Umm, this patch doesn't apply to trunk. This is only for 68 ESR? I tried on trunk and link already opens in the external browser.

Comment 16

[Khushil Mistry [:khushil324]]

Yes, it's TB68 only patch.

Comment 17

[Jorg K (CEST = GMT+2)]

OK, no checkin-needed, it's handled via the other flags.

Comment 18

[Jorg K (CEST = GMT+2)]

TB 68.2.0 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/6e8cdcaf6051959c5d6ec23d5c5b2881815187f4

Comment 19

[Jorg K (CEST = GMT+2)]

Working in TB 68.2.0 ESR.