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

Simplify max nicklen checks for hostmask parsing #523

Merged
merged 1 commit into from Dec 12, 2018

Conversation

@jwheare
Copy link
Contributor

@jwheare jwheare commented Nov 8, 2018

This removes checking isupport NICKLEN for isHostmaskNicknameOn (used for sender/prefix parsing) in all cases.

Reasons:

The most serious issue here is for servers with ISUPPORT but no NICKLEN key, with nicks longer than 9 chars, where parsing just silently breaks and stuff like server-initiated JOINs fail to register.

This now always uses a limit of 50, which is arbitrary but probably a safe enough sanity check.

This removes checking isupport NICKLEN for `isHostmaskNicknameOn` (used for sender/prefix parsing) in all cases.

Reasons:
* This was explicitly disabled in several cases anyway (missing isupport entirely, znc)
* This would fallback to a default of 9 when no NICKLEN was provided in isupport, but a default of 50 when explicitly disabled
* NICKLEN is not strictly the maximum length of a nickname on an IRC server. See:
** https://tools.ietf.org/html/draft-hardy-irc-isupport-00#section-4.14 "A client elsewhere on the network MAY use a nick length of higher value"
** https://tools.ietf.org/html/draft-brocklesby-irc-isupport-03#section-3.13 "   This parameter does not restrict the length of any nicknames other clients on the network may use"
** https://defs.ircdocs.horse/defs/isupport.html#nicklen "Other clients on the network may have nicknames longer than this."

The most serious issue here is for servers *with* ISUPPORT but no NICKLEN key, with nicks longer than 9 chars, where parsing just silently breaks and stuff like server-initiated JOINs fail to register.

This now always uses a limit of 50, which is arbitrary but probably a safe enough sanity check.
@emsquared emsquared merged commit 2d82787 into Codeux-Software:master Dec 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants