NewNick handler needs a max length check

[purpleidea]

If you use the NewNick handler and return a nick with a length that's longer than 16 chars, the whole lib breaks, in particular the NAMES/353 responses no longer work and you get ghosts in your code.

Ask me how I know...

There should be a doc patch to mention this, and probably some sort of error propagation in here.

It's possible that 16 is a server specific limit-- I just found this empirically when using Freenode.

HTH's someone else.

[eviltwin]

Fun. Buckle up, let's read some RFCs together.

RFC 2812 specifies in Section 1.2.1 that a "user is distinguished from other users by a unique nickname having a maximum length of nine (9) characters." and further states "While the maximum length is limited to nine characters, clients SHOULD accept longer strings as they may become used in future evolutions of the protocol." Finally, if we look to the protocol grammar specification in Section 2.3.1 we can see that anywhere in the protocol that they mention a token of type nickname, it is defined thusly:

nickname   =  ( letter / special ) *8( letter / digit / special / "-" )
...
digit      =  %x30-39                 ; 0-9
special    =  %x5B-60 / %x7B-7D
                  s ; "[", "]", "\", "`", "_", "^", "{", "|", "}"

Good times. We can therefore infer that your particular server is indeed relying on the whole "future evolutions of the protocol" jam. A quick google search reveals that some ircds even make this admin-configurable: https://askubuntu.com/a/578614

Great.

I tried and failed to find any documentation on servers advertising an extension to the default nickname length. Looks like you basically have to adopt the "try it and see" mentality.

Love it.

So, NewNick is used to generate a new nickname, which we then dispatch a NICK command to acquire. You can check out Section 3.1.2 for details on the NICK command, but basically it uses the definition of the nickname token above. It does, however, list a few error codes that one might expect to see.

ERR_NICKNAMEINUSE seems most pertinent, for which you will receive the numeric message 433 from the server. See Section 5.2 for details on that stuff.

So, in conclusion, if we wanted to validate the length of nicknames then the latest specifications suggest that 9 characters is the way to go, but it seems to be that most servers these days have extended that and clients are expected to Just Know(TM) what their new limit is. In the case of interactive clients, obviously there are actually very few situations where a new nickname needs to be chosen automatically by the client (probably just on connection) but in the case of bots (which is honestly all this lib is used for) we do need to be able to reliably pick a new name each time, every time.

Perhaps we need some handler for numeric 433, and a limit of how many times we'll call NewNick in a row (to prevent problems with infinitely spinning on choosing a nick) before we finally generate one that's DEFINITELY safe (or just die).

Worth saying, I haven't contributed anything to this project in years and I think the only reason that @fluffle keeps me around is because I do stupid things with my evenings like read 19 year old protocol grammars.

[purpleidea]

The bug isn't that servers accept short names, it's that this library behaves erratically when someone exceeds the name length. If you try my code with a long nick, just before this commit: purpleidea/ircdns@c8eb09f then you'll see all sorts of weird bugs appear.

HTH

[fluffle]

Thanks for the bug report @purpleidea! And thanks for the RFC reading @eviltwin, you always knew how to party hard ;-p

Embarassingly this made me realise that I'd not pushed the go.mod / go.sum files I created over a year ago to master.

A simple "fix" is to swap out the very simple default NewNick implementation for one which doesn't make the nick longer. I can push something that does that tonight.

The "correct" fix is more involved. There's a longstanding TODO in handlers.go to implement some sort of basic support for 005 server messages, in which the server describes it's behaviour and configuration. The pertinent key for this particular issue is NICKLEN, which freenode sets to 16. Once this information is available to the library it can make more intelligent decisions about nicks in various places. Pull requests welcome ;-)

[fluffle]

FWIW, while I appreciate the bug report, some debug logs demonstrating the problem and the server<-> client communication in its entirety would have been very welcome when addressing this issue. Rather than the not-so-cryptic "ask me how I know" you could have shown how you know in ways that help me also know, rather than forcing me to reverse-engineer the problem. The former approach is not particularly respectful of other people's time, and I had computer games I was intending to play tonight with this tasty glass of whisky :-)

[purpleidea]

Sorry about that. The event handlers started returning weird nil values, as you can see in the comments in my code. I thought it was more obvious, I should have pointed it out more explicitly sorry. I've switched to a different IRC lib anyways, so I'm going to bow out of this discussion. You can close if you'd like. Thanks!

…

On Thu, Oct 17, 2019 at 3:00 PM Alex Bee ***@***.***> wrote: FWIW, while I appreciate the bug report, some debug logs demonstrating the problem and the server<-> client communication in its entirety would have been very welcome when addressing this issue. Rather than the not-so-cryptic "ask me how I know" you could have shown how you know in ways that help me also know, rather than forcing me to reverse-engineer the problem. The former approach is not particularly respectful of other people's time, and I had computer games I was intending to play tonight with this tasty glass of whisky :-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[fluffle]

Hey, I'm still grateful you filed the report, it was definitely a bug. I'm sorry you had to switch libraries in the meantime. I just don't spend much time coding these days, I scratch that itch at work instead. I only wanted to remind you that it's important to remember that the more information you give in your bug report, the easier it is for the person responding to deal with it :-)

Anyway, I just pushed the "easy" fix and tagged 1.0.2 so hopefully future people won't run into this as easily.