search

jaraco / irc

Full-featured Python IRC library for Python.
MIT License
390 stars 84 forks source link

real_nickname is not properly set when connecting to ZNC bouncers #152

Open sbraz opened 5 years ago

sbraz commented 5 years ago

Hi, I noticed that real_nickname is always wrong when I connect to ZNC bouncers:

#!/usr/bin/env python3

import ssl
import logging
import sys

import irc.connection
import irc.client

def on_welcome(c, e):
    print(c.get_nickname())

if __name__ == "__main__":
    #logging.basicConfig(level=logging.DEBUG)
    ssl_factory = irc.connection.Factory(wrapper=ssl.wrap_socket)
    reactor = irc.client.Reactor()
    reactor.server().connect(
            server,
            port,
            "", # nick doesn't matter
            connect_factory=ssl_factory,
            password=password,
    )
    reactor.add_global_handler("welcome", on_welcome)
    reactor.process_forever()

This will print Welcome to the freenode Internet Relay Chat Network sbraz. The debug log shows the following:

DEBUG:irc.client:FROM SERVER: :barjavel.freenode.net 001 :Welcome to the freenode Internet Relay Chat Network sbraz
jaraco commented 5 years ago

It's unclear to me exactly what's going on.

It seems to me that the client is expecting the argument to the 001 (welcome) command to be the real nickname. The code is here.

So I guess the questions are - What is the spec for 001? What does that imply for irc.client? Even if ZNC is violating the spec, should the client account for this deviation?

Can you do more investigation into what the IRC specs say about the 001 Welcome message?

DarthGandalf commented 5 years ago

ZNC version? debug logs from ZNC?

sbraz commented 5 years ago

I looked at rfc2812 and it contains the following example:

   001    RPL_WELCOME
         "Welcome to the Internet Relay Network
          <nick>!<user>@<host>"

@DarthGandalf I'm running 1.7.1.

[2018-10-23 21:08:29.978562] (sbraz/freenode) ZNC -> CLI [:kornbluth.freenode.net 001 :Welcome to the freenode Internet Relay Chat Network sbraz]
DarthGandalf commented 5 years ago

Can you show the full logs from the moment when client connects? Obviously, replace the password with XXX

sbraz commented 5 years ago

I get it now, it's because I passed it an empty nickname.

[2018-10-23 21:08:29.836468] _LISTENER == ConnectionFrom(xx, xx) [Allowed]
[2018-10-23 21:08:29.836618] There are [0] clients from [xx]
[2018-10-23 21:08:29.909490] (xx) CLI -> ZNC [PASS sbraz/freenode:<censored>]
[2018-10-23 21:08:29.978129] (xx) CLI -> ZNC [NICK]
[2018-10-23 21:08:29.978370] (xx) CLI -> ZNC [USER  0 * :]
[2018-10-23 21:08:29.978562] (sbraz/freenode) ZNC -> CLI [:kornbluth.freenode.net 001 :Welcome to the freenode Internet Relay Chat Network sbraz]

If I use a non-empty nickname, I see it in the reply. However, it doesn't help the library find out what the real nickname is.

DarthGandalf commented 5 years ago

IRC protocol doesn't allow empty parameters like that, CLI -> ZNC [USER 0 * :] is broken.

sbraz commented 5 years ago

A few more logs after setting my nickname to notreal:

DEBUG:irc.client:TO SERVER: NICK notreal
DEBUG:irc.client:TO SERVER: USER notreal 0 * :notreal
DEBUG:irc.client:process_forever(timeout=0.2)
DEBUG:irc.client:FROM SERVER: :kornbluth.freenode.net 001 notreal :Welcome to the freenode Internet Relay Chat Network sbraz
…
DEBUG:irc.client:FROM SERVER: :notreal!~sbraz@gentoo/developer/sbraz NICK :sbraz
DEBUG:irc.client:command: nick, source: notreal!~sbraz@gentoo/developer/sbraz, target: sbraz, arguments: [], tags: None

In this case, real_nickname is set to notreal, it looks like the client is missing a callback to change its value when we receive the nick change.

To summarize:

sbraz commented 5 years ago

After performing some more tests, I noticed that real_nickname is properly updated if I start the connection with a non-empty nickname: https://github.com/jaraco/irc/blob/dd16a701cb357cbd547dd631e7f942473a0dd897/irc/client.py#L288 The only remaining problem is that the client allows us to use an empty nick and this should be banned.

jaraco commented 5 years ago

the client allows us to use an empty nick and this should be banned.

I haven't looked into it, but if you can say with confidence that an empty nick is never valid, then I agree - the client should disallow invalid inputs.

sbraz commented 5 years ago

the client allows us to use an empty nick and this should be banned.

I haven't looked into it, but if you can say with confidence that an empty nick is never valid, then I agree - the client should disallow invalid inputs.

I don't see anything about an empty nick in the RFC so perhaps it's allowed: https://tools.ietf.org/html/rfc2812#section-3.1.2

@DarthGandalf what do you think?

DarthGandalf commented 5 years ago

Hey, how do you imagine someone talking on IRC with an empty nick? RFC doesn't mention all possible corner cases, yes, but this is one where it's obvious what shouldn't be done.

The section you need here is https://tools.ietf.org/html/rfc1459#section-2.3.1 - it says that there can be multiple spaces between parameters, and it says that the last parameter can be empty.

Btw, the RFC you linked is not used much; the reality is somewhere between RFC 1459, and RFC 2812, closer to RFC 1459. And there is stuff coming from non-RFC, some of which is described at https://modern.ircdocs.horse/ or in https://ircv3.net

jaraco commented 4 years ago

So the conclusion is that empty nicks should be rejected? Can someone send a PR?