1134635 - Thunderbird loops GET requests when set as default browser

Status: RESOLVED FIXED

(Thunderbird :: OS Integration, defect)

Description

[benbruecker]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.111 Safari/537.36

Steps to reproduce:

- Thunderbird has to be (errornously) set as default WEB application in Ubuntu 14.04. 
- Click the link to an url in a received e-mail.


Actual results:

- The url does not open (Naturally)
- On the background Thunderbird sends a HTTP GET request every second to the clicked URL
- This behaviour continues, even when thunderbird is closed.
- When clicking multiple links, or the same link multiple times, more then one GET request is sent every second
-- This was discovered after receiving an incident report from a company that traced an `attack' of 25.000 requests in less then 1,5 hours to my IP address
-- It is repeatable on multiple systems


Expected results:

- Thunderbird should show an error message that the url can not be opened
- Also, thunderbird should not try to repeat the GET request every second
- Additionaly, this behaviour should not continue in the backgorund after Thunderbird is closed.

Comment 1

[Wayne Mery (:wsmwk)]

Is there really anything we can/should do about this?
I don't see how.

Comment 2

[Wayne Mery (:wsmwk)]

Does this still reproduce for you?

Comment 3

[Phoenix]

Resolved per whiteboard

Comment 4

[Magnus Melin [:mkmelin]]

I tested this now and it's reproducible, although nowadays it's certainly not easy to get yourself into this situation since the Ubuntu default browser selector only lists browsers and you can't directly add a path (which IIRC one used to be able to do).

Anyway, to reproduce this (use a trash profile), do this (I'm using Ubuntu 18.10):

1. in a terminal execute xdg-settings set default-web-browser thunderbird.desktop
2. send yourself an email with an http link, open the mail and click the link
3. observe the massive traffic!

Now, exactly how to prevent this, I'm not sure. Any ideas? I wonder if it can happen in windows too...

Comment 5

[Magnus Melin [:mkmelin]]

I actually didn't test this with trunk or 68 , only the system version (60) pointed to through thunderbird.desktop

Comment 6

[Magnus Melin [:mkmelin]]

I wonder if we're ending up here: https://searchfox.org/comm-central/rev/72694e3d97e48704952acf28871e0b64a38081f9/mail/components/nsMailDefaultHandler.js#306

Benjamin, can you investigate - verify it's reproducible on trunk? If that's the spot, can we just add an !/^http?s:/i.test(uri) check to the else above it?

Comment 7

[Benjamin Flanagin]

Confirmed in 69.0b1 and 70.0b1

I've added the suggested fix to the top most if statement here: https://searchfox.org/comm-central/rev/72694e3d97e48704952acf28871e0b64a38081f9/mail/components/nsMailDefaultHandler.js#293
and it resolves the issue. I'm not sure if it is the best play for the check as it might keep some web addresses that link to eml files or feeds from being accessed.

I'll move it around a bit and do some testing.

Comment 8

[Magnus Melin [:mkmelin]]

If it works, submit a patch.

Comment 9

[Benjamin Flanagin]

Attachment: bug-1134635-fix.patch

I've tested the issue against the bugs description. No signs of regression in testing normal TB operations with patch applied.

Comment 10

[Jorg K (CEST = GMT+2)]

You mean ^https? - right?

Comment 11

[Benjamin Flanagin]

I tried with ^https? but that didn't catch "non-secure" sites. I know we're all moving in that direction, but until all sites are secure ^http? catches both. At least in my tests.

Comment 12

[Jorg K (CEST = GMT+2)]

Umm, try https://regex101.com/ with ^https? - Also see:
https://searchfox.org/comm-central/search?q=%5Ehttps%3F&case=false&regexp=false&path=
All that code can't we wrong ;-)

Actually, it should be ^https?:

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9086037 [details] [diff] [review]
bug-1134635-fix.patch

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

::: mail/components/nsMailDefaultHandler.js
@@ +288,5 @@
>        } catch (e) {
>          dump(e);
>        }
>      }
> +    //checks for the existance of a uri and whether or not it is a web address.

please remove, this is self explanatory

@@ +289,5 @@
>          dump(e);
>        }
>      }
> +    //checks for the existance of a uri and whether or not it is a web address.
> +    if (uri && !/^http?/i.test(uri)) {

sorry for putting the ?  at the wrong place earlier, but it should be with the optional s, and required colon, as 

if (uri && !/^https?:/i.test(uri)) {

Comment 14

[alta88]

Since firefox removed feed support, and thus no longer prefixes a feed with |feed:|, attempts to subscribe with Tb as a default result in just an http url being passed to Tb, which opens compose as a default. The feed: protocol never became a standard and it's not probable that other browsers preface with |feed:| either.

The problem here is that passing an http url probably shouldn't open compose, so a decision needs to be made how to handle http in a command line without any flags. If it doesn't go to feed handling, then where? And if not, then the feed bit here is useless.

Comment 15

[Benjamin Flanagin]

No worries Magnus,

I've made the changes to the patch as suggested. Should I go ahead and edit the code that alta88 suggests in comment 14 for good measure?

Comment 16

[Magnus Melin [:mkmelin]]

Yeah let's pass the urls to subscribeToFeed

Comment 18

[Benjamin Flanagin]

Attachment: bug-1134635-fix.patch

Passing http urls to subscribeToFeed. All is well in my tests.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9091271 [details] [diff] [review]
bug-1134635-fix.patch

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

Looks good, I'll attach a patch which removes the unncessary try-catch and fixes the commit message.

::: mail/components/nsMailDefaultHandler.js
@@ +295,5 @@
>            Cc["@mozilla.org/newsblog-feed-downloader;1"]
>              .getService(Ci.nsINewsBlogFeedDownloader)
>              .subscribeToFeed(uri, null, null);
>          } catch (e) {
>            // If feed handling is not installed, do nothing

there's no such thing...

Comment 20

[Magnus Melin [:mkmelin]]

Attachment: bug-1134635-fix.patch

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/47eb1c03649d
make http urls passed from the command line trigger feed subscription. r=mkmelin

Comment 22

[Jorg K (CEST = GMT+2)]

TB 70 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/0783b216edb1080d3a60900e4442a5e42871c15e

Comment 23

[Jorg K (CEST = GMT+2)]

TB 68.1.1 ESR:
https://hg.mozilla.org/releases/comm-esr68/rev/5d2d0a94b89b30a432e8d4e62366e59674fbec94