Fix pretty printing of kick messages

[bmeh]

Fix pretty printing of kick messages

[bmeh]

It's worth noting that the kick reason is not necessarily in e.Trailing. Some (or all?) ircv3 servers, such as oragono omits the colon, therefore instead of e.Trailing, the reason is in e.Params[2], but other servers, such as InspIRCd appends the colon, so the reason will be in e.Trailing.

Another difference is that ircv3 servers - oragono in particular - don't append your nickname in their responses (or does so inconsistently), which makes many IRC clients (including irssi) have off-by-one format issues at some places.

Examples:

Input:
/invite foo #bar

Output:
Inviting #bar to

Input:
/whois foo

Output:
[...]
00:00:00 -!- account : is logged in as
[...]

This might warrant a re-write for portability. I don't use e.Pretty() so I added my own checks to handle the incompatibilities.

[lrstanley]

As for the original issue, b102aff will suffice for now until the trailing->param changes are in place.

It's worth noting that the kick reason is not necessarily in e.Trailing. Some (or all?) ircv3 servers, such as oragono omits the colon, therefore instead of e.Trailing, the reason is in e.Params[2], but other servers, such as InspIRCd appends the colon, so the reason will be in e.Trailing.

1. Most of the IRC servers I've seen don't do this unless it's just the nickname of the user being kicked and isn't an actual reason (i.e. e.Params[2] like you mentioned), even IRCv3 servers. Parameters, other than the last parameter, cannot have spaces or anything in it (thus why the trailing parameter exists).
2. Per the RFC spec, and the general use of the colon as a trailing parameter, it should be specified as a trailing parameter. There is no good reason why a server shouldn't as it's safer, though some do (see above, "just the nickname").
3. Oragono uses a colon when a reason is specified.

Reason:

@time=2018-09-05T23:43:31.117Z :[email protected] KICK #girc-test Guest35 :test

No reason:

@time=2018-09-05T23:44:29.538Z :[email protected] KICK #girc-test Guest35 Guest35

Another server, no reason (still provides it as a trailing, supports IRCv3, charybdis):

:[email protected] KICK #dev girc-test1_ :girc-test1_

In any case, the change I mentioned to move Event.Trailing to a standard parameter, will ensure servers that do this won't cause an issue.

As for the "ircv3 servers don't [...]" -- let's not push IRCv3 servers into one group. No server does everything correctly, or in a standard way. The IRCv3 spec does not change any base commands/numerics of how standard commands should work. They're all different unfortunately.

As for off-by-one, that's a fairly rare occurrence I feel. Servers should not omit things that are documented in the RFC (they do often stray from the RFC, but only to add features usually, not break backwards compatibility for no reason). I can understand for things that aren't in the RFC too. This doesn't happen with IRSSI for me:

20:11 -!- liam invites you to #dev

It's possible the server is sending INVITE arguments in an invalid fashion. The spec defines that the username has to be provided, and in a specific order, so if it's not being provided, it's wrong. I don't see any benefit in excluding it either (in fact, it's likely there, to allow INVITE forwarding, where the source of the message isn't the inviter).

:[email protected] INVITE girc-test1 :#dev

As for whois output, there is no spec for this. The output is generic and can be anything. Clients try and guess, and there isn't much else you can do, other than just show the full whois output (what most clients do). When you see account : is logged in as [...], account : is actually sent by the server, unlikely parsed by the client. Many ircd have weird whitespaces on the left side of colons, and isn't a client bug.

[bmeh]

No, it doesn't necessarily use colons. For a one word reason, it omits the colon. I also tested it on InspIRCd-2.0, where it doesn't omit the colon even when the reason is one word.

$ netcat localhost 6667
:127.0.0.1 NOTICE * :*** Looking up your username
:127.0.0.1 NOTICE * :*** Could not find your username
USER foo foo foo :foo
NICK foo
:127.0.0.1 001 foo :Welcome to the Internet Relay Network foo
:127.0.0.1 002 foo :Your host is 127.0.0.1, running version oragono-0.11.0-31e5db9
:127.0.0.1 003 foo :This server was created Wed, 05 Sep 2018 22:42:06 CEST
:127.0.0.1 004 foo 127.0.0.1 oragono-0.11.0-31e5db9 aBioRsE bEeIikmntRsl
:127.0.0.1 005 foo ACCCOMMANDS=CREATE,VERIFY AWAYLEN=500 CASEMAPPING=ascii CHANMODES=beI,,lk,imntEs CHANNELLEN=64 CHANTYPES=# ELIST=U EXCEPTS= INVEX= KICKLEN=1000 MAXLIST=beI:60 MAXTARGETS=4 MODES= :are supported by this server
:127.0.0.1 005 foo MONITOR=100 NETWORK=OragonoTest NICKLEN=32 PREFIX=(qaohv)~&@%+ REGCALLBACKS= REGCREDTYPES=passphrase,certfp RPCHAN=E RPUSER=E STATUSMSG=~&@%+ TARGMAX=NAMES:1,LIST:1,KICK:1,WHOIS:1,USERHOST:10,PRIVMSG:4,TAGMSG:4,NOTICE:4,MONITOR: TOPICLEN=1000 UTF8MAPPING=rfc8265 :are supported by this server
:127.0.0.1 422 foo :MOTD File is missing
JOIN #channel
:[email protected] JOIN #channel
:127.0.0.1 353 foo = #channel @foo
:127.0.0.1 366 foo #channel :End of NAMES list
:127.0.0.1 MODE #channel +o foo
KICK #channel foo reason
:[email protected] KICK #channel foo reason

03:18:09 -!- foo was kicked from #channel by foo [reason]

---

No, the off-by-one with irssi (v1.1.1) actually happens with oragono, as I have said, most servers append the nickname (or username), whereas for example oragono doesn't (or does it so inconsistently, and/or it's configurable), causing this issue.

Evidence, confirming what I have said: https://i.imgur.com/EbvZa3U.png

It does the same with /quote INVITE bar #channel.

It happens because the response is:
:127.0.0.1 341 bar #channel instead of
:127.0.0.1 341 foo bar #channel

i.e.

:[email protected] INVITE bar #channel
:127.0.0.1 341 bar #channel

---

I literally have to add this code to make it work with both oragono, and InspIRCd:

  c.Handlers.Add(girc.KICK, func(c *girc.Client, e girc.Event) {
    var reason string
    if len(e.Params) > 2 {
      reason = e.Params[2]
    } else {
      reason = e.Trailing
    }

    if e.Params[1] == cfg.User.Nickname {
      if len(reason) > 0 {
        log.Printf("Kicked from %s by %s: %s",
          e.Params[0], e.Source.Name, reason)
      } else {
        log.Printf("Kicked from %s by %s", e.Params[0], e.Source.Name)
      }
    }
  })

  c.Handlers.Add(girc.INVITE, func(c *girc.Client, e girc.Event) {
    var channel string
    if len(e.Params) > 1 {
      channel = e.Params[1]
    } else {
      channel = e.Trailing
    }

    log.Printf("Invitation from %s to %s", e.Source.Name, channel)
  })

on InspIRCd:

2018/09/06 04:16:59 Joined #channel
2018/09/06 04:17:07 Kicked from #channel by foo
2018/09/06 04:17:18 Joined #channel
2018/09/06 04:17:30 Kicked from #channel by foo: reason
2018/09/06 04:24:28 Invitation from foo to #channel2

on oragono:

2018/09/06 04:18:10 Joined #channel
2018/09/06 04:18:11 Kicked from #channel by foo: bar
2018/09/06 04:18:24 Joined #channel
2018/09/06 04:18:26 Kicked from #channel by foo: reason
2018/09/06 04:22:45 Invitation from foo to #channel2

All the commands I am sending are, in order:

1. KICK bar
2. KICK bar reason
3. INVITE bar #channel2

Not having those checks would not result in the correct output, in fact, in one case it panics if I dare to access e.Params[2] on a server (or if that's the case) that it has its reason in e.Trailing instead. It's extremely inconsistent, but this code works with both IRCd servers.

---

Bottom line is that these servers are extremely inconsistent and incompatible. These checks are necessary, otherwise you get garbage, run into off-by-one issues, or get a panic. Please do try the above snippet on the IRCd you are using, run the same commands, and let me know if you are getting the right output.

[lrstanley]

Will test tomorrow/look at starting to rewrite the trailing implementation. These are some good usecases for additional testing too.

…

---- On Wed, 05 Sep 2018 21:47:34 -0500 [email protected] wrote ---- No, it doesn't necessarily use colons. For a one word reason, it omits the colon. I also tested it on InspIRCd-2.0, it doesn't omit the colon even when the reason is one word. $ netcat localhost 6667 :127.0.0.1 NOTICE * :*** Looking up your username :127.0.0.1 NOTICE * :*** Could not find your username USER foo foo foo :foo NICK foo :127.0.0.1 001 foo :Welcome to the Internet Relay Network foo :127.0.0.1 002 foo :Your host is 127.0.0.1, running version oragono-0.11.0-31e5db9 :127.0.0.1 003 foo :This server was created Wed, 05 Sep 2018 22:42:06 CEST :127.0.0.1 004 foo 127.0.0.1 oragono-0.11.0-31e5db9 aBioRsE bEeIikmntRsl :127.0.0.1 005 foo ACCCOMMANDS=CREATE,VERIFY AWAYLEN=500 CASEMAPPING=ascii CHANMODES=beI,,lk,imntEs CHANNELLEN=64 CHANTYPES=# ELIST=U EXCEPTS= INVEX= KICKLEN=1000 MAXLIST=beI:60 MAXTARGETS=4 MODES= :are supported by this server :127.0.0.1 005 foo MONITOR=100 NETWORK=OragonoTest NICKLEN=32 PREFIX=(qaohv)~&@%+ REGCALLBACKS= REGCREDTYPES=passphrase,certfp RPCHAN=E RPUSER=E STATUSMSG=~&@%+ TARGMAX=NAMES:1,LIST:1,KICK:1,WHOIS:1,USERHOST:10,PRIVMSG:4,TAGMSG:4,NOTICE:4,MONITOR: TOPICLEN=1000 UTF8MAPPING=rfc8265 :are supported by this server :127.0.0.1 422 foo :MOTD File is missing JOIN #channel :[email protected] JOIN #channel :127.0.0.1 353 foo = #channel @foo :127.0.0.1 366 foo #channel :End of NAMES list :127.0.0.1 MODE #channel +o foo KICK #channel foo reason :[email protected] KICK #channel foo reason 03:18:09 -!- foo was kicked from #channel by foo [reason] No, the off-by-one with irssi (v1.1.1) actually happens with oragono, as I have said, most servers append the nickname (or username), whereas for example oragono doesn't (or it's inconsistent), causing this issue. Evidence, confirming what I have said: https://i.imgur.com/EbvZa3U.png It does the same with /quote INVITE bar #channel. It happens because the response is: :127.0.0.1 341 bar #channel instead of :127.0.0.1 341 foo bar #channel i.e. :[email protected] INVITE bar #channel :127.0.0.1 341 bar #channel I literally have to add this code to make it work with both oragono, and InspIRCd: c.Handlers.Add(girc.KICK, func(c *girc.Client, e girc.Event) { var reason string if len(e.Params) > 2 { reason = e.Params[2] } else { reason = e.Trailing } if e.Params[1] == cfg.User.Nickname { if len(reason) > 0 { log.Printf("Kicked from %s by %s: %s", e.Params[0], e.Source.Name, reason) } else { log.Printf("Kicked from %s by %s", e.Params[0], e.Source.Name) } } }) c.Handlers.Add(girc.INVITE, func(c *girc.Client, e girc.Event) { var channel string if len(e.Params) > 1 { channel = e.Params[1] } else { channel = e.Trailing } log.Printf("Invitation from %s to %s", e.Source.Name, channel) }) on InspIRCd: 2018/09/06 04:16:59 Joined #channel 2018/09/06 04:17:07 Kicked from #channel by foo 2018/09/06 04:17:18 Joined #channel 2018/09/06 04:17:30 Kicked from #channel by foo: reason 2018/09/06 04:24:28 Invitation from foo to #channel2 on oragono: 2018/09/06 04:18:10 Joined #channel 2018/09/06 04:18:11 Kicked from #channel by foo: bar 2018/09/06 04:18:24 Joined #channel 2018/09/06 04:18:26 Kicked from #channel by foo: reason 2018/09/06 04:22:45 Invitation from foo to #channel2 All the commands I am sending are, in order: 1. KICK bar 2. KICK bar reason 3. INVITE bar #channel2 Not having those checks would not result in the correct output, in fact, in one case it panics if I dare to access e.Params[2] on a server (or if that's the case) that it has its reason in e.Trailing instead. It's extremely inconsistent, but this code works with both IRCd servers. Bottom line is that these servers are extremely inconsistent and incompatible. These checks are necessary, otherwise you get garbage, run into off-by-one issues, or get a panic. Please do try the above snippet on the IRCd you are using, run the same commands, and let me know if you are getting the right output. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[bmeh]

Both my friend and I used the same version of oragono, and his version, probably due to its configuration didn't seem to append the nickname like other servers[1] (or my oragono) do, which caused more off-by-one issues, but mine does append it, apart from INVITE, and who knows what else... really need to write test cases that cover this completely (most, if not all commands), using both single-word, and multi-word arguments.

[1] This is why I could only give you an example of INVITE (and KICK with any single-word parameter), that didn't work even in my case, but I remember sending a WHOIS on his server through openssl s_client, and it didn't append the nickname in the response, while it did on my server. I will have to ask him to set it up to do further testing.

Basically the netcat output I posted a few comments earlier was different from his, the foo was completely missing. I remember this because I ranted for hours about this incompatibility to two people, and I probably still have parts of the output, but of course that's not enough and I would have to reproduce it. The first step would be asking for his configuration file. We both use the same version of oragono, so it has to be his configuration that makes it different. I am using default configuration for the most part, which doesn't make it omit the nickname foo from its responses.

Heck, his server didn't even allow me to do NICK[2] where the argument was the name of an account (registered), but according to the documentation, the nick has nothing to do with the account name. It doesn't happen on my server either, or rather with my (mostly) default configuration. Again, will have to ask him for his configuration and whatnot so I could reproduce the aforementioned issues.

[2] :my.friends.oregono.server 433 * foo :Nickname is reserved by a different account
What makes it more weird is that the next time it was:
:my.friends.oregono.server 433 foo foobar :Nickname is reserved by a different account

Another odd behavior: no one was online with the nick foo, yet I got the response: -!- Nick foo is already in use, and when I tried doing (because it was registered at the time) PRIVMSG NickServ GHOST foo, I got: !NickServ No such nick.

---

I did run into many, currently non-reproducible behaviors that might have been due to his configuration for all I know, since some of the odd behaviors are not present on my server and I am using the same version of oragono, but mostly a default configuration, and potentially a different configuration from his. I am not sure if it should affect it in such a way that I should experience those issues, so yeah.

When I have the time I will do ask for his configuration, try to reproduce these issues, and if they do occur, I will open an issue for oragono, unless the configuration settings imply it's supposed to happen. Regardless, this is a potential issue for a library such as girc, and extensive testing is required. For now, everything works on both IRCds as far as I know, but I am only handling KICK and INVITE on my end (using the same snippet I posted), and I use SASL PLAIN for authentication, SSL, etc.

---

Edit:

The reason for Nickname is reserved by a different account are the following settings:

nick-reservation: enabled: true
nick-reservation: method: strict

[bmeh]

Oops, sorry, I am new to GitHub, I didn't know that a direct link to my previous comment would cause a reference. I think it should have its own issue, but I guess for now it's easy to keep track of it. I am not sure if you have the permissions or the ability to fix it, if you do, then feel free to do so if you would like. :)