solanum-ircd / solanum

An IRCd for unified networks
https://solanum.chat/
GNU General Public License v2.0
216 stars 53 forks source link

Incorrect multi-prefix handling on WHOIS #431

Closed JustAnotherArchivist closed 2 weeks ago

JustAnotherArchivist commented 3 weeks ago

Prior to #430, Solanum was always returning all prefixes, even if a client had not enabled the multi-prefix capability. However, it turned out that the fix there was incorrect due to remote WHOIS not having access to client capabilities, so 746ced23 disabled it altogether and always only returns the highest prefix.

The desired behaviour, of course, is to send the highest prefix to clients without multi-prefix and all prefixes to those with.

jillest commented 2 weeks ago

Keep in mind what the purpose of the multi-prefix capability is: not breaking old clients while still allowing @+ combinations as much as possible. Sending @+ in WHOIS was implemented long before the multi-prefix capability. Apparently, old ircd developers had found that sending @+ in WHOIS broke few or no clients, while sending @+ in NAMES and WHO did break some clients. This does not seem entirely surprising to me since NAMES and WHO replies are often processed automatically while WHOIS replies tend to be merely displayed.

Since WHOIS has sent @+ for so long, making it follow multi-prefix will not make any meaningful old clients work. Any actively developed clients will support multi-prefix, so the proposed change will not help them.

Also note that WHO will send @+ if WHOX flags are used. That feature was likewise originally implemented (in ircu) before multi-prefix existed.

I think this issue should be closed without any code change.

dwfreed commented 2 weeks ago

I think this issue should be closed without any code change.

It's a bit late for that. The reversion, instead of going back to the old behavior, changed it so WHOIS never uses multiple prefixes, regardless of capabilities or anything else.

JustAnotherArchivist commented 2 weeks ago

432 reverted this now and claims that sending all prefixes even if the client hasn't enabled them has 'no known impact on clients that exist today'. I definitely ran into spec compliance walls just like this (albeit not this exact one) when I implemented my own IRC client (bot) from scratch based on the RFC and modern.ircdocs.horse, neither of which allow/document multiple prefixes in RPL_WHOISCHANNELS. I don't think willfully violating the spec is a good idea. But if this is intentional behaviour across independent ircds, it should be documented on modern.ircdocs.horse.

dwfreed commented 2 weeks ago

Please keep in mind that the RFCs are horribly outdated, and often aren't useful for much more than a general idea on how the protocol works. All ircds do many things that don't comply with the RFCs, which is why ircdocs was created. But ircdocs is also a work in progress, so just because it's not documented there doesn't mean it doesn't happen. I opened ircdocs/modern-irc#239 to request that it be updated to reflect that WHOIS may always be multi-prefix. Notably this also happens in oftc-hybrid, so this behavior is not without precedent.