indrora / Atomic

Atomic: an IRC client from the ashes of yaaic
https://indrora.github.io/Atomic/
GNU General Public License v3.0
124 stars 29 forks source link

Lines from self detected as PRIVMSG #83

Open liotier opened 9 years ago

liotier commented 9 years ago

With Atomic 1.19(r19) used to connect to the bip IRC proxy.

Context: the use of the bip IRC proxy implies that several IRC clients are simultaneously connected to the proxy - from the IRC server's point of view a single connection is seen. A line entered in a channel on one of the clients is seen on the others - likewise that channel's whole traffic.

Steps to reproduce:

Desired behavior: a lines from self in a channel should be detected as normal channel traffic, not as PRIVMSG.

liotier commented 9 years ago

In https://github.com/indrora/Atomic/blob/master/application/src/org/jibble/pircbot/PircBot.java#L1112 we see :

if (command.equals("PRIVMSG") && _channelPrefixes.indexOf(target.charAt(0)) >= 0) {
      // This is a normal message to a channel.
      this.onMessage( messageDate, target, sourceNick, sourceLogin, sourceHostname, line.substring(line.indexOf(" :") + 2));
    } else if (command.equals("PRIVMSG")) {
      // This is a private message to us.
      // XXX PircBot patch to pass target info to privmsg callback
      this.onPrivateMessage(messageDate, sourceNick, sourceLogin, sourceHostname, target, line.substring(line.indexOf(" :") + 2));

From this I am guessing that && _channelPrefixes.indexOf(target.charAt(0)) >= 0) is insufficient to detect that the message is a normal message to a channel and not a private message... But I am unable to offer a better discriminant.

indrora commented 9 years ago

I'll look into it. Bio may be wrong in its implemented behavior. It doesn't happen with znc. On Feb 12, 2015 10:07 AM, "Jean-Marc Liotier" notifications@github.com wrote:

In https://github.com/indrora/Atomic/blob/master/application/src/org/jibble/pircbot/PircBot.java#L1112 we see :

if (command.equals("PRIVMSG") && _channelPrefixes.indexOf(target.charAt(0)) >= 0) { // This is a normal message to a channel. this.onMessage( messageDate, target, sourceNick, sourceLogin, sourceHostname, line.substring(line.indexOf(" :") + 2)); } else if (command.equals("PRIVMSG")) { // This is a private message to us. // XXX PircBot patch to pass target info to privmsg callback this.onPrivateMessage(messageDate, sourceNick, sourceLogin, sourceHostname, target, line.substring(line.indexOf(" :") + 2));

From this I am guessing that && _channelPrefixes.indexOf(target.charAt(0))

= 0) is insufficient to detect that the message is a normal message to a channel and not a private message... But I am unable to offer a better discriminant.

— Reply to this email directly or view it on GitHub https://github.com/indrora/Atomic/issues/83#issuecomment-74109798.

liotier commented 9 years ago

I'll get you packet captures tomorrow.

liotier commented 9 years ago

First sample packet: line sent from bip to Atomic, written by another participant in the channel. It shows in Atomic as a normal channel message:

0000  3a 56 61 64 6f 52 21 7e 56 61 64 6f 52 40 6b 69   :VadoR!~VadoR@ki
0010  76 75 2e 67 72 61 62 65 75 68 2e 63 6f 6d 20 50   vu.grabeuh.com P
0020  52 49 56 4d 53 47 20 23 62 5e 32 20 3a 62 65 6e   RIVMSG #b^2 :ben
0030  20 63 61 20 64 c3 a9 70 65 6e 64 2c 20 c3 a7 61    ca d..pend, ..a
0040  20 62 20 6f 75 67 65 20 61 75 73 73 69 20 73 69    b ouge aussi si
0050  20 74 75 20 72 65 6d 70 6c 61 63 65 73 20 22 76    tu remplaces "v
0060  65 6e 64 75 73 22 20 70 61 72 20 22 6c 75 73 22   endus" par "lus"
0070  0d 0a

Second sample packet: line sent from bip to Atomic, written by me. It shows in Atomic in the PRIVMSG tab:

0000  3a 4a 69 4d 4c 20 50 52 49 56 4d 53 47 20 23 62   :JiML PRIVMSG #b
0010  5e 32 20 3a 4a 65 20 6c 61 20 76 6f 69 73 20 62   ^2 :Je la vois b
0020  69 65 6e 20 70 6f 75 72 20 64 75 20 73 69 67 6e   ien pour du sign
0030  61 67 65 2c 20 6d 61 69 73 20 70 6f 75 72 20 64   age, mais pour d
0040  75 20 74 65 78 74 65 20 6c 6f 6e 67 20 6a 65 20   u texte long je
0050  6e 65 20 73 75 69 73 20 70 61 73 20 63 65 72 74  ne suis pas cert
0060  61 69 6e 20 64 65 20 73 61 20 6c 69 73 69 62 69  ain de sa lisibi
0070  6c 69 74 c3 a9 0d 0a                                                lit....

Then, in another one of the clients simultaneously connected to bip, I changed my nick to something entirely unknown to either bip or Atomic or anything else. Then, what I uttered in the channel has also been detected by Atomic to be displayed in the PRIVMSG tab. So Atomic does not use nickname detection - it really seems to attempt discriminating private messages from normal channel ones. Sample:

0000  3a 47 75 65 73 74 33 31 35 31 33 20 50 52 49 56   :Guest31513 PRIV
0010  4d 53 47 20 23 62 5e 32 20 3a 54 65 73 74 0d 0a   MSG #b^2 :Test..

And for completeness' sake, here is a genuine private message sent to me by another user. With bip, all clients including Atomic interpret it as a "new tab" named with the sender's nick - which I believe is the correct behaviour.

0000  3a 54 65 73 74 77 68 61 74 21 64 34 35 35 39 61   :Testwhat!d4559a
0010  30 66 40 67 61 74 65 77 61 79 2f 77 65 62 2f 66   0f@gateway/web/f
0020  72 65 65 6e 6f 64 65 2f 69 70 2e 32 31 32 2e 38   reenode/ip.212.8
0030  35 2e 31 35 34 2e 31 35 20 50 52 49 56 4d 53 47   5.154.15 PRIVMSG
0040  20 4a 69 4d 4c 20 3a 48 65 6c 6c 6f 20 74 68 69    JiML :Hello thi
0050  73 20 69 73 20 61 20 74 65 73 74 2e 0d 0a         s is a test...

(Note that I may have edited spaces from the capture - I can send you raw captures if you wish, though I'll have to re-do them since I did not keep clean pcap files)

So, to summarize...

A line beginning like this is identified by Atomic as a normal channel message: :VadoR!~VadoR@kivu.grabeuh.com PRIVMSG #b^2 :ben

Lines beginning like those are identified by Atomic as private messages in the PRIVMSG tab: :Guest31513 PRIVMSG #b^2 :Test.. :JiML PRIVMSG #b^2 :Je la vois bien

This private message is correctly put into its own tab: :Testwhat!d4559a0f@gateway/web/freenode/ip.212.85.154.15 PRIVMSG JiML :Hello this is a test...

Now, how does that mesh with _channelPrefixes.indexOf(target.charAt(0)) >= 0) ?

channelPrefixes is defined in application/src/org/jibble/pircbot/PircBot.java at line 3228 (damn - the latest commit changed all the paths... Ok, it is now https://github.com/indrora/Atomic/blob/master/atomic/src/main/java/org/jibble/pircbot/PircBot.java#L3228): private final String _channelPrefixes = "#&+!";

The indexOf method deals with finding one of the channelPrefixes string's chars in (probably self-explanatory to any Java developer, but this is my first time reading Java - or almost).

In many of PircBot.java's methods, @param target is the name of the channel or user nick to send to. The charAt method returns the string's char at the given position - here position 0, the first. Since we are in PircBot.java's handleLine method, target is - uh... I don't quite understand: in some cases of (senderInfo.startsWith(":")) it is not changed from its String target = null; initialization.

One of the obvious differences between the lines is the exclamation mark right after the sender's nick, one of the signs defined in channelPrefixes.

Would the lack of exclamation mark right after the sender's nick be enough for Atomic to decide that the line must be displayed in the PRIVMSG tab ?

Isn't the mere existence of a PRIVMSG tab actually a bug ? The tab should either have the sender's name or a channel's name.

Let's look at the structure of my example captures...

Parsed as normal channel message: :NICK!whatever PRIVMSG text text

Erroneously parsed to display in the PRIVMSG tab: :NICK PRIVMSG text text

Correctly parsed as private message in its own tab: :NICK!whatever PRIVMSG MY_OWN_NICK text text

So maybe the whole else if (command.equals("PRIVMSG")) {} block could be deleted and this would solve this bug. But I'm not confident enough - I lack IRC parsing experience for a more definitive opinion.

Anyway... I'm not sure I'm getting anywhere... Over to you !

And oh - look ! A bip special case !

            // XXX: PircBot Patch - (Needed for BIP IRC Proxy)
            //      If this is a NICK command, use next token as target
            if (command.equalsIgnoreCase("nick")) {
              target = tokenizer.nextToken();
            }

It has nothing to do with the present bug but I take special cases as a bad sign - even in the unstandardized mess that IRC is...