Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sbraz opened this issue Oct 4, 2018 · 12 comments
Open

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

sbraz opened this issue Oct 4, 2018 · 12 comments

Comments

@sbraz
Copy link
Contributor

sbraz commented Oct 4, 2018

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
Copy link
Owner

jaraco commented Oct 23, 2018

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
Copy link

ZNC version? debug logs from ZNC?

@sbraz
Copy link
Contributor Author

sbraz commented Oct 23, 2018

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
Copy link

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

@sbraz
Copy link
Contributor Author

sbraz commented Oct 23, 2018

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
Copy link

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

@sbraz
Copy link
Contributor Author

sbraz commented Oct 23, 2018

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:

  • the client shouldn't allow an empty nickname (and maybe ZNC shouldn't either)
  • the client should update the nickname when it receives the NICK command

@sbraz
Copy link
Contributor Author

sbraz commented Oct 26, 2018

After performing some more tests, I noticed that real_nickname is properly updated if I start the connection with a non-empty nickname:

if command == "nick":

The only remaining problem is that the client allows us to use an empty nick and this should be banned.

@jaraco
Copy link
Owner

jaraco commented Mar 29, 2019

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
Copy link
Contributor Author

sbraz commented Apr 2, 2019

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
Copy link

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
Copy link
Owner

jaraco commented Oct 27, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants