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

multiline responses to CAP LS 302 are not parsed correctly #8

Closed
slingamn opened this issue Oct 12, 2020 · 12 comments
Closed

multiline responses to CAP LS 302 are not parsed correctly #8

slingamn opened this issue Oct 12, 2020 · 12 comments
Labels

Comments

@slingamn
Copy link

I'm using Revolution 0.5.2 (from F-Droid) against testnet.oragono.io. I created an account revolution, with password revolutionpassphrase, for testing:

-> : :oragono.test NOTICE * :*** Looking up your username
-> : :oragono.test NOTICE * :*** Could not find your username
<- : CAP LS 302
-> : :oragono.test CAP * LS * :account-notify account-tag away-notify batch cap-notify chghost draft/channel-rename draft/chathistory draft/event-playback draft/languages=16,en,~bs,~de,~el,~en-AU,~es,~fi,~fr-FR,~it,~nl,~no,~pl,~pt-BR,~ro,~tr-TR,~zh-CN draft/multiline=max-bytes=4096,max-lines=24 draft/resume-0.5 echo-message extended-join invite-notify labeled-response message-tags multi-prefix oragono.io/nope sasl=PLAIN,EXTERNAL server-time setname sts=duration=86400,port=6697 userhost-in-names znc.in/playback
-> : :oragono.test CAP * LS znc.in/self-message
-> : :oragono.test NOTICE * :*** Looking up your hostname...
-> : :oragono.test NOTICE * :*** Found your hostname
<- : NICK revolution
<- : USER revolution 0 * :revolution
<- : CAP END
-> : :oragono.test 433 * revolution :Nickname is reserved by a different account
<- : CAP REQ :znc.in/self-message
-> : :oragono.test CAP * ACK znc.in/self-message
[connection hangs]

This is a multiline reply to CAP LS 302. It looks like Revolution is only parsing the final line (CAP * LS znc.in/self-message), so it doesn't realize that the server supports SASL and is unable to send SASL. This causes the handshake to stall (because in our server implementation, SASL is required to claim a registered nickname).

Thanks for your time!

@MCMrARM MCMrARM added the bug label Oct 12, 2020
@MCMrARM
Copy link
Owner

MCMrARM commented Nov 15, 2020

Looks like I misunderstood/wrongly implemented the handling and interpreted as if if should be * LS and not LS * (

if (subcmd.equals("*")) {
expectMore = true;
subcmd = CommandHandler.getParamWithCheck(params, ++baseSubcmdI);
}
)

@MCMrARM
Copy link
Owner

MCMrARM commented Jan 5, 2021

Fixed with 579d7fe

@MCMrARM MCMrARM closed this as completed Jan 5, 2021
@MCMrARM
Copy link
Owner

MCMrARM commented Jan 5, 2021

Note that connecting oragono.io now fails with:

I/System.out: Sent: CAP LS 302
I/System.out: Sent: NICK test
    Sent: USER test 0 * :test
    Sent inital commands
I/System.out: Got: :oragono.test NOTICE * :*** Looking up your username
I/System.out: Got: :oragono.test NOTICE * :*** Could not find your username
I/System.out: Got: :oragono.test CAP * LS * :account-notify account-tag away-notify batch cap-notify chghost draft/channel-rename draft/chathistory draft/event-playback draft/languages=16,en,~bs,~de,~el,~en-AU,~es,~fi,~fr-FR,~it,~nl,~no,~pl,~pt-BR,~ro,~tr-TR,~zh-CN draft/multiline=max-bytes=4096,max-lines=24 draft/register=before-connect draft/relaymsg=/ draft/resume-0.5 echo-message extended-join invite-notify labeled-response message-tags multi-prefix oragono.io/nope sasl=PLAIN,EXTERNAL server-time setname
I/System.out: Got: :oragono.test CAP * LS :sts=duration=86400,port=6697 userhost-in-names znc.in/playback znc.in/self-message
I/System.out: Sent: CAP REQ :batch cap-notify multi-prefix sasl server-time znc.in/self-message
I/System.out: Got: @time=2021-01-05T16:06:19.220Z :oragono.test CAP * ACK :batch cap-notify multi-prefix sasl server-time znc.in/self-message
I/System.out: Sent: AUTHENTICATE PLAIN
I/System.out: Got: AUTHENTICATE +
I/System.out: Sent: AUTHENTICATE cmV2b2x1dGlvbgByZXZvbHV0aW9uAHJldm9sdXRpb25wYXNzcGhyYXNl
I/System.out: Got: @time=2021-01-05T16:06:19.371Z :oragono.test 900 * * revolution :You are now logged in as revolution
I/System.out: Got: @time=2021-01-05T16:06:19.371Z :oragono.test 903 * :Authentication successful
I/System.out: Sent: CAP END
I/System.out: Got: @time=2021-01-05T16:06:19.457Z :oragono.test 400 * NICK :Could not set or change nickname
I/System.out: Got: ERROR :You can't mix secure and insecure connections to this account

and to be honest, I'm not sure if I am doing something wrong here?

@slingamn
Copy link
Author

slingamn commented Jan 5, 2021

Is this on a local instance or on the testnet?

If it's the testnet, try using TLS on 6697 instead; if a local instance (and you're not connecting on loopback), try adding the IP range you're connecting from to secure-nets:

https://github.com/oragono/oragono/blob/0db4b42ffe178a8267fd82f3e6ffbe86ccdf2139/default.yaml#L334

@slingamn
Copy link
Author

slingamn commented Jan 5, 2021

(Ah, this is the testnet --- you need to use TLS because your account is set to always-on.)

@MCMrARM
Copy link
Owner

MCMrARM commented Jan 6, 2021

Okay you're right - I could connect w/o issues now. Thanks.

@MCMrARM
Copy link
Owner

MCMrARM commented Jan 6, 2021

By the way, one more issue I noticed is that channel list breaks, as there is no RPL_LISTSTART message sent:

01-06 13:30:37.208 10610 26435 I System.out: Sent: LIST 
01-06 13:30:37.231  8585  8585 I GoogleInputMethodService: GoogleInputMethodService.onFinishInput():3308 
01-06 13:30:37.232  8585  8585 I GoogleInputMethodService: GoogleInputMethodService.onStartInput():1887 
01-06 13:30:37.267 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #healthcare 1 :
01-06 13:30:37.268 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #space 1 :
01-06 13:30:37.269 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #oowifi 1 :
01-06 13:30:37.270 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #test 3 :
01-06 13:30:37.272 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #ircwifi 1 :
01-06 13:30:37.273 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution # 1 :
01-06 13:30:37.274 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #roleplay 1 :
01-06 13:30:37.275 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #jwheare 1 :
01-06 13:30:37.276 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #physics 1 :
01-06 13:30:37.278 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #bitbot 1 :
01-06 13:30:37.314 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #math 1 :
01-06 13:30:37.316 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #ChatLand 1 :
01-06 13:30:37.317 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #cy 1 :
01-06 13:30:37.318 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #subline 1 :
01-06 13:30:37.319 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #vaccine 1 :
01-06 13:30:37.320 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #pokey 1 :
01-06 13:30:37.322 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 322 revolution #chat 7 :this channel is not monitored; send any bug reports to #oragono on Freenode or our GitHub
01-06 13:30:37.323 10610 26439 I System.out: Got: @time=2021-01-06T12:30:37.454Z :oragono.test 323 revolution :End of LIST

@slingamn
Copy link
Author

slingamn commented Jan 6, 2021

Ah, yes. We have an open issue about it (ergochat/ergo#1335) but I haven't addressed it yet, due to my understanding that this numeric is now deprecated:

  1. https://modern.ircdocs.horse/#list-message
  2. https://tools.ietf.org/html/rfc2812#page-45

@slingamn
Copy link
Author

slingamn commented Jan 6, 2021

Haven't tested, but this should work:

diff --git a/chatlib/src/main/java/io/mrarm/chatlib/irc/handlers/ListCommandHandler.java b/chatlib/src/main/java/io/mrarm/chatlib/irc/handlers/ListCommandHandler.java
index f6fc420..c47c516 100644
--- a/chatlib/src/main/java/io/mrarm/chatlib/irc/handlers/ListCommandHandler.java
+++ b/chatlib/src/main/java/io/mrarm/chatlib/irc/handlers/ListCommandHandler.java
@@ -34,8 +34,11 @@ public class ListCommandHandler implements CommandDisconnectHandler {
         }
         if (request == null)
             throw new InvalidMessageException("Channel list entry without a request context");
-        if (numeric == RPL_LISTSTART) {
+        if (request.entries == null) {
             request.entries = new ArrayList<>();
+        }
+        if (numeric == RPL_LISTSTART) {
+            // ignore
         } else if (numeric == RPL_LISTEND) {
             ChannelList resp = new ChannelList(currentRequest.entries);
             request.retVal.set(resp);
@@ -43,8 +46,6 @@ public class ListCommandHandler implements CommandDisconnectHandler {
                 request.callback.onResponse(resp);
             handleNextRequest(connection);
         } else if (numeric == RPL_LIST) {
-            if (request.entries == null)
-                throw new InvalidMessageException("Channel list entry without a list start message");
             ChannelList.Entry entry;
             try {
                 entry = new ChannelList.Entry(CommandHandler.getParamWithCheck(params, 1),

@MCMrARM
Copy link
Owner

MCMrARM commented Jan 7, 2021

Okay. I opted for a different way to fix this, and this is addressed in 7025570

@slingamn
Copy link
Author

slingamn commented Jan 7, 2021

Awesome. Just checking, does that work if you receive RPL_LISTEND with no RPL_LIST before it (as you would if there were no channels)?

@MCMrARM
Copy link
Owner

MCMrARM commented Jan 7, 2021

Not sure how the client app handles a null list at the moment, so I corrected it so the list never is null since it's in general safer for the library to do that.

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

No branches or pull requests

2 participants