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

mention reading RPL_WELCOME to get the assigned nick #146

Merged
merged 3 commits into from
Jan 7, 2022

Conversation

slingamn
Copy link
Contributor

No description provided.

@progval
Copy link
Member

progval commented Nov 28, 2021

Thanks!

What about moving this paragraph from the "Numerics" section to "Connection registration", so it is more visible? I doubt new client authors will read the details of each numerics, especially as this one looks straightforward

@DanielOaks
Copy link
Member

Noice \o/

@progval progval added the feedback wanted We need to make sure this is correct label Dec 3, 2021
emersion added a commit to emersion/soju that referenced this pull request Dec 6, 2021
@slingamn
Copy link
Contributor Author

slingamn commented Dec 6, 2021

ping

@progval
Copy link
Member

progval commented Dec 6, 2021

@slingamn In this repo usually wait 2-4 weeks to give people time for feedback

Copy link
Contributor

@emersion emersion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tommyrot
Copy link
Contributor

tommyrot commented Dec 6, 2021

The server typically responds to rejected NICK messages with ERR_ERRONEUSNICKNAME. It seems this is purely about how to deal with a server assigned nick as a client? Is that new functionality? I'm sceptical about this suggested change because many clients do not currently parse RPL_WELCOME for their actual nick and the change would bring an assumption that isn't backward compatible.

@dgw
Copy link

dgw commented Dec 6, 2021

@tommyrot, Hybrid and Plexus both truncate the nick to NICKLEN before validity checking. Likely others do, too. (Code search is tedious on mobile.)

I've already tested (against live servers) this behavior in my bot, just for the convenience of its raw log output. Specifying a longer-than-NICKLEN nickname results in the server sending back that nick truncated to NICKLEN in 001 RPL_WELCOME, as described in the added prose here.

While I can't speak to how long this behavior has been true, it doesn't appear to be new. I'm glad it's been brought up, because it'll let me fix some code that doesn't take this into account.

@tommyrot
Copy link
Contributor

tommyrot commented Dec 6, 2021

NICKLEN is a constraint on the validity of a nick and should result in a transparant ERR_ERRONEUSNICKNAME to the client. I think it's a weird choice to silently truncate but can see how it could have been conceived: a client is at fault for requesting a nick longer than NICKLEN but during registration it doesn't know of the NICKLEN value yet. I'm guessing. Still doesn't make it right.

@dgw
Copy link

dgw commented Dec 6, 2021

but during registration it doesn't know of the NICKLEN value yet. I'm guessing.

Accurate guess: NICKLEN is part of 005 RPL_ISUPPORT, which the client doesn't receive until after registration.

It's a fair point that this seems like a weird choice. If you have an idea for improving the spec, I'd suggest popping into #ircv3 at Libera, or opening an issue at https://github.com/ircv3/ircv3-ideas. This repo exists to document what IRCds already do; if you want to see the underlying specs changed, you have to go through the working group that ratifies them. 😺

@tommyrot
Copy link
Contributor

tommyrot commented Dec 6, 2021

Did some testing as well. Inspircd, ngircd and ergo all respond with ERR_ERRONEUSNICKNAME during registration when using a nick that exceeds NICKLEN. Unrealircd and solanum truncate the nick to NICKLEN in which case RPL_WELCOME is indeed the first message to reflect that. Adding hybrid to the servers that truncate (as @dgw tested) probably makes this about half of all out there.

PR is OK

@slingamn
Copy link
Contributor Author

slingamn commented Dec 6, 2021

FWIW, one of the cases that's important to Ergo is that in the default/recommended configuration, if the client authenticates with SASL, we ignore the nickname sent on the NICK line and instead coerce the nickname to be equal to the SASL account name:

https://github.com/ergochat/ergo/blob/v2.8.0/default.yaml#L498-L502

and the primary means of communicating that this happened is RPL_WELCOME.

k4bek4be pushed a commit to k4bek4be/gamja that referenced this pull request Dec 23, 2021
@progval progval merged commit 547ec0b into ircdocs:master Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback wanted We need to make sure this is correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants