1309063 - Error connecting to an IRC v3 server with an invalid capability

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Vincent Le Goff]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160922113459

Steps to reproduce:

I have created a project on Planio, accessible at cocomud.plan.io.  The project I created has the team chat included, so I should be able to connect to the server using cocomud.plan.io both as username and server hostname (I also have a password).  I can do so with other clients, as a matter of fact.

When trying to connect on the port 6697 (with SSL) using Instantbird to this account, it is displayed as "connected", but the log shows errors (see below).



Actual results:

[10/10/2016 4:59:12 PM] ERROR (@ prpl-irc: ircHandlers._handleMessage resource://gre/modules/ircCAP.jsm:47)
Error running command CAP with handler Client Capabilities:
{"rawMessage":":Instantbird CAP * LS :sasl server-time znc.in/server-time-iso znc.in/playback palaverapp.com","command":"CAP","params":["*","LS","sasl server-time znc.in/server-time-iso znc.in/playback palaverapp.com"],"origin":"Instantbird","tags":{},"source":"","cap":{"subcommand":"LS"}}
matches is null
[10/10/2016 4:59:12 PM] WARN. (@ prpl-irc: ircSocket.prototype.onDataReceived resource://gre/components/irc.js:680)
Unhandled IRC message:
:Instantbird CAP * LS :sasl server-time znc.in/server-time-iso znc.in/playback palaverapp.com


Expected results:

Normal connection.

Comment 1

[Martin Giger [:freaktechnik]]

This error happens because of the "palaverapp.com" capability, which is not a valid capability according to the IRCv3 spec. It should be palaverapp.com/capabilityname. How instantbird should deal with that is not my decision, but here's the RegExp that fails on palaverapp.com: https://dxr.mozilla.org/comm-central/rev/e2aac779fb822973a141a0f2f2d42840ee563bae/chat/protocols/irc/ircCAP.jsm#26

Comment 2

[Patrick Cloke [:clokep]]

We should just treat these as strings, no reason to verify the content. I think the regex is trying to get the capability modifier off, but that seems like a poor implementation.

Comment 3

[Patrick Cloke [:clokep]]

The result of discussing this with IRCv3 people: https://github.com/ircv3/ircv3-specifications/pull/274

Comment 4

[Patrick Cloke [:clokep]]

Attachment: Patch v1

This should fix the parsing issue and adds some tests. I'm going to add IRCv3.2 support in a separate patch.

Comment 5

[Martin Giger [:freaktechnik]]

Comment on attachment 8802487 [details] [diff] [review]
Patch v1

Review of attachment 8802487 [details] [diff] [review]:
-----------------------------------------------------------------

There is nothing actually testing if the modifiers get stripped, and I think it's worth adding a test case for that.

::: chat/protocols/irc/ircCAP.jsm
@@ +46,4 @@
>  
> +    // If there's a modifier...pull it off. (This is pretty much unused, but we
> +    // have to pull it off for backward compatibility.)
> +    if (['-', '=', '~'].some((c) => aParameter.startsWith(c))) {

I think it would be faster to do something like '-=~'.includes(aParameter[0]), even though equivalent in function it would avoid creating an array and function on the fly. I am also unsure if startsWith is to be preferred over an equality comparison in this case, since it's just a single character.

@@ +49,5 @@
> +    if (['-', '=', '~'].some((c) => aParameter.startsWith(c))) {
> +      message.cap.modifier = aParameter[0];
> +      aParameter = aParameter.substr(1);
> +    } else
> +      message.cap.modifier = null;

This would've been undefined and not null with the old code. I don't know if any code actually does an exact check, but keeping the value the same seems safer to me.

::: chat/protocols/irc/test/test_ircCAP.js
@@ +73,5 @@
> +  for (let i = 0; i < a.length; ++i)
> +    cmp(a[i], b[i]);
> +}
> +
> +function equal_objects(a, b) {

It seems there is a deepEqual somewhere in the test suite already: https://dxr.mozilla.org/comm-central/rev/6d80e3a2d552dd5a603cc9f5fbc5321085d93cd3/chat/protocols/xmpp/test/test_dnsSrv.js#112, which I think is the deepEqual from https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Assertions_and_logging
Typically deepEqual would also work with Arrays.

@@ +105,5 @@
> +    if (!Array.isArray(expectedCaps))
> +      expectedCaps = [expectedCaps];
> +
> +    // Add defaults to the expected output.
> +    expectedCaps.forEach((cap) => {

I guess it's kind of random whether to use for...of or forEach() on arrays?

@@ +108,5 @@
> +    // Add defaults to the expected output.
> +    expectedCaps.forEach((cap) => {
> +      // By default there's no modifier.
> +      if (!('modifier' in cap))
> +        cap.modifier = null;

This would've been undefined with the RegExp based code. I guess the modifier should be checked with strictEqual to ensure it is undefined if unset.

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Thanks for the review! I think I've taken all your comments into account.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8802515 [details] [diff] [review]
Patch v2

This has a minor bug in it.

Comment 8

[Patrick Cloke [:clokep]]

Attachment: Patch v2.1

My last version had a minor bug in it that caused connections to fail. This fixes it (and reverts some code that there's no reason I should have touched).

Comment 9

[Martin Giger [:freaktechnik]]

Comment on attachment 8802517 [details] [diff] [review]
Patch v2.1

Review of attachment 8802517 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good now and tests pass for me.

I wonder if the error with v2 should be tested, but that doesn't block this patch for sure.

Comment 10

[Patrick Cloke [:clokep]]

Attachment: Patch v2.2

Adds an additional test condition to avoid the failure in Patch v2.

Comment 11

[Patrick Cloke [:clokep]]

Thanks!

https://hg.mozilla.org/comm-central/rev/dda2ecc3395d57d165034a2687074da0455bf121

Comment 13

[Patrick Cloke [:clokep]]

jorgk: Just had a report of this on TB 45.6 and I'd like to uplift this to be included in ESR 45.7. How should I go about doing that? (I think I've asked this before, sorry!)

Comment 14

[Jorg K (CEST = GMT+2)]

This is already in TB 52, so maybe uplift this to beta 51 and request TB 45 uplift as well.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8802588 [details] [diff] [review]
Patch v2.2

OK, I can take this to TB 51 beta (if we build another one).

Patrick, please fill in the details below for Kent to approve:

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):

Comment 16

[Patrick Cloke [:clokep]]

(In reply to Jorg K (GMT+1) from comment #15)
> Comment on attachment 8802588 [details] [diff] [review]
> Patch v2.2
> 
> OK, I can take this to TB 51 beta (if we build another one).

Thanks!

> Patrick, please fill in the details below for Kent to approve:

Done!

[Approval Request Comment]
Regression caused by (bug #): None, this bug has always existed in the IRC code.
User impact if declined: Users are unable to connect to IRC networks under certain situations.
Testing completed (on c-c, etc.): This has been tested on c-c and c-a, has unit tests for the changes and has been confirmed to work by the original bug reporter.
Risk to taking this patch (and alternatives if risky): Little risk, this patch simplifies the code to accept more parameters from the server.

Comment 17

[Jorg K (CEST = GMT+2)]

Beta (TB 51):
https://hg.mozilla.org/releases/comm-beta/rev/2fd0685a35c66569cfbbd180fbf278ea924c388a

I hope Kent will uplift to the next TB 45.x.

Comment 18

[Kent James (:rkent)]

Comment on attachment 8802588 [details] [diff] [review]
Patch v2.2

https://hg.mozilla.org/releases/comm-esr45/rev/5120ef957c0a