Skip to content

CAP: send NEW/DEL to cap-notify clients that are still registering - #124

Closed
MrIron-no wants to merge 1 commit into
UndernetIRC:mainfrom
MrIron-no:fix/capnew
Closed

MrIron-no wants to merge 1 commit into
UndernetIRC:mainfrom
MrIron-no:fix/capnew

Conversation

@MrIron-no

Copy link
Copy Markdown
Contributor

Problem

cap_new() and cap_del() skip every connection that is not yet IsUser. A client that has sent CAP LS 302 but 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 LS shows no sasl, the SASL server becomes reachable a moment later — before they are registered — and the CAP NEW :sasl=... is dropped. They register and never learn that sasl exists.

Spec

capability-negotiation:

  • "The NEW subcommand signals that the server supports one or more new capabilities, and may be sent at any time."
  • cap-notify "MUST be implicitly enabled if the client requests CAP LS with 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".
  • "Replies from the server must contain the client identifier name or asterisk if one is not yet 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 both cap_new() and cap_del(): any local user connection — registered, or unregistered on a user/websocket port (the same test cap_ls()/cap_req() use for "registration not complete") — that has cap-notify active. %C already yields * for a client without a nick.

Test

tests/pr66_capsasl/test_cap_notify_registering.py: a client sends CAP LS 302 + NICK/USER without CAP END, the SASL server links and is configured, the client must receive CAP NEW :sasl=PLAIN and be able to CAP REQ :sasl before registering. Fails on main without the fix, passes with it; tests/pr66_capsasl + tests/cap otherwise unchanged (44 passed).

The DEL direction for a registering client is not asserted here: on main, exit_one_client() re-evaluates SASL availability while the exiting server is still in server_list, so no DEL is sent when the SASL server itself leaves — that is addressed by #100.

…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)
@MrIron-no

Copy link
Copy Markdown
Contributor Author

Folded into #100 as 3306538: that branch already carries the SASL availability fixes and the P10Server burst helpers, so the test can assert the DEL direction too (not possible on main alone, see the note above) and no second, overlapping test file is needed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant