Closed Bug 362434 Opened 15 years ago Closed 13 years ago

Add IPv6 support to phishingDetector.js


(Thunderbird :: General, defect)

Not set


(Not tracked)

Thunderbird 3.0b1


(Reporter: mscott, Assigned: mkmelin)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

Our built in phishing detector has static tests, one of which involves looking for ipv4 host names in the url. We should make it smart enough to detect ipv6 as well. This could be a good starter bug for someone looking to get involved.
OS: Windows XP → All
Hardware: PC → All
I started to look at this. So the first thing I tried was mailing me a mail containing an IPv6 address. The problem was the
Attached patch patch #1 (obsolete) — Splinter Review
Sorry for the last comment. Just ignore it.

This is my try to add IPv6 support to the phishing detector. I also made some changes to the IPv4 detection.

As local address I added fe80::/64 which is listed on wikipedia as The link-local prefix.

One question is that ::ffff:0:0/96 exists for mapping IPv4 addresses. Should ::ffff:192.16.*.* count as a local address (and the same for the other IPv4 local addresses)?

Then there exists fc00::/7 (Unique Local Addresses (ULA)) should I add that as a local address?

Info about the special addresses I found on Wikipedia:
Assignee: mscott → hesslow
Attachment #267151 - Flags: review?(mscott)
Attached patch patch #2 (obsolete) — Splinter Review
Of cause I found an error almost directly.
Attachment #267151 - Attachment is obsolete: true
Attachment #267155 - Flags: review?(mscott)
Attachment #267151 - Flags: review?(mscott)
Attachment #267155 - Flags: superreview?(mscott)
Attachment #267155 - Flags: review?(mscott)
Attachment #267155 - Flags: review?(mkmelin+mozilla)
Comment on attachment 267155 [details] [diff] [review]
patch #2

>+    return (this.isIPv6HostName(aHostName, aUnobscuredHostName) ||
>+            this.isIPv4HostName(aHostName, aUnobscuredHostName));

Since v4 is much more likely, move that first.

>+  /**
>+   * Private helper method.
>+   * @return unobscured host name (if there is one)
>+   * @return true if aHostName is an IPv4 address

The first return should probably be @param aUnobscuredHostName <description of the side effect>
Same thing later...

>+  isIPv4HostName: function(aHostName, aUnobscuredHostName)
>+  {
>     var index;

It's more readable if you declare loop variables inside the for clause | for (var i = 0 ... |, in cases where they aren't used for anything outside it. Same thing for "count" later on.

Other than that, looks ok to me;
Attachment #267155 - Flags: review?(mkmelin+mozilla) → review+
Emil, do you mind posting a new patch with Magnus's comments? I'll sr that new patch, I don't see any additional comments. Thanks!
Scott, the only problem is that I'm on vacation which mean that I will not have access to my computer until the end of July. So if there is no hurry I can fix it then. But if you want it before that I have no problem with someone else take my patch and fixes the review comments.
Emil: any update on the patch?
Emil: ping on an updated patch?
need no owner for this patch.
emil is no longer using thunderbird afaik, so resetting assignee to nobody
Assignee: hesslow → nobody
Keywords: helpwanted
Whiteboard: needs owner
Attached patch proposed fixSplinter Review
Unbitrotted version of the one I reviewed a year ago. 
I also changed some functions so that we wouldn't pass unobscuredHostName and rely on side effects, as it's soo ugly and hard to read in js (when there's no real need for it).
Assignee: nobody → mkmelin+mozilla
Attachment #267155 - Attachment is obsolete: true
Attachment #347851 - Flags: review?(philringnalda)
Attachment #267155 - Flags: superreview?(mscott)
Keywords: helpwanted
Whiteboard: needs owner
Comment on attachment 347851 [details] [diff] [review]
proposed fix

Looks fine, which means it's time for grammar nits :)

>+   * Check if a host name is and IPv4 host name.


>+    // Make sure there is at least 3 components.
>+    // Make sure there is 8 components.

are rather than is
Attachment #347851 - Flags: review?(philringnalda) → review+
changeset:   1124:8ad921a602ef

Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 3 → Thunderbird 3.0b1
Blocks: TB2SM
You need to log in before you can comment on or make changes to this bug.