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

Sopel blindly requests unadvertised CAPs #972

Closed
progval opened this issue Dec 19, 2015 · 2 comments · Fixed by #2341
Closed

Sopel blindly requests unadvertised CAPs #972

progval opened this issue Dec 19, 2015 · 2 comments · Fixed by #2341

Comments

@progval
Copy link

progval commented Dec 19, 2015

Hi,

I noticed Sopel requests multi-prefix, even if it is not in the CAP LS response.
This is technically valid according to the spec, but you have code in coretask.py that looks supposed to filter capabilities in such cases. I guess its purpose is to avoid getting a CAP NAK if a single capability in the list is not available.

raw log file:

>>1450516159.3153656    CAP LS 302
>>1450516159.3155537    NICK Sopel
>>1450516159.315693 USER sopel +iw Sopel :Sopel: http://sopel.chat
<<1450516159.3162677    CAP * LS :
>>1450516159.31694  CAP REQ multi-prefix
>>1450516159.3170621    CAP END

Version: 047a99e (Git master at the time I am writing this)

@embolalia
Copy link
Contributor

Thanks for these bug reports. I imagine the same is probably true of the additional CAPs requested in #961, too.

@dgw dgw changed the title multi-prefix capability is requested even if it is not available Sopel blindly requests unadvertised CAPs Jun 22, 2021
@dgw dgw added this to the 8.0.0 milestone Jun 22, 2021
@dgw
Copy link
Member

dgw commented Jun 22, 2021

Title change reason: Embo was right that it isn't just multi-prefix, as is easily demonstrated by the following raw log snippet from an unrelated test session I ran earlier today:

2021-06-22 17:29:58,308 >>      CAP LS 302
2021-06-22 17:29:58,309 >>      NICK SopelTest
2021-06-22 17:29:58,309 >>      USER sopel 0 * :Sopel: https://sopel.chat/
    <snip irrelevant NOTICEs>
2021-06-22 17:29:59,399 <<      :irc.sxci.net CAP * LS :away-notify chghost invite-notify multi-prefix sasl userhost-in-names
2021-06-22 17:29:59,400 >>      CAP REQ echo-message
2021-06-22 17:29:59,400 >>      CAP REQ multi-prefix
2021-06-22 17:29:59,400 >>      CAP REQ away-notify
2021-06-22 17:29:59,400 >>      CAP REQ cap-notify
2021-06-22 17:29:59,400 >>      CAP REQ server-time
2021-06-22 17:29:59,400 >>      CAP REQ account-notify
2021-06-22 17:29:59,400 >>      CAP REQ extended-join
2021-06-22 17:29:59,400 >>      CAP REQ account-tag
2021-06-22 17:29:59,400 >>      CAP END
2021-06-22 17:29:59,410 <<      :irc.sxci.net CAP SopelTest NAK :echo-message
2021-06-22 17:29:59,421 <<      :irc.sxci.net CAP SopelTest ACK :multi-prefix
2021-06-22 17:29:59,422 <<      :irc.sxci.net CAP SopelTest ACK :away-notify
2021-06-22 17:29:59,422 <<      :irc.sxci.net CAP SopelTest NAK :cap-notify
2021-06-22 17:29:59,422 <<      :irc.sxci.net CAP SopelTest NAK :server-time
2021-06-22 17:29:59,422 <<      :irc.sxci.net CAP SopelTest NAK :account-notify
2021-06-22 17:29:59,423 <<      :irc.sxci.net CAP SopelTest NAK :extended-join
2021-06-22 17:29:59,423 <<      :irc.sxci.net CAP SopelTest NAK :account-tag

Capability negotiation is definitely due for an overhaul. I, and perhaps also @Exirel, have already started thinking about it for 8.0. It would also be nice to CAP REQ all of the things Sopel wants into one request, which is only practical if Sopel does not request unavailable CAPs. ("The capability identifier set must be accepted as a whole, or rejected entirely." — CAP REQ spec)

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

Successfully merging a pull request may close this issue.

4 participants