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

RESOLVED FIXED

Status

--
minor
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: rdmsoft, Assigned: rginda)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cz-patch][cz-0.9.69.2])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 171573 [details] [diff] [review]
draft patch

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

Updated

14 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

14 years ago
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.
(Reporter)

Comment 3

14 years ago
Created attachment 178369 [details] [diff] [review]
patch 2
Attachment #171573 - Attachment is obsolete: true
Attachment #178369 - Flags: review?(jessiebarnes72)
(Reporter)

Updated

14 years ago
Attachment #178369 - Flags: review?(jessiebarnes72) → review?(samuel)

Comment 4

14 years ago
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.
Attachment #178369 - Flags: review?(samuel) → review-
(Reporter)

Comment 5

14 years ago
Created attachment 178377 [details] [diff] [review]
patch 3 - avoid strict warning
Attachment #178369 - Attachment is obsolete: true
Attachment #178377 - Flags: review?(samuel)

Updated

14 years ago
Attachment #178377 - Flags: review?(samuel) → review+

Updated

14 years ago
Whiteboard: cz-patch

Updated

14 years ago
Attachment #178377 - Flags: approval1.8b2?

Comment 6

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

a=asa
Attachment #178377 - Flags: approval1.8b2? → approval1.8b2+

Comment 7

14 years ago
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/
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 8

13 years ago
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 :-)
Severity: normal → minor
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

13 years ago
Blocks: 299458

Comment 9

13 years ago
Created attachment 200810 [details] [diff] [review]
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?
Attachment #200810 - Flags: review?(silver)

Comment 10

13 years ago
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.
Attachment #200810 - Flags: review?(silver) → review-

Comment 11

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

Updated

13 years ago
Attachment #178377 - Attachment is obsolete: true

Comment 12

13 years ago
Created attachment 206769 [details] [diff] [review]
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 :-).
Attachment #200810 - Attachment is obsolete: true
Attachment #206769 - Flags: review?(silver)

Comment 13

13 years ago
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.
Attachment #206769 - Flags: review?(silver) → review+

Comment 14

13 years ago
Checked in --> (RE)FIXED.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Whiteboard: cz-patch → [cz-patch][cz-0.9.69.2]
You need to log in before you can comment on or make changes to this bug.