Closed Bug 1134635 Opened 5 years ago Closed Last month

Thunderbird loops GET requests when set as default browser

Categories

(Thunderbird :: OS Integration, defect)

31 Branch
x86_64
Linux
defect
Not set

Tracking

(thunderbird_esr6869+ fixed, thunderbird70 fixed, thunderbird71 fixed)

RESOLVED FIXED
Thunderbird 71.0
Tracking Status
thunderbird_esr68 69+ fixed
thunderbird70 --- fixed
thunderbird71 --- fixed

People

(Reporter: benbruecker, Assigned: benjamin)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Is there really anything we can/should do about this?
I don't see how.
Flags: needinfo?(mkmelin+mozilla)
Summary: Thunderbird spams GET requests when set as default browser → Thunderbird loops GET requests when set as default browser
Does this still reproduce for you?
Flags: needinfo?(mkmelin+mozilla) → needinfo?(benbruecker)
Whiteboard: [closeme 2017-03-20]
Resolved per whiteboard
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(benbruecker)
Resolution: --- → INCOMPLETE
Whiteboard: [closeme 2017-03-20]

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

Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INCOMPLETE → ---
See Also: → 1564176

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

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?

Flags: needinfo?(benjamin)

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.

Flags: needinfo?(benjamin)

If it works, submit a patch.

Assignee: nobody → benjamin
Attached patch bug-1134635-fix.patch (obsolete) — Splinter Review

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

Attachment #9086037 - Flags: review?(mkmelin+mozilla)

You mean ^https? - right?

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.

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 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)) {
Attachment #9086037 - Flags: review?(mkmelin+mozilla) → review-

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.

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?

Flags: needinfo?(mkmelin+mozilla)

Yeah let's pass the urls to subscribeToFeed

Flags: needinfo?(mkmelin+mozilla)
See Also: → 1396243
Duplicate of this bug: 1396243
Attached patch bug-1134635-fix.patch (obsolete) — Splinter Review

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

Attachment #9086037 - Attachment is obsolete: true
Attachment #9091271 - Flags: review?(mkmelin+mozilla)
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...
Attachment #9091271 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9091271 - Attachment is obsolete: true
Attachment #9091351 - Flags: review+

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

Status: REOPENED → RESOLVED
Closed: 3 years agoLast month
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0
Attachment #9091351 - Flags: approval-comm-esr68?
Attachment #9091351 - Flags: approval-comm-beta?
Attachment #9091351 - Flags: approval-comm-beta? → approval-comm-beta+
Attachment #9091351 - Flags: approval-comm-esr68? → approval-comm-esr68+
You need to log in before you can comment on or make changes to this bug.