278763 - ChatZilla's kick-ban command should put * at the end of an IP address, not the beginning

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Rob Marshall [tH]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050115 Firefox/1.0+
Build Identifier: Chatzilla 0.9.67 [Firefox 1.0+/20050115]

With hostnames, the most specific part is usually before the first dot, so
ChatZilla puts a * there when banning. With numeric IP addresses, it should do
things the other way around.

Reproducible: Always

Steps to Reproduce:
Kick and ban a user without a hostname using the context menu or the kick-ban
command.
Actual Results:  
A mode like "+b *!user@*.0.0.1" is set.

Expected Results:  
A mode like "+b *!user@127.0.0.*" should be set instead.

Comment 1

[Rob Marshall [tH]]

Attachment: draft patch

This seems to work, I'm not too sure about coding style though...

Comment 2

[:Gijs (he/him)]

ChatZilla coding style would have:

if (condition)
    statement;
else
    statement;

As long as there's only one statement with only one line of code in each
if-block and else-block.
See http://www.hacksrus.com/~ginda/pedant.html

I'm not entitled to review this or anything, but I guessed you might appreciate
some feedback. If you're done, you can request review from ssieb or rginda.

Comment 3

[Rob Marshall [tH]]

Attachment: patch 2

Comment 4

[Samuel Sieb]

Comment on attachment 178369 [details] [diff] [review]
patch 2

>+            var hostmask = e.user.host.replace(/[^.]+$/, "*");
>+        else
>+            var hostmask = e.user.host.replace(/^[^.]+/, "*");

This will cause a strict warning, you declared the variable twice. Declare it
before the test.

Comment 5

[Rob Marshall [tH]]

Attachment: patch 3 - avoid strict warning

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 178377 [details] [diff] [review]
patch 3 - avoid strict warning

a=asa

Comment 7

[James Ross]

Checked in --> FIXED.

An automatic nightly ChatZilla build containing this fix will be available in
around 40 minutes from http://twpol.dyndns.org/mozilla/chatzilla/nightly/

Comment 8

[:Gijs (he/him)]

This bug regressed when ban and unban were implemented, and determining the ban
mask was moved to CIRCUser.prototype.getBanMask.

0.9.68.* all have a 'proper' getBanMask, cvs does not. We need to migrate this
patch to the appropriate place in irc.js

Note that we still no longer put a * on the first bit of the ip - we just fail
to replace the last bit with a *. Setting minority to minor. This may also be a
wontfix - depends how useful we think this really is. It's 3.30am atm, so don't
ask me to make any sensible decisions. James, Robert, Samuel, it's your call :-)

Comment 9

[:Gijs (he/him)]

Attachment: Fix regression by porting older patch

As I'm adding this, I wonder if this would work with ipv6? Do we work with ipv6? Do IRC servers work with ipv6 at all?

Comment 10

[James Ross]

Comment on attachment 200810 [details] [diff] [review]
Fix regression by porting older patch

>-    if (!/^\d+\.\d+\.\d+\.\d+$/.test(hostmask))
>+    if (hostmask.match(/^[\d\.]*$/) != null)
>+        hostmask = hostmask.replace(/[^.]+$/, "*");
>+    else

That regexp wont work. It requires the IP to be "123.123.123.123." which is wont ever be (no trailing period).

The original one used by the code that you are removing will, however, work.

I'm not actually sure we want this at all, though.

Comment 11

[James Ross]

Sorry, I misread the regexp there. I still prefer the one being removed, though.

Comment 12

[:Gijs (he/him)]

Attachment: Patch with old regexp in place

Alright. So then this patch should do, or we should just close the bug as fixed again. That's up to you though :-).

Comment 13

[James Ross]

Comment on attachment 206769 [details] [diff] [review]
Patch with old regexp in place

I think, per a discussion on IRC, we want to review these munges at some point, but for now I'm OK with just banning an entire subnet.

Comment 14

[James Ross]

Checked in --> (RE)FIXED.