1182735 - irc.oftc.net sends NOTICE before connection

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Description

[arlolra]

Attachment: oftc.patch

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.132 Safari/537.36

Steps to reproduce:

Doesn't respect account._showServerTab

Comment 1

[arlolra]

Comment on attachment 8632395 [details] [diff] [review]
oftc.patch

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

::: chat/protocols/irc/ircNonStandard.jsm
@@ +45,5 @@
>        // directions to users.
> +      if (!this.connected && !isAuth &&
> +        // But irc.oftc.net's isn't that helpful.
> +        !(!this.imAccount._showServerTab &&
> +          aMessage.params[1].startsWith("*** Connected securely via"))

Maybe the starts with is unnecessary or a check like (aMessage.params[1].startsWith("***")) is better?

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 8632395 [details] [diff] [review]
oftc.patch

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

I need to see the *exact* wire message sent before reviewing this.

I'd love to see tests for this code, by the way. It very much scares me all the hacks around it.

::: chat/protocols/irc/ircNonStandard.jsm
@@ +44,5 @@
>        // Some servers , e.g. irc.umich.edu, use NOTICE before connection to give
>        // directions to users.
> +      if (!this.connected && !isAuth &&
> +        // But irc.oftc.net's isn't that helpful.
> +        !(!this.imAccount._showServerTab &&

Does this._showServerTab not work? I'm skeptical of checking this flag here.

Comment 3

[arlolra]

The exact wire message is,

{"rawMessage":":beauty.oftc.net NOTICE myusername :*** Connected securely via UNKNOWN AES128-SHA-128","command":"NOTICE","params":["myusername","*** Connected securely via UNKNOWN AES128-SHA-128"],"origin":"beauty.oftc.net","tags":{},"source":""}

and this arrives before 001.

Comment 4

[Patrick Cloke [:clokep]]

Attachment: Patch v2

Instead of adding more hacks onto that NOTICE handler, I rewrote it to be clearer about what we expect. This does slightly change behavior (see the changes to the tests), but overall I think is more consistent.

Comment 5

[Patrick Cloke [:clokep]]

Attachment: Patch v2 for real

Oops, I attached the wrong patch last time!

Comment 6

[aleth [:aleth]]

Comment on attachment 8637308 [details] [diff] [review]
Patch v2 for real

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

Looks like a good improvement! r+ with comments fixed/added.

::: chat/protocols/irc/ircNonStandard.jsm
@@ -33,5 @@
> -      // user. Generally AUTH messages start with ***, but this could pretty
> -      // easily be faked.
> -      // Freenode simply sends * for the target. Moznet sends Auth (used to send
> -      // AUTH); in this case, check if the user's nickname is not auth, or the
> -      // the message starts with ***.

Some of this useful comment seems to have gone missing.

::: chat/protocols/irc/test/test_ircNonStandard.js
@@ +108,5 @@
>    do_check_true(account.shouldAuthenticate === false);
>  
>    // Finally, check if the message is wrong.
>    account = new FakeAccount("password");
> +  message.params[1] = "Test";

Why are we setting this param? A comment would be nice.

Comment 7

[Patrick Cloke [:clokep]]

This includes an updated comment that aleth reviewed in person.

https://hg.mozilla.org/comm-central/rev/1072781ba002

Comment 8

[Patrick Cloke [:clokep]]

Updating milestone from 1.6 to 42.