Error connecting to an IRC v3 server with an invalid capability

RESOLVED FIXED in Instantbird 52

Status

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: vincent.legoff.srs, 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

3 years 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

3 years 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

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

Updated

3 years ago
Summary: Error connecting to an IRC v3 server → Error connecting to an IRC v3 server with an invalid capability
Assignee

Comment 4

3 years ago
Posted patch Patch v1 (obsolete) — Splinter Review
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

3 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
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

3 years 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

3 years ago
Posted patch Patch v2.1 (obsolete) — Splinter Review
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

3 years ago
Posted patch Patch v2.2Splinter Review
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

3 years ago
Thanks!

https://hg.mozilla.org/comm-central/rev/dda2ecc3395d57d165034a2687074da0455bf121
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 52
Assignee

Updated

3 years ago
Duplicate of this bug: 1331188
Assignee

Comment 13

3 years 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

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

Comment 15

3 years 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

3 years 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 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+
You need to log in before you can comment on or make changes to this bug.