Conversation
…tering cap_new() and cap_del() skipped every connection that was not yet IsUser. A client that sent CAP LS 302 while its server was still bursting with the network saw no sasl in the listing, the path to the SASL server completed before it registered, and the NEW was dropped: it registered without ever learning that sasl exists. This is what clients of a restarted leaf hit when they reconnect while the leaf links. cap-notify is enabled by negotiation (implicitly by CAP LS 302), not by registration, and the spec lets NEW be "sent at any time" with "*" as the target when no nick is available yet. Notify any local user connection, registered or not, that has cap-notify active. Test: a registering client receives NEW and can REQ the capability before CAP END. (cherry picked from commit e0aaf35; tests adapted to the main harness)
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
cap_new()andcap_del()skip every connection that is not yetIsUser. A client that has sentCAP LS 302but has not finished registering when a capability appears is never told about it.In practice this hits the clients of a restarted (or re-linking) leaf: they reconnect while the leaf is still linking to the network,
CAP LSshows nosasl, the SASL server becomes reachable a moment later — before they are registered — and theCAP NEW :sasl=...is dropped. They register and never learn that sasl exists.Spec
capability-negotiation:
CAP LSwith a version of 302 or newer", and "When enabled, the server MUST notify the client about all new capabilities and about existing capabilities that are no longer available".cap-notify is enabled by negotiation, not by registration, so the
IsUser()filter was a deviation.Fix
A
cap_notify_target()helper used by bothcap_new()andcap_del(): any local user connection — registered, or unregistered on a user/websocket port (the same testcap_ls()/cap_req()use for "registration not complete") — that has cap-notify active.%Calready yields*for a client without a nick.Test
tests/pr66_capsasl/test_cap_notify_registering.py: a client sendsCAP LS 302+ NICK/USER withoutCAP END, the SASL server links and is configured, the client must receiveCAP NEW :sasl=PLAINand be able toCAP REQ :saslbefore registering. Fails onmainwithout the fix, passes with it;tests/pr66_capsasl+tests/capotherwise unchanged (44 passed).The
DELdirection for a registering client is not asserted here: onmain,exit_one_client()re-evaluates SASL availability while the exiting server is still inserver_list, so noDELis sent when the SASL server itself leaves — that is addressed by #100.