312643 - phishingdetector exception for relative urls

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Magnus Melin [:mkmelin]]

Mails that contain relative links e.g. <a href="somefile.html">file</a> will
make the phishingdetector throw an exception -> no more checking for the message.

Error: [Exception... "Component returned failure code: 0x804b000a
[nsIIOService.newURI]"  nsresult: "0x804b000a (<unknown>)"  location: "JS frame
:: chrome://messenger/content/phishingDetector.js :: isPhishingURL :: line 67" 
data: no]
Source File: chrome://messenger/content/phishingDetector.js
Line: 67

Patch coming up.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Shouldn't you be passing in the baseURI of the document so that the link is
resolved correctly? What happens if the email contains a <base href=""> tag? I'm
not familiar with how Thunderbird displays messages with such links, but it
looks to me like this patch would introduce a failure point for the phishing
detector.

Comment 3

[Magnus Melin [:mkmelin]]

(In reply to comment #2)
> Shouldn't you be passing in the baseURI of the document so that the link is
> resolved correctly? What happens if the email contains a <base href=""> tag? I'm

That resolving handled by the dom automatically AFAIK. This patch wouldn't
change how <base> is handled.

> not familiar with how Thunderbird displays messages with such links, but it
> looks to me like this patch would introduce a failure point for the phishing
> detector.

I don't see how. It would only make a difference in the cases where someone has
included a relative link. Now we just stop checking the message because of the
exception. With the patch, we go on to check the next link.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #3)
> That resolving handled by the dom automatically AFAIK. This patch wouldn't
> change how <base> is handled.

Ah, indeed, it's using the property and not the attribute. Nevermind! :)

> I don't see how. It would only make a difference in the cases where someone has
> included a relative link. Now we just stop checking the message because of the
> exception. With the patch, we go on to check the next link.

Right, but my example was:
<base href="http://1.2.3.4/">
<a href="page.html">

and I was worried you'd be skipping over such a link.

Comment 5

[Scott MacGregor]

Magnus/Gavin are you both in agreement with the patch now? Sounds like there was some discussion going on here. 

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Yes, that patch looks correct to me. If the href property cannot be parsed as a URI, the link should be harmless.

Comment 7

[Magnus Melin [:mkmelin]]

Yeah, I'm ok with it too.

Comment 8

[Scott MacGregor]

Comment on attachment 199758 [details] [diff] [review]
proposed fix

cool.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

I can check this in later tonight.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Checked in on the trunk.
mozilla/mail/base/content/phishingDetector.js; new revision: 1.14;

Comment 11

[:Gavin Sharp [email: [e-mail]]]

mozilla/mail/base/content/phishingDetector.js; new revision: 1.12.2.4;