Error connecting to an IRC v3 server with an invalid capability

RESOLVED FIXED in Instantbird 52

Status

Chat Core
IRC
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Vincent Le Goff, Assigned: clokep)

Tracking

trunk
Instantbird 52

Thunderbird Tracking Flags

(thunderbird_esr4551+ fixed, thunderbird51 fixed, thunderbird52 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
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.
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
(Assignee)

Comment 2

a year ago
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.
Assignee: nobody → clokep
Status: UNCONFIRMED → ASSIGNED
Component: Other → IRC
Ever confirmed: true
Product: Instantbird → Chat Core
(Assignee)

Comment 3

a year ago
The result of discussing this with IRCv3 people: https://github.com/ircv3/ircv3-specifications/pull/274
(Assignee)

Updated

a year ago
Summary: Error connecting to an IRC v3 server → Error connecting to an IRC v3 server with an invalid capability
(Assignee)

Comment 4

a year ago
Created attachment 8802487 [details] [diff] [review]
Patch v1

This should fix the parsing issue and adds some tests. I'm going to add IRCv3.2 support in a separate patch.
Attachment #8802487 - Flags: review?(martin)
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.
Attachment #8802487 - Flags: review?(martin) → review-
(Assignee)

Comment 6

a year ago
Created attachment 8802515 [details] [diff] [review]
Patch v2

Thanks for the review! I think I've taken all your comments into account.
Attachment #8802487 - Attachment is obsolete: true
Attachment #8802515 - Flags: review?(martin)
(Assignee)

Comment 7

a year ago
Comment on attachment 8802515 [details] [diff] [review]
Patch v2

This has a minor bug in it.
Attachment #8802515 - Flags: review?(martin) → review-
(Assignee)

Comment 8

a year ago
Created attachment 8802517 [details] [diff] [review]
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).
Attachment #8802515 - Attachment is obsolete: true
Attachment #8802517 - Flags: review?(martin)
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.
Attachment #8802517 - Flags: review?(martin) → review+
(Assignee)

Comment 10

a year ago
Created attachment 8802588 [details] [diff] [review]
Patch v2.2

Adds an additional test condition to avoid the failure in Patch v2.
Attachment #8802517 - Attachment is obsolete: true
Attachment #8802588 - Flags: review?(martin)
Attachment #8802588 - Flags: review?(martin) → review+
(Assignee)

Comment 11

a year ago
Thanks!

https://hg.mozilla.org/comm-central/rev/dda2ecc3395d57d165034a2687074da0455bf121
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 52
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1331188
(Assignee)

Comment 13

11 months ago
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!)
Flags: needinfo?(jorgk)

Comment 14

11 months ago
This is already in TB 52, so maybe uplift this to beta 51 and request TB 45 uplift as well.
status-thunderbird51: --- → affected
status-thunderbird52: --- → fixed
status-thunderbird_esr45: --- → affected
tracking-thunderbird_esr45: --- → ?
Flags: needinfo?(jorgk)

Comment 15

11 months ago
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):
Attachment #8802588 - Flags: approval-comm-esr45?
Attachment #8802588 - Flags: approval-comm-beta+
(Assignee)

Comment 16

11 months ago
(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

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

I hope Kent will uplift to the next TB 45.x.
status-thunderbird51: affected → fixed

Comment 18

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

https://hg.mozilla.org/releases/comm-esr45/rev/5120ef957c0a
Attachment #8802588 - Flags: approval-comm-esr45? → approval-comm-esr45+

Updated

11 months ago
status-thunderbird_esr45: affected → fixed
tracking-thunderbird_esr45: ? → 51+
You need to log in before you can comment on or make changes to this bug.