extensions/sasl-3.1: Fix when to send 904 #408
Draft
Conversation
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
It's been pointed out to me that servers often send an 908 numeric to the client, but then also send an 904. When looking at this behavior, I think this is so is because servers are generally built by default with 904 no matter what failed and then later, the 908 functionality is implemented. In order to keep compatibility with clients that may not understand the 908 numeric but do understand the 904 numeric as a generic "retry". However, some clients do understand a 908 and use that as a way to say "downgrade the mechanism and try again". So, they can begin reconnecting as of the 908 numeric, but then get sent a 904 numeric, which some clients also take as a "try again" and do not blindly consume the message. The fix for the client would be to keep a state of which related numerics are sent to the server, then begin the next attempt once the 904 numeric is received. This would *still work* with this proposed change. This also means that any numerics added in the future are compatible with this specification, as the numerics will need to be sent before the 904 numeric which the client can use to restart the authentication with any context sent from the other numerics. --- I also noticed that there was only example of a "failed" case. I have manually added one. I will be reattempting it sometime in the future to get an actual response, I was just being ~~really lazy~~ busy writing the example. I will also go attempt to get further examples of numerics that are not currently covered by examples.
grawity
reviewed
Apr 21, 2020
Member
|
First bit and example looks good. Actually had a bug around this in IRCCloud that I worked through with amdj/atheme a few months ago. |
Contributor
Author
|
TODO split RPL_ and ERR_ into sections, rewrite to say that 904 should be sent in addition to RPL_ messages, NOT other ERR_ messages. |
Member
|
@RyanSquared are you able to finish this up? If not we can try get someone else to take it over. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
It's been pointed out to me that servers often send an 908 numeric to
the client, but then also send an 904. When looking at this behavior, I
think this is so is because servers are generally built by default with
904 no matter what failed and then later, the 908 functionality is
implemented. In order to keep compatibility with clients that may not
understand the 908 numeric but do understand the 904 numeric as a
generic "retry". However, some clients do understand a 908 and use that
as a way to say "downgrade the mechanism and try again". So, they can
begin reconnecting as of the 908 numeric, but then get sent a 904
numeric, which some clients also take as a "try again" and do not
blindly consume the message.
The fix for the client would be to keep a state of which related
numerics are sent to the server, then begin the next attempt once the
904 numeric is received. This would still work with this proposed
change.
This also means that any numerics added in the future are compatible
with this specification, as the numerics will need to be sent before the
904 numeric which the client can use to restart the authentication with
any context sent from the other numerics.
I also noticed that there was only example of a "failed" case. I have
manually added one. I will be reattempting it sometime in the future to
get an actual response, I was just being
really lazybusy writingthe example. I will also go attempt to get further examples of numerics
that are not currently covered by examples.
I do know the branch is called 804. It is a typo.